Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe watcher now uses typed watch-item indices and a dynamic eviction queue. Hot-reload and dev-server consumers use the updated index APIs. A testing binding pads the watchlist, and a hot-reload test covers file operations around and above index 65,535. ChangesWatcher Index and Hot-Reload Flow
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: using a typed usize watchlist index and testing high watchlist positions. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description clearly explains the problem, implementation, scope, tradeoffs, and verification results. It does not use the template headings exactly, but it contains the required information under … |
Full details: Description check
-
Explanation
The description clearly explains the problem, implementation, scope, tradeoffs, and verification results. It does not use the template headings exactly, but it contains the required information under equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Updated 4:05 AM PT - Oct 22nd, 2026
❌ @robobun, your commit 058212b has some failures in 🧪 To try this PR locally: bunx bun-pr 46029That installs a local version of the PR into your bun-46029 --bun |
|
Status #46270 took the crash fix of this PR in part: How the abort was reproduced before #46270 (Linux x64, release build, default limits):
The test is in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked the refactor hunks in src/watcher/Watcher.rs for behavior drift: the dropped parent-eviction loop in remove_at_index was dead at every call site (all three passed &[] for parents), the sort + dedup() pair is equivalent to the old last_item skip in both passes, and the only sentinel-dependent behavior change is the one the description names (an eviction queued at position 65,535 is no longer skipped). The evict_list.push growth goes through Rust's allocator abort rather than bun_core::handle_oom, which is a minor divergence from the repo's OOM convention but not a correctness issue.
Extended reasoning...
The change replaces the u16 watchlist index with a typed usize index across the watcher core and its Linux/macOS/Windows backends, makes the eviction queue a growable Vec, and adds a bun:internal-for-testing hook plus a regression test. It touches no security-sensitive surface. A human look remains worthwhile because the kqueue and Windows paths are type-checked only, and one confirmed finding (an unregistered bindgen schema in the bun-types integration test) is posted inline.
189bf83 to
5f526e5
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the follow-up commit; the bun-types schema listing I flagged earlier is now in place, and this second pass found no bugs. Since this reshapes the watcher's shared index type across all three platform backends and only the Linux/macOS paths can be exercised here, a human look would still be worthwhile.
What was reviewed:
- Every remaining consumer of
WatchItemIndex,remove_at_index,index_of,add_directoryandEntry.watch_index(hot_reloader, dev_server watch/directory_watch_store, Windows/kqueue/inotify backends, WatcherTrace) — no staleas u16/as _narrowing or old 4-argument call sites remain, including in cfg-gated code. flush_evictionsbehaviour: sort-desc +deduppreserves the old removal order, both passes keep the stale-udata bounds guard, and theVecpush inremove_at_indexnever aliases the watchlist column slices held by callers.GenericIndexsemantics:Defaultis 0 (same as the old0u16forWatchEvent),init/getonly debug-assert againstusize::MAX, so a stale kevent udata or index 65,535 cannot trip them.pad_watchlist_for_testing: takeswatcher.mutex, routes capacity throughhandle_oom, and the padded entries carryeventlist_index: -1/Fd::INVALIDso neither inotify nor kqueue can route an event to them.
Extended reasoning...
The diff widens the watcher's watchlist index from u16 to a typed usize GenericIndex across src/watcher (Watcher.rs, INotify/KEvent/Windows backends, WatcherTrace), adapts hot_reloader.rs and the dev-server watch store, replaces the fixed 8096-slot eviction array with a Vec, and adds a bun:internal-for-testing binding plus a Linux/macOS test that places real entries at positions 65,534-65,536 and 65,544. It touches no security-sensitive surface (no auth, crypto, or untrusted parsing), but the Watcher is shared between the watcher thread and the JS thread and the Windows backend is only type-checked, not run. The prior inline finding was resolved by commit 5f526e5 and no CODEOWNERS entry covers these paths; the decision to defer rather than approve rests on the cross-thread core-subsystem scope and the uncompiled Windows path.
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.
The bun-types schema registration from the last round is in place (5f526e5). Beyond the inline finding, I also checked the deleted parent-eviction loop in remove_at_index — all four callers (hot_reloader.rs x2, dev_server/watch.rs, directory_watch_store.rs) passed Kind::File with an empty parents slice, so the loop was dead and its removal is behavior-preserving — and the new pad_watchlist_for_testing binding: it takes watcher.mutex like the inotify snapshot path and add_file, and its Fd::INVALID / eventlist_index: -1 entries are skipped by both the inotify wd lookup and the kqueue re-registration in flush_evictions after a swap_remove.
Extended reasoning...
The change widens the watcher's watchlist index from u16 to a usize-backed GenericIndex newtype across Watcher.rs, the three platform watchers, the hot reloader, and the dev server's directory watch store, replaces the fixed eviction array with a Vec, and adds a test-only bindgen module exposed through bun:internal-for-testing with a new --hot test at positions 65,534 through 65,544. It touches no auth, crypto, or input-parsing surface; the adversarial inputs are kevent udata and inotify wd values, whose bounds guards in flush_evictions and on_file_update survive release builds. The one posted finding is a pre-existing stale-index issue the author also lists as not fixed here; the earlier PUBLISHED_AS finding and the bot's comment-style notes were addressed in later commits. Deferral rather than approval because the watcher core is shared between the JS and watcher threads, the Windows arm is proven by type check only, and the swap_remove ordering in flush_evictions warrants a maintainer's eye.
…6270) ### What does this PR do? Hardening follow-up to #46021. It fixes regressions from #46021 and #44618 that were never released, includes more of the open pull requests that fix a crash, and fixes a few crashes that had no pull request. #### Regressions fixed (none was released) - `spyOn(Bun, "$")`, and the same for `"sql"`, `"postgres"` and `"SQL"`, threw "does not support accessor properties yet". - Async `fs.read`, `fs.write` and `zlib.brotliCompress` on a view over a `WebAssembly.Memory` could lose data or copy far more than they needed. - `node:vm` with `vm.constants.DONT_CONTEXTIFY`: `this` in host accessors, prototype handling, `util.types.isProxy`, `Bun.deepEquals`, `assert.deepEqual` and `console.log` of a context. - `zlib` `stream.flush()` threw for arguments it used to accept, and streams had a new enumerable property. - `Bun.Transpiler` `exports.replace` with the name `"default"`. - `process.env.X = 3000` could store the number once the function was JIT-compiled. - `node:sqlite` and `bun:sqlite` could lose rows at exit. - Dev server: stylesheets shared between pages after a build error. - Shell `cp` of a file onto itself exited 1. - `error.stack`: the frame of `require()` reads `at anonymous (unknown)` again. - `import * as process from "process"` followed by `process.on(...)` threw at load. The EventEmitter methods that the module exports now act on `process`. - `delete res.statusCode; res.end()` in a `node:http` handler threw. It sends 200, as in Node. (from #46128) - `node:tls`, `node:https` and `node:http2`: connection teardown and write callbacks behave as in 1.4.3 and Node again. #### Crash fixes Bundler, parser and CSS: #42286, #46092, #46097, #46177, #46239, #36334, #43024, #40769. `bun test`, `--hot` and `--watch`: #30776, #38206, #38974. Shell: #43183, #37743, #46168, #46207, #34900. `node:http`, `node:http2`, `Bun.serve`, streams: #43449, #45380, #44457, #42197, #42485, #38082, #38124, #46074, #45999, #46045. `node:vm`, native addons, workers: #46129, #46198, #45615, #33412, #38031. Dev server: #42790, #42850, #39488. Other: #35991, #38030, #38586, #40526, #41830, #46029, #46078, #46114, and the bug of #42215. #### Behavior changes - `crypto.randomFill(view, cb)` reports `ERR_CRYPTO_OPERATION_FAILED` if it could not fill the view. It used to report success. - A function registered twice on `process` runs twice, as in Node. (#41830) - Under `--hot`, a reload after an uncaught exception cancels the timers that the failed run created. The new run's timers fire, and so do timers and `fs.watchFile` watchers that earlier runs kept on `globalThis`. The reload also resets the exit code to 0. (#38206) - The parent of a Worker no longer gets an `'error'` event for a failed `Bun.serve` request in that Worker. The error is printed, as on the main thread. - `hash.write()` after `digest()` drops the chunk, as in Node. (#35991) - `http2` over TLS with `server.emit("connection", socket)`: `session.socket` is a `TLSSocket` and a `'connection'` listener fires, as in Node. (#38124) - A `slice()` with no type of a body `Blob` has `type === ""`, as in Node. (#46114) - `util.types.isProxy(globalThis)` is false, as in Node. - A write on a `node:net` or `node:tls` socket that is still pending when the connection closes calls back with `ECANCELED`, as in Node. - `HTMLRewriter.on()` throws for a selector that is nested too deeply. - `Bun.Image`: `maxPixels` also bounds PNG metadata, `metadata()` of a GIF reports the size that gets decoded, and a path with a NUL byte throws. (#40526) - `bun build --no-bundle` with the `base64` or `dataurl` loader prints an error. (#36334) - Bundled output for a default-exported function or class expression changes to `var file_default = function name() {}`, as in esbuild. `.name` is unchanged. (#46097) - A Proxy in the prototype chain of a `node:vm` global is allowed. jsdom 30 with `runScripts` needs this. - `bun:sqlite` commits a write that `iterate()` left open when the main thread exits. An explicit `BEGIN` is still rolled back. - When a Worker stops while a native addon's async work is still running, the addon's finalizers run after that work is done, as in Node. - Shell: `mv -n` and `cp -n` don't overwrite, and `cp` reads `R`, `v` and `n` anywhere in a flag group. #### Performance - Ordinary `node:vm` contexts run at the same speed. With `DONT_CONTEXTIFY`, stores to globals and reads of inherited names in a hot loop cost 10 to 20 % more than on `main`. - A `process.env` write costs 20 to 35 % less than after #46021. - An async read into a `WebAssembly.Memory` view looks through the live memories: about 1.5 µs more with one memory. #### Not included - Taken in part, so these should stay open for the rest: #36334, #38974, #39488, #40526, #40769, #42485, #45615, #45999, #46029, #46092, #46114, #46168, #46207, #46239. - Needs a WebKit change: #34655 #39204 #40170 #40253 #41587 #41900 #42699 #42897 #42903 #43084 #43813 #44518 #44949 #45770 #45804 #45827. - #44723 (covered by #46097), #41675, #42753, #38762, #41475, #46016. ### How did you verify your code works? - Every item was reproduced first on release builds of `main`, of its parent and of 1.4.3, and compared with Node v26 where Node has the API. - Every fix has a test that fails on `main` and passes on a debug + ASAN build of this branch, except where that isn't practical (huge inputs, code that only runs on Windows or macOS). - The regressions were checked on grids of calls against the build before #46021 and against 1.4.3. - Bun's own test suites and Node's `test-vm-*`, `test-http2-*` and `test-zlib-*` files for the areas touched.
…ast 65,535 `WatchItemIndex` is `bun_core::GenericIndex<usize, WatchItem>`, not a `u32` alias. `init` makes a position and `get_usize` reads one, so an `as` cast cannot narrow it. "No entry" is `None`, not `u32::MAX`. The eviction queue is a `Vec`, not an inline `[u32; 8096]`. `remove_at_index` loses three parameters that no caller used. `padWatchlistForTesting` in `bun:internal-for-testing` appends entries that no event reaches. The new test in test/cli/hot/watch-many-dirs.test.ts uses it to put real files at positions 65,534, 65,535, 65,536 and 65,544. It checks a reload after a write, a replace and an eviction at those positions.
be48da6 to
058212b
Compare
robobun
changed the title
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.
This push adds the watchlist_testing.bind.ts entry to PUBLISHED_AS, so the bun-types schema check now matches. Beyond the inline nit, I also checked: the directory-parent loop deleted from remove_at_index was dead on the base (all four callers passed Kind::File and an empty parents slice, and it pushed a hash as an index); padWatchlistForTesting takes the same Watcher.mutex that add_file and the inotify watchlist snapshot take; and the >= len guards on event.index in hot_reloader.rs, dev_server/watch.rs and both flush_evictions passes are plain runtime checks, so the debug-only asserts in GenericIndex are not the only bound.
Extended reasoning...
The diff replaces the u32 watchlist index with a nominal GenericIndex<usize, WatchItem> across Watcher.rs, the inotify/kqueue/Windows loops, hot_reloader.rs and the dev server, makes evict_list a growable Vec with sort+dedup, and adds a bun:internal-for-testing binding that pads the watchlist under the watcher mutex. No security-sensitive surface is touched. The only remaining finding is a stale test comment (the base already widened the index to u32 in a950fc5, so the described panics do not reproduce on base); the kqueue and Windows arms are verified by type-check only in this checkout, which is why a human look is still worthwhile rather than an approval.
| // The position of a watchlist entry was a u16. A change to the entry at | ||
| // position 65,535 aborted the process with "index out of bounds: the len is 0 | ||
| // but the index is 0" when no entry before it changed in the same batch, and a | ||
| // change to a later entry aborted it with "TryFromIntError(PosOverflow)". An | ||
| // eviction of the entry at 65,535 was dropped. |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a test whose comment describes a crash the base branch no longer has, so the test cannot fail on the base for the reason it states. The base already widened WatchItemIndex to u32 in a950fc5 (Watcher.rs:51), so neither the "TryFromIntError(PosOverflow)" nor the "index out of bounds" abort at 65,535 is reachable there, and the 65,535 eviction skip no longer exists either. Fix: state what the test actually guards on this base (the typed index, the growable eviction queue, the 8,097th-eviction panic) and, if a defect of the base is meant to be the regression target, add a fixture that fails on base plus the binding, which the current fixture does not.
Why this was flagged
The base commit 2a5cbbc has pub type WatchItemIndex = u32; at src/watcher/Watcher.rs:51 and const NO_WATCH_ITEM: WatchItemIndex = WatchItemIndex::MAX; at :54; git blame shows the u16-to-u32 widening landed in a950fc5 before this branch. The comment at test/cli/hot/watch-many-dirs.test.ts:261-265 says the position was a u16 and that positions 65,535 and 65,536 aborted with the quoted panics; with u32 the inotify try_from(idx).unwrap() at base INotifyWatcher.rs:448 only fails past 4,294,967,295 and the u32::MAX sentinel at :494 never collides with 65,535. Therefore on the base plus the new padWatchlistForTesting binding the fixture at positions 8, 65_534, 65_535, 65_536 and 65_544 passes, so the test does not demonstrate the change it claims to guard; it only fails under USE_SYSTEM_BUN=1 because the binding is missing. The consequence is a misleading comment and a regression test that does not pin the described behaviour.
Verification: The base commit 2a5cbbc already has pub type WatchItemIndex = u32; (base src/watcher/Watcher.rs:51); the widening landed in a950fc5 before this branch. On the base, INotifyWatcher.rs:448 WatchItemIndex::try_from(idx).unwrap() is a usize->u32 conversion that cannot overflow at 65,536, so with the binding added the test passes both before and after the Rust change.
|
I ran this commit against the real-file case, without the test binding. Setup: 65,700 files loaded with
|
This branch has not been deployed
1 participant
Problem
WatchItemIndex:u16tou32) and left the PR open for the rest. Main no longer aborts at watchlist position 65,535 or later withTryFromIntError(PosOverflow)orindex out of bounds: the len is 0 but the index is 0.u32::MAXmeans "no entry" in three places, and seven sites narrow ausizewithas.Fix
WatchItemIndexisbun_core::GenericIndex<usize, WatchItem>.ascannot make or read one, so a narrowing does not compile.None. The eviction queue is aVec.remove_at_indexloses three dead parameters.test/cli/hot/watch-many-dirs.test.ts, with the test bindingpadWatchlistForTesting: a write, a replace and an eviction at positions 65,534 to 65,544.hot.test.ts,watch.test.ts,bake/dev/bundle.test.ts,rust:check-all.Background
--hot,--watchand the dev server watch. Events and the eviction queue carry an entry's position.GenericIndex<I, M>wraps the integerI, and the markerMmakes it a distinct type.u32: eachusizeproducer needs a cast again.Downsides
WatchEventis 16 B (was 8 B): the event queue grows 1,024 B, theWatchershrinks 32,368 B. First eviction: +1 allocation.Notes
What #46270 took, and what is left
pub type WatchItemIndex = u16tou32andentry_id as u16toentry_id as bun_watcher::WatchItemIndex. It lists this PR under "Taken in part, so these should stay open for the rest".u32each abort needs 4,294,967,295 entries. So on main no abort is left in reach, and this PR changes no behavior that a test can show.Option, theVec, the test binding and the test.u32::MAXmeans "no entry" atWatcher.rs:54,INotifyWatcher.rs:494andWindowsWatcher.rs:500. The sevenascasts arehot_reloader.rs:1197,KEventWatcher.rs:48,WindowsWatcher.rs:466andWatcher.rs:682,:715,:722,:923.Watcher.rs:949, in a branch that no caller reaches, casts a path hash to a position.Reach of the aborts that #46270 fixed (releases 1.4.0 to 1.4.3)
@intCastand ani32value for "no event yet".fs.inotify.max_user_watches). A transpiled module also holds a descriptor, and bun stops keeping descriptors at half the open-file limit (about 2,040 modules at a limit of 4,096).u16main, on a 16 GB host with default limits:bun --watch --check entry.tsin a project of 66,000 TypeScript files. Sincebun check- a TypeScript type checker builtin to Bun #44361 (v1.4.3) the watcher takes each file the type checker reads, with no descriptor: 66,069 entries, 74 descriptors. An edit to the file added last ended withSIGABRTandTryFromIntError(PosOverflow).bun --hotwith 65,540require("x.file")calls: an in-place write at position 65,535 ended with theindex out of boundspanic.index out of boundsabort needed a file outside the root, or a directory that is not watched.The test
padWatchlistForTesting(length)is inbun:internal-for-testing. It appends entries that no event reaches until the watchlist haslengthentries. The test puts real files at positions 8, 65,534, 65,535 and 65,536, and a transpiled module at 65,544.BUN_WATCHER_TRACEpath of each event.panic: index out of bounds: the len is 0 but the index is 0at position 65,535.Measurements (main 2a5cbbc against this branch, x64 Linux, release builds with the same flags)
size_of:WatchEvent8 to 16 B, so the 128-slot queue is 1,024 to 2,048 B.WatchItemIndex4 to 8 B.Watcher35,936 to 3,568 B.DirectoryWatchStore::Entry16 to 32 B.sizeofbun-profile): text 107,440,881 to 107,442,992 B. Data and bss are equal. The stripped binary is 107,587,296 B in both. The three functions of the test binding are 1,087 B of the text (nm -S)..fileimport outside the project root (one event, 1,000 changes): 1,320.4 to 1,324.5 instructions, 259.2 to 258.4 branches. Runs of one build differ by 0.2, and one run by 1.2..fileimport (an eviction and a new entry, 200 changes): 1,841.6 to 1,848.0 instructions, 357.8 to 354.6 branches. Runs of one build differ by 16.mi_malloccalls for each change: 1, 2 and 2, in both builds.mi_zalloc,mi_reallocandmi_malloc_aligned: 0 in both.Vec. The counts above start after 20 changes, so they do not show it.read, oneppolland onewritefor each event.perf_event_opencounters on the watcher thread, hardware breakpoints on the four allocator functions,size,nm -S.perf,bpftrace,straceandbloatyare not on the machine.Other platforms
rust:check-allpasses on 12 targets.Behavior changes against main (none has a known route)
add_directoryreturnsNonefor a directory outside the project root. It returnsu32::MAXon main, andDirectoryWatchStorelater queues that value for eviction. A release build skips it. A debug build failsdebug_assert!(index != NO_WATCH_ITEM).flush_evictionsremoves duplicates withVec::dedupafter the sort. The order of removal is the same.Order with open PRs on the same lines
Watcher.rs.evict_listtoVecchange. Remove dead code from the bundler, the resolver, bun_css, the Valkey client, and six smaller areas #45723 has the sameremove_at_indexparameter cut. Those hunks drop out of them on a rebase.WatchItemIndex::init(position)makes a position,.get_usize()reads one, andNoneis "no entry".Not fixed here (older than this change, and no issue tracks them)
add_fileandadd_directoryknow a path by a 32-bit hash. A second path with the same hash gets no entry. The expected rate of such a pair is 39% for 65,540 paths.DirectoryWatchStorekeeps a position after the batch that produced it. Aswap_removemakes it stale.index_ofis a linear scan for each added path. The inotify loop scans the watch descriptors for each event.flush_evictionsdoes not callinotify_rm_watch.--hothas a test at these positions.--watch,bun test --watch,bun build --watchand the dev server use the same type and the same inotify loop.Self-review
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