Keep the watcher off the index until the gitignore matcher exists by shengyfu · Pull Request #94 · microsoft/tgrep

Shengyu Fu (shengyfu) · 2026-08-19T06:46:46Z

The bug

On a warm start (a complete index already on disk), the file watcher indexes gitignored files.

ServerState.indexing is initialized to needs_build. On a warm start that is false, so the indexing guard in handle_fs_event never engages — but state.gitignore is RwLock::new(None) and is only populated later, on a background thread. In that window should_skip_watcher_path gets None and applies no ignore rules at all, so the watcher indexes exactly the build output the initial walk was careful to skip. The next flush persists it.

On a big repo the matcher build is slow enough for this to matter, so the window is wide open in practice.

Reproduction

Built a 30k-file fixture as a real git repo with build/ in .gitignore, ran serve, and wrote build/leaked.txt right after file watcher started:

[trace] reindex: modified build/leaked.txt

The content was searchable, and Files: 30003 confirmed it reached disk. The stale check independently reported "index is up-to-date", which proves the watcher was the only path writing that file into the index.

A note for anyone extending the test: the ignore crate only applies .gitignore inside a git repo (require_git defaults to true and we never override it). A fixture without a .git directory silently ignores .gitignore entirely — my first repro was invalid for exactly this reason.

The fix

Two parts, both needed:

A gitignore_pending: AtomicBool gate. Set at construction when the watcher will actually use ignore rules (!no_watch && !no_ignore), cleared at all three matcher-publish sites. While set, handle_fs_event drops events rather than indexing them unfiltered.
Reordering warm start so the matcher is built before background_refresh_stale.

The reorder is what makes the gate safe: events dropped during the gap are recovered by the stale check that follows. I verified this rather than assuming — an edit made inside the window is dropped by the watcher and then picked up by the stale check.

Only the first publish is gated. Later rebuilds (e.g. an edited .gitignore) swap the matcher atomically and leave the previous one readable, so events keep being filtered by slightly stale but valid rules instead of being dropped.

No wedge risk: both bootstrap_index_build failure paths fall through to background_index_build, which publishes and clears the flag. build_gitignore_matcher_after_ready also clears the flag when the matcher is None, since a repo with no ignore rules is a legitimate final answer rather than a missing matcher.

Verification

A/B'd pre-fix and fixed binaries with an identical probe script: pre-fix LEAKED, fixed clean (no results).
Regression checked that live incremental indexing still works after startup (an edit and a new file both FOUND, a gitignored file absent).
New integration test warm_start_gitignore.rs — uses a real (empty) .git dir so .gitignore is honored, hammers a gitignored file for 3 s to remove the race, and asserts a positive control (a normal source edit is indexed) before asserting the leak marker is absent. Verified to fail without the fix (left: 1, right: 0).
Full workspace suite green, clippy -D warnings clean, fmt applied.

On a warm start the server has a complete index on disk, so it never sets `indexing` and the watcher is live from the first event. The `.gitignore` matcher, though, is built on a background thread. In that gap `should_skip_watcher_path` sees `gitignore == None`, applies no ignore rules, and indexes exactly the build output the walk skips. Reproduced on a 30k-file git repo: writing `build/leaked.txt` right after "file watcher started" logged `reindex: modified build/leaked.txt` and the content stayed searchable, while the stale check correctly reported the tree up to date -- the watcher was the only way it got in. Gate the watcher on a `gitignore_pending` flag, and build the matcher before the stale check rather than after, so the stale walk that follows recovers the events dropped during the gap. Verified: an edit made inside the window is dropped by the watcher and then picked up by the stale check. Only the first publish is gated. Later rebuilds swap the matcher atomically and leave the previous one readable, so events keep being filtered by slightly stale but valid rules instead of being dropped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9162aa2a-9639-412f-9c72-29d8418bb961

Copilot

Pull request overview

Fixes a warm-start race where the file watcher could index gitignored build output before the .gitignore matcher is available, permanently polluting the on-disk index.

Changes:

Add a gitignore_pending atomic gate to prevent the watcher from touching the index until the initial matcher publish completes.
Reorder warm-start initialization so the matcher is built before the stale-refresh pass.
Add an integration regression test covering the warm-start watcher/gitignore gap.

Show a summary per file

File	Description
tgrep-cli/src/serve.rs	Introduces `gitignore_pending` and warm-start ordering changes to ensure watcher events are filtered (or dropped/recovered) until ignore rules are ready.
tgrep-cli/tests/warm_start_gitignore.rs	Adds an integration test to reproduce and prevent the warm-start gitignore indexing leak.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

Copilot

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

Review details

Suppressed comments (1)

tgrep-cli/tests/warm_start_gitignore.rs:161

This hammer loop runs for a fixed 3s before wait_for_port(). If tgrep serve takes longer than 3s to become reachable on a slow/loaded CI runner, the writes can finish before the watcher is active, and the test may pass even without the fix (no event occurred during the warm-start window). Consider waiting for the server (and ideally watcher_active) before starting the hammer loop to make the regression test robust.

    // Hammer a gitignored file for the whole startup window. Rewriting in a
    // loop rather than once removes the race: whatever the exact moment the
    // watcher comes up and the matcher lands, some write falls between them.
    let ignored = root.join("build").join("leaked.txt");
    let writer_deadline = Instant::now() + Duration::from_secs(3);
    while Instant::now() < writer_deadline {
        let _ = fs::write(&ignored, "gitignored_leak_marker build artifact\n");
        thread::sleep(Duration::from_millis(10));
    }

    let server = ServerGuard {
        child,
        port: wait_for_port(&index_dir),
    };

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

Two fixes from review feedback. The regression test could pass vacuously. It hammered the gitignored file for a fixed 3s *before* waiting for serve to be ready, so on a slow or loaded runner every write could land before the watcher existed, no event would reach the matcher-build gap, and the test would report success having exercised nothing. The writer now runs on its own thread that starts before serve is spawned and keeps going until two seconds after the port opens, so writes straddle the whole startup window regardless of how long it takes. Write errors are no longer discarded: successful writes are counted and asserted non-zero, so a probe that never ran says so instead of masquerading as a pass. Verified the restructured test still fails against the unfixed serve.rs from main (left: 1, right: 0), so the added robustness did not come at the cost of the thing it exists to catch. The warm-start thread comment also claimed "The watcher is already live by now", which is false -- start_file_watcher runs after this thread is spawned. Correctness never depended on that ordering, because the gitignore_pending gate is armed during ServerState construction, before either starts. Comment now says that instead. Also take ownership of the serve child process immediately after spawn so it is reaped even if the readiness wait times out and panics.

Copilot

Review details

Files reviewed: 2/2 changed files
Comments generated: 0 new
Review effort level: Lite

Updates the workspace version, which both crates inherit, and syncs Cargo.lock in the same commit. Every release target builds with --locked (release.yml and sign-and-release.yml), so a bump that leaves the lockfile behind fails all six builds rather than producing a wrong version -- verified here by running the exact release command, `cargo build --release --locked -p tgrep-cli --bin tgrep`. fuzz/Cargo.lock is deliberately untouched: fuzz is not a workspace member, cargo fuzz does not run with --locked, and that lockfile has long since diverged (it still pins tgrep-core 0.1.4).

Copilot