fix(stella-cli): resume a crash that recorded an error at boot (#1696)

crates/stella-cli/src/daemon/boot.rs

@@ -24,26 +24,43 @@
 //! # Which runs it continues — the conservative rule
 //!
 //! Exactly the rows `stella daemon list` paints `Crashed ↩`: a **supervised**
-//! run whose stored status is still *live*
-//! (`SessionStatus::is_live`) while its liveness lock is not
-//! held, whose workspace still exists, and which left a resume point this
-//! build can see.
+//! run whose stored status is still *live* (`SessionStatus::is_live`) — or is
+//! `Error`, a crash that lived just long enough to say so (#1696) — while its
+//! liveness lock is not held, whose workspace still exists, and which left a
+//! resume point this build can see.
 //!
-//! The load-bearing half is "stored status still live". Every deliberate end
-//! writes a terminal status on the way out — `Complete` when it finished,
-//! `Cancelled` when `stella daemon stop` or a Ctrl-C ended it, `Paused` when a
-//! deck set it aside, `Error` when it fell over having lived long enough to
-//! say so. A process the kernel took mid-turn writes nothing, so a live status
-//! with a dead lock is the signature of interruption and of nothing else.
+//! The load-bearing half is the stored status. Every deliberate end writes
+//! one on the way out — `Complete` when it finished, `Cancelled` when `stella
+//! daemon stop` or a Ctrl-C ended it, `Stopped` when the run ended itself by
+//! policy, `Paused` when a deck set it aside. A process the kernel took
+//! mid-turn writes nothing, so a live status with a dead lock is the
+//! signature of interruption.
 //!
-//! That rule is deliberately chosen to be **immune to #1653**, which records
-//! that a deliberate policy stop is stored as `Error`, indistinguishable from
-//! a crash. It is immune because no terminal status is resumable here at all:
-//! `Error` is skipped whichever of the two it means. The cost is the honest
-//! one — a crash that *did* manage to record `Error` is not resumed at boot —
-//! and it is the right side to be wrong on, because the failure this module
-//! must never have is resuming, unattended and at the operator's expense,
-//! work the operator deliberately ended.
+//! # Why an `Error` is continued too, and why that is safe (#1696)
+//!
+//! This rule used to skip *every* terminal status, `Error` included. That was
+//! a compromise forced by #1653: a deliberate policy stop (a stuck-loop
+//! escalation, the step cap, an enforced budget, an ended scope review) was
+//! recorded as `Error`, identical to a genuine crash, so continuing an
+//! `Error` could have restarted — unattended, at the operator's expense —
+//! work the operator ended on purpose. The cost was the honest one: a real
+//! crash that *did* manage to write `Error` before dying was left stranded.
+//!
+//! #1653 removed the ambiguity. A policy stop now records
+//! [`SessionStatus::Stopped`] with every other deliberate ending, which
+//! leaves `Error` meaning only "it fell over" — a run exactly as entitled to
+//! be continued as one the kernel took without warning. Two independent
+//! facts make the widening safe rather than merely intended:
+//!
+//! - **A deliberate stop has no resume point.** The engine driver discards
+//!   the checkpoint on every terminal path, abort included, so a policy stop
+//!   retracts its resume point on the way out. A row written by a build that
+//!   predates #1653 — where a policy stop really did store `Error` — is
+//!   therefore filtered by [`SkipReason::NoResumePoint`] anyway, without this
+//!   module having to trust its status.
+//! - **The attempt bound still applies.** An `Error` that resumes into
+//!   another `Error` is counted like any other continuation and retired
+//!   after `MAX_BOOT_ATTEMPTS`.
 //!
 //! # What stops a boot loop
 //!
@@ -180,10 +197,13 @@ pub(super) enum SkipReason {
     /// Still running: the sweep found a run that survived, and starting a
     /// second copy of it is the one outcome worse than not resuming.
     StillRunning,
-    /// The run recorded a terminal status, so it ended on purpose or ended
-    /// having lived long enough to say so. See the module docs on #1653.
+    /// The run recorded a terminal status that says it *ended* rather than
+    /// broke — `Complete`, `Cancelled`, `Stopped`, `Paused`, `Archived`.
+    /// Since #1653 this no longer covers `Error`; see the module docs on
+    /// #1696.
     EndedDeliberately,
-    /// Nothing to continue from — a clean exit discards its resume point.
+    /// Nothing to continue from — a clean exit, and every deliberate stop,
+    /// discards its resume point.
     NoResumePoint,
     /// The workspace is gone; a resumed turn must run where its work is.
     WorkspaceGone,
@@ -201,7 +221,8 @@ impl SkipReason {
             Self::NotSupervised => "not a supervised run".to_string(),
             Self::StillRunning => "still running".to_string(),
             Self::EndedDeliberately => {
-                "ended deliberately — only an interrupted run is resumed at boot".to_string()
+                "ended deliberately — only an interrupted or crashed run is resumed at boot"
+                    .to_string()
             }
             Self::NoResumePoint => "no resume point".to_string(),
             Self::WorkspaceGone => "workspace no longer exists".to_string(),
@@ -242,7 +263,12 @@ pub(super) fn decide(candidate: &BootCandidate) -> BootDecision {
     if candidate.lock_held {
         return BootDecision::Skip(SkipReason::StillRunning);
     }
-    if !candidate.stored_status.is_live() {
+    // `Error` deliberately falls through to the resume-point check rather
+    // than being skipped here: since #1653 it means only "the run fell over",
+    // and a crash with a resume point is the case this whole module exists
+    // for (#1696). Every *other* terminal status is a run that ended on
+    // purpose.
+    if !candidate.stored_status.is_live() && candidate.stored_status != SessionStatus::Error {
         return BootDecision::Skip(SkipReason::EndedDeliberately);
     }
     if !candidate.has_resume_point {

crates/stella-cli/src/daemon/boot/tests.rs

@@ -37,17 +37,16 @@ fn a_run_killed_mid_turn_is_continued_at_boot() {
 
 #[test]
 fn a_run_the_operator_ended_is_never_continued_at_boot() {
-    // Every terminal status, including the two #1653 cannot tell apart. The
-    // whole point of the rule is that the ambiguity does not matter here:
-    // `Error` is skipped whether it means "crashed after saying so" or
-    // "stopped by policy", and the operator's deliberate stop is safe either
-    // way.
+    // Every status that means "this run ended rather than broke". Since
+    // #1653 a deliberate policy stop records `Stopped` instead of hiding
+    // among the crashes as `Error`, which is what lets `Error` itself be
+    // continued (#1696, below).
     for status in [
         SessionStatus::Cancelled,
+        SessionStatus::Stopped,
         SessionStatus::Complete,
         SessionStatus::Paused,
         SessionStatus::Archived,
-        SessionStatus::Error,
     ] {
         let candidate = BootCandidate {
             stored_status: status,
@@ -61,6 +60,50 @@ fn a_run_the_operator_ended_is_never_continued_at_boot() {
     }
 }
 
+/// The #1696 witness: a crash that lived long enough to record `Error` is
+/// continued, where every terminal status used to be skipped wholesale.
+///
+/// That blanket skip was the price of #1653's ambiguity — a policy stop and a
+/// crash both stored `Error`, so continuing one risked restarting work the
+/// operator ended on purpose. With #1653 landed, `Error` means only "it fell
+/// over", and stranding those was the honest cost this pays back.
+#[test]
+fn a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_not() {
+    let crashed = BootCandidate {
+        stored_status: SessionStatus::Error,
+        ..killed_mid_turn()
+    };
+    assert_eq!(
+        decide(&crashed),
+        BootDecision::Continue,
+        "an Error holding a resume point is a crash, and a crash is what this sweep continues"
+    );
+
+    // The same row without a resume point — which is what a pre-#1653 build's
+    // policy stop actually looks like, because every deliberate ending
+    // retracts its checkpoint on the way out — is still skipped, and says the
+    // most specific true thing about itself.
+    let stopped_by_policy = BootCandidate {
+        stored_status: SessionStatus::Error,
+        has_resume_point: false,
+        ..killed_mid_turn()
+    };
+    assert_eq!(
+        decide(&stopped_by_policy),
+        BootDecision::Skip(SkipReason::NoResumePoint)
+    );
+
+    // And the status a policy stop records today is skipped outright.
+    let stopped = BootCandidate {
+        stored_status: SessionStatus::Stopped,
+        ..killed_mid_turn()
+    };
+    assert_eq!(
+        decide(&stopped),
+        BootDecision::Skip(SkipReason::EndedDeliberately)
+    );
+}
+
 #[test]
 fn a_run_that_survived_the_boot_is_not_started_a_second_time() {
     let candidate = BootCandidate {
@@ -128,7 +171,15 @@ fn nothing_is_ever_continued_without_a_resume_point() {
                                 "a boot-time action without a resume point would be a restart, \
                                  not a resume: {candidate:?}"
                             );
-                            assert!(status.is_live() && !lock_held, "{candidate:?}");
+                            assert!(!lock_held, "{candidate:?}");
+                            // Interrupted, or crashed: since #1653 an `Error`
+                            // means only "it fell over", so it joins the live
+                            // statuses as continuable (#1696); every other
+                            // terminal status is a run that ended on purpose.
+                            assert!(
+                                status.is_live() || status == SessionStatus::Error,
+                                "{candidate:?}"
+                            );
                         }
                     }
                 }

website/content/docs/commands/daemon.mdx

@@ -189,7 +189,9 @@ It prints one line per supervised run, continued or skipped, with the reason:
   boot-time resume 1 of 3 for ses-1785956826121
 ```
 
-**What it continues.** Exactly the rows `list` paints `Crashed ↩`: a supervised run whose recorded status is still live while its liveness lock is gone, whose workspace still exists, and which left a resume point. A run that finished, was stopped, was set aside, or recorded an error is never resumed — every deliberate ending writes a status on the way out, and a process the kernel took writes nothing, so a live status with a dead lock is the signature of interruption and of nothing else. The rule is deliberately conservative on the one ambiguous case: work you ended on purpose is never resumed behind your back, at the cost of not resuming a crash that managed to record itself.
+**What it continues.** A supervised run that was interrupted — its liveness lock gone, its workspace still there, and a resume point left behind. That is the rows `list` paints `Crashed ↩`, and now also a run that fell over having lived just long enough to record the error: both are work that stopped without meaning to.
+
+A run that finished, was stopped, was set aside, or **stopped itself by policy** — a stuck loop escalated past its warning, the step cap, an enforced budget, a scope review you ended — is never resumed. Every one of those writes a status on the way out saying the work was *ended* rather than broken, and every one of them retracts its resume point as it goes, so the rule holds twice over. Work you ended on purpose is never resumed behind your back.
 
 **What it skips because nobody is there.** A run that was waiting on a plan review when the machine went down is *not* resumed at boot. Resuming it would not fail — it would park again, waiting for an answer, and because the sweep streams one run at a time to completion it would sit there forever with every later run behind it unresumed. So the sweep names it and moves on: