Repository navigation
Conversation
After a graceful stop() (or node:http server.close()) the drained NewServer is only freed once its JS wrapper is finalized. When that happens during VM teardown, finalize() has to free it inline because schedule_deinit() cannot enqueue anything any more, but it only did so when TERMINATED was set, which only an abrupt stop sets. A gracefully stopped server still referenced at exit therefore leaked its Box, uws app and routers (158 allocations; LeakSanitizer hides them behind the uws_create_app suppression at the cost of a symbolizer run per child on the ASAN lane, and a Worker leaks them for real). finalize() now frees every server that schedule_deinit() found drained during shutdown, and deinit() closes the app before destroying it when no stop closed it already. The teardown's stop phase has closed every socket before finalizers run, so that close is a no-op walk there; it also covers a listen() that fails after binding the TCP listener.
|
Status: reproduced with the script in the description (debug ASAN build, |
|
Updated 8:55 AM PT - Aug 11th, 2026
✅ @robobun, your commit f7bc94e8db8160fd39ba79e62c8f309f7f3d2863 passed in 🧪 To try this PR locally: bunx bun-pr 37500That installs a local version of the PR into your bun-37500 --bun |
WalkthroughThe PR updates ChangesServer shutdown cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…s reported instead of hanging
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; the earlier nit about wiring the WebSocket close event in the test fixture was addressed in d4f46aa. Because the fix removes the TERMINATED gate on the synchronous deinit() in a JSC finalizer and adds an app.close() inside deinit() — correctness resting on the teardown ordering between close_all_socket_groups and lastChanceToFinalize — a human look at the server-lifecycle invariants would still be worthwhile.
What was reviewed:
- Traced
finalize()→deinit_if_we_can()→schedule_deinit()'sis_shutting_down()early-return to confirmDEINIT_SCHEDULEDhere means "drained, nothing enqueued". - Confirmed
close_all_socket_groupsruns inVirtualMachine::teardownbefore JSC heap destruction, so groups are empty whenlastChanceToFinalizeruns finalizers. - Checked that
deinit_running.set(true)in the newdeinit()branch guards re-entrance fromapp.close()callbacks (matches the pattern instop_listening). - Tests: four LSan children run against
leaksan.suppminusuws_create_appso the leak is reportable;test.concurrent.each,tempDir, pipes drained concurrently.
Extended reasoning...
Overview
Two Rust changes in the Bun.serve server lifecycle plus a test-file refactor:
src/runtime/server/server_body.rs(finalize()): drops the&& flags.contains(ServerFlags::TERMINATED)condition on the inlinedeinit()call, so a gracefully-stopped server whose wrapper is finalized during VM shutdown is freed rather than leaked.src/runtime/server/mod.rs(deinit()): ifTERMINATEDis not yet set when the app is destroyed, sets it, sets thedeinit_runningre-entrance guard, and callsapp.close()beforeNewApp::destroy. Thedeinit_runningfield doc comment is updated to namedeinitalongsidestop_listening.test/js/bun/http/bun-serve-html-405.test.ts: extracts anexitUnderLeakSanitizerhelper, keeps the two existing teardown tests, and adds adescribewith four scenarios (Bun.serve stop after/before a request, node:http close, Bun.serve stop with a live websocket atprocess.exit) each run against a suppression file withleak:uws_create_appremoved.
Security risks
None identified. No user-input parsing, auth, crypto, or network-protocol changes. The change is confined to VM-teardown-time and listen-failure-time cleanup of an already-owned uWS app handle.
Level of scrutiny
High. This is the memory-safety category REVIEW.md flags as most-blocked: it removes a guard on a synchronous free inside a JSC finalizer, and its correctness depends on a non-local ordering invariant — that VirtualMachine::teardown's stop phase (close_all_socket_groups, plus abruptly stopping every registered server) has emptied every socket group before lastChanceToFinalize runs. I verified that ordering in src/jsc/VirtualMachine.rs:1919 and src/jsc/rare_data.rs:860-866, and the PR description enumerates the paths (abrupt-stop, graceful-stop-then-teardown, websocket-outliving-stop, in-flight-request-at-exit). The new app.close() in deinit() can in principle fire close callbacks that re-enter via AnyServer dispatch; the added deinit_running.set(true) blocks that, matching the existing pattern in stop_listening and deinit_if_we_can. But because a mistake here is a UAF or an orphaned socket rather than a visible error, and the invariant spans several files, a maintainer familiar with the server lifecycle should confirm the reasoning.
Other factors
- The tests are well-constructed for this class of fix: they run under LSan with the masking suppression removed, so they fail on the unfixed build (per the PR: 158/173 allocations) and would catch a regression. They use
tempDir,test.concurrent.each, drain stdout/stderr/exited concurrently, andskipIf(!isASAN || isWindows). - All prior feedback is addressed and resolved: my WebSocket-open nit (d4f46aa), and the comment-cop bot's length complaints (4d8f2c6, 9164664 — the SAFETY block in
finalize()is now the same length as on main, and thedeinit()comment is one line). - The non-shutdown path is unchanged (
is_shutting_down()still gates the inline free), so the blast radius of a mistake is limited to process/worker exit and the http3-bind-failure listen path.
Covers the listen() failure path that now closes the app in deinit(): on the unfixed debug build the child aborts in us_socket_group_deinit with the listener still linked, and a release build keeps the port bound.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; the earlier nit about wiring ws.onclose was addressed in d4f46aa. Because the fix removes a safety gate in the server's GC finalizer and adds a synchronous app.close() inside deinit() — both memory-lifecycle paths whose correctness rests on VM-teardown ordering — a human look would still be worthwhile.
Checked: schedule_deinit() only sets DEINIT_SCHEDULED after deinit_if_we_can() has verified the server is fully drained and js_value is Finalized, so removing the TERMINATED gate does not admit a non-drained server.
Checked: the new close() in deinit() runs after heap::take(this) reclaims the Box; re-entry via socket-close callbacks would alias the unique borrow, but on both callers (VM teardown after close_all_socket_groups, http3 UDP bind failure) the groups hold no connected sockets, and deinit_running is set defensively.
Checked: the http3 test's retry loop only retries on EADDRINUSE from the TCP bind, and the leak tests strip leak:uws_create_app so the cycle is actually reportable.
Extended reasoning...
Overview
The PR fixes a leak where a gracefully stopped Bun.serve (or node:http) server whose JS wrapper survives to lastChanceToFinalize leaks its entire NewServer Box + uws app graph. Two native changes: server_body.rs:2823 drops the && TERMINATED gate on the inline deinit() call in finalize(), and mod.rs:2119-2126 makes deinit() call app.close() before NewApp::destroy when nothing has closed the app yet. Tests: four new ASAN/LSan child-process scenarios in bun-serve-html-405.test.ts (run against leaksan.supp minus uws_create_app so the leak is reportable), plus an http3 UDP-bind-failure test that proves the TCP listener is released.
Security risks
None. This is teardown-time resource cleanup; no user input reaches the changed code, and no auth/crypto/permissions are involved.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked category, and this change removes a gate whose stated purpose was to prevent NewApp::destroy from orphaning keep-alive sockets. The argument that the gate is now redundant depends on non-local invariants: (a) VirtualMachine::teardown runs a stop phase + close_all_socket_groups before JSC finalizers, so by lastChanceToFinalize every group is empty; (b) DEINIT_SCHEDULED at that point implies the server was already drained (traced: deinit_if_we_can → js_value == Finalized → schedule_deinit sets the flag and returns early past is_shutting_down()). The reasoning is sound and the PR description is unusually thorough, but a maintainer who owns the teardown ordering (#36750, close_all_socket_groups) should confirm it.
The deinit() change also introduces an app.close() after heap::take(this) has formed the unique Box<Self>. If close() fired a socket-close callback that dispatched back through an AnyServer raw pointer, that would alias the Box's unique borrow. On the two paths that reach this branch (VM teardown with emptied groups; listen-failure with only a listen socket linked) no such callback fires, and deinit_running.set(true) is set to short-circuit deinit_if_we_can if one ever did — but the invariant is worth a maintainer's eye.
Other factors
- Tests are well-constructed: verified to fail on the unfixed build (158/173 allocations),
test.concurrentfor the four independent children, subprocess pipes drained viaPromise.all, WebSocket wait now rejects onclose,tempDir+usingfor cleanup, and the http3 test retries on TCP-sideEADDRINUSEto avoid port-collision flake. - All prior review threads (my WebSocket nit, the comment-cop notices) are resolved; the SAFETY comment in
finalize()is the same length as on main with theTERMINATEDsentence swapped for the teardown-ordering fact. - The non-shutdown
finalize()path is unchanged (is_shutting_down()still gates the inline free), so runtime behavior outside VM teardown and the http3 listen-failure path is unaffected.
…ox in deinit() The close callbacks of any socket still linked re-derive &Self from the same pointer, so do the close before a Box<Self> exists rather than while holding one.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier WebSocket nit was addressed in d4f46aa). Because the fix removes a safety gate in finalize() and adds an unsafe close-before-destroy step in NewServer::deinit() whose correctness depends on non-local teardown ordering (the VM stop phase having emptied every socket group before lastChanceToFinalize), a human look at the lifecycle argument would be worthwhile.
Checked: the new app.close() runs through the raw *mut Self before the Box is reclaimed and sets deinit_running so re-entrant close callbacks short-circuit; the finalize() change is still gated on DEINIT_SCHEDULED && is_shutting_down(), so the non-shutdown path is unchanged. The four LSan children strip leak:uws_create_app from the suppression list so the leak is actually reportable, and the http3 UDP-bind-failure test covers the other new caller of the close path.
Extended reasoning...
Overview
The PR fixes a leak where a gracefully-stopped Bun.serve (or node:http) server whose JS wrapper survives to lastChanceToFinalize was never freed: finalize() only freed the Box inline when TERMINATED was set, and only an abrupt stop sets that flag. The native change is two hunks — dropping the TERMINATED condition in server_body.rs:finalize(), and adding a close-before-destroy step in mod.rs:deinit() for apps that were never closed (the graceful-stop-at-teardown path and the http3 UDP-bind-failure path). Tests add four LSan children run against leaksan.supp minus uws_create_app, plus an http3 test that exercises the listen-failure teardown.
Security risks
None identified. This is teardown-time resource release; no new user-controlled input reaches the changed code, and the change is confined to VM shutdown and a listen-failure error path.
Level of scrutiny
High. This is unsafe native code in Bun.serve's finalize/deinit path — REVIEW.md's most-blocked category. The correctness argument is non-local: it relies on VirtualMachine::teardown's stop phase (close_all_socket_groups) having emptied every socket group before JSC finalizers run, on deinit_running correctly short-circuiting any close-callback re-entry into deinit_if_we_can, and on the DEINIT_SCHEDULED-during-shutdown flag meaning exactly "drained and nothing enqueued". The PR description traces each of these carefully and the ASAN tests (including the websocket-still-open scenario) exercise the debug us_socket_group_deinit assertions, but a maintainer who owns the server lifecycle should confirm the invariant chain.
Other factors
The prior CI run (build 92266 on 9164664) was reported green on every lane; the head commit f7bc94e only reorders the app.close() ahead of the Box reclaim (a safety improvement) and CI is running. All prior review threads (my WebSocket nit and the comment-cop bot's comment-length flags) are resolved. Test coverage is thorough — each new child was verified to fail on the unfixed build — but the change removes a gate that was originally added for a reason, so human sign-off on the replacement reasoning is appropriate.
|
Closing this PR because it has been inactive for more than 90 days. |
Repro
Everything is "indirect" because the app's filter closure points back at the
NewServerBox: the whole server graph (Box,TemplatedApp,HttpContext, ~36 router nodes, config strings) is an unreachable cycle.server.stop(true)instead: nothing reported. A node:http server afterserver.close(): the same, 173 allocations. Whether a request was served makes no difference; what matters is that the server's JS object is still alive when the VM is torn down (without theglobalThis.keepline an ordinary GC sometimes collects it first, and that path is clean).On the ASAN CI lane this never fails a test, because
leak:uws_create_appintest/leaksan.suppmatches the app and LSan also suppresses everything reachable from a suppressed block. It does cost time: to match a suppression LSan has to run llvm-symbolizer over the binary, about 7s per child with a local debug build and about 15s with the lane's release binary, and every test child that stops a server gracefully (or closes a node:http server) and still references it at exit pays that.test/regression/issue/12042.test.tstook 16s on the asan lane and 0.03s everywhere else for this reason (#37479 works around it in that test);test/js/node/http/node-http-server-socket-end-drain.test.tswas another one. A server that is gracefully stopped inside a Worker leaks the same graph for real every time the worker exits, since worker teardown runs the same code.Cause
After a graceful
stop()the server downgrades its wrapper ref and waits for the wrapper'sfinalize()to free the Box. #36750 madefinalize()free it synchronously when it runs during VM teardown (no event-loop tick will ever run the tasksschedule_deinit()would enqueue, soschedule_deinit()just setsDEINIT_SCHEDULEDand returns pastis_shutting_down()), but gated that onTERMINATED. Only an abrupt stop setsTERMINATED; the gate was there because, at the time, a graceful stop left keep-alive sockets in the app's socket group andNewApp::destroywould have orphaned them. So for a gracefully stopped server finalized bylastChanceToFinalize,schedule_deinit()sets the flag,finalize()declines to free, and nothing else ever will.Fix
The fixing line is the removal of the
TERMINATEDcondition inNewServer::finalize()(server_body.rs): the Box is freed wheneverDEINIT_SCHEDULEDwas set during shutdown. That flag can only be set by thedeinit_if_we_can()call insidefinalize()itself (its only caller requires theFinalizedstate thatfinalize()has just set), so it means precisely "drained, and nothing was enqueued to free it".NewServer::deinit()(mod.rs) additionally closes the app before destroying it when nothing has closed it yet. The close happens through the raw*mut Self, before the Box is reclaimed, because the close callback of any socket that is still linked re-derives&Selffrom that same pointer (thedeinit_runningguard keeps such a callback from re-enteringdeinit_if_we_can, as instop_listening).NewApp::destroyonly deinits the socket groups (us_socket_group_deinitunlinks and, in debug builds, asserts they are empty); every existing caller reached it with the app closed because every path that setsTERMINATEDcloses it (the abrupt stop synchronously,schedule_deinitin the task queued ahead of the deinit task), and that is still the case, so the new branch is only taken by the teardown path above and by alisten()that fails after binding its TCP listener (the http3 UDP bind failure), which used to destroy the app with the listener still linked.Why destroying a gracefully stopped app at teardown is safe now:
VirtualMachine::teardownruns a stop phase before the JSC heap is destroyed that stops every still-registered server abruptly and closes every connected socket in every linked group (close_all_socket_groups), and a gracefulstop()itself now closes idle keep-alives. By the timelastChanceToFinalizereaches a server wrapper its groups are empty, sodeinit()'sclose()is a walk over empty lists anddestroyhas its precondition; the websocket scenario below is the case where a connection outlivesstop()and is closed by the sweep, and the debug build's assertions inus_socket_group_deinitrun under these tests. The Drop chain that runs is the one the abrupt path has been running at teardown since #36750. The non-shutdown path is unchanged.I also checked the other ways a server could be drained only at teardown: the wrapper ref stays
Stronguntil the server is drained, so a wrapper cannot be finalized by an ordinary GC while connections remain, and a request still in flight atprocess.exit()is released by the stop phase's abrupt close before finalizers run (tried it; clean). So after this every server that reaches teardown is freed by its finalizer.Verification
test/js/bun/http/bun-serve-html-405.test.ts, next to the teardown tests from #36750. Four new children, each run againstleaksan.suppminus theuws_create_appentry so the cycle is reportable, each creating its server after a timer tick (aBun.serve()made synchronously from the module body hasJSModuleLoader::evaluateNonVirtual, also suppressed, on every allocation's stack) and keeping it onglobalThis:Bun.servestop()after serving a requestBun.servestop()before any connectionserver.close()Bun.servestop()with a websocket still open atprocess.exit()(drained by the teardown sweep, freed by the finalizer)All four fail on the unfixed build with the reports above (158 and 173 allocations) and pass with this change; the two existing teardown tests still pass, and the unsuppressed graceful one drops from 7.2s to 0.4s locally since its child no longer needs the symbolizer. The children of
12042.test.tsandnode-http-server-socket-end-drain.test.tsno longer use any suppression under the lane's environment; on this PR's asan lane (build 92266) those two files take 0.23s and 0.65s, against 16.0s and 15.0s in builds 91926/91993 on main.websocket-server.test.ts,websocket-server-stop-retained-ws.test.ts,websocket-server-reload-leak.test.ts,server-url-invalid.test.tsandnode-http-server-abort-events.test.tsbehave the same with and without the change here (thewebsocket-server.test.tstimeouts I saw under the debug ASAN build reproduce on the unfixed build in the same container; the change cannot run while a test is in progress, it only touches teardown and the listen-failure path).test/js/bun/http/serve-http3.test.tsgets a validation test for the other path that now reaches the close indeinit(): a child occupies a UDP port, asks forhttp3: trueon that port number (TCP binds, UDP fails, no retry for a fixed port), then binds plain TCP on it. With this changeBun.servethrowsFailed to listen on UDP port N for HTTP/3and the TCP bind succeeds; on the unfixed debug build the child dies with(a release build would instead keep the port bound by a listen socket whose group has been freed).
Also run by hand under the same LSan setup with the app suppressions removed, each stopped gracefully and still referenced at exit: a TLS server with
serverName, anhttp3server with and withouthttp1, a node:http server closed while a request was still in flight, adevelopmentserver with an HTML route, and a websocket server inside a Worker (exits with the worker's loop still alive, as in the process case). All of them are freed; the only things left in those reports are two unrelated leaks this turned up, tracked separately: the per-thread RequestContext pool a Worker never frees, and theHTMLBundlecreated by importing an.htmlfile.Not changed:
leak:uws_create_appstays inleaksan.supp; removing it is a separate question once the lane shows nothing else hides behind it.