Repository navigation
deflake: fix 7 flaky tests with proper root-cause fixes - #28873
Conversation
|
Updated 7:05 AM PT - Apr 12th, 2026
❌ @Jarred-Sumner, your commit 781cb52 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 28873That installs a local version of the PR into your bun-28873 --bun |
WalkthroughTest files updated with warm-up loops for memory testing, explicit profiling parameters, terminal data synchronization waits, and deferred HTTP response emission until client data receipt. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — all changes are test-only, each fix addresses the documented root cause, and the one flagged nit doesn't affect correctness.
Extended reasoning...
Overview
This PR modifies 7 test files to fix documented flaky tests. No production/runtime code is touched. The fixes span: file-gate synchronization (test-reporter), warm-up before RSS baseline (bun-serve-file), reduced profiler sample interval (bun-jsc), promise-gated PTY data (terminal), socket data-event guard for all 23 chunked-trailing servers, abort controller for driver shutdown (fetch-proxy-tls-intern-race-fixture), and a labeled break + reader cancel on WebSocket close frame (fetch.upgrade).
Security risks
None. All changes are confined to test files and exercise no auth, crypto, or permission-sensitive paths.
Level of scrutiny
Low — test-only changes with clear, documented root causes and verified local pass rates. The patterns used (Promise.withResolvers, AbortController, labeled break, socket.once) are all idiomatic and correct.
Other factors
The one flagged nit (missing proc.exited race check in the test-reporter polling loop) is genuine but minor: the afterEach kill and the bun:test global timeout still bound the hang, and the happy path (which is what the fix targets) is correct. The nit affects failure-mode diagnostics only and does not invalidate the PR's approach.
| // Wait for test collection and first test to start running | ||
| // The first test has a 500ms sleep, so waiting 200ms ensures we're in execution phase | ||
| await Bun.sleep(200); | ||
| // Wait for the child to enter test A1. This proves discovery has completed |
There was a problem hiding this comment.
🟡 The polling loop while (!existsSync(startedFile)) await Bun.sleep(10) at line 235 has no check on proc.exited, so if the child crashes before writing startedFile the parent spins until the bun:test global timeout fires. This produces a confusing generic timeout error rather than the real crash reason; a proc.exited race check inside the loop would surface the failure immediately.
Extended reasoning...
What the bug is and how it manifests
The test at line 235 of test/cli/inspect/test-reporter.test.ts enters a tight polling loop after spawning the inspector child process:
while (\!existsSync(startedFile)) await Bun.sleep(10);
There is no exit condition tied to the liveness of the child process. If the child exits for any reason before it executes writeFileSync(process.env.STARTED_FILE!, '1'), this loop never terminates on its own.
The specific code path that triggers it
After session.initialize() sends Inspector.initialized (line 232), the child is free to begin test collection and run test A1. If anything goes wrong before A1 writes the started-file — for example, the inspector handshake fails silently, bunExe() is missing or crashes at startup, the STARTED_FILE environment variable is somehow absent, or the child hits an unhandled runtime error — proc exits but startedFile is never created. The parent loop then spins at 10 ms intervals forever.
Why existing code does not prevent it
The afterEach hook (which calls proc?.kill()) only runs after the test body returns or throws. Because the test body is blocked in the infinite polling loop, afterEach never gets a chance to clean up the child. Nothing inside the loop inspects proc.exited or proc.exitCode.
What the impact would be
The test hangs for the full bun:test per-test timeout (15+ seconds by default), then fails with a generic timeout message. The actual crash reason is swallowed. This is a regression in failure-mode diagnostics compared to the previous sleep(200) approach, which always proceeded to waitForFoundTests(5, 15000) and emitted a descriptive 'Timeout waiting for N found tests' message that included the observed found-test state.
How to fix it
Race proc.exited against each Bun.sleep(10) iteration and throw if the process exits early:
while (\!existsSync(startedFile)) {
const result = await Promise.race([proc.exited, Bun.sleep(10).then(() => undefined)]);
if (result \!== undefined) throw new Error('child exited early with code ' + result);
}
Step-by-step proof of the hang
- Parent spawns child with --inspect-wait, connects, calls session.initialize().
- Suppose the inspector handshake fails and the child exits (code 1) before test A1 runs and writes startedFile.
- proc.exited is now resolved; proc.exitCode is 1.
- Parent enters the polling loop; existsSync(startedFile) returns false.
- Loop sleeps 10 ms, wakes, checks again — still false. Repeats.
- No timeout guards the loop internally; the only escape is the bun:test global timeout.
- Eventually the test runner kills the test with a generic timeout error — no indication of the actual child failure.
| await proc.exited; | ||
| expect(proc.exitCode).toBe(0); | ||
|
|
||
| // Data should have been received | ||
| // PTY data is delivered asynchronously and may arrive after the child exits; | ||
| // wait for the data callback rather than assuming it already fired. | ||
| await gotData; | ||
| const output = Buffer.concat(dataChunks).toString(); |
There was a problem hiding this comment.
🟡 The await gotData at line 1174 has no explicit timeout, so if the PTY never delivers a chunk containing 'hello' the test hangs until bun:test's per-test timeout fires (default ~5 s) and reports a generic 'test timed out' rather than the clear 'expected output to contain hello' assertion the old code would have given immediately. Adding Promise.race([gotData, Bun.sleep(5000).then(() => { throw new Error('PTY never delivered hello'); })]) would preserve the correct fix while restoring clear failure diagnostics.
Extended reasoning...
What the bug is and how it manifests
The PR correctly identifies that PTY data may arrive asynchronously after proc.exited resolves, and introduces a gotData promise that resolves only when the data callback fires with a buffer containing 'hello'. However, there is no timeout guard on await gotData. In the failure case — where the PTY never delivers 'hello' — the test hangs silently until bun:test's per-test global timeout fires, producing a generic 'test timed out' error rather than a useful assertion failure.
The specific code path that triggers it
At terminal.test.ts line ~1174 the test does: await proc.exited; expect(proc.exitCode).toBe(0); await gotData; [no timeout] const output = Buffer.concat(dataChunks).toString(); expect(output).toContain('hello'). gotData resolves only when Buffer.concat(dataChunks).includes('hello') becomes true inside the data callback. If the PTY data callback never fires — e.g., on Alpine/musl where PTY output delivery is known to be unreliable (the exact platform this test was flaky on) — this await never resolves on its own.
Why existing code does not prevent it
The test() call has no explicit per-test timeout argument, so it relies on bun:test's built-in default. The refutation correctly notes that bun:test does have a default 5000 ms per-test timeout (confirmed at src/cli.zig:342), so the test will not hang indefinitely — it will eventually fail with a generic 'test timed out' message after 5 seconds. However, the old code checked output immediately after proc.exited and would have failed instantly with 'expected empty string to contain hello', which is far more actionable. The new code converts an instant diagnostic failure into a delayed, less informative timeout.
What the impact would be
The impact is a diagnostic regression in the failure case: a developer investigating CI failures on Alpine would see a generic timeout rather than the specific missing-output message. This makes root-cause analysis harder on exactly the platform targeted by this fix.
How to fix it
Race gotData against a short timeout: await Promise.race([gotData, Bun.sleep(5000).then(() => { throw new Error('PTY never delivered hello'); })]). This preserves the correct behavior (waiting for async PTY data) while restoring a clear failure message if data never arrives.
Step-by-step proof of the diagnostic regression
- On Alpine/musl, echo hello exits but the PTY slave side closes without flushing the output chunk — the data callback never fires.
- proc.exited resolves (exit code 0); expect(proc.exitCode).toBe(0) passes.
- await gotData suspends until bun:test's 5 s timeout fires.
- bun:test kills the test and reports 'test timed out'.
- With the old code, Buffer.concat(dataChunks).toString() would be empty and expect(output).toContain('hello') would immediately throw 'expected empty string to contain hello' — far more actionable for debugging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/fetch/fetch-proxy-tls-intern-race-fixture.ts`:
- Around line 67-68: The warmup gate can deadlock because the hard-cap that sets
stop is started only after the await loop; start the hard-cap timer concurrently
before entering the wait (e.g., kick off a Bun.setTimeout/Bun.sleep promise or
an async timer that sets stop after the cap) so the while (driverOk === 0 &&
!stop) await Bun.sleep(1); loop is always bounded; ensure the timer/timeout
reference is started earlier (before the loop) and still allows probe() to run
when driverOk becomes >0 or stop flips to true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa21881d-0473-4eed-a76d-acf2af20426a
📒 Files selected for processing (1)
test/js/web/fetch/fetch-proxy-tls-intern-race-fixture.ts
…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>
…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 #28873). Supersedes the `fetch-proxy-tls-intern-race-fixture.ts` portion of #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>
18dfa21 to
94e054a
Compare
- bun-serve-file.test.ts: warm up before taking RSS baseline so one-time allocations (connection pool, retained buffers) are not counted as leak - bun-jsc.test.ts, terminal.test.ts, chunked-trailing.test.js: see diff
94e054a to
781cb52
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/fetch/chunked-trailing.test.js (1)
203-203: 🧹 Nitpick | 🔵 TrivialUse
Buffer.allocfor repetitive strings.Per coding guidelines, use
Buffer.alloc(count, fill).toString()instead of'x'.repeat(count).Suggested fix
- const largeValue = "x".repeat(16384); // 16KB value + const largeValue = Buffer.alloc(16384, "x").toString(); // 16KB value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/fetch/chunked-trailing.test.js` at line 203, The test currently creates a large string using "'x'.repeat(16384)" assigned to the variable largeValue; replace that repetition with a Buffer-based creation to follow guidelines by using Buffer.alloc(16384, 'x').toString() so largeValue is produced via Buffer.alloc instead of String.prototype.repeat; update the assignment to largeValue in chunked-trailing.test.js accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/fetch/chunked-trailing.test.js`:
- Line 16: The test contains unnecessary socket.uncork() calls (calls to the
variable named socket) that have no effect without a preceding socket.cork();
either remove the socket.uncork() calls at the three occurrences or, if you
intended to batch writes, add socket.cork() immediately before the related write
sequence and keep socket.uncork() after the writes; update the occurrences where
socket.uncork() is used accordingly.
---
Outside diff comments:
In `@test/js/web/fetch/chunked-trailing.test.js`:
- Line 203: The test currently creates a large string using "'x'.repeat(16384)"
assigned to the variable largeValue; replace that repetition with a Buffer-based
creation to follow guidelines by using Buffer.alloc(16384, 'x').toString() so
largeValue is produced via Buffer.alloc instead of String.prototype.repeat;
update the assignment to largeValue in chunked-trailing.test.js accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6467acf4-0678-4d08-916c-884d9f23789c
📒 Files selected for processing (4)
test/js/bun/http/bun-serve-file.test.tstest/js/bun/jsc/bun-jsc.test.tstest/js/bun/terminal/terminal.test.tstest/js/web/fetch/chunked-trailing.test.js
| socket.write("5\r\nHello\r\n"); | ||
| socket.write("7\r\n, world\r\n"); | ||
| socket.write("0\r\n"); | ||
| socket.uncork(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unnecessary socket.uncork() calls.
These uncork() calls have no effect without a preceding cork(). They can be safely removed or you could add socket.cork() before the writes if you intended to batch them for a single TCP segment.
Also applies to: 121-121, 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/fetch/chunked-trailing.test.js` at line 16, The test contains
unnecessary socket.uncork() calls (calls to the variable named socket) that have
no effect without a preceding socket.cork(); either remove the socket.uncork()
calls at the three occurrences or, if you intended to batch writes, add
socket.cork() immediately before the related write sequence and keep
socket.uncork() after the writes; update the occurrences where socket.uncork()
is used accordingly.
There was a problem hiding this comment.
LGTM — the unbounded-wait-loop concern has been resolved in the follow-up commit.
Extended reasoning...
Overview
The PR touches only test files across four suites: bun-serve-file.test.ts, bun-jsc.test.ts, terminal.test.ts, and chunked-trailing.test.js. Changes are all synchronization and timing improvements (warm-up before RSS baseline, explicit sampleInterval for profiling, promise-based PTY data synchronization, and deferring server writes until the client request arrives).
Security risks
None — these are test-only changes with no production code modifications.
Level of scrutiny
Low. All changes are confined to test helpers with no exported API surface. Each fix has a clear, documented root cause and a mechanically sound fix. The one concern I raised repeatedly (the while (driverOk === 0 && \!stop) loop being potentially unbounded) has been addressed in commit 18dfa21: the setTimeout that flips stop is now registered before the loop, so it is always authoritative.
Other factors
The new bug report about two other PTY tests having the same async-data race is a pre-existing condition not introduced by this PR. The PR's own fix is correct and the remaining two tests are out of scope. The await gotData with no explicit timeout is a minor diagnostic nit but does not affect correctness — bun:test's default per-test timeout provides a backstop.
|
|
||
| test("existing terminal works with subprocess", async () => { | ||
| const dataChunks: Uint8Array[] = []; | ||
| const { promise: gotData, resolve: gotDataResolve } = Promise.withResolvers<void>(); | ||
|
|
||
| await using terminal = new Bun.Terminal({ | ||
| data: (_t, data) => dataChunks.push(data), | ||
| data: (_t, data) => { | ||
| dataChunks.push(data); | ||
| if (Buffer.concat(dataChunks).includes("hello")) gotDataResolve(); | ||
| }, | ||
| }); | ||
|
|
||
| const proc = Bun.spawn(["echo", "hello"], { terminal }); |
There was a problem hiding this comment.
🟣 The PR correctly identifies and fixes the PTY async-data race in 'existing terminal works with subprocess', but two other tests in the same describe block — 'creates subprocess with terminal attached' and 'terminal option creates proper PTY for interactive programs' — have the identical vulnerable pattern: await proc.exited followed immediately by Buffer.concat(dataChunks) without waiting for async data delivery. These tests predate this PR, but the root cause explicitly documented here applies equally to them.
Extended reasoning...
What the bug is and how it manifests
The PR author correctly identified that PTY data callbacks fire asynchronously and may arrive after proc.exited resolves, producing empty output. This is documented in the comment added to the fixed test: 'PTY data is delivered asynchronously and may arrive after the child exits; wait for the data callback rather than assuming it already fired.' However, the same race is present in two other tests in the same describe block that were not touched by this PR.
The specific code paths that are affected
-
'creates subprocess with terminal attached' (around line 959): sets up a data callback pushing to dataChunks[], then does await proc.exited, then immediately evaluates Buffer.concat(dataChunks).toString() checking for 'hello from terminal' — no synchronization on data delivery.
-
'terminal option creates proper PTY for interactive programs' (around line 987): same pattern — data callback pushes to dataChunks[], await proc.exited, then immediately Buffer.concat(dataChunks).toString() checking for 'TTY=true' — no synchronization on data delivery.
Both tests use inline terminal: { data: ... } options rather than a pre-existing Bun.Terminal object, but PTY data delivery is asynchronous regardless of how the terminal was constructed — the kernel delivers PTY output independently of process exit signaling.
Why existing code does not prevent it
The Node/Bun PTY master side delivers data events asynchronously through the event loop. When the child process exits, the OS signals process death before necessarily flushing all pending PTY output. The proc.exited promise resolves on the death signal, but buffered PTY data may still be in-flight. Without a synchronization point (like the gotData promise introduced for 'existing terminal works with subprocess'), reading Buffer.concat(dataChunks) immediately after await proc.exited is a race.
What the impact would be
These tests can produce intermittent failures on Alpine/musl — the exact platform cited in the PR's failure table (24/200 CI builds) — for the same reason the fixed test was failing. The PR's stated goal was to fix flaky tests in terminal.test.ts; leaving two structurally identical patterns unfixed means the file may continue to produce flaky results on Alpine even after this PR lands.
How to fix it
Apply the same Promise.withResolvers pattern to both tests: resolve when the expected string appears in the accumulated chunks, and await that promise after proc.exited.
Step-by-step proof of the race in 'creates subprocess with terminal attached'
- Bun.spawn(['echo', 'hello from terminal'], { terminal: { data: ... } }) is called.
- echo runs under the PTY and writes 'hello from terminal\n' to the PTY slave.
- The kernel schedules the process to exit; exit notification arrives and proc.exited resolves.
- The PTY master-side data event has not yet fired in the JS event loop (buffered in kernel or libuv).
- The test evaluates Buffer.concat(dataChunks).toString() — dataChunks is still empty.
- expect(combinedOutput).toContain('hello from terminal') fails with 'expected empty string to contain hello from terminal'.
This is a pre-existing condition. The PR increases awareness of this pattern by documenting and fixing one instance, making the remaining two unfixed instances more conspicuous by comparison.
There was a problem hiding this comment.
LGTM — the four test fixes address their documented root causes cleanly.
Extended reasoning...
Overview
This PR fixes flakiness in four test files: a pre-request warmup before the RSS baseline in bun-serve-file.test.ts, an explicit 50 µs sample interval in bun-jsc.test.ts, a Promise-based synchronization gate for async PTY data delivery in terminal.test.ts, and deferring all chunked-trailing server responses until the first socket.once('data', ...) event on Windows. All changes are test-only with no production code impact.
Security risks
None — no auth, crypto, permissions, or production code paths are touched.
Level of scrutiny
These are surgical, isolated test fixes. Each fix directly and mechanically addresses the documented race or resource-state issue (ECONNRESET on Windows, zero JIT traces, empty PTY output, cold-RSS baseline). The pattern is straightforward and the rationale is well-documented.
Other factors
No bugs were found by the automated hunting system. Earlier concerns I raised about fetch-proxy-tls-intern-race-fixture.ts are moot — that file's fix was addressed and merged separately as #29212 (commit 7c75a87 on main). The pre-existing PTY race in sibling terminal tests is a pre-existing issue not introduced by this PR. The await gotData in terminal.test.ts lacks an explicit timeout, but bun:test's 5-second default bounds it adequately for a process-exit scenario.
…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>
Each of these is a root-cause fix — no timeout increases, no threshold
bumps.
| Test | Failure rate (last 200 CI builds) | Root cause | Fix |
|---|---|---|---|
| `chunked-trailing.test.js` | 19/200 (Windows) | Server writes response
on `connection` before request arrives; closing with unread recv data
sends RST on Windows → ECONNRESET | Wrap all 23 servers in
`socket.once('data', ...)` |
| `fetch.upgrade.test.ts` | 33/200 (aarch64) | Read loop only exits on
stream `done`, which the upgraded body may never signal; `resolve()` was
called but loop kept awaiting `reader.read()` forever | Break loop and
cancel reader on close frame |
| `fetch-proxy-tls-intern-race-fixture.ts` | 41/200 (Linux) | After
probe loop floods proxy, driver's unbounded `fetch()` stalls so `await
driverDone` never returns after `stop = true` | Abort in-flight driver
fetch on shutdown; let driver complete one request before the probe
flood so the `driverOk > 0` sanity check holds |
| `bun-jsc.test.ts` | 10/200 (macOS x64) | After JIT warmup `fib(30)`
completes within the default 1ms sample interval → 0 traces on 3rd
`profile()` call | Pass `sampleInterval = 50` µs |
| `terminal.test.ts` | 24/200 (Alpine) | PTY data callback is async; may
fire after `proc.exited` resolves → empty output | Await the data
callback via promise |
| `bun-serve-file.test.ts` | 41/200 (Alpine) | RSS baseline taken cold;
first fetches allocate connection pool / read buffers that don't return
to OS after GC | Warm up before baseline |
Verified locally: all files pass repeatedly (fetch.upgrade 8/8 including
parallel stress, others 3/3+).
**Dropped from this PR:**
- `test-reporter.test.ts` — the gate-file approach still races on CI
because the inspector's `TestReporter.enable` is dispatched via
`postTaskTo` and may not drain while the child polls; A1 can end with
`test_id_for_debugger=0` and emit no end event. Needs product-side work.
- Webview tests — handled by separate PR.
Co-authored-by: Jarred Sumner <jarred@jarredsumner.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>
Each of these is a root-cause fix — no timeout increases, no threshold
bumps.
| Test | Failure rate (last 200 CI builds) | Root cause | Fix |
|---|---|---|---|
| `chunked-trailing.test.js` | 19/200 (Windows) | Server writes response
on `connection` before request arrives; closing with unread recv data
sends RST on Windows → ECONNRESET | Wrap all 23 servers in
`socket.once('data', ...)` |
| `fetch.upgrade.test.ts` | 33/200 (aarch64) | Read loop only exits on
stream `done`, which the upgraded body may never signal; `resolve()` was
called but loop kept awaiting `reader.read()` forever | Break loop and
cancel reader on close frame |
| `fetch-proxy-tls-intern-race-fixture.ts` | 41/200 (Linux) | After
probe loop floods proxy, driver's unbounded `fetch()` stalls so `await
driverDone` never returns after `stop = true` | Abort in-flight driver
fetch on shutdown; let driver complete one request before the probe
flood so the `driverOk > 0` sanity check holds |
| `bun-jsc.test.ts` | 10/200 (macOS x64) | After JIT warmup `fib(30)`
completes within the default 1ms sample interval → 0 traces on 3rd
`profile()` call | Pass `sampleInterval = 50` µs |
| `terminal.test.ts` | 24/200 (Alpine) | PTY data callback is async; may
fire after `proc.exited` resolves → empty output | Await the data
callback via promise |
| `bun-serve-file.test.ts` | 41/200 (Alpine) | RSS baseline taken cold;
first fetches allocate connection pool / read buffers that don't return
to OS after GC | Warm up before baseline |
Verified locally: all files pass repeatedly (fetch.upgrade 8/8 including
parallel stress, others 3/3+).
**Dropped from this PR:**
- `test-reporter.test.ts` — the gate-file approach still races on CI
because the inspector's `TestReporter.enable` is dispatched via
`postTaskTo` and may not drain while the child polls; A1 can end with
`test_id_for_debugger=0` and emit no end event. Needs product-side work.
- Webview tests — handled by separate PR.
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
…always fires (#33998) Fixes `test/js/bun/jsc/bun-jsc.test.ts` going red on the debian 13 x64 lane in [build 72010](https://buildkite.com/bun/bun/builds/72010#019f546c-0875-422c-8f22-fa88eac52a1b): ``` 215 | expect(result2.stackTraces.traces.length).toBeGreaterThan(0); error: expect(received).toBeGreaterThan(expected) Expected: > 0 Received: 0 ✗ bun:jsc > profile can be called multiple times [4.71ms] ``` ### Cause The test profiles `fib(26)` three times at a 50µs sample interval and asserts each run collected at least one trace. #33072 shrank the workload from `fib(30)` to `fib(26)` so three runs fit inside the per-test timeout on slow debug builds. On fast release hardware a JIT-compiled `fib(26)` can finish inside one 50µs interval (the whole 3-call test completed in 4.71ms there), so the sampler thread simply never fires during the second call. This is the same race #28873 addressed by lowering the sample interval; lowering the instruction count in #33072 reopened it. ### Fix Loop `fib(18)` for a fixed 10ms of wall-clock per `profile()` call instead of relying on a single `fib(n)` being slow enough. 10ms is ~200 sample intervals, so the sampler thread is guaranteed time to fire regardless of JIT tier or how slowly it wakes after `pause()`/`start()`. On debug builds the loop still exits after ~12ms of JS work (one `fib(18)` of overshoot), so the test is no slower there than before. The test still catches the original regression from #25939 (using `shutdown()` instead of `pause()`): a shut-down sampler returns zero traces no matter how long the workload runs. ### Verification - `USE_SYSTEM_BUN=1 bun test test/js/bun/jsc/bun-jsc.test.ts -t 'profile can be called multiple times'`: 30/30 pass, ~80 traces per call - `bun bd test test/js/bun/jsc/bun-jsc.test.ts`: 36 pass / 0 fail, profile test ~1.6s (dominated by the profiler's JSON reporting, same as before) This is a timing flake that does not reproduce on the dev container (`fib(26)` takes ~800µs there after JIT warmup, well above 50µs), so there is no deterministic fail-before to show; the CI failure above is the reproduction. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
Each of these is a root-cause fix — no timeout increases, no threshold bumps.
chunked-trailing.test.jsconnectionbefore request arrives; closing with unread recv data sends RST on Windows → ECONNRESETsocket.once('data', ...)fetch.upgrade.test.tsdone, which the upgraded body may never signal;resolve()was called but loop kept awaitingreader.read()foreverfetch-proxy-tls-intern-race-fixture.tsfetch()stalls soawait driverDonenever returns afterstop = truedriverOk > 0sanity check holdsbun-jsc.test.tsfib(30)completes within the default 1ms sample interval → 0 traces on 3rdprofile()callsampleInterval = 50µsterminal.test.tsproc.exitedresolves → empty outputbun-serve-file.test.tsVerified locally: all files pass repeatedly (fetch.upgrade 8/8 including parallel stress, others 3/3+).
Dropped from this PR:
test-reporter.test.ts— the gate-file approach still races on CI because the inspector'sTestReporter.enableis dispatched viapostTaskToand may not drain while the child polls; A1 can end withtest_id_for_debugger=0and emit no end event. Needs product-side work.