fix(stella-cli): resume a crash that recorded an error at boot (#1696) - #1939
Merged
Conversation
added 4 commits
August 5, 2026 20:04
…mentations 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.
…1653) #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
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<Drainable>`.
- `fleet_claims.rs` matches `cli.command` as a bare `Command` where it is an
`Option<Command>`, 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.
…ehind a parked one (#1696, #1698) 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-<millis>-<pid>` 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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
macanderson
enabled auto-merge (squash)
August 7, 2026 00:25
Contributor
Reviewer's GuideBoot-time resume logic is updated to (1) distinguish crashes from deliberate stops and continue crashed runs with resume points, (2) avoid re-parking runs stuck on scope reviews and instead skip them with an explicit NeedsInput reason, and (3) allow daemon commands to resolve runs by pid as well as id, with stronger status semantics and supporting tests/docs. Flow diagram for updated boot-time decide logic (resume vs skip)flowchart TD
A[BootCandidate] --> B{lock_held?}
B -- yes --> R1[Skip StillRunning]
B -- no --> C{stored_status is_live or Error?}
C -- no
--> R2[Skip EndedDeliberately]
C -- yes --> D{parked_on_approval?}
D -- yes --> R3[Skip NeedsInput]
D -- no --> E{has_resume_point?}
E -- no --> R4[Skip NoResumePoint]
E -- yes --> F{workspace_exists?}
F -- no --> R5[Skip WorkspaceGone]
F -- yes --> G{attempts < MAX_BOOT_ATTEMPTS?}
G -- no --> R6[Skip AttemptsExhausted]
G -- yes --> R7[Resume]
%% Key fields / values
subgraph BootCandidate
A1[stored_status]
A2[lock_held]
A3[parked_on_approval]
A4[has_resume_point]
A5[workspace_exists]
A6[attempts]
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Resolves the conflicts in favour of the implementations that already shipped on main where the two sides fixed the same issue independently: - #1653 (deliberate stop vs crash): main's SessionStatus::Stopped design and Result-shaped outcome_status/record_outcome_if_supervised (#1828, #1878) over this branch's Cancelled-collapsing Option shape. - #1690 (pid addressing): main's resolve() with the shared only() helper and its error wording (#1723). - #1616 drain residue: main's arm_panic_drain + pump_thread self-join guard; this branch's unwired drain_shared duplicate is dropped. - fleet claims parse test: main moved it to stella-cli/src/tests.rs; the copy this branch kept in fleet_claims.rs would have duplicated it.
… a recorded-error crash at boot (#1696) 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
macanderson
changed the title
macanderson
disabled auto-merge
August 7, 2026 00:57
This was referenced Aug 7, 2026
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…mon::boot (#1970) ## What & why `main`'s workspace doc gate is red: `crates/stella-cli/src/daemon/boot.rs`'s module doc intra-doc-links `SkipReason::NoResumePoint`, but `daemon::boot` is a **private** module and `SkipReason` is `pub(super)`, so rustdoc resolves nothing and `rustdoc::broken_intra_doc_links` fails under `-D warnings`. The link landed in #1939 and was invisible until now, which is the part worth recording: `cargo doc` bails on the first crate that fails, so `stella-store`'s private-link error (fixed in #1965) masked this one entirely. Fixing one rustdoc break surfaces the next one down — expect to iterate, not to assume the first fix was the last. The fix is the same shape as #1965's: name the item in prose rather than link something rustdoc cannot see, with a parenthetical saying why. ## The witness - [ ] No witness needed (docs) — because: this is a doc-comment-only change, and the gate itself is the check. `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps` exits **101** on `main` and **0** with this change, verified by exit code rather than by reading output. ## The gate - [x] `cargo doc --workspace --no-deps` under `-D warnings` (exit 0, the whole workspace — re-run after the fix to confirm nothing further was masked) - [x] `cargo fmt --check` - [x] CLA signed Refs #1939, #1965. ## Summary by Sourcery Documentation: - Clarify daemon boot module documentation by naming SkipReason::NoResumePoint in text instead of using an intra-doc link that rustdoc cannot resolve.
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…ead's tasks rows (#1708) (#1967) ## What A sub-session worker's closeout wrote **its own** private task board into the shared `tasks` table under the **lead's** `session_id` (`crates/stella-cli/src/subsession.rs`, `run_worker`). At the table's `UNIQUE(session_id, task_id)` key that write was pure corruption, twice over: 1. **Ordinal collision** — a worker board numbers from "1" in its own namespace, so its task "1" upserted over the lead's unrelated task "1". 2. **`/clear` seal bypass (#1692)** — the seal that quarantines pre-clear workers lives on the driver (`SubSessions::seal_task_board` → `session_clear::settle_worker_task`); this write ran on the worker's own thread where no seal can reach, so a pre-clear worker repopulated the mirror the user destroyed. ## The decision (of the three shapes in #1708) **Drop the write** — the shape #1708's investigation identified as the only one that fixes both defects alone: - The write's stated purpose ("so `tasks` queries see sub-agent boards too") is unachievable at this key — neither reader (`Store::list_session_tasks`, the Observatory's sessions query) can distinguish a worker row from a lead row. - The delegation outcome the session cares about is the **lead's** board row, which the driver already mirrors at both of its own write sites (the lead's turn end in `command_deck.rs`, worker settlement in `session_clear::settle_worker_task`). - Namespacing fixes only defect 1; routing through the driver fixes only defect 2. Dropping fixes both, with no migration and no cross-thread plumbing — and it makes the `/clear` seal airtight rather than advisory, because the driver becomes the session mirror's *sole* writer. ## The witness seam (the part #1708's investigation could not find) The worker's store closeout moves to a new sibling module, `crates/stella-cli/src/subsession/closeout.rs` — required anyway, since `subsession.rs` sits 24 lines under the file-size gate's 1500-line ceiling and could not absorb tests. `close_worker_execution` is the exact production path `run_worker` calls, and its signature takes **no session id**, so the closeout structurally cannot address session-keyed rows. Two witness tests drive that seam against a real in-memory store: - `a_workers_private_board_never_lands_in_the_sessions_tasks_rows` — the lead's mirror survives a worker closeout byte-identical, the table row **count** is pinned too (so a NULL-session append can't hide), and the execution row still closes (so "fixed by deleting the closeout" can't pass). - `a_pre_clear_workers_closeout_cannot_repopulate_a_cleared_mirror` — after `clear_session_tasks` (the persisted half of `/clear`), a worker closeout leaves the mirror exactly as empty as the user made it. **Fail→pass flip, demonstrated:** the seam is new, so `git stash` alone can't show the flip. Instead the pre-#1708 board mirror was temporarily grafted back into `close_worker_execution` (same write, keyed to the lead's session exactly as `run_worker` was) — both witnesses **fail** against the graft with the exact corruption #1708 describes ("the lead's mirror survives a worker closeout untouched" assertion trips), and **pass** with it removed. The graft was reverted before shipping. ## Verification Measured on this branch **with current `main` merged in** (head `73f99926`): - `cargo test -p stella-cli` — **1456 pass**, 0 fail, including both new witnesses - `cargo clippy -p stella-cli --all-targets -- -D warnings` — clean - `cargo fmt --check` — clean - `make guards-fast` (all 25 guards) — clean; `file-size` reports **"none grew"** against main's regenerated baseline ## CI is red for reasons outside this diff `fmt + clippy + test` fails on **five breaks inherited from `main`**, none in a file this PR touches (the diff is 3 files, all `stella-cli`): | Break | Crate | Already covered by | |---|---|---| | `unresolved link to SkipReason::NoResumePoint` (`daemon/boot.rs:59`, added by `a2806246` / #1939) | stella-cli | **#1970** | | `too many arguments (8/7)` (clippy) | stella-pipeline | **#1971** | | `PassingShell` not found | stella-pipeline | **#1971** | | `shell_call_result` not found | stella-pipeline | **#1971** | | non-exhaustive match: `ModelCallRole::Research` not covered | stella-pipeline | **#1971** | The last one is the root cause of three of these: `main` added a `Research` variant to `stella_protocol::ModelCallRole` without updating downstream exhaustive matches. This branch point has no `Research` variant at all, which is why the break is provably inherited rather than caused. No competing unbreak is included here deliberately — duplicate PRs racing the same seam is how `main` gets re-broken. This PR should go green once #1970 and #1971 land; it is already merged up to current `main` and conflict-free. No migration: the corrupted rows self-heal — the driver's next lead-board mirror upserts the colliding ordinals back to the lead's state, and `/clear` deletes the rest; stray rows beyond the lead's ordinal range in existing databases are dead rows in a table whose only production readers are per-session queries the driver now exclusively feeds. Exemplar for the shape: the pipeline's witness stage gives authored witnesses exactly this lifetime — scaffolding for one run, discarded with it — which is the model applied to a worker's private board here. ## Filed, not fixed - **#1968** — this PR deliberately closes the door on persisting sub-agent boards. If they are wanted, it takes a `lane` column + migration, routing through the driver, *and* a lane-aware reader, all three together; the issue scopes each and says why any one alone reintroduces a defect. - **#1969** — existing databases already hold corrupt rows from this bug, and they are **indistinguishable by construction** (that is #1708's whole complaint), so no migration can repair them. They render in the Observatory's Sessions tab. Needs a maintainer's call between accept / caveat / document. Refs #1692, #1631. Closes #1708
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…a dangling boot.rs doc link (#1985) ## What & why `main`'s required `fmt + clippy + test` job has been **red since #1951** (five consecutive merges), so every open PR inherits a red gate. Four distinct errors are involved. This PR fixes the two that **no open PR covers**. ### 1. `cargo clippy -D warnings` — `plan_stage` has 8 arguments (limit 7) `crates/stella-pipeline/src/pipeline.rs`. #1953 added a `research` parameter, pushing `plan_stage` over `clippy::too_many_arguments`. `Spend<'_>` (`crates/stella-pipeline/src/pipeline/stage_budget.rs`) already groups the `budget`/`total` pair, and **seven** sibling stage methods take it; `plan_stage` was the last one carrying the two loose. Adopting it takes the count to 7. This is the right fix rather than `#[allow(clippy::too_many_arguments)]` because there is no argument that the lint is wrong *here* — the grouping type the lint is asking for already exists — and because `pipeline.rs` is a grandfathered god file closed to growth, which this change **shrinks**. ### 2. `cargo doc -D warnings` — unresolved link in `boot.rs` `crates/stella-cli/src/daemon/boot.rs`. #1939 left ``[`SkipReason::NoResumePoint`]`` in the module doc unresolvable (`no item named `SkipReason` in scope`), which fails `rustdoc::broken_intra_doc_links`. Qualified to the full path — the same remedy #1927 applied to *this same file* for *this same reason* after #1920. That recurrence is filed as #1986. ## What this PR deliberately does NOT fix The other two errors belong to the open PR **#1964**: - `flip_halt_arming.rs` references `PassingShell` / `shell_call_result`, test helpers #1945 landed the test file without. - `crates/stella-pipeline/src/management_prompt/tests.rs`'s exhaustive match omits `ModelCallRole::Research`, which #1953 added. Duplicating them here is how two parallel unbreak PRs collide, so they are left to #1964. **`main` needs both PRs.** This one alone leaves `cargo test` red; #1964 alone leaves clippy and rustdoc red (verified: #1964's own CI still fails both, and its clippy failure is the identical `plan_stage` error). Neither is sufficient on its own, and the merge order does not matter. Because of that, **this PR's own `cargo clippy` and `cargo test` steps will stay red until #1964 merges** — clippy `--all-targets` reaches the missing test helpers once the lib error is gone. That is expected, not a regression. ## The witness - [x] No witness test needed — a lint fix and a doc-link fix, neither a behavior change. Verified the artisanal way: - `RUSTDOCFLAGS="-D warnings" cargo doc -p stella-cli --no-deps` fails on `main` and **exits 0** with this change. - `cargo check -p stella-pipeline --all-targets` reports **only** the three #1964-owned errors; the `plan_stage` arg-count error is gone and both call sites (`pipeline/scope_stage.rs`, `pipeline/tests/management_accounting.rs`) compile. - `cargo fmt --check -p stella-pipeline -p stella-cli` exits 0. ## The gate - [x] `check-file-size.sh` and `check-god-files.sh` pass; `pipeline.rs` shrinks, so no baseline change is needed. - [x] No behavior change, no new flags, no docs pages affected. - [x] No new dependencies. ## Nothing left behind - **#1986** — `boot.rs`'s module doc has now broken `main`'s rustdoc twice in two days by the same mechanism (#1920 → #1927, then #1939 → this PR), because `ci.yml` does not run on a push to `main`. Filed as a handoff. - **#1974** — `CandidateState` is hand-built at two sites, which is how #1951's field addition silently broke PR #1962 at merge time. Noticed in the same investigation. Refs #1953, #1939, #1964
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this delivers now
#1696 only. While this batch sat, main independently landed the rest of it:
SessionStatus::Stopped(fix(stella-cli): record a deliberate stop distinctly from a crash in the session registry #1828, fix(stella-cli): record a deliberate stop distinctly on the unsupervised writers too (#1826) #1878), a richer design than this branch'sCancelledcollapseonly()resolverarm_panic_drain+ thepump_threadself-join guard; this branch's unwireddrain_sharedduplicate is droppedThe merge takes main's shipped implementations for all of those, and rebuilds the one unshipped fix on top of them.
The remaining fix (#1696)
boot::decideused to skip every terminal status,Errorincluded — a compromise forced by #1653's ambiguity, at the honest cost of stranding a crash that lived just long enough to recordError. With #1653 landed, a policy stop recordsStopped, soErrormeans only "it fell over", and this lets it fall through to the resume-point check.Safe twice over, both checked rather than assumed:
Errorfor a policy stop is filtered byNoResumePointanyway;Errorthat resumes into anotherErrorafterMAX_BOOT_ATTEMPTS.Witness
a_crash_that_recorded_itself_is_continued_but_a_policy_stop_is_notfails on main (wheredecideskips every terminal status) and passes here. The exhaustive propertynothing_is_ever_continued_without_a_resume_pointis widened to pin the new rule: only live statuses andErrorare ever continued.Verified locally:
cargo test -p stella-clidaemon suite (75 passed),cargo clippy -p stella-cli --all-targets -- -D warningsclean.Closes #1696
Refs #1653, #1690, #1698 — shipped on main by #1878, #1723, #1920 respectively.