Repository navigation
Conversation
libuv maps every inotify mask bit outside IN_ATTRIB|IN_MODIFY to rename, and the kernel sets IN_ISDIR on every event about a directory. node thus reports a chmod of the watched directory, or of a subdirectory, as rename. Bun reported change. node's recursive watcher is not libuv. It reports no event for an attribute change of a directory, root or subdirectory. Bun reported change, with an undefined filename for the root. A recursive watch now reports nothing for it.
|
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; 0 remain after this review. WalkthroughLinux inotify event classification now distinguishes structural, directory attribute, and file attribute events. Recursive watchers suppress directory-child events. Linux tests cover root directory, child directory, child file, and recursive behavior. ChangesLinux fs.watch event handling
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The Linux watcher classification and its tests have no confirmed issue requiring changes before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The automatic review was rate limited and produced no findings. The diff is two files: one decision in the inotify dispatch in src/runtime/node/path_watcher.rs and three Linux-only tests in test/js/node/watch/fs.watch.test.ts. CI is running. Nothing to address from this comment. |
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 finding, I also checked the new continue for a recursive owner: it skips only the touched insertion and the add-watch branch, and the latter is gated on IN_CREATE|IN_MOVED_TO (structural), so no subdirectory watch is missed. The non-recursive root IN_ATTRIB case still reaches the nameless-event branch and reports basename(watched path), matching libuv.
Extended reasoning...
The inline IN_UNMOUNT finding stands on its own. Separately, I traced the early continue at path_watcher.rs:1087-1089 through the rest of the per-owner loop: the only work it bypasses is touched.get_or_put (only needed when something was emitted) and the recursive add-watch block, which requires a structural bit that the skipped branch can never have. I also confirmed the non-recursive root attribute event (no name, IN_ISDIR set) now maps to Rename and still falls into the basename(watcher_path) branch at line 1098, which is the libuv behavior the PR cites. The macOS/Windows arms are untouched, which is consistent with libuv's FSEvents/ReadDirectoryChangesW mapping of metadata changes to change rather than a divergence introduced here.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
|
Thanks for the trace. The IN_UNMOUNT point is real: a recursive watch on an unmounted filesystem would have gone silent. da526b1 changes the structural test to the libuv rule (every bit outside IN_ATTRIB|IN_MODIFY|IN_ISDIR is rename), which covers IN_UNMOUNT. The three new tests and the rest of test/js/node/watch/ still pass. |
|
50b7532 shortens the two comments the lint flagged. The first one is removed. The second is down to the libuv rule, its link, and the one line on node's recursive watcher. No code change. |
|
Updated 7:31 AM PT - Sep 17th, 2026
✅ @robobun, your commit f6e911585fb9bdbf472e93ed0d625e524d7acf5e passed in 🧪 To try this PR locally: bunx bun-pr 43083That installs a local version of the PR into your bun-43083 --bun |
|
f6e9115 trims the remaining comment to two lines. The coderabbit review produced no findings. No code change. |
|
The comment lint flags the two-line comment at the event kind decision again. I keep it as is. It names the libuv rule the code mirrors and the reason a recursive watch skips the event. It is not a justification of a workaround. No further push for this. |
There was a problem hiding this comment.
I re-reviewed the current head after the follow-up commits and found no bugs; the IN_UNMOUNT concern from the earlier pass is now covered by the !(IN_ATTRIB|IN_MODIFY|IN_ISDIR) structural test. A human look is still worthwhile because the recursive-watch behavior (silently dropping a directory's IN_ATTRIB) is a Node-compat claim I could not verify against a Node binary here, and the new tests are Linux-only so proof rests on CI.
What was reviewed:
- The per-owner
event_typedecision inpath_watcher.rs:1066-1077: the recursivecontinueskipsemit,touched, and the add-watch block, and none of those are needed for a pure IN_ATTRIB|IN_ISDIR event (add-watch requires IN_CREATE|IN_MOVED_TO,touchedonly drivesflush). - The subscribed masks (
WATCH_DIR_MASK/file mask) contain no IN_ACCESS/IN_OPEN/IN_CLOSE bits, sois_structuralcannot misclassify events Bun never asked for; IN_Q_OVERFLOW and IN_IGNORED are still handled before this point. - The three new tests: each fails on the old classification (test 1 yields
change),should_emitdedupe cannot fold thesub/f.txtpairs (different paths), and the watcher is closed infinallywith errors wired to reject.
Extended reasoning...
Overview
The PR changes how the Linux inotify backend of fs.watch (src/runtime/node/path_watcher.rs, Linux::thread_main) classifies an event as rename vs change. The old single pre-loop decision (rename iff a create/delete/move bit is set) is replaced by a pre-loop is_structural = mask & !(IN_ATTRIB|IN_MODIFY|IN_ISDIR) != 0 and a per-owner decision: structural -> Rename; non-directory -> Change; directory attribute event on a recursive owner -> skipped; on a non-recursive owner -> Rename. Three Linux-only tests in test/js/node/watch/fs.watch.test.ts cover chmod of the watched directory, of a subdirectory and file under a plain watch, and of root/subdir/file under recursive: true.
Security risks
None identified. The change only alters which WatchEventKind is passed to emit for events on already-registered watch descriptors; it adds no new parsing of untrusted data, no new allocation, and no new unsafe blocks. The continue path is taken before any raw-pointer use beyond the scalar copies already made under manager.mutex, so the existing SAFETY invariants around wd_map rehashing are unaffected.
Level of scrutiny
Moderate. The diff is small (about 15 lines of logic) but sits in a subtle area: shared-inotify-fd dispatch with per-owner semantics and a Node-compat behavior claim for the recursive watcher. I traced the skipped side effects (emit, touched.get_or_put, the recursive add-watch branch) and confirmed none apply to a pure IN_ATTRIB|IN_ISDIR event: add-watch requires IN_CREATE|IN_MOVED_TO (structural, so never reaches the skip), and touched only drives flush, which is meaningless when nothing was emitted. I also checked the subscribed masks (WATCH_DIR_MASK and the file mask at lines 720-729) to confirm the widened structural rule cannot misclassify access/open/close events, since Bun never subscribes to them; IN_Q_OVERFLOW and IN_IGNORED are still intercepted earlier. The earlier concern about IN_UNMOUNT|IN_ISDIR being swallowed on recursive watches is addressed by excluding IN_ISDIR from the negated set. What I could not do is execute a Node binary in this environment to confirm the recursive-watcher rows of the author's table, so that compat claim rests on the author's verification against node v26.3.0.
Other factors
The tests are wired correctly: the helper resolves on an exact event count, rejects on watcher error, and closes in finally; the assertions are exact toEqual on the full event list; test 1 fails on the old code (change instead of rename), so the suite is not vacuous. should_emit dedupes only same-path same-type events within a millisecond, which cannot collapse the sub/f.txt pairs. The tests are Linux-only and the PR itself notes no local test proof, so CI is the first real run of them. Combined with the unverifiable Node-compat claim for the recursive case, deferring to a human rather than approving is the honest call.
|
Both reviews of f6e9115 report no findings. On the open point, the recursive rows of the table in the PR body come from node v26.3.0 on Linux x64, run with the same script as bun. With a recursive watch, node reports no event for a chmod of the root or of a subdirectory, and |
Problem
fs.watch(dir)reports achmod(orutimes) of the watched directory, or of a subdirectory, aschange. Node reportsrename. A recursive watch reports('change', undefined)for achmodof its root. Node reports nothing. Fixes fs.watch on Linux: an event about the watched directory itself differs from Node (event type, filename for a trailing slash, recursive root) #43066, items 1 and 3.src/runtime/node/path_watcher.rs:1021setrenameonly for the create, delete and move bits. libuv maps every bit outsideIN_ATTRIB|IN_MODIFYto rename, and the kernel setsIN_ISDIRon every event about a directory (linux.c#L2611-L2615).Fix
rename, a file's attribute change ischange, a directory's attribute change isrenamefor a plain watch and no event for a recursive watch.lib/internal/fs/recursive_watch.js) rescans a directory whose watch fires and reports only entries that came or went, so it reports nothing in both cases. Verified against node v26.3.0.test/js/node/watch/fs.watch.test.ts(three new Linux-only tests, all fail on bun 1.4.3). Also all oftest/js/node/watch/and thetest-fs-watch*andtest-fs-promises-watch*files intest/js/node/test/parallel/.Background
fs.watchon Linux is one inotify fd shared by every watcher.Linux::thread_maininpath_watcher.rsreads the events and dispatches each one to the owners of its watch descriptor.IN_ATTRIB,IN_DELETE_SELF) has no name. libuv reports the basename of the watched path for it.Notes
node v26.3.0 on Linux, same script as the issue, plus a recursive watch with a subdirectory:
With this PR, bun matches node on every row.
IN_MODIFYnever carriesIN_ISDIR, so the only event this changes isIN_ATTRIBon a directory.The new tests resolve on an event count and copy the list at that moment.
writeFileSyncqueuesIN_CREATEandIN_MODIFYin one batch, so the recursive test useschmodof a file as its terminating event.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/watch/fs.watch.test.ts