Repository navigation
fix(tunnel): cancel preparation on native frontend closure - #3438
Draft
polaris-arch wants to merge 13 commits into
Draft
polaris-arch wants to merge 13 commits into
polaris-arch wants to merge 13 commits into
Conversation
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.
Cancel outbound preparation when a capable native TCP frontend fully terminates, and detach the notification before successful relay handoff. Peer FIN alone retains the response direction.
Concrete problem and normal-path reachability
A native frontend can terminate while its outbound is still preparing. With only #3439, preparation can continue until its remaining dial budget expires. This draft adds earlier cancellation; it is not another claim of removing an unbounded wait after that generic fix.
The intended path is Mips forwarder → accepted mipstack TCP connection → sing listener → buffered tunnel connection →
CancelOnClose. A valid peer RST or full applicationClosecan terminate preparation. The new tests use real native forwarded endpoints and the tunnel's actual buffered connection. They replace route/proxy selection, do not instantiate the configured sing-tun listener, and do not claim a physical-TUN or external-node measurement. Source inspection confirms the ordinary accepted native connection reaches this wrapper path.Prerequisites and draft status
This stacked draft includes #3439.
git diff 4f6bd36b3231ab996fb232f3d46fdc06194ae613 74eb0b0aa696c06659af6868b44cbdac773b4ad2 -- tunnel/isolates the native hook, ownership handoff and native-specific tests between the current trees; the shared deferred-handshake test file is identical.Executable native protocol reproduction
In a disposable Mihomo checkout, with Go 1.26.10:
The required flag makes an unavailable API a failure, not a skip. The override changes a test-only modfile, not the production declaration.
Test the same native reset fixture with only the generic budget, then restore the candidate:
The negative command must fail its one-second assertion after the Upgrade request was read and reset was sent, not fail to build. Cleanup then closes the peer; total failing-test duration is not a measurement of five-second retention.
Both native cases pass five repetitions in validation. The earlier four-way run also records the same-source negative and protocol controls. These establish cancellation/half-close correctness under controlled events, not production frequency or whole-process resource savings.
Fault injection and defensive coverage
Existing fake-source tests explicitly force claim-before-callback scheduling and late successful dial returns. They remain labelled adversarial ownership/fault-injection coverage. The portable synthetic-source WebSocket test exercises cancellation during the actual deferred first Write and now requires completion within one second; its cleanup allowance stays seven seconds. It is retained because real native tests can skip when the proposed API is unavailable. Successful relay lifetime and registration detachment remain positive controls.
Head
98d8db81changes tests only relative toaeb69e57. The final frozen-batch validation executes the required real native reset/FIN controls five times, focused/race suites, the already-terminal registration test, and the explicitwith_gvisorcapability-negative test. The production dependency is unchanged. Keep draft until API acceptance, an actual dependency update and safe integration prerequisites are resolved.Additional boundary controls
TestLifecycleAlreadyTerminatedBeforeRegistrationenters the actual handler with a mock that cancels immediately during registration; no outbound dial is attempted. This is an injected registration-state boundary, not a real already-terminal socket test.TestLifecycleGVisorHasNoNativeCloseCapabilitychecks the real pinnedgonet.TCPConntype through the ordinary connection wrapper. Its zero value is used for capability traversal only; no live gVisor forwarding is claimed.Final frozen-batch validation also passes full root
./...default/with_gvisor, vet and Windows/macOS changed-package compilation in the declared combination: tunnel98d8db81, TFOb32780aa, groupe8ffb994, context completion23285c8f, and native API5b6706ccthrough a test-only modfile. These full-suite results bind that combination, not a new standalone full-suite run for every PR.SKIP_INTEROP_TEST=1; the nestedtest/module is outside root./.... Older validation links retain their original source identities. No physical-TUN, kernel Fast Open SYN/cookie, production incidence or whole-process resource claim is made.Current test-only revision
Head
74eb0b0achanges only one existing test assertion relative to98d8db81: primary completion wait 7s → 1s. Runtime source, test count and cleanup are unchanged. The inherited #3439 test file remains identical; it is dependency coverage, not a second independently added suite.Targeted validation of this exact head used Go 1.26.10 in private CI: the default tunnel package, race/
with_gvisor, required native RST/FIN controls (three repetitions each), and vet passed. Public GitHub Actions is disabled for the fork, so no new public CI pass is claimed. The earlier public full-suite result above remains bound to98d8db81and its stated combination.The additional generic-budget-only control uses
4f6bd36b:tunnel/tunnel.go. That implementation never registers the mock hook, so its control-only fixture guards a nilsource.claim()callback to reach the one-second assertion. The control fails that assertion; no panic/build failure is counted as reproduction. This fixture guard is not part of the PR.The portable positive check is reproducible without the proposed native API: