Repository navigation
Conversation
…tics Store process.exitCode as an int32 instead of a masked u8 so the getter and the beforeExit/exit event arguments keep the raw value (-1, 300), reset it on a nullish assignment (including process.exit(null) and process.exit(undefined)), and re-read it after the exit listeners run so a listener can override the code passed to process.exit(). Only the final exit(2) masks to 0-255. Route the default unhandled-rejection mode through the same path as --unhandled-rejections=throw, which is Node's default: with no unhandledRejection listener the rejection is raised as an uncaught exception so uncaughtExceptionMonitor and uncaughtException listeners run, and an uncaughtException listener keeps the process alive. This also removes a duplicate error report on the throw-mode fatal path. events: read EventEmitter.prototype[kCapture] at emit time instead of baking the choice in at construction, so the global EventEmitter.captureRejections setting applies to instances whose constructor never ran (util.inherits), and duck-type .then on any non-nullish listener return value like Node so plain thenables are captured and a throwing `then` getter is routed to the error event.
|
Updated 4:24 AM PT - Jun 27th, 2026
❌ @robobun, your commit 3a986dd has some failures in 🧪 To try this PR locally: bunx bun-pr 32814That installs a local version of the PR into your bun-32814 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR updates EventEmitter rejection capture, exit-code representation and propagation across VM/runtime bindings, unhandled-rejection handling in the VM, and tests for thenables, exit-code semantics, and rejection routing. ChangesProcess and event handling
Suggested reviewers
🚥 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: 3
🤖 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 `@test/js/node/events/event-emitter.test.ts`:
- Around line 849-853: The subprocess tests around the `proc`/`proc2` assertions
are not reading `stderr`, so the pipe can block and failures won’t show stderr
in the expectation. Update both tests to drain `stderr` alongside `stdout` and
`exited`, then include the drained stderr in the final object asserted by
`toEqual` (use `.trim()` on the collected output to ignore harmless noise).
Locate the changes in the `Promise.all` blocks that currently only await
`proc.stdout.text()`/`proc.exited` and adjust the corresponding `expect` calls
to compare the combined stdout/stderr result.
In `@test/js/node/process/process.test.js`:
- Around line 998-1008: The new inline subprocess cases in runInlineFixture
leave stderr unread, which can cause hangs or missed failures. Update the
runInlineFixture helper so it drains proc.stderr concurrently by including
proc.stderr.text() in the same Promise.all as the other pipe reads, and ensure
the subprocess test cases that use it continue to await all streams through that
helper.
- Around line 879-890: The spawned-process test currently relies on setTimeout
and only reads stdout, which can make ordering flaky and leave stderr
unconsumed. Update the test around the proc fixture to use a turn-based callback
like setImmediate instead of setTimeout in the spawned script, and in the
assertion path read stdout, stderr, and proc.exited together in a single
Promise.all so both pipes are drained concurrently. Keep the change localized to
the test that exercises process behavior and its spawned fixture.
🪄 Autofix (Beta)
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: 9f7c7484-9e7b-4a28-862b-219a889d74cb
📒 Files selected for processing (8)
src/js/node/events.tssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/web_worker.rssrc/runtime/hw_exports.rssrc/runtime/node/node_process.rstest/js/node/events/event-emitter.test.tstest/js/node/process/process.test.js
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Process_functionReallyExit masked process.exit(N)'s code to a u8 before
handing it to Bun__Process__exit, so in a worker the parent's
worker.on("exit", code) saw 44 for process.exit(300) while the
process.exitCode = 300 spelling correctly produced 300. Node passes the
raw int32 to the parent for both. The main thread is unaffected: the OS
status is still masked to 0-255 at the final exit(2).
Also drain the subprocess stderr pipes in the new tests, replace the
setTimeout in the rejection-routing fixture with setImmediate, and pin
the int32 wrap boundary (process.exitCode = 2**31 reads back as
-2147483648, matching Node).
|
Pushed 1daeb95 addressing the review feedback:
On the suggested issue links: I reproduced and tested each before claiming anything.
|
The parameter widening has to touch this declaration, and the trailing note was a TODO marker. Keep the durable part (the attribute is deliberately left as a comment because the real [[noreturn]] trips ASan) as a plain why-comment on its own lines.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/process/process.test.js (1)
12-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHave the helper pin
stderrfor clean-path fixtures.
runInlineFixture()now drainsstderr, but it still treats it as informational only. That means the new success-path callers at Line 1028 and Line 1155 would keep passing if Bun starts printing warnings or duplicate errors, so this helper is not asserting the strongest invariant yet. Add anexpectedStderrparameter (defaulting to"") and assert it before the exit-code check.Suggested fix
-async function runInlineFixture(script, expectedStdout = null, expectedCode = 0) { +async function runInlineFixture(script, expectedStdout = null, expectedCode = 0, expectedStderr = "") { using dir = tempDir("process-test", { "index.js": script, }); @@ const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); if (expectedStdout !== null) { expect(stdout).toBe(expectedStdout); } + if (expectedStderr !== null) { + expect(stderr).toBe(expectedStderr); + } expect(exitCode).toBe(expectedCode); return { stdout, stderr, exitCode }; }As per coding guidelines, subprocess tests should assert the strongest invariant, and based on learnings, clean subprocess cases in this repo are expected to pin
stderr: "".🤖 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 `@test/js/node/process/process.test.js` around lines 12 - 31, runInlineFixture() currently reads stderr but does not verify it for clean-path cases, so success fixtures can still pass if Bun emits unexpected warnings or errors. Update the helper to accept an expectedStderr parameter defaulting to an empty string, and assert stderr against it alongside the existing stdout and exitCode checks. Then update the success-path callers that rely on runInlineFixture() to explicitly pin stderr to "" so the process tests enforce the strongest invariant.Sources: Coding guidelines, Learnings
🤖 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 `@test/js/node/events/event-emitter.test.ts`:
- Around line 852-853: The EventEmitter test is still asserting an exact empty
stderr string, which is brittle in debug/ASAN lanes. Update the assertion in the
stdout/stderr/exitCode check to normalize stderr first (for example by using
stderr.trim()) or omit stderr from the equality if it is not part of the
behavior under test. Keep the existing Promise.all drain and the focus on the
relevant symbols in this test block, such as proc.stderr.text() and the expect({
stdout, stderr, exitCode }) assertion, and apply the same fix to the other
matching assertion pair as well.
---
Outside diff comments:
In `@test/js/node/process/process.test.js`:
- Around line 12-31: runInlineFixture() currently reads stderr but does not
verify it for clean-path cases, so success fixtures can still pass if Bun emits
unexpected warnings or errors. Update the helper to accept an expectedStderr
parameter defaulting to an empty string, and assert stderr against it alongside
the existing stdout and exitCode checks. Then update the success-path callers
that rely on runInlineFixture() to explicitly pin stderr to "" so the process
tests enforce the strongest invariant.
🪄 Autofix (Beta)
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: ca1cc2f8-1310-45f6-a611-37e0cf04eacc
📒 Files selected for processing (7)
src/jsc/VirtualMachine.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/headers.hsrc/runtime/jsc_hooks.rssrc/runtime/node/node_process.rstest/js/node/events/event-emitter.test.tstest/js/node/process/process.test.js
… report The previous commit routed the default mode (Mode::Bun) through the uncaughtException machinery. That regressed bun --hot (the reused VM's armed exit_on_uncaught_exception guard killed the watcher on the second reload's error, plus a second sourcemap regression in hot.test.ts) and also reaches a worker-thread panic path and bypasses HTMLRewriter's rejection-capture scope. uncaught_exception() carries invariants that only its existing callers satisfy, so the default mode is restored to its previous behavior; changing it needs that function hardened first. The --unhandled-rejections=throw arm keeps one fix: it no longer falls through to a second error report after uncaught_exception has already printed one and set the exit code. Also from review: the EventEmitter.captureRejections setter no longer clobbers a user-installed prototype emit, the new subprocess tests normalize stderr with .trim() and use setImmediate, and the note on the Bun__Process__exit declaration (a line the parameter widening has to touch) is reworded from a TODO marker into the why-comment it expressed.
robobun
changed the title
|
Scope change in 4e2609a, summarizing for anyone following along rather than reading the threads. The default unhandled-rejection routing (originally fix #4 of this PR: make Bun's default mode raise an unhandled rejection as an uncaught exception so
All four already exist, latently, on the opt-in What ships here is the part that is independently proven and has no CI regressions: the whole |
There was a problem hiding this comment.
Thanks for updating the title/description — I have nothing further to flag, but this touches core process-lifecycle paths (exit-code ABI widening across the C++/Rust boundary, unhandled-rejection dispatch, worker exit propagation) and went through a mid-review scope change, so a maintainer should sign off.
Extended reasoning...
Overview
This PR widens process.exitCode storage from u8 to i32 end-to-end (Rust ExitHandler, C++ Bun__getExitCode/Bun__setExitCode/Bun__Process__exit/Process__dispatchOn{Before,}Exit signatures, headers.h, web_worker.rs, hw_exports.rs, jsc_hooks.rs, node_process.rs), reworks Process_functionExit/setProcessExitCodeInner/Process_functionReallyExit for Node-matching nullish-reset / re-read-after-exit-listener / raw-int32 semantics, fixes the --unhandled-rejections=throw double-report fall-through in VirtualMachine::unhandled_rejection, and fixes two node:events captureRejections divergences (thenable duck-typing in addCatch, prototype-emit sync in the global setter). 17 new tests across two files; an extensive Node-parallel test sweep is listed as re-verified.
Security risks
None identified. No auth/crypto/permissions/network surface; the changes are Node-compat semantics for exit codes and error routing. The FFI signature widening (u8→i32) is the closest thing to a hazard, but every declaration site on both sides of the boundary was updated in lockstep and the diff is internally consistent.
Level of scrutiny
High. This is core runtime: process exit, worker-thread exit-code propagation, and the unhandled-rejection dispatch path. The PR already surfaced four real regressions during review (two bun --hot breakages, a worker-thread panic!, and an UnhandledRejectionScope bypass) that led the author to back out the Mode::Bun routing change in 4e2609a. The remaining diff is well-scoped and well-tested, but cross-language ABI changes plus a mid-review scope reduction are exactly the kind of thing a maintainer should eyeball before merge. The robobun build status on 4e2609a also showed failures (Build #65346), with a subsequent merge-from-main on top — CI on the final head should be confirmed green.
Other factors
All seven of my prior inline findings and all coderabbit findings are resolved; the author updated the PR title/description per my last comment, and the description now accurately documents what was backed out and why. The bug-hunting system found nothing on this run. I'm deferring rather than approving solely because of the criticality of the touched paths and the FFI signature changes, not because of any open concern with the code itself.
|
CI status, for whoever merges this. Build 65373 is now final: out of 287 jobs, 285 passed and exactly 1 failed. The single failure is BuildKite infrastructure, not a test. The diff is done and fully reviewed: every review thread from claude[bot] (7) and CodeRabbit (4) is resolved, and the latest reviews from both report nothing further to flag. The one red job is a recurring artifact-download timeout on the macOS 26 aarch64 test agents. In two consecutive builds, on two different agents, the
I already spent this PR's one Earlier CI history for completeness: build 65346's 3 Windows failures were an upstream test broken at this branch's original base ( |
|
A fresh differential fuzz report landed on the While confirming that, the same bug turned out to exist in the browser |
|
The "intentionally out of scope" note at the bottom mentions that worker exit codes outside 0..65535 stay wrong in the parent because they ride The i32 widening itself overlaps this PR's VirtualMachine.rs / BunProcess.cpp / headers.h / node_process.rs / hw_exports.rs / jsc_hooks.rs / web_worker.rs hunks; the Worker.h / Worker.cpp / JSWorker.cpp / worker_threads.ts parts are additive. Happy to fold those into this PR or open a follow-up once this lands, whichever is easier. |
## What
`process.on('beforeExit', ...)` fired after a default-fatal uncaught
exception. Node never emits `'beforeExit'` in that case: its
fatal-exception path is effectively `process.exit(1)`, and the docs say
`'beforeExit'` is "not emitted for conditions causing explicit
termination, such as calling process.exit() or uncaught exceptions".
## Repro
```js
process.on('beforeExit', () => console.log('BEFOREEXIT'));
process.on('exit', c => console.log('EXIT', c));
setTimeout(() => { throw new Error('boom'); }, 1);
```
| | stdout | exit |
|---|---|---|
| Node v26.3.0 | `EXIT 1` | 1 |
| Bun (before) | `BEFOREEXIT` then `EXIT 1` | 1 |
| Bun (after) | `EXIT 1` | 1 |
## Cause
`VirtualMachine::uncaught_exception()` has two paths when no
`'uncaughtException'` listener handled the error:
- If `exit_on_uncaught_exception` is set, it hard-exits via
`process_exit(global, 1)`. But that flag is only armed inside
`Process__dispatchOnBeforeExit`, i.e. *after* `beforeExit` has already
fired.
- Otherwise it records the fatal state (`unhandled_error_counter += 1`,
`exit_handler.exit_code = 1`), prints the error and returns. The main
drain loop in `Run::start` then falls out (because
`is_event_loop_alive()` sees the nonzero counter) and unconditionally
calls `vm.on_before_exit()`.
## Fix
Gate both dispatch sites in `on_before_exit()` on
`unhandled_error_counter == 0` so `'beforeExit'` only fires on a natural
drain:
- Entry: skip entirely when we arrived via a fatal throw. Arm
`exit_on_uncaught_exception` ourselves here since the skipped
`Process__dispatchOnBeforeExit` was its sole setter; without it a throw
from an `'exit'` listener after a fatal throw would fall through and let
subsequent `'exit'` listeners run.
- Re-dispatch inside the drain loop: skip when work scheduled by a
`beforeExit` listener itself threw. On the main thread
`exit_on_uncaught_exception` already hard-exits this case; the guard
covers workers where that flag's hard-exit is main-thread-only.
Controls verified:
- With `process.on('uncaughtException', ...)` installed: `beforeExit`
still fires, exit 0. (New regression-guard test.)
- After a fatal throw, a throw from the first `'exit'` listener still
stops subsequent `'exit'` listeners. (New regression-guard test; would
regress without the flag-arming.)
- `test/cli/hot/hot.test.ts` including "should recover from errors"
passes; the watcher loop is unaffected since it proceeds to
`tick_possibly_forever()` either way, and `exit_on_uncaught_exception`
was already armed by the previous unconditional dispatch.
- All `test-process-beforeexit*` / `test-process-exit*` /
`test-worker-*beforeexit*` / `test-promises-unhandled-*` Node parallel
tests pass.
### Side effect: unhandled rejections
`unhandled_error_counter` is also bumped on the unhandled-rejection path
(`Mode::Bun` fallthrough, `Mode::Throw`), so this guard now also skips
`'beforeExit'` when an unhandled rejection is processed before the drain
loop exits (e.g. `Promise.reject(...); setImmediate(() => {});`). That
is a Node-parity improvement on that path, but it is not the whole
story: Bun's default mode still processes some rejections only at
`handle_rejected_promises()` *after* `on_before_exit()`, so
`setTimeout(() => Promise.reject(...), 1)` is unchanged. The full fix
(routing the default unhandled-rejection mode through
`uncaughtException`) is #32814's backed-out scope and needs the
watcher/worker hardening first; no test pins the partial behavior here.
## Verification
```
$ USE_SYSTEM_BUN=1 bun test test/js/node/process/process.test.js -t "is skipped after a fatal"
(fail) process.onBeforeExit > is skipped after a fatal uncaught exception
+ "beforeExit
+ exit 1
"
$ bun bd test test/js/node/process/process.test.js -t "is skipped after a fatal"
(pass) process.onBeforeExit > is skipped after a fatal uncaught exception
```
Found while investigating #34627; reproduces with or without top-level
`await`.
<!-- robobun:evidence:begin -->
---
**no test proof** · iteration 1 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/js/node/process/process.test.js
<!-- robobun:evidence:end -->
|
Overlap note. The What stays unique here is the exit-code half: Suggestion: when rebasing (this conflicts with main), drop |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-27, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
From a differential fuzz report comparing Bun against Node (v26.3.0) on process-lifecycle semantics. The report had five divergences; this PR fixes three (the exit-code ones) plus two
node:eventsbugs found while verifying it. The other two are deliberately not in this PR: #1 (a worker thread's exit suppressing the parent'sexitevent) is the same root cause and fix as the already-open #32264, and #4 (unhandled rejections never reaching theuncaughtExceptionmachinery in the default mode) was implemented here, caused twobun --hotregressions in review, and was backed out (details below).Repro
Cause
process.exitCodewas backed by au8on the VM (exit_handler.exit_code) and masked to 0-255 at assignment time, so the raw value was gone by the time the getter or thebeforeExit/exitevents read it. Node stores it in an int32 slot and masks only at the actualexit(2)(it wraps at the int32 boundary too:2**31reads back as-2147483648on Node v26.3.0, which a test pins).Process_functionExitalso read the code once before emittingexit, so a listener reassigning it was ignored (Node re-reads), and a nullish assignment was a no-op (Node resets to unset).Process_functionReallyExitstill narrowed the code back to au8, which only mattered for workers: the parent'sworker.on("exit", code)saw 44 forprocess.exit(300)while theprocess.exitCode = 300spelling produced 300. Both now produce the raw 300, matching Node.node:events captureRejections (verified against Node stage by stage)
EventEmitter.captureRejections = true(the global) was ignored by instances whoseEventEmitterconstructor never ran (theutil.inheritspattern), because Bun chooses the capturing vs non-capturingemitat construction. The global setter now keepsEventEmitter.prototype.emitin sync (without clobbering a user-installed prototype wrapper), andaddCatchgates onemitter[kCapture]at emit time like Node. That gate is load-bearing: Node does not retroactively capture for an already-constructed default emitter, and a test covers that negative contract.addCatchonly handled real Promises ($isPromise). Node duck-types.thenon any non-nullish listener return value inside a try/catch, so plain thenables are captured and a throwingthengetter is routed to theerrorevent. Both cases were silently dropped.--unhandled-rejections=throw double error report
The
throwarm's unhandled path calleduncaught_exception()(which reports the error and sets the exit code) and then still fell through to the default printer, so a truly unhandled rejection under--unhandled-rejections=throwwas reported twice. The fall-through is gone. Covered by a fail-before test.Backed out: routing the default unhandled-rejection mode through uncaughtException
Node's default raises an unhandled rejection as an uncaught exception, so
uncaughtExceptionMonitorand auncaughtExceptionlistener can handle it. Bun's default mode skips that machinery entirely;bun --unhandled-rejections=throwalready matches Node exactly (verified identical in every listener combination). Merging the default into thethrowarm was the obvious fix and this PR originally shipped it, but review and verification found thatuncaught_exception()carries invariants only its existing callers satisfy, and the default path violates them:bun --hotcallson_before_exit()after every drain, which arms the one-shotexit_on_uncaught_exceptionguard that nothing resets, so the second reload's error hard-exited the watcher.--hotregression inhot.test.ts's sourcemap tests (verified green with main'ssrc/and red with this PR's via a src-for-src swap).process_exithook returns instead of exiting, and the guard thenpanic!s the whole process.UnhandledRejectionScopecallers (HTMLRewriter) that overrideon_unhandled_rejectionget bypassed by an installeduncaughtExceptionlistener and leakexit_code = 1.All four are latent today on the opt-in
throw/strictarms (which already calluncaught_exception()); the change made them the default. Changing the default needsuncaught_exception()hardened for the--hotloop, workers, and capture scopes first, which is its own PR. The review threads onsrc/jsc/VirtualMachine.rshave the full analysis of each.Intentionally out of scope
process.on("exit")being suppressed after aworker_threadsWorker ran: process: add regression tests for per-VM 'exit' event guard #32264 (sameprocessIsExitingprocess-global static, already open).process.exit(-1)in a worker reports 65535; Node reports -1): the worker exit code reachesnode:worker_threadsthrough aCloseEventwhosecodeis anunsigned short. That transport predates this PR (it previously masked to a u8) and needs a non-CloseEventchannel.Verification
12 new tests in
test/js/node/process/process.test.jsand 5 intest/js/node/events/event-emitter.test.ts; 13 of the 17 fail on the unfixed build (the other 4 are negative contracts or regression guards that must not change). Every expected value was taken from Node v26.3.0.test/cli/hot/hot.test.ts(all 4 sourcemap tests) and the--hoterror-reload probe were re-verified green against the final head.Node parallel tests re-verified against the debug build (all pass)
test-event-capture-rejections.js,test-process-exit-code-validation.js,test-process-exit{,-recursive,-handler,-from-before-exit}.js,test-process-beforeexit{,-throw-exit}.js,test-promise-unhandled-{default,error,flag,throw,throw-handler,silent,silent-no-hook,warn-no-hook,issue-43655}.js,test-promises-{unhandled-rejections,unhandled-proxy-rejections,unhandled-symbol-rejections,warning-on-unhandled-rejection}.js,test-promise-handled-rejection-no-warning.js,test-worker-{on-process-exit,nested-on-process-exit,exit-from-uncaught-exception,uncaught-exception-async}.js,test-unhandled-exception-with-worker-inuse.js,test-util-callbackify.js,test-queue-microtask.js, alltest-event-emitter-*.js, plustest/js/node/promise/reject-tostring.test.tsandtest/js/bun/spawn/exit-code.test.ts.