Repository navigation
Conversation
A node:worker_threads worker that died from an uncaught error posted its 'error' to the parent before it ran its process 'exit' listeners. The stdio flush is one of those listeners, so the parent's 'error' event fired while the worker's last output was still unsent, and a parent that left from its 'error' listener lost that output. WebWorker__dispatchError is split into the worker's own 'error' event and the post to the parent. report_fatal runs a node:worker_threads worker's 'exit' listeners between the two, which is the order of node's workerOnGlobalUncaughtException. It reports an uncaught error, and a start that fails after a preload ran. As in node, the exit code is 1 before the listeners run, and a listener that stops the worker (process.exit(), or terminate() arriving meanwhile) withdraws the error. An error that the worker's 'uncaughtException' handler threw is always posted: node runs no 'exit' listeners there. A Web Worker keeps its order.
|
Reproduced on main 449877f (Linux x64, release and debug builds).
PR: #46948 |
|
Updated 10:21 PM PT - Oct 25th, 2026
✅ @robobun, your commit 6dcf502bb57ad5e7c381bc0eeb83af579e43cf87 passed in 🧪 To try this PR locally: bunx bun-pr 46948That installs a local version of the PR into your bun-46948 --bun |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Error dispatch contract and worker state src/jsc/web_worker.rs, src/jsc/bindings/webcore/Worker.cpp, src/jsc/VirtualMachine.rs |
The worker binding separates global-scope error-event dispatch from parent Worker notification. Worker state records when an uncaughtException handler throws. |
Fatal and log error reporting src/jsc/web_worker.rs, src/jsc/VirtualMachine.rs |
Log errors dispatch on the worker global and then post to the parent. Fatal reporting runs Node worker exit handlers before parent notification and may suppress notification if termination follows an uncaught error. Startup failures report a fatal error when one can be built from the VM log. |
Worker error and startup lifecycle tests test/js/node/worker_threads/worker_threads.test.ts, test/js/bun/cron/in-process-cron.test.ts |
Tests cover output and event ordering, exit codes, termination, and startup failures. Some tests compare behavior with Node 26.3.0 when available. The cron regression-test comment names the parent-notification entry point. |
Suggested reviewers: dylan-conway, jarred-sumner
Priority:
Merge Risk: 
6dcf5
-
No specific issue has been established that would block merging after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the primary change: posting a worker's fatal error after its 'exit' listeners. Description check The description explains the problem, implementation, behavior changes, limitations, and verification results. It does not use the template headings exactly, but it includes the required information a… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, implementation, behavior changes, limitations, and verification results. It does not use the template headings exactly, but it includes the required information and is substantially complete.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the BunString ownership across the split FFI (&BunString borrow for WebWorker__dispatchErrorEvent, by-value transfer consumed by transferToWTFString in WebWorker__postErrorToWorkerObject; the withdrawn-post early return in report_fatal drops the string via bun_core::String's Drop), and the swap of on_unhandled_rejection to the quiet capture handler, which is never restored but is only reached on paths that arm termination or shutdown() immediately after — neither is a problem.
Extended reasoning...
The change splits the worker fatal-error FFI into two calls and reorders a node:worker_threads worker's 'exit' listeners ahead of the parent-side error post, touching src/jsc/web_worker.rs, src/jsc/VirtualMachine.rs and src/jsc/bindings/webcore/Worker.cpp plus tests; no security-sensitive surface. Three confirmed inline findings (exit-code override when an 'exit' listener throws, parent terminate() during listeners now losing 'error', and a vacuous stderr assertion in the tests) already signal that a human should look; this note only records the ownership and handler-swap checks that were ruled out.
| // As in node, a stop during the listeners withdraws the error, unless a handler threw it. | ||
| if !vm.script_allowed() && cause == FatalCause::Uncaught { | ||
| return; |
There was a problem hiding this comment.
🟡 (optional) Parents that terminate a dying node worker on seeing its last output now get 'exit' with code 1 and no 'error', where the base delivered 'error' first. report_fatal runs the 'exit' listeners (the stdio flush at src/js/builtins/ProcessObjectInternals.ts:108) before the post, and src/jsc/web_worker.rs:1201 withdraws the post once the parent's terminate() flips script_allowed. The flush posts the parked stdout batch synchronously (src/js/internal/worker/stdio.ts:101-104), so the parent thread handles that data concurrently while the worker is still in its listeners. Fix: post the error whenever the stop came from the parent (stopped_by_parent), withdrawing only on the worker's own process.exit(); node delivers an already-queued error after terminate().
Why this was flagged
A node:worker_threads worker throws uncaught; its parent has worker.stdout.on('data') that calls worker.terminate() on the final line. On base, on_unhandled_rejection posted the error before dispatch_on_exit, so the parent's task queue held 'error' ahead of the stdout data. With the diff, report_fatal at src/jsc/web_worker.rs:1196-1197 runs the listeners first; the stdio flush pushes the batch over the MessagePort and the parent thread dispatches 'data' while the worker thread is still inside dispatch_on_exit. The parent's terminate() calls request_termination, so vm.script_allowed() at src/jsc/web_worker.rs:1201 is false and report_fatal returns without WebWorker__postErrorToWorkerObject. Even if it had posted, Worker::dispatchEvent at src/jsc/bindings/webcore/Worker.cpp:134 drops it after m_wasTerminated. The parent sees only 'exit' with code 1 and the actual Error object is gone. Remedy: do not withdraw the post when the stop was requested by the parent (terminated_by_parent) rather than by the worker's own exit.
Verification: src/jsc/web_worker.rs:1189-1207 (report_fatal): for a node worker, ExitHandler::dispatch_on_exit(vm) (line 1196-1198) runs before WebWorker__postErrorToWorkerObject (line 1205-1207). src/jsc/bindings/webcore/Worker.cpp:111-115 Worker::terminate() sets m_wasTerminated = true, and lines 132-137 Worker::dispatchEvent returns early if (m_wasTerminated || ...). The parent observes 'exit' 1 with no 'error'.
There was a problem hiding this comment.
Not changed, for two reasons.
A post from the worker cannot reach the parent's listeners in this case. Worker::dispatchEvent returns early once terminate() was called (Worker.cpp:134), on main too. The suggested change would also make the worker post after a terminate() that arrives during its 'exit' listeners. Node does not post there, because the isolate stops inside the listener. The test terminate() during the 'exit' listeners leaves no 'error' runs under node and passes.
The case in the finding is a race in every runtime. A worker prints 30 lines and throws, and the parent calls terminate() when it reads the last line. In 30 runs each, 'error' fires in 17 on main, 0 with this PR and 9 on node v26.3.0. To deliver an error that was queued before terminate() needs a change to the parent's terminate(), which #44252 rewrites. The PR body now lists this case under Downsides.
… worker throws report_fatal raised the uncaught-exception guard around the 'exit' listeners. What a listener threw then took the "handler threw" arm of uncaught_exception, which sets exit code 1. A code that an earlier listener had set was lost on two paths that kept it before: an unhandled rejection in a worker that has an 'uncaughtException' listener, and a start that fails after a preload ran. report_fatal no longer changes the guard or the rejection handler. After an uncaught error or a rejection, a throw from an 'exit' listener is handled as it was before the post moved. After a failed start the listeners run with is_shutting_down set, as they do in shutdown(), so the VM drops what one throws. The new tests now assert the parent's actual stderr.
|
Review follow-up in 6dcf502.
|
There was a problem hiding this comment.
I re-reviewed after the second commit and found no new bugs; the exit-code overwrite and the vacuous stderr assertion from my earlier comments are addressed by it. Because this reorders user-JS execution inside the worker's fatal/shutdown path (VM flags, termination state, FFI ownership), a human look at the lifecycle reasoning is still worthwhile, in particular the deliberate choice that a parent terminate() during the 'exit' listeners now withdraws the error.
What was reviewed:
report_fatalon the StartFailure path:is_shutting_downis set before the listeners (asexit_and_freedoes), a throwing listener is dropped byuncaught_exception's early return, anddispatchExitInternal'sm_isExitingguard keepsshutdown()from running the listeners a second time.- The Uncaught/HandlerThrew paths no longer touch
is_handling_uncaught_exceptionor the rejection handler, so a listener throw is handled exactly as on the base branch. - Ownership at the split FFI boundary:
&BunString->toWTFString()(non-consuming), by-valueBunString->transferToWTFString(); the early-return withdraw drops the Rust-owned string normally. - Tests:
runParentnow returns the real stderr, no sleeps, pipes drained concurrently,nodeExeAtLeastskips cleanly when node is absent.
Extended reasoning...
The change splits the worker fatal-error FFI into a worker-scope 'error' event and the post to the parent, and for node:worker_threads workers runs the process 'exit' listeners between them in src/jsc/web_worker.rs, with a small hook in src/jsc/VirtualMachine.rs and the C++ split in src/jsc/bindings/webcore/Worker.cpp, plus ~360 lines of new subprocess tests. It touches no security-sensitive surface (no auth, parsing of untrusted bytes, or crypto) but does touch VM shutdown state and exception handling around user JS. The second commit addressed two of my three prior inline comments; the third (error withdrawn when the parent terminates during the listeners) is a documented design choice the author made deliberately. The lifecycle complexity and that open behavioural trade-off are why this is a defer rather than an approve.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Problem
node:worker_threadsworker that dies from an uncaught error posts'error'before it runs its process'exit'listeners. The stdio flush is one, so'error'fires with 1 of 30 lines delivered (node: 30).on_unhandled_rejectioncallsWebWorker__dispatchError(src/jsc/web_worker.rs:1216) beforedispatch_on_exit(:1231). A failed start after a preload (:877) does the same.Fix
WebWorker__dispatchErrorinto the worker's own'error'event and the post.WebWorker::report_fatalruns a node worker's'exit'listeners between them, as node'sworkerOnGlobalUncaughtExceptiondoes.'uncaughtException'handler's own throw is always posted.test/js/node/worker_threads/worker_threads.test.ts(32 new tests, 14 fail on main, 10 run under node),test/js/web/workers/, vendoredtest-worker-*.js.terminate()test (the parent drops those events).Background
MessageChannelper stream. Writes wait for the parent's ack. A worker that dies first sends them from an'exit'listener (worker_threads: don't drop stdout/stderr on synchronous worker exit; honor exitCode set in 'exit' listeners #38229).'error'event moves.Downsides
process.exit()in an'exit'listener after an uncaught error withdraws'error'. After an unhandled rejection in a callback the worker exits 1, not 0.terminate()on the worker's last output gets no'error'(main: 17 of 30 runs, node: 9).Notes
Repro without a gate (
bun busy.cjs). A worker logs, works for 300 ms, logs again and throws. The parent exits in its'error'listener.Without the
'error'listener (the parent dies of the worker's error): main 16 of 20, this PR 20 of 20, node 10 of 10.Gated repro. The worker blocks until the parent has read
W0, then writes 29 more lines and throws in the same turn.{"linesAtError":1,"linesAtExit":30}on main,30and30with this PR and on node.Node reference.
workerOnGlobalUncaughtExceptionin lib/internal/main/worker_thread.js:process.exitCode = 1at L261,process.emit('exit')at L263 (skipped when the handler threw),serializeErrorat L273,ERROR_MESSAGEat L280. The flush is an'exit'listener there too (is_not_main_thread.js#L43).Behaviour, each row run on main, this PR and node v26.3.0.
'exit'listener writes to stdout'error''error''error''exit'listener changeserror.message'exit'listener callsprocess.exit(7)'error', exit 7process.exit()'error', exit 0process.exit(7)'error', exit 7--requirethat fails)terminate()during an'exit'listener'error', exit 1'exit'listener throws'error'(first), exit 1'error', exit 1'error', exit 1'uncaughtException'handler throws, an'exit'listener callsprocess.exit()'error', exit 1'error', exit 1'error', exit 1'exit'listener'error', exit 0'error', exit 1'error', exit 1Where Bun differs from node on purpose. After a handler that throws, node runs no
'exit'listeners, so it loses the parked output (1 of 30 lines ever arrive). Bun runs them there, as before this PR, and now posts the handler's error after them. pino'sthread-streamworker has this shape (a handler that can throw, and an'exit'listener that callsprocess.exit()): its parent still ends with the worker's error.Kept as on main.
'error'event (globalThis.addEventListener("error")) still fires first, withprocess._exitingfalse. When its listener callsprocess.exit(5), the parent still gets'error'and exit 5.Worker) keeps its order: the post, then the'exit'listeners. Its stdio writes to the file descriptor, so it loses no output, and the HTML spec has no process'exit'to order against. Two new tests pin that order.flush_logsis unchanged for every call that cannot follow user code, and for a worker that runs. worker: fire 'error' for the VM log only when the worker fails #45203 owns the report of a worker that runs.postMessageToThread()message still reaches its'workerMessage'listeners before the'exit'listeners, and aprocess.exit()in one does not lose the start failure.'exit'listener, on every path (next table). After a failed start the listeners ran insideshutdown()on main, withis_shutting_downset: the VM drops the throw there.report_fatalsets that flag for a failed start, and only for it.Exit code when one
'exit'listener setsprocess.exitCode = 7and a later one throws.'uncaughtException'listenerpreloadoption)'uncaughtException'handler threwOther behaviour changes.
'exit'listeners post withparentPort.postMessage()now reach the parent before'error'. Node has no order for these.'exit'listeners. A listener that makes the clone fail (for example astackgetter that throws) leaves the parent with the error's formatted text, as for any error that Bun cannot clone. Node still delivers the message.terminate()from a handler of the dying worker's last output gets no'error': theWorkerdrops every event afterterminate()(Worker.cpp:134), and the output now comes first. This is a race on main and on node. A worker prints 30 lines and throws, and the parent terminates on the last line:'error'fires in 17 of 30 runs on main, 0 of 30 with this PR, 9 of 30 on node.Measurements (release builds, main 449877f against this PR).
'error': gated repro 20 of 20 runs at 30/30, stderr variant 20 of 20, start failure after a preload 40 of 40 at 31/31. Main: 1/30, 1/30, and 11 of 20 runs. Node: all.'error'event timing is unchanged in 5 of 5.flush_logscalls 2 -> 2 (each now calls the newlog_erroronce), error-path calls 0 -> 0. Dying worker: error posts 1 -> 1, exit dispatches 2 -> 2 (gdb hit counts).size).WebWorker__symbols: 5 -> 5 (nm).size_of::<WebWorker>(): 216 -> 216.MessagePort::postMessage/MessagePortPipe::drainAndDispatch/ScriptExecutionContext::postTaskTosizes changed by 0 / 0 / 0 bytes.'error'(node 3000), and late stdio attach 0 of 30 (node 30).What this does not fix.
'exit'listeners: the parent drains 1024 port messages per task, so the rest can arrive after'error'.process.stdout,process.stderrandconsoleare flushed ahead of the post. Output throughBun.file().writer()orBun.stdout.writer()is not.'exit'listener on a natural exit is still not reported (node:'error', exit 0). Report a loop turn's rejections before the loop-alive check, and keep the exit code when an 'exit' listener throws #42032 owns it.'exit'listener, Bun still runs the listeners that follow. Node stops at the throw.'uncaughtException'listeners.terminate()is still dropped. Node delivers it. worker_threads: settle terminate() from the exit handler #44252 rewritesterminate().Designs weighed. A swap of the two calls (see Background). One port for stdio and the error, as in node: every stdio batch pays for it.
Changed after review. The first commit raised the uncaught-exception guard around the
'exit'listeners. A throw from a listener then set exit code 1 on the two paths that show 7 in the exit code table. The second commit removes the guard.Open PRs on the same lines.
on_unhandled_rejectionfor every kind of VM. This PR sets it for a node worker only, because a withdrawn error needs it.'error'event, so a listener's change does not reach the parent. This PR serializes after the'exit'listeners, as node does. The two cannot both hold, and need one ruling.WebWorker__dispatchErrorand conflict with the split.addEventListenerfrom a node worker's global. One new test uses it.Tests. 14 of the 32 new tests fail on main (debug and release). 10 are node's runs of the same scripts. 8 pass on main and guard behaviour that must stay. Each of 12 single edits to the fix makes at least one new test fail (for example: remove the stand-down, remove the exit code, post before the listeners for a Web Worker, move the hook behind the report, remove the shutdown flag). One edit passes every test: the shutdown flag for every cause, not only for a failed start.
Self-review, not done.
terminate()test fails on main, but it does not pin the worker-side return: the parent drops events afterterminate()either way.Suites (debug build).
test/js/node/worker_threads/(212 tests pass).test/js/web/workers/(487 pass, 4 time out: run alone, 2 of them time out on main too, 1 only on main, and 1 passes on both). 109 of 110 vendoredtest-worker-*.jsfiles as scripts (test-worker-arraybuffer-zerofill.jsneeds the test runner, on main too).worker_threads.test.tswith the ASAN lane's environment (BUN_JSC_validateExceptionChecks=1,BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1): 192 of 193 pass, andprocess.exit() with a shell cp in flightreports a leak in the shellcpbuiltin in 1 of 3 runs on main too.