Repository navigation
test(serve): speed up bun-server.test.ts and tighten its assertions - #40093
Conversation
The graceful-stop GC stress test ran 400 rounds or 8 seconds because Bun.gc(false) only collects: dead cells stay intact until the lazy sweeper reaches their block, so the unfixed build took 15 to 150 rounds to crash. Bun.gc(true) also sweeps synchronously, which zaps a collected wrapper at once, and the unfixed build then crashes in round 2 every time. Run 16 rounds with a smaller heap churn. Start the idle-CPU samples after the echo burst and sample 3 x 500 ms. Poll heapStats() with a deadline instead of fixed 30-iteration loops with sleeps, and read the prototype floor while the throwaway server is the only live instance. Await the server-side abort instead of sleeping in the abort-signal tests, half-close the truncated upload once the server has consumed the chunk, and end HEAD probes with Connection: close instead of a 100 ms timer. Run the remaining top-level tests and describes concurrently where the one-line test form still fits. Assert full responses (status, headers, body), pendingRequests counts around stop(), exact status lines, the negotiated subprotocol and close frame, and piped stdout/stderr of every fixture. Bind the fixtures to 127.0.0.1 and pass PORT=0 to the nitro fixture instead of port 3000.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 1 minute), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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)
Comment |
|
Updated 11:30 AM PT - Aug 22nd, 2026
✅ @robobun, your commit 8503cfa5e35d5fd20181feaf196a5f33479530e3 passed in 🧪 To try this PR locally: bunx bun-pr 40093That installs a local version of the PR into your bun-40093 --bun |
|
Status: PR ready for review. How I measured, same machine and build each time:
Detection check for the GC stress fixture on bun 1.3.14 (the last release without #35130): the old fixture failed between round 15 and 153 (median 19, 40 runs). The new fixture fails in round 2 in 60 of 60 runs. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a test-only speedup that follows the repo's test guidelines closely (await events instead of sleeps, stronger assertions, concurrent subprocess tests, drained pipes). Because it's a large diff that also reduces the graceful-stop GC stress test from 400 rounds to 16 (justified by switching to Bun.gc(true) with empirical data on the unfixed build) and reworks how the heapStats prototype floor is measured, a human look at those two coverage-sensitive changes would still be worthwhile.
What was checked:
toSpawn("Completed: 10")/toSpawn("")match the fixtures' actual output afterbunRun's trim.- The new
baseline = serverCount() - 1computation: the trivial server is stopped and unreferenced, so the laterdrain(baseline)/ 5-round GC loops collect it alongside the test server; the existingafterStopGC > baselineandafter == baselineassertions remain sound. tryWritePending'sdata.slice(written)fix keeps the unsent tail (was a real bug in the old test), and thechunkConsumedhandshake can't deadlock.- Newly-concurrent describes each use
port: 0and per-test servers; the in-processBun.gc(true)calls in "handler liveness" don't affect correctness of siblings holding live references.
Extended reasoning...
Overview
This PR touches only test code: test/js/bun/http/bun-server.test.ts (~600 diff lines in a ~3400-line file) plus three fixtures it spawns (bun-websocket-cpu-fixture.js, js-sink-sourmap-fixture/index.mjs, rejected-promise-fixture.js). No src/ changes. The goal is to cut the file's fixed-wait cost from ~10-16s to ~2s while tightening assertions.
Security risks
None. Test-only; no auth, crypto, or user-input paths touched. Fixture hostname changes from localhost to 127.0.0.1 are a robustness fix for dual-stack hosts (bind and dial the same address).
Level of scrutiny
Medium-high. Although test-only, two changes trade iteration count for determinism in tests that guard against use-after-free regressions in the HTTP server (js_value downgrade gate from #35130):
- The graceful-stop GC stress fixture drops from 400 rounds / 8s to a fixed 16 rounds by switching
Bun.gc(false)→Bun.gc(true). The author's reasoning (synchronous sweep zaps dead cells immediately, so a collected wrapper faults on the very next dispatch) is correct per JSC semantics, and the description reports 60/60 round-2 failures on bun 1.3.14. This is a well-argued change but reducing a stress test's iteration count is the kind of coverage decision a maintainer should sign off on. - The heapStats prototype-floor baseline is now read as
count - 1while a throwaway server is the only live instance, replacing a 30-iterationdrain(0)that could never exit early. I traced the assertions (afterStopGC > baseline,after == baseline) and confirmed the still-uncollected trivial server is swept by the subsequent GC loops before those checks, so the invariant holds — but it's subtle.
Other factors
Everything else lines up with REVIEW.md's test rules and is strictly a strengthening: sleeps replaced with awaited events (abort resolvers, chunkConsumed, Connection: close + end for the HEAD probe, echo-count gate in the CPU fixture); spawnSync → spawn inside a concurrent describe so the event loop isn't blocked; every spawned fixture now asserts {stdout, stderr, exitCode} via toEqual; the upload test's expect moved out of a catch block into a resolved payload; a real bug in the test's partial-write path (slice(0, written) → slice(written)) is fixed; the WebSocket protocol test now checks the negotiated subprotocol, echoed data, and close frame (and dropped a stray console.log). I verified the new toSpawn(...) stdout expectations against the fixtures and bunRun's trim, and that the nitro fixture's log line matches the new normalized-stdout assertion.
Given the size and the two coverage-sensitive judgment calls, deferring to a human reviewer rather than shadow-approving.
|
On the two coverage-sensitive points:
|
…keep the localhost fixtures The stress fixture parked keep-alive connections on a Bun.serve server and stopped it gracefully. Since #37074 that stop() closes idle connections inside the call, so every late request found a closed socket and the fixture's failure branches never ran (0 of 162 late requests dispatched on main, release and debug). node:http close() skips busy connections and leaves them keep-alive afterwards, as in Node, so the fixture now holds one request per connection across close(), drops the binding, GCs with a synchronous sweep, and requires every late request to be answered by the original handler. With the downgrade gate removed (is_drained without the connection term) it fails in round 1, 3 of 3 runs under the debug build; on main 162 of 162 late requests are answered. Keep the three hostname: "localhost" fixtures as they are. They reproduce the loopback resolution asymmetry that #38818 fixes in bun.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The de-flaking and assertion-tightening changes look correct. Because the graceful-stop GC stress fixture is a full rewrite — it retargets from Bun.serve (fetch + routes: trampolines, with a FinalizationRegistry "collected server answered" check) to node:http only, and flips "connection closed" from acceptable to a failure — a human should confirm that the #37074 rationale really makes the old Bun.serve/routes: coverage vacuous rather than dropped.
What was reviewed:
toSpawn("")/toSpawn("Completed: 10")against the unref fixtures —bunRuntrims stdout so both match.- The nitro fixture stdout regex vs
js-sink-sourmap-fixture/index.mjs:5597, andPORT=0is read at line 5580. tryWritePendingnow keepsdata.slice(written)(unsent tail) instead of the written prefix.- Every describe moved to
.concurrentruns its heapStats/liveness cases in separate subprocesses, so they don't shareobjectTypeCounts.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/bun-server.test.ts and its bun-websocket-cpu-fixture.js. It replaces fixed sleeps with awaited events, converts spawnSync to async Bun.spawn with piped/asserted {stdout, stderr, exitCode}, moves independent tests/describes to test.concurrent/describe.concurrent, tightens assertions to full-object toEqual shapes, fixes a real bug in the upload test's tryWritePending (kept the written prefix instead of the unsent tail), and rewrites the graceful-stop GC stress fixture. No src/ code changes.
Security risks
None. All changes are to tests and a test fixture; no auth, crypto, or runtime code is touched.
Level of scrutiny
Medium-high. The mechanical parts (await-the-event, concurrent describes, combined-object assertions, Connection: close in the HEAD probe, Bun.gc(true) in drain loops) are exactly what REVIEW.md prescribes and are low-risk. The GC stress fixture rewrite is not mechanical: the old fixture alternated Bun.serve({ fetch }) and Bun.serve({ routes: { "/r/:id" } }) rounds and used a FinalizationRegistry to detect "collected server answered"; the new fixture uses only node:http.createServer, drops the FinalizationRegistry, and treats a closed parked connection as a failure instead of acceptable. The stated rationale is that #37074 makes Bun.serve mark busy connections close-when-idle at stop(), so a late request there never reaches a handler and the old rounds were passing vacuously via the if (!r) continue escape hatch. If that's accurate, the retarget is a strengthening; if not, it drops the paramsObjectForRoute-on-a-swept-cell path the old comment called out. That's a judgment about the runtime's current behavior that a human familiar with #35130/#37074 should confirm — REVIEW.md is explicit that de-flaking must keep asserting the property the original assertion protected.
Other factors
The bug-hunting pass found nothing. I checked that bunRun trims stdout so toSpawn("Completed: 10") matches console.log("Completed:", 10) in unref-fixture-2.ts, that the nitro fixture reads process.env.PORT (so PORT: "0" takes effect) and prints the exact line the new regex normalizes, and that every describe moved to .concurrent runs its heapStats cases inside a spawned subprocess so process-global objectTypeCounts are not shared. The doHead change to Connection: close + read-to-end is strictly stronger than the old 100 ms wait for stray body bytes. The one open question — whether the Bun.serve routes: trampoline still needs its own GC-stress coverage — is why I'm deferring rather than approving.
|
On whether the old
|
One conflict, in test/js/bun/http/bun-websocket-cpu-fixture.js: main's version (#40093) waits for the echo burst to finish before sampling, which supersedes this branch's drain-then-settle change, so main's file is taken as is.
Problem
test/js/bun/http/bun-server.test.tstakes 9 to 16 s on every CI lane. The time is fixed waits, not work.stop()closes the idle parked connections, so every late request meets a closed socket (0 of 4770 dispatched on main).Fix
close()leaves busy connections keep-alive afterwards, as in Node. Each round holds a request per parked connection acrossclose(), drops the binding, runsBun.gc(true), and requires every late request to be answered by the original handler. With the downgrade gate removed it fails in round 1 (3 of 3). On main 162 of 162 late requests are answered.test.concurrent(fits on one line.pendingRequestsaroundstop(), exact status lines, and pipedstdout/stderr/exitCodefor every fixture (list in the notes).Background
Bun.gc(false)isHeap::collectSync: collection only.Bun.gc(true)isHeap::collectNow(Sync), which also sweeps, so dead cells with destructors are finalized inside the call.is_drained()lets it go Weak only when no request, listener, websocket, or connection is left. Without it the wrapper is collected while connections are open and its finalizer closes them.heapStats().objectTypeCountscounts prototypes with instances.count - 1while one instance is alive is the per-build floor the old 30-iteration drain produced.Notes
Runs of this revision: debug+ASAN
bun bd test2 runs (25.5 s, 26.2 s), releaseUSE_SYSTEM_BUN=15 runs (2.11 to 2.16 s), Windows x64 canary build of the same commit 4 runs (2.10 to 2.12 s, 79 pass). In this container three tests fail on main and on this branch alike (rejected promise handled by error method,parse source map and fetch small stream,abrubtly close a upload request): they bindhostname: "localhost", which lands on::1here while the client dials127.0.0.1. #38818 fixes that in bun, so the fixtures stay as they are. The local runs unsetHTTP_PROXY/HTTPS_PROXY: this container's egress proxy does not honor a bare::1inNO_PROXYfor the newhttp://[::1]:port/request (#37429). CI has no proxy.Stress fixture evidence:
is_drained()returnsis_closed()), debug build:FAIL round 1: a parked connection closed, 3 of 3. The finalizer'sapp.close()lands before the same round's late requests are answered.close()closes busy connections too.Bun.gc(true)and 16 rounds (round-2 detection on 1.3.14, 60 of 60, versus round 15 to 153 withBun.gc(false)). Review found the late requests unreachable on main, hence the retarget.Assertions added:
{status, text}on plain responses,pendingRequestsbefore and afterstop()plus the exact status line in the drain fixtures (HTTP/1.1 200 OK,HTTP/1.1 500 Internal Server Error,""for the force-closed connection), the negotiated subprotocol and the close frame in the custom-protocol test,AbortErrorplus the exact byte count for the truncated upload,redirected/urlon the redirect,Completed: 10from the unref fixture, and{stdout, stderr, exitCode}viatoEqualfor every spawned fixture. The nitro fixture runs withPORT=0instead of port 3000.Concurrency: six long-named tests (or tests with a timeout argument) stay sequential
test(because prettier reflowstest.concurrent(calls that exceed 120 columns and would reindent their bodies. The twoawait server.stop()tests also stay sequential: they assert that a fetch to the freed port fails.Timers replaced by events: the abort-signal stream tests await the server-side
abort; the truncated upload half-closes once the server consumed the chunk; the HEAD probe sendsConnection: closeand reads toend; the CPU fixture samples 3 x 500 ms after the last echo; the heapStats loops pollobjectTypeCountswith a 5 s deadline.bun testruns consecutive concurrent tests as one group (20 at a time, 5 under ASAN); a plaintest(ends the group.Fixed waits removed: the
Bun.sleep(15)/Bun.sleep(10)pairs in the two abort-signal stream tests,setTimeout(..., 100)in the upload test and indoHead, theBun.sleep(10)in six GC loops, the 8 s floor of the stress test, the 1000 ms CPU windows, and the 30-iterationdrain(0)baseline loops. TheBun.sleep(15)in "abort signal on server should only fire if aborted" stays: a negative check with no event to await, inside a concurrent block.Bun.spawnSyncin two "Server" tests blocked the event loop of the concurrent block. Both useBun.spawnnow.tryWritePendingkept the written prefix on a partial write (data.slice(0, written)). It keeps the unsent tail now.Release per-test after the change: the idle-CPU fixture (1.5 s) bounds the file. Everything else is under 0.2 s. No file under
src/changes. The edited CPU fixture is referenced only by this test file.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts