fix(supervisor): make unobserved daemon deaths retryable after a crash

[jdx]

Context

Follow-up to #633, closing a divergence Bugbot flagged during its review (thread) that I deliberately deferred rather than fold into that PR.

Startup orphan cleanup and the runtime reconciler disagreed about the same situation. When a daemon's recorded PID is dead (or has been recycled), startup cleared the record to stopped while the interval reconciler marked it errored(-1). Since startup runs first on every restart, the startup answer always won — so a daemon that died while its supervisor was crashed never became eligible for its configured retries, and stayed silently stopped instead.

Why not just mark it errored

Two hazards make the one-line version wrong, and both shape this change:

Neither fabricating nor keeping an exit result is safe. CronRetrigger::Success and Fail read last_exit_success (watchers.rs). Writing false for an exit nobody observed would block a one-shot cron daemon that actually succeeded during the crash window from its next retrigger = "success" run. But keeping the previous value is wrong too — it attributes an earlier run's outcome to this one. These transitions now clear the field: None restores the "no result yet" reading both retriggers already handle.

A reboot, and a deliberate stop, are both indistinguishable from a crash by PID alone. After a hard power-off every record is running with a stale PID. And on Windows a deliberate pitchfork supervisor stop leaves the same footprint: the stop signal is delivered and handled on Unix, where close() stops each daemon, but Windows has no POSIX signals so the supervisor is force-terminated and never runs its shutdown path. A blanket errored would report intentionally stopped daemons as failures and auto-restart the ones with retry configured. (Windows CI caught this on the first revision.)

So erroring a daemon requires two pieces of positive evidence, not an assumption:

1. The previous supervisor exited uncleanly, which its own state entry already tells us: a clean shutdown removes that entry, while a crash, an external kill, or a --force replacement leaves it. The supervisor stop command now removes the entry itself on the platform where the supervisor cannot, so both behave identically — and a test pins that invariant, since it is exactly what the platforms disagreed about.
2. The daemon belonged to the current boot. The existing start_time can't answer this: fix(supervisor): verify process identity before killing orphaned daemons #632 replaced it with a platform-specific high-resolution token (Linux clock ticks since boot, macOS µs since epoch, Windows FILETIME), so it isn't comparable to a wall-clock boot time — and on Linux a token from a previous boot looks plausibly current. Hence a small, explicitly wall-clock companion field.

Changes

• Daemon.boot_time: system boot time (epoch seconds, via sysinfo::System::boot_time()) recorded alongside the PID at spawn. skip_serializing_if so it's absent for daemons with no live process, and old state files simply lack it.
• unobserved_exit_status(status, recorded_boot_time, current_boot_time, supervisor_exited_uncleanly): one pure function deciding the terminal status for an exit nobody observed. Running + same boot + unclean shutdown → Errored(-1) (retryable). Previous boot, or a clean shutdown → Stopped. Any other status, in practice Stopping → Stopped (an intentional stop that completed while the supervisor was gone). Only Running and Stopping can carry a PID, so that's the whole space.
• Both paths now call it: startup's dead-PID and recycled-PID branches, and the reconciler's equivalents. The kill-policy path still records Stopped — we terminated that process ourselves, so its exit was observed and intentional.
• Boot times within 2s compare as the same boot. That window exists only for Windows, which recomputes boot time as now - GetTickCount64() and can drift about a second between samples; Linux (/proc/stat btime) and macOS (kern.boottime) are exact. Kept deliberately tight so a genuine reboot can never fall inside it — a wider window would misread a short-lived previous boot (a device in a reboot loop) as the current one.
• Missing boot_time (state written before this change) → Stopped, i.e. exactly today's behavior. Fail-closed, consistent with the identity handling fix(supervisor): verify process identity before killing orphaned daemons #632/feat(supervisor): re-adopt orphaned daemons by default after a crash #633 settled on.

No hooks fire from these transitions, unchanged from before: we don't know the daemon failed, and firing on_fail/on_exit for events that happened while the supervisor was dead would be a larger semantic change. Retries still fire on_retry normally through check_retry.

Tests

• daemon that dies during a supervisor crash is retried on restart — the user-visible win: crash the supervisor, kill the daemon while nothing is watching, restart, and the daemon comes back on its own with a new PID.
• daemon record from a previous boot is not retried — a crafted record with an ancient boot_time stays stopped and is left alone despite retry = 3.
• Seven unit tests over unobserved_exit_status covering same-boot, previous-boot, the jitter boundary, short-previous-boot gaps (5/30/59/60s), a missing field, Stopping, and clean shutdown.
• supervisor stop clears the supervisor record on every platform — pins the clean-shutdown invariant the two platforms disagreed about.
• The recycled-PID bystander test keeps asserting stopped — its crafted record has no boot_time, which is now exactly the legacy fail-closed case; comment updated to say so.

Compatibility

User-visible: after a supervisor crash, an affected daemon now reads errored rather than stopped in pitchfork list/status, and daemons with retry configured come back by themselves. That's the intended fix, but it's worth a changelog line. Downgrade is safe — an older binary ignores the unknown boot_time key.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes core supervisor recovery, daemon terminal status, retry eligibility, and cron last_exit_success semantics after crashes; behavior is heavily tested but affects restart paths on all platforms.

Overview
Fixes a mismatch where startup orphan cleanup always cleared dead PIDs to stopped while the interval reconciler used errored(-1)—startup ran first, so daemons that died during a supervisor crash never became retry-eligible.

Daemon.boot_time is recorded at spawn (epoch seconds via PROCS.boot_time()). unobserved_exit_status picks the terminal status when nobody observed the exit: Running + same boot + unclean supervisor shutdown → Errored(-1); previous boot, clean shutdown, missing boot_time, or Stopping → Stopped. Startup orphan scan and runtime reconciler both use this logic.

ExitObservation drives last_exit_success: unobserved exits clear it to None (avoids lying to cron retrigger); supervisor-initiated orphan kills set success like a deliberate stop. supervisor stop removes the global/pitchfork state entry on Windows after force-kill so the next start does not treat a deliberate stop as a crash.

Bats and unit tests cover crash→retry, previous-boot no-retry, clean stop marker, and boot-time jitter.

^{Reviewed by Cursor Bugbot for commit e66e910. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• Bug Fixes
  • Improved supervisor recovery after crashes so unexpectedly terminated daemons can be retried appropriately.
  • Prevented daemons from being incorrectly retried after a machine reboot or intentional supervisor shutdown.
  • Improved handling of orphaned processes and recycled process IDs.
  • Ensured intentional supervisor stops are recorded correctly across platforms.
• Tests
  • Added coverage for crash recovery, intentional shutdowns, reboot scenarios, and orphan cleanup.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Boot-aware daemon lifecycle

Layer / File(s)	Summary
Persist boot metadata src/daemon.rs, src/procs.rs, src/supervisor/state.rs	Daemon state stores the system boot time while a PID is active.
Classify unobserved exits src/supervisor/mod.rs, src/supervisor/adopt.rs	Supervisor reconciliation distinguishes retryable unobserved deaths from stopped daemons and clears terminal runtime metadata.
Record intentional stops and validate lifecycle behavior src/cli/supervisor/stop.rs, test/supervisor.bats	Intentional supervisor stops remove the state marker, with tests covering crashes, prior boots, recycled PIDs, and clean shutdowns.

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

Sequence Diagram(s)

sequenceDiagram
  participant Supervisor
  participant StateFile
  participant DaemonProcess
  Supervisor->>StateFile: Read daemon boot metadata
  Supervisor->>DaemonProcess: Check liveness and process identity
  Supervisor->>StateFile: Write retryable or stopped status

Loading

Possibly related PRs

• jdx/pitchfork#585: Updates supervisor state-file loading used by unclean-exit detection.
• jdx/pitchfork#632: Overlaps with orphan cleanup and daemon identity handling.
• jdx/pitchfork#633: Overlaps with supervisor orphan re-adoption logic.

Suggested reviewers: gaojunran

Poem

A rabbit hops through boot-time snow,
Marking which old daemons should go.
Crash or stop, the state now knows,
Retry blooms where recovery grows.
Clean markers fade beneath moonlight bright.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 accurately summarizes the main change: retrying unobserved daemon deaths after a supervisor crash.

---

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.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit f9b2f91. Configure here.}

[greptile-apps]

Greptile Summary

This PR aligns startup and runtime reconciliation of unobserved daemon exits.

• Records each spawned daemon’s system boot time to distinguish same-boot crashes from reboots.
• Makes same-boot daemon deaths retryable after an unclean supervisor exit while preserving stopped behavior for clean shutdowns, previous boots, and legacy records.
• Clears unknown cron exit outcomes and records supervisor-initiated orphan termination as successful.
• Removes the supervisor state marker after deliberate forced shutdowns on platforms where the supervisor cannot clean up itself.
• Adds unit and integration coverage for crash recovery, reboot gating, deliberate shutdowns, and legacy state.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failures remain in the fixes associated with the previous review threads.

Important Files Changed

Filename	Overview
src/supervisor/mod.rs	Unifies startup orphan-state resolution around boot identity, shutdown cleanliness, and explicit exit-observation semantics; the displayed follow-up fixes are complete.
src/supervisor/adopt.rs	Applies the same unobserved-exit and exit-outcome handling during runtime reconciliation while retaining PID and monitor ownership guards.
src/supervisor/state.rs	Records system boot time whenever a live PID is persisted.
src/cli/supervisor/stop.rs	Clears the supervisor marker after deliberate forced termination so later startup does not infer a crash.
src/daemon.rs	Adds a backward-compatible optional boot-time field to persisted daemon state.
src/procs.rs	Exposes the system boot timestamp used for daemon boot identity.
test/supervisor.bats	Adds end-to-end coverage for retry after supervisor crashes, deliberate stop cleanup, and previous-boot suppression.

_{Reviews (4): Last reviewed commit: "docs(supervisor): record why an unobserv..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/supervisor/mod.rs (1)

1482-1508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Doc comment omits the supervisor_exited_uncleanly condition.

The list of "two cases stay Stopped" is now three: a clean prior shutdown also yields Stopped regardless of boot alignment. That third condition is the one callers hardcode (adopt.rs passes true), so it deserves to be spelled out here.

📝 Suggested doc addition

 /// - any other status (in practice `Stopping`), i.e. an intentional stop that
 ///   completed while the supervisor was gone
+/// - a *clean* previous shutdown, where the daemon's disappearance was
+///   deliberate even though no monitor recorded it (see
+///   `supervisor_exited_uncleanly`)

🤖 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/mod.rs` around lines 1482 - 1508, Update the doc comment for
unobserved_exit_status to document the additional Stopped case when
supervisor_exited_uncleanly is false, regardless of boot alignment. Clarify that
a clean prior supervisor shutdown represents an intentional stop and remains
Stopped, matching callers such as adopt.rs that supply this condition.

src/cli/supervisor/stop.rs (1)

28-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Surface failures to clear the marker.

Both the read and the write are silently discarded. On Windows this is the only code path that clears the supervisor entry, so a parse error or a failed write leaves the marker behind and the next supervisor start classifies these intentionally stopped daemons as Errored(-1) — the exact behavior this PR sets out to prevent, with no trace in the logs.

🔎 Log the failure paths

-                if let Ok(mut sf) = StateFile::read(&*env::PITCHFORK_STATE_FILE)
-                    && sf.daemons.remove(&DaemonId::pitchfork()).is_some()
-                {
-                    let _ = sf.write();
-                }
+                match StateFile::read(&*env::PITCHFORK_STATE_FILE) {
+                    Ok(mut sf) => {
+                        if sf.daemons.remove(&DaemonId::pitchfork()).is_some()
+                            && let Err(e) = sf.write()
+                        {
+                            warn!("failed to clear supervisor state entry: {e}");
+                        }
+                    }
+                    Err(e) => warn!("failed to read state file to clear supervisor entry: {e}"),
+                }

🤖 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/cli/supervisor/stop.rs` around lines 28 - 32, Update the supervisor
cleanup logic around StateFile::read and sf.write to log failures instead of
silently discarding them. Preserve removal and writing on success, and include
enough error context to identify whether reading or writing the state file
failed so marker-clearance problems are visible.

src/supervisor/adopt.rs (1)

213-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Function rustdoc is now stale. The summary above (Lines 167-168) still promises dead PID → mark Errored(-1) and recycled PID → ... mark Errored(-1), but both paths now delegate to unobserved_exit_status, which returns Stopped for previous-boot records and for legacy records with no boot_time. Please update those two bullets to describe the boot-aware classification.

🤖 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/adopt.rs` around lines 213 - 228, Update the rustdoc bullets
above the adoption logic to describe the boot-aware classification performed by
unobserved_exit_status instead of promising Errored(-1) for dead or recycled
PIDs. Document that previous-boot records and legacy records without boot_time
are classified as Stopped, while preserving the existing descriptions for other
outcomes.

🤖 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/adopt.rs`:
- Around line 326-333: Restore an outcome parameter on finalize_if_pid so
deliberate terminations preserve the known-good marker: pass None from the two
unobserved_exit_status callers around the startup/adoption paths, and pass
Some(true) from the kill-policy termination caller around Line 301, matching
Supervisor::stop and the adopted-monitor behavior. Keep the existing
unobserved-exit handling and rustdoc aligned with this distinction.

In `@test/supervisor.bats`:
- Around line 328-333: Replace the fixed sleep after supervisor start in the
stop_marker test with the existing wait_for_status polling helper, waiting until
the marker reaches “stopped” before invoking pitchfork status. Preserve the
current success and partial-output assertions.
- Around line 346-364: Update the crafted state in the supervisor boot-time test
to include a [daemons."global/pitchfork"] marker with a dead PID, ensuring
supervisor_exited_uncleanly reports an unclean shutdown. Keep the prevboot/svc
record’s boot_time = 1 and assert startup succeeds because the previous-boot
comparison, rather than the clean-shutdown path, keeps the daemon stopped.

---

Nitpick comments:
In `@src/cli/supervisor/stop.rs`:
- Around line 28-32: Update the supervisor cleanup logic around StateFile::read
and sf.write to log failures instead of silently discarding them. Preserve
removal and writing on success, and include enough error context to identify
whether reading or writing the state file failed so marker-clearance problems
are visible.

In `@src/supervisor/adopt.rs`:
- Around line 213-228: Update the rustdoc bullets above the adoption logic to
describe the boot-aware classification performed by unobserved_exit_status
instead of promising Errored(-1) for dead or recycled PIDs. Document that
previous-boot records and legacy records without boot_time are classified as
Stopped, while preserving the existing descriptions for other outcomes.

In `@src/supervisor/mod.rs`:
- Around line 1482-1508: Update the doc comment for unobserved_exit_status to
document the additional Stopped case when supervisor_exited_uncleanly is false,
regardless of boot alignment. Clarify that a clean prior supervisor shutdown
represents an intentional stop and remains Stopped, matching callers such as
adopt.rs that supply this condition.

🪄 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: 90646c5f-f4f7-462c-9a0d-8558d1ce89eb

📥 Commits

Reviewing files that changed from the base of the PR and between 7d2bb25 and b6392a9.

📒 Files selected for processing (7)

• src/cli/supervisor/stop.rs
• src/daemon.rs
• src/procs.rs
• src/supervisor/adopt.rs
• src/supervisor/mod.rs
• src/supervisor/state.rs
• test/supervisor.bats