worker: drain queued cross-thread tasks on termination

[robobun]

Problem

ScriptExecutionContext::postTaskConcurrently heap-allocates an EventLoopTask, and Bun__queueTaskConcurrently wraps it in a heap-allocated auto_delete ConcurrentTask that is pushed onto the worker's concurrent_tasks queue. The only free path for both is the target event loop actually ticking the task.

When any cross-thread task lands in a worker's queue after the worker has left its spin() loop but before ~GlobalObject removes its ScriptExecutionContext from the global map, the task and everything it references leaks until process exit. With Worker.postMessage's drain task this transitively holds a Ref<Worker>, which keeps the entire m_toWorker.queue of buffered SerializedScriptValues alive. In long-running servers that repeatedly spawn and terminate workers, this accumulates unbounded.

Reproduction

Worker signals (via SharedArrayBuffer) when it reaches process.on('beforeExit') — past its event loop, before JSC teardown. Parent waits for that signal, then floods postMessage; the first one posts a drain task that the worker will never tick.

import { Worker } from "worker_threads";
const sab = new SharedArrayBuffer(8);
const arr = new Int32Array(sab);
const w = new Worker(`
  const arr = new Int32Array(require("worker_threads").workerData);
  process.on("beforeExit", () => {
    Atomics.store(arr, 0, 1); Atomics.notify(arr, 0);
    const t = Date.now(); while (Date.now() - t < 100) {}
  });
`, { eval: true, workerData: sab });
Atomics.wait(arr, 0, 0);
for (let i = 0; i < 300; i++) w.postMessage(new ArrayBuffer(128 * 1024));
await new Promise(r => w.once("exit", r));

Repeated 20× this leaks ~890 MB on main.

Fix

Call EventLoop::drop_concurrent_cpp_tasks() (added in #30875 for the main-thread global_exit path) from WebWorker::shutdown() at the end of step 2, immediately before WebWorker__teardownJSCVM. This matches global_exit's ordering: deleting an EventLoopTask after ~VM can run ~JSEventListener against freed Weak-handle storage (a nested worker's close task captures Ref<WorkerB> whose listener Weak handles live in this worker's heap).

This is the worker-side complement to #30875's main-thread drain: that one frees tasks posted from workers to the main thread on process exit; this one frees tasks posted to a terminating worker. A task posted between this drain and removeFromContextsMap() inside teardownJSCVM still leaks — same as before this change, just a much smaller window.

Verification

test/js/web/workers/worker-terminate-pending-message-leak.test.ts — 4 warm-up rounds then 20 measured rounds of the reproduction above, asserting RSS grows by less than isASAN ? 300 : 100 MB. Skipped on Windows (Atomics.wait on main thread + RSS accounting not reliable there).

	without fix	with fix
run 1	892.71 MB	pass (−24.83 MB)
run 2	880.05 MB	pass (−25.83 MB)
run 3	—	pass (−22.16 MB)

[robobun]

Status

Closed as superseded. The worker-side concurrent-queue drain this PR adds landed on main via #31216 -> #34278 -> #37075 (VirtualMachine::teardown phase B, release_queued_work(), runs before the JSC VM is destroyed for workers too). drop_concurrent_cpp_tasks() no longer exists.

Verified on unpatched main @ 1805964: the repro grows RSS 5-16 MB over 20 rounds (was 880-893 MB), and this PR's test passes without the src/ change. See the closing comment below.

[github-actions]

---

Found 2 issues this PR may fix:

1. Bun leaks memory in Workers #5709 - Directly reports unbounded RSS growth when spawning/terminating workers in a loop, exactly the leak pattern this PR task-draining fix addresses
2. macOS Apple Silicon: memory invisible to RSS — bmalloc slabs, worker cleanup gaps, GC safety bugs #28318 - Section 2 identifies incomplete worker thread cleanup; this PR fixes one of those gaps by draining queued cross-thread tasks on worker exit

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

Fixes #5709
Fixes #28318

🤖 Generated with Claude Code

---

[coderabbitai]

Walkthrough

Implements cleanup for pending event-loop tasks during worker termination. Adds a C++ function to delete EventLoopTask objects, an event-loop drain routine to clear cancelled tasks, and updates worker exit logic to call this cleanup. Includes a regression test verifying no memory leaks when workers terminate with pending postMessage calls.

Changes

Cohort / File(s)	Summary
Event Loop Task Deletion src/bun.js/bindings/ZigGlobalObject.cpp, src/bun.js/event_loop/CppTask.zig	New C++ exported function Bun__deleteEventLoopTask and CppTask.deinit method to explicitly delete unexecuted EventLoopTask objects without invoking performTask.
Event Loop Drain Routine src/bun.js/event_loop.zig	New drainCancelledTasks method to pop and iterate pending concurrent tasks, deferring cleanup to safely read next pointers, and freeing unrun Task wrappers.
Worker Termination Integration src/bun.js/web_worker.zig	Calls drainCancelledTasks after JSC VM teardown to clean up EventLoopTask/ConcurrentTask pairs from pending postMessage operations, preventing memory leaks.
Memory Leak Regression Test test/js/web/workers/worker-terminate-pending-message-leak.test.ts	New test spawning child process with worker that receives 300 postMessage calls while terminating; measures RSS delta across 20 GC-instrumented rounds to detect leaks.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding task draining for worker termination to prevent queued task leaks.
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.
Description check	✅ Passed	The PR description is comprehensive and detailed, covering the problem, reproduction, fix, and verification with examples.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/web/workers/worker-terminate-pending-message-leak.test.ts`:
- Around line 71-74: The test currently asserts that proc.stderr is empty (using
the stderr variable) but on ASAN/debug shards a known startup warning can
appear; update the assertion to first split stderr into lines, remove any line
that begins with "WARNING: ASAN interferes" (filter out that known ASAN startup
noise), then assert that the remaining stderr lines are empty before checking
stdout contains "PASS" and exitCode; update the block around the Promise.all
result/expect calls (variables proc, stdout, stderr, exitCode) to perform this
filtering step.

🪄 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: aec74410-f483-42cb-bc5e-221317393d2c

📥 Commits

Reviewing files that changed from the base of the PR and between 1ba8712 and aa695f6.

📒 Files selected for processing (5)

• src/bun.js/bindings/ZigGlobalObject.cpp
• src/bun.js/event_loop.zig
• src/bun.js/event_loop/CppTask.zig
• src/bun.js/web_worker.zig
• test/js/web/workers/worker-terminate-pending-message-leak.test.ts

[claude]

I didn't find any bugs, but this adds a new step to the worker-thread teardown sequence (exitAndDeinit) with cross-thread ordering invariants around ScriptExecutionContext removal and concurrent-queue draining, plus a new C++ delete path for EventLoopTask — worth a human look to confirm the race-freedom claim and that ~EventLoopTask releases the captured SerializedScriptValue as expected.

Extended reasoning...

Overview

This PR fixes a memory leak where EventLoopTask + ConcurrentTask pairs (and their captured SerializedScriptValue payloads) leak when a worker terminates with cross-thread messages still queued. It adds Bun__deleteEventLoopTask (C++), CppTask.deinit() (Zig binding), EventLoop.drainCancelledTasks() / freeUnrunTask(), and inserts a drain call into WebWorker.exitAndDeinit immediately after WebWorker__teardownJSCVM. A new RSS-growth regression test is included.

Security risks

None apparent — this is internal memory cleanup on a teardown path with no user-controlled input beyond what was already accepted by postMessage. No auth, crypto, or permission surfaces touched.

Level of scrutiny

High. exitAndDeinit is a carefully-ordered, heavily-documented teardown sequence with explicit threading invariants (see the ~90-line file header in web_worker.zig). The fix's correctness rests on the claim that ~GlobalObject → removeFromContextsMap() (inside WebWorker__teardownJSCVM) plus allScriptExecutionContextsMapLock in postTaskTo together guarantee no new enqueues race the drain — that's a cross-language (C++/Zig), cross-thread invariant that deserves a human reviewer who knows the ScriptExecutionContext locking model. Additionally, freeUnrunTask only frees CppTask and ManagedTask variants and silently skips other Task union members; that's likely intentional for this code path but is a design choice worth confirming. The new delete task path also assumes EventLoopTask's destructor (not its performTask) is sufficient to release the SerializedScriptValue lambda capture.

Other factors

• The drain reads vm.event_loop after WebWorker__teardownJSCVM has torn down the JSC VM but before vm.deinit() — the Zig EventLoop struct should still be valid here, but it's another ordering subtlety.
• CodeRabbit flagged that the new test's expect(stderr).toBe("") may flake on ASAN shards without filtering the known WARNING: ASAN interferes line; that's unaddressed.
• The change is well-motivated, well-documented, and includes a regression test with before/after numbers, but the blast radius (worker teardown for every worker in every Bun process) and concurrency subtlety put it outside the auto-approve bar.

[claude]

I didn't find any bugs, but this touches concurrency-sensitive worker teardown and event loop internals — the correctness argument hinges on postTaskTo holding allScriptExecutionContextsMapLock across the enqueue and on ~GlobalObject having already run removeFromContextsMap() before the drain, so it's worth a human look at those ordering invariants.

Extended reasoning...

Overview

This PR fixes a memory leak where EventLoopTask + ConcurrentTask pairs (and their captured SerializedScriptValue) leak when a worker terminates with cross-thread messages still queued. It adds:

• Bun__deleteEventLoopTask in ZigGlobalObject.cpp (a one-line delete task)
• CppTask.deinit() wrapper in CppTask.zig
• EventLoop.drainCancelledTasks() + freeUnrunTask() in event_loop.zig — pops the concurrent batch and walks the tasks FIFO, freeing CppTask/ManagedTask payloads and auto_delete ConcurrentTask wrappers
• A call to drainCancelledTasks() in WebWorker.exitAndDeinit immediately after WebWorker__teardownJSCVM
• A new RSS-based regression test (skipped on Windows, ASAN-stderr filtered)

Security risks

None identified. This is internal memory cleanup on the worker-thread teardown path; no user-controlled input flows into the new code beyond what was already being processed.

Level of scrutiny

High. The change sits at the intersection of cross-thread task queues, JSC VM teardown, and worker lifecycle ordering. Correctness depends on non-local invariants:

• That ~GlobalObject (run inside WebWorker__teardownJSCVM) has already called removeFromContextsMap() so no new concurrent enqueues can race with popBatch().
• That postTaskTo / postTaskConcurrently holds allScriptExecutionContextsMapLock across the push, closing the window between the map lookup and the enqueue.
• That vm.event_loop (a Zig-side struct on VirtualMachine) remains valid after JSC teardown but before vm.deinit() — which appears true since only the JSC VM is torn down at that point, but is worth confirming.
• That the deferred-destroy pattern for ConcurrentTask nodes (mirroring tickConcurrentWithCount) is correct for the iterator semantics.

A subtle bug here could manifest as a use-after-free or double-free under load rather than a simple test failure.

Other factors

• freeUnrunTask only frees CppTask and ManagedTask payloads; other task variants in the queue at teardown are silently dropped. That's strictly no worse than the status quo (they'd have leaked before too), but a reviewer may want to confirm no other heap-owning task type can land in a worker's queue via this path.
• The new test is RSS-threshold-based (100 MB headroom vs ~20-40 MB observed noise) and skipped on Windows; the description notes 3 pre-existing debug-build flakes in worker.test.ts that are unrelated.
• The CodeRabbit ASAN-stderr comment was addressed in commit 37a7698.
• No CODEOWNERS cover these files.

Given the cross-thread ordering subtleties, I'm deferring rather than approving.

[claude]

I didn't find any bugs, but this touches concurrency-sensitive worker termination and cross-thread event-loop task lifecycle (including a new free path for C++ EventLoopTask that bypasses performTask), so it's worth a human verifying the no-new-enqueues-after-removeFromContextsMap invariant and that freeUnrunTask's coverage of only CppTask/ManagedTask is sufficient.

Extended reasoning...

Overview

This PR fixes a memory leak where EventLoopTask + ConcurrentTask pairs (and their captured SerializedScriptValue/Ref<Worker>) leak when a worker terminates with cross-thread messages still queued. It adds:

• Bun__deleteEventLoopTask (C++): a bare delete task wrapper exported to Zig.
• CppTask.deinit() (Zig): calls the above.
• EventLoop.drainCancelledTasks() + freeUnrunTask() (Zig): pops the concurrent batch and walks the tasks FIFO, freeing auto-delete ConcurrentTask wrappers and inner CppTask/ManagedTask payloads without running them.
• A call site in WebWorker.shutdown() immediately after WebWorker__teardownJSCVM.
• An RSS-delta regression test (skipped on Windows, ASAN-stderr filtered).

Security risks

None identified — this is a teardown-path memory cleanup with no input parsing, auth, or crypto involvement.

Level of scrutiny

High. The change sits at the intersection of cross-thread task queues, worker lifecycle ordering, and C++/Zig heap ownership. Correctness depends on a subtle invariant: after ~GlobalObject → removeFromContextsMap(), no thread can still be mid-postTaskConcurrently pushing onto this worker's concurrent_tasks. The PR description argues postTaskTo holds allScriptExecutionContextsMapLock across its push, which would make the drain race-free — but that's exactly the kind of invariant a maintainer familiar with the WebKit ScriptExecutionContext bindings should confirm. A race here would be a use-after-free on a ConcurrentTask node, not just a residual leak.

Other factors

• freeUnrunTask only handles CppTask and ManagedTask; other Task union variants are silently dropped. That's likely fine for the targeted leak (cross-thread postMessage always produces CppTask), but a human should confirm no other heap-owning task variants can be sitting in tasks/concurrent_tasks at this point in shutdown().
• The deferred-destroy iterator pattern correctly mirrors tickConcurrentWithCount(), and the call site is placed after JSC teardown but before dispatchExit (so this is still valid) and before vm.deinit() (so the FIFO buffer is still live).
• The new RSS-based test has a generous 100 MB threshold vs. ~750 MB+ unfixed delta, uses a SharedArrayBuffer handshake to deterministically hit the teardown window, and follows repo conventions (ASAN filter, isWindows skip, exit-code asserted last).
• CodeRabbit's one comment (ASAN stderr filter) was addressed; no outstanding human review comments.

[claude]

I didn't find any bugs, but this touches concurrency-sensitive worker shutdown ordering and cross-thread task lifecycle (event loop drain after JSC VM teardown), so it's worth a human look — particularly the race-freedom argument that removeFromContextsMap() inside teardownJSCVM fully fences against new postTaskConcurrently enqueues, and whether freeUnrunTask covering only CppTask/ManagedTask is sufficient.

Extended reasoning...

Overview

This PR fixes a memory leak where cross-thread tasks (EventLoopTask + ConcurrentTask wrappers) posted to a worker after it leaves its event loop but before ~GlobalObject removes its ScriptExecutionContext are never freed. It adds:

• Bun__deleteEventLoopTask (C++) and CppTask.deinit() (Zig) to free an EventLoopTask without running it.
• EventLoop.drainCancelledTasks() which pops the concurrent batch and walks the tasks FIFO, freeing auto-delete ConcurrentTask wrappers and inner CppTask/ManagedTask payloads.
• A call to drainCancelledTasks() in WebWorker.shutdown() immediately after WebWorker__teardownJSCVM.
• An RSS-based regression test using a SharedArrayBuffer handshake to deterministically hit the teardown window.

I verified EventLoopTask (src/jsc/bindings/EventLoopTask.h) holds its lambda in a Function<void(ScriptExecutionContext&)> m_task, so delete task correctly destroys captured state (including Ref<Worker>) without needing performTask(). The deferred-destroy pattern in drainCancelledTasks correctly mirrors tickConcurrentWithCount() to avoid reading next from a freed node.

Security risks

None. This is a resource-cleanup change on the worker shutdown path; no auth, crypto, input parsing, or external data handling is involved.

Level of scrutiny

High. This modifies the worker shutdown sequence and event loop task lifecycle — both concurrency-critical. The correctness argument hinges on ordering guarantees:

1. WebWorker__teardownJSCVM → ~GlobalObject → removeFromContextsMap() completes before the drain, and postTaskTo holds allScriptExecutionContextsMapLock across its enqueue, so no new tasks race in. This is asserted in the PR description and code comments but should be verified by someone familiar with the C++ side.
2. vm.event_loop is dereferenced after teardownJSCVM but before vm.deinit() — looks safe since the event loop struct lives in the Zig VM, not the JSC heap, but worth confirming.
3. freeUnrunTask only handles CppTask and ManagedTask; other Task union variants in the queue would be silently dropped. That may be intentional (most other variants are not heap-owned by the queue), but a reviewer familiar with the full Task union should confirm nothing else needs explicit cleanup here.
4. The EventLoopTask lambda destructor (releasing Ref<Worker>) runs on the worker thread during shutdown — Worker is ThreadSafeRefCounted, so this should be fine.

Other factors

• The bug hunting system found no issues.
• The CodeRabbit feedback (ASAN stderr filter, Windows skip) was addressed.
• CI failures in build #49870 (s3-storage-class, no-orphans, next-pages, fetch-http2-client) appear unrelated to this change.
• The test methodology (RSS delta with 100 MB threshold vs ~900 MB observed leak) is reasonable and follows repo conventions for leak tests.
• The change is well-documented with inline comments explaining the ordering constraints, which aids review.

[claude]

Thanks — all three of my earlier points are addressed (drain now precedes teardownJSCVM matching the global_exit ordering, isASAN threshold added, stale comment fixed). I don't see further issues, but given this sits in the worker-thread teardown ordering (where the previous revision had a subtle UAF), I'd like a human to confirm the reasoning around the remaining late-enqueue leak window before merge.

Extended reasoning...

Overview

This PR adds a single call to the existing EventLoop::drop_concurrent_cpp_tasks() inside WebWorker::shutdown() (src/jsc/web_worker.rs), plus a new RSS-delta regression test. It frees cross-thread EventLoopTask/ConcurrentTask pairs that were posted to a worker after it left its event loop but before ~GlobalObject removed its ScriptExecutionContext — previously these (and the Ref<Worker> → buffered SerializedScriptValue queue they hold) leaked until process exit.

Changes since my last review

All three of my prior inline comments have been addressed in the current revision:

• UAF ordering (🔴): the drain call was moved from after WebWorker__teardownJSCVM to before it (now in step 2, preceding step 3), matching the documented contract and the existing global_exit caller at VirtualMachine.rs:1596. The new comment explicitly documents the nested-worker Ref<WorkerB> / ~JSEventListener Weak-handle hazard and the residual late-enqueue leak window.
• ASAN threshold (🟡): the test now imports isASAN and uses ${isASAN ? 300 : 100}, matching the directory convention.
• Stale function name (🟡): test comment now references drop_concurrent_cpp_tasks.

Security risks

None. This is a memory-leak fix in worker teardown; no auth, crypto, input parsing, or privilege boundaries are touched.

Level of scrutiny

Moderate-to-high. The runtime change is one line calling an existing function, and it now mirrors the already-merged global_exit pattern exactly. However, it sits in the cross-thread shutdown ordering of WebWorker::shutdown(), where the previous revision of this same PR had a real UAF due to mis-ordering relative to ~VM. The current placement looks correct and is well-documented (including the honest acknowledgment that tasks posted between the drain and removeFromContextsMap() still leak), but worker-teardown ordering invariants are subtle enough that a maintainer familiar with the JSC VM lifecycle and the nested-worker "Known gap" should confirm.

Other factors

• No CODEOWNERS coverage for src/jsc/web_worker.rs.
• Bug-hunting system found nothing this round.
• The two CI failures (bundler_jsx segfault on macOS aarch64, v8-heap-snapshot SIGKILL on Ubuntu x64) appear unrelated to this change — neither exercises worker teardown.
• Test follows repo conventions (ASAN stderr filter, isWindows skip, isASAN threshold, exit-code assertion last).

[robobun]

Closing as superseded.

While this PR was open, the worker-side drain it adds landed on main through a different route:

• node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector #31216 added a concurrent-queue drain to WebWorker::shutdown().
• worker: mark the context terminating before the final concurrent-queue drain #34278 closed the residual late-enqueue window this PR's description called out (markTerminating() now takes allScriptExecutionContextsMapLock, so postTaskTo either enqueues before the final drain or refuses).
• Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 folded both into the single VirtualMachine::teardown funnel: phase B release_queued_work() -> EventLoop::release_queued_tasks() runs for Teardown::Worker before phase C destroys the JSC VM, and EventLoop::deinit now debug-asserts both queues are empty.

EventLoop::drop_concurrent_cpp_tasks(), which this diff calls, no longer exists.

Verified against unpatched main @ 1805964 (debug build): the reproduction from the description now grows RSS by 4.9 / 10.9 / 15.6 MB over 20 rounds (was 880-893 MB when this PR was opened), and this PR's test passes as-is without the src/ change (2/2). Coverage for posts landing during worker shutdown lives in test/js/node/worker_threads/worker-shutdown-post-leak.test.ts plus the deinit assertion, so the RSS test here would be redundant.