worker: mark the context terminating before the final concurrent-queue drain

[robobun]

Problem

test/js/node/test/parallel/test-worker-stdio-flush.js went red on the debian 13 x64-asan lane of build 73374 with:

==18202==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #9  ConcurrentTask::new src/event_loop/ConcurrentTask.rs:305
    #10 ConcurrentTask::create src/event_loop/ConcurrentTask.rs:319
    #12 bun_jsc::virtual_machine_exports::queue_task_concurrently src/jsc/virtual_machine_exports.rs:140
    #13 ScriptExecutionContext::postTaskConcurrently src/jsc/bindings/ScriptExecutionContext.cpp:266
    #14 ScriptExecutionContext::postTaskTo src/jsc/bindings/ScriptExecutionContext.cpp:125
    #15 MessagePortPipe::scheduleDrain src/jsc/bindings/webcore/MessagePortPipe.cpp:74
    #16 MessagePort::postMessage src/jsc/bindings/webcore/MessagePort.cpp:143

The leaked allocation is a ConcurrentTask (and the EventLoopTask it wraps) left in an exiting worker's concurrent_tasks queue after the queue has been drained for the last time.

Cause

WebWorker::shutdown() runs process.on('exit') handlers, then drains the worker's concurrent queue via release_queued_tasks_for_shutdown(), then enters WebWorker__teardownJSCVM which (first thing) calls ctx->markTerminating(). ScriptExecutionContext::postTaskTo already refuses to enqueue onto a terminating context, but between the drain and the flag flip there is a short window where a cross-thread poster still sees isTerminating() == false and enqueues.

In the failing test the worker writes to process.stdout inside its exit handler. The parent's captured-stdout reader acks each chunk with port.postMessage(true) (src/js/node/worker_threads.ts makePortReadable._read), which routes through MessagePortPipe::scheduleDrain to postTaskTo(workerCtxId, ...). When the ack lands in that window it is pushed onto the worker's concurrent_tasks; nothing drains it again, and the worker's VM box is dealloc'd raw, so LSan reports the ConcurrentTask as a direct leak.

The window is a few assignments plus one FFI call wide, so it hits probabilistically; the release-asan build is fast enough to line up occasionally, debug essentially never.

The ordering was introduced in #31216; #29917 described the same gap ("a task posted between this drain and removeFromContextsMap() inside teardownJSCVM still leaks") but left it open.

Fix

• ScriptExecutionContext::markTerminating() now takes allScriptExecutionContextsMapLock, the same lock postTaskTo holds across its isTerminating() check and postTaskConcurrently() enqueue. That makes the flag flip a proper fence against concurrent posters: any postTaskTo critical section either runs entirely before markTerminating() (its task is visible to the subsequent drain) or entirely after (it observes true and drops).
• WebWorker::shutdown() calls the new extern "C" ScriptExecutionContext__markTerminating immediately before release_queued_tasks_for_shutdown(), closing the window. The later markTerminating() inside WebWorker__teardownJSCVM is now redundant but harmless.

No behaviour change for process.on('exit') itself: that runs before the new call, so a parent ack posted while the handler is running is still enqueued and then freed by the drain (never executed, same as before). Only posts that would have landed after the drain are now dropped instead of leaked.

Verification

The gap is too narrow to reproduce unassisted against a debug build: 200 iterations of the Node test with the CI LSan env, and 150 worker shutdowns with 64 Atomics-synchronized MessagePorts each, all pass on an unpatched bun bd. Widening the gap with a temporary std::thread::sleep(5ms) between release_queued_tasks_for_shutdown() and WebWorker__teardownJSCVM makes it deterministic:

build	test-worker-stdio-flush.js under LSan	200-port Atomics-synchronized probe
unpatched + 5 ms sleep	5/5 leak (32 byte(s) ConcurrentTask)	5/5 leak
this PR + 5 ms sleep	10/10 clean	5/5 clean
this PR (no sleep)	50/50 clean	clean

test/js/node/worker_threads/worker-shutdown-post-leak.test.ts runs the worker-stdio-on-exit scenario under detect_leaks=1 as an ASAN-lane guard (in a fresh file so it actually runs; worker_destruction.test.ts is ASAN-quarantined via test/expectations.txt). The race is not observable on the debug gate without src/ instrumentation, so the fail-before half will not fire there; test-worker-stdio-flush.js on the release-asan lane remains the primary signal.

Related: #31216 (introduced the ordering), #29917 (described but left the remaining window).

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/worker_threads/worker-shutdown-post-leak.test.ts

[robobun]

^{Updated 6:31 PM PT - Jul 15th, 2026}

❌ @robobun, your commit 85398c0 has 1 failures in Build #73470 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 34278

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

bun-34278 --bun

[coderabbitai]

Walkthrough

Changes

Worker shutdown now marks the associated ScriptExecutionContext as terminating before queued task draining and JSC teardown. A native binding exposes this transition, and an ASAN/LSAN worker destruction test validates clean process exit.

Worker termination coordination

Layer / File(s)	Summary
Termination state contract and native binding src/jsc/bindings/ScriptExecutionContext.h, src/jsc/bindings/ScriptExecutionContext.cpp	markTerminating() synchronizes termination state updates and is exposed through ScriptExecutionContext__markTerminating.
Shutdown integration and destruction validation src/jsc/web_worker.rs, test/js/node/worker_threads/worker_destruction.test.ts	Worker shutdown marks the context before draining queued work, while an ASAN/LSAN test checks clean worker destruction.

Possibly related issues

• [CRASH] Worker termination races in-flight fetch(), corrupting the event loop's concurrent task queue (two crash signatures) #33911 — Addresses worker termination races involving task or callback enqueueing during VM teardown.

Possibly related PRs

• oven-sh/bun#34154 — Adjusts the worker shutdown sequence to close VM and shutdown handles during teardown.
• oven-sh/bun#34270 — Gates pending task execution during worker termination.

Suggested reviewers: jarred-sumner

🚥 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: marking worker contexts terminating before the final queue drain.
Description check	✅ Passed	It includes a clear problem/cause/fix explanation and a detailed verification section covering the template’s required information.

---

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

[claude]

No bugs found beyond the timeout nit, but this is a cross-thread synchronization change in worker VM teardown — worth a human look at the lock-ordering and shutdown-sequence reasoning.

Checked that the two pre-existing markTerminating() callers (WebWorker__teardownJSCVM, Zig__GlobalObject__destructOnExit) do not hold allScriptExecutionContextsMapLock, so making it acquire that lock does not deadlock.
Verified the second markTerminating() inside teardownJSCVM is idempotent (atomic store of true under the same lock).
Confirmed postTaskTo (both overloads) holds the lock across the isTerminating() check + enqueue, so the new fence closes the described window; ensureOnContextThread does not check isTerminating() but is pre-existing and unchanged here.

Extended reasoning...

Overview

The PR closes a narrow race in worker shutdown where a cross-thread postTaskTo could enqueue a ConcurrentTask onto a worker's concurrent queue after the queue's final drain but before the context was marked terminating, leaking the task under LSan. It touches four files: ScriptExecutionContext.{h,cpp} (moves markTerminating() out-of-line and makes it take allScriptExecutionContextsMapLock; adds an extern "C" shim), src/jsc/web_worker.rs (declares the shim and calls it in shutdown() immediately before release_queued_tasks_for_shutdown()), and adds an ASAN-gated regression test.

Security risks

None identified. This is an internal shutdown-ordering fix; no user-controlled input, auth, or crypto is involved. The change adds a lock acquisition on an existing internal lock and reorders an atomic flag flip relative to a queue drain.

Level of scrutiny

High. This is memory-safety-adjacent concurrency code in the worker VM teardown path — exactly the "most-blocked category" per repo review norms. The reasoning about allScriptExecutionContextsMapLock serializing markTerminating() against postTaskTo's check-then-enqueue is sound on inspection, and I verified neither pre-existing caller (WebWorker__teardownJSCVM in Worker.cpp:649, Zig__GlobalObject__destructOnExit in ZigGlobalObject.cpp:4024) holds the lock, so the new acquisition can't self-deadlock. But subtle lock-ordering and "which thread observes what when" arguments in VM teardown warrant a human maintainer confirming the model matches their understanding.

Other factors

• The PR description is unusually thorough (mechanism, verification table with instrumented sleep, related PRs) and the in-code comments precisely restate the invariant, which raises confidence.
• The new call is placed after close_all_socket_groups (which fires JS on_close callbacks) and before the drain — correct: JS callbacks may still trigger legitimate cross-thread posts that the drain will then reclaim.
• ensureOnContextThread also calls postTaskConcurrently under the same lock without an isTerminating() gate; that's a pre-existing gap not introduced or widened here, so I did not flag it.
• The bug hunt found only a nit (explicit 30_000 test timeout violates test/CLAUDE.md); the "Set once" header comment was examined and ruled out as stale (it describes monotonicity, not call-count).
• The test is honestly labeled as a guard that won't fail-before on debug; given the race requires src/ instrumentation to reproduce deterministically, that's acceptable but means the test alone doesn't prove the fix.

[claude]

No issues found, but this is a cross-thread ordering change in the worker VM teardown path (markTerminating() now takes allScriptExecutionContextsMapLock, and shutdown() flips the flag before the final concurrent-queue drain), so it's worth a human sign-off.

Checked: the two other markTerminating() callers (WebWorker__teardownJSCVM, Zig__GlobalObject__destructOnExit) hold no locks at the call site, so the new lock acquisition can't deadlock. postTaskTo's check-then-enqueue was already under the same lock, so the fence argument holds. The earlier timeout nit was addressed in 6d25f45.

Extended reasoning...

Overview

The PR closes a narrow race in worker shutdown where a cross-thread postTaskTo (parent-side MessagePort ack for worker stdio) lands between release_queued_tasks_for_shutdown() and ctx->markTerminating(), leaking a ConcurrentTask under LSan. It (a) moves ScriptExecutionContext::markTerminating() out-of-line and has it take allScriptExecutionContextsMapLock so the flag flip is a proper fence against postTaskTo's check-then-enqueue critical section, (b) exposes an extern "C" shim, and (c) calls it from WebWorker::shutdown() immediately before the final drain. An ASAN-gated regression test spawns the worker-stdio-on-exit scenario under detect_leaks=1.

Security risks

None. This is a shutdown-ordering / leak fix; no user-controlled input, no auth/crypto/permissions surface.

Level of scrutiny

High. src/jsc/web_worker.rs::shutdown() is one of the most delicately ordered functions in the runtime (every step is commented as a barrier for the next), and markTerminating() gaining a lock changes its contract for every caller. I verified the other two call sites (WebWorker__teardownJSCVM at Worker.cpp:649 and Zig__GlobalObject__destructOnExit at ZigGlobalObject.cpp:4024) hold nothing that could invert against allScriptExecutionContextsMapLock, and that postTaskConcurrently (called under that lock in postTaskTo) doesn't re-enter the map. The reasoning in the PR description is sound and the fix is minimal, but concurrency changes in VM teardown are exactly the category where a maintainer familiar with #31216/#29917 should confirm the ordering.

Other factors

• The new test is honestly labeled as a guard that won't fail-before on debug (the race needs release-asan timing or an injected sleep). It follows harness conventions: bunEnv spread, combined {stdout, stderr, exitCode} assertion, await using for the subprocess, concurrent pipe drains.
• ensureOnContextThread also enqueues cross-thread under the same lock but does not check isTerminating(); that's a pre-existing gap this PR does not claim to address, and the LSan trace was specifically postTaskTo.
• The prior inline nit (explicit 30_000 timeout) was resolved in 6d25f45 and the thread is marked resolved.
• CI build #73444 is still in flight per the robobun comment.

[claude]

No issues found, but this reorders cross-thread synchronization in worker VM teardown (new lock acquisition in markTerminating(), new FFI call in shutdown()'s step 2), so it's worth a human look.

Checked: allScriptExecutionContextsMapLock is not held at any of the three markTerminating() call sites (Worker.cpp:649, ZigGlobalObject.cpp:4024, new FFI export) — no re-entrant/recursive lock. vm_lock is released before the new call in shutdown(), so no lock-ordering inversion. ensureOnContextThread still lacks the isTerminating() gate, but that's pre-existing and not the path in the LSan trace. The earlier timeout nit was addressed in 6d25f45.

Extended reasoning...

Overview

The PR closes a narrow race in WebWorker::shutdown() where a cross-thread postTaskTo could enqueue onto the worker's concurrent queue after the final drain but before markTerminating() flipped the flag inside WebWorker__teardownJSCVM. The fix: (1) move markTerminating() out-of-line and have it take allScriptExecutionContextsMapLock so it serializes against postTaskTo's check-then-enqueue critical section; (2) call it from Rust immediately before release_queued_tasks_for_shutdown(). A new ASAN-only LSan guard test is added.

Security risks

None. This is internal shutdown-ordering; no user-controlled input, auth, or crypto surface is touched.

Level of scrutiny

High. Worker VM teardown is one of the most memory-safety-critical paths in the runtime — it interleaves JSC heap finalization, cross-thread task posting, and raw dealloc of the VM box. Adding a lock acquisition to markTerminating() changes the contract for all three of its callers, and shutdown-sequence reordering is exactly where subtle UAF/deadlock regressions hide. CLAUDE.md flags native memory safety and thread affinity as the most-blocked review category.

Other factors

• Verified no re-entrant lock: none of the existing markTerminating() callers (Worker.cpp:649 in WebWorker__teardownJSCVM, ZigGlobalObject.cpp:4024 in global exit) hold allScriptExecutionContextsMapLock when calling it, and WTF::Lock is non-recursive, so the new lock is safe there.
• Verified no lock-ordering issue at the new Rust call site: vm_lock is released in shutdown step 1 before ScriptExecutionContext__markTerminating runs in step 2.
• ensureOnContextThread (ScriptExecutionContext.cpp:185) holds the same lock but does not gate on isTerminating(); a task posted through it could still land after the flag flip. That path isn't in the LSan stack trace and is pre-existing, so it's out of scope, but worth a maintainer's awareness.
• The previous inline nit (explicit test timeout) was addressed in 6d25f45, and b1a4e05 moved the test to its own file. No outstanding review comments remain.
• The reasoning in the PR description and inline comments is thorough and the fix is minimal, but concurrency ordering in this path deserves a maintainer's sign-off rather than auto-approval.

[claude]

I didn't find any correctness issues, but this changes lock semantics in the worker VM teardown path, so it's worth a human look.

What was reviewed:

• markTerminating() now taking allScriptExecutionContextsMapLock — checked all three call sites (WebWorker__teardownJSCVM, Zig__GlobalObject__destructOnExit, the new FFI shim); none hold the map lock already, so no self-deadlock, and WTF::Lock re-acquisition on the redundant second call is fine.
• Placement in shutdown() — runs after close_all_socket_groups (whose on_close callbacks still see isTerminating()==false, unchanged) and before the drain, so no behavior change for exit handlers.
• ensureOnContextThread still doesn't gate on isTerminating(); that's a pre-existing separate path, not a regression here.
• The earlier nit (explicit test timeout) was addressed in 6d25f45.

Extended reasoning...

Overview

The PR closes a narrow race in WebWorker::shutdown() where a cross-thread postTaskTo could enqueue a ConcurrentTask between the final release_queued_tasks_for_shutdown() drain and the markTerminating() call inside WebWorker__teardownJSCVM, leaking the task under LSan. It moves markTerminating() out-of-line so it takes allScriptExecutionContextsMapLock (the same lock postTaskTo holds across its check-and-enqueue), adds an extern "C" shim, and calls it from Rust immediately before the drain. Four files touched: ScriptExecutionContext.{h,cpp}, web_worker.rs, plus a new ASAN-gated test.

Security risks

None. No user-controlled input, no auth/crypto, no external I/O — this is internal lifetime/ordering during worker teardown.

Level of scrutiny

High. The diff is small (~30 native lines), but it changes locking behavior on a hot cross-thread coordination path (worker VM teardown ↔ parent-thread MessagePort posting). I verified the three existing markTerminating() callers do not already hold the contexts-map lock (WTF::Lock is non-recursive), that the new call site in shutdown() sits after on_exit()/close_all_socket_groups so user-visible exit-handler behavior is unchanged, and that the redundant second call in teardownJSCVM is harmless. The reasoning in the PR description is sound and matches the code. That said, worker teardown ordering has historically been delicate (this PR itself references #31216/#29917), so a maintainer familiar with that history should sign off.

Other factors

The new test is ASAN-lane-only and the PR acknowledges the race is not reproducible on debug builds without instrumentation, so test-worker-stdio-flush.js on the release-asan lane remains the real signal — CI results on build #73470 should be checked. My previous inline nit (explicit 30_000 timeout) was addressed and the thread is resolved. No outstanding reviewer comments.

[robobun]

CI on builds #73458 and #73470: the diff is green. test-worker-stdio-flush.js and the new worker-shutdown-post-leak.test.ts both pass on the release-asan lane. Remaining red lanes are unrelated to this change:

• test/js/node/test/parallel/test-net-connect-memleak.js on alpine x64 / x64-baseline (pre-existing on main; GC-timing assertion, owned separately)
• test/regression/issue/30205.test.ts NapiFinalizerTask leak on asan (flaky; also in the original reporting build 73374 before this change existed)
• pglite.test.ts / s3.test.ts on Windows aarch64, no-orphans.test.ts / vendor/elysia/.../stop.test.ts on macOS 14 x64, test-net-write-slow.js on macOS 26 aarch64 (all flaky, single-retry)

Ready for review.