Fix watcher rebuild storm: skip re-indexing gitignored directories#13151
Fix watcher rebuild storm: skip re-indexing gitignored directories#13151acarl005 wants to merge 7 commits into
Conversation
Removes the [watcher-diag] error-level instrumentation added to diagnose the rebuild storm; the fix in the previous commit stands on its own. Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
This comment was marked as resolved.
This comment was marked as resolved.
…red parent as ignored in incremental watcher updates, matching the initial index
|
/oz-review |
This comment was marked as resolved.
This comment was marked as resolved.
|
/oz-review |
This comment was marked as resolved.
This comment was marked as resolved.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the repo metadata watcher path so incremental events under gitignored directories insert unloaded placeholders instead of rebuilding whole ignored subtrees, while preserving force-included paths such as skill providers and adding regression coverage.
Concerns
- For this user-facing performance/behavior change, please include screenshots or a short screen recording demonstrating it working end to end under ignored-directory churn, for example opening a new session while a
cargo buildupdatestarget/. The description includes useful manual before/after results, but the repo review guidance requires visual evidence for user-perceivable behavior changes.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Description
Fixes a bug where the repository file watcher re-indexed entire gitignored directories on every filesystem change. On Windows each re-walk is slow, blocking I/O that runs on the background-executor's worker threads, so the walks saturate the entire background thread pool and starve other background work - most visibly, new terminal sessions bootstrap far more slowly because their startup tasks queue behind the walks.
On a git repository, an incremental watcher update (
compute_file_tree_mutationsincrates/repo_metadata/src/local_model.rs) calledEntry::build_tree_with_standing_queriesfor every newly-created directory, even gitignored ones. Inside the tree walk,evaluate_entrychecks gitignore against the literal path only (check_ancestors = false), so a deep ignored path such astarget/debug/.fingerprintwas not recognized as ignored and its whole subtree was walked. During acargobuild this re-walked ~27,000 files per event (50-90s each), fanned across the background-executor pool.The background-executor is a tokio runtime with one worker thread per logical core (
num_cpus). On the repro machine (16 logical cores) the dump showed all 16 workers parked in the walk, so the entire runtime was busy and nothing else scheduled on it could run until a walk finished. (Task Manager showed only ~10-20% CPU because the walks are I/O-bound and mostly waiting on disk - that number understates the saturation.)The fix: in
compute_file_tree_mutations, when an added directory is gitignored (using the ancestor-awarepath_is_ignored), insert an unloaded placeholder instead of walking it, exactly how the initial index represents ignored directories (IgnoredPathStrategy::IncludeLazy). Force-included paths (e.g. skill-provider dirs) are exempted so they still materialize.Root cause was found by symbolicating a Windows process dump (all 16 background-executor workers were in
repo_metadata::entry::Entry::build_tree_with_force_included_paths_and_ancestordoing filesystem syscalls, starving other background tasks) and confirmed with temporary watcher instrumentation that is removed in this PR.Created with Warp Agent Mode: https://app.warp.dev/conversation/3ac9dd74-82ae-4401-aea2-ae0e8bfc8b0d
Linked Issue
Closes #13153
Testing
Ran the app locally (OSS build via
cargo run) with repositories open and triggered heavytarget/churn with a fullcargo build:target/debug/.fingerprintproduced full-subtree rebuilds (files=27245 elapsed_ms=53021-style, 50-90s each) that occupied every worker thread; new sessions were slow to bootstrap and background updates lagged.target/paths with zero subtree rebuilds; the pool stayed free and sessions/background work were unaffected.Confirmed with temporary
[watcher-diag]logging that was used to capture the before/after and then removed in this PR.cargo checkandcargo clippypass onrepo_metadata.No new automated tests were added; the change brings the incremental watcher path back in line with the initial index's existing ignored-directory handling. Happy to add a
compute_file_tree_mutationsunit test (ignored dir under a git repo gives an unloaded placeholder, not a walk) if reviewers prefer../script/run(viacargo run)Screenshots / Videos
N/A - no UI changes.
CHANGELOG-BUG-FIX: [Windows] Fixed background work (including new terminal sessions) being starved and slow to start, plus sustained disk usage, caused by repeatedly re-indexing gitignored directories (e.g. build output like
target/) when many files changed at once.