Bun.serve: close WebSockets with 1001 on server.stop() and let stop(true) follow stop(false) - #34961
Open
robobun wants to merge 6 commits into
Open
Bun.serve: close WebSockets with 1001 on server.stop() and let stop(true) follow stop(false)#34961robobun wants to merge 6 commits into
robobun wants to merge 6 commits into
Conversation
…rue) follow stop(false) server.stop() was blind to open WebSockets in three ways: - stop(false) only closed the listen socket. Open WebSockets stayed connected and kept serving traffic, and the returned promise never resolved because deinit_if_we_can() is gated on has_active_web_sockets(). - stop(true) closed WebSockets by raw us_socket_close, so both the server close handler and the peer observed code 1006 (abnormal) with no close frame instead of 1001 Going Away. - stop_from_js wrapped the whole call in has_listener(); after a prior graceful stop the listener is gone, so a following stop(true) did nothing. Combined with the first point, a "graceful then force" shutdown could never complete once a WebSocket was connected. Fix: add TemplatedApp::endAllWebSockets(code, reason) which walks every WebSocket group and calls WebSocket::end() (sends the close frame, fires the close handler, FIN). stop_listening now calls it with 1001 before closing the listener (graceful) or the app (abrupt), under the existing deinit_running re-entrance guard. stop_from_js/dispose_from_js also run when the listener is already gone but the app has not been terminated, and get_all_closed_promise's fast-path now checks has_active_web_sockets().
Contributor
|
Warning Review limit reached
Next review available in: 18 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 (3)
WalkthroughWebSocket shutdown now exposes an end-all-connections API through uWS, integrates close handshakes and abrupt termination into server lifecycle paths, updates closed-state checks, and adds Bun and Node HTTP regression tests for closure and garbage-collection behavior. ChangesWebSocket shutdown lifecycle
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
Author
|
Updated 12:05 PM PT - Jul 21st, 2026
✅ @robobun, your commit fae66be168f4015b9a70956d26275b7c4584f1d0 passed in 🧪 To try this PR locally: bunx bun-pr 34961That installs a local version of the PR into your bun-34961 --bun |
Collaborator
Author
|
Reproduced all three behaviours on
|
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
stop() now ends open websockets with 1001 before closing the listener, so the "stopped server with a live websocket" state these tests relied on no longer exists. Rewrite them to exercise the same invariants (wrapper survives GC while connected; error handler copied before user JS; no wrapper leak) via a WeakRef handle so the test can call stop() without itself rooting the wrapper.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ning guard endAllWebSockets walked head_sockets with a pre-captured next pointer, but end() fires the close handler synchronously and user JS there can terminate() a later socket, which rewrites its ->next into the loop's closed_head and derails the walk. Snapshot into a vector first. end_all_websockets_going_away/terminate_app set deinit_running with set(true)/set(false); a nested server.stop(true) from a close handler would clear the outer frame's guard. Use replace(true)/set(prev), and gate stop_from_js/dispose_from_js on !deinit_running so a nested stop() during an outer drain is a no-op rather than a re-entrant &mut borrow. Also wire test error events to reject the awaited promise, and add coverage for a close handler that terminates other sockets and calls stop(true) during the drain.
node:http Server#close() must leave upgraded sockets to the user (Node only stops accepting and closes idle keep-alives). Gate end_all_websockets_going_away() on !on_node_http_request so the Bun-native stop() behaviour is the only path that closes WebSockets. Add a node-http-with-ws regression test that keeps the ws open across server.close() and then observes a user-chosen close code. Also restore the let-else in stop_listening and drop an unread field from the bun-server.test.ts GC subprocess output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/mod.rs`:
- Around line 1696-1702: Trim the newly added explanatory comments in
end_all_websockets_going_away and terminate_app to three lines or fewer each.
Preserve only the essential re-entrancy and ordering invariants, including the
guard spanning the drain and stop() performing the idle pass, without adding
broader documentation.
In `@test/js/bun/http/bun-server.test.ts`:
- Around line 879-883: Shorten the three explanatory comment blocks near the
websocket test and the referenced sections to no more than three lines each.
Preserve only the essential test intent and behavior, including GC survival
while connected and collectability after stop where relevant; do not alter the
tests.
In `@test/js/bun/websocket/websocket-server.test.ts`:
- Around line 1745-1748: Update the serverCodes sorting in the assertion to use
an explicit numeric comparator instead of the default lexicographic sort, while
preserving the expected close-code values and pendingWebSockets check.
🪄 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: 9b509e63-4373-4b7e-b798-26cbdebf84f2
📒 Files selected for processing (8)
packages/bun-uws/src/App.hsrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/uws_sys/App.rssrc/uws_sys/libuwsockets.cpptest/js/bun/http/bun-server.test.tstest/js/bun/websocket/websocket-server.test.tstest/js/node/http/node-http-with-ws.test.ts
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 5, 2026
…#35130) ## Problem The `server.stop(false)` drain promise resolved while keep-alive HTTP connections were still open and still serving. ```js // bun stopcensus.mjs [idle|inflight] import net from "node:net"; const mode = process.argv[2] || "idle"; const server = Bun.serve({ port: 0, hostname: "127.0.0.1", async fetch(req) { const p = new URL(req.url).pathname; if (p === "/slow") await Bun.sleep(600); return new Response("resp:" + p + ";"); } }); const c = net.connect(server.port, "127.0.0.1"); // ... one GET, then await server.stop(false), then a second GET on the same socket ``` `idle` → `stopResolvedAfterMs: 0, connFinAt: null, servedAfterResolve: 1`; `inflight` → resolves at response-finish (~450 ms), connection open, `/second` served. Deterministic on 1.4.0. Separately, `server.stop(true)` after an earlier `server.stop(false)` was a silent no-op: `stop_from_js` only entered `stop()` while `has_listener()`, and a prior graceful stop had already taken the listener. ## Cause `deinit_if_we_can` (and the `get_all_closed_promise` early-return, and the `stop_listening` unref gate) tested `pending_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 - New `active_connection_count: Cell<u32>` on `NewServer`, fed by a uWS `filter` registered in `listen()` (fires `+1` on accept / post-TLS-handshake, `-1` from `HttpContext::onClose`). On WebSocket upgrade the socket is `us_socket_adopt`-ed out of the HTTP group and `HttpContext::onClose` never fires for it, so `note_websocket_opened` moves the count to the existing WebSocket tally. - The drain predicate, the `get_all_closed_promise` early-return and the `stop_listening` unref 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 repeat `stop()` 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 via `idleTimeout`, client disconnect, `server.closeIdleConnections()` or `server.stop(true)`. - `stop_from_js` / `dispose_from_js` enter `stop()` for an abrupt stop whenever the app has not yet been terminated, and `stop_listening` performs the `app.close()` teardown in that state, so `stop(true)` after `stop(false)` force-closes the surviving connections. ## Memory safety The deferred `js_value` downgrade is also a use-after-free fix. On `main`, once `pending_requests` hits 0 after a graceful `stop()`, `deinit_if_we_can` downgrades the wrapper to `Weak` while surviving keep-alive connections can still dispatch. The wrapper's slots are the only GC root of the configured handlers, and `JsRef::try_get()` returns the raw `JSValue` of a `Weak` ref with no liveness check, so after a GC pass a late request on such a connection calls swept cells: - release build: a freshly allocated object can reuse the swept handler cell and be invoked as the fetch handler. When the occupant is the fetch handler of another `Bun.serve` instance 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 as `error: Expected a Response object, but received '6'` (also `''` / `undefined`), response bodies resolving to unrelated objects, or a segfault - debug/ASAN build: UBSan `Structure.h: member call on null pointer of type 'JSC::ClassInfo'` in `Bun__JSValue__call`, reached from `NewServer::on_request` via `us_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 `Strong` until the last connection is gone, so a late dispatch always sees live cells. A standalone stress driver that creates fresh `Bun.serve` instances 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 connections` block in `test/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-before `resolvedEarly: true, resolvedWhileOpen: true`; after `false, false` and the promise resolves once the client destroys the socket. - `stop(true) after stop(false) force-closes the surviving connection`: fail-before `closed: false`; after `closed: true`. New `request on a connection surviving graceful stop() never reaches a collected handler` stress test: parks pooled keep-alive connections across `stop()`, drops the server binding, churns the heap and forces GC, then sends late requests on the surviving connections. Rounds alternate between a plain `fetch` handler and a `routes:` param-route server, because the route dispatch reads the wrapper's `ServerRouteList` cell, a second collected-cell site (UBSan member call on null `TrailingArray<...ServerRouteList::IdentifierRange>` in `paramsObjectForRoute`, reached from `on_user_route_request`). Fails consistently on `main`: 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 and `server.upgrade()` succeeds (previously it was refused because `handler.server` had been cleared). ``` bun bd test test/js/bun/http/bun-server.test.ts -t "drain promise counts open connections" # 3 pass USE_SYSTEM_BUN=1 bun test <same> # 3 fail ``` `bun-server.test.ts`, `serve.test.ts`, `node-http.test.ts` and `websocket-server.test.ts` are unchanged apart from the usual environment-only failures that also fail on `main`. `node:http`'s `server.close()` calls `closeIdleConnections()` 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. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
server.stop()is blind to open WebSockets:Observed on
main(release andrelease-asan, deterministic):stop(false)never resolves with a WebSocket connected; the socket keeps serving traffic.stop(true)afterstop(false)is a no-op (same listener gate as Bun.serve: make stop(true) force-close after a prior graceful stop #33662), so a "graceful then force" shutdown can never complete once a WebSocket is connected.stop(true)kills WebSockets by raw socket close: the serverclosecallback and the peer both see1006(abnormal, no close frame) instead of1001Going Away.Cause
stop_listeningon the graceful path only callslistener.close(). Open WebSockets are untouched, sodeinit_if_we_can(which gates on!has_active_web_sockets()) never resolves the returned promise.stop_listeningon the abrupt path callsapp.close(), which walks the WebSocket groups viaus_socket_group_close_alland callsus_socket_closeon each: raw fd close, no close frame, andWebSocketContext::onClosehard-codes1006for that path.stop_from_js/dispose_from_jsonly callstop()whenhas_listener(); a prior graceful stop has already taken the listener, so a laterstop(true)returns without touching the app.get_all_closed_promisefast-path returns a resolved promise when the listener is gone and there are no pending HTTP requests, ignoring open WebSockets.Fix
TemplatedApp::endAllWebSockets(code, message)(packages/bun-uws/src/App.h) which walks every WebSocket socket group and callsWebSocket::end(code, message)on each open socket: sends the close frame, fires the close handler, and FINs. Exposed to Rust viauws_app_end_all_websocketsandNewApp::end_all_websockets.stop_listeningnow callsend_all_websockets_going_away()(which sends1001 "Server closed") before closing the listener or terminating the app, under the existingdeinit_runningre-entrance guard so the synchronouson_closedefers do not dispatchdeinit_if_we_canwhile this frame still holds&mut self.stop_listeningalso terminates the app on an abrupt stop when the listener was already taken (via the newterminate_app()helper, guarded byTERMINATED), sostop(true)afterstop(false)still force-closes in-flight HTTP connections.stop_from_js/dispose_from_jsnow enterstop()in that state.get_all_closed_promisefast-path also checks!has_active_web_sockets().After:
Verification
New
server.stop() with open WebSocketsblock intest/js/bun/websocket/websocket-server.test.tscovers all three cases plus the "client already closing when stop(false) is called" edge.Full
websocket-server.test.tsis green (111 pass).serve.test.tshas the same environment-only failures asmain(IPv6, root-port, egress).This overlaps with #33662 (the
stop(true)-after-stop(false)gate) and extends it to the WebSocket path.no test proof · iteration 3 · 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 test/js/bun/websocket/websocket-server.test.ts
Closes #25722