watcher: stop serving the watchlist's stored fd to --hot reload reads - #37050
Conversation
bun --hot re-transpiled a changed module through a file descriptor snapshotted from the watcher's watchlist. The watcher thread's flush_evictions closes that descriptor concurrently (a directory event for an edited file evicts its entry), so the reload read failed with EBADF, or EISDIR once a resolver openat() recycled the number, and the reload aborted. A stored descriptor could also point at the pre-rename inode after an atomic save and return stale contents. Reloads now always open the file by path. The watchlist keeps sole ownership of its stored descriptor: add_file never replaces a valid stored fd (it only upgrades fd-less entries added by path) and reports whether it adopted the caller's descriptor, so callers close what the watcher did not take. This also fixes the entrypoint leaking one descriptor per reload: the already-watched branch used to overwrite the stored fd without closing the one it replaced.
WalkthroughChangesThe watcher now returns descriptor ownership. Hot-reload and transpilation paths reopen files by path and close descriptors unless ownership transfers to the watcher. A Linux regression test checks descriptor counts across repeated reloads. File descriptor ownership
Possibly related PRs
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/watcher/Watcher.rs (1)
870-902: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
Callerwhen Windows skips registration.On Windows,
append_file_assume_capacityreturnsOk(())without appending a file when the path is outsidetop_level_dir. Line 902 converts this no-op intoFdOwnership::Watcher.
src/runtime/jsc_hooks.rsandsrc/jsc/RuntimeTranspilerStore.rsthen disable their close guards. This leaks the input descriptor for an absolute import outside the project directory.Check path eligibility before appending, or return an insertion result from
append_file_maybe_lock. ReturnFdOwnership::Callerwhen the watchlist did not adoptfd. UpdateFdOwnership::Callerdocumentation because non-adoption is not limited to an existing valid descriptor.Proposed fix
if let Some(index) = self.index_of(hash) { // existing ownership handling } + #[cfg(windows)] + if bun_paths::resolve_path::is_parent_or_equal(self.top_level_dir(), file_path) + == bun_paths::resolve_path::ParentEqual::Unrelated + { + self.mutex.unlock(); + return Ok(FdOwnership::Caller); + } + let r = self.append_file_maybe_lock::<CLONE_FILE_PATH, false>(Also applies to: 1064-1073
🤖 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 `@src/watcher/Watcher.rs` around lines 870 - 902, The append path currently always maps a no-op Windows registration to FdOwnership::Watcher, leaking descriptors that the watchlist did not adopt. Update append_file_maybe_lock and the caller around its map(|()| ...) conversion to report whether fd was actually inserted, returning FdOwnership::Caller when the path is ineligible or otherwise not adopted; apply the same behavior to the corresponding flow around the additional reported location. Revise FdOwnership::Caller documentation to describe general non-adoption, not only an existing valid descriptor.
🤖 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.
Outside diff comments:
In `@src/watcher/Watcher.rs`:
- Around line 870-902: The append path currently always maps a no-op Windows
registration to FdOwnership::Watcher, leaking descriptors that the watchlist did
not adopt. Update append_file_maybe_lock and the caller around its map(|()| ...)
conversion to report whether fd was actually inserted, returning
FdOwnership::Caller when the path is ineligible or otherwise not adopted; apply
the same behavior to the corresponding flow around the additional reported
location. Revise FdOwnership::Caller documentation to describe general
non-adoption, not only an existing valid descriptor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2d22e03-c682-462a-b0bb-ca8a7df4843f
📒 Files selected for processing (8)
src/bundler/bundle_v2.rssrc/jsc/AsyncModule.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/hot_reloader.rssrc/runtime/jsc_hooks.rssrc/watcher/Watcher.rssrc/watcher/lib.rstest/cli/hot/watch-many-dirs.test.ts
…e-add The pending-imports path carried parse_result.input_fd into the queued AsyncModule, but the transpile frame's fd guard closes that descriptor when the watcher does not adopt it, so the deferred re-add could store a closed (possibly recycled) number in the watchlist. The original parse already registers the file before the enqueue, so the re-add was redundant; delete it along with the AsyncModule fields that only served it. Also return FdOwnership::Caller when append_file_assume_capacity skips a path outside the project root on Windows, so callers close the descriptor the watchlist never stored.
|
Addressed in 123d163: The same commit also removes the deferred watcher re-add in |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/AsyncModule.rs:1337-1352—resume_loading_modulenow closesfd_(=parse_result.input_fd) whenadd_filereturnsCaller, but that descriptor's ownership was already settled bymaybe_watch_fileintranspile_source_code_innerbefore the AsyncModule was enqueued — so this either closes the watchlist's own stored fd (dropping the kqueue watch and setting up a later double-close inflush_evictions) or double-closes a stale fd that_fd_guardalready closed onreturn Err(AsyncModule). The pre-PR code didlet _ = watcher.add_file(...)here without closing; this site should not closefd_at all (drop thebun_sys::close(fd_), or passFd::INVALIDtoadd_file).Extended reasoning...
What the bug is
The new close-on-
FdOwnership::Callerbranch inAsyncModule::resume_loading_modulecloses a file descriptor whose ownership was already transferred (or already closed) during the original parse intranspile_source_code_inner. Depending on which path the original parse took, this is either a close of the watchlist's owned descriptor or a double-close of a stale/possibly-recycled fd number — exactly the EBADF/EISDIR-on-recycled-fd class this PR is meant to eliminate.The code path
transpile_source_code_inner(jsc_hooks.rs) parses the file.transpiler.rs:1456-1458writes the sameentry.fdto bothparse_result.input_fdand*file_fd_ptr(the caller'sinput_file_fdlocal).- At jsc_hooks.rs:2639,
maybe_watch_file(..., &mut should_close_input_file_fd, input_file_fd, ...)runs and callswatcher.add_file(input_file_fd, ...). Under this PR, it setsshould_close_input_file_fd = falseonly if the watcher adopted the fd (Ok(FdOwnership::Watcher)). - At jsc_hooks.rs:2922,
parse_result.pending_imports.len() > 0is detected;parse_result(carryinginput_fd = Some(X)) is moved intoQueue::enqueue, and the function returnsErr(Error::AsyncModule)at line 2968. - On that return,
_fd_guard(jsc_hooks.rs:2385) fires: ifshould_close_input_file_fdis stilltrue, it closesinput_file_fd(= X). - Later,
resume_loading_moduleruns on the enqueued module.input_fdisSome(X). It callswatcher.add_file(X, ...)— the file is already watched, soadd_filereturnsOk(FdOwnership::Caller)(on macOSATOMIC_FILE_WATCHER = falseso it always returnsCalleron the already-watched branch; on Linux it returnsCallerbecause the stored fd is valid). The new code then callsbun_sys::close(X).
Why nothing prevents it
The two disjoint scenarios:
A.
maybe_watch_fileadopted X (first transpile of this file under--hot/--watch):should_close = false, so_fd_guardskipped the close. The watchlist now owns X — its stored fd is X.resume_loading_module'sadd_file(X)sees the file already watched with a valid stored fd →Ok(Caller)→close(X). This closes the descriptor the watchlist owns. On macOS/FreeBSD closing the kqueue-registered fd drops the vnode watch for this file; on all platformsflush_evictionswill laterclose(X)again on the (now stale, possibly recycled) number.B.
maybe_watch_filedid not adopt (file was already watched from a prior reload →Caller):should_closestaystrue, so_fd_guardclosed X whenErr(AsyncModule)unwound.parse_result.input_fdstill holds the closed number X.resume_loading_module→add_file(X)→Caller→close(X): a double-close of a possibly-recycled fd.The guards around the AsyncModule
add_filecall (is_watcher_enabled,is_absolute,!contains(node_modules)) are the same asmaybe_watch_file's guards, so wheneverresume_loading_modulereaches itsadd_file,maybe_watch_filealready ran on the same fd during the original parse.Impact
Reachable under
--hot/--watchwhen a module has an auto-install pending import (unresolved bare specifier with the package manager enabled). Narrow, but the failure mode is precisely what this PR fixes elsewhere: closing an fd the watchlist owns (silent watch loss on kqueue) and/or a double-close that can hit an unrelated recycled descriptor (EBADF/EISDIR). It's a regression: the pre-PR code didlet _ = watcher.add_file(...)with no close at this site.Step-by-step proof (scenario A, macOS)
bun --hot app.jswhereapp.jscontainsimport 'not-yet-installed-pkg'(auto-install on).- Parse opens
app.jsas fd 12;parse_result.input_fd = Some(12),input_file_fd = 12. maybe_watch_file: not yet watched →append_file_maybe_lockstores fd 12 in the watchlist and registers it with kqueue → returnsOk(Watcher)→should_close = false.- Pending import found → AsyncModule enqueued with
input_fd = Some(12)→return Err(AsyncModule)→_fd_guardseesshould_close == false, skips. - Auto-install completes;
resume_loading_moduleruns.fd_ = 12.add_file(12, ...):index_of(hash)finds the entry;ATOMIC_FILE_WATCHER = falseon macOS → returnsOk(Caller). New branch callsbun_sys::close(12). - fd 12 was the watchlist's stored, kqueue-registered descriptor. The kernel drops the EVFILT_VNODE registration; edits to
app.jsno longer trigger reloads. When the entry is later evicted,flush_evictionscallsclose(12)again — if 12 has been recycled by a resolveropenat, that's a wrong-fd close.
Fix
Ownership of
parse_result.input_fdwas already settled intranspile_source_code_innerbefore the AsyncModule was enqueued, soresume_loading_modulemust not close it. Either revert this hunk tolet _ = watcher.add_file::<true>(fd_, ...)(matching pre-PR behavior — the redundantadd_fileis harmless and updatespackage_json), or passFd::INVALIDin place offd_since the fd is not this call site's to give.
|
Right, and this was caught and fixed in 123d163 (the review ran against the first commit): |
There was a problem hiding this comment.
Beyond the inline nit, I checked a few cross-platform concerns and ruled them out: the removed #[cfg(target_os = "macos")] gate on the auto-watch close in jsc_hooks.rs is safe because non-kqueue platforms pass Fd::INVALID there (the is_valid() guard covers it); the add_file never-replace-valid-fd change is gated on ATOMIC_FILE_WATCHER (Linux-only), so kqueue's watch-descriptor semantics are unaffected; and the new close in bundle_v2.rs's plugin-watch path targets a locally-opened fd, not a borrowed one.
Extended reasoning...
This PR reworks fd ownership between the watcher and the transpile paths — add_file now returns FdOwnership, the watchlist's stored fd is never handed out for reads, and the AsyncModule deferred re-add is removed along with its supporting fields. The one inline finding is a dead-code nit (ParseResult::input_fd lost its last reader). I additionally verified that the platform-gate removal on the auto-watch close and the add_file already-watched branch change do not alter macOS/FreeBSD kqueue semantics, and that the newly-added close in the bundler plugin path targets a descriptor opened in the same block. The change is substantive (cross-thread fd ownership across 6 native files, with acknowledged overlap with open #36675), so it warrants human review; this note just records what was already checked.
|
@robobun submit a follow-up PR |
|
Follow-up PR: #37071, removes the write-only |
Follow-up to #37050, requested in #37050 (comment). ## What #37050 removed the deferred watcher re-add in `AsyncModule::resume_loading_module`, which was the only reader of `ParseResult::input_fd`. That left the field write-only: assigned at every construction site and threaded through `parse_maybe`, `ParseResult::empty_with`, and the four cold loader helpers (`parse_data_loader`, `parse_text_loader`, `parse_md_loader`, `parse_wasm_loader`) without ever being read, as review on that PR pointed out. This deletes the field, the `parse_maybe` local that fed it, and the pass-through parameters. The fd the parse opened still reaches the watcher handoff sites through `ParseOptions::file_fd_ptr`, which was already written from the same `entry.fd` on the same line. The comment in `AsyncModule.rs` that named the deleted field is reworded. No behavior change, dead-code removal only, so there is no fail-before test to write. While auditing the touched loader paths, the runtime `.md` module loader turned out to have no coverage at all (the existing md tests only exercise the `Bun.markdown` API), so this adds tests locking it in: `import`/`require` of a `.md` file yields the rendered HTML as the default export, and an empty `.md` file produces a module with no default export (the `empty_with` path). ## Verification - `cargo check -p bun_bundler` and a full debug build pass. - On the debug build: `test/js/bun/md/md-edge-cases.test.ts` (84 pass, including the new import tests), `test/cli/hot/watch-many-dirs.test.ts` (2 pass, including the fd-count test added in #37050), `test/cli/hot/hot.test.ts` (12 pass), `test/js/bun/resolve/toml` + `yaml` + `jsonc` (21 pass), `test/bundler/bundler_loader.test.ts` (45 pass).
Follow-up to #37050, requested in oven-sh/bun#37050 (comment). ## What #37050 removed the deferred watcher re-add in `AsyncModule::resume_loading_module`, which was the only reader of `ParseResult::input_fd`. That left the field write-only: assigned at every construction site and threaded through `parse_maybe`, `ParseResult::empty_with`, and the four cold loader helpers (`parse_data_loader`, `parse_text_loader`, `parse_md_loader`, `parse_wasm_loader`) without ever being read, as review on that PR pointed out. This deletes the field, the `parse_maybe` local that fed it, and the pass-through parameters. The fd the parse opened still reaches the watcher handoff sites through `ParseOptions::file_fd_ptr`, which was already written from the same `entry.fd` on the same line. The comment in `AsyncModule.rs` that named the deleted field is reworded. No behavior change, dead-code removal only, so there is no fail-before test to write. While auditing the touched loader paths, the runtime `.md` module loader turned out to have no coverage at all (the existing md tests only exercise the `Bun.markdown` API), so this adds tests locking it in: `import`/`require` of a `.md` file yields the rendered HTML as the default export, and an empty `.md` file produces a module with no default export (the `empty_with` path). ## Verification - `cargo check -p bun_bundler` and a full debug build pass. - On the debug build: `test/js/bun/md/md-edge-cases.test.ts` (84 pass, including the new import tests), `test/cli/hot/watch-many-dirs.test.ts` (2 pass, including the fd-count test added in #37050), `test/cli/hot/hot.test.ts` (12 pass), `test/js/bun/resolve/toml` + `yaml` + `jsonc` (21 pass), `test/bundler/bundler_loader.test.ts` (45 pass).
2 participants
What
Fixes
test/cli/hot/watch-many-dirs.test.ts("handles 129 directories being updated simultaneously"), which has been failing on main and across PR CI runs on Linux with:followed by the 30s test timeout (the failed reload never prints the expected output). Seen red on main builds 86133/86209/86347 and in many PR runs, e.g. build 89567. The race is timing-sensitive: it reproduced once in 80 local runs of an unmodified release build (with the exact EBADF + EISDIR + timeout signature), while loaded CI machines hit it regularly.
Cause
bun --hotre-transpiled a changed module by reading through a file descriptor snapshotted from the watcher's watchlist (ImportWatcher::snapshot_fd_and_package_json). The snapshot copies the fd number under the watcher mutex, but the read intranspiler.rs:read_file_with_allocatorhappens after the mutex is released.Concurrently, the watcher thread's
flush_evictionscloses stored descriptors under that same mutex. On Linux, a write to a file inside a watched directory produces both a file event and a directory event; the directory-event arm inhot_reloader.rsevicts the watched file's entry (to handle atomic saves that replace the inode). With 129 directories the inotify events span multiple batches, so a reload triggered by batch N snapshots a descriptor that batch N+1's eviction then closes:EBADF reading "<path>"openat(O_DIRECTORY)calls:EISDIR reading "<path>"The mutex ordering added previously (
flush_evictionsbeforeenqueue, snapshot under the mutex) only closed the same-event window; nothing can serialize a later batch's eviction against a read that happens after the snapshot returns. Reading through the stored fd was also wrong after atomic saves (pre-rename inode, stale contents), which is why the entrypoint already had a workaround skipping it.Fix
Reloads now always open the file by path, and the watchlist keeps sole ownership of its stored descriptor:
snapshot_fd_and_package_jsonbecomessnapshot_package_json; the stored fd is never handed out for reads. This removes the race structurally and makes the entrypoint's open-by-path workaround universal, so it is deleted.Watcher::add_fileno longer replaces a valid stored fd on the already-watched branch (it only upgrades fd-less entries inserted byadd_file_by_path_slow, e.g. the--hotentrypoint, so the directory-event recovery path keeps working). The old overwrite dropped the replaced descriptor without closing it, leaking one fd on the entrypoint per reload.add_filenow returnsFdOwnershipsaying whether it adopted the caller's descriptor; the transpiler handoff sites,AsyncModule, the bundler's plugin watch path, andadd_file_by_path_slowclose the descriptor when the watcher did not take it. The bundler'swatcher_datapath intentionally ignores the outcome and keeps today's behavior, since its descriptor can be borrowed from the resolver's entry cache.The fd-per-reload behavior is locked in by a new deterministic test in
watch-many-dirs.test.tsthat counts/proc/<pid>/fdentries for the entrypoint and an edited dependency across 15 reloads. On an unfixed build the entrypoint gains exactly one fd per reload (+15); with the fix both counts are stable. (The remaining per-reload directory-handle growth comes from the resolver'sDirEntrycache and is addressed separately in #36675.)Verification
entryDelta: 15) and passes with the fix.test/cli/hot/hot.test.ts(12),test/cli/hot/watch.test.ts(2),test/cli/watch/(7),test/bake/dev/hot.test.ts(11, including DEV:hot-9),test/js/bun/util/filesystem_router.test.ts(33),test/cli/test/test-changed.test.ts(20) all pass on a debug ASAN build.bun run rust:check-all: 10/10 target combos clean.The race predates any recent change (the fd-reuse pattern and evicting close shipped with #30412 and were inherited from the original design); CI frequency rose recently with timing shifts. The open #36675 fixes the adjacent resolver-side leaks and includes a different change to the same
add_filebranch; this PR supersedes that hunk by never replacing the stored fd at all.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/hot/watch-many-dirs.test.ts