Conversation
…fatal
When the entry module is suspended in top-level await and a callback
(timer, EventEmitter 'error', setImmediate, etc.) throws with no
process.on('uncaughtException') listener installed, Bun printed the
error but then resumed the suspended module and ran it to completion,
only exiting 1 at natural module end. Node exits immediately at the
throw.
The default uncaught-exception path in VirtualMachine::uncaught_exception
records the error (unhandled_error_counter += 1, exit_code = 1) and
returns. The main drain loop in Run::start checks unhandled_error_counter
via is_event_loop_alive() and bails, which is why a throw after TLA has
settled is already fatal. But load_entry_point waited on the TLA promise
via the generic wait_for_promise, which only watches promise status and
never consults unhandled_error_counter, so the suspended module kept
running.
Inline the wait loop in load_entry_point's non-watcher arm and break on
unhandled_error_counter > 0, matching the main drain loop.
|
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 (5)
Comment |
|
Updated 7:30 AM PT - Jul 18th, 2026
❌ @robobun, your commit bd5ceb1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34627That installs a local version of the PR into your bun-34627 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ssert; note watcher-arm exclusion
There was a problem hiding this comment.
No issues found — the inlined wait loop is a faithful copy of wait_for_promise plus the unhandled_error_counter bail-out, and all earlier review nits are addressed. Deferring to a human only because this changes exit timing on the core bun <file> entry-point path (VirtualMachine::load_entry_point).
What was reviewed:
- Confirmed the new loop matches
EventLoop::wait_for_promise(event_loop.rs:963-977) line-for-line aside from the addedunhandled_error_counter == 0guard. load_entry_pointhas a single caller (Run::startat run_command.rs:1506), so no other consumers are affected by the earlier bail.- Sibling TLA-wait sites (watcher arm,
--preload, test-runner) are documented as intentional exclusions in the PR body; the earlier stderr-empty and 4-line-comment nits were fixed in 23bd4d6.
Extended reasoning...
Overview
The PR replaces one wait_for_promise call in VirtualMachine::load_entry_point's non-watcher arm with an inlined tick/auto_tick loop that additionally breaks when unhandled_error_counter > 0. This makes an uncaught exception (or unhandled rejection) that fires while the entry module is suspended in top-level await immediately fatal, matching Node. Three fixtures + three tests cover the fatal throw case, the #22546 unhandled-rejection case, and the negative control (listener installed → module completes, exit 0). Net Rust change is ~15 lines; the rest is tests.
Security risks
None. No parsing of untrusted input, no auth/crypto/permissions surface. The change only causes the process to exit sooner on a path that already ends in exit code 1 — it strictly reduces post-fatal side effects rather than introducing any.
Level of scrutiny
High: load_entry_point is the core entry for bun run <file>, and this alters when the runtime stops driving the event loop during module evaluation. The mechanics look correct — I diffed the inlined loop against EventLoop::wait_for_promise and the only semantic delta is the new guard, which mirrors what is_event_loop_alive() already gates on in Run::start's main drain loop. load_entry_point has exactly one caller (run_command.rs:1506), so scope is contained. Still, a human maintainer should confirm this is the right layer for the fix (vs. e.g. threading a flag into wait_for_promise itself) and that the documented deferral of the --preload sibling is acceptable.
Other factors
All three inline nits from earlier review rounds are resolved (fixture comment reflowed to 3 lines, expect(stderr).toBe("") replaced with .not.toContain(...), watcher-arm exclusion commented in-source). The two additional sibling sites (load_preloads non-watcher arm, load_entry_point_for_test_runner) are now documented in the PR body with rationale. The gate evidence shows fails-without-fix / passes-with-fix on both ASAN debug and release. Tests follow harness conventions (bunEnv, concurrent pipe drain, exitCode asserted last, no exact-empty stderr).
|
CI on build 75380: the new test (
Remaining flaky tests ( Ready for review. |
## 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 -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#22546) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
An uncaught exception thrown while the entry module is suspended in top-level
awaitwas not fatal: Bun printed the error, then resumed the suspended module and ran it to completion (potentially seconds of continued side effects), only exiting 1 at natural module end. Node exits immediately at the throw.Repro
Any callback source triggers it (setTimeout throw, EventEmitter
'error'with no listener, setImmediate, Worker'error'); the only condition is that the entry module is TLA-suspended when the exception reaches the top with nouncaughtExceptionlistener.Cause
VirtualMachine::uncaught_exceptionrecords the fatal state (unhandled_error_counter += 1,exit_handler.exit_code = 1) and returns. The main drain loop inRun::startchecksunhandled_error_counterviais_event_loop_alive()and bails, which is why a throw after TLA settles is already fatal. Butload_entry_pointwaited on the TLA promise via the genericwait_for_promise, which only watches promise status and never consultsunhandled_error_counter, so the suspended module kept running until the promise settled naturally.Fix
Inline the wait loop in
load_entry_point's non-watcher arm and break onunhandled_error_counter > 0, matching the main drain loop. The genericwait_for_promiseis left unchanged since other callers (macros, HTMLRewriter, test expect, REPL) have different error semantics.Sibling TLA-wait sites left as-is and why:
load_entry_pointwatcher arm: watch mode survives errors by design; the outertick_possibly_forever()loop inRun::startkeeps ticking regardless ofunhandled_error_counter, so a guard here alone would be a no-op.load_preloadsnon-watcher arm (jsc_hooks.rs): same underlying gap for a--preloadmodule suspended in TLA, but stopping post-fatal execution there also requiresreload_entry_pointto bail before loading the main module andload_preloadsto skip remaining preloads, which is a broader change than this PR's scope. Left for a follow-up.load_entry_point_for_test_runner:bun testis designed to survive uncaught exceptions (records the failure, continues to the next file), so the guard does not belong there.Controls verified to still hold:
process.on('uncaughtException', ...): both survive, module completes, exit 0.Verification
Fixes #22546
[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file