Repository navigation
Windows watcher: treat every watched path as changed after a ReadDirectoryChangesW overflow - #42951
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 8 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/watcher/WindowsWatcher.rs— pre-existing, made routine by this PR: Windowsbun --watchusers whose project sees a write burst (for examplebun installinto node_modules) now get a restart straight into that burst, and the restarted process can crash.watch_loop_cyclereadsthis.watchlist.items_file_path()at WindowsWatcher.rs:447-455 withoutthis.mutexwhile the new process's JS thread appends under the mutex, soensure_unused_capacity(Watcher.rs:706-708) can free the column the compare is reading. Fix: takethis.mutexaround the watchlist scan inwatch_loop_cycle(or snapshot the paths under it), asresync_after_overflowalready does. The PR calls this crash out of scope, but its restart-on-overflow is what puts a fresh process under the burst.Extended reasoning...
Population: any Windows
--watch(orbun test --watch) project where a burst of writes under the recursively watched root overflows the 64 KiB buffer;bun installwriting thousands of files into node_modules is the everyday case, since every record costs the watcher thread one compare per watched path (WindowsWatcher.rs:447-456). On the base, the overflow branch re-armed and continued; the running process's watchlist was static (add_filededupes viaindex_of, Watcher.rs:876), so no concurrent growth occurred. With this PR,nextreturnsNext::Overflow(WindowsWatcher.rs:349),resync_after_overflowdispatches WRITE for the first 128 entries (WindowsWatcher.rs:540),Task::enqueuewithRELOAD_IMMEDIATELYcallsreload_process(hot_reloader.rs:568-576), which isTerminateProcessplus parent respawn on Windows (util.rs:4316-4329). The respawned process starts its watcher inenable_hot_module_reloadingbefore the entry point loads, while the burst is still running. Its JS thread resolves modules and callsappend_file_maybe_lock::<_, true>(Watcher.rs:691-750), which…Verification: pre-existing (the race lives in untouched code) but this PR adds a new, everyday route into it; acknowledged in diff: the PR description says "a
--watchrestart under a sustained burst starts a process whose watcher scans the watchlist without the mutex while the JS thread grows it (#31695 item 3). That crash is pre-existing and out of scope here" — the "pre-existing" claim is accurate,…
|
Reworked in 1242f17478 after the review. The resync no longer runs inside the burst: after an overflow the watcher waits until no read completes for 250 ms, then dispatches one batch. So |
be8c53b to
871e545
Compare
|
Updated 2:43 PM PT - Sep 16th, 2026
✅ @robobun, your commit debddb1a733f3d0218be8018b93dfe7cfebc88fb passed in 🧪 To try this PR locally: bunx bun-pr 42951That installs a local version of the PR into your bun-42951 --bun |
|
Restructured after a self-review. The Windows event loop wakeup is now its own PR, #42968, with a direct test, and this PR is stacked on it. |
…toryChangesW overflow burst ends
…it, and scan the watchlist under the mutex
WindowsWatcher::next issued a new read before every wait, also while the read of an earlier, timed out wait was still queued. Every watcher cycle left one more read queued on the directory handle. All reads share one buffer. A read that finds changes already recorded completes inside the call and fills the buffer at once. A read that completes later fills it only when its packet is dequeued. With two reads in flight, the dequeue of the older packet overwrote the records of the newer read. The watcher parsed the older records twice and never reported the newer changes. Issue a read only when none is outstanding. (cherry picked from commit a9a3b37)
c764d96 to
debddb1
Compare
Problem
--hot,--watch,bun build --watchand the dev server read file changes withReadDirectoryChangesW. The kernel records changes between two reads in a 64 KiB buffer. When that buffer overflows, the next read completes with zero bytes and every record since the last read is lost. An edit of the entry point that lands in that window never reloads. Fixes Windows watcher: a ReadDirectoryChangesW buffer overflow drops the changes and nothing reloads #42936.WindowsWatcher::next(src/watcher/WindowsWatcher.rs, thenbytes == 0branch) logged the overflow and re-armed the read. The context never learned that records were lost.Watcher.mutex. A process that starts inside a burst (a--watchrestart) reads a column the JS thread frees while it grows the watchlist, and crashes inis_parent_or_equal.Fix
nextreturnsNext::Overflowon a zero byte completion.watch_loop_cyclethen callsWatcher::resync_after_overflow(src/watcher/Watcher.rs), at most once per second. It stats every watched path and dispatches one batch with a syntheticWRITEfor each path whose mtime is newer than the previous resync (or the start), or which no longer exists.--watchrestarts,--hotreloads,bun build --watchrebuilds, and the dev server invalidates its graph. Unwatched writes, including the process's own output, produce no events.Watcher.mutexper item, so the JS thread's own appends keep flowing during a burst.IN_Q_OVERFLOW.statgoes through libuv, which has no long path support, so a drive letter path gets the\\?\form.test/cli/hot/hot.test.ts"--hot reloads an edit that lands in a ReadDirectoryChangesW buffer overflow". Canary 1.4.3 fails 3 of 3 runs (the edit is lost, 60 s timeout). A debug build of this branch passes 5 of 5 in about 19 s. Alsotest/cli/watch/watch.test.tsandtest/js/node/timers/node-timers.test.tson Windows.Background
WatcherContext::on_file_updatereceivesWatchEvents (anOpand a watchlist index). The hot reloader turns aWRITEon a file into a reload task and aWRITEon a directory into a directory cache bust. The dev server records the file as changed.Notes
Reproduction. Windows Server 2019 x64, 16 vCPUs. A
bun --watch index.jsproject with 15000 imported modules and 8 writer processes that callutimeson files under the project root (320000 calls, 23 s). An edit ofindex.jsafter the burst did not reload on the 1.4.3 canary, 2 of 2 runs. The same edit with no burst reloads. A debug build withBUN_DEBUG_watcher=1shows 8 overflow lines, 2498 records seen of 80000, and no record forindex.jsuntil the next edit.Test design. The project puts 3000 modules under a 2000 byte directory prefix that the burst files share, so every record costs 3000 long compares and the watcher thread falls behind 8 writers. Each round: start the writers, wait for their "ready", edit the entry, wait for the writers to exit, then wait for the new output. On the canary the edit is lost in 98 of 100 mid-burst writes (54 writes were needed before one got through), so two rounds give a reliable failure. The test cannot assert that an overflow happened without a src hook, so a runner whose watcher keeps up passes on the base too. It never fails with the fix.
Why the resync stats instead of reporting every path. The first version reported every watchlist entry. Under a sustained burst
--hotreloaded several times per second,--watchrestarted into the burst, and abun build --watchwhose output lands under the root would restart on its own writes. With the mtime filter an unrelated burst produces an empty resync. A deferral until the root went quiet was tried in between and dropped: inside an overflowing burst completions arrive in groups about 55 ms apart with gaps up to 123 ms, quiet detection either fired early or held real edits back, and a steady background writer would hold them forever.Probes on a debug build of this branch (3000 modules, 8 writers, one edit 1.5 s into a 30 s burst, one edit after):
--hotand--watchboth print the mid-burst edit within about 11 s (one debug reload) and the later edit at once, with no crash or restart loop. Without the #42938 commit the--watchprocess that restarts inside the burst never reports the later edit. Without the per item lock it crashes inis_parent_or_equalon every restart.Related. #42938 keeps one
ReadDirectoryChangesWoutstanding. #36416 adds the inotify resync. #31695 item 3 locks the same scan. #40017 and #39820 touch the same function.Suites run on Windows (debug build of this branch):
test/cli/hot/hot.test.ts"overflow" 5 of 5 and "hundreds" 1 of 1,test/cli/watch/watch.test.ts(6 pass, 7 skip),test/js/node/timers/node-timers.test.ts(23 pass).cargo check -p bun_watcheris clean for linux and windows-msvc.no test proof · iteration 1 · 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, test/cli/hot/hot.test.ts