Repository navigation
Conversation
The 'close' event code after worker.terminate() depended on timing: 1 if terminate() landed while the worker was still evaluating its entry module (the load path in web_worker.rs sets exit_code = 1 on WorkerTerminated), 0 once its loop was idle. docs/runtime/workers.mdx says the code is the one passed to process.exit(), or 0. The proxy already overrides the code for a worker stopped by its parent: node:worker_threads reports 1 like Node. A Web Worker now reports 0 in that case, as the comment at that site already claimed.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 5 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Deciding in the proxy from stoppedByParent alone could also zero a code the worker did choose (a resolve failure's 1 or an unsettled top-level await's 13) when a terminate() raced the worker's own exit, because stoppedByParent only excludes process.exit(). Instead, stop producing the spurious 1: the entry-load Err arm no longer treats a load that the parent's terminate() cut short as a load failure. terminated_by_parent is read under the vm_handle lock that request_termination holds while it sets requested_terminate and then terminated_by_parent, so a worker that already saw the request cannot read a stale false. The proxy keeps its Node-only override to 1.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Status: ready for review. Reproduced and verified with |
There was a problem hiding this comment.
LGTM — moving the fix to the producer side addresses the earlier concern: only the entry-load Err arm is guarded now, so an uncaught-error 1 or unsettled-TLA 13 set elsewhere is no longer overwritten when a terminate() races in.
Checked: request_termination holds vm_handle.lock() across both set_requested_terminate() and the terminated_by_parent store, so the helper's Relaxed load under the same lock is sound; the worker thread already takes vm_handle.lock() under the API lock in shutdown(), so no new lock-ordering edge. The Err path is only reachable after vm_handle is published (earlier has_requested_terminate() checks at 793/836 short-circuit before), so terminated_by_parent is always set when the terminate came from the parent. Test awaits the worker's own message before terminating and asserts a combined {code, wasClean} object.
Extended reasoning...
Overview
The PR fixes the close event code reported when a parent calls worker.terminate() while the worker is still evaluating its entry module. Previously the Err arm of load_entry_point_for_web_worker set exit_code = 1 unconditionally (unless process.exit() had run), treating a parent-initiated TerminationException as a load failure. The fix adds !self.termination_requested_by_parent() to that guard and introduces a small helper that reads terminated_by_parent under vm_handle.lock(). A test.each in worker.test.ts covers both a busy-evaluating worker and an idle-loop worker.
The first revision fixed this at the consumer (WorkerMessagingProxy.cpp), which I flagged would overwrite error-driven exit codes (uncaught error's 1, unsettled TLA's 13) when a terminate() raced them. Commits 9fadaef and 67022da moved the fix to the worker thread's load path, so only the specific "TerminationException aborted the entry load" case skips the failure code; all other exit-code producers are untouched and the proxy is unchanged.
Security risks
None. This is exit-code reporting for Web Workers; no auth, crypto, untrusted-input parsing, or resource limits are involved.
Level of scrutiny
Moderate — threading correctness matters here. I verified request_termination (web_worker.rs:540-561) takes vm_handle.lock() before calling set_requested_terminate() and holds it through the terminated_by_parent.store(true, Relaxed), so the new helper's lock-then-Relaxed-load cannot observe a stale false once the worker has seen requested_terminate (the mutex provides the happens-before edge; the Relaxed ordering is fine per REVIEW.md's "comment any weakened ordering", and the comment does). Lock ordering matches existing code: the worker thread already takes vm_handle.lock() while holding the JSC API lock in shutdown(), and handle.request_termination() on the parent side does not need the API lock, so no new deadlock edge is introduced. The Err arm is only reachable after vm_handle has been published (start_vm publishes at line 732; earlier terminate checks at 793 and 836 return before entry load), so the if let Some(handle) branch in request_termination will have fired and set terminated_by_parent whenever the parent is the cause.
Other factors
The test follows harness conventions: test.each matrix, awaits once(w, "message") (an observable readiness signal from the worker itself, not a sleep), registers the close listener before calling terminate(), and asserts a combined object with .toEqual. The busy-evaluating case is the regression (fails on the base build with {code: 1, wasClean: false}); the idle case is matrix coverage confirming the existing correct path stays correct. The github-actions bot inline comments on lines 868/1100/1104 were self-resolved and appear to have been addressed by the "shorten comments" commit. No human CHANGES_REQUESTED reviews are outstanding.
…its neighbour has It boots 48 workers in a child process. A debug+ASAN build on a loaded machine takes about 7s for that, past the 5s default, which fails the whole file for unrelated changes. Same isDebug ? 30_000 : 5_000 ceiling as the preload test above it.
|
Updated 8:03 AM PT - Sep 8th, 2026
✅ @robobun, your commit eeed83e7a3df032abb4109e30ca29e560b6faac1 passed in 🧪 To try this PR locally: bunx bun-pr 41969That installs a local version of the PR into your bun-41969 --bun |
There was a problem hiding this comment.
LGTM — moving the guard to the producer's Err arm addresses the concern from the earlier revision (error-driven codes like 13 and the uncaught-exception 1 are set on separate paths this change doesn't touch, so they can no longer be zeroed).
Checked: termination_requested_by_parent() takes the same vm_handle lock that request_termination holds across set_requested_terminate() → terminated_by_parent.store(true), so a worker that bailed on WorkerTerminated cannot read a stale false; the worker thread does not hold that lock at the call site in spin, so no self-deadlock. The new tests await the worker's own message before terminating (no sleep) and cover both the busy-load and idle-loop timing.
Extended reasoning...
Overview
The PR narrows one branch in WebWorker::spin (src/jsc/web_worker.rs:863-876): when load_entry_point_for_web_worker returns Err, the worker no longer stamps exit_code = 1 if the parent's terminate() is what aborted the load. A new helper termination_requested_by_parent() reads terminated_by_parent under the vm_handle mutex. Two new test.each cases in worker.test.ts assert { code: 0, wasClean: true } for terminate() during entry evaluation and while idle. The final commit adds an isDebug ? 30_000 : 5_000 per-test timeout to the pre-existing 48-worker readFile-churn test.
Security risks
None. No user input parsing, no auth, no network. The change is a read of an internal atomic under an existing lock and a conditional skip of an exit-code assignment.
Level of scrutiny
Moderate — cross-thread state on the worker termination path warrants a synchronization check, which I did: request_termination (parent thread) acquires vm_handle.lock() at line 546, publishes requested_terminate, and only then stores terminated_by_parent at line 554 while still holding the lock. The new reader takes the same lock, so if the worker's load bailed because it observed requested_terminate, acquiring the lock here blocks until the parent has released it after writing terminated_by_parent = true. The Ordering::Relaxed is fine because the mutex provides the happens-before edge; the doc comment states this explicitly per REVIEW.md's "comment any weakened ordering" rule. The worker thread does not hold vm_handle at the spin call site (it's published-then-released earlier, re-taken only in shutdown()), so no self-deadlock.
Other factors
This is my fourth look at this PR. The first revision fixed it in the proxy (WorkerMessagingProxy.cpp), which I flagged as able to zero out a worker-chosen error code (uncaught exception's 1, unsettled-TLA's 13) when terminate() raced the error. The author moved the fix to the specific load-abort Err arm on the worker thread, which is the only path that was wrongly producing a 1 — the uncaught-exception and TLA paths write their codes elsewhere and are untouched. The latest push only adds a debug-build timeout to a neighbouring stress test (48 worker boots in a subprocess), which is the "rare outlier" case the root CLAUDE.md carves out. Tests await real signals (once(w, "message"), once(w, "close")) with no sleeps, use test.each, and assert a combined object with .toEqual. The bug hunter ran to dry_streak with no findings.
…of raising its timeout REVIEW.md: shrink the workload rather than raise a per-test timeout. Four rounds of four workers still put terminate() against in-flight readFile completions at delays 0-6ms; a debug build gets through them in ~2.5s.
This branch has not been deployed
2 participants
Problem
closeevent code afterworker.terminate()depends on timing. A worker stopped while it still evaluates its entry module reports{ code: 1, wasClean: false }. A worker stopped once its loop is idle reports{ code: 0, wasClean: true }. Aterminate()about 30 ms afternew Worker()gives a mix of both.CloseEvent"contains the exit code passed toprocess.exit(), or 0 if it closed for another reason".src/jsc/web_worker.rs:load_entry_point_for_web_workerreturnsErr(WorkerTerminated)once the parent asked for termination, and theErrarm setexit_code = 1as if the load had failed. The idle loop breaks out without touchingexit_code.Fix
Errarm no longer setsexit_code = 1when the parent'sterminate()is what cut the load short (termination_requested_by_parent()). It still does for a real load failure, and still leaves aprocess.exit()code alone.termination_requested_by_parent()readsterminated_by_parentunder thevm_handlelock.request_termination(parent thread) setsrequested_terminateand thenterminated_by_parentwhile it holds that lock, so a worker that already saw the request cannot read a stalefalse.node:worker_threadsstill reports 1 for a worker its parent stopped, as Node does. A Web Worker now reports the untouched 0. A code the worker chose itself (an uncaught error's 1, an unsettled top-level await's 13,process.exit(n)) is never overwritten.test/js/web/workers/worker.test.ts("close code is 0 after terminate() ..."). The busy case fails on 1.4.3 with{ code: 1, wasClean: false }, and 40 alternating busy/idle runs on this build all give 0. Also ran the other terminate tests in that file and the terminate/exit-code tests intest/js/node/worker_threads/worker_threads.test.ts(still expect 1).Background
WebWorker__workerGlobalScopeDestroyed(proxy, exit_code, stopped_by_parent). The proxy (WorkerMessagingProxy, parent thread) turns that into thecloseevent for the globalWorkerand theexitevent fornode:worker_threads, and already maps "stopped by parent" to Node's 1 forKind::Node.requested_terminateis set by the parent'sterminate(), by an exiting ancestor, or by the worker itself (process.exit(), an uncaught error).terminated_by_parentis the subset "the parent asked while the VM was live", which is the case that must not count as a failure.Notes
stoppedByParentforces 0 forKind::Web). Review pointed out thatstoppedByParentonly excludesprocess.exit(), so aterminate()that raced a resolve failure or an unsettled top-level await would have turned their 1 or 13 into 0. Fixing the producer avoids that.