Draft
Conversation
check if peers slice is non-empty before accessing first element add retry mechanism for connection verification with 1s timeout improve error message when no peers are found
add process_tracker.go with daemon PID tracking register/unregister daemons in StartDaemon/StopDaemon track all spawned processes for later cleanup
add defer/recover in NewT to ensure cleanup on panic force kill daemons even if normal cleanup fails register double cleanup to handle edge cases make Cleanup() more resilient to failures
fix compilation error by using local helper instead of os.IsProcessDone
run gofmt on harness package add missing newline at end of file
tested process tracker with race detector - no issues found confirmed no zombie daemons left after test panics cleanup mechanism works correctly even with concurrent operations
use ConnectAndWait for reliable connections with 10s timeout collect and report all connection failures with details verify expected peer count and protocols use errgroup for better concurrent error handling simplify by removing unused env variable and method
disable MDNS to prevent test interference when running in parallel add retry logic for daemon startup to handle transient failures replace fixed sleep with proper readiness check remove unnecessary parallel execution of subtests
follow Go idioms by keeping internal methods unexported
- automatically register any process started with RunFunc that doesn't wait - add RegisterBackgroundProcess for tests using raw exec.Command - ensure all background processes are killed on test failure/panic
use serial Connect() implementation from PR #10933 to avoid TLS handshake issues
- fix process count logging bug - use errors.Is() for error checking - extract daemon retry logic to helper methods - consolidate cleanup paths
lidel
changed the title
Contributor
I want to be more careful about not masking any startup failures, even if transient. When we identify a specific failure condition, like a TCP port is still in use but will not be at the next retry, then we can retry on detecting that specific case. We should not retry on any unknown failure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
Do not merge, using this PR as a sandbox for now, to see what fails on CI but rarely locally.
This PR is exploration into making test/cli/harness more robust when things go bad with spawned processes and we run on self-hosted runners.
Impact: