node:http: defer server 'close' until every tracked connection has ended by robobun · Pull Request #35837 · oven-sh/bun

robobun · 2026-07-26T00:04:48Z

Repro

import http from "node:http"; import net from "node:net";
const server = http.createServer((req, res) => setTimeout(() => res.end("resp:" + req.url), 200));
server.keepAliveTimeout = 60000;
server.listen(0, "127.0.0.1", () => {
  const c = net.connect(server.address().port, "127.0.0.1");
  let closeCbAt = null, finAt = null, data = "";
  c.on("data", d => data += d); c.on("end", () => finAt = Date.now());
  c.on("connect", () => {
    c.write("GET /first HTTP/1.1\r\nHost: x\r\n\r\n");
    setTimeout(() => {
      server.close(() => { closeCbAt = Date.now(); });
      setTimeout(() => {
        c.write("GET /second HTTP/1.1\r\nHost: x\r\n\r\n");
        setTimeout(() => {
          console.log({ closeCbFired: closeCbAt !== null, connFin: finAt !== null,
                        resps: data.match(/resp:\/\w+/g) });
          process.exit(0);
        }, 900);
      }, 900);
    }, 50);
  });
});

	`closeCbFired`	`connFin`	`resps`
bun	true	false	`["resp:/first","resp:/second"]`
node	false	false	`["resp:/first","resp:/second"]`

The server.close() callback is documented as firing once all connections have ended. Bun fires it as soon as the in-flight response is written, yet the keep-alive connection is still open and keeps serving new requests. Any server.close(() => process.exit()) style graceful drain is told the server is drained while a live client can still issue requests into it.

Cause

getBunServerAllClosedPromise (bound at listen() time) resolves when the native server reaches pending_requests == 0 && !listener && !websockets and drives emitCloseServer directly. A keep-alive connection that was serving a request when close() ran is not idle (so closeIdleConnections() skips it), and after the response finishes pending_requests drops to zero while the TCP connection stays open. Nothing in that condition tracks open connections.

Fix

Gate emitCloseServer on kTrackedConnections.size and on the native handle being gone, mirroring Node's net.Server#_emitCloseIfDrained. When the native promise resolves with connections still tracked, record a pending-drain flag; the last connection's #onClose re-checks and schedules the actual 'close' emit. kRealListen resets the flag so a re-listen does not inherit the previous cycle's pending state.

closeAllConnections() and closeIdleConnections() now fall back to iterating kTrackedConnections when close() has already dropped the native handle, so the documented deadline pattern (server.close(cb); setTimeout(() => server.closeAllConnections(), N)) can force the drain.

Verification

New test/js/node/http/node-http-server-close-drain.test.ts:

server.close(cb) does not fire while a keep-alive connection is still open: fail-before closeCbFired: true after the first response; after the fix the callback is withheld until the client closes the socket.
server.close(cb) fires once an idle keep-alive connection is reaped: control for the existing idle-at-close path.
closeAllConnections() after close() force-drains the withheld callback: the deadline pattern releases the callback.
no 'close' is emitted on a re-listened server when an earlier connection ends: re-listen guard.

Also green: node-http.test.ts (same pass/fail as main), 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, and bun-server.test.ts's late keep-alive request to a node:http server after close().

[review] gate passed · iteration 2 · 2 files touched

fails on main (without fix)

ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-server-close-drain.test.ts
bun test v1.4.0 (765455a4a)

test/js/node/http/node-http-server-close-drain.test.ts:
50 |     // Yield a few event-loop turns after the bytes arrive so the server's
51 |     // "all requests done" task chain has run before the callback is checked.
52 |     releaseResponse();
53 |     while (!body.includes("resp:/first")) await once(socket, "data");
54 |     for (let i = 0; i < 4; i++) await new Promise<void>(r => setImmediate(r));
55 |     expect(closeCbFired).toBe(false);
                              ^
error: expect(received).toBe(expected)

Expected: false
Received: true

      at <anonymous> (/workspace/bun/test/js/node/http/node-http-server-close-drain.test.ts:55:26)
(fail) server.close(cb) does not fire while a keep-alive connection is still open [773.59ms]
(pass) server.close(cb) fires once an idle keep-alive connection is reaped [329.26ms]
134 |       closed.resolve();
135 |     });
136 |     releaseResponse();
137 |     while (!body.includes("ok")) await once(socket, "data
... (truncated)

release without fix: 1 FAILED
bun test v1.4.0-canary.1 (1dcca2f57)

test/js/node/http/node-http-server-close-drain.test.ts:
(pass) server.close(cb) does not fire while a keep-alive connection is still open [27.04ms]
(pass) server.close(cb) fires once an idle keep-alive connection is reaped [5.49ms]
(pass) closeAllConnections() after close() force-drains the withheld callback [6.41ms]
198 | 
199 |     // Closing the new server then emits 'close' exactly once. The first
200 |     // cycle's callback was registered via once('close') and fires now too,
201 |     // like Node (and passing a second callback does not throw).
202 |     const closed = Promise.withResolvers<void>();
203 |     server.close(() => closed.resolve());
                 ^
error: Close callback already set
      at setCloseCallback (node:_http_server:95:16)
      at <anonymous> (node:_http_server:310:21)
      at <anonymous> (/workspace/bun/test/js/node/http/node-http-server-close-drain.test.ts:203:12)
(fail) no 'close' is emitted on a re-listened server when an earlier connection ends [14.62ms]

 3 pass
 1 fail
 12 expect() calls
Ran 4 tests across 1 file. [360.00ms]
__F:1:S:0

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-server-close-drain.test.ts
bun test v1.4.0 (765455a4a)

test/js/node/http/node-http-server-close-drain.test.ts:
(pass) server.close(cb) does not fire while a keep-alive connection is still open [838.70ms]
(pass) server.close(cb) fires once an idle keep-alive connection is reaped [192.25ms]
(pass) closeAllConnections() after close() force-drains the withheld callback [209.61ms]
(pass) no 'close' is emitted on a re-listened server when an earlier connection ends [288.07ms]

 4 pass
 0 fail
 14 expect() calls
Ran 4 tests across 1 file. [5.09s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 842ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/31] gen cpp.rs (cppbind)
[2/31] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (13 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser (31 fields)
Found 8 classes from /workspace/bun/src/runtime/api/html_rewriter.classes.ts
  - HTMLRewriter (3 fields)
  - TextChunk (7 fi
... (truncated)

diff hotspot

src/js/node/_http_server.ts                        |  50 ++++-
 .../node/http/node-http-server-close-drain.test.ts | 211 +++++++++++++++++++++
 2 files changed, 251 insertions(+), 10 deletions(-)

gate history · 3 passed · 1 rejected · iteration 2

evidence per changed file

file                                                    reads  edits  tests
src/js/node/_http_server.ts                                19     18      0
test/js/node/http/node-http-server-close-drain.test.ts      2      3      0

server.close(cb) was firing as soon as the native all-closed promise resolved (pending_requests == 0), even though a keep-alive connection that was mid-request at close() time is still open at that point and keeps accepting requests. Node's net.Server#_emitCloseIfDrained holds 'close' until every accepted connection has ended. Gate emitCloseServer on kTrackedConnections.size and re-check when the last tracked connection's #onClose runs.

coderabbitai · 2026-07-26T00:04:55Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3094e6bd-8c84-4c37-aa10-62f49736f2d3

📥 Commits

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

📒 Files selected for processing (2)

src/js/node/_http_server.ts
test/js/node/http/node-http-server-close-drain.test.ts

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

robobun · 2026-07-26T00:08:15Z

^{Updated 6:19 PM PT - Jul 25th, 2026}

🔄 @robobun, the build for 1dcca2f5 was cancelled — 765455a4 is building instead in Build #81871. Stay tuned...

node-http.test.ts has an environment-dependent proxy test that fails in this CI container; a standalone file lets the mechanical fail-before / pass-after check run cleanly.

…tions after close() close() clears the native handle, which made both methods no-ops and left no way to force-drain a connection that had been mid-request at close() time. Iterate the tracked set directly in that state. Also guard emitCloseServer against a re-listened server and reset the pending-drain flag in kRealListen so the previous cycle's flag cannot fire 'close' on the new server.

…pread)

robobun · 2026-07-26T01:09:47Z

Subsumed by #35844, which adds the same kTrackedConnections.size gate and additionally keeps the native handle reachable under a closing flag so closeIdleConnections()/closeAllConnections() keep working after close(), and rewrites both off the tracked set so they no longer touch the listener. The scenario-C test here is covered there as scenario C of the four-scenario harness.

setCloseCallback throws when a callback is already stored, so a deferred callback that was never reached (because emitCloseServer bailed on open connections) would block the next close(cb) after a re-listen. Node's net.Server#close uses once('close'), which stacks listeners across cycles and needs no separate clear step.

claude

All three earlier findings (closeAllConnections/closeIdleConnections no-op after close(), stale kPendingDrainClose across re-listen, and the kCloseCallback "already set" throw) are addressed and covered by new tests. I didn't find further issues this pass. Leaving for a human to weigh the overlap with #35844 (which the author noted subsumes this) and to sign off on the server-close lifecycle change.

What was reviewed:

emitCloseServer gate on serverSymbol + kTrackedConnections; verified the once('close', cb) switch means the removed callCloseCallback(self) is dead for the server path.
closeAllConnections/closeIdleConnections fallback: snapshot via Array.from before destroy, and ordering when closeAllConnections() runs before the native promise resolves — the later emitCloseServer still fires because size is 0 by then.
kRealListen resets kPendingDrainClose; #onClose only re-checks when the flag is set, so a re-listened server with new connections won't spuriously emit.

Extended reasoning...

Overview

This PR changes src/js/node/_http_server.ts to defer the server 'close' event and server.close(cb) callback until every tracked TCP connection has ended, matching Node's net.Server#_emitCloseIfDrained. It introduces a kPendingDrainClose flag, gates emitCloseServer on both serverSymbol (the native handle) and kTrackedConnections.size, has NodeHTTPServerSocket#onClose re-trigger the emit when the last tracked socket drops, resets the flag in kRealListen, and switches the close callback registration from the single-slot setCloseCallback to this.once('close', cb) (matching Node). closeAllConnections() and closeIdleConnections() gain a fallback that iterates kTrackedConnections when close() has already dropped the native handle. A new 4-test file exercises the withheld-callback path, the idle-reap control, the deadline force-drain, and the re-listen guard.

Security risks

None identified. This is event/lifecycle sequencing in the Node HTTP compat layer; no parsing of untrusted input, no auth/crypto, no resource-limit changes.

Level of scrutiny

Medium-high. node:http server close/listen is a widely-exercised compat surface with subtle re-entrancy (re-listen, forced drain, keep-alive). This PR went through two rounds of review that surfaced three real bugs (post-close force-drain no-op, stale pending flag across re-listen, and a "Close callback already set" throw), all now fixed with tests. The once('close', cb) switch is a small but real behavior change (callbacks now stack across cycles like Node instead of throwing on the second), which is the correct direction but worth a human confirming.

Other factors

The author noted this is subsumed by #35844, which apparently takes a broader approach (keeps the native handle reachable under a closing flag rather than falling back to the tracked set). A human should decide which PR to land.
Tests are in a new file rather than the existing node-http.test.ts; they use bounded polling (while (!body.includes(...)) await once(socket, 'data') + a few setImmediate turns) rather than sleeps, and each has a finally cleanup. The for (i < 4) setImmediate yield-loop is a mild smell per REVIEW.md but is bounded and comments why.
I traced the ordering where closeAllConnections() is called synchronously after close() (before the native all-closed promise resolves and sets kPendingDrainClose): sockets are destroyed with the flag still false so #onClose doesn't schedule, but the pending native-promise resolution then runs emitCloseServer with size === 0 and emits — no hang.
setCloseCallback/callCloseCallback are now unused for the server object (still used for sockets/responses), so removing callCloseCallback(self) from emitCloseServer is safe.

robobun · 2026-07-26T03:07:18Z

Build #81871: 190 jobs passed, 1 failed. The only red is the pre-existing freebsd x64 build-bun step failure (also failing on main; step-failed-outside-runner). All test lanes either passed or were flaky and passed on retry. node-http-server-close-drain.test.ts is green on every lane that ran it.

Re: #35844, both PRs gate emitCloseServer on kTrackedConnections. This PR is the minimal fix for the reported premature-callback bug plus the force-drain and re-listen guards that review surfaced; #35844 additionally changes closeAllConnections() to leave the listener running (closer to Node, but a behaviour change for existing callers). Either approach fixes the reported scenario; happy to close this if #35844 is preferred.

robobun · 2026-08-13T01:39:15Z

Triage note: this PR is the remaining carrier for the "server 'close' fires while keep-alive connections are still open" gap. The overlapping PRs have been consolidated: #35839 now owns closeAllConnections() / closeIdleConnections() (listener stays up, both work after close()), and #35844, which bundled both changes, was closed in favour of the two of them.

Two things to pick up here once #35839 lands:

The post-close() branches this PR adds to closeAllConnections() / closeIdleConnections() become redundant (both methods iterate the tracked set unconditionally there), so this will need a rebase that drops them and keeps only the 'close' gate.
Scenario D from node:http: gate server.close() on connection drain, not request count #35844's tests (the callback must not fire while a client keeps issuing requests on the surviving connection) is worth folding in; this PR's current tests cover the single in-flight case and the forced drain.

Re-checked today: 3 of the 4 tests here still fail on a fresh main build, so the bug is still live (#35130 gated Bun.serve's drain promise on open connections but explicitly left node:http out).

robobun assigned Jarred-Sumner Jul 26, 2026

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

move the server-close drain test to its own file

a691af8

node-http.test.ts has an environment-dependent proxy test that fails in this CI container; a standalone file lets the mechanical fail-before / pass-after check run cleanly.

github-actions Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts Outdated

autofix-ci Bot and others added 2 commits July 26, 2026 00:25

[autofix.ci] apply automated fixes

88c18a4

trim the emitCloseServer gate comment

68845f5

claude Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts

Comment thread src/js/node/_http_server.ts

robobun added 2 commits July 26, 2026 00:47

use Array.from over spread for the tracked-set snapshot (no-useless-s…

1dcca2f

…pread)

robobun mentioned this pull request Jul 26, 2026

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

Closed

claude Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread src/js/node/_http_server.ts

robobun mentioned this pull request Jul 26, 2026

node:http: make closeAllConnections()/closeIdleConnections() leave the listener alone and work after close() #35839

Open

claude Bot reviewed Jul 26, 2026

View reviewed changes

Navigation Menu

Uh oh!

node:http: defer server 'close' until every tracked connection has ended - #35837

node:http: defer server 'close' until every tracked connection has ended#35837
robobun wants to merge 7 commits into
mainfrom
farm/f86f69f4/http-server-close-drain

robobun commented Jul 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

Review limit reached

Uh oh!

robobun commented Jul 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 26, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jul 26, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

robobun commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

robobun commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 26, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jul 26, 2026

Uh oh!

robobun commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Jul 26, 2026 •

edited

Loading

coderabbitai Bot commented Jul 26, 2026 •

edited

Loading

robobun commented Jul 26, 2026 •

edited

Loading