Conversation
server.stop(false) takes the listener and closes the listen socket while leaving in-flight connections draining. A subsequent server.stop(true) then hit the has_listener() guard in stop_from_js and did nothing; even without the guard, stop_listening(true) early-returned because the listener was already gone. SSE/streaming responses kept flowing, the ReadableStream cancel() and req.signal never fired, and pendingRequests never drained, so the usual graceful-then-deadline shutdown pattern could not force anything closed. stop_from_js / dispose_from_js now also call stop() when the listener is gone but the app has not been terminated, and stop_listening closes the app on an abrupt stop even if the listener was taken by an earlier graceful stop.
|
Updated 12:05 PM PT - Jul 7th, 2026
❌ @robobun, your commit 831d8a6 has some failures in 🧪 To try this PR locally: bunx bun-pr 33662That installs a local version of the PR into your bun-33662 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 12 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 (1)
WalkthroughServer stop and dispose logic now trigger abrupt (forceful) teardown even after a prior graceful stop has already removed the listener, clearing websocket handler state and closing the app handle once. New tests verify pendingRequests reaches zero, stream cancel/request abort fire exactly once, and stop() promise resolution after force-close. ChangesForce-stop server after prior graceful stop
Related PRs: None identified. Suggested labels: Suggested reviewers: None identified. 🐰 A server stopped once, then stopped again, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/server/mod.rs (1)
1521-1539: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated abrupt-close-app logic into a helper.
The new no-listener abrupt branch (lines 1528-1536) duplicates the existing abrupt branch at the bottom of this function (lines 1564-1570) almost verbatim (clear
ws.handler.app, insertTERMINATED, closeself.app). Per repo convention, a block repeated a second time in the same diff should be extracted into a shared helper used at both sites — this also removes theself.app.unwrap()vs.if let Some(app)inconsistency between the two copies.♻️ Proposed refactor
+ fn abrupt_close_app(&mut self) { + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.app = None; + } + self.flags.insert(ServerFlags::TERMINATED); + if let Some(app) = self.app { + // S012: `NewApp<SSL>` is a ZST opaque — safe `*mut → &mut` deref. + bun_opaque::opaque_deref_mut(app).close(); + } + } + pub fn stop_listening(&mut self, abrupt: bool) { ... let Some(listener) = self.listener.take() else { if Self::HAS_H3 && self.h3_app.is_some() { self.unref(); self.notify_inspector_server_stopped(); } // A previous graceful stop already took the listener. An abrupt stop // still needs to close the app so in-flight connections are torn down. if abrupt && !self.flags.contains(ServerFlags::TERMINATED) { - if let Some(ws) = self.config.websocket.as_mut() { - ws.handler.app = None; - } - self.flags.insert(ServerFlags::TERMINATED); - if let Some(app) = self.app { - // S012: `NewApp<SSL>` is a ZST opaque — safe `*mut → &mut` deref. - bun_opaque::opaque_deref_mut(app).close(); - } + self.abrupt_close_app(); } return; }; ... if !abrupt { // S012: `app::ListenSocket<SSL>` is a ZST opaque — safe deref. bun_opaque::opaque_deref_mut(listener).close(); } else if !self.flags.contains(ServerFlags::TERMINATED) { - if let Some(ws) = self.config.websocket.as_mut() { - ws.handler.app = None; - } - self.flags.insert(ServerFlags::TERMINATED); - // S012: `NewApp<SSL>` is a ZST opaque — safe `*mut → &mut` deref. - bun_opaque::opaque_deref_mut(self.app.unwrap()).close(); + self.abrupt_close_app(); } }Based on learnings/coding guidelines: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site. If your fix makes two functions byte-identical, delete one."
🤖 Prompt for 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. In `@src/runtime/server/mod.rs` around lines 1521 - 1539, The abrupt-close cleanup for the no-listener path is duplicated with the existing abrupt branch in this function, so extract that shared logic into a helper and call it from both places. Move the websocket/app teardown and TERMINATED flag handling into a dedicated method on the server type (using the existing abrupt-stop code paths in Server::stop), then replace both branches with that helper so the behavior stays identical and the `if let Some(app)`/`unwrap` inconsistency is removed.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/server/mod.rs`:
- Around line 1521-1539: The abrupt-close cleanup for the no-listener path is
duplicated with the existing abrupt branch in this function, so extract that
shared logic into a helper and call it from both places. Move the websocket/app
teardown and TERMINATED flag handling into a dedicated method on the server type
(using the existing abrupt-stop code paths in Server::stop), then replace both
branches with that helper so the behavior stays identical and the `if let
Some(app)`/`unwrap` inconsistency is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33361958-b7d3-421d-a835-ddcbdde778ad
📒 Files selected for processing (3)
src/runtime/server/mod.rssrc/runtime/server/server_body.rstest/js/bun/http/serve.test.ts
Deduplicates the two stop_listening branches that clear the websocket handler, set TERMINATED and close the uws app, and replaces the self.app.unwrap() with an if-let so both call sites agree.
|
Re the duplicate flag for #30505: that PR reaches the same native fix from the Re the review: extracted the shared abrupt-close logic into CI status (build 69914): every test lane that ran passed, including the ASAN lane and |
|
Heads up: #34961 folds in the same |
…#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 -->
|
Closing as superseded. Main fixed
Main covers the path with the test "stop(true) after stop(false) force-closes the still-busy connection" in test/js/bun/http/bun-server.test.ts. I also ran the three tests from this PR against a build of main at 299bc9a: the SSE stream case (socket closes, The WebSocket side of |
1 participant
Problem
server.stop(true)is a silent no-op ifserver.stop(false)was called first. The standard graceful-shutdown-with-a-deadline pattern (stop(false)to stop accepting and drain, thenstop(true)when the deadline hits) could never force-close anything: SSE/streaming responses kept flowing,cancel()andreq.signalnever fired,pendingRequestsnever drained.Before:
After:
Cause
Two layers, both gated on the listener still being present:
stop_from_js(src/runtime/server/server_body.rs) wrapped the wholestop()call inif self.has_listener(). After a graceful stop,stop_listeninghas already doneself.listener.take(), so the second call returned the all-closed promise and did nothing else.dispose_from_js([Symbol.dispose]) had the same guard.stop_listening(src/runtime/server/mod.rs) itself early-returned whenself.listenerwasNone, so even reachingstop(true)would not close the app.Fix
stop_from_js/dispose_from_jsalso callstop()when the listener is gone but the app has not been terminated yet.stop_listeningcloses the uws app on an abrupt stop even if the listener was taken by an earlier graceful stop, guarded by!TERMINATEDso repeatedstop(true)stays idempotent.stop()'s other effects (js_value.downgrade(), hot-map removal,unref(),notify_inspector_server_stopped()) are all idempotent.Verification
New
server.stop(true) after a prior graceful stopblock intest/js/bun/http/serve.test.tsopens a raw TCP client against a streaming SSE response, callsstop(false)thenstop(true)(and separately[Symbol.dispose]()), and asserts the socket closes,cancel()and the abort signal fire,pendingRequestsreaches 0, and both the graceful and forcestop()promises resolve.Full
serve.test.tsandwebsocket-server.test.tshave the same failure set asmain(environment-only: IPv6, root-port, egress proxy, debug-build benchmark timeouts).