[RFC] fix(supervisor): prevent stale monitor state overwrites

[brandon-julio-t]

RFC status

This is a Draft PR intended to make the failure mode and one possible fix concrete for maintainer feedback. I am happy to change the design or combine the relevant pieces with ongoing supervisor work.

Related Discussion: #634

Problem

A forced restart can register a replacement daemon as running, then a late exit-state write from the old process monitor can overwrite it with pid = None and stopped.

The replacement remains alive but untracked. When it later exits, retry logic does not restart it because the supervisor already considers the daemon stopped.

The core invariant proposed here is:

A process exit callback for PID P may mutate daemon state only while the current state still belongs to PID P.

This was observed in production on v2.17.0 during a cron retrigger = "always" restart. Current main narrows one timing window with a pre-drain stopping snapshot, but the final ownership check and state write are still separate operations.

Root cause

There are two related races:

1. The monitor exit path reads state, drains output, and later upserts a terminal state without atomically confirming PID ownership.
2. Concurrent forced lifecycle requests can overlap check, stop, and spawn. A stop completion from one request can overwrite the owner installed by another request.

Readiness failure adds another ownership case: failure can be reported before graceful termination completes. A retry must replace its own still-live failed PID, but must not replace a different PID installed by a newer request.

Proposed implementation

• Add an atomic StateFile::record_daemon_exit transition that updates status only when the stored PID matches the exiting PID.
• Move active-port clearing into that same ownership-gated transition.
• Serialize each daemon check/stop/spawn transition with a per-daemon lifecycle lock.
  • The lock map stores weak references and prunes unused entries.
  • The guard is released after the replacement is registered and its monitor is installed, before readiness waiting.
• Carry a { PID, lifecycle generation } token through readiness retry decisions.
  • Same owner: finish stopping that attempt and retry.
  • Different PID or generation: treat the newer lifecycle request as superseding the old retry, even if the newer PID has since been explicitly stopped.
• Return a failed stop response without spawning another process.
• Gate explicit-stop terminal writes and failed-stop recovery by PID ownership.
• Treat an explicit stop as lifecycle cancellation during a PID-less readiness-retry backoff.
  • Record the stop only while no replacement PID owns the state.
  • Forward explicit CLI and MCP targets without expanding dependencies for inactive roots.
• Coordinate request-owned readiness retries with the periodic retry watcher.
  • The watcher checks readiness ownership, revalidates retry eligibility, schedules on_retry, and registers the replacement PID under one lifecycle guard.
  • Missing-command exhaustion mutates only a still-eligible errored daemon instead of performing a broad upsert.
• Log whether an exit was recorded or ignored because state belongs to another PID.

Reproduction and regression coverage

Exact stale monitor callback

1. Register replacement PID 200 as running with an active port.
2. Complete the old PID 100 exit transition.
3. Assert that PID 200, running status, exit metadata, and active port remain unchanged.

Companion tests cover a matching PID transitioning from stopping to stopped, an already-cleared PID ignoring a stale callback, and dirty-state behavior.

Overlapping forced restarts

The end-to-end test:

1. Starts restart A.
2. Waits until the daemon is explicitly stopping.
3. Starts restart B.
4. Asserts the final tracked PID is alive.
5. Reads every PID synchronously recorded by supervisor start logs and asserts all non-current PIDs are dead.

Still-terminating retry owner

Another end-to-end test makes readiness fail after 100 ms while the daemon ignores TERM for two seconds. The one-second retry backoff expires while the original PID is still alive; the test asserts that retry attempt 1 is nevertheless started.

Superseded retry after explicit stop

A third end-to-end test lets an older readiness retry enter backoff, starts and explicitly stops a newer owner, then asserts the old request cannot resurrect the daemon after the current PID has been cleared.

Explicit stop during retry backoff

The cancellation test waits until readiness attempt 3 enters its eight-second backoff, issues an explicit stop while there is no current PID, then asserts the daemon remains stopped and attempt 4 never starts. Companion coverage asserts an inactive requested daemon does not cause its running dependency to be stopped.

Foreground/background retry ownership

State-level coverage keeps a foreground readiness-retry marker alive and asserts the periodic watcher neither starts a process nor mutates retry state. Companion tests cover safe missing-command exhaustion and confirm on_retry is scheduled exactly once for a successful background retry.

Validation

• cargo nextest run --all-features: 525 passed
• cargo clippy --all-targets --all-features --no-deps -- -D warnings -A clippy::collapsible_if: passed
• Focused Bats tests:
  • concurrent restarts leave one tracked process: passed
  • retry replaces its still-terminating failed attempt: passed
  • stopping a superseding owner cancels an older retry: passed
  • explicit stop during retry backoff prevents resurrection: passed
  • stopping an inactive daemon does not stop its running dependency: passed
  • retry with ready_output re-checks on each attempt: passed
  • on_retry hook fires for a background retry: passed
• Iterative correctness, maintainability, and verification reviews: no remaining findings

A broader local Bats run encountered existing macOS /var versus /private/var path-equivalence assertion failures before reaching the full suite; the new focused tests pass independently.

Related work

• Supervisor crash orphans daemons; restarted supervisor doesn't re-adopt and can start duplicates #631 describes supervisor-crash orphaning, which is a different failure mode.
• refactor(supervisor): fix monitor exit path race + rewrite architecture docs #620 covers nearby monitor-exit architecture and races. This RFC focuses on atomic PID ownership and serializing concurrent forced lifecycle transitions.

---

Authorship note: the investigation, reproduction, and implementation were assisted by OpenAI GPT-5.6 Sol with High reasoning effort. The changes and this RFC were reviewed by @brandon-julio-t.

Summary by CodeRabbit

• Bug Fixes
  • Made daemon terminal-state, status, and port handling PID-aware so stale exit/stop events can’t affect replacement processes.
  • Improved daemon lifecycle coordination to serialize start/stop and prevent outdated retries from taking ownership.
  • Added more reliable retry eligibility/exhaustion behavior, ensuring retries only progress when appropriate.
• Tests
  • Extended lifecycle/retry coverage for overlapping restarts, retry progression across attempts, and retry supersession.
  • Added coverage confirming the background retry on_retry hook fires exactly once.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Daemon lifecycle operations now serialize per daemon, classify attempts by spawn state, and prevent stale processes from overwriting replacement state. Exit monitoring records state only for matching PIDs, with coverage for concurrent restarts and retry supersession.

Changes

Daemon lifecycle ownership

Layer / File(s)	Summary
PID-checked daemon state src/state_file.rs, src/daemon.rs, src/supervisor/state.rs	Daemon status, exit, stop, and retry-exhaustion updates now enforce PID or retry eligibility checks.
Per-daemon lifecycle serialization src/supervisor/mod.rs, src/supervisor/lifecycle.rs	Lifecycle guards, generation tracking, classified attempts, and ownership-aware stop and exit handling coordinate replacements and readiness.
Retry coordination and validation src/supervisor/retry.rs, src/supervisor/lifecycle.rs, test/supervisor.bats, test/hooks.bats	Background retries use the lifecycle path, with tests covering overlapping restarts, retry replacement, supersession, lifecycle activity, and retry hooks.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Supervisor
  participant Lifecycle
  participant DaemonProcess
  participant StateFile
  Supervisor->>Lifecycle: acquire daemon lifecycle guard
  Lifecycle->>DaemonProcess: stop owned attempt before replacement
  Lifecycle->>DaemonProcess: spawn replacement attempt
  DaemonProcess-->>Lifecycle: PID and readiness result
  Lifecycle->>StateFile: record exit or stop for PID
  StateFile-->>Lifecycle: record or ignore based on PID ownership

Loading

Possibly related PRs

• jdx/pitchfork#602: Both modify lifecycle stop and exit-state handling during daemon races.
• jdx/pitchfork#620: Both modify monitor exit handling and PID-aware state cleanup.

Suggested reviewers: gaojunran

Poem

I’m a rabbit guarding each PID,
While locks keep racing paws well hid.
Old exits knock; the state says “No,”
New daemons safely start and grow.
Retry hops now follow the light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: preventing stale supervisor/monitor state from overwriting newer daemon state.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR makes daemon lifecycle transitions ownership-aware and serializes retry, restart, and explicit-stop operations.

• Adds PID-gated terminal-state updates so stale process monitors cannot overwrite replacement state.
• Adds per-daemon lifecycle generations and readiness-retry ownership to prevent superseded retries from restarting stopped daemons.
• Coordinates foreground and periodic retries under the lifecycle guard and revalidates persisted retry eligibility.
• Forwards inactive explicitly requested daemons through stop batching so stops during retry backoff reach the supervisor.
• Adds state-level and end-to-end regression coverage for stale exits, overlapping restarts, and retry cancellation.

Confidence Score: 5/5

The PR appears safe to merge, with the previously reported explicit-stop retry and supersession failures addressed.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/supervisor/lifecycle.rs	Serializes daemon lifecycle transitions, tracks retry ownership by PID and generation, and makes explicit stops supersede foreground readiness retries.
src/state_file.rs	Adds atomic PID-owned exit and stop transitions while preserving replacement process state and active-port ownership.
src/supervisor/retry.rs	Delegates periodic retries to the lifecycle-coordinated retry path with eligibility revalidation.
src/supervisor/state.rs	Exposes ownership-gated state mutations through the supervisor's state lock.
src/supervisor/mod.rs	Adds per-daemon lifecycle state and readiness-retry activity tracking.
src/ipc/batch.rs	Sends explicit stop requests for inactive retrying daemons while retaining reverse ordering for active daemons.
src/daemon.rs	Centralizes the persisted-state predicate used to determine retry eligibility.

_{Reviews (6): Last reviewed commit: "fix(supervisor): make explicit stop canc..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

src/supervisor/lifecycle.rs (1)

1593-1688: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

stop_inner's terminal writes aren't PID-ownership-checked like the new record_daemon_exit.

stop_inner's two terminal branches (Lines 1647-1656, 1668-1677) still call upsert_daemon unconditionally with the pid snapshotted at the top of the function — there's no re-check that the daemon still belongs to that PID before writing status/last_exit_success.

stop_inner is now also invoked from run_once_serialized via stop_before_replacement (the new retry-replacement path), which is exactly the "process may still be exiting on its own" scenario this PR targets. Since the daemon_lifecycle_guard only excludes other guarded callers, the daemon's own (unguarded) exit monitor can race on the same PID and call the ownership-checked record_daemon_exit concurrently. Whichever write lands last wins unconditionally — so a monitor-recorded Errored/last_exit_success=false (from e.g. a forced SIGKILL after a readiness timeout) can be silently overwritten by stop_inner's Stopped/last_exit_success=true, or vice versa. No process is leaked (both writers reference the same dead PID), but the persisted status/last_exit_success can end up wrong and is visible via pitchfork status/list.

Consider re-verifying daemon.pid == Some(pid) at the point of these two writes (mirroring record_daemon_exit's pattern) so a losing writer no-ops instead of clobbering.

♻️ Sketch of the fix direction

-                    self.upsert_daemon(
-                        UpsertDaemonOpts::builder(id.clone())
-                            .set(|o| {
-                                o.pid = None;
-                                o.status = DaemonStatus::Stopped;
-                                o.last_exit_success = Some(true);
-                            })
-                            .build(),
-                    )
-                    .await?;
+                    // Only commit this write if the daemon still belongs to `pid`;
+                    // a concurrently-racing exit monitor for the same PID may have
+                    // already recorded a more accurate terminal status.
+                    let mut state_file = self.state_file.lock().await;
+                    if state_file.daemons.get(id).and_then(|d| d.pid) == Some(pid) {
+                        state_file.daemons.get_mut(id).map(|d| {
+                            d.pid = None;
+                            d.status = DaemonStatus::Stopped;
+                            d.last_exit_success = Some(true);
+                        });
+                        state_file.mark_dirty();
+                    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/supervisor/lifecycle.rs` around lines 1593 - 1688, Update both terminal
upsert paths in stop_inner—after the kill attempt and in the already-not-running
branch—to re-fetch the daemon and verify its PID is still Some(pid) immediately
before writing terminal status. Only apply the Stopped/last_exit_success update
when ownership remains; otherwise skip the write so a concurrent
record_daemon_exit cannot be clobbered.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/supervisor/lifecycle.rs`:
- Around line 1593-1688: Update both terminal upsert paths in stop_inner—after
the kill attempt and in the already-not-running branch—to re-fetch the daemon
and verify its PID is still Some(pid) immediately before writing terminal
status. Only apply the Stopped/last_exit_success update when ownership remains;
otherwise skip the write so a concurrent record_daemon_exit cannot be clobbered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d677780-bc53-4440-b550-abcafb81d837

📥 Commits

Reviewing files that changed from the base of the PR and between 26f8016 and d85da9e.

📒 Files selected for processing (5)

• src/state_file.rs
• src/supervisor/lifecycle.rs
• src/supervisor/mod.rs
• src/supervisor/state.rs
• test/supervisor.bats

[coderabbitai]

🧹 Nitpick comments (1)

src/state_file.rs (1)

286-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

PID-ownership gating in record_daemon_terminal_state/set_daemon_status_if_owned looks correct.

Mutation only happens after confirming daemon.pid == Some(pid), mark_dirty is only invoked on the success path, and the whole check-then-act runs synchronously inside the caller's held lock (per src/supervisor/state.rs), so there's no TOCTOU window. This matches all of the new regression tests below.

One minor duplication: set_daemon_status_if_owned (Lines 293-298) and record_daemon_terminal_state (Lines 340-345) repeat the identical "look up daemon, verify pid ownership" guard. Consider extracting a small fn owned_daemon_mut(&mut self, id: &DaemonId, pid: u32) -> Option<&mut Daemon> helper to keep the ownership check in one place as more PID-gated mutators get added.

♻️ Optional refactor

+    fn owned_daemon_mut(&mut self, id: &DaemonId, pid: u32) -> Option<&mut Daemon> {
+        let daemon = self.daemons.get_mut(id)?;
+        if daemon.pid != Some(pid) {
+            return None;
+        }
+        Some(daemon)
+    }
+
     pub fn set_daemon_status_if_owned(
         &mut self,
         id: &DaemonId,
         pid: u32,
         status: DaemonStatus,
     ) -> bool {
-        let Some(daemon) = self.daemons.get_mut(id) else {
-            return false;
-        };
-        if daemon.pid != Some(pid) {
-            return false;
-        }
+        let Some(daemon) = self.owned_daemon_mut(id, pid) else {
+            return false;
+        };
 
         daemon.status = status;
         self.mark_dirty();
         true
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/state_file.rs` around lines 286 - 355, Optionally extract the repeated
daemon lookup and PID ownership check from set_daemon_status_if_owned and
record_daemon_terminal_state into an owned_daemon_mut helper returning
Option<&mut Daemon>. Replace both guard blocks with this helper while preserving
their existing success, failure, and mark_dirty behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/state_file.rs`:
- Around line 286-355: Optionally extract the repeated daemon lookup and PID
ownership check from set_daemon_status_if_owned and record_daemon_terminal_state
into an owned_daemon_mut helper returning Option<&mut Daemon>. Replace both
guard blocks with this helper while preserving their existing success, failure,
and mark_dirty behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: d6a1d417-7519-49ad-845b-b88f26dda062

📥 Commits

Reviewing files that changed from the base of the PR and between 26a7f88 and c3204c3.

📒 Files selected for processing (3)

• src/state_file.rs
• src/supervisor/lifecycle.rs
• src/supervisor/state.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/supervisor/state.rs
• src/supervisor/lifecycle.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/supervisor/retry.rs`:
- Around line 48-53: Update the background retry flow around
has_readiness_retry_for_current_generation and self.run(...) so the
readiness-retry ownership check and background start form one atomic lifecycle
claim under the daemon’s per-daemon lifecycle lock. Hold or otherwise serialize
that lock through the decision and start initiation, preventing a foreground
request from registering readiness retries between the check and background
retry execution.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: ce29c2e8-3aea-4916-a0a9-eaf017e8d8dc

📥 Commits

Reviewing files that changed from the base of the PR and between c3204c3 and ff1a574.

📒 Files selected for processing (3)

• src/supervisor/lifecycle.rs
• src/supervisor/mod.rs
• src/supervisor/retry.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/supervisor/lifecycle.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/hooks.bats`:
- Around line 302-309: Update the marker-waiting flow in the hook test to add a
settle delay after the marker first appears and before counting its lines. Keep
the existing existence assertion, then wait long enough for the retried attempt
and any delayed on_retry firing to complete before asserting the marker contains
exactly one line.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: cf019fa9-0636-4826-a3e8-bfe6fae7a30b

📥 Commits

Reviewing files that changed from the base of the PR and between ff1a574 and 09ebbf5.

📒 Files selected for processing (6)

• src/daemon.rs
• src/state_file.rs
• src/supervisor/lifecycle.rs
• src/supervisor/mod.rs
• src/supervisor/retry.rs
• test/hooks.bats

🚧 Files skipped from review as they are similar to previous changes (3)

• src/supervisor/mod.rs
• src/state_file.rs
• src/supervisor/lifecycle.rs

[github-actions]

This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically.

This is warning day 1 of 7.

Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it.

This comment was generated by an automated workflow.

[github-actions]

This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically.

This is warning day 3 of 7.

Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it.

This comment was generated by an automated workflow.

[brandon-julio-t]

Invalidated and superseded by #634 (comment) 🤞