Repository navigation
Conversation
…ners under bun test
The process-global isBunTest flag made a worker VM's uncaught_exception
and unhandled_rejection short-circuit into the worker's last-resort error
dispatcher, skipping Bun__handleUncaughtException/Bun__handleUnhandledRejection
and the exit_code = 1 write. So under 'bun test' a worker that threw
synchronously emitted 'exit' with code 0 (Node and standalone Bun give 1),
and its own process.on('uncaughtException'/'unhandledRejection') listeners
were never consulted.
The isBunTest short-circuit exists so jest's main-thread reporter captures
errors; in a worker VM on_unhandled_rejection is the worker error dispatcher,
not jest's, and the test runner has no visibility into the worker's error
counter. Gate the short-circuit on is_main_thread() so workers under
'bun test' take the normal path and behave the same as they do standalone.
|
Warning Review limit reached
Next review available in: 7 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrowly scopes the isBunTest error short-circuit to the main-thread VM so worker error handling matches standalone behaviour.
What was reviewed:
- Confirmed
is_main_thread()isself.worker.is_none()and that a worker VM'son_unhandled_rejectionis theweb_worker.rsdispatcher, not jest's — so the short-circuit was indeed skippingBun__handleUncaughtExceptionand theexit_code = 1write for workers. - Checked these are the only two Rust-side
isBunTestreads; no sibling sites left unfixed. - Tests run in-process (required — the bug only reproduces under
bun test), assert exact event sequences and exit codes, and the re-enabledgetHeapSnapshot"entry throws" case was previously omitted for exactly this reason.
Extended reasoning...
Overview
Two-line fix in src/jsc/VirtualMachine.rs: adds && self.is_main_thread() to the isBunTest short-circuit in both uncaught_exception and unhandled_rejection. Three new in-process tests in worker_threads.test.ts covering sync-throw exit code, worker-side uncaughtException, and worker-side unhandledRejection; one previously-skipped test.each row re-enabled.
Security risks
None. Pure control-flow gating on an existing predicate; no new inputs, no allocation, no FFI surface change.
Level of scrutiny
Low-to-moderate. The change is a two-token predicate addition at each of two sites; the surrounding fall-through path is the same code that already runs for workers outside bun test. I verified is_main_thread() returns self.worker.is_none() (VirtualMachine.rs:970) and that web_worker.rs installs its own on_unhandled_rejection on the worker VM (web_worker.rs:953, 1496), which confirms the PR description's mechanism: under bun test the short-circuit was calling the worker error dispatcher (fine) but skipping Bun__handleUncaughtException/Bun__handleUnhandledRejection (so worker process.on listeners never ran) and skipping exit_handler.exit_code = 1.
Other factors
- Grepped for other
isBunTestreads in Rust — these are the only two, so the whole class is covered. - The tests are correctly placed in the existing
error eventdescribe, run the Worker in-process (with a comment explaining why spawning would defeat the repro), assert combined{events, code}objects, and wireerrorinto the observed events so a regression would show up as an extra entry rather than a hang. - The re-enabled
getHeapSnapshot"entry throws" row is a good signal: its exclusion comment described exactly this bug, and removing the workaround comment while re-enabling the case is the right cleanup. - PR description traces the mechanism end-to-end (JSInternalPromise →
spin()→uncaught_exceptionreturninghandled=true→exit_codenever set) and notes composition with #34193.
|
Closing this PR because it has been inactive for more than 90 days. |
Repro
A
node:worker_threadsWorker that throws synchronously emits'exit'with code 1 when run standalone, but code 0 when run insidebun test. Node gives 1 in both contexts. Separately, a worker's ownprocess.on('uncaughtException')/process.on('unhandledRejection')listeners are never called underbun test.Cause
isBunTestis a process-global atomic, so it is true inside a worker VM spawned by a test. BothVirtualMachine::uncaught_exceptionandVirtualMachine::unhandled_rejectionhave a short-circuit whenisBunTestis set: incrementunhandled_error_counter, call(self.on_unhandled_rejection)(...), return. That short-circuit skipsBun__handleUncaughtException/Bun__handleUnhandledRejection(so the worker's ownprocess.on(...)listeners never run) and skips theexit_handler.exit_code = 1write. The short-circuit exists so jest's main-thread reporter captures errors instead of the user'sprocesslisteners swallowing them, but in a worker VMon_unhandled_rejectionis the worker error dispatcher inweb_worker.rs, not jest's, and the test runner has no visibility into the worker's error counter.For the sync-throw case the entry promise is a
JSInternalPromise, so the rejection is not tracked viapromiseRejectionTracker;spin()seesRejected, callsuncaught_exception, getshandled == trueback, and continues withexit_codestill 0. (An asyncPromise.rejectat top level accidentally gave 1 because the rejection tripshandle_rejected_promisesinsidewait_for_promise_with_termination, which makesload_entry_point_for_web_workerreturnErr(WorkerTerminated)and hitspin()'s Err arm.)Fix
Gate both
isBunTestshort-circuits onself.is_main_thread(). Worker VMs underbun testnow take the normal path: the worker's ownuncaughtException/unhandledRejectionlisteners run, and an unhandled error setsexit_code = 1before dispatching to the parent, same as standalone.Verification
Three new tests in the
error eventdescribe ofworker_threads.test.tsrun the Worker directly inside the test process (the bug only reproduces underbun test): sync-throw exit code 1, worker'suncaughtExceptionlistener is called, worker'sunhandledRejectionlistener is called. All three fail on main and pass with the fix; all three match Node's output.Also re-enabled the
entry throwscase in thegetHeapSnapshotrejects-ERR_WORKER_NOT_RUNNING table, which had been omitted with a comment explaining thatisBunTestmade it resolve instead of reject underbun test.Full
worker_threads.test.ts(95 tests),test/js/web/workers/worker.test.ts, and the vendoredtest-worker-*uncaught*/test-worker-*exit*Node tests pass.Related
#34193 sets
exit_code = 1in the worker's last-resorton_unhandled_rejectionfor the standalone unhandled-rejection path; this PR fixes thebun test-specific divergence. They compose without conflict.[review] gate passed · iteration 0 · 2 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
self-review · no surviving concerns
30 concerns were raised and did not survive verification.