control r10 (do not merge) - #117
Closed
Shengyu Fu (shengyfu) wants to merge 20 commits into
Closed
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
The kernel releases an inotify watch by itself when its directory is deleted or moved away, and nothing reports that the descriptor is gone. The path stayed in `watched`, so a directory recreated at the same location looked subscribed while receiving no events at all. Nothing downstream could recover it either. `add_all` skipped it as already watched, and so did every later `sync`: the path is in `desired` *and* in `watched`, which is indistinguishable from a live subscription. The entry stayed poisoned for the life of the process, so `rm -rf build && mkdir build`, a branch switch or a `git clean` silently stopped the directory being watched until the server restarted. Two cheap halves. `forget` clears the entry when a removal event arrives, which is a single hash lookup — deleting a tree delivers one event per directory in it, so anything proportional to the whole watched set would make that quadratic. And `watch_new_subtree` now re-issues subscriptions rather than trusting `watched`, since a directory that has just appeared is precisely the case where that belief is worthless; `inotify_add_watch` is idempotent, so re-adding costs a syscall and returns the existing descriptor. The forced path still reports only genuinely new directories, so the recovery scan does not treat a whole subtree as freshly watched. Descendants carried off by a move deliver no events of their own, but the next sync no longer finds them under the root and unsubscribes them there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Three follow-ups from review, all cases where the watcher and the walk could disagree about the index. 1. The directories returned by the three non-stale subscription passes were discarded. The justification comments were wrong: on a warm start the stale check runs on a thread spawned before the watcher, so it can publish while the registry is still empty, making the startup sync the first descendant pass with nothing following it. Files written to a directory between the walk reaching it and the subscription being taken were then invisible until the hourly reconcile. All three sites now hand their directories to spawn_recovery_scan, which waits out `indexing` before looking -- during a cold build the stamps are not yet written and every file would read as changed. 2. watch_new_subtree ran for every directory event, including Modify(Metadata). A recursive chmod or a branch switch re-walked and re-subscribed each subtree once per directory in it. Gated to the kinds that can actually introduce a directory: Create and Modify(Name). 3. reindex_file ignored the walker's binary-extension and max-filesize rules, so a file arriving through the watcher was indexed even when a walk of the same tree would reject it -- and the next reconcile deleted it again. The check now mirrors walk_file_metadata, and drops any entry it already holds when a file stops being eligible. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Symlink escape (high). `reindex_file` stat'd and read through `std::fs::metadata`, which follows links. The indexer walks with `follow_links(false)`, where a symlink is neither file nor dir and is skipped outright, so a link inside the repository had its target's bytes indexed under the link's own path — and the target need not be under the served root. Switched to `symlink_metadata` and made `is_file` on the link's own metadata an eligibility rule, so a link falls into the branch that drops whatever was indexed there before. Conditional drop. That branch only deleted when a stamp entry existed, so a stamp map that no longer describes the overlay left ineligible content searchable. It now also consults the live overlay. Deliberately not unconditional the way the removal branch is: removals are rare, this runs for every ineligible file a recovery scan passes, and `delete_file` records a tombstone even for a path that was never indexed. Stamp publication race. `background_index_build` assigned `state.file_stamps` after dropping the publish gate, but cleared `indexing` before it. The recovery scan waits on `indexing` and then on the gate, so it ran in that window against empty stamps: every file read as changed and the whole repository was re-read — exactly what the wait exists to prevent — and the assignment then discarded the stamps the scan had just recorded. The stamps are now published inside the gate, before the flag flips, and the flush is handed a read guard rather than a clone. Recovery scan gaps. `reindex_files_in` looked only at files directly inside each newly watched directory. It now also picks up subdirectories created in the same window (filtered by subscription membership, so the startup case stays a hash lookup apiece rather than a re-walk per level), drops entries for files removed in it, and defers to an ignore-rules refresh when it finds an ignore file that landed inside the window. That last test is bounded at both ends by mtime so a clock-skewed network mount cannot arm a refresh that arms the next one. Over-subscription. `watch_new_subtree` recorded an ignore-rules file and kept descending, taking a watch descriptor per level of a tree the rules it was about to publish would exclude — in the moved-in `node_modules` case this change exists to fix. It now abandons the descent at the point of discovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Review round 5. Read the file through one no-follow handle. `symlink_metadata` and a later `std::fs::read` are two lookups of the same name, and a tree being rewritten underneath us can swap a regular file for a link in between, so the bytes indexed were not necessarily the ones judged eligible. `open_no_follow` opens the path itself -- O_NOFOLLOW on unix, the reparse point on Windows -- and the type, size, mtime and contents all come off that handle. Stop keying the ineligible-file drop on a stamp entry. `ServerState` accepts an empty stamp map when filestamps.json is missing or unreadable, and the reader can still hold the path in that state, so the delete now always happens. An existing tombstone is taken as proof there is nothing left to do, which keeps a startup scan over a repository full of binary assets from dirtying the overlay once per file. Anchor the recovery window at the walk that produced the matcher, not at the subscription sync that follows it. A nested `.ignore` written between the two carries an mtime predating a timestamp taken later and would be read as already accounted for, leaving its subtree indexed under rules that never saw it. `publish_ignore_matcher` now takes `since` from the caller. Replay the events discarded during the initial build. They could not be applied then -- the stamps do not describe the index yet -- but they are the only record that those paths moved, and the build's own walk misses anything written to a directory it has already passed. Buffered, capped, and replayed as synthetic create events once the build publishes, so they go through the same filtering an ordinary event gets; an overflowing burst falls back to a full reconcile. This is also the first recovery any of this offers on a whole-subtree backend, where there are no per-directory subscriptions to have raced. Include the root in the recovery scan. It is subscribed as the watcher starts, so every later sync sees it as already watched and it never appears in the newly-watched list -- nothing covered a file written to the top level while a build walk was deeper in the tree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Review round 6. Guard every path component, not just the last one. `O_NOFOLLOW` and `FILE_FLAG_OPEN_REPARSE_POINT` protect the file being opened and nothing above it, so `root/a/file` still resolves through `a` if `a` is a link -- and the file at the end of that is a perfectly ordinary file outside the tree we serve, which is exactly the containment the walker's `follow_links(false)` promises. `open_within_root` resolves the path a component at a time from the root: with `openat` on unix, where the name is never re-resolved and there is no window to swap a directory for a link, and by checking each ancestor on Windows, where there is no `openat` and creating a symlink needs a privilege that is not granted by default. Non-literal components are refused rather than interpreted. Serialize the check-read-commit cycle in `reindex_file`. The snapshot gate is held for read by everything that indexes a file, so a recovery scan and the watcher worker can both be inside it for the same path, both see the same old stamp, and the one that read the older content can commit last -- the newer event already consumed, the stale version surviving until the next reconcile. The new lock is taken per file, so the two interleave rather than one waiting out the other, and searches do not take it at all. Also open with `O_NONBLOCK`, so a fifo in the tree answers immediately instead of blocking the watcher until someone opens the write end. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Five follow-ups from review, all about a decision made on evidence that does not support it. Replaying a deferred event reconstructed it as a creation regardless of what actually happened, so a recursive chmod during a build put every directory through watch_new_subtree. The flag that distinguishes them is now carried with the path. The deferral itself re-checked `indexing` outside the buffer lock, so an event could land in a set that had already been replayed and swapped out. It is now read under the lock, which is what makes the handoff provable: the replay cannot swap without that lock. reindex_file treated any failure to open as proof of ineligibility, so a permission error or a Windows sharing violation evicted content that was still perfectly valid — while the stale path, deliberately, keeps unreadable files and retries them. Only errors that establish something structural drop the entry now. A recovery scan could see an ignore file that arrived, by its mtime, but not one that was deleted, since a deleted file leaves nothing to stat. The published matcher's sources are recorded and checked directly. And containment on Windows was still check-then-open: an ancestor could become a junction between the check and the open. The handle is now asked where it ended up, which cannot be raced, and the per-ancestor stat walk goes away with it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
…hat still exists Three more from review, all about the watcher acting on evidence it does not have. read_dir's per-entry errors were being flattened away, and the directory was then recorded as fully enumerated. A name that failed to yield is simply missing from `present`, so the sweep read it as a deletion and tombstoned a file it had no reason to believe was gone. The directory now has to have enumerated cleanly before it can claim to have been swept, which is what a failed listing already did. An indexed file atomically replaced by a fifo, a socket, or a symlink to a directory is not a removal — the path still exists — and it is not a directory either, so the non-file branch returned without touching the index and the old contents stayed searchable. Real directories keep the subtree handling; everything else drops what was indexed. And notify registers inotify watches without IN_DONT_FOLLOW, so the descriptor lands on whatever the name resolves to at that instant, not on the directory the walk validated earlier. A swap in between left a descriptor watching outside the root while the registry recorded the name as covered. The registration is now re-checked no-follow and undone on a mismatch. That narrows the window rather than closing it, since notify takes a path and not a handle; what remains is missed events on a real directory, which the reconcile picks up, and never misplaced content, which open_within_root settles from the handle it reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
A control run showed it passing with the incomplete-listing fix reverted, which is correct: it exercises sweep_removed_files directly, so it guards the contract that withholding a directory is sufficient, not the wiring that withholds it. A per-entry readdir failure cannot be induced portably, so that half stays untested and the doc comment now says so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
…t the cap Two races the reviewer found in the reconcile path. The sweep deleted files on the strength of a directory listing taken earlier in the scan, without the lock `reindex_file` holds. A file recreated in between has already had its create event consumed by the watcher, so deleting it here lost it until the next reconcile with nothing left to replay. Each candidate is now rechecked against the filesystem while holding `reindex_lock`, so the observation the delete acts on is its own and cannot be overtaken. The content read was unbounded even though the size that qualified the file was stat'd before it. A file appended to in between -- a log, a build artifact -- was pulled into memory whole and indexed past `--max-filesize`. `read_within_limit` reads at most one byte past the cap and drops what the index holds when that byte is there. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
A control run with the read bound reverted left this test passing: the eligibility check catches a file that outgrew the cap between visits long before the read does. It pins that gate, not the new bound, and the name now says so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
Three defects the reviewer found, all in the reconcile path. A removal handled by the watcher mutated the index without `reindex_lock`, so a `reindex_file` already holding a file's bytes could commit them after the delete. That resurrects a file that is gone, with a fresh stamp, so nothing afterwards disagrees and no further event is coming to correct it. Both removal branches now take the lock; it is the caller's rather than `drop_indexed_file`'s because `reindex_file` calls in while already holding it. Arrival of an ignore file was detected by mtime alone, and `git checkout`, `tar -x` and `rsync -a` all restore mtimes from what they unpack -- so a nested `.gitignore` could arrive dated months ago and sail past the window. Absence from `ignore_sources` is the exact question instead: this file did not feed the published matcher. The mtime window stays for the case the source list cannot answer, an existing source that has just been edited. Ignore files reached through a symlink were dropped before either test ran. `DirEntry::file_type` does not follow links, but the walker collects rule files with `Path::is_file`, which does -- so those files carried rules that the recovery scan and the new-subtree descent were both blind to. The check now runs ahead of the type dispatch and follows links to decide. They are still never indexed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
The macOS runner caught this: `read_dir` promises no ordering, and there `.gitignore` routinely comes back after its siblings -- so a per-entry check indexes part of a directory under the stale rules before it ever reaches the file that changes them. Across the scan it is worse: rules in a directory later in `dirs` arrive after earlier ones were indexed. `changed_ignore_rules_in` answers for the whole scan up front, probing by name the way the walker discovers these files. That agrees with the walker by construction, is ordering-independent, and follows symlinks through `Path::is_file` -- which is also what makes a symlinked `.gitignore` visible, so the per-entry check it replaces is gone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bcc85b55-e821-447e-b9a9-144b78829ae4
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.
control