fix: cancel abandoned SOCKS UDP setup - #31
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A moderate deadline-aware context-check issue remains at the setup guards.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes abandoned SOCKS5 UDP setup cancellation and resource cleanup when the control connection closes.
Changes:
- Cancels setup across lookup, dialing, and forwarding.
- Safely joins readers/workers and owns packet connections.
- Adds regression tests and documents lifecycle behavior.
| File | Reviewed changes |
|---|---|
internal/proxy/socks5.go |
Setup cancellation and worker lifecycle handling. |
internal/proxy/socks5_udp_test.go |
Validation and admission-release coverage. |
internal/proxy/socks5_udp_setup_native_test.go |
Native QUIC cancellation regression coverage. |
internal/proxy/socks5_udp_setup_cancel_test.go |
Cancellation races, ownership, timers, and shutdown. |
internal/proxy/socks5_udp_control_test.go |
Control-reader lifecycle and reply-stall behavior. |
docs/DEPLOYMENT.md |
SOCKS UDP cancellation semantics and boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+208
to
211
| if err := ctx.Err(); err != nil { | ||
| _ = reply(socksReplyForError(err), nil) | ||
| return | ||
| } |
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
Verification
make check, fullmake race, targeted race x5 and independent review passed. Single-change negative controls fail the intended cancellation, timer-cleanup and packet-ownership assertions.d99f9b160493320969cecb1e5d2febec484d4e6bpassed CI (all 11 jobs), CodeQL, and Linux namespace/netem. Actual Docker integration, OCI export and netem steps and success log markers were checked. No open review threads at merge verification.Boundaries
Only this caller's setup lifetime is changed. Native shared physical dialing remains client-owned; H3 keeps its existing distinct cold-dial retry policy. TCP CONNECT handling and graceful/forced shutdown contracts are unchanged. Custom resolvers/dialers that ignore context cannot be forcibly interrupted. The DNS fixture checks watcher-to-validation context propagation, not a full platform resolver integration experiment.
No dependency, wire format, release or tag change. This source-build fix is not in v1.0.1. These are correctness/resource-lifecycle checks, not throughput or browser-similarity experiments.
Merge receipt
Merged as
dc0a714ae319394359f647e4b216fb5e89c711c8. Its tree1981c16150d5a2c03b293100225c8df38bc93908is identical to the tested PR head. Exact-main CI (all 11 latest job results), CodeQL, and Linux namespace/netem passed. Main Docker integration/OCI export and netem success logs were verified.CI transparency: main CI attempt 1 failed only the unchanged
TestQUICPacingRecoversAfterReceiverBackpressureon macOS at its fixture's aggregate Write-vs-Wait time assertion; the proxy package passed. The failure log is retained. There is no tunnel/accel source diff in this PR. The unchanged pacing test subsequently passed 10 local macOS repetitions, and a failed-jobs-only CI rerun passed (attempt 2). This does not establish that the timing-based fixture assertion is reliable; replacing it with causal backpressure evidence remains separate follow-up work. No threshold was relaxed and no test was skipped here.Local main is synchronized and clean. No release/tag was created.