JSCTaskScheduler: drop deferred-work tasks scheduled after the event loop's last tick - #34293
Conversation
…loop's last tick Worker teardown runs release_queued_tasks_for_shutdown, then WebWorker__teardownJSCVM which ends in ~VM(). ~VM() calls WaiterListManager::unregister for every Atomics.waitAsync ticket still pending on that VM, and each one reaches DeferredWorkTimer::scheduleWorkSoon -> JSCTaskScheduler::onScheduleWorkSoon, which allocates a JSCDeferredWorkTask and a ConcurrentTask and enqueues into the worker's concurrent queue. That queue was already drained for the last time, so the nodes become unreachable when the worker's VirtualMachine box is dealloc'd and LSan reports them. Gate onScheduleWorkSoon and onAddPendingWork on a new m_isShuttingDown atomic, set at the start of WebWorker__teardownJSCVM and Zig__GlobalObject__destructOnExit so work scheduled from the final collectNow or ~VM() is dropped instead of enqueued. A JSCDeferredWorkTask can also land in the queue before the flag is set (cross-thread Atomics.notify from another VM while this one is between its last tick and teardown). release_queued_tasks_for_shutdown forwards it into self.tasks where __bun_release_task_at_shutdown didn't recognise the tag, so it was re-queued into a freshly allocated LinearFifo buffer that leaked on worker dealloc. Add a JSCDeferredWorkTask arm that deletes the job via a new Bun__deleteDeferredWorkTask FFI. Surfaced by test/js/web/timers/timer-heap-race.test.ts on the x64-asan lane (build 73570) after #33131 added the cross-thread Atomics.waitAsync fixture.
|
Updated 3:19 PM PT - Jul 16th, 2026
❌ @robobun, your commit 4d9b926 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34293That installs a local version of the PR into your bun-34293 --bun |
WalkthroughChangesDeferred work scheduling now observes VM shutdown, rejects new work, and reclaims queued deferred-work tasks during teardown. VM and worker shutdown paths signal the scheduler, and an ASAN-gated Deferred work shutdown
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 118-124: Release the pending ticket before reclaiming queued jobs:
in src/jsc/bindings/JSCTaskScheduler.cpp lines 118-124, add a cancel-and-delete
FFI entry point that invokes onCancelPendingWork() for job->ticket before
deleting the job; in src/runtime/dispatch.rs lines 1206-1214, bind and call this
new entry point instead of Bun__deleteDeferredWorkTask.
In `@src/jsc/bindings/JSCTaskScheduler.h`:
- Around line 23-31: Serialize shutdown with deferred-work enqueueing: in
src/jsc/bindings/JSCTaskScheduler.h#L23-L31, update
JSCTaskScheduler::markShuttingDown to use the scheduler’s synchronization
protocol; in src/jsc/bindings/JSCTaskScheduler.cpp#L55-L68, hold the same lock
across the m_isShuttingDown check and the ownership handoff to
Bun__queueJSCDeferredWorkTaskConcurrently, so no enqueue can occur after
teardown releases queued tasks.
🪄 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: 7a6e08f6-4bce-49e8-9e29-c8c73d3fe101
📒 Files selected for processing (7)
src/jsc/bindings/JSCTaskScheduler.cppsrc/jsc/bindings/JSCTaskScheduler.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/webcore/Worker.cppsrc/runtime/dispatch.rstest/js/web/timers/timer-heap-atomics-teardown-fixture.tstest/js/web/timers/timer-heap-race.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review: the atomic flag alone leaves a gap where a cross-thread Atomics.notify that reads m_isShuttingDown == false before the worker thread sets it can still enqueue after release_queued_tasks_for_shutdown has drained. Hold m_lock across the check and the enqueue in onScheduleWorkSoon (and across the check in onAddPendingWork, matching the existing lock there), and take the same lock in markShuttingDown. Introduce Bun__JSCTaskScheduler__markShuttingDown so worker shutdown can flip the flag before the drain: a notifier that enqueues under the lock before the flip is visible to the drain via the queue's own release/acquire, and a notifier that acquires the lock after the flip sees the flag and drops. Factor the pending-ticket removal into dropPendingTicketLocked so the shutdown paths (onScheduleWorkSoon's early return and Bun__deleteDeferredWorkTask) balance the event-loop ref and ticket-set entry the same way runPendingWork would have. Relax the new test's stderr assertion to not.stringContaining per the file convention; abort_on_error=1 already turns a leak into SIGABRT.
There was a problem hiding this comment.
Beyond the inline nit, I checked a7c0da0's lock change for ordering hazards — onScheduleWorkSoon now holds m_lock across Bun__queueJSCDeferredWorkTaskConcurrently, but that's a lock-free MPSC push (enqueue_task_concurrent), so no nesting concern. Also verified the onCancelPendingWork → dropPendingTicketLocked refactor is behavior-preserving (else-branch just unlocks slightly earlier). My two earlier nits (drain→flag window on the worker path, stderr: "" assertion) are both addressed.
Extended reasoning...
a7c0da0 addressed all four prior review threads (two CodeRabbit majors, my two nits). The remaining inline finding is the main-thread sibling of the drain→flag ordering fix — nit-level and BUN_DESTRUCT_VM_ON_EXIT-gated. Not approving: this is cross-thread VM-teardown code with a known overlap to coordinate against #34270, so a human should sign off.
Mirror the worker shutdown path so a cross-thread scheduleWorkSoon that races the main-thread BUN_DESTRUCT_VM_ON_EXIT teardown is either caught by the drain or observes m_isShuttingDown under m_lock. The main VM box is static-rooted so LSan would not flag it, but the sibling paths should match.
There was a problem hiding this comment.
bf3fe64 addresses my last note — global_exit() now fences JSCTaskScheduler before both concurrent-queue drains, symmetric with web_worker.rs::shutdown(). No further issues found; deferring to a human for the cross-thread teardown / m_lock-scope changes and the merge-order call vs #34270.
What was reviewed:
m_locknow held acrossBun__queueJSCDeferredWorkTaskConcurrently— verified that path is a lock-free MPSC push + wakeup (JSCScheduler.rs:56-65), no lock-order hazard.Bun__deleteDeferredWorkTaskmirrorsrunPendingWork's ticket take + event-loop-ref balance under the shareddropPendingTicketLockedhelper.global_exit()placement ofmarkShuttingDownbeforedrop_concurrent_cpp_tasksvs. web_worker.rs placing it afterclose_all_socket_groups— ruled out as a problem (onAddPendingWorkalso gates on the flag, so JS run during shutdown that creates new deferred work just drops it).
Extended reasoning...
Overview
This PR gates JSCTaskScheduler::onScheduleWorkSoon/onAddPendingWork on a new lock-protected m_isShuttingDown flag so deferred-work tasks scheduled during VM teardown (~VM → WaiterListManager::unregister, collectNow → JSFinalizationRegistry::finalizeUnconditionally) are dropped instead of enqueued into a concurrent queue that will never drain again. It also adds a JSCDeferredWorkTask arm to __bun_release_task_at_shutdown so tasks already queued at shutdown are deleted (with pending-ticket / event-loop-ref balance) rather than re-queued and leaked. Touches JSCTaskScheduler.{h,cpp}, Worker.cpp, ZigGlobalObject.cpp, web_worker.rs, VirtualMachine.rs, dispatch.rs, plus a new ASAN-gated fixture and test.
Review history
I left three prior inline comments across two review passes; all were addressed:
- a7c0da0 serialized the shutdown transition under
m_lock(closing the check-then-enqueue TOCTOU CodeRabbit and I both flagged), moved the worker-sidemarkShuttingDownbefore the drain, madeBun__deleteDeferredWorkTaskrelease the pending ticket, and switched the test'sstderrassertion toexpect.not.stringContaining("LeakSanitizer"). - bf3fe64 mirrored the pre-drain fence into
VirtualMachine::global_exit().
The bug-hunting system found no issues on the current head. One finder candidate — that global_exit() sets the flag before any close_all_socket_groups-equivalent JS runs, unlike web_worker.rs — was verified not to be a bug: onAddPendingWork also checks m_isShuttingDown, so new tickets created by JS during shutdown are dropped rather than half-tracked.
Security risks
None. This is internal VM/worker teardown lifecycle management with no user-controlled input, auth, or data-exposure surface.
Level of scrutiny
High. This is exactly the category CLAUDE.md flags as most-blocked: cross-thread lifetime management, a lock-scope change (m_lock now held across an FFI enqueue), and refcount balancing on error/shutdown paths. I verified the enqueue under lock is a lock-free MPSC push (no re-entry into m_lock), that dropPendingTicketLocked is a behavior-preserving extraction of the pre-existing onCancelPendingWork body, and that the three markShuttingDown call sites are all idempotent. The reasoning looks sound, but VM-teardown concurrency is subtle enough that a maintainer should sign off.
Other factors
- The PR description explicitly calls out overlap with #34270 (same
m_isShuttingDowngate under a different name). Whichever lands first, the other needs a rebase overJSCTaskScheduler.{h,cpp}— that's a human coordination decision. - Test coverage: the new ASAN-gated fixture terminates a worker with 32 pending
Atomics.waitAsynctickets (some notified cross-thread first) underdetect_leaks=1:abort_on_error=1; assertssignal: null/exitCode: 0so an LSan SIGABRT fails the test independent of stderr noise.
|
CI on bf3fe64 (build 73608): 277 passed, 2 failed. The only red test is Everything else is yellow (passed on retry): Ready for review. |
|
@robobun fix conflicts |
Resolve conflict in src/jsc/web_worker.rs: both ScriptExecutionContext markTerminating (postTaskTo fence, #34278) and JSCTaskScheduler markShuttingDown (scheduleWorkSoon fence, this PR) are called before the final concurrent-queue drain.
|
CI on 4d9b926 (build 74072): 285/286 passed. The one red is Everything else is yellow (passed on retry). Ready for review. |
…hutdown handling
WebKit: merged 8be995561a (AsyncContextSwapScope RAII refactor),
ae5110d307 (reifyStaticProperty termination), 365cb02471 (Dockerfile.windows)
into the upgrade branch. JSMicrotask.cpp resolved by keeping upstream's
microtaskCallCache threading and asyncFunctionGeneratorBodyCall extraction on
top of the new RAII helper; AsyncGeneratorDriverResume now uses
wrapWithCurrent / unwrapContextTuple like its siblings.
Bun: merged origin/main. JSCTaskScheduler.{h,cpp} resolved by keeping the
new m_isShuttingDown gate, dropPendingTicketLocked, markShuttingDown and
Bun__deleteDeferredWorkTask from #34293, updated to the new
DeferredWorkTimer::Ticket / Ref<Ticket>&& / Ticket& API.
WEBKIT_VERSION bumped to the preview tag for the new WebKit head.
Only src/js/node/worker_threads.ts conflicted, in three hunks where #34338 ("don't hang when captured stdout/stderr is never consumed") and this branch touch the same lines. #34338 removed the #stdoutAutoPipe/#stderrAutoPipe fields and moved the stdio port ref/unref out of ref()/unref() — ports now manage their own ref via makePortReadable's incrementsPortRef. This branch only added #hasRef bookkeeping there, so main's structure is taken wholesale and only the two `if (!this.#exited) this.#hasRef = ...` lines and the field are kept. async_hooks.ts (#31825) and VirtualMachine.rs (#34293, #32498) auto-merged. `git diff origin/main -- src/js/node/worker_threads.ts` is a pure addition: zero deleted lines, so nothing from #34338 or #31825 is reverted. Verified on the merge result: test-worker-hasref, test-worker-error-stack- getter-throws, test-perf-hooks-worker-timeorigin, test-diagnostics-channel- worker-threads and the new "online fires before the entry point finishes" all pass; #34338's own repro still exits 0 like node; BroadcastChannel ref()/unref() and the 'online' timing fix both still match node v26.3.0.
… atomics fixture by iterations (#41428) ### Problem - `test/js/web/timers/timer-heap-race.test.ts` is in the slowest 5 percent of test files in CI: 12s on debian 13 x64-asan (build 110300). Locally on the debug ASAN build it takes 11.3s. - The three tests run serially (3.5s + 2.1s + 3.7s) although each one spawns one independent child. The atomics fixture runs for a fixed 3s of wall time, so a release build does 25x more rounds than a debug build for the same 3s. ### Fix - The three tests run with `it.concurrent`. The file takes as long as its slowest child. - `timer-heap-atomics-fixture.ts` runs a fixed number of pump rounds per worker instead of a clock deadline. The test passes the count (100 rounds, 3 workers). 100 rounds is what the old 3s gave on the debug ASAN build, the lane where the heap assertion lives, so that lane keeps the same number of cross-thread collisions. The main thread hammers until every worker reports done. - Assertions: the atomics fixture reports `OK 3 workers 300 pumps` and the test asserts the exact line from the shared constants. The GC fixture takes its tick count from the test (`GC_TICKS = 30`) and the test asserts `ok 30` from the same constant. The LeakSanitizer stderr check on the teardown test is unchanged. - Verified: `bun bd test test/js/web/timers/timer-heap-race.test.ts` went from 11.28s to 5.6s (three runs: 5.63s, 5.55s, 5.56s). `USE_SYSTEM_BUN=1 bun test` on the same file went from 3.2s to 0.2s. Also ran `test/js/web/timers/setTimeout.test.js` and `timer-gc-roots.test.ts`: the same four pre-existing debug ASAN failures as on main (the three RSS leak tests and one 5s timeout), none in files this PR touches. ### Background - The atomics fixture guards #33131. `Atomics.waitAsync` timeouts are `WTF::RunLoop` timers that Bun keeps in its own heap (`All.wtf_timers` in `src/runtime/timer/mod.rs`). Other threads can touch that heap, so it has its own mutex, separate from the `setTimeout` heap. - The teardown fixture guards #34293: a worker terminated with live `waitAsync` tickets must drop its deferred-work tasks, not enqueue them into the dead loop. It runs only under ASAN with `detect_leaks=1`. - The 20s ceilings stay. Each fixture still runs for seconds on a debug build and the default is 5s. <details><summary>Notes</summary> - About 3s of the teardown test is LeakSanitizer's exit scan. A trivial `bun -e 'console.log(1)'` with `ASAN_OPTIONS=detect_leaks=1` takes 3.3s on the debug build. The fixture itself is 0.5s. That cost is not reachable from the test and is now overlapped with the other two tests. - I tried to calibrate the round count against the original race by building a debug binary with the `wtf_timers` pop moved outside its lock. The fixture ran 30s without tripping. The vendored JSC no longer stops a `waitAsync` timer from the notifying thread (`Waiter::clearTimer` in `WaiterListManager.h` only drops its reference), and `DeferredWorkTimer::scheduleWorkSoonIfActive` goes through Bun's `onScheduleWorkSoon` hook instead of a RunLoop timer. So the cross-thread cancel the fixture was written against does not exist in this tree, and the count cannot be proven against it. The count keeps the debug ASAN lane at its previous iteration budget. - Release builds did about 2500 rounds per worker in the old 3s. They now do 100. The release binary compiles out the heap assertion, so the debug lane is the one that detects a heap corruption. - GC fixture per-tick cost on the debug build is dominated by `Bun.gc(true)`, not by the 16ms delay, so the delay is unchanged. </details> <!-- robobun:evidence:begin --> --- **[auto-merge]** gate passed · iteration 1 · 3 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/web/timers/timer-heap-race.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/web/timers/timer-heap-race.test.ts bun test v1.4.3 (e0a2b82) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap stays consistent while GC re-arms the RunLoop timer [1945.78ms] (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3519.14ms] (pass) terminating a worker with pending Atomics.waitAsync tickets does not leak deferred-work tasks [3624.34ms] 3 pass 0 fail 3 expect() calls Ran 3 tests across 1 file. [5.55s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/web/timers/timer-heap-atomics-fixture.ts | 27 +++++++++------ test/js/web/timers/timer-heap-gc-fixture.ts | 2 +- test/js/web/timers/timer-heap-race.test.ts | 44 ++++++++++++++++-------- 3 files changed, 47 insertions(+), 26 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/web/timers/timer-heap-atomics-fixture.ts 1 2 10 test/js/web/timers/timer-heap-gc-fixture.ts 1 2 7 test/js/web/timers/timer-heap-race.test.ts 1 2 6 ``` </details> **root cause** · written by the author bot The file was slow because its three independent subprocess tests ran serially and the atomics fixture spun against a fixed wall-clock deadline rather than stopping once it had done a bounded amount of race-provoking work, so every run paid the full sleep regardless of how quickly the race reproduced. The fix switches the tests to it.concurrent so the file's wall time collapses to the slowest single fixture, and replaces the clock-based loop with a fixed per-worker pump budget that each worker reports back, calibrated to match the previous duration on the debug ASAN lane where the heap asser… <!-- robobun:evidence:end -->
2 participants
test/js/web/timers/timer-heap-race.test.tswent red on the x64-asan lane after #33131 landed (build 73570):The leak is pre-existing; #33131's new cross-thread
Atomics.waitAsyncfixture is the first test that terminates a worker with pending async waiters on a SharedArrayBuffer the parent keeps alive.Cause
Worker
shutdown()drains the concurrent task queue once (release_queued_tasks_for_shutdown), then callsWebWorker__teardownJSCVM, which ends in~VM().~VM()runsWaiterListManager::unregister(this), and for every pendingAtomics.waitAsyncticket that reachesWaiter::cancelAndClear→DeferredWorkTimer::scheduleWorkSoon→ ouronScheduleWorkSoonhook. The hook allocates aJSCDeferredWorkTaskand aConcurrentTaskand enqueues them into the worker's concurrent queue, which was just drained for the last time. When the worker'sVirtualMachinebox is raw-dealloc'd, both become unreachable. The same path is reachable from the finalcollectNowviaJSFinalizationRegistry::finalizeUnconditionally.A second, narrower leak: a cross-thread
Atomics.notifythat lands between the worker's last tick andteardownJSCVMenqueues aJSCDeferredWorkTaskthatrelease_queued_tasks_for_shutdownforwards intoself.tasks.__bun_release_task_at_shutdownhad no arm for that tag, so it was re-queued, andEventLoop::deinitre-queued it once more into a freshly allocatedLinearFifobuffer that leaked on worker dealloc.Fix
JSCTaskSchedulergets anstd::atomic<bool> m_isShuttingDown, set at the start ofWebWorker__teardownJSCVMandZig__GlobalObject__destructOnExit(mirroring the existingctx->markTerminating()).onScheduleWorkSoonandonAddPendingWorkdrop the work once it's set;onScheduleWorkSoonalso balances theonAddPendingWorkref viaonCancelPendingWork.__bun_release_task_at_shutdowngains aJSCDeferredWorkTaskarm that deletes the job via a newBun__deleteDeferredWorkTaskFFI. This runs before JSC teardown, so~Ref<TicketData>and the capturedTasklambda release against a live VM.Test
timer-heap-atomics-teardown-fixture.tsterminates a worker with 32 pendingAtomics.waitAsynctickets on a parent-owned SAB, a few of them notified cross-thread first, underdetect_leaks=1. Without the fix LSan reports ~29ConcurrentTaskallocations fromWaiterListManager::unregisterand SIGABRTs; with it the fixture exits clean. The original race fixture is also 0/10 failures under the CI env (was ~1/5 on a debug build and 1/1 on release-asan).Overlap with #34270
#34270 adds the same
m_isShuttingDown/onScheduleWorkSoongate (there namedm_isTerminating) while fixing a separateFinalizationRegistryassert, but without the__bun_release_task_at_shutdownarm the race fixture still fails ~1/10 on that branch. Whichever lands first, the other is a small rebase overJSCTaskScheduler.{h,cpp}.[review] gate passed · iteration 3 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file