fix(watch): implement file watching for daemon auto-restart#255
fix(watch): implement file watching for daemon auto-restart#255
Conversation
File watching (watch field) was documented but not functional. The supervisor's file watcher was reading patterns from config files at startup rather than from daemon state, meaning: 1. Watch patterns weren't persisted to daemon state (watch/watch_base_dir fields missing from Daemon struct and state serialization) 2. File watcher used PitchforkToml::all_merged() at startup, missing dynamically started daemons 3. restart_watched_daemon() re-parsed config instead of using daemon state Changes: - Add watch and watch_base_dir fields to Daemon and RunOptions structs - Persist watch config in state file via UpsertDaemonOpts - Rewrite daemon_file_watch() to read from supervisor state instead of config - Add get_all_watch_configs() to dynamically fetch watch patterns from state - Make watcher refresh every 10s to pick up new daemons without restart - Fix restart_watched_daemon() to use daemon state values instead of config - Update all callers (autostop, retry, batch, lifecycle) to pass watch fields Fixes: File watching now triggers daemon restart when watched files change
Summary of ChangesHello @benjaminwestern, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a long-standing issue where the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request does a great job of implementing the file watching feature for daemon auto-restarts. The logic has been thoughtfully rewritten to be dynamic, reading from the supervisor state and allowing for new daemons to be picked up without a full restart. The propagation of watch configuration through the various parts of the system is also handled well. However, I've identified a critical regression in the run_adhoc function that completely breaks the command-line options for pitchfork run. This needs to be addressed before merging.
File watching (watch field) was documented but not functional. The supervisor's file watcher was reading patterns from config files at startup rather than from daemon state, meaning: 1. Watch patterns weren't persisted to daemon state (watch/watch_base_dir fields missing from Daemon struct and state serialization) 2. File watcher used PitchforkToml::all_merged() at startup, missing dynamically started daemons 3. restart_watched_daemon() re-parsed config instead of using daemon state Changes: - Add watch and watch_base_dir fields to Daemon and RunOptions structs - Persist watch config in state file via UpsertDaemonOpts - Rewrite daemon_file_watch() to read from supervisor state instead of config - Add get_all_watch_configs() to dynamically fetch watch patterns from state - Make watcher refresh every 10s to pick up new daemons without restart - Fix restart_watched_daemon() to use daemon state values instead of config - Update all callers (autostop, retry, batch, lifecycle) to pass watch fields Also includes: - fix: restore opts parameter in run_adhoc() to fix CLI options regression (fixes critical bug where --delay, --retry, --force, and readiness checks were ignored and hardcoded values were used instead) Fixes: File watching now triggers daemon restart when watched files change
|
@jdx Please tell me if there is anything that I can do to make this better for you! |
The recent merge from origin/main reverted the fix for the critical regression in run_adhoc() where CLI options were being ignored. This restores proper use of opts parameter: - shell_pid: opts.shell_pid (was None) - force: opts.force (was hardcoded true) - retry: opts.retry.unwrap_or(0) (was hardcoded 0) - ready_delay: opts.delay.or(Some(3)) (was hardcoded 0) - ready_output: opts.output (was None) - ready_http: opts.http (was None) - ready_port: opts.port (was None) - ready_cmd: opts.cmd (was None) Fixes: pitchfork run --delay, --retry, --force, and readiness checks
There was a problem hiding this comment.
Code Review
This pull request provides a comprehensive fix for the previously non-functional file watching feature. The changes correctly persist watch configurations in the daemon state, rewrite the watcher to dynamically read from this state, and use the state for restarts instead of re-parsing configuration files. The new watch and watch_base_dir fields are propagated consistently throughout the codebase. The core logic in watchers.rs is a significant improvement, introducing a periodic refresh to handle dynamically started daemons. I have one suggestion to further enhance the file watcher by handling the removal of watches for inactive daemons, which would prevent a minor resource leak. Overall, this is a well-executed and important fix.
Add unwatch method to WatchFiles and refactor the file watcher to: - Unwatch directories no longer needed by removed/reconfigured daemons - Canonicalize paths to deduplicate watches for the same directory - Handle unwatch failures by still removing from tracking set - Add daemon context to watch/unwatch log messages This prevents resource leaks from accumulating stale directory watches.
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the long-standing issue of non-functional file watching for daemon auto-restart. The changes correctly persist watch configurations in the daemon state, dynamically read patterns, and fix the restart logic to use stored state values. The rewrite of daemon_file_watch and the introduction of a refresh interval are significant improvements for reliability and responsiveness. The mise.lock file updates are incidental but reflect necessary tool version bumps. Overall, the solution appears robust and well-tested, resolving the identified critical flaws.
jdx
left a comment
There was a problem hiding this comment.
Code Review
The core fix is correct — reading watch configs from daemon state instead of re-parsing PitchforkToml::all_merged() at startup addresses the root cause well. The dynamic refresh loop and unwatch cleanup are solid additions.
Suggestions
1. Dead still_watched logic (src/supervisor/watchers.rs)
The still_watched clone + removal pattern is unnecessary since watched_dirs gets overwritten unconditionally at the end:
let mut still_watched = watched_dirs.clone();
for dir in watched_dirs.difference(&required_dirs) {
// ...
still_watched.remove(dir);
}
for dir in required_dirs.difference(&still_watched) { ... }
// ...
watched_dirs = required_dirs; // overwrites unconditionallyThis can be simplified to:
for dir in watched_dirs.difference(&required_dirs) {
wf.unwatch(dir);
}
for dir in required_dirs.difference(&watched_dirs) {
wf.watch(dir, RecursiveMode::Recursive);
}
watched_dirs = required_dirs;2. Inline std::collections usage — src/supervisor/watchers.rs uses std::collections::HashSet and std::collections::HashMap inline in the function body rather than importing at the top. The rest of the codebase uses top-level imports.
3. No tests — normalize_watch_path() and get_all_watch_configs() would be straightforward to unit test, and the watch state persistence round-trip through UpsertDaemonOpts is worth a test given the number of callers that need to propagate these fields correctly.
jdx
left a comment
There was a problem hiding this comment.
Correction on my earlier testing suggestion: unit tests for normalize_watch_path() / get_all_watch_configs() wouldn't catch the actual bugs this PR fixes. An integration test (Rust e2e or bats) that starts a daemon with watch configured, modifies a watched file, and asserts the daemon restarted (PID changed / logs show restart) would be much more valuable — and would match the existing test patterns in the repo.
- Simplify still_watched logic in daemon_file_watch - Move std::collections imports to top-level - Add unit tests for watch path normalization and patterns - Add integration tests for file watch restarts and persistence
There was a problem hiding this comment.
Code Review
This pull request provides a comprehensive fix for the file watching functionality, addressing several critical flaws in the original implementation. The rewrite of daemon_file_watch to dynamically read configurations from the supervisor state and periodically refresh is a significant improvement, making the feature robust and usable for dynamically started daemons. Persisting the watch configuration in the state file and ensuring it's propagated through all relevant code paths (autostop, retry, batch processing) is well-executed. The addition of extensive end-to-end tests in tests/test_e2e_watch.rs is excellent and provides strong confidence in the correctness of the new implementation.
I have found one potential medium-severity issue related to cross-platform path handling in src/watch_files.rs that could affect Windows users. Please see the specific comment for details.
| base_dir.to_path_buf() | ||
| }; | ||
| dirs_to_watch.insert(dir_to_watch); | ||
| dirs_to_watch.insert(normalize_watch_path(&dir_to_watch)); |
There was a problem hiding this comment.
The logic to determine the base directory for wildcard patterns might not be robust on Windows. The code at line 105 uses pattern.split('/') which assumes forward slashes as path separators. If a user on Windows specifies a pattern with backslashes (e.g., src\**\*.js), this logic will fail to correctly identify the non-wildcard prefix of the path.
To improve cross-platform compatibility, consider normalizing the pattern to use forward slashes before splitting, similar to what's done in path_matches_patterns:
let normalized_pattern = normalize_path_for_glob(pattern);
let parts: Vec<&str> = normalized_pattern.split('/').collect();
// ... rest of the logicThere was a problem hiding this comment.
Code Review
This is a solid pull request that fixes a non-functional watch feature. The approach to make file watching dynamic and driven by the supervisor's state is well-implemented. Persisting the watch configuration and using it during restarts and retries is a crucial fix. The addition of comprehensive unit and end-to-end tests is particularly commendable as it provides high confidence in the correctness of these significant changes. I have a couple of minor suggestions for improvement.
src/supervisor/watchers.rs
Outdated
| required_dirs.insert(dir.clone()); | ||
| dir_to_daemons | ||
| .entry(dir.clone()) | ||
| .or_default() | ||
| .push(id.clone()); |
There was a problem hiding this comment.
You are cloning dir twice here, which is unnecessary. You can save a clone by moving dir into the dir_to_daemons hash map after cloning it for required_dirs.
| required_dirs.insert(dir.clone()); | |
| dir_to_daemons | |
| .entry(dir.clone()) | |
| .or_default() | |
| .push(id.clone()); | |
| required_dirs.insert(dir.clone()); | |
| dir_to_daemons.entry(dir).or_default().push(id.clone()); |
src/watch_files.rs
Outdated
| /// Normalize a path by canonicalizing it if it exists, or making it absolute otherwise. | ||
| /// This ensures that different relative paths to the same directory are deduplicated. | ||
| fn normalize_watch_path(path: &Path) -> PathBuf { | ||
| path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) | ||
| } |
There was a problem hiding this comment.
The comment on line 64 is slightly inaccurate. The fallback path.to_path_buf() does not make a relative path absolute. This could lead to failed deduplication in watched_dirs if the same directory is referenced by both a relative and an absolute path. A more robust implementation would attempt to resolve the path to an absolute one even if canonicalize fails, for example by joining it with the current working directory.
/// Normalize a path by attempting to canonicalize it. If that fails, it attempts
/// to resolve it as an absolute path. This helps ensure that different relative
/// paths to the same directory are deduplicated.
fn normalize_watch_path(path: &Path) -> PathBuf {
path.canonicalize().unwrap_or_else(|_| {
if path.is_absolute() {
path.to_path_buf()
} else {
// Using a crate like `path-clean` would be even better to resolve `..`,
// but joining with CWD is a good improvement.
crate::env::CWD.join(path)
}
})
}- Remove unnecessary second clone of dir in watchers.rs - Fix normalize_watch_path to properly resolve relative paths to absolute
There was a problem hiding this comment.
Code Review
This pull request introduces a new file watching feature for daemons, allowing them to automatically restart when specified files or directories change. Key changes include adding watch patterns and watch_base_dir fields to daemon configurations, which are persisted in the supervisor's state. The file watching mechanism has been refactored to dynamically monitor directories based on the current daemon state, rather than static configuration, and now includes unwatch and path normalization utilities. Additionally, several dependencies in mise.lock have been updated to newer versions. The review comments highlight that the newly added end-to-end tests for the watch functionality, specifically test_watch_glob_patterns and test_watch_relative_paths, need to be strengthened. The reviewer suggests replacing fixed sleep durations with polling mechanisms to verify that daemon restarts (indicated by a change in PID) actually occur after file modifications, making the tests more robust and less prone to flakiness.
| // Modify a file in lib directory | ||
| fs::write(lib_dir.join("helper.ts"), "export const x = 1;").unwrap(); | ||
| env.sleep(Duration::from_secs(3)); | ||
|
|
||
| // Verify daemon is still running | ||
| let status = env.get_daemon_status("glob_watch_test"); | ||
| assert_eq!( | ||
| status, | ||
| Some("running".to_string()), | ||
| "Daemon should be running after lib file change" | ||
| ); | ||
|
|
||
| // Modify config file | ||
| fs::write(config_dir.join("app.json"), r#"{"port": 9090}"#).unwrap(); | ||
| env.sleep(Duration::from_secs(3)); | ||
|
|
||
| // Verify daemon is still running | ||
| let status = env.get_daemon_status("glob_watch_test"); | ||
| assert_eq!( | ||
| status, | ||
| Some("running".to_string()), | ||
| "Daemon should be running after config change" | ||
| ); |
There was a problem hiding this comment.
This test is intended to verify that glob patterns trigger a restart, but it only asserts that the daemon is still in a "running" state after the file modification. It doesn't confirm that a restart actually occurred (i.e., that the PID changed). The test should be strengthened to check for a PID change to ensure the watch functionality is working as expected for each pattern.
// Modify a file in lib directory
fs::write(lib_dir.join("helper.ts"), "export const x = 1;").unwrap();
let mut pid_after_first_change = original_pid;
for _ in 0..10 { // Poll for up to 5 seconds
pid_after_first_change = env.get_daemon_pid("glob_watch_test");
if pid_after_first_change != original_pid {
break;
}
env.sleep(Duration::from_millis(500));
}
assert_ne!(
pid_after_first_change, original_pid,
"Daemon should have restarted after lib file change"
);
assert!(pid_after_first_change.is_some());
// Modify config file
fs::write(config_dir.join("app.json"), r#"{"port": 9090}"#).unwrap();
let mut pid_after_second_change = pid_after_first_change;
for _ in 0..10 { // Poll for up to 5 seconds
pid_after_second_change = env.get_daemon_pid("glob_watch_test");
if pid_after_second_change != pid_after_first_change {
break;
}
env.sleep(Duration::from_millis(500));
}
assert_ne!(
pid_after_second_change, pid_after_first_change,
"Daemon should have restarted after config change"
);| // Modify the watched file | ||
| fs::write(&watched_file, "modified").unwrap(); | ||
| env.sleep(Duration::from_secs(3)); | ||
|
|
||
| // Verify daemon is still running | ||
| let status = env.get_daemon_status("relative_watch_test"); | ||
| assert_eq!( | ||
| status, | ||
| Some("running".to_string()), | ||
| "Daemon should be running after file change" | ||
| ); |
There was a problem hiding this comment.
Similar to other tests in this file, this test only asserts that the daemon is still "running" after a file change. It should be strengthened to assert that a restart has actually occurred by checking for a change in the process ID (PID). This will ensure the watch functionality is correctly triggering restarts for relative paths.
// Modify the watched file
fs::write(&watched_file, "modified").unwrap();
let mut new_pid = original_pid;
for _ in 0..10 { // Poll for up to 5 seconds
new_pid = env.get_daemon_pid("relative_watch_test");
if new_pid != original_pid {
break;
}
env.sleep(Duration::from_millis(500));
}
assert_ne!(
new_pid, original_pid,
"Daemon should have restarted after file change"
);
tests/test_e2e_watch.rs
Outdated
| // Wait for the file watcher to detect the change and restart | ||
| // File watcher checks every 1s (debounce) + some processing time | ||
| env.sleep(Duration::from_secs(3)); |
There was a problem hiding this comment.
Using a fixed sleep can lead to flaky tests, especially in CI environments where performance can vary. It's more robust to poll for the expected state change—in this case, a change in the daemon's PID. This makes the test more reliable and potentially faster if the restart happens quickly.
| // Wait for the file watcher to detect the change and restart | |
| // File watcher checks every 1s (debounce) + some processing time | |
| env.sleep(Duration::from_secs(3)); | |
| let mut new_pid = original_pid; | |
| for _ in 0..10 { // Poll for up to 5 seconds | |
| new_pid = env.get_daemon_pid("watch_test"); | |
| if new_pid != original_pid { | |
| break; | |
| } | |
| env.sleep(Duration::from_millis(500)); | |
| } |
…ility Strengthen watch functionality tests as requested in PR review: - Replace fixed sleep durations with PID polling to verify actual daemon restarts - Add assert_ne! assertions to confirm PID changes after file modifications - Make watch interval configurable via PITCHFORK_WATCH_INTERVAL_MS env var - Set fast 100ms interval in tests (vs 10s default) for quicker test execution - Add path canonicalization in TestEnv to handle macOS /var -> /private/var symlinks - Improve cleanup() to gracefully handle errors without panicking during Drop - Add strip_prefix(./) handling for relative watch patterns - Add unit tests for relative path pattern matching
There was a problem hiding this comment.
Code Review
This pull request provides a comprehensive fix for the non-functional file watching feature. The changes are well-structured, addressing the root causes by persisting watch configurations, rewriting the file watcher to be dynamic, and ensuring the configuration is propagated correctly throughout the system. The addition of extensive unit and end-to-end tests is excellent and ensures the new implementation is robust. I have one suggestion to improve performance and maintainability within the new file watcher loop.
src/supervisor/watchers.rs
Outdated
| _ = tokio::time::sleep(Duration::from_millis( | ||
| std::env::var("PITCHFORK_WATCH_INTERVAL_MS") | ||
| .ok() | ||
| .and_then(|v| v.parse().ok()) | ||
| .unwrap_or(10_000), | ||
| )) => { |
There was a problem hiding this comment.
For performance, the watch interval is read from the environment and parsed on every iteration of this loop. This is inefficient.
It would be better to read this value once outside the loop. For consistency with the rest of the codebase, you could add a new lazy_static to src/env.rs for PITCHFORK_WATCH_INTERVAL_MS.
Example of defining it outside the loop:
// Before the loop
let watch_interval = Duration::from_millis(
std::env::var("PITCHFORK_WATCH_INTERVAL_MS")
.ok()
.and_then(|v| v.parse().ok())
.unwrap_or(10_000),
);
// Inside the loop
tokio::select! {
// ...
_ = tokio::time::sleep(watch_interval) => {
// ...
}
}There was a problem hiding this comment.
Code Review
This pull request significantly improves the file watching functionality by addressing several critical flaws in the previous implementation. The changes introduce proper state persistence for watch configurations, rewrite the file watching logic to dynamically adapt to daemon state, and ensure that watch parameters are correctly propagated across various daemon lifecycle events. The addition of comprehensive unit and end-to-end tests provides strong validation for the new feature, covering various scenarios including persistence, glob patterns, and relative paths. The path normalization logic is a particularly good improvement for cross-platform consistency. Overall, this is a robust and well-implemented solution to enable reliable daemon auto-restart based on file changes.
|
@jdx I hope this is a much more well rounded implementation now |
Greptile SummaryThis PR implements file watching for daemon auto-restart, fixing a documented but non-functional feature. The implementation adds watch configuration persistence to daemon state and rewrites the file watcher to dynamically track daemons. Key changes:
Architecture improvement: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (start command)
participant Supervisor
participant State as State File
participant Watcher as File Watcher
participant FileSystem as File System
Note over CLI,FileSystem: Initial daemon start with watch config
CLI->>Supervisor: start daemon with watch patterns
Supervisor->>State: persist daemon state (includes watch config)
State-->>Supervisor: state saved
Supervisor->>Watcher: register daemon patterns
Note over Watcher: Every 10s: refresh configs from state
Watcher->>State: get_all_watch_configs()
State-->>Watcher: return watch configs for all daemons
Watcher->>Watcher: expand patterns to directories
Watcher->>Watcher: unwatch removed dirs, watch new dirs
Watcher->>FileSystem: watch directories (recursive)
Note over Watcher,FileSystem: File change detected
FileSystem->>Watcher: file modification event
Watcher->>Watcher: debounce (1 second)
Watcher->>Watcher: match changed path against patterns
Watcher->>State: get daemon state by ID
State-->>Watcher: return daemon (with cmd, env, watch config)
Watcher->>Supervisor: stop daemon
Watcher->>Supervisor: run daemon (using state values)
Supervisor->>State: persist updated daemon state
Note over Supervisor: Daemon restarted with new PID
Last reviewed commit: c01af2b |
Problem
The
watchfield inpitchfork.tomlwas documented but non-functional. Daemons with watch patterns configured would not restart when watched files changed.Believed Root Cause
The file watcher implementation had several critical flaws:
watchandwatch_base_dirfields were not stored in daemon state, causing patterns to be lost after daemon startPitchforkToml::all_merged()at startup, so dynamically started daemons were never watchedrestart_watched_daemon()re-parsed config files instead of using daemon state, causing restart failuresWorking Solution
watch: Vec<String>andwatch_base_dir: Option<PathBuf>toDaemonandRunOptionsstructsUpsertDaemonOptsdaemon_file_watch()to dynamically read patterns from supervisor staterestart_watched_daemon()to use stored daemon state valuesTesting Process
File watching now correctly triggers daemon restart with 1-second debouncing when watched files change.
Files Changed
src/daemon.rs- Added watch fields to structssrc/daemon_list.rs- Initialize watch fields in RunOptionssrc/ipc/batch.rs- Propagate watch config through IPCsrc/supervisor/autostop.rs- Pass watch fields on daemon restartsrc/supervisor/lifecycle.rs- Persist watch config in statesrc/supervisor/retry.rs- Include watch fields in retry optssrc/supervisor/state.rs- Handle watch in state persistencesrc/supervisor/watchers.rs- Complete rewrite of file watching logicmise.lock- Tool version updates (incidental)