Repository navigation
Bun.serve: gate the graceful stop() drain promise on open connections - #35130
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesServer shutdown now tracks active HTTP connections and delays deinitialization until connections drain. uWS filter callbacks now remain balanced across socket closure and WebSocket upgrades. Tests cover keep-alive, TLS, reload, pipelined request, and WebSocket behavior. Server shutdown and connection accounting
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:30 PM PT - Aug 4th, 2026
❌ @robobun, your commit 4ca8102 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35130That installs a local version of the PR into your bun-35130 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/server/mod.rs:1652-1664— Thejs_value_for_new_request()503 gate is only wired into the dynamic-handler trampolines;StaticRoute::on/on_head,FileRoute::on_request,html_bundle::Route::on_request, andtrampoline::on_404have nohas_listener()check, so a keep-alive socket that survivedstop(false)(in-flight at stop time) can pipeline requests to a static route indefinitely withidleTimeout: 0. Since b2f9c2c dropped thedeinit_if_we_canidle sweep and made this gate the sole close mechanism, the comment here ("Any request that races this on a surviving socket is answered 503") is false for those siblings — either add the gate to each (they all reach the server via(*this).server.get()/ userdata) or restore a post-drain sweep.Extended reasoning...
What the bug is
This PR's fix has two mechanisms for closing keep-alive connections after a graceful
stop(false):close_idle_connections()instop_listening— closes sockets that are idle at stop time.- The new
js_value_for_new_request()gate in the dispatch trampolines — refuses (503 +Connection: close) any request that arrives on a socket that was non-idle at stop time and therefore survived (1).
Commit b2f9c2c removed the third mechanism (the
deinit_if_we_canidle sweep) with the stated rationale: "The per-request 503 + Connection: close already closes a surviving keep-alive on its next request." The comment atmod.rs:1654-1656restates this: "Any request that races this on a surviving socket is answered 503 by thejs_value_for_new_requestgate."But the 503 gate is only applied to the dynamic-handler trampolines (
on_request,on_user_route_request,on_request_for,on_user_route_request_for,upgrade_web_socket_user_route, and the Bun.serve arm ofon_websocket_upgrade_request). The following sibling trampolines — all registered directly with uWS viaapply_static_route/set_routes— dispatch new requests on the same keep-alive socket with nohas_listener()/js_value_for_new_request()check:StaticRoute::on(StaticRoute.rs:351) andStaticRoute::on_head(StaticRoute.rs:284) — callserver.on_pending_request()and serve the blob unconditionally, ending withresp.should_close_connection()(which only reflects the request'sConnectionheader, not listener state).FileRoute::on_request(FileRoute.rs:303) — same shape.html_bundle::Route::on_request— same shape.trampoline::on_404(mod.rs:3178) —resp.end(b"", false), i.e. keep-alive.
Step-by-step proof
Bun.serve({ port: 0, idleTimeout: 0, routes: { "/s": new Response("static") }, async fetch() { inflight.resolve(); await release.promise; return new Response("ok"); }, });
- Client opens a TCP connection, sends
GET / HTTP/1.1→on_request→ handler parks onrelease. Connection is non-idle (pending_requests == 1). server.stop(false):listener.take()succeeds;close_idle_connections()runs but skips this socket (uWS marks it non-idle while a response is outstanding).release.resolve()→ handler returns → 200 keep-alive →on_request_complete→deinit_if_we_can. Since b2f9c2c,deinit_if_we_cancontains noclose_idle_connections()call andpending_requests == 0alone does not close the socket. The socket is now idle and open.- Client pipelines
GET /s HTTP/1.1on the same socket → uWS routes toStaticRoute::on_request→StaticRoute::on. Nohas_listener()check.server.on_pending_request()bumps the count, the blob is written,resp.should_close_connection()returnsfalse(client did not sendConnection: close), response ends keep-alive. on_static_request_complete→deinit_if_we_can(again no sweep). Socket stays open.- Repeat step 4 forever. The fd is never released and the "stopped" server keeps serving
/s.
The PR's own third test (
graceful stop() closes a connection once its in-flight request completes) demonstrates step 3 reaches a trampoline (it observes["200","503"]); changing the pipelinedGET /afterto hit the static route would yield["200","200"]withclosed: falseinstead.Why existing code doesn't prevent it
resp.should_close_connection()only reads uWS'sHTTP_CONNECTION_CLOSEstate (client'sConnection: closeor HTTP/1.0), never the server's listener state.deinit_if_we_can(mod.rs:1707+) no longer sweeps — the block that calledclose_idle_connections()on!has_listener() && !TERMINATEDwas deleted in b2f9c2c.StaticRoute::onreaches the server via(*this).server.get()but never consultshas_listener();on_404ignores itsuser_dataentirely.
Impact
This is the exact bug class the PR is fixing (a stopped server keeps serving and holds fds forever with
idleTimeout: 0), left open on a common configuration —routes: { "/": new Response(...) }is the documented static-route form. The rationale for b2f9c2c ("the 503 gate already closes a surviving keep-alive on its next request") is invalid for these paths, and the in-code comment at 1654-1656 now overstates the guarantee. REVIEW.md: "Fix the whole class in the same PR — grep for every sibling site sharing the pattern." Bounded by the in-flight count atstop()time and recoverable viastop(true), but withidleTimeout: 0a client that keeps pipelining to a static route holds a "stopped" server open indefinitely.How to fix
Either:
- Add the
has_listener()gate to each sibling trampoline.StaticRoute::on/on_headandFileRoute::onalready have(*this).server.get(); check!server.has_listener()and callrespond_stopped_503(resp)(or end withclose_connection = true) before serving.on_404receives the server asuser_data— cast and check the same way. This mirrors the gate the PR added toon_request/on_user_route_request. - Or restore a targeted idle sweep (e.g. one
close_idle_connections()whenpending_requestsfirst reaches 0 on a gracefully-stopped Bun.serve server), if the Windows race that motivated b2f9c2c can be avoided by deferring the sweep past the flush.
The PR description's Fix bullet ("
deinit_if_we_cansweeps idle connections whenever it runs on a gracefully-stopped Bun.serve server") also needs updating — that sweep was removed in b2f9c2c.
|
Addressed the sibling-trampoline gap in 3413389: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/StaticRoute.rs`:
- Around line 262-277: Consolidate the duplicated stopped-server check into a
shared pub(super) refuse_if_stopped helper beside respond_stopped_503_any in
src/runtime/server/server_body.rs, accepting Option<AnyServer> and AnyResponse
and returning whether it responded. In
src/runtime/server/StaticRoute.rs:262-277, replace the local helper logic with a
call passing (*this).server.get(); in src/runtime/server/FileRoute.rs:331-336,
replace the inline check with the shared helper; and in
src/runtime/server/HTMLBundle.rs:271-275, preserve the existing None handling
and route the listener check through the shared helper.
🪄 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: 3d477c48-2200-40fb-83b9-2b83c2a0c96f
📒 Files selected for processing (7)
src/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rstest/js/bun/http/bun-server.test.tstest/vendor.json
There was a problem hiding this comment.
No bugs found this pass; the earlier rounds of feedback (sibling-trampoline gates, DEBUG diagnostic routes, on_404 test coverage, shared refuse_if_stopped_any helper) are all addressed in the latest commits. Deferring to a human because this is a user-facing semantic change to Bun.serve's stop() — it now closes idle keep-alive sockets and 503s post-stop requests, which the added test/vendor.json skip notes breaks an elysia test that asserted the old behaviour, and the PR overlaps #33662 / #34961 on the stop(true)-after-stop(false) gate.
What was reviewed:
stop_listeningabrupt-after-graceful block: unref/TERMINATED ordering,deinit_runningre-entrancy guard instop_from_js, and that the pre-callalready_terminatedcapture keeps the TCPapp.close()from being masked on SSL+http3. (The H3-side escalation gap flagged earlier was left as-is; already noted, not repeating.)- Every trampoline registered in
set_routesnow routes throughhas_listener()(StaticRoute/FileRoute/HTMLBundle viarefuse_if_stopped_any, on_404, on_bun_info_request, on_chrome_devtools_json_request, upgrade, H3 siblings); node:http dispatch intentionally keeps the finalize-only gate. refuse_if_stopped_anyplacement: called beforeref_()/on_pending_request()in each route so no ref/pending-count leaks on the 503 path;Noneserver left to caller as documented.- New tests: setup errors reject the awaited promise, subprocess pipes drained concurrently, bounded polling instead of fixed sleeps, and the routes-only fixture now makes the
on_404gate load-bearing.
Extended reasoning...
Overview
The PR changes Bun.serve's graceful stop() to (1) close idle keep-alive connections via close_idle_connections(), (2) refuse post-stop requests on surviving sockets with 503 + Connection: close at every request trampoline, and (3) let stop(true) after stop(false) actually escalate to a force-close instead of no-opping. It touches src/runtime/server/{mod,server_body,StaticRoute,FileRoute,HTMLBundle}.rs, adds ~230 lines of tests to bun-server.test.ts, updates two existing tests to expect 503 instead of 200, and adds a vendor-test skip for elysia's core*stop.test.ts.
Security risks
None identified. The change tightens behaviour (a stopped server refuses more, not less). The 503 gate on the DEBUG-only diagnostic routes (/bun:info, chrome-devtools JSON) is a hardening, and refuse_if_stopped_any runs before any ref-count bump or on_pending_request() so the refusal path cannot leak.
Level of scrutiny
High — this is the Bun.serve lifecycle/teardown state machine. It changes what stop(false) means for existing keep-alive clients (previously they kept being served; now they're closed/503'd), which is a deliberate semantic shift the elysia vendor test was asserting against. The stop_listening change adds a new state (listener-already-taken + abrupt) with its own unref/TERMINATED/app.close() sequencing, and stop_from_js/dispose_from_js gain a re-entrancy guard on deinit_running. That's exactly the kind of ref-count / event-loop-ref / connection-lifecycle interaction a maintainer should sign off on.
Other factors
- API-semantics decision: the PR body argues the old behaviour was a bug (fd leak, stopped server keeps serving), and the linked #6632 supports that. But the elysia skip shows at least one downstream framework encoded the old semantics in a test; a maintainer should confirm this is the intended contract for
stop(false). - PR overlap: the description says this overlaps #33662 and #34961 on the abrupt-after-graceful gate; a human should coordinate which lands.
- Prior review rounds: three rounds of my inline findings (sibling trampolines, DEBUG routes, on_404 coverage, unref-claim in test comment) were each addressed in follow-up commits (6a82c47, 3413389, cc765e4, 3b0a53d, f3226d1). The remaining H3 abrupt-after-graceful gap I flagged at 14:07 was resolved without a code change — low-impact and already on record, not re-raising.
- Test quality: the new tests spawn subprocesses with
bunEnv, drain stdout/stderr/exited concurrently, poll with a deadline rather than fixed sleeps, and assert combined{stderr, out, exitCode}objects. The routes-only fixture in the pipelined-503 test now actually reacheson_404(per f3226d1).
Given the semantic change, the server-lifecycle state-machine edits, and the cross-PR overlap, this warrants a human look even with no bugs found.
|
CI at f3226d1 (build #77903): 191/196 jobs passed. The two red tests are |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can we move this check to the very start of a request handler before we allocate? Instead of allocating and then checking. We could even move this to uWS with a flag on the App itself.
|
Moved into uWS in cb06df6: a new
|
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Note that stop() / stop(false) should be graceful; by design it should not be closing existing connections
|
Understood. To make sure I scope this correctly, which of these do you want
If (1) is the intended graceful semantics, then the bug from the original report is just the Happy to narrow it down; just want to pick the right contract before I rework it. |
There was a problem hiding this comment.
Beyond the two inline nits, on the cb06df6 architecture I checked that the uWS draining gate is inert for node:http (start_draining() is gated on on_node_http_request.is_empty(), so the flag stays false and the HttpContext.h check never fires there), and that endWithoutBody(std::nullopt, true) sets HTTP_CONNECTION_CLOSE so the post-parse shouldCloseConnection() path shuts the socket down after the 503 run flushes. Deferring — this changes stop() semantics (breaks elysia's stop test), touches vendored uWS, and overlaps #33662/#34961; a maintainer should sign off.
Extended reasoning...
The final commit moved the 503 gate into uWS as a per-context draining bit checked in the request-handler lambda before routing, replacing the per-trampoline Rust gates my earlier reviews covered. Since I hadn't reviewed that architecture, I verified the two obvious cross-cutting concerns: node:http shares HttpContext<SSL> but never has draining set (the Rust caller gates on on_node_http_request.is_empty()), and the 503's closeConnection=true reaches internalEnd → HTTP_CONNECTION_CLOSE → the post-consumePostPadded shutdown check, so the socket does close after the pipelined 503s are written. The two nits this run are test-prose/assertion-strength only. Not approving because this is a user-visible behaviour change to Bun.serve().stop() (the elysia vendor-test skip documents that), it modifies vendored uWS lifecycle code, and it overlaps two other open PRs on the stop(true)-after-stop(false) gate — those are maintainer calls.
cb06df6 to
0a1c51f
Compare
|
Reworked per the review ("stop(false) should be graceful; by design it should not be closing existing connections"): dropped the uWS |
…leConnections() (#37074) Requested by @Jarred-Sumner: make `server.stop()` close idle connections, and add a `closeIdleConnections()` method to the HTTP server. The method already existed at runtime (node:http uses it internally) but was missing from the type declarations and docs; the real change is in `stop()`. ## Before ```js const server = Bun.serve({ idleTimeout: 30, fetch: () => new Response("ok") }); // open a keep-alive connection, complete one request, leave it idle await server.stop(); // hangs until idleTimeout reaps the idle socket ``` A graceful `stop(false)` closed only the listener. Idle keep-alive connections lingered until `idleTimeout` (default 10s, forever with `idleTimeout: 0`), and since #35130 the drain promise waits on open connections, so `await server.stop()` hung on them. ## What changes Connection state at `stop(false)` time: | state | result | | --- | --- | | idle keep-alive | closed immediately | | request/response in flight | response delivered in full, then closed | | mid-request (partial head received) | spared until it completes or the client closes | | open WebSocket | untouched (drains on its own, as before) | | node:http server | unchanged, exact Node semantics (see below) | `server.closeIdleConnections()` keeps its one-shot Node semantics: closes currently idle connections, spares busy ones without marking them, keeps listening. It returns the number of connections it closed, and is now declared in `packages/bun-types` and documented. ## How - uws `App::closeIdle(closeWhenIdle)` keeps the existing idle sweep and, when asked, marks busy connections with a new connection-scoped `HTTP_CLOSE_WHEN_IDLE` state bit. `shouldCloseConnection()` honors the bit only while the socket is actually idle, so mid-request connections are left alone until their work completes. Graceful `stop_listening` runs the sweep with the mark for Bun.serve servers. - `markDone()` used to set `isIdle` while node:http pipelined responses were still queued, so a `closeIdleConnections()` in that window could kill a connection mid-pipeline; `isIdle` now accounts for the queue, and `resetResponseState()` clears it when a response (re)starts. - The post-completion close gates never ran for a response that ends corked outside the parser (the common async-handler case): `internalEnd`'s own uncork releases the cork slot, so `cork()`'s post-uncork close check was skipped. `internalEnd` and the end-without-body shim now run the gate themselves when outside the parser. This also makes `Connection: close` teardown reliable on that path. - node:http servers are exempt from the `stop()` sweep: Node's `close()` closes idle connections exactly once (Bun's `_http_server.ts` already calls `closeIdleConnections()` explicitly), and a connection whose response completes after `close()` stays keep-alive until its timeout reaps it. Verified against Node v26.3.0; the sweep would have closed it early and raced already-queued pipelined requests. A connection that was busy at stop() time and closes after its response also drops a request pipelined behind that response (the connection closes at its first idle moment). That matches the existing policy for async pipelining on Bun.serve, and RFC 9112 9.3.2 requires pipelining clients to retry unanswered requests on a new connection. ## Tests Reworked the drain-promise fixtures in `test/js/bun/http/bun-server.test.ts` to pin the new semantics (idle closed by stop, in-flight response delivered then closed, mid-request spared, stop(true) escalation, one-shot `closeIdleConnections()` sweep that spares busy connections and keeps listening), and updated the late-keep-alive tests: Bun.serve now closes the drained connection instead of serving a late pipelined request, while the node:http variant still delivers the queued pipelined response. Local runs: `bun-server.test.ts`, `serve.test.ts`, `serve-http3.test.ts`, `bun-serve-static.test.ts`, `serve-body-leak.test.ts`, `websocket-server.test.ts`, `node-http.test.ts`, and 46 vendored Node http server/keepalive/pipeline/close tests (including `test-http-server-close-idle.js` and `test-http-server-close-idle-wait-response.js`) pass with the change; the only failures are ones that also fail on an unmodified build in this environment (requestIP v6, privileged port, http proxy, /bun:info loopback, two stream tests). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · 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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
The stop-keepalive churn test added here was flaking on darwin CI with foreign-listener responses (a macOS ephemeral-allocator quirk lets a wildcard port-0 bind land on a port a leaked process holds at 127.0.0.1). Fixed by binding loopback in #37141. |
`test/js/bun/http/bun-server.test.ts` has been flaking on darwin lanes since the graceful `stop()` drain work landed (#35130 on Aug 5, #37074 on Aug 6): about 30 failing builds in the 36 hours after the second merge, plus the 90s-timeout reported red in build [90105](https://buildkite.com/bun/bun/builds/90105). Three distinct failure modes, each with a verified cause. This PR is test-only; the code paths the tests pin are unchanged and still covered (see Verification). ## 1. GC churn test answered by a foreign server `request on a connection surviving graceful stop() never reaches a collected handler` failed with ``` FAIL fetch round 11: bad initial response [{"status":200,"body":"\n <!DOCTYPE html>\...//x/-/static/main.55cdd4aea43adabcb109.js\"></script>..."}] ``` That body is a verdaccio web UI page; the `routes` variant got verdaccio's 404 `{"error":"no such package available"}`. A freshly dialed connection to the fixture's own freshly bound port was answered by an npm test registry. Cause, reproduced on a macOS 14 CI host: the ephemeral port allocator on macOS honors only exact-address conflicts. A wildcard `Bun.serve({ port: 0 })` bind gets handed a port that another process already holds at `127.0.0.1` (one collision per ~16,384 binds, i.e. once per wrap of the ephemeral range; a loopback-bound holder under both a real node and a bun process, same result). Connects to `127.0.0.1:port` then reach the more specific foreign listener. The churn fixture performs hundreds of wildcard binds per run, so it finds such a port regularly, and leaked verdaccio processes supply the listeners: `VerdaccioRegistry.stop()` calls `kill(0)`, which is a liveness probe rather than a kill (regressed in #16540; fix open in #36352). Fix: bind the fixture's servers (and decoys) to `127.0.0.1`, the address the parks dial. Verified on the same host: 36,000 loopback-bound port-0 binds against a live loopback holder, zero collisions. This protects the test regardless of what else leaks on the machine; #36352 independently removes the main leak source. ## 2. Parse-window test 90s timeout (the build 90105 red) ``` ✗ server.stop() drain promise counts open connections > a response completing inside another socket's parse window still closes its drained connection [90001.99ms] ^ this test timed out after 90000ms. ``` The fixture wrote a partial request head on connection A and waited 20 `setImmediate` ticks before `stop(false)`. A partial head gives the server nothing observable, and on a loaded darwin host those ticks can elapse before the server has even accepted the socket. Verified on a macOS 14 CI host: when `stop(false)` runs while a handshake-completed connection is still waiting in the accept queue, macOS strands it - never accepted, never counted, never closed, and the client side stays silently open. (Linux keeps the pending accept, which is why this never fired there.) In the fixture that means `/poke` never dispatches, `releaseB` never resolves, B stays parked, and `while (!a.closed || !b.closed)` spins until the test timeout with no other output - exactly the observed signature. Fix: A's poke is now a `POST` with a held body. The handler's dispatch is awaited before `stop()` (so the sweep provably sees a busy connection), and writing the 2-byte body afterwards completes the request. The body's fin chunk is delivered inside A's parse window, so B's completion still runs in that window's microtask drain - the per-socket close-gate property the test exists to pin is exercised exactly as before. ## 3. Mid-request sparing test resolving early Build [89946](https://buildkite.com/bun/bun/builds/89946) hit `error: stop() resolved while a mid-request connection was open` in `a connection mid-request survives stop() until the client closes`. Same root cause: the connection's partial head had not reached the server when `stop()` ran, so the server (correctly) had nothing to count and the drain promise resolved. Fix: the mid-request state is staged as a partial second head on a keep-alive connection that already completed a full request, so the server demonstrably owns the socket. If the sweep still closes it, the head had not arrived and the connection was legitimately idle - that round proves nothing about sparing, so it is voided and retried on a fresh server (bounded, fails loudly if every round races). A resolution while the socket is left open is still reported as the bug it would be. Confirmed-spared rounds run in two variants, and the test requires one success of each: - destroy: the client hangs up and that resolves the drain (the original assertion). - complete: the client finishes the head after `stop()`. The request must still dispatch and be answered, which requires the close-when-idle mark to survive the dispatch's response-state reset (`HTTP_CONNECTION_SCOPED` in uWS `HttpResponseData.h`), and the mark must then close the served connection. This preserves the dispatch-after-stop coverage the old parse-window staging provided incidentally (its partial head completed after `stop()`), which the redesign in section 2 otherwise moves ahead of the stop. ## Verification - `bun bd test test/js/bun/http/bun-server.test.ts`: 74 pass / 3 fail, the 3 failures (`parse source map and fetch small stream`, `rejected promise handled by error method`, `abrubtly close a upload request`) fail identically on an unmodified checkout in this environment. - Drain block run 5x, churn test 2x locally: all pass. - Both redesigned fixtures extracted and run 30x on a macOS 14 CI host under the exact canary from build 90105 (`89d30ad11`): 60/60 pass, plus 40x under a 14-way CPU-spin load: all pass. - Coverage check for the parse-window test: patching the `internalEnd` close gates back to the context-wide `isParsingHttp` bit (the bug #37074 guards against) makes the redesigned test fail by timeout, and restoring the per-socket gate makes it pass. - Coverage check for the mid-request test's complete variant: dropping `HTTP_CLOSE_WHEN_IDLE` from `HTTP_CONNECTION_SCOPED` (so the mark is wiped when the post-stop request dispatches) makes it fail by timeout; restored, it passes. The updated fixture also runs 30/30 on the macOS 14 CI host under the build 90105 canary, and hangs as expected under a pre-#37074 build. The remaining hazard - any long-running wildcard port-0 listener on darwin CI can have its loopback traffic stolen by a later explicit `127.0.0.1` bind such as `VerdaccioRegistry`'s `randomPort()` (range 1024-65535 overlaps the kernel's ephemeral range) - is worth addressing in the harness separately; noted on #36352. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
…37141) `test/js/bun/http/bun-server.test.ts` has been flaking on darwin lanes since the graceful `stop()` drain work landed (oven-sh#35130 on Aug 5, oven-sh#37074 on Aug 6): about 30 failing builds in the 36 hours after the second merge, plus the 90s-timeout reported red in build [90105](https://buildkite.com/bun/bun/builds/90105). Three distinct failure modes, each with a verified cause. This PR is test-only; the code paths the tests pin are unchanged and still covered (see Verification). ## 1. GC churn test answered by a foreign server `request on a connection surviving graceful stop() never reaches a collected handler` failed with ``` FAIL fetch round 11: bad initial response [{"status":200,"body":"\n <!DOCTYPE html>\...//x/-/static/main.55cdd4aea43adabcb109.js\"></script>..."}] ``` That body is a verdaccio web UI page; the `routes` variant got verdaccio's 404 `{"error":"no such package available"}`. A freshly dialed connection to the fixture's own freshly bound port was answered by an npm test registry. Cause, reproduced on a macOS 14 CI host: the ephemeral port allocator on macOS honors only exact-address conflicts. A wildcard `Bun.serve({ port: 0 })` bind gets handed a port that another process already holds at `127.0.0.1` (one collision per ~16,384 binds, i.e. once per wrap of the ephemeral range; a loopback-bound holder under both a real node and a bun process, same result). Connects to `127.0.0.1:port` then reach the more specific foreign listener. The churn fixture performs hundreds of wildcard binds per run, so it finds such a port regularly, and leaked verdaccio processes supply the listeners: `VerdaccioRegistry.stop()` calls `kill(0)`, which is a liveness probe rather than a kill (regressed in oven-sh#16540; fix open in oven-sh#36352). Fix: bind the fixture's servers (and decoys) to `127.0.0.1`, the address the parks dial. Verified on the same host: 36,000 loopback-bound port-0 binds against a live loopback holder, zero collisions. This protects the test regardless of what else leaks on the machine; oven-sh#36352 independently removes the main leak source. ## 2. Parse-window test 90s timeout (the build 90105 red) ``` ✗ server.stop() drain promise counts open connections > a response completing inside another socket's parse window still closes its drained connection [90001.99ms] ^ this test timed out after 90000ms. ``` The fixture wrote a partial request head on connection A and waited 20 `setImmediate` ticks before `stop(false)`. A partial head gives the server nothing observable, and on a loaded darwin host those ticks can elapse before the server has even accepted the socket. Verified on a macOS 14 CI host: when `stop(false)` runs while a handshake-completed connection is still waiting in the accept queue, macOS strands it - never accepted, never counted, never closed, and the client side stays silently open. (Linux keeps the pending accept, which is why this never fired there.) In the fixture that means `/poke` never dispatches, `releaseB` never resolves, B stays parked, and `while (!a.closed || !b.closed)` spins until the test timeout with no other output - exactly the observed signature. Fix: A's poke is now a `POST` with a held body. The handler's dispatch is awaited before `stop()` (so the sweep provably sees a busy connection), and writing the 2-byte body afterwards completes the request. The body's fin chunk is delivered inside A's parse window, so B's completion still runs in that window's microtask drain - the per-socket close-gate property the test exists to pin is exercised exactly as before. ## 3. Mid-request sparing test resolving early Build [89946](https://buildkite.com/bun/bun/builds/89946) hit `error: stop() resolved while a mid-request connection was open` in `a connection mid-request survives stop() until the client closes`. Same root cause: the connection's partial head had not reached the server when `stop()` ran, so the server (correctly) had nothing to count and the drain promise resolved. Fix: the mid-request state is staged as a partial second head on a keep-alive connection that already completed a full request, so the server demonstrably owns the socket. If the sweep still closes it, the head had not arrived and the connection was legitimately idle - that round proves nothing about sparing, so it is voided and retried on a fresh server (bounded, fails loudly if every round races). A resolution while the socket is left open is still reported as the bug it would be. Confirmed-spared rounds run in two variants, and the test requires one success of each: - destroy: the client hangs up and that resolves the drain (the original assertion). - complete: the client finishes the head after `stop()`. The request must still dispatch and be answered, which requires the close-when-idle mark to survive the dispatch's response-state reset (`HTTP_CONNECTION_SCOPED` in uWS `HttpResponseData.h`), and the mark must then close the served connection. This preserves the dispatch-after-stop coverage the old parse-window staging provided incidentally (its partial head completed after `stop()`), which the redesign in section 2 otherwise moves ahead of the stop. ## Verification - `bun bd test test/js/bun/http/bun-server.test.ts`: 74 pass / 3 fail, the 3 failures (`parse source map and fetch small stream`, `rejected promise handled by error method`, `abrubtly close a upload request`) fail identically on an unmodified checkout in this environment. - Drain block run 5x, churn test 2x locally: all pass. - Both redesigned fixtures extracted and run 30x on a macOS 14 CI host under the exact canary from build 90105 (`89d30ad11`): 60/60 pass, plus 40x under a 14-way CPU-spin load: all pass. - Coverage check for the parse-window test: patching the `internalEnd` close gates back to the context-wide `isParsingHttp` bit (the bug oven-sh#37074 guards against) makes the redesigned test fail by timeout, and restoring the per-socket gate makes it pass. - Coverage check for the mid-request test's complete variant: dropping `HTTP_CLOSE_WHEN_IDLE` from `HTTP_CONNECTION_SCOPED` (so the mark is wiped when the post-stop request dispatches) makes it fail by timeout; restored, it passes. The updated fixture also runs 30/30 on the macOS 14 CI host under the build 90105 canary, and hangs as expected under a pre-oven-sh#37074 build. The remaining hazard - any long-running wildcard port-0 listener on darwin CI can have its loopback traffic stolen by a later explicit `127.0.0.1` bind such as `VerdaccioRegistry`'s `randomPort()` (range 1024-65535 overlaps the kernel's ephemeral range) - is worth addressing in the harness separately; noted on oven-sh#36352. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
…close() (#38661) ### What does this PR do? Fixes a GC lifetime bug in node:http servers: after `server.close()`, a connection that was mid-request survives the close (it is not idle) and can still complete a request or hit a parse error, but the native server could already have let its JS wrapper — the only GC root of the request and `'clientError'` callbacks — be collected. A late dispatch then either silently dropped the event (wrapper already finalized) or called a collected function (collected but not yet swept), which surfaced under a fuzzer as UBSan `Structure.h: member call on null pointer of type 'JSC::ClassInfo'` in `Bun__JSValue__call` reached from `on_client_error_callback` via `HttpContext::onEnd`. Root cause: `listen()` skipped the uWS connection filter for node:http, so `active_connection_count` was always 0 there and the idle pass downgraded the wrapper to a bare weak `JSValue` as soon as the listener was gone and no request was in flight. #35130 fixed the same window for `Bun.serve` and left node:http out to keep its `close()` semantics. The filter is now registered for every server, and the idle predicate is split: `is_closed()` keeps driving the `stop()`/`'close'` promise and the loop unref exactly as before (node:http still does not wait for open connections there), while the new `is_drained()` also requires zero open HTTP connections before the wrapper may go weak. The `js_value_for_dispatch` doc comment now describes that invariant. Out of scope: Node's own `server.close()` waits for every connection before `'close'` and treats a never-used connection as idle; Bun's node:http still differs on both. ### How did you verify your code works? New test in `test/js/node/http/node-http-uaf.test.ts` (partial request → `close()` → `Bun.gc(true)` → finish the request, then send a malformed one): fails with `USE_SYSTEM_BUN=1` and on an unfixed debug build (no response, no `clientError`), passes with `bun bd test`; the same scenario under Node 26 gives the identical `200` + `HPE_INVALID_HEADER_TOKEN`. Also ran `node-http.test.ts`, the `bun-server.test.ts` stop/close/drain tests and the upstream `test-http-server-close-*` files on the debug build. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · 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 <!-- robobun:evidence:end -->
…ecycle paths on it (#38755) Follow-up to the review comment on 43afad2 (#38661) asking for a `config.is_node_http_server` flag. ### Problem - Five places inferred "this is a node:http server" from `config.on_node_http_request` being set: the close/drain predicate (`is_closed`, mod.rs:1616), the graceful-stop idle sweep exemption (`stop_listening`, mod.rs:1768), the custom `Expect` handler (`listen`, mod.rs:2986), the switch of the uWS context into node:http mode (`set_routes`, mod.rs:2738) and the reload guard that keeps a Bun.serve server from adopting the handler (`on_reload_from_zig`, server_body.rs:2291). - `on_node_http_request` is the current handler, and a `reload()` that omits it clears it on purpose (so requests route to `fetch` afterwards), while the server's kind cannot change after `listen()`: the uWS context mode and per-socket allocation size are fixed there. So a node:http server reloaded that way flipped to Bun.serve's stop semantics (`stop(false)` closed its idle keep-alive connections and waited for the rest) on a context still running in node:http mode, and could never get its handler back through a later reload. ### Fix - `ServerConfig` gains `is_node_http_server`, set once in `ServerConfig::from_js` when `onNodeHTTPRequest` is passed (how `node:http` creates its server) and carried across `clone_for_reloading_static_routes`. `on_reload_from_zig` copies individual fields, so a reload can neither set nor clear it, which is the property the five sites above want; they now read it, as does the "fetch or routes required" check in `from_js`. - Request routing (`negative_h1` and the `/*` fallback in `set_routes`, `on_web_socket_upgrade`) still follows the handler value, matching the reload comment in server_body.rs. - The connection filter registered in `listen()` is left unconditional on purpose. Bun.serve has had it since #35130 (it feeds `active_connection_count`, which makes `stop(false)` wait for open connections and keeps the wrapper alive while one can still dispatch; `bun-server.test.ts`, "server.stop() drain promise counts open connections"), and #38661 added it to node:http because `is_drained` needs the same count there (`node-http-uaf.test.ts`). Gating it on the flag in either direction removes one of those two fixes; if its per connection cost matters, replacing it with a counter maintained inside uWS for every server kind is the follow-up, and it does not need this flag. - Test: `test/js/bun/http/serve.test.ts`, "reload() that drops the node:http handler keeps the server's node:http stop() semantics", next to the two existing server-kind reload tests. It creates a server with `onNodeHTTPRequest`, reloads it over to `fetch`, parks one idle keep-alive connection, awaits `stop(false)` and sends a second request on the same connection. Without the src change the connection is closed by the sweep and the test sees `"closed"` (checked with main's src on a debug build, and with the released binary); with it the second request is answered. - Also verified on the debug build: `node-http-uaf.test.ts` (8 pass), `node-http.test.ts` (143 pass) and `bun-server.test.ts` (75 pass); the remaining failures in the last two are `localhost` connection refusals that fail identically with the released binary in this environment. ### Background - `node:http`'s `Server` is implemented on top of `Bun.serve()`: `_http_server.ts` passes an internal `onNodeHTTPRequest` option, and that option is what puts a server into node:http mode. `Bun.serve()` accepts it directly, which is how the tests build such a server without node:http. - The uWS connection filter is a callback uWS invokes when an HTTP connection is accepted (after the TLS handshake for HTTPS) and when it closes or is upgraded to a WebSocket; `NewServer` uses it to maintain `active_connection_count`. - `is_closed` is what `server.stop()`'s promise (node:http's `'close'` event) and the event loop ref wait for. Bun.serve's graceful stop also sweeps idle connections and waits for the rest; node:http's `close()` does neither, because node's JS layer sweeps idle connections itself and the rest are left to their timeouts. `is_drained` additionally requires zero open connections and is the point where the server's JS wrapper, the GC root of the handlers, may become collectible; it applies to both kinds. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts <!-- robobun:evidence:end -->
…40093) ### Problem - `test/js/bun/http/bun-server.test.ts` takes 9 to 16 s on every CI lane. The time is fixed waits, not work. - The GC stress test runs 400 rounds or 8 s, and since #37074 it tests nothing: a graceful `stop()` closes the idle parked connections, so every late request meets a closed socket (0 of 4770 dispatched on main). - Other fixed costs: 3 x 1000 ms CPU samples during the echo burst, 30-iteration GC loops with 10 ms sleeps, two 100 ms timers in the HEAD probe. ### Fix - The stress test targets node:http, whose `close()` leaves busy connections keep-alive afterwards, as in Node. Each round holds a request per parked connection across `close()`, drops the binding, runs `Bun.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. - Every other timer becomes the event it waited for (list in the notes). Top-level tests run concurrently where `test.concurrent(` fits on one line. - Stronger checks on full responses, `pendingRequests` around `stop()`, exact status lines, and piped `stdout`/`stderr`/`exitCode` for every fixture (list in the notes). - Verified, same machine and build: debug+ASAN 88.8 s to 25 s, release 10.4 s to 2.1 s, Windows x64 canary 11.1 s to 2.1 s. Pass set unchanged. ### Background - `Bun.gc(false)` is `Heap::collectSync`: collection only. `Bun.gc(true)` is `Heap::collectNow(Sync)`, which also sweeps, so dead cells with destructors are finalized inside the call. - The downgrade gate (#35130): the JS wrapper is the handlers' only GC root. `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().objectTypeCounts` counts prototypes with instances. `count - 1` while one instance is alive is the per-build floor the old 30-iteration drain produced. <details><summary>Notes</summary> Runs of this revision: debug+ASAN `bun bd test` 2 runs (25.5 s, 26.2 s), release `USE_SYSTEM_BUN=1` 5 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 bind `hostname: "localhost"`, which lands on `::1` here while the client dials `127.0.0.1`. #38818 fixes that in bun, so the fixtures stay as they are. The local runs unset `HTTP_PROXY`/`HTTPS_PROXY`: this container's egress proxy does not honor a bare `::1` in `NO_PROXY` for the new `http://[::1]:port/` request (#37429). CI has no proxy. Stress fixture evidence: - main's fixture on main (release, 400 rounds): answered 0, closed 4770. Debug build, 40 rounds: answered 0, closed 366. - PR fixture on main: answered 162 of 162, release (about 160 ms) and debug (5.8 s standalone). - gate removed (`is_drained()` returns `is_closed()`), debug build: `FAIL round 1: a parked connection closed`, 3 of 3. The finalizer's `app.close()` lands before the same round's late requests are answered. - bun 1.3.14 is not a control for the new fixture: its node:http `close()` closes busy connections too. - The first revision kept the Bun.serve fixture with `Bun.gc(true)` and 16 rounds (round-2 detection on 1.3.14, 60 of 60, versus round 15 to 153 with `Bun.gc(false)`). Review found the late requests unreachable on main, hence the retarget. Assertions added: `{status, text}` on plain responses, `pendingRequests` before and after `stop()` 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, `AbortError` plus the exact byte count for the truncated upload, `redirected`/`url` on the redirect, `Completed: 10` from the unref fixture, and `{stdout, stderr, exitCode}` via `toEqual` for every spawned fixture. The nitro fixture runs with `PORT=0` instead of port 3000. Concurrency: six long-named tests (or tests with a timeout argument) stay sequential `test(` because prettier reflows `test.concurrent(` calls that exceed 120 columns and would reindent their bodies. The two `await 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 sends `Connection: close` and reads to `end`; the CPU fixture samples 3 x 500 ms after the last echo; the heapStats loops poll `objectTypeCounts` with a 5 s deadline. `bun test` runs consecutive concurrent tests as one group (20 at a time, 5 under ASAN); a plain `test(` 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 in `doHead`, the `Bun.sleep(10)` in six GC loops, the 8 s floor of the stress test, the 1000 ms CPU windows, and the 30-iteration `drain(0)` baseline loops. The `Bun.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.spawnSync` in two "Server" tests blocked the event loop of the concurrent block. Both use `Bun.spawn` now. `tryWritePending` kept 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. </details> <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
Problem
The
server.stop(false)drain promise resolved while keep-alive HTTP connections were still open and still serving.idle→stopResolvedAfterMs: 0, connFinAt: null, servedAfterResolve: 1;inflight→ resolves at response-finish (~450 ms), connection open,/secondserved. Deterministic on 1.4.0.Separately,
server.stop(true)after an earlierserver.stop(false)was a silent no-op:stop_from_jsonly enteredstop()whilehas_listener(), and a prior graceful stop had already taken the listener.Cause
deinit_if_we_can(and theget_all_closed_promiseearly-return, and thestop_listeningunref gate) testedpending_requests == 0 && !has_listener() && !has_active_web_sockets(). Idle keep-alive HTTP connections are not in any of those terms; the predicate had no connection count, so it was satisfied while sockets were open and uWS kept routing requests on them.Fix
active_connection_count: Cell<u32>onNewServer, fed by a uWSfilterregistered inlisten()(fires+1on accept / post-TLS-handshake,-1fromHttpContext::onClose). On WebSocket upgrade the socket isus_socket_adopt-ed out of the HTTP group andHttpContext::onClosenever fires for it, sonote_websocket_openedmoves the count to the existing WebSocket tally.get_all_closed_promiseearly-return and thestop_listeningunref gate now include!has_active_connections(). The early-return also gains!has_active_web_sockets(): after an upgrade the connection count is 0, so on a websocket-only server this term is what keeps a repeatstop()call from returning a fresh resolved promise while the stored one is still pending.stop(false)does not close existing connections (per the review on the previous revision of this PR); the promise waits for them to close viaidleTimeout, client disconnect,server.closeIdleConnections()orserver.stop(true).stop_from_js/dispose_from_jsenterstop()for an abrupt stop whenever the app has not yet been terminated, andstop_listeningperforms theapp.close()teardown in that state, sostop(true)afterstop(false)force-closes the surviving connections.Memory safety
The deferred
js_valuedowngrade is also a use-after-free fix. Onmain, oncepending_requestshits 0 after a gracefulstop(),deinit_if_we_candowngrades the wrapper toWeakwhile surviving keep-alive connections can still dispatch. The wrapper's slots are the only GC root of the configured handlers, andJsRef::try_get()returns the rawJSValueof aWeakref with no liveness check, so after a GC pass a late request on such a connection calls swept cells:Bun.serveinstance created after the stop, a request on the stopped server's surviving keep-alive connection is answered by that other instance's handler, crossing any in-process boundary between listeners (public vs admin, per-tenant servers). Other occupants surface aserror: Expected a Response object, but received '6'(also''/undefined), response bodies resolving to unrelated objects, or a segfaultStructure.h: member call on null pointer of type 'JSC::ClassInfo'inBun__JSValue__call, reached fromNewServer::on_requestviaus_internal_dispatch_ready_poll(a loop dispatch against the collected wrapper, not a finalizer-ordering problem)With the connection count in the predicate, the wrapper stays
Stronguntil the last connection is gone, so a late dispatch always sees live cells.A standalone stress driver that creates fresh
Bun.serveinstances after every graceful stop confirms this: on unfixed builds it produces corrupted responses in release (about 1 per 120 stops over 72k rounds) and swept-cell sanitizer crashes under ASAN within 500 rounds, while this branch runs 1,000+ rounds under ASAN with zero reports.Verification
New
server.stop() drain promise counts open connectionsblock intest/js/bun/http/bun-server.test.ts:idle keep-alive connection holds the promise until the client closes/in-flight request's connection holds the promise past response end: fail-beforeresolvedEarly: true, resolvedWhileOpen: true; afterfalse, falseand the promise resolves once the client destroys the socket.stop(true) after stop(false) force-closes the surviving connection: fail-beforeclosed: false; afterclosed: true.New
request on a connection surviving graceful stop() never reaches a collected handlerstress test: parks pooled keep-alive connections acrossstop(), drops the server binding, churns the heap and forces GC, then sends late requests on the surviving connections. Rounds alternate between a plainfetchhandler and aroutes:param-route server, because the route dispatch reads the wrapper'sServerRouteListcell, a second collected-cell site (UBSan member call on nullTrailingArray<...ServerRouteList::IdentifierRange>inparamsObjectForRoute, reached fromon_user_route_request). Fails consistently onmain: 6/6 with the release build (wrong bodies, responses from an already-collected server, segfaults) and 9/9 with the debug ASAN build across both shapes of the test, hitting both UBSan sites. Passes repeatedly with this PR (~35 s under ASAN, ~8 s release).The
late keep-alive WebSocket upgrade after stop()test is updated: the wrapper downgrade is now deferred while the connection is open, so a pipelined upgrade on that connection reaches a live handler andserver.upgrade()succeeds (previously it was refused becausehandler.serverhad been cleared).bun-server.test.ts,serve.test.ts,node-http.test.tsandwebsocket-server.test.tsare unchanged apart from the usual environment-only failures that also fail onmain.node:http'sserver.close()callscloseIdleConnections()itself, so its observable behaviour is the same before and after.The
stop(true)-after-stop(false)gate overlaps #33662 and #34961; this PR carries it because the connection-count term makes it the only way to force the promise through when a client keeps the socket open.no test proof · iteration 24 · 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