fix(fs): emit rename event when watched directory is deleted on Linux#26498
Closed
robobun wants to merge 1 commit into
Closed
fix(fs): emit rename event when watched directory is deleted on Linux#26498robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Conversation
Collaborator
Author
Contributor
WalkthroughWrap FD closes with validity checks; avoid acquiring directory FDs on Linux inotify paths by storing invalid FDs; emit rename on IN_DELETE_SELF and clean up watches; defer deinit/unref actions outside locks; add skipped regression tests for watched-directory deletion/recreation. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/node/path_watcher.zig (1)
786-792: AddisValid()check before closing FDs during deinit.In the
deinitfunction's cleanup loop,path.fd.close()is called without checking validity. On Linux, many of these FDs will be invalid (since we now close them early). For consistency with other changes in this PR (e.g., lines 125, 248, 284, 609, 695), add the validity check.🔧 Proposed fix
var it = this.file_paths.iterator(); while (it.next()) |*entry| { const path = entry.value_ptr.*; - path.fd.close(); + if (path.fd.isValid()) path.fd.close(); bun.default_allocator.free(path.path); }
🤖 Fix all issues with AI agents
In `@test/regression/issue/23306.test.ts`:
- Around line 23-30: The test currently uses fixed Bun.sleep delays around the
watcher and folder removal which causes flakiness; replace those sleeps with
condition-based polling: wait for the watcher to be ready by polling a readiness
flag or invocation (e.g., check a watcherReady boolean or the watcher callback)
before calling fs.rmdirSync(folderToWatch), and after removal poll for the
expected event (e.g., an eventReceived variable or entries in the events array
the test asserts on) with a reasonable timeout and short interval; update the
test around the Bun.sleep calls to use these condition loops so the test
proceeds as soon as conditions are met or fails after timeout.
- Around line 51-85: Replace the sequence of fixed Bun.sleep delays around
watcher setup/teardown with condition-based waiting: after creating
watcher1/watcher2 and after file ops (fs.rmdirSync, fs.mkdirSync,
fs.writeFileSync) await a Promise that resolves when the expected event appears
(e.g., poll or use an event-driven Promise that resolves when events or events2
receive an entry) instead of fixed waits; specifically update the logic around
watcher1, watcher1.close(), and watcher2 (and the final file creation) to wait
for events.length or events2.length > 0 (or a resolved event Promise) so the
test proceeds as soon as the actual filesystem event is delivered rather than
relying on cumulative Bun.sleep delays.
robobun
force-pushed
the
claude/fix-fswatch-dir-delete-23306
branch
from
a9c2a47 to
084573e
Compare
Fixes #23306 On Linux with inotify, when a watched directory is deleted: 1. The watcher now correctly emits a "rename" event with the directory's basename (matching Node.js behavior) 2. After closing a watcher on a deleted directory and recreating the directory, new watchers correctly receive file change events The root cause was that Bun kept file descriptors open for watched directories. Since inotify watches by inode, keeping the FD open kept the inode alive, preventing IN_DELETE_SELF events from being generated when the directory was deleted via rmdir(). Changes: - Close directory FDs immediately after setting up inotify watches on Linux (inotify watches by path/inode, not FD) - Handle IN_DELETE_SELF events by emitting rename and cleaning up the file_paths HashMap entry - Fix potential deadlock in unrefPendingDirectory by releasing mutex before calling deinit - Add validity checks before closing FDs to prevent double-close errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-fswatch-dir-delete-23306
branch
from
084573e to
d139875
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/node/path_watcher.zig (1)
602-634: Race condition incurrent_fd_taskdeduplication on Linux with invalid FDs.On Linux, all directory and file
PathInfoentries havefd = invalid(set in_fdFromAbsolutePathZat lines 73-74 and 102). InDirectoryRegisterTask.schedule(), the lookup at line 410 (manager.current_fd_task.getEntry(path.fd)) uses this FD as the key.When two watchers for different paths are created concurrently (before the first task starts processing), they both hash to the same key (
invalid), causing:
- First watcher creates
DirectoryRegisterTaskwith its path- Second watcher finds the existing entry and appends to the first path's task
- When
run()executes,processWatcher()iterates over the wrong directory for the second watcherConsider using
path.hashorpath.pathas the deduplication key on Linux instead ofpath.fd:♻️ Suggested approach
fn schedule(manager: *PathWatcherManager, watcher: *PathWatcher, path: PathInfo) !void { // keep the path alive manager._incrementPathRef(path.path); errdefer manager._decrementPathRef(path.path); var routine: *DirectoryRegisterTask = undefined; { manager.mutex.lock(); defer manager.mutex.unlock(); - // use the same thread for the same fd to avoid race conditions - if (manager.current_fd_task.getEntry(path.fd)) |entry| { + // use the same thread for the same directory to avoid race conditions + // On Linux, use path hash since fd is always invalid for directories + const task_key = if (comptime Environment.isLinux) + `@as`(bun.FD, `@bitCast`(path.hash)) // or use a separate hash map + else + path.fd; + if (manager.current_fd_task.getEntry(task_key)) |entry| {Alternatively, change
current_fd_taskto usepath.hashas the key type on Linux.
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.
Summary
Fixes #23306
On Linux with inotify, when a watched directory is deleted:
Root Cause
The root cause was that Bun kept file descriptors open for watched directories. Since inotify watches by inode, keeping the FD open kept the inode alive, preventing
IN_DELETE_SELFevents from being generated when the directory was deleted viarmdir().Changes
IN_DELETE_SELFevents by emitting rename and cleaning up thefile_pathsHashMap entryunrefPendingDirectoryby releasing mutex before calling deinitTest Plan
test/regression/issue/23306.test.tspassfs.watchtests pass (29 pass, 2 pre-existing failures unrelated to this change)🤖 Generated with Claude Code