Bun.serve: close the listen socket in finalize() so worker terminate releases the port - #36094
Bun.serve: close the listen socket in finalize() so worker terminate releases the port#36094robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Reproduced on stock 1.4.0: 5 serve-in-worker → terminate cycles leak 5 listen fds and every port stays Test covers Re-verified 2026-07-29 after rebase onto main@59242d6:
CI (build 84783): new test passes on all lanes. Remaining red is unrelated:
Ready for review. #36097 takes a different approach to the same bug: register every server in the existing per-VM |
|
Superseded by #36097, which runs |
|
Updated 11:07 PM PT - Jul 28th, 2026
❌ @robobun, your commit 3237ab8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36094That installs a local version of the PR into your bun-36094 --bun |
There was a problem hiding this comment.
All four earlier findings (H3 close re-entering JS, unix-socket unlink, exit-mode variant coverage, fd-delta poll race + slow-build scaling) are addressed and I found nothing new, but this is GC-finalizer + worker-VM-teardown code where the correctness argument hinges on lastChanceToFinalize ordering and us_listen_socket_close being JS-free — worth a human pass, and there's the open question of whether #36097's registry-walk approach supersedes this.
What was reviewed:
NewServer::finalizenow closes only the TCP listener (JS-free) and nullsh3_listenerwithout closing — tracedus_quic_listen_socket_close→on_abortto confirm the asymmetry is required.deinit_if_we_can()reachability fromlastChanceToFinalizeafterhas_listener()goes false: theall_closed_promiseenqueue is gated on!vm.is_shutting_down(), andschedule_deinit()'senqueue_taskcalls land afterrelease_queued_tasks_for_shutdown— verifiers concluded this doesn't crash.- Test:
test.eachcovers all three exit modes; port-await promise not wiring workererror→ reject was checked and ruled out (worker body is trivial, no error path).
Extended reasoning...
Overview
Three files: NewServer::finalize() in src/runtime/server/server_body.rs gains a TCP listener close + unix-path unlink + h3_listener = None before dispatching to deinit_if_we_can(); src/jsc/web_worker.rs has a one-word doc-comment update naming NewServer alongside Listener in the drain_closed_sockets invariant; and test/js/web/workers/worker-terminate-lifetime.test.ts adds a test.each over three worker-exit modes that spawns 5 serve-in-worker cycles, rebinds each port, and (on Linux) polls /proc/self/fd with a slow-build-scaled deadline.
Security risks
None identified. The change closes a resource leak; no new attack surface, no auth/crypto/permissions.
Level of scrutiny
High. finalize() runs inside JSC's lastChanceToFinalize() heap sweep during worker VM teardown — the exact context REVIEW.md's memory-safety section is written about. The PR itself already went through one round where an earlier revision would have re-entered JS from the H3 close path during the sweep; the current revision deliberately leaks the H3 listener to avoid that. The soundness of the TCP close depends on the Strong js_value root guaranteeing that a still-listening server can only reach finalize() from VM teardown (never ordinary GC), which I believe is correct but is a load-bearing invariant a maintainer should confirm. The newly-reachable schedule_deinit() path (once has_listener() goes false) enqueues two ManagedTasks onto a VM whose event loop will never tick again; verifiers examined this and concluded it does not crash, but a human should confirm the leaked-task-box tradeoff is acceptable versus #36097's approach of walking the IsolationHandle::Server registry before teardown.
Other factors
- The author's own comment says this is "superseded by #36097" (which stops servers via the per-VM registry in shutdown step 2, before JSC teardown, and also covers accepted-connection fds). Commits continued on this PR afterwards, so it's unclear which approach is intended to land — that's a human call.
- Previous review rounds: I raised four issues across two passes; all were addressed in dba40ef / 18e395f / e0dd43d and the threads are resolved. The comment-cop bot's length nags were also addressed.
- Test coverage looks solid: fails on
mainwithEADDRINUSEon all three exit modes, port-rebind is the primary race-free assertion, fd-delta poll now scales via the file's existingslowfactor.
…releases the port Terminating a Worker that was running Bun.serve() left the listening socket open for the life of the process: the port stayed bound (EADDRINUSE on any rebind), new connections were accepted by the kernel into a socket nothing serviced, and every terminate cycle leaked one fd. WebWorker::shutdown's close_all_socket_groups() deliberately skips listen sockets because the owning object holds a raw pointer to the us_listen_socket_t and closing it there would UAF after drain_closed_sockets(). The design relies on the owner's GC finalize() closing the listener, which the lastChanceToFinalize() sweep reaches during teardownJSCVM. Bun.listen's Listener::finalize() does that; Bun.serve's NewServer::finalize() did not: it called deinit_if_we_can(), which early-returns while has_listener() is true. A still-listening NewServer can only reach finalize() from lastChanceToFinalize(): the Strong js_value root keeps the wrapper alive through every ordinary GC while the listener is set. So close the listener (and h3_listener) synchronously there, mirroring Listener::finalize(). deinit_if_we_can() then sees has_listener() == false and proceeds to schedule_deinit().
…exit-mode variants; poll fdCount us_quic_listen_socket_close walks every live QUIC conn with lsquic_conn_abort + us_quic_process, which synchronously fires on_stream_close -> RequestContext::on_abort (JS abort listeners). That is unsound inside lastChanceToFinalize(); close_all_socket_groups never drained QUIC conns, so they can still be live there. Drop the H3 close (back to pre-PR state for that narrow path) and document why. Unlink the unix-socket path before closing the listener, matching both Listener::finalize and stop_listening. Test: cover terminate() / process.exit() / unref()+drain with test.each, and poll /proc/self/fd with a bounded deadline instead of relying on Bun.gc(true) as an implicit sleep (the close event fires before the detached worker thread closes its epoll/eventfd in on_thread_exit).
07e26b4 to
d4aec73
Compare
Mirrors the guard on the adjacent ServerAllConnectionsClosedTask block: schedule_deinit() enqueues two ManagedTask boxes that no event-loop tick will ever drain once this is reached from lastChanceToFinalize(). EventLoop::deinit() drops them without running the callbacks, so the NewServer/App leaked pre-PR too; this just avoids the transient allocation now that finalize() clears listener/h3_listener and makes the schedule_deinit branch reachable from that path.
There was a problem hiding this comment.
No issues found this pass, but this closes a listen socket from inside a GC finalizer during VM teardown — memory-safety-critical ordering that relies on the Strong js_value root guaranteeing listener.is_some() in finalize() ⇒ lastChanceToFinalize(). That invariant plus the choice between this finalizer-level close and #36097's registry-based stop(true) in shutdown() step 2 are both maintainer calls.
What was reviewed
finalize()block:us_listen_socket_closeis JS-free (poll stop + close + queue), unix-path unlink matchesstop_listening/Listener::finalize,h3_listenerdeliberately left unclosed sinceus_quic_listen_socket_closecan dispatch JSon_abort.deinit_if_we_canis_shutting_down()guard mirrors the adjacentServerAllConnectionsClosedTaskblock; ordinary-GCfinalize()still reachesschedule_deinit().- Test:
test.eachcovers all three exit modes, rebind assertion is race-free (listen fd closed in step 3 pre-dispatchExit), fd-poll deadline scaled via the file's existingslowfactor.
Extended reasoning...
Overview
Four files: server_body.rs adds a listener-close block to NewServer::finalize() (unix unlink + us_listen_socket_close, nulls h3_listener), mod.rs gates schedule_deinit() on !is_shutting_down(), web_worker.rs updates the step-3 comment to name NewServer alongside Listener, and a new test.each regression covers terminate / process.exit() / unref()+drain.
Security risks
None. This is resource-lifecycle cleanup on worker teardown; no auth/crypto/parsing/user-input surface touched.
Level of scrutiny
High. finalize() runs from JSC's GC sweep during lastChanceToFinalize(), and correctness depends on (a) the Strong js_value root making listener.is_some() in finalize() reachable only from VM teardown, (b) us_listen_socket_close never re-entering JS, and (c) WebWorker::shutdown()'s post-teardown drain_closed_sockets() reclaiming what was just queued. All three check out against the current source, but this is exactly the layer REVIEW.md flags as most-blocked ("anything that can run user JS can synchronously free your state", finalizer ordering, refcount balance). The H3 asymmetry (fd deliberately still leaked to avoid JS re-entry) is a documented tradeoff a maintainer should sign off on.
Other factors
- The PR thread itself notes #36097 as an alternative that runs
stop(true)via the per-VMIsolationHandle::Serverregistry inshutdown()step 2 (VM still live, H3 covered, accepted-connection fds covered). "Either lands, not both" — a human needs to pick. - All four prior inline findings (H3 close hazard, unix unlink, exit-mode variant matrix, fd-poll race + deadline scaling) were addressed in follow-up commits and are verified in the current diff.
- The
is_shutting_down()guard onschedule_deinit()is defensive symmetry with the adjacent block, not a behavior change users can observe — checked that ordinary-GCfinalize()(is_shutting_down() == false) still reachesschedule_deinit().
|
Closing: this is fixed on current main by the Worker teardown rewrite in #37075, which stops every registered Bun.serve server during the VM teardown stop phase, so a terminated (or exited) worker no longer leaves its listen socket behind. The three cases this PR adds to test/js/web/workers/worker-terminate-lifetime.test.ts (port rebinds after terminate(), process.exit() and unref() + drain, plus the /proc/self/fd delta check) pass unmodified against a debug build of main at 04148c8, three runs in a row. #36097, which superseded this PR, is being closed for the same reason. |
Repro
After
worker.terminate()the worker'sBun.servelisten socket stays open for the life of the process: the port is permanently bound (EADDRINUSEon any rebind), new clients are accepted by the kernel into a socket nothing services, and every terminate cycle leaks one fd. Same for a worker thatprocess.exit()s or exits naturally with anunref()'d server.Cause
WebWorker::shutdown()'sclose_all_socket_groups()deliberately skips listen sockets (us_loop_close_all_groupspassesalso_listeners = 0) because the owning Rust object holds a raw*mut us_listen_socket_tand closing it there would dangle that pointer oncedrain_closed_sockets()frees the queued struct. The design relies on the owner's GCfinalize()closing the listener, whichlastChanceToFinalize()reaches duringteardownJSCVM:https://github.com/oven-sh/bun/blob/4eb6f99c1a/src/jsc/web_worker.rs#L1327-L1330
Bun.listen'sListener::finalize()honours that contract.Bun.serve'sNewServer::finalize()did not: it calleddeinit_if_we_can(), which early-returns whilehas_listener()is true, so the listen fd, the uWSApp, and theNewServerallocation were all stranded.Fix
Close the listener (and
h3_listener) inNewServer::finalize()before dispatching todeinit_if_we_can(), matchingListener::finalize(). A still-listening server can only reachfinalize()fromlastChanceToFinalize(): whilelisteneris set theStrongjs_valueroot keeps the JS wrapper alive through every ordinary GC, so the only sweep that reaches it with a live listener is VM teardown.us_listen_socket_close()is JS-free (poll stop +close(fd)+ queue ontoloop->data.closed_head), andWebWorker::shutdown()already runsdrain_closed_sockets()right afterteardownJSCVMto free the queued struct.Verification
New test in
test/js/web/workers/worker-terminate-lifetime.test.tsspawns 5 serve-in-worker → terminate cycles and asserts each port can be immediately rebound, plus a/proc/self/fddelta check on Linux. Fails onmainwithEADDRINUSE, passes with this change.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-terminate-lifetime.test.ts