fix(repo_metadata): cache failed-walk paths and exclude Windows system dirs from watcher#11448
Open
cbeaulieu-gt wants to merge 4 commits into
Open
fix(repo_metadata): cache failed-walk paths and exclude Windows system dirs from watcher#11448cbeaulieu-gt wants to merge 4 commits into
cbeaulieu-gt wants to merge 4 commits into
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @cbeaulieu-gt on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
Contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Overview
This PR reduces Windows file-tree watcher churn by caching paths whose subtree walk exceeded the repo file limit and by filtering known noisy Windows AppData system directories before watcher events reach repo metadata processing. The new regression tests cover cache hits, component-boundary behavior, and Windows-only system-directory filtering.
Concerns
- The Windows system-directory filter currently matches the AppData patterns at any component position, which can drop watcher events for a legitimate project subtree with the same component sequence. This is worth tightening, but it is not blocking for the reported home-directory CPU issue.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Author
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
cbeaulieu-gt
added a commit
to cbeaulieu-gt/warp
that referenced
this pull request
May 20, 2026
…ata root Previously `is_system_dir_excluded_windows` matched the AppData pattern anywhere in the path component list, meaning a workspace subtree like `C:\workspace\fixtures\AppData\Local\Temp\sample.tmp` would be silently excluded from file-tree watcher events even though it is not the user's real system temp directory. Addresses oz-bot suggestion on PR warpdotdev#11448 (discussion_r3277342979): anchor the match to `%USERPROFILE%\AppData` before checking the known noisy sub-paths (Local\Temp, Local\Microsoft\Windows, LocalLow). If USERPROFILE is unset the function falls back to the previous unanchored scan on a best-effort basis. Also updates the existing Windows-only regression tests to construct paths dynamically from the real %USERPROFILE% (the old tests used a hardcoded `C:\Users\TestUser` path that would never match on CI) and adds a new test `test_watch_filter_workspace_appdata_subtree_not_excluded` covering the false-positive case: a workspace path that contains the AppData segments as a middle component must NOT be excluded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
|
Hey @cbeaulieu-gt, thanks for sending this our way. I'll take a look at this tomorrow and leave some more detailed feedback! |
cbeaulieu-gt
force-pushed
the
cbeaulieu-gt/fix-watcher-temp-dir
branch
from
4ddfc01 to
6946f40
Compare
cbeaulieu-gt
added a commit
to cbeaulieu-gt/warp
that referenced
this pull request
May 21, 2026
…ata root Previously `is_system_dir_excluded_windows` matched the AppData pattern anywhere in the path component list, meaning a workspace subtree like `C:\workspace\fixtures\AppData\Local\Temp\sample.tmp` would be silently excluded from file-tree watcher events even though it is not the user's real system temp directory. Addresses oz-bot suggestion on PR warpdotdev#11448 (discussion_r3277342979): anchor the match to `%USERPROFILE%\AppData` before checking the known noisy sub-paths (Local\Temp, Local\Microsoft\Windows, LocalLow). If USERPROFILE is unset the function falls back to the previous unanchored scan on a best-effort basis. Also updates the existing Windows-only regression tests to construct paths dynamically from the real %USERPROFILE% (the old tests used a hardcoded `C:\Users\TestUser` path that would never match on CI) and adds a new test `test_watch_filter_workspace_appdata_subtree_not_excluded` covering the false-positive case: a workspace path that contains the AppData segments as a middle component must NOT be excluded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rom watcher Adds a `failed_walk_paths` cache on `LocalRepoMetadataModel` to short-circuit re-walks of paths that previously hit `ExceededMaxFileLimit`, and extends `WatchFilter` with a platform-specific system-dir exclude list (Windows: AppData\Local\Temp, AppData\Local\Microsoft\Windows, AppData\LocalLow; case-insensitive, component-wise match). Reduces CPU when the file-tree view is rooted at the user's home directory on Windows. Companion to warpdotdev#11350 (cancellable in-flight walks; out of scope for this PR). fixes warpdotdev#11349
…t unbounded-cache tradeoff Adds `test_failed_walk_cache_respects_component_boundaries` as a regression guard against `StandardizedPath::starts_with` silently switching to byte-prefix semantics in a future `typed-path` upgrade. Asserts that a cached `Temp` path does NOT short-circuit a sibling `Tempest` directory. Also expands the doc-comment on `LocalRepoMetadataModel::failed_walk_paths` to note the unbounded-growth tradeoff and the conditions under which an eviction policy would be worth adding. Both changes follow up on pre-PR review feedback for warpdotdev#11349 — no behavior change, just stronger guard rails.
Caught by `./script/presubmit` (cargo clippy --workspace --all-targets --tests -- -D warnings) — the per-crate clippy run during initial implementation didn't include `--tests` so the unused-variable lint wasn't surfaced.
…ata root Previously `is_system_dir_excluded_windows` matched the AppData pattern anywhere in the path component list, meaning a workspace subtree like `C:\workspace\fixtures\AppData\Local\Temp\sample.tmp` would be silently excluded from file-tree watcher events even though it is not the user's real system temp directory. Addresses oz-bot suggestion on PR warpdotdev#11448 (discussion_r3277342979): anchor the match to `%USERPROFILE%\AppData` before checking the known noisy sub-paths (Local\Temp, Local\Microsoft\Windows, LocalLow). If USERPROFILE is unset the function falls back to the previous unanchored scan on a best-effort basis. Also updates the existing Windows-only regression tests to construct paths dynamically from the real %USERPROFILE% (the old tests used a hardcoded `C:\Users\TestUser` path that would never match on CI) and adds a new test `test_watch_filter_workspace_appdata_subtree_not_excluded` covering the false-positive case: a workspace path that contains the AppData segments as a middle component must NOT be excluded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cbeaulieu-gt
force-pushed
the
cbeaulieu-gt/fix-watcher-temp-dir
branch
from
6946f40 to
465f391
Compare
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
Cuts sustained idle CPU on Windows when the file-tree view is rooted at the user's home directory. Two complementary mitigations:
failed_walk_pathscache onLocalRepoMetadataModelshort-circuits re-walks of paths that previously hitExceededMaxFileLimit, so a singleAppData\Local\Tempwrite doesn't trigger a fullEntry::build_treere-walk on every filesystem event.WatchFilterkeeps known-noisy system directories (Windows:AppData\Local\Temp,AppData\Local\Microsoft\Windows,AppData\LocalLow) out of the recursive watcher entirely. Case-insensitive, component-wise match.Closes #11349.
Companion architectural fix (cancellable in-flight walks) is tracked as #11350 and is out of scope for this PR.
Empirical verification (Windows 11)
Reproducer from the issue: file tree view rooted at
%USERPROFILE%, with Office / browser / AV background activity touchingAppData\Local\Temp.Entry::build_treecallsExceededCommits
c4f1001— fix(repo_metadata): cache failed-walk paths and exclude system dirs from watcher0994217— test(repo_metadata): pin component-aware cache short-circuit, document unbounded-cache tradeoff (regression guard againstStandardizedPath::starts_withswitching to byte-prefix semantics in a futuretyped-pathupgrade — asserts cachedTempdoes NOT short-circuit siblingTempest)3c27727— test(repo_metadata): drop unused child_path binding in cache test (caught by./script/presubmit)Tradeoffs / known limitations
failed_walk_pathsis unbounded. Documented in the field's doc-comment alongside the conditions under which an eviction policy would be worth adding. In practice the set is bounded by the number of distinct subtrees in the workspace that exceedMAX_FILES_PER_REPO, which is small (typically O(10) on a home-dir watch).Test plan
cargo test -p repo_metadata— all green, includes the new component-aware cache test./script/presubmit— full workspace clippy + tests greenRelated
Entry::build_tree(architectural follow-up)🤖 Generated by Claude Code on behalf of @cbeaulieu-gt