[robobun]

Problem

• --watch, --hot and bun test --watch never reload an imported file whose path only holds the letters node_modules, as in a folder named node_modules-cleaner. A fuzz run found it, no user report.
• Six watcher gates test strings::contains(path, b"node_modules"): RuntimeTranspilerStore.rs:906, :933, jsc_hooks.rs:3367, :3478, Watcher.rs:788, :974.

Fix

• bun_paths tests for a path component named node_modules or node_modules.nosync. A real node_modules stays unwatched.
• ImportWatcher::add_file holds the rule for modules, Watcher::is_eligible_directory for directories. A source lint bans the bare test.
• Verified: test/cli/watch/watch.test.ts, 35 new cases fail on main. Also test/cli/hot/, test-changed.test.ts. Self-reviewed: 20 concerns raised, 19 addressed. Rejected: leaving an answered review thread open (Notes).

Background

• ImportWatcher is the handle of --watch and --hot on the Watcher. The loader offers it each file by real path. The Watcher also watches each file's directory.
• iCloud Drive users keep the store as node_modules.nosync behind a link named node_modules. Main skipped it by the letters.
• Considered: the predicate at each site (six copies), a resolver verdict for the linked store (every process pays).

Downsides

• A store linked as node_modules -> node_modules.noindex is watched (main skipped it): at 200 packages and 256 descriptors a later project file reloads in 9 of 16 runs. node_modules -> store is watched on main too.
• Linux: an entry file outside the working directory, in a path with the letters, no longer reloads after a save by rename, like other folders on main. watcher: keep no descriptor for a watched file outside kqueue #39571 removes the cause.
• Watch mode: one more path scan for a file outside the root or a file-loader import, one direct call per ESM module. Binary: +1,682 bytes.

Notes

How it was found. A fuzz run, not a user report. A second fuzz run on this PR found the two cases that an earlier head lost against main: the store behind node_modules -> node_modules.nosync (fixed here) and the save by rename outside the working directory (below, not fixed here). Searches of the tracker find no issue for this symptom. The nearest ask is #42702 (closed): it asks to "skip anything under a node_modules path segment ... as the docs describe". docs/runtime/watch-mode.mdx:108 says that Bun watches "all imported source files (excluding those in node_modules)".

Repro. index.js imports ./src/util.js. The script rewrites that file after the first print and waits for the second print. On bun 1.4.3-canary.1 (367d939) and on main a8b5e92:

--watch cleaner/src/util.js                 | reloaded
--watch node_modules-cleaner/src/util.js    | NOT RELOADED in 12 s
--watch cleaner/src/list-node_modules.js    | NOT RELOADED in 12 s
--hot cleaner/src/util.js                   | reloaded
--hot node_modules-cleaner/src/util.js      | NOT RELOADED in 12 s
--hot cleaner/src/list-node_modules.js      | NOT RELOADED in 12 s

With this change all six lines print reloaded.

What main does in such a folder, by mode and file.

• An import, no --check: never reloads. This is the bug.
• An import under --watch --check: reloads on main. The type checker adds each file that it reads through add_file_by_path_slow, with its own component rule (check_command.rs:608).
• The entry file: reloads on main after a write in place, because it also goes through add_file_by_path_slow. On Linux under --hot, main loses the entry when the path is absent at the moment of the reload (renamed away or removed, then written again): 0 of 3 on main, 3 of 3 here, 3 of 3 in a folder with a harmless name. The directory was not watched, so nothing saw the file come back. On kqueue the code defers that reload to a directory event (hot_reloader.rs, the is_waiting_for_dir_change branch). That was read, not run.

The two forms. Both look for the letters with strings::index_of, so a path without them costs one search, as before. On a hit, the byte before must be a separator or the start of the path.

• File form, path_is_inside_node_modules: the byte after must be a separator. /p/node_modules/foo.js is inside. A file named node_modules is not.
• Directory form, dir_is_or_is_inside_node_modules: the byte after must be a separator or the end of the path. The resolver hands the watcher /p/node_modules/ in most cases, and /p/node_modules when the first read of that directory is a relative lookup that fails (require("./node_modules/x.js"), traced with gdb).
• Separators come from Platform::AUTO: / everywhere, \ also on Windows. On POSIX a backslash is a byte of a file name. The private scan takes the platform as a parameter, so the unit table runs its Windows rows on every host.
• Both forms also accept a component named node_modules.nosync. The test for the suffix runs only after the letters are found and the component does not close.
• NODE_MODULES_NEEDLE, path_contains_node_modules_folder and Path::is_node_module() are not changed.

Where the rule lives now.

• ImportWatcher::add_file (src/jsc/hot_reloader.rs): its four callers are the loader sites. It returns FdOwnership::Caller for a package file, before the watcher mutex.
• jsc_hooks.rs, file-loader arm: keeps its own call, because on kqueue it opens a descriptor before add_file and must not open one for a package file.
• Watcher::is_eligible_directory: on_maybe_watch_directory calls it. The package test runs first, as it did in on_maybe_watch_directory, so a resolver miss under node_modules is still rejected by one scan.
• Not given a rule, on purpose: add_file_by_path_slow. It has three producers and each one decides: the entry file (VirtualMachine.rs:4659), each file that --check reads (VirtualMachine.rs:4685, rule at check_command.rs:608) and the seeding of test --changed (test_command.rs:2438). Also directory_watch_store.rs:360, where the dev server adds a directory itself.

Behaviour changes besides the fix.

• A hoisted store behind a link named node_modules. The loader sees only the real path of a package, <target>/pkg/.... Paths under .bun/<pkg>/node_modules/, .pnpm/<pkg>/node_modules/ and nested node_modules keep a component and stay unwatched.
  • node_modules -> node_modules.nosync (the iCloud Drive layout) is a package store again for --watch, --hot and bun test --watch: they register no file and no directory of it, as on main. An earlier head of this PR (2e865b7) registered all of them. Why it matters: on kqueue each watched file holds one descriptor, the cost that Stop keeping resolver file descriptors in watch mode #42714 removed for bun --watch watches node_modules despite docs saying it doesn't (isolated linker) #42702 and that macOS: bun --watch retains high FDs and breaks piped posix_spawn with EBADF #40907 reports. bun build --watch registers the files of the store on purpose and no directory, as on main (the earlier head also took the directories).
  • Not changed: the dev server and --watch --check have their own test for /node_modules/ (BundleV2::should_add_watcher, check_command.rs). They register the files of a .nosync store on main and here. Path::is_node_module() (the macro guard and others), coverage and test discovery also decide by /node_modules/.
  • A store under another name that holds the letters is watched: node_modules.noindex, node_modules.nosync.noindex, .node_modules.nosync, node_modules_linux. Main skipped these by the letters, so this is new against main. A target without the letters (node_modules -> store) is watched on main and here.
  • A reload of a project file is lost with no message when the watcher runs out of one of two budgets. (1) Descriptors: past half of RLIMIT_NOFILE the reader closes each file (need_to_close_files, src/resolver/lib.rs), and the loader only offers a file that has an open descriptor (watcher: watch a file read from disk even when its descriptor was closed early #45594). (2) inotify watches: inotify_add_watch fails at fs.inotify.max_user_watches and the loader sites print nothing (watcher: warn once when a watch cannot be added #41489). Bun raises the soft descriptor limit to the hard limit, so the first budget binds at a low hard limit and the second on a machine with a high one.
  • Two routes keep such a store unwatched today, measured on main and here (0 store descriptors, 3 inotify watches): a link target that ends in a real component (node_modules -> deps.nosync/node_modules), or --preserve-symlinks, which also stops the reload of workspace packages that are linked into the store.
  • The name rule is the author's pick, not a maintainer's. Question for a maintainer: is the one name enough, or does the watcher owe every linked store a test on the file system? That test was weighed: when a path holds the letters but no component, ask whether a sibling link named node_modules points at that directory. It puts a verdict cache, a second lock and a realpath call into the shared Watcher, and it does not see a target without the letters. Include/exclude config for bun run --watch #5278 asks for an exclude setting for watch mode.
• Linux, a save by rename of a file outside the working directory. cd app && bun --watch ../node_modules-tools/server.js: main reloads 3 of 3, this branch 0 of 3. The control ../tools/server.js is 0 of 3 on both. --hot gives the same numbers. Writes in place are 3 of 3 everywhere, and ../node_modules/tools/server.js is 3 of 3 on both. The earlier body recorded the same loss for an import outside the working directory under --watch --check. On main the loader refused the path, so the file had only the path watch of add_file_by_path_slow, which holds no descriptor and sees the replace. Now the loader's descriptor is stored, as for every other file, and it pins the old inode.
  • Not fixed here, on purpose. The fix that a design review picked is the rule of watcher: keep no descriptor for a watched file outside kqueue #39571: store no descriptor for a watched file outside kqueue. It changes every watched file on Linux and Windows and re-anchors the tests that pin a stored descriptor, so it is a second concern. watcher: keep no descriptor for a watched file outside kqueue #39571 is being refreshed so that it can merge before this PR. Its commits can also be folded in here if a maintainer prefers that.
  • A rule for the entry file alone was weighed and dropped. An import outside the working directory loses the same save on main and here: ../shared/lib.js by rename reloads 0 of 3 under --watch and --hot on both, and 3 of 3 in place.
• The dev server and bun build --watch get the directory watch in a folder whose name holds the letters. On Linux that watch is how they see a save by rename.
• Windows: a file outside the project directory whose path holds the letters now reaches the existing warning File ... is not in the project directory and will not be watched.
• Not changed: a project that lives inside a real node_modules/<pkg>. Its entry reloads, its imports do not. A rule relative to the root is its own decision.

Tests.

• test/cli/watch/watch.test.ts, 35 cases that fail on a debug build of main:
  • 18: {--watch, --hot, test --watch} x {letters in the project folder, in the file name, in a directory name} x {import, require}.
  • 10 in a folder named node_modules-cleaner: a file-loader import, a save by rename of an import and a module that did not parse at first (each under --watch and --hot), an edit to the test file itself (test --watch), a save by rename under bun build --watch, an entry file renamed away and written again (--hot, Linux), and the descriptor of a directory in which a lookup failed.
• 7 of the 35 are the block a store behind the link node_modules -> node_modules.nosync. Not on Windows: the descriptor listing has no Windows form, and the unit table covers both separators.
  • 6: {--watch, --hot, test --watch} x {import, require}. A case loads a package from behind the link and a project file named src/list-node_modules.js. It asserts that the process holds no descriptor below node_modules*, and then that an edit to the project file reloads. On main the 6 fail at the reload. On the earlier head the 6 fail with four store paths held.
  • 1: bun build --watch holds no directory of the store and rebuilds a project file in src/node_modules_helpers/ after a save by rename. On main it fails at the rebuild. On the earlier head it fails with two store directories held.
• Kept contract, passes on main and here: --hot sees no event under node_modules. It reads BUN_WATCHER_TRACE and has no platform skip, so it is the case that needs a true verdict on Windows. It loads and rewrites a file of a package and a module that is a file of node_modules itself, and it makes a lookup fail in node_modules first.
• The cases are not concurrent. With six debug builds started together a case took up to 3.6 s, too near the 5 s default. Alone a case takes under 0.8 s.
• The three saves by rename skip Windows: a rename over a watched file fails there with EPERM (Windows: --watch / --hot prevents atomic replacement of watched files with EPERM #41045), and that watcher follows paths. The entry case is Linux only, because it waits for the error of the reload that fails at once there.
• On Windows each case repeats its edit, at doubling intervals, until the reload shows. That watcher loses a change that lands before its thread first calls ReadDirectoryChangesW (WindowsWatcher.rs, prepare), and a case edits the file as soon as the first output arrives. Other platforms make one edit.
• test/internal/source-lints/watcher-node-modules-needle.test.ts: no search call with the literal b"node_modules" as its needle in src/watcher/, src/jsc/ and src/runtime/jsc_hooks.rs. It fails on main with exactly the six gates.
• cargo test -p bun_paths: a verdict table for both forms and both platforms. The test module gets a scalar stub for highway_memmem, next to the other highway stubs.
• Mutation checks on debug builds:
  • bare contains in ImportWatcher::add_file: 26 of the 28 fail (all but the two directory cases).
  • bare contains in the file-loader arm: its 2 cases fail.
  • bare contains in is_eligible_directory: 5 fail (the directory of a failed lookup, the three saves by rename, the entry case).
  • the file form used for directories: the trace case fails.
  • a predicate that always answers "not inside": the trace case and the three descriptor cases fail.
  • the predicate without the .nosync suffix: the unit table fails at /p/node_modules.nosync/pkg/a.js, and the 7 store cases fail.
• A test that creates a missing import and expects a reload is not possible on main: a created file triggers no reload in any folder (watch: reload when a missing import appears, keep scanning after a resolve error #41461 is open for that). The resolver's directory watch is pinned by its descriptor.

Measurements. Release builds of main a8b5e92 and of this branch, Linux x64, unless a line says debug. strace, valgrind, perf and bloaty are not installed in the container and it has no network, so the counts come from gdb breakpoint tallies, a small ptrace counter, perf_event_open and size.

• node_modules scans under --watch (debug builds, tally of highway_memmem calls with that needle): 2,001 package modules 2,004 (main 2,004). 2,001 project modules 4,004 (main 4,004).
• No --watch or --hot: 0 scans in 2,002 module loads (main 0).
• Resolver misses under node_modules, require("express") under --watch (debug): 134 package scans, 0 top-level-dir scans (main 134, 0).
• File-loader import under --hot (debug): 2 scans per accepted file (main 1). A package file: 1 (main 1). The kqueue-only open for a package file is not measured here. The test still precedes it.
• is_eligible_directory scans per new file (debug): outside the root 2 (main 1). A package file under bun build --watch 1 (main 2). A project file 2 (main 2).
• Syscalls for 2,001 modules under --watch, minimum of 5 runs, openat/read/close/inotify_add_watch: package modules 2032/4030/2029/2 (main 2032/4030/2029/2). Project modules 2035/4031/30/2004 (main 2035/4030/30/2004). Runs of one build differ by up to 11 calls.
• The same project in a folder named node_modules-proj: 2,004 inotify_add_watch (main 0).
• inotify watches once settled: control 4, node_modules-cleaner 4, list-node_modules.js 4, node_modules_helpers 4 (main 4, 1, 2, 2). A real node_modules/pkg: 2 watches, 8 descriptors, 0 under node_modules (main the same).
• Out-of-line ImportWatcher::add_file entries for 2,001 package modules: 0. The loader has it inlined.
• Instructions per scan, scratch bench with the helper body and glibc memmem, perf_event_open counter: a path without the letters +13 to +14, a package path +37 to +39, letters inside a longer name +155 to +159 (one more search).
• Release binary: +768 bytes text (size), data and bss equal.

Measurements for the node_modules.nosync clause. Linux x64, 12 cores, release builds (LTO) of main a0cc096, of the earlier head 2e865b7 and of this head, unless a line says debug. strace, perf, valgrind and bloaty are not installed, so the counts come from gdb (stepi, breakpoint tallies), a ptrace counter, nm, objdump and size. The module fixtures have 2,000 modules, one per package directory, plus the entry.

• The reported layout, node_modules -> node_modules.nosync, 200 packages, ulimit -n 256, then an edit of the project file d.js: 9 descriptors, 0 under the store, 3 inotify watches, d.js reloads 8 of 8. Main the same, 6 of 6. The earlier head, 8 runs: 128 to 151 descriptors, 120 to 142 under the store, 122 to 145 watches, reloads 4 of 8 (a debug build of it: 133 to 135 descriptors, 0 of 4). The loss is a race there: d.js is read before or after the descriptor cut-off.
• The same store with no race: an inotify budget of 150 watches for the user, descriptor limit 4096, d.js loaded by a late import(). This head uses 3 watches and reloads. The earlier head uses all 141 free watches, prints nothing and does not reload. With the store named node_modules.noindex this head behaves like the earlier head, and main uses 3 and reloads.
• Other link names (node_modules.noindex, node_modules.nosync.noindex, .node_modules.nosync, node_modules_linux), 200 packages, ulimit -n 256, 4 runs each: 128 to 152 descriptors, d.js reloads in 9 of 16 runs (main 9 descriptors, 4 of 4 in one run each). node_modules -> store: 133 to 151 descriptors and 3 of 5 here, 130 to 151 and 4 of 5 on main.
• A workspace link inside the store (node_modules.nosync/@s/ws -> ../../packages/ws): 5 paths held and 5 watches, packages/ws and its file among them (main the same, the earlier head 11 and 11).
• bun build --watch, 2 packages behind the link: 11 descriptors and 5 inotify watches (main 11 and 5, the earlier head 13 and 7).
• Syscalls for 2,001 modules behind the link under --watch, openat/read/close/inotify_add_watch: 6033/4020/6029/2 (main 6031/4020/6028/2, the earlier head 7054/4019/5003/2050). In a real store: 4029/4020/4026/2 (main 4028/4019/4025/2).
• node_modules searches for 2,001 module loads under --watch (debug builds, gdb tally of highway_memmem with that needle): project modules 4,002, a real store 4,002, the store behind the link 4,002 (the earlier head 6,072, it stops at the descriptor cut-off). No watcher: 0.
• Directory gate, instructions per call (gdb stepi over get_resolve_watcher::wrap, first instruction to return): a directory without the letters 1421 (the earlier head 1417, main 1415, each with the watch registration). In a real node_modules 170 (173, main 150). In node_modules.nosync 170 (1537 with the registration, main 135). In node_modules-cleaner 1539 (1534, main 135: main refuses it, which is the bug).
• Inlining (nm, objdump): the predicate has one out-of-line copy, as on the earlier head (250 bytes, was 183). One thing changed: LLVM no longer inlines the bun_jsc instance of ImportWatcher::add_file into the transpile job, so each ESM module that is offered in watch mode pays one direct call (17 instructions for the call, the prologue and the epilogue, plus the argument moves). Two other shapes of the clause were built and measured. A #[cold] helper for the suffix: +256 bytes, but the predicate falls out of line at the directory gate (+16 to +21 instructions per call). The suffix stripped before the close test: +1,024 bytes, +11 instructions for a real store. Both keep the same out-of-line add_file.
• Release binary text (size): main 109,016,628, the earlier head 109,017,396 (+768), this head 109,018,310 (+914 for the clause, +1,682 against main). Data and bss are equal.
• A scratch program compares the predicate with the rule in words (a component named node_modules or node_modules.nosync, followed by a separator in the file form) on 12,000,000 generated paths, both forms, both platforms: no difference.

The same bare test in other places. Main has 14 search calls in src/ whose needle is the bare b"node_modules". The six watcher gates are removed here. The other eight stay:

• Defects, each handed to its own change: src/runtime/cli/package_manager_command.rs:889, :896, :945 (bun pm ls --all panics with index out of bounds for a dependency named like node_modules-x, and needs no search at all), src/bundler/linker_context/MetafileBuilder.rs:884 (--metafile-md counts src/list-node_modules.js as a node_modules file), src/js_parser/visit/visit_expr.rs:2458 (a prefix test by letters).
• On purpose, must stay: src/lint/react_compiler/rule.rs:104, src/lint/react_compiler/program.rs:1052 and src/lint/driver/lint.rs:287. They match what oxlint, the React Compiler plugin and ESLint 8 do.
• src/resolver/resolver.rs:5615 and src/install/PackageManager/patchPackage.rs:241 test a half-anchored node_modules<SEP>. Other features, not changed.
• BundleV2::should_add_watcher, BunTest__shouldGenerateCodeCoverage and check_command.rs:608 already test a component.

Sequencing with open PRs. Six open PRs move or add a bare test on these lines. None of them fixes this bug. The new lint fails each of them after a rebase until the line is changed:

• watcher: watch a file read from disk even when its descriptor was closed early #45594: add_module in hot_reloader.rs. Delete the bare clause, and use path_is_inside_node_modules for its file-loader test.
• watcher: watch a file before it is read, not after it is parsed #42749: RuntimeTranspilerStore.rs and jsc_hooks.rs, in front of its new entry point. Put the component test inside that entry point.
• watch: reload when a missing import appears, keep scanning after a resolve error #41461: is_watchable_directory in Watcher.rs. Call is_eligible_directory.
• watcher: accept the real path spelling of the project root on Windows #39832: Watcher.rs. Keep dir_is_or_is_inside_node_modules when it rewrites the root test.
• watcher: report a file that was replaced by rename on Linux #36248 (INotifyWatcher.rs, file_was_replaced) and Watch files loaded through runtime plugin onLoad callbacks in --hot and --watch #33093 (VirtualMachine.rs, in front of add_file_by_path_slow): use path_is_inside_node_modules.
• The message of the lint names the helper to use, so no note is posted on those PRs.
• The idea that ImportWatcher owns the runtime rule is from watcher: watch a file read from disk even when its descriptor was closed early #45594. The fold of on_maybe_watch_directory is also in watcher: accept the real path spelling of the project root on Windows #39832 and watch: reload when a missing import appears, keep scanning after a resolve error #41461.
• watcher: count node_modules from the package folder that holds the start folder #46910 is stacked on this branch. It rewrites the predicates into one scan (node_modules_component_end, NodeModulesScope). Its unit tables pin /p/node_modules.nosync/pkg/a.js as not inside, and project_root("/p/node_modules.nosync/pkg") as None. After a rebase the first row is inside. Whether the name also counts for project_root, and for the coverage gate of bun test --coverage: count the files of a project that is a package folder of node_modules #46909, is a decision of those PRs: here it is a watcher rule only.
• watcher: keep no descriptor for a watched file outside kqueue #39571 (no stored descriptor outside kqueue) is the fix for the save by rename outside the working directory. It should merge first, or its commits get folded in here.

Self-review. Ten concerns survived it. What was done:

1. No user report: stated in Problem.
2. The class is half closed: the eight other lines are listed above, the five defect lines are handed off.
3. Copies of the bare gate wait in open PRs: the source lint, and the sequencing list above. The review also asked for a note on each of those PRs. Not posted: the lint fails there with a message that names the helper.
4. The account of add_file_by_path_slow was wrong: the comment is one line now and the three producers are listed.
5. Nothing on Windows needed a true verdict: the trace case runs there, and the unit table runs its Windows rows on every host. The review's first choice, to export the predicates to JS tests and delete the Rust table, would add two host functions for a test. Not taken.
6. A case lost against main on Linux (save by rename outside the working directory): in Downsides, with the open PRs for the defect.
7. The --hot entry file was fixed with no test: the entry case and the two runtime save-by-rename cases.
8. The .nosync bullet did not name the layout or the ceiling: done, above and in Downsides.
9. A project below a real node_modules: listed as not changed, handed to its own change.
10. Multi-line comments in src/: each is one line now.

Second self-review, of the node_modules.nosync commit. Ten concerns survived it. Nine are done, one is rejected:

1. The body and an earlier reply said that main watches a store linked under any other name. That is wrong for a name that holds the letters: corrected in Downsides, in the Notes and in the reply.
2. The clause closes the store in three modes only: the result per mode is above, and the dev server and --watch --check are listed as not changed.
3. The case rested on the reload that is lost at ulimit -n 256, a race: it now rests on the registrations (held descriptors on kqueue, Stop keeping resolver file descriptors in watch mode #42714), and a run with no race is in the measurements.
4. One silent cut-off was named and there are two: both are named, with watcher: watch a file read from disk even when its descriptor was closed early #45594 and watcher: warn once when a watch cannot be added #41489.
5. The row for the earlier head was a debug figure beside release rows: every row is a release figure now, and a debug figure says so.
6. The directory rule had no end-to-end case: the bun build --watch case.
7. "Registers no file and no directory" held only for the three loader modes: the sentence is limited to them.
8. Two caller comments still named the one folder: they name the concept now.
9. The unit table passed with the byte compare of the suffix deleted: the row /p/node_modules-report/src/util.js fails that change. The two rows that pinned other names as not a store are gone, because that is the open question.
10. Rejected: leave the review thread open for a maintainer. The layout that the thread reports is fixed, so the thread is answered and resolved. The open question is in the Notes above and in Downsides.

Suites run on the debug build. test/cli/watch/watch.test.ts (53 pass, 1 skip), watcher-trace.test.ts, test/cli/hot/watch.test.ts, watch-many-dirs.test.ts, hot.test.ts (the two "does not take a promise of the program's" cases fail about 3 times in 8 on a debug build of main too), test/cli/test/test-changed.test.ts, test/js/bun/resolve/bun-main-entry-point.test.ts, cargo test -p bun_paths, and the source lints byte-search, comment-cop and the new one. bun run rust:check-all passes for x86_64-pc-windows-msvc, aarch64-apple-darwin and x86_64-unknown-freebsd.

---

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/watch/watch.test.ts

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: oven-sh/bun
• Branch: robobun/c6f8fb9b/watch-node-modules-segment
• File: .coderabbit.yaml
• Commit: 9c6c8ee
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 101536df-2a86-467d-b25a-b1f426811455

Commits

Reviewing files that changed from the base of the PR and between 179664e and 9c6c8ee.

Files selected for processing: 8

• src/jsc/RuntimeTranspilerStore.rs
• src/jsc/hot_reloader.rs
• src/paths/resolve_path.rs
• src/paths/string_paths.rs
• src/runtime/jsc_hooks.rs
• src/watcher/Watcher.rs
• test/cli/watch/watch.test.ts
• test/internal/source-lints/watcher-node-modules-needle.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

Watcher path checks now distinguish node_modules path components from names that contain the same letters. File and directory watcher paths use these checks, and CLI tests cover watch, hot-reload, test-watch, and build-watch behavior.

Changes

Watcher path handling

Layer / File(s)	Summary
Path component detection src/paths/string_paths.rs, src/paths/resolve_path.rs	Added file and directory checks that match node_modules as a complete path component. Added Posix and Windows tests for component boundaries and separators.
Watcher registration and descriptor ownership src/runtime/jsc_hooks.rs, src/watcher/Watcher.rs, src/jsc/hot_reloader.rs, src/jsc/RuntimeTranspilerStore.rs	File and directory watcher eligibility now uses path-aware checks. ImportWatcher::add_file declines registration for paths inside node_modules and returns caller-owned descriptor status. Transpiler call paths no longer filter those paths before calling the watcher.
Watch behavior tests test/cli/watch/watch.test.ts, test/internal/source-lints/watcher-node-modules-needle.test.ts	Added CLI tests for paths containing node_modules, including watch, hot-reload, test-watch, and build-watch cases. Added a source lint for literal substring searches.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: Normal

Merge Risk: Minimal · up to 9c6c8

No actionable issue remains in the supplied review evidence; this change is mergeable after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely summarizes the main change: watcher checks now use a node_modules path component instead of a substring match.
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 follows the template with both required sections, "What does this PR do?" (as "Problem", "Fix", "Background", "Downsides") and "How did you verify your code works?" (as "Tests", "Measu…

Full details: Description check

Explanation

The description follows the template with both required sections, "What does this PR do?" (as "Problem", "Fix", "Background", "Downsides") and "How did you verify your code works?" (as "Tests", "Measurements", "Suites run on the debug build"). It is detailed and specific: it names the bug, the affected gates, the repro, the fix, the tests added, the tradeoffs, and the mutation checks. Its main gap is that the verification relies on platform-specific tests that do not run on the author's machine, which the evidence block flags as "no test proof". Remediation needs a confirmation from CI, not a rewrite. The description is long and includes a large details block, but the content is on topic and mostly complete.

• 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 9:10 PM PT - Oct 27th, 2026}

❌ @robobun, your commit c03f1f0 has 11 failures in Build #134138 (All Failures):

• test/aot/perf/generated-code-of-idioms-10.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-operations-3.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-12.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-16.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-8.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-5.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-13.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-6.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-9.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/perf/generated-code-of-idioms-1.test.ts - code 1 on 🐧 13 aarch64 - test-aot-drawn
• test/aot/perf/generated-code-of-operations-1.test.ts - code 1 on 🐧 13 x64 - test-aot-drawn

---

🧪   To try this PR locally:

bunx bun-pr 46809

That installs a local version of the PR into your bun-46809 executable, so you can run:

bun-46809 --bun

[robobun]

Status: ready for review.

How it was reproduced, on bun 1.4.3-canary.1 (367d939) and on main a8b5e92, Linux x64:

1. Make a project in a folder named node_modules-cleaner. index.js imports ./src/util.js, which prints v1.
2. Run bun --watch index.js (or bun --hot index.js).
3. Rewrite src/util.js to print v2.

Nothing reloads and nothing is printed. The same project in a folder named cleaner reloads. A file named src/list-node_modules.js and a directory named src/node_modules_helpers/ behave like the first folder. With this branch all of them reload, and a file inside a real node_modules stays unwatched.

A fuzz run on this PR then found two cases that main reloads and an earlier head (2e865b7) did not. Release builds of main a0cc096 and of both heads, Linux x64:

1. A store behind the link node_modules -> node_modules.nosync, 200 packages, ulimit -n 256, then an edit of the project's own d.js. The earlier head watched the store: 128 to 151 descriptors, and d.js reloaded 4 of 8 times (0 of 4 on a debug build). Main and this head hold 9 descriptors and reload every time (6 of 6 and 8 of 8). Fixed in ec504f6.
2. cd app && bun --watch ../node_modules-tools/server.js, then a save by rename. Main reloads 3 of 3 times, this branch 0 of 3, and ../tools/server.js reloads 0 of 3 times on both. Not fixed here: watcher: keep no descriptor for a watched file outside kqueue #39571 removes the cause for every folder. It is the second Downsides bullet.

PR: #46809

[claude]

Beyond the inline findings, I also checked the fd ownership on the new decline path: the three loader sites that now reach ImportWatcher::add_file for package files (maybe_watch_file in src/runtime/jsc_hooks.rs and both sites in src/jsc/RuntimeTranspilerStore.rs) only clear the close flag on FdOwnership::Watcher, so the early FdOwnership::Caller return leaves the descriptor closed by the caller as before. The has_node_modules_component scan was also checked for the boundary cases in the unit table (leading component, trailing separator, backslash on POSIX) and matches its stated contract.

Extended reasoning...

The change moves the watcher's node_modules exclusion from a bare substring test to a path-component test in src/paths/string_paths.rs and centralizes it in ImportWatcher::add_file and Watcher::is_eligible_directory, with a source lint and 28 new watch tests. It touches no security-sensitive surface. Inline findings already cover the behavioral regressions (symlinked .nosync stores, rename saves outside the cwd) and test nits; this note only records the fd-ownership and scan-boundary checks that were ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• 🔴 src/jsc/hot_reloader.rs:82 — macOS/iCloud users whose node_modules is a symlink to node_modules.nosync get every package file watched under --watch/…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

• 🔴 src/jsc/hot_reloader.rs:82 — macOS/iCloud users whose node_modules is a symlink to node_modules.nosync get every package file watched under --watch/…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline nit on the new source lint, this pass also checked the node_modules.nosync store case raised earlier: commit ec504f6 now treats that suffix as a package component in has_node_modules_component (src/paths/string_paths.rs) with unit rows for it, and the early FdOwnership::Caller return in ImportWatcher::add_file (src/jsc/hot_reloader.rs) matches the None arm so the three loader sites that dropped their own guard still close the fd themselves. The serial spawn-and-reload cases in test/cli/watch/watch.test.ts were looked at for runtime and left alone since the file states why it stays serial.

Extended reasoning...

The change replaces six bare-substring node_modules checks in the watcher, transpiler store and JSC hooks with two component-aware helpers in bun_paths, adds a cfg(test) highway_memmem stub, and ships watch-mode CLI tests plus a source lint. It touches no security-sensitive surface; the only inline finding is a nit about the lint guarding a pattern the PR itself removed. The earlier open concern about iCloud .nosync stores was addressed by a later commit, and the fd-ownership contract on the new early return was verified against the None arm.

[robobun]

Linked package stores: the wider rule is not built

A later report asked for a wider rule. The rule: watch mode skips every hoisted store that it reaches through a link named node_modules, whatever the name of the target. I weighed it and did not build it. This PR stays at the component rule plus the one name node_modules.nosync. No issue is filed, because no user reported the case.

Checked on bun 1.4.3-canary.1 (367d939), Linux x64:

• No report exists. Tracker searches for nosync, icloud and a symlinked node_modules in watch mode find none. Include/exclude config for bun run --watch #5278 (open, 42 thumbs-up) asks for an exclude setting, which also serves this layout.
• The cost is small at default limits. node_modules -> store with 200 packages holds 400 descriptors under the store and 403 inotify watches. An edit of a project file reloads 3 of 3 times. At ulimit -n 256 it reloads 2 of 6 times, with no message. This PR does not change that target.
• The rule stops reloads that work today, because it cannot tell a store from own source behind the link. With node_modules -> ./vendor, an edit of vendor/lib/index.js reloads. With packages/app/node_modules -> .., an edit of a sibling package reloads.
• The loader sees only the real path of a module. The rule needs a verdict that the resolver passes to the loader, or a realpath probe in the Watcher. The dev server and bun build --watch share that Watcher.
• The other watch gates test /node_modules/ in the real path (bundle_v2.rs:5883, check_command.rs:659, run_compiled.rs:217). node --watch (v26.3.0) restarts on an edit in node_modules.

Two routes keep such a store unwatched today, with 0 store descriptors in each:

• Link to a target whose last component is node_modules, for example node_modules -> deps.nosync/node_modules. Workspace packages that are linked into the store still reload.
• Run bun --preserve-symlinks --watch .... Workspace packages that are linked into the store no longer reload.

A maintainer can decide otherwise. If the .nosync name is not wanted either, ec504f6 is the one commit to revert.