fix(http): preserve server reference across close() for closeAllConnections() - #30505
fix(http): preserve server reference across close() for closeAllConnections()#30505robobun wants to merge 4 commits into
Conversation
|
Updated 9:36 AM PT - Jun 18th, 2026
❌ @robobun, your commit 73af2d7 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 30505That installs a local version of the PR into your bun-30505 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
I tested the repro from #23648 against this branch — it still hangs (supertest creates an internal keep-alive socket that For #22490 there's no repro to test against; this PR makes |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSeparates "close initiated" from native server teardown via a new kServerClosed flag; preserves native handle during shutdown to allow closeAllConnections()/unref(); guards against stale close callbacks when re-listening; and adds runtime graceful-to-abrupt shutdown upgrades plus regression tests verifying teardown behavior. ChangesHTTP Server Shutdown Lifecycle
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 81-89: The deferred close callback (emitCloseNTServer) must avoid
clearing a newly-restarted server; capture the current server handle or
generation when scheduling the all-closed promise and in emitCloseNTServer only
clear this[serverSymbol] and call emitCloseServer if the stored
handle/generation still matches the one captured at scheduling time. Update the
code paths that schedule the callback (the place that registers the all-closed
promise against this) and modify emitCloseNTServer to compare the savedHandle or
generation token against this[serverSymbol] before setting it to undefined and
calling emitCloseServer to prevent a re-listen race; reference
emitCloseNTServer, emitCloseServer, and serverSymbol when making the change.
In `@test/js/node/http/node-http.test.ts`:
- Around line 1833-1869: The test "process exits after
close()+closeAllConnections()+unref() teardown" currently uses Bun-only APIs
(Bun.spawn, bunExe, bunEnv, proc.stdout.text(), proc.exited) which breaks the
file's Node.js compatibility; either rewrite the test to use Node's
child_process APIs (e.g., spawn/exec from node:child_process and replace
bunExe/bunEnv usage with node executable and process.env) and wire up
stdout/stderr and exit handling via the ChildProcess streams/promises, or move
this test into a Bun-only test file; update references inside the test
(Bun.spawn, bunExe, bunEnv, proc.stdout.text(), proc.exited) accordingly so the
test runs under Node.js or is located in a Bun-only suite.
- Around line 1857-1873: Remove the embedded 5s watchdog: delete the
setTimeout(...) block that writes "STILL_ALIVE\n" and calls process.exit(2) (the
code using setTimeout and the "STILL_ALIVE" sentinel), and update the test
assertions to stop expecting the sentinel by removing the
expect(stdout).not.toContain("STILL_ALIVE") check; keep the subprocess
spawn/await logic (proc, stdout, exitCode) and the existing
expect(exitCode).toBe(0) and expect(stdout).toContain("TEARDOWN_DONE") so the
outer test timeout handles hangs and stderr can be used for additional
diagnostics if needed.
🪄 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: 3f3d1c1e-36ea-487e-adff-4c119943f79e
📒 Files selected for processing (3)
src/js/node/_http_server.tssrc/runtime/server/server.zigtest/js/node/http/node-http.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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-close-all-connections.test.ts`:
- Around line 41-51: Capture and check the subprocess stderr before asserting
exitCode to surface runtime diagnostics: await proc.stderr.text() alongside
proc.stdout.text() (e.g., const [stdout, stderr, exitCode] = await
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])) and add an
assertion such as expect(stderr).toBe("") or expect(stderr).toHaveLength(0)
(placed before expect(exitCode).toBe(0)) so any uncaught exceptions or ASAN
messages are visible in test output; update references to proc.stdout.text(),
proc.stderr.text(), and proc.exited accordingly.
🪄 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: 9cb8dd67-cbbd-4431-b2f1-8cf459cc0e86
📒 Files selected for processing (3)
src/js/node/_http_server.tstest/js/node/http/node-http-close-all-connections.test.tstest/js/node/http/node-http.test.ts
|
CI is clean on everything except |
Per the Node docs, `Server.prototype.closeAllConnections()` should forcefully close every established HTTP(S) connection but leave the listening socket alone. Bun's wrapper was calling `server.stop(true)`, which goes through `stop_listening` and tears down the listener, so any caller that relied on the documented contract — Playwright's `TestServer.reset()` between tests is the canonical case — got ECONNREFUSED on every subsequent request. Add a dedicated path through every layer parallel to the existing `closeIdleConnections` plumbing (uws → libuwsockets shim → App.rs → server host fn → class registration), then make the JS wrapper delegate to it without touching `serverSymbol`, `kConnectionsCheckingInterval`, or `listening`. Related: oven-sh#30501, oven-sh#30505 (different framing of an overlapping bug).
…ctions() 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
4c4dc72 to
7f65d17
Compare
|
Rebased onto main and resolved the conflict. Since this PR was opened, main ported the HTTP server from Zig to Rust and added connection tracking (
Squashed into one commit. Regression tests pass with the fix, time out without it; the rest of |
|
CI red lanes on this build are all unrelated flake, none touch this diff (which is limited to
The HTTP-server lanes are green. Already used my one CI re-roll this cycle, so not pushing another empty commit. Ready for a maintainer to merge. |
…lose() 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).
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.
|
Latest CI (build 63336) red lanes are again all unrelated flake, none touching this diff (
HTTP-server lanes are green. The four claude[bot] review findings are addressed and all review threads resolved. Re-roll already spent, so not pushing another empty commit. Ready for a maintainer to merge. |
|
Heads up: #33394 touches the same function from the other direction and likely supersedes this PR. The report that prompted it is that This PR preserves If that holds up in review, this one can be closed in its favour. |
|
Closing in favour of #35839. Status of the two halves of this PR on current
The msal-node teardown scenario from #30501 still reproduces on |
1 participant
What
@azure/msal-node'sLoopbackClient.closeServer()(the code that tears down the localhost server after an interactive token flow) calls:Under Bun, this sequence left the process hanging because:
Server.prototype.closesynchronously nulled outthis[serverSymbol](the reference to the underlyingBun.servehandle). The subsequentcloseAllConnections()/unref()then sawundefinedand early-returned. The 'force-close every socket' step msal relies on never reached the native layer.closeAllConnections()could reachserver.stop(true),stopFromJSgated onhasListener(), whichstop(false)(the graceful close) had already cleared. AndstopListeningbailed out viathis.listener orelse returnbefore it could runthis.app.?.close()— the call that force-closes open uWS sockets.Result: the keep-alive connection from the browser tab stayed open, pinning the loop (macOS hung until the user closed the tab; Windows hung indefinitely).
Fix
src/js/node/_http_server.tsthis[serverSymbol]populated pastclose(); clear it only inemitCloseNTServer(fires when the allClosed promise fulfills).kServerClosedas the 'has close been called' flag soaddress()still returns null and double-close()still errors withERR_SERVER_NOT_RUNNING, matching Node.kServerClosedinkRealListenso re-listen works.src/runtime/server/server.zigstopFromJSanddisposeFromJS, allow the abrupt stop to proceed when the listener was already nulled by a prior graceful stop — the app may still own open connections worth force-closing.stopListening, runthis.app.?.close()on the abrupt path even whenthis.listeneris null (graceful-then-abrupt sequence).terminatedremains the single-shot guard.Test
Two tests in
test/js/node/http/node-http.test.ts:closeAllConnections() after close() force-closes in-flight sockets— opens a keep-alive TCP connection, lets the server receive the request but never reply (so the socket is in-flight, not idle), runs the msal teardown sequence, and waits for'close'on the client socket.process exits after close()+closeAllConnections()+unref() teardown— spawns a subprocess that does the same flow end-to-end and checks the subprocess exits promptly instead of hanging on the keep-alive timer.Both fail on main (timeout) and pass with the fix.
Fixes #30501