Repository navigation
Conversation
WalkthroughChangesWorker shutdown now preserves termination exceptions, request handlers stop after termination-related exceptions, and regression tests cover streaming, asynchronous, and idle worker termination. Worker termination handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:59 AM PT - Jul 30th, 2026
✅ @robobun, your commit 2183b9d3d41e5bab34305cbf9c914add2f50610c passed in 🧪 To try this PR locally: bunx bun-pr 36342That installs a local version of the PR into your bun-36342 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Beyond the inline nit, I verified the four earlier findings are addressed (clear_termination_exception now runs after on_exit; both new tests gate on isDebug || isASAN; on_response has the sibling guard; the dead clear_has_termination_request wrapper + FFI shim are deleted). I also checked ref balance on the two new early returns: in on_response the bare return matches the adjacent is_aborted_or_ended() path and ctx.ref_() hasn't run yet; in do_render_stream this.ref_() is at line 2178 (after the guard), and set_has_marked_pending only suppresses the caller's auto-render (RequestContext.rs:793/974) — no extra ref is expected.
Extended reasoning...
All four issues from the previous review pass were addressed in 4dafa97/97822300. This run's only new finding is a nit about the third test's comment overstating what ordering it guards. I additionally traced the two new has_exception() early-return sites for ref-count imbalance and confirmed neither runs after a ref_(); the has_marked_pending flag set in the do_render_stream guard is read only as a suppress-auto-render gate, not as a ref-held indicator. Deferring rather than approving because the change sits at the intersection of JSC VMTraps/TerminationException state and Bun.serve request-context teardown, which warrants a maintainer look.
There was a problem hiding this comment.
All four earlier concerns are addressed (clear-after-on_exit ordering, on_response sibling guard, dead FFI shim removed, load-bearing mid-JS-terminate exit-handler check via sab[1]). No new issues found.
Checked: the on_response early return sits alongside the existing is_aborted_or_ended() bare-return, so no cleanup is skipped; do_render_stream's guard mirrors the Pending arm's on_first_write/ctx/has_marked_pending handling; clear_termination_exception() is placed after on_exit() so dispatchExitInternal's guard still trips, and before close_all_socket_groups so on_close callbacks can re-enter JS.
Deferring to a human given this touches the JSC termination-exception state machine and worker-shutdown ordering.
Extended reasoning...
Overview
Fixes a debug/ASAN-only SIGABRT when worker.terminate() lands mid-drain_microtasks() while Bun.serve is streaming a type:"direct" body. Two has_exception() early-returns are added in RequestContext (do_render_stream, on_response) after their drain_microtasks() calls; WebWorker::shutdown now calls clear_termination_exception() after on_exit() (was clear_has_termination_request() before it, which was a no-op on the flag and left the sticky exception for close_all_socket_groups to trip on). The dead VM::clear_has_termination_request wrapper and its JSC__VM__clearHasTerminationRequest FFI shim are removed. Three regression tests added.
Security risks
None. No user-controlled input parsing, auth, or crypto is touched. The change only affects control flow when a worker VM is already terminating.
Level of scrutiny
High. The change is small in line count but sits at the intersection of the JSC VMTraps/TerminationException state machine, cross-thread worker shutdown ordering, and HTTP request-context lifecycle. Getting the clear_termination_exception() placement wrong changes observable Node-compat behavior (whether process.on('exit') fires on terminate()), and getting the RequestContext early-return cleanup wrong could leak per-request state. Both were the subject of earlier review rounds and are now covered by load-bearing tests, but a maintainer familiar with the VMEntryScope/executeEntryScopeServicesOnExit/DeferTermination interaction should confirm the state-machine reasoning.
Other factors
All four inline findings from my earlier passes were addressed across commits 4dafa97, 9782230, f56cd1c, and 6717d6b. The author verified the mid-JS-terminate process.on('exit') test is load-bearing by locally swapping the clear before on_exit() and observing the test fail. The PR description's verification section reports the new test failing 5/5 on main and passing 5/5 with the fix, plus 200+ passing tests across the affected serve/stream/worker suites. The related PR #36331 covers the non-direct stream sibling. Comment-cop flags are all resolved.
|
CI on 65d6b7d (build 85636, post-rebase): debian-13 x64-asan lane, where the new |
…ct" body A Worker running Bun.serve() that streams a type:"direct" ReadableStream body, terminated mid-stream, SIGABRTed debug/ASAN builds with ASSERTION FAILED: vm.hasTerminationRequest() VMTraps.cpp(539) : void JSC::VMTraps::deferTerminationSlow(DeferAction) do_render_stream drives the async pull() loop via drain_microtasks(). When the termination trap fires inside it, the outermost VMEntryScope's executeEntryScopeServicesOnExit clears hasTerminationRequest while the TerminationException stays pending. do_render_stream then kept going and lazy-initialized NativePromiseContextStructure; LazyProperty::callFunc opens a DeferTerminationForAWhile scope unconditionally, whose deferTerminationSlow asserts hasTerminationRequest() whenever a termination exception is pending. WebWorker::shutdown had the same inconsistency: it cleared only the request flag before on_exit / close_all_socket_groups, leaving the sticky exception in place. Fix: - do_render_stream: bail out after drain_microtasks() when an exception is pending instead of re-entering JS; worker shutdown tears down the sink. - drainMicrotasks: re-set hasTerminationRequest before returning the termination signal so the DeferTermination invariant holds for any caller that proceeds to touch a LazyProperty. - WebWorker::shutdown: clear the pending TerminationException along with the request flag so on_exit handlers and close_all_socket_groups can re-enter JS.
…drainMicrotasks change, cover on_response sibling, delete dead clear_has_termination_request
6717d6b to
65d6b7d
Compare
…tasks -> then2 shape)
There was a problem hiding this comment.
No new findings on 2183b9d — all four prior review threads (on_response/on_node_http_request siblings, dead clear_has_termination_request wrapper, clear_termination_exception ordering vs on_exit, load-bearing exit-handler test) are addressed. Deferring to a human because the change sits at the intersection of JSC termination invariants (VMTraps/DeferTermination), worker shutdown ordering, and HTTP request-context resource release across three early-return sites.
What was reviewed:
- The three new
has_exception()guards return before anythen2/NativePromiseContext::create, and each releases what was acquired above (strong_promise.deinit(), sinkon_first_write/ctxcleared). WebWorker::shutdownnow runson_exit()before clearing the termination exception, sodispatchExitInternalstill skipsprocess.on('exit')on both the mid-JS and idle terminate paths; the async-fetch test'ssab[1]counter locks that ordering.clear_has_termination_requestand its FFI shim have no remaining callers.
Extended reasoning...
Overview
Fixes a debug/ASAN-only SIGABRT (ASSERT(vm.hasTerminationRequest()) in VMTraps::deferTerminationSlow, and assertNoException in executeCallImpl) when a Worker running Bun.serve is terminated mid-stream/mid-drain. Touches src/jsc/web_worker.rs (shutdown ordering), src/runtime/server/RequestContext.rs (do_render_stream, on_response), src/runtime/server/mod.rs (on_node_http_request_with_upgrade_ctx), deletes the now-dead VM::clear_has_termination_request + its FFI shim, and adds four test cells to worker-terminate-lifetime.test.ts.
Security risks
None identified. This is a lifecycle/assertion fix; no user-input parsing, auth, or crypto is touched.
Level of scrutiny
High. The change reasons about JSC's termination state machine (trap bit vs hasTerminationRequest flag vs the sticky TerminationException), reorders worker shutdown so dispatchExitInternal still trips on terminate (Node compat), and inserts three early-return paths into HTTP request handling — each of which must not leak refs or leave the request context in a state that survives close_all_socket_groups. The do_render_stream guard clears on_first_write/ctx and sets has_marked_pending, mirroring the Pending arm's shape; the on_response guard sits before any state mutation (same position as the existing is_aborted_or_ended() bail); the mod.rs guard deinit()s strong_promise and returns Success, which falls through to the abort-handler wiring without re-entering JS (needs_to_drain was already flipped false). These look consistent, but a maintainer familiar with the request-context refcount model should confirm the do_render_stream early return leaves the sink in a state that shutdown's socket-group teardown fully reclaims.
Other factors
This PR went through three review iterations; every prior finding (sibling coverage in on_response and on_node_http_request_with_upgrade_ctx, dead-code deletion, skipIf(!(isDebug || isASAN)) gate, the clear_termination_exception-before-on_exit regression, and the non-load-bearing idle test) has been addressed with a corresponding commit and test. CI on the prior commit was green on the release-asan lane. The remaining reason not to auto-approve is domain complexity, not any concrete unresolved concern.
What
A Worker running
Bun.serve()that streams atype:"direct"ReadableStreambody, terminated mid-stream, SIGABRTs debug/ASAN builds with:Release builds are unaffected (the
ASSERTis compiled out). Deterministic on main, first iteration every run. The same assert also fires for an asyncfetch()handler that spins onawaitwhen terminated mid-drain.Repro
Cause
do_render_streamcallsassign_to_stream(which kicks off the direct stream's asyncpull), thendrain_microtasks()to drive theawait ctrl.flush()continuations. Whenterminate()lands cross-thread during that drain:VMTraps::handleTrapssetshasTerminationRequestand throws theTerminationException.VMEntryScopedestructor runsexecuteEntryScopeServicesOnExit(), which clearshasTerminationRequest(theNeedTerminationtrap bit has been consumed) but leavesm_exceptionpointing at the termination exception.GlobalObject::drainMicrotasksobserves the pending termination and returns1;RequestContext::drain_microtasksdiscards that.do_render_streamcontinues into the Pending branch and callsNativePromiseContext::create. That is the first touch ofm_NativePromiseContextStructure, soLazyProperty::callFuncruns and opens aDeferTerminationForAWhilescope.VMTraps::deferTerminationSlowseeshasPendingTerminationException()true and assertshasTerminationRequest(), which is now false.With the lazy-init assert avoided, the very next step (
then_with_value→JSC::profiledCall) tripsexecuteCallImpl'sscope.assertNoException()for the same reason.on_responsehas the identicaldrain_microtasks()→NativePromiseContext::create→then_with_valuesequence for an asyncfetch()handler.Separately,
WebWorker::shutdowncalledclear_has_termination_request()beforeon_exit(), which was a no-op in practice (thedispatchExitInternaltermination guard still tripped on the pending exception, matching Node's "skipprocess.on('exit')on terminate"), and left the sticky exception in place forclose_all_socket_groups' on_close callbacks to re-enter JS against.Fix
RequestContext::do_render_stream,RequestContext::on_response, andon_node_http_request_with_upgrade_ctx(the node:http-compat twin): afterdrain_microtasks(), if an exception is pending (only theTerminationExceptionsurvives the drain), return instead of continuing into paths that re-enter JS. Worker shutdown'sclose_all_socket_groupsaborts the response and tears the sink down.WebWorker::shutdown: drop the no-opclear_has_termination_request()beforeon_exit()and instead callclear_termination_exception()(clears both the flag and the pending exception) afteron_exit(), sodispatchExitInternalstill skipsprocess.on('exit')on terminate (Node semantics) whileclose_all_socket_groups' on_close callbacks can re-enter JS.VM::clear_has_termination_requestwrapper and itsJSC__VM__clearHasTerminationRequestFFI shim.#36331 fixes the sibling
assertNoExceptionface for the non-direct stream path at theHTTP(S)ResponseSink__onClose/__onReady/detachsites; this PR covers thetype:"direct", async-handler, and node:http-compat paths atdo_render_stream/on_response/on_node_http_request_with_upgrade_ctxand at shutdown.Verification
type:"direct"cell inworker-terminate-lifetime.test.ts: fails on main with thedeferTerminationSlowassert (first iteration, 5/5), passes with the fix (5/5).fetch()cell: same assert on main, passes with the fix; also assertsprocess.on('exit')is skipped (sab[1] stays 0), which fails ifclear_termination_exception()is moved back beforeon_exit().process.on('exit')is skipped whenterminate()lands while the worker is parked (Node compat; the trap-bit path).test/js/bun/http/serve-direct-readable-stream.test.ts: 13 pass.serve-stream-body-error.test.ts,serve-error-handler-stream.test.ts,async-iterator-stream.test.ts,serve-async-stream-client-abort.test.ts: 121 pass.test/js/node/worker_threads/worker_threads.test.ts: 91 pass.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