hot/watch: keep driving timers and surviving errors after an unhandled error - #38206
hot/watch: keep driving timers and surviving errors after an unhandled error#38206robobun wants to merge 1 commit into
Conversation
Under --hot and --watch the process stays alive after an error nothing handled, but uncaught_exception() and unhandled_rejection() still counted it in unhandled_error_counter. That makes is_event_loop_alive() false for the rest of the process, so the watcher run loop falls back to tick_possibly_forever(), which polls sockets but never drains the timer heap: after a rejected entry point (or any later uncaught exception or unhandled rejection) no setTimeout/setInterval callback ever ran again, while Bun.serve kept answering. on_before_exit() on that path also armed exit_on_uncaught_exception, so the next uncaught exception exited the --hot process with code 1; a plain drain arms it too via beforeExit. Record the error as fatal only when no watcher is installed, and do not take the exit_on_uncaught_exception hard exit in watcher mode either. The error is still printed and exit_code is still set; the run loop just keeps going like it does for a generation that did not fail.
|
Warning Review limit reached
Next review available in: 30 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 (3)
Comment |
|
Status: reproduced on the released 1.4.0 build (linux x64) with |
| it("still fires timers after the entry point's promise rejected", async () => { | ||
| await using proc = startFixture("entry-rejects"); | ||
| const { get, reportedErrors } = await ready(proc); | ||
|
|
||
| expect(await get("/timer")).toBe("timer fired"); | ||
| expect(await get("/throw")).toBe("alive"); | ||
| expect(await get("/")).toBe("alive"); | ||
| expect(await get("/timer")).toBe("timer fired"); | ||
|
|
||
| expect(await reportedErrors()).toEqual([ | ||
| "error: rejected by the entry point", | ||
| "error: uncaught exception from /throw", | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🟡 The three new it() calls in the describe.each(["--hot", "--watch"]) block don't pass the file-local timeout (isDebug ? Infinity : 10_000) as their third argument, unlike every other it() in this file — so they fall back to the 5000ms default. Each spawns a debug bun --hot/--watch subprocess, waits for it to bind a server, does 3-5 sequential HTTP round-trips, then kills and awaits stderr, which can plausibly exceed 5s on debug+ASAN CI. Append , timeout to each of the three it(...) calls to match the file's convention.
Extended reasoning...
What the bug is
test/cli/hot/hot.test.ts defines const timeout = isDebug ? Infinity : 10_000; at the top of the file, and every existing it() in the file passes either timeout or longTimeout as its third argument. The file has no setDefaultTimeout call. The three new it() calls added inside describe.each(["--hot", "--watch"])("%s after an unhandled error", ...) — at roughly lines 826, 841, and 858 — omit this third argument, so all six test instances (three it() × two flags) fall back to Bun's default per-test timeout of 5000ms.
The specific code path
Each of the six new tests:
- Spawns a debug
bun --hotorbun --watchsubprocess againsthot-error-server-fixture.js. - Awaits the subprocess printing its port on stdout (i.e. waits for a debug bun to start, evaluate the entry point, and bind a
Bun.serveserver). - Performs 3-5 sequential
fetchround-trips against that server, one of which awaits a 10ms server-sidesetTimeout. - Kills the subprocess, awaits
proc.exited, and awaits the buffered stderr text.
On a debug+ASAN CI runner, step 2 alone (debug subprocess startup) can take multiple seconds — this is precisely why the file uses isDebug ? Infinity : 10_000 for every other subprocess-spawning test in the file. With the default 5000ms budget, the sum of debug+ASAN subprocess startup + server bind + several HTTP round-trips + kill/wait/stderr collection is plausibly over 5s under CI load.
Why existing code doesn't prevent it
There is no setDefaultTimeout call anywhere in hot.test.ts, so the only way a test in this file gets more than 5s is by passing the third argument explicitly. Every sibling test does so; these three do not. The block's own leading comment even calls out timeout semantics ("bun test only kills a timed-out test's leftover processes for sequential tests"), while the tests themselves rely on the default that comment is reasoning about.
Step-by-step proof
- File top:
const timeout = isDebug ? Infinity : 10_000;— nosetDefaultTimeoutin the file. - Every pre-existing
it()in the file ends with}, timeout);or}, longTimeout);. - New test at ~line 826:
it("still fires timers after the entry point's promise rejected", async () => { ... });— no third argument. Same for the two followingit()calls. - On a debug+ASAN Linux CI runner: debug
bun --hotsubprocess startup ≈ 2-4s under load; server bind + port print; 4 sequential fetches (one includes a 10ms server timer); kill + await exited + await stderr text. Total plausibly > 5000ms → test fails with a timeout, not with the assertion the test is meant to check. - In release (
!isDebug) the file convention is 10_000ms, so even the non-debug lane gets half the headroom every other test in the file gets.
Impact
Potential CI flake on the debug/ASAN lanes (and reduced headroom on release lanes). Not a runtime correctness issue — the fix under test is unaffected.
How to fix
Append , timeout as the third argument to each of the three new it() calls, matching every other it() in the file:
it("still fires timers after the entry point's promise rejected", async () => {
...
}, timeout);REVIEW.md: "Copy harness conventions exactly" and "Match the exact file's local conventions".
|
Updated 2:14 PM PT - Aug 13th, 2026
✅ @robobun, your commit 87d7637cdb25ff862a9c928cd1e1b0c10c3d4f6a passed in 🧪 To try this PR locally: bunx bun-pr 38206That installs a local version of the PR into your bun-38206 --bun |
Problem
bun --hotorbun --watch, once any error goes unhandled (the entry point's promise rejecting, an uncaught exception in a callback, an unhandled rejection), nosetTimeout/setIntervalcallback runs again for the rest of the process, while sockets keep being serviced: aBun.servestarted before the error still answers. Smallest form:setTimeout(() => process.exit(0), 10); await 0; throw new Error("x")never exits under--hot.--hotprocess with code 1. The exit also happens with no prior error when the loop drained once (beforeExitwas dispatched) and a later generation throws from a callback.!handledtail ofVirtualMachine::uncaught_exceptionand the tail ofVirtualMachine::unhandled_rejection(src/jsc/VirtualMachine.rs) dounhandled_error_counter += 1.is_event_loop_alive()is false while that counter is nonzero, so the watcher arm of the run loop inRun::start(src/runtime/cli/run_command.rs) never re-enters itswhile vm.is_event_loop_alive()body and only repeatson_before_exit()+tick_possibly_forever().tick_possibly_forever(src/jsc/event_loop.rs) polls sockets and runs tasks but never callstimer::All::get_timeout/drain_timers; on epoll/kqueue those only run fromauto_tick_active, which is inside the skipped body. (On Windows libuv drives the heap, so only the exit symptom reproduces there.)on_before_exit()with the counter nonzero setsexit_on_uncaught_exception, andProcess__dispatchOnBeforeExitsets it on every drain; with it set,uncaught_exceptioncallsprocess.exit(1)for the next unhandled throw.tick_possibly_forevernever drained JS timers, before or after uws: drop us_timer_t on epoll/kqueue in favor of bun's timer heap #33359.Fix
VirtualMachine::unhandled_errors_are_fatal()(!is_watcher_enabled()) and gate the two counter bumps and theexit_on_uncaught_exceptionhard exit on it. Printing andexit_code = 1are unchanged.process.exit()or a signal ends it), so recording exit state there is the bug. Once the state is not entered, the run loop,on_before_exit, andtick_possibly_forever(still used for the genuinely idle case) already do the right thing; the alternative of teaching the parked loop to also drive timers would add a second timer path and still leave the hard exit armed.beforeExitwhen it drains.bun testis unaffected (itsisBunTestbranches return before these lines), workers have no watcher, plainbun runis unchanged.reload(), which only takes effect once a fixing reload happens; with the state never entered, that scenario works as well (checked by hand: an interval kept ticking through a saved syntax error and after the fix was saved). runtime: make uncaught exceptions during top-level await immediately fatal #34627 stops the non-watcherload_entry_pointwait on the counter and leaves the watcher arm alone, which this change is consistent with.test/cli/hot/hot.test.ts,describe.each(["--hot", "--watch"]), three tests per flag againsttest/cli/hot/hot-error-server-fixture.js(no files are edited; the flag alone puts the run loop in watcher mode): timers after a rejected entry point; an uncaught exception, an unhandled rejection and a second uncaught exception at runtime, then a timer; an uncaught exception afterbeforeExitwas dispatched. Each also checks the exacterror:lines reported.USE_SYSTEM_BUN=1 bun test: 6 fail (entry-point ones time out waiting on the timer-backed response, the others getConnectionRefused/ECONNRESETbecause the fixture exited 1).bun bd test: 6 pass.hot.test.ts(error-reload tests included),test/cli/hot/watch.test.ts,test/cli/watch/watch.test.ts,test/cli/test/test-changed.test.ts(bun test --watch),test/cli/run/run-eval.test.ts,test/js/node/process/process.test.js(two failures there are container-specific:process.env.USERunset, and a worker test that takes 4.6s on this debug build with no errors involved).Background
unhandled_error_counter: VM field bumped when an error reaches the top with nouncaughtException/unhandledRejectionlistener.is_event_loop_alive()is false while it is nonzero; that is how plainbun runexits 1 after such an error, since its run loop iswhile vm.is_event_loop_alive() { tick(); auto_tick_active(); }.bun testbumps it on separate branches and uses it as a tally.exit_on_uncaught_exception: VM flag meaning "pastbeforeExit"; set byProcess__dispatchOnBeforeExitand byon_before_exit()when an error was counted. With it set, an unhandled throw callsprocess.exit(1)directly because outside watch mode there is no loop turn left to unwind through.--hotre-evaluates the entry point in-process on file changes,--watchre-execs the process. Both installvm.bun_watcher(is_watcher_enabled()) and share the watcher arm of the run loop, which never returns: drive the loop while something is alive, dispatchbeforeExitwhen it drains, park intick_possibly_forever()until the watcher thread wakes it.timer::All) and fire only fromauto_tick/auto_tick_active, which compute the poll timeout from the heap and drain it after the poll.tick_possibly_foreveris a fixed one-second park that does neither.Probes on the released build (1.4.0, linux x64)