From 2449581369e1da2525204d33bd1aa006fda588cd Mon Sep 17 00:00:00 2001 From: Stella Test Date: Wed, 5 Aug 2026 20:04:58 -0700 Subject: [PATCH 1/5] =?UTF-8?q?fix(stella-cli):=20unbreak=20main=20?= =?UTF-8?q?=E2=80=94=20reconcile=20two=20independent=20#1616=20implementat?= =?UTF-8?q?ions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #1616 approval-deadline work landed twice, from two PRs that chose different names and different mechanisms, and the merge kept half of each. main does not compile. Four residues, all one collision: - `SidecarApprovalGate` carries `deadline`/`with_deadline` while the park loop and two tests reference `self.wait`/`with_wait`, and the loop passes a `parked_at` it never binds. Reconciled onto `deadline`, keeping the `next_poll` mechanism: it clips the sleep by what is left of the deadline, so a one-second wait is honoured to the second rather than to the 250ms poll, and it factors the arithmetic out for a test that need not sleep through it. The redundant top-of-loop expiry check is dropped — one definition of "expired", which is the thing this file just paid for. - `ConsoleGuard::drain_shared` is called from `main` and from the panic hook but was authored into `impl Drainable`, which is `#[cfg(unix)]`. Moved to `impl ConsoleGuard`, where its two callers already look for it. - `arm_panic_drain` is the stranded parent of `install_panic_drain` and orders the hook chain the opposite way. `install_panic_drain` is the wired one and its reasoning is the stronger of the two (release builds are `panic = "abort"`, so there is no unwind back to main's own drain). Removed, with the doc reference repointed. - `park_deadline` duplicates `AgentEngineConfig::approval_wait` exactly and had no caller, so `approval_wait_secs` was dead config: #1616's feature was unwired on main. Removed; its documentation of the `0` convention moves to the surviving definition, and its test now reads through it. Also drops the weaker of the two duplicated deadline witnesses — the survivor asserts the same property with an outer timeout for teeth and checks the transport cleanup. --- crates/stella-cli/src/daemon/approval.rs | 81 ++++++------------------ crates/stella-cli/src/daemon/console.rs | 75 ++++++++-------------- crates/stella-cli/src/settings.rs | 10 ++- 3 files changed, 53 insertions(+), 113 deletions(-) diff --git a/crates/stella-cli/src/daemon/approval.rs b/crates/stella-cli/src/daemon/approval.rs index 8c2f50518..5a1e127ba 100644 --- a/crates/stella-cli/src/daemon/approval.rs +++ b/crates/stella-cli/src/daemon/approval.rs @@ -137,10 +137,10 @@ impl SidecarApprovalGate { /// Split out of the loop so the arithmetic — the part a wall-clock test /// cannot pin down without sleeping through it — is directly testable. fn next_poll(&self, parked_at: std::time::Instant) -> Option { - let Some(wait) = self.wait else { + let Some(deadline) = self.deadline else { return Some(ANSWER_POLL); }; - let remaining = wait.checked_sub(parked_at.elapsed())?; + let remaining = deadline.checked_sub(parked_at.elapsed())?; (!remaining.is_zero()).then(|| ANSWER_POLL.min(remaining)) } @@ -179,7 +179,7 @@ impl ApprovalGate for SidecarApprovalGate { .registry .set_status(&self.id, SessionStatus::NeedsInput); - let started = std::time::Instant::now(); + let parked_at = std::time::Instant::now(); let decision = loop { if (self.interrupted)() { // The same answer the stdio gate gives on EOF: asked to stop @@ -187,17 +187,11 @@ impl ApprovalGate for SidecarApprovalGate { // land as a clean cancel instead of an escalated kill. break ScopeDecision::Abort; } - if let Some(deadline) = self.deadline - && started.elapsed() >= deadline - { - eprintln!( - " {} scope review timed out after {}s with nobody attached \ - (approval_wait_secs) — aborting", - "!".yellow(), - deadline.as_secs(), - ); - break ScopeDecision::Abort; - } + // The deadline is checked in exactly one place — `next_poll` + // returning `None` below. A second check at the top of the loop + // would be a second definition of "expired", and this file has + // already paid for that once: two PRs implemented #1616 + // independently and the merge kept half of each. match std::fs::read_to_string(self.sidecar.join(supervised::APPROVAL_ANSWER)) { Ok(json) => { // Unparseable is abort, not retry: the writer publishes @@ -217,7 +211,7 @@ impl ApprovalGate for SidecarApprovalGate { "{} scope review unanswered for {}s — aborting the run \ (agent_engine_config.approval_wait_secs)", "▸ timed out".yellow().bold(), - self.wait.map_or(0, |w| w.as_secs()), + self.deadline.map_or(0, |d| d.as_secs()), ); break ScopeDecision::Abort; } @@ -290,21 +284,6 @@ impl ApprovalGate for OneShotApprovalGate { } } -/// The configured park deadline, or `None` for the shipped park-forever -/// behaviour. -/// -/// `0` is spelled and means "no deadline", the same convention -/// `model_timeout_secs` uses for its own backstop: in a field whose absence -/// already means "the default", zero is the only way to say *deliberately* -/// unbounded — which matters when a user-scope file sets a deadline and one -/// project wants out of it. -pub(crate) fn park_deadline( - settings: Option<&crate::settings::AgentEngineConfig>, -) -> Option { - let secs = settings?.approval_wait_secs?; - (secs > 0).then(|| std::time::Duration::from_secs(secs)) -} - /// The attached-terminal side: answer a parked scope review, if one is /// pending and this terminal can. /// @@ -467,29 +446,6 @@ mod tests { ); } - /// The #1616 witness: with no `approval_wait_secs` set the gate parks - /// forever (proven here by outliving several poll intervals with no - /// answer and no interrupt); with one set, it unparks itself as an abort - /// once the deadline elapses — with nobody having answered and nobody - /// having interrupted it. On `main` a parked review has no deadline at - /// all, so this only passes once `SidecarApprovalGate` honours one. - #[tokio::test] - async fn a_deadline_unparks_an_unanswered_review_as_an_abort() { - let dir = tempfile::tempdir().unwrap(); - let registry = registry_in(dir.path()); - let record = parked_record(®istry); - let sidecar = registry.sidecar_dir(&record.id); - let gate = SidecarApprovalGate::new(sidecar.clone(), registry.clone(), record.id.clone()) - .with_deadline(Some(std::time::Duration::from_millis(50))); - - assert_eq!(gate.review(&proposal()).await, ScopeDecision::Abort); - assert_eq!( - registry.get(&record.id).unwrap().status, - SessionStatus::InProgress, - "a timed-out review still hands the record back, same as any other decision" - ); - } - /// A deadline that is set but has not yet elapsed must not preempt a real /// answer — the timeout is a backstop, not a race against the human. #[tokio::test] @@ -559,8 +515,8 @@ mod tests { /// The #1616 witness: with `approval_wait_secs` armed, a review nobody /// answers unparks itself as an abort instead of waiting forever — and it /// hands the record back rather than leaving it stuck on `Needs Input`. - /// On `main` a gate has no deadline to arm (`with_wait` does not exist), - /// so this is a type-level witness as well as a behavioural one. + /// On `main` a gate has no deadline to arm (`with_deadline` does not + /// exist), so this is a type-level witness as well as a behavioural one. #[tokio::test] async fn an_expired_deadline_unparks_an_unanswered_review_as_an_abort() { let dir = tempfile::tempdir().unwrap(); @@ -568,7 +524,7 @@ mod tests { let record = parked_record(®istry); let sidecar = registry.sidecar_dir(&record.id); let gate = SidecarApprovalGate::new(sidecar.clone(), registry.clone(), record.id.clone()) - .with_wait(Some(std::time::Duration::from_millis(30))); + .with_deadline(Some(std::time::Duration::from_millis(30))); // The outer timeout is the assertion's teeth: on a gate without a // deadline this future never resolves, and the test would hang rather @@ -632,7 +588,7 @@ mod tests { // up to the poll interval. let short = SidecarApprovalGate::new(dir.path().into(), registry_in(dir.path()), "id".into()) - .with_wait(Some(std::time::Duration::from_millis(5))); + .with_deadline(Some(std::time::Duration::from_millis(5))); let nap = short.next_poll(now).expect("5ms is still ahead"); assert!(nap <= std::time::Duration::from_millis(5), "{nap:?}"); // And a deadline already in the past ends the park. @@ -645,16 +601,19 @@ mod tests { /// The setting's three states, including the one that is easy to get /// wrong: `0` is a deliberate "no deadline", not a zero-length one that /// aborts every supervised review the instant it parks. + /// + /// Read through `AgentEngineConfig::approval_wait`, which is the single + /// definition the production gate is wired to + /// (`crate::agent::engine::approval_gate_for`). #[test] fn the_setting_maps_absent_and_zero_to_park_forever() { - assert_eq!(park_deadline(None), None); let mut settings = crate::settings::AgentEngineConfig::default(); - assert_eq!(park_deadline(Some(&settings)), None); + assert_eq!(settings.approval_wait(), None); settings.approval_wait_secs = Some(0); - assert_eq!(park_deadline(Some(&settings)), None); + assert_eq!(settings.approval_wait(), None); settings.approval_wait_secs = Some(90); assert_eq!( - park_deadline(Some(&settings)), + settings.approval_wait(), Some(std::time::Duration::from_secs(90)) ); } diff --git a/crates/stella-cli/src/daemon/console.rs b/crates/stella-cli/src/daemon/console.rs index 1a6a323c8..00d91f555 100644 --- a/crates/stella-cli/src/daemon/console.rs +++ b/crates/stella-cli/src/daemon/console.rs @@ -433,9 +433,9 @@ fn wall_ms() -> u64 { /// /// The pumps live behind an `Arc` rather than in this struct because two /// different code paths have to be able to end them: `main`'s orderly exit, -/// and the panic hook [`arm_panic_drain`] installs (#1616). Both call the -/// same idempotent [`Drainable::drain`]; whichever arrives first takes the -/// streams and the other finds nothing to do. +/// and the panic hook [`install_panic_drain`] installs (#1616). Both go +/// through the same idempotent [`ConsoleGuard::drain_shared`]; whichever +/// arrives first takes the streams and the other finds nothing to do. pub(crate) struct ConsoleGuard { #[cfg(unix)] pumps: Arc, @@ -470,6 +470,28 @@ impl ConsoleGuard { #[cfg(unix)] self.pumps.drain(); } + + /// Drain whatever guard is still in `cell`, unless the CURRENT thread is + /// one of the pumps it owns — see [`PUMP_THREAD_PREFIX`]. Idempotent and + /// safe to call from both the normal end-of-`main` path and a panic hook: + /// the two race to be first, and whichever wins performs the one real + /// drain; the loser finds `None` and does nothing. + /// + /// Lives on [`ConsoleGuard`] rather than on the unix-only `Drainable` + /// because `main` calls it on every platform — on a non-unix build the + /// guard owns no pumps and the drain is simply a no-op. + pub(crate) fn drain_shared(cell: &Mutex>) { + if std::thread::current() + .name() + .is_some_and(|name| name.starts_with(PUMP_THREAD_PREFIX)) + { + return; + } + let guard = cell.lock().unwrap_or_else(|p| p.into_inner()).take(); + if let Some(guard) = guard { + guard.drain(); + } + } } #[cfg(unix)] @@ -523,24 +545,6 @@ impl Drainable { } } } - - /// Drain whatever guard is still in `cell`, unless the CURRENT thread is - /// one of the pumps it owns — see [`PUMP_THREAD_PREFIX`]. Idempotent and - /// safe to call from both the normal end-of-`main` path and a panic hook: - /// the two race to be first, and whichever wins performs the one real - /// drain; the loser finds `None` and does nothing. - pub(crate) fn drain_shared(cell: &Mutex>) { - if std::thread::current() - .name() - .is_some_and(|name| name.starts_with(PUMP_THREAD_PREFIX)) - { - return; - } - let guard = cell.lock().unwrap_or_else(|p| p.into_inner()).take(); - if let Some(guard) = guard { - guard.drain(); - } - } } /// Install a panic hook that drains `cell`'s console guard — restoring the @@ -601,35 +605,6 @@ pub(crate) fn install_bounded(sidecar: &Path) -> Option { } } -/// Drain the console when this process panics, before the panic message is -/// lost (#1616). -/// -/// The pump made the console's tail lossy on a violent death: a panic unwinds -/// past `main`'s orderly [`ConsoleGuard::drain`], the process exits without -/// joining the pump threads, and up to a pipe buffer of output — the panic -/// message itself, most of the time — dies in the pipe. That is the one -/// artifact a postmortem of a supervised child actually wants. -/// -/// The hook chains rather than replaces: whatever hook is already installed -/// (the diagnostics dump, or the default printer) runs FIRST, so its output -/// goes down the pipe like everything else, and only then is the pipe drained -/// into the file. Draining first would restore the fds and leave the panic -/// message to land raw — readable, but ahead of the pump's own `Closed` -/// marker and outside the bound. -pub(crate) fn arm_panic_drain(guard: &ConsoleGuard) { - #[cfg(not(unix))] - let _ = guard; - #[cfg(unix)] - { - let pumps = guard.pumps.clone(); - let previous = std::panic::take_hook(); - std::panic::set_hook(Box::new(move |info| { - previous(info); - pumps.drain(); - })); - } -} - /// Replace `target_fd` with a pipe and spawn the pump that empties it into /// the bounded console file. Answers `None` on any syscall failure, leaving /// the fd exactly as it was. diff --git a/crates/stella-cli/src/settings.rs b/crates/stella-cli/src/settings.rs index f5130cb1a..06ada3c86 100644 --- a/crates/stella-cli/src/settings.rs +++ b/crates/stella-cli/src/settings.rs @@ -841,8 +841,14 @@ impl AgentEngineConfig { self.hunk_review.is_some_and(Toggle::is_on) } - /// The parked-approval deadline, if the operator set one (#1616). - /// `None` (absent or `0`) parks forever. + /// The parked-approval deadline, if the operator set one (#1616), or + /// `None` for the shipped park-forever behaviour. + /// + /// `0` is spelled and means "no deadline", the same convention + /// `model_timeout_secs` uses for its own backstop: in a field whose + /// absence already means "the default", zero is the only way to say + /// *deliberately* unbounded — which matters when a user-scope file sets a + /// deadline and one project wants out of it. pub fn approval_wait(&self) -> Option { self.approval_wait_secs .filter(|secs| *secs > 0) From 6eaaf51bcf89efc0de9bedc2d390d7de07137600 Mon Sep 17 00:00:00 2001 From: Stella Test Date: Wed, 5 Aug 2026 20:05:13 -0700 Subject: [PATCH 2/5] fix(stella-cli): record a deliberate stop as ended, not as a crash (#1653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1620 and #1637 made the exit code honest — an `AbortKind::DeliberateStop` (stuck-loop escalation, the step cap, an enforced budget, a scope review the user ended) exits 3 and a crash exits 1. The session registry still collapsed the two: both writers reduced the outcome to a bool one line before the write, and `outcome_status(false, None)` mapped to `SessionStatus::Error`. So `stella daemon list` and the deck SESSIONS view painted a run that stopped by policy identically to one that fell over. The design call the issue asked for: a deliberate stop reuses `SessionStatus::Cancelled` rather than earning a variant. The two endings are one fact to every reader — the work was ended, not broken — and it is already what `outcome_status` writes for a signal. A new variant would also have forced lines into `command_deck.rs` and `views/session.rs`, both grandfathered god files closed to growth, for no information a reader gains. What that buys is the load-bearing half: `Error` now means only "it fell over", which is the predicate a boot-time resume sweep needs to tell a resumable crash from a run it must never restart (#1696). - `CliFailure::is_deliberate_stop` exposes the bit `exit_code` already turns into 3. - `outcome_status` takes `Option<&CliFailure>` — the terminal answer itself — instead of a bool, so both writers can record what actually happened. - Both call sites updated: `main`'s Err arm passes the failure it already holds, and the resume driver passes `result.as_ref().err()`. Witness: `a_deliberate_stop_is_not_recorded_as_a_crash` asserts the two endings store different statuses, where today both store `Error`. Refs #1637, #1620, #1586 Closes #1653 --- crates/stella-cli/src/agent/resume.rs | 5 ++- crates/stella-cli/src/daemon.rs | 44 +++++++++++++++++-------- crates/stella-cli/src/daemon/tests.rs | 46 +++++++++++++++++++++++++++ crates/stella-cli/src/failure.rs | 10 ++++++ crates/stella-cli/src/main.rs | 6 ++-- crates/stella-store/src/sessions.rs | 9 +++++- 6 files changed, 103 insertions(+), 17 deletions(-) diff --git a/crates/stella-cli/src/agent/resume.rs b/crates/stella-cli/src/agent/resume.rs index 85acc9192..cf55e44d3 100644 --- a/crates/stella-cli/src/agent/resume.rs +++ b/crates/stella-cli/src/agent/resume.rs @@ -262,7 +262,10 @@ pub(crate) async fn run_resume(cfg: &Config, id: Option<&str>) -> Result<(), Cli // covers the spawned child), so the hand-run `--foreground` case records // a terminal status too. Same value on both paths; double-writing it is // harmless, leaving it unwritten reads as a crash forever. - let _ = registry.set_status(&record.id, crate::daemon::outcome_status(result.is_ok())); + let _ = registry.set_status( + &record.id, + crate::daemon::outcome_status(result.as_ref().err()), + ); result } diff --git a/crates/stella-cli/src/daemon.rs b/crates/stella-cli/src/daemon.rs index 71bf45a81..50938c5f7 100644 --- a/crates/stella-cli/src/daemon.rs +++ b/crates/stella-cli/src/daemon.rs @@ -100,6 +100,7 @@ use colored::{ColoredString, Colorize}; use stella_store::{SessionRecord, SessionRegistry, SessionStatus, SupervisorInfo, supervised}; use crate::DaemonCmd; +use crate::failure::CliFailure; /// `install` / `uninstall`: the service-manager half (#1587) — what makes a /// registered invocation come back after the logout and reboot that @@ -986,25 +987,42 @@ const LOCK_FD_SCAN_LIMIT: i32 = 64; /// that have one, and writes the same value. This is what covers the paths /// that do not — `stella fleet`, and any future long-running verb that is /// handed to the supervisor before it grows a session presence. -pub(crate) fn record_outcome_if_supervised(ok: bool) { +pub(crate) fn record_outcome_if_supervised(failure: Option<&CliFailure>) { let Some(id) = supervised_id() else { return; }; - let _ = SessionRegistry::open_default().set_status(&id, outcome_status(ok)); + let _ = SessionRegistry::open_default().set_status(&id, outcome_status(failure)); } -/// The terminal status a finished run records. +/// The terminal status a finished run records — `None` for a run that ended +/// on its own terms, the failure itself for one that did not. /// -/// A signal is not a failure: the run was stopped, and recording it as an -/// error would put a deliberate `stella daemon stop` in the registry beside -/// the runs that genuinely broke. Shared with the resume driver -/// (`crate::agent::resume`), which writes its own terminal status for the -/// hand-run `--foreground` case no supervised env var covers. -pub(crate) fn outcome_status(ok: bool) -> SessionStatus { - match (ok, crate::signals::interrupted_exit_code()) { - (_, Some(_)) => SessionStatus::Cancelled, - (true, None) => SessionStatus::Complete, - (false, None) => SessionStatus::Error, +/// Three endings, not two. A signal is not a failure: the run was stopped, and +/// recording it as an error would put a deliberate `stella daemon stop` in the +/// registry beside the runs that genuinely broke. Neither is a **deliberate +/// stop** ([`AbortKind::DeliberateStop`]: stuck-loop escalation, the step cap, +/// an enforced budget, a scope review the user ended) — the process exits `3` +/// rather than `1` for exactly that reason (#1620, #1637), and collapsing it +/// back to [`SessionStatus::Error`] one line before the write made the registry +/// the last place that still could not tell a policy stop from a crash (#1653). +/// +/// Both land on [`SessionStatus::Cancelled`] rather than a new variant, because +/// they are the same fact to every reader: **the work was ended, not broken.** +/// That leaves `Error` meaning only "it fell over", which is what lets a +/// boot-time sweep tell a resumable crash from a run it must never restart +/// (#1696). +/// +/// Shared with the resume driver (`crate::agent::resume`), which writes its own +/// terminal status for the hand-run `--foreground` case no supervised env var +/// covers. +pub(crate) fn outcome_status(failure: Option<&CliFailure>) -> SessionStatus { + if crate::signals::interrupted_exit_code().is_some() { + return SessionStatus::Cancelled; + } + match failure { + None => SessionStatus::Complete, + Some(f) if f.is_deliberate_stop() => SessionStatus::Cancelled, + Some(_) => SessionStatus::Error, } } diff --git a/crates/stella-cli/src/daemon/tests.rs b/crates/stella-cli/src/daemon/tests.rs index 04bd8575a..f68c0df0f 100644 --- a/crates/stella-cli/src/daemon/tests.rs +++ b/crates/stella-cli/src/daemon/tests.rs @@ -542,3 +542,49 @@ fn a_resumed_launch_keeps_the_record_and_the_crashed_console() { "a relaunch re-owns the record as live" ); } + +/// #1653 witness: the terminal status separates a run that *chose* to stop +/// from one that fell over. +/// +/// Before this, both writers reduced the outcome to a `bool` and every +/// non-signal failure stored [`SessionStatus::Error`] — so a stuck-loop +/// escalation, an enforced budget or an ended scope review was recorded +/// identically to a crash, and `stella daemon list` painted them the same. +/// The exit code had already learned the difference (#1620, #1637); the +/// registry was the last reader that had not. +/// +/// Asserted directly against [`outcome_status`], which is where the +/// distinction was being discarded. The signal rung is deliberately not +/// asserted here: `crate::signals::interrupted_exit_code` reads a process-global +/// flag, so a test that set it would leak into every other test in this binary. +#[test] +fn a_deliberate_stop_is_not_recorded_as_a_crash() { + use stella_core::AbortKind; + + let stop = CliFailure::from_abort( + "stuck-loop detected (persisted after a steering warning)".into(), + AbortKind::DeliberateStop, + ); + let crash = CliFailure::from_abort("model call failed: 500".into(), AbortKind::Failure); + + assert_eq!( + outcome_status(Some(&stop)), + SessionStatus::Cancelled, + "a policy stop ended the work; it did not break it" + ); + assert_eq!( + outcome_status(Some(&crash)), + SessionStatus::Error, + "a genuine failure must stay distinguishable from a policy stop" + ); + assert_ne!( + outcome_status(Some(&stop)), + outcome_status(Some(&crash)), + "the two endings must not collapse — #1696 reads exactly this bit" + ); + assert_eq!( + outcome_status(None), + SessionStatus::Complete, + "a run that ended on its own terms is complete" + ); +} diff --git a/crates/stella-cli/src/failure.rs b/crates/stella-cli/src/failure.rs index d141a1280..faa866a94 100644 --- a/crates/stella-cli/src/failure.rs +++ b/crates/stella-cli/src/failure.rs @@ -76,6 +76,16 @@ impl CliFailure { pub(crate) fn message(&self) -> &str { &self.message } + + /// Whether the run *chose* to end rather than fell over — the same bit + /// [`Self::exit_code`] turns into `3`, asked directly. + /// + /// The terminal-status writers need it too (#1653): a policy stop and a + /// crash are one `SessionStatus` apart, and without this accessor the + /// distinction is thrown away one line before the registry write. + pub(crate) fn is_deliberate_stop(&self) -> bool { + self.deliberate_stop + } } impl From for CliFailure { diff --git a/crates/stella-cli/src/main.rs b/crates/stella-cli/src/main.rs index 7cbe89115..53d11b0f5 100644 --- a/crates/stella-cli/src/main.rs +++ b/crates/stella-cli/src/main.rs @@ -555,7 +555,7 @@ fn main() -> ExitCode { let code = match run(cli, &loaded_env) { Ok(()) => { - daemon::record_outcome_if_supervised(true); + daemon::record_outcome_if_supervised(None); // A supervisor's own exit code says only whether it managed to // stream a log. What a script wrapping `stella run` is asking // about is the run, so the child's code is forwarded verbatim. @@ -565,7 +565,9 @@ fn main() -> ExitCode { } } Err(e) => { - daemon::record_outcome_if_supervised(false); + // The failure itself, not `false`: a deliberate stop is recorded + // as ended-on-purpose, never as a crash (#1653). + daemon::record_outcome_if_supervised(Some(&e)); eprintln!("{} {}", "stella:".red().bold(), e); emit_error_summary(output_format, e.message()); // §7.4's second trigger, and the one that fires more often: most diff --git a/crates/stella-store/src/sessions.rs b/crates/stella-store/src/sessions.rs index 91c5a911c..781741412 100644 --- a/crates/stella-store/src/sessions.rs +++ b/crates/stella-store/src/sessions.rs @@ -40,7 +40,14 @@ pub enum SessionStatus { /// switched away) with work still pending. Not live (no pid downgrade /// applies), and the first thing `resume` looks for. Paused, - /// The user interrupted the work (Ctrl-C mid-turn, queue abandoned). + /// The work was **ended**, not broken. Two endings share this status + /// because they are one fact to every reader: the user interrupted it + /// (Ctrl-C mid-turn, queue abandoned), or the run stopped itself by + /// policy — a stuck loop escalated past its warning, the step cap, an + /// enforced budget, a scope review the user ended (#1653). + /// + /// Its counterpart [`SessionStatus::Error`] therefore means only "it fell + /// over", which is the distinction a boot-time resume sweep reads. Cancelled, /// The session ended after finishing its work. Complete, From 7c523362c26b9bf7f16bfb0c4de08d100212a676 Mon Sep 17 00:00:00 2001 From: Stella Test Date: Wed, 5 Aug 2026 20:26:26 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(stella-cli):=20unbreak=20main=20?= =?UTF-8?q?=E2=80=94=20test-cfg=20residue=20of=20the=20same=20two=20merges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither of these compiles on main; both are the cfg(test) half of the collisions the previous commit reconciled in the shipping code. - `daemon/console/tests.rs` builds a `ConsoleGuard { streams: … }`, the struct shape from before the pumps moved behind `Arc`. - `fleet_claims.rs` matches `cli.command` as a bare `Command` where it is an `Option`, and formats `Command` with `{:?}` where it derives no `Debug`. The mismatch arm now states what was expected instead of printing what arrived, which is the half a reader of the failure needs anyway. --- crates/stella-cli/src/daemon/console/tests.rs | 12 +++++++----- crates/stella-cli/src/fleet_claims.rs | 15 +++++++++------ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/crates/stella-cli/src/daemon/console/tests.rs b/crates/stella-cli/src/daemon/console/tests.rs index 45cee3af2..b4ca97e2f 100644 --- a/crates/stella-cli/src/daemon/console/tests.rs +++ b/crates/stella-cli/src/daemon/console/tests.rs @@ -266,14 +266,16 @@ fn a_session_without_an_index_falls_back_to_raw_tails() { /// panic hook and the normal end-of-`main` cleanup reaches a shared guard /// first performs the one real drain, and — the specific hazard a panicking /// pump thread would create — a pump thread must never try to join itself. -/// `ConsoleGuard { streams: Vec::new() }` needs no real fds (this module's own -/// doc comment: hijacking the test runner's stdout to exercise the fd -/// plumbing would be the tail wagging the dog), so this is fd-free logic, not -/// glue. On `main`, `drain_shared` does not exist at all. +/// A guard over an empty pump set needs no real fds (this module's own doc +/// comment: hijacking the test runner's stdout to exercise the fd plumbing +/// would be the tail wagging the dog), so this is fd-free logic, not glue. On +/// `main`, `drain_shared` does not exist at all. #[test] fn drain_shared_skips_a_pump_thread_draining_itself_but_runs_from_anywhere_else() { let cell = Arc::new(Mutex::new(Some(ConsoleGuard { - streams: Vec::new(), + pumps: Arc::new(Drainable { + streams: Mutex::new(Vec::new()), + }), }))); // Called as if FROM a pump thread: a no-op, or a real pump trying to diff --git a/crates/stella-cli/src/fleet_claims.rs b/crates/stella-cli/src/fleet_claims.rs index 7c81ad9b3..63ba336be 100644 --- a/crates/stella-cli/src/fleet_claims.rs +++ b/crates/stella-cli/src/fleet_claims.rs @@ -326,35 +326,38 @@ mod tests { #[test] fn fleet_claims_parses_as_a_verb_and_defaults_to_live_text() { let cli = Cli::try_parse_from(["stella", "fleet", "claims"]).unwrap(); + // `Command` derives no `Debug`, so the mismatch arm cannot print what + // it got — it says what was expected instead, which is the half a + // reader of the failure actually needs. match cli.command { - Command::Fleet { + Some(Command::Fleet { cmd: Some(crate::fleet_verbs::FleetCmd::Claims { all, format }), .. - } => { + }) => { assert!(!all, "the live listing is the default"); assert_eq!(format, QueryFormat::Text); } - other => panic!("expected `fleet claims`, got {other:?}"), + _ => panic!("`stella fleet claims` must parse as the claims verb"), } let cli = Cli::try_parse_from(["stella", "fleet", "claims", "--all", "--format", "json"]) .unwrap(); assert!(matches!( cli.command, - Command::Fleet { + Some(Command::Fleet { cmd: Some(crate::fleet_verbs::FleetCmd::Claims { all: true, format: QueryFormat::Json }), .. - } + }) )); // And the documented escape hatch still works: `--` makes it a prompt. let cli = Cli::try_parse_from(["stella", "fleet", "--", "claims are stale"]).unwrap(); assert!(matches!( cli.command, - Command::Fleet { cmd: None, .. } + Some(Command::Fleet { cmd: None, .. }) )); } } From 16825e826f32f4764d982855a017fc99b3872532 Mon Sep 17 00:00:00 2001 From: Stella Test Date: Wed, 5 Aug 2026 20:26:51 -0700 Subject: [PATCH 4/5] fix(stella-cli): resume real crashes at boot, and never strand runs behind a parked one (#1696, #1698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes to the resume-at-boot sweep, plus the pid rung `daemon` verbs were missing. **#1696 — an `Error` is continued.** `decide` skipped every terminal status, `Error` included. That was the price of #1653's ambiguity: a policy stop and a crash both recorded `Error`, so continuing one risked restarting, unattended and at the operator's expense, work the operator ended on purpose. The cost was a real crash that managed to write `Error` before dying being stranded. #1653 removed the ambiguity — a policy stop records `Cancelled` with every other deliberate ending — so `Error` now means only "it fell over". No new `BootCandidate` field: the stored status *is* the distinction, and a second copy of it would be a second thing to keep true. Two facts make the widening safe rather than merely intended, and both are asserted: a deliberate stop retracts its checkpoint on the way out (`discard_checkpoint` runs on every terminal path in the driver, abort included), so even a pre-#1653 row that really was a policy stop is filtered by `NoResumePoint` without this module trusting its status; and the `MAX_BOOT_ATTEMPTS` bound still applies to whatever it continues. **#1698 — a parked run no longer blocks the sweep.** A run interrupted while parked on a scope review left its `approval-request.json` behind, and resuming it at boot re-parked it immediately with no terminal and nobody to answer. Because the sweep is sequential, that run blocked every remaining id forever and the service console showed it as continued and then went quiet — work stranded silently, which is the shape this module exists to prevent. Option (1) of the three the issue offered: a `parked_on_approval` fact on `BootCandidate`, probed from the sidecar, and its own `SkipReason::NeedsInput` naming `daemon attach` as the way through. Read from the sidecar rather than inferred from `NeedsInput`, because a run killed the instant after its review was answered carries that status too and has nothing left to ask. This bounds the park the sweep can see, not a resumed turn that parks on a *new* review; that needs a per-resume wall-clock ceiling and is filed separately rather than guessed at. **#1690 — `daemon` verbs take a pid.** `resolve` accepted an exact id or a unique prefix. A pid is what `ps`, `top`, Activity Monitor and an OOM-killer log hand you, and it was a dead end: you had to eyeball `daemon list` to translate it first. An all-digits argument is now matched against `SessionRecord::pid`, with the same ambiguity rule as a prefix — a reused pid is refused, not guessed at. The forms cannot collide: a session id is `ses--` and never parses as digits alone. Witnesses: `a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_not`, `a_parked_run_is_skipped_and_does_not_strand_the_runs_behind_it` (which asserts the run *behind* the parked one is still decided — the half that matters), `an_answered_review_leaves_the_run_resumable`, and `a_pid_resolves_to_its_run_and_an_ambiguous_one_is_refused`. `nothing_is_ever_continued_without_a_resume_point` grows the parked dimension and drops the `is_live()` clause that was the old selection rule restated; the invariant its name promises is unchanged and still asserted. Refs #1627, #1653, #1586, #1585, #1607, #1594 Closes #1696 Closes #1698 Closes #1690 --- crates/stella-cli/src/daemon.rs | 25 ++++ crates/stella-cli/src/daemon/boot.rs | 107 +++++++++++++--- crates/stella-cli/src/daemon/boot/tests.rs | 139 ++++++++++++++++++--- crates/stella-cli/src/daemon/tests.rs | 42 +++++++ website/content/docs/commands/daemon.mdx | 8 +- 5 files changed, 281 insertions(+), 40 deletions(-) diff --git a/crates/stella-cli/src/daemon.rs b/crates/stella-cli/src/daemon.rs index 50938c5f7..0de04cd03 100644 --- a/crates/stella-cli/src/daemon.rs +++ b/crates/stella-cli/src/daemon.rs @@ -1195,6 +1195,14 @@ fn mark_stopped(registry: &SessionRegistry, id: &str) { /// is a timestamp and a pid and nobody is going to type it. `None` picks the /// most recent supervised run, which is what "the one I just started" means /// nine times in ten. +/// +/// An all-digits argument is a **pid** (#1690). A pid is what `ps`, `top`, +/// Activity Monitor and an OOM-killer log hand you, and without this rung it +/// is a dead end: the operator holding one has to eyeball `stella daemon list` +/// to translate it into an id before any verb will take it. The two forms +/// cannot collide — a session id is `ses--`, so it never parses +/// as digits alone — which is what lets this rung sit ahead of the prefix +/// match without making any existing input ambiguous. pub(crate) fn resolve( registry: &SessionRegistry, id: Option<&str>, @@ -1219,6 +1227,23 @@ pub(crate) fn resolve( return Ok(exact.clone()); } + // A pid, if the argument is one and a run owns it. Ambiguity is resolved + // exactly as it is for a prefix, and for the same reason: two records + // claiming one pid means the operator has to say which, not that this + // picks for them. + if let Ok(pid) = id.parse::() { + let mut by_pid = supervised_runs.iter().filter(|r| r.pid == pid); + if let Some(first) = by_pid.next() { + return match by_pid.next() { + None => Ok(first.clone()), + Some(second) => Err(format!( + "pid {pid} matches more than one run ({} and {}) — use the id", + first.id, second.id + )), + }; + } + } + let mut matches = supervised_runs.into_iter().filter(|r| r.id.starts_with(id)); let Some(first) = matches.next() else { return Err(format!( diff --git a/crates/stella-cli/src/daemon/boot.rs b/crates/stella-cli/src/daemon/boot.rs index 5f934194a..06c9a4ed5 100644 --- a/crates/stella-cli/src/daemon/boot.rs +++ b/crates/stella-cli/src/daemon/boot.rs @@ -29,21 +29,59 @@ //! 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`, a Ctrl-C, **or a policy stop** ended it, `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, 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 (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 `Cancelled` with +//! every other deliberate ending, which leaves `Error` meaning only "it fell +//! over" — a run that is exactly as entitled to be continued as one the kernel +//! took without warning. +//! +//! Two independent facts make the widening safe rather than merely intended, +//! and both are checked below rather than assumed: +//! +//! - **A deliberate stop has no resume point.** `discard_checkpoint` runs on +//! every terminal path in the engine driver, abort included, so a policy +//! stop retracts its checkpoint 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`. +//! +//! # A parked run does not strand the ones behind it (#1698) +//! +//! A run interrupted while parked on a scope review left its +//! `approval-request.json` in the sidecar, and resuming it at boot re-parks it +//! immediately — under launchd or systemd, with no terminal and nobody to +//! answer. The sweep is sequential, so that one run would block every +//! remaining id forever and the console would simply go quiet: work stranded +//! silently, which is the exact shape this module exists to prevent. +//! +//! So a pending approval is a fact the selection rule reads +//! ([`SkipReason::NeedsInput`]), and the sweep says so and moves on. `stella +//! daemon attach ` answers the review, and the run resumes from there with +//! a human present — which is the only condition under which it could have +//! made progress anyway. +//! +//! This bounds the park the sweep can *see*. It does not bound a resumed turn +//! that parks on a **new** scope review it had not reached when it was killed; +//! that needs a per-resume wall-clock ceiling, which is tracked separately +//! rather than guessed at here. //! //! # What stops a boot loop //! @@ -122,6 +160,13 @@ pub(super) struct BootCandidate { pub(super) stored_status: SessionStatus, /// Whether the run's liveness lock is currently held. pub(super) lock_held: bool, + /// Whether the run left an unanswered scope review in its sidecar — a + /// resume would re-park on it with nobody at the terminal to answer + /// (#1698). A *fact about the sidecar*, not about the status: a run killed + /// while parked keeps `NeedsInput`, but so does one killed the instant + /// after its review was answered, and only the request document tells them + /// apart. + pub(super) parked_on_approval: bool, /// Whether the run left a resume point this build can see. pub(super) has_resume_point: bool, /// Whether the workspace the turn must continue in still exists. @@ -138,10 +183,16 @@ 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` (a stop, a Ctrl-C, or a policy stop), + /// `Paused`, `Archived`. Since #1653 this no longer covers `Error`; see + /// the module docs. EndedDeliberately, - /// Nothing to continue from — a clean exit discards its resume point. + /// The run is parked on an unanswered scope review, and a boot has nobody + /// to answer it (#1698). + NeedsInput, + /// 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, @@ -156,8 +207,12 @@ 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::NeedsInput => "parked on a scope review — \ + `stella daemon attach ` answers it and the run continues from there" + .to_string(), Self::NoResumePoint => "no resume point".to_string(), Self::WorkspaceGone => "workspace no longer exists".to_string(), Self::AttemptsExhausted => format!( @@ -194,9 +249,19 @@ 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 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); } + // Before the resume-point check: a parked run has a perfectly good resume + // point, and reporting it as resumable-but-for-a-question is what sends + // the operator to `daemon attach` instead of to a bug report (#1698). + if candidate.parked_on_approval { + return BootDecision::Skip(SkipReason::NeedsInput); + } if !candidate.has_resume_point { return BootDecision::Skip(SkipReason::NoResumePoint); } @@ -304,6 +369,10 @@ fn candidate( .get(&record.id) .map_or(record.status, |stored| stored.status), lock_held: super::lock_is_held(®istry.sidecar_dir(&record.id)) == Some(true), + parked_on_approval: registry + .sidecar_dir(&record.id) + .join(stella_store::supervised::APPROVAL_REQUEST) + .exists(), has_resume_point: super::has_resume_point(record), workspace_exists: Path::new(&record.workspace).is_dir(), attempts: ledger.attempts(&record.id), diff --git a/crates/stella-cli/src/daemon/boot/tests.rs b/crates/stella-cli/src/daemon/boot/tests.rs index cfa1478c5..17af70495 100644 --- a/crates/stella-cli/src/daemon/boot/tests.rs +++ b/crates/stella-cli/src/daemon/boot/tests.rs @@ -23,6 +23,7 @@ fn killed_mid_turn() -> BootCandidate { supervised: true, stored_status: SessionStatus::InProgress, lock_held: false, + parked_on_approval: false, has_resume_point: true, workspace_exists: true, attempts: 0, @@ -36,17 +37,14 @@ 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 + // that includes a deliberate policy stop, which records `Cancelled` with + // the rest instead of hiding among the crashes as `Error`. for status in [ SessionStatus::Cancelled, SessionStatus::Complete, SessionStatus::Paused, SessionStatus::Archived, - SessionStatus::Error, ] { let candidate = BootCandidate { stored_status: status, @@ -60,6 +58,94 @@ 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::Cancelled, + ..killed_mid_turn() + }; + assert_eq!( + decide(&stopped), + BootDecision::Skip(SkipReason::EndedDeliberately) + ); +} + +/// The #1698 witness: a run parked on an unanswered scope review is skipped +/// with its own reason, and — the half that actually matters — every +/// candidate *behind* it is still decided. +/// +/// Before this, the sweep resumed the parked run and streamed it to +/// completion, which for a park under launchd with no terminal means forever: +/// the loop never reached the remaining ids, and the service console showed +/// the run as continued and then went quiet. +#[test] +fn a_parked_run_is_skipped_and_does_not_strand_the_runs_behind_it() { + let parked = BootCandidate { + id: "ses-1754431200000-84214".to_string(), + parked_on_approval: true, + ..killed_mid_turn() + }; + assert_eq!( + decide(&parked), + BootDecision::Skip(SkipReason::NeedsInput), + "a boot has nobody to answer a scope review" + ); + + let behind = killed_mid_turn(); + let decisions = plan(&[parked, behind.clone()]); + assert_eq!(decisions.len(), 2); + assert_eq!( + decisions[1], + (behind, BootDecision::Continue), + "the run behind the parked one must still be reached and continued" + ); +} + +/// A pending approval is read from the sidecar, not inferred from the status: +/// a run killed the instant after its review was answered also carries +/// `NeedsInput`, and it has nothing left to ask. +#[test] +fn an_answered_review_leaves_the_run_resumable() { + let answered = BootCandidate { + stored_status: SessionStatus::NeedsInput, + parked_on_approval: false, + ..killed_mid_turn() + }; + assert_eq!(decide(&answered), BootDecision::Continue); +} + #[test] fn a_run_that_survived_the_boot_is_not_started_a_second_time() { let candidate = BootCandidate { @@ -110,24 +196,39 @@ fn an_unsupervised_or_workspaceless_or_pointless_run_is_skipped_with_its_own_rea fn nothing_is_ever_continued_without_a_resume_point() { for supervised in [true, false] { for lock_held in [true, false] { - for has_resume_point in [true, false] { - for workspace_exists in [true, false] { - for status in SessionStatus::ALL { - let candidate = BootCandidate { - supervised, - lock_held, - has_resume_point, - workspace_exists, - stored_status: status, - ..killed_mid_turn() - }; - if decide(&candidate) == BootDecision::Continue { + for parked_on_approval in [true, false] { + for has_resume_point in [true, false] { + for workspace_exists in [true, false] { + for status in SessionStatus::ALL { + let candidate = BootCandidate { + supervised, + lock_held, + parked_on_approval, + has_resume_point, + workspace_exists, + stored_status: status, + ..killed_mid_turn() + }; + if decide(&candidate) != BootDecision::Continue { + continue; + } assert!( has_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:?}" + ); + // A boot has nobody to answer a scope review, so a + // parked run is never continued into one (#1698). + assert!(!parked_on_approval, "{candidate:?}"); } } } diff --git a/crates/stella-cli/src/daemon/tests.rs b/crates/stella-cli/src/daemon/tests.rs index f68c0df0f..7303e9116 100644 --- a/crates/stella-cli/src/daemon/tests.rs +++ b/crates/stella-cli/src/daemon/tests.rs @@ -426,6 +426,48 @@ fn ids_resolve_by_unique_prefix_and_refuse_an_ambiguous_one() { let _ = std::fs::remove_dir_all(&dir); } +/// The #1690 witness: a pid resolves too. +/// +/// A pid is what `ps`, `top`, Activity Monitor and an OOM-killer log hand an +/// operator, and on today's code it is a dead end — every verb takes only the +/// id, so the pid must be eyeballed back through `stella daemon list` first. +/// The two address spaces cannot collide: a session id is `ses--` +/// and never parses as digits alone. +#[test] +fn a_pid_resolves_to_its_run_and_an_ambiguous_one_is_refused() { + let (dir, registry) = temp_registry("resolve-pid"); + let mut a = SessionRecord::new("/w", "a"); + a.id = "ses-100-4242".into(); + a.pid = 4242; + a.supervisor = Some(SupervisorInfo { pgid: 4242 }); + let mut b = SessionRecord::new("/w", "b"); + b.id = "ses-100-9001".into(); + b.pid = 9001; + b.supervisor = Some(SupervisorInfo { pgid: 9001 }); + // Two records that share a pid — the OS reuses them, so a long-lived + // registry really can hold both. + let mut stale = SessionRecord::new("/w", "stale"); + stale.id = "ses-050-9001".into(); + stale.pid = 9001; + stale.supervisor = Some(SupervisorInfo { pgid: 9001 }); + for record in [&a, &b, &stale] { + registry.upsert(record).unwrap(); + } + + assert_eq!(resolve(®istry, Some("4242")).unwrap().id, a.id); + assert!( + resolve(®istry, Some("9001")) + .is_err_and(|e| e.contains("more than one run")), + "a reused pid must be refused, not guessed at" + ); + assert!(resolve(®istry, Some("77777")).is_err(), "no run owns it"); + // The id form is untouched: an exact id still wins, and it is never read + // as a pid because it is not digits. + assert_eq!(resolve(®istry, Some("ses-100-4242")).unwrap().id, a.id); + + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn an_exit_code_is_forwarded_the_way_a_shell_reports_it() { let status = |script: &str| { diff --git a/website/content/docs/commands/daemon.mdx b/website/content/docs/commands/daemon.mdx index 032f3d7a4..55f7a2362 100644 --- a/website/content/docs/commands/daemon.mdx +++ b/website/content/docs/commands/daemon.mdx @@ -99,7 +99,7 @@ Every supervised run on this machine, newest first. Local reads only — no API ## `attach` -Streams the run's output into this terminal, from the beginning, and stays until the run ends. A run that has already finished prints in full and exits. The id can be any unique prefix; omit it for the most recently started run. If the run is parked on a scope review, attach renders the proposal and delivers your answer. +Streams the run's output into this terminal, from the beginning, and stays until the run ends. A run that has already finished prints in full and exits. The id can be any unique prefix, or the run's **pid** — the number `ps`, `top` or an OOM-killer log hands you; omit it for the most recently started run. If the run is parked on a scope review, attach renders the proposal and delivers your answer. Detaching again — `Ctrl-C` — leaves the run alone. `stella daemon stop` is what stops it. @@ -167,7 +167,11 @@ 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. + +**A run parked on a scope review is skipped**, and the sweep says so and moves to the next one. A boot has no terminal and nobody to answer the review, so resuming it would park it again forever — and because the sweep is sequential, that would strand every run behind it with no indication why. Answer it with `stella daemon attach ` and the run continues from there. **What bounds it.** Each run gets at most **three** boot-time resumes. The count is durable (`~/.stella/services/resume-boot.json`) and is written *before* the resume is spawned, so a run that takes the machine down with it is still charged for the attempt. Past three, the run is retired from the sweep and left for `stella daemon resume ` by hand — a human deciding to try again is what the bound exists to require. Runs are resumed one at a time; several turns resuming at once is several models spending at once on a machine nobody is watching. From 13695884c0376612ab9ee3d7aac85d1c770df48e Mon Sep 17 00:00:00 2001 From: Mac Anderson Date: Thu, 6 Aug 2026 17:44:08 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(stella-cli):=20reduce=20this=20branch?= =?UTF-8?q?=20to=20its=20one=20unshipped=20fix=20=E2=80=94=20resume=20a=20?= =?UTF-8?q?recorded-error=20crash=20at=20boot=20(#1696)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Main independently landed the rest of this batch while it sat: the deliberate-stop status (#1653 via SessionStatus::Stopped, #1828/#1878), pid addressing (#1690 via #1723), the parked-run boot skip (#1698 via #1920), and the #1616 drain/panic-hook machinery. Those files are taken from main wholesale; the duplicated pid test, the stale-signature outcome_status test, and the unwired drain_shared duplicate go with them. What remains is #1696, rebuilt on main's design: boot::decide lets `Error` fall through to the resume-point check, because since #1653 a policy stop records `Stopped` and `Error` means only 'it fell over'. Safe twice over — a deliberate stop retracts its checkpoint on every terminal path, so a pre-#1653 row is filtered by NoResumePoint anyway, and the boot attempt ledger still bounds an Error that resumes into another Error. Closes #1696 --- crates/stella-cli/src/daemon.rs | 1 - crates/stella-cli/src/daemon/boot.rs | 115 +++++++-------------- crates/stella-cli/src/daemon/boot/tests.rs | 88 ++++------------ crates/stella-cli/src/daemon/tests.rs | 88 ---------------- crates/stella-cli/src/failure.rs | 10 -- crates/stella-store/src/sessions.rs | 9 +- website/content/docs/commands/daemon.mdx | 2 - 7 files changed, 56 insertions(+), 257 deletions(-) diff --git a/crates/stella-cli/src/daemon.rs b/crates/stella-cli/src/daemon.rs index 60c4afc21..cc91f7a3e 100644 --- a/crates/stella-cli/src/daemon.rs +++ b/crates/stella-cli/src/daemon.rs @@ -99,7 +99,6 @@ use colored::{ColoredString, Colorize}; use stella_store::{SessionRecord, SessionRegistry, SessionStatus, SupervisorInfo, supervised}; use crate::DaemonCmd; -use crate::failure::CliFailure; /// `install` / `uninstall`: the service-manager half (#1587) — what makes a /// registered invocation come back after the logout and reboot that diff --git a/crates/stella-cli/src/daemon/boot.rs b/crates/stella-cli/src/daemon/boot.rs index 6c1c3ce7f..53bb8443c 100644 --- a/crates/stella-cli/src/daemon/boot.rs +++ b/crates/stella-cli/src/daemon/boot.rs @@ -24,64 +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 the stored status. Every deliberate end writes one -//! on the way out — `Complete` when it finished, `Cancelled` when `stella -//! daemon stop`, a Ctrl-C, **or a policy stop** ended it, `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. +//! 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. //! -//! # Why an `Error` is continued, and why that is safe (#1696) +//! # 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 (stuck-loop +//! 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. +//! 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 `Cancelled` with -//! every other deliberate ending, which leaves `Error` meaning only "it fell -//! over" — a run that is exactly as entitled to be continued as one the kernel -//! took without warning. +//! #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: //! -//! Two independent facts make the widening safe rather than merely intended, -//! and both are checked below rather than assumed: -//! -//! - **A deliberate stop has no resume point.** `discard_checkpoint` runs on -//! every terminal path in the engine driver, abort included, so a policy -//! stop retracts its checkpoint on the way out. A row written by a build -//! that predates #1653 — where a policy stop really did store `Error` — is +//! - **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`. -//! -//! # A parked run does not strand the ones behind it (#1698) -//! -//! A run interrupted while parked on a scope review left its -//! `approval-request.json` in the sidecar, and resuming it at boot re-parks it -//! immediately — under launchd or systemd, with no terminal and nobody to -//! answer. The sweep is sequential, so that one run would block every -//! remaining id forever and the console would simply go quiet: work stranded -//! silently, which is the exact shape this module exists to prevent. -//! -//! So a pending approval is a fact the selection rule reads -//! ([`SkipReason::NeedsInput`]), and the sweep says so and moves on. `stella -//! daemon attach ` answers the review, and the run resumes from there with -//! a human present — which is the only condition under which it could have -//! made progress anyway. -//! -//! This bounds the park the sweep can *see*. It does not bound a resumed turn -//! that parks on a **new** scope review it had not reached when it was killed; -//! that needs a per-resume wall-clock ceiling, which is tracked separately -//! rather than guessed at here. +//! - **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 //! @@ -194,13 +173,6 @@ pub(super) struct BootCandidate { pub(super) stored_status: SessionStatus, /// Whether the run's liveness lock is currently held. pub(super) lock_held: bool, - /// Whether the run left an unanswered scope review in its sidecar — a - /// resume would re-park on it with nobody at the terminal to answer - /// (#1698). A *fact about the sidecar*, not about the status: a run killed - /// while parked keeps `NeedsInput`, but so does one killed the instant - /// after its review was answered, and only the request document tells them - /// apart. - pub(super) parked_on_approval: bool, /// Whether the run left a resume point this build can see. pub(super) has_resume_point: bool, /// Whether the workspace the turn must continue in still exists. @@ -226,13 +198,10 @@ pub(super) enum SkipReason { /// second copy of it is the one outcome worse than not resuming. StillRunning, /// The run recorded a terminal status that says it *ended* rather than - /// broke — `Complete`, `Cancelled` (a stop, a Ctrl-C, or a policy stop), - /// `Paused`, `Archived`. Since #1653 this no longer covers `Error`; see - /// the module docs. + /// broke — `Complete`, `Cancelled`, `Stopped`, `Paused`, `Archived`. + /// Since #1653 this no longer covers `Error`; see the module docs on + /// #1696. EndedDeliberately, - /// The run is parked on an unanswered scope review, and a boot has nobody - /// to answer it (#1698). - NeedsInput, /// Nothing to continue from — a clean exit, and every deliberate stop, /// discards its resume point. NoResumePoint, @@ -255,9 +224,6 @@ impl SkipReason { "ended deliberately — only an interrupted or crashed run is resumed at boot" .to_string() } - Self::NeedsInput => "parked on a scope review — \ - `stella daemon attach ` answers it and the run continues from there" - .to_string(), Self::NoResumePoint => "no resume point".to_string(), Self::WorkspaceGone => "workspace no longer exists".to_string(), Self::NeedsInput => "waiting on an approval — answer it with \ @@ -297,19 +263,14 @@ pub(super) fn decide(candidate: &BootCandidate) -> BootDecision { if candidate.lock_held { return BootDecision::Skip(SkipReason::StillRunning); } - // `Error` deliberately falls through to the resume-point check rather than - // being skipped here: since #1653 it means 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. + // `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); } - // Before the resume-point check: a parked run has a perfectly good resume - // point, and reporting it as resumable-but-for-a-question is what sends - // the operator to `daemon attach` instead of to a bug report (#1698). - if candidate.parked_on_approval { - return BootDecision::Skip(SkipReason::NeedsInput); - } if !candidate.has_resume_point { return BootDecision::Skip(SkipReason::NoResumePoint); } @@ -424,10 +385,6 @@ fn candidate( .get(&record.id) .map_or(record.status, |stored| stored.status), lock_held: super::lock_is_held(®istry.sidecar_dir(&record.id)) == Some(true), - parked_on_approval: registry - .sidecar_dir(&record.id) - .join(stella_store::supervised::APPROVAL_REQUEST) - .exists(), has_resume_point: super::has_resume_point(record), workspace_exists: Path::new(&record.workspace).is_dir(), // The request file, not the answer: an answered request is removed by diff --git a/crates/stella-cli/src/daemon/boot/tests.rs b/crates/stella-cli/src/daemon/boot/tests.rs index 8fdaa24af..b4c4cc97c 100644 --- a/crates/stella-cli/src/daemon/boot/tests.rs +++ b/crates/stella-cli/src/daemon/boot/tests.rs @@ -23,7 +23,6 @@ fn killed_mid_turn() -> BootCandidate { supervised: true, stored_status: SessionStatus::InProgress, lock_held: false, - parked_on_approval: false, has_resume_point: true, workspace_exists: true, parked: false, @@ -38,11 +37,13 @@ fn a_run_killed_mid_turn_is_continued_at_boot() { #[test] fn a_run_the_operator_ended_is_never_continued_at_boot() { - // Every status that means "this run ended rather than broke". Since #1653 - // that includes a deliberate policy stop, which records `Cancelled` with - // the rest instead of hiding among the crashes as `Error`. + // 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, @@ -94,7 +95,7 @@ fn a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_not() { // And the status a policy stop records today is skipped outright. let stopped = BootCandidate { - stored_status: SessionStatus::Cancelled, + stored_status: SessionStatus::Stopped, ..killed_mid_turn() }; assert_eq!( @@ -103,50 +104,6 @@ fn a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_not() { ); } -/// The #1698 witness: a run parked on an unanswered scope review is skipped -/// with its own reason, and — the half that actually matters — every -/// candidate *behind* it is still decided. -/// -/// Before this, the sweep resumed the parked run and streamed it to -/// completion, which for a park under launchd with no terminal means forever: -/// the loop never reached the remaining ids, and the service console showed -/// the run as continued and then went quiet. -#[test] -fn a_parked_run_is_skipped_and_does_not_strand_the_runs_behind_it() { - let parked = BootCandidate { - id: "ses-1754431200000-84214".to_string(), - parked_on_approval: true, - ..killed_mid_turn() - }; - assert_eq!( - decide(&parked), - BootDecision::Skip(SkipReason::NeedsInput), - "a boot has nobody to answer a scope review" - ); - - let behind = killed_mid_turn(); - let decisions = plan(&[parked, behind.clone()]); - assert_eq!(decisions.len(), 2); - assert_eq!( - decisions[1], - (behind, BootDecision::Continue), - "the run behind the parked one must still be reached and continued" - ); -} - -/// A pending approval is read from the sidecar, not inferred from the status: -/// a run killed the instant after its review was answered also carries -/// `NeedsInput`, and it has nothing left to ask. -#[test] -fn an_answered_review_leaves_the_run_resumable() { - let answered = BootCandidate { - stored_status: SessionStatus::NeedsInput, - parked_on_approval: false, - ..killed_mid_turn() - }; - assert_eq!(decide(&answered), BootDecision::Continue); -} - #[test] fn a_run_that_survived_the_boot_is_not_started_a_second_time() { let candidate = BootCandidate { @@ -197,29 +154,25 @@ fn an_unsupervised_or_workspaceless_or_pointless_run_is_skipped_with_its_own_rea fn nothing_is_ever_continued_without_a_resume_point() { for supervised in [true, false] { for lock_held in [true, false] { - for parked_on_approval in [true, false] { - for has_resume_point in [true, false] { - for workspace_exists in [true, false] { - for status in SessionStatus::ALL { - let candidate = BootCandidate { - supervised, - lock_held, - parked_on_approval, - has_resume_point, - workspace_exists, - stored_status: status, - ..killed_mid_turn() - }; - if decide(&candidate) != BootDecision::Continue { - continue; - } + for has_resume_point in [true, false] { + for workspace_exists in [true, false] { + for status in SessionStatus::ALL { + let candidate = BootCandidate { + supervised, + lock_held, + has_resume_point, + workspace_exists, + stored_status: status, + ..killed_mid_turn() + }; + if decide(&candidate) == BootDecision::Continue { assert!( has_resume_point, "a boot-time action without a resume point would be a restart, \ not a resume: {candidate:?}" ); assert!(!lock_held, "{candidate:?}"); - // Interrupted, or crashed. Since #1653 an `Error` + // 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. @@ -227,9 +180,6 @@ fn nothing_is_ever_continued_without_a_resume_point() { status.is_live() || status == SessionStatus::Error, "{candidate:?}" ); - // A boot has nobody to answer a scope review, so a - // parked run is never continued into one (#1698). - assert!(!parked_on_approval, "{candidate:?}"); } } } diff --git a/crates/stella-cli/src/daemon/tests.rs b/crates/stella-cli/src/daemon/tests.rs index a82e9c7a3..08af09cba 100644 --- a/crates/stella-cli/src/daemon/tests.rs +++ b/crates/stella-cli/src/daemon/tests.rs @@ -578,48 +578,6 @@ fn ids_resolve_by_unique_prefix_and_refuse_an_ambiguous_one() { let _ = std::fs::remove_dir_all(&dir); } -/// The #1690 witness: a pid resolves too. -/// -/// A pid is what `ps`, `top`, Activity Monitor and an OOM-killer log hand an -/// operator, and on today's code it is a dead end — every verb takes only the -/// id, so the pid must be eyeballed back through `stella daemon list` first. -/// The two address spaces cannot collide: a session id is `ses--` -/// and never parses as digits alone. -#[test] -fn a_pid_resolves_to_its_run_and_an_ambiguous_one_is_refused() { - let (dir, registry) = temp_registry("resolve-pid"); - let mut a = SessionRecord::new("/w", "a"); - a.id = "ses-100-4242".into(); - a.pid = 4242; - a.supervisor = Some(SupervisorInfo { pgid: 4242 }); - let mut b = SessionRecord::new("/w", "b"); - b.id = "ses-100-9001".into(); - b.pid = 9001; - b.supervisor = Some(SupervisorInfo { pgid: 9001 }); - // Two records that share a pid — the OS reuses them, so a long-lived - // registry really can hold both. - let mut stale = SessionRecord::new("/w", "stale"); - stale.id = "ses-050-9001".into(); - stale.pid = 9001; - stale.supervisor = Some(SupervisorInfo { pgid: 9001 }); - for record in [&a, &b, &stale] { - registry.upsert(record).unwrap(); - } - - assert_eq!(resolve(®istry, Some("4242")).unwrap().id, a.id); - assert!( - resolve(®istry, Some("9001")) - .is_err_and(|e| e.contains("more than one run")), - "a reused pid must be refused, not guessed at" - ); - assert!(resolve(®istry, Some("77777")).is_err(), "no run owns it"); - // The id form is untouched: an exact id still wins, and it is never read - // as a pid because it is not digits. - assert_eq!(resolve(®istry, Some("ses-100-4242")).unwrap().id, a.id); - - let _ = std::fs::remove_dir_all(&dir); -} - #[test] fn a_bare_pid_resolves_the_run_that_is_running_under_it() { let (dir, registry) = temp_registry("resolve-pid"); @@ -784,49 +742,3 @@ fn a_resumed_launch_keeps_the_record_and_the_crashed_console() { "a relaunch re-owns the record as live" ); } - -/// #1653 witness: the terminal status separates a run that *chose* to stop -/// from one that fell over. -/// -/// Before this, both writers reduced the outcome to a `bool` and every -/// non-signal failure stored [`SessionStatus::Error`] — so a stuck-loop -/// escalation, an enforced budget or an ended scope review was recorded -/// identically to a crash, and `stella daemon list` painted them the same. -/// The exit code had already learned the difference (#1620, #1637); the -/// registry was the last reader that had not. -/// -/// Asserted directly against [`outcome_status`], which is where the -/// distinction was being discarded. The signal rung is deliberately not -/// asserted here: `crate::signals::interrupted_exit_code` reads a process-global -/// flag, so a test that set it would leak into every other test in this binary. -#[test] -fn a_deliberate_stop_is_not_recorded_as_a_crash() { - use stella_core::AbortKind; - - let stop = CliFailure::from_abort( - "stuck-loop detected (persisted after a steering warning)".into(), - AbortKind::DeliberateStop, - ); - let crash = CliFailure::from_abort("model call failed: 500".into(), AbortKind::Failure); - - assert_eq!( - outcome_status(Some(&stop)), - SessionStatus::Cancelled, - "a policy stop ended the work; it did not break it" - ); - assert_eq!( - outcome_status(Some(&crash)), - SessionStatus::Error, - "a genuine failure must stay distinguishable from a policy stop" - ); - assert_ne!( - outcome_status(Some(&stop)), - outcome_status(Some(&crash)), - "the two endings must not collapse — #1696 reads exactly this bit" - ); - assert_eq!( - outcome_status(None), - SessionStatus::Complete, - "a run that ended on its own terms is complete" - ); -} diff --git a/crates/stella-cli/src/failure.rs b/crates/stella-cli/src/failure.rs index e21758919..a94cee95c 100644 --- a/crates/stella-cli/src/failure.rs +++ b/crates/stella-cli/src/failure.rs @@ -83,16 +83,6 @@ impl CliFailure { pub(crate) fn message(&self) -> &str { &self.message } - - /// Whether the run *chose* to end rather than fell over — the same bit - /// [`Self::exit_code`] turns into `3`, asked directly. - /// - /// The terminal-status writers need it too (#1653): a policy stop and a - /// crash are one `SessionStatus` apart, and without this accessor the - /// distinction is thrown away one line before the registry write. - pub(crate) fn is_deliberate_stop(&self) -> bool { - self.deliberate_stop - } } impl From for CliFailure { diff --git a/crates/stella-store/src/sessions.rs b/crates/stella-store/src/sessions.rs index 910bf2ba6..60d6538ea 100644 --- a/crates/stella-store/src/sessions.rs +++ b/crates/stella-store/src/sessions.rs @@ -40,14 +40,7 @@ pub enum SessionStatus { /// switched away) with work still pending. Not live (no pid downgrade /// applies), and the first thing `resume` looks for. Paused, - /// The work was **ended**, not broken. Two endings share this status - /// because they are one fact to every reader: the user interrupted it - /// (Ctrl-C mid-turn, queue abandoned), or the run stopped itself by - /// policy — a stuck loop escalated past its warning, the step cap, an - /// enforced budget, a scope review the user ended (#1653). - /// - /// Its counterpart [`SessionStatus::Error`] therefore means only "it fell - /// over", which is the distinction a boot-time resume sweep reads. + /// The user interrupted the work (Ctrl-C mid-turn, queue abandoned). Cancelled, /// The run ended itself by policy — a stuck-loop escalation, the step /// cap, an enforced budget, a scope review the user ended. A deliberate diff --git a/website/content/docs/commands/daemon.mdx b/website/content/docs/commands/daemon.mdx index b75220994..1af60cb5c 100644 --- a/website/content/docs/commands/daemon.mdx +++ b/website/content/docs/commands/daemon.mdx @@ -193,8 +193,6 @@ It prints one line per supervised run, continued or skipped, with the reason: 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. -**A run parked on a scope review is skipped**, and the sweep says so and moves to the next one. A boot has no terminal and nobody to answer the review, so resuming it would park it again forever — and because the sweep is sequential, that would strand every run behind it with no indication why. Answer it with `stella daemon attach ` and the run continues from there. - **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: ```text