From 7f65d179fc2c71b7d329ad6736947355b2c82be1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 18 Jun 2026 14:48:24 +0000 Subject: [PATCH 1/4] fix(http): preserve server reference across close() for closeAllConnections() Calling server.close() followed by server.closeAllConnections() is the idiomatic way to kill both idle and in-flight HTTP connections. It's what @azure/msal-node's LoopbackClient does at the end of an interactive token flow. Bun's close() nulled out the internal Bun.serve reference before the follow-up calls could reach it, so closeAllConnections() early-returned and any in-flight keep-alive socket (the browser tab in the msal flow) kept the event loop alive indefinitely. Changes: - _http_server.ts: keep this[serverSymbol] alive past close() and clear it only after the allClosed promise fulfills; track 'has close been called' with a new kServerClosed flag so address() still returns null post-close, and reset the flag on re-listen. emitCloseNTServer captures the Bun.serve handle for its listen generation so a racing re-listen doesn't null out the new handle. - server_body.rs / mod.rs: let stop_from_js(true)/dispose_from_js proceed when the app is still alive even after a graceful stop took the listener; and in stop_listening, run app.close() on the abrupt path even when the listener was already taken (runs before the h3 branch so the TERMINATED flag doesn't short-circuit it for h3 servers). Fixes #30501 --- src/js/node/_http_server.ts | 58 ++++++++- src/runtime/server/mod.rs | 22 ++++ src/runtime/server/server_body.rs | 32 +++-- .../node-http-close-all-connections.test.ts | 120 ++++++++++++++++++ 4 files changed, 215 insertions(+), 17 deletions(-) create mode 100644 test/js/node/http/node-http-close-all-connections.test.ts diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 8e515ee41c0e..62068c9da77a 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -75,6 +75,11 @@ const OutgoingMessagePrototype = OutgoingMessage.prototype; const { kIncomingMessage } = require("node:_http_common"); const kConnectionsCheckingInterval = Symbol("http.server.connectionsCheckingInterval"); const kTrackedConnections = Symbol("http.server.trackedConnections"); +// Set synchronously in close(); tracks whether the server has entered +// the shutdown sequence, so address()/close() behave like Node even +// while the underlying Bun.serve reference is still populated for +// closeAllConnections()/unref() to reach. +const kServerClosed = Symbol("http.server.closed"); const getBunServerAllClosedPromise = $newZigFunction("node_http_binding.zig", "getBunServerAllClosedPromise", 1); const sendHelper = $newZigFunction("node_cluster_binding.zig", "sendHelperChild", 3); @@ -95,7 +100,22 @@ function emitCloseServer(self: Server) { callCloseCallback(self); self.emit("close"); } -function emitCloseNTServer(this: Server) { +function emitCloseNTServer(this: Server, closingServer) { + // The underlying Bun server has finished shutting down — drop our + // reference so the allClosed promise chain can settle and the event + // loop can exit. msal and other consumers call + // `close() → closeAllConnections() → unref()` in sequence; the + // underlying reference has to survive past `close()` so the follow-up + // calls can still reach the native layer. + // + // Guard against re-listen: if the caller already started a fresh + // `listen()` before this callback fired, `this[serverSymbol]` now + // points at a NEW Bun.serve handle — nulling it would trash the new + // listener. Only clear when the handle still matches the one whose + // allClosed promise we're resolving. + if (this[serverSymbol] === closingServer) { + this[serverSymbol] = undefined; + } process.nextTick(emitCloseServer, this); } @@ -351,10 +371,15 @@ Server.prototype.closeAllConnections = function () { if (!server) { return; } - this[serverSymbol] = undefined; + this[kServerClosed] = true; clearInterval(this[kConnectionsCheckingInterval]); this.listening = false; + // Abrupt stop — force-closes any open connections, including the ones + // a graceful `close()` would have left open. We keep `this[serverSymbol]` + // populated so subsequent calls (e.g. `unref()`) still reach the native + // server; it's cleared in `emitCloseNTServer` once everything actually + // drains. server.stop(true); }; @@ -378,13 +403,18 @@ Server.prototype.close = function (optionalCallback?) { // Node.js's httpServerPreClose clears the connections-checking interval // even when the server was never listening. clearInterval(this[kConnectionsCheckingInterval]); - if (!server) { + if (!server || this[kServerClosed]) { if (typeof optionalCallback === "function") process.nextTick(optionalCallback, $ERR_SERVER_NOT_RUNNING()); return; } - this[serverSymbol] = undefined; + this[kServerClosed] = true; if (typeof optionalCallback === "function") setCloseCallback(this, optionalCallback); this.listening = false; + // Graceful stop: stop accepting, close idle connections. Callers like + // `@azure/msal-node` follow this with `closeAllConnections()` to force- + // close remaining in-flight sockets, so we must NOT null out + // `this[serverSymbol]` here — that reference is still needed by the + // subsequent `closeAllConnections()` / `unref()` calls. server.closeIdleConnections(); server.stop(); }; @@ -426,7 +456,12 @@ Server.prototype[Symbol.asyncDispose] = function () { }; Server.prototype.address = function () { - if (!this[serverSymbol]) return null; + // Node returns null from address() once close() has been called, even + // if draining isn't finished. We keep `this[serverSymbol]` populated + // past `close()` so the msal pattern `close() → closeAllConnections()` + // still reaches the native layer — `kServerClosed` is the "has close + // been called" signal. + if (!this[serverSymbol] || this[kServerClosed]) return null; return this[serverSymbol].address; }; @@ -541,6 +576,10 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (tls) { this.serverName = tls.serverName || host || "localhost"; } + // Reset the "has close been called" flag — Node allows listening + // again after close, so address()/close() need to behave normally + // on the fresh listen. + this[kServerClosed] = false; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, @@ -877,8 +916,13 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort // }, }); - getBunServerAllClosedPromise(this[serverSymbol]).$then(emitCloseNTServer.bind(this)); - isHTTPS = this[serverSymbol].protocol === "https"; + // Capture the Bun.serve handle for this listen generation so the + // close callback can tell whether it's still the current one (see + // `emitCloseNTServer`). Without this, a fresh `listen()` that races + // the previous shutdown would have its new handle nulled out. + const bunServer = this[serverSymbol]; + getBunServerAllClosedPromise(bunServer).$then(emitCloseNTServer.bind(this, bunServer)); + isHTTPS = bunServer.protocol === "https"; // always set strict method validation to true for node.js compatibility setServerCustomOptions( this[serverSymbol], diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 9a5e5f90b6f4..489886b5564f 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1539,6 +1539,28 @@ impl NewServer { } let Some(listener) = self.listener.take() else { + // Upgrading from graceful (`stop(false)`) to abrupt: + // `stop(false)` already took the listener, but the uWS App + // still owns open/idle keep-alive connections that would + // otherwise pin the event loop. `app.close()` force-closes + // every remaining socket on the app. This is the path that + // makes `server.close(); server.closeAllConnections();` + // actually drain msal-style loopback servers. + // + // This runs BEFORE the h3 branch below, because the h3 branch + // flips `TERMINATED` on abrupt stops, which would make our + // `!TERMINATED` guard false and skip the app close for + // h3-enabled servers (HTTPS + h3: true). + if abrupt && !self.flags.contains(ServerFlags::TERMINATED) { + if let Some(app) = self.app { + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.app = None; + } + self.flags.insert(ServerFlags::TERMINATED); + // S012: `NewApp` is a ZST opaque — safe deref. + bun_opaque::opaque_deref_mut(app).close(); + } + } if Self::HAS_H3 && self.h3_app.is_some() { self.unref(); self.notify_inspector_server_stopped(); diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 6182510d5cd2..751a0090767f 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -2499,23 +2499,35 @@ 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 - }; - self.stop(abrupt); + } + false + }; + + // For an abrupt stop, the app may still have open connections + // even after a graceful `stop(false)` cleared `self.listener`. + // `stop_listening` force-closes those via `app.close()` only when + // it actually enters the abrupt branch, so we can't gate on + // `has_listener()` here. `stop`/`stop_listening` are idempotent: + // second calls short-circuit via the `terminated` flag and the + // `deinit_scheduled` flag, so this is safe. + if abrupt { + if self.has_listener() || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) { + self.stop(true); + } + } else if self.has_listener() { + self.stop(false); } rc } pub fn dispose_from_js(&mut self) -> JSValue { - if self.has_listener() { + if self.has_listener() || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) { self.stop(true); } JSValue::UNDEFINED diff --git a/test/js/node/http/node-http-close-all-connections.test.ts b/test/js/node/http/node-http-close-all-connections.test.ts new file mode 100644 index 000000000000..1a0499dd14e9 --- /dev/null +++ b/test/js/node/http/node-http-close-all-connections.test.ts @@ -0,0 +1,120 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import { once } from "node:events"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { connect } from "node:net"; + +// Regression: `@azure/msal-node`'s `LoopbackClient.closeServer` calls +// server.close(); +// server.closeAllConnections(); +// server.unref(); +// in sequence. Bun used to null out the internal server reference in +// `close()`, so the subsequent `closeAllConnections()` was a no-op — +// the keep-alive socket kept the event loop alive and the process hung. +// Issue: https://github.com/oven-sh/bun/issues/30501 +test("closeAllConnections() after close() force-closes in-flight sockets", async () => { + const { promise: requestReceived, resolve: resolveReceived } = Promise.withResolvers(); + const server = http.createServer((req, _res) => { + // Signal receipt but DO NOT reply — socket is "in flight" (not idle) + // when the teardown sequence runs. This is the case where close() + // alone (which only closes idle connections) cannot reclaim the + // socket, and closeAllConnections() must do it. + resolveReceived(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as AddressInfo; + + const sock = connect(port, "127.0.0.1"); + const { promise: sockClosed, resolve: resolveClosed } = Promise.withResolvers(); + sock.on("close", () => resolveClosed()); + sock.on("error", () => {}); + await once(sock, "connect"); + sock.write("GET / HTTP/1.1\r\nHost: localhost\r\nConnection: keep-alive\r\n\r\n"); + await requestReceived; + + // msal-style teardown — no waiting between calls + server.close(); + server.closeAllConnections(); + server.unref(); + + // The client socket must be force-closed by closeAllConnections(). + // Without the fix, the socket stays open indefinitely (msal hang). + await sockClosed; +}); + +// Regression: when a caller does close() and then listen() again before +// the previous shutdown's allClosed promise has fulfilled, the stale +// callback must not null out the newly-created server handle. +test("listen() during an in-flight close() doesn't corrupt the new server", async () => { + const server = http.createServer((_req, res) => res.end("ok")); + + await once(server.listen(0, "127.0.0.1"), "listening"); + + // Fire-and-forget close; don't wait for the allClosed callback. + server.close(); + + // Re-listen immediately while the previous shutdown is still settling. + await once(server.listen(0, "127.0.0.1"), "listening"); + const secondAddress = server.address() as AddressInfo | null; + expect(secondAddress).not.toBeNull(); + const secondPort = secondAddress!.port; + expect(secondPort).toBeInteger(); + + // Drain the new server — address() must still return a port after + // microtasks run (this is where the stale close callback would have + // hit, pre-fix). + await new Promise(r => setImmediate(r)); + expect((server.address() as AddressInfo | null)?.port).toBe(secondPort); + + // Confirm the new server actually serves requests, not just has a port. + const res = await fetch(`http://127.0.0.1:${secondPort}`); + expect(await res.text()).toBe("ok"); + + await new Promise(r => server.close(() => r())); +}); + +// End-to-end: spawn a child that opens an HTTP server, accepts a +// keep-alive connection, and calls the msal teardown. Must exit +// immediately — not wait for the keep-alive idle timeout to reclaim +// the in-flight socket. +test("process exits after close() + closeAllConnections() + unref() teardown", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const net = require("node:net"); + const server = http.createServer((req, _res) => { + // Never reply — keep the socket in-flight (not idle) so that + // only closeAllConnections() (abrupt) can reclaim it. + server.close(); + server.closeAllConnections(); + server.unref(); + process.stdout.write("TEARDOWN_DONE\\n"); + }); + server.listen(0, "127.0.0.1", () => { + const port = server.address().port; + const sock = net.connect(port, "127.0.0.1", () => { + sock.write("GET / HTTP/1.1\\r\\nHost: localhost\\r\\nConnection: keep-alive\\r\\n\\r\\n"); + }); + sock.on("data", () => {}); + sock.on("error", () => {}); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + // If the teardown path is broken, the subprocess never exits and + // `proc.exited` never resolves — the bun:test runner's default 5s + // timeout catches that. + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Surface any uncaught exception / ASAN trace before the exit-code + // assertion so failures point at the real cause. + expect(stderr).toBe(""); + expect(stdout).toContain("TEARDOWN_DONE"); + expect(exitCode).toBe(0); +}); From 282497a12c29fef42549849d6946112c4b85219d Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2026 14:50:47 +0000 Subject: [PATCH 2/4] [autofix.ci] apply automated fixes --- src/runtime/server/server_body.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 751a0090767f..9d8ba08f0a47 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -2516,7 +2516,9 @@ where // second calls short-circuit via the `terminated` flag and the // `deinit_scheduled` flag, so this is safe. if abrupt { - if self.has_listener() || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) { + if self.has_listener() + || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) + { self.stop(true); } } else if self.has_listener() { @@ -2527,7 +2529,9 @@ where } pub fn dispose_from_js(&mut self) -> JSValue { - if self.has_listener() || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) { + if self.has_listener() + || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) + { self.stop(true); } JSValue::UNDEFINED From e66460b8e0596f75d5ef27b950b57b38cb344ecc Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 18 Jun 2026 15:47:21 +0000 Subject: [PATCH 3/4] fix(http): gate ref()/setTimeout()/listening on kServerClosed after close() Keeping serverSymbol populated past close() (so closeAllConnections() and unref() can reach the native handle) changed the meaning of a populated serverSymbol for three call sites that used it as a 'still listening' proxy: - ref(): re-pinned the event loop on a closed server, keeping the loop alive until GC (effectively a hang in the zero-connection case). - setTimeout(): configured the idle timeout on the stopped handle instead of deferring it so the next listen() replays it onto the fresh server. - emitListeningNextTick(): could re-announce 'listening' (and flip this.listening back to true) if a close() raced the deferred tick. Gate all three on !kServerClosed, matching address()/close(). unref() stays ungated since it is the third step of the msal close() -> closeAllConnections() -> unref() teardown this PR enables. Adds a ref()-after-close() regression test (hangs without the gate). --- src/js/node/_http_server.ts | 17 ++++++++-- .../node-http-close-all-connections.test.ts | 33 +++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 62068c9da77a..281d1e67c473 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -232,7 +232,10 @@ function emitRequestCloseNT(self) { } function emitListeningNextTick(self, hostname, port) { - if ((self.listening = !!self[serverSymbol])) { + // `serverSymbol` now survives past close() (see close()/emitCloseNTServer), + // so a close() that raced this deferred tick must not re-announce + // 'listening'; gate on `kServerClosed` like address()/close() do. + if ((self.listening = !!self[serverSymbol] && !self[kServerClosed])) { // TODO: remove the arguments // Note does not pass any arguments. self.emit("listening", null, hostname, port); @@ -356,7 +359,11 @@ function setupConnectionsTracking(this: any) { Server.prototype.ref = function () { this._unref = false; - this[serverSymbol]?.ref?.(); + // Don't re-pin the loop on a server that's already closing — after + // close() `serverSymbol` is still populated but `kServerClosed` is set. + // (unref() below is intentionally NOT gated: it's the third step of + // msal's `close() → closeAllConnections() → unref()` teardown.) + if (!this[kServerClosed]) this[serverSymbol]?.ref?.(); return this; }; @@ -953,7 +960,11 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort Server.prototype.setTimeout = function (msecs, callback) { const server = this[serverSymbol]; - if (server) { + // After close() `serverSymbol` is still populated but `kServerClosed` + // is set; defer the timeout (as when not yet listening) so the next + // listen() replays it onto the fresh server rather than configuring + // the stopped one. + if (server && !this[kServerClosed]) { setServerIdleTimeout(server, Math.ceil(msecs / 1000)); if (typeof callback === "function") this.once("timeout", callback); } else { diff --git a/test/js/node/http/node-http-close-all-connections.test.ts b/test/js/node/http/node-http-close-all-connections.test.ts index 1a0499dd14e9..e5e4611bbf72 100644 --- a/test/js/node/http/node-http-close-all-connections.test.ts +++ b/test/js/node/http/node-http-close-all-connections.test.ts @@ -74,6 +74,39 @@ test("listen() during an in-flight close() doesn't corrupt the new server", asyn await new Promise(r => server.close(() => r())); }); +// Regression: close() now leaves the native handle populated (so +// closeAllConnections()/unref() can still reach it), which means ref() +// must not re-pin the event loop on an already-closed server. A stray +// ref() after close() would otherwise keep the loop alive until GC. +test("ref() after close() does not keep the event loop alive", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const server = http.createServer(); + server.listen(0, "127.0.0.1", () => { + server.close(); + // Pre-fix this re-activated the poll ref on a closed server and + // pinned the loop (no listener, no connections) until GC. + server.ref(); + process.stdout.write("REF_DONE\\n"); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + // If ref() re-pinned the loop, the subprocess never exits and + // `proc.exited` never resolves — the runner's timeout catches that. + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toContain("REF_DONE"); + expect(exitCode).toBe(0); +}); + // End-to-end: spawn a child that opens an HTTP server, accepts a // keep-alive connection, and calls the msal teardown. Must exit // immediately — not wait for the keep-alive idle timeout to reclaim From 73af2d7791619e1934fb52fdb1822e3b268a54ff Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 18 Jun 2026 15:50:43 +0000 Subject: [PATCH 4/4] fix(http): reset kServerClosed only after Bun.serve() succeeds In kRealListen the flag was cleared before Bun.serve() ran. If Bun.serve() throws (e.g. EADDRINUSE) on a re-listen after close(), serverSymbol still points at the old draining handle; clearing kServerClosed early would make address()/close() treat that stopped handle as live. Reset the flag only after the handle is successfully reassigned. --- src/js/node/_http_server.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 281d1e67c473..3675c4e20f5d 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -583,10 +583,6 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (tls) { this.serverName = tls.serverName || host || "localhost"; } - // Reset the "has close been called" flag — Node allows listening - // again after close, so address()/close() need to behave normally - // on the fresh listen. - this[kServerClosed] = false; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, @@ -923,6 +919,14 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort // }, }); + // Reset the "has close been called" flag — Node allows listening + // again after close, so address()/close() behave normally on the + // fresh listen. Done only after `Bun.serve()` succeeds: if it threw + // (e.g. EADDRINUSE) `serverSymbol` still points at the old draining + // handle, and leaving `kServerClosed` set keeps address()/close() + // reporting "not running" until that handle actually drains. + this[kServerClosed] = false; + // Capture the Bun.serve handle for this listen generation so the // close callback can tell whether it's still the current one (see // `emitCloseNTServer`). Without this, a fresh `listen()` that races