fs.watch: decouple from bun.Watcher, own inotify/FSEvents/kqueue directly#29952
Merged
Conversation
…ctly
The POSIX fs.watch() backend (path_watcher.zig) previously routed every
watch through a full bun.Watcher instance — the bundler/--watch/--hot
watcher, whose WatchItem carries options.Loader, *PackageJSON, and a
*bun.fs.FileSystem, and whose Windows backend is pinned to top_level_dir.
Adapting that for fs.watch() required:
* a 1068-line shim with three interacting mutexes and deferred-deinit
workarounds (60 lock/atomic sites)
* a WorkPool DirectoryRegisterTask that crawled the tree once and
registered every file with the bundler watcher (recursive never saw
directories created after watch())
* a bolted-on FSEvents side-channel for macOS directories, so
fs.watch(dir) on macOS spun up two watcher threads (kqueue +
CFRunLoop), guarded by 'if (fsevents_watcher != null) continue'
everywhere
* O(watchers×events) string-prefix re-fanout in onFileUpdate
win_watcher.zig never went through bun.Watcher — it wraps uv_fs_event
directly in 310 lines. This gives Linux/macOS/FreeBSD the same shape:
PathWatcherManager process-global, one mutex, one inotify/kqueue fd
+ reader thread (or FSEvents CFRunLoop on macOS)
PathWatcher one per unique (realpath, recursive) — deduped
handlers[] JS FSWatcher contexts sharing this OS watch
Duplicate fs.watch() on the same path appends a handler to the existing
PathWatcher; detach() removes one, last-out tears down the OS watch.
Linux recursive now adds an inotify wd on IN_CREATE|IN_ISDIR so new
subdirectories are tracked (fixes #15939/#15085). macOS uses FSEvents
for both files and directories (matching libuv) so fs.watch() no longer
runs two watcher threads. bun.Watcher is unchanged; --watch/--hot/
DevServer still use it.
1068 → 636 lines, one mutex instead of three.
Collaborator
Author
|
Updated 7:43 PM PT - Apr 29th, 2026
❌ @robobun, your commit dd8732c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29952That installs a local version of the PR into your bun-29952 --bun |
Contributor
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
The manager is process-global and never torn down, so the inotify/kqueue reader is a daemon — detach() it at spawn instead of stashing a std.Thread handle that defaulted to undefined (ban-words lint) and would never be joined anyway.
Collaborator
|
@claude review |
Collaborator
|
@robobun theres only one global watcher thread right? It doesn’t create one per fs.Watch. |
Collaborator
Author
Correct — one thread total for all
The reader thread is detached (daemon) since the manager is process-global and never torn down. |
Contributor
|
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:
WalkthroughReplaces the previous bun.Watcher-based fs.watch with a process-global PathWatcherManager (deduplicated by resolved path + recursive flag), adds platform-specific backends (Linux inotify, macOS FSEvents, FreeBSD kqueue), changes the FSEvents callback type, and adds fs.watch behavioral tests. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/node/path_watcher.zig`:
- Around line 438-452: The wd_map entry is currently overwritten with a single
owner (gop.value_ptr.* = { .watcher = watcher, ... }), which breaks overlapping
recursive watches when inotify reuses the same wd; instead, change the wd_map
value to track a collection of owners (e.g., an owners list/set of PathWatcher
pointers and associated subpaths), update the insertion code to append the new
watcher+subpath to that owners collection (rather than freeing and replacing
subpath), update watcher.platform.wds.append usage to register the wd but not to
remove previous owners, and modify rm_watch/cleanup to remove only the departing
watcher from the owners collection and only call inotify_rm_watch/free the
wd_map entry when the owners collection becomes empty; update any
allocation/free logic around value_ptr.subpath to manage per-owner subpaths
accordingly.
- Around line 131-141: The predicate in shouldEmit is using a logical AND
between event_type and hash comparisons which suppresses valid events; change
the condition so that a differing event_type OR a differing hash triggers
emission (i.e., replace the combined "this.event_type != event_type and
this.hash != hash" with an OR-based check), keeping the existing timestamp check
and updating this.timestamp, this.event_type, and this.hash only when the
function decides to emit; this change should be made in the shouldEmit function
to correctly detect per-path or per-type differences.
- Around line 465-478: The watcher code constructs file paths using
std.fmt.bufPrint* and std.fs.path functions (e.g., creating child_abs and
child_rel with abs_buf/rel_buf and using basename elsewhere); replace those
manual joins and basename uses with Bun's path helpers (bun.path.join,
bun.path.basename, and related bun.path APIs) and use bun.path_buffer_pool where
appropriate so code uses bun.path utilities instead of std.fmt/std.fs.path;
update the blocks that build child_abs/child_rel (variables abs_buf, rel_buf,
child_abs, child_rel) and the other mentioned ranges (around lines 566-575,
777-789, 836-839) to call bun.path functions and bun.path_buffer_pool
equivalents.
- Around line 660-677: onFSEvent() is iterating watcher.handlers on the
CFRunLoop thread without synchronizing with watch()/detach(), which can
rehash/move the AutoArrayHashMapUnmanaged under manager.mutex; fix by taking
manager.mutex (the same mutex used by watch()/detach()) or by creating a safe
snapshot of watcher.handlers under manager.mutex and then iterating the snapshot
without the lock; update onFSEvent() (and onFSEventFlush() if it touches
handlers) to either lock manager.mutex around map access or copy entries into a
temporary array while holding manager.mutex, then release the mutex and call
watcher.emit()/emitError() on the snapshot; keep detach()’s
emit_in_progress/pending_deinit behavior unchanged so frees remain deferred.
In `@test/js/node/watch/fs.watch.rewrite.test.ts`:
- Around line 33-35: Replace the fixed Bun.sleep(100) readiness delay before
calling mkdir with an explicit await of the watch callback observing the
readiness file (seed.txt) so the test only proceeds once the root watch is
actually registered; specifically, remove the Bun.sleep(100) and add logic to
await the watch handler/event that reports creation or presence of "seed.txt"
(or another explicit readiness signal observed by the fs.watch callback used in
this test) before invoking mkdir so the mkdir event is not lost and retries can
succeed.
🪄 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: 5eb7fd16-4a6f-4f82-be5f-bc56fc29be38
📒 Files selected for processing (4)
src/bun.js/node/fs_events.zigsrc/bun.js/node/path_watcher.zigtest/internal/ban-limits.jsontest/js/node/watch/fs.watch.rewrite.test.ts
Linux wd_map: inotify_add_watch returns the same wd for the same inode,
so a recursive watch on /a and a plain watch on /a/sub share /a/sub's
wd. The single-owner map meant the inner watch stole dispatch from the
parent, and closing it rm_watch'd the wd out from under the parent.
wd_map is now wd → list of {watcher, subpath}; dispatch iterates every
owner and inotify_rm_watch only fires when the last owner detaches.
New test covers this.
detach() now does the full teardown (handlers, dedup-map removal,
platform dispatch-map removal) inside one manager.mutex critical
section. Previously it dropped the lock between 'handlers empty' and
'remove from map', so a concurrent watch() from another Worker could
find and reuse a PathWatcher that was about to be destroyed.
macOS lock order: onFSEvent/onFSEventFlush now take manager.mutex so
iterating watcher.handlers can't race with attach/detach mutating the
map. To avoid AB/BA with the FSEvents loop mutex (which _events_cb
holds while calling onFSEvent), Darwin.addWatch/removeWatch are now
called *outside* manager.mutex — the JS thread never holds both, so
the order is one-way: fsevents_loop.mutex → manager.mutex. With every
emit path now under manager.mutex, the emit_in_progress/pending_deinit
deferred-free machinery is no longer needed and is removed.
The shouldEmit predicate is left as-is — it is copied verbatim from
win_watcher.zig (and the old path_watcher.zig); changing it here would
diverge POSIX from Windows.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The owners loop cached `plat.wd_map.getPtr(ev.wd)` once and iterated its .items. When a recursive owner saw IN_CREATE|IN_ISDIR it called addOne/walkAndAdd, which do wd_map.getOrPut() and can rehash — moving the ArrayList header the cached pointer was pointing at, so the next `oi < owners.items.len` read dangling memory. The existing comment guarded against the inner ArrayList's .items growing but missed that the pointer to the ArrayList itself lives in the hashmap's reallocatable storage. Now re-fetch wd_map.getPtr(ev.wd) at the top of each iteration. The owner's heap-owned subpath slice survives a rehash (only the header moves), so no further copying is needed. Also drop an unused `const plat` local in Linux.addWatch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/node/path_watcher.zig`:
- Around line 819-824: The kevent() return value is ignored, so registration
failures still add the entry and return success; update the logic around the
std.posix.system.kevent call (the call using plat.kq.native() and var changes)
to check its return and errno, and if it fails propagate an appropriate Zig
error instead of proceeding to bun.handleOom(plat.entries.put(...)) and
watcher.platform.fds.append(...); only insert the entry and append the fd on
successful kevent registration, and ensure any allocated resources are cleaned
up on failure.
🪄 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: 69649f79-058a-4b99-969e-77d4d1e505dc
📒 Files selected for processing (1)
src/bun.js/node/path_watcher.zig
Previously discarded the return value of the EVFILT_VNODE registration kevent(), so a failed registration left a dead entry in the map that would never deliver events. Now close the fd, free the entry, and return the errno to the caller (best-effort skip for children of a recursive walk, matching the inotify backend).
…ip err.path - On macOS the PathWatcher is published into the dedup map before the lock is released for Darwin.addWatch (required by the fsevents→manager lock order). If addWatch then fails after a concurrent Worker attached a handler, the old error path freed the watcher unconditionally, leaving the other thread's FSWatcher holding a freed pointer. Now remove only our handler under the lock and let any survivors' detach() free it. - Linux/FreeBSD addWatch-error return: strip .path before returning; Linux.addOne's error borrowed watcher.path which we destroy() two lines earlier. Caller only reads errno today, but every other return here already does .withoutPath(). - PathWatcherManager.get(): drop the unlocked fast-path read of default_manager (broken DCLP on weakly-ordered targets). get() runs once per fs.watch(); always taking default_manager_mutex is fine.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd354eb8-25c6-4350-8a0b-cbabd98689fe
📒 Files selected for processing (1)
src/bun.js/node/path_watcher.zig
When a subdirectory inside a recursive watch is renamed, inotify keeps the wd attached to the inode. IN_MOVED_TO on the parent triggers addOne() for the new name, inotify_add_watch returns the *same* wd, and the existing owner entry's subpath was left stale — subsequent events under the moved directory were reported under the old name. addOne() now overwrites the owner's subpath when the watcher already owns the returned wd. walkAndAdd only descends into entry.kind == .directory (symlinks are .sym_link), so this can't pick a longer alias via a cycle; the IN_CREATE-race case passes the same subpath so the reassign is a no-op. Adds a test that renames root/a → root/b under a recursive watch and asserts writes surface as b/inside.txt (matches Node).
…; pace cold-VM test for FSEvents Kqueue: between kevent() returning and the reader taking manager.mutex, a JS thread can removeWatch (close fd N, drop entries[N]) and addOne for an unrelated path — POSIX hands back the lowest fd, so entries[N] now points at the new watch and the stale event would be dispatched to it. kev.udata already carries the original entry pointer; compare it against the map lookup and skip on mismatch. Test: the cold-VM fixture's setImmediate retry loop can exhaust its 200 writes in ~20ms on macOS, before the async-scheduled FSEventStream (with kFSEventStreamEventIdSinceNow + 50ms latency) delivers anything. Pace it at 25ms/tick like the other tests in this file.
…pe walk, O_PATH probe, by-value KqEntry watch(): - Replace lstat + conditional open + stat with a single open(O_PATH| O_DIRECTORY) → retry without O_DIRECTORY on ENOTDIR. The retry tells us file-vs-dir, open() follows symlinks, and the fd feeds getFdPath for the realpath. One/two syscalls instead of up to four. Linux: - inotify_init1/add_watch/rm_watch and read() via bun.sys.syscall with Maybe.errnoSys instead of catching Zig errors. - Reuse INotifyWatcher.Event for the kernel inotify_event header (watch_descriptor/name_len field names). - bun.path.basename / joinZBuf / joinStringBuf instead of std.fs.path and std.fmt.bufPrint. Shared: - walkSubtree() replaces the duplicated Linux/Kqueue walkAndAdd bodies; uses bun.sys.open + bun.DirIterator (no std.fs) and bun.path joins. Kqueue (FreeBSD): - Open monitored fds with O_EVTONLY|O_RDONLY|O_CLOEXEC. - Store KqEntry by value in the entries map — drops the per-entry allocator.create() so a recursive tree costs one map growth plus one subpath dupe per entry instead of two allocations. - Generation counter in KqEntry.gen / kev.udata for fd-reuse detection (entry pointer is no longer stable, so @intFromPtr no longer works). - kqueue()/kevent() via bun.sys.syscall.
Collaborator
|
@robobun its suspicious that this test is now failing in this branch on Windows arm64: |
Collaborator
Author
|
This one's a pre-existing flake on
(All marked This PR doesn't touch anything $ git diff 6c21a7e7f3..HEAD --name-only
src/bun.js/node/fs_events.zig # macOS FSEvents only
src/bun.js/node/path_watcher.zig # POSIX fs.watch; Windows stub is type-only
test/internal/ban-limits.json
test/js/node/watch/fs.watch.rewrite.test.ts
Every other lane on #49273 that shares the same shard (ubuntu x64, alpine, debian asan, macOS x64+aarch64, windows x64-baseline) passed |
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…ctly (oven-sh#29952) ## What Rewrite the POSIX `fs.watch()` backend (`src/bun.js/node/path_watcher.zig`) to talk to inotify/FSEvents/kqueue directly instead of routing through `bun.Watcher` (the bundler/`--watch`/`--hot` watcher). `bun.Watcher` itself is unchanged. ## Why `bun.Watcher` is shaped around a module graph: its `WatchItem` carries `options.Loader`, `*PackageJSON`, a `*bun.fs.FileSystem`, and on Windows is pinned to `top_level_dir`. None of that applies to `fs.watch()`. Adapting it required a 1068-line shim with: - Three interacting mutexes + two atomics, 60 lock sites, five separate "defer deinit after unlock or UAF" workarounds - A `WorkPool` `DirectoryRegisterTask` that crawled the tree **once** and registered every entry with the bundler watcher — `{recursive: true}` never tracked directories created after `watch()` (→ oven-sh#15939, oven-sh#15085, oven-sh#24875) - A bolted-on FSEvents side-channel for macOS directories, so `fs.watch(dir)` on macOS spun up **two** watcher threads (kqueue via `bun.Watcher` + `CFRunLoop` via `fs_events.zig`), with `if (watcher.fsevents_watcher != null) continue` guards scattered through the dispatch loop - O(watchers × events) string-prefix re-fanout in `onFileUpdate` - Dummy `.loader = .file`, `.package_json = null` threaded through every call `win_watcher.zig` never went through `bun.Watcher` — it wraps `uv_fs_event` directly in ~300 lines. This PR gives the other platforms the same shape. ## How ``` PathWatcherManager process-global, lazy, owns the OS resource ├─ Linux: one inotify fd + one reader thread, wd → PathWatcher map ├─ macOS: delegates to fs_events.zig (one CFRunLoop thread, one FSEventStream) └─ FreeBSD: one kqueue fd + one reader thread, fd → PathWatcher map PathWatcher one per unique (realpath, recursive) — deduped └─ handlers[] the JS FSWatcher contexts sharing this watch ``` - **Dedup:** two `fs.watch()` on the same path share one `PathWatcher` (one OS watch, `handlers` list). `detach()` removes one handler; last one out tears down the OS watch. Dedup key is `realpath + recursive-bit` since recursive/non-recursive need different registrations. - **One mutex** guards the watchers map + platform dispatch maps. The reader thread holds it while dispatching so `detach()` can't free mid-emit. Replaces the three interacting mutexes. - **Linux recursive** now adds a new inotify `wd` on `IN_CREATE|IN_ISDIR` (and walks the new subtree once), so files inside directories created after `watch()` are delivered. Fixes oven-sh#15939 / oven-sh#15085. - **macOS** uses FSEvents for both files *and* directories (matching libuv), so `fs.watch()` no longer runs two watcher threads. The FSEvents callback routes through `PathWatcher.emit()` → `handlers[]`, same as the other platforms. - **FreeBSD** uses a shared kqueue fd with `EVFILT_VNODE` per path. kqueue gives no filenames; directory events surface as a bare `rename` with empty path (libuv semantics). - No more `vm.transpiler.fs` / `options.Loader` / `*PackageJSON` anywhere in the `fs.watch` path. `src/Watcher.zig`, `src/watcher/*`, `hot_reloader.zig`, `bake/DevServer.zig` are untouched — `--watch`/`--hot` keep their own watcher. ## Verification - `zig:check-all` green on linux/mac/freebsd/windows × debug/release - All 20 Node `test/js/node/test/parallel/test-fs-watch*.js` pass - `test/js/node/watch/*.test.ts` green (the two permission tests that fail are pre-existing; they fail on main too when running as root) - `test/cli/hot/watch.test.ts` green (bun.Watcher unchanged) - New `fs.watch.rewrite.test.ts`: recursive-new-subdir test **fails** on `main` (the oven-sh#15939 bug), passes here ## Size `path_watcher.zig`: 1068 → 636 lines (−40%), `fs_events.zig` −3 lines, net −204 lines with tests included. Fixes oven-sh#15939 Fixes oven-sh#15085 --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Rewrite the POSIX
fs.watch()backend (src/bun.js/node/path_watcher.zig) to talk to inotify/FSEvents/kqueue directly instead of routing throughbun.Watcher(the bundler/--watch/--hotwatcher).bun.Watcheritself is unchanged.Why
bun.Watcheris shaped around a module graph: itsWatchItemcarriesoptions.Loader,*PackageJSON, a*bun.fs.FileSystem, and on Windows is pinned totop_level_dir. None of that applies tofs.watch(). Adapting it required a 1068-line shim with:WorkPoolDirectoryRegisterTaskthat crawled the tree once and registered every entry with the bundler watcher —{recursive: true}never tracked directories created afterwatch()(→ fs.watch {recusive:true} does not react to new items. #15939, fs.watch cannot detect changes in files that are created after bun starts #15085,fs.watchstops emittingchangeevents for a file after it is deleted and recreated (Linux: WSL2/Ubuntu) #24875)fs.watch(dir)on macOS spun up two watcher threads (kqueue viabun.Watcher+CFRunLoopviafs_events.zig), withif (watcher.fsevents_watcher != null) continueguards scattered through the dispatch looponFileUpdate.loader = .file,.package_json = nullthreaded through every callwin_watcher.zignever went throughbun.Watcher— it wrapsuv_fs_eventdirectly in ~300 lines. This PR gives the other platforms the same shape.How
fs.watch()on the same path share onePathWatcher(one OS watch,handlerslist).detach()removes one handler; last one out tears down the OS watch. Dedup key isrealpath + recursive-bitsince recursive/non-recursive need different registrations.detach()can't free mid-emit. Replaces the three interacting mutexes.wdonIN_CREATE|IN_ISDIR(and walks the new subtree once), so files inside directories created afterwatch()are delivered. Fixes fs.watch {recusive:true} does not react to new items. #15939 / fs.watch cannot detect changes in files that are created after bun starts #15085.fs.watch()no longer runs two watcher threads. The FSEvents callback routes throughPathWatcher.emit()→handlers[], same as the other platforms.EVFILT_VNODEper path. kqueue gives no filenames; directory events surface as a barerenamewith empty path (libuv semantics).vm.transpiler.fs/options.Loader/*PackageJSONanywhere in thefs.watchpath.src/Watcher.zig,src/watcher/*,hot_reloader.zig,bake/DevServer.zigare untouched —--watch/--hotkeep their own watcher.Verification
zig:check-allgreen on linux/mac/freebsd/windows × debug/releasetest/js/node/test/parallel/test-fs-watch*.jspasstest/js/node/watch/*.test.tsgreen (the two permission tests that fail are pre-existing; they fail on main too when running as root)test/cli/hot/watch.test.tsgreen (bun.Watcher unchanged)fs.watch.rewrite.test.ts: recursive-new-subdir test fails onmain(the fs.watch {recusive:true} does not react to new items. #15939 bug), passes hereSize
path_watcher.zig: 1068 → 636 lines (−40%),fs_events.zig−3 lines, net −204 lines with tests included.Fixes #15939
Fixes #15085