worker: make termination interrupt a worker blocked in Atomics.wait

Closed
RO
3mo ago
+68-0

Fixes #29173

Problem

Two symptoms, one cause.

  1. worker.terminate() never completes when the worker thread is blocked in an infinite Atomics.wait(). The worker thread stays parked in the futex forever, the exit/close event never fires, and if the worker was keeping the event loop alive the whole process hangs. This is #29173 (Tinypool's pool.destroy() hangs; its idle workers block in Atomics.wait waiting for work).

  2. Since the worker lifetimes rework (#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 in Atomics.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:

// main -> child -> grandchild. Grandchild parks in Atomics.wait; child calls process.exit(5).
import { Worker, parentPort, workerData } from "node:worker_threads";
const role = workerData?.role ?? "main";
if (role === "main") {
  const c = new Worker(new URL(import.meta.url), { workerData: { role: "child" } });
  c.on("exit", code => console.log("child exit-event", code));
} else if (role === "child") {
  const g = new Worker(new URL(import.meta.url), { workerData: { role: "grand" } });
  g.on("message", () => setTimeout(() => process.exit(5), 150));
} else {
  parentPort.postMessage("parked");
  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0);
}

Before the rework this printed child exit-event 5 and exited in about a second; on current main it hangs forever (the grandchild thread stays in futex_wait, the child blocks joining it, and await child.terminate() from the parent never resolves either). Node exits (V8's TerminateExecution interrupts Atomics.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.

  1. vm.notifyNeedTermination() fires the NeedTermination VM trap bit. VMTraps::requestThreadStopIfNeeded then wakes the target VM's sync Atomics waiter:

    // VMTraps.cpp:419
if (hasTrapBit(NeedTermination))
    vm.syncWaiter()->condition().notifyOne();

  2. The waiter that just woke up re-checks its loop condition, which is keyed on a different flag:

    // WaiterListManager.cpp:95
while (syncWaiter->isOnList() && time.now() < time && !vm.hasTerminationRequest())
    syncWaiter->condition().waitUntil(list->lock, time.approximate<WallTime>());

m_hasTerminationRequest is normally set by VMTraps::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, sees hasTerminationRequest() still false, and goes right back to waitUntil(infinity). JSC's SignalSender even 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__requestTermination sets the request (used by bun:test timeouts, JS-thread only), and JSC__VM__notifyNeedTermination fired the trap (used by every worker termination path).

Fix

Set hasTerminationRequest in JSC__VM__notifyNeedTermination before firing the trap. The order matters: requestThreadStopIfNeeded's notifyOne() 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 returns WaitSyncResult::Terminated and atomicsWaitImpl throws the termination exception, unwinding the worker normally. A worker that calls Atomics.wait after 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 and node:worker_threads workers, 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 a ConcurrentEntryScopeService, whose request set is atomic.

Worker exit semantics are unchanged: on_exit clears the flag (Bun__GlobalObject__clearExceptionsForExit) before dispatching 'exit' handlers, so a worker exiting by its own process.exit() still runs them, and a parent-terminated worker still skips them via forbid_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.notify reports one woken agent. The standalone nested repro above prints child exit-event 5 and 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 in worker_threads.test.ts instead, because that file currently fails under ASAN on main for an unrelated pre-existing leak (terminate() mid-dns leaks the node:fs binding 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:

  • Blocking syscalls (readFileSync on a pipe that never delivers, execSync, spawnSync) still block termination until they return, matching Node, whose JoinThread waits for them the same way. Releasing the blocked call (e.g. writing to the pipe) completes teardown promptly.
  • A worker spinning in an infinite WebAssembly loop: wasm execution does not reach the VM trap check that JS loop back-edges do, so 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.sleepSync is a native sleep with the same blocking shape; #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 the SignalSender; teardown also already clears the flag before 'exit' handlers). This PR does not touch teardown.

Related

This change also closes the Atomics route into the fuzz-reported ASSERTION FAILED: vm.hasTerminationRequest() at VMTraps::deferTerminationSlow: with the request set by the requester, throwTerminationException() from atomicsWaitImpl no longer runs with the request flag unset.

One reader of hasTerminationRequest() gets a wider window from this change: napi.h's mustDeferFinalizers(), 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-thread terminate() can also flip it during an earlier collection. Keying mustDeferFinalizers() on a worker-thread-set teardown flag would be its own hardening in napi.h.