Fix lost wakeup when a worker message arrives during a nested event-loop wait - #37190
Fix lost wakeup when a worker message arrives during a nested event-loop wait#37190robobun wants to merge 6 commits into
Conversation
…oop wait The coalesced drain loops from #29937 kept the drain-scheduled flag set while dispatching user JS. A message sent while a handler's continuation was parked in a nested event-loop wait (bun:test's expect().rejects) was appended to the inbox without posting a wakeup task, so the wait never woke and the channel stayed wedged for the rest of the process. Clear the flag before any user JS runs, so a racing send posts a fresh drain task (at most one no-op drain per racing send): - Worker's drainInbox clears drainScheduled up front and now pops messages one at a time under the lock instead of swapping the whole queue out, so a nested drain observes the shared queue and delivery stays FIFO. - MessagePortPipe::drainAndDispatch hands DrainScheduled off to a new Dispatching bit before dispatching; the bit keeps the port's hasPendingActivity() true across the dispatch window that DrainScheduled used to cover, and is cleared on detach/close or when the owning drain finishes. Fixes #37189
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesMessage dispatch wakeup handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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/webcore/MessagePortPipe.cpp`:
- Around line 118-135: Update the drain implementation around the Dispatching
transitions and finish lambda to track whether the current invocation set
Dispatching, such as with an ownsDispatching flag or dispatch depth. Only the
invocation that acquired ownership may clear the shared bit, including the
empty-inbox, budget-exhaustion, and finish() paths; nested drains must leave the
outer drain’s Dispatching state intact.
In `@test/regression/issue/37189.test.ts`:
- Around line 60-64: Update expectExitsCleanly to consume proc.stderr
concurrently with proc.stdout and proc.exited, preventing the child from
blocking on an undrained pipe. Assert the complete stderr result alongside the
existing stdout and exitCode expectations, preserving the required ordered
stage-output checks.
🪄 Autofix
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: 2984f69b-168d-4bb1-9cfc-d7e137b99b6f
📒 Files selected for processing (5)
src/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePortPipe.cppsrc/jsc/bindings/webcore/MessagePortPipe.hsrc/jsc/bindings/webcore/Worker.cpptest/regression/issue/37189.test.ts
A nested drain on the same side would clear the shared Dispatching bit while the outer drain's dispatch was still on the stack, reopening the GC window it exists to cover. Only the invocation that set the bit clears it now. Also trims review-flagged comments and drains stderr in the regression test's child processes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the cross-thread wakeup/drain state machine for both MessagePortPipe and Worker inboxes — atomics, lock protocol, and a new state bit that participates in hasPendingActivity() GC liveness — a human look from someone who owns this subsystem would still be worthwhile.
What was reviewed:
DrainScheduled→Dispatchinghandoff andownsDispatchingso a nested drain can't clear the outer invocation's GC-liveness bit;detach()/close()reset it and stale drains bail on the ctxId/port check without touching flags.Worker::drainInboxswitch from batch-swap to per-message pop: FIFO holds under a nested drain, and the budget-exhaustion path now checksdrainScheduledbefore reclaiming it so a racing send's task isn't double-scheduled.- Regression test: both spawns now drain stdout/stderr/exit concurrently and assert the combined object (prior nit addressed in baf23c3).
Extended reasoning...
Overview
The PR fixes a lost-wakeup deadlock (#37189) introduced in 1.3.14: when a message handler parks in a nested event-loop wait (expect().rejects → waitForPromise), a reply arriving on the same channel was queued without a wakeup because DrainScheduled stayed set for the whole dispatch. The fix clears DrainScheduled before user JS runs and introduces a separate Dispatching bit (MessagePortPipe only) to preserve the GC-liveness window that DrainScheduled used to cover in hasPendingActivity(). Worker::drainInbox is reworked from swap-the-whole-queue to per-message pops so a nested drain observes the shared queue and delivery stays FIFO. Five files touched: MessagePortPipe.{h,cpp}, MessagePort.cpp, Worker.cpp, and a new regression test.
Security risks
None identified. No untrusted-input parsing, no auth/crypto, no path handling. The change is internal event-loop scheduling and atomic state management.
Level of scrutiny
High. This is exactly the category REVIEW.md flags as most-blocked: cross-thread atomics with weakened orderings, a lock protocol shared between sender and receiver threads, a new state bit read locklessly by the concurrent GC visitor, and re-entrancy via nested event-loop spins. The invariants are subtle — ownsDispatching tracking so a nested drain doesn't strip the outer's liveness bit, detach()/close() resetting Dispatching so a stale drain that bails on the ctxId/port identity check leaves flags to the new owner, and the budget-exhaustion reschedule now conditional on whether a racing send already claimed DrainScheduled. I traced these paths and they look correct, but the failure modes (dropped wakeup → hang; premature Dispatching clear → GC severs wrapper mid-dispatch) are severe and hard to catch in CI.
Other factors
- The PR description is thorough and the mechanism explanation is convincing; existing worker/MessagePort suites are reported passing.
- Two CodeRabbit findings (nested-drain
Dispatchingownership; undrained stderr in the test) were both addressed in follow-up commits, as was my own prior inline nit on the same stderr issue. - The
Worker::drainInboxchange also removes the batch-swap optimization the deleted comment justified ("sustained producer would otherwise make every per-message pop a contended acquire") — that's a deliberate correctness-over-throughput trade the author explains, but a maintainer may want to weigh it. - No prior human review on the PR yet; suggested reviewers are cirospaciari / jarred-sumner who own this area.
…ield handoff The per-pop rewrite left terminated workers' undelivered messages in the shared inbox until ~Worker, so transferred ports stranded there kept their peers' loop refs alive. Swap the queue out and destruct it in the termination branch, restoring the prompt close notification the old batch-drop provided. Adds a deterministic MessageChannel chain test that spends the drain budget while an in-handler send holds the continuation task, checking order across the handoff.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks the cross-thread drain/wakeup protocol in MessagePortPipe and Worker::drainInbox — atomics, lock scope, GC-liveness bits, and nested-reentrancy ownership — a human look would still be worthwhile.
What was reviewed:
- The
DrainScheduled→Dispatchinghandoff andownsDispatchingper-invocation guard — a nested drain leaves the outer's bit intact, anddetach()/close()reset it so a stale drain can't clobber a new owner. hasPendingActivity()now checksDrainScheduled | Dispatching, so the wrapper stays live across the window where the in-hand message is invisible to the queued count.- The budget-yield path in both loops: if a racing send already set
DrainScheduledthe loop returns without double-scheduling; the newmessage-port-pipe.test.tsself-feeding test exercises this. Worker::drainInboxswitching from batch-swap to per-message pop under the lock — trades the old "avoid contended acquires" fast path for FIFO correctness under nested drains; worth a maintainer glance at the perf implication the deleted comment called out.
Extended reasoning...
Overview
The PR fixes a deadlock regression (#37189) introduced by #29937: when a message handler's continuation parks in a nested event-loop wait (expect().rejects → waitForPromise), a reply arriving on the same channel was appended with no wakeup because DrainScheduled stayed set for the whole dispatch. The fix clears DrainScheduled before user JS runs and introduces a separate Dispatching bit (with per-invocation ownership) to keep hasPendingActivity() true across the dispatch window. Worker::drainInbox is restructured from swap-whole-queue to per-message pop so a nested drain observes the shared queue and delivery stays FIFO. Tests cover both the Worker and same-thread MessageChannel shapes plus the budget-yield handoff.
Security risks
None. This is internal event-loop / message-dispatch plumbing; no untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
High. This is concurrency-critical native code: per-side locks, atomic state words with release/acquire ordering, GC-thread lockless reads via hasPendingActivity(), and reentrant nested-drain semantics. The ownsDispatching logic and its interaction with detach() / same-context re-attach is subtle enough that CodeRabbit flagged it and a follow-up commit was needed. The switch from batch-swap to per-message locking in Worker::drainInbox deletes a comment that explicitly justified the old shape on contention grounds — a maintainer should confirm that trade-off is acceptable.
Other factors
The bug-hunting pass found nothing. All prior bot feedback (comment-cop, CodeRabbit's Dispatching-ownership concern, my own stderr-drain nit) has been addressed in commits baf23c3 and 2e5bc02. The PR description states the existing worker/MessagePort suites pass and the one observed failure reproduces on main. Given the complexity class (cross-thread + GC liveness + reentrancy), this is not a change I can approve without human review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/webcore/MessagePortPipe.cpp (1)
195-204: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRevalidate the execution context before draining microtasks.
dispatchOneMessage()runs user JavaScript. That code can destroy the context beforeglobalObject->drainMicrotasks()uses the captured pointer. Re-fetchport->scriptExecutionContext()and its global object after dispatch. If either is unavailable or no longer matches the active port, callfinish()and return.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/MessagePortPipe.cpp` around lines 195 - 204, Update dispatchOneMessage() after user-code dispatch and before globalObject->drainMicrotasks(): re-fetch the active port’s scriptExecutionContext() and global object, then compare them with the captured context/global object. If either is unavailable or no longer matches the active port, call finish() and return; otherwise drain microtasks using the revalidated objects.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/webcore/MessagePortPipe.cpp`:
- Around line 195-204: Update dispatchOneMessage() after user-code dispatch and
before globalObject->drainMicrotasks(): re-fetch the active port’s
scriptExecutionContext() and global object, then compare them with the captured
context/global object. If either is unavailable or no longer matches the active
port, call finish() and return; otherwise drain microtasks using the revalidated
objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76dd8b3a-9ddf-4d80-93da-be002ed36750
📒 Files selected for processing (1)
src/jsc/bindings/webcore/MessagePortPipe.cpp
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it reworks the cross-thread drain/wakeup state machine (new Dispatching bit, per-invocation ownership, GC-visitor liveness in hasPendingActivity) and changes reentrancy semantics under nested event-loop waits, a human look would still be worthwhile.
What was reviewed
- The
DrainScheduled→Dispatchinghandoff andownsDispatchingtracking — nested drains correctly leave the outer invocation's bit alone;detach()/close()clear it so a stale drain can't clobber a new owner. Worker::drainInboxper-message pop under the lock — FIFO is preserved when a nested drain runs; the budget-yield path checksdrainScheduledbefore reclaiming to avoid double-scheduling.hasPendingActivity()now checksDrainScheduled | Dispatching— the in-hand-message GC window stays covered.- Regression tests spawn with a 15s SIGKILL timeout and drain stdout/stderr/exit concurrently; the self-feeding-chain test exercises the budget-yield handoff.
Extended reasoning...
Overview
The PR fixes a deadlock introduced in 1.3.14 where a Worker/MessagePort message arriving while the receiver is parked in a nested event-loop wait (e.g. expect(p).rejects inside a message continuation) is enqueued without posting a wakeup task, because drainScheduled stayed set for the entire dispatch. The fix splits the state: DrainScheduled now covers only "posted task not yet started", and a new Dispatching bit covers the dispatch loop for GC-liveness purposes. Worker::drainInbox switches from swap-whole-queue to per-message pops under the lock so nested drains stay FIFO. Touches MessagePortPipe.{h,cpp}, MessagePort.cpp, Worker.cpp, plus a new regression test and one added test in message-port-pipe.test.ts.
Security risks
None identified. This is internal event-loop / message-dispatch machinery with no parsing of untrusted input, no auth/crypto, no resource-limit changes. The state bits are internal and not user-controllable.
Level of scrutiny
High. This is subtle cross-thread concurrency code: atomic state bits read locklessly by the GC visitor (hasPendingActivity), lock/atomic interactions, ownership tracking across nested reentrant drains, and detach/transfer semantics. The ownsDispatching mechanism was added in response to a CodeRabbit finding about nested drains prematurely clearing the outer drain's Dispatching bit — the kind of edge case that's easy to miss. The change to per-message locking in Worker::drainInbox also trades the previous batch-swap contention optimization for correctness under nesting, which is the right call but changes hot-path lock behavior.
Other factors
- Test coverage is solid: two subprocess regression tests (Worker + MessageChannel shapes) that deadlock/SIGKILL on the unfixed build, plus a 2500-message self-feeding chain that exercises the budget-yield handoff. The PR body shows both tests failing on main and passing with the fix in both debug+ASAN and release.
- All prior review feedback (comment-cop, CodeRabbit's
ownsDispatchingand stderr-drain findings, my earlier stderr note) has been addressed and marked resolved. - The termination path now explicitly drains and drops the queue outside the lock so transferred ports' peers see 'close' promptly — a behavior improvement over the old code.
- No bugs surfaced in the automated hunt, but the interaction surface (nested waits × transfer × GC × budget-yield × termination) is large enough that a maintainer familiar with the #29937 design should confirm the state-machine changes.
1 participant
Fixes #37189
Repro
The second assertion never settles; the test times out and
bun testthen hangs and must be killed. A same-threadMessageChannelRPC hits the identical deadlock. Regression introduced in 1.3.14 by #29937; 1.3.13 is fine.Cause
expect(p).rejectsawaits synchronously: the matcher entersEventLoop.waitForPromise, which spins the event loop from inside the current dispatch. The drain loops from #29937 keepdrainScheduledset for the whole dispatch, includingdrainMicrotasks(), andWorker::enqueueToParent/MessagePortPipe::sendskip posting a wakeup task while the flag is set.So when the test continuation (run from the previous message's microtask drain, with the drain loop still on the native stack) issues a second call and parks in
waitForPromise, the reply is appended to the inbox with no wakeup. The code that would clear the flag sits below the nested wait on the same stack, so the wait parks forever and every later message on that channel is silently queued.Fix
Clear the flag before dispatching any user JS, so a racing send posts a fresh drain task (at most one extra no-op drain per racing send):
Worker.cppdrainInbox: cleardrainScheduledup front, and pop messages one at a time under the lock instead of swapping the whole queue into a local batch, so a nested drain task observes the shared queue and delivery stays FIFO.MessagePortPipe.cppdrainAndDispatch: handDrainScheduledoff to a newDispatchingstate bit before dispatching.MessagePort::hasPendingActivity()needs the dispatch window covered for GC wrapper liveness (the in-hand message is invisible to the queued count), so it now checksDrainScheduled | Dispatching.Dispatchingis cleared when the owning drain finishes, or bydetach()/close(); a stale drain never clears it after losing ownership, so it cannot clobber a new owner's drain after a transfer.Verification
test/regression/issue/37189.test.tscovers both theWorkerand theMessageChannelshape. Both child processes deadlock and get killed by the spawn timeout on the unfixed build; both printOKand exit 0 with the fix.test/js/web/workers/(message-port-pipe, message-channel, message-event, leak tests, worker.test.ts, worker-postmessage-transfer),test/js/node/worker_threads/(incl. transfer-terminate stress), and the node parallel MessagePort tests. The only failure seen (worker-terminate-lifetime.test.tsdns.lookup LeakSanitizer report) reproduces identically on main without this diff.[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The drain task kept the wakeup flag set for its entire run, including while user message handlers executed, so a message arriving while a handler was parked in a nested event-loop wait saw a drain as already scheduled, skipped posting a wakeup, and the wait never resumed. The fix clears the flag before any dispatch and pops messages one at a time under the inbox lock, so any send that lands mid-drain posts a fresh wakeup task that can drain it, preserving FIFO order across nested drains. When the per-drain budget is exhausted, the drain reclaims the flag only if no racing send already did, …