Skip to content

feat(scheduler): add WatchMode enum and mtime polling fallback for FileWatchStrategy#868

Open
geoffjay wants to merge 2 commits intoissue-810from
issue-811
Open

feat(scheduler): add WatchMode enum and mtime polling fallback for FileWatchStrategy#868
geoffjay wants to merge 2 commits intoissue-810from
issue-811

Conversation

@geoffjay
Copy link
Copy Markdown
Owner

Add WatchMode enum (Native/Polling/Auto) and MtimePollingWatcher background task that scans file mtimes via tokio::time::interval, detecting creates/modifies/deletes and forwarding synthetic notify::Events through the same mpsc channel as the native watcher. FileWatchStrategy::new() now accepts a WatchMode parameter.

Closes #811

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): add WatchMode enum and mtime polling fallback for FileWatchStrategy

Clean, well-structured implementation. The polling watcher integrates seamlessly with the existing mpsc channel bridge, and the Auto fallback path handles the native-unavailable case correctly. Two non-blocking observations worth tracking.


Stack context

issue-811 is the second branch in the 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. Parent PR #867 should land first.

Note: FileWatchHandle variants have no #[allow(dead_code)] in this PR — clippy will flag them as dead_code on this branch in isolation. This is expected and intentional: the annotation is added by PR #876 at the top of the stack. The stack is clean as a whole.


snapshot_mtimes — correct, one caveat

The iterative stack traversal is the right choice here — avoids stack overflow on deeply nested directories, and the let Ok(walker) = std::fs::read_dir(root) else { return map } guard handles inaccessible roots gracefully.

Caveat — symlinks to directories are followed:

if meta.is_dir() {
    if let Ok(sub) = std::fs::read_dir(&path) {
        stack.push(sub);
    }
}

std::fs::metadata follows symlinks, so a symlink pointing to a directory inside the watched tree is traversed. A symlink pointing outside the watched tree (e.g. /etc) would be scanned too. Using entry.file_type() (which does not follow symlinks) would scope the walk to the actual watched subtree. For most practical use cases this is low risk, but worth tracking for environments where watched directories may contain external symlinks.


spawn_polling_watcher — correct

  • interval_secs.max(1) guards against zero-interval spinning ✅
  • Initial ticker.tick().await consumes the immediate first tick, giving the caller time to establish the baseline snapshot before the first comparison fires ✅
  • Send-error check (if tx.send(...).is_err() { return; }) correctly exits when receiver is dropped ✅
  • Synthetic events use the same notify::Event structure as native events — the downstream FileWatchStrategy::next_tasks processes them identically ✅

Non-blocking: polling task lingers up to interval_secs after strategy drop:
Dropping JoinHandle<()> in Rust/Tokio detaches but does not abort the task. The polling task exits naturally only when the next tx.send fails (i.e., after FileWatchStrategy and its rx are dropped, the task runs until the next tick). With interval_secs = 1 this is a ~1s linger; with larger intervals it could be longer. For long-lived workflow triggers this is harmless, but if workflows are frequently toggled, polling tasks could briefly stack up. Adding an abort call on drop or passing a shutdown watch::Receiver into the task would fix this cleanly — but it's a follow-up, not a blocker for the current use case.


WatchMode::Auto fallback — correct

The tx.clone() and paths.clone() in the Auto match arm are necessary to support the fallback path without consuming the values on a failed native attempt. The lifetime of each clone is correct:

  • Native success: tx.clone() lives inside the RecommendedWatcher closure; the original tx is dropped at end of create_handle
  • Native failure: the cloned tx and paths.clone() are dropped with the failed watcher; the original tx is moved into the polling task ✅

The info! / warn! log lines on native success/fallback are good for operators diagnosing why polling mode is active. ✅


create_native_watcher extraction — good

Extracting the native watcher construction into its own method is the right call — it's reused in both Native and Auto branches, and the error mapping (map_err with informative messages) is applied in one place. ✅


Minor: interval_secs.max(1) is a silent clamp

If a caller passes WatchMode::Polling { interval_secs: 0 }, the interval is silently changed to 1 second. A tracing::warn! when clamping would help operators notice the misconfiguration. The API validation layer (#875) prevents poll_interval_secs == 0 at the API boundary, but FileWatchStrategy::new is also called directly in tests and could be called from non-API paths.


What's good

  • WatchMode enum design is clean — Polling carries interval_secs, Auto carries poll_interval_secs as a separate field rather than reusing the same name, which is correct since they serve slightly different semantic roles ✅
  • DefaultAuto { poll_interval_secs: 5 } is a sensible production default ✅
  • FileWatchHandle as an internal drop-guard enum cleanly replaces the _watcher: notify::RecommendedWatcher field without changing the external FileWatchStrategy API ✅
  • _handle rename is consistent with the "intentional drop-guard" naming convention already used in the codebase ✅
  • "rename" removed from the event_kinds doc comment — correct, the mtime poller can't generate rename events ✅

Approving. The symlink traversal and task-linger notes are follow-up suggestions.

@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