node:http: gate server.close() on connection drain, not request count - #35844
node:http: gate server.close() on connection drain, not request count#35844robobun wants to merge 6 commits into
Conversation
server.close(cb) was wired to Bun.serve's all-closed promise, whose condition is pending_requests == 0 && !listener && !websockets. A keep-alive connection that was serving a request when close() ran is still open once that request finishes, so the callback fired (and 'close' emitted) while the connection kept accepting requests. close() also nulled this[serverSymbol] immediately, so closeIdleConnections() and closeAllConnections() became no-ops after close(), and closeAllConnections() was a full stop(true) that tore down the listener. Gate emitCloseServer on kTrackedConnections.size (re-checked from the last connection's #onClose), keep the native handle reachable under a kClosing flag until the server has actually drained, and rewrite closeAllConnections()/closeIdleConnections() to iterate kTrackedConnections so they work before and after close() without touching the listener.
WalkthroughChangesThe HTTP server now keeps its native listener during HTTP server close and drain lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…isten The kTrackedConnections pass in closeIdleConnections() treated a connection whose request head is still arriving as idle (no _httpMessage yet). On a live server the native isIdle sweep already spares it, so restrict the Set pass to the kClosing window it exists for. Reset kClosing/kPendingDrainClose in kRealListen and bail in emitCloseServer when !kClosing so a listen() during a prior close()'s drain does not have its new handle cleared by the stale drain, and gate emitListeningNextTick on !kClosing for the same _handle-stand-in consistency as address().
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 10:18 PM PT - Jul 25th, 2026
❌ @robobun, your commit 768edc0 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35844That installs a local version of the PR into your bun-35844 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/_http_server.ts`:
- Around line 527-534: Update the kClosing idle-sweep loop in the HTTP server to
reuse the native idle-socket predicate, preserving sockets currently parsing
request headers even when _httpMessage is not yet attached. Keep the existing
checks and destruction behavior for truly idle sockets, and add a regression
test covering closeIdleConnections() after close() during mid-header parsing.
In `@test/js/node/http/node-http.test.ts`:
- Around line 3471-3475: Move the entire “server.close() drains connections, not
requests” suite and its supporting setup into a dedicated topic-specific HTTP
test file named node-http-close-drain.test.ts, removing it from
node-http.test.ts. Preserve all six tests and their existing behavior without
unrelated refactoring.
🪄 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: d12f1121-2c81-466a-b805-49f3961fe8ef
📒 Files selected for processing (8)
src/js/node/_http_server.tstest/js/bun/test/parallel/test-http-server.listening-should-work.tstest/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.tstest/js/first_party/ws/ws.test.tstest/js/node/http/node-http-with-ws.test.tstest/js/node/http/node-http.test.tstest/js/web/fetch/client-fetch.test.tstest/js/web/fetch/fetch.stream.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/_http_server.ts:713-716— The re-listen-during-drain reset at lines 715–716 clearskClosingandkPendingDrainClosebut notkCloseCallback, and the newif (!self[kClosing]) returnat line 133 skipscallCloseCallback— soclose(cb1); listen(); /* old drain resolves */ close(cb2)now throwsError: Close callback already setatsetCloseCallback(line 154), where pre-PR it firedcb1and cleared the slot, and Node stacks both viaonce('close', cb). Addthis[kCloseCallback] = undefinedalongside the other resets (or have the!kClosingearly-return still runcallCloseCallback).Extended reasoning...
What the bug is
Commit e0f88d5 (the follow-up in this PR that fixes the re-listen-during-drain handle leak flagged in the earlier review) added two things: an
if (!self[kClosing]) return;guard at the top ofemitCloseServer(line 133), and a drain-state reset block inkRealListen(lines 715–716) that clearsthis[kClosing] = falseandthis[kPendingDrainClose] = false. But the reset block does not clearthis[kCloseCallback], and the new early-return inemitCloseServerbypassescallCloseCallback(self)at line 142 — the only place that clearskCloseCallback(src/js/internal/http.ts:174-179). The result is that a re-listen during a drain leaves the first close callback stuck in the slot, and the nextserver.close(cb)throws.Step-by-step proof
server.listen(0)→kRealListensetsserverSymbol = OLD, armsgetBunServerAllClosedPromise(OLD).$then(emitCloseNTServer)(line ~1194).server.close(cb1)→ line 550 setsthis[kClosing] = true; line 551setCloseCallback(this, cb1)setsthis[kCloseCallback] = cb1;OLD.stop().server.listen(0)(before OLD's all-closed promise resolves) →kRealListenruns: line 715this[kClosing] = false, line 716this[kPendingDrainClose] = false, line 717this[serverSymbol] = NEW, arms NEW's all-closed promise.this[kCloseCallback]is stillcb1— the reset block does not touch it.- OLD's all-closed promise resolves →
emitCloseNTServer→process.nextTick(emitCloseServer, this).emitCloseServerline 133:if (!self[kClosing]) return—kClosingwas reset tofalsein step 3, so it returns immediately without reachingcallCloseCallback(self)at line 142.kCloseCallbackstill holdscb1. - Later,
server.close(cb2)→ line 537if (!server || this[kClosing])—serverSymbol = NEWandkClosing = false, so it proceeds; line 550 setskClosing = true; line 551setCloseCallback(this, cb2).setCloseCallback(lines 149–157):cb2 !== self[kCloseCallback](which iscb1), andself[kCloseCallback]is truthy, so line 154 throwsnew Error("Close callback already set").
Why this is a regression introduced by this PR
Before this PR,
emitCloseServerwas justcallCloseCallback(self); self.emit("close")with no!kClosingguard, so step 4 would have unconditionally firedcb1and cleared the slot (with a spurious'close'emit), and step 5'ssetCloseCallback(this, cb2)would have found an empty slot and succeeded. Node usesthis.once('close', cb)for the close callback, so bothcb1andcb2stack as listeners and fire together when the re-listened server eventually closes — it never throws. The throw is new behavior introduced by e0f88d5, in exactly the close→re-listen-during-drain scenario that commit's own comment at lines 713–714 says it handles ("A listen() during a prior close()'s drain installs a fresh handle; the pending drain must not clear it").Why existing code doesn't prevent it
callCloseCallback(src/js/internal/http.ts:174-179) is the only path that nullskCloseCallback, and it is now reachable fromemitCloseServeronly whenself[kClosing]is stilltrueat the time the drain completes.kRealListen's reset block is the third piece of drain state alongsidekClosingandkPendingDrainClose, and per REVIEW.md's "fix the whole class in the same PR" it belongs in the same reset — the previous review round's fix that added lines 715–716 was two-out-of-three.Impact
A hard synchronous
throwon a publicnode:httpAPI in a sequence that worked before this PR and works in Node. The window this PR intentionally lengthens (drain now waits for connections, not requests — up tokeepAliveTimeout) makes the re-listen-during-drain race correspondingly easier to hit than before.Fix
One line: add
this[kCloseCallback] = undefined;next to lines 715–716 inkRealListen, so the reset block clears all three pieces of drain state together. This silently dropscb1— the least-surprising choice given the reset block's stated intent (cancel the pending drain), and closer to pre-PR behavior minus the spurious'close'. The alternative — having the!kClosingearly-return still callcallCloseCallback(self)before returning — firescb1at re-listen time, which is closer to Node's eventual-fire semantics but conflates "drain cancelled" with "drain completed".
…inally blocks The re-listen reset cleared kClosing/kPendingDrainClose but not kCloseCallback, so close(cb1); listen(); close(cb2) hit setCloseCallback's 'Close callback already set' throw. The new drain tests' finally blocks only called closeAllConnections(), which no longer stops the listener; add server.close() so a failure before the happy-path close() does not leak the listener.
|
Addressed both findings from the second review round in 0a908e4:
|
…ves no stale handle
Clearing serverSymbol before the fallible Bun.serve() call orphaned a live listener in the listen() -> listen(fails) path. Move the drain-state reset to after the assignment so a throw leaves the server in exactly its pre-listen() state.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/_http_server.ts (1)
293-293: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBind queued
listeningemission to the current listen cycle.
kClosingalone does not block a stale tick afterlisten(cb1); close(); listen(cb2): the secondlisten()clears it before the first timer runs, so the old callback can emitlisteningon the new handle and fire the wrongonce("listening")listener. Pass a generation token or the installed handle intoemitListeningNextTickand require it to still matchserver[serverSymbol]before emitting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/node/_http_server.ts` at line 293, Update the listening emission flow around emitListeningNextTick to bind each queued callback to the listen cycle that created it, using a generation token or the installed handle. Before setting listening or emitting the event, require the captured token/handle to still match server[serverSymbol], while preserving normal emission for the current listen cycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js/node/_http_server.ts`:
- Line 293: Update the listening emission flow around emitListeningNextTick to
bind each queued callback to the listen cycle that created it, using a
generation token or the installed handle. Before setting listening or emitting
the event, require the captured token/handle to still match
server[serverSymbol], while preserving normal emission for the current listen
cycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 183ed8f0-c428-4b0e-bd6f-9e53647da0d9
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/http/node-http.test.ts
|
Re the outside-diff finding on |
There was a problem hiding this comment.
No new issues found after 768edc0. Deferring to a maintainer given the scope: this reworks the node:http server close/drain state machine (kClosing/kPendingDrainClose, deferred serverSymbol clear, re-listen reset) and changes closeAllConnections() from stop(true) to a socket-set sweep — broad Node-compat surface that's worth a human sign-off.
What was reviewed:
- The final
kRealListenshape (768edc0): resets committed only afterBun.serve()succeeds, so bothclose()→listen(fails)andlisten()→listen(fails)leave state intact. emitCloseServer'skTrackedConnectionsgate +#onClosereschedule;!kClosingbail correctly suppresses the OLD promise after a successful re-listen.closeIdleConnections()Set fallback is gated onkClosingso the native mid-parse predicate stays authoritative on live servers.- Test cleanups: all seven new tests plus the five external call sites now
server.close()infinally.
Extended reasoning...
Overview
The PR changes src/js/node/_http_server.ts to gate server.close(cb)'s 'close' emit on kTrackedConnections.size === 0 (mirroring Node's net.Server#_emitCloseIfDrained) instead of the native pending_requests == 0 signal alone. To make that work it introduces a kClosing/kPendingDrainClose state pair, defers clearing serverSymbol until emitCloseServer, rewrites closeAllConnections() to iterate the tracked-socket Set (leaving the listener alone), extends closeIdleConnections() with a post-close Set fallback, and adds a re-listen reset in kRealListen. Seven new tests in node-http.test.ts cover the drain contract; five other test files add server.close() after closeAllConnections() since the latter no longer stops the listener.
Security risks
None identified. This is server-lifecycle bookkeeping in the JS compat layer; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
High. node:http server close/drain is a hot compat path (graceful shutdown, k8s SIGTERM handlers, test frameworks). The PR went through four prior review rounds here, each surfacing a real edge (test cleanup leak, stale kCloseCallback on re-listen, stale serverSymbol on failed re-listen, orphaned OLD handle on listen()→listen(fails)) — all now addressed, but the density of state-machine edges argues for a maintainer confirming the final shape and the closeAllConnections() semantic change (which, while Node-correct, is a behavioral change for anyone using it as a stand-in for close()).
Other factors
The PR subsumes three open PRs (#35837, #35839, #33394) and fixes #31301/#30501, so a maintainer should also decide the disposition of those. All prior inline findings on this PR are resolved; the bug-hunting pass on 768edc0 found nothing. I traced the two re-listen failure shapes against the final code and both leave the server in its pre-listen() state as claimed.
|
CI on 768edc0 (#82044): 171 lanes passed and no file this PR touches appears in any failure annotation. The seven new drain tests and all Remaining red is unrelated to this diff:
Ready for maintainer review. |
|
Closing to consolidate; the two things this PR does are now split across two open PRs:
All of these scenarios were re-tested against a fresh |
2 participants
Repro
cbFiredAtresponsesnull0server.close()'s callback fires as soon as the first response is written, then five more requests are served on that same connection. Anyserver.close(() => process.exit())style graceful drain is told the server is drained while a live keep-alive client is still issuing requests into it.Cause
emitCloseNTServeris driven bygetBunServerAllClosedPromise, whose condition (deinit_if_we_can) ispending_requests == 0 && !listener && !websockets. That is a pending-request counter, where Node'snet.Serverwaits for_connectionsto reach zero. A keep-alive connection that was mid-request whenclose()ran dropspending_requeststo zero once that response is written, so the promise resolves while the TCP connection is still open and serving.Two consequences of
close()also nullingthis[serverSymbol]immediately:closeIdleConnections()/closeAllConnections()became no-ops afterclose(), so the standardclose(); closeAllConnections()drain could not force the orphaned connection down.closeAllConnections()itself calledserver.stop(true), which tears down the listener and fires'close'; Node's contract is to destroy the connections and keep accepting.Fix
All in
_http_server.ts(no native change):emitCloseServeris gated onkTrackedConnections.size === 0, mirroring Node'snet.Server#_emitCloseIfDrained. When the native promise resolves with connections still tracked it recordskPendingDrainCloseand returns; the last connection's#onClosere-checks and schedules the actual'close'emit.close()keeps the native handle reachable under akClosingflag instead of nullingserverSymbol(soaddress()reportsnulland a secondclose()reportsERR_SERVER_NOT_RUNNING, while the drain helpers still work).closeAllConnections()iterateskTrackedConnectionsanddestroy()s each socket, leaving the listener alone.closeIdleConnections()does the native idle sweep (knows about partially-parsed requests) and additionally iterateskTrackedConnections, skipping sockets with an in-flight response, so it keeps working afterclose()once the native app has deinit'd.Verification
New
describe("server.close() drains connections, not requests")intest/js/node/http/node-http.test.tscovers the four graceful-shutdown shapes plus the listener invariant:Also green:
test-http-server-close-{all,idle,idle-wait-response,destroy-timeout}.js,test-https-server-close-{all,idle}.js,test-http-server-connection-list-when-close.js,test-http-req-res-close.js,bun-server.test.ts"late keep-alive request",node-http-server-timeouts.test.ts.Two bun-parallel tests and five test cleanups that used
closeAllConnections()as a stand-in forclose()now callclose()explicitly, matching Node.Relationship to open PRs
#35837 adds the
kTrackedConnections.sizegate alone (scenario C); #35839 rewritescloseIdleConnections()/closeAllConnections()off the set (scenarios B/B' and the listener invariant, like #33394). #31302 and #30505 are earlier partial approaches to thecloseAllConnections()half. This PR is the unified fix for the full drain contract and subsumes #35837, #35839, and #33394.no test proof · iteration 0 · 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
Fixes #31301
Fixes #30501