diff --git a/CHANGELOG.md b/CHANGELOG.md index dc2762c4a..107a2b36d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,20 @@ breaking changes may land in a minor release. mismatch mislabelled. The decision is unchanged — a byte-identical answer still activates whatever scope supplied it, every unconfirmed answer still degrades, and the repo-format flag is still rolled back. +- **A hard stop rides `stop-request.json` with `mode: "hard"` (#319).** It is lodged before the + engine is signalled — the atomic write also supersedes a pending graceful request — and honored + at item boundaries and mid-session, where both real adapter wait loops poll it twice per iteration, + which normally lands well inside the 10s grace window. An iteration blocked on a transport call + or waiting for an artifact can exceed it, and the stop then degrades to the force-kill backstop — + the pre-#319 outcome, never a worse one. SIGTERM is now the POSIX fast path rather than the mechanism, so a hard stop + lands on every platform and multiplexer backend, and reaches a nested auto-sweep through the + owning run's channel — hard-only; `stop ` is unchanged. A run directory that rejects + the write degrades to the signal path with the stop still delivered. +- **`status --json`'s `graceful_stop_pending` is now mode-exact (#319)** — it reports only + genuinely graceful requests. A modeless pre-#319 request body still reads graceful. +- **`stop --graceful` and the TUI report an already-pending request without calling it graceful + (#319).** The request standing on disk may be a hard one — `stop` leaves one lodged when it + could not prove the engine dead — and the idempotency answer is deliberately mode-blind. ### Removed @@ -46,6 +60,42 @@ breaking changes may land in a minor release. ### Fixed +- **Native Windows: `bmad-loop stop` no longer burns the full 10s grace window into a blind + `taskkill /F` (#319).** An inter-process SIGTERM is never delivered to a native-Windows engine, + so every stop completed through the external fallback with no engine teardown at all. The engine + honors the stop request itself now, so it is the single writer of `stopped` again and + `run-stop fallback=True` is no longer stamped on a stop the engine recorded itself — it marks + one this tool had to complete from outside. +- **`bmad-loop stop` no longer reports success when it delivered neither channel (#319).** A run + directory that rejects the request write, followed by a signal the OS refuses, left the CLI + saying the run had stopped — and stamping `run-stop fallback=True` — over an engine that may + still be running with nothing on disk to stop it. It now kills the agent session as a backstop, + records the undelivered attempt, and exits non-zero naming the retry. +- **Two concurrent `stop` invocations against one run no longer collide on a staging temp (#319).** + The write staged through a fixed `stop-request.json.tmp`, so the loser's rename raised + `FileNotFoundError`. It now stages under a per-writer name: the last write wins and neither + caller errors. +- **`resume` no longer re-arms a run whose stale stop request it could not remove (#319).** It read + "could not remove it" as "nothing was pending", wrote the pid, and stopped again at the first + item boundary with nothing printed to say why. Resume now refuses before the pid lands and names + the file, without re-stamping the host-exec integrity pin for a run it never started. + `stop --cancel-graceful` likewise stops reporting "no stop request pending" for a request still + on disk and still honorable — same exit code, accurate message. +- **A `stop` that never proved the engine dead keeps its request lodged (#319).** A `terminate` or + `force_kill` refused with `PermissionError`, or a `taskkill /F /T` that failed silently, + discarded the hard request while reporting the run stopped — throwing away the only channel left + to stop a live engine. Death is now distinguished from refusal, so the stop stays genuinely in + flight. +- **A hard stop arriving as the last item finishes stops the run instead of reporting it completed + (#319).** Covers `max-stories-reached` too; a graceful request at an exhausted queue still + finishes truthfully. +- **`stop --graceful` no longer downgrades a hard request that landed while it ran (#319).** The + graceful lodge is now an atomic `O_CREAT | O_EXCL` create that answers "already pending" for + anything already there, and a symlink planted at the path is refused rather than followed. Two + concurrent graceful asks resolve the same way, which is the documented idempotency. A write that + fails part-way leaves the request standing instead of rolling back — an unlink there resolves the + path, not the file the call created, so it could remove a hard request escalated onto it — and + `stop --graceful` reports it as possibly pending rather than as a clean failure. - **A policy field of the wrong TOML type now raises `PolicyError` naming `section.key` (#440).** `loads()` coerced with bare `int()`/`float()`/`bool()`/`str()` outside the `PolicyError` funnel, so a wrong-typed value escaped every handler written to degrade on diff --git a/README.md b/README.md index be4ea2273..f9af16dbd 100644 --- a/README.md +++ b/README.md @@ -592,19 +592,19 @@ Game-engine (Unity) runs read a wider `BMAD_LOOP_UNITY_*` / `BMAD_LOOP_ENGINE_*` ## Run state -Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `verify/` (verifier command stdout/stderr, pointed at by the journal's `verify-command-result` records; the retained tail is capped per stream by `[verify] stream_capture_kb`, `0` to keep nothing), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a graceful stop is pending — `stop-request.json` (the control file the engine consumes at the next item boundary). +Everything about a run lives in `.bmad-loop/runs//` (gitignored): `state.json` (resumable engine state), `journal.jsonl` (every decision), `tasks//` (per-session prompt + result + escalations, plus diagnostic breadcrumbs — `session-lifecycle.jsonl` records when a timeout fired, `heartbeat.json` is the wait loop's proof-of-life, `resultless-stops.jsonl` records give-up Stops), `logs/` (raw pane output, debugging only), `verify/` (verifier command stdout/stderr, pointed at by the journal's `verify-command-result` records; the retained tail is capped per stream by `[verify] stream_capture_kb`, `0` to keep nothing), `deferred/` (stashed specs from deferred stories), `resolve//` (escalation `context.json` + the resolve agent's `resolution.json`), `ATTENTION` (human-readable alerts), and — only while a stop request is pending — `stop-request.json` (the control file carrying the requested mode, consumed by the engine when it honors it). One piece deliberately lives elsewhere: the **hook-event channel** (the session completion signals the orchestrator waits on) sits under the user-scoped state root at `///events/`, outside the project tree — a branch switch, a worktree mount or a rollback must not be able to take a live run's control plane away. See `BMAD_LOOP_STATE_DIR` above for where that root resolves. The orchestrator also keeps polling the old in-tree `events/` location, so a project whose installed hook relay predates the move still completes its sessions; re-run `bmad-loop init` to refresh the relay. That out-of-tree directory is collected with the run: `delete`, `archive` and `clean` remove it alongside the run dir, and `clean` also sweeps this project's orphans there — control planes whose run dir is already gone, e.g. from a hand-removed run (`clean --dry-run` previews the count; `--json` reports it as `state_dirs_swept`). Two consequences worth knowing: an archived run's tarball no longer contains `events/` (transient completion signals, consumed while the run was live), and a project that is deleted, moved or renamed leaves its old subtree behind — the key is derived from the project's resolved path, so after a move the project itself now keys somewhere new and nothing can name the old key to sweep it. Remove it by hand if you care; it is events-sized, not run-sized. -A run can be stopped two ways. A **hard stop** (`bmad-loop stop`, TUI `x`, Ctrl+C) SIGTERMs the engine mid-item and always kills the agent session. A **graceful stop** (`bmad-loop stop --graceful`, TUI `S`) instead writes `stop-request.json` — no signal, so it works on every platform and multiplexer backend — which the engine consumes at the next item boundary: the in-flight story (or sweep bundle) finishes through commit — or, mid-triage, the sweep's triage session completes and no bundles start — the run finalizes as `stopped` (not `finished`) and stays resumable, and pending auto-sweeps are suppressed. Its session teardown follows `cleanup_session_on_finish` like a normal finish, rather than the hard stop's unconditional kill; a hard stop always supersedes a pending graceful request. +A run can be stopped two ways, and both requests travel over the same `stop-request.json` control file — no signal needed, so a stop works on every platform and multiplexer backend. A **hard stop** (`bmad-loop stop`, TUI `x`; Ctrl+C in the run's own terminal does the same thing directly) abandons the in-flight item and always kills the agent session: `stop` lodges the file with `mode: "hard"` _before_ it signals — the same atomic write supersedes any pending graceful request — and the engine honors it at the next item boundary, or mid-item, where each adapter's wait loop reads the file on both sides of the up-to-5s wait it blocks in — so a quiet session normally aborts within a few seconds, while one already waiting out its result grace, or blocked in a transport call, sees the request only when that call returns. SIGTERM still goes out as the POSIX fast path, but the file is what makes the stop land; the force-kill past the 10s grace window — and the `run-stop fallback=True` it stamps — now marks a stop this tool had to finish from outside, which a slow teardown reaches as readily as an engine that never read the request. A **graceful stop** (`bmad-loop stop --graceful`, TUI `S`) lodges the same file in its default `graceful` mode, which the engine consumes at the next item boundary only: the in-flight story (or sweep bundle) finishes through commit — or, mid-triage, the sweep's triage session completes and no bundles start — the run finalizes as `stopped` (not `finished`) and stays resumable, and pending auto-sweeps are suppressed. Its session teardown follows `cleanup_session_on_finish` like a normal finish, rather than the hard stop's unconditional kill. `journal.jsonl` records a `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared elapsed), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both`); `wall` alone fingerprints a host suspend (e.g. macOS sleep) that froze the monotonic clock. Every entry whose usage was read also carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the read failed and absent on an `aborted` end. `tokens_weighted` is the end-of-session total, distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. Per-session `tokens_weighted` sums to within a token or two of the run total, which rounds per story rather than per session. Token usage is read from each CLI's local session transcript (selected by the profile's `usage_parser`) and aggregated per story (`bmad-loop status`); the hookless `opencode` profile is the exception — its adapter pulls token usage from the OpenCode server over HTTP just before teardown (server state is sqlite; there is no local transcript). -Each run drives its agents inside a dedicated tmux session, `bmad-loop-`. It is torn down automatically when the run finishes (disable with `[adapter] cleanup_session_on_finish = false` to inspect agent windows afterwards), and `stop` always kills it. A paused or interrupted run keeps its session for `resume`, which clears any stale session and spins up a fresh one. Sessions left behind by older runs — or by a `cleanup_session_on_finish = false` policy — can be swept any time with `bmad-loop cleanup` (or `c` in the TUI). +Each run drives its agents inside a dedicated tmux session, `bmad-loop-`. It is torn down automatically when the run finishes (disable with `[adapter] cleanup_session_on_finish = false` to inspect agent windows afterwards); a hard `stop` kills it regardless of that setting, while a graceful `stop --graceful` tears it down under the same gate. A paused or interrupted run keeps its session for `resume`, which clears any stale session and spins up a fresh one. Sessions left behind by older runs — or by a `cleanup_session_on_finish = false` policy — can be swept any time with `bmad-loop cleanup` (or `c` in the TUI). ### Scripting `status` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index eb19afb63..c07536ef8 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -212,7 +212,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop adapters` — list registered coding-CLI adapter **kinds** (name · builtin/external · whether the family drives a multiplexer · which profiles select it), the CLI axis's counterpart to `mux`. Unlike `mux` there is no global choice to persist: a kind is selected per profile by its `adapter` field. A profile referencing an unregistered kind, and any out-of-tree adapter/profile package that failed to load, get a `warning:` on stderr; `validate` reports the same as `adapter.kind` / `adapter.external` / `adapter.external-profile`. - `bmad-loop run` — drive the dev → review → verify → commit loop. - `bmad-loop sweep` — triage + execute open deferred-work entries. -- `bmad-loop resume ` — continue a paused/interrupted run. +- `bmad-loop resume ` — continue a paused/interrupted run. A resume is fresh intent, so a stop request the prior run left behind is discarded first, in either mode — and if it cannot be removed, resume refuses and names the file rather than re-arming into a run that would stop again at its first item. - `bmad-loop resolve ` — resolve a CRITICAL escalation, then re-arm + resume (`--story`, `--no-interactive`, `--restore-patch ` for intent-gap patch-restore, `--resume`/`--no-resume`). - `bmad-loop decisions` — answer deferred-work decisions past sweeps left unanswered (`--list` to just show them). `--json` instead emits a stable machine-readable document (schema-versioned; per decision the id, question, context, recommendation and every option's key/label/effect/intent/resolution/bundle-name plus a derived `recommended` flag) per the [contract below](#machine-readable-output---json); it implies the listing and never prompts, and nothing pending yields a valid empty document. - `bmad-loop confirm ` — complete a story parked at `awaiting-operator` once you have carried out the external actions it owes: acknowledges each in turn (`--yes` skips the prompts), writes the spec's `## Operator Confirmation` audit section, advances spec and board to `done`, and commits the pair. `--list` shows every parked story and what it owes; `--reverify` re-runs your `[verify]` commands first and blocks on failure; re-running it on an interrupted confirmation finishes that confirmation. `--json` emits a stable machine-readable document per the [contract below](#machine-readable-output---json) — per parked story the key, actions, spec file, spec/board status, the parking run and the `commit` carrying the park (empty until the record is in a commit), plus derived `confirmable`/`resumable` flags, the `confirmation_recorded` reading behind the latter, and a human `drift` reason; it implies the listing and never prompts, and nothing parked yields a valid empty document. @@ -220,7 +220,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop status []` — run + sprint summary with per-story token totals, cost-weighted with the raw count alongside. `--json` instead emits a stable machine-readable document (schema-versioned; run state, snapshot `cache_read_weight`, per-story phase/attempt/review-cycle/tokens/commit/defer-reason, plus the additively-added run-level `adapters` — the dev/review/triage adapter the policy snapshot resolves to, `null` on a run predating adapter stamping — and per-story `adapters_used`, the adapter identity actually recorded per role) per the [contract below](#machine-readable-output---json) — the supported surface for scripts; the text output is best-effort. - `bmad-loop diagnose []` (`diag`) — emit a sanitized diagnostic dump of a run/sweep to hand maintainers (histograms, counts, env, file sizes — no code/spec/prompts/paths/PII); a stray pseudonymized identifier is auto-substituted with its alias (disclosed in the report), while PII/secret hits still refuse to emit; defaults to the latest run (`--all`, `--out`, `--max-journal-entries`). `--json` emits the same dump as a stable machine-readable document per the [contract below](#machine-readable-output---json) instead of the markdown report. - `bmad-loop attach []` — tmux-attach to a run's live agent session. -- `bmad-loop stop ` — stop a live run. The default is a **hard stop**: SIGTERM the engine mid-item and kill its agent session. `--graceful` instead requests a **graceful stop** — the engine finishes the in-flight item (a story through commit, a sweep bundle through commit, or an in-progress sweep triage — after which no bundles start), then finalizes cleanly and stops as a resumable `stopped` run, suppressing any pending auto-sweeps; `--cancel-graceful` withdraws a pending request. Delivery is a `stop-request.json` control file consumed at the next item boundary (no signal, so it works on every platform and multiplexer backend); a hard stop always supersedes a pending graceful one. The TUI surfaces the same pair: `x` hard-stops, `S` requests a graceful stop. +- `bmad-loop stop ` — stop a live run. The default is a **hard stop**: stop now, abandoning the in-flight item and killing the agent session. `--graceful` instead requests a **graceful stop** — the engine finishes the in-flight item (a story through commit, a sweep bundle through commit, or an in-progress sweep triage — after which no bundles start), then finalizes cleanly and stops as a resumable `stopped` run, suppressing any pending auto-sweeps; `--cancel-graceful` withdraws a pending request. Both modes ride the same `stop-request.json` control file, which carries the mode: a hard stop lodges `mode: "hard"` **before** it signals, and that atomic write is also what supersedes a pending graceful request. The engine honors a hard request at the next item boundary and mid-session, where each adapter's wait loop polls it twice per iteration — before and after the loop's own up-to-5s wait — so a quiet session normally lands the stop well inside the 10s grace window. That is the common case rather than a bound: an iteration blocked on a transport call, or waiting out `RESULT_GRACE_S` for an artifact, can exceed the window on either adapter before the next poll — an in-flight socket read or tmux call cannot be interrupted from the polling thread, so no placement of the check makes the interval unconditionally short. What the file does guarantee is reach: a hard stop lands on every platform and multiplexer backend, including one where an inter-process signal is never delivered at all (#319). A nested auto-sweep runs inside its parent but mints its own run dir, so it also polls the _owning_ run's channel: stopping the parent stops the child mid-session rather than leaving it to the force-kill backstop. The child's read of the parent channel is hard-only — a graceful stop already keeps a child sweep from starting, and lets one already in flight finish. Teardown is unbounded on top of that — the opencode HTTP adapter then asks the server to abort and to report usage, and a server that will not answer those leaves the stop to the force-kill backstop, exactly as it would have before #319. SIGTERM still goes out alongside it as the POSIX fast path, but it is no longer the mechanism; the engine stays the single writer of `stopped`, and the external force-kill + `run-stop fallback=True` past the grace window now marks a stop this tool had to finish from outside — a teardown that outran the window reaches it as readily as an engine that never read the request — where before #319 it marked every native-Windows stop. The TUI surfaces the same pair: `x` hard-stops, `S` requests a graceful stop. - `bmad-loop delete ` — delete a run directory and its out-of-tree control-plane dir (`--force` stops it first if live). - `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it, control-plane dir included (`--force` stops it first if live). The tarball holds the run dir, so it carries no `events/`. - Removal refuses while a matching agent session is live that the project cannot prove is another one's, even when the engine is dead: for an untagged session the run dir is the last ownership proof `cleanup` can read, so removing it would leak the session ([#419](https://github.com/bmad-code-org/bmad-loop/issues/419)). A session tagged to another project carries its own proof and never blocks. Run `cleanup` first, having confirmed the session is this project's (`attach`): for an _untagged_ session `cleanup` proves ownership by that same run dir, so two projects sharing a run id can prune each other's. Or pass `--force`, which removes anyway and kills nothing. `clean` leaves such a run untouched and reports it as protected. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index a7089f850..1b35a5b36 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -512,7 +512,7 @@ Three frozen dataclasses cross the seam: window id, HTTP session id, …), `launched_ns` (wall-clock ns just before launch; the floor for hook events). - **`SessionResult`** (returned by `wait_for_completion`) — `status` (one of - `completed`, `stalled`, `timeout`, `crashed`, `over_budget`), `result_json`, + `completed`, `stalled`, `timeout`, `crashed`, `over_budget`, `aborted`), `result_json`, `session_id`, `transcript_path`, and the optional post-mortem forensics `env_fault` / `env_fault_evidence` (set by `_classify_env_fault` when a non-completed session is matched as a transport/API **environment fault** — @@ -524,7 +524,19 @@ Required (abstract): - `start_session(spec) -> SessionHandle` — launch the session. - `wait_for_completion(handle, spec) -> SessionResult` — block until the session - ends (or stalls/times out), then report status. + ends (or stalls/times out), then report status. Poll + `runs.read_stop_request_mode(run_dir) == "hard"` on both sides of the loop's + own blocking wait and return `SessionResult(status="aborted")` when it is true + (#319): that is what makes `bmad-loop stop` land mid-session where a signal to + the engine cannot be delivered. Return the verdict — never raise, never unlink + the file (the engine consumes it and attributes the stop). Keep that wait + short — both bundled adapters cap theirs at 5s and inherit the poll from + `_ResultFileMixin` — but do not read it as a bound on the stop: a dispatch leg + that waits out an artifact grace or blocks on a transport call outlasts + `stop_run`'s 10s grace window on its own, and the force-kill backstop is what + catches that. Skipping it is + not fatal: the engine still honors the request at the next item boundary, which + is where an adapter without the poll leaves the operator waiting. The base class provides `run(spec)`, the template that chains `start_session` → `wait_for_completion` → `kill` (the kill runs in a `finally`). diff --git a/docs/porting-to-a-new-os.md b/docs/porting-to-a-new-os.md index fdf733c34..a1eef483f 100644 --- a/docs/porting-to-a-new-os.md +++ b/docs/porting-to-a-new-os.md @@ -230,7 +230,10 @@ register_process_host("windows", lambda platform: platform == "win32", WindowsPr - `terminate(pid)` — politely stop it (POSIX `SIGTERM` / Windows `taskkill`). Raise the `OSError` family (`ProcessLookupError` / `PermissionError`) so callers keep - their "already gone / not ours" handling. + their "already gone / not ours" handling. This is the polite fast path, not the + stop guarantee: `bmad-loop stop` also lodges a hard `stop-request.json` the engine + reads itself, so a port whose `terminate` cannot actually be delivered still stops + runs (#319). - `force_kill(pid)` — escalation when `terminate` is ignored (POSIX `SIGKILL` / Windows `taskkill /F /T`). Only ever called once identity is confirmed. - `is_alive(pid)` — read-only liveness probe, no signal sent. diff --git a/docs/setup-guide.md b/docs/setup-guide.md index 1b9765a7f..f30138f1f 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -50,7 +50,10 @@ of the README. not yet at the Linux/macOS/WSL support tier — the remaining native-Windows work (window hosting, attach/detach, Unity cache paths) is tracked in [the roadmap](ROADMAP.md#native-windows-multiplexer-backend); the port path is in - [Porting bmad-loop to a new OS](porting-to-a-new-os.md). Inside WSL, install with the + [Porting bmad-loop to a new OS](porting-to-a-new-os.md). Stopping a run is not part of + that gap: `bmad-loop stop` lodges its request in a control file the engine reads itself, + at item boundaries and mid-session, so it no longer depends on Windows signal delivery + (#319). Inside WSL, install with the **Linux** interpreter — a Windows-installed bmad-loop is reachable from the bash prompt and silently behaves as Windows ([why](multiplexer-backends.md#psmux-native-windows-experimental)). To check: diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 519f2a49f..87dab6fa3 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -139,7 +139,7 @@ One row per run dir under `.bmad-loop/runs/`, oldest first (run ids are `YYYYMMDD-HHMMSS-` and sort chronologically). Columns: `st` (status glyph, see below), `run` (the id), `type` (`story` or `sweep`), `note` (a colored pause-kind badge on a paused run — `plan` / `story` / `spec` / `epic` / -`gate` / `esc`, or `⏹ stop` when a running run has a graceful stop pending). +`gate` / `esc`, or `⏹ stop` when a running run has a stop request pending). When any run is paused awaiting a human the pane's title shows a global **`⚑ N need attention`** count. On first load the newest run is auto-selected; arrow keys or mouse select another. A run you just launched is @@ -221,7 +221,11 @@ situational banners: - `⏹ graceful stop pending — will stop after the current item` — a graceful stop was requested (`S`, or `bmad-loop stop --graceful`); the run finishes the in-flight story/bundle through commit (or, mid-sweep-triage, lets triage - complete and starts no bundles), then finalizes and stops (resumable). + complete and starts no bundles), then finalizes and stops (resumable). The + underlying read is the control file's presence, not its mode, so the same line + shows for as long as a **hard** stop's request sits on disk before the engine + honors it — usually seconds, longer if the session is blocked in a transport + call — and there the current item does not finish. - `✖ engine gone — run was interrupted · press e to resume` — the recorded engine pid is dead. - `⚑ decision needed: DW- — / press a to attach and answer` — @@ -336,7 +340,7 @@ Journal kinds are styled by substring, first match wins: | `R` | resolve a run paused at an escalation (interactive, then re-arm) | | `d` | answer deferred-work decisions past sweeps left unanswered (modal walk) | | `a` | attach to the selected run's live session or orchestrator window | -| `x` | stop the selected live run immediately (confirm modal) | +| `x` | stop the selected live run, abandoning the in-flight item (confirm modal) | | `S` | graceful stop: finish the in-flight item, then finalize & stop (confirm) | | `D` | delete the selected run's directory (confirm modal) | | `A` | archive the selected run to `.bmad-loop/archive` (confirm modal) | diff --git a/src/bmad_loop/adapters/base.py b/src/bmad_loop/adapters/base.py index eafa92948..7c1daef6e 100644 --- a/src/bmad_loop/adapters/base.py +++ b/src/bmad_loop/adapters/base.py @@ -117,7 +117,12 @@ class SessionHandle: @dataclass(frozen=True) class SessionResult: - status: str # "completed" | "stalled" | "timeout" | "crashed" | "over_budget" + # "aborted" is the in-session hard-stop verdict (#319): the wait loop saw a + # `mode: "hard"` stop-request.json and tore the session down. It is an abort, + # NEVER a completion — sessions complete only on hook Stop events or window + # death (AGENTS.md) — and it never escapes `Engine._run_session`, which + # unwinds it as a RunStopped before any SessionRecord is written. + status: str # "completed" | "stalled" | "timeout" | "crashed" | "over_budget" | "aborted" result_json: dict[str, Any] | None = None session_id: str | None = None transcript_path: str | None = None diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index 0ef0986a7..9512a1636 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/src/bmad_loop/adapters/generic.py @@ -176,11 +176,12 @@ class _ResultFileMixin: skill-written result dict and fold it into the session's final ``SessionResult``. Transport-agnostic — shared by the tmux adapters and any adapter whose skill writes ``tasks//result.json``; needs - only ``self.tasks_dir``.""" + only ``self.tasks_dir`` and ``self.run_dir``.""" - # Set by the concrete adapter's __init__; bare annotation (no runtime effect) - # tells the type checker the host attribute this mixin reads. + # Set by the concrete adapter's __init__; bare annotations (no runtime + # effect) tell the type checker the host attributes this mixin reads. tasks_dir: Path + run_dir: Path # Whether `_final` applies the #261 proof-of-work gate to its read-back. False # here, and that is not a conservative default — it is the correct answer for @@ -192,6 +193,38 @@ class _ResultFileMixin: # concurrent run — the one place a result can belong to somebody else. _READBACK_NEEDS_PROOF_OF_WORK = False + def _hard_stop_requested(self) -> bool: + """Has an operator lodged a *hard* stop request that this session must + honor (#319)? Either this run's own, or the owning run's. + + Polled twice per wait-loop iteration by both real adapters — on either + side of the loop's own blocking wait — so a + ``bmad-loop stop`` is honored mid-session on platforms where the + engine's SIGTERM path is unreachable. Read-only by contract: the + adapter never unlinks ``stop-request.json`` — the engine consumes it + when it raises, and must still see it to attribute the stop. A torn or + modeless read already leans ``"graceful"`` inside + ``read_stop_request_mode``, so this can never abort a session + spuriously. + + Both dirs are read because a nested auto-sweep is a first-class run *and* + somebody else's child: it mints its own id and appears in ``list``, so + ``stop `` must still reach it, while ``stop `` lodges + in a dir this adapter would otherwise never look at. The owner leg is + hard-only, like this whole predicate — a graceful request already + suppresses a child sweep from *starting*, and letting one already in flight + finish is exactly what graceful means.""" + if runs.read_stop_request_mode(self.run_dir) == "hard": + return True + owner = runs.owner_run_dir() + # `!=` is a cheap dedupe for the common top-level case, not a correctness + # dependency: two spellings of one dir cost a redundant read, same answer. + return ( + owner is not None + and owner != self.run_dir + and runs.read_stop_request_mode(owner) == "hard" + ) + def _result_json(self, handle: SessionHandle, spec: SessionSpec, *, wait: bool) -> dict | None: """Acquire this session's result dict. Base behavior: read the skill-written ``result.json`` (briefly awaiting it on the Stop event, @@ -630,6 +663,27 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi budget_weighted=budget_weighted, stop_seen=stop_seen, ) + # Hard-stop poll (#319), per-iteration and deliberately NOT inside + # the heartbeat throttle below: the loop's own wait is capped at 5s + # (`watcher.wait_for(..., timeout_s=min(remaining, 5.0))`), so a stop + # normally lands well inside `stop_run`'s 10s grace window, while riding + # the 30s HEARTBEAT_INTERVAL_S would be worse than the status quo. Read + # that as the common case, not a bound: an iteration that goes on to + # wait RESULT_GRACE_S for an artifact, or to block on a tmux call under + # TMUX_TIMEOUT_S, exceeds the grace window on its own. See the second + # poll after the wait below for how the interval is split, and why it + # still cannot be made unconditionally short. Return the verdict — never raise `RunStopped` here: that would + # skip `run()`'s finally-kill + `_post_kill_reconcile`. The file is + # left on disk for the engine to consume and attribute the stop. + if self._hard_stop_requested(): + self._note_lifecycle(handle.task_id, "stop-abort-fired") + return SessionResult( + status="aborted", + session_id=session_id, + transcript_path=transcript_path, + budget_weighted=budget_weighted, + stop_seen=stop_seen, + ) now = time.monotonic() if last_heartbeat is None or now - last_heartbeat >= HEARTBEAT_INTERVAL_S: last_heartbeat = now @@ -772,6 +826,27 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi timeout_s=min(remaining, 5.0), since_ns=handle.launched_ns, ) + # Second poll, and the reason there are two (#319). The arm at the top of + # the loop is separated from its next run by everything between: the 5s + # wait above, plus whichever dispatch leg the event selects — a + # `_window_alive` or `send_text` bounded only by TMUX_TIMEOUT_S (30s), or + # a `_result_json(wait=True)` that waits RESULT_GRACE_S (15s) for an + # artifact. The last of those alone outlasts `stop_run`'s 10s grace on a + # perfectly healthy box, with no transport fault anywhere. Polling here + # splits the iteration so at most one leg sits between two checks. It + # cannot make the interval unconditionally short — an in-flight + # subprocess is not interruptible from this thread — so a leg that does + # outlast the window still degrades to `stop_run`'s force-kill backstop: + # the pre-#319 outcome, never a worse one. + if self._hard_stop_requested(): + self._note_lifecycle(handle.task_id, "stop-abort-fired") + return SessionResult( + status="aborted", + session_id=session_id, + transcript_path=transcript_path, + budget_weighted=budget_weighted, + stop_seen=stop_seen, + ) if event is None: try: alive = self._window_alive(handle) @@ -1822,9 +1897,20 @@ def _post_kill_reconcile( cap-exhausted injected-workflow stall whose marker landed before the kill is rescued by the same trust model. ``over_budget`` joins the set (#158): an artifact the wrap-up nudge flushed at kill-time is honored - the same way.""" + the same way. + + ``aborted`` joins it too (#319): an operator's hard stop kills the + window mid-wait, so a Stop event that had already landed — or was one + tick away — is never read, leaving exactly the same evidence problem. + The same trust model settles it: a provably dead window plus a + self-consistent *successful* terminal plus proof-of-work means the + session did finish, and discarding that work would misreport what + happened rather than be cautious about it. The upgrade to + ``completed`` does NOT resume the run — the engine re-reads the + hard-stop file after saving the rescued session and stops there, so a + rescue records the finished work and still honors the stop.""" if ( - result.status not in ("stalled", "timeout", "over_budget") + result.status not in ("stalled", "timeout", "over_budget", "aborted") or result.result_json is not None ): return result diff --git a/src/bmad_loop/adapters/opencode_http.py b/src/bmad_loop/adapters/opencode_http.py index c4afe7f25..6f1b7ac36 100644 --- a/src/bmad_loop/adapters/opencode_http.py +++ b/src/bmad_loop/adapters/opencode_http.py @@ -1074,6 +1074,37 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi timeout_expired_clock=expired, budget_weighted=budget_weighted, ) + # Hard-stop poll (#319), per-iteration and deliberately NOT inside the + # heartbeat throttle below: the loop blocks up to `POLL_TICK_S` (5s) per + # tick, so *detection* normally lands well inside `stop_run`'s 10s + # grace window — the common case, not a bound: the dispatch legs below + # the wait are bounded only by the client's own timeouts, and the + # generic adapter is no better off (its `_await_result` waits + # RESULT_GRACE_S on a healthy box). Beyond detection, this arm then + # makes two + # HTTP round-trips against a server that may itself be wedged, and the + # client's 10s per-phase timeout applies to each. So the arm is NOT + # bounded by the grace window, by design: it gives the engine its best + # chance to tear itself down cleanly, and when the server will not answer + # it degrades to `stop_run`'s force-kill backstop — the same outcome + # every native-Windows stop had before #319, never a worse one. Don't + # "fix" this by trimming the timeouts: the same two calls serve the + # timeout arm, where the transcript is the whole diagnostic payload. + # + # Mirror the timeout arm exactly — without `_abort` the in-flight HTTP + # turn keeps running until teardown. Return the verdict; never raise + # `RunStopped` here, and never unlink the request file: the engine + # consumes it and attributes the stop. + if self._hard_stop_requested(): + self._note_lifecycle(handle.task_id, "stop-abort-fired") + self._abort(sess) + transcript = self._capture_usage(handle, sess) + return SessionResult( + status="aborted", + session_id=session_id, + transcript_path=transcript, + budget_weighted=budget_weighted, + ) now = time.monotonic() if last_heartbeat is None or now - last_heartbeat >= HEARTBEAT_INTERVAL_S: last_heartbeat = now @@ -1209,6 +1240,25 @@ def wait_for_completion(self, handle: SessionHandle, spec: SessionSpec) -> Sessi if event is not None: last_seen = time.monotonic() + # Second poll (#319) — see the arm at the top of the loop. What follows + # here is the dispatch: `_probe_completion`'s two GETs, which are NOT + # throttled (once a turn goes quiet past SILENCE_THRESHOLD_S they run on + # every tick), a `_session_status` GET, or `_result_json(wait=True)`'s + # RESULT_GRACE_S wait. Each is bounded only by the client's own timeouts, + # so a single iteration can outlast `stop_run`'s 10s grace. Polling here + # keeps at most one leg between two checks. It cannot bound an in-flight + # socket read, so when one does outlast the window the stop degrades to + # the force-kill backstop exactly as it did before #319. + if self._hard_stop_requested(): + self._note_lifecycle(handle.task_id, "stop-abort-fired") + self._abort(sess) + transcript = self._capture_usage(handle, sess) + return SessionResult( + status="aborted", + session_id=session_id, + transcript_path=transcript, + budget_weighted=budget_weighted, + ) if event == "error": # session.error may precede a retry, not a turn-end (status # "retry" exists); only a PROVABLY settled session reads as a diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 11a314a60..915595d15 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2310,6 +2310,48 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: # does — `_sweep_factory(..., new_digest)` — so the same reasoning applies. profiles = _launch_profiles(pol, project) new_digest = _trusted_config_digest(pol, project, profiles=profiles) + # Discard any stop request left over from a prior stopped run — either mode — so + # the re-armed engine does not consume it at the first item boundary and + # immediately re-stop. A resume is fresh user intent, which is what makes a + # request lodged against the previous one stale. + # + # Placed here, and not beside write_pid with the rest of the arming, because this + # branch RETURNS. `_require_base_skills` above used to be this function's last + # early exit — everything below it ran straight through — so a refusal sited + # further down leaves persistent side effects behind for a resume that never + # happened: the `run-resume` journal entry, and the re-stamped integrity pin. + # The pin is the one that bites. `write_trusted_config_digest` below writes the + # exact file the NEXT resume reads back as `pinned`, so re-baselining it on a + # refusal inverts the advisory: it fires on the attempt that stopped and goes + # silent on the attempt that actually armed an engine. The re-stamp's own + # justification — that the engine this process is about to arm re-reads the + # config from there — is false on a path that arms nothing. + # + # No earlier than here either: `_launch_profiles` and `_trusted_config_digest` + # above both raise SystemExit on a bad profile, and clearing ahead of them would + # destroy the operator's lodged request on a resume that then aborts. This window + # is the only one past every raise site and ahead of both writes — and it is + # still before write_pid, the constraint that governs correctness: the moment the + # pid lands the engine is "live" and a lingering request becomes honorable. + if runs.clear_graceful_stop(run_dir): + print( + f"run {run_dir.name}: discarded a stale stop request before resuming", + file=sys.stderr, + ) + elif runs.graceful_stop_requested(run_dir): + # The clear is never-raise by contract (five callers depend on that), so it + # answers False for "nothing was pending" and "could not remove it" alike. + # Re-read to tell them apart: a request that survived the clear would be + # consumed at the very first item boundary and re-stop the run, and because + # the print above never fired the operator would see no reason why — then + # resume again, to the same end. + print( + f"run {run_dir.name}: a stale stop request could not be discarded " + f"({runs.STOP_REQUEST_FILE} is not removable); resuming would stop again " + "at the first item. Remove it and retry.", + file=sys.stderr, + ) + return 1 # #461 point 4, human-present half. A resume IS a deliberate human choice, so # the on-disk config is re-blessed (new_digest is re-stamped below) and the run # proceeds — the auto-sweep child is the only path that refuses. But the issue's @@ -2408,15 +2450,6 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: # different problem with no fix at equal privilege — #571. state.trusted_config_digest = new_digest state.clear_pause() - # A resume is fresh user intent: discard any graceful-stop request left over from - # a prior stopped-gracefully run so the re-armed engine does not consume it at the - # first item boundary and immediately re-stop. Fire before write_pid — the moment - # the pid lands the engine is "live" and a lingering request becomes honorable. - if runs.clear_graceful_stop(run_dir): - print( - f"run {run_dir.name}: discarded a stale graceful-stop request before resuming", - file=sys.stderr, - ) runs.write_pid(run_dir) # Persist before the engine starts: status, the TUI and diagnose only ever # read state.json, and Engine._save() may not fire for minutes. write_pid @@ -3149,11 +3182,14 @@ def cmd_status(args: argparse.Namespace) -> int: state = load_state(run_dir) # A pending graceful stop is not in state.json (it's the control file + a live # engine), so derive it here and hand it to the builder / text branch. Order the - # `and` so the cheap file check gates the engine_liveness probe: skip it when the - # run is already concluded or no request is on disk. + # `and` so the cheap file read gates the engine_liveness probe: skip it when the + # run is already concluded or no request is on disk. The mode check is exact — + # a lodged `mode: hard` request is a stop in flight, not a *graceful* stop + # pending, and reporting it as one would promise an operator the current item + # still finishes. Absent and hard both read False here; only "graceful" is True. graceful_pending = ( not (state.finished or state.paused or state.stopped or state.crashed) - and runs.graceful_stop_requested(run_dir) + and runs.read_stop_request_mode(run_dir) == "graceful" and runs.engine_liveness(run_dir) != "dead" ) if args.json: @@ -3281,7 +3317,8 @@ def cmd_stop(args: argparse.Namespace) -> int: return _cmd_cancel_graceful(run_dir, args.run_id) if args.graceful: return _cmd_request_graceful(run_dir, args.run_id) - # Hard stop (unchanged): SIGTERM the engine, kill its agent window, mark stopped. + # Hard stop: lodge a `mode: "hard"` stop request, signal the engine (the POSIX + # fast path), and let it tear the run down; kill its agent window either way. try: stopped = runs.stop_run(run_dir) except (runs.StopRunError, ProcessHostError) as e: @@ -3295,11 +3332,28 @@ def cmd_stop(args: argparse.Namespace) -> int: def _cmd_cancel_graceful(run_dir: Path, run_id: str) -> int: - """`stop --cancel-graceful`: discard a pending request so the run keeps going.""" + """`stop --cancel-graceful`: discard a pending request so the run keeps going. + + Mode-neutral, like the clear it delegates to: the only hard request that can + still be on disk for a human to reach is one `stop_run` deliberately left + lodged after refusing to force-kill an unverifiable pid, and withdrawing that + is a legitimate thing to want. So the messages name a *stop request*, not a + graceful one (#319).""" if runs.clear_graceful_stop(run_dir): - print(f"run {run_id}: graceful stop request cancelled") + print(f"run {run_id}: stop request cancelled") return 0 - print(f"run {run_id} has no graceful stop pending", file=sys.stderr) + if runs.graceful_stop_requested(run_dir): + # The clear answers False for "nothing pending" and "could not remove it" + # alike; re-read so we never tell an operator their request is gone while it + # is still on disk and still honorable. Exit 1 either way — only the message + # differs, so no caller's exit-code expectation moves. + print( + f"run {run_id}: stop request could not be cancelled " + f"({runs.STOP_REQUEST_FILE} is not removable) — it is still pending", + file=sys.stderr, + ) + return 1 + print(f"run {run_id} has no stop request pending", file=sys.stderr) return 1 @@ -3312,8 +3366,28 @@ def _cmd_request_graceful(run_dir: Path, run_id: str) -> int: except runs.GracefulStopError as e: print(str(e), file=sys.stderr) return 1 + except OSError as e: + # The lodge creates the file first and writes the body into it, and it + # deliberately does not roll back a failed write (see _create_stop_request: + # an unlink there resolves the *name*, so it could delete a hard request a + # concurrent `stop` escalated onto it). So a write that failed part-way + # still leaves a request standing, and a short body reads as graceful — + # the mode we were asked for. Say so rather than reporting a clean failure + # the operator would act on by asking again (#319). + print( + f"run {run_id}: stop request could not be written ({e}) — a graceful " + f"request may still be pending; check `bmad-loop status {run_id}` and " + f"use `bmad-loop stop {run_id} --cancel-graceful` to withdraw it", + file=sys.stderr, + ) + return 1 if outcome == "already-pending": - print(f"run {run_id} already has a graceful stop pending") + # Mode-neutral for the same reason `--cancel-graceful` is: the pending + # request may be a *hard* one (a `stop` that could not prove the engine + # dead leaves it lodged at rest), and the token is deliberately mode-blind. + # Naming it "graceful" would report a strictly stronger stop as a weaker + # one (#319). + print(f"run {run_id} already has a stop request pending") return 0 if outcome == "requested-unverifiable": print( @@ -4265,7 +4339,7 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: "--graceful", action="store_true", help="finish the in-flight item (through commit), then stop cleanly and stay " - "resumable — instead of the hard SIGTERM stop; also suppresses pending auto-sweeps", + "resumable — instead of the default hard stop; also suppresses pending auto-sweeps", ) stop_grp.add_argument( "--cancel-graceful", diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index ce84961f7..c5991b998 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -243,9 +243,11 @@ def status_document(state: RunState, *, graceful_stop_pending: bool = False) -> derived from state.json alone — never from live policy or other project files — so a consumer can reproduce the document, and the weight matches what the run actually enforced (see run_token_totals). The one exception is - ``graceful_stop_pending``: liveness plus the presence of the control file is - not in state.json, so the caller supplies it (default False keeps the - builder a pure projection); ``status`` itself is unaffected. + ``graceful_stop_pending``: liveness plus a *graceful*-mode stop-request + control file is not in state.json, so the caller supplies it (default False + keeps the builder a pure projection); ``status`` itself is unaffected. The + mode read is exact — a lodged hard request (#319) is a stop in flight, not a + graceful stop pending, and reports False here. Two adapter-identity keys (#153 phase 3), both derived from the snapshot and the recorded sessions — never live policy — and deliberately named apart: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index d22b4cacc..19e63e943 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -58,7 +58,17 @@ from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .recovery_flow import RecoveryFlow -from .runs import clear_graceful_stop, events_dir_for, graceful_stop_requested, kill_session +from .runs import ( + clear_graceful_stop, + consume_stop_request, + events_dir_for, + graceful_stop_requested, + kill_session, + owner_run_dir, + read_stop_request_mode, + reset_owner_run_dir, + set_owner_run_dir, +) from .sprintstatus import ACTIONABLE_STATUSES, STATUS_ORDER, SprintStatusError from .sprintstatus import advance as sprint_advance from .sprintstatus import advanced_bytes as sprint_advanced_bytes @@ -160,19 +170,28 @@ class RunStopped(Exception): Two flavors, distinguished by ``graceful``: - - ``graceful=False`` (default) — a *hard* stop from the SIGTERM/SIGINT handler. - The loop is interrupted mid-session, so the in-flight agent window is still - live and must be torn down unconditionally. + - ``graceful=False`` (default) — a *hard* stop: the SIGTERM/SIGINT handler, or + a ``mode: "hard"`` stop request the engine honored at an item boundary + (:meth:`Engine._check_stop_request`) or on either side of a session + (:meth:`Engine._run_session`). The loop may have been interrupted + mid-session, so the in-flight agent window can still be live and must be + torn down unconditionally. - ``graceful=True`` — a stop requested via the ``stop-request.json`` control - file and detected at an item boundary (:meth:`Engine._check_graceful_stop`). - The in-flight item already completed through commit, so ``run()`` runs the - wanted subset of the clean-finish path (worktree GC + ``post_run`` + - policy-gated session teardown) rather than a hard kill, and the run stays - resumable.""" - - def __init__(self, graceful: bool = False): + file in its default ``graceful`` mode and detected at an item boundary + (:meth:`Engine._check_stop_request`). The in-flight item already completed + through commit, so ``run()`` runs the wanted subset of the clean-finish path + (worktree GC + ``post_run`` + policy-gated session teardown) rather than a + hard kill, and the run stays resumable. + + ``via`` names the channel a hard stop arrived on — ``"stop-request"`` for the + control file, ``None`` for a signal — and rides the ``run-stop`` journal entry. + It is the only evidence that separates the two on a native-Windows run, where + the signal path cannot fire at all (#319).""" + + def __init__(self, graceful: bool = False, via: str | None = None): super().__init__("graceful stop" if graceful else "stopped") self.graceful = graceful + self.via = via class SweepFactory(Protocol): @@ -598,9 +617,19 @@ def run(self) -> RunSummary: depth = _run_depth.get() self._is_nested = depth > 0 token = _run_depth.set(depth + 1) + # Publish this run dir as the owner for everything below, so a nested + # auto-sweep's adapters poll the file an operator can actually write to + # (#319): `stop ` lodges here, while the child's own dir stays + # empty. Gated on depth, not on `_owns_signals` — a top-level run off the + # main thread installs no handlers yet still owns the channel. Reset by + # token in the same finally, ahead of the depth, so the nested re-raise arms + # unwind through both. + owner_token = None if self._is_nested else set_owner_run_dir(self.run_dir) try: return self._run_inner() finally: + if owner_token is not None: + reset_owner_run_dir(owner_token) _run_depth.reset(token) def _run_inner(self) -> RunSummary: @@ -622,6 +651,23 @@ def _run_inner(self) -> RunSummary: self._prune_preserve_refs() self._replay_unlatched_ledger_carries() self._loop() + # A hard request that landed after `_loop`'s head check reaches none + # of the raise sites on an exhausted-queue return: sites A and B live + # inside `_run_session`, which the `story is None` branch never + # enters, and the run-end auto-sweep predicate is mode-blind, so it + # *suppresses* and returns rather than raising. Without this the run + # would record `finished` — which `documents.py` ranks above + # `stopped` — while the operator's hard stop went unhonored, and + # `stop_run`'s fallback would then journal `fallback=True` against a + # perfectly responsive engine, contradicting what that flag now means. + # Covering it here rather than at the suppression site closes every + # `_loop` return path at once (including `max-stories-reached`) and + # keeps the per-epic sweep caller untouched. Mode-exact on purpose: a + # *graceful* request at an exhausted queue finishes truthfully, which + # is long-documented, separately tested behavior this must not disturb. + if read_stop_request_mode(self.run_dir) == "hard": + clear_graceful_stop(self.run_dir) + raise RunStopped(via="stop-request") self.state.finished = True self._gc_run_worktrees() self._emit("post_run") @@ -645,7 +691,7 @@ def _run_inner(self) -> RunSummary: except RunStopped as stop: if stop.graceful: # Graceful stop: the request was consumed at an item boundary - # (_check_graceful_stop), so the in-flight item already ran to + # (_check_stop_request), so the in-flight item already ran to # completion through commit — nothing mid-session to kill. Run # the wanted subset of the clean-finish path so a resumable # `stopped` run is finalized as tidily as a finished one. @@ -671,14 +717,30 @@ def _run_inner(self) -> RunSummary: if self._owns_signals and self.policy.adapter.cleanup_session_on_finish: kill_session(self.state.run_id) else: - # Hard stop: the loop was interrupted inside adapter.run(), so - # the agent window is still live — tear the whole run session - # down. + # Hard stop: the loop was interrupted inside adapter.run() (a + # signal), or unwound on either side of it because a hard stop + # request was honored — so the agent window may still be live. + # Tear the whole run session down. kill_session(self.state.run_id) if self._is_nested: raise # nested auto-sweep: let the owner record the stop self.state.stopped = True - self.journal.append("run-stop") + # The signal path consumes nothing on its way here, and `stop_run` + # now lodges a hard request *before* it signals — so on POSIX the + # file is still on disk for every routine stop. `run()`'s finally + # would then discard it as *stale* and journal + # `stop-request-discarded`, misreporting the very request this + # stop delivers. Consume it here, on the same rule the boundary and + # in-session sites already follow. Mode-exact: a pending *graceful* + # request really is superseded by a hard stop, so it is left for + # the finally to discard and journal, as it always has been. + if read_stop_request_mode(self.run_dir) == "hard": + clear_graceful_stop(self.run_dir) + # `via` rides only when the control file delivered the stop; + # the signal path keeps journaling a bare `run-stop` (precedent: + # the KeyboardInterrupt arm's `reason=` extra below). + extras = {"via": stop.via} if stop.via is not None else {} + self.journal.append("run-stop", **extras) except KeyboardInterrupt: # Some Windows console/control events can still surface as a raw # KeyboardInterrupt without routing through the installed signal @@ -737,11 +799,16 @@ def _run_inner(self) -> RunSummary: ): # nosec B110 - journal write is best-effort; crash.txt + state flag already persisted pass finally: - # Any pending stop-request control file that outlived this run - # (the run finished/paused/crashed, or a hard stop superseded it, - # before an item boundary consumed it) is discarded here so a later - # resume does not re-honor a stale request. The graceful arm already - # consumed its own file, so this only fires for a superseded one. + # Any pending stop-request control file that outlived this run is + # discarded here so a later resume does not re-honor a stale request. + # Every arm that *honors* a request consumes its own file first — the + # boundary and in-session sites, and the hard arm above, which has to + # because `stop_run` lodges before it signals and the signal path + # reads nothing. So this fires only for a request no arm honored: the + # run finished, paused or crashed with one pending, or a hard stop + # superseded a *graceful* one. Journaling those as discarded is + # accurate; journaling a request that just stopped the run would not + # be, which is the whole reason the honoring arms consume. if clear_graceful_stop(self.run_dir): with contextlib.suppress(Exception): self.journal.append("stop-request-discarded") @@ -931,21 +998,38 @@ def _remaining_estimate(self) -> int | None: except Exception: # a hint must never break the stop return None - def _check_graceful_stop(self) -> None: - """Honor a pending graceful-stop request at an item boundary. + def _check_stop_request(self) -> None: + """Honor a pending stop request at an item boundary, in the mode it asks for. Consumes (deletes) the ``stop-request.json`` control file and raises - :class:`RunStopped` with ``graceful=True`` so ``run()`` unwinds into the - clean-finalization arm. An exception, not a sentinel return, because the - sweep check fires two frames below ``_loop`` (inside ``_cycle``) where a - return could not stop the loop. Called as the first statement of the loop - body (and, in the sweep engine, before each bundle): by the time control - reaches here the in-flight item has already completed through commit, so - the stop takes effect cleanly at the next boundary and the run stays - resumable.""" - if graceful_stop_requested(self.run_dir): - clear_graceful_stop(self.run_dir) - raise RunStopped(graceful=True) + :class:`RunStopped` — ``graceful=True`` for a ``graceful`` request (the + default mode, and every pre-#319 modeless body, which + :func:`runs.read_stop_request_mode` deliberately reads as graceful) so + ``run()`` unwinds into the clean-finalization arm; ``via="stop-request"`` + for a ``hard`` one so it takes the hard arm instead. An exception, not a + sentinel return, because the sweep check fires two frames below ``_loop`` + (inside ``_cycle``) where a return could not stop the loop. Called as the + first statement of the loop body (and, in the sweep engine, before each + bundle): by the time control reaches here the in-flight item has already + completed through commit, so the stop takes effect cleanly at the next + boundary and the run stays resumable. + + A *hard* request that reaches a boundary is honored right here rather than + deferred to the adapter's in-session poll — aborting at the boundary is + both faster and cleaner than launching the next session only to abort it + mid-flight.""" + # One atomic take, never a read then an unlink: a `stop` escalating to + # "hard" between the two would be deleted unread while this engine routed + # on the stale graceful mode it already held. Consuming on BOTH arms is + # still required — `run()`'s finally discards any surviving file as *stale* + # and journals `stop-request-discarded`, which would misreport a request + # this engine just honored. + mode = consume_stop_request(self.run_dir) + if mode is None: + return + if mode == "hard": + raise RunStopped(via="stop-request") + raise RunStopped(graceful=True) def _loop(self) -> None: self._finish_inflight() @@ -954,7 +1038,7 @@ def _loop(self) -> None: # boundary this base loop reaches — between stories, right after # _finish_inflight on resume, and the epic boundary + run-end (the # StoriesEngine has no _loop override, so it is covered here too). - self._check_graceful_stop() + self._check_stop_request() if self.max_stories is not None and self._dispatched_count() >= self.max_stories: self.journal.append("max-stories-reached", count=self._dispatched_count()) return @@ -4919,6 +5003,20 @@ def _run_session( task.ledger_changed_before_harvest = ( self._ledger_digest() != task.baseline_ledger_digest ) + # A hard stop honored *inside* the session: the adapter's wait loop + # saw a `mode: "hard"` stop-request.json, tore its window down and + # returned this abort verdict. Position is load-bearing at both ends. + # Inside the `try`, so the `finally` below journals the paired + # session-end with status="aborted" — the same literal the exception + # path writes there. Before `record_session`, so NO SessionRecord is + # written: an abort is not a session outcome, and this matches the + # signal-path hard stop, which interrupts inside `adapter.run()` and + # records nothing either. "aborted" therefore never escapes this + # method — no downstream status set (env-fault, retry, escalation) + # needs to learn it. + if result.status == "aborted": + clear_graceful_stop(self.run_dir) + raise RunStopped(via="stop-request") task.record_session( SessionRecord( task_id=task_id, @@ -4994,6 +5092,33 @@ def _run_session( pass self._save() self._note_story_token_budget(task) + # A hard stop request that raise site A could not see as an abort: it + # landed in the gap after the wait loop's last poll, or the session DID + # abort and `_post_kill_reconcile` rescued it back to `completed` (the + # abort tore the window down before a landed Stop event was read). This + # check fires regardless of status, and that rescue is exactly why: + # without it a hard-stopped run would silently carry on into verify / + # review / retry on the strength of a rescued result. The session is fully + # recorded, saved and accounted for first, leaving the run byte-equivalent + # to the replayable host-death-after-save state documented above — a + # resume picks up from a complete session record, not a torn one. + if read_stop_request_mode(self.run_dir) == "hard": + clear_graceful_stop(self.run_dir) + raise RunStopped(via="stop-request") + # The same check against the *owning* run, for a nested auto-sweep child + # whose own dir is empty because the operator stopped the parent. Without + # it the fix above is inert on exactly the shape it exists for: the child's + # adapter aborts off the parent's file, `_post_kill_reconcile` rescues that + # `aborted` back to `completed`, so raise site A never fires — and the child + # would carry on into verify/review on the strength of the rescued result. + # Deliberately does NOT consume: the file is the parent's, and the parent's + # own hard arm must still find it to record and attribute the stop. The + # nested re-raise below hands this exception up before that arm consumes + # anything, and `via` rides the exception rather than the file. + if self._is_nested: + owner = owner_run_dir() + if owner is not None and read_stop_request_mode(owner) == "hard": + raise RunStopped(via="stop-request") self._emit( "post_session", task, diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index f34a963ed..3d3e04f9e 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -3,6 +3,7 @@ from __future__ import annotations import contextlib +import contextvars import hashlib import json import math @@ -36,11 +37,21 @@ RUNS_DIR = Path(".bmad-loop") / "runs" ARCHIVE_DIR = Path(".bmad-loop") / "archive" PID_FILE = "engine.pid" -# Cross-process channel for a graceful-stop request: a control file the requester -# (CLI/TUI) writes and the engine polls at item boundaries. Distinct from the hard -# SIGTERM stop_run delivers — there is no SIGUSR1 on Windows/psmux and SIGTERM -# already means "hard stop". The engine stays the single writer of journal.jsonl; -# requesters only ever touch this file. +# Cross-process channel for a stop request: a control file the requester (CLI/TUI) +# writes and the engine reads. The body carries a `mode` — "graceful" or "hard". +# +# graceful (`stop --graceful`): finish the in-flight item, then finalize and stop. +# Honored at item boundaries only; resumable. +# hard (`stop`): stop now. Lodged by stop_run *before* it signals, honored by the +# engine at item boundaries and mid-session by the adapter wait loop. +# +# The file exists because signals are not a portable stop channel: there is no +# SIGUSR1 on Windows/psmux, and an inter-process SIGTERM is never delivered to a +# native-Windows engine at all, so the win32 "graceful" terminate is a no-op that +# only ever burned _STOP_WAIT_S into a force-kill (#319). SIGTERM remains the POSIX +# fast path — the file is what makes a stop work everywhere else. The engine stays +# the single writer of journal.jsonl, and the single *consumer* of this file; +# requesters only ever write it, adapters only ever read it. STOP_REQUEST_FILE = "stop-request.json" # The host-exec config baseline's name inside a run's state dir (see # `config_digest_path_for`). A bare hex digest, not JSON: one opaque token, and a @@ -55,9 +66,10 @@ class StopRunError(Exception): - """A live run could not be stopped — the engine ignored SIGTERM and its pid's - identity can no longer be verified, so force-killing would risk an unrelated - (reused) pid. The caller surfaces this rather than silently marking stopped.""" + """A live run could not be stopped — the engine honored neither channel (the + lodged stop request nor SIGTERM) and its pid's identity can no longer be + verified, so force-killing would risk an unrelated (reused) pid. The caller + surfaces this rather than silently marking stopped.""" class GracefulStopError(Exception): @@ -76,6 +88,13 @@ class LiveSessionError(Exception): # marking the run stopped itself. _STOP_WAIT_S = 10.0 _STOP_POLL_S = 0.1 +# How long stop_run lets a force-kill settle before deciding it failed. A kill that +# returns cleanly is not proof of death — win32 shells `taskkill /F /T` with +# `check=False`, so a refused kill raises nothing — but the pid can also linger for a +# moment after a delivered SIGKILL, and `is_alive` is a bare existence probe that +# reads a not-yet-reaped process as alive. Long enough to outlast that, short enough +# that a genuinely surviving engine is still noticed while the operator waits. +_KILL_CONFIRM_S = 0.5 def new_run_id() -> str: @@ -910,19 +929,184 @@ def prune_sessions( return prunable, live, unknown +# The run dir of the OUTERMOST engine in this call stack (#319). A nested auto-sweep +# runs synchronously in its parent's thread but mints its own run id and dir, so its +# adapters would poll a control file no operator ever writes to: `bmad-loop stop +# ` lodges in the parent's dir. This carries the owning run dir down to +# them. A ContextVar, mirroring `engine._run_depth`, because the nesting it tracks is +# same-thread by construction; set once by the outermost `Engine.run()` and reset by +# token, so a later top-level run in the same process is never poisoned. +_owner_run_dir: contextvars.ContextVar[Path | None] = contextvars.ContextVar( + "bmad_loop_owner_run_dir", default=None +) + + +def set_owner_run_dir(run_dir: Path) -> contextvars.Token[Path | None]: + """Claim ``run_dir`` as the owning run for this call stack. Returns the token the + caller must hand to :func:`reset_owner_run_dir` from a ``finally``.""" + return _owner_run_dir.set(run_dir) + + +def reset_owner_run_dir(token: contextvars.Token[Path | None]) -> None: + """Release the claim made by :func:`set_owner_run_dir`.""" + _owner_run_dir.reset(token) + + +def owner_run_dir() -> Path | None: + """The outermost engine's run dir, or None outside any run — which is what a + standalone adapter (tests, probes) reads, so callers fall back to their own.""" + return _owner_run_dir.get() + + def graceful_stop_requested(run_dir: Path) -> bool: - """True when a graceful-stop request is pending for this run (its control file - is present). The single definition of "requested" the engine checks at item - boundaries and the CLI/TUI surface — a bare existence read, never raising.""" + """True when *some* stop request is pending for this run — either mode. A bare + existence read of the control file, never raising and deliberately never parsing. + + Every consumer wants exactly that existence question, not the mode: the + ``stopping`` projection and the TUI badge (a run with a hard request lodged is + stopping too), the ``--graceful`` idempotency check (a lodged hard request means + a *stronger* stop already stands — "already-pending" is the right answer), the + stories done-checkpoint skip, and auto-sweep suppression. Only ``status``'s + ``graceful_stop_pending`` field is mode-exact; it calls + :func:`read_stop_request_mode` instead.""" return (run_dir / STOP_REQUEST_FILE).is_file() +def read_stop_request_mode(run_dir: Path) -> str | None: + """The mode of this run's pending stop request: ``"hard"``, ``"graceful"``, or + ``None`` when none is pending. + + ``None`` means *absent*, and only absent — it is returned for + ``FileNotFoundError`` alone. Everything else about a file that is *present* + reads ``"graceful"``: a modeless body (every pre-#319 writer and test fixture + wrote one — this is the back-compat pin), unparseable or non-object JSON, and a + transient read failure such as the win32 sharing violation a concurrent + ``atomic_replace`` raises mid-write. + + Leaning graceful on every ambiguity is load-bearing, not defensive habit. A + misread graceful costs at most one more item before the run stops; a spurious + ``"hard"`` would abort a live session — so a torn read must never be able to + produce one.""" + return _stop_request_mode_of(run_dir / STOP_REQUEST_FILE) + + +def _stop_request_mode_of(path: Path) -> str | None: + """The parse half of :func:`read_stop_request_mode`, split out so + :func:`consume_stop_request` can answer for the file it *took* rather than for + whatever currently answers to the channel name.""" + try: + raw = path.read_text(encoding="utf-8") + except FileNotFoundError: + return None + except (OSError, ValueError): + # present but unreadable this tick (sharing violation, undecodable bytes) — + # answer for the file we know is there, never escalate on a failed read. + return "graceful" + try: + body = json.loads(raw) + except ValueError: + return "graceful" + if isinstance(body, dict) and body.get("mode") == "hard": + return "hard" + return "graceful" + + +def _write_stop_request(run_dir: Path, mode: str) -> None: + """Lodge a stop request of ``mode`` on the control-file channel, written + atomically so a concurrent engine read never sees a partial body. + + The atomic replace *is* the supersede: writing ``"hard"`` over a pending + ``"graceful"`` escalates the request in one step, with no window in which + nothing is pending for the engine to find. + + That is the only direction this function arbitrates, and the only one it may: + ``stop_run`` shares it and its escalation must stay unconditional. The channel is + otherwise last-writer-wins, so the *reverse* — a graceful write landing on a + pending hard request and downgrading it — is refused by a different writer + entirely: :func:`_create_stop_request`, which lodges the graceful mode with + ``O_CREAT | O_EXCL`` so "is one pending?" and "lodge mine" are a single atomic + step. Splitting the two directions across two functions is what lets this one + stay an unconditional replace. + + Goes through :func:`platform_util.atomic_write_text` rather than a hand-rolled + ``tmp + atomic_replace``, for the reason ``operatoractions`` was migrated under + #379: this is the one control file with genuinely *concurrent* writers — two + ``stop`` invocations against the same run, in either mode — and a fixed ``.tmp`` + sibling is exactly what two writers of the same key collide on. Interleaved, + both stage over one name and the loser's ``os.replace`` raises + ``FileNotFoundError`` after the winner's consumed it; on the hard path that + would abort ``stop_run`` *before* it ever signals. A ``mkstemp`` temp per writer + removes the collision: the last replace wins and neither writer errors. + + ``follow_symlinks=False`` preserves what the bare ``os.replace`` did — it never + dereferenced this destination — and matches what the file is: machine-minted + control state under a run dir a driven session can reach. It now lands at + ``mkstemp``'s ``0600`` instead of ``0644 & ~umask``; nothing reads it + cross-user.""" + body = json.dumps({"requested_at": time.strftime("%Y-%m-%dT%H:%M:%S"), "mode": mode}) + atomic_write_text(run_dir / STOP_REQUEST_FILE, body, follow_symlinks=False) + + +def _create_stop_request(run_dir: Path) -> bool: + """Lodge a *graceful* request only if none is pending; False when one already is. + + ``O_CREAT | O_EXCL`` is the arbitration. It makes "is a request pending?" and + "lodge mine" one atomic step against the destination name, so a hard request + landing at any instant either already exists — we refuse, leaving it standing — + or replaces what we wrote, which is escalation, the direction + :func:`_write_stop_request` owns. A re-read immediately before an unconditional + replace could only ever *narrow* that window (~1.3ms on a journalling + filesystem, where the fsync dominates); this closes it. + + Graceful-ONLY by construction, and that is what makes the non-atomic body safe. + The bytes are written *into* the created file rather than replaced in, so a + concurrent reader can catch it empty — and :func:`read_stop_request_mode` + answers ``"graceful"`` for a present-but-unparseable body, which is the very + mode being written. The invariant that matters is untouched: a torn read must + never produce ``"hard"``, so a hard writer must keep the atomic replace. + + Refuses a planted symlink rather than following it — ``O_EXCL`` never + dereferences — which is stricter than the ``follow_symlinks=False`` replace it + replaces. + + A failed write is deliberately NOT rolled back, and that is load-bearing rather + than sloppy. ``unlink`` resolves a *name*, not the inode this call created, so a + rollback here would delete whatever occupies the path at that moment — including + a ``"hard"`` request a concurrent ``stop`` escalated onto it while this write was + in flight. That is a ``hard -> absent`` drop, the one descent the mode lattice + :func:`consume_stop_request` documents must never happen, and on native Windows + it would silently withdraw the only channel that can stop the engine. Guarding it + is not available: an "unlink only if still my inode" step does not exist as one + atomic operation, and both check-then-unlink shapes measure *worse* than no guard + at all — the check moves the decision earlier and the destructive act later by + its own cost, shifting the window rather than narrowing it (inode compare 1.39x, + mode compare 2.30x, over a rendezvous-synchronised escalation sweep on btrfs). + + What a failed write leaves behind is a short or empty body, which + :func:`read_stop_request_mode` reads as ``"graceful"`` — exactly the mode this + function was asked to lodge, for the one caller (``stop --graceful``) that an + operator drove. It does not wedge the channel: a later graceful ask answers + "already-pending", a later *hard* stop supersedes it unconditionally, and + ``stop --cancel-graceful`` or ``resume`` withdraws it. Leaving a graceful request + standing is the bounded direction this channel already leans on everywhere else.""" + body = json.dumps({"requested_at": time.strftime("%Y-%m-%dT%H:%M:%S"), "mode": "graceful"}) + path = run_dir / STOP_REQUEST_FILE + try: + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError: + return False # a request is already pending — a planted link included + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(body) + return True + + def clear_graceful_stop(run_dir: Path) -> bool: - """Consume a pending graceful-stop request, returning True iff one was present - and removed. Never raises: a hard stop and a resume both call this to cancel a - superseded request, and a missing file (already consumed by the engine, or - never written) or an unremovable one must not wedge those paths. Uses the same - win32 sharing-violation retry the atomic write pairs with.""" + """Consume a pending stop request of *either* mode, returning True iff one was + present and removed. Never raises: the engine calls this the moment it honors a + request, a resume calls it to discard a stale one, and stop_run calls it on the + paths where nothing is left alive to read what it lodged — a missing file + (already consumed) or an unremovable one must not wedge any of them. Uses the + same win32 sharing-violation retry the atomic write pairs with.""" try: retrying_unlink(run_dir / STOP_REQUEST_FILE) except OSError: @@ -932,19 +1116,75 @@ def clear_graceful_stop(run_dir: Path) -> bool: return True +def consume_stop_request(run_dir: Path) -> str | None: + """Take the pending request off the channel and answer the mode of the very file + removed, or ``None`` when none was pending. + + The reader-side counterpart of :func:`_create_stop_request`'s + ``O_CREAT | O_EXCL``: the rename *is* the consume, so "what mode is pending?" + and "take it" cannot disagree. A read followed by an unlink can, and the gap is + not academic — a concurrent ``stop`` escalating to ``"hard"`` in between is + deleted unread while the caller routes on the stale ``"graceful"`` it already + holds. + + Only that direction can lose anything, because the mode lattice is monotone: + the graceful writer refuses to overwrite an existing request and the hard writer + only ever writes ``"hard"``, so ``absent < graceful < hard`` until consumed. A + stale ``"hard"`` read is therefore always still true; a stale ``"graceful"`` may + not be. + + Monotone requires that no writer *descends* either, which is why + :func:`_create_stop_request` has no rollback on a failed write: an ``unlink`` + keyed on the path rather than the inode it created is a ``hard -> absent`` drop, + and it would put a second way to lose a hard request in a *writer* — leaving the + three read-then-unlink sites in ``engine.py`` that rely on this argument resting + on something untrue. + + Re-reading the mode immediately before the unlink does NOT fix this, and is a + trap worth naming: measured over 4000 injected races it made the loss *more* + likely, not less (164 -> 929 swallowed), because the extra read lengthens the + interval an escalation has to land in. Narrowing a window is not closing it — + only one atomic step is. + + A hard request lodged *after* the take is a new request against a run already + stopping. It stays at the canonical name for ``run()``'s finally to discard and + journal as ``stop-request-discarded`` — a record, not a silent loss.""" + src = run_dir / STOP_REQUEST_FILE + taken = run_dir / (STOP_REQUEST_FILE + ".consumed") + try: + atomic_replace(src, taken) + except FileNotFoundError: + return None + except OSError: + # Could not take it (read-only dir, a sharing violation past its retries). + # Leave it on the channel and answer from the canonical name: the next + # boundary re-asks, which is strictly better than losing the request. + return read_stop_request_mode(run_dir) + try: + return _stop_request_mode_of(taken) + finally: + with contextlib.suppress(OSError): + retrying_unlink(taken) + + def request_graceful_stop(run_dir: Path) -> str: """Ask a live run to stop gracefully: finish the in-flight item (story -> dev/review/commit, or a sweep bundle through commit) cleanly, then finalize and - stop — resumable, unlike the hard SIGTERM :func:`stop_run` delivers. + stop — resumable, unlike the hard stop :func:`stop_run` delivers. - Delivery is the :data:`STOP_REQUEST_FILE` control file, written atomically (tmp - + ``atomic_replace``) so a concurrent engine read never sees a partial file. + Delivery is the :data:`STOP_REQUEST_FILE` control file, written atomically by + :func:`_write_stop_request` so a concurrent engine read never sees a partial file. Never signals the process and never writes ``journal.jsonl`` (engine-owned single-writer). Returns a status token for the caller to message on: - ``"requested"`` — file written; a provably-live engine will honor it. - ``"already-pending"`` — a request was already on disk; left untouched so its - original ``requested_at`` stands (idempotent — a second ask is a no-op). + original ``requested_at`` stands (idempotent — a second ask is a no-op). The + token is mode-blind, and the pending request is not necessarily graceful: a + *hard* one sits there at rest whenever a `stop` could not prove the engine + dead, and one can also land while this call is in flight. A stronger stop + stands either way and must not be downgraded — so callers message this token + as a *stop request*, never as a graceful one (#319). - ``"requested-unverifiable"`` — file written, but engine liveness read ``'unknown'`` (e.g. a win32 access-denied pid): the request stands and fires if an engine is in fact running; the caller warns that it can't confirm. @@ -963,33 +1203,74 @@ def request_graceful_stop(run_dir: Path) -> str: f"run {run_dir.name} has no live engine — a graceful stop request would " f"never be consumed; use `bmad-loop resume {run_dir.name}` to continue it" ) - path = run_dir / STOP_REQUEST_FILE - tmp = path.with_name(path.name + ".tmp") - body = json.dumps({"requested_at": time.strftime("%Y-%m-%dT%H:%M:%S"), "mode": "graceful"}) - tmp.write_text(body, encoding="utf-8") - atomic_replace(tmp, path) + # The write IS the check. The existence test at the top of this function is + # separated from here by a pid-file read, a liveness probe and (formerly) a + # mkstemp and an fsync — measured at ~1.3ms median on btrfs, wide enough for a + # concurrent `stop` to lodge `"hard"` in between — and the channel is + # last-writer-wins, so an unconditional replace here would silently *downgrade* + # it and cost the abort the operator asked for. A re-read just before the replace + # narrows that window; a create-if-absent removes it, because there is no longer + # a gap between deciding and writing. "already-pending" is the same answer the + # check at the top gives, and the right one either way: a lodged hard request is + # a *stronger* stop already standing. Two concurrent *graceful* asks resolve the + # same way, which is the documented idempotency — the first one's timestamp + # stands. The escalation direction is untouched and stays unconditional. + if not _create_stop_request(run_dir): + return "already-pending" return "requested" if liveness == "alive" else "requested-unverifiable" def stop_run(run_dir: Path) -> bool: """Stop a live run. Returns False if it was already finished. - Prefers the engine's own SIGTERM handler so the engine stays the single - writer of `stopped` (it marks the run, kills its in-flight agent window, and - exits). Falls back to an external kill + mark when there is no live engine - pid, it is a legacy run, or it does not exit in time. A wedged engine that - ignores SIGTERM past the grace window is force-killed — but only while we can - still prove the pid is the same process we signalled (a pid-reuse guard); - otherwise we raise StopRunError rather than risk killing an unrelated process. + The request is delivered two ways at once, and the engine wins whichever race + it can: a ``mode: hard`` :data:`STOP_REQUEST_FILE` is lodged *first*, then the + engine is signalled. SIGTERM is the POSIX fast path — the handler stops the run + within the tick. The file is what makes the stop work where the signal cannot + land: a native-Windows engine never receives an inter-process SIGTERM, so before + #319 every Windows stop burned the full grace window into a blind force-kill. + Now the engine reads the file at its next item boundary, or mid-session in the + adapter wait loop, and performs its own teardown either way. + + That ordering is the whole point: lodging before signalling means the engine can + never exit the signal path having missed a request that was only written after. + + Either way the engine stays the single writer of `stopped` (it marks the run, + kills its in-flight agent window, and exits). Falls back to an external kill + + mark when there is no live engine pid, it is a legacy run, or it does not exit + in time. A wedged engine that ignores both channels past the grace window is + force-killed — but only while we can still prove the pid is the same process we + signalled (a pid-reuse guard); otherwise we raise StopRunError rather than risk + killing an unrelated process. + + The lodged file is consumed by whoever settles the run: the engine when it + honors the request, or this function on the paths where nothing is left alive to + read it. Both exceptions to that turn on the same question — did we ever *prove* + the engine dead? Where we did not, the file stays lodged, because it is then the + only channel that can still stop it: the StopRunError refusal below (we decline + to force-kill an unverifiable pid), and the ``engine_may_live`` paths where the + signal or the kill was refused outright rather than racing us to exit. """ state = load_state(run_dir) if state.finished: return False - # A hard stop always supersedes a pending graceful request — cancel it so a - # later resume doesn't re-honor a stop the operator escalated past (covers the - # signalled, force-kill, and mark-stopped fallback paths below alike). - clear_graceful_stop(run_dir) + # Lodge the hard request before signalling. The atomic replace also supersedes a + # pending *graceful* request in the same step: the operator escalated past it, and + # a stronger request must never leave a window where nothing at all is pending. + # + # Degrade rather than abort when the lodge fails (read-only run dir, ENOSPC — and + # the run's own session logs tee into this very directory, so a run can fill the + # disk that then blocks stopping it). The doctrine's unit is the *repair*, not the + # syscall: this stop is "delivered two ways at once" per the docstring above, so + # failing the whole thing because one of two redundant channels failed would leave + # a run alive that the pre-#319 signal path could still have killed. Keep the + # signal, and stay loud where it actually matters — see the refusal branch below. + try: + _write_stop_request(run_dir, "hard") + lodged = True + except OSError: + lodged = False host = get_process_host() pid, identity = read_pid_identity(run_dir) # identity recorded at run start, not sampled now @@ -997,11 +1278,24 @@ def stop_run(run_dir: Path) -> bool: # the pid we recorded is already gone, or was reused by an unrelated # process before stop_run ran — never signal a stranger; mark stopped below. pid = None + # Whether this call ever proved the engine dead. Only a confirmed death licenses + # the fallback below to discard the request we lodged: while the engine may still + # be running, that file is the one channel left that can stop it (on native + # Windows it is the *only* one), so retracting it would throw away the very + # repair #319 exists to deliver. + engine_may_live = False if pid is not None: try: host.terminate(pid) - except (ProcessLookupError, PermissionError, OSError): - pid = None # already gone / not ours — go straight to fallback + except ProcessLookupError: + pid = None # provably gone — the fallback's discard is correct + except (PermissionError, OSError): + # We could not signal it and it was `alive_and_ours` a moment ago, so it + # may well still be running (an EPERM mismatch, or a win32 taskkill that + # errored). Skip the wait — there is nothing to wait for — but keep the + # request lodged so the engine can still stop itself off the file. + engine_may_live = True + pid = None if pid is not None: deadline = time.monotonic() + _STOP_WAIT_S while time.monotonic() < deadline: @@ -1019,23 +1313,112 @@ def stop_run(run_dir: Path) -> bool: if guard is not None and host.identity(pid) == guard: try: host.force_kill(pid) - except (ProcessLookupError, PermissionError, OSError): + except ProcessLookupError: pass # raced us to exit — that's the outcome we wanted + except (PermissionError, OSError): + # Unlike ESRCH above, this is the opposite news: the process is + # there and we were refused. Keep the request lodged. + engine_may_live = True + else: + # A kill that returned cleanly is not a death certificate — on + # win32 `force_kill` shells `taskkill /F /T` with `check=False`, + # so a refused kill raises nothing at all, and win32 is the + # platform this whole channel exists for. Confirm rather than + # infer, since the answer decides whether we discard the request. + # Let it settle first: a delivered SIGKILL is immediate but the + # pid can linger a moment before it is reaped, and reading that + # as "still alive" would strand the file on the ordinary + # wedged-engine path. + confirm_deadline = time.monotonic() + _KILL_CONFIRM_S + while host.is_alive(pid) and time.monotonic() < confirm_deadline: + time.sleep(_STOP_POLL_S) + engine_may_live = host.is_alive(pid) else: + # Refusing to kill leaves the hard request lodged on purpose: if that + # pid *is* still our engine, the file is the only channel left that + # can stop it, and discarding it here would retract a request the + # operator made while we decline to enforce it ourselves. + # + # That reasoning only holds while the lodge succeeded. If it did not, + # nothing at all is pending and we are declining to force-kill on top + # of that — the operator must be told, or they are left believing a + # request is in flight that was never written. + if lodged: + raise StopRunError( + f"run {run_dir.name}: engine pid {pid} honored neither the " + "lodged stop request nor SIGTERM, and its identity can no " + "longer be verified; refusing to force-kill a possibly-reused " + "pid" + ) raise StopRunError( - f"run {run_dir.name}: engine pid {pid} ignored SIGTERM and its " - "identity can no longer be verified; refusing to force-kill a " - "possibly-reused pid" + f"run {run_dir.name}: the stop request could not be written to " + f"the run directory and engine pid {pid} did not honor SIGTERM; " + "its identity can no longer be verified, so it will not be " + "force-killed. No stop is pending — free space in the run " + "directory and retry, or stop the process yourself" ) - # the engine clears its agent window itself, but kill the session as a - # backstop in case it died before tearing it down - kill_session(run_dir.name) - if load_state(run_dir).stopped: - return True - - # Fallback: no live engine (or it never confirmed). Mark it stopped here. + # the engine clears its agent window itself, but kill the session as a backstop + # in case it died before tearing it down. Ahead of everything below, because both + # exits from here need it — an engine that honored the stop and died before + # tearing its window down leaks the session just as surely as one we killed. kill_session(run_dir.name) state = load_state(run_dir) + if state.stopped: + # The engine honored the stop and is gone, and its own `run-stop` already + # stands in the journal. Stamping `fallback=True` on top would describe an + # engine that did its own teardown as one that had to be stopped from + # outside. This check deliberately sits out here rather than inside the + # `pid is not None` arm it used to live in: every path that clears `pid` + # early — a pid that is no longer ours, a `terminate` that raced the exit + # and got `ProcessLookupError`, a refusal that could not verify it — skipped + # it and fell straight through to the append. The plainest case needs no race + # at all: `stop` on a run a previous `stop` already stopped (`stopped` is set, + # `finished` is not, so the guard at the top does not fire). + # + # It normally consumes the file on the way out; clear it belt-and-braces so a + # run that is later resumed can never find our request still lodged and + # re-stop at its first item. Safe on the `engine_may_live` paths too: a + # written `stopped` *is* the engine reporting it honored the request, so + # there is no live consumer left to strand. + clear_graceful_stop(run_dir) + return True + + # Neither channel was delivered: nothing is lodged, and we never proved the engine + # dead. This is the one outcome `stop` must not report as success — the operator is + # left believing a request is in flight that was never written, while an engine we + # could not signal keeps mutating the project. The pid-reuse guard above already + # refuses for its own path; these are its siblings, and the only reason they stayed + # quiet is that they clear `pid` and skip that block. Not a regression — on the + # merge-base this was the state of *every* refused signal, because `stop_run` cleared + # the request as its first statement — but the earlier decision to report success + # rested on the request being retained, which is exactly what did not happen here. + # + # Placement is load-bearing, twice over. It sits *after* the session backstop + # because refusing to report a stop is no reason to leak the window, and *after* the + # `state.stopped` return because a run the engine already honored must not be + # reported as a failure. Journal the attempt before raising: the `run-stop` append + # below is skipped, and an unrecorded stop attempt is its own trap. + if engine_may_live and not lodged: + Journal(run_dir).append("run-stop-undelivered", pid=pid) + raise StopRunError( + f"run {run_dir.name}: the stop request could not be written to the run " + "directory and the engine could not be proved dead, so no stop is pending. " + "Its agent session was killed as a backstop. Free space in the run directory " + "and retry, or stop the process yourself" + ) + + # Fallback: no live engine (or it never confirmed). Mark it stopped here. Discard + # the request first — nothing is left alive to consume it, and a file outliving + # the run it asked to stop is a trap for the next resume. + # + # Unless we never actually proved that. Where the engine may still be running, + # the request stays lodged and the stop is genuinely still in flight: the engine + # honors the file at its next poll and writes `stopped` itself. Discarding it here + # would leave a live engine with no channel left while we report the run stopped — + # the stale-request trap above is the lesser of the two, and it only bites a run + # that is later resumed, which this one cannot be until that engine exits. + if not engine_may_live: + clear_graceful_stop(run_dir) state.stopped = True save_state(run_dir, state) Journal(run_dir).append("run-stop", pid=pid, fallback=True) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 7aff42d48..b0f3d8d9f 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -568,7 +568,7 @@ def _loop(self) -> None: # request during a cycle is caught before the next _run_bundle (see # _cycle); one landing between cycles stops here before cycle N+1 # re-triages. - self._check_graceful_stop() + self._check_stop_request() self.state.sweep_cycle = cycle self._save() text = ledger.read_text(encoding="utf-8") if ledger.is_file() else "" @@ -693,7 +693,7 @@ def _cycle(self, cycle: int, open_now: set[str]) -> bool: # bundles run. Mid-cycle stop is resume-safe: sweep_cycle is # persisted, triage.json is cached, closes are idempotent, and # terminal tasks are skipped on re-drive. - self._check_graceful_stop() + self._check_stop_request() self._run_bundle(bundle, cycle) bundles_done = sum( 1 diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 558e1fdb9..b81fb4acc 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -937,12 +937,13 @@ def _stop_run_worker(self, run_id: str, run_dir: Path) -> None: def action_graceful_stop_run(self) -> None: """Ask the selected live run to stop *gracefully*: finish the in-flight item (story dev/review/commit, or a sweep bundle through commit), then finalize - cleanly and stop — resumable, unlike the hard SIGTERM `x` delivers. + cleanly and stop — resumable, unlike the hard stop `x` delivers, which + abandons the in-flight item. Deliberately no `_mux_missing` gate: unlike `x` (which kills the agent - window) this touches no multiplexer — the request is a control file the - engine polls at item boundaries — so it must work even with the backend - down. The liveness gate is also deliberately looser than `x`'s: it rejects + window) this touches no multiplexer — the request rides the same control + file a hard stop uses, in its graceful mode, read by the engine at item + boundaries — so it must work even with the backend down. The liveness gate is also deliberately looser than `x`'s: it rejects only a *provably dead* engine, so an unverifiable (`unknown`) pid — a win32 access-denied pid, a psmux backend, a run on another host — still lodges the request, matching `runs.request_graceful_stop`'s `requested-unverifiable` @@ -964,7 +965,8 @@ def done(ok: bool | None) -> None: "graceful stop", f"stop run {run_id} after the current item finishes?\n" "the in-flight story/bundle completes through commit, then the run " - "finalizes and stops (resumable). `x` stops immediately instead.", + "finalizes and stops (resumable). `x` instead abandons the " + "in-flight item.", confirm_label="graceful stop", ), done, @@ -983,7 +985,9 @@ def _graceful_stop_worker(self, run_id: str, run_dir: Path) -> None: self.call_from_thread(self.notify, str(e), severity="error") return if outcome == "already-pending": - self.call_from_thread(self.notify, f"run {run_id} already has a graceful stop pending") + # Mode-neutral: the pending request may be a hard one, and this token + # cannot tell (#319) — same wording as the CLI's `stop --graceful`. + self.call_from_thread(self.notify, f"run {run_id} already has a stop request pending") return if outcome == "requested-unverifiable": self.call_from_thread( diff --git a/src/bmad_loop/tui/data.py b/src/bmad_loop/tui/data.py index fbd2a1a48..264083676 100644 --- a/src/bmad_loop/tui/data.py +++ b/src/bmad_loop/tui/data.py @@ -96,10 +96,13 @@ def status(self) -> str: return _classify(state.finished, state.paused, state.stopped, state.crashed, self.run_dir) def stopping(self) -> bool: - """True when a graceful-stop request is pending for this run (its control - file is present) — a bare existence read for the run-header pending line, - mirroring runs.graceful_stop_requested. The caller gates on a RUNNING - status so a file lingering on a stopped run doesn't read as still-stopping.""" + """True when a stop request of *either* mode is pending for this run (its + control file is present) — a bare existence read for the run-header pending + line, mirroring runs.graceful_stop_requested. Deliberately mode-blind: a run + with a hard request lodged (#319) is stopping too, and the request is on disk + only for the seconds it takes the engine to honor it. The caller gates on a + RUNNING status so a file lingering on a stopped run doesn't read as + still-stopping.""" return (self.run_dir / STOP_REQUEST_FILE).is_file() def attention(self) -> str: diff --git a/src/bmad_loop/tui/screens/dashboard.py b/src/bmad_loop/tui/screens/dashboard.py index f92603462..105a3c983 100644 --- a/src/bmad_loop/tui/screens/dashboard.py +++ b/src/bmad_loop/tui/screens/dashboard.py @@ -130,7 +130,7 @@ class _Snapshot: has_run: bool = False run_id: str = "" status: str = data.UNKNOWN - stopping: bool = False # selected run has a graceful stop pending (RUNNING only) + stopping: bool = False # selected run has a stop request pending, either mode (RUNNING only) agent: data.ActiveAgent | None = None # agent driving the selected run, live only state: RunState | None = None stories_mode: bool = False # selected run is stories mode (source == "stories") @@ -766,9 +766,10 @@ def _poll( snap.run_id = ctx.run_dir.name snap.state = ctx.watcher.state() snap.status = ctx.watcher.status() - # A graceful stop pending is the control file, meaningful while an - # engine is still around to consume it — RUNNING or UNKNOWN (an - # unverifiable pid still honors it, matching the CLI's != "dead"). + # A pending stop is the control file's presence in either mode, + # meaningful while an engine is still around to consume it — RUNNING + # or UNKNOWN (an unverifiable pid still honors it, matching the CLI's + # != "dead"). snap.stopping = ( snap.status in (data.RUNNING, data.UNKNOWN) and ctx.watcher.stopping() ) diff --git a/src/bmad_loop/tui/widgets.py b/src/bmad_loop/tui/widgets.py index c14527097..2d82e354b 100644 --- a/src/bmad_loop/tui/widgets.py +++ b/src/bmad_loop/tui/widgets.py @@ -95,9 +95,10 @@ def pause_label(stage: str) -> tuple[str, str]: def stopping_tag() -> Text: - """Compact tag for a run with a graceful stop pending, shown in the runs-table - note cell in place of a pause badge. The glyph + style match - STATUS_GLYPHS/STATUS_STYLES[STOPPED] — the end state a graceful stop lands in.""" + """Compact tag for a run with a stop request pending — either mode (#319), + matching the mode-blind read behind it — shown in the runs-table note cell in + place of a pause badge. The glyph + style match + STATUS_GLYPHS/STATUS_STYLES[STOPPED] — the end state either stop lands in.""" return Text("⏹ stop", style=STATUS_STYLES[data.STOPPED]) diff --git a/tests/test_cli.py b/tests/test_cli.py index 8dcc17b44..9ea43d121 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2092,11 +2092,58 @@ def test_stop_graceful_is_idempotent(tmp_path, monkeypatch, capsys): run_dir = _pending_graceful_run(tmp_path) # request already on disk before = (run_dir / runs.STOP_REQUEST_FILE).read_text() assert cli.main(["stop", "--project", str(tmp_path), "r1", "--graceful"]) == 0 - assert "already has a graceful stop pending" in capsys.readouterr().out + assert "already has a stop request pending" in capsys.readouterr().out # left untouched — the original request's timestamp stands assert (run_dir / runs.STOP_REQUEST_FILE).read_text() == before +def test_stop_graceful_reports_a_pending_hard_request_without_calling_it_graceful( + tmp_path, monkeypatch, capsys +): + """A lodged *hard* request answers "already-pending" too, and the message must + not describe it as graceful — that reports a strictly stronger stop as a weaker + one. Reachable with no race at all: `stop_run` leaves a hard request lodged when + it could not prove the engine dead, and it sits there at rest. + + Written directly rather than through `_pending_graceful_run`, which hardcodes + the graceful mode. + """ + from bmad_loop import runs + + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + run_dir = _make_run_with_state(tmp_path, "r1") + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "now", "mode": "hard"}', encoding="utf-8" + ) + before = (run_dir / runs.STOP_REQUEST_FILE).read_text() + assert cli.main(["stop", "--project", str(tmp_path), "r1", "--graceful"]) == 0 + out = capsys.readouterr().out + assert "already has a stop request pending" in out + assert "graceful stop pending" not in out # the hard request is not a graceful one + # and the stronger request still stands, unchanged and un-downgraded + assert (run_dir / runs.STOP_REQUEST_FILE).read_text() == before + assert runs.read_stop_request_mode(run_dir) == "hard" + + +def test_stop_graceful_reports_a_failed_write_as_possibly_pending(tmp_path, monkeypatch, capsys): + """The lodge deliberately does not roll back a failed write — an unlink there + resolves the name and could delete a hard request a concurrent `stop` escalated + onto it. So a request can be standing even though the write raised, and saying + "failed" flatly would invite the operator to ask again for something already + pending. Same exit code; accurate message.""" + from bmad_loop import runs + + def _boom(_run_dir): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(runs, "request_graceful_stop", _boom) + _make_run_with_state(tmp_path, "r1") + assert cli.main(["stop", "--project", str(tmp_path), "r1", "--graceful"]) == 1 + err = capsys.readouterr().err + assert "may still be pending" in err + assert "--cancel-graceful" in err # names the way to withdraw it + + def test_stop_cancel_graceful_clears_pending(tmp_path, capsys): from bmad_loop import runs @@ -2109,7 +2156,43 @@ def test_stop_cancel_graceful_clears_pending(tmp_path, capsys): def test_stop_cancel_graceful_without_pending_errors(tmp_path, capsys): _make_run_with_state(tmp_path, "r1") # nothing on disk to cancel assert cli.main(["stop", "--project", str(tmp_path), "r1", "--cancel-graceful"]) == 1 - assert "no graceful stop pending" in capsys.readouterr().err + assert "no stop request pending" in capsys.readouterr().err + + +def test_stop_cancel_clears_a_pending_hard_request(tmp_path, capsys): + """`--cancel-graceful` is mode-neutral by contract (#319): the one hard request a + human can still reach is the one `stop_run` deliberately leaves lodged after + refusing to force-kill an unverifiable pid, and withdrawing that is a legitimate + thing to want. The graceful twin above proves the wiring for one mode only — the + function is named `clear_graceful_stop` while its contract is mode-neutral, so + mode-gating the clear is a live regression, and this is what reddens on it.""" + from bmad_loop import runs + + run_dir = _pending_hard_run(tmp_path) + assert cli.main(["stop", "--project", str(tmp_path), "r1", "--cancel-graceful"]) == 0 + assert "cancelled" in capsys.readouterr().out + assert not (run_dir / runs.STOP_REQUEST_FILE).exists() + + +def test_stop_cancel_reports_a_request_it_could_not_remove(tmp_path, monkeypatch, capsys): + """`clear_graceful_stop` never raises — five callers depend on that — so it + answers False for "nothing was pending" and "could not remove it" alike. Cancel + must not read the second as the first and tell the operator their request is gone + while it is still on disk and still honorable. Exit stays 1 either way; only the + message moves.""" + from bmad_loop import runs + + run_dir = _pending_graceful_run(tmp_path) + + def _refuse(_path): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(runs, "retrying_unlink", _refuse) + assert cli.main(["stop", "--project", str(tmp_path), "r1", "--cancel-graceful"]) == 1 + err = capsys.readouterr().err + assert "still pending" in err + assert "no stop request pending" not in err # the misleading line, specifically + assert (run_dir / runs.STOP_REQUEST_FILE).exists() # and it really did survive def test_stop_graceful_and_cancel_are_mutually_exclusive(tmp_path): @@ -2142,6 +2225,53 @@ def test_status_json_graceful_stop_pending_true(tmp_path, monkeypatch, capsys): assert doc["schema_version"] == 1 # additive field — no schema bump +def _pending_hard_run(tmp_path, run_id="r1", **state_kwargs): + """A run with a HARD-mode stop request on disk — what `bmad-loop stop` lodges + before signalling, still unconsumed because the engine has not reached a + boundary (or, on native Windows, was never reachable by the signal at all).""" + from bmad_loop import runs + + run_dir = _make_run_with_state(tmp_path, run_id, **state_kwargs) + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "now", "mode": "hard"}', encoding="utf-8" + ) + return run_dir + + +def test_status_json_graceful_stop_pending_false_for_hard_request(tmp_path, monkeypatch, capsys): + """A hard stop in flight is not a *graceful* stop pending. The field is + mode-exact, not an existence check: reporting True here would promise an + operator that the in-flight item still finishes, when a hard request stops the + run as soon as the engine sees it. + + Ablation: reverting cli.py's derivation to `runs.graceful_stop_requested` + (bare existence) turns this True and fails the assertion — confirmed, restored. + """ + from bmad_loop import runs + + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + _pending_hard_run(tmp_path) + doc = machine_json(["status", "--project", str(tmp_path), "r1", "--json"], capsys) + assert doc["graceful_stop_pending"] is False + assert doc["schema_version"] == 1 # same field, same type — narrowed, not bumped + + +def test_status_text_does_not_claim_graceful_for_hard_request(tmp_path, monkeypatch, capsys): + """The text branch reads the same derivation, so it inherits the fix: no + "will stop after the current item" promise for a hard request. + + Ablation: with the bare-existence derivation restored this prints the graceful + line and fails — confirmed, restored.""" + from bmad_loop import runs + + monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") + _pending_hard_run(tmp_path) + assert cli.main(["status", "--project", str(tmp_path), "r1"]) == 0 + out = capsys.readouterr().out + assert "graceful stop pending" not in out + assert "in progress" in out # still reported live — only the promise is gone + + def test_status_json_graceful_stop_pending_false_without_request(tmp_path, capsys): # no control file -> the cheap existence check short-circuits the liveness probe _make_run_with_state(tmp_path, "r1") @@ -3805,8 +3935,8 @@ def test_resume_under_an_unchanged_host_exec_config_reports_no_security_change( def test_resume_discards_stale_graceful_stop_request(project, monkeypatch, capsys): - """A resume is fresh user intent: a graceful-stop request left over from the - prior stopped-gracefully run must be cleared before write_pid re-arms the + """A resume is fresh user intent: a stop request left over from the prior + stopped run — either mode — must be cleared before write_pid re-arms the engine, or the re-driven loop would consume it at the first item boundary and immediately re-stop. The clear is noted on stderr.""" from bmad_loop import runs @@ -3820,7 +3950,101 @@ def test_resume_discards_stale_graceful_stop_request(project, monkeypatch, capsy assert cli._resume_paused_run(project.project, run_dir) == 0 assert not (run_dir / runs.STOP_REQUEST_FILE).exists() # consumed before the engine ran - assert "discarded a stale graceful-stop request" in capsys.readouterr().err + assert "discarded a stale stop request" in capsys.readouterr().err + + +def test_resume_discards_a_stale_hard_stop_request(project, monkeypatch, capsys): + """The mode-neutral half of the docstring above, which the graceful test alone + could not prove. A hard request survives `stop_run`'s refusal to force-kill an + unverifiable pid, and `_resume_paused_run` gates on `finished`, not `stopped`, so + such a run is genuinely resumable with a hard file still on disk. + + Ablation: mode-gate the clear at its call site to + `read_stop_request_mode(...) == "graceful"` — this reddens and its graceful twin + stays green.""" + from bmad_loop import runs + + run_dir = _paused_run_for_resume(project, monkeypatch) + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "old", "mode": "hard"}', encoding="utf-8" + ) + monkeypatch.setattr(cli, "Engine", _StubEngine) + + assert cli._resume_paused_run(project.project, run_dir) == 0 + + assert not (run_dir / runs.STOP_REQUEST_FILE).exists() + assert "discarded a stale stop request" in capsys.readouterr().err + + +def test_resume_refuses_when_a_stale_request_cannot_be_discarded(project, monkeypatch, capsys): + """Fail closed. The clear conflates "nothing pending" with "could not remove it", + so on a removal failure the discard notice never prints and resume used to arm + the pid anyway — the engine then consumed the surviving request at the very first + item boundary and re-stopped, with nothing on stderr to say why. Resuming again + repeats it: a livelock, not a one-shot annoyance. + + The refusal has to land *before* write_pid, or the run is already re-armed.""" + from bmad_loop import runs + + run_dir = _paused_run_for_resume(project, monkeypatch) + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "old", "mode": "hard"}', encoding="utf-8" + ) + + def _refuse(_path): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(runs, "retrying_unlink", _refuse) + started: list[str] = [] + monkeypatch.setattr(runs, "write_pid", lambda _d: started.append("armed")) + monkeypatch.setattr(cli, "Engine", _StubEngine) + + assert cli._resume_paused_run(project.project, run_dir) == 1 + assert started == [] # refused before the engine was re-armed + assert "could not be discarded" in capsys.readouterr().err + + +def test_refused_resume_leaves_the_pin_and_the_journal_untouched(project, monkeypatch, capsys): + """A refusal past this function's commit point must leave no trace of a resume + that did not happen. `_require_base_skills` used to be the last early exit here, + so the stale-request refusal above is the first branch that returns from *below* + the journal append and the integrity re-stamp — and both of those are persistent + writes, not in-memory state. + + The pin is the one that lasts. `write_trusted_config_digest` writes the exact + file the next resume reads back as `pinned`, so re-baselining it on a refusal + inverts the advisory it feeds: the warning fires on the attempt that stopped and + goes silent on the attempt that actually arms an engine — for a config change the + operator never accepted by resuming. The re-stamp's stated justification is that + the engine this process is about to arm re-reads the config; a path that arms + nothing does not earn it. + + Ablation: move the clear/refuse block back below + `runs.write_trusted_config_digest`. Both assertions redden, on two independent + axes — the pin becomes the freshly computed sha256, and one `run-resume` entry + appears — while `test_resume_refuses_when_a_stale_request_cannot_be_discarded` + above stays green, which is what separates this guard from that one.""" + from bmad_loop import runs + + run_dir = _paused_run_for_resume(project, monkeypatch) + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "old", "mode": "hard"}', encoding="utf-8" + ) + # A sentinel the re-stamp cannot reproduce: the real digest is a sha256, so + # equality against this is a positive assertion, not "some value is present" — + # which `is not None` would have been, and which would survive the ablation. + runs.write_trusted_config_digest(project.project, run_dir.name, "OLDPIN") + + def _refuse(_path): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(runs, "retrying_unlink", _refuse) + monkeypatch.setattr(cli, "Engine", _StubEngine) + + assert cli._resume_paused_run(project.project, run_dir) == 1 + assert "could not be discarded" in capsys.readouterr().err + assert runs.read_trusted_config_digest(project.project, run_dir.name) == "OLDPIN" + assert _resume_entries(run_dir) == [] def test_resume_refuses_live_run(tmp_path, monkeypatch, capsys): diff --git a/tests/test_engine.py b/tests/test_engine.py index ce926534c..0351859f9 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -33,7 +33,7 @@ write_sprint, ) -from bmad_loop import deferredwork, platform_util, verify +from bmad_loop import deferredwork, platform_util, runs, verify from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter from bmad_loop.engine import Engine, RunPaused, RunStopped, _digest_of, _run_depth @@ -67,7 +67,14 @@ SweepPolicy, VerifyPolicy, ) -from bmad_loop.runs import STOP_REQUEST_FILE, graceful_stop_requested, rearm_escalation +from bmad_loop.runs import ( + STOP_REQUEST_FILE, + graceful_stop_requested, + owner_run_dir, + rearm_escalation, + reset_owner_run_dir, + set_owner_run_dir, +) from bmad_loop.sprintstatus import story_status from bmad_loop.verify import ( GitError, @@ -10401,6 +10408,58 @@ def test_graceful_stop_finishes_current_story_then_stops(project, monkeypatch): assert stops[-1]["remaining"] == 1 # 1-2-b still actionable, never picked +def test_boundary_consume_does_not_swallow_a_concurrent_escalation(project, monkeypatch): + """WIRING, not predicate. `runs.consume_stop_request` being atomic proves nothing + unless the boundary check actually calls it — a revert of this call site to + read-then-unlink is a separate regression with its own ablation axis. + + An escalating `stop` landing while the boundary consumes must survive as a + record. This engine routes on the graceful body it took (correct — that is the + request it holds), and the hard request that arrived afterwards is a new request + against a run already stopping: `run()`'s finally discards it and journals + `stop-request-discarded`, so it is accounted for rather than vanishing. + + Ablation: revert `_check_stop_request` to `read_stop_request_mode` + + `clear_graceful_stop`. The escalation is then injected into a take that never + happens, so `escalated` stays empty and the test reddens on that assert — which + IS the wiring proof: no atomic consume, no `.consumed` read to hook. Keep the + take but drop the survival and the `stop-request-discarded` assert is what + catches it instead.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, _ = make_engine( + project, + [ + _lodge_after(dev_effect(project, "1-1-a"), run_dir), + review_effect(project, "1-1-a", clean=True), + ], + ) + + real = runs._stop_request_mode_of + escalated: list[str] = [] + + def _escalate_then_read(path): + # only the read of the TAKEN file, which is the consume and nothing else — + # raise site B reads the canonical name and must not be perturbed here + if path.name.endswith(".consumed") and not escalated: + escalated.append("hard") + runs._write_stop_request(run_dir, "hard") + return real(path) + + monkeypatch.setattr(runs, "_stop_request_mode_of", _escalate_then_read) + engine.run() + + assert escalated == ["hard"] # the interleave really happened + saved = load_state(engine.run_dir) + assert saved.stopped is True and saved.finished is False + kinds = [e["kind"] for e in engine.journal.entries()] + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + assert stops and stops[-1]["graceful"] is True # routed on the body it took + # the escalation was not swallowed: it survived the consume to be recorded + assert "stop-request-discarded" in kinds + + def test_graceful_stop_runs_clean_finalization_and_notifies(project, monkeypatch): """Unlike a hard stop, the graceful arm runs worktree GC + the post_run hook + the policy-gated session teardown, and the trailing notify is worded for a @@ -10577,6 +10636,94 @@ def stopping_loop(): assert "stop-request-discarded" in [e["kind"] for e in engine.journal.entries()] +def test_signal_stop_consumes_the_lodged_hard_request(project, monkeypatch): + """A bare ``RunStopped()`` — the signal handler's shape — with the hard request + ``stop_run`` lodges before signalling: the file is consumed, not journaled as + stale debris. + + ``stop_run`` lodges *before* it signals, so on POSIX every routine stop reaches + the hard arm with the file still on disk; nothing on the signal path consumes it. + Left there, ``run()``'s finally discards it as stale and journals + ``stop-request-discarded``, misreporting the very request that caused the stop. + Contrast :func:`test_hard_stop_wins_over_pending_graceful_stop`: a *graceful* + request really is superseded by a hard stop, and still journals the discard. + + The gate here is the absent journal entry, not the absent file — the finally + clears the file either way, so that assertion holds with the guard ablated.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, _ = make_engine(project, []) + + def stopping_loop(): + _lodge_hard_stop_request(run_dir) + raise RunStopped() # hard (graceful=False), as the signal handler raises + + monkeypatch.setattr(engine, "_loop", stopping_loop) + engine.run() + + assert load_state(engine.run_dir).stopped + assert not graceful_stop_requested(run_dir) + kinds = [e["kind"] for e in engine.journal.entries()] + assert "stop-request-discarded" not in kinds # honored, not stale debris + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + # the signal delivered this stop, so it keeps journaling a bare `run-stop` — + # consuming the co-lodged file must not start attributing it to the channel. + assert stops and "via" not in stops[-1] + + +def test_hard_request_at_an_exhausted_queue_stops_instead_of_finishing(project, monkeypatch): + """A hard request landing on the exhausted-queue return path stops the run. + + None of the three raise sites reach it: A and B are inside ``_run_session``, + which an empty queue never enters, and the run-end auto-sweep predicate is + mode-blind, so it suppresses and *returns* rather than raising. Uncovered, the + run records ``finished`` — which ``documents.py`` ranks above ``stopped`` — while + the operator's hard stop went unhonored, and ``stop_run`` would then journal + ``fallback=True`` against a perfectly responsive engine.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, _ = make_engine(project, []) + + def empty_loop(): + # the queue drained, and the request lands before `_loop` returns — i.e. + # after its own head check has already run, which is the whole window. + _lodge_hard_stop_request(run_dir) + + monkeypatch.setattr(engine, "_loop", empty_loop) + engine.run() + + saved = load_state(engine.run_dir) + assert saved.stopped is True and saved.finished is False + assert not graceful_stop_requested(run_dir) # consumed before the raise + kinds = [e["kind"] for e in engine.journal.entries()] + assert "run-complete" not in kinds + assert "stop-request-discarded" not in kinds # honored, not stale + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + assert stops and stops[-1]["via"] == "stop-request" + + +def test_graceful_request_at_an_exhausted_queue_still_finishes(project, monkeypatch): + """The mode-exact half of the guard above, and its second ablation axis. + + A *graceful* request on that same return path finishes truthfully: the story + queue is empty, so there is nothing left to stop before, and the finally discards + the superseded file. Widening the new check to ``is not None`` reddens exactly + this test and leaves its hard twin green.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, _ = make_engine(project, []) + + monkeypatch.setattr(engine, "_loop", lambda: _lodge_stop_request(run_dir)) + engine.run() + + saved = load_state(engine.run_dir) + assert saved.finished is True and saved.stopped is False + kinds = [e["kind"] for e in engine.journal.entries()] + assert "run-complete" in kinds + assert "stop-request-discarded" in kinds # superseded nothing — genuinely stale + assert "run-stop" not in kinds + + def test_crash_wins_over_pending_graceful_stop(project, monkeypatch): """An unexpected crash while a stop is pending wins; the crash arm records and the finally discards the stale control file (no run-stop).""" @@ -10698,6 +10845,338 @@ def crashing_emit(stage, *args, **kwargs): assert stops and stops[-1]["graceful"] is True +# ----------------------------------------------------------- hard stop (#319) +# +# The same stop-request.json control file, written with `mode: "hard"` by +# `runs.stop_run`, carries a stop the signal path cannot deliver: a native-Windows +# engine never receives an inter-process SIGTERM. The engine honors it in three +# places — the item boundary (`_check_stop_request`) and the two `_run_session` +# raise sites: an `"aborted"` result handed back by the adapter's in-session poll +# (site A), and a hard file still on disk once a session has been recorded and +# saved (site B, the `_post_kill_reconcile`-rescued case). All three take the HARD +# arm: unconditional teardown, `run-stop` carrying `via="stop-request"` and no +# `graceful` flag. These tests lodge the control file directly, exactly as the +# graceful ones do. +# +# A nested auto-sweep child reads a second channel: the OWNING run's dir, published +# by the outermost `run()` and polled by the adapter and by site B's owner leg. That +# leg never consumes — the file belongs to the parent, whose own hard arm has to +# find it to record and attribute the stop — and a nested engine re-raises rather +# than recording, so those tests assert on the propagated exception. + + +def _lodge_hard_stop_request(run_dir: Path) -> None: + """Drop the control file `runs.stop_run` writes before it signals — the hard + sibling of :func:`_lodge_stop_request`.""" + (run_dir / STOP_REQUEST_FILE).write_text( + '{"requested_at": "2026-07-20T00:00:00", "mode": "hard"}', encoding="utf-8" + ) + + +def _lodge_hard_after(inner, run_dir: Path): + """Wrap a scripted effect so a HARD request lands as ``inner`` returns — i.e. + in the window between the adapter's last poll and the engine's post-session + check, the gap raise site B exists to cover.""" + + def effect(spec): + result = inner(spec) + _lodge_hard_stop_request(run_dir) + return result + + return effect + + +def test_session_abort_status_unwinds_run_stopped(project, monkeypatch): + """RAISE SITE A. An adapter whose wait loop saw the hard file tears its window + down and hands back `status="aborted"`; the engine unwinds that into a hard + RunStopped *before* any SessionRecord exists, so an abort is never mistaken for + a session outcome and no further session is launched. The paired session-end is + still journaled — through the `finally`, which is why the raise sits inside the + try. + + Ablation: delete the `result.status == "aborted"` gate in `_run_session` and the + abort is recorded as an ordinary session; the run drives on into the review leg + and this test fails (verified).""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [SessionResult(status="aborted"), review_effect(project, "1-1-a", clean=True)], + ) + + engine.run() + + saved = load_state(engine.run_dir) + assert saved.stopped is True and saved.finished is False + assert killed == ["test-run"] # hard arm's unconditional teardown + assert len(adapter.sessions) == 1 # nothing launched after the abort... + assert len(adapter.script) == 1 # ...the review entry is still unspent + assert saved.tasks["1-1-a"].sessions == [] # no SessionRecord for the abort + entries = engine.journal.entries() + assert "run-complete" not in [e["kind"] for e in entries] + starts = [e for e in entries if e["kind"] == "session-start"] + ends = [e for e in entries if e["kind"] == "session-end"] + assert len(starts) == 1 and len(ends) == 1 # paired through the finally + assert ends[0]["task_id"] == starts[0]["task_id"] + assert ends[0]["status"] == "aborted" + stops = [e for e in entries if e["kind"] == "run-stop"] + assert stops and stops[-1]["via"] == "stop-request" + assert "graceful" not in stops[-1] + + +def test_nested_child_stops_on_the_owning_runs_hard_request(project, monkeypatch): + """RAISE SITE B, OWNER LEG. A nested auto-sweep child honors the *parent's* hard + request even when its own session came back `completed`. + + This is the nested form of the rescued-completion trap. `stop ` lodges + in the parent's dir, so the child's own channel is empty; its adapter aborts off + the owner leg, and `_post_kill_reconcile` can then upgrade that `aborted` back to + `completed` — at which point raise site A never fires. Without the owner leg here + the child would carry straight on into its review leg on the strength of the + rescue, which is exactly what #319 closed at top level. + + The child must NOT consume the parent's file: the parent's own hard arm has to + find it to record and attribute the stop, and `via` rides the exception anyway. + A nested engine re-raises `RunStopped` rather than recording it, so the owner + sees it — hence `pytest.raises` and no `stopped` flag on the child's state. + + Ablation: delete the owner leg at raise site B and the child drives its review + leg — `adapter.sessions` grows to 2 and this fails.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + child_run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + owner = project.project / ".bmad-loop" / "runs" / "parent-run" + owner.mkdir(parents=True, exist_ok=True) + (owner / STOP_REQUEST_FILE).write_text( + '{"requested_at": "2026-08-22T00:00:00", "mode": "hard"}', encoding="utf-8" + ) + + engine, adapter = make_engine( + project, + [ + dev_effect(project, "1-1-a"), # returns `completed` — no abort to see + review_effect(project, "1-1-a", clean=True), + ], + ) + + depth_token = _run_depth.set(1) # simulate the parent's run() frame + owner_token = set_owner_run_dir(owner) + try: + with pytest.raises(RunStopped) as caught: + engine.run() + finally: + reset_owner_run_dir(owner_token) + _run_depth.reset(depth_token) + + assert caught.value.via == "stop-request" + assert len(adapter.sessions) == 1 # the review leg never started... + assert len(adapter.script) == 1 # ...its script entry is still unspent + assert killed == ["test-run"] # the child tore its own session down + # the parent's request survives the child untouched — the owner consumes it + assert (owner / STOP_REQUEST_FILE).is_file() + assert not graceful_stop_requested(child_run_dir) # child's own was always empty + # a nested child re-raises instead of recording: the owner writes `stopped` + assert load_state(engine.run_dir).stopped is False + + +def test_nested_child_ignores_the_owning_runs_graceful_request(project, monkeypatch): + """The mode-exact twin: graceful means *finish the in-flight item*, and for a + nested sweep that means letting the child finish. A graceful request on the + owning run must not stop the child mid-flight — the parent suppresses the next + child from starting instead. + + Ablation: widen the owner leg at raise site B to `is not None` and this reddens + alone, leaving its hard twin green.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + owner = project.project / ".bmad-loop" / "runs" / "parent-run" + owner.mkdir(parents=True, exist_ok=True) + (owner / STOP_REQUEST_FILE).write_text( + '{"requested_at": "2026-08-22T00:00:00", "mode": "graceful"}', encoding="utf-8" + ) + + engine, adapter = make_engine( + project, + [ + dev_effect(project, "1-1-a"), + review_effect(project, "1-1-a", clean=True), + ], + ) + + depth_token = _run_depth.set(1) + owner_token = set_owner_run_dir(owner) + try: + engine.run() # runs to completion, no RunStopped + finally: + reset_owner_run_dir(owner_token) + _run_depth.reset(depth_token) + + assert len(adapter.sessions) == 2 # dev AND review — the child finished its item + + +def test_run_publishes_the_owner_run_dir_and_resets_it(project, monkeypatch): + """`run()` publishes its run dir as the owning run for everything below and + releases it on the way out, by token — so a later top-level run in the same + process/thread is never poisoned by a previous one. A nested frame must not + overwrite the owner it inherited.""" + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: None) + engine, _ = make_engine(project, []) + + seen = [] + monkeypatch.setattr(engine, "_loop", lambda: seen.append(owner_run_dir())) + + assert owner_run_dir() is None + engine.run() + assert seen == [engine.run_dir] # published for the duration + assert owner_run_dir() is None # and released again + + # a nested frame leaves the inherited owner alone + outer = project.project / ".bmad-loop" / "runs" / "outer-run" + depth_token = _run_depth.set(1) + owner_token = set_owner_run_dir(outer) + seen.clear() + try: + engine.run() + finally: + reset_owner_run_dir(owner_token) + _run_depth.reset(depth_token) + assert seen == [outer] # the child inherited, it did not republish its own + + +def test_hard_stop_after_completed_session_stops_before_next_leg(project, monkeypatch): + """RAISE SITE B. A hard request landing too late for the in-session poll — here + as the dev session returns `completed` — still stops the run. This is also the + rescued-completion shape: `_post_kill_reconcile` can upgrade an aborted session + back to `completed`, and without this site the run would carry straight on into + verify/review on the strength of that rescue. The session is fully recorded and + saved first, so the run stays resumable from a complete record. + + Ablation: delete raise site B (the post-`_save()` hard-file check in + `_run_session`) and the run drives the review leg — `adapter.sessions` grows to + 2 and this test fails (verified).""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, adapter = make_engine( + project, + [ + _lodge_hard_after(dev_effect(project, "1-1-a"), run_dir), + review_effect(project, "1-1-a", clean=True), + ], + ) + + engine.run() + + saved = load_state(engine.run_dir) + assert saved.stopped is True and saved.finished is False + assert killed == ["test-run"] + assert len(adapter.sessions) == 1 # the review leg never started... + assert len(adapter.script) == 1 # ...its script entry is still unspent + # the completed session IS on the record — durable before the stop unwound + records = saved.tasks["1-1-a"].sessions + assert len(records) == 1 and records[0].status == "completed" + assert not graceful_stop_requested(run_dir) # consumed before the raise + entries = engine.journal.entries() + kinds = [e["kind"] for e in entries] + assert "run-complete" not in kinds + assert "stop-request-discarded" not in kinds # honored, not discarded as stale + ends = [e for e in entries if e["kind"] == "session-end"] + assert len(ends) == 1 and ends[0]["status"] == "completed" + stops = [e for e in entries if e["kind"] == "run-stop"] + assert stops and stops[-1]["via"] == "stop-request" + assert "graceful" not in stops[-1] + + +def test_boundary_hard_file_takes_hard_arm(project, monkeypatch): + """A hard request that lands after the story's last session — lodged at + `post_story`, past raise site B — is honored at the next item boundary and takes + the HARD arm there: unconditional teardown, no clean-finish subset, `run-stop` + with `via="stop-request"` and no `graceful` flag. The in-flight story still ran + to completion, exactly as the graceful sibling does; only the arm differs.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + engine, adapter = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + + post_run_stages = [] + original_emit = engine._emit + + def spy_emit(stage, *args, **kwargs): + if stage == "post_run": + post_run_stages.append(stage) + if stage == "post_story": + # the operator's `bmad-loop stop` lands between story 1 and story 2 + _lodge_hard_stop_request(run_dir) + return original_emit(stage, *args, **kwargs) + + engine._emit = spy_emit + + engine.run() + + saved = load_state(engine.run_dir) + assert saved.tasks["1-1-a"].phase == Phase.DONE # in-flight story finished + assert "1-2-b" not in saved.tasks # the next story was never dispatched + assert saved.stopped is True and saved.finished is False + assert len(adapter.sessions) == 2 # dev + review, nothing after the boundary + assert not graceful_stop_requested(run_dir) # consumed at the boundary + assert killed == ["test-run"] # hard arm, not the policy-gated graceful teardown + assert post_run_stages == [] # hard path skips the clean-finish subset + kinds = [e["kind"] for e in engine.journal.entries()] + assert "run-complete" not in kinds + assert "stop-request-discarded" not in kinds # honored, not discarded as stale + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + assert stops and stops[-1]["via"] == "stop-request" + assert "graceful" not in stops[-1] and "remaining" not in stops[-1] + + +def test_boundary_modeless_file_reads_graceful(project, monkeypatch): + """BACK-COMPAT PIN. A bare `{}` stop-request body — what every pre-#319 writer + and fixture lodged — still takes the GRACEFUL arm: `read_stop_request_mode` + reads any present-but-modeless file as graceful, so raise site B ignores it + mid-story and the boundary finalizes cleanly with `graceful=True` and no + `via`.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_sprint(project, {"1-1-a": "ready-for-dev", "1-2-b": "ready-for-dev"}) + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + + def lodge_modeless(inner): + def effect(spec): + result = inner(spec) + (run_dir / STOP_REQUEST_FILE).write_text("{}", encoding="utf-8") + return result + + return effect + + engine, adapter = make_engine( + project, + [ + lodge_modeless(dev_effect(project, "1-1-a")), + review_effect(project, "1-1-a", clean=True), + ], + ) + + engine.run() + + saved = load_state(engine.run_dir) + # the modeless file did NOT abort mid-story: the review leg still ran + assert len(adapter.sessions) == 2 + assert saved.tasks["1-1-a"].phase == Phase.DONE + assert "1-2-b" not in saved.tasks + assert saved.stopped is True and saved.finished is False + assert not graceful_stop_requested(run_dir) + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + assert stops and stops[-1]["graceful"] is True + assert "via" not in stops[-1] + + # ------------------------------- review.on_timeout = "salvage-if-done" (#271) diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 078336bc7..19f7f099f 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -22,7 +22,7 @@ import pytest import regex -from bmad_loop import devcontract +from bmad_loop import devcontract, runs from bmad_loop.adapters import env_fault, generic, tmux_base from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec, SpecSnapshot from bmad_loop.adapters.generic import GenericDevAdapter, GenericTmuxAdapter @@ -1903,6 +1903,213 @@ def test_lifecycle_and_heartbeat_write_failure_is_swallowed(tmp_path): adapter._write_heartbeat("3-1-dev-1", {"ts": 0.0}) # must not raise +# ------------------------------ in-session hard-stop poll (#319) +# +# `bmad-loop stop` lodges a mode-aware stop-request.json before it signals, so a +# stop reaches a session on platforms where the engine's SIGTERM never arrives. +# The wait loop reads that file twice per iteration and returns the non-completion +# `aborted` verdict; the engine raises RunStopped off it. The adapter never +# unlinks the file — the engine consumes it, and must still see it to attribute +# the stop. +# +# Contract parity: test_opencode_http.py holds the same pair over the HTTP +# transport. A behavior change here must land in both or record the divergence. + + +def _lodge_stop_request(adapter, mode: str) -> Path: + """Lodge a stop request of ``mode`` on this run's control-file channel, as + ``bmad-loop stop`` does. Written directly rather than through + ``runs._write_stop_request`` so the adapter's read stays pinned to the + on-disk shape, not to the writer's guards.""" + adapter.run_dir.mkdir(parents=True, exist_ok=True) + path = adapter.run_dir / runs.STOP_REQUEST_FILE + path.write_text( + json.dumps({"requested_at": "2026-08-22T00:00:00", "mode": mode}), encoding="utf-8" + ) + return path + + +def test_wait_aborts_on_hard_stop_request(tmp_path, monkeypatch): + """A hard stop pending on the channel ends the wait on its very next + iteration with the non-completion ``aborted`` verdict — no artifact + read-back (that rescue is `_post_kill_reconcile`'s job) and no timeout burn. + + The abort fires before the loop ever reaches its event source, so the + steerable clock never advances: the pass is deterministic, not a race. + + Ablation: delete the `_hard_stop_requested()` arm from `wait_for_completion` + and the clock runs the session to its scripted `timeout` verdict instead — + proven red once, then restored. + """ + adapter, clock = _timeout_clock_adapter(tmp_path, monkeypatch) + request = _lodge_stop_request(adapter, "hard") + + def advance(call_n): + clock["mono"] += 11.0 # only reached if the abort arm is gone + + adapter.watcher = _ScriptedWatcher([], on_call=advance) + + result = adapter.wait_for_completion(_dev_handle(), _short_spec(tmp_path)) + + assert result.status == "aborted" + assert result.result_json is None # an abort is never a completion path + assert adapter.watcher.calls == 0 # aborted before the first event wait + fired = [ln for ln in _lifecycle_lines(adapter) if ln["event"] == "stop-abort-fired"] + assert len(fired) == 1 + # The engine consumes the request when it raises; an adapter that unlinked it + # would leave the engine unable to attribute the stop. + assert request.is_file() + + +def test_wait_polls_the_hard_stop_channel_after_the_event_wait_too(tmp_path, monkeypatch): + """The arm at the top of the loop is not enough by itself. Between it and its + next run sit the loop's own 5s wait *and* whichever dispatch leg the event + selects — a `_window_alive` or `send_text` bounded only by TMUX_TIMEOUT_S (30s), + or a `_result_json(wait=True)` that waits RESULT_GRACE_S (15s) for an artifact. + That last one outlasts `stop_run`'s 10s grace window on a perfectly healthy box. + Polling again straight after the wait leaves at most one leg between two checks. + + The request is lodged *during* the event wait, so it is absent at the + top-of-loop check and present immediately after — the exact interval this + second poll exists to cover. + + It does not make the interval unconditionally short, and the prose no longer + claims it does: an in-flight subprocess cannot be interrupted from this thread, + so a leg that outlasts the window still degrades to the force-kill backstop. + + Ablation: delete the second `_hard_stop_requested()` arm (the one just below + `watcher.wait_for`) -> `_window_alive` is called, because the loop enters the + `event is None` dispatch leg and only notices the request on its next + iteration. The verdict stays `aborted` either way, which is exactly why the + dispatch spy is the assertion carrying the proof and the status is not.""" + adapter, _clock = _timeout_clock_adapter(tmp_path, monkeypatch) + alive_calls: list[int] = [] + adapter._window_alive = lambda handle: (alive_calls.append(1), True)[1] + + lodged: list[str] = [] + + def _lodge_during_the_wait(_call_n): + if not lodged: + lodged.append("hard") + _lodge_stop_request(adapter, "hard") + + adapter.watcher = _ScriptedWatcher([], on_call=_lodge_during_the_wait) + + result = adapter.wait_for_completion(_dev_handle(), _short_spec(tmp_path)) + + assert result.status == "aborted" + assert lodged == ["hard"] # the interleave really happened + assert adapter.watcher.calls == 1 # caught on the same iteration, not the next + assert alive_calls == [] # never entered the dispatch leg below the wait + + +def _lodge_owner_stop_request(tmp_path, mode: str) -> Path: + """Lodge a stop request in a *different* run dir and publish it as the owning + run, the way `stop ` reaches a nested auto-sweep child: the request + lands in the parent's dir while the child's own stays empty.""" + owner = tmp_path / ".bmad-loop" / "runs" / "parent-run" + owner.mkdir(parents=True, exist_ok=True) + (owner / runs.STOP_REQUEST_FILE).write_text( + json.dumps({"requested_at": "2026-08-22T00:00:00", "mode": mode}), encoding="utf-8" + ) + return owner + + +def test_wait_aborts_on_owning_runs_hard_stop_request(tmp_path, monkeypatch): + """A nested auto-sweep child aborts on the *parent's* hard request (#319). + + The child mints its own run dir, so `stop ` writes a file this + adapter would otherwise never read — and on native Windows, where the shared + SIGTERM cannot land, that left the parent stop force-killing blind. The poll now + reads the owning run's channel too. + + Ablation: drop the owner leg from `_hard_stop_requested` and the clock runs the + session to its scripted `timeout` verdict instead.""" + adapter, clock = _timeout_clock_adapter(tmp_path, monkeypatch) + owner = _lodge_owner_stop_request(tmp_path, "hard") + assert not (adapter.run_dir / runs.STOP_REQUEST_FILE).exists() # child's own is empty + + def advance(call_n): + clock["mono"] += 11.0 # only reached if the owner leg is gone + + adapter.watcher = _ScriptedWatcher([], on_call=advance) + + token = runs.set_owner_run_dir(owner) + try: + result = adapter.wait_for_completion(_dev_handle(), _short_spec(tmp_path)) + finally: + runs.reset_owner_run_dir(token) + + assert result.status == "aborted" + assert result.result_json is None + assert adapter.watcher.calls == 0 + # The child never consumes the parent's file — the parent's own hard arm must + # still find it to record and attribute the stop. + assert (owner / runs.STOP_REQUEST_FILE).is_file() + + +def test_wait_ignores_owning_runs_graceful_stop_request(tmp_path, monkeypatch): + """The mode-exact twin of the owner leg: graceful already suppresses a child + sweep from *starting*, and letting one already in flight finish is what graceful + means — so a graceful request on the owning run must not abort this session. + + Ablation: widen the owner leg to `is not None` and this reddens alone.""" + adapter, clock = _timeout_clock_adapter(tmp_path, monkeypatch) + owner = _lodge_owner_stop_request(tmp_path, "graceful") + + def advance(call_n): + clock["mono"] += 11.0 + + adapter.watcher = _ScriptedWatcher([], on_call=advance) + + token = runs.set_owner_run_dir(owner) + try: + result = adapter.wait_for_completion(_dev_handle(), _short_spec(tmp_path)) + finally: + runs.reset_owner_run_dir(token) + + assert result.status == "timeout" # ran on, exactly as with no request at all + + +def test_hard_stop_requested_falls_back_to_own_dir_outside_any_run(tmp_path, monkeypatch): + """With no owning run published — a standalone adapter, as in probes and most + tests — the predicate is its own dir alone, and answers without raising.""" + adapter, _clock = _timeout_clock_adapter(tmp_path, monkeypatch) + assert runs.owner_run_dir() is None + assert adapter._hard_stop_requested() is False + _lodge_stop_request(adapter, "hard") + assert adapter._hard_stop_requested() is True + + +def test_wait_ignores_graceful_stop_request(tmp_path, monkeypatch): + """Graceful means *finish the in-flight item*, so a graceful request pending + on the same channel must not touch a running session — only ``hard`` aborts. + Since every pre-#319 (modeless) body reads graceful, this is the back-compat + pin for the in-session poll as well. + + Ablation: widen the adapter's check to any pending request (drop the + ``== "hard"`` comparison in `_hard_stop_requested`) and this test reddens + with an `aborted` verdict. + """ + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + monkeypatch.setattr(generic, "RESULT_POLL_S", 0.0) + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: True + _lodge_stop_request(adapter, "graceful") + (impl / "spec-3-1-foo.md").write_text( + "---\nstatus: done\n---\n\n## Auto Run Result\n\nStatus: done\n" + ) + adapter.watcher = _ScriptedWatcher([_stop_event("3-1-dev-1", "sess", "/t.jsonl")]) + + result = adapter.wait_for_completion(_dev_handle(), _dev_spec(tmp_path)) + + assert result.status == "completed" + assert result.result_json["status"] == "done" + assert not _lifecycle_lines(adapter) or not [ + ln for ln in _lifecycle_lines(adapter) if ln["event"] == "stop-abort-fired" + ] + + # ------------------------------ mid-session token-budget guard (#158) # # The wait loop samples cumulative weighted usage on the heartbeat cadence and @@ -2470,6 +2677,32 @@ def test_post_kill_reconcile_no_artifact_keeps_stall(tmp_path): assert adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), original) is original +def test_post_kill_reconcile_rescues_aborted(tmp_path): + """An operator's hard stop (#319) kills the window mid-wait, so a Stop event + that had already landed is never read — the same lost-vouching problem + `stalled` and `timeout` have, settled by the same trust model. + + The upgrade to `completed` does NOT resume the run: the engine re-reads the + hard-stop file after saving the rescued session and stops there. So this + rescue records the finished work *and* the stop is still honored — the pair + is pinned engine-side by + `test_engine.py::test_hard_stop_after_completed_session_stops_before_next_leg`. + + Ablation: drop `"aborted"` from the rescue tuple and the verdict stands + unrescued. + """ + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + (impl / "spec-3-1-foo.md").write_text(_DONE_SPEC) + result = adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), _unvouched("aborted")) + assert result.status == "completed" + assert result.result_json["status"] == "done" + assert result.result_json["post_kill_reconciled"] is True + # the abort verdict's identity is preserved on the rescued result + assert result.session_id == "sess" + assert result.transcript_path == "/t.jsonl" + + def test_post_kill_reconcile_ignores_pre_launch_artifact(tmp_path): """The launch floor still applies: a terminal spec predating this session is a stale prior artifact, not evidence this session finished.""" @@ -4734,6 +4967,29 @@ def test_proof_of_work_gates_post_kill_reconcile(tmp_path, monkeypatch): assert rescued.result_json["post_kill_reconciled"] is True +def test_proof_of_work_gates_the_aborted_rescue(tmp_path, monkeypatch): + """The #261 gate covers the abort leg (#319) exactly as it covers the others: + a session a hard stop killed before it did anything produced nothing, so a + qualifying artifact on disk is not its output and the `aborted` verdict + stands. With proof-of-work the same artifact rescues it. + + Ablation: delete the `_produced_work` gate in `_post_kill_reconcile` and the + first arm reddens with a `completed` rescue. + """ + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + adapter._window_alive = lambda handle: False + (impl / "spec-3-1-foo.md").write_text(_DONE_SPEC) + _pane_log(adapter, "3-1-dev-1", 0) + aborted = SessionResult(status="aborted") + assert adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), aborted) is aborted + + _pane_log(adapter, "3-1-dev-1", generic.PROOF_OF_WORK_MIN_LOG_BYTES + 1) + rescued = adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), aborted) + assert rescued.status == "completed" + assert rescued.result_json["post_kill_reconciled"] is True + + def test_proof_of_work_journals_the_refusal(tmp_path, monkeypatch): """The refusal is observable — a silent downgrade would be its own #261.""" adapter, impl = make_dev_adapter(tmp_path) diff --git a/tests/test_opencode_http.py b/tests/test_opencode_http.py index 24c60c90d..a0492cf59 100644 --- a/tests/test_opencode_http.py +++ b/tests/test_opencode_http.py @@ -24,6 +24,7 @@ import pytest from conftest import write_script_launcher +from bmad_loop import runs from bmad_loop.adapters import generic, opencode_http from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec from bmad_loop.adapters.generic import BUDGET_NUDGE_TEXT, NUDGE_TEXT, STALL_NUDGE_TEXT @@ -2034,6 +2035,185 @@ def advance(): assert ticks["n"] == 3 # same tick count as an untouched wall clock +# ---------------------------- in-session hard-stop poll (#319) +# +# Contract parity: tests/test_generic_tmux.py carries the identically named pair +# over the tmux transport. `bmad-loop stop` lodges a mode-aware stop-request.json +# before it signals; the wait loop reads it twice per iteration and returns the +# non-completion `aborted` verdict, cancelling the in-flight HTTP turn exactly as +# the timeout arm does. The adapter never unlinks the file — the engine consumes +# it, and must still see it to attribute the stop. + + +class _AbortRecordingClient: + """Minimal opencode HTTP client stand-in: records every POST path so a test + can prove the abort really went out, and answers the usage GET + `_capture_usage` makes on the way out.""" + + def __init__(self): + self.posts: list[str] = [] + + def get(self, path): + class _Resp: + status_code = 200 + + @staticmethod + def json(): + return [{"info": {"role": "assistant", "tokens": {"input": 10, "output": 5}}}] + + return _Resp() + + def post(self, path): + self.posts.append(path) + + class _Resp: + status_code = 200 + + return _Resp() + + def close(self): + pass + + +def _lodge_stop_request(adapter, mode: str) -> Path: + """Lodge a stop request of ``mode`` on this run's control-file channel, as + ``bmad-loop stop`` does.""" + adapter.run_dir.mkdir(parents=True, exist_ok=True) + path = adapter.run_dir / runs.STOP_REQUEST_FILE + path.write_text( + json.dumps({"requested_at": "2026-08-22T00:00:00", "mode": mode}), encoding="utf-8" + ) + return path + + +def test_wait_aborts_on_hard_stop_request(tmp_path, monkeypatch): + """A hard stop pending on the channel ends the wait on its very next + iteration with the non-completion `aborted` verdict, and takes the timeout + arm's exit shape: `_abort` cancels the in-flight turn, then `_capture_usage` + reads usage back before teardown. Without the abort the HTTP turn would keep + running server-side until the session is torn down. + + The verdict fires before the loop ever polls its event queue, so the + steerable clock never advances: the pass is deterministic, not a race. + + Ablation: delete the `_hard_stop_requested()` arm from `wait_for_completion` + and the clock runs the session to its scripted `timeout` verdict instead — + proven red once, then restored. + """ + adapter = make_adapter(tmp_path) + clock = _install_clock(monkeypatch) + (adapter.tasks_dir / "t-1").mkdir(parents=True) + request = _lodge_stop_request(adapter, "hard") + + ticks = {"n": 0} + + def advance(): + ticks["n"] += 1 + clock["mono"] += 11.0 # only reached if the abort arm is gone + + sess = _timeout_driven_session(adapter, advance) + sess.client = _AbortRecordingClient() + + result = adapter.wait_for_completion( + SessionHandle(task_id="t-1", native_id="ses_1"), _timeout_spec(tmp_path) + ) + + assert result.status == "aborted" + assert result.result_json is None # an abort is never a completion path + assert ticks["n"] == 0 # aborted before the first event-queue poll + assert sess.client.posts == ["/session/ses_1/abort"] + assert result.transcript_path == str(adapter.tasks_dir / "t-1" / "messages.json") + fired = [ln for ln in _lifecycle_lines(adapter) if ln["event"] == "stop-abort-fired"] + assert len(fired) == 1 + # The engine consumes the request when it raises; an adapter that unlinked it + # would leave the engine unable to attribute the stop. + assert request.is_file() + + +def test_wait_polls_the_hard_stop_channel_after_the_event_queue_too(tmp_path, monkeypatch): + """The arm at the top of the loop is not enough by itself. Below the event-queue + wait sit the dispatch legs — `_probe_completion`'s two GETs (not throttled: once + a turn goes quiet past SILENCE_THRESHOLD_S they run every tick), a + `_session_status` GET, or `_result_json(wait=True)`'s grace wait — each bounded + only by the client's own timeouts. One iteration can outlast `stop_run`'s 10s + grace window, and on native Windows that is the force-kill this issue exists to + avoid. A second poll straight after the queue wait leaves at most one leg between + two checks. + + The request is lodged *inside* the queue poll, so it is absent at the top-of-loop + check and present immediately after — the interval the second poll covers. + + This does not make the interval unconditionally short, and the prose no longer + claims it does: an in-flight socket read cannot be interrupted from this thread. + + Ablation: delete the second `_hard_stop_requested()` arm (below the queue wait) + -> `_probe_completion` is called, because the loop enters the silent-turn + dispatch leg and only notices the request on its next iteration. The verdict + stays `aborted` either way, which is why the probe spy carries the proof and the + status does not.""" + adapter = make_adapter(tmp_path) + clock = _install_clock(monkeypatch) + (adapter.tasks_dir / "t-1").mkdir(parents=True) + adapter.silence_threshold_s = 0.0 # the quiet-turn leg fires on every tick + + probes: list[int] = [] + monkeypatch.setattr( + type(adapter), "_probe_completion", lambda self, sess: (probes.append(1), False)[1] + ) + + lodged: list[str] = [] + + def advance(): + if not lodged: # absent at the top-of-loop check, present right after + lodged.append("hard") + _lodge_stop_request(adapter, "hard") + clock["mono"] += 11.0 # makes the turn read as silent below the wait + + sess = _timeout_driven_session(adapter, advance) + sess.client = _AbortRecordingClient() + + result = adapter.wait_for_completion( + SessionHandle(task_id="t-1", native_id="ses_1"), _timeout_spec(tmp_path) + ) + + assert lodged == ["hard"] # the interleave really happened + assert result.status == "aborted" + assert sess.client.posts == ["/session/ses_1/abort"] # took the abort exit shape + assert probes == [] # never entered the dispatch leg below the wait + + +def test_wait_ignores_graceful_stop_request(tmp_path, monkeypatch): + """Graceful means *finish the in-flight item*, so a graceful request pending + on the same channel must not touch a running session — only `hard` aborts. + Every pre-#319 (modeless) body reads graceful, so this pins the back-compat + case for the HTTP transport too. + + Ablation: widen the adapter's check to any pending request (drop the + ``== "hard"`` comparison in `_hard_stop_requested`) and this test reddens + with an `aborted` verdict. + """ + adapter = make_adapter(tmp_path) + clock = _install_clock(monkeypatch) + (adapter.tasks_dir / "t-1").mkdir(parents=True) + _lodge_stop_request(adapter, "graceful") + + def advance(): + clock["mono"] += 11.0 + + sess = _timeout_driven_session(adapter, advance) + sess.client = _AbortRecordingClient() + + result = adapter.wait_for_completion( + SessionHandle(task_id="t-1", native_id="ses_1"), _timeout_spec(tmp_path) + ) + + # the loop ran on to its scripted verdict rather than aborting + assert result.status == "timeout" + events = [ln["event"] for ln in _lifecycle_lines(adapter)] + assert "stop-abort-fired" not in events + assert events.count("timeout-fired") == 1 + + # -------------------------- launch-stall transport parity (#411/#470) # # Two-way contract-parity link: tests/test_generic_tmux.py carries identically diff --git a/tests/test_runs.py b/tests/test_runs.py index be3871def..d0671e81a 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -1,11 +1,14 @@ """Run-directory helper tests.""" +import contextlib import json import os import re import subprocess import sys import tarfile +import threading +import time from pathlib import Path from unittest import mock @@ -59,10 +62,11 @@ class _FakeHost(ProcessHost): are inherited, so these tests exercise the production decision table instead of a hand-copied mirror that could silently drift.""" - def __init__(self, *, alive, identity=1.0, on_terminate=None): + def __init__(self, *, alive, identity=1.0, on_terminate=None, on_force_kill=None): self._alive = alive self._identity = identity self.on_terminate = on_terminate + self.on_force_kill = on_force_kill self.terminated: list[int] = [] self.force_killed: list[int] = [] @@ -73,6 +77,8 @@ def terminate(self, pid): def force_kill(self, pid): self.force_killed.append(pid) + if self.on_force_kill is not None: + self.on_force_kill(pid) def is_alive(self, pid): return self._alive() if callable(self._alive) else self._alive @@ -407,13 +413,23 @@ def test_stop_run_dead_pid_falls_back(tmp_path, monkeypatch): def test_stop_run_signals_live_process(tmp_path, monkeypatch): + # Bound the grace window: this child is settled either way, and the default + # 10s is pure dead time here. It is also burned on *every* platform, not just + # win32 — the exited child stays an unreaped zombie while this test holds its + # Popen handle, and a zombie answers the POSIX `os.kill(pid, 0)` liveness + # probe as alive. + monkeypatch.setattr(runs, "_STOP_WAIT_S", 2.0) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.05) monkeypatch.setattr(runs, "kill_session", lambda _rid: None) run_dir = _make_state_run(tmp_path, "r1") proc = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) try: (run_dir / "engine.pid").write_text(str(proc.pid)) assert runs.stop_run(run_dir) is True - # the process received SIGTERM and is gone + # the process is gone. On POSIX it took the SIGTERM; on win32 `taskkill` + # without /F posts a WM_CLOSE a console child has no window to receive, so + # there it is force-killed after the bounded wait instead. Either way + # stop_run settles the run — which is the whole point of the file channel. assert proc.poll() is not None or proc.wait(timeout=5) is not None assert load_state(run_dir).stopped is True finally: @@ -422,6 +438,304 @@ def test_stop_run_signals_live_process(tmp_path, monkeypatch): proc.wait(timeout=10) +def test_stop_run_lodges_hard_request_before_signalling(tmp_path, monkeypatch): + """The hard request is on disk *before* terminate() is called. That ordering is + the guarantee: an engine that is signal-deaf, or that dies to the signal before + reading anything, can never exit having missed a request written only after it + was signalled. Read from inside the host at terminate time so the assertion + cannot be satisfied by a write that lands later.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + + seen: list[str | None] = [] + + def _read_at_terminate(_pid): + seen.append(runs.read_stop_request_mode(run_dir)) + st = load_state(run_dir) # emulate the engine honoring it, then exiting + st.stopped = True + save_state(run_dir, st) + + host = _FakeHost(alive=False, identity=100.0, on_terminate=_read_at_terminate) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + assert runs.stop_run(run_dir) is True + assert seen == ["hard"] + assert host.force_killed == [] # the engine settled it — no escalation + + +def test_stop_run_still_signals_when_the_lodge_fails(tmp_path, monkeypatch): + """A run dir that rejects the write must not cost the signal path too. + + The lodge goes first (see the test above), so before it was guarded an OSError + escaped `stop_run` ahead of `terminate` and left alive a POSIX run the pre-#319 + code would have killed — a stop that does nothing at all, replacing one that + worked. Reachable without exotic setup: every session tees its pane into + `run_dir/logs/`, so a long run can fill the very directory the request must be + written to, and then `stop` is what fails. + + Degrading is right *here* specifically because the hard stop is delivered two + ways at once. `request_graceful_stop` has only the file, so its write still + raises — that asymmetry is the point, and `test_request_graceful_stop_*` holds + the other side.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + + def _enospc(_run_dir, _mode): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(runs, "_write_stop_request", _enospc) + host = _FakeHost(alive=False, identity=100.0) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + + assert runs.stop_run(run_dir) is True + assert host.terminated == [4242] # the signal went out despite the failed lodge + assert load_state(run_dir).stopped is True # and the run is settled + + +def test_stop_run_refusal_says_nothing_is_pending_when_the_lodge_failed(tmp_path, monkeypatch): + """The force-kill refusal justifies itself by the file still being lodged — "the + only channel left that can stop it". When the lodge failed that sentence is + false: nothing is pending, and the operator is declining a force-kill on top of + a request that was never written. The message has to say so, or they wait on a + stop that can never arrive.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 0.0) # expire the grace window at once + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + + def _enospc(_run_dir, _mode): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(runs, "_write_stop_request", _enospc) + # Alive throughout, and the identity goes unreadable right after the stop-time + # read: the grace window expires and the pid-reuse guard then refuses to kill. + identities = iter([100.0] + [None] * 50) + host = _FakeHost(alive=True, identity=lambda: next(identities)) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + + with pytest.raises(runs.StopRunError, match="could not be written"): + runs.stop_run(run_dir) + assert host.force_killed == [] # still refuses to kill an unverifiable pid + + +def test_stop_run_refuses_when_the_lodge_failed_and_the_signal_was_refused(tmp_path, monkeypatch): + """Neither channel delivered: nothing written, nothing signalled, nothing proved. + + The refusal above only fires from inside the `pid is not None` arm. A `terminate` + we were *refused* clears `pid` and skips that arm entirely, so this combination + used to reach the fallback and report success — writing `stopped=True` and + stamping `fallback=True` over a run whose engine may still be mutating the + project, with no request on disk for it to honor. + + Not a regression: on the merge-base every refused signal ended here, because + `stop_run` cleared the request as its first statement. What does not survive is + the *justification* for reporting success — that the request stays lodged, so the + stop is still in flight. When the lodge failed, nothing is in flight. + + Ablation: delete the `engine_may_live and not lodged` branch -> this reddens on + the `pytest.raises` itself ("DID NOT RAISE"), not on the asserts below it, which + are never reached; `stop_run` falls through to the fallback and returns True. The + twin below is the position axis, and stays green under this one.""" + killed = [] + monkeypatch.setattr(runs, "kill_session", lambda rid: killed.append(rid)) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + def _enospc(_run_dir, _mode): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(runs, "_write_stop_request", _enospc) + host = _FakeHost(alive=True, identity=123.0, on_terminate=_raise(PermissionError())) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + + with pytest.raises(runs.StopRunError, match="no stop is pending"): + runs.stop_run(run_dir) + assert killed == ["r1"] # the session backstop still runs, ahead of the refusal + assert load_state(run_dir).stopped is False # never claimed a stop it did not make + journal = (run_dir / "journal.jsonl").read_text() + assert "run-stop-undelivered" in journal # the attempt is on the record + assert '"fallback": true' not in journal # and not as a completed stop + + +def test_stop_run_still_trusts_an_engine_written_stop_when_the_lodge_failed(tmp_path, monkeypatch): + """The placement ablation for the refusal above: it must sit *after* the + `state.stopped` return, not before it. + + Same failed lodge and same refused signal, but the engine already honored an + earlier stop and recorded it. `stop` is then reporting a stop that genuinely + happened, so it must return True — raising here would turn a settled run into a + CLI failure on the operator's second `stop`. + + Ablation: move the refusal ahead of the `if state.stopped:` branch -> this test + raises while the one above still passes. Both are needed: the pair pins the + branch's presence *and* its position.""" + killed = [] + monkeypatch.setattr(runs, "kill_session", lambda rid: killed.append(rid)) + run_dir = _make_state_run(tmp_path, "r1") + st = load_state(run_dir) + st.stopped = True # an earlier stop the engine honored and recorded itself + save_state(run_dir, st) + (run_dir / "engine.pid").write_text("4242 123.0") + + def _enospc(_run_dir, _mode): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(runs, "_write_stop_request", _enospc) + host = _FakeHost(alive=True, identity=123.0, on_terminate=_raise(PermissionError())) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + + assert runs.stop_run(run_dir) is True + assert killed == ["r1"] + + +def test_stop_run_stops_sigterm_immune_child_via_stop_request_file(tmp_path, monkeypatch): + """THE #319 acceptance test: a stand-in engine that cannot be reached by signal + stops *itself* off the control file, and stop_run confirms rather than blindly + force-killing it. + + The child ignores SIGTERM, modelling a native-Windows engine — which never + receives an inter-process SIGTERM at all, and whose `taskkill` "graceful" step + posts a WM_CLOSE that a console process has no window to receive. So the only + channel that can reach it is the `mode: hard` request stop_run lodges before + signalling. It polls for that file, marks the run stopped exactly as the + engine's own handler does, and exits 0. Before #319 this was unreachable: every + Windows stop burned the full grace window into a blind force-kill and `stopped` + was written by the external fallback, so the engine-is-single-writer invariant + held only by fallback. + + The reaper thread clears the exited child so the liveness probe stops reading it + as alive: `os.kill(pid, 0)` answers True for an unreaped zombie, and this test + holds the Popen handle. Production never has that problem — the engine is not + the CLI's child — so without the reaper the assertions below would still pass + but take the whole (shortened) wait window, hiding the speed this fixes. + + Ablation: with `_write_stop_request(run_dir, "hard")` deleted from stop_run, the + child never sees a request, burns the wait, and is SIGKILLed — returncode -9 + instead of 0, plus a `fallback: true` journal entry. Run once against the + ablated source, confirmed failing on both, then restored. + """ + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 5.0) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.02) + run_dir = _make_state_run(tmp_path, "r1") + request = run_dir / runs.STOP_REQUEST_FILE + state_path = run_dir / "state.json" + ready = run_dir / "child-ready" + + # A stand-in engine: deaf to SIGTERM, awake to the control file. Marks the run + # stopped itself — the engine is the single writer of `stopped`, and this test + # exists to prove that stays true when no signal can be delivered. The ready + # file is published only after the handler is installed; see the wait below. + child = ( + "import json, pathlib, signal, sys, time\n" + "try:\n" + " signal.signal(signal.SIGTERM, signal.SIG_IGN)\n" + "except (AttributeError, OSError, ValueError):\n" + " pass\n" # a platform that refuses the handler still can't reach us + "req, state = pathlib.Path(sys.argv[1]), pathlib.Path(sys.argv[2])\n" + "pathlib.Path(sys.argv[3]).write_text('ready')\n" + "deadline = time.monotonic() + 60\n" + "while time.monotonic() < deadline:\n" + " if req.exists():\n" + " d = json.loads(state.read_text())\n" + " d['stopped'] = True\n" + " state.write_text(json.dumps(d))\n" + " sys.exit(0)\n" + " time.sleep(0.05)\n" + "sys.exit(3)\n" # never saw a request — the outcome the ablation produces + ) + proc = subprocess.Popen( + [sys.executable, "-c", child, str(request), str(state_path), str(ready)] + ) + + def _reap(): + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait(timeout=90) + + reaper = threading.Thread(target=_reap, daemon=True) + reaper.start() + try: + # Wait for SIG_IGN to be installed before stopping. Interpreter startup is + # tens of milliseconds and stop_run signals immediately, so without this the + # SIGTERM lands on a child still importing and kills it by default action — + # rc -15, and the test would be measuring the race instead of the channel. + deadline = time.monotonic() + 60 + while time.monotonic() < deadline and not ready.exists(): + time.sleep(0.02) + assert ready.exists(), "stand-in engine never became SIGTERM-immune" + + (run_dir / "engine.pid").write_text(str(proc.pid)) + assert runs.stop_run(run_dir) is True + reaper.join(timeout=30) + # the child exited ITSELF: rc 0, not -15 (SIGTERM), -9 (SIGKILL) or a + # taskkill /F status. This is the assertion the whole issue is about. + assert proc.returncode == 0 + assert load_state(run_dir).stopped is True + # ...and stop_run trusted it, rather than marking the run stopped behind it + journal = run_dir / "journal.jsonl" + assert not journal.exists() or "fallback" not in journal.read_text() + assert runs.read_stop_request_mode(run_dir) is None # request consumed + finally: + if proc.poll() is None: + proc.kill() + proc.wait(timeout=10) + + +def test_stop_run_fallback_clears_hard_request(tmp_path, monkeypatch): + """On the mark-stopped fallback nothing is left alive to consume the request, so + stop_run discards what it lodged. A file outliving the run it asked to stop is a + trap: the next resume would find it and stop again at the first item.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") # no engine.pid -> straight to fallback + assert runs.stop_run(run_dir) is True + assert load_state(run_dir).stopped is True + assert runs.read_stop_request_mode(run_dir) is None + assert '"fallback": true' in (run_dir / "journal.jsonl").read_text() + + +def test_stop_run_engine_confirmed_leaves_nothing_pending(tmp_path, monkeypatch): + """When the engine confirms the stop itself the request is consumed too. The + engine normally clears it on the way out; this is the belt-and-braces half, and + it is what keeps a confirmed stop from stranding a request on disk.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + + def _mark_stopped(_pid): + st = load_state(run_dir) + st.stopped = True + save_state(run_dir, st) + + host = _FakeHost(alive=False, identity=100.0, on_terminate=_mark_stopped) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + assert runs.stop_run(run_dir) is True + assert runs.read_stop_request_mode(run_dir) is None + journal = run_dir / "journal.jsonl" + assert not journal.exists() or "fallback" not in journal.read_text() + + +def test_stop_run_refusal_leaves_hard_request_lodged(tmp_path, monkeypatch): + """StopRunError is the one path that leaves the request on disk. We refused to + force-kill a pid whose identity we can no longer verify — but if it *is* still + our engine, the file is now the only channel that can stop it. Clearing it here + would retract the operator's request while simultaneously declining to enforce + it, leaving a live run nobody asked to keep running.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 0.05) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.01) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + identities = iter([123.0, 999.0]) # identity changes mid-grace -> possible reuse + host = _FakeHost(alive=True, identity=lambda: next(identities)) + monkeypatch.setattr(runs, "get_process_host", lambda: host) + with pytest.raises(runs.StopRunError): + runs.stop_run(run_dir) + assert host.force_killed == [] + assert runs.read_stop_request_mode(run_dir) == "hard" + + def test_stop_run_respects_engine_written_stopped(tmp_path, monkeypatch): """When a live engine exits having already marked the run stopped, stop_run trusts it and does not re-journal a fallback entry.""" @@ -445,6 +759,162 @@ def _mark_stopped(_pid): assert not journal.exists() or "fallback" not in journal.read_text() +def test_stop_run_does_not_stamp_fallback_on_an_already_stopped_run(tmp_path, monkeypatch): + """`fallback=True` says this tool completed the stop from outside. An engine that + honored a stop and exited must never collect that stamp on a later `stop`. + + The check that trusts an engine-written `stopped` used to sit *inside* the + `pid is not None` arm, so every path that clears the pid early skipped it and + fell straight through to the append: a pid that is no longer ours, a `terminate` + that raced the exit into `ProcessLookupError`, or a refusal that could not verify + it. This case needs no race at all — `stopped` is set and `finished` is not, so + the guard at the top of `stop_run` does not fire — which is why the check now + sits outside that arm, where every path reaches it. + + The session backstop must still run: an engine that honored the stop and died + before tearing its window down leaks the session exactly like one we killed. + + Ablation: delete the hoisted `if state.stopped:` branch -> a second `run-stop` + carrying `"fallback": true` is appended and the last assert fails.""" + killed = [] + monkeypatch.setattr(runs, "kill_session", lambda rid: killed.append(rid)) + run_dir = _make_state_run(tmp_path, "r1") + st = load_state(run_dir) + st.stopped = True # an earlier stop the engine honored and recorded itself + save_state(run_dir, st) + (run_dir / "engine.pid").write_text("4242 123.0") + + host = _FakeHost(alive=False) # the engine is gone + monkeypatch.setattr(runs, "get_process_host", lambda: host) + + assert runs.stop_run(run_dir) is True + assert host.terminated == [] and host.force_killed == [] # nothing left to signal + assert killed == ["r1"] # the session backstop still runs + journal = run_dir / "journal.jsonl" + assert not journal.exists() or '"fallback": true' not in journal.read_text() + + +def _raise(exc): + """A `_FakeHost` hook that refuses the kill instead of performing it.""" + + def _hook(_pid): + raise exc + + return _hook + + +def test_stop_run_keeps_the_hard_request_when_the_signal_is_refused(tmp_path, monkeypatch): + """A `terminate` we were *refused* leaves the lodged request on disk. + + The pid was `alive_and_ours` a moment earlier and we could not signal it, so it + may well still be running — and on native Windows the control file is then the + only channel that can still stop it. Discarding it here would retract the repair + #319 exists to deliver, while reporting the run stopped. Contrast the + ProcessLookupError twin below: that one is proof of death, so the file goes.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + host = _FakeHost(alive=True, identity=123.0, on_terminate=_raise(PermissionError())) + _use_host(monkeypatch, host) + + assert runs.stop_run(run_dir) is True + assert runs.read_stop_request_mode(run_dir) == "hard" # still lodged, still honorable + assert host.force_killed == [] # unsignalable — never escalated to a kill + assert load_state(run_dir).stopped is True + + +def test_stop_run_discards_the_hard_request_when_the_signal_proves_it_gone(tmp_path, monkeypatch): + """The mode-exact twin, and the second ablation axis: `ProcessLookupError` from + `terminate` says the process is *gone*, so nothing is left to consume the request + and leaving it would trap the next resume. Collapsing the two excepts back into + one reddens exactly one of this pair, whichever way it is collapsed.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + host = _FakeHost(alive=True, identity=123.0, on_terminate=_raise(ProcessLookupError())) + _use_host(monkeypatch, host) + + assert runs.stop_run(run_dir) is True + assert not runs.graceful_stop_requested(run_dir) # provably dead — discarded + assert load_state(run_dir).stopped is True + + +def test_stop_run_keeps_the_hard_request_when_the_force_kill_is_refused(tmp_path, monkeypatch): + """A `force_kill` that raises `PermissionError` is the opposite of a race: the + process is there and we were refused. The request stays lodged.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 0.05) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.01) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + host = _FakeHost(alive=True, identity=123.0, on_force_kill=_raise(PermissionError())) + _use_host(monkeypatch, host) + + assert runs.stop_run(run_dir) is True + assert host.force_killed == [4242] # we did try + assert runs.read_stop_request_mode(run_dir) == "hard" # and kept the channel + + +def test_stop_run_keeps_the_hard_request_when_a_clean_force_kill_did_not_take( + tmp_path, monkeypatch +): + """A force-kill that returns cleanly is not a death certificate. + + `WindowsProcessHost.force_kill` shells `taskkill /F /T` with `check=False`, so a + refused kill raises nothing at all — and win32 is the platform this channel + exists for. The engine is re-probed after the kill settles, and a survivor keeps + its request.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 0.05) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.01) + monkeypatch.setattr(runs, "_KILL_CONFIRM_S", 0.05) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + # never dies: the silent-taskkill-failure shape + host = _FakeHost(alive=True, identity=123.0) + _use_host(monkeypatch, host) + + assert runs.stop_run(run_dir) is True + assert host.force_killed == [4242] + assert runs.read_stop_request_mode(run_dir) == "hard" + + +def test_stop_run_discards_the_hard_request_once_the_force_kill_confirms(tmp_path, monkeypatch): + """The settle window's positive control: a pid that disappears once the kill + lands reads as dead, so the request is discarded rather than stranded on the + ordinary wedged-engine path. Without the settle loop an immediate sample of a + not-yet-reaped pid would keep the file here and trap the next resume.""" + monkeypatch.setattr(runs, "kill_session", lambda _rid: None) + monkeypatch.setattr(runs, "_STOP_WAIT_S", 0.05) + monkeypatch.setattr(runs, "_STOP_POLL_S", 0.01) + monkeypatch.setattr(runs, "_KILL_CONFIRM_S", 1.0) + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 123.0") + + lingering = {"ticks": 3} # still in the pid table for a few probes after the kill + + def _alive(): + if killed["yes"] and lingering["ticks"] > 0: + lingering["ticks"] -= 1 + return not killed["yes"] or lingering["ticks"] > 0 + + killed = {"yes": False} + + def _on_force_kill(_pid): + killed["yes"] = True + + host = _FakeHost(alive=_alive, identity=123.0, on_force_kill=_on_force_kill) + _use_host(monkeypatch, host) + + assert runs.stop_run(run_dir) is True + assert host.force_killed == [4242] + assert not runs.graceful_stop_requested(run_dir) # confirmed dead — discarded + + def test_stop_run_force_kills_wedged_engine(tmp_path, monkeypatch): """An engine that ignores SIGTERM past the grace window is force-killed, then marked stopped — as long as its pid identity still matches what we recorded.""" @@ -548,8 +1018,50 @@ def test_request_graceful_stop_writes_file_when_alive(tmp_path, monkeypatch): body = json.loads((run_dir / runs.STOP_REQUEST_FILE).read_text()) assert body["mode"] == "graceful" assert body["requested_at"] # an ISO timestamp is stamped - # written atomically — no staging temp left behind - assert not (run_dir / (runs.STOP_REQUEST_FILE + ".tmp")).exists() + # No sibling left behind. The graceful lodge is an O_CREAT|O_EXCL create written + # in place, so it has no staging temp *by construction* — this asserts the + # absence of stray debris, not the atomicity of a replace. The staging-temp + # guarantee belongs to `_write_stop_request`, which still replaces, and is + # asserted where it is exercised: see + # test_write_stop_request_survives_an_interleaved_concurrent_writer. + assert [p.name for p in run_dir.glob(runs.STOP_REQUEST_FILE + "*")] == [runs.STOP_REQUEST_FILE] + + +def test_write_stop_request_survives_an_interleaved_concurrent_writer(tmp_path, monkeypatch): + """Two `stop` invocations against one run stage at the same time — the only + control file with genuinely concurrent writers. + + With a fixed `.tmp` sibling the second writer's staging file overwrote the + first's, one `os.replace` consumed the single name, and the loser raised + `FileNotFoundError`. On the hard path that aborts `stop_run` *before* it signals, + so a collision between two operators cost the stop entirely. A per-writer + `mkstemp` temp removes the collision: both calls return, the survivor is a + complete body, and neither leaves a staging file behind. + + Patched on both namespaces so reverting `_write_stop_request` to the hand-rolled + `tmp + atomic_replace` still routes through the interleave — that ablation must + redden this test.""" + from bmad_loop import platform_util + + run_dir = _make_state_run(tmp_path, "r1") + real_replace = platform_util.atomic_replace + nested: list[str] = [] + + def _interleave(tmp, target): + if not nested: # inside writer A's replace, run writer B end to end + nested.append("b") + runs._write_stop_request(run_dir, "graceful") + real_replace(tmp, target) + + monkeypatch.setattr(platform_util, "atomic_replace", _interleave) + monkeypatch.setattr(runs, "atomic_replace", _interleave) + + runs._write_stop_request(run_dir, "hard") # writer A — must not raise + + assert nested == ["b"] # the interleave really happened + body = json.loads((run_dir / runs.STOP_REQUEST_FILE).read_text()) + assert body["mode"] == "hard" # A replaced last, so A wins — never a torn body + assert [p.name for p in run_dir.glob(runs.STOP_REQUEST_FILE + "*")] == [runs.STOP_REQUEST_FILE] def test_request_graceful_stop_idempotent_keeps_timestamp(tmp_path, monkeypatch): @@ -587,6 +1099,51 @@ def test_request_graceful_stop_unknown_liveness_is_unverifiable(tmp_path, monkey assert runs.graceful_stop_requested(run_dir) +def test_read_stop_request_mode_matrix(tmp_path): + """The mode reader answers ``None`` for *absent*, and only absent. Every other + state of a file that is present reads "graceful". + + The asymmetry is deliberate and load-bearing: "hard" aborts a live session + mid-flight, so no torn, odd, or unreadable file may be able to produce it. A + misread graceful costs at most one more item before the run stops.""" + run_dir = _make_run(tmp_path, "r1") + path = run_dir / runs.STOP_REQUEST_FILE + + assert runs.read_stop_request_mode(run_dir) is None # nothing pending + + path.write_text('{"requested_at": "now", "mode": "graceful"}', encoding="utf-8") + assert runs.read_stop_request_mode(run_dir) == "graceful" + + path.write_text('{"requested_at": "now", "mode": "hard"}', encoding="utf-8") + assert runs.read_stop_request_mode(run_dir) == "hard" + + # Back-compat pin: every pre-#319 writer — and the fixtures still written by + # hand across this suite — produced a body with no mode at all. It must keep + # reading as the graceful request it was, not fall through to a hard abort. + path.write_text("{}", encoding="utf-8") + assert runs.read_stop_request_mode(run_dir) == "graceful" + + path.write_text('{"mode": "har', encoding="utf-8") # torn mid-write + assert runs.read_stop_request_mode(run_dir) == "graceful" + + path.write_text('["hard"]', encoding="utf-8") # valid JSON, but not an object + assert runs.read_stop_request_mode(run_dir) == "graceful" + + path.write_text('"hard"', encoding="utf-8") # a bare JSON scalar + assert runs.read_stop_request_mode(run_dir) == "graceful" + + path.write_bytes(b"\xff\xfe\x00 not utf-8") # undecodable bytes + assert runs.read_stop_request_mode(run_dir) == "graceful" + + # A read that fails outright, standing in for the win32 sharing violation a + # concurrent atomic_replace raises: a directory in the file's place is an + # OSError on every platform (IsADirectoryError on POSIX, PermissionError on + # win32) and must not be mistaken for absence. + path.unlink() + path.mkdir() + assert runs.read_stop_request_mode(run_dir) == "graceful" + + def test_clear_graceful_stop_removes_or_noops(tmp_path): run_dir = _make_run(tmp_path, "r1") assert runs.clear_graceful_stop(run_dir) is False # nothing pending → no-op, never raises @@ -596,21 +1153,238 @@ def test_clear_graceful_stop_removes_or_noops(tmp_path): assert runs.clear_graceful_stop(run_dir) is False # already gone → no-op again -def test_stop_run_clears_pending_graceful_request(tmp_path, monkeypatch): - """A hard stop supersedes a pending graceful request: the control file is - cleared even on the no-live-engine mark-stopped fallback path, so a later - resume doesn't re-honor the stop the operator escalated past.""" +def test_stop_run_supersedes_pending_graceful_request(tmp_path, monkeypatch): + """A hard stop supersedes a pending graceful request by *overwriting* it, not by + clearing it. The atomic replace escalates the mode in one step, so there is no + instant in which the operator has asked for a stop and nothing at all is pending + for the engine to find. Nothing is left on disk once the run is settled, so a + later resume can't re-honor the stop the operator escalated past.""" monkeypatch.setattr(runs, "kill_session", lambda _rid: None) - run_dir = _make_state_run(tmp_path, "r1") # no engine.pid → mark-stopped fallback - (run_dir / runs.STOP_REQUEST_FILE).write_text("{}") # a graceful request pending + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "old", "mode": "graceful"}', encoding="utf-8" + ) + + seen: list[str | None] = [] + + def _read_at_terminate(_pid): + seen.append(runs.read_stop_request_mode(run_dir)) + st = load_state(run_dir) + st.stopped = True + save_state(run_dir, st) + + host = _FakeHost(alive=False, identity=100.0, on_terminate=_read_at_terminate) + _use_host(monkeypatch, host) assert runs.stop_run(run_dir) is True assert load_state(run_dir).stopped is True - assert not runs.graceful_stop_requested(run_dir) + assert seen == ["hard"] # escalated in place — never a gap with nothing pending + assert not runs.graceful_stop_requested(run_dir) # and nothing pending after # ---------------------------------------------------------------- prune sessions +@pytest.mark.parametrize("lodge_at", ["engine_liveness", "just_before_the_create"]) +def test_request_graceful_stop_cannot_downgrade_a_hard_request_at_any_instant( + tmp_path, monkeypatch, lodge_at +): + """A hard request landing anywhere inside the check -> write window is not + downgraded, and the guarantee holds at the *last* instant, not just an early one. + + `request_graceful_stop` clears its existence check, then spends a pid-file read + and a liveness probe before it lodges — measured at ~1.3ms median on btrfs back + when an fsync sat in there too. The channel is last-writer-wins, so an + unconditional write here silently supersedes the stronger stop and costs the + operator the abort they asked for. `O_CREAT | O_EXCL` fuses the decision to the + write so no interleaving can land between them. + + The two parameters are the point. `engine_liveness` lodges early — a re-read + immediately before the write already catches that one. `just_before_the_create` + lodges from the last statement that runs ahead of the `os.open`, which only real + arbitration catches; a re-read narrows that window but cannot close it. + + Ablation, two axes, and axis 2 is what proves this is not merely re-testing the + re-read it replaced: + 1. Make `_create_stop_request` an unconditional + `_write_stop_request(run_dir, "graceful")` — BOTH parameters redden. + 2. Same, but restore a `read_stop_request_mode(...) == "hard"` guard ahead of + it — `engine_liveness` goes GREEN while `just_before_the_create` stays RED. + Both going green would mean this test measures the old guard, not the new + arbitration.""" + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + lodged: list[str] = [] + + def _lodge_hard() -> None: + if not lodged: # once — the injection points are per-call, not per-test + lodged.append("hard") + runs._write_stop_request(run_dir, "hard") + + if lodge_at == "engine_liveness": + + def _alive(_run_dir): + _lodge_hard() + return "alive" + + monkeypatch.setattr(runs, "engine_liveness", _alive) + else: + # the last statement before the O_EXCL open, so the hard request lands with + # nothing but the create left to run + real_strftime = time.strftime + + def _strftime(fmt, *a): + _lodge_hard() + return real_strftime(fmt, *a) + + monkeypatch.setattr(runs.time, "strftime", _strftime) + monkeypatch.setattr(runs, "engine_liveness", lambda _d: "alive") + + # the same answer a request found at entry gets: a stronger stop already stands + assert runs.request_graceful_stop(run_dir) == "already-pending" + assert lodged == ["hard"] # the interleave really happened + assert runs.read_stop_request_mode(run_dir) == "hard" # not downgraded + + +def test_consume_stop_request_never_removes_a_request_it_did_not_read(tmp_path, monkeypatch): + """The reader-side half of the arbitration. A `stop` escalating to hard while the + engine is consuming must not be deleted unread: a read followed by an unlink + removes whatever answers to the name *now*, which may not be the request whose + mode the caller is about to route on. + + Only this direction can lose anything — the mode lattice is monotone (graceful + refuses to overwrite, hard only ever writes hard), so a stale "hard" read is + still true while a stale "graceful" may not be. + + ⚠️ Re-reading the mode just before the unlink does NOT fix this and measurably + worsens it (164 -> 929 swallowed over 4000 injected races): the extra read + widens the interval the escalation has to land in. Narrowing is not closing. + + Ablation, two axes, and axis 2 is what proves this is not the shape-2 guard + wearing a new hat: + 1. Revert `consume_stop_request` to `read_stop_request_mode` + a + `clear_graceful_stop` unlink -> the FIRST assert fails, returning "hard": + with no take, the mode answered is whatever the name resolves to at read + time, which the escalation has already changed, and the unlink then removes + that one too. Read and consume disagree about which request was handled, + which is the whole defect; the channel is left empty, so the second assert + would fail as well were it reached. + 2. Revert `_create_stop_request` to an unconditional + `_write_stop_request(run_dir, "graceful")`, undoing the writer-side fix, + but keep the atomic take -> this test still PASSES. The two axes redden + disjoint sets, which is the proof the two guards are independent.""" + runs._create_stop_request(tmp_path) # operator: stop --graceful + real = runs._stop_request_mode_of + escalated: list[str] = [] + + def _escalate_then_read(path): + if not escalated: # once — the take happens before this, which is the point + escalated.append("hard") + runs._write_stop_request(tmp_path, "hard") # concurrent `bmad-loop stop` + return real(path) + + monkeypatch.setattr(runs, "_stop_request_mode_of", _escalate_then_read) + + assert runs.consume_stop_request(tmp_path) == "graceful" # the body we took + assert escalated == ["hard"] # the interleave really happened + assert runs.read_stop_request_mode(tmp_path) == "hard" # NOT swallowed + + +def test_create_stop_request_failed_write_never_deletes_a_concurrent_hard_request( + tmp_path, monkeypatch +): + """The writer's *rollback* path is the third way a hard request could be lost, + and it is the one the monotone-lattice argument did not cover: that argument + enumerates the readers and the writers' success paths, and concludes only a + reader acting on a stale "graceful" can lose anything. + + `_create_stop_request` creates the file with `O_EXCL` and then writes the body + into it, so a failed write once rolled back with `path.unlink()`. `unlink` + resolves the *name*, not the inode this call created — so a `stop` escalating to + `mode: "hard"` onto that name while the write was in flight was deleted by the + cleanup of a graceful lodge that never completed. That is a `hard -> absent` + drop, below both rungs of the lattice, and on native Windows it withdraws the + only channel that can stop the engine while `stop_run` still reports the request + lodged. + + Closed by subtraction: there is no rollback. A short or empty body reads as + "graceful", which is the mode this call was asked to lodge anyway. + + ⚠️ Guarding the unlink instead does NOT fix this and measurably worsens it, the + same trap `consume_stop_request` documents: the check moves the decision earlier + and the destructive act later by its own cost, shifting the window rather than + narrowing it (inode compare 1.39x, mode compare 2.30x worse over a + rendezvous-synchronised escalation sweep). There is also no atomic + "unlink only if still my inode" to reach for, and `st_ino` is 0 on several + Windows filesystems, which would make the compare a false *equal*. + + Ablation: restore the `except BaseException: path.unlink(); raise` cleanup -> + the last assert fails with the mode `None`, because the escalation this test + injects is exactly what that unlink removes.""" + escalated: list[str] = [] + real_fdopen = os.fdopen + + def _escalate_then_fail(fd, *a, **kw): + if escalated: # nested use by the hard writer's own staged write + return real_fdopen(fd, *a, **kw) + escalated.append("hard") + os.close(fd) # the graceful file exists and is empty, as O_EXCL left it + runs._write_stop_request(tmp_path, "hard") # concurrent `bmad-loop stop` + raise OSError(28, "No space left on device") + + monkeypatch.setattr(os, "fdopen", _escalate_then_fail) + + with pytest.raises(OSError): + runs._create_stop_request(tmp_path) + assert escalated == ["hard"] # the interleave really happened + assert runs.read_stop_request_mode(tmp_path) == "hard" # NOT swallowed + + +def test_create_stop_request_failed_write_leaves_a_graceful_request_standing(tmp_path, monkeypatch): + """The other half of removing the rollback, stated as its own behavior rather + than left implicit: a write that fails part-way leaves the request pending. + + That is the bounded direction. The one production caller is `stop --graceful`, + which an operator drove, so the standing request is the one they asked for; a + short body reads as "graceful"; a later hard stop supersedes it unconditionally + and a later graceful ask answers "already-pending", so the channel is never + wedged; and `--cancel-graceful` or `resume` withdraws it. The CLI says so + instead of reporting a clean failure. + + Ablation: restore the cleanup -> the file is gone and the mode is `None`.""" + real_fdopen = os.fdopen + + def _fail(fd, *a, **kw): + os.close(fd) + raise OSError(28, "No space left on device") + + monkeypatch.setattr(os, "fdopen", _fail) + with pytest.raises(OSError): + runs._create_stop_request(tmp_path) + monkeypatch.setattr(os, "fdopen", real_fdopen) + + assert (tmp_path / runs.STOP_REQUEST_FILE).is_file() + assert runs.read_stop_request_mode(tmp_path) == "graceful" + + +def test_request_graceful_stop_keeps_escalation_unconditional(tmp_path, monkeypatch): + """The mirror direction, which the refusal above must not have cost. A hard + request landing *after* the graceful file exists still supersedes it — that is + `_write_stop_request`'s unconditional replace, which `stop_run` depends on. + + Ablation: give `_write_stop_request` the same create-if-absent treatment and + this reddens, reading "graceful" — the asymmetry between the two writers is the + whole design.""" + run_dir = _make_state_run(tmp_path, "r1") + (run_dir / "engine.pid").write_text("4242 100.0") + _use_host(monkeypatch, _FakeHost(alive=True, identity=100.0)) + + assert runs.request_graceful_stop(run_dir) == "requested" + runs._write_stop_request(run_dir, "hard") # a later `stop`, escalating + + assert runs.read_stop_request_mode(run_dir) == "hard" + + def test_mux_sessions_no_tmux(monkeypatch): # mux_sessions now delegates to the multiplexer backend; patch its seam. monkeypatch.setattr(tmux_base.shutil, "which", lambda _name: None) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index ef82c6e79..dc78d0431 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -4355,6 +4355,70 @@ def test_graceful_stop_between_cycles_skips_next_triage(project): assert stops[-1]["remaining"] == 1 # DW-2, generated in cycle 1, still open +def test_hard_stop_between_bundles_takes_hard_arm(project, monkeypatch): + """Sibling of the graceful bundle-boundary test for the HARD mode (#319): the + same control file carrying `mode: "hard"` is honored at the same boundary, but + takes the hard arm — unconditional teardown, no clean-finish subset, `run-stop` + with `via="stop-request"` and no `graceful` flag. Lodged at `post_bundle`, so + it lands after bundle 1 is fully done and past `_run_session`'s own hard-file + check; the boundary before bundle 2 is what sees it.""" + killed = [] + monkeypatch.setattr("bmad_loop.engine.kill_session", lambda rid: killed.append(rid)) + write_ledger(project, {"DW-1": "open", "DW-2": "open"}) + run_dir = project.project / ".bmad-loop" / "runs" / "sweep-run" + plan = triage_result( + ["DW-1", "DW-2"], + bundles=[ + {"name": "first-fix", "dw_ids": ["DW-1"], "intent": "a"}, + {"name": "second-fix", "dw_ids": ["DW-2"], "intent": "b"}, + ], + ) + engine, adapter = make_sweep( + project, + [ + triage_effect(plan), + bundle_dev_effect(project, "first-fix", ["DW-1"]), + bundle_review_effect(project, "first-fix"), + ], + ) + + post_run_stages = [] + original_emit = engine._emit + + def spy_emit(stage, *args, **kwargs): + if stage == "post_run": + post_run_stages.append(stage) + if stage == "post_bundle": + # the operator's `bmad-loop stop` lands between bundle 1 and bundle 2 + (run_dir / runs.STOP_REQUEST_FILE).write_text( + '{"requested_at": "2026-07-20T00:00:00", "mode": "hard"}', encoding="utf-8" + ) + return original_emit(stage, *args, **kwargs) + + engine._emit = spy_emit + + summary = engine.run() + + assert not summary.paused + tasks = engine.state.tasks + assert tasks["dw-first-fix"].phase == Phase.DONE and tasks["dw-first-fix"].commit_sha + assert "dw-second-fix" not in tasks # bundle 2 never dispatched + assert len(adapter.sessions) == 3 # triage + bundle-1 dev + bundle-1 review + saved = load_state(engine.run_dir) + assert saved.stopped is True and saved.finished is False + assert not runs.graceful_stop_requested(engine.run_dir) # consumed at the boundary + assert killed == ["sweep-run"] # hard arm's unconditional teardown + assert post_run_stages == [] # hard path skips the clean-finish subset + kinds = [e["kind"] for e in engine.journal.entries()] + assert "run-complete" not in kinds + assert "stop-request-discarded" not in kinds # honored, not discarded as stale + stops = [e for e in engine.journal.entries() if e["kind"] == "run-stop"] + assert stops and stops[-1]["via"] == "stop-request" + assert "graceful" not in stops[-1] + entries = ledger_entries(project) + assert entries["DW-1"].status.startswith("done") and entries["DW-2"].open + + def test_bundle_dispatch_does_not_pin_expected_spec(project, tmp_path): """A sweep bundle's fresh dispatch points at `intent.md`, never at a spec — the session is free to CREATE one, and #161 has it legitimately adopting a diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 38a21770a..83e7ae544 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -3635,7 +3635,7 @@ async def test_graceful_stop_requests_via_helper(project, monkeypatch): @pytest.mark.parametrize( "token, needle", [ - ("already-pending", "already has a graceful stop pending"), + ("already-pending", "already has a stop request pending"), ("requested-unverifiable", "could not confirm a live engine"), ], )