Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesWindows watcher lifecycle
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No merge-blocking issue was identified in the Windows watcher lifecycle or its regression coverage. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: closed in favor of #44344. Its first change is the same gate. How to reproduce, on Windows x64: With this change the new test passes 10 of 10 runs on a release build and on a debug build (+0 bytes), and the debug build finishes |
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 (both on the new test), I also checked the Rust side of the change: read_pending is cleared only when the dequeued overlapped is the watcher's own, so a foreign/spurious completion at src/watcher/WindowsWatcher.rs:353 correctly leaves the flag set (the real read is still queued), and the rc == 0 error and nbytes == 0 re-arm paths both keep the flag consistent with what is on the port. Closing the directory handle in stop() while a read is outstanding is unchanged from before the PR (the old code had more reads queued, not fewer), so it is not introduced here.
Extended reasoning...
The native change is a single read_pending flag with three touch points (arm(), the post-GetQueuedCompletionStatus clear keyed on pointer identity, and the two prepare() call sites replaced by arm()). I traced each exit of next(): timeout returns leave the flag set (read still queued, which is the whole fix); the matching-overlapped branch clears it before either the error return or the event return; the non-matching-overlapped continue leaves it set, which is correct since a foreign packet does not consume the watcher's read. The stop()-with-pending-read concern is pre-existing behaviour and is not made worse by this PR. The two confirmed findings are on the test's runtime budget and its undeadlined setup loop; nothing further on the Rust side needed to be added.
|
Updated 8:50 AM PT - Sep 16th, 2026
✅ @robobun, your commit a9a3b370799436101986c920ad4ef644c48696e3 passed in 🧪 To try this PR locally: bunx bun-pr 42938That installs a local version of the PR into your bun-42938 --bun |
f1f574d to
272d0df
Compare
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.
272d0df to
a9a3b37
Compare
There was a problem hiding this comment.
I reviewed the updated push and found no bugs; since this changes the Windows watcher's I/O-completion handshake and the test can only run on Windows, a human look on the Windows CI result would still be worthwhile.
What was reviewed:
read_pendingbookkeeping inWindowsWatcher::next: checkedGetQueuedCompletionStatusout-param semantics —lpOverlappedis NULL on timeout and set to the failed OVERLAPPED on an aborted read, so clearing before therc == 0branch is correct;stop()is the only lifecycle exit and is terminal (no re-init), so no stale-flag path.- Spurious-packet
continueand the nbytes==0 re-arm path keep the flag consistent with what is actually queued. - Test: the setup loop is now bounded and the explicit timeout is gone. Not verifiable from Linux: whether 200 watcher round-trips plus PowerShell startup fit the default per-test timeout on the Windows debug lane (the PR description still cites 100 reloads, so the numbers there look stale relative to the code).
Extended reasoning...
Overview
The diff (+104/-6) touches src/watcher/WindowsWatcher.rs and test/cli/hot/watch-many-dirs.test.ts. The Rust change adds a read_pending: bool to WindowsWatcher, an arm() helper that only calls DirWatcher::prepare() (the ReadDirectoryChangesW issue) when no read is outstanding, and clears the flag in next() whenever GetQueuedCompletionStatus hands back the watcher's own OVERLAPPED. Both former prepare() call sites (top of next() and the nbytes==0 overflow re-arm) go through arm(). The test adds a Windows-only case that drives ~200 watcher cycles by writing an unimported file and compares the process's nonpaged-pool counter before and after via a single long-lived PowerShell.
Security risks
None identified. The change does not touch input parsing, paths, or privileges; it only changes when an already-issued kernel read is re-issued. The test spawns PowerShell with a fixed command string and the child's PID interpolated, which is a number.
Level of scrutiny
Moderate. The logic is small but sits on a kernel-level handshake where a wrong assumption about out-param semantics would silently re-introduce the double-read or, worse, never re-arm. I checked: on timeout GetQueuedCompletionStatus sets *lpOverlapped to NULL (so the equality check does not fire and the flag stays true, which matches reality — the read is still queued); on a failed I/O such as ERROR_OPERATION_ABORTED after stop() closes the directory handle it sets the failed OVERLAPPED, so the flag is cleared and Err is returned. stop() has a single caller in Watcher::thread_body on the error exit and nothing re-initializes the same WindowsWatcher afterward, so there is no lifecycle path where a stale read_pending = true would suppress a needed prepare(). The spurious-packet continue leaves the flag untouched, which is correct because the watcher's own packet is still on the port.
Other factors
The exit reason was dry_streak with no findings. The prior review's two test concerns (unbounded setup loop, explicit 30 s timeout / large workload) are addressed in this push: the setup loop is capped at 20 attempts of 100 ms with a named failure, and the explicit timeout is gone. The workload changed shape rather than shrinking — 200 cycles of a non-reloading write instead of ~100 full reloads — and the single PowerShell now answers both samples. I cannot execute the test from this Linux checkout, so whether it stays under the default per-test timeout on the Windows debug lane is something the CI run has to settle; the PR description's "100 reloads" figures appear to predate this version of the test. I am not approving outright because the correctness of the fix is observable only on Windows and the claimed measurements cannot be reproduced here.
|
Closing in favor of #44344. Its first change (
If #44344 does not land, reopen this PR. |
Problem
WindowsWatcher::next(src/watcher/WindowsWatcher.rs:296) issues aReadDirectoryChangesWbefore every wait, also while an earlier read is still queued. Each cycle leaves one more read queued: 200 watcher cycles add about 100 KiB of nonpaged pool.Fix
WindowsWatcher::armissues a read only when none is outstanding. The buffer then belongs to one completion at a time.test/cli/hot/watch-many-dirs.test.tsmeasures the nonpaged pool over 200 watcher cycles. The 1.4.3 canary fails 8 of 8 runs (+101 KiB, limit 16 KiB). With the fix, release and debug builds pass 10 of 10 (+0 bytes).test/cli/hot/hot.test.ts, 12 of 12.Background
ReadDirectoryChangesWcalls. An I/O completion port queues one packet per completed read.Notes
Where this came from. A Windows debug build never finishes
test/cli/hot/hot.test.ts(its timeout isInfinityon debug builds). In "should work with sourcemap loading" the--hotside is fine. Thebun build --watchchild never reports the write tobundle_in.ts, so the outfile is never rebuilt.bun build --watchand--watchrestart on every change, so each generation has a new watcher with one read, which is the state that loses a change. Log of the child that hung (BUN_DEBUG_watcher=1, read numbers added):The
renamed()test loses theRemovedrecord of the entry point the same way.BUN_WATCHER_TRACEof its--hotchild shows{dir: []} {dir: [write]}and nothing more without the fix, and{dir: [delete, rename, write], entry: [delete, write]}with it. The same analysis is in a comment on #40017.Counts. Windows Server 2019 x64, 16 vCPUs, no extra load. "Without the fix" is a debug build of main, or the 1.4.3 canary where named. The
hot.test.tsrows are from main at f937cf4, the rows of the new test from main at c6b7fcb, which this branch is now based on.hot.test.ts, debug buildNonpaged pool of the
--hotprocess over 200 watcher cycles (Get-Process,NonpagedSystemMemorySize64): canary 7,240 to 104,520 bytes, release build with the fix 7,240 to 7,240, debug build with the fix 11,456 to 11,456. The counter does not move for the first 10 to 25 queued reads, then it grows by 500 to 1,000 bytes for each one. So a short run proves nothing: 30 reloads measured +0 bytes on the canary, 100 reloads +43 to +57 KiB.Probe. A 100 line program with raw
kernel32calls and bun's setup: an overlapped directory handle on a completion port, one buffer, oneOVERLAPPED. One read is queued. File A is written, then file B (a write is two records). Then a second read, then two dequeues:Source of the first probe (rustc, no crates)
A second probe runs bun's loop shape against bursts of writes, with a new watcher for each trial, and counts the trials in which the one write to the watched file is never reported. "Dispatch" is a busy wait after each batch that stands in for
on_file_update:Source of the second probe. The table rows are
rdcw_burst3.exe every|once 200 2 4 200 <dispatch_us>This is the model for release builds. I could not make the 1.4.3 canary lose a change: about 2,000 generations of
--watchwith bursts of up to 64 writes around the watched write, and 130 more with 3,000 to 10,000 modules in the watchlist, all restarted. A long lived process is also protected by the defect itself: each cycle leaves one more read queued, so a burst rarely finds the handle without a read.The test.
bun --hot entry.jswithBUN_WATCHER_TRACE. A write of a file that nothing imports is one watcher cycle: the watcher traces a batch for the directory and reloads nothing. The test makes 200 such writes and waits for the trace to grow after each one, which takes about 17 ms in total. It readsNonpagedSystemMemorySize64of the child before and after from one PowerShell process (test/bundler/compile-windows-metadata.test.tsalso asks PowerShell). Windows charges each queued read to that quota, so the old loop adds about 100 KiB and the new one nothing. The limit is 16 KiB. The whole test takes 1.0 to 1.4 s on a release build and on a debug build, most of it the start of PowerShell, and it has no timeout of its own. Before the first sample the test writes until the trace shows a batch, at most 20 times: a new watcher records nothing before its thread issues the first read, which is the separate problem of #40017. Two earlier drafts are gone. The first checked the lost change directly (a write of the watched file inside a burst of other writes, 10 of 10 lost on a debug build), but the release lanes of CI pass it with or without the fix. The second counted the pool over 100 reloads with two PowerShell runs, which review found too slow.Not changed. When Windows reports that its own buffer overflowed (
nbytes == 0), the watcher still drops those changes and tells nobody. That is #42936.Overlap. Five open PRs carry an equivalent gate inside a larger change: #30644, #35596, #39488, #39820 and #40017 (the hunk in #40017 is the same code). None of them has merged in four weeks to four months. This PR is the gate alone with the cause and a test, and is meant to land first. #40017 then keeps only the start-up arming and the
stopdrain, and #39820 can drop its pending flag commit.Suites run with the fix. Windows debug and Windows release:
test/cli/hot/hot.test.ts,test/cli/hot/watch-many-dirs.test.ts,test/cli/hot/watch.test.ts,test/cli/watch/watch.test.ts,test/cli/watch/watcher-trace.test.ts. Windows debug only:test/bake/dev/hot.test.ts,test/bake/deinitialization.test.ts,test/js/bun/resolve/bun-main-entry-point.test.ts,test/cli/test/test-filter-lifecycle-snapshot.test.ts,test/js/bun/http/bun-serve-html-hot-reload-drop.test.ts. Linux debug:test/cli/hot/watch-many-dirs.test.ts(the new test is skipped).cargo check -p bun_watcher --target x86_64-pc-windows-msvcpasses.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