Skip to content

fix(outboundgroup): exclude caller cancellation from TCP failure checks - #3437

Open
polaris-arch wants to merge 2 commits into
MetaCubeX:Alphafrom
polaris-arch:fix/group-tcp-cancellation
Open

polaris-arch wants to merge 2 commits into
MetaCubeX:Alphafrom
polaris-arch:fix/group-tcp-cancellation

Conversation

@polaris-arch

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

Copy link
Copy Markdown

Exclude errors observed after caller cancellation from TCP group-failure accounting in fallback, url-test and load-balance, including deferred first Write/WriteBuffer. Preserve deadline failures, live-context transport failures and failures attributed before a later cancellation.

Concrete bugs and normal-path reachability

  1. A canceled caller can receive a raw socket error. The old dial/first-write callbacks count it toward node failures and can trigger provider health checks. One existing source path is controller getGroupDelay → request-derived context → GroupBase.URLTest → child proxy URLTest → DialContext. With nested groups, a canceled HTTP request can therefore reach an affected child group. The ordinary tunnel's current background-derived dial context does not itself translate frontend closure into caller cancellation; fix(tunnel): cancel preparation on native frontend closure #3438 adds that separate source. No production incidence rate or controller-to-real-node end-to-end reproduction is claimed here.
  2. Success/failure/health-check completion can access failedTimes without a common lock. Parallel connection refused handling can also race the old load-then-store health-check guard. These are source-identified concurrency defects under concurrent connections, independent of caller cancellation. The same new public-path fixture now reproduces a race in unchanged Alpha between onDialSuccess and asynchronous onDialFailed/counter reset, specifically on failedTimes. The candidate passes the same fixture under -race; this is separate evidence from the 9 canceled-attribution cases.

Source: controller request context, proxy URLTest dial, old accounting.

Protect counters/resets consistently and claim health checks with compare-and-swap. Moving health-check work outside the counter mutex is required by that locking change; no pre-existing deadlock is claimed from that movement alone.

Attribution policy: ctx.Err() == context.Canceled is sampled before asynchronous accounting. This is not proof that the transport error was caused by cancellation or that the node is healthy: a genuine error coinciding with cancellation can also be excluded. DeadlineExceeded remains countable; cancellation after attribution does not erase the failure.

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 e8ffb994387ce0a63f54372936c91bf0376b8b10
git checkout --detach e8ffb994387ce0a63f54372936c91bf0376b8b10
go test -count=1 -timeout 2m -v -run '^TestLifecycle' ./adapter/outboundgroup
CGO_ENABLED=1 go test -race -count=1 -timeout 3m -run '^TestLifecycle' ./adapter/outboundgroup

To test the same relevant fixture against unchanged Alpha:

git worktree add --detach ../mihomo-3437-alpha 478af2a57c48bdb8e85f6ed93b2c3735e90ceabd
python3 - <<'PYFIXTURE'
from pathlib import Path
import subprocess
name = 'adapter/outboundgroup/lifecycle_health_test.go'
s = subprocess.check_output(['git', 'show', 'e8ffb994387ce0a63f54372936c91bf0376b8b10:' + name], text=True)
s = s.split('func TestLifecycleGroupSuccessfulFirstWrite')[0].replace('\n\t"time"', '')
Path('../mihomo-3437-alpha', name).write_text(s)
PYFIXTURE
(cd ../mihomo-3437-alpha && go test -count=1 -timeout 35s -v -run '^TestLifecycleGroupTCPFailureAttribution$' ./adapter/outboundgroup)

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

The baseline fixture is intentionally truncated before candidate-only helper tests and drops the then-unused time import, matching the archived validation procedure; the attribution test itself is unchanged.

The copied prefix also includes TestLifecycleGroupConcurrentPublicPaths, which alternates successful first writes and failed dials through public group methods. Run it unchanged on the baseline:

(cd ../mihomo-3437-alpha && CGO_ENABLED=1 go test -race -count=1 -timeout 1m -run '^TestLifecycleGroupConcurrentPublicPaths$' ./adapter/outboundgroup)

Recorded comparison: Alpha reports WARNING: DATA RACE on failedTimes in groupbase.go; head e8ffb994 passes. Providers/leaf outcomes are synthetic, but the counter is accessed via the public concurrent dial/write paths, not directly by the fixture.

Controlled scenario Unchanged Alpha Fixed source
3 group kinds × dial/Write/WriteBuffer, canceled caller followed by raw socket error All 9 canceled cases fail: failure accounting/probe activity is observed No corresponding failure count or fake-provider probe
Live-caller failures, deadlines, late cancellation Boundary controls, not the 9-case benefit count Genuine failures remain countable

Fault injection and defensive coverage

The leaf proxy, socket error, provider and cancellation timing are synthetic fixtures; no actual node is probed. synctest.Wait drains accounting before negative assertions. Nested-group, ordering, exemptions and held-provider concurrency tests are boundary coverage. These counts are test coverage, not production failure frequency or saved real probes.

Focused/race tests, complete default/with_gvisor suites, vet and Windows/macOS package compilation passed in standalone validation; combined validation covers the tunnel consumers. The generic timeout correction #3439 can be reviewed separately; native cancellation integration should retain this attribution policy.

Current head e8ffb994 passes focused/race, full default/with_gvisor, vet and Windows/macOS compilation in standalone validation; final combined validation covers its tunnel consumers. The head delta adds tests only.

@polaris-arch
polaris-arch marked this pull request as ready for review November 9, 2026 12:26
@polaris-arch
polaris-arch marked this pull request as draft November 9, 2026 14:23
@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