Skip to content

feat(scheduler): implement FileWatchStrategy with notify crate#867

Open
geoffjay wants to merge 2 commits intoepic-804from
issue-810
Open

feat(scheduler): implement FileWatchStrategy with notify crate#867
geoffjay wants to merge 2 commits intoepic-804from
issue-810

Conversation

@geoffjay
Copy link
Copy Markdown
Owner

Add FileWatchStrategy that subscribes to OS-native filesystem events (inotify/FSEvents/kqueue) via the notify crate, with globset pattern filtering, event-kind filtering (create/modify/delete), and a debounce window that resets on each burst event.

Closes #810

Copy link
Copy Markdown
Owner Author

@geoffjay geoffjay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(scheduler): implement FileWatchStrategy with notify crate

Well-structured base implementation. The sync-to-async bridge, glob filtering, event-kind filtering, and resettable debounce are all correct. A couple of minor observations, none blocking.


Stack context

issue-810 is the bottom of a four-deep stack: epic-804 (#866)issue-810 (#867)issue-811 (#868)issue-812 (#875)issue-813 (#876). The epic-804 base shows needs restack against main, so the whole stack inherits that status. Adding needs-restack.


Dependency choices — correct

notify = "6" in crates/orchestrator/Cargo.toml is the filesystem watcher crate from crates.io — not { path = "../notify" } which would pull in the agentd notification service. The Cargo.lock correctly shows both notify 0.2.0 (agentd notification service) and notify 6.1.1 (filesystem watcher) as distinct packages. ✅

globset = "0.4" pinned directly, consistent with other non-workspace deps in this crate. ✅


Sync-to-async bridge — correct

let (tx, rx) = mpsc::unbounded_channel();
let mut watcher = notify::RecommendedWatcher::new(
    move |res: notify::Result<notify::Event>| {
        let _ = tx.send(res);
    },
    notify::Config::default(),
)?;

notify's watcher callback runs on a background thread (sync context). Bridging through mpsc::unbounded_channel is the correct pattern — the UnboundedSender is Send, and let _ = tx.send(res) ignores errors that occur when the receiver has been dropped during shutdown. ✅

_watcher held in the struct as a drop guard is correct — dropping the watcher stops watching and the background thread exits. ✅


next_tasks — correct two-phase logic

Phase 1 (first event): The select! loop correctly handles all four cases: matching event (break), non-matching event (continue), watcher error (log + continue), channel closed (return empty). The shutdown.changed() arm checks *shutdown.borrow() after firing, which is the right guard against spurious wakeups if the value is still false. ✅

Phase 2 (debounce): The resettable timer pattern is correct:

loop {
    let sleep = tokio::time::sleep(debounce);
    tokio::pin!(sleep);
    tokio::select! {
        _ = &mut sleep => { break; }        // window expired
        maybe_event = self.rx.recv() => { /* loop to reset */ }
        ...
    }
}

A new sleep future is created at the top of each iteration. When any event arrives, the loop restarts with a fresh debounce-duration sleep, effectively resetting the timer. The old pinned future is dropped. This is idiomatic tokio and correct. ✅


Minor: debounce resets on non-matching events

During the debounce phase, Some(Ok(_)) matches all received events — matching or not. A non-matching event (wrong kind, excluded by glob) still resets the debounce timer. This is a conservative design choice (any filesystem activity in the watched tree extends the quiet period), but in high-noise directories (e.g., watching /var/log for a specific pattern) the debounce window may never expire. Worth a doc comment, not a blocker.


Minor: "rename" in event_kinds doc comment is a dead filter value

The doc comment lists "create", "modify", "delete", "access", "rename" as accepted event kind strings. However, event_kind_to_str never returns "rename" — rename events in notify arrive as Modify(ModifyKind::Name(_)) and map to "modify". A caller filtering on "rename" would never receive a task. This inaccuracy is already corrected by PR #876 at the top of the stack (which removes "rename" from the doc). Non-blocking since it's a doc-only issue fixed upstack.


build_task — correct

All five required metadata keys are populated: file_path, file_name, file_dir, event_type, timestamp. The source_id format "file:<path>:<kind>" is consistent with how the template variable event_type references these in PR #875. ✅

Using event.paths.first() for single-file events is correct; multi-path events (rare, mostly internal notify bookkeeping) silently use only the first path. ✅


matches_event — correct

Empty event_kinds → accept all; patterns None → accept all. The any(|k| k == kind_str) and any(|p| patterns.is_match(p)) semantics are correct (OR across patterns, not AND). ✅


Glob ordering note

In new(), the glob set is built after the watcher is created and paths are registered. If glob validation fails, the native watcher is dropped immediately (no background task to clean up), so there's no resource leak. The "validate before spawn" concern is more critical for the polling task in PR #868. For this native-only PR the ordering is acceptable — but the fix in PR #876 (validating before spawning) is the right long-term approach for consistency. ✅


Summary

Correct, clean base implementation. The two minor observations (debounce-on-all-events, "rename" doc inaccuracy) are both addressed further up the stack. Approving; restacking needed once epic-804 is current with main.

@geoffjay geoffjay added needs-restack Branch is behind its stack parent, needs git-spice restack and removed review-agent Used to invoke a review by an agent tracking this label labels Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-restack Branch is behind its stack parent, needs git-spice restack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant