feat(scheduler): add WatchMode enum and mtime polling fallback for FileWatchStrategy#868
Conversation
geoffjay
left a comment
There was a problem hiding this comment.
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().awaitconsumes 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::Eventstructure as native events — the downstreamFileWatchStrategy::next_tasksprocesses 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 theRecommendedWatcherclosure; the originaltxis dropped at end ofcreate_handle - Native failure: the cloned
txandpaths.clone()are dropped with the failed watcher; the originaltxis 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
WatchModeenum design is clean —Pollingcarriesinterval_secs,Autocarriespoll_interval_secsas a separate field rather than reusing the same name, which is correct since they serve slightly different semantic roles ✅Default→Auto { poll_interval_secs: 5 }is a sensible production default ✅FileWatchHandleas an internal drop-guard enum cleanly replaces the_watcher: notify::RecommendedWatcherfield without changing the externalFileWatchStrategyAPI ✅_handlerename is consistent with the "intentional drop-guard" naming convention already used in the codebase ✅"rename"removed from theevent_kindsdoc comment — correct, the mtime poller can't generate rename events ✅
Approving. The symlink traversal and task-linger notes are follow-up suggestions.
Add
WatchModeenum (Native/Polling/Auto) andMtimePollingWatcherbackground task that scans file mtimes viatokio::time::interval, detecting creates/modifies/deletes and forwarding syntheticnotify::Events through the same mpsc channel as the native watcher.FileWatchStrategy::new()now accepts aWatchModeparameter.Closes #811