[JSC] Atomics.wait / memory.atomic.wait: wake up for a termination requested from another thread - #432
Conversation
…ng outside any VMEntryScope VMTraps::deferTerminationSlow() suspends a pending TerminationException and relies on VM::hasTerminationRequest() staying set so that undoDeferTermination() re-throws it. That holds while a VMEntryScope is on the stack, but the outermost VMEntryScope's exit resets the request. A client that deliberately leaves the TerminationException pending past that exit — so that nothing re-enters the VM until it tears it down — can still reach a DeferTermination scope from host code (a LazyProperty / LazyClassStructure initializer, JSObject::putDirect...) before then. In debug that failed ASSERT(vm.hasTerminationRequest()); in release the exception was cleared, undoDeferTermination() saw no request and never re-threw it, so the pending termination was silently dropped (and m_suspendedTerminationException left stale). When the request has already been reset, re-establish it for the duration of the deferral and reset it again once the exception has been re-thrown, so the scope is transparent in that state. Behaviour with a VMEntryScope on the stack, or without a pending TerminationException, is unchanged.
…quested from another thread
WaiterListManager::waitSyncImpl() parks the thread until it is dequeued by a
notify, times out, or `vm.hasTerminationRequest()`. Since 305643@main
("Fix a race condition with the setting of VM::m_hasTerminationRequest")
that request is only established by the mutator thread itself when it handles
the NeedTermination trap — which it cannot do while parked — and
VM::notifyNeedTermination() from another thread merely fires the trap. So a
worker blocked in Atomics.wait() with no timeout could no longer be stopped:
VMTraps woke its condition, the predicate was still false, and it went back to
sleep for good (worker.terminate() never completing; the sync-over-async
worker pools that park in Atomics.wait leak their thread).
- The wait predicate also honours a pending NeedTermination trap, and on that
exit waitSyncImpl() handles the trap itself (establishing the request and
throwing the TerminationException) before returning Terminated, so both
callers just propagate.
- The wake-up moves from requestThreadStopIfNeeded() — which only runs when
no other async event already has a stop requested — to fireTrap() for
NeedTermination, and is delivered by Waiter::notifyOfTerminationRequest()
under the lock of the list the waiter is parked on, so it cannot fall
between the waiter's predicate check and its park. That matters where traps
are polled rather than signalled (no SignalSender re-notifying every
millisecond), e.g. on Windows.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
…nation in waitSyncImpl without handleTraps() - deferTerminationSlow(): re-establishing the request does not depend on being outside a VMEntryScope, so don't assert it. - waitSyncImpl(): consume the NeedTermination trap and establish/throw the termination directly instead of calling VMTraps::handleTraps(), whose trap check also walks vm.topCallFrame to jettison optimized code — not needed to throw here, and best avoided from wasm's memory.atomic.wait operation.
…pt a wait while termination is deferred DeferTermination (commit 1): - undoDeferTermination() takes its slow path when a TerminationException was suspended even if the request has meanwhile been reset - which happens when the deferred code itself enters the VM (a LazyProperty initializer may) and that outermost VMEntryScope exits. The slow path re-establishes the request for the re-throw if needed and then leaves it as it was before the deferral, so neither the exception nor the flags go stale in that case. Atomics.wait (commit 2): - While termination is being deferred, a termination request does not cut the wait short (it takes effect when the deferral ends); previously that corner returned Terminated without an exception thrown, handing Atomics.wait an empty value / wasm a bogus trap. The tail now always throws and both callers assert a pending exception. - Waiter::setParkedList() accessor instead of befriending WaiterListManager; CONCURRENT_SAFE on the notifySyncWaiterOfTermination definition; comment fixes (handleTraps() jettisons trap-breakpointed code blocks rather than walking the stack; ASCII).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies cross-thread termination signaling in VMTraps and introduces new lock interactions in WaiterListManager, a human look would still be worthwhile.
Checked the new lock ordering: notifyOfTerminationRequest() releases m_parkedListLock before taking the list lock, and setParkedList() (called under the list lock) only holds m_parkedListLock briefly, so no AB/BA cycle.
Checked that fireTrap(NeedTermination) sets the trap bit before notifySyncWaiterOfTermination(), so a waiter that publishes its list after the notify still sees the trap in shouldStopWaitingForTermination().
Checked undoDeferTerminationSlow() — m_didSetTerminationRequestForDeferral is always cleared via std::exchange on the suspended-exception path, and is never set on the other path.
Confirmed the out-of-line ~Waiter() is needed for RefPtr<WaiterList> with the forward declaration.
Extended reasoning...
Overview
This PR makes two independent changes to JSC's termination handling:
VMTraps::deferTerminationSlow/undoDeferTerminationSlow(VMTraps.cpp, VMTraps.h, VMTrapsInlines.h): addsm_didSetTerminationRequestForDeferralso that aTerminationExceptionpending outside anyVMEntryScopeis correctly suspended and re-thrown across aDeferTerminationscope, and changes theundoDeferTermination()fast-path predicate to checkm_suspendedTerminationExceptionin addition tohasTerminationRequest().- Atomics.wait / memory.atomic.wait termination wake-up (WaiterListManager.{h,cpp}, VMTraps.{h,cpp}, AtomicsObject.cpp, WasmOperationsInlines.h): the wait predicate now also checks
traps().needHandling(NeedTermination);Waiterrecords theWaiterListit is parked on under a newm_parkedListLock;fireTrap(NeedTermination)unconditionally calls a new out-of-linenotifySyncWaiterOfTermination()that notifies the condition while holding the list lock; on terminationwaitSyncImpl()now consumes the trap and throws the exception itself, and callers just assert it is pending.
Security risks
No injection/auth/data-exposure surface. The concern here is concurrency correctness inside the VM: lock ordering between m_parkedListLock, WaiterList::lock, and m_trapSignalingLock, and lost-wakeup / state-machine correctness for termination. A regression here could deadlock or leak worker threads. I traced the lock acquisition orders and did not find a cycle (notifyOfTerminationRequest() drops m_parkedListLock before taking the list lock; fireTrap calls the notify helper after releasing m_trapSignalingLock in updateThreadStopRequestIfNeeded()).
Level of scrutiny
High. This touches core VM termination machinery and cross-thread condition-variable signaling in the JavaScriptCore runtime — code that ships in Bun's production engine. The DeferTermination state machine change and the new lock in Waiter are both the kind of change where a subtle mistake produces rare hangs or assertion failures under load. This is well outside the "simple, mechanical, obvious" bar for automated approval.
Other factors
The PR description is thorough, references the upstream regression, and the commit history shows review follow-ups already applied. Verification was done via Bun debug+ASAN builds and a 40-worker stress test, and a Bun-side PR with a regression test is referenced. The bug-hunting system found nothing. Still, given the concurrency subtlety and that this diverges from upstream WebKit behavior, a human reviewer familiar with JSC's VMTraps/termination model should sign off.
Preview Builds
|
…ion wake-up The state the DeferTermination change handled (TerminationException left pending outside any VMEntryScope while host code keeps initialising lazy structures) is one WebKit's own clients never produce: WebCore either clears the exception at the JS entry (NakedPtr<Exception> calls) or, in WorkerDedicatedRunLoop::Task::performTask, leaves it pending but then runs no further non-cleanup task. It is the embedder's job to keep "TerminationException pending => hasTerminationRequest()" true if it keeps the exception; Bun will do that on its side. Restores deferTerminationSlow(), undoDeferTerminationSlow(), undoDeferTermination() and VMTraps' members to origin/main.
dylan-conway
changed the title
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it changes cross-thread trap signaling and adds new lock interactions in core JSC (VMTraps / WaiterListManager), a human look would still be worthwhile.
Checked the new m_parkedListLock ↔ WaiterList::lock ordering — notifyOfTerminationRequest() drops m_parkedListLock before taking the list lock, so it does not nest against the waiter's list→parked order.
Checked that moving throwTerminationException() into waitSyncImpl() runs after VMBlockingScope and the list lock are released, so the throw is not under either.
Checked that fireTrap(NeedTermination) sets the trap bit before notifySyncWaiterOfTermination(), so a waiter that has published its list but not yet tested the predicate observes the bit.
Extended reasoning...
Overview
This PR fixes worker.terminate() hanging when the worker is parked in Atomics.wait / memory.atomic.wait. It touches six files in JavaScriptCore's runtime and wasm layers: VMTraps.{h,cpp} (adds notifySyncWaiterOfTermination() called unconditionally from fireTrap(NeedTermination), removes the transition-gated notify from requestThreadStopIfNeeded()), WaiterListManager.{h,cpp} (adds Waiter::m_parkedListLock/m_parkedList, setParkedList(), notifyOfTerminationRequest(), and shouldStopWaitingForTermination(); waitSyncImpl() now consumes the NeedTermination trap and throws the TerminationException itself), and the two callers in AtomicsObject.cpp / WasmOperationsInlines.h (now assert the exception is already pending instead of throwing it).
Security risks
No auth/crypto/permissions surface. The risk here is concurrency correctness inside the VM: lost wake-ups, deadlock from the new lock, or mis-handling the termination exception state. A regression could hang or crash worker threads rather than expose data, so I don't see a security-sensitive angle per se, but the correctness bar for this code is very high.
Level of scrutiny
High. This is subtle cross-thread synchronization in core JSC internals — the exact area (VMTraps termination signaling vs. parked waiters) where the PR description itself cites three prior upstream commits that each shifted the behavior. The change introduces a new lock (m_parkedListLock) that interacts with the existing WaiterList::lock, moves where TerminationException is thrown, and directly consumes a trap bit outside handleTraps(). These are design-level decisions in production-critical engine code, not mechanical edits.
Other factors
The PR description is thorough and the change was validated end-to-end via a Bun build (debug+ASAN) with a 40-worker stress test and the full Bun suite on all platforms including Windows (the polling-traps path). The lock-ordering and publish-before-test reasoning look sound to me on read-through, and the throw now happens outside the VMBlockingScope. Still, the combination of new lock ordering, altered trap-consumption semantics, and DeferTermination interaction is the kind of change that benefits from a maintainer who knows the VMTraps invariants signing off.
…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>
1 participant
Symptom
worker.terminate()never stops a worker parked inAtomics.wait(i32, 0, 0)(no timeout); the terminate promise never settles and the thread leaks. Node stops such a worker immediately.Cause
WaiterListManager::waitSyncImpl()waitswhile (isOnList() && now < deadline && !vm.hasTerminationRequest()). Since upstream 305643@main ("Fix a race condition with the setting of VM::m_hasTerminationRequest", 2026-01)VM::notifyNeedTermination()no longer sets that flag — onlyVMTraps::handleTraps()on the mutator thread does, and the mutator is the parked thread.VMTrapsstill notifies the waiter's condition, the predicate is still false, and it parks again. (Same code upstream today; upstream bug 289686 / WebKit PR 70964 is an in-flight fix for the predicate.) Independently, that notify was issued without the lock the waiter parks under, and only fromrequestThreadStopIfNeeded()— i.e. only on the transition to "stop requested", not if another async trap already had one pending — so on polling-trap platforms (noSignalSenderre-notifying every 1 ms; for Bun that is Windows) it could also simply be lost.Change
hasTerminationRequest() || traps().needHandling(NeedTermination), except while termination is being deferred (DeferTermination), when neither cuts the wait short — it takes effect when the deferral ends. When it ends the wait,waitSyncImpl()consumes the trap, establishes the request and throws theTerminationExceptionitself, and returnsTerminated;atomicsWaitImpl()and wasm'swaitImpl()assert the pending exception and propagate instead of callingthrowTerminationException()themselves.Waiter(the per-VM sync waiter) records theWaiterListit is parked on under a small lock (setParkedList());Waiter::notifyOfTerminationRequest()reads it and notifies while holding that list's lock, which the waiter holds from its predicate check until it parks, so the wake-up cannot fall in between. If the waiter is not parked yet there is nothing to do: it publishes the list before testing the predicate, so it observes the already-fired trap.VMTraps::fireTrap(NeedTermination)calls it (out of line) unconditionally — where 282974@main originally put the notify, before 298039@main moved it intorequestThreadStopIfNeeded()and 303185@main made that transition-gated; the copy there is removed andSignalSender's periodic one uses the same helper.deferTerminationSlow()/undoDeferTerminationSlow()are unchanged.Verification
Through Bun built against this branch (debug+ASAN,
debug-local):new Worker(... Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0) ...)+terminate()hangs on the current prebuilt and exits with code 1 immediately with this change; a 40-worker stress mixing infinite waits, 50 ms timed waits in a loop, andterminate()issued immediately vs. after a delay completes 40/40. The Bun PR pinning this branch's preview build (oven-sh/bun#38447) adds that as a test and ran the full suite on every platform (including Windows, the polling-traps case) with no differences.