Worker: flush the worker→parent inbox before 'close' on natural exit - #35302
Worker: flush the worker→parent inbox before 'close' on natural exit#35302robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 4 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 (4)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked the two suggested issues against both current main and this branch:
Leaving both out of the PR description. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the timing-pad note in a838f51 — the widened isDebug ? 3000 : 500 and the "no JS-visible hook after step 2" comment cover it. I traced the fix and it checks out (drain runs while m_state == Running, worker VM is already torn down so m_toParent can't grow, limit ≥ queue.size() guarantees no reschedule, m_terminateRequested still gates dispatch), but this is cross-thread C++ worker lifecycle so a maintainer should confirm.
What was reviewed:
- Verified one
drainToParentpass empties the inbox with no reschedule (limit budget vs. post-decrement loop; queue can't grow after VM teardown). - Checked
drainScheduledinteraction with an already-queued T2 reschedule — idempotent, T2 sees an empty queue. - Confirmed
terminate()semantics preserved (dispatches no-op viam_terminateRequested) and close event still fires (bypasses the gate). - Ruled out handler re-entrancy hazards: handler calling
postMessage()hits the worker'smarkTerminating()context and drops; handler callingterminate()just no-ops the remaining dispatches.
Extended reasoning...
Overview
One-line functional change in src/jsc/bindings/webcore/Worker.cpp: the close task posted by dispatchExit() now calls protectedThis->drainToParent(context) before flipping m_state to Closing. This flushes the worker→parent inbox so a re-posted drain that landed behind the close task in the parent's queue no longer dispatches into a Closed worker whose dispatchEvent() is a no-op. Accompanied by a ~12-line comment explaining the invariants (VM already torn down → inbox can't grow; limit = max(queue.size(), 1000) ≥ queue.size() → single pass; m_terminateRequested still short-circuits dispatch). The test addition in worker_threads.test.ts uses a subprocess with a SharedArrayBuffer handshake plus a documented timing pad to reproduce the drain-reschedule-behind-close ordering.
Security risks
None identified. This is message-delivery ordering during worker shutdown; no auth, crypto, or untrusted-input parsing. The drain runs on the parent thread with the same dispatchEvent path that already handles worker→parent messages.
Level of scrutiny
High. This is C++ JSC bindings on the worker teardown path — cross-thread state (m_toParent inbox, m_state, drainScheduled), event dispatch that runs user JS while the close task is mid-execution, and interaction with terminate(). REVIEW.md's memory-safety and thread-affinity sections apply directly. The fix is small and the comment is thorough, but the ordering of "drain user-visible messages → flip state → dispatch close" inside a lambda that also manages refcounts and parent_poll_ref is exactly the kind of thing a maintainer familiar with the shutdown() step ordering should sign off on.
Other factors
- My prior review (a 🟡 nit on the test's 500ms timing pad not being a deterministic pin on debug/ASAN) was addressed in a838f51: pad widened to
isDebug ? 3000 : 500and the comment now names why no observable signal exists between shutdown step 2 and step 4, satisfying REVIEW.md's ≥50ms-sleep rule. Author re-verified the test still fails withsrc/reverted. - I traced
drainInbox's budget arithmetic: withlimit = max(N, 1000)and a batch of size N (post-decrement check), all N items dispatch beforelimithits 0, then the empty-queue check returnsfalse— no reschedule. A stale T2 already in the queue runs afterward, sees an empty inbox, and idempotently clearsdrainScheduled. - Re-entrancy: message handlers run with
m_state == Running(same as the pre-fix delivered-prefix behavior). A handler callingworker.postMessage()posts to amarkTerminating()worker context (dropped). A handler callingworker.terminate()setsm_terminateRequested, remaining dispatches no-op, and the close event still fires because it goes throughEventTargetWithInlineData::dispatchEventdirectly. - Minor: the test asserts
stderr: "", which some sibling tests in this file avoid on ASAN/debug lanes (they pass stderr through or gate onexitCode). Not blocking, but worth watching if CI flakes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
|
Added a web-Worker test for the same flush ( Verified the web-Worker variant:
Retitled the PR accordingly. |
|
Updated 7:40 AM PT - Jul 29th, 2026
❌ @robobun, your commit 461c7c4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35302That installs a local version of the PR into your bun-35302 --bun |
…xit' A worker that posts a burst via parentPort.postMessage() and then exits (natural end-of-script or process.exit) was silently dropping the tail of that burst. enqueueToParent() coalesces: N posts schedule one drainToParent task, which dispatches up to max(queue.size(), 1000) messages and then re-posts itself to yield to the event loop. When the worker is still posting while the parent is draining, the re-post lands behind the close task that dispatchExit() queues, and once the close task flips m_state to Closed every subsequent Worker::dispatchEvent() is a no-op, so the re-posted drain quietly discards whatever is left. Node flushes the worker's parentPort queue before teardown and delivers every message ahead of 'exit'. Match that by calling drainToParent() at the top of the close task: the worker's VM is already torn down by the time the close task runs (it is posted from shutdown() step 4), so m_toParent cannot grow and a single pass empties it. terminate() keeps dropping pending messages because dispatchEvent() is separately gated on m_terminateRequested.
The Atomics handshake can observe process.on('exit') (shutdown step 2) but
nothing JS-visible runs between there and dispatchExit (step 4, after the
full sync GC in step 3). The busy-wait covering that gap is a timing pad,
not a synchronization point; widen it on debug/ASAN so the test keeps its
regression-guard property on slow lanes.
The close-task flush in dispatchExit serves both node's parentPort (fakeParentPort wraps jsFunctionPostMessage) and the web Worker global postMessage, since both feed m_toParent. Add the web-API variant of the SAB-pinned test so the fix is exercised through Worker.onmessage / 'close' as well as worker_threads 'message' / 'exit'.
db2a68b to
d751c8e
Compare
…ronously The close task now re-posts itself on the same lane drainToParent's reschedule uses whenever m_toParent is non-empty (or a drain is still scheduled), and only proceeds to Closing/close/Closed once the inbox is observed empty. terminate() skips the chase so the undrained tail is still discarded. This replaces the earlier synchronous drainToParent() call in the close task: a synchronous drain-to-empty defeats the drain's per-turn yield budget, so a large backlog would be dispatched in one unbroken run on the parent. Chasing keeps the drain as the pacing mechanism (one cheap re-check per drain turn) and composes with any future change to the drain's reschedule lane or per-turn cap.
|
Reworked the fix per review: the close task now chases the drain instead of draining synchronously. Rationale: a synchronous Both SAB-pinned tests still fail-before (999/5000, no sentinel) / pass-after (5000/5000 + sentinel). The 300k repro delivers all 300k + sentinel. |
There was a problem hiding this comment.
No issues found in the reworked chase-based fix, but this touches cross-thread Worker lifecycle (refcounting, event-loop keep-alive timing, task-queue ordering) and the design changed substantially since my last look — worth a human pass.
Checked: the chase terminates (worker VM is down so m_toParent is bounded; drainScheduled is cleared on every non-reschedule return of drainInbox, and both the drain reschedule and closeTask re-post go through the same postTaskToParent FIFO). Verified terminate() mid-chase short-circuits on the next turn and dispatchEvent no-ops the leftover drain. Ref balance across the re-post chain looks correct — each hop captures its own Ref{*this}. The delayed WebWorker__releaseParentPollRef keeps the parent loop ref'd while messages are still draining, which is the intended semantic. The stuck-drainScheduled-after-parent-termination concern was examined and ruled out (parent context teardown stops task delivery, so no unbounded re-post).
Extended reasoning...
Overview
Extracts the dispatchExit close-task lambda body into Worker::closeTask() and adds a chase: if m_toParent still has pending messages (or a drain is scheduled) and terminate() wasn't called, the close task re-posts itself via postTaskToParent and returns without flipping state. The rest of the close path (Closing → 'close' → Closed, m_pendingTasks.clear(), rejectAllCrossVMRequests, WebWorker__releaseParentPollRef) is unchanged, just relocated. Two SAB-pinned regression tests cover both the web-Worker and node worker_threads entry points.
Security risks
None. No untrusted input parsing; the change reorders existing parent-thread tasks.
Level of scrutiny
High. This is cross-thread C++ lifecycle code with the exact hazards REVIEW.md calls out: refcounts must balance on every terminal path, thread affinity of every line matters, and the event-loop keep-alive (WebWorker__releaseParentPollRef) now fires later than before. The fix went through a redesign during review (synchronous drainToParent() in the close task → chase-and-repost), so the shipped mechanism is not what earlier review passes examined.
Other factors
- My earlier feedback (test timing pad on debug/ASAN, missing no-observable-signal comment) was addressed in a838f51 and is reflected in both tests.
- The candidate infinite-repost issue (drainScheduled stuck true after parent-VM termination) was verified not to be reachable:
closeTaskruns on the parent thread, so if the parent context is terminating,postTaskTostops delivering and the chain ends; anddrainInboxclearsdrainScheduledon every path that doesn't reschedule except the parent-termination early-return, which is exactly the case where task delivery has stopped. terminate()between two chase hops is handled: the nextcloseTaskturn observesm_terminateRequestedand proceeds straight to close; the still-queued drain task'sdispatchEventcalls are already no-ops under that flag.- One behavioral consequence a human should sanity-check:
WebWorker__releaseParentPollRef(impl_)(andm_state → Closed) are now delayed until the inbox drains, so a parent whose only keep-alive is this worker stays alive across the whole backlog delivery. That's the intended semantic (deliver everything before exit), but it widens the window during whichterminate()/setKeepAlive()can be called against an already-torn-down worker VM — both were already possible in the pre-PR one-turn window, so this isn't new, just longer.
|
CI status for this diff: Build #85053 (finished): 195/196 passed.
The diff itself is green everywhere it ran. Ready for review. |
There was a problem hiding this comment.
No issues found in the reworked chase-the-drain approach, but this is cross-thread worker lifecycle C++ that went through a mid-review redesign — worth a human look at the final shape.
What was reviewed:
- Chase termination: worker VM is down before
dispatchExitsom_toParentcan't grow; each drain turn shrinks the queue and clearsdrainScheduledwhen empty, socloseTaskre-posts a bounded number of times. - Ref balance: each re-post captures a fresh
Ref{*this}; the originaldispatchExitlambda's ref drops on return;WebWorker__releaseParentPollReffires exactly once on the terminal call. terminate()path unchanged: chase skipped,dispatchEventgate still discards the tail, close fires next turn.
Extended reasoning...
Overview
Extracts the close-task lambda body into Worker::closeTask() and adds a pre-close guard: if m_toParent still has pending work (!queue.isEmpty() || drainScheduled, checked under the inbox lock) and terminate() hasn't been requested, re-post self via postTaskToParent and return. This lets the coalesced drainToParent reschedule(s) run to completion before m_state flips to Closed (after which dispatchEvent is a no-op). The rest of the close-task body is unchanged, just moved from the lambda to a member function. Two SAB-pinned regression tests cover both the web Worker and node:worker_threads entry points.
Security risks
None. No untrusted-input parsing, no auth/crypto surface. The change is internal task-queue ordering.
Level of scrutiny
High. This is C++ cross-thread lifecycle in Worker.cpp — the exact area REVIEW.md flags as most-blocked (ref counts balanced on every terminal path, thread affinity, re-posted tasks). The approach was redesigned mid-review from a synchronous drainToParent() flush to the current chase pattern; that's a design decision (pacing vs. one unbounded run) a maintainer should sign off on.
Other factors
- The
drainScheduled-stuck-true livelock candidate was examined and refuted:closeTaskruns on the parent context, sopostTaskToParentsucceeding for the drain reschedule implies it succeeds for the chase re-post too, and every path indrainInbox/drainToParentthat leavesdrainScheduledtrue also leaves work queued or a task posted that will clear it. - The two comment-cop flags on the current revision are resolved; the remaining comment blocks in
closeTask()are per-invariant, matching the file's existing style for cross-thread lifecycle notes. - My earlier note on the test's timing pad was addressed (widened on debug/ASAN with a comment naming why no observable signal exists past shutdown step 2); the tests can't false-fail with the fix applied.
- CI on the latest push (#85053) was retriggered after fleet-wide build infra issues; the diff compiled and passed on every lane that ran.
2 participants
What
A worker that posts a burst via
postMessage()(web Worker global or nodeparentPort) and then exits naturally silently drops the tail of that burst. The parent gets a prefix, never sees the final messages, and'close'/'exit'fires anyway. Node and browsers deliver every message then fire the exit event.Why
Worker::enqueueToParent()coalesces: N posts schedule onedrainToParenttask on the parent, which dispatches up to its budget and then re-posts itself to yield to the event loop. The close task thatdispatchExit()queues fromshutdown()lands in the same FIFO task queue, so a drain reschedule can sit behind the close task. The close task flipsm_statetoClosed, after whichWorker::dispatchEvent()is a no-op, so the re-posted drain walks the remaining inbox dispatching into a void.A transferred
MessagePortis unaffected (its inbox lives in aMessagePortPipe::Sidethat nothing gates on worker state); a worker kept alive (setInterval) is unaffected (no close task posted). Only the built-inm_toParentinbox is lost.Fix
The close task body is pulled out into
Worker::closeTask(). At the top, ifterminate()has not been requested and the inbox is still pending (!queue.isEmpty() || drainScheduled, checked under the inbox lock), it re-posts itself viapostTaskToParent(the same lane the drain reschedule uses) and returns without touchingm_state. Each re-post lands behind one drain turn in FIFO order, so close re-checks once per turn and only proceeds toClosing → 'close' → Closedonce the inbox is empty and no drain is scheduled.terminate()is unchanged: the chase is skipped whenm_terminateRequestedis set, anddispatchEvent()'s existing gate on that flag makes any remaining dispatches no-ops, so terminate still discards the undrained tail and close fires on the next turn.A synchronous
drainToParent()call in the close task was considered and rejected: it dispatches the whole backlog in one unbroken run on the parent, defeating the drain's yield budget. Chasing keeps the drain as the pacing mechanism and composes with any change to its reschedule lane or per-turn cap.Verification
Two SAB-pinned tests (web Worker in
test/js/web/workers/worker.test.ts, nodeworker_threadsintest/js/node/worker_threads/worker_threads.test.ts) hold the parent's first drain handler open until the worker has enqueued its whole burst and posted its close task, so the drain's reschedule lands behind close deterministically. Without the fix both receive exactly 999 of 5000 and never see the sentinel; with the fix both receive all 5000 plus the sentinel before'close'/'exit'. The 300k repro above now delivers all 300k + sentinel.[review] gate passed · iteration 4 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 4
evidence per changed file