hot: report a reloaded generation's error while a beforeExit listener keeps the loop alive

[robobun]

Problem

• Under bun --hot, a program using the keep-alive idiom process.on("beforeExit", () => setTimeout(...)) loses error reporting for reloads: when a saved generation throws at top level, nothing is printed, and every save after that one is ignored for the rest of the process (reproduced on 1.4.0 and main, probe below). The same saves work when the program is kept alive by a setInterval instead.
• With that idiom the process lives inside the drain loop of VirtualMachine::on_before_exit (src/jsc/VirtualMachine.rs): each beforeExit dispatch arms a timer, the timer drains, beforeExit is dispatched again, and the function never returns. The ticks in that loop do run the watcher's reload tasks, so the new generation is evaluated there.
• report_exception_in_hot_reloaded_module_if_needed() is what prints a reloaded generation's rejection (and runs a reload that reload() deferred). It was only called from the watcher arm of the run loop in Run::start (src/runtime/cli/run_command.rs), between that loop's own ticks and once after on_before_exit() returns. While on_before_exit() keeps draining, neither point is reached, so the rejection is never reported.
• reload() defers whenever the current generation rejected and has not been reported yet (pending_internal_promise_reported_at != hot_reload_counter, from hot: defer reload while a rejected module is unreported #29740), and the deferred reload is retried by the same reporting function. After one unreported rejection every later reload defers and nothing retries it.

Fix

• on_before_exit() now delegates to on_before_exit_with(after_tick), which calls after_tick after every tick of the drain loop. The --hot/--watch run loop passes report_exception_in_hot_reloaded_module_if_needed; on_before_exit() itself passes a no-op, so bun run without a watcher, bun test, the REPL and workers are unchanged.
• Why this is the right shape: the drain inside on_before_exit is a continuation of the caller's run loop (same tick(); auto_tick_active(); turn), and the run command already polls after each of its own ticks. The poll is passed in by the run command rather than keyed on the watcher inside on_before_exit because the state it reads (pending_internal_promise_reported_at, the deferred reload, re-watching the entry file) is only maintained by the run command: bun test --watch installs the same watcher and sets the same hot_reload field, but reports a rejected test file itself and leaves reported_at untouched, so the poll would report that file again if it ran there (today it would only be spared by bun test returning before its own on_before_exit() call for such a file).
• The beforeExit semantics are untouched: the dispatch, the re-dispatch-only-after-work rule and both unhandled_error_counter guards are the same code, with one extra call in the loop body. When the poll reports an error the counter becomes nonzero, so the drain stops and no further beforeExit is dispatched for the failed generation, which is what already happens when a generation fails outside the drain.
• Test: test/cli/hot/hot.test.ts, "should report a reloaded module's error while a beforeExit listener keeps re-arming the event loop". Generation 1 installs the idiom and the test waits for the second beforeExit line (only dispatched from inside the drain); generation 2 throws and the test expects error: boom; generation 3 must then load. Everything the generations print goes to stderr, where the error is reported too, so the beforeExit lines after gen2 double as the bound for the unfixed case: the test stops waiting once the drain has cycled 20 more times without a report, instead of running into the timeout.
  • Without the src/ change (release 1.4.0, and a debug build with src/ stashed): fails in under a second with the output gen1, beforeExit x2, gen2, beforeExit x20 and no error. With it: passes (five runs, 0.6 to 0.8s on the debug build; the error is reported before any further beforeExit line).
• Also run on the debug build: the rest of test/cli/hot/hot.test.ts, test/cli/hot/watch.test.ts, the process.onBeforeExit block of test/js/node/process/process.test.js, the node test-process-beforeexit*, test-beforeexit-event-exit, test-process-exit-from-before-exit, test-timers-unrefed-in-beforeexit and test-worker-beforeexit-throw-exit / test-worker-ref / test-worker-voluntarily-exit-* files, the -e and exit tests of test/js/bun/repl/repl.test.ts, and the process.on('exit') block of test/cli/test/bun-test.test.ts. cargo fmt --all -- --check is clean.
• Nearby open PRs, none of which add this poll: hot/watch: keep driving timers and surviving errors after an unhandled error #38206 and hot: reset unhandled-error state on reload so timers recover after a failed reload #34657 change what happens to unhandled_error_counter after an error is reported (with hot/watch: keep driving timers and surviving errors after an unhandled error #38206 the drain keeps going after the report, which the test allows); hot: replace a generation that is parked on a top-level await #38613 changes when reload() defers on a pending generation; worker_threads: stop the 'beforeExit' drain when the worker is terminating #34645 adds termination checks to the same drain loop for workers and will need a one-line rebase onto on_before_exit_with (or the other way round).

Background

• --hot keeps one process and one process object across reloads: on a file change the watcher thread posts a reload task to the event loop, and running it (VirtualMachine::reload) clears the module registry and re-evaluates the entry point. Listeners registered by an earlier generation, such as the beforeExit listener here, stay installed.
• pending_internal_promise is the promise for the entry point's evaluation in the current generation. The initial load waits for it and reports a rejection directly; a reload does not wait, which is why the run loop polls report_exception_in_hot_reloaded_module_if_needed() after every tick. The function prints the rejection once per generation, keyed on hot_reload_counter, then retries a deferred reload, then re-adds the entry file to the watcher.
• beforeExit: node emits it when the event loop drains; if a listener schedules more work, the loop runs again and beforeExit is emitted again when it drains. on_before_exit implements this as dispatch, drain, re-dispatch if the drain did any work. A listener that always schedules work therefore keeps the drain going indefinitely; under --hot this is the process's steady state.

Probe on the released build (1.4.0, linux x64)

$ printf 'console.log("gen1");\nprocess.on("beforeExit", () => setTimeout(() => {}, 20));\n' > app.ts
$ bun --hot --no-clear-screen app.ts &
$ sleep 1; printf 'console.log("gen2");\nthrow new Error("boom");\n' > app.ts
$ sleep 1; printf 'console.log("gen3 fine");\n' > app.ts; sleep 1
gen1
gen2
# nothing else; the process is still alive, and gen3 never loads.

# Control, same sequence with `setInterval(() => {}, 1e6)` in place of the listener:
gen1
gen2
1 | console.log("gen2");
2 | throw new Error("boom");
              ^
error: boom
      at /tmp/hotrepro/app.ts:2:11
gen3 fine

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 23 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c4d5b36-045d-421d-8225-0bd849e87c1a

📥 Commits

Reviewing files that changed from the base of the PR and between b9ef885 and f998d18.

📒 Files selected for processing (3)

• src/jsc/VirtualMachine.rs
• src/runtime/cli/run_command.rs
• test/cli/hot/hot.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status

• Reproduced on the released 1.4.0 and on a debug build of main: bun --hot with process.on("beforeExit", () => setTimeout(() => {}, 20)), then saving a generation that throws prints nothing, and a further save never loads. The new test in test/cli/hot/hot.test.ts fails the same way without the src/ change (output gen1, beforeExit x2, gen2, beforeExit x20, no error) and passes with it.
• Fix is in this PR; see the description for the reasoning and the nearby PRs (hot/watch: keep driving timers and surviving errors after an unhandled error #38206, hot: reset unhandled-error state on reload so timers recover after a failed reload #34657, hot: replace a generation that is parked on a top-level await #38613, worker_threads: stop the 'beforeExit' drain when the worker is terminating #34645).

[claude]

I reviewed this PR and didn't find any bugs. The Rust change is minimal and the no-op delegation keeps every other on_before_exit() caller bit-identical, but since it threads a hook into the VM's beforeExit drain — core process-lifecycle code that also runs for workers, bun test, and the REPL — a human look would still be worthwhile.

What was reviewed

• Confirmed on_before_exit() → on_before_exit_with(|_| {}) is behavior-preserving for the four other call sites (worker, test_command, repl, non-watcher run).
• Verified the after_tick placement (between tick() and auto_tick_active()) exactly mirrors the outer watcher run loop at run_command.rs:1476-1478.
• Checked report_exception_in_hot_reloaded_module_if_needed is idempotent per generation via pending_internal_promise_reported_at, so the extra post-return call is safe.
• Test: event-driven waits, exit wired to reject, bounded fallback (20 beforeExit cycles) instead of timeout-based failure, resave interval cleared in finally.

Extended reasoning...

Overview

The PR fixes a bun --hot bug where a program kept alive by a self-re-arming beforeExit listener never surfaces a reloaded generation's top-level error, and then silently ignores every subsequent save. Root cause: the process lives permanently inside VirtualMachine::on_before_exit's drain loop, whose ticks do run reload tasks but which never returns to the caller's per-tick report_exception_in_hot_reloaded_module_if_needed() poll. The fix parameterizes the drain with an after_tick: impl FnMut(&mut Self) hook; on_before_exit() delegates with a no-op so all other callers are unchanged, and the --hot watcher arm passes the reporting function.

Files: 2 lines of real logic in src/jsc/VirtualMachine.rs (delegation + one after_tick(self) call, plus a doc comment), a 3-line swap in src/runtime/cli/run_command.rs, and a ~90-line test in test/cli/hot/hot.test.ts.

Security risks

None. No untrusted input handling; this is internal event-loop control flow for --hot mode.

Level of scrutiny

Medium-high. The diff itself is tiny and mechanical, and the no-op-closure delegation makes it easy to see that non-hot callers (workers, bun test, REPL, plain bun run) execute identical instructions to before. However, on_before_exit is core process-lifecycle code that gates beforeExit re-dispatch on unhandled_error_counter and interacts with worker termination and Node exit semantics. Introducing a caller-supplied hook mid-drain is a design decision (vs. e.g. checking self.hot_reload inline) that a maintainer should ratify — the PR description argues convincingly for the callback shape (avoids double-reporting under bun test --watch), but that's exactly the kind of layering call REVIEW.md flags for maintainer agreement.

Other factors

• The after_tick call sits between tick() and auto_tick_active(), matching byte-for-byte what the outer watcher run loop already does per tick (run_command.rs:1476-1478), so the drain is now a faithful continuation of that loop.
• report_exception_in_hot_reloaded_module_if_needed is idempotent per generation (pending_internal_promise_reported_at guard), so the retained post-on_before_exit_with call and any repeated invocation inside the drain are harmless.
• After the hook reports, unhandled_error_counter > 0 suppresses the next beforeExit re-dispatch and the drain returns — same outcome as when a generation fails outside the drain today.
• The test is well-constructed by the repo's standards: awaits observable conditions (second beforeExit line proves we're inside the drain), wires process exit to reject, uses a bounded 20-cycle fallback so the unfixed case fails fast with diagnostic output rather than timing out, and cleans up the resave interval in finally. The await using runner handles process teardown.
• PR description documents USE_SYSTEM_BUN-equivalent verification (fails on 1.4.0 and stashed-src debug build; passes with the change) and lists the adjacent test suites re-run.

Deferring rather than approving because on_before_exit is load-bearing lifecycle machinery and the callback-parameter shape, while well-argued, is a small design decision on a shared function that a maintainer should sign off on.

[robobun]

On the shape question raised in the review (hook parameter vs. deciding inside on_before_exit): the two inline predicates on_before_exit could use, is_watcher_enabled() and vm.hot_reload, are also true under bun test --watch / --hot (test_command.rs installs the same watcher and sets the same field), and the test runner reports a rejected test file itself without touching pending_internal_promise_reported_at, so the promise stays Rejected and unmarked. If the poll ran in on_before_exit under that predicate it would report such a file again; today it would not get the chance only because bun test returns before its on_before_exit() call for a file that rejected, which is ordering the poll should not depend on. The state the poll is keyed on (reported_at, the deferred reload, re-watching the entry file) is maintained by the run command alone, so the run command passes the poll in and the other callers keep the unchanged on_before_exit().