From 2564661c65f493d6c70dbebd4ba7135d665d1d72 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:28:00 +0000 Subject: [PATCH 1/6] Bun.serve: close WebSockets with 1001 on server.stop() and let stop(true) 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(). --- packages/bun-uws/src/App.h | 19 +++ src/runtime/server/mod.rs | 91 ++++++++---- src/runtime/server/server_body.rs | 20 +-- src/uws_sys/App.rs | 21 +++ src/uws_sys/libuwsockets.cpp | 15 ++ .../js/bun/websocket/websocket-server.test.ts | 136 ++++++++++++++++++ 6 files changed, 263 insertions(+), 39 deletions(-) diff --git a/packages/bun-uws/src/App.h b/packages/bun-uws/src/App.h index 2a2c5c6a5250..eda954951dea 100644 --- a/packages/bun-uws/src/App.h +++ b/packages/bun-uws/src/App.h @@ -385,6 +385,25 @@ struct TemplatedApp { return std::move(*this); } + /* Send a close frame to every open WebSocket and fire its close handler. + * Unlike close(), this performs the WebSocket closing handshake so peers + * observe the given status (e.g. 1001 Going Away) rather than 1006. */ + TemplatedApp &&endAllWebSockets(int code, std::string_view message = {}) { + for (us_socket_group_t *g : webSocketGroups) { + struct us_socket_t *s = g->head_sockets; + while (s) { + struct us_socket_t *next = s->next; + if (!us_socket_is_closed(s)) { + /* USERDATA is erased in the handler slots; see the + * TopicTree cast above. end() no-ops on isShuttingDown. */ + ((WebSocket *) s)->end(code, message); + } + s = next; + } + } + return std::move(*this); + } + /** Closes all connections connected to this server which are not sending a request or waiting for a response. Does not close the listen socket. */ TemplatedApp &&closeIdle() { auto *group = httpContext->getSocketGroup(); diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 0c1c04fcd33f..98d87726d499 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1628,22 +1628,22 @@ impl NewServer { } } - let Some(listener) = self.listener.take() else { + let listener = self.listener.take(); + if listener.is_none() { if Self::HAS_H3 && self.h3_app.is_some() { self.unref(); self.notify_inspector_server_stopped(); - if abrupt { - self.flags.insert(ServerFlags::TERMINATED); - } + } + // A prior graceful stop already took the listener. An abrupt stop + // still needs to tear down in-flight connections so a "graceful + // then force" shutdown can complete. + if abrupt { + self.end_all_websockets_going_away(); + self.terminate_app(); } return; - }; - if abrupt || (self.pending_requests == 0 && !self.has_active_web_sockets()) { - self.unref(); } - // A graceful stop with work in flight keeps the ref (deinit_if_we_can - // unrefs when the drain completes): on Windows uv_run skips I/O with - // zero ref'd handles, so unrefing here wedged server.close() teardown. + let listener = listener.unwrap(); if !SSL { // SAFETY: `listener` is a live uws ListenSocket FFI handle just taken @@ -1664,31 +1664,64 @@ impl NewServer { } } + // Send 1001 Going Away to every open WebSocket so peers see a clean + // protocol close instead of a dropped connection (1006), and so the + // graceful-stop promise can observe the count reaching zero. + self.end_all_websockets_going_away(); + + if abrupt || (self.pending_requests == 0 && !self.has_active_web_sockets()) { + self.unref(); + } + // A graceful stop with work in flight keeps the ref (deinit_if_we_can + // unrefs when the drain completes): on Windows uv_run skips I/O with + // zero ref'd handles, so unrefing here wedged server.close() teardown. + if !abrupt { // S012: `app::ListenSocket` 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); - // `app.close()` synchronously drains every open websocket; their - // `on_close` defers call `on_websocket_closed`, which would - // dispatch `deinit_if_we_can` through a fresh `&mut NewServer` - // while this frame still holds `&mut self`. Hold the re-entrance - // guard across the drain so that nested call early-returns — - // `stop()` runs `deinit_if_we_can` itself right after this returns. + } else { + self.terminate_app(); + } + } + + fn end_all_websockets_going_away(&mut self) { + if !self.has_active_web_sockets() { + return; + } + let Some(app) = self.app else { return }; + // `end()` fires the close handler synchronously; its `on_close` defer + // calls `on_websocket_closed`, which would dispatch `deinit_if_we_can` + // through a fresh `&mut NewServer` while this frame still holds + // `&mut self`. Hold the re-entrance guard across the drain so the + // nested call early-returns; `stop()` runs the idle pass afterwards. + self.deinit_running.set(true); + // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. + bun_opaque::opaque_deref_mut(app).end_all_websockets(1001, b"Server closed"); + self.deinit_running.set(false); + } + + /// Force-close every connection on the uws app and mark the server + /// terminated. Guarded by `TERMINATED` so repeated abrupt stops are no-ops. + fn terminate_app(&mut self) { + if self.flags.contains(ServerFlags::TERMINATED) { + return; + } + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.app = None; + } + self.flags.insert(ServerFlags::TERMINATED); + if let Some(app) = self.app { self.deinit_running.set(true); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. - bun_opaque::opaque_deref_mut(self.app.unwrap()).close(); + bun_opaque::opaque_deref_mut(app).close(); self.deinit_running.set(false); - // Only clear after the drain — `on_close` defers reach - // `on_websocket_closed` through `handler.server`, so wiping it - // earlier would strand the live-socket count and the idle pass - // would never see it drained. - if let Some(ws) = self.config.websocket.as_mut() { - ws.handler.server = None; - } + } + // Only clear after the drain — `on_close` defers reach + // `on_websocket_closed` through `handler.server`, so wiping it + // earlier would strand the live-socket count and the idle pass + // would never see it drained. + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.server = None; } } diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 90cfadc45540..b45532ca248a 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -2549,15 +2549,15 @@ where pub fn stop_from_js(&mut self, abruptly: Option) -> JSValue { let rc = self.get_all_closed_promise(&self.global()); - if self.has_listener() { - let abrupt = 'brk: { - if let Some(val) = abruptly { - if val.is_boolean() && val.to_boolean() { - break 'brk true; - } + let abrupt = 'brk: { + if let Some(val) = abruptly { + if val.is_boolean() && val.to_boolean() { + break 'brk true; } - false - }; + } + false + }; + if self.has_listener() || (abrupt && !self.flags.contains(ServerFlags::TERMINATED)) { self.stop(abrupt); } @@ -2565,7 +2565,7 @@ where } pub fn dispose_from_js(&mut self) -> JSValue { - if self.has_listener() { + if self.has_listener() || !self.flags.contains(ServerFlags::TERMINATED) { self.stop(true); } JSValue::UNDEFINED @@ -2742,7 +2742,7 @@ where } pub fn get_all_closed_promise(&mut self, global: &JSGlobalObject) -> JSValue { - if !self.has_listener() && self.pending_requests == 0 { + if !self.has_listener() && self.pending_requests == 0 && !self.has_active_web_sockets() { return JSPromise::resolved_promise(global, JSValue::UNDEFINED).to_js(); } if self.all_closed_promise.has_value() { diff --git a/src/uws_sys/App.rs b/src/uws_sys/App.rs index 6d3b03ca9f78..81f668b409f4 100644 --- a/src/uws_sys/App.rs +++ b/src/uws_sys/App.rs @@ -105,6 +105,20 @@ impl App { c::uws_app_close_idle(Self::SSL_FLAG, self.as_raw()) } + pub fn end_all_websockets(&mut self, code: i32, message: &[u8]) { + // SAFETY: `message` is only read for the synchronous duration of the + // call; uWS copies it into the close frame before returning. + unsafe { + c::uws_app_end_all_websockets( + Self::SSL_FLAG, + self.as_raw(), + code, + message.as_ptr(), + message.len(), + ) + } + } + pub fn create(opts: &BunSocketContextOptions) -> Option<*mut Self> { // SAFETY: FFI call; uws_create_app returns null on failure. let app = unsafe { c::uws_create_app(Self::SSL_FLAG, *opts) }; @@ -513,6 +527,13 @@ pub mod c { unsafe extern "C" { pub(crate) safe fn uws_app_close(ssl: i32, app: &mut uws_app_s); pub(crate) safe fn uws_app_close_idle(ssl: i32, app: &mut uws_app_s); + pub(crate) fn uws_app_end_all_websockets( + ssl: i32, + app: &mut uws_app_s, + code: c_int, + message: *const u8, + length: usize, + ); // safe: `&mut uws_app_s` is ABI-identical to a non-null `*mut`; // `handler`/`user_data` are stored opaquely (never dereferenced by the // C++ shim itself) — no preconditions on this call. diff --git a/src/uws_sys/libuwsockets.cpp b/src/uws_sys/libuwsockets.cpp index e0525a110310..54225fa599b3 100644 --- a/src/uws_sys/libuwsockets.cpp +++ b/src/uws_sys/libuwsockets.cpp @@ -401,6 +401,21 @@ extern "C" } } + void uws_app_end_all_websockets(int ssl, uws_app_t *app, int code, const char *message, size_t length) + { + std::string_view msg = message ? std::string_view(message, length) : std::string_view(); + if (ssl) + { + uWS::SSLApp *uwsApp = (uWS::SSLApp *)app; + uwsApp->endAllWebSockets(code, msg); + } + else + { + uWS::App *uwsApp = (uWS::App *)app; + uwsApp->endAllWebSockets(code, msg); + } + } + void uws_app_set_on_clienterror(int ssl, uws_app_t *app, void (*handler)(void *user_data, int is_ssl, struct us_socket_t *rawSocket, uint8_t errorCode, char *rawPacket, int rawPacketLength), void *user_data) { if (ssl) diff --git a/test/js/bun/websocket/websocket-server.test.ts b/test/js/bun/websocket/websocket-server.test.ts index 0e5fe71fb919..b31bf4944e34 100644 --- a/test/js/bun/websocket/websocket-server.test.ts +++ b/test/js/bun/websocket/websocket-server.test.ts @@ -1562,3 +1562,139 @@ it.each(["server", "client"] as const)( await server.stop(); }, ); + +describe("server.stop() with open WebSockets", () => { + async function openServerWithWS() { + const serverClose = Promise.withResolvers<{ code: number; reason: string }>(); + const srv = serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("http"); + }, + websocket: { + message(ws, m) { + ws.send("echo:" + m); + }, + close(_ws, code, reason) { + serverClose.resolve({ code, reason }); + }, + }, + }); + const ws = new WebSocket(`ws://127.0.0.1:${srv.port}/`); + const opened = Promise.withResolvers(); + const clientClose = Promise.withResolvers(); + ws.onopen = () => opened.resolve(); + ws.onerror = () => {}; + ws.onclose = e => clientClose.resolve(e); + await opened.promise; + return { srv, ws, serverClose: serverClose.promise, clientClose: clientClose.promise }; + } + + it("stop(false) sends 1001 to open WebSockets and resolves", async () => { + const { srv, ws, serverClose, clientClose } = await openServerWithWS(); + try { + expect(srv.pendingWebSockets).toBe(1); + await srv.stop(false); + const [s, c] = await Promise.all([serverClose, clientClose]); + expect({ + serverCode: s.code, + clientCode: c.code, + wasClean: c.wasClean, + readyState: ws.readyState, + pendingWebSockets: srv.pendingWebSockets, + }).toEqual({ + serverCode: 1001, + clientCode: 1001, + wasClean: true, + readyState: WebSocket.CLOSED, + pendingWebSockets: 0, + }); + } finally { + ws.close(); + srv.stop(true); + } + }); + + it("stop(true) sends 1001 to open WebSockets", async () => { + const { srv, ws, serverClose, clientClose } = await openServerWithWS(); + try { + await srv.stop(true); + const [s, c] = await Promise.all([serverClose, clientClose]); + expect({ + serverCode: s.code, + clientCode: c.code, + wasClean: c.wasClean, + pendingWebSockets: srv.pendingWebSockets, + }).toEqual({ + serverCode: 1001, + clientCode: 1001, + wasClean: true, + pendingWebSockets: 0, + }); + } finally { + ws.close(); + srv.stop(true); + } + }); + + it("stop(true) after stop(false) force-closes remaining connections", async () => { + // Use a pending HTTP request so graceful stop has something to wait for + // even after the WebSocket is gone. + const httpDone = Promise.withResolvers(); + const srv = serve({ + port: 0, + idleTimeout: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response( + new ReadableStream({ + pull() { + return httpDone.promise; + }, + }), + ); + }, + websocket: { message() {}, close() {} }, + }); + const sock = net.connect(srv.port, "127.0.0.1", () => { + sock.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n"); + }); + const gotData = Promise.withResolvers(); + const sockClosed = Promise.withResolvers(); + sock.once("data", () => gotData.resolve()); + sock.on("error", () => {}); + sock.on("close", () => sockClosed.resolve()); + const ws = new WebSocket(`ws://127.0.0.1:${srv.port}/`); + const wsClosed = Promise.withResolvers(); + ws.onopen = () => {}; + ws.onerror = () => {}; + ws.onclose = e => wsClosed.resolve(e); + await Promise.all([gotData.promise, new Promise(r => (ws.onopen = r))]); + try { + const graceful = srv.stop(false); + expect((await wsClosed.promise).code).toBe(1001); + expect(srv.pendingRequests).toBe(1); + + const force = srv.stop(true); + await Promise.all([graceful, force, sockClosed.promise]); + expect(srv.pendingRequests).toBe(0); + } finally { + httpDone.resolve(); + sock.destroy(); + ws.close(); + srv.stop(true); + } + }); + + it("stop(false) is not kept alive by an already-closing WebSocket", async () => { + const { srv, ws } = await openServerWithWS(); + try { + ws.close(1000, "bye"); + await srv.stop(false); + expect(srv.pendingWebSockets).toBe(0); + } finally { + srv.stop(true); + } + }); +}); From 411c2577a0da028f4118e77c53df5e80fe04a2d1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:49:08 +0000 Subject: [PATCH 2/6] test: update bun-server GC-tracing tests for stop() closing websockets 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. --- test/js/bun/http/bun-server.test.ts | 91 +++++++++++++++-------------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index 32a8b92f381c..4602ccf6ae53 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -875,15 +875,12 @@ test("late keep-alive request to a node:http server after close() dispatches whi ); }); -test("server wrapper survives GC while a websocket is connected after stop()", async () => { +test("server wrapper survives GC while a websocket is connected and is collected after stop()", async () => { // The previous test exercises the one-tick HTTP keep-alive race; this one - // covers the steadier websocket case. After a graceful stop() with a live - // websocket, the user may drop their `server` binding. The native struct - // stays alive (active_websockets > 0), but stop() previously downgraded - // js_value immediately, so GC could finalize the JS wrapper — and with it - // m_routeList — while the connection was still in use. With the downgrade - // deferred into deinit_if_we_can's idle predicate, the wrapper must outlive - // the websocket and become collectable only after the last close. + // covers the websocket case. While a websocket is connected (listener still + // open) the wrapper must survive GC even with no user-held `server` + // binding; once stop() runs (which now also ends open websockets with 1001) + // deinit_if_we_can downgrades js_value and the wrapper becomes collectable. await using proc = Bun.spawn({ cmd: [ bunExe(), @@ -917,6 +914,8 @@ test("server wrapper survives GC while a websocket is connected after stop()", a await drain(0); const baseline = serverCount(); + let serverRef; + const closed = Promise.withResolvers(); const ws = await (async () => { const server = Bun.serve({ port: 0, @@ -928,19 +927,18 @@ test("server wrapper survives GC while a websocket is connected after stop()", a }, websocket: { open() {}, message() {}, close() {} }, }); + serverRef = new WeakRef(server); const opened = Promise.withResolvers(); const ws = new WebSocket("ws://127.0.0.1:" + server.port); ws.onopen = () => opened.resolve(); ws.onerror = e => opened.reject(e); + ws.onclose = () => closed.resolve(); await opened.promise; - - // Graceful stop: listener closes, the live websocket stays open. - server.stop(); return ws; })(); - // The only \`server\` binding is now out of scope; only the live - // websocket keeps the native side around. + // The only strong \`server\` binding is now out of scope; js_value + // stays Strong while the listener/socket are live. for (let i = 0; i < 30; i++) { Bun.gc(true); @@ -948,17 +946,17 @@ test("server wrapper survives GC while a websocket is connected after stop()", a await new Promise(r => setImmediate(r)); await Bun.sleep(10); } - const afterStopGC = serverCount(); + const whileConnected = serverCount(); - const closed = Promise.withResolvers(); - ws.onclose = () => closed.resolve(); - ws.close(); + // Graceful stop now also ends open websockets with 1001. + await serverRef.deref().stop(); + serverRef = null; await closed.promise; await drain(baseline); - const afterCloseGC = serverCount(); + const afterStop = serverCount(); - console.log(JSON.stringify({ baseline, afterStopGC, afterCloseGC })); + console.log(JSON.stringify({ baseline, whileConnected, afterStop, closeCode: ws.readyState })); process.exit(0); `, ], @@ -968,15 +966,13 @@ test("server wrapper survives GC while a websocket is connected after stop()", a }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - const { baseline, afterStopGC, afterCloseGC } = JSON.parse(stdout.trim() || "{}"); + const { baseline, whileConnected, afterStop } = JSON.parse(stdout.trim() || "{}"); expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); - // js_value stays Strong while a websocket is connected → GC must not - // collect the wrapper. baseline already includes the prototype(s), so the - // live instance shows as baseline+1. - expect(afterStopGC).toBeGreaterThan(baseline); - // Last websocket closing triggers deinit_if_we_can → downgrade → wrapper - // becomes collectable again (no leak). - expect(afterCloseGC).toBe(baseline); + // baseline already includes the prototype(s), so the live instance shows as + // baseline+1 while connected. + expect(whileConnected).toBeGreaterThan(baseline); + // stop() ended the websocket and downgraded the wrapper: no leak. + expect(afterStop).toBe(baseline); }, 15_000); test("should be able to async upgrade using custom protocol", async () => { @@ -2194,11 +2190,13 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { const echoed = Promise.withResolvers(); const closed = Promise.withResolvers(); - // Scope server so the only post-stop root is the connected websocket. - // Assign client directly to the outer var rather than returning it — - // returning keeps the async frame's scope (which contains server) - // alive via the resolved-value chain in JSC. + // Scope server so the only strong root while the ws is open is the + // native side (listener + ServerWebSocket). Assign client directly to + // the outer var rather than returning it — returning keeps the async + // frame's scope (which contains server) alive via the resolved-value + // chain in JSC. let client; + let serverRef; await (async () => { const server = Bun.serve({ port: 0, @@ -2210,18 +2208,17 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { message(ws, m) { ws.send(server.port + ":" + m); }, }, }); + serverRef = new WeakRef(server); client = new WebSocket(server.url.href.replace("http", "ws")); client.onopen = () => clientOpen.resolve(); client.onmessage = e => echoed.resolve(e.data); client.onclose = () => closed.resolve(); await opened.promise; // server-side ws created (roots wrapper) await clientOpen.promise; // client ready to send (avoid InvalidStateError) - server.stop(); // graceful — listener gone, ws stays })(); - // server out of scope. Wrapper is rooted only via: - // ServerWebSocket(this_value strong) → JSServerWebSocket → m_server → JSServer - // GC must NOT collect while the ws is open. + // server out of scope. Wrapper is rooted via js_value (Strong while + // listener/socket live) and the wsHandlers cycle. GC must NOT collect. Bun.gc(true); fullGC(); const whileConnected = liveServer(); @@ -2229,12 +2226,12 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { client.send("hi"); const echo = await echoed.promise; - client.close(); + // Graceful stop now also ends open websockets with 1001. + await serverRef.deref().stop(); + serverRef = null; await closed.promise; client = null; - // The last ws closing triggers on_websocket_closed → deinit_if_we_can, - // which downgrades the wrapper without an explicit stop(true) — that's - // the path under test, so no force-finish here. + // stop() drained the last ws and ran deinit_if_we_can → downgrade. const afterClose = await gcUntilCountAtMost(baseline); console.log(JSON.stringify({ baseline, whileConnected, echo, afterClose })); @@ -2594,10 +2591,12 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { const opened = Promise.withResolvers(); const closed = Promise.withResolvers(); let ws; - // Scope server so the module-level frame holds no reference to the - // wrapper when message(ws) runs; after ws.close() downgrades js_value - // and clears m_server, the wrapper must have zero roots for Bun.gc to - // reach wsOnError. + let serverRef; + // Scope server so the module-level frame holds no strong reference to + // the wrapper when message(ws) runs; after ws.close() + stop() + // downgrade js_value, the wrapper must have zero roots for Bun.gc to + // reach wsOnError. A WeakRef lets the handler call stop() without + // itself rooting the wrapper. await (async () => { const server = Bun.serve({ port: 0, hostname: "127.0.0.1", @@ -2605,19 +2604,21 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { websocket: { open() {}, message(ws) { - ws.close(); // last socket of a stopped server → wrapper downgrades + ws.close(); // last socket → count=0 + serverRef.deref()?.stop(); // listener gone → wrapper downgrades + serverRef = null; Bun.gc(true); throw new Error("boom"); }, error(e) { errorFired++; }, }, }); + serverRef = new WeakRef(server); ws = new WebSocket("ws://127.0.0.1:" + server.port); ws.onopen = () => opened.resolve(); ws.onerror = e => opened.reject(e); ws.onclose = () => closed.resolve(); await opened.promise; - server.stop(); // graceful: listener gone, this ws keeps wrapper Strong })(); ws.send("go"); await closed.promise; From 4a078a0f0755501b97a032843dd592f5cfcfe52b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:56:01 +0000 Subject: [PATCH 3/6] review: snapshot sockets in endAllWebSockets; save/restore deinit_running 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. --- packages/bun-uws/src/App.h | 22 ++++--- src/runtime/server/mod.rs | 10 +-- src/runtime/server/server_body.rs | 13 +++- .../js/bun/websocket/websocket-server.test.ts | 64 +++++++++++++++++-- 4 files changed, 89 insertions(+), 20 deletions(-) diff --git a/packages/bun-uws/src/App.h b/packages/bun-uws/src/App.h index eda954951dea..52d42b42205e 100644 --- a/packages/bun-uws/src/App.h +++ b/packages/bun-uws/src/App.h @@ -389,16 +389,20 @@ struct TemplatedApp { * Unlike close(), this performs the WebSocket closing handshake so peers * observe the given status (e.g. 1001 Going Away) rather than 1006. */ TemplatedApp &&endAllWebSockets(int code, std::string_view message = {}) { + /* end() fires the close handler synchronously; user JS there can + * terminate() a later socket, which rewrites its ->next into the + * loop's closed_head and would derail an in-place walk. Snapshot. */ + std::vector sockets; for (us_socket_group_t *g : webSocketGroups) { - struct us_socket_t *s = g->head_sockets; - while (s) { - struct us_socket_t *next = s->next; - if (!us_socket_is_closed(s)) { - /* USERDATA is erased in the handler slots; see the - * TopicTree cast above. end() no-ops on isShuttingDown. */ - ((WebSocket *) s)->end(code, message); - } - s = next; + for (struct us_socket_t *s = g->head_sockets; s; s = s->next) { + sockets.push_back(s); + } + } + for (us_socket_t *s : sockets) { + if (!us_socket_is_closed(s)) { + /* USERDATA is erased in the handler slots; see the TopicTree + * cast above. end() no-ops on isShuttingDown. */ + ((WebSocket *) s)->end(code, message); } } return std::move(*this); diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 98d87726d499..b028d9a2e5ab 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1694,10 +1694,12 @@ impl NewServer { // through a fresh `&mut NewServer` while this frame still holds // `&mut self`. Hold the re-entrance guard across the drain so the // nested call early-returns; `stop()` runs the idle pass afterwards. - self.deinit_running.set(true); + // Save/restore so a nested `server.stop(true)` from a close handler + // cannot clear the outer frame's guard. + let prev = self.deinit_running.replace(true); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. bun_opaque::opaque_deref_mut(app).end_all_websockets(1001, b"Server closed"); - self.deinit_running.set(false); + self.deinit_running.set(prev); } /// Force-close every connection on the uws app and mark the server @@ -1711,10 +1713,10 @@ impl NewServer { } self.flags.insert(ServerFlags::TERMINATED); if let Some(app) = self.app { - self.deinit_running.set(true); + let prev = self.deinit_running.replace(true); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. bun_opaque::opaque_deref_mut(app).close(); - self.deinit_running.set(false); + self.deinit_running.set(prev); } // Only clear after the drain — `on_close` defers reach // `on_websocket_closed` through `handler.server`, so wiping it diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index b45532ca248a..d73983abaf4a 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -2557,7 +2557,14 @@ where } false }; - if self.has_listener() || (abrupt && !self.flags.contains(ServerFlags::TERMINATED)) { + // `!deinit_running`: a `server.stop()` from inside a websocket close + // handler fired by an outer `stop()`'s drain would re-enter + // `stop_listening` with a fresh `&mut self` under the outer borrow. + if self.has_listener() + || (abrupt + && !self.flags.contains(ServerFlags::TERMINATED) + && !self.deinit_running.get()) + { self.stop(abrupt); } @@ -2565,7 +2572,9 @@ where } pub fn dispose_from_js(&mut self) -> JSValue { - if self.has_listener() || !self.flags.contains(ServerFlags::TERMINATED) { + if self.has_listener() + || (!self.flags.contains(ServerFlags::TERMINATED) && !self.deinit_running.get()) + { self.stop(true); } JSValue::UNDEFINED diff --git a/test/js/bun/websocket/websocket-server.test.ts b/test/js/bun/websocket/websocket-server.test.ts index b31bf4944e34..96e73bba3e58 100644 --- a/test/js/bun/websocket/websocket-server.test.ts +++ b/test/js/bun/websocket/websocket-server.test.ts @@ -1585,7 +1585,7 @@ describe("server.stop() with open WebSockets", () => { const opened = Promise.withResolvers(); const clientClose = Promise.withResolvers(); ws.onopen = () => opened.resolve(); - ws.onerror = () => {}; + ws.onerror = e => opened.reject(e); ws.onclose = e => clientClose.resolve(e); await opened.promise; return { srv, ws, serverClose: serverClose.promise, clientClose: clientClose.promise }; @@ -1663,14 +1663,15 @@ describe("server.stop() with open WebSockets", () => { const gotData = Promise.withResolvers(); const sockClosed = Promise.withResolvers(); sock.once("data", () => gotData.resolve()); - sock.on("error", () => {}); + sock.on("error", e => gotData.reject(e)); sock.on("close", () => sockClosed.resolve()); const ws = new WebSocket(`ws://127.0.0.1:${srv.port}/`); + const wsOpened = Promise.withResolvers(); const wsClosed = Promise.withResolvers(); - ws.onopen = () => {}; - ws.onerror = () => {}; + ws.onopen = () => wsOpened.resolve(); + ws.onerror = e => wsOpened.reject(e); ws.onclose = e => wsClosed.resolve(e); - await Promise.all([gotData.promise, new Promise(r => (ws.onopen = r))]); + await Promise.all([gotData.promise, wsOpened.promise]); try { const graceful = srv.stop(false); expect((await wsClosed.promise).code).toBe(1001); @@ -1697,4 +1698,57 @@ describe("server.stop() with open WebSockets", () => { srv.stop(true); } }); + + it("stop(false) reaches every WebSocket when a close handler terminates another", async () => { + const sockets: ServerWebSocket[] = []; + const serverCodes: number[] = []; + const srv = serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("http"); + }, + websocket: { + open(ws) { + sockets.push(ws); + }, + message() {}, + close(ws, code) { + serverCodes.push(code); + // From the first close handler, terminate every other open socket and + // re-enter stop(true) so the drain has to survive list mutation and + // nested re-entry. + if (serverCodes.length === 1) { + for (const other of sockets) if (other !== ws) other.terminate(); + srv.stop(true); + } + }, + }, + }); + const clients: WebSocket[] = []; + const clientCloses: Promise[] = []; + try { + for (let i = 0; i < 4; i++) { + const opened = Promise.withResolvers(); + const closed = Promise.withResolvers(); + const c = new WebSocket(`ws://127.0.0.1:${srv.port}/`); + c.onopen = () => opened.resolve(); + c.onerror = e => opened.reject(e); + c.onclose = () => closed.resolve(); + clients.push(c); + clientCloses.push(closed.promise); + await opened.promise; + } + expect(srv.pendingWebSockets).toBe(4); + await srv.stop(false); + await Promise.all(clientCloses); + expect({ pendingWebSockets: srv.pendingWebSockets, serverCodes: serverCodes.sort() }).toEqual({ + pendingWebSockets: 0, + serverCodes: [1001, 1006, 1006, 1006], + }); + } finally { + for (const c of clients) c.close(); + srv.stop(true); + } + }); }); From aa8a42071a183e7fdf65af87d8407a8e4a03b288 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 16:29:43 +0000 Subject: [PATCH 4/6] review: exempt node:http servers from the graceful-stop websocket 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. --- src/runtime/server/mod.rs | 12 ++++--- test/js/bun/http/bun-server.test.ts | 2 +- test/js/node/http/node-http-with-ws.test.ts | 39 +++++++++++++++++++++ 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index b028d9a2e5ab..ba5c6626b125 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1628,8 +1628,7 @@ impl NewServer { } } - let listener = self.listener.take(); - if listener.is_none() { + let Some(listener) = self.listener.take() else { if Self::HAS_H3 && self.h3_app.is_some() { self.unref(); self.notify_inspector_server_stopped(); @@ -1642,8 +1641,7 @@ impl NewServer { self.terminate_app(); } return; - } - let listener = listener.unwrap(); + }; if !SSL { // SAFETY: `listener` is a live uws ListenSocket FFI handle just taken @@ -1688,6 +1686,12 @@ impl NewServer { if !self.has_active_web_sockets() { return; } + // node:http `Server#close()` must leave upgraded sockets to the user + // (Node only stops accepting and closes idle keep-alives); the `ws` + // shim tracks its own `clients` set for the user to drain. + if !self.config.on_node_http_request.is_empty() { + return; + } let Some(app) = self.app else { return }; // `end()` fires the close handler synchronously; its `on_close` defer // calls `on_websocket_closed`, which would dispatch `deinit_if_we_can` diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index 4602ccf6ae53..e87177871947 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -956,7 +956,7 @@ test("server wrapper survives GC while a websocket is connected and is collected await drain(baseline); const afterStop = serverCount(); - console.log(JSON.stringify({ baseline, whileConnected, afterStop, closeCode: ws.readyState })); + console.log(JSON.stringify({ baseline, whileConnected, afterStop })); process.exit(0); `, ], diff --git a/test/js/node/http/node-http-with-ws.test.ts b/test/js/node/http/node-http-with-ws.test.ts index a3ef8cac6a29..9f713f36e96a 100644 --- a/test/js/node/http/node-http-with-ws.test.ts +++ b/test/js/node/http/node-http-with-ws.test.ts @@ -1,6 +1,8 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, tls as options } from "harness"; +import http from "http"; import https from "https"; +import { once } from "node:events"; import type { AddressInfo } from "node:net"; import tls from "tls"; import { WebSocketServer } from "ws"; @@ -103,3 +105,40 @@ test.concurrent("should not crash when closing sockets after upgrade", async () await promise; expect().pass(); }); + +test.concurrent("http.Server.close() does not close open WebSocket connections", async () => { + const server = http.createServer(); + const wss = new WebSocketServer({ server }); + const echoed = Promise.withResolvers(); + wss.on("connection", ws => { + ws.on("message", m => ws.send("echo:" + m)); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const port = (server.address() as AddressInfo).port; + + const client = new WebSocket(`ws://127.0.0.1:${port}/`); + const opened = Promise.withResolvers(); + const closed = Promise.withResolvers(); + client.onopen = () => opened.resolve(); + client.onerror = e => opened.reject(e); + client.onmessage = e => echoed.resolve(String(e.data)); + client.onclose = e => closed.resolve(e); + await opened.promise; + try { + // Node's server.close() stops accepting but leaves upgraded sockets to + // the user; the ws connection must stay open. + server.close(); + client.send("hi"); + expect(await echoed.promise).toBe("echo:hi"); + expect(client.readyState).toBe(WebSocket.OPEN); + + // User-chosen code reaches the peer. + for (const c of wss.clients) c.close(4001, "draining"); + const ev = await closed.promise; + expect({ code: ev.code, reason: ev.reason }).toEqual({ code: 4001, reason: "draining" }); + } finally { + for (const c of wss.clients) c.terminate(); + client.close(); + server.closeAllConnections(); + } +}); From c9ba870e5ea374442fa7b4d92a5c7ab52fc5428f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 17:27:32 +0000 Subject: [PATCH 5/6] ci: retrigger From fae66be168f4015b9a70956d26275b7c4584f1d0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 21 Jul 2026 17:41:53 +0000 Subject: [PATCH 6/6] review: trim comment blocks to <=3 lines; numeric sort comparator --- src/runtime/server/mod.rs | 17 +++++--------- test/js/bun/http/bun-server.test.ts | 23 +++++++------------ .../js/bun/websocket/websocket-server.test.ts | 2 +- 3 files changed, 15 insertions(+), 27 deletions(-) diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index ba5c6626b125..f377a3f481f7 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1693,13 +1693,9 @@ impl NewServer { return; } let Some(app) = self.app else { return }; - // `end()` fires the close handler synchronously; its `on_close` defer - // calls `on_websocket_closed`, which would dispatch `deinit_if_we_can` - // through a fresh `&mut NewServer` while this frame still holds - // `&mut self`. Hold the re-entrance guard across the drain so the - // nested call early-returns; `stop()` runs the idle pass afterwards. - // Save/restore so a nested `server.stop(true)` from a close handler - // cannot clear the outer frame's guard. + // `end()` fires close handlers synchronously; hold (and save/restore) + // the re-entrance guard so nested `deinit_if_we_can` / `stop(true)` + // early-return instead of running under this frame's `&mut self`. let prev = self.deinit_running.replace(true); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. bun_opaque::opaque_deref_mut(app).end_all_websockets(1001, b"Server closed"); @@ -1722,10 +1718,9 @@ impl NewServer { bun_opaque::opaque_deref_mut(app).close(); self.deinit_running.set(prev); } - // Only clear after the drain — `on_close` defers reach - // `on_websocket_closed` through `handler.server`, so wiping it - // earlier would strand the live-socket count and the idle pass - // would never see it drained. + // Clear only after the drain: `on_close` defers reach + // `on_websocket_closed` via `handler.server`, so wiping it earlier + // would strand the live-socket count. if let Some(ws) = self.config.websocket.as_mut() { ws.handler.server = None; } diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index e87177871947..7c85d2546385 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -876,11 +876,9 @@ test("late keep-alive request to a node:http server after close() dispatches whi }); test("server wrapper survives GC while a websocket is connected and is collected after stop()", async () => { - // The previous test exercises the one-tick HTTP keep-alive race; this one - // covers the websocket case. While a websocket is connected (listener still - // open) the wrapper must survive GC even with no user-held `server` - // binding; once stop() runs (which now also ends open websockets with 1001) - // deinit_if_we_can downgrades js_value and the wrapper becomes collectable. + // Wrapper must survive GC while a ws is connected with no user-held + // `server` binding; stop() ends the ws and downgrades js_value so the + // wrapper becomes collectable. await using proc = Bun.spawn({ cmd: [ bunExe(), @@ -2190,11 +2188,9 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { const echoed = Promise.withResolvers(); const closed = Promise.withResolvers(); - // Scope server so the only strong root while the ws is open is the - // native side (listener + ServerWebSocket). Assign client directly to - // the outer var rather than returning it — returning keeps the async - // frame's scope (which contains server) alive via the resolved-value - // chain in JSC. + // Scope server so only the native side roots it while the ws is open. + // Assign client to the outer var (returning it would keep the async + // frame's scope, which contains server, alive via the resolved value). let client; let serverRef; await (async () => { @@ -2592,11 +2588,8 @@ describe("handler GC tracing (heapStats wrapper-count)", () => { const closed = Promise.withResolvers(); let ws; let serverRef; - // Scope server so the module-level frame holds no strong reference to - // the wrapper when message(ws) runs; after ws.close() + stop() - // downgrade js_value, the wrapper must have zero roots for Bun.gc to - // reach wsOnError. A WeakRef lets the handler call stop() without - // itself rooting the wrapper. + // Scope server so nothing roots the wrapper when message(ws) runs; + // a WeakRef lets the handler call stop() without itself rooting it. await (async () => { const server = Bun.serve({ port: 0, hostname: "127.0.0.1", diff --git a/test/js/bun/websocket/websocket-server.test.ts b/test/js/bun/websocket/websocket-server.test.ts index 96e73bba3e58..c703f81fc044 100644 --- a/test/js/bun/websocket/websocket-server.test.ts +++ b/test/js/bun/websocket/websocket-server.test.ts @@ -1742,7 +1742,7 @@ describe("server.stop() with open WebSockets", () => { expect(srv.pendingWebSockets).toBe(4); await srv.stop(false); await Promise.all(clientCloses); - expect({ pendingWebSockets: srv.pendingWebSockets, serverCodes: serverCodes.sort() }).toEqual({ + expect({ pendingWebSockets: srv.pendingWebSockets, serverCodes: serverCodes.sort((a, b) => a - b) }).toEqual({ pendingWebSockets: 0, serverCodes: [1001, 1006, 1006, 1006], });