JSCTaskScheduler: skip DeferredWorkTimer tasks once a worker is terminating

[robobun]

Debug/ASAN abort when worker.terminate() lands while a FinalizationRegistry has a pending cleanup in the worker:

ASSERTION FAILED: (null)
!exception()
JavaScriptCore/ExceptionScope.h(61) : void JSC::ExceptionScope::assertNoException()

Repro

import { Worker } from "node:worker_threads";
const src = `
  const reg = new FinalizationRegistry(() => {});
  (function () { reg.register({}, 1); })();
  globalThis.keepReg = reg;
  setInterval(() => {}, 1e9);
  postMessage("go");
`;
const w = new Worker(src, { eval: true });
w.on("message", () => w.terminate());
await new Promise(r => w.on("exit", r));

Aborts reliably on a debug build. Also reachable via fs.promises.open in a worker (which registers a FileHandle with a FinalizationRegistry), which is how #34260 tripped over it, and via node:diagnostics_channel subscribe/unsubscribe (any dc-based APM in a worker pool).

Cause

JSC schedules FinalizationRegistry cleanup (and Atomics.waitAsync, Wasm streaming compilation) through DeferredWorkTimer. JSC's own DeferredWorkTimer::doWork checks scriptExecutionStatus before each task and drops it on Stopped; it also bails when vm.hasPendingTerminationException(). Bun's replacement, JSCTaskScheduler::runPendingWork, does neither.

Sequence:

1. Worker entry module finishes (setInterval registered, postMessage sent).
2. Parent receives the message and calls terminate(); notifyNeedTermination sets the termination trap.
3. spin()'s initial run_gc() collects the registered target; JSFinalizationRegistry::finalizeUnconditionally schedules a JSCDeferredWorkTask.
4. The first event-loop tick runs a CppTask whose post-run return_if_exception handles the trap and throws the TerminationException; tick() returns with it pending. The JSCDeferredWorkTask is still in the queue.
5. spin()'s loop calls tick() again before its requested_terminate check. The JSCDeferredWorkTask runs, runFinalizationCleanup calls JSC::call, and Interpreter::executeCallImpl's entry scope.assertNoException() fires.

Stack:

#5  JSC::JSFinalizationRegistry::runFinalizationCleanup
#6  JSC::JSFinalizationRegistry::finalizeUnconditionally::$_2::operator()
#9  Bun::runPendingWork
#10 Bun__runDeferredWork
#17 bun_jsc::event_loop::EventLoop::tick
#19 bun_jsc::web_worker::WebWorker::spin

Fix

runPendingWork: gate on scriptExecutionStatus == Running && !vm.hasPendingTerminationException(), matching DeferredWorkTimer::doWork. The task is dropped instead of entering JS.

A dropped FinalizationRegistry cleanup is the expected behaviour: node runs no user JS after terminate(). Main-thread cleanups still fire, including after a transient vm.runInNewContext(..., {timeout}) termination, since scriptExecutionStatus only reports Stopped for worker terminate or VM shutdown.

The onScheduleWorkSoon half of this (dropping tasks scheduled during teardownJSCVM's collectNow) is already covered by m_isShuttingDown.

Verification

New case in test/js/web/workers/worker-terminate-lifetime.test.ts (debug/ASAN-gated; the assert compiles to a no-op in release):

• git stash push -- src/ && bun bd test ... -t FinalizationRegistry → SIGABRT, ASSERTION FAILED: !exception()
• git stash pop && bun bd test ... -t FinalizationRegistry → pass
• FinalizationRegistry still fires on the main thread, including after vm.runInNewContext(..., {timeout}) times out
• test/js/web/atomics.test.ts, test/regression/issue/atomics-waitasync-wtftimer-uaf.test.ts pass

Adjacent: #30857 adds exception reporting after the task runs in the same block; this change gates before it runs. Both are needed; whichever lands second rebases.

[robobun]

^{Updated 3:06 PM PT - Jul 22nd, 2026}

❌ @robobun, your commit aab4381 has 2 failures in Build #78046 (All Failures):

• test/js/node/test/parallel/test-gc-http-client-connaborted.js - timeout on 🐧 25.04 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 25.04 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 13 x64

---

🧪   To try this PR locally:

bunx bun-pr 34270

That installs a local version of the PR into your bun-34270 executable, so you can run:

bun-34270 --bun

[github-actions]

Found 1 issue this PR may fix:

1. ASAN CI: JSC assertion in JSObject::getOwnPropertyDescriptor during worker terminate (test-worker-message-port-transfer-terminate) #34095 - ASAN CI assertion in JSObject::getOwnPropertyDescriptor during worker terminate shares the same root cause: a JSC assertion fires because code executes while a TerminationException is already pending during worker shutdown. This PR's guard skipping deferred work tasks when TerminationException is pending would prevent this class of assertion failure.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #34095

🤖 Generated with Claude Code

[robobun]

Re the bot's suggestion: #34095 is the JSObject::getOwnPropertyDescriptor assert from a lazy property builder interrupted by terminate (addressed by #33418 / #33966). This PR fixes the Interpreter::executeCallImpl assert on the DeferredWorkTimer path, a different mechanism, so not adding a Fixes #34095 here.

[coderabbitai]

Walkthrough

Changes

The scheduler now marks VM teardown atomically, rejects deferred work during termination, and skips execution when script execution is stopped or a termination exception is pending. A worker lifetime regression test verifies clean process termination.

Worker termination handling

Layer / File(s)	Summary
Deferred work termination guard src/jsc/bindings/JSCTaskScheduler.h, src/jsc/bindings/JSCTaskScheduler.cpp, src/jsc/bindings/ZigGlobalObject.cpp, src/jsc/bindings/webcore/Worker.cpp	Adds the termination flag, sets it during VM teardown, cancels newly scheduled work, and gates pending task execution on VM state.
Worker termination regression coverage test/js/web/workers/worker-terminate-lifetime.test.ts	Adds ASAN/debug coverage for FinalizationRegistry cleanup during worker termination and verifies clean process exit.

Possibly related issues

• [CRASH] Worker termination races in-flight fetch(), corrupting the event loop's concurrent task queue (two crash signatures) #33911: Addresses a related worker VM termination race involving asynchronous work after teardown.

Possibly related PRs

• oven-sh/bun#33956: Modifies the same JavaScriptCore deferred-work scheduler pipeline.
• oven-sh/bun#34278: Addresses the worker-shutdown enqueue/drain race through earlier termination marking.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: skipping deferred work tasks during worker termination.
Description check	✅ Passed	The description covers the change, cause, fix, repro, and verification, though it uses custom headings instead of the template.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 166-176: Update the expectation in the worker termination test to
assert the raw stderr value unconditionally, without gating it on exitCode.
Reorder the expected fields to the established canonical sequence: stderr,
stdout, exitCode, then signalCode, while preserving their existing expected
values.

🪄 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: fde047fb-1537-41c8-bc81-bf916f0d730c

📥 Commits

Reviewing files that changed from the base of the PR and between be77b65 and 092a32e.

📒 Files selected for processing (2)

• src/jsc/bindings/JSCTaskScheduler.cpp
• test/js/web/workers/worker-terminate-lifetime.test.ts

[claude]

I didn't find any bugs, but this touches the core JSC DeferredWorkTimer integration path (shared by FinalizationRegistry, Atomics.waitAsync, and Wasm streaming) during worker termination — worth a maintainer's eyes, especially given the noted overlap with #30857 in the same block.

What was reviewed:

• The new guard mirrors upstream DeferredWorkTimer::doWork (scriptExecutionStatus == Running && !hasPendingTerminationException()); scriptExecutionStatus is populated in both the ZigGlobalObject and NodeVM method tables.
• ticket->target() / scriptExecutionOwner() are dereferenced only after !isCancelled(), matching the existing job->vm() accessor's assumption.
• Main-thread path: status is Running and no termination exception is pending, so the guard is a no-op there.
• Test follows the file's conventions; the CodeRabbit stderr-assertion feedback was applied in 2204cec.

Extended reasoning...

Overview

The PR adds a two-condition guard in JSCTaskScheduler::runPendingWork (src/jsc/bindings/JSCTaskScheduler.cpp) that skips a queued DeferredWorkTimer task when the ticket's global object reports scriptExecutionStatus != Running or the VM has a pending TerminationException. It threads the JSC::VM& through from Bun__runDeferredWork and adds DeferredWorkTimerInlines.h / GlobalObjectMethodTable.h includes. A new debug/ASAN-gated regression test in worker-terminate-lifetime.test.ts reproduces the executeCallImpl assertNoException() abort via a FinalizationRegistry cleanup racing worker.terminate().

Security risks

None identified. The change is purely a drop-this-task guard on an internal scheduler path; no user input parsing, no auth/crypto, no new external surface.

Level of scrutiny

Medium-high. The diff is tiny (~10 C++ lines) and directly mirrors upstream JSC's DeferredWorkTimer::doWork, which is a strong correctness signal. But the code path is load-bearing for every DeferredWorkTimer client — not just FinalizationRegistry but also Atomics.waitAsync and Wasm streaming compilation — and worker-termination races are historically subtle in this repo (the test file itself is a catalogue of prior UAFs and thread-affinity asserts). A maintainer who owns Bun's JSC integration should confirm that silently dropping the task (rather than, e.g., cancelling the ticket or clearing the exception) is the intended semantics for all three clients, and coordinate the merge order with #30857 which the description says touches the same block.

Other factors

• I checked that scriptExecutionStatus is a non-null method-table entry for both Zig::GlobalObject and the NodeVM global (ZigGlobalObject.h:208, NodeVM.cpp:906), so the indirect call is safe.
• job->ticket->target() and ->scriptExecutionOwner() are only reached after pendingTicket && !pendingTicket->isCancelled(); the pre-existing JSCDeferredWorkTask::vm() already dereferences scriptExecutionOwner() unconditionally, so no new null-deref surface is introduced.
• On the main thread, scriptExecutionStatus returns Running and there's no termination exception, so the guard is a no-op — no behaviour change for the common case.
• The test is well-structured (subprocess spawn, concurrent pipe drain, unconditional {stderr, stdout, exitCode, signalCode} assertion) and the one CodeRabbit review comment was addressed and resolved.
• No prior claude[bot] review on this PR.

[coderabbitai]

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 `@src/jsc/bindings/JSCTaskScheduler.cpp`:
- Around line 55-64: Update the scheduling path around the termination check and
Bun__queueJSCDeferredWorkTaskConcurrently so termination cannot begin between
the check and enqueue. Serialize the m_isTerminating transition with enqueue, or
implement an equivalent cancellation-safe handoff that removes or cancels any
task raced by teardown while preserving onCancelPendingWork balancing.

🪄 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: a15d5590-6e21-469b-b5d6-1d9698b125ea

📥 Commits

Reviewing files that changed from the base of the PR and between 092a32e and 91d991a.

📒 Files selected for processing (5)

• src/jsc/bindings/JSCTaskScheduler.cpp
• src/jsc/bindings/JSCTaskScheduler.h
• src/jsc/bindings/ZigGlobalObject.cpp
• src/jsc/bindings/webcore/Worker.cpp
• test/js/web/workers/worker-terminate-lifetime.test.ts

[claude]

91d991a applied the destructOnExit sibling store — all three of my earlier findings are now addressed and I have nothing further. Deferring to a human given this is worker-VM teardown / JSC DeferredWorkTimer plumbing with cross-thread atomics.

What was reviewed:

• runPendingWork gate matches JSC's DeferredWorkTimer::doWork (scriptExecutionStatus == Running && !hasPendingTerminationException).
• onScheduleWorkSoon gates on the new dedicated m_isTerminating atomic (not the transient VM::hasTerminationRequest), set from both teardown mirrors.
• onCancelPendingWork on the drop path balances the onAddPendingWork event-loop ref; job is deleted on the skipped-run path.
• Test follows the file's existing subprocess pattern; asserts stderr unconditionally per earlier feedback.

Extended reasoning...

Overview

The PR adds two guards to Bun's JSCTaskScheduler (the DeferredWorkTimer replacement) so that FinalizationRegistry / Atomics.waitAsync / Wasm-streaming tasks are dropped once a worker is terminating: a run-time gate in runPendingWork mirroring JSC's own DeferredWorkTimer::doWork, and a schedule-time gate in onScheduleWorkSoon keyed on a new std::atomic<bool> m_isTerminating. The atomic is set from both WebWorker__teardownJSCVM and Zig__GlobalObject__destructOnExit. A debug/ASAN-gated regression test spawns a subprocess that terminates workers with pending FinalizationRegistry cleanup.

Files touched: JSCTaskScheduler.{cpp,h}, one-line stores in Worker.cpp and ZigGlobalObject.cpp, and worker-terminate-lifetime.test.ts.

Review history on this PR

I've left three prior inline findings, each addressed by a subsequent commit:

• Botched force-push that stripped the src change → fixed in 3ba6d4a.
• onScheduleWorkSoon gating on transient VM::hasTerminationRequest() (which node:vm {timeout}/{breakOnSigint} sets and clears) → replaced with a dedicated non-transient atomic in 69525d3.
• Missing sibling store in Zig__GlobalObject__destructOnExit → added in 91d991a.

The bug hunting system found nothing new at HEAD.

Security risks

None identified. This is teardown-path lifecycle management; no user input parsing, auth, or crypto.

Level of scrutiny

High. This is JSC bindings code on the worker VM teardown path, involving cross-thread atomics with explicit acquire/release ordering, GC finalization callbacks that can run off the JS thread (Atomics.notify), and TerminationException sequencing. CLAUDE.md's memory-safety and thread-affinity sections flag exactly this class of change as the most-blocked category. The PR has already been through three correctness iterations during review, which itself signals the subtlety.

Other factors

• The runPendingWork change is a direct port of upstream JSC's DeferredWorkTimer::doWork behavior, which lowers risk for that hunk.
• The m_isTerminating atomic is set-once/never-cleared, read with acquire against a release store — the ordering choice is defensible and commented.
• The PR description notes an adjacent PR (#30857) touching the same block; whichever lands second needs a rebase, worth a human confirming the interaction.
• The test is debug/ASAN-only (the assert compiles out in release), which is appropriate but means release CI won't exercise it.

Given all prior feedback is addressed and no new issues surfaced, this looks correct to me — but it is not the kind of mechanical change I'd approve without a human maintainer's eyes on worker teardown sequencing.

[robobun]

CI on 91d991a (build 73453): the new worker-terminate-lifetime.test.ts case passed on the x64-asan lane. Remaining failures:

• test-worker-message-port-transfer-terminate.js SIGABRT with getOwnPropertyDescriptor assert on x64-asan: this is ASAN CI: JSC assertion in JSObject::getOwnPropertyDescriptor during worker terminate (test-worker-message-port-transfer-terminate) #34095 (lazy property builder path, addressed by worker_threads: don't abort when terminate() interrupts a lazy property builder #33418 / process: defer termination across lazy PropertyCallback builders #33966), intermittent on main, unrelated to the DeferredWorkTimer path changed here.
• bun-install-registry / bun-install-proxy / terminal-platform-gaps / transpiler-cache / fs.watchFile on Windows/x64-asan, all tagged flaky and unrelated to this diff.

Diff is ready for review.

[robobun]

This also fixes test/js/web/timers/timer-heap-race.test.ts going red on x64-asan after #33131 (build 73570), except the race fixture still leaks ~1/10 on this branch because a cross-thread Atomics.notify can enqueue a JSCDeferredWorkTask before m_isTerminating is set, and __bun_release_task_at_shutdown re-queues it into a buffer that outlives worker VM dealloc. #34293 adds that arm plus an LSan test targeting the race fixture; the onScheduleWorkSoon gate is the same as here, so whichever merges first the other is a small rebase.

[robobun]

Rebased onto current main. The m_isShuttingDown mechanism that landed since this PR was opened subsumes the onScheduleWorkSoon / m_isTerminating half of the original diff, so only the runPendingWork gate remains.

CI (build 78046): the new worker-terminate-lifetime.test.ts case passed on x64-asan. Remaining failures are all unrelated to this diff:

• test-net-connect-memleak.js and test-gc-http-client-connaborted.js on ubuntu x64: both also red on main at the same base commit (build 77601 at 47597ab), reported to main-break triage.
• proxy-stress-errors, napi, dev-server-ssr-100, no-orphans, 26143 on debian/darwin/windows: tagged flaky.

Ready for review.

[claude]

No issues found in the rebased diff — the remaining change is the runPendingWork guard mirroring JSC's DeferredWorkTimer::doWork, plus the regression test. Deferring to a human because this sits in the worker-termination / JSC VM lifecycle path.

What was reviewed:

• The scriptExecutionStatus + hasPendingTerminationException gate matches JSC's own dispatch check; all Bun global-object method tables (Zig, NodeVM, Bake) populate scriptExecutionStatus, so the vtable call is never null.
• Confirmed the m_isShuttingDown mechanism now on main covers what the earlier onScheduleWorkSoon/m_isTerminating hunk did, so dropping it in the rebase leaves no gap.
• Test follows the file's conventions (unconditional stderr assert, signalCode: null, debug/ASAN-gated) and the earlier CodeRabbit feedback is applied.

Extended reasoning...

Overview

After the rebase onto current main, the PR is now two files: a ~12-line addition to runPendingWork in src/jsc/bindings/JSCTaskScheduler.cpp that gates job->task(...) on scriptExecutionStatus == Running && !vm.hasPendingTerminationException(), and a new debug/ASAN-gated case in test/js/web/workers/worker-terminate-lifetime.test.ts. The earlier onScheduleWorkSoon / m_isTerminating half of the diff was dropped because the m_isShuttingDown mechanism that landed on main since (visible in the current file: onAddPendingWork, onScheduleWorkSoon, markShuttingDown, Bun__deleteDeferredWorkTask) already covers scheduling during teardown. I checked that the dropped hunk's intent is fully subsumed — onScheduleWorkSoon now holds m_lock across the m_isShuttingDown check and the enqueue, and both teardown paths flip it via markShuttingDown.

Security risks

None. This is a debug-assertion fix in the worker-termination path; the guard only ever drops work that would otherwise run against a terminating VM. No user-controlled input reaches the new code, and the failure mode being fixed is a debug/ASAN abort rather than a release-build behaviour change.

Level of scrutiny

Medium-high. The logic change is tiny and directly mirrors JSC's upstream DeferredWorkTimer::doWork (the code comment says so, and the PR description traces the exact assertion path), so the correctness argument is straightforward. But it lives in the JSC VM lifecycle / worker-termination path, which the review history of this PR itself shows is subtle (transient hasTerminationRequest vs. permanent teardown, cross-thread Atomics.notify, mirror-path divergence with Zig__GlobalObject__destructOnExit). Those earlier concerns are all resolved — either by the rebase making them moot or by the m_isShuttingDown machinery now on main — but the area warrants a maintainer's eyes.

Other factors

• I verified every method table in Bun (ZigGlobalObject.cpp:957/985, NodeVM.cpp:906, BakeGlobalObject.cpp:220) sets scriptExecutionStatus non-null, so the new globalObjectMethodTable()->scriptExecutionStatus(...) call cannot be a null deref regardless of which global the ticket's owner belongs to. The PR description also notes main-thread vm.runInNewContext(..., {timeout}) was tested and cleanups still fire, since scriptExecutionStatus reports Running there.
• The test matches the file's established pattern: subprocess spawn with bunEnv, concurrent stdout/stderr/exited drain, unconditional stderr assertion first, signalCode: null to catch SIGABRT, and test.skipIf(!isASAN && !isDebug) since the assert is compiled out in release.
• All three of my prior inline findings (empty src diff, transient hasTerminationRequest, destructOnExit mirror) and CodeRabbit's stderr-gating comment are resolved; the rebase makes two of them structurally moot. No outstanding reviewer comments remain.
• The bug hunting system found nothing on the current revision.

[robobun]

Superseded by #34278 and #34293. #33911 is now closed: the worker-terminate-vs-fetch race no longer crashes on main (50/50 clean on df84f8d vs reliable panic on a build predating both).