Repository navigation
Conversation
|
Reproduced with Do not merge the head |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
All four of those reports show the |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughDirectory watching now applies platform-specific eligibility rules. Linux and Android file event handling detects orphaned files after rename or metadata events. Tests cover atomic rename-save updates to external modules, symlinked directories, and workspace packages. ChangesWatcher eligibility and reload validation
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The linked-package rename-save path has the required rebuild and watch-registration handling. No identified issue remains to block merging after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Direct issue
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/watcher/Watcher.rs`:
- Around line 750-766: Constrain non-Windows parent-directory watches in
is_eligible_directory to avoid unbounded watches for imported files outside the
project, while preserving the atomic rename-save recovery behavior for supported
in-project directories. Reuse the existing top_level_dir containment logic or an
equivalent established project-boundary check, and keep node_modules excluded.
- Around line 750-766: Update the Windows branch of is_eligible_directory to use
bun_paths::resolve_path::is_parent_or_equal for path-aware containment against
self.top_level_dir(), matching the existing check elsewhere in Watcher. Preserve
the node_modules exclusion and the unconditional non-Windows behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70ec9520-d869-4ab5-b4cd-45b126678e54
📒 Files selected for processing (2)
src/watcher/Watcher.rstest/cli/hot/watch.test.ts
|
Updated 1:44 PM PT - Oct 7th, 2026
⏳ @robobun, your commit 008a31c is still building in
|
d907bf7 to
c02258b
Compare
There was a problem hiding this comment.
Verified 0ae564d scopes the DevServer metadata-only skip inside the #[cfg(linux/android)] block — Windows/macOS now compile to the pre-PR unconditional append_file path. All three earlier findings are addressed; nothing new this round. Deferring to a human given this is cross-platform watcher internals.
What was reviewed:
is_eligible_directorycwd-drop on POSIX and its reuse inon_maybe_watch_directory; Windows path is byte-identical to pre-PR.IN_ATTRIBaddition and thest_nlink == 0eviction in bothNewHotReloader::on_file_updateandDevServer::on_file_update; confirmed a bare METADATA doesn't reload in either consumer, and the LinuxRENAMEFile-arm change in hot_reloader only adds eviction to the pre-existing append.- macOS kqueue path is untouched (no
NOTE_ATTRIBin the file mask, soOp::METADATAcannot reach the File arms there). - New tests: await the observable EVAL line (no sleeps),
test.concurrent,skipIf(isWindows), two rename-saves per variant.
Extended reasoning...
Overview
Fixes atomic rename-save detection for modules imported from outside the process cwd under --hot/--watch on POSIX. Two independent changes: (1) drop the cwd-containment gate from Watcher::is_eligible_directory on non-Windows so the parent directory of every imported file is watched (dedup'd by dir-fd/hash), and (2) add IN_ATTRIB to the Linux per-file mask and evict the watchlist entry when fstat(pinned fd).st_nlink == 0, so a rename-over of a held-open inode is recovered even without a parent-dir watch. Touches src/watcher/Watcher.rs, src/watcher/INotifyWatcher.rs, src/jsc/hot_reloader.rs, src/runtime/bake/DevServer.rs, and adds three tests in test/cli/hot/watch.test.ts.
Security risks
None identified. No untrusted-input parsing, no auth/crypto, no path traversal surface — the change loosens which parent directories of already-resolved imports get an inotify/kqueue watch. The node_modules exclusion is preserved.
Level of scrutiny
High. This is cross-platform, cfg-gated watcher plumbing shared by three consumers (NewHotReloader for --hot/--watch, bake::DevServer, and BundleV2 --watch), and prior review rounds each surfaced a real cross-consumer or cross-platform interaction (DevServer's unconditional append_file receiving the new METADATA op; a follow-up gate that would have dropped Windows RenamedNew events). The current revision resolves all of those, but the interaction surface is subtle enough that a maintainer familiar with the watcher should sign off, and CI needs to confirm the macOS/Windows lanes.
Other factors
- All prior review threads (mine and CodeRabbit's) are resolved with concrete commits; the author re-ran
rust:check-alland the affected test suites after each fix. - The new tests await stdout lines directly (no sleeps), use
test.concurrent, and cover--watch,--hot, and the symlink-resolved-to-real-path variant with two back-to-back rename-saves each. They areskipIf(isWindows)matching the PR's stated POSIX-only scope. - Resource-footprint concern from CodeRabbit was addressed: watches are bounded to one per unique parent directory of an already-imported file, matching the existing in-cwd model.
- The PR deliberately leaves Windows behavior unchanged (verified in the final DevServer diff); #35596 tracks the Windows counterpart.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/hot_reloader.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared rename-over orphan-detection heuristic instead of duplicating it.
Both files implement the identical Linux/Android-only "RENAME forces orphaned, else fstat the cached fd and check
st_nlink == 0" logic independently. A shared helper (e.g. inbun_watcheror a small free function takingop: WatchOp/Opandfd: Fd) would prevent the two copies from silently diverging if the heuristic is later refined (e.g. to address the hardlink edge case noted separately).
src/jsc/hot_reloader.rs#L922-940: replace the inlineorphanedcomputation with a call to the shared helper.src/runtime/bake/DevServer.rs#L6040-6060: replace the inlineevict-via-fstat computation with a call to the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jsc/hot_reloader.rs` at line 1, Extract the duplicated Linux/Android orphan-detection heuristic into one shared helper, accepting the watch operation and cached file descriptor, while preserving the RENAME-implies-orphaned behavior and the fstat/st_nlink == 0 fallback. Replace the inline computations in the hot-reloader flow and the DevServer eviction flow with calls to this helper so both use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/bake/DevServer.rs`:
- Around line 6040-6060: Add a concise rationale comment immediately before the
Linux/Android `fstat` link-count check in the event eviction logic, explaining
that rename-over of a held-open inode may report only a metadata event and
`st_nlink == 0` identifies it for eviction. Keep the existing behavior and TODO
unchanged.
In `@src/watcher/INotifyWatcher.rs`:
- Around line 132-138: No code change is requested. Keep the IN::ATTRIB flag and
existing watch_file_mask behavior unchanged; treat the comment as an operational
observation about increased event volume and potential buffer overruns.
---
Outside diff comments:
In `@src/jsc/hot_reloader.rs`:
- Line 1: Extract the duplicated Linux/Android orphan-detection heuristic into
one shared helper, accepting the watch operation and cached file descriptor,
while preserving the RENAME-implies-orphaned behavior and the fstat/st_nlink ==
0 fallback. Replace the inline computations in the hot-reloader flow and the
DevServer eviction flow with calls to this helper so both use the same logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09937d17-d5c3-4ef1-bfe1-05790a7456bd
📒 Files selected for processing (5)
src/jsc/hot_reloader.rssrc/runtime/bake/DevServer.rssrc/watcher/INotifyWatcher.rssrc/watcher/Watcher.rstest/cli/hot/watch.test.ts
There was a problem hiding this comment.
No new issues found; the three earlier findings (vacuous stderr assertion, DevServer picking up IN_ATTRIB, and the un-cfg-gated skip regressing Windows) are all addressed in the current diff. This still touches shared watcher infrastructure across three consumers with platform-specific gating and a POSIX policy change, so it's worth a human pass.
What was reviewed:
- Verified the final
DevServer::on_file_updateFile arm: on Windows/macOS the cfg block compiles out and the path is byte-for-byte the pre-PR evict-then-unconditional-append_file; on Linux only pure-METADATA withst_nlink > 0hitscontinue. - Re-checked
hot_reloader.rs: the new Linuxst_nlink==0eviction is guarded by!DELETEso it can't double-evict, and a bare METADATA falls through to the existingWRITE|DELETE|RENAMEgate without triggering a reload. is_eligible_directory: Windows branch is unchanged from pre-PR;on_maybe_watch_directorynow shares the same predicate instead of open-coding it.- Tests: stderr is now
inherit(matches the neighboring test), the load-bearingnextEvalassertions are exact-value.toBe, andawait using proccovers cleanup.
Extended reasoning...
Overview
The PR fixes --hot/--watch missing atomic rename-saves of modules imported from outside the process cwd on POSIX. Two independent changes: (1) drop the cwd-containment gate from Watcher::is_eligible_directory on non-Windows so the parent directory of every imported file is watched, and (2) add IN_ATTRIB to the per-file inotify mask and evict the watchlist entry when fstat(fd).st_nlink == 0 so a rename-over of a held-open inode is recovered even without a parent-dir watch. Five files: Watcher.rs, INotifyWatcher.rs, hot_reloader.rs, DevServer.rs, and new tests in test/cli/hot/watch.test.ts.
Security risks
None identified. No untrusted-input parsing, auth, or crypto. The change broadens which parent directories get an inotify watch (any imported file's parent, minus node_modules), but that set is bounded to unique parents of already-imported files and the existing dedupe by dir-fd/hash keeps it at one watch per directory — the resource-footprint concern raised by CodeRabbit was addressed and withdrawn.
Level of scrutiny
Medium-high. The watcher is shared by three WatcherContext implementers (--hot/--watch via NewHotReloader, and bake DevServer), and adding IN_ATTRIB to the shared per-file mask changes the event stream all three see. Two of my three earlier findings were exactly this class of cross-consumer / cross-platform fallout (DevServer rebuilding on chmod; the fix for that then dropping RenamedNew events on Windows). The current revision scopes both the st_nlink check and the metadata-only skip inside a single #[cfg(linux/android)] block, which I verified leaves Windows and macOS on the pre-PR path.
Other factors
- Three prior review rounds from me; all findings resolved and threads marked resolved on the timeline.
- The CodeRabbit resource-footprint and Windows-prefilter comments were withdrawn after the author's response; the comment-cop "paragraph-long comment" flags were addressed by trimming to single-line comments.
- New tests cover
--watch,--hot, and the symlink variant with two back-to-back rename-saves each; theyskipIf(isWindows)(Windows counterpart is #35596). The DevServer change has no new test in this PR — author reportstest/bake/dev/hot.test.ts11/11 green. - Not approving because this is cross-platform core-infrastructure with a policy change (unbounded-outside-cwd parent-dir watches) and the review history shows the fallout surface is subtle enough that a human should sign off.
is_eligible_directory() gated the parent-directory watch on the path containing the cwd as a substring. A module reached via a relative import that climbs out of the cwd (../shared/lib.ts when running from app/) or via a resolved symlink therefore got only the per-inode file watch. The first atomic rename-save (write temp + rename over: vim, sed -i, prettier, JetBrains safe-write, git checkout) replaces that inode, the kernel never delivers IN_DELETE_SELF while the transpiler's fd keeps the old inode alive, and with no directory watch there is no IN_MOVED_TO to recover from. Under --watch every later save of that file was ignored; under --hot every later reload re-transpiled the stale pre-save source from the pinned fd. On Linux/macOS the platform watchers are inode-based and have no rooted-tree restriction, so drop the cwd check there (node_modules stays excluded). Windows keeps the check because the platform watcher is a single recursive ReadDirectoryChangesW rooted at cwd. on_maybe_watch_directory now calls the same predicate instead of open-coding it.
Safety net for the parent-directory watch: add IN_ATTRIB to the per-file inotify mask and, when the pinned fd's st_nlink drops to 0 (the only event a rename-over delivers while bun still holds the old inode open; IN_DELETE_SELF waits for the last close), evict the watchlist entry so snapshot_fd_and_package_json cannot serve the stale fd on reload. IN_MOVE_SELF is treated the same. This recovers an atomic rename-save even when the parent-directory watch is absent (node_modules, or inotify_add_watch failed for the dir). Also drop the vacuous stderr not.toContain() assertion (the warning is Windows-only and the describe is skipIf(isWindows)), and add a directory-symlink test case: an in-cwd ./link/dep.ts resolved to its out-of-cwd real path hit the same gate.
…ATTRIB is delivered The per-file inotify mask now includes IN_ATTRIB, and DevServer shares that mask. Its Kind::File arm previously enqueued a rebuild for every delivered event, which was implicitly gated by the mask itself. Gate on WRITE/MOVE_TO/CREATE (or an evicting DELETE/RENAME/st_nlink==0) so chmod/touch/chown do not rebuild, and evict on st_nlink==0 so the per-file watch is re-armed on the new inode after a rename-over.
Windows' create_watch_event leaves op empty for Added/RenamedNew, so the unguarded else-if would have dropped those events. Move the skip inside the existing #[cfg(linux/android)] METADATA block so Windows and macOS behavior is unchanged.
…se; test bare-name workspace import After rebasing onto main, `ctx` in NewHotReloader::on_file_update is a *mut Watcher and remove_at_index takes a LOCK const generic; call it the same way the adjacent DELETE eviction does. Add the workspace-root case: `import 'lib/index.js'` resolved through packages/app/node_modules/lib -> ../../lib with bun started from the workspace root. Everything is inside cwd and the real-path parent dir is watched, but the resolver's dir-entry cache is keyed by the node_modules spelling while the watchlist entry carries the real path, so the directory IN_MOVED_TO matched nothing and the rename-save was dropped. The per-file IN_ATTRIB / st_nlink==0 eviction recovers it.
72a58de to
e95181e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
One more shape for the test matrix here, on the dev server rather than
mkdir -p r/app/node_modules r/lpkg && cd r/app
echo '{"name":"lpkg","version":"1.0.0","type":"module","main":"index.js"}' > ../lpkg/package.json
echo 'export const V = "LINKED_V0";' > ../lpkg/index.js
ln -s ../../lpkg node_modules/lpkg
echo '{"name":"app","type":"module"}' > package.json
echo 'import { V } from "lpkg"; export const D = "DEP_V0"; console.log("VALS", V, D);' > dep.ts
echo 'import "./dep.ts";' > client.ts
echo '<!doctype html><script type="module" src="./client.ts"></script>' > index.html
bun --port=0 ./index.html &
# fetch / and the /_bun/client/*.js it references after each step:
echo 'export const V = "LINKED_V1_INPLACE";' > ../lpkg/index.js # served: V1 (ok)
echo 'export const V = "LINKED_V2_ATOMIC";' > ../lpkg/.t && mv ../lpkg/.t ../lpkg/index.js # served: still V1
echo 'export const V = "LINKED_V3_INPLACE";' > ../lpkg/index.js # served: still V1 (deaf now)
sed -i 's/DEP_V0/DEP_V1/' dep.ts # rebuilt: DEP_V1 + V1, disk has V3On 1.4.2/1.4.3 the rename-over is never seen (0/10), the module is then deaf to in-place saves, and rebuilds triggered by other files keep bundling the stale linked-module source, since the dev server reads it through the pinned fd. A |
…arent-dir-outside-cwd
There was a problem hiding this comment.
Beyond the inline findings, I also checked the raw ctx use in the new hot_reloader.rs eviction block — it reuses the same *mut Watcher as the existing DELETE arm and remove_at_index::<false> only buffers the eviction, so the "same as the DELETE eviction above" SAFETY claim holds. I also confirmed the kqueue backend already maps NOTE_ATTRIB to METADATA and the new fstat block is Linux-gated, so a non-orphaned METADATA-only event falls through to the unchanged WRITE|DELETE|RENAME gate and does not trigger a reload on either platform.
Extended reasoning...
The change touches the inotify file mask, the directory-eligibility predicate, and the File-event arms of both watcher consumers (NewHotReloader and bake DevServer); no security-sensitive surface. Three inline findings remain open (RENAME now evicts on Linux where base only did so under kqueue, lost file watches when the parent directory cannot be watched, and no DevServer test coverage), so a human should review; this note only records the additional checks that were ruled out.
…arent-dir-outside-cwd
A rename-over or an unlink leaves the watched inode without a link while the watchlist's fd keeps it open, so the kernel holds back IN_DELETE_SELF. The inotify backend now asks for IN_ATTRIB and reports the file as deleted when fstat shows st_nlink == 0. It drops every other IN_ATTRIB, so consumers see no new kind of event. hot_reloader.rs and DevServer.rs go back to main. Both already handle a deleted file. A moved file (IN_MOVE_SELF) keeps its watch again, so moving a dependency away and back reloads under --hot. The parent-directory watch is now best-effort: when it cannot be set, the file is still watched, and the directory fd opened for it is closed.
Re-cut after review. The previous heads watched the parent directory of every file outside cwd and reported a file with no link left as deleted. Both had regressions against main: a program that removes a module it imported from a temporary directory reloaded forever, and an in-place save of a module outside cwd could lose a write. The inotify backend still asks for IN_ATTRIB on each watched file. On that event it now asks inotify for the watch of the file's path. A different watch descriptor means the path names another inode: the file was replaced, and it is reported as deleted. A path that is gone, or that names the same inode, reports nothing, as on main. Paths under node_modules are left alone. Watcher.rs is back to main: no directory is watched that main does not watch. Task::append skips a hash it already holds, because a replaced file inside cwd is now reported by its directory and by itself.
Problem
--watch,--hotand the dev server miss a file replaced by rename (sed -i, vim) when it is outside the cwd, behind a symlink, or in a workspace package imported by bare name. Later saves are missed too.IN_ATTRIB, which the file mask (src/watcher/INotifyWatcher.rs) did not request.Fix
IN_ATTRIB. On it,file_was_replacedasks inotify for the watch of the path. Another descriptor means another inode: the file is reported as deleted.Task::appendskips a hash it already holds.test/cli/hot/watch.test.ts(13 new cases, 9 fail on main), one intest/bake/dev/html.test.ts. Alsotest/cli/hot/,test/cli/watch/,test/bake/dev/hot.test.ts.Background
inotify_add_watchreturns the existing descriptor when the inode is already watched.Downsides
--watch: 14 evaluations in 12 s, main 1).--hot, vim's save (rename to a backup first) is evaluated twice: at the rename, as on main, and when the backup is removed.inotify_add_watchcall perchmod. Binary size not measured (nobloaty).Notes
What this head is. Earlier heads had two halves: watch the parent directory of every file outside cwd, and report a file with
st_nlink == 0as deleted. A self-review of that diff asked to split them. It found two regressions against main, and both reproduced:os.tmpdir()and then removes it. In 12 s main runs it once. The earlier head ran it 12 times under--watchand 188 times under--hot.The review proposed to keep the directory half here, on top of #41617, and to decide the file-level half together with #39571. This head does the reverse: it keeps the file-level half and drops the directory half. Reasons: the review measured that the file-level signal alone fixes 8 of the 9 failing cases with no new watch and no lost write, against 5 for the directory half. The directory half also needs #41617 and an eviction gate first. A maintainer can ask for the other packaging.
Review concerns on the file-level half, and what was done.
st_nlink == 0is the wrong test in both directions (hard link, overlayfs lower layer, single-file bind mount): done, the test is what the path names now. The two hard-link cells that were 0/3 are 3/3.Task::appendthree times: done, with the dedupe from watcher: coalesce per-save event bursts into a single --hot reload #30617 and a test that stops the process, replaces three modules and resumes it.bun build --watchwatches files undernode_modules: paths that containnode_modulesare not reported, as on main.Shapes. Linux x64,
--hot, three rename-saves per cell, main against this head:../shared/lib.ts0/3 and 3/3../link/dep.tswithapp/link -> ../realdir0/3 and 3/3.lib/index.jsthroughpackages/app/node_modules/lib -> ../../libfrom the workspace root 0/3 and 3/3. Each of the three with a second hard link on the file: 3/3 on this head. The entry point outside cwd: covered by a test.Not changed by this head. inotify watches and open fds of one
bun --hotprocess with 40 modules in 8 directories outside cwd: 42 and 50 on main and on this head. 1000chmod,touch,linkandunlinkoperations on an imported file: 0 reloads and no watcher event.mv dep.ts dep.bakthenmv dep.bak dep.tsunder--hotreloads as on main. Windows and macOS are not touched.Still missed, as on main.
config.json -> ..data/config.json,..dataswapped by rename, old directory removed): the watched real path is removed, not replaced, so nothing is reported. hot reloader: follow a retargeted symlink on the import path #41534 and dev server: follow a retargeted symlink on the import path #41539 are the open work on a retargeted symlink.rm -rf dist && tsc).The loop in Downsides.
import state from "../data/state.json", then the program writes a temporary file and renames it overstate.json. Main: 1 evaluation. This head: 14 in 12 s under--watch, 260 under--hot(debug build). With the file inside cwd, main itself makes 247 and 1609 evaluations in 8 s. inotify cannot tell the program's own write from an editor's.The double evaluation in Downsides.
mv lib.ts lib.ts~, write a newlib.ts,rm lib.ts~, three times, module outside cwd,--hot. Main: the first save is evaluated once (therenameevent of the file) and the next two are missed. This head: each save is evaluated twice.BUN_WATCHER_TRACEshowsrenameand thendeletefor the file. When both events reach the watcher in one batch they are one evaluation. #30617 is the open work on one reload per burst of events.What
--hotdoes on main after a missed save. Since #37050 a reload reads the module by path, so another file's reload picks up the new source. The saves of the file itself are never seen.Tests. All new cases run on Linux only. 9 cases in
watch.test.tsfail on main: module outside cwd, workspace package and entry point outside cwd under both flags, directory symlink with and without a hard link, hard link outside cwd. 4 are guards that pass on main:mvaway and back, three modules replaced in one batch evaluate once, a metadata-only change is not reported, a removed file outside cwd is not reported. The dev server case replacesoutside/dep.tsby rename twice and times out on main.Tools.
strace,perfandbloatyare not installed in the build container and a release build was not feasible there, so there is no syscall trace and no binary size.Related: #39571 (keep no descriptor for a watched file outside kqueue), #41617 (map a directory event to the watchlist by path), #36416 and #36418 (other uses of
IN_ATTRIB), #44344 (Windows watcher and the delete model), #13511, issue 9547 (the Windows report of the monorepo case, #35596).no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/hot/watch.test.ts