test(serve): run serve-error-handler-stream cases in one subprocess - #35262
test(serve): run serve-error-handler-stream cases in one subprocess#35262robobun wants to merge 3 commits into
Conversation
Twelve concurrent debug/ASAN subprocesses each starting a server for a single request made this file the slowest on the debian x64-asan lane at 39s. The fixture now starts one server, runs all twelve (path, Connection) combinations against it, and prints the observed status/len/pulls for each as a JSON array; the test asserts the whole array in one toEqual so a regression shows exactly which cases truncated. The per-chunk Bun.sleep(4) / Bun.sleep(10) delays are replaced with Bun.sleep(0), which still yields to a macrotask and so still reaches the Pending branch in do_render_stream / handle_reject. Reverting the #33816 src/ change reintroduces the heap-use-after-free in uws_res_has_responded under the single subprocess, so regression detection is preserved. bun bd test (local, debug+ASAN): 8.4s -> 3.8s wall, 24.6s -> 3.7s user.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesStreaming error-handler test coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup that collapses twelve subprocess spawns into one while keeping all twelve cases and their exact expected values.
What was reviewed:
- Verified all 12 original (path × Connection) cases and their expected
{status, len, pulls}are preserved verbatim in the new results array. - Checked the shared
pullscounter is reset before each case and that sequentialawait res.text()means no cross-case bleed. Bun.sleep(0)still yields a macrotask so the Pending branch is reached; PR description confirms the UAF reproduces 3/3 with the fix reverted.- Harness conventions:
Promise.allpipe drain,await using,bunEnvspread,signalCode: nullassertion all present.
Extended reasoning...
Overview
This PR refactors test/js/bun/http/serve-error-handler-stream.test.ts and its fixture to run all twelve test cases in a single subprocess instead of twelve concurrent ones. The fixture now loops over the case matrix, resets the pulls counter per case, and emits a JSON array; the test asserts the full array plus stderr, exitCode, and signalCode in one toEqual. Per-chunk Bun.sleep(4)/Bun.sleep(10) are replaced with Bun.sleep(0), and the trailing orphaned-producer window drops from 100ms to 20ms. No production code is touched.
Security risks
None. Test-only change to a subprocess fixture that hits 127.0.0.1 on an ephemeral port.
Level of scrutiny
Low-to-moderate. This is a test-performance refactor with no runtime code changes. The main risk is silently weakening coverage, which the repo's review guidance calls out explicitly. I compared the old per-test expectations against the new results array line by line — every case, status, len, and pulls value matches. The PR author also verified the test still fails (ASAN UAF on debug, truncated len/pulls on release) after reverting the #33816 fix, which is exactly the "prove the test fails for the right reason" bar.
Other factors
- The module-global
pullscounter is now shared across cases, but each case runs sequentially andawait res.text()fully drains the body beforepullsis read and reset, so there is no interleaving. - The new
stderr: ""assertion is a slight tightening (previously stderr was only surfaced when stdout was empty); withdevelopment: falseand anerror()handler this holds, and the author reports 5/5 identical runs. - Harness idioms (
await using, concurrent pipe drain,{...bunEnv, ...},signalCodecheck) are all followed. The subprocess boundary that isolates ASAN crashes from the parent runner is preserved.
Address review: batch-at-end stdout left per-case progress invisible on a hang or mid-loop ASAN abort. The fixture now prints one JSON line per case as it completes and the test parses stdout line-by-line, so partial output names every case that finished and the diff points at the first one that didn't.
|
Updated 8:00 AM PT - Jul 23rd, 2026
✅ @robobun, your commit 52e45af082ac2c660393daee3961ee20b6aeed4a passed in 🧪 To try this PR locally: bunx bun-pr 35262That installs a local version of the PR into your bun-35262 --bun |
There was a problem hiding this comment.
LGTM — test-only refactor that collapses twelve subprocess spawns into one while preserving all twelve assertions. One inline nit about a stale header comment.
What was reviewed
- Confirmed all twelve original (path × Connection) cases and their expected
{status, len, pulls}values carry over unchanged into the newtoEqualarray. - Checked the shared module-level
pullscounter is reset before each sequential fetch and thatawait res.text()drains each stream before the next case starts, so counts don't bleed across cases. - Checked the
Bun.sleep(0)substitution and the reduced 100ms→20ms trailing window — the PR description verifies the pre-fix UAF still reproduces 3/3 under the new timing.
Extended reasoning...
Overview
Test-only performance refactor of test/js/bun/http/serve-error-handler-stream.test.ts and its fixture. Previously the test spawned twelve concurrent debug/ASAN subprocesses (one Bun.serve + one request each); it now spawns one subprocess that runs all twelve cases sequentially against a single server and emits NDJSON, which the test parses and asserts as one {results, stderr, exitCode, signalCode} object. Per-chunk Bun.sleep(4/10) delays become Bun.sleep(0) via a tick() helper. No production code is touched.
Security risks
None. Changes are confined to a regression test fixture and its driver; no auth, crypto, or user-facing code paths.
Level of scrutiny
Moderate — the file exists to guard a specific UAF regression (#33816), so the key question is whether coverage is preserved. The PR description addresses this directly: the author reverted the two set_has_marked_pending(true) lines and confirmed the new single-subprocess test still reproduces the ASAN heap-use-after-free 3/3 and fails on truncated len/pulls in release. I also line-by-line matched the twelve expected result objects against the original per-test assertions — every status, len, and pulls value is identical.
Other factors
- The refactor follows several REVIEW.md test conventions better than the original: it now asserts
signalCode: null(hang-guard), includesstderrin the asserted object so an ASAN report surfaces in the diff, and drains stdout/stderr concurrently withproc.exited. - The shared
pullscounter is module-level but reset per iteration; since cases run strictly sequentially and eachawait res.text()fully consumes the response stream before the next request, there's no cross-case bleed on the fixed build. - The one finding is a stale header comment ("as a JSON array" → NDJSON) left over from the first commit before 7188e2f switched to per-line output — cosmetic only.
- Wall time drops from 8.4s→3.8s locally (39s→much less on the ASAN lane), which is the stated goal.
There was a problem hiding this comment.
LGTM — the header-comment nit from my earlier pass is fixed in 52e45af, and no new concerns on this revision.
What was reviewed:
- All twelve original (path × Connection) cases and their expected
{status, len, pulls}values are preserved 1:1 in the consolidated assertion. pullsis reset per-iteration and requests are strictly sequential (await res.text()before the nextfetch), so the shared counter can't bleed across cases.Bun.sleep(0)still yields a macrotask; PR description confirms the reverted-fix UAF still reproduces 3/3 under the new timing, so the Pending branch is still reached.- Subprocess conventions (bunEnv spread,
await using, concurrent pipe drain,signalCodeassertion) match harness norms.
Extended reasoning...
Overview
Test-only refactor of test/js/bun/http/serve-error-handler-stream.test.ts and its fixture. The previous shape spawned twelve concurrent debug/ASAN subprocesses (one Bun.serve per case); the new shape spawns one subprocess that runs all twelve cases sequentially against a single server and emits one NDJSON line per case. Per-chunk Bun.sleep(4|10) becomes Bun.sleep(0), and the trailing orphaned-producer window shrinks 100ms → 20ms. No production code is touched.
Security risks
None. Changes are confined to two files under test/js/bun/http/; no runtime, native, or bindings code is modified.
Level of scrutiny
Low-to-moderate. It's a test-performance refactor, so the only real question is whether coverage or regression-detection power was silently lost. The PR description addresses that head-on: reverting the two set_has_marked_pending(true) lines from #33816 still reproduces the ASAN heap-use-after-free 3/3 under the new single-subprocess / sleep(0) timing, and on a release build the assertion fails on truncated len/pulls instead. That's the empirical proof the repo's review guidance asks for when de-flaking or speeding up a regression test.
Other factors
- I diffed the twelve expected result objects against the twelve original
test.concurrentbodies — everystatus,len, andpullsvalue matches exactly. - The shared module-level
pullscounter is safe: it's zeroed at the top of each loop iteration, and the loop awaitsres.text()before starting the next request, so no two stream bodies overlap. - The new assertion shape (
{results, stderr, exitCode, signalCode}in onetoEqual) is stronger than before — it now also assertsstderr === ""andsignalCode === null, and a mid-loop crash produces a diff naming the first missing case rather than a bare timeout. - My earlier inline nit (stale "JSON array" wording in the fixture header) was fixed in 52e45af and the thread is resolved; nothing else is outstanding on the timeline.
|
The changed test file passed on every lane across builds #78603 and #78627. The only red on #78627 is |
1 participant
What does this PR do?
serve-error-handler-stream.test.tswas the slowest file on the debian 13 x64-asan lane at 39s (build #78397). It spawned twelve concurrent debug/ASAN subprocesses, each starting aBun.serveinstance to handle a single request, so twelve copies of process and server startup were paid for ~150ms of actual stream pumping apiece.The fixture now starts one server, runs all twelve (path × Connection header) combinations against it in sequence, and prints one JSON line per case as it completes. The test spawns once, parses stdout line-by-line, and asserts the whole array with a single
toEqual, so a regression shows exactly which cases truncated; a mid-loop hang or crash leaves the completed cases in stdout so the diff points at the first case that didn't finish, and the subprocess'sstderrandsignalCodeare asserted alongside.The per-chunk
Bun.sleep(4)/Bun.sleep(10)delays are replaced withBun.sleep(0): that still yields to a fresh macrotask, so the sink's pump promise is still observed as Pending and thedo_render_stream/handle_rejectPending branch under test is still reached.Coverage is preserved
Connection: close) are still exercised with the same expectedstatus,len, andpulls.set_has_marked_pending(true)lines that Bun.serve: mark request pending when error() returns a streaming body #33816 added tosrc/runtime/server/RequestContext.rsand rebuilding reproduces theheap-use-after-freeinuws_res_has_respondedunder the single subprocess (3/3 runs) and the test fails; on a release build without ASAN the assertion fails on the truncatedlen/pullsvalues instead.Assertion improvements
toEqualover the full results array plusstderr,exitCode, andsignalCode, instead of twelve separate{result, exitCode}checks. A pre-fix release build now prints a diff naming every case that truncated rather than a mix of timeouts and scattered assertion failures.How did you verify your code works?
bun bd test test/js/bun/http/serve-error-handler-stream.test.tslocally (debug + ASAN):5/5 consecutive runs pass with identical output.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.