[travsr-daemon] Fix #801: stop watching SKIP_DIRS, and bound the raw event queue

[ritikpal1122]

Closes #801.

Diagnosis confirmed against master (a498453) before changing anything: grep -n unwatch returns only the two comment lines, metadata at :212 precedes should_skip_all at :224, and :108 is an unbounded channel().

Three fixes

1. Skip dirs are unwatched. Two doc comments promised this and no code did it.

Not only a cost fix. spawn's contract, which lib.rs:9271 relies on before binding the control socket, is that .travsr/ is unwatched by the time it returns, "otherwise kqueue opens the socket file and gets ENOTSUP, crashing the entire watch setup". That guarantee has never held. The existing mitigation at lib.rs:9268 removes a stale socket from a previous run; it does nothing about the one created moments after spawn() returns. So this may also explain some silent-staleness reports.

2. The cheap prefix test runs before the stat. Ordering only, a skipped path was discarded either way.

3. The raw channel is bounded and sheds. It sat upstream of the debounce table's MAX_PENDING cap, so that cap never applied to it. The callback uses try_send, not send: it runs on the notify thread, and blocking that stalls the OS event source itself.

Platform finding, not in the issue

unwatch needs per-directory watches. I probed rather than assumed:

PROBE: unwatch ERR kind=WatchNotFound msg=No watch was found.

macOS uses FSEvents (fsevent-sys is in the lockfile, macos_kqueue is not enabled), which streams one recursive watch from the root and has no per-directory watch to remove. So fix 1 helps on inotify, the platform the 13 MB/s failure was measured on, and cannot help on the macOS default backend.

That makes fixes 2 and 3 the mitigations there rather than optional extras, and it is recorded on the unwatch loop so it is not later "simplified" as redundant.

The regression test, and why it is shaped this way

The obvious test does not work. Asserting "no reindex happened for target/" passes on the broken code: should_skip_all genuinely works and always did, so the event is discarded either way. That indistinguishability is exactly why the two existing filter tests passed while the daemon grew 13 MB/s.

The only externally observable difference is raw events counted before filtering, so WatcherHandle now exposes raw_events(). Failure output before the fix:

400 writes under target/ produced 840 raw events: the tree is still watched,
so every file is queued and stat'd before the skip filter discards it (#801).

It carries a control arm asserting a watched directory does move the counter. Without it the test passes if the counter simply never increments, which is the same shape of self-agreeing guard that let this ship.

#[cfg(target_os = "linux")], for the backend reason above: on FSEvents the property is unachievable, and asserting it there would pin behaviour no code can deliver.

A test I wrote, mutation-checked, and threw away

I first wrote the queue test as "flood the watcher, assert it keeps accepting". Mutation-checking it against the unbounded channel:

MUTATED: back to unbounded channel
test raw_event_queue_is_bounded_under_flood ... ok

It passed, because an unbounded channel also keeps accepting. It discriminated nothing. Replaced with a deterministic assertion of the shed policy, whose doc comment states plainly what it does not cover: forcing a real overflow through the watcher is timing-dependent, since the consumer drains about as fast as a test loop can create files.

Verification

cargo test -p travsr-daemon                    168 passed, 0 failed
cargo clippy --workspace --all-targets -- -D warnings   clean
cargo fmt --all -- --check                     clean
check-em-dash.sh                               OK

One file changed. The Linux-only test runs on test (ubuntu-latest) in CI, which is where it was verified to fail before the fix.

[ritikpal1122]

Swept the file for the same bug classes before review, per the ask. Found one real gap in this fix, and closing it exposed a second bug underneath. Both pushed in 4453038.

The fix only covered half the filter

should_skip_all discards SKIP_DIRS and gitignored paths. The unwatch loop walked SKIP_DIRS alone, so a repo whose ignored build/, vendor/ or .venv/ holds tens of thousands of files stayed fully watched and paid the entire cost this PR exists to remove.

Nothing would have caught it, for the same reason nothing caught the original: the userspace filter discards those events either way, so the only symptom is cost.

The unwatch set is now derived from the filter rather than restating it, so the watched set and the discarded set cannot disagree. A .travsrignore re-include (!vendored/) keeps its watch for free rather than by remembering to special-case it, and unwatching that would have silently stopped indexing a tree the user explicitly asked to index.

Mutation checked: reverting to SKIP_DIRS-only fails with build must be unwatched; got {target, node_modules}.

Writing the test portably found a second bug

The decision test failed immediately:

build must be unwatched; got {"build/obj", "target", "node_modules"}

should_skip_all asks the gitignore matcher with is_dir = false, which is correct for events, where the path is usually a file and often already deleted. But a build/ rule is directory-only, so asking about the directory build itself with false answers "not ignored". The walk then descended into the subtree it meant to prune and returned build/obj after already walking all of build: pruning defeated, and children unwatched instead of the root.

Split into should_skip_all (files) and should_skip_dir, over one shared body so the SKIP_DIRS half cannot drift.

That bug was only reachable because the decision is now split from the unwatch call and asserted on every platform. As an end-to-end test it would have been Linux-only, since unwatch is a no-op on FSEvents, and would have left this logic unverified on the machine the repo is developed on.

Also swept, nothing found

• Other unbounded channels: ready_tx (:197) is one-shot by design; lib.rs:9260 is already bounded at 256. No other instance of the class.
• Re-include correctness: pinned explicitly, per above.

Verification

cargo test -p travsr-daemon                             169 passed, 0 failed
cargo clippy --workspace --all-targets -- -D warnings   clean
cargo fmt --all -- --check                              clean
check-em-dash.sh                                        OK

Still one file changed.

[raj-rkv]

Really thorough piece of work, and the writeup made this easy to review. The diagnosis matches what I see in the code, all three fixes are real, and I especially appreciate two things: the honesty about the mutation-checked test you threw away, and the raw_events() counter, which is genuinely the only externally observable difference and gets the regression test out of the "self-agreeing guard" trap you called out.

What I verified

• cargo test -p travsr-daemon: 169 passed, 0 failed
• cargo clippy -p travsr-daemon --all-targets -- -D warnings: clean (the one warning is the pre-existing MSRV note from clippy.toml)
• cargo fmt --all -- --check: clean
• Branch is up to date with master at a498453, no rebase needed
• Read notify 8.2.0's inotify.rs to check the unwatch semantics rather than take them on trust. remove_watch(path, false) on an entry stored with is_recursive = true does remove the descendants too, so fix 1 genuinely drops the subtree on inotify. Your FSEvents finding also checks out.

Deriving the unwatch set from the filter instead of restating SKIP_DIRS is the right call, and the re-include case (!vendored/) falling out for free is a nice consequence rather than a special case.

Two things I ran into

I have left both inline. Neither is a reason to hold the PR in my view, since it clearly makes things better than master, but the first one narrows the fix more than the description suggests and I think it is worth knowing before this lands.

1. Skip dirs created after spawn() are re-watched by notify. The unwatch is a one-shot snapshot, and notify re-adds a recursive watch for any directory created later under a recursive parent. I measured 1604 raw events from a target/ created after spawn, against the under-40 your own test expects when it exists at spawn. So cargo clean && cargo build or npm install on a fresh clone brings #801 back.

2. A self-sustaining rebuild loop that this PR does not touch. Pre-existing, not yours, but it turned up while I was checking fix 3 and I think it is closer to the real driver of the 13 MB/s than the queue is. One touch of .gitignore on an idle single-directory repo produced ~190k events/sec indefinitely, on both this branch and master.

Suggestion

Merge-wise I am happy for this to go in: the bounded queue is the part that stops a workstation from hard-locking, and that is the critical half of #801. My ask is that we open follow-ups for the two items above rather than let the issue close as fully fixed, since #801's headline symptom is likely to survive this PR. Happy to file both if that helps.

One small thing on the description: fix 3 is doing less than "the raw channel sheds" implies. In the loop above raw_dropped stayed at 0, because the consumer keeps pace on a small repo. It is the RSS ceiling that matters here, not the shedding, and that is worth stating plainly since shedding drops real edits when it does engage.

[raj-rkv]

The unwatch is a one-shot snapshot, so a skip dir created after this point comes back.

notify re-adds a recursive watch on its own for any directory created later whose parent is watched recursively. From notify 8.2.0 inotify.rs:

fn add_watch_by_event(...) {
    if event.mask.contains(EventMask::ISDIR) {
        if let Some(parent_path) = path.parent() {
            if let Some(&(_, _, is_recursive, _)) = watches.get(parent_path) {
                if is_recursive {
                    add_watches.push(path.to_owned());

target/ is a direct child of repo_root, which is watched recursively, so creating it after spawn() pulls the whole subtree back under watch. That happens inside notify's own event loop before our callback ever runs, so should_skip_all cannot head it off.

I ran your test shape with target/ absent at spawn:

PROBE: 400 writes under a POST-SPAWN target/ produced 1604 raw events

against the under-40 you get when it exists at spawn. So the daemon regains #801 behaviour after cargo clean && cargo build, npm install on a fresh clone, or rm -rf dist && npm run build, all with the daemon already running. Meeting the daemon on a fresh clone and then building is a fairly ordinary first experience, which is what makes me want to flag it.

The bounded queue keeps this from reaching 10 GB, so I do not think it blocks the PR. One option, if you want it in scope: when a Create event names a directory the filter would skip, unwatch it there, roughly where the ignore-file rebuild already sits in the loop. That reuses should_skip_dir and stays derived from the filter the same way this does.

Related, and by inspection rather than measurement: the same one-shot property cuts the other way. If someone removes build/ from .gitignore while the daemon runs, the loop below rebuilds the matcher and starts accepting build/ events, but the watch was dropped at spawn and nothing re-adds it, so the tree stays silently unindexed until a restart. On master it was watched all along, so this is new. I could not measure it cleanly because the loop I describe in my other comment swamps the counter as soon as an ignore file is touched, so please sanity-check my reasoning rather than take it as verified.

Would you rather I open these as follow-up issues and keep this PR to the three fixes? Happy either way.

[raj-rkv]

Pre-existing, not introduced here, but it changes how I read fix 3, so raising it where the code is.

is_ignore_file matches on any event kind, Access(Open) included. build_ignore_matcher then does two full WalkDir passes and calls builder.add() on every ignore file it finds, which opens them, which inotify reports as Access(Open) on .gitignore, which matches is_ignore_file again. It feeds itself.

One touch of .gitignore, on an idle repo with a single directory and nothing else writing:

PROBE3 idle_before=4  after_1_touch(+3s)=553883  (+6s)=1120680
PROBE3 events in the last quiet 3s window: 566797
PROBE3 raw_dropped=0

About 190k events/sec, indefinitely. The same probe on master gives 651,966 in the same window, so this branch trims it a little but the loop survives. Anything that reads .gitignore can start it: a git status, an editor, ripgrep, or the rebuild's own walk.

This fits #801's evidence better than the queue alone does: 60,600 read syscalls/sec, 8.8 MB/s rchar, and growth that kept going while the embed sidecar was SIGSTOPed. On a 19,317-directory repo each rebuild walks the tree twice, so the consumer falls a long way behind the producer and the unbounded channel grows, which would be the ~13 MB/s.

Where that lands on fix 3: raw_dropped=0 above shows the shed policy never engages in this loop, because the consumer does keep pace on a small repo. The bounded queue still fixes the RSS blowup and the machine lock, which is the part that actually hurt someone, so this is not an argument against it. But the CPU spin outlives this PR, and on a large repo the shedding that does kick in will drop real edits.

Not asking you to fix it here. It looks like a small change (gate the rebuild on Create/Modify/Remove and skip Access), so a follow-up issue seems right, and it would be worth #801 staying open for it rather than closing as fully fixed.

[raj-rkv]

Small one, and only because shedding is silent by design: raw_dropped is reachable only through raw_dropped(), and nothing outside tests calls it. If the queue ever does shed on a user's machine, the only trace is the throttled tracing::warn!, which is easy to miss when the symptom shows up later as "my graph is stale".

Surfacing it in daemon status next to the existing counters would make that self-diagnosing. Worth it given the shed policy is deliberately lossy.

Also, for the record rather than as an ask: #801 suggested asserting the drop counter increments, and a_full_raw_queue_sheds_instead_of_blocking exercises std::sync::mpsc::sync_channel directly rather than this counter or this channel. Your doc comment says so plainly and explains why the honest version is hard, which I would much rather have than a test that quietly proves nothing. Wiring the counter into daemon status would also give it a natural place to be asserted later.

[raj-rkv]

Tiny nit, take it or leave it: this new line uses an em-dash. The CI gate skips // comments by design so it passes, but CLAUDE.md asks for none in code comments either. A comma reads the same here ("Signal ready, watch is established and ..."). Not worth a CI re-run on its own, just if you end up touching the file again.