node:http: make closeAllConnections()/closeIdleConnections() leave the listener alone and work after close() by robobun · Pull Request #35839 · oven-sh/bun

robobun · 2026-07-26T00:38:17Z

Consolidated fix for node:http server.closeAllConnections() / server.closeIdleConnections(). Supersedes #31302, #33394, #30505 and #35844 (see "Consolidation" below).

Fixes #31301
Fixes #30501

Problem

server.closeAllConnections() was implemented as stop(true) on the native server: it tore down the listen socket, flipped listening to false, fired 'close', and made a later close(cb) report ERR_SERVER_NOT_RUNNING. Node only destroys the connections and keeps accepting (node:http: Server.closeAllConnections() shuts down the listening socket #31301, Playwright's test server calls it between tests and got ECONNREFUSED on every request after the first).
close() nulls the native handle synchronously, and both closeAllConnections() and closeIdleConnections() read that handle, so after close() both were no-ops. @azure/msal-node tears its loopback server down with close(); closeAllConnections(); unref() while the browser's connection is still in flight, so the connection was never reclaimed and the process hung (getTokenInteractive in @azure/msal-node will cause Bun to hang after the end of the script #30501). The same no-op defeats Node's documented drain pattern (close(); setTimeout(() => closeIdleConnections(), grace)) and http-terminator.
Both methods live in src/js/node/_http_server.ts (Server.prototype.closeAllConnections / closeIdleConnections).

Fix

Both methods now iterate kTrackedConnections (the per-server Set of NodeHTTPServerSockets that getConnections() and the 'connection' event already maintain) and socket.destroy() each matching socket. The set outlives close(), so the post-close path works, the listener is never touched, and the socket objects get Node's observable state synchronously (destroyed === true, 'close' fires), which the native close path did not give them.
Both methods skip sockets that Node's parser-keyed ConnectionsList would no longer contain: freeParser() removes a connection once its 'upgrade' / 'connect' request has been received in full, which is at the handoff for CONNECT and body-less upgrades and after the body for an upgrade that carries one (Node 26 delivers upgrade bodies). releaseServerParserShim nulls socket.parser at the handoff in every case, so the skip is parser == null plus "no request message still being received" (isOutsideConnectionsList). Upgraded WebSocket and CONNECT sockets stay alive; an upgrade whose body is still arriving is still destroyed by closeAllConnections() and skipped by closeIdleConnections(), both as in Node.
closeIdleConnections() additionally skips connections with a response in flight (_httpMessage not finished) and connections currently receiving a request. The latter uses a new read-only hasIncompleteRequest getter on the native socket handle (JSNodeHTTPServerSocket): "a request message (head or body) is being received on this connection", the state headersTimeout / requestTimeout already track, and the same thing ConnectionsList::idle() tests via last_message_start_. So a freshly accepted connection, a partial request head, and a request body that is still uploading after an early response are all not idle. The getter exposes that state, not the field; when http: classify idle connections by request message state, not response end #37889 changes how the native side tracks it, only the getter's body changes.
One deliberate difference from Node: a connection with pipelined responses still queued behind a finished one is treated as in flight. Node v26 destroys it at that point and drops the queued responses; the native idle sweep that close() runs already keeps it (Bun.serve: close idle connections on graceful stop(), declare closeIdleConnections() #37074), so the JS method does too.
Behaviour change to be aware of: code that used closeAllConnections() alone as a shutdown keeps its listener (and so its process) alive now, as it would on Node; it needs close() as well. The handful of in-tree tests doing that are updated in this PR.
close() itself is unchanged: it still runs the native idle sweep before stopping the listener. Switching it to the JS predicate as well is a behaviour change to close() that belongs with http: classify idle connections by request message state, not response end #37889, which is where the native sweep's idle classification is being fixed; this PR is limited to the two public methods.
This works on current main without further native changes because node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 made the tracked set complete (connections are tracked from accept, not from the first request) and Bun.serve: gate the graceful stop() drain promise on open connections #35130 made the native stop(true)-after-stop(false) path sound; before those landed, earlier PRs needed native additions.
Verified with test/js/node/http/node-http-server-close-connections.test.ts: 16 tests, all pass with this branch. 9 of the first 12 fail on main (the 3 that pass either way are the body-less upgrade and never-listened controls); the 4 added after self-review pin the upgrade-with-body rule, the uploading-body clause and the pipelined clause, none of which the rest of the suite exercised (closeAllConnections() on an uploading upgrade failed on the previous revision of this branch). Every in-process assertion was checked against Node v26.3.0, except the pipelining test, which documents the difference above.
Also green locally: test/js/node/test/parallel/test-http{,s}-server-close-{all,idle,destroy-timeout}.js, test-http-server-close-idle-wait-response.js, test-http-server-connection-list-when-close.js, node-http.test.ts, node-http-with-ws.test.ts, node-http-server-timeouts.test.ts, client-fetch.test.ts, ws.test.ts, @fastify/websocket.
Existing tests that used closeAllConnections() as a full shutdown now also call close(), which is what they would need on Node. Two bun-parallel tests that asserted the old teardown (listening === false, the connections-checking interval destroyed) now assert Node's behaviour.

Consolidation

Five open PRs overlapped here. Tested against a fresh main debug build before choosing: none of the scenarios is fixed on main yet (closeAllConnections() still stops the listener; both methods are still no-ops after close()).

fix(node:http): keep listener open in Server.closeAllConnections #31302 (@mxschmitt): fixed the listener teardown by adding a native close-all primitive through uWS. Now that the JS layer tracks every connection from accept, the JS-side loop covers the same sockets with Node's synchronous semantics and also covers the post-close() case, so the native primitive is no longer needed; the branch also conflicts with main in three files. Its "no connections established" test case is folded in here with credit.
http: closeAllConnections() must not stop the listener #33394: the closeAllConnections() half of this change; its multi-connection test is folded in.
fix(http): preserve server reference across close() for closeAllConnections() #30505: kept the native handle alive past close() so stop(true) could still run. Its Rust half landed in Bun.serve: gate the graceful stop() drain promise on open connections #35130; the JS half is superseded by not depending on the handle at all. Its msal-node teardown test (the literal getTokenInteractive in @azure/msal-node will cause Bun to hang after the end of the script #30501 scenario, as a subprocess that must exit) is folded in.
node:http: gate server.close() on connection drain, not request count #35844: the same two-method rewrite plus a JS-side gate that delays the server 'close' event until every tracked connection has ended. That gate is a separate Node-compat gap (the 'close' callback currently fires while keep-alive connections are still open) and is carried on its own by node:http: defer server 'close' until every tracked connection has ended #35837, so it is not duplicated here.

Adjacent, not overlapping: #37889 reworks how the native idle sweep (used by close() and Bun.serve) classifies idle connections and replaces the state that hasIncompleteRequest currently reads; whichever of the two lands second adapts the getter's body, its meaning stays the same. #37717 does the equivalent of this change for the HTTP/1 fallback helpers that both methods call first.

Background

kTrackedConnections: a Set on each node:http Server holding the JS NodeHTTPServerSocket wrapper for every open connection. The native server calls into JS when it accepts a connection (post-handshake for TLS), the wrapper adds itself on construction and removes itself when the native socket closes. getConnections() reports its size.
socket.parser: Node attaches an HTTPParser to every server connection and frees it once an 'upgrade' / 'connect' request has been received in full, handing the raw socket to the listener. Bun keeps a parser shim on the wrapper and nulls it at the handoff, so parser == null identifies handed-off sockets; the body-still-arriving window between Bun's handoff and Node's free is what the hasIncompleteRequest half of the skip accounts for.
hasIncompleteRequest / lastMessageStartMs: uWS's node-compat response data records when the message currently being received started (set on accept and whenever a request head starts arriving, cleared once head and body are in). It backs headersTimeout / requestTimeout and corresponds to last_message_start_ in Node's parser, which is what Node's closeIdleConnections() consults; the getter reports whether it is set.
Pipelining: a client may send its next request before reading the previous response. Bun dispatches the second request immediately and queues its response behind the first (kPipelinedResponses); Node also dispatches it but its idle check only looks at the first response.

Repro (bun: exit 86, node: exit 0 before this change)

// repro.mjs [idle|all]
import http from "node:http"; import net from "node:net";
const mode = process.argv[2] === "all" ? "all" : "idle";
const s = http.createServer((req, res) => setTimeout(() => res.end("ok"), 300));
s.keepAliveTimeout = 60000;
s.listen(0, "127.0.0.1", () => {
  const c = net.connect(s.address().port, "127.0.0.1");
  let fin = null;
  c.on("data", () => {}); c.on("end", () => (fin = Date.now())); c.on("error", () => {});
  c.on("connect", () => {
    c.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");        // in flight at close() time
    setTimeout(() => {
      s.close();
      setTimeout(() => {
        mode === "all" ? s.closeAllConnections() : s.closeIdleConnections();
        setTimeout(() => { c.destroy(); process.exit(fin === null ? 86 : 0); }, 500);
      }, 1500);
    }, 100);
  });
});

	node v26.3.0	bun (before)	bun (after)
`closeIdleConnections()` after `close()`	FIN sent, exit 0	no-op, exit 86	FIN sent, exit 0
`closeAllConnections()` after `close()`	FIN sent, exit 0	no-op, exit 86	FIN sent, exit 0
`closeAllConnections()` on a live server	connections dropped, listener stays up	listener stopped, next request `ECONNREFUSED`	connections dropped, listener stays up

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.stream.test.ts

…lose() Both Server.prototype.closeIdleConnections() and closeAllConnections() read this[serverSymbol] and returned early when it was undefined. close() nulls that reference synchronously, so the canonical graceful-drain pattern server.close(cb); setTimeout(() => server.closeIdleConnections(), grace); and the http-terminator force path server.close(cb); server.closeAllConnections(); were both no-ops on Bun: a connection that was in flight at close() time and went idle afterwards could not be reaped by the application and lived until keepAliveTimeout fired. Rewrite both methods to iterate the kTrackedConnections set (the one that already backs getConnections() and the 'connection' event) and destroy() each socket, which is exactly what Node.js does. This also stops closeAllConnections() from tearing down the listener (Node leaves it accepting), so tests that used it as a full shutdown now call close() too.

robobun · 2026-07-26T00:38:26Z

Status: this is now the consolidated PR for the closeAllConnections() / closeIdleConnections() cluster (#31302, #33394, #30505 and #35844 were closed in its favour; #35837 remains for the separate 'close'-event timing gap).

Reproduced on a fresh main debug build before consolidating: closeAllConnections() still stops the listener, and both methods are still no-ops after close(). The branch is merged up to current main (one conflict in _http_server.ts, resolved by keeping the HTTP/1 fallback sweeps main added at the top of both methods).

Self-review follow-up (4a10e91): a handed-off upgrade connection whose request body is still arriving is now treated as Node does (still destroyed by closeAllConnections(), skipped by closeIdleConnections()), and the uploading-body and pipelined-queue clauses of the idle predicate have their own tests; the pipelined one is documented as a deliberate difference from Node.

Proof: bun bd test test/js/node/http/node-http-server-close-connections.test.ts passes 16/16 with the fix; 9 of the original 12 fail with src/ at main. Tests folded in from the closed PRs: the msal-node teardown subprocess (#30501), the multi-connection sweep (#33394) and the no-connections case (#31302, co-authored). Node's test-http{,s}-server-close-* and connection-list-when-close parallel tests, node-http-with-ws, node-http-server-timeouts, ws.test.ts and @fastify/websocket also pass locally.

robobun · 2026-07-26T00:38:29Z

^{Updated 5:46 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 96f948f has 1 failures in Build #94320 (All Failures):

test/js/third_party/grpc-js/test-tonic.test.ts - code 1 on 🍎 14 aarch64

🧪 To try this PR locally:

bunx bun-pr 35839

That installs a local version of the PR into your bun-35839 executable, so you can run:

bun-35839 --bun

github-actions · 2026-07-26T00:41:46Z

Found 1 issue this PR may fix:

node:http: Server.closeAllConnections() shuts down the listening socket #31301 - Reports that closeAllConnections() incorrectly shuts down the listening socket via server.stop(true), which this PR fixes by iterating kTrackedConnections instead

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #31301

🤖 Generated with Claude Code

coderabbitai · 2026-07-26T00:42:15Z

Walkthrough

Changes

HTTP server shutdown

Layer / File(s)	Summary
Tracked socket drain implementation `src/js/node/_http_server.ts`	`closeAllConnections()` destroys all tracked sockets, while `closeIdleConnections()` skips sockets with active or queued responses.
Shutdown behavior coverage `test/js/node/http/node-http-server-close-connections.test.ts`, `test/js/bun/test/parallel/*`	Tests cover idle and active connections, listening state, interval destruction, forced shutdown, and unopened servers.
Explicit server cleanup updates `test/js/first_party/ws/ws.test.ts`, `test/js/node/http/`, `test/js/web/fetch/`	Test teardown paths now call `close()` after `closeAllConnections()`.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main behavior change for both connection-closing methods after server.close().
Description check	✅ Passed	The description explains the problem, fix, behavior changes, consolidation, and verification results in detail.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/http/node-http-server-close-connections.test.ts`:
- Around line 72-113: Extend the test “skips in-flight connections and reaps
idle ones” with a raw pipelined-request case that queues a second response on
the same socket, then call server.closeIdleConnections() and verify that socket
remains open while the pipelined response is pending. Use the existing server
lifecycle cleanup and assert the socket is only closed after
server.closeAllConnections().

🪄 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: 2304c417-248a-4c5c-b257-ce58221e1680

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and ba8e513.

📒 Files selected for processing (9)

src/js/node/_http_server.ts
test/js/bun/test/parallel/test-http-server.listening-should-work.ts
test/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts
test/js/first_party/ws/ws.test.ts
test/js/node/http/node-http-server-close-connections.test.ts
test/js/node/http/node-http-with-ws.test.ts
test/js/node/http/node-http.test.ts
test/js/web/fetch/client-fetch.test.ts
test/js/web/fetch/fetch.stream.test.ts

github-actions · 2026-07-26T00:44:59Z

This PR may be a duplicate of:

fix(http): preserve server reference across close() for closeAllConnections() #30505 - Fixes closeAllConnections() becoming a no-op after close() by preserving the server reference; alternative approach to the same bug
fix(node:http): keep listener open in Server.closeAllConnections #31302 - Fixes closeAllConnections() stopping the listener, which is part of the same bug surface
http: closeAllConnections() must not stop the listener #33394 - Uses the kTrackedConnections approach for closeAllConnections(); node:http: make closeAllConnections()/closeIdleConnections() leave the listener alone and work after close() #35839 extends this to also cover closeIdleConnections()

🤖 Generated with Claude Code

robobun · 2026-07-26T01:18:16Z

Outdated: #35844 has since been closed in favour of this PR (for the two drain methods) plus #35837 (for the 'close' event gate). See the PR description for the current relationship between the PRs.

…ion-drain methods Both methods iterate kTrackedConnections; that set includes sockets handed over to 'upgrade'/'connect' listeners and sockets whose first (or next) request head has not been fully received yet. Node.js's ConnectionsList is parser-keyed (freeParser removes the entry on handoff) and its idle() skips any parser whose last_message_start_ is non-zero (set on accept as DoS protection and on each message begin), so neither class of socket is touched there. Match that: - Skip socket.parser == null in both methods: releaseServerParserShim nulls it on the same 'upgrade'/'connect' handoff where Node frees the parser. - Add a hasIncompleteRequest getter on the native NodeHTTPServerSocket handle that exposes lastMessageStartMs != 0 (the same field isRequestTimedOut reads), and skip those sockets in closeIdleConnections(). New tests: an upgraded socket survives both calls; closeIdleConnections() leaves fresh-accept and partial-head sockets alone while reaping a keep-alive idle one. All assertions verified against Node.js v26.3.0.

claude

Additional findings (outside current diff — PR may have been updated during review):

🟡 src/js/node/_http_server.ts:525 — Node's closeIdleConnections() checks socket._httpMessage && !socket._httpMessage.finished — it only spares a socket whose response is unfinished. Line 525 checks only socket._httpMessage truthy, so a socket whose res.end() has set finished = true but which hasn't yet been detached (detach runs on the async 'finish' event) is spared here where Node destroys it. Adding && !socket._httpMessage.finished matches Node exactly; the practical impact is only that the socket becomes reapable one tick later, so not blocking.
Extended reasoning...

What the bug is

Node.js's Server.prototype.closeIdleConnections() (lib/_http_server.js, ~line 701) filters in-flight connections with:
```
if (socket._httpMessage && !socket._httpMessage.finished) continue;
```
i.e. a socket is spared only when it has an outgoing response and that response is not yet finished. A socket whose _httpMessage.finished === true falls through and is destroyed.

Bun's new predicate at src/js/node/_http_server.ts:525 is:
```
if (socket.parser == null || socket._httpMessage || socket[kPipelinedResponses]?.length) {
  continue;
}
```
which spares the socket on _httpMessage truthiness alone, regardless of .finished.

The window in which they diverge

ServerResponse.prototype.end() sets this.finished = true synchronously (_http_server.ts:3263 / :3802 / :3820). But socket._httpMessage is only cleared by detachSocket(), which is called from the response 'finish' handler (onServerResponseFinish, _http_server.ts:2491) or from the dispatcher after the handler returns (:1118) — both of which run on a later turn than the synchronous res.end() when the handler is async or when 'finish' is deferred by write backpressure.

So there is a real, non-zero window where socket._httpMessage is set and socket._httpMessage.finished === true. During that window Node's closeIdleConnections() destroys the socket; Bun's spares it.

Why the other guards don't cover it
- socket.parser == null filters upgraded/CONNECT sockets — irrelevant here.
- socket[kPipelinedResponses]?.length filters queued pipelined responses — irrelevant here.
- socket[kHandle]?.hasIncompleteRequest filters partial request heads — the request was fully received, so lastMessageStartMs is 0 and this returns false.
None of these fire, so the only remaining guard is socket._httpMessage, which is truthy → continue.

Step-by-step proof
1. Client sends GET / HTTP/1.1 with Connection: keep-alive; request head is fully parsed → lastMessageStartMs = 0, assignSocketInternal sets socket._httpMessage = res.
2. Handler runs res.end('ok'). Synchronously: res.finished = true (_http_server.ts:3263), 'prefinish' is emitted, process.nextTick(markResponseEndedNT, this) is queued. socket._httpMessage is still res.
3. Before the next tick, application code (or the connections-checking interval, or an awaiting caller in the same microtask window) calls server.closeIdleConnections().
4. Node: socket._httpMessage && !socket._httpMessage.finished → res && !true → false → falls through → socket.destroy().
  Bun (line 525): socket._httpMessage → truthy → continue → socket survives.
5. On the next tick, 'finish' fires → detachSocket() clears _httpMessage → the socket is now reapable by a subsequent closeIdleConnections() call.
Impact

The divergence is real but small: the socket becomes reapable one event-loop turn later than in Node. In the graceful-drain pattern this PR targets (server.close(cb); setTimeout(() => server.closeIdleConnections(), grace)), the timer callback runs on a fresh turn where detachSocket() has already run, so the difference is essentially unobservable there. It is only observable to code that calls closeIdleConnections() in the same turn as res.end() (or between res.end() and 'finish' under write backpressure).

Per REVIEW.md's "the reference implementation is the spec" rule for Node compat, and given the fix is a one-clause addition to code this PR just wrote to mirror Node, worth doing — but not worth blocking merge over.

Fix
```
if (socket.parser == null || (socket._httpMessage && !socket._httpMessage.finished) || socket[kPipelinedResponses]?.length) {
  continue;
}
```

robobun · 2026-07-26T01:43:28Z

Applied the _httpMessage.finished refinement in b5a1b9d: closeIdleConnections() now skips on socket._httpMessage && !socket._httpMessage.finished, matching Node's check at lib/_http_server.js ~701 exactly. All 9 tests and the node-parallel close-idle tests still pass.

claude

No further issues found — both earlier concerns (upgraded/partial-head sockets, and the _httpMessage.finished clause) are addressed in the current revision. Deferring to a human on the merge decision given the competing approaches in #30505/#31302/#33394 and the note that #35844 may subsume this.

What was reviewed:

closeIdleConnections idle predicate vs Node's ConnectionsList.idle(): parser == null skip, _httpMessage && !finished, and the new hasIncompleteRequest gate all match the reference semantics.
closeAllConnections no longer stops the listener; the 7 updated tests now match what Node requires.
New C++ hasIncompleteRequest getter mirrors the existing isRequestTimedOut guard structure and reads the same lastMessageStartMs field.
Set iteration during socket.destroy(): #onClose deletion is deferred via postTask, and JS Set iteration tolerates concurrent deletion of visited elements regardless.

Extended reasoning...

Overview

This PR rewrites Server.prototype.closeAllConnections() and Server.prototype.closeIdleConnections() in src/js/node/_http_server.ts to iterate the JS-side kTrackedConnections Set instead of routing through the native this[serverSymbol] reference (which close() nulls). It adds a native hasIncompleteRequest getter on JSNodeHTTPServerSocket (C++ header + impl + prototype table) exposing lastMessageStartMs != 0, so the JS idle predicate can match Node's ConnectionsList.idle() semantics for fresh-accept and partial-head connections. It also stops closeAllConnections() from tearing down the listener (previously server.stop(true)), which is a user-visible behavior change that brings Bun in line with Node. A new 9-test file plus updates to 7 existing tests cover the new behavior.

Security risks

None identified. The change is confined to connection-drain logic in the node:http compat layer; no auth, crypto, path handling, or untrusted-input parsing is touched. The new native getter is read-only and follows the exact guard pattern of the neighbouring isRequestTimedOut.

Level of scrutiny

High. This is a behavioral change to a widely-used node:http API surface (closeAllConnections previously acted as a full shutdown in Bun; now it does not stop listening). It also adds a new C++ getter to a JSC-bound class. The change is well-reasoned and thoroughly tested against Node v26.3.0, but node:http compat is production-critical and the semantic change to closeAllConnections could surprise Bun users who relied on the old (Bun-specific) behavior.

Other factors

I raised two issues in earlier passes (upgraded-socket / partial-head destruction, and the missing .finished refinement); both were fixed and are covered by the new test file.
Three open PRs (#30505, #31302, #33394) take alternative approaches to overlapping bugs, and the author noted this PR is subsumed by #35844. A maintainer should decide which of these lands.
The bug-hunting system found no issues on the current head.
CI build #81913 was in progress at review time; result not yet visible in the timeline.

claude

Both earlier findings (upgraded-socket destruction, _httpMessage.finished gate) are addressed in 61aa232 / b5a1b9d, and I found nothing further on this revision. Deferring to a maintainer given the overlap with #30505 / #31302 / #33394 / #35844 and the user-visible behavior change to closeAllConnections() (no longer stops the listener / fires 'close').

What was reviewed:

socket.parser == null skip vs. the releaseServerParserShim call sites (upgrade at ~1025, CONNECT at ~812, close at ~1687) — upgraded/CONNECT sockets are now spared in both methods.
hasIncompleteRequest getter mirrors isRequestTimedOut's lastMessageStartMs read and guards (!socket || upgraded || closed); the NodeHttpResponseData<SSL> cast matches the sibling impl.
Iterating the live kTrackedConnections Set while destroying entries — the JS-side #onClose delete runs from a posted task, not synchronously, so no iteration hazard.
The seven test-file edits that add server.close() after closeAllConnections() — each was previously relying on the removed listener-stop side effect.

Extended reasoning...

Overview

Rewrites Server.prototype.closeAllConnections() and closeIdleConnections() in src/js/node/_http_server.ts to iterate this[kTrackedConnections] and socket.destroy() matching entries, so both work after close() has nulled this[serverSymbol]. Adds a hasIncompleteRequest getter on the native JSNodeHTTPServerSocket (C++, three files) exposing lastMessageStartMs != 0 so closeIdleConnections() can spare fresh-accept and partial-head connections like Node's ConnectionsList.idle(). New test file with 9 cases verified against Node v26.3.0; seven existing tests updated to call close() after closeAllConnections() now that the latter no longer stops the listener.

Prior findings resolved

My first review flagged that upgraded/CONNECT sockets and partial-head connections were destroyed as "idle"; 61aa232 added the socket.parser == null skip and the native hasIncompleteRequest gate, with tests for both. My second review flagged the missing !message.finished clause; b5a1b9d applied it. Both threads are resolved and covered by the new test file.

Security risks

None identified. The change narrows what gets destroyed relative to the previous server.stop(true); no untrusted-input parsing, no auth/TLS paths touched. The new native getter is read-only and guarded against null/closed/upgraded sockets.

Level of scrutiny

Medium-high. This is a Node-compat behavior change on a hot public API surface: closeAllConnections() previously stopped the listener and fired 'close' in Bun (divergent from Node), and code in the wild may depend on that. The seven in-tree tests that had to be updated are evidence. The idle predicate is subtle (four gates: parser released, _httpMessage && !finished, pipelined queue, hasIncompleteRequest), each mapped to a specific Node semantics — the mapping looks correct and is now tested, but it is the kind of change a maintainer should sign off on.

Other factors

The author noted this is "subsumed by #35844", and github-actions flagged three other open PRs (#30505, #31302, #33394) covering overlapping ground with different approaches. Which of these lands is a maintainer call. CI (#81913) was retriggered in b60e4c4; I did not verify its outcome.

robobun · 2026-07-26T04:28:53Z

Quick note on the review summary above: this PR is not subsumed by #35844. #35844 fixes the close(cb) callback firing before connections have drained (the closeCbAt half of the repro table); this PR fixes closeIdleConnections() / closeAllConnections() being no-ops after close() (the finAt half). They are complementary. The overlap with #30505 / #31302 / #33394 is described in the PR body.

…ose-connections-after-close # Conflicts: # src/js/node/_http_server.ts

…eAllConnections() Folds in the scenarios from #30505 (issue #30501: close(); closeAllConnections(); unref() with a request in flight must let the process exit) and #33394 (every tracked connection is destroyed synchronously and the listener stays up).

…e listener alone Test case from #31302. Co-authored-by: Max Schmitt <max@schmitt.mx>

…ose-connections-after-close

…as arrived Node only frees the parser (and so drops the connection from the list the two drain methods walk) once the upgrade request is complete, which for an upgrade that carries a body is after the body. Use the native "message still being received" state to decide when a handed-off socket has left the list, so closeAllConnections() still destroys such a connection and closeIdleConnections() still skips it, as in Node. Also cover the two closeIdleConnections() clauses that had no discriminating test: a request body still arriving after an early response, and a pipelined response queue (which is deliberately kept, unlike Node).

claude

I reviewed this and didn't find any bugs. All three findings from earlier passes (upgraded/CONNECT sockets, partial-head via hasIncompleteRequest, the .finished clause, and the stale halfclose-midupload comment) are addressed. Given it's a behavior change to two public node:http methods with a deliberate Node divergence on pipelined queues and an interaction with #37889, a maintainer sign-off would still be worthwhile.

What was reviewed:

isOutsideConnectionsList and the idle predicate against Node's ConnectionsList semantics; upgrade-with-body window covered by hasIncompleteRequest.
hasIncompleteRequest C++ getter — same guard/dereference pattern as isRequestTimedOut; safe on closed/upgraded sockets.
Set iteration during socket.destroy() — #onClose deletes from kTrackedConnections via a posted task, not synchronously; ES Set iteration is deletion-safe regardless.
The 16-test suite pins each predicate clause (fresh-accept, partial-head, uploading-body, upgrade-with-body, pipelined) plus the #30501 subprocess repro.

Extended reasoning...

Overview

Rewrites Server.prototype.closeAllConnections() and closeIdleConnections() in src/js/node/_http_server.ts to iterate kTrackedConnections instead of calling into the native server handle, so both methods (a) leave the listener alone and (b) keep working after close() has nulled the handle. Adds a shared isOutsideConnectionsList(socket) helper mirroring Node's parser-keyed ConnectionsList membership. Adds a read-only hasIncompleteRequest getter on JSNodeHTTPServerSocket (C++) exposing lastMessageStartMs != 0, the same field isRequestTimedOut reads. Updates seven existing tests that used closeAllConnections() as a full shutdown to also call close(), flips two bun-parallel tests to assert Node's behavior, removes a stale comment, and adds a 458-line test file with 16 cases.

Security risks

None. The change moves connection-teardown iteration from native code to JS over a set the JS layer already maintains; no new user-controlled input reaches native code. The new C++ getter is read-only and guards !socket || upgraded || us_socket_is_closed before dereferencing us_socket_ext, exactly like the existing isRequestTimedOut.

Level of scrutiny

High — this is a behavior change to two public node:http methods that real packages depend on for graceful shutdown (Playwright, @azure/msal-node, http-terminator). The PR went through three earlier review rounds where I flagged real bugs (upgraded WebSocket sockets being destroyed as idle; partial-head connections being reaped; missing .finished check), all of which were fixed. The idle predicate now has four clauses, each with a dedicated test, and the isOutsideConnectionsList helper correctly models the upgrade-with-body window where Bun's handoff runs earlier than Node's freeParser().

Other factors

The PR documents one deliberate divergence from Node v26: a connection with pipelined responses queued behind a finished one is kept, where Node destroys it and drops the queue. This aligns with Bun's native idle sweep (#37074) and is pinned by a test that says "unlike Node.js" — but it's a design call a maintainer should acknowledge.
The PR notes #37889 will change the state hasIncompleteRequest reads; whichever lands second adapts the getter body. That coordination is worth a human eye.
Code that previously used closeAllConnections() alone as a shutdown will now leave the listener alive (matching Node); the PR updates in-tree callers, but this is a user-visible behavior change.
All comment-cop and prior inline threads are resolved. The most recent commit (96f948f) only removed the stale comment I flagged; nothing else has changed since my last pass.

Given the scope, the deliberate Node divergence, and the cross-PR coordination, this warrants a maintainer sign-off rather than auto-approval.

robobun assigned Jarred-Sumner Jul 26, 2026

github-actions Bot added the claude label Jul 26, 2026

coderabbitai Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread test/js/node/http/node-http-server-close-connections.test.ts

claude Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts

github-actions Bot mentioned this pull request Jul 26, 2026

node:http: gate server.close() on connection drain, not request count #35844

Closed