Repository navigation
fix(bench): cancel and join owned benchmark transfers - #46
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused ownership changes have targeted regression coverage and no unresolved blocking findings.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes benchmark lifecycle handling so cancellation closes owned connections and joins transfers without closing the shared tunnel dialer. The published v1.0.1 binary is unchanged.
Changes:
- Cancel and join server transfers, including connections awaiting admission.
- Interrupt client I/O and handle failed, nil, or canceled dial results safely.
- Add lifecycle regression tests and document source-build availability.
| File | Description |
|---|---|
| internal/netbench/netbench.go | Adds cancellation-aware connection ownership and cleanup. |
| internal/netbench/listener_ownership_test.go | Tests listener and worker cleanup after accept failures. |
| internal/netbench/dial_ownership_test.go | Tests dial-result ownership and cancellation-close completion. |
| internal/netbench/cancellation_test.go | Tests shutdown during stalled I/O and pending admission. |
| docs/BENCHMARK.md | Documents cancellation behavior and release boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Summary
Validation
make checkandmake racepass.0x102with the global-limit reason. The separate auth control retains the 500 ms budget / 2 s upper bound and checks no premature expiry plus remote0x103.0x102; auth control passes and independent cleanup joins complete. This proves the fixture does not let per-source rejection mask a broken global limit.Initial CI failure retained
Initial head
9e5da60d82b5bb416c606245b1a16b1c36a8c180, CI run 37070510464 attempt 1: 10 jobs passed, Go 1.25.13 job 111048589015 failed in the existing QUIC connection-limit test; benchmark package passed. The complete failure/shuffle log is retained. Client TLS completion was not a server-admission witness in that fixture. Follow-up commit adds that witness and separates the timer control without changing production or weakening the rejection policy. The exact cause of the original CI observation remains unproven. No blind workflow rerun was requested, and the initial failed head was not merged.Boundaries
No protocol/default/dependency/credential/authentication-rule changes, release/tag, or remote deployment. Not performance or traffic-classification evidence. External listener closure while Serve is already blocked on pending admission is not claimed as promptly detected; parent-context cancellation is the tested shutdown path. No new Windows benchmark-runtime claim.
Base:
0a23ee20d28f642557e5dc7841cb842cd293a6d9. Final candidate:a3f06ab0f38fe49c42c7293a2a91f7c8ba0a01c9; tree:53a389dbd1053fbf293b08f1000b5678f6cb692c. Six changed files, two commits.Merge and main verification
Squash-merged as
d78cbc599468c19ae848f44b3b2ea76f0efd76bdafter the final-head CI (37071474072, all 11 jobs), CodeQL (37071474085), and netem (37071474120) succeeded. The official merge tree equals the frozen candidate tree above; its sole parent is the exact base. Local main is clean and matches origin/main; all frozen source checksums are unchanged.Independent exact-merge main push / attempt 1 checks also completed successfully: CI 37071902553, all 11 jobs; CodeQL 37071903385; Linux netem 37071902493. This does not replace the retained original failure evidence. No release/tag or remote deployment was performed.