Coalesce watcher bursts before daemon reconciliation#72
Conversation
The daemon now merges filesystem event bursts into one logical wakeup window, keeps only one pending rerun while a tick is in flight, and exposes coalescing state in daemon_status. This brings the watcher backend in line with the next behavioral donor lane from watchexec without changing the public MCP surface. Constraint: Keep daemon_tick as the single reconciliation implementation while reducing watcher burst amplification Rejected: A separate coalescing tool or queue API | unnecessary surface area for internal daemon behavior Rejected: Unbounded pending reruns | risks event storms and duplicate work under bursty filesystems Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep watcher burst handling single-flight until a richer scheduler policy is proven with production telemetry Tested: cargo fmt --check; cargo test -p m1nd-mcp run_daemon_tick_marks_pending_rerun_when_already_in_flight -- --nocapture; cargo test -p m1nd-mcp daemon_ -- --nocapture; cargo test -p m1nd-mcp native_watcher_ -- --nocapture; cargo test -p m1nd-mcp daemon_wait_duration_ -- --nocapture; MCP burst smoke Not-tested: Multi-client contention across long-lived sessions
Constraint: the prior CI run captured a stale format failure even though the branch is now rustfmt-clean\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Remove the need for noop retriggers by ensuring format is rechecked before first push on future watcher-loop branches\nTested: cargo fmt --check\nNot-tested: N/A
Constraint: keep the coalescing branch clean and rustfmt-stable before starting the next daemon phase\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Finish the current daemon tranche completely before opening the next stack branch\nTested: cargo fmt --check\nNot-tested: N/A
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae25ca5d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state.daemon_state.tick_in_flight = true; | ||
| state.daemon_state.last_tick_trigger = Some(trigger.to_string()); | ||
| let _ = crate::daemon_handlers::handle_daemon_tick( |
There was a problem hiding this comment.
run_daemon_tick sets tick_in_flight = true before calling handle_daemon_tick, but handle_daemon_tick persists daemon_state during that call; this means the on-disk daemon_state.json can be saved with tick_in_flight: true and never persisted back to false afterward. On the next process start (when daemon state is reloaded and auto-resumed), every trigger can hit the early return at the top of run_daemon_tick, leaving the daemon stuck in perpetual pending_rerun without reconciling changes.
Useful? React with 👍 / 👎.
| pub coalesce_window_ms: u64, | ||
| pub pending_rerun: bool, | ||
| pub tick_in_flight: bool, | ||
| pub last_coalesced_event_ms: Option<u64>, | ||
| pub coalesced_event_count: u64, |
There was a problem hiding this comment.
These newly added non-optional fields are deserialized from daemon_state.json, but older state files will not contain them. Because load_daemon_state uses serde_json::from_str::<DaemonRuntimeState>(...).ok().unwrap_or_default(), a missing-field decode error causes the entire daemon runtime state to be dropped to defaults (losing prior tracked files, counters, and active status) after upgrade.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a “watchexec-style” coalescing layer for filesystem watcher bursts and introduces single-flight daemon tick execution (with at most one pending rerun), while exposing the new coalescing/tick state via daemon_status.
Changes:
- Add coalescing/tick-control fields to
DaemonRuntimeStatefor tracking burst windows and in-flight/pending tick state. - Coalesce watcher bursts in the server loop using a timeout-based drain window, and gate tick execution via a single-flight mechanism.
- Surface the new state in
daemon_start/daemon_statusoutputs and extend tests accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
m1nd-mcp/src/session.rs |
Extends persisted daemon runtime state with coalescing + tick single-flight fields. |
m1nd-mcp/src/server.rs |
Implements burst coalescing in the event loop and adds single-flight run_daemon_tick behavior + a unit test. |
m1nd-mcp/src/daemon_handlers.rs |
Initializes and reports new daemon coalescing/single-flight fields in start/status, updates tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub poll_interval_ms: u64, | ||
| pub coalesce_window_ms: u64, | ||
| pub pending_rerun: bool, | ||
| pub tick_in_flight: bool, | ||
| pub last_coalesced_event_ms: Option<u64>, | ||
| pub coalesced_event_count: u64, |
There was a problem hiding this comment.
DaemonRuntimeState is persisted to daemon_state.json and loaded via serde_json::from_str(...).ok().unwrap_or_default(). Adding new non-optional fields here (e.g. coalesce_window_ms, last_coalesced_event_ms, coalesced_event_count) will make deserializing older state files fail (missing fields), which resets the entire daemon state to Default on upgrade. Add #[serde(default)] (or explicit #[serde(default = "...")] where you want a non-zero default like 75ms) so older daemon_state.json continues to load correctly.
| pub coalesce_window_ms: u64, | ||
| pub pending_rerun: bool, |
There was a problem hiding this comment.
tick_in_flight / pending_rerun look like transient, in-memory scheduling flags, but they’re part of DaemonRuntimeState which is persisted by handle_daemon_tick() via state.persist_daemon_state(). Because run_daemon_tick() sets tick_in_flight = true before calling handle_daemon_tick(), the persisted daemon_state.json will often contain tick_in_flight: true, and on restart the daemon can get stuck permanently treating every tick as "already in flight". Make these fields non-persistent (e.g., #[serde(skip)]/skip_serializing + #[serde(default)], or move them out of the persisted struct) and ensure they default to false on load.
| pub coalesce_window_ms: u64, | |
| pub pending_rerun: bool, | |
| pub coalesce_window_ms: u64, | |
| #[serde(skip, default)] | |
| pub pending_rerun: bool, | |
| #[serde(skip, default)] |
| let coalesced_at_ms = now_ms(); | ||
| self.state.daemon_state.last_watch_event_ms = Some(coalesced_at_ms); | ||
| loop { | ||
| match rx.try_recv() { | ||
| match rx.recv_timeout(Duration::from_millis( | ||
| self.state.daemon_state.coalesce_window_ms.max(1), | ||
| )) { | ||
| Ok(ServerEvent::WatchNotice) => { | ||
| watch_events_seen = watch_events_seen.saturating_add(1); |
There was a problem hiding this comment.
Inside the watch-burst coalescing loop, last_watch_event_ms is only set once (to the timestamp of the first WatchNotice) and then never updated for subsequent WatchNotice/WatchError events drained during the burst. This makes daemon_status.last_watch_event_ms misleading for bursts and errors. Update last_watch_event_ms each time you receive a watch notice/error in the coalescing loop (and consider whether coalesced_at_ms should represent the end-of-burst timestamp instead).
Summary
daemon_tickas the only reconciliation implementationdaemon_statusValidation
Why this matters
This is the watchexec-style behavior layer on top of the native watcher backend. The daemon now reacts once per logical change burst instead of stampeding itself with redundant ticks.