worker: make termination interrupt a worker blocked in Atomics.wait - #32802
worker: make termination interrupt a worker blocked in Atomics.wait#32802robobun wants to merge 2 commits into
Conversation
|
Updated 7:25 AM PT - Aug 9th, 2026
❌ @robobun, your commit f9540be has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32802That installs a local version of the PR into your bun-32802 --bun |
|
Warning Review limit reached
Next review available in: 23 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 (2)
WalkthroughThe VM termination notification now marks the termination request before waking waiters. A regression test was added that spawns a worker blocked in ChangesWorker termination during Atomics.wait
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
StatusFixes #29173 (verified with that issue's exact Tinypool repro: hangs 3/3 without the diff, exits cleanly 3/3 with it). Reproduced on main, deterministically: Fix: one line in CIHead
The diff has nothing left to change; a maintainer can retry the darwin shards or merge. |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Confirmed: this fixes #29173. I ran the exact repro from that issue ( Worth flagging explicitly: there was a prior attempt at #29173, #29179, which was closed unmerged after it introduced two crashes in CI. I dug into its review history to make sure this is not the same mistake twice. Both of its failures came from code that PR added beyond the one-line flag, and both of those parts have since been implemented on main:
I re-ran all three of those, plus the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
About the
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 111-114: The test in worker-terminate-lifetime.test.ts is using a
fixed Atomics.wait(ia, 1, 0, 100) delay to time terminate(), which can miss the
intended blocked-wait state. Replace that timeout-based pause with a second
shared-state handshake in the worker test flow that explicitly signals when the
worker has re-entered Atomics.wait(ia, 0, 0), and only call w.terminate() after
that observable parked-again condition is confirmed. Keep the change localized
to the worker lifetime test logic so the termination assertion proves the
blocked-wait path rather than relying on elapsed time.
🪄 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: 126e2351-30c7-491d-8975-13be9a0f4a20
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/web/workers/worker-terminate-lifetime.test.ts
af521b2 to
4e4d839
Compare
There was a problem hiding this comment.
No issues found on this pass — the last nit (file-level timeout) was applied in 4a16bda93c and all earlier threads are resolved. Deferring final sign-off to a maintainer given this is a cross-thread write to VM termination state on the worker lifecycle path.
What was reviewed:
- The
setHasTerminationRequest()ordering vs.notifyNeedTermination()and theWaiterListManager::waitForSyncloop condition it targets. - Readers of
hasTerminationRequest()across JSC/Bun (napimustDeferFinalizers,node:vmcheckForTermination,VM::executeEntryScopeServicesOnExit) — no new failure mode beyond the already-documented follow-ups. - The regression test's park-proof handshake and file-convention timeout; confirmed it matches the sibling tests' subprocess/assert shape.
Extended reasoning...
Overview
One production line: vm.setHasTerminationRequest() added to JSC__VM__notifyNeedTermination in src/jsc/bindings/bindings.cpp before the existing vm.notifyNeedTermination() trap fire, plus a ~50-line subprocess regression test in test/js/web/workers/worker-terminate-lifetime.test.ts. The helper is the shared entry point for every worker termination path (Web Worker#terminate, worker_threads, in-worker process.exit, process-exit sweep).
Security risks
None. No user-controlled input, no parsing, no auth/crypto surface. The change sets an existing atomic VM flag one call earlier on a path that already fires a cross-thread trap on the same VM.
Level of scrutiny
High. Despite being one line, this is a parent-thread write to worker VM termination state — exactly the class REVIEW.md calls out under "know the thread affinity of every line you touch". The prior attempt at this issue (#29179) was closed after CI crashes, and the PR's own analysis documents two adjacent-but-out-of-scope concerns (napi mustDeferFinalizers window widening; the ~1/3000 deferTerminationSlow assertion class) plus a pre-existing node:vm RELEASE_ASSERT reader. The mechanism is well-argued and matches WebKit's WorkerOrWorkletScriptController::scheduleExecutionTermination pattern, but a maintainer with JSC termination-lifecycle context should confirm the scoping decisions.
Other factors
All prior review threads on this PR are resolved: comment-length nit (af521b2), CodeRabbit's re-park handshake suggestion (withdrawn after fail-before measurements), the CI SIGABRT investigation (370-run local repro + main flakiness baseline established), the napi finalizer note (acknowledged as follow-up in the PR description), and the file-level timeout nit (4a16bda). The bug hunting system found nothing this run. The test follows the file's existing subprocess pattern including the expect(stderr).toBe("") shape used by all sibling tests.
…e ordered VM teardown (#37075) Makes `Worker` / `node:worker_threads` stable rather than experimental: every crash, use-after-free, assertion, leak and hang class around a VM's lifetime and teardown, on all platforms. Missing `worker_threads` API surface (`resourceLimits`, `trackUnmanagedFds`, `moveMessagePortToContext`, …) is out of scope. ### Lifetime model - WebCore's `ActiveDOMObject` / `ScriptExecutionContext` registry is restored, so `Worker`, `MessagePort`, `BroadcastChannel` and `WebSocket` are stopped in a real stop phase before the JSC VM is destroyed instead of from inside `~VM`. `Worker` is split, as upstream, into the script object and a `WorkerMessagingProxy` that owns the parent↔thread relationship. - Worker threads are refcounted and **joined** by their parent (Node's model). A parent tracks its children, stops them in its own stop phase and joins them before its VM goes away, so `terminate()` propagates through nested workers and resolves only once the thread is gone. No `pthread_exit`. - `VirtualMachine::teardown()` is the one ordered sequence for a finished worker and for main-thread exit: exit handlers run, then script is forbidden and everything the VM owns is stopped natively (WebCore objects, servers, listeners, watchers, sockets, dns, sqlite, in-flight `fetch`/S3 requests, `Bun.build` passes waiting on this VM's plugins — as in Node, no `'close'`/`'error'` handler runs after `'exit'`) → timers cancelled, children joined, in-flight off-thread work waited for or released, VM handle closed, queued work released → JSC VM destroyed → loops freed (uSockets; libuv on Windows) → destroy. - Every off-thread completion — thread pool (fs, crypto, zlib, transpiler, `dns.lookup`, shell builtins, `Bun.Archive`, password hashing), the HTTP thread (`fetch`, S3), the bundle thread, child-process waiter, fs watcher threads, napi async work / threadsafe functions, JSC helper threads — reaches a VM only through a per-VM `VmHandle` that teardown closes. Pool work is one typed carrier (`bun_jsc::Job`) whose JS-affine half only the owning thread can touch and teardown releases; work whose storage lives in JS objects or on another thread is counted and waited for; work that can block on an external party is registered so the stop phase aborts it. A late completion is refused and released by its producer instead of touching a dead VM. `EventLoop` has no cross-thread entry points any more. - A worker's "may run script" gate closes the moment its stop is requested — a parent's `terminate()`, its own `process.exit()`, or an uncaught error — not when its thread gets around to tearing down (Node's `can_call_into_js` / `is_stopping`). Every native→JS entry consults it (timer and immediate callbacks, event listeners, socket/server callbacks, pool-job completions, JSC deferred work, N-API), so nothing dispatches into a worker that is being stopped, whichever event source it came from. Promise settlement is the one native→promise boundary and never accepts an empty value: a JS conversion that a termination interrupted becomes "reject with the pending exception", which itself yields to the termination. - The event loop stays fair under producers that outpace it: one turn refills from the concurrent queue a bounded number of times; message drains take a fixed budget per task (a bounded batch per lock acquisition, never a whole-queue hand-back) and continue after the loop has polled; a UDP socket is read a bounded number of batches per readiness event. A worker posting faster than its parent deserializes, or a datagram socket that never runs dry, no longer holds that loop's timers, I/O — or its own pending stop. - Cross-thread costs of the handle are kept off hot paths: its read-mostly state sits on its own cache line away from the counters other threads update, and C++ tests the "may run script" byte inline rather than calling out per callback. ### Behaviour changes (Node parity) - `parentPort` is a real `MessagePort`: `parentPort.close()` ends the worker, `.ref()`/`.unref()` work, `receiveMessageOnPort` returns falsy messages, and parent messages are delivered only after the worker's entry module has run (a preload's un-awaited `import()` does not count as the entry running). - A worker with a pending top-level await starts and receives messages; it exits 13 if the await never settles, and a top-level await rejecting later fails the worker at that moment. - `await worker.terminate()` resolves the exit code (`1` for a running worker); `threadId` stays valid until exit; everything a worker posted before it exited is delivered before `'exit'`/`'close'`; `postMessage()` to a terminated worker is a no-op rather than an error; a rejection that is only a consequence of `terminate()` (a lookup or request cancelled by the stop) is not reported as the worker's `'error'`. - `process.exit()` / worker exit no longer runs microtasks or `nextTick`s queued before it. A worker's own `process.exit()` or uncaught error runs its `'exit'` handlers; a parent `terminate()` does not. `process.exit()` from inside (nested) `node:vm` contexts in a worker unwinds like any exception, and a `node:vm` `timeout` inside a worker no longer leaves the worker unable to run script afterwards. - Workers inside a process that has an IPC channel do not get a `process.send()` of their own over the process's channel fd. - N-API's pure constructors/accessors are callable while an exception is pending (as in Node), so node-addon-api can build the `Error` for a call a termination interrupted instead of aborting the process. - Assigning a non-function to `port.onmessage` releases the keep-alive a handler took. - Servers, listeners, sockets, UDP sockets, watchers and `dns.Resolver`s are closed by the exiting VM rather than left to GC finalizers; sqlite connections a VM opened are checkpointed and closed by that VM's exit; a `Bun.build` whose VM goes away mid-build is cancelled (its plugin requests failed, the pass finished) rather than abandoned or waited on, and one still queued behind other builds is released without waiting for them. - A connect-path DNS lookup (`Bun.connect`, `net`, `WebSocket` to a hostname) is process-wide and outlives the thread that happened to issue it: on macOS a worker exiting mid-lookup no longer answers every other thread's coalesced waiters with an error (and caches it for the TTL). - An addon's external-buffer finalizers run when the Worker that loaded it exits (`napi_create_external_{arraybuffer,buffer}`), as Node's environment teardown finalizes every remaining reference. - Releasing the last keep-alive from an immediate or a late promise reaction (e.g. `port.close()` inside `setImmediate`) is noticed before the loop parks. - Windows: a worker thread closes its loops. Open pipe/tty/process handles and readers mid file-read are closed through their owners in the stop phase, and requests still in flight are drained there — against a live VM that still accepts (and then awaits) the follow-on work a completion may start — before anything is released; sockets over named pipes and TLS-over-duplex sockets join the stop phase; a reader dropped mid-read keeps the buffer its pending read lands in. Two of the crashes were in JavaScriptCore rather than Bun: a `TerminationException` raised while the module loader resolves an import continued into `finishLoadingImportedModule` (fixed in oven-sh/WebKit#391, picked up by the WebKit version bump here). A worker parked in `Atomics.wait` with no timeout still cannot be terminated (#32802); that needs a JSC change and is tracked separately. ### Testing New tests accompany each behaviour fix (worker_threads, Web Worker lifecycle edges — `terminate()` at every phase of dns/fs/build/vm/napi/http work, message ordering and flooding, process exit ordering, sqlite). Seven more upstream `test-worker-*` files are vendored (one of them, the message-port infinite-message-loop test, passes only with these changes) and previously todo/skipped worker-related napi and regression cases run again. LeakSanitizer validation is turned back on for the ~70 worker / MessagePort / BroadcastChannel test files that were exempt. A source lint rejects laundering a `JsResult<JSValue>` into an empty `JSValue`. Main-thread `process.exit()` keeps its current fast path by default; the full main-thread teardown stays behind `BUN_DESTRUCT_VM_ON_EXIT=1`. Workers always tear down fully. ### Known / not in this PR - A worker parked in `Atomics.wait` with no timeout still cannot be terminated (#32802; needs a JSC change). - `worker_threads` message throughput through the real `MessagePort` is ~0.8× the previous ad-hoc path in a flood microbenchmark (round-trip latency and Web `Worker` messaging are unchanged); a follow-up, not a behaviour regression. - Windows: several concurrent connects to `localhost` can leave one connect stuck (pre-existing; reproduces on current releases; DNS-coalescing on the connect path). - A UDP socket whose receive buffer never drains (e.g. one echoing datagrams to itself on a fast machine) keeps its event loop from running anything else, including a worker's own exit; pre-existing loop-fairness issue, most visible on Windows, follow-up. - Memory a burst of concurrent workers used stays resident after they exit (sequential worker churn plateaus; it is the concurrent peak that is not returned to the OS) — allocator thread-exit policy, follow-up. - `Worker` start semantics for a never-settling top-level await, file-stream fairness on a saturated loop, and a few diagnostics-only items found while fuzzing are tracked separately. Fixes #31281 Fixes #30421 Fixes #15964 Fixes #29173 Fixes #34690 Fixes #31880 Fixes #33936 Fixes #32073 Fixes #33313 Fixes #32828 Fixes #11760 Fixes #26501 Fixes #18661 Fixes #15408 Fixes #23102 Fixes #21101 Fixes #13570 Fixes #31224 Fixes #28643 Fixes #37163 Fixes #25860 Likely also addressed (mechanism matches, not verified end-to-end): #34095; #22376 and the other emscripten-pthread reports (#25454, #19453, #29211, #29635) whose glue installs both a `parentPort` listener and `self.onmessage` — the double delivery behind them (#25860) is fixed, the packages themselves were not run; and the `parentPort.on('message').unref()` hang half of #32609 (its `Worker.performance` half is API surface, not addressed here). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…e ordered VM teardown (oven-sh#37075) Makes `Worker` / `node:worker_threads` stable rather than experimental: every crash, use-after-free, assertion, leak and hang class around a VM's lifetime and teardown, on all platforms. Missing `worker_threads` API surface (`resourceLimits`, `trackUnmanagedFds`, `moveMessagePortToContext`, …) is out of scope. - WebCore's `ActiveDOMObject` / `ScriptExecutionContext` registry is restored, so `Worker`, `MessagePort`, `BroadcastChannel` and `WebSocket` are stopped in a real stop phase before the JSC VM is destroyed instead of from inside `~VM`. `Worker` is split, as upstream, into the script object and a `WorkerMessagingProxy` that owns the parent↔thread relationship. - Worker threads are refcounted and **joined** by their parent (Node's model). A parent tracks its children, stops them in its own stop phase and joins them before its VM goes away, so `terminate()` propagates through nested workers and resolves only once the thread is gone. No `pthread_exit`. - `VirtualMachine::teardown()` is the one ordered sequence for a finished worker and for main-thread exit: exit handlers run, then script is forbidden and everything the VM owns is stopped natively (WebCore objects, servers, listeners, watchers, sockets, dns, sqlite, in-flight `fetch`/S3 requests, `Bun.build` passes waiting on this VM's plugins — as in Node, no `'close'`/`'error'` handler runs after `'exit'`) → timers cancelled, children joined, in-flight off-thread work waited for or released, VM handle closed, queued work released → JSC VM destroyed → loops freed (uSockets; libuv on Windows) → destroy. - Every off-thread completion — thread pool (fs, crypto, zlib, transpiler, `dns.lookup`, shell builtins, `Bun.Archive`, password hashing), the HTTP thread (`fetch`, S3), the bundle thread, child-process waiter, fs watcher threads, napi async work / threadsafe functions, JSC helper threads — reaches a VM only through a per-VM `VmHandle` that teardown closes. Pool work is one typed carrier (`bun_jsc::Job`) whose JS-affine half only the owning thread can touch and teardown releases; work whose storage lives in JS objects or on another thread is counted and waited for; work that can block on an external party is registered so the stop phase aborts it. A late completion is refused and released by its producer instead of touching a dead VM. `EventLoop` has no cross-thread entry points any more. - A worker's "may run script" gate closes the moment its stop is requested — a parent's `terminate()`, its own `process.exit()`, or an uncaught error — not when its thread gets around to tearing down (Node's `can_call_into_js` / `is_stopping`). Every native→JS entry consults it (timer and immediate callbacks, event listeners, socket/server callbacks, pool-job completions, JSC deferred work, N-API), so nothing dispatches into a worker that is being stopped, whichever event source it came from. Promise settlement is the one native→promise boundary and never accepts an empty value: a JS conversion that a termination interrupted becomes "reject with the pending exception", which itself yields to the termination. - The event loop stays fair under producers that outpace it: one turn refills from the concurrent queue a bounded number of times; message drains take a fixed budget per task (a bounded batch per lock acquisition, never a whole-queue hand-back) and continue after the loop has polled; a UDP socket is read a bounded number of batches per readiness event. A worker posting faster than its parent deserializes, or a datagram socket that never runs dry, no longer holds that loop's timers, I/O — or its own pending stop. - Cross-thread costs of the handle are kept off hot paths: its read-mostly state sits on its own cache line away from the counters other threads update, and C++ tests the "may run script" byte inline rather than calling out per callback. - `parentPort` is a real `MessagePort`: `parentPort.close()` ends the worker, `.ref()`/`.unref()` work, `receiveMessageOnPort` returns falsy messages, and parent messages are delivered only after the worker's entry module has run (a preload's un-awaited `import()` does not count as the entry running). - A worker with a pending top-level await starts and receives messages; it exits 13 if the await never settles, and a top-level await rejecting later fails the worker at that moment. - `await worker.terminate()` resolves the exit code (`1` for a running worker); `threadId` stays valid until exit; everything a worker posted before it exited is delivered before `'exit'`/`'close'`; `postMessage()` to a terminated worker is a no-op rather than an error; a rejection that is only a consequence of `terminate()` (a lookup or request cancelled by the stop) is not reported as the worker's `'error'`. - `process.exit()` / worker exit no longer runs microtasks or `nextTick`s queued before it. A worker's own `process.exit()` or uncaught error runs its `'exit'` handlers; a parent `terminate()` does not. `process.exit()` from inside (nested) `node:vm` contexts in a worker unwinds like any exception, and a `node:vm` `timeout` inside a worker no longer leaves the worker unable to run script afterwards. - Workers inside a process that has an IPC channel do not get a `process.send()` of their own over the process's channel fd. - N-API's pure constructors/accessors are callable while an exception is pending (as in Node), so node-addon-api can build the `Error` for a call a termination interrupted instead of aborting the process. - Assigning a non-function to `port.onmessage` releases the keep-alive a handler took. - Servers, listeners, sockets, UDP sockets, watchers and `dns.Resolver`s are closed by the exiting VM rather than left to GC finalizers; sqlite connections a VM opened are checkpointed and closed by that VM's exit; a `Bun.build` whose VM goes away mid-build is cancelled (its plugin requests failed, the pass finished) rather than abandoned or waited on, and one still queued behind other builds is released without waiting for them. - A connect-path DNS lookup (`Bun.connect`, `net`, `WebSocket` to a hostname) is process-wide and outlives the thread that happened to issue it: on macOS a worker exiting mid-lookup no longer answers every other thread's coalesced waiters with an error (and caches it for the TTL). - An addon's external-buffer finalizers run when the Worker that loaded it exits (`napi_create_external_{arraybuffer,buffer}`), as Node's environment teardown finalizes every remaining reference. - Releasing the last keep-alive from an immediate or a late promise reaction (e.g. `port.close()` inside `setImmediate`) is noticed before the loop parks. - Windows: a worker thread closes its loops. Open pipe/tty/process handles and readers mid file-read are closed through their owners in the stop phase, and requests still in flight are drained there — against a live VM that still accepts (and then awaits) the follow-on work a completion may start — before anything is released; sockets over named pipes and TLS-over-duplex sockets join the stop phase; a reader dropped mid-read keeps the buffer its pending read lands in. Two of the crashes were in JavaScriptCore rather than Bun: a `TerminationException` raised while the module loader resolves an import continued into `finishLoadingImportedModule` (fixed in oven-sh/WebKit#391, picked up by the WebKit version bump here). A worker parked in `Atomics.wait` with no timeout still cannot be terminated (oven-sh#32802); that needs a JSC change and is tracked separately. New tests accompany each behaviour fix (worker_threads, Web Worker lifecycle edges — `terminate()` at every phase of dns/fs/build/vm/napi/http work, message ordering and flooding, process exit ordering, sqlite). Seven more upstream `test-worker-*` files are vendored (one of them, the message-port infinite-message-loop test, passes only with these changes) and previously todo/skipped worker-related napi and regression cases run again. LeakSanitizer validation is turned back on for the ~70 worker / MessagePort / BroadcastChannel test files that were exempt. A source lint rejects laundering a `JsResult<JSValue>` into an empty `JSValue`. Main-thread `process.exit()` keeps its current fast path by default; the full main-thread teardown stays behind `BUN_DESTRUCT_VM_ON_EXIT=1`. Workers always tear down fully. - A worker parked in `Atomics.wait` with no timeout still cannot be terminated (oven-sh#32802; needs a JSC change). - `worker_threads` message throughput through the real `MessagePort` is ~0.8× the previous ad-hoc path in a flood microbenchmark (round-trip latency and Web `Worker` messaging are unchanged); a follow-up, not a behaviour regression. - Windows: several concurrent connects to `localhost` can leave one connect stuck (pre-existing; reproduces on current releases; DNS-coalescing on the connect path). - A UDP socket whose receive buffer never drains (e.g. one echoing datagrams to itself on a fast machine) keeps its event loop from running anything else, including a worker's own exit; pre-existing loop-fairness issue, most visible on Windows, follow-up. - Memory a burst of concurrent workers used stays resident after they exit (sequential worker churn plateaus; it is the concurrent peak that is not returned to the OS) — allocator thread-exit policy, follow-up. - `Worker` start semantics for a never-settling top-level await, file-stream fairness on a saturated loop, and a few diagnostics-only items found while fuzzing are tracked separately. Fixes oven-sh#31281 Fixes oven-sh#30421 Fixes oven-sh#15964 Fixes oven-sh#29173 Fixes oven-sh#34690 Fixes oven-sh#31880 Fixes oven-sh#33936 Fixes oven-sh#32073 Fixes oven-sh#33313 Fixes oven-sh#32828 Fixes oven-sh#11760 Fixes oven-sh#26501 Fixes oven-sh#18661 Fixes oven-sh#15408 Fixes oven-sh#23102 Fixes oven-sh#21101 Fixes oven-sh#13570 Fixes oven-sh#31224 Fixes oven-sh#28643 Fixes oven-sh#37163 Fixes oven-sh#25860 Likely also addressed (mechanism matches, not verified end-to-end): `self.onmessage` — the double delivery behind them (oven-sh#25860) is fixed, the packages themselves were not run; and the `parentPort.on('message').unref()` hang half of oven-sh#32609 (its `Worker.performance` half is API surface, not addressed here). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
JSC's WaiterListManager::waitSyncImpl only exits its park loop on vm.hasTerminationRequest(), but that flag was only set by VMTraps::handleTraps at a JS safepoint on the waiting thread itself. A cross-thread NeedTermination trap woke the sync waiter, which saw the flag still false and re-parked forever. So worker.terminate() on a worker blocked in Atomics.wait never completed, and because worker teardown joins child workers, a worker whose own child was parked in Atomics.wait could never finish its process.exit() and its parent never received 'exit'. Set the flag in JSC__VM__notifyNeedTermination before firing the trap; the woken waiter observes it and returns WaitSyncResult::Terminated.
4a16bda to
7a7f2d0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline SigintWatcher note: the other notifyNeedTermination() grep hit at bindings.cpp:5183 (JSC__VM__ensureTerminationExceptionPending) runs on the JS thread and branches on hasTerminationRequest() in the very next lines, so it is not a same-class sibling. The tests' new home in worker_threads.test.ts already sets setDefaultTimeout(isDebug ? 90_000 : 10_000), so my earlier per-test timeout nit from worker-terminate-lifetime.test.ts does not recur.
Extended reasoning...
This run's only finding is the SigintWatcher sibling nit (pre-existing, not regressed). I checked the remaining notifyNeedTermination() call sites: bindings.cpp:5183 is on the JS thread inside ensureTerminationExceptionPending and immediately reads/handles hasTerminationRequest() itself, so it does not need the off-thread pairing this PR adds. I also verified the tests' new location has the file-level 90s/10s default timeout and all required harness imports (bunEnv, bunExe, tempDir, once), so the timeout concern I raised on the previous file does not apply here. Not approving because this is a cross-thread JSC VM termination path where a prior attempt (#29179) introduced crashes; a maintainer should sign off on the scoping (the acknowledged napi mustDeferFinalizers and deferTerminationSlow follow-ups).
|
Re-checked against current main (165dc9f) while closing the worker PRs made obsolete by #37075. This one is not obsolete: #37075 lists the Atomics.wait case as not addressed, and on main the PR's "terminate() interrupts a worker blocked in Atomics.wait" test still times out (debug build), with the test process then unable to exit because the parked worker cannot be joined. The standalone repro also still hangs on the 1.4.0 canary that includes #37075, where node resolves terminate() with 1. Leaving open. |
|
Another report of this bug, for the record: I ended up with the same The branch's tests are the piscina shape (wait entered from a |
…ifetimes; WebKit bump for Atomics.wait (#38457) ### What does this PR do? Follow-up to #38436 with further fixes for stopping a worker (`worker.terminate()`, or `process.exit()` inside it) while it still has native work in flight. **`terminate()` during the `'beforeExit'` re-run of the loop is acted on.** After a worker's loop drains it emits `'beforeExit'` and, if the listeners scheduled more work, re-runs the loop until idle. That inner drain only watched for idleness: a `terminate()` arriving during it closed the VM's gate and woke the loop, but nothing there checked for the stop, so the loop went back to sleep — and because a stopped VM no longer has completions delivered to it, the in-flight work (e.g. a `fetch`) never released it. The worker slept forever and `terminate()` never settled (Node exits the worker within milliseconds). The drain now ends as soon as the stop is requested, as the worker's main loop already does; teardown cancels what is left in flight. **`node:path`'s binding creator checks for an exception before storing each `createPath()` result.** `createNodePathBinding()` passed `Zig::createPath()`'s result straight into `putDirectIndex()` and checked the scope only afterwards; `createPath()` returns `nullptr` when its own `RETURN_IF_EXCEPTION` fires (a worker terminated while its entry point is materialising `node:path`), and `putDirectIndex()` then inspected a null cell. The other object-building lazy binding creators were audited for the same pattern; this was the only instance. **JSC's termination-request flag is kept set for as long as a stopped worker's TerminationException is kept pending.** Bun deliberately leaves the TerminationException that unwound a stopped worker's script pending until teardown, while it finishes draining the current loop tick. JSC resets `VM::hasTerminationRequest()` when the outermost `VMEntryScope` exits and expects the two to agree while the exception is pending — its own clients never keep the exception past that point without also ceasing to touch the VM (WebCore's worker run loop runs no further task once terminating). Host code that ran in the rest of the tick and initialised a lazy structure (building an error or result object) therefore tripped `VMTraps::deferTerminationSlow()`'s `ASSERT(vm.hasTerminationRequest())` on debug builds and had the pending termination silently dropped on release builds. The invariant is now kept on our side, next to where it was already maintained for teardown (`Zig__GlobalObject__forbidExecution` / `Bun__GlobalObject__clearExceptionsForExit`): when a call into JSC comes back with the TerminationException pending and the request already reset, it is set again — on the cold error arm of every Rust→JSC exception-check boundary (the generated `*_is_throw` wrappers and the C++ shim behind `return_if_exception()`), in the timer callback landing frame, and in the microtask drain. **WebKit bump: `worker.terminate()` stops a worker blocked in `Atomics.wait()`** (oven-sh/WebKit@f0f60fd23248, oven-sh/WebKit#432). A worker parked in `Atomics.wait()` / wasm `memory.atomic.wait` with no timeout could not be terminated — the terminate promise never settled and the thread leaked; Node stops such a worker immediately. `WaiterListManager::waitSyncImpl`'s wake-up predicate only looked at `VM::hasTerminationRequest()`, which since the last upstream merge is only ever set by the parked thread itself, so the notify woke it and it parked again; the wake-up is also delivered under the waiter's lock now so it cannot be lost where VM traps are polled (Windows). Sync-over-async worker pools (synckit, Prettier/eslint plugins, piscina's Atomics mode) park exactly there. Fixes #32802. **Errors built for a stopped worker are always objects.** Every `ERR::*` helper, `Bun__createErrorWithCode` and the WebStreams code throw or reject with what `Bun::createError()` returns; it built the error through `ErrorInstance::create(JSGlobalObject*, …)`, which converts the message first and hands back `nullptr` when that conversion is interrupted — and now that a stopped worker keeps draining its tick with its TerminationException pending, sites like `ERR::OUT_OF_RANGE` (zlib option validation), `writableStreamDefaultWriterRelease`'s "released" error and the `Response`/`Request` body readers passed that null to `ThrowScope::throwException` / `JSPromise::rejectedPromise` (SEGV inspecting a null cell). `ErrorCodeCache::createError` now converts message/`cause` itself and constructs through the infallible `VM&` overload (the termination stays pending for the caller; anything else thrown while building the message becomes the error, as before), and `JSC__JSPromise__rejectedPromise` returns an inert promise if it is ever handed an empty value. **Prime generation gives up once its worker has been asked to stop.** `crypto.generatePrime()`/`generatePrimeSync()`/`checkPrime()`/`checkPrimeSync()` with `safe: true` or awkward `add`/`rem` constraints can run for minutes; a worker's teardown waits for its pool jobs and the sync forms cannot observe a termination at all, so `terminate()` / `process.exit()` hung for as long as BoringSSL took. The `BN_GENCB` progress callback (a `return true` stub with a TODO) now returns whether the VM the work is for may still run script, aborting the generation as soon as the stop is requested — where Node checks `is_stopping()`. A failed/aborted generation is reported as `ERR_CRYPTO_OPERATION_FAILED` rather than converting a half-made BIGNUM (`checkPrimeSync` previously returned `true` for `BN_is_prime_ex`'s -1). Key-pair generation goes through `EVP_PKEY_keygen`, which has no progress hook in BoringSSL, and pbkdf2/scrypt/argon2 with extreme parameters have none either (as in Node); those still make the teardown wait. **A streaming fetch's teardown no longer writes into a freed response stream source** (worker exit; heap-use-after-free WRITE under ASAN). With both a streaming request body (its sink cell holds the FetchTasklet) and a JS-touched `response.body` (a ByteStream source owned by the stream's source cell) alive at exit, the VM's last sweep destroys cells in no particular order and the tasklet unhooked itself as the stream's producer through the ReadableStream wrapper into a source that sweep had already freed. The tasklet now holds a counted ref on the source while it is its producer and unhooks through that, touching no JS cell (which also lets the Response weak-finalizer path unhook instead of skipping it). **Subprocess: no pending-activity bookkeeping once the wrapper is finalized.** A worker exiting while a spawned child still had a pending pipe-backed stdin (a Blob the child never read — the default stdin path on Windows) finalized the Subprocess in the last sweep; `finalize()` marks the wrapper finalized and then closes stdio, whose close path re-evaluated pending activity and tried to re-root the dead wrapper (debug assert). **valkey: `close()` returns what a half-open socket's `onclose` left pending instead of folding it.** `close()` runs the close event itself for a half-open socket and folded the result on the spot, but it is also reached beneath frames that go on to return their own `Err` (`fail_with_js_value`, the HELLO-failure path, `fail_handshake`); a dispatcher fold beneath a frame that still propagates can take the exception that frame's `Err` refers to, and the timer fold above then finds nothing pending. Callers now sequence the result with their own and only the deferred-close task folds. ### How did you verify your code works? New tests in `test/js/web/workers/worker-terminate-lifetime.test.ts`: a worker whose `'beforeExit'` listener starts a `fetch()` to a server that never answers and tells the parent, which then calls `terminate()`; it must settle with exit code 1. Times out without the change, passes with it (release and debug). Also checked that the natural `'beforeExit'` cycle (listener re-scheduling work N times, then `'exit'`) and `process.exit()` from a `'beforeExit'` listener behave as before and as in Node. The `node:path` change has no dedicated test (termination-timing window only); `node:path` in the main thread and in a worker still behaves. For the termination flag: (debug only) workers that start a refused redis connect in the same immediate tick as `process.exit()` must exit cleanly — asserts in `deferTerminationSlow` 3/3 without the change, passes with it; the earlier `Bun.serve`-in-a-stopped-worker repro from #38436 also stays clean with that PR's server-side gate disabled, i.e. the flag maintenance alone covers it. For the WebKit bump: `terminate()` of a worker blocked in `Atomics.wait(i32, 0, 0)` must complete with exit code 1 — times out on the previous pin, passes now (this ran green on every platform, Windows included, as #38447 before being folded in here). Prime generation: `terminate()` of workers grinding `generatePrimeSync(2048, {safe})`, `generatePrime(2048, {safe})` and a 200-round `checkPrime` of a 4423-bit Mersenne prime resolves promptly (times out on the current release binary). Streaming-request-body fetches with touched `response.body` at worker exit: heap-use-after-free 3/3 before, clean after (new test). Blob-stdin child at worker exit with `BUN_FEATURE_FLAG_DISABLE_MEMFD`: asserts before, clean after (new test). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Closed by #38457 (merge commit 97a4363), which fixes this on the WebKit side instead: the bump to oven-sh/WebKit@f0f60fd (oven-sh/WebKit#432) makes a termination request from another thread wake a parked Verified against main at 97a4363 (debug build) that it covers both cases from this PR, which both still hang on the 1.4.0-canary.1 binary built before that merge:
Both tests from this PR's diff also pass unchanged on main. Only the first shape is covered by the tests #38457 added ( |
2 participants
Fixes #29173
Problem
Two symptoms, one cause.
worker.terminate()never completes when the worker thread is blocked in an infiniteAtomics.wait(). The worker thread stays parked in the futex forever, theexit/closeevent never fires, and if the worker was keeping the event loop alive the whole process hangs. This is Bun process does not exit after Tinypooldestroy()completes (worker threads remain alive) #29173 (Tinypool'spool.destroy()hangs; its idle workers block inAtomics.waitwaiting for work).Since the worker lifetimes rework (Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075), a worker's own exit joins its child workers during teardown. If a worker calls
process.exit()(or dies on an uncaught throw) while one of its own children is parked inAtomics.wait, the middle worker can never finish exiting, its parent never receives'exit', and the process never exits. This is a regression relative to pre-rework main:Before the rework this printed
child exit-event 5and exited in about a second; on current main it hangs forever (the grandchild thread stays infutex_wait, the child blocks joining it, andawait child.terminate()from the parent never resolves either). Node exits (V8's TerminateExecution interruptsAtomics.wait).Cause
JSC's off-thread termination contract has two parts, and Bun's shared helper
JSC__VM__notifyNeedTermination(src/jsc/bindings/bindings.cpp) only did one of them.vm.notifyNeedTermination()fires theNeedTerminationVM trap bit.VMTraps::requestThreadStopIfNeededthen wakes the target VM's sync Atomics waiter:The waiter that just woke up re-checks its loop condition, which is keyed on a different flag:
m_hasTerminationRequestis normally set byVMTraps::handleTraps(case NeedTermination), but that only runs at a JS safepoint on the worker thread itself. A thread parked in a futex never reaches one. So the waiter wakes, seeshasTerminationRequest()still false, and goes right back towaitUntil(infinity). JSC'sSignalSendereven re-notifies it every 1ms, and it re-parks every time.Bun already has both halves of the contract, just in two different helpers that no caller combined:
JSGlobalObject__requestTerminationsets the request (used bybun:testtimeouts, JS-thread only), andJSC__VM__notifyNeedTerminationfired the trap (used by every worker termination path).Fix
Set
hasTerminationRequestinJSC__VM__notifyNeedTerminationbefore firing the trap. The order matters:requestThreadStopIfNeeded'snotifyOne()fires on the thread-stop transition, so the flag has to already be visible when the woken waiter re-checks its loop condition. The waiter then returnsWaitSyncResult::TerminatedandatomicsWaitImplthrows the termination exception, unwinding the worker normally. A worker that callsAtomics.waitafter termination was requested is caught by the same condition before it ever parks.This is the one shared helper behind every worker termination path (
Worker#terminate()for both Web andnode:worker_threadsworkers, an exiting parent or exiting worker stopping its children,process.exit()inside a worker, an uncaught worker error), so the single change covers all of them.setHasTerminationRequest()is safe off-thread: it writes the flag and requests aConcurrentEntryScopeService, whose request set is atomic.Worker exit semantics are unchanged:
on_exitclears the flag (Bun__GlobalObject__clearExceptionsForExit) before dispatching'exit'handlers, so a worker exiting by its ownprocess.exit()still runs them, and a parent-terminated worker still skips them viaforbid_script(the existing "worker stop ordering as seen by the worker's own handlers" tests cover both and pass).Verification
Debug+ASAN build,
test/js/node/worker_threads/worker_threads.test.ts:terminate() interrupts a worker blocked in Atomics.wait: times out (90s) without the fix, passes in ~1.9s with it.process.exit() in a worker completes while its own child worker is parked in Atomics.wait: times out without the fix, passes in ~4.4s with it.Both tests prove the worker is really parked before termination fires by spinning until
Atomics.notifyreports one woken agent. The standalone nested repro above printschild exit-event 5and exits 0 in ~3.8s with the fix; without it, it hangs until killed (3/3 each way). Full file: 123 pass, 0 fail.The first revision of this PR carried the Web Worker flavor of the terminate test in
test/js/web/workers/worker-terminate-lifetime.test.ts; this revision keeps the coverage inworker_threads.test.tsinstead, because that file currently fails under ASAN on main for an unrelated pre-existing leak (terminate() mid-dns leaks thenode:fsbinding box, tracked in #35159) which would mask results here. The code path under test (WebWorker__requestTermination) is shared by both Worker flavors.Not covered by this change, for completeness:
readFileSyncon a pipe that never delivers,execSync,spawnSync) still block termination until they return, matching Node, whoseJoinThreadwaits for them the same way. Releasing the blocked call (e.g. writing to the pipe) completes teardown promptly.terminate()on such a worker already hung before the worker lifetimes rework, without any nesting (Node interrupts it). With the rework it also blocks an exiting parent's join. Separate pre-existing bug.Bun.sleepSyncis a native sleep with the same blocking shape; Bun.sleepSync: make worker.terminate() interrupt a worker blocked in sleepSync #35103 addresses it separately.Prior attempt: #29179
A previous attempt at #29173 (#29179, against the Zig codebase) was closed unmerged after CI surfaced two residual crashes. Both were caused by what that PR added beyond the one-line flag, and both of those parts have since landed separately on main (the trap is already fired by
JSC__VM__notifyNeedTermination, and worker teardown already runs~VM()which stops theSignalSender; teardown also already clears the flag before'exit'handlers). This PR does not touch teardown.Related
This change also closes the
Atomicsroute into the fuzz-reportedASSERTION FAILED: vm.hasTerminationRequest()atVMTraps::deferTerminationSlow: with the request set by the requester,throwTerminationException()fromatomicsWaitImplno longer runs with the request flag unset.One reader of
hasTerminationRequest()gets a wider window from this change:napi.h'smustDeferFinalizers(), which decides whether a non-experimental napi module's finalizer runs synchronously during GC or is deferred to the next tick. That state is already reached deterministically on every napi worker teardown (teardown sets the flag and then runs a full collection), so this is not a new failure mode; the delta is only that a parent-threadterminate()can also flip it during an earlier collection. KeyingmustDeferFinalizers()on a worker-thread-set teardown flag would be its own hardening innapi.h.