watch mode: re-run files replaced by an atomic save (write temp + rename) - #33071
watch mode: re-run files replaced by an atomic save (write temp + rename)#33071robobun wants to merge 4 commits into
Conversation
WalkthroughRewrites the directory-event-to-watched-file mapping in ChangesHot Reloader Atomic Save Support
Documentation Cleanups
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:27 AM PT - Jun 30th, 2026
❌ @robobun, your commit ad81bf7 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 33071That installs a local version of the PR into your bun-33071 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Pushed c7e0716 to fix the cargo clippy failure (clippy::undocumented_unsafe_blocks wants the SAFETY comment on the line directly above the unsafe block; rustfmt had wrapped them apart). The test still passes against the debug build. Note for reviewers: the two docs/*.mdx files in the diff come from the autofix-ci bot formatting files that were already unformatted on main. They are not part of this change. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this rewrites the directory-event → watchlist mapping in on_file_update (unsafe deref, manual path-buffer indexing, kqueue/inotify split), so it's worth a human look from someone familiar with the watcher.
Extended reasoning...
Overview
The substantive change is in src/jsc/hot_reloader.rs: the directory-event branch of NewHotReloader::on_file_update is rewritten (~120 lines). Previously it mapped a changed file name to a watchlist entry by going through the resolver's directory-entry cache (dir_ent.entries().get(name).abs_path), which returned an empty path for files loaded under a prior cache generation, silently dropping the reload. Now it joins the watched directory path with the event's basename, hashes that, and scans the watchlist directly. The resolver cache is still touched, but only to invalidate the entry's cached fd/stat — it no longer gates whether a reload fires. Dead prev_entry_id bookkeeping and a buggy never-matching path-hash arm are removed. A new regression test in test/cli/hot/watch.test.ts exercises write-temp-then-rename with two test files plus in-directory write activity. The two docs/*.mdx diffs are autofix-ci whitespace reflows unrelated to the fix.
Security risks
None apparent. The changed path is internal watcher bookkeeping; inputs are file-system event basenames from inotify/kqueue, joined into a fixed-size stack PathBuffer with an explicit length guard before the copy_from_slice calls. No new external input surface, auth, or crypto is involved.
Level of scrutiny
Medium-high. This is core --watch / --hot runtime behavior that every developer using watch mode hits, the code path is platform-split (kqueue vs inotify) with manual byte-buffer indexing and an unsafe { &*dir_ent } deref, and watcher logic in this file has a documented history of subtle cross-thread races (see the surrounding comments about flush_evictions ordering). The refactor changes control flow (the inner hash-match loop now breaks after the first hash match regardless of fd validity, and the resolver-cache lookup pattern-matches on Fs::EntriesOption::Entries where the old code called .entries() unconditionally). These look correct but are the kind of thing a maintainer who knows the watchlist invariants should eyeball.
Other factors
The bug-hunting system found no issues. The PR description is thorough, the new test is targeted and verifies the unfixed binary fails, and the author reports the broader watch/hot test suites pass. CI on commit 6516af4 failed on a clippy undocumented_unsafe_blocks lint, which c7e0716 says it fixes; the robobun status comment hasn't yet updated for the new commit. Given the subsystem's sensitivity I'm deferring rather than approving.
|
Two notes on the control-flow points raised above, for whoever picks this up:
CI is green so far on c7e0716 (clippy included); the remaining lanes are still running. |
|
Status: the change and its regression test are done, all review threads are resolved, and the remaining CI red is unrelated to this diff. Final state of build 67252 (post-rebase head ad81bf7): 282 jobs passed, 3 lanes red, each from tests this PR does not touch:
None of these involve watch mode or the hot reloader, and the new regression test passed on every lane that ran it. I used my one CI re-trigger earlier for an infra failure, so I'm not pushing more empty commits; this is ready for review. |
There was a problem hiding this comment.
Thanks for addressing the test nits — those all look good now. The substantive change is the rewrite of the directory-event → watchlist mapping in on_file_update (path-join + hash lookup replacing the resolver-cache abs_path path), which is well-reasoned but touches core watcher-thread logic with unsafe blocks and eviction ordering, so it's worth a human pass on the Rust side.
Extended reasoning...
Overview
The PR fixes bun --watch / bun test --watch failing to detect atomic saves (write-temp + rename) on Linux when a directory has multiple watched files plus other write activity. Files touched:
src/jsc/hot_reloader.rs— rewrites the directory-event handler's mapping from changed-name → watched-file. Previously it went through the resolver's directory-entry cache (dir_ent.entries().get(name).abs_path), which returns an empty path for entries loaded under a prior cache generation. The new code joins the watched directory path with the event's filename, hashes that, and looks it up directly in the watchlist. It also removes deadprev_entry_idbookkeeping and a broken constructed-path hash arm. The resolver cache is still consulted for fd/stat invalidation but no longer gates the reload.test/cli/hot/watch.test.ts— new regression test (skipped on Windows) that reproduces the bug with two test files + in-directory write noise, then atomically replaces the first-loaded file viawriteFileSync+renameSync. Also adds a file-levelsetDefaultTimeout.docs/guides/util/base64.mdx,docs/runtime/web-apis.mdx— autofix-ci formatting noise (whitespace/table reflow only, no content change), explicitly flagged by the author as not part of the change.
Security risks
None identified. The change is confined to the file-watcher's internal event-to-reload mapping; no new external input surfaces, no auth/crypto/permissions code. The one unsafe block in the diff narrows an existing &mut *dir_ent deref to &*dir_ent with the same SAFETY justification (points into rfs.entries or a tombstoned copy that outlives the iteration). Path construction writes into a fixed-size PathBuffer with an explicit length guard before copy_from_slice.
Level of scrutiny
Medium-high. This is core runtime watcher-thread logic that every --watch / --hot user hits on every save. The diff is ~120 changed lines of Rust replacing one lookup algorithm with another, interacting with: the watchlist hash table, remove_at_index eviction (which feeds flush_evictions → fd close + swap_remove ordering already documented as race-sensitive in this file), and the kqueue/inotify split. The PR description and inline comments are thorough and the regression test is targeted, but the correctness depends on the constructed path matching exactly what Watcher::add_file hashed when the entry was added — including trailing-slash handling via strings::trim_right and SEP. That's the kind of invariant a human familiar with bun_watcher should sign off on.
Other factors
- All three of my earlier test-quality nits (vacuous assertion, per-test timeout, redundant manual cleanup alongside
await using) were addressed in a7e01a2 and 905fefb; the inline threads are resolved. - The author preemptively explained two control-flow subtleties (single-match
breaksemantics preserved from the old code;Fs::EntriesOption::Entries(..)match instead of.entries()to avoid panicking on a cachedErrvariant), which are reasonable. - CI: the only red lane reported is a darwin-aarch64 artifact-download infrastructure failure unrelated to this diff; the new test passes on the lanes that ran. One unrelated flaky (
test-net-connect-memleak.js) on Alpine x64. - The two
docs/*.mdxchanges are pure autofix reformatting and don't need review.
Given the scope of the hot_reloader.rs rewrite and its position on the watcher thread, I'm deferring rather than approving.
…ir cache A file saved via write-temp-then-rename (vim, VS Code, sed -i, git checkout) replaces its inode, so the per-file inotify watch goes permanently quiet: the watcher holds an open fd to the old inode, so not even IN_DELETE_SELF is delivered. The parent directory's IN_MOVED_TO is the only remaining signal. The directory-event handler mapped the event's file name to a watched file through the resolver's directory-entry cache (`dir_ent.entries().get(name)`, then `abs_path`). `abs_path` is populated lazily, only for files resolved through the current cache generation, and any unrelated write in the directory (a log file, editor temp files, test output) busts that cache. Once a later module load rebuilds it, previously-loaded files hash to an empty path, the watchlist lookup misses, and the reload is silently dropped; the file is never watched again. Join the watched directory's path with the event's file name and look that up in the watchlist instead. The resolver cache is still consulted, but only to invalidate the entry's cached fd/stat.
clippy's undocumented_unsafe_blocks requires the comment on the line immediately preceding the unsafe block; rustfmt had wrapped the if-let so the block landed on its own line. Bind the deref first instead.
…eanly Drop the per-test timeout argument in favor of setDefaultTimeout like the sibling watch suites, and replace the assertion that restated the awaited condition with one that checks the re-run had no failing tests.
905fefb to
ad81bf7
Compare
There was a problem hiding this comment.
All my earlier nits are addressed and I didn't find any new issues, but this rewrites the directory-event → watchlist mapping in the hot reloader (path construction, hash lookup, eviction, resolver-cache invalidation under the per-entry mutex) across both the kqueue and inotify paths, so it's worth a human pass on the core logic.
Extended reasoning...
Overview
The PR rewrites a ~100-line block inside NewHotReloader::on_file_update in src/jsc/hot_reloader.rs — the path that maps a directory-level filesystem event to a specific watched file — and adds a regression test in test/cli/hot/watch.test.ts. The old code looked the changed name up in the resolver's directory-entry cache and used the cached abs_path; the new code joins the watched directory path with the event's file name, hashes that, and looks it up directly in the watchlist. The resolver cache is still consulted, but only to invalidate the cached fd/stat (under the per-entry mutex introduced by #33056). Dead bookkeeping (prev_entry_id, the broken not-in-cache path-construction arm) is removed.
Security risks
None apparent. The change is local to the file-watcher's reload-trigger path and does not touch auth, network, process spawning, or untrusted input parsing. The one unsafe block (dereferencing the *mut Fs::EntriesOption tombstone pointer) is narrower than before (&* instead of &mut *) and keeps the pre-existing SAFETY justification.
Level of scrutiny
Medium-high. This is core runtime behavior for --watch, --hot, and bun test --watch on Linux and macOS. The new path-construction + hash-lookup logic must agree exactly with how watchlist entries were hashed when added (trailing-slash trimming, separator handling), and the eviction/break ordering interacts with the watcher-thread vs JS-thread flush_evictions sequencing documented elsewhere in the file. The author's analysis is detailed and the regression test reproduces the failure on the unfixed binary, but a human familiar with the watcher should confirm the watchlist-hash equivalence and the kqueue branch (which the new test skips on Windows but does exercise on macOS CI).
Other factors
All three of my earlier inline nits (vacuous assertion, explicit per-test timeout, redundant manual proc.kill() alongside await using) have been addressed and the threads are resolved. The bug-hunting pass found nothing this round. The author reports the full hot/watch test suite passes and CI is green except for unrelated node-compat flakes. Given the change is a real logic rewrite in a concurrency-sensitive subsystem rather than a mechanical fix, I'm deferring rather than auto-approving.
1 participant
Problem
With
bun test --watch(or--watch/--hot) on Linux, saving a test file the way editors do (write a temp file,rename()it over the original: vim, VS Code safe-write,sed -i,git checkout) is never picked up when the directory has two or more test files plus any other write activity (a log file, editor temp files, bun's own terminal output). The watcher goes permanently quiet for that file: later atomic saves and even in-place appends to it do nothing until the watcher is restarted.Reproduced on 1.4.0 and current main (0/5 re-runs with two files, 5/5 with one file or with an in-place append).
Cause
Two things combine:
IN_DELETE_SELF; the watch is a silent zombie. The parent directory'sIN_MOVED_TOis the only signal left.NewHotReloader::on_file_updatemapped the event's file name to a watched file through the resolver's directory-entry cache:dir_ent.entries().get(name)thenabs_path.abs_pathis populated lazily, only for files resolved through the current cache generation, and every directory event busts that cache. So when another test file loads after any unrelated write in the directory, the rebuilt generation has an emptyabs_pathfor the files loaded earlier, the hash lookup misses, and the reload is dropped. Editing the last-loaded test file still worked, which is what made this look timing-dependent.Fix
In the directory-event path, join the watched directory's path with the event's file name and look that path up in the watchlist (the watcher's own source of truth), instead of going through the resolver cache. The resolver cache is still consulted, but only to invalidate the entry's cached fd/stat. The matched entry is evicted as before so the reload re-arms a watch on the new inode.
This also removes the dead
prev_entry_idbookkeeping and the unused constructed-path hash in the old not-in-cache arm (it overwrote the path separator and hashed a stale trailing byte, so it could never match anything).Verification
New test in
test/cli/hot/watch.test.ts: two test files where the first writes into the watched directory while it runs (forcing the cache regeneration), then the first-loaded file is replaced via write-temp-then-rename.USE_SYSTEM_BUN=1): fails 3/3, no re-run ever happensbun bd test): passestest/cli/hot/watch.test.ts,test/cli/hot/hot.test.ts,test/cli/hot/watch-many-dirs.test.ts,test/cli/watch/watch.test.ts,test/cli/watch/watcher-trace.test.ts, andtest/cli/test/test-changed.test.tsall pass with the fix.Rebase note
Rebased onto main to resolve a conflict with #33056, which added a per-entry mutex around the resolver-cache fd/stat invalidation inside the block this PR rewrites. The resolution keeps that locking: the
set_cache_fd/need_statwrites in the new loop now takeEntry.mutexexactly as #33056 does. The rebase also drops the autofix-ci docs commit (main is formatted now), so the PR no longer touchesdocs/.