Atomics.waitAsync: keep the event loop alive once notify schedules the resolve task - #34911
Atomics.waitAsync: keep the event loop alive once notify schedules the resolve task#34911robobun wants to merge 3 commits into
Conversation
… resolve task An Atomics.waitAsync waiter registers its DeferredWorkTimer ticket as WorkType::AtSomePoint, so it is tracked in m_pendingTicketsOther and holds no event-loop ref while dormant (a pending waitAsync must not, by itself, keep the process alive). When Atomics.notify wakes the waiter, WaiterListManager::notifyWaiterImpl calls scheduleWorkSoon on that ticket. JSCTaskScheduler::onScheduleWorkSoon enqueues a ConcurrentTask to run the promise resolution but left the ticket in m_pendingTicketsOther. VirtualMachine::is_event_loop_alive() counts the uws active handles, EventLoop::tasks, and EventLoop::concurrent_ref, but not the concurrent_tasks queue itself, so if the notify happens inside the callback of the last live handle (the repro's setTimeout), the run loop in run_command.rs:boot exits with the resolution tasks still sitting in concurrent_tasks, and the waitAsync promises never settle. Promote the ticket from m_pendingTicketsOther to m_pendingTicketsKeepingEventLoopAlive and take an event-loop ref at the point scheduleWorkSoon makes the work imminent. runPendingWork already decrements the ref when it takes the ticket from the keep-alive set, so the balance is unchanged for tickets that were ImminentlyScheduled from the start. Tickets that were cancelled before scheduling (Waiter::cancelAndClear) are absent from both sets and are skipped as before.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Reproduced from the report; the 8-line change in |
|
Updated 5:12 AM PT - Jul 21st, 2026
✅ @robobun, your commit bd1eeb0fbc780b6d9e1ce5d3d65b6f789d63e75e passed in 🧪 To try this PR locally: bunx bun-pr 34911That installs a local version of the PR into your bun-34911 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per REVIEW.md, avoid a standalone expect(stderr).toBe("") so ASAN/debug
allocator warnings do not cause spurious failures. stderr is matched with
expect.any(String) so it still appears in the diff on a real failure.
There was a problem hiding this comment.
The earlier stderr nit is addressed. I didn't find any correctness issues, but this touches event-loop keep-alive ref accounting in the JSC deferred-work scheduler, so it's worth a maintainer glance before merging.
What was reviewed:
- Traced ref balance across all ticket paths: add(AtSomePoint) → scheduleWorkSoon promotes + refs → runPendingWork / onCancelPendingWork / Bun__deleteDeferredWorkTask each deref via the keep-alive set. ImminentlyScheduled tickets and cancelled-then-scheduled tickets miss the
m_pendingTicketsOther.take()and are unaffected. - Confirmed
Bun__eventLoop__incrementRefConcurrently(+1)underm_lockmatches the existing pattern inonAddPendingWorkand is just an atomic bump on the Rust side. - The negative test ("waitAsync alone does not keep the event loop alive") would hang if the fix over-reffed a dormant waiter — it doesn't, since promotion only happens inside
onScheduleWorkSoon.
Extended reasoning...
Overview
The PR adds 8 lines to JSCTaskScheduler::onScheduleWorkSoon in src/jsc/bindings/JSCTaskScheduler.cpp: when an AtSomePoint ticket (Atomics.waitAsync) is scheduled for imminent work, it is moved from m_pendingTicketsOther to m_pendingTicketsKeepingEventLoopAlive and an event-loop ref is taken. Two subprocess tests are added to test/js/web/atomics.test.ts — one asserting notify-scheduled resolutions run before exit, one asserting a dormant waiter does not keep the loop alive.
Security risks
None. This is internal event-loop bookkeeping with no user-controlled input reaching the changed code path beyond the existing Atomics.waitAsync/notify surface.
Level of scrutiny
Moderate-to-high. The diff is small and the mechanism is well-argued in the PR description, but event-loop ref accounting is a critical path: an unbalanced ref means either a process hang (over-ref) or premature exit with queued work dropped (under-ref). I traced every path that touches these two sets — onAddPendingWork, onScheduleWorkSoon (both the shutting-down and normal branches), onCancelPendingWork, runPendingWork, and Bun__deleteDeferredWorkTask — and the +1 taken here is balanced by the existing -1 in each terminal path. Tickets that were already in the keep-alive set (ImminentlyScheduled: FinalizationRegistry, Wasm compile) or already removed (cancelAndClear → cancelPendingWork before scheduleWorkSoon) return null from m_pendingTicketsOther.take() and skip the new block, so their accounting is unchanged.
Calling Bun__eventLoop__incrementRefConcurrently while holding m_lock mirrors onAddPendingWork and runPendingWork; the Rust side (src/jsc/JSCScheduler.rs:40) is just an atomic inc/dec on EventLoop::concurrent_ref, so no lock-ordering concern.
Other factors
- My earlier inline nit about
expect(stderr).toBe("")was addressed in bd1eeb0 — both tests now use the combined{ stdout, stderr, exitCode }form withstderr: expect.any(String). - The mechgate evidence's "release without fix: all passed" line is a labeling artifact (that run used canary 9375564 which already contains the fix commit 18da7dc); the
USE_SYSTEM_BUN=1failure in the description is the real fails-without-fix proof. - Given this is C++ ref-count bookkeeping on a path reachable cross-thread from
Atomics.notify, I'm deferring rather than approving so a maintainer can confirm the accounting reasoning.
|
Closing: the behavior this PR adds is on main as of #37075 (9d519e8, "Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown"). That change fixed the gap described in this PR at the event loop rather than in Verified against a debug build of main (04148c8): this PR's version of |
1 participant
Repro
Node prints
notify count = 5thenresolved: w0:ok,w1:ok,w2:ok,w3:ok,w4:ok. Bun printsnotify count = 5thenonly 0/5 settled before exit.Cause
Atomics.waitAsyncregisters itsDeferredWorkTimerticket asWorkType::AtSomePoint, soJSCTaskScheduler::onAddPendingWorkstores it inm_pendingTicketsOtherwithout taking an event-loop ref. That is correct while the waiter is dormant: a pendingwaitAsyncalone must not keep the process alive.When
Atomics.notifywakes the waiter,WaiterListManager::notifyWaiterImplcallsscheduleWorkSoonon that ticket.JSCTaskScheduler::onScheduleWorkSoonenqueues aConcurrentTaskto run the promise resolution but leaves the ticket inm_pendingTicketsOther.VirtualMachine::is_event_loop_alive()counts the uws active handles,EventLoop::tasks, andEventLoop::concurrent_ref, but not theconcurrent_tasksqueue itself. In the repro thenotifyruns inside the callback of the last live handle, so when control returns to thewhile vm.is_event_loop_alive()loop inrun_command.rsevery input is zero and the process exits with the five resolution tasks still sitting inconcurrent_tasks.Fix
In
JSCTaskScheduler::onScheduleWorkSoon, when the ticket is found inm_pendingTicketsOther, move it tom_pendingTicketsKeepingEventLoopAliveand take an event-loop ref viaBun__eventLoop__incrementRefConcurrently(+1). The work is imminent at this point, so the ticket now has the same lifetime accounting as anImminentlyScheduledone.runPendingWorkalready decrements the ref when it takes the ticket from the keep-alive set, andBun__deleteDeferredWorkTaskdoes the same viadropPendingTicketLocked, so the balance is unchanged for tickets that started asImminentlyScheduled. Tickets that were cancelled before scheduling (Waiter::cancelAndClearcallscancelPendingWorkthenscheduleWorkSoonwith a no-op) are absent from both sets and take the existing no-ref path.A pending
waitAsyncwith nonotifystill does not keep the loop alive; a second test asserts that.Verification
test/js/web/timers/timer-heap-race.test.tsandtest/regression/issue/atomics-waitasync-wtftimer-uaf.test.tsstill pass.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file