Skip to content

fix(dialer): synchronize lazy TFO connection lifetime - #3436

Open
polaris-arch wants to merge 4 commits into
MetaCubeX:Alphafrom
polaris-arch:fix/tfo-close-publication
Open

polaris-arch wants to merge 4 commits into
MetaCubeX:Alphafrom
polaris-arch:fix/tfo-close-publication

Conversation

@polaris-arch

@polaris-arch polaris-arch commented Nov 9, 2026 •

Copy link
Copy Markdown

Synchronize lazy TFO socket publication and closure, serialize the first dial, and retire a successful socket if closure or cancellation won. Network I/O remains outside the publication mutex; the existing independent dial context is retained.

Concrete bug and normal-path reachability

The old tfoConn.Dial publishes c.Conn after dialFn returns, while Close reads that field independently. A close between successful dial completion and publication can observe no socket and return; the writer can then publish the socket and report success. closed and connection access are also unsynchronized.

This requires the lazy TFO path (tfo: true, with TFO not globally disabled) and concurrent access; it is not a defect on every connection. An existing production route to concurrent close is SOCKS5 DialContext → clientHandshakeContext → SetupContextForConn: caller cancellation closes the connection while the handshake can be doing its first write. The publication/close interleaving is reachable by source inspection. No naturally occurring production instance or incidence rate is established by these tests.

Source: old TFO implementation, SOCKS5 caller, context interrupter.

Executable reproduction and observed result

Run in a disposable checkout with Go 1.26.10 (the recorded validation toolchain):

git fetch https://github.com/polaris-arch/mihomo.git b32780aa1ece313708a95991b57ac13aeb0aa396
git checkout --detach b32780aa1ece313708a95991b57ac13aeb0aa396
go test -count=1 -timeout 2m -v -run '^TestLifecycleTFO' ./component/dialer
CGO_ENABLED=1 go test -race -count=1 -timeout 3m -run '^TestLifecycleTFO' ./component/dialer

To test the same relevant fixture against unchanged Alpha:

git worktree add --detach ../mihomo-3436-alpha 478af2a57c48bdb8e85f6ed93b2c3735e90ceabd
git show b32780aa1ece313708a95991b57ac13aeb0aa396:component/dialer/tfo_lifecycle_test.go > ../mihomo-3436-alpha/component/dialer/tfo_lifecycle_test.go
(cd ../mihomo-3436-alpha && go test -count=1 -timeout 35s -v -run '^TestLifecycleTFOCloseRetiresLateDialSuccess$' ./component/dialer)

The baseline command is expected to fail an assertion, not fail to compile.

Controlled test Unchanged Alpha Fixed source
CloseRetiresLateDialSuccess Write returns success after wrapper closure io.ErrClosedPipe, no published upstream, late socket peer observes EOF

Real-path cancellation control

TestLifecycleTFORealSOCKS5Cancellation keeps the production tfo-go dial function unchanged, sends an actual SOCKS5 greeting to a loopback peer, withholds method selection, then cancels the caller through SetupContextForConn. It asserts the canceled handshake and socket EOF within one second. Ten repetitions pass in final validation; this proves a real consumer/cancellation path, not a natural occurrence rate of the publication race.

The required Linux control can be reproduced directly:

MIHOMO_TEST_TFO_REQUIRED=1 go test -count=10 -timeout 45s -v -run '^TestLifecycleTFORealSOCKS5Cancellation$' ./component/dialer

Only an explicit library unsupported-platform/kernel error permits skipping this optional wire control. Linux validation sets MIHOMO_TEST_TFO_REQUIRED=1, making that condition a failure rather than a skip. Fake-dial ownership tests remain platform-independent.

Fault injection and defensive coverage

  • The reproducer replaces dialFn with a gated fake that deliberately returns success after cancellation. It demonstrates the ownership defect deterministically; it does not demonstrate that the real tfo-go dialer ignores cancellation. The context-interrupter integration fixture uses the same late-success technique and is also fault injection.
  • Concurrent accessor/deadline/close tests are concurrency stress coverage. The two-first-writers test checks the net.Conn concurrency contract; simultaneous initial writers have not been demonstrated in the ordinary tunnel's single initial-write path. Treat that part as defensive contract coverage, not a separately observed production incident.
  • No whole-process resource reduction is inferred from these fixtures.

Focused/race tests, full default/with_gvisor suites, vet and Windows/macOS changed-package compilation passed in standalone validation. Consumer validation covers the context-interrupter boundary. #3439 should follow this fix for safe concurrent TFO closure.

Head b32780aa passes standalone dialer focused tests (ten repetitions) and package race tests (five repetitions) in final frozen-batch validation. Compared with 06f16aaf, the runtime delta reuses the existing published state when a waiting reader observes preparation cancellation; no new field, helper or watcher is added.

Synchronization scope

The two locks have separate duties: dialMu serializes initial writers, while mu orders socket publication against Close and never covers network I/O. The ready/closed atomic snapshot publishes one immutable connection for subsequent access; completion uses the existing channel. The atomic lookup is a deliberate access-cost choice, not a claim that a state mutex cannot be correct. A standard sync.Once plus state mutex alternative was checked against the same lifetime tests; the current implementation is retained without a production-level performance claim. No new dependency, configuration option or resident watcher is introduced.

Accepted connection versus waiting-reader cancellation

A reader that started before the first write can still be waiting when Dial publishes the accepted socket, before it closes dialed. Selecting an already-canceled preparation context must not discard that accepted connection. Read now rechecks existing state: explicit close wins, an accepted socket remains readable, and dialErr is read only after the completion channel synchronizes it. Pending cancellation keeps its previous error.

The net.Conn concurrent-read/write contract makes this ordering legal. A pre-first-write reader has not been established in the ordinary tunnel path; this is not a separately observed production incident. The regression fixture directly arranges publication-before-notification with four waiting readers and synctest.Wait; this is deterministic scheduling injection, not a claim that the real dialer ignores cancellation.

git worktree add --detach ../mihomo-3436-prior 06f16aaf30faa8672982fcb64392844c36e1c1d5
git show b32780aa1ece313708a95991b57ac13aeb0aa396:component/dialer/tfo_read_test.go > ../mihomo-3436-prior/component/dialer/tfo_read_test.go
(cd ../mihomo-3436-prior && go test -count=1 -timeout 1m -v -run '^TestLifecycleTFOWaitingRead/published$' ./component/dialer)
go test -count=10 -timeout 2m -v -run '^TestLifecycleTFOWaitingRead$' ./component/dialer
Regression control Recorded result
Previous proposed runtime 06f16aaf plus the new fixture All four waiting reads fail with unexpected EOF; no compile failure
Candidate b32780aa Published / failed / closed / pending cases pass ten repetitions

The previous-runtime control is distinct from the unchanged-Alpha close/publication reproducer above.

Final frozen-batch validation also passes full root ./... default/with_gvisor, vet and Windows/macOS changed-package compilation in the declared combination: tunnel 98d8db81, TFO b32780aa, group e8ffb994, context completion 23285c8f, and native API 5b6706cc through 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 nested test/ 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.

@polaris-arch
polaris-arch marked this pull request as ready for review November 9, 2026 12:25
@polaris-arch
polaris-arch marked this pull request as draft November 9, 2026 14:10
@polaris-arch
polaris-arch marked this pull request as ready for review November 10, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant