Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughWatcher initialization and startup failures now use shared error reporting and exit with status 1. Unix EMFILE and ENFILE errors can include platform-specific limit advice. Linux tests inject EMFILE across four watch-mode commands. ChangesWatcher error handling
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Watcher initialization failures now produce a clear error with a limit hint and exit code 1 instead of a crash. No merge-blocking risk was identified from the supplied evidence. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Reworked in 3742fa9. CI is green on that commit (Buildkite build 122806). Reproduced on an unfixed release build (Linux x64) two ways:
With this branch the same run prints: and exits with code 1. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces the Output::panic on Watcher::init failure with a proper CLI error + exit(1), matching what the start() arm already did.
What was reviewed:
exit_on_watcher_erroris-> !, so thematcharm typechecks; the droppedhandle_error_return_traceis a no-op shim inbun_core.Error::limit_hint:#[cfg(not(unix))]returnsNone(Windows variant never reaches the unixmatch);ErrorisCopyso by-valueselfis fine.- Test refactor: the extracted
compileShim/runWatcheeWithShimhelpers preserve the existingstart()test's assertions; new block covers all four watch entry points concurrently and assertssignalCode === null+exitCode === 1.
Extended reasoning...
Overview
This PR changes one error-handling arm in enable_hot_module_reloading (src/jsc/hot_reloader.rs) from Output::panic to a printed error: line plus Global::exit(1), and factors both the init and start failure arms into a shared #[cold] fn exit_on_watcher_error(action, err) -> !. It adds Error::limit_hint() in src/watcher/error.rs to name the exhausted resource for EMFILE/ENFILE per-platform, and adds a Linux-only LD_PRELOAD fault-injection test block in test/cli/watch/watch.test.ts that covers --watch, --hot, bun test --watch, and bun build --watch. The existing start()-failure test is refactored to share the shim compile/spawn helpers with the new block; its assertions are unchanged.
Security risks
None. This is CLI error-reporting on a startup-time failure path. No user input is parsed, no new syscalls are made, and the process exits immediately after printing. The hint strings are static &'static str constants.
Level of scrutiny
Low-to-medium. The functional change is a one-arm swap of panic → error + exit(1), which strictly improves behavior (a user-reachable environment limit was going through the crash reporter). The sibling start() arm already did exactly this; the PR unifies them. limit_hint is pure and cfg-gated correctly for Linux/Android vs other unix vs Windows. The removed bun_core::handle_error_return_trace(&err) call is documented as a no-op tier-0 shim, so nothing is lost.
Other factors
- Tests are strong for this class of change: the
LD_PRELOADshim forcesinotify_init1to returnEMFILE, and the block asserts the exact error text, the presence of thenote:line namingfs.inotify.max_user_instances, that the script body never ran,signalCode === null(proving no abort), andexitCode === 1. It runs concurrently across all four watch entry points including theBundleV2path. - The PR description confirms cross-target
cargo checkfor macOS/Windows/FreeBSD, clippy/fmt clean, and that the tests fail on the release build. - The leaked
reloaderbox on the exit path is called out in the description and is harmless (process exits in the same frame; LSAN is silent because the pointer is still stack-live). This matches how the pre-existingstart()arm already behaved. - No prior human review comments to address; no CODEOWNERS on these paths that I can see.
When Watcher::init fails (inotify_init1 or kqueue returns EMFILE or ENFILE), --watch, --hot, bun test --watch and bun build --watch went through Output::panic. The user got a crash report for a descriptor limit in their environment. Print the errno as a normal CLI error, add a note that names the limit, and exit with code 1. The start() failure path below it already did this. Both paths now share one helper.
d3f29c9 to
a4d107a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased on main (bf42a52, force pushed). The old head was d3f29c9, the new head is a4d107a. Two conflicts, both in
No source line of the diff changed. The test lines that this PR adds and removes are also the same as before. Checked on Linux x64 with a debug (ASAN) build:
The build with the change also had the other branches that I rebased in the same pass. CI is the check for this branch alone. |
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 finding, I also checked whether dropping the two bun_core::handle_error_return_trace(&err) calls loses any output — it is a no-op (pub fn handle_error_return_trace<E>(_err: E) {} in src/bun_core/lib.rs), so the shared exit_on_watcher_error path prints exactly what the old start() arm did. Passing err by value is fine since bun_watcher::Error derives Copy.
Extended reasoning...
The change reroutes the Watcher::init failure in src/jsc/hot_reloader.rs from Output::panic to a shared cold exit helper that prints an error plus an optional EMFILE/ENFILE note from the new bun_watcher::Error::limit_hint, and adds a Linux-only LD_PRELOAD test matrix. It touches no security-sensitive surface. The one confirmed finding (a vacuous stdout assertion for the bun build --watch row) is posted inline; the trace-call removal and Copy-by-value concerns were ruled out by reading the helper and the enum derive.
The note for a failed watcher init now lives in hot_reloader.rs and prints in one case: Watcher::init on Linux or Android fails with EMFILE. inotify_init1 returns EMFILE both when the user is out of inotify instances and when the process is out of file descriptors. bun_watcher::Error::limit_hint is removed, so src/watcher/error.rs is the same as on main. Generic descriptor advice stays with handle_root_error. ENFILE, ENOMEM, the kqueue errors and the Windows errors print the error line only. The helper takes the failed step as an enum, so the start() arm cannot print the note. The test covers eight command forms plus ENFILE and ENOMEM, compares the whole output, and checks that no crash report is sent.
There was a problem hiding this comment.
LGTM — small, self-contained change that turns a watcher-init failure into a plain error: + exit 1 instead of a crash report, and the follow-up commit addresses the earlier vacuous-stdout point (every row now asserts stdout: "" exactly via one toEqual).
What was reviewed:
exit_on_watcher_errorin src/jsc/hot_reloader.rs:bun_watcher::ErrorderivesPartialEq/Eqso the==onSys(EMFILE)compiles;bun_core::note!exists and prefixesnote:as the test expects;bun_errnois already a dependency of thebun_jsccrate; the droppedhandle_error_return_traceis a no-op insrc/bun_core/lib.rs:1109, so nothing observable is lost.- The
Startarm's output and exit code are unchanged from before; all five watch entry points (run_command.rs,test_command.rs,bundle_v2.rs) route through the modifiedInitarm, so the matrix rows cover real paths. - Test harness use: leading
--debug-crash-handler-use-trace-stringbeforetest/buildis skipped bycommand::whichand unrecognized-flag warnings are off by default, so it does not pollute the exact-stderr assertion;tempDirdisposal inafterAll,port: 0, and concurrent pipe draining are all in place. I did not execute the Linux-only shim tests locally (no debug build here), so CI remains the proof for those rows.
Extended reasoning...
The PR touches one Rust function pair in src/jsc/hot_reloader.rs (replacing an Output::panic and a duplicated exit path with one cold exit_on_watcher_error helper, plus a Linux/Android-only inotify EMFILE note) and extends test/cli/watch/watch.test.ts with shared LD_PRELOAD shim helpers and a 10-row it.concurrent.each matrix asserting exact stdout/stderr, zero crash-report uploads, null signal and exit 1. It touches no security-sensitive surface (no auth, parsing of untrusted data, or memory-unsafe code added). The diff is about 35 lines of production code with intent clearly stated, the behavior change (exit 1 instead of SIGABRT) is deliberate and acknowledged in the description, no CODEOWNERS entry covers either file, the bug hunt ran dry, and the only prior inline finding from this bot was fixed by the second commit. The remaining judgment call the author flags (exiting 1 for KQueueError/Windows init errors as well) mirrors what the start() arm already did and is documented in the PR.
This branch has not been deployed
2 participants
Problem
--watch,--hot,bun test --watch,bun build --watch) aborts withpanic: Failed to enable File Watcher: EMFILEwhenWatcher::initfails. An environment limit produces a crash report (Sentry BUN-402B) and SIGABRT.Errarm inenable_hot_module_reloading(src/jsc/hot_reloader.rs:728on main) callsOutput::panic. Thestart()arm below already exits 1.Fix
exit_on_watcher_error. It printserror: Failed to enable File Watcher: <name>and exits 1.note:line, only forEMFILEfromWatcher::initon Linux and Android. It namesfs.inotify.max_user_instancesandulimit -n, becauseinotify_init1returnsEMFILEfor both.test/cli/watch/watch.test.ts, blockwatcher init failure. 10/10 rows fail on the unfixed build and pass here.Related to #15329 and #19051. Carries #34070 forward.
Background
Watcher::initcreates the kernel object (inotify_init1,kqueue). ItsErrarm is the one frame all five call sites cross.bun test --watchandbun build --watchblock on the watch flag alone, so the arm must not return.handle_root_error(it cannot know which syscall failed), an advice table inbun_watcher(a second owner of descriptor advice), aResultto the callers (Run::startnever returns).Downsides
EMFILE, andENFILEandENOMEMeverywhere, print no remedy. That advice stays withhandle_root_error(usockets: report an out-of-descriptors event loop init as an error, not a crash #39641, Suggest bounded fd limits in the fd-exhaustion error hints #37339).KQueueErrorand the two Windows kinds, which are not environment limits..textis 256 B smaller and.rodatais unchanged (Notes).Notes
Output with the change (Linux,
inotify_init1returnsEMFILE):Measurements
Two release builds of the same tree, linux-x64. A has main's
src/jsc/hot_reloader.rs(bf42a52). B is this PR (3742fa9).nm+size, A vs B):.text58,159,221 -> 58,158,965 B (-256 B),.rodata19,804,908 B in both.enable_hot_module_reloadingx3: 1306/1306/1374 B -> 1020/1020/1097 B.exit_on_watcher_error: 369 B. The stripped binary is 80,844,360 B in both. The note text is 370 B in B (stored plain and with color codes).nm+llvm-objdump, A vs B):Run::start3282 B / 745 instructions in both builds. Mnemonic diff overRun::start,RunCommand::boot,TestCommand::exec,BundleV2::init: 0 lines.inotify_init1to exit,bun --watchunder theEMFILEshim (gdbcatch syscall): A 182 syscalls (write 89, getpid 30, process_vm_readv 29, clone 1, tgkill 1, and others) -> B 2 syscalls (write 1, exit_group 1, clone 0, prlimit64 0, fcntl 0).USE_SYSTEM_BUN=1(release build 367d939) and with build A. 10/10 pass withbun bd testand with build B.src/crash_handler(git grep -E 'ulimit -n|out of file descriptors|max_user_instances'): main 0, the first shape of this PR 4 (431 B), this PR 1 (179 B, Linux and Android only).bun run rust:check-all: 12/12 targets compile. That run was on this code before a one-line comment edit. The kqueue and Windows arms are type-checked only. No test injects a failure there.Self-review findings and what this PR does about them
Generic advice.
handle_root_errorholds theulimit -nadvice, keyed on error names. It does not accept errno names on main, it cannot tell an inotify limit from a descriptor limit, and usockets: report an out-of-descriptors event loop init as an error, not a crash #39641 and Suggest bounded fd limits in the fd-exhaustion error hints #37339 are changing it. So this PR adds no generic descriptor text. Once those land, the helper can passEMFILEandENFILEfrom kqueue to it.Scope. This PR changes the watcher init arm and keeps the
start()arm's behavior. Other init-time sites still panic on an OS resource errno (event loop init in usockets: report an out-of-descriptors event loop init as an error, not a crash #39641, thread spawn sites in Report a refused HTTP, bundle, IO or process waiter thread as an error instead of a crash #39864). They do not pass through a watcher frame and are not touched here.ENOMEM. This PR exits 1 for everyWatcher::initerror,ENOMEMincluded, the same as thestart()arm has done for every error kind since node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660. usockets: report an out-of-descriptors event loop init as an error, not a crash #39641 keeps the panic for errnos other thanEMFILEandENFILEat event loop init. The two sites then differ forENOMEM. A maintainer can pick one rule.Open PRs in the same area: watcher: add a stat-polling backend for --watch and --hot #42750 (stat-polling backend, edits
hot_reloader.rsandwatch.test.ts), Report a refused HTTP, bundle, IO or process waiter thread as an error instead of a crash #39864 (editswatch.test.ts), watcher: warn once when a watch cannot be added #41489 and watcher: wake the watcher thread on shutdown so a stopped dev server releases it #36253 (src/watcher/), bun build: install a file watcher for --no-bundle --watch #35633 (a new caller ofenable_hot_module_reloading, covered with no edit), Suggest bounded fd limits in the fd-exhaustion error hints #37339 and fix: don't guess file descriptor exhaustion for root errors #40627 (handle_root_erroradvice text). None of them changes this arm. If watcher: add a stat-polling backend for --watch and --hot #42750 lands, the note can also nameBUN_WATCHER_USE_POLLING=1.Test details
LD_PRELOADshim makesinotify_init1fail withEMFILE,ENFILEorENOMEM. Rows:bun --watch FILE,bun --hot FILE,bun run --watch FILE,bun --watch -e,BUN_OPTIONS=--watch,bun test --watch,bun test --hot,bun build --watch, plusENFILEandENOMEMonbun --watch FILE.toEqual. Each child getsBUN_CRASH_REPORT_URLat a local server, so the unfixed build uploads to that server and not to bun.report.curlthat keeps the stderr pipe open (src/crash_handler/lib.rs,report()), so the count is final when stderr reaches EOF.bun build --compilewrites an 823 MB file, and the setup hook passed the 5 s default timeout in a whole-file run.start()test shares the shim helpers. Its assertions did not change.test/no-validate-leaksan.txt).Other facts
(ulimit -n 7; bun --watch app.js). The number depends on how many descriptors are open before the watcher starts.node --watch app.js) runs the script once, printsWaiting for file changes before restarting..., and exits 0 with nothing on stderr. This PR does not copy that: the flag was given explicitly, and two commands would block with nothing watched.src/runtime/bake/DevServer.rs) callsWatcher::inititself and throws a JS error. It is not affected.ENOSPCfrominotify_add_watch(fs.inotify.max_user_watches) does not reach this arm. It comes back fromadd_file, and callers such asRuntimeTranspilerStoreignore theErr(watcher: warn once when a watch cannot be added #41489 is about that path).bun teston large projects #19051 stay open. Neither report's descriptor exhaustion was reproduced. In Bun crashes when runningbun teston large projects #19051 it may be Bun's own open directories.ulimit -n.bun_watcher::Error::limit_hint()with four strings forEMFILEandENFILEper OS. The second commit removes it.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/watch/watch.test.ts