[Dan Elksnitis (dpelksnitis)]

Summary

The file watcher's notify callback ran handle_fs_event inline. That handler takes snapshot_gate.read(), which an index flush / auto-save holds in write mode for the entire flush — seconds to minutes on large repositories. Blocking the callback that long overflows the OS event buffer, so filesystem changes made during a flush are silently dropped and the index drifts out of sync until the next restart.

Change

Move event processing to a dedicated worker thread fed by an mpsc channel. The notify callback now only enqueues events, so it always returns promptly and the OS buffer keeps draining. Events that arrive while a flush holds the gate wait in the in-memory queue and are processed once the flush completes — no events are lost.

cargo fmt --all --check, cargo clippy --all-targets, and cargo test pass.

[Copilot]

Pull request overview

This PR updates the tgrep serve file-watching pipeline so filesystem events are no longer processed inline in the notify callback (which can block for long periods on snapshot_gate during flush/auto-save), reducing the risk of OS watcher buffer overflow and dropped events.

Changes:

• Introduces an mpsc channel and a dedicated worker thread to drain and process notify::Events.
• Updates the notify callback to only enqueue events and return quickly.
• Keeps existing handle_fs_event behavior intact, but moves where it executes (worker thread vs callback thread).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Dan Elksnitis (dpelksnitis)]

I am waiting on internal approval from Epic before I can agree to the CLA, hopefully this won't take too long.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

[Dan Elksnitis (dpelksnitis)]

@microsoft-github-policy-service agree company="Epic Games"

[Dan Elksnitis (dpelksnitis)]

Thanks — addressed all three in the latest push (5998414):

• --watcher-queue-cap parsed as usize (main.rs): dropped the u64 + as usize cast in favour of a usize value parser (parse_positive_usize), so a value that doesn't fit the target's pointer width is rejected at parse time instead of silently truncating, and the min-of-1 check is preserved. The cast at the call site is gone.

• Stale-refresh baseline (serve.rs): the diff now baselines on the persisted on-disk stamps and overlays the in-memory stamps (freshest wins) rather than using the in-memory map alone. This keeps the baseline complete — an incomplete in-memory map (e.g. an older index whose filestamps.json was empty at load) no longer makes existing files look "added" — while still reflecting mid-session updates so an overflow-triggered refresh reliably repairs dropped events.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

tgrep-cli/src/serve.rs:867

• As written, the overflow flag can trigger repeated full background_refresh_stale runs if the queue keeps overflowing while a refresh is in progress (the callback can set overflowed again before the next loop iteration). This can lead to back-to-back full-tree walks/flushes under heavy churn. Consider coalescing/rate-limiting overflow-triggered refreshes so at most one runs per short interval.

                if worker_overflowed.swap(false, Ordering::Relaxed) {
                    eprintln!("[trace] watcher queue overflowed; reconciling with a stale refresh");
                    background_refresh_stale(&worker_state, &worker_root, &index_dir);
                }

tgrep-cli/src/serve.rs:1286

• read_filestamps errors (I/O/JSON parse) are currently silently swallowed via unwrap_or_default(), which can hide corruption and makes the later "no filestamps yet" message misleading. Consider logging the error and explicitly falling back to an empty baseline (still allowing the in-memory overlay to proceed).

    let mut old_stamps = tgrep_core::meta::read_filestamps(index_dir).unwrap_or_default();

[Shengyu Fu (shengyfu)]

Two overflow-recovery correctness issues to address before merging.

[Shengyu Fu (shengyfu)]

The overflow refresh runs after processing only one queued event, before draining older events that were queued before the overflow. If the queue contains Remove(file) and a recreate event is dropped, the refresh indexes the recreated file and the older queued removal then deletes it again. Please drain or discard all pre-overflow queued events before performing the reconciliation.

[Shengyu Fu (shengyfu)]

This reconciliation compares only file size and whole-second mtime (as_secs()). A dropped same-size modification within the same second is therefore considered unchanged and leaves stale indexed content indefinitely. Please retain higher-resolution timestamps or force reindexing for paths/events affected by an overflow.

[Shengyu Fu (shengyfu)]

Dan Elksnitis (@dpelksnitis), sorry for the delayed review.

[Shengyu Fu (shengyfu)]

Thanks Dan — your diagnosis was right and the fix has landed in #95, with you as a co-author.

The reasoning in your description is the part that convinced me: doing file I/O and trigram extraction inside the notify callback stalls the platform notification thread, and on Windows that thread owns a fixed-size ReadDirectoryChangesW buffer, so everything arriving during the stall is dropped by the OS with no error we can observe. That failure mode is indistinguishable from "the watcher stopped working", which is exactly what makes it worth fixing.

#95 keeps your structure — bounded sync_channel, try_send from the callback, a worker thread doing the real work — with a couple of adaptations to current main:

• Overflow recovery waits for a quiet interval. The worker reconciles on recv_timeout expiry rather than immediately, so one overflow triggers a single stale check instead of one per event still queued behind it.
• It defers to in-flight work. If an index build is running or the ignore matcher hasn't been published yet, the overflow flag is left set — both of those paths end with their own stale check, which reconciles the same drift, and racing them would just duplicate the walk.
• The merged-stamps change wasn't needed. main now seeds state.file_stamps at construction, so an overflow-triggered refresh already sees existing files rather than treating them as new.

The queue bound is exposed as --watcher-queue-cap (default 16384) as you had it.

Closing in favour of #95. Thanks — this was a good catch on a genuinely invisible failure.

[Shengyu Fu (shengyfu)]

Correction to my previous comment — I was wrong about the merged-stamps change.

I said the old_stamps merge "wasn't needed because state.file_stamps is seeded from read_filestamps() at startup". That's true only at startup. The watcher updates state.file_stamps on every indexed change, so between flushes the in-memory map diverges from filestamps.json on disk. An overflow-triggered stale check that reads only the on-disk baseline therefore has two real failure modes:

1. Files indexed since the last flush get re-classified as "new" and redundantly re-indexed.
2. Worse: a file created and then deleted inside that window is in neither the on-disk stamps nor the filesystem. It is never classified as deleted, so it lingers in the index indefinitely.

The second one is a genuine correctness bug and your change prevents it. I've now implemented it in background_refresh_stale — the on-disk stamps are used as the baseline and the in-memory ones folded in on top. Thanks for catching this; my original reasoning stopped at the constructor and missed the watcher's writes.