Repository navigation
Conversation
On Linux the host-service watched each worktree with chokidar 4, which opens an fs.watch on every file and every directory and crawls the tree on the host event loop with two lstat calls per entry. For an LLVM worktree (17k directories, 186k files) that is 203k inotify watches, a ~9 s attach with event-loop stalls near 0.5 s, and ~725 MB of heap. While it runs, terminals and every other request to the host wait. An inotify watch on a directory already reports writes, attribute changes, creates, deletes and renames of its children, so the new backend watches directories only, as @parcel/watcher does. It watches a directory before listing it, and reconciles every reported path against lstat and the recorded children through one queue, which also tells create from update. The same LLVM worktree attaches in ~3 s with 17k watches, stalls of a few milliseconds and ~50 MB of heap. A write made through a hard link outside the tree is no longer reported, since inotify only reports a directory's children by the path used. The request gains an abort signal so that a cancelled attach stops crawling. Fixes ParadevAI#8324
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe workspace filesystem replaces its Linux Chokidar backend with a directory-only backend. The backend reconciles filesystem events and supports aborting an attachment in progress. Other platforms continue to use the Parcel backend. ChangesLinux directory-only watching
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant FsWatcherManager
participant directoryWatchBackend
participant DirectoryWatch
participant fs.watch
participant lstat
FsWatcherManager->>directoryWatchBackend: subscribe with abort signal
directoryWatchBackend->>DirectoryWatch: create and attach
DirectoryWatch->>fs.watch: watch directory before listing
fs.watch-->>DirectoryWatch: report directory event
DirectoryWatch->>lstat: classify queued path
lstat-->>DirectoryWatch: return current path state
DirectoryWatch-->>FsWatcherManager: deliver create, update, or delete event
Merge Risk: ⚪ Minimal · up to No merge-blocking issue was established; the watcher retains a recovery path when the watched root is replaced. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
attach() drained the event queue until it was empty. A process that keeps writing to the tree refills the queue as fast as it is drained, so subscribe() never resolved: with a script creating files during attach, 2 of 3 attaches were still pending after 5 s under Node. Settle only what the crawl queued and drain later changes after attaching; the same script now attaches in about 90 ms.
211428a to
1c67e08
Compare
What & why
On Linux the host-service watches each worktree with chokidar 4, which opens an
fs.watch(one inotify watch) on every file and every directory, and crawls the tree on the host event loop with twolstatcalls per entry. On a large repository that blocks the host: with several LLVM worktrees (186k files each) open, the vitals log showedFSEventWrapswinging between ~100 and 420k, event-loopbusyShareat 1.0 and delays up to 4.4 s. During those stalls, opening a terminal hangs, and so does every other request to that host. Each such worktree also uses ~200k of the user'sfs.inotify.max_user_watches.A directory's inotify watch already reports writes, attribute changes, creates, deletes and renames of its children, by name, so the per-file watches only duplicate it. This PR replaces the chokidar backend with one that watches directories only, as
@parcel/watcher(and so VS Code) does on Linux, and as chokidar's unreleased v6 does. It watches a directory before listing it. Callbacks only queue the path they name, and one queue reconciles each path againstlstatand the children recorded per directory. That is also how a create is told apart from an update. A replaced directory is detected by its device and inode, and an event about a watched directory itself reopens its watch.NativeWatchRequestgains an optional abort signal, so a cancelled attach stops crawling instead of finishing a crawl nobody will use.The one behaviour change is that a write made through a hard link outside the tree is no longer reported, because inotify reports a directory's children by the path that was used. Runtime watch errors such as ENOSPC still go to the same log-only
onUnexpectedErroras before. Moving the crawl into a worker, and using this backend forgit-directory-worker.ts(whose recursivefs.watchon Linux also watches every file), are possible follow-ups. This is complementary to #7878 and #7879, which reduce how often a worktree is re-crawled; this PR reduces what each crawl costs.Fixes #8324
How I tested it
LLVM worktree (17k directories, 186k files), Node 22.13 on a 32-core Linux machine under load, two runs each:
llvm/test(72.5k entries) out of the treeBoth report the same 72,532 events for that move, out of the tree and back in. A harness ran both backends side by side through 21 operations: create, append, atomic save, chmod,
mkdir -pfollowed by an immediate write, file and directory renames,rm -rf, file↔directory replacement, symlinks, a 200-file burst, and a directory moved in from outside. AftercoalesceWatchEvents, the events matched except in the two hard-link cases.The shared backend suite now also runs the two inotify race tests that parcel skips. New tests cover:
These tests passed 100/100 runs on Linux. The rest of the workspace-fs suite and the host-service watcher tests pass on Linux, except
search.adversarial3.test.ts, which fails the same way on main. Bun'sfs.watchdrops a second event for the same name within a few milliseconds (Node does not), so two tests pause briefly before recreating an entry.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Replaces the chokidar backend for Linux worktree watching with a directory-only backend using one inotify watch per directory. On large worktrees this cuts attach time from ~9 s to ~3 s, drops inotify watches from ~200k to ~17k per worktree, reduces event-loop stalls from hundreds of milliseconds to a few, and prevents host hangs with multiple large worktrees open (#8324). Attach also no longer hangs when the tree keeps changing: it settles what the crawl queued and drains later events after attaching.
Behavior changes
NativeWatchRequestgains an optionalAbortSignalso a cancelled attach stops crawling instead of finishing a crawl nobody will use.Written for commit 1c67e08. Summary will update on new commits.
Summary by CodeRabbit