Repository navigation
deflake: fetch-proxy-tls-intern-race timeout on Linux release builds - #29212
Conversation
…ounded shutdown) Root cause: the CONNECT proxy only handled client 'error', not 'close'. When a probe aborts, the client→proxy socket usually closes with FIN, so the proxy→backend upstream socket was never destroyed. On release builds the setImmediate probe loop fires thousands/sec; orphaned TLS connections pile up on the backend until the driver's fetch stalls. With no abort signal on the driver fetch, `await driverDone` then hung past the 30s test timeout. Fix: - Proxy: destroy upstream on client 'close' (and vice versa). - Fixture: start the hard cap as a setTimeout up front so it is authoritative; it flips `stop` and aborts the driver fetch. - Fixture: let the driver land one request before the probe flood so the driverOk>0 sanity check holds; the warmup wait is bounded by the same hard-cap timer. Supersedes the fixture portion of #28873 and addresses its review feedback about the unbounded warmup loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Updated 7:05 AM PT - Apr 12th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 29212That installs a local version of the PR into your bun-29212 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded an AbortController-driven hard cap and warmup before probe flood in the fixture; adjusted driver to pass the abort signal. Updated test proxy to track and destroy upstream sockets on client errors/close and destroy client when upstream closes to prevent socket leaks. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
driverDone can only resolve after the timer fires (stop is only flipped by the timer), so clearTimeout was dead code. unref() is correct hygiene for a watchdog even though process.exit(0) makes it moot here. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — test-only deflake with clear root cause and correct bidirectional socket teardown.
Extended reasoning...
Overview
This PR modifies two test files only: the CONNECT proxy fixture and its test harness. No production code is touched.
Changes
- Proxy socket leak fix: adds → and → for proper bidirectional teardown. Previously only events were handled, so FIN-closed sockets from aborted probes leaked upstream connections until the backend's FD table saturated.
- Hard cap as immediate timer: replaces with a started before the async loops, ensuring the cap fires even when the event loop is blocked on stalled I/O.
- AbortController on driver fetch: prevents from hanging when the hard cap fires.
- Warmup loop: lets the driver complete one successful request before the probe flood, making the assertion reliable on loaded CI without introducing an unbounded wait (bounded by the hard cap).
Security risks
None — test-only changes with no user-facing or production code modifications.
Level of scrutiny
Low. These are test infrastructure fixes following standard Node.js networking patterns (bidirectional / handling on piped sockets). The logic is straightforward and the root cause is well-documented.
Other factors
The fix has been validated with 20× sequential and 18× parallel runs on release builds plus debug+ASAN, all passing. No CODEOWNER files cover test helpers. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — test-only fix with correct socket lifecycle management and bounded hard-cap timer.
Extended reasoning...
Overview
This PR modifies two test files only: fetch-proxy-tls-intern-race-fixture.ts and fetch-proxy-tls-intern-race.test.ts. No production code is touched. The changes fix a 41/200 flake rate on Linux release CI targets.
Security risks
None — both files are test infrastructure. No auth, crypto, or permission-sensitive code is involved.
Level of scrutiny
Low. The root cause is clearly identified (missing close handler on the CONNECT proxy causing upstream socket leaks that saturate the backend's FD table), the fix is mechanically sound, and each individual change is straightforward:
- Promoting
upstreamto outer scope to allow reference in early-registered handlers - Adding
client.on('close', () => upstream?.destroy())with correct optional chaining (upstream may not be assigned yet) - Adding
upstream.on('close', () => client.destroy())for symmetric teardown - Replacing
await Bun.sleep(HARD_CAP_MS)+ post-sleepstop=truewith an upfrontsetTimeout+AbortControllerso the cap is authoritative even if the await-loop is blocked - Adding a warmup phase bounded by the same hard-cap timer
Other factors
No bugs were found by the automated system. CodeRabbitAI found no actionable issues. The author ran 20× sequential, 18× parallel, and 4× debug+ASAN test runs — all passing. The build failures in the robobun comment are against the initial commit (9fdbf165) and are unrelated Zig/libuv infrastructure issues; the latest commit (c427de9) fixes the unref and drops the dead clearTimeout.
…ven-sh#29212) ## Summary Fixes the 41/200 timeout flake of `test/js/web/fetch/fetch-proxy-tls-intern-race.test.ts` on Linux release targets (🐧 25.04 x64-baseline, 🐧 3.23 aarch64, 🐧 13 x64-baseline). **Root cause:** the test's CONNECT proxy only handled `client.on("error")`, not `"close"`. The probe loop aborts each fetch immediately, so the client→proxy socket usually closes with FIN (not RST). The proxy→backend `upstream` socket was never destroyed in that path. On release builds the `setImmediate` probe loop fires thousands of times per second, so orphaned TLS connections accumulate on the backend `Bun.serve` until its accept queue / FD table saturates and the driver's fetch stalls. The driver fetch had no abort signal, so `await driverDone` hung past the 30s test timeout. **Fix:** - Proxy: destroy `upstream` on client `"close"` (and vice versa) so aborted probes don't leak backend connections. - Fixture: start the hard cap as a `setTimeout` up front so it's authoritative regardless of what the loops are blocked on; it flips `stop` and aborts the in-flight driver fetch. - Fixture: let the driver complete one request against an idle proxy before the probe flood so the `driverOk > 0` sanity check holds on loaded CI. The warmup wait is bounded by the same hard-cap timer (addresses the unbounded-loop review feedback on oven-sh#28873). Supersedes the `fetch-proxy-tls-intern-race-fixture.ts` portion of oven-sh#28873. ## Test plan - [x] 20× sequential runs with release build (bun-pr 29163): all pass at ~5.4s - [x] 18× parallel runs (6 concurrent × 3 rounds) under load: all pass - [x] 4× `bun bd test` (debug+ASAN): all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: root <root@ip-10-0-2-234.us-west-2.compute.internal> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ven-sh#29212) ## Summary Fixes the 41/200 timeout flake of `test/js/web/fetch/fetch-proxy-tls-intern-race.test.ts` on Linux release targets (🐧 25.04 x64-baseline, 🐧 3.23 aarch64, 🐧 13 x64-baseline). **Root cause:** the test's CONNECT proxy only handled `client.on("error")`, not `"close"`. The probe loop aborts each fetch immediately, so the client→proxy socket usually closes with FIN (not RST). The proxy→backend `upstream` socket was never destroyed in that path. On release builds the `setImmediate` probe loop fires thousands of times per second, so orphaned TLS connections accumulate on the backend `Bun.serve` until its accept queue / FD table saturates and the driver's fetch stalls. The driver fetch had no abort signal, so `await driverDone` hung past the 30s test timeout. **Fix:** - Proxy: destroy `upstream` on client `"close"` (and vice versa) so aborted probes don't leak backend connections. - Fixture: start the hard cap as a `setTimeout` up front so it's authoritative regardless of what the loops are blocked on; it flips `stop` and aborts the in-flight driver fetch. - Fixture: let the driver complete one request against an idle proxy before the probe flood so the `driverOk > 0` sanity check holds on loaded CI. The warmup wait is bounded by the same hard-cap timer (addresses the unbounded-loop review feedback on oven-sh#28873). Supersedes the `fetch-proxy-tls-intern-race-fixture.ts` portion of oven-sh#28873. ## Test plan - [x] 20× sequential runs with release build (bun-pr 29163): all pass at ~5.4s - [x] 18× parallel runs (6 concurrent × 3 rounds) under load: all pass - [x] 4× `bun bd test` (debug+ASAN): all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: root <root@ip-10-0-2-234.us-west-2.compute.internal> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes the 41/200 timeout flake of
test/js/web/fetch/fetch-proxy-tls-intern-race.test.tson Linux release targets (🐧 25.04 x64-baseline, 🐧 3.23 aarch64, 🐧 13 x64-baseline).Root cause: the test's CONNECT proxy only handled
client.on("error"), not"close". The probe loop aborts each fetch immediately, so the client→proxy socket usually closes with FIN (not RST). The proxy→backendupstreamsocket was never destroyed in that path. On release builds thesetImmediateprobe loop fires thousands of times per second, so orphaned TLS connections accumulate on the backendBun.serveuntil its accept queue / FD table saturates and the driver's fetch stalls. The driver fetch had no abort signal, soawait driverDonehung past the 30s test timeout.Fix:
upstreamon client"close"(and vice versa) so aborted probes don't leak backend connections.setTimeoutup front so it's authoritative regardless of what the loops are blocked on; it flipsstopand aborts the in-flight driver fetch.driverOk > 0sanity check holds on loaded CI. The warmup wait is bounded by the same hard-cap timer (addresses the unbounded-loop review feedback on deflake: fix 7 flaky tests with proper root-cause fixes #28873).Supersedes the
fetch-proxy-tls-intern-race-fixture.tsportion of #28873.Test plan
bun bd test(debug+ASAN): all pass🤖 Generated with Claude Code