DO NOT MERGE: control run for watcher ignore-rule detection - #108
Closed
Shengyu Fu (shengyfu) wants to merge 6 commits into
Closed
DO NOT MERGE: control run for watcher ignore-rule detection#108Shengyu Fu (shengyfu) wants to merge 6 commits into
Shengyu Fu (shengyfu) wants to merge 6 commits into
Conversation
`is_ignore_rules_file` decides whether a filesystem event should rebuild and republish the matcher in `ServerState::gitignore`. It recognized `.gitignore` at any depth and root-level `p4ignore.ini`, but not `.ignore`. `.ignore` is a first-class ignore source everywhere else: the walk collects it separately, the matcher applies it, and it even outranks `.gitignore`. Unlike `.gitignore` it is not git-gated, so it is the one source that works outside a repository. Because the event never matched, a `.ignore` written while the server was live never scheduled the refresh, and the write then fell through to the reindex path where `should_skip_watcher_path` drops any dot-prefixed segment. Nothing happened at all: the startup matcher stayed published and files under the newly excluded directory kept being indexed until the hourly reconcile or a restart. Match `.ignore` by file name alongside `.gitignore`, using the existing `tgrep_core::gitignore` filename constants. `p4ignore.ini` stays root-scoped, mirroring the walker. Fixes #104 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The watcher already discarded events for ignored paths, but only after the OS had delivered them. On Linux that is too late to matter: inotify has no recursive mode, so notify's `RecursiveMode::Recursive` walks the tree and spends one watch descriptor per directory. A repository whose `target/` or `node_modules/` holds most of its directories therefore burns most of the per-user `fs.inotify.max_user_watches` budget on events that are thrown away -- and because notify propagates the first registration failure, exhausting that budget makes `watch()` return an error and the server loses its watcher entirely. Subscribe per directory on inotify backends instead. `watchable_dirs` walks the tree once, pruning ignored, hidden and `--exclude`d subtrees before descending, and `WatchRegistry::sync` reconciles the live subscription set against it. The sync runs whenever the ignore matcher is published, so relaxing a rule subscribes to the tree it used to hide and tightening one drops it. A directory that cannot be subscribed is now reported and skipped rather than taking down the whole watcher. Non-recursive watches are not extended by notify, so a directory that appears at runtime is picked up in `watch_new_subtree`, which also indexes the files already inside it to close the create race. Windows (ReadDirectoryChangesW) and macOS (FSEvents) subscribe once for the whole subtree, so there is no per-directory registration to withhold; they keep the single recursive watch and delivery-time filtering. The behaviour the two paths must share -- new directories get indexed, new directories under an ignored path do not -- is tested everywhere. Along the way, `state.gitignore` had three publish sites and only one of them went through the helper. They are unified behind `publish_ignore_matcher` so the sync hook cannot be missed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
`watch_new_subtree` merged the new subtree into the live subscription set by cloning both into a union and handing that to `sync`. That is correct but proportional to the whole watched set, and it runs once per directory created at runtime -- so on a repository holding tens of thousands of watched directories, a checkout or a build that creates many directories does quadratic work copying `PathBuf`s. Split the additive half of `sync` into `add_all` and call that instead. It iterates only the new subtree and tests membership, so the cost is proportional to what actually appeared. `sync` keeps its prune-and-add behaviour for the whole-tree case and now shares the same code. The distinction matters beyond performance: passing a subtree to `sync` would treat the entire rest of the repository as stale and unsubscribe from it, so a new folder would silently disable file watching. Added a test that pins both behaviours. Measured against the layout reported by the Office monorepo team -- 40,210 directories, ~200 searchable files, `.git` both hidden and `--exclude`d -- the subscription set is 202 directories, computed in 13.7ms. The walk cost is set by the tree that survives pruning, not the physical tree: the same measurement over 8,458 directories takes 12.6ms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
The watcher fixes on this branch change observable behaviour on Linux -- `tgrep serve` no longer takes an inotify watch per directory -- and the Office monorepo report that corroborated the bug was filed against the bundled 1.0.2. A distinct version is what lets that team tell whether a build contains the fix. Both crates inherit `version.workspace`, so the manifest change is one line; `Cargo.lock` is regenerated with `cargo update --workspace` rather than hand-edited. Note that the third-party `equivalent` crate is also at 1.0.2 and is deliberately untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Subscriptions were established only after the walk that produced the ignore matcher, so a file written to a directory in that window was in neither place: not in the walk's results, and not able to report itself yet. It stayed invisible until the hourly reconcile. sync_watch_registrations now returns the directories it newly subscribed to, and the stale check rechecks them once the merge has settled. It has to be after the merge: stream_merge_stale_changes replaces file_stamps wholesale, so an earlier scan would be discarded and would re-read every changed file on the way. reindex_file compares stamps first, so on a tree that did not move under us this costs one metadata call per file. The two index-build publishes deliberately skip the scan. There "newly watched" is the whole repository, and the stale check that follows startup already does a full walk-versus-index diff, which is a superset. Scanning there would stat the entire tree while holding the gate, on the path a warm start exists to keep fast. watch_new_subtree had two more problems of its own. It read each directory before subscribing to it, leaving the same race one level down for anything created in between; it now subscribes to a level before enumerating it. And a subtree that arrives already populated -- a clone, a mv, a branch switch -- can carry its own .gitignore. Those files are dot-prefixed, so the recovery scan dropped them silently and indexed the rest of the subtree against rules that had never heard of it. It now looks for ignore rules first and defers to a refresh rather than indexing under stale ones. Finally, Path::is_dir follows symlinks. A link to a directory was therefore subscribed to and walked through, indexing a target the walker never descends into and that may sit outside the root entirely. is_real_dir asks the question we actually mean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Member
Author
|
Control experiment complete. With the ignore-rule detection disabled, ubuntu fails exactly as intended: 'watcher indexed a file excluded by a .gitignore that arrived inside the same subtree'. Confirms the new test in #105 is not vacuous on the only platform that exercises per-directory watches. Closing and deleting the branch. |
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
1 participant
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.
Temporary control experiment. Disables the ignore-rule detection added in #105 to confirm the new e2e test actually fails without it on Linux. Will be closed and the branch deleted as soon as CI reports.