Repository navigation
Conversation
`DirectoryWatchStore` kept the position of its watch item in the watcher's list and removed the item by that position when a failed import resolved. The watcher moves its last item into the slot of a removed item, so the position could name the item of another file. That file then stopped hot reloading until restart. - `Watcher::remove_directory(hash)` finds the directory item under the watcher mutex and queues it for removal. - `Watcher::add_directory` returns no position. For the store it also keeps an item that a release queued, so a failed import in the same directory right after a release is still retried. - `Watcher::remove_at_index` is for `on_file_update` only. Its `LOCK` generic and its unused `kind`, `hash` and `parents` parameters are gone. - The watcher opens the directory of a store watch on every platform, with `O_DIRECTORY`, and closes it if the watch fails. On macOS the store no longer opens a descriptor that it and the watch item both close.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status Reproduced on linux-x64 with the dev server test harness. The first new test in
On main, step 6 produces no hot update (10 of 10 runs on a release build of 179664e). With this change it does (20 of 20). PR: #46916 |
|
Updated 2:20 PM PT - Oct 25th, 2026
✅ @robobun, your commit 63d28045b9f0f5068b07cf90221321eed056456f passed in 🧪 To try this PR locally: bunx bun-pr 46916That installs a local version of the PR into your bun-46916 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 findings, I also checked two things in the watcher refactor and ruled them out: the parent-eviction loop deleted from remove_at_index was dead at every caller (all passed Kind::File and an empty parents slice), so dropping it is behavior-preserving; and the CANCEL_REMOVAL compaction in add_directory cannot discard watcher-thread evictions, because on_file_update flushes its queue under the same mutex before releasing it, so only remove_directory entries are ever in evict_list when the main thread takes the lock.
Extended reasoning...
The change replaces the dev server's stored WatchItemIndex/fd with a hash-keyed removal in src/watcher/Watcher.rs and src/runtime/bake/dev_server/directory_watch_store.rs, touches no security-sensitive surface, and has four confirmed inline findings (an unbounded evict_list write, hash-only removal evicting the resolver's shared directory watch, and two platform-flag issues) that warrant a human look.
| bun_sys::open_a(file_path, bun_sys::O::RDONLY | bun_sys::O::CLOEXEC, 0)? | ||
| // `O_DIRECTORY`: a FIFO or a file at this path fails here and is not opened. | ||
| #[cfg(not(windows))] | ||
| let flags = bun_sys::O::RDONLY | bun_sys::O::DIRECTORY | bun_sys::O::CLOEXEC; |
There was a problem hiding this comment.
🟡 nit (optional): on macOS, directories watched for a failed import are now held open with a plain read handle instead of the event-only flag the rest of the watcher uses. src/watcher/Watcher.rs:622 builds O::RDONLY | O::DIRECTORY | O::CLOEXEC, while every other watch open in this file uses WATCH_OPEN_FLAGS (O_EVTONLY | O_CLOEXEC on darwin, Watcher.rs:44), and the store's removed open used O::DIRECTORY | WATCH_OPEN_FLAGS. Fix: open with WATCH_OPEN_FLAGS | bun_sys::O::DIRECTORY on non-Windows so the directory handle never blocks an unmount and matches the file-watch opens. The PR description names the O_RDONLY/O_EVTONLY difference as an unrun downside. [also at: src/watcher/Watcher.rs:624 - nit: on macOS the directory watched for a failed import is now held open with O_RDONLY instead of O_EVTONLY, which the base opened it with.]
Why this was flagged
A macOS dev server user has a failed relative import whose directory exists (for example on an external or network volume); DirectoryWatchStore::insert at src/runtime/bake/dev_server/directory_watch_store.rs:285 passes Fd::INVALID so the watcher opens the directory itself at src/watcher/Watcher.rs:625. The flags at Watcher.rs:622 are O_RDONLY|O_DIRECTORY|O_CLOEXEC, not the O_EVTONLY|O_CLOEXEC that WATCH_OPEN_FLAGS (Watcher.rs:44) provides and that add_file_by_path_slow (Watcher.rs:865) and the base-branch store open (O::DIRECTORY | WATCH_OPEN_FLAGS) used. An O_RDONLY directory handle counts as a vnode in use, so the volume cannot be unmounted until the watch is flushed, whereas the base branch's O_EVTONLY handle does not block unmount. The PR description lists this flag difference as an untested downside rather than an intended behaviour.
Verification: Trigger: a macOS dev server records a failed relative import whose directory exists. Mechanism verified. src/watcher/Watcher.rs:622 builds bun_sys::O::RDONLY | bun_sys::O::DIRECTORY | bun_sys::O::CLOEXEC. So for the failed-import directory, merging changes the macOS descriptor from O_EVTONLY to a plain O_RDONLY read handle, which blocks eject/unmount while the dev server runs.
There was a problem hiding this comment.
I kept O_RDONLY on purpose. This line opens every directory item that the watcher creates, not only the one of the store. On main it already opens the parent directory of each watched file with O_RDONLY, because the bundler passes no dir_fd (src/bundler/ParseTask.rs:2393). Each watched file also keeps the read descriptor of its parse. So a volume with dev server sources is busy with or without this one descriptor.
WATCH_OPEN_FLAGS here would change those parent items to O_EVTONLY on macOS, and --hot calls faccessat on the descriptor of a directory item there (src/jsc/hot_reloader.rs:982). I have no macOS machine to run that. The description names the flag difference under Downsides.
| pub fn remove_directory(&mut self, hash: HashType) { | ||
| let _guard = self.mutex.lock_guard(); | ||
| let Some(index) = self.index_of(hash) else { | ||
| return; | ||
| }; | ||
| // `add_directory` matches by hash alone, so the item can be a file's. | ||
| if self.watchlist.items_kind()[index as usize] == WatchItemKind::Directory { | ||
| // Queued, not removed: events in flight name items by position until the watcher thread flushes. | ||
| self.remove_at_index(index); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Dev-server users whose failed import names a directory the resolver already watches lose the resolver's watch on that directory when the import resolves; the resolver's cached fd for it is closed at the next flush. remove_directory at src/watcher/Watcher.rs:944-950 looks up by hash alone and queues whichever Directory item owns that hash, including one the resolver registered through on_maybe_watch_directory with its own dir_fd. Fix: the store must only release an item it created (track that add_directory::<true,true> appended rather than hash-hit, or refcount the item), so a shared item survives the store's release. [also at: src/watcher/Watcher.rs:817 - Dev-server users can lose the resolver's directory watch for a project directory after a failed import there resolves, so the resolver's dir cache is never busted again for that directory. src/watcher/Watcher.rs:804-817 only cancels a queued eviction when CANCEL_REMOVAL is true; the resolver's add_directory::<false, false> at src/watcher/Watcher.rs:983 hash-hits the item that free_entry just queued and returns Ok without cancelling, and the next flush evicts it.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The resolver registers directory D with its cached fd via src/watcher/Watcher.rs:976-984 (hash of D without trailing slash). A failed relative import under D then calls insert, which hash-hits at src/watcher/Watcher.rs:804 and returns Ok(()) with no new item (src/runtime/bake/dev_server/directory_watch_store.rs:283-295). When the import resolves, free_entry calls remove_directory (directory_watch_store.rs:66-67); index_of finds the resolver's item, kind is Directory, so it is queued at src/watcher/Watcher.rs:950 and flush_evictions closes its fd at :437 and drops it at :451. The fd was the resolver's DirEntry.fd (src/resolver/lib.rs:795), now EBADF, and the directory has no watch so bust_dir_cache (hot_reload_event.rs:109) never runs for it again. The base had the same consequence but this change rewrote the release and added a kind check while leaving ownership unchecked, which REVIEW.md's one-owner rule flags.
Verification: pre-existing — the base already released the same shared item by the same route. free_entry (directory_watch_store.rs:66-67) calls remove_directory(hash), which takes the first item with that hash and queues it if its kind is Directory (Watcher.rs:944-951) — there is no producer check, so the resolver's item is queued and flush_evictions closes its fd (Watcher.rs:436-451).
There was a problem hiding this comment.
This is the shared item case that the description lists under "Not fixed here". It is older than this change: main releases the same item by the position that add_directory returned for the hash hit.
Two corrections for the dev server:
- No resolver descriptor is closed. The dev server resolvers keep
store_fdfalse (src/resolver/resolver.rs:969, the one= trueis the completions helper atsrc/runtime/cli/run_command.rs:3727). SoDirEntry.fdis invalid (resolver.rs:3512), and the watcher opens the descriptor of that item itself. - A directory event feeds the retry of the store and, on Linux, the names of changed files (
src/runtime/bake/dev_server/hot_reload_event.rs:97-173). The store registers the directory again at the next failed import. A directory with watched files keeps its parent item (<dir>/, another hash), so the names still arrive.
A release of only the items that the store appended does not cover the other order: the store appends the item and the resolver hook hits it later. That needs an owner count on the item, and a write on the hash-hit path of the hook that --hot and --watch also run. I left it out of this change.
| // `O_DIRECTORY`: a FIFO or a file at this path fails here and is not opened. | ||
| #[cfg(not(windows))] | ||
| let flags = bun_sys::O::RDONLY | bun_sys::O::DIRECTORY | bun_sys::O::CLOEXEC; | ||
| #[cfg(windows)] | ||
| let flags = bun_sys::O::RDONLY | bun_sys::O::CLOEXEC; | ||
| bun_sys::open_a(file_path, flags, 0)? |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: on Windows a failed import below a regular file still makes the file untrackable, so a dev-server user's later edits to it are not hot reloaded. The #[cfg(windows)] arm at src/watcher/Watcher.rs:624 omits bun_sys::O::DIRECTORY, so for import './dep.ts/x' the store's add_directory opens the file dep.ts and appends it as a Directory item. Fix: make the open fail for non-directories on every platform, e.g. use O::DIRECTORY in both arms (it is defined on Windows and routes to the directory NtCreateFile path), so the POSIX fix in this PR covers the sibling Windows branch too.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
On Windows, inside the project root, a file imports ./dep.ts/x; DirectoryWatchStore::insert (src/runtime/bake/dev_server/directory_watch_store.rs:285) calls add_directory::<true, true> with the dir …/dep.ts. append_directory_assume_capacity uses the Windows flags at src/watcher/Watcher.rs:624 (O::RDONLY | O::CLOEXEC, no O::DIRECTORY); openat_windows_impl (src/sys/lib.rs:7063) only takes the directory NtCreateFile path when O::DIRECTORY is set, so the regular file opens and a WatchItem of kind Directory with hash(…/dep.ts) is appended (src/watcher/Watcher.rs:683). When the bundler later watches dep.ts itself, add_file (src/watcher/Watcher.rs:903) finds that hash and returns Ok(FdOwnership::Caller) without appending a File item. The Windows watcher matches the edit of dep.ts to that Directory item (src/watcher/WindowsWatcher.rs:451) and the dev server routes it to append_dir (src/runtime/bake/dev_server/watch.rs:143); process_file_list only retries the still-failing ./dep.ts/x and never invalidates dep.ts, so the edit is lost. On POSIX the new O::DIRECTORY makes the open fail with ENOTDIR; the base branch behaved the same way on Windows, so this is pre-existing.
Verification: pre-existing (the base already fails the same way on Windows by the same route; this PR fixes the class with O_DIRECTORY on POSIX only and leaves the Windows arm out). src/watcher/Watcher.rs:621-625: the #[cfg(windows)] arm is O::RDONLY | O::CLOEXEC only. src/sys/lib.rs:7063 takes the directory branch only when (flags & O::DIRECTORY) != 0, so a regular file opens.
There was a problem hiding this comment.
Correct, and older than this change. I left the Windows arm as it is on main because I cannot run it. With O::DIRECTORY, openat_windows_impl takes open_dir_at_windows_nt_path (src/sys/lib.rs:7063). That path asks for other rights than the file path does (FILE_TRAVERSE, FILE_LIST_DIRECTORY) and sets FILE_DIRECTORY_FILE, for all three producers of directory items. I added the Windows case to "Not fixed here" in the description.
| pub fn remove_at_index(&mut self, index: WatchItemIndex) { | ||
| debug_assert!(self.mutex.is_held_by_current_thread()); | ||
|
|
||
| self.evict_list[self.evict_list_i as usize] = index; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: The dev server panics with an index out of bounds if more than 8096 directory watches are released between two watcher flushes. remove_at_index at src/watcher/Watcher.rs:958 writes self.evict_list[self.evict_list_i as usize] with no bound against MAX_EVICTION_COUNT (src/watcher/Watcher.rs:52), and the new public remove_directory (src/watcher/Watcher.rs:942) lets the main thread queue from free_entry at any time, while the queue is only drained by the watcher thread inside on_file_update. Fix: bound the write (flush or drop when full, or make the queue growable) so a burst of releases cannot index past the fixed array.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
free_entry (src/runtime/bake/dev_server/directory_watch_store.rs:66-67) runs on the main thread for every directory whose deps all resolved, and also from disconnect_and_delete_file (src/runtime/bake/dev_server/incremental_graph.rs:755) during a rebuild. Each call reaches remove_at_index and increments evict_list_i (src/watcher/Watcher.rs:958-959). evict_list is a fixed array of MAX_EVICTION_COUNT = 8096 (src/watcher/Watcher.rs:52,129) and is reset only in flush_evictions at :470, which runs only on the watcher thread when a batch of events is dispatched. A large rebuild that releases many entries (for example deleting a directory tree whose files each had failed imports, or a 'use client' sweep) can queue more than 8096 indices before the next batch; the array index then panics in release builds, taking the dev server down. The base is the same, but this change made the entry point public and hash-keyed, and the cancel path at :808-815 also assumes the list is well-formed. Remedy: guard the write against MAX_EVICTION_COUNT.
Verification: pre-existing. Trigger: more than 8096 free_entry calls on the main thread between two watcher-thread flushes. evict_list is a fixed [WatchItemIndex; MAX_EVICTION_COUNT] with MAX_EVICTION_COUNT = 8096 (src/watcher/Watcher.rs:52). remove_at_index (src/watcher/Watcher.rs:955-959) writes it with no bound check. On the base, free_entry called remove_at_index from the main thread.
There was a problem hiding this comment.
Older than this change. On main free_entry queues from the same thread through remove_at_index::<true>, one entry per release, into the same fixed array. This change adds no producer and does not raise the rate. #46029 replaces the array with a Vec, which removes the bound. I left it to that PR so that the two changes do not collide on evict_list.
|
Cross-reference from #39571 (no stored descriptor for a watched file outside kqueue). With #39571 a watched file that is removed on Linux reports So #39571 needs this PR to merge first. Its body now says so. This PR's test "keeps the watch of a file that was saved by rename" covers the save side. The removal sequence above is not in either PR yet: a dev server test for it is on the branch |
Problem
DirectoryWatchStore::free_entryremoves the directory watch of the failed import by a stored position in the watcher's list (src/runtime/bake/dev_server/directory_watch_store.rs:66).Watcher::flush_evictionsmoves the last item into a removed slot (src/watcher/Watcher.rs:450), so the position can name another file's watch.Fix
Watcher::remove_directory(hash)finds the item under the watcher mutex and queues it.add_directoryreturns no position and keeps a queued item for the store.O_DIRECTORY. The store holds no position and no descriptor.test/bake/dev/html.test.tsandbundle.test.ts(8 fail on main), pluscss,hot,esmthere,test/cli/hot,test/cli/watch.add_directory(the release has one), a second trigger test (same code path).Background
free_entryonly (feat(bake): await plugin preparation and track load dependencies #46706 has one). It leaves the second case broken.Downsides
free_entryscans the watchlist: 144 to 5,247 instructions at 1,000 watch items. A file's first watch: -21 to +54. Text: +256 B.O_RDONLY, notO_EVTONLY.Notes
Where it comes from. There is no issue and no user report. The wrong removal showed in the dev server's debug logs.
free_entryhas removed by a stored position since the store was written.Who can hit it. Any failed relative import (also one left by a moved or deleted file), then the removal of a file's watch item, then the import resolves.
free_entryclosed it, andflush_evictionsclosed the same number again for the watch item. The watch item is now the one owner.u32::MAXas its position and queued it at release. The store keeps no position now../dep.ts/xnames the watched filedep.tsas its directory. On main the release removed the watch item of that file.remove_directoryonly queues a directory item.open(2).O_DIRECTORYmakes that open fail.Deleted.
Entry.watch_index,Entry.dir,Entry.dir_fd_owned, the resolver probe and the descriptor code ininsert, theFdExtimport. FromWatcher::remove_at_index: theLOCKgeneric, the unusedkind,hashandparentsparameters, the branch that cast a hash to a position, the sentinel assert. The position thatappend_directory_assume_capacityreturned, withparent_watch_iteminappend_file_maybe_lock.Other designs.
--hotand--watchtoo.remove_directoryqueues and does not remove. Each platform loop maps events to positions before it takes the mutex, so only the watcher thread can reorder the list.flush_evictionsnow asserts that thread in debug builds.Watcher.remove(hash). Remove ~39k lines of dead Rust across the workspace #35002 removed it as dead code.Tests. "main" is a release build of 179664e, one process per run.
free_entryremove_directoryO_DIRECTORYremove_directory, noO_DIRECTORY, a store directory named with a trailing separator.cargo check -p bun_runtimepasses for aarch64-apple-darwin, x86_64-pc-windows-msvc, x86_64-unknown-freebsd, aarch64-unknown-linux-musl and x86_64-linux-android.Measurements. linux-x64, release builds of main 179664e and of this branch.
perfandvalgrindare not on this machine, so an instruction count is a gdbstepicount of one call.nm -S,objdump -d). The sixth,on_file_update_wrapped::<DevServer>, has the same size and instruction count and encodes one bound check differently.get_resolve_watcher::wrap): -3 B, -1 instruction on theadd_directoryhash-hit path (objdump).free_entry: 747 / 5,247 / 50,264 instructions at 100 / 1,000 / 10,000 watch items, 144 on main.track_resolution_failure: -264 instructions with an empty eviction queue, -221 with one pending release (3,568 to 3,304 and 1,991 to 1,770 at 100 items).append_file_maybe_lock): 1,228 to 1,207 instructions at 1,000 items and 1,176 to 1,230 at 10,000 when the parent directory is watched. 6,855 to 2,934 and 51,804 to 12,480 when it is not.size).free_entry372 to 202 B,track_resolution_failure2,906 to 2,421 B plus 452 B foradd_directory::<true, true>,remove_directory240 B.size_of::<Entry>()16 to 4 B.size_of::<Watcher>()35,936 B, unchanged./proc/<pid>/fd, 20 cycles).Not fixed here.
remove_directorymust not get the hash of a watched file's parent item. The store names its directory without the trailing separator and the parent item has one, so the two differ. One test pins this.flush_evictionscloses descriptors on POSIX only).O_DIRECTORY, as on main. A failed import below a file that is not watched yet still registers that file as a directory item there (read, not run).inotify_rm_watch. The eviction queue is a fixed array of 8,096 entries.Overlap. #46029 and #45723 cut the same
remove_at_indexparameters. #46029 lists this defect under "Not fixed here", and itsEntry.watch_indexhunks go away with this change. #46706 looks the item up by hash insidefree_entry. #42129, #39834 and #46809 also changeWatcher.rs.Related. #29529 fixed the same kind of stale position for kqueue (#29524). #40640 fixed the same symptom for files outside the root from another cause (#42094).
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/html.test.ts