diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ed07475..00db6b69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ breaking changes may land in a minor release. transcript moves again; `0` disables the pair. The TUI agent line shows the open stretch as `· idle `. Observability only: nothing bounds the stretch. +### Changed + +- Document the live-session removal guard's measured ceiling (#732): `delete`, `archive` and `clean` still remove a run directory when a listing omits a live session. Behavior unchanged; the psmux half is reported upstream (psmux/psmux#622), its retirement tracked in #754. + ### Fixed - Distinguish confirmed missing tmux-family sessions from failed window listings; diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 8912cddc..6b9bf36c 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -766,7 +766,7 @@ persisted artifacts. - `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, project-confined 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). The destructive transaction re-checks liveness after acquiring the per-run lock, so force cannot remove a run a rival resume claimed after that stop; unknown liveness still warns and proceeds. - `bmad-loop archive ` — compress a run into `.bmad-loop/archive` and remove it, control-plane dir included (`--force` stops it first if live). The destructive transaction re-checks liveness after acquiring the per-run lock, so force cannot archive a run a rival resume claimed after that stop; unknown liveness still warns and proceeds. The hold covers tar snapshot, durable publication, source removal, and control-plane cleanup. The tarball holds the run dir, so it carries no `events/`. It is staged through an exclusively created temp under a fresh unpredictable name per attempt, so a planted name is never followed or reused, the failure cleanup is provably its own, and a temp stranded by a kill cannot deny later attempts; the tarball is `fsync`ed before the publish — the run dir is removed immediately after, so it is the only remaining copy. A published archive lands at mode `0600` rather than a umask-derived one (#591). -- 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. +- 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. The check is an observation with a measured ceiling ([#732](https://github.com/bmad-code-org/bmad-loop/issues/732)): a listing that omits a live session reads as "no session", so removal proceeds. Two ways to meet it, both measured on psmux 3.3.8 — a `has-session` whose 500 ms connect does not land reaps a live session's registry entry, and until the server's registry maintenance re-writes it `ls` omits that session while exiting 0 with nothing on stderr (that maintenance runs on a nominal 5 s check in the server's own loop, so the window has no hard bound; 1.7 s was one measured sample); and a process whose PATH lacks the multiplexer binary reads every session as absent for as long as that PATH lasts, however long the server keeps running. The first is a successful query with an incomplete answer and the second is a missing binary, and they do not even look alike — a reap takes one entry, so its listing may still name every other session, while a missing binary yields nothing at all — but both reach the guard as the same verdict, this run's name is not in the listing, which is what a genuinely dead session produces too. So it accepts both rather than wedging every removal path behind `--force`. If a removal does strand a session, clear it with `kill-session` on `bmad-loop-` — but neither failure leaves it reachable straight away: restore the multiplexer to PATH first, and after a reap wait for the server's next registry tick to republish the entry, or the target will not resolve. On psmux the sessions live in the per-project registry, so run it with the export `bmad-loop mux` prints. Confirm the session is this project's before killing, since the name carries no project — and note that `attach` is no longer the way to do it: it resolves the run directory first, which the removal just deleted. Attach in the multiplexer directly instead, and for an _untagged_ session accept that the run dir it lost was its only ownership proof, so what is left is the session's own contents and what you know of the run. - `bmad-loop cleanup` — remove leftover tmux artifacts for finished/stopped runs. `--json` emits the sessions and ctl windows removed (or, with `--dry-run`, that would be) as a stable machine-readable document per the [contract below](#machine-readable-output---json). - `bmad-loop clean` — reclaim disk from concluded runs per `[cleanup]`: tear down worktrees a mid-flight stop orphaned, trim heavy `worktrees/` from runs kept for history, archive/delete past the retention window, and sweep orphaned run control-plane dirs from the out-of-tree state root (`--dry-run`, `--keep`, `--retain N`, `--hard`). A run that resumes before its final removal is classified as protected or trimmed according to work already completed, and unrelated candidates continue. `--json` emits what was reclaimed (or would be) as one stable machine-readable document per the [contract below](#machine-readable-output---json), with `freed_bytes` a raw integer. - `bmad-loop tui` — the interactive dashboard (`--low-frame-rate` for slow/SSH links). diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 79ce55ef..0d9655c5 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -2406,10 +2406,72 @@ def live_session_may_be_ours(project: Path, run_id: str) -> bool: reaped port file on a 5 s tick, source-read at v3.3.8), a binary's PATH presence is per-process while the server is not, and the listing is load-sensitive; so a "proof of absence" either wedges every removal behind - `--force` or quietly accepts a refutable proof. The degrade above is the - guard's owner's documented trade, kept deliberately; the measured cost of - the unobservable-multiplexer window is filed for that owner to revisit - rather than overturned here. + `--force` or quietly accepts a refutable proof. + + Revisited with the cost measured (#732), and the degrade is KEPT — not for + want of a stronger read, but because the more dangerous of the two costs + reaches this function as an ordinary answer. On psmux 3.3.8 a live session's + registry entry is reaped by any `has-session` whose 500 ms connect does not + land, and until the server's registry maintenance re-writes it the listing + does not name that session while reporting no fault at all: exit **0**, no + stderr (measured; the listing empty in that run because the session was the + only one, the server pid alive throughout, the entry back 1.7 s later). + That maintenance runs when the server's own loop notices 5 s have elapsed — + a nominal interval, not a scheduling guarantee, so 1.7 s is a sample and 5 s + is not a bound. The listing therefore does not fail, it answers wrongly — + and the read this guard makes is the one with no discrimination in it at + all. ``mux_sessions()`` lands on ``BaseTmuxBackend.list_sessions``, which + returns ``[]`` under the three conditions its own comment names — the binary + is missing, no server is running, or the query itself fails — and says + nothing about which. None of the three raises: the bundled backend folds + ``SubprocessError`` and ``OSError`` into that same sentinel, and only an + out-of-tree backend raises ``MultiplexerError`` here. Nor does the + diagnostic #525 added: ``_warn_unproven_listing`` sits on + ``session_options`` and the window path, not on this one. And the reap + reaches none of those branches anyway, because its exit is 0. So there is + neither a signal to condition on nor a word on stderr about it. + + The second cost lasts as long as its cause: a process whose PATH lacks the + binary reads every session as absent for as long as that PATH does (measured + through the seam — the sessions reappear when PATH is restored), because the + binary is per-process while the server is not. That one is no mystery to the + transport: ``list_sessions`` decides it from ``shutil.which`` on purpose, + being one of the two methods that report what exists, and folds it into the + same ``[]`` deliberately (its own comment: the absence of sessions and the + absence of the multiplexer are indistinguishable here). The fold is the + seam's decision; what reaches this function is its result. + + What the two costs share is not a shape but a verdict. A reap takes one + session's entry, so its listing may still name every other session on the + box; a missing binary yields nothing at all. Both arrive as the same + *negative membership* — this run's name is not in the listing — which is + exactly what a genuinely dead session produces, and there is no fourth + answer to distinguish them by. + + The named alternatives were weighed and refused. Retry-with-delay, in the + shape this guard could hold, pays a delay on every removal that finds no + session — the common case — once per run inside `clean`; a retry hoisted to + command scope would pay it once, but neither can bound a window whose close + is a nominal tick rather than a deadline. A tag-first read draws on the same + listing and is blind in exactly the same window. Warning whenever the + multiplexer is unusable fires on every box that simply has no multiplexer, + which is the noise ``BaseTmuxBackend._warn_unproven_listing`` already stays + silent about. What is left is the accepted ceiling, written down here and in + docs/FEATURES.md so an operator meets it as a documented limit rather than a + surprise. + + The psmux half is a *defect*, not a design: four sibling reap sites in that + binary gate on `ConnectionRefused` precisely because "a timeout means + busy-but-alive and must not be deleted" (its own comment), the `has-session` + site does not, and tmux 3.4 blocks rather than reaps under the same + treatment (measured). Reported as psmux/psmux#622, which is the thing to + watch. Retiring it takes two parties, though, and an upstream fix alone is + not enough: the reap window closes for this project once a psmux release + repairs that site AND the supported floor is raised to that release or + later, since an installed 3.3.8 keeps reaping whatever master does. And it + retires only itself — the PATH case is ours, and so are the two arms above + it, a backend that cannot be selected and a listing that raises. What that + fix removes is one measured case, not the ceiling. Two registry-root-era additions on that unchanged contract: diff --git a/tests/test_runs.py b/tests/test_runs.py index 369a4901..e19a3ca2 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -6763,6 +6763,41 @@ def unselectable(): assert not runs.live_session_may_be_ours(tmp_path, "ctl") +def test_live_session_may_be_ours_reads_a_successful_listing_as_the_whole_truth( + tmp_path, monkeypatch +): + """The accepted ceiling of #732, pinned so it cannot be closed by accident. + + The two tests above cover a listing that *fails*. This one covers the + listing that succeeds and is wrong: psmux reaps a live session's registry + entry on any `has-session` whose 500 ms connect does not land, and until the + server's registry maintenance re-writes it `ls` exits 0 with that session + simply missing — no error, no stderr. (That maintenance runs on a nominal + 5 s check in the server's own loop, so the window has no hard bound; 1.7 s + was one measured sample.) The guard therefore cannot tell "absent" from + "omitted", and removal proceeds. That is the documented trade, not an + oversight; a change that makes an empty or short listing block instead has + to delete this test and read why. + + The third case is the positive control that makes the first two mean + something: the same guard, same fixtures, answers True the moment the + listing does name the session, so the two Falses are answers rather than a + harness that cannot say anything else.""" + name = runs.session_name("20260826-000000-run1") + monkeypatch.setattr(runs, "ctl_session_for", lambda project, mux=None: runs.CTL_SESSION) + + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([])) + assert not runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + # Non-empty and still omitting it: a reap takes one session's entry, not the + # registry, so "the listing came back with rows in it" is no reassurance either. + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([runs.session_name("other")])) + assert not runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + monkeypatch.setattr(runs, "get_multiplexer", lambda: _LivenessMux([name])) + assert runs.live_session_may_be_ours(tmp_path, "20260826-000000-run1") + + def test_prune_sessions_claims_a_historical_ctl_prefixed_session(tmp_path, monkeypatch): """End to end through the sweep: a `bmad-loop-ctl-foo` session minted by an older release (`--run-id ctl-foo`), untagged, with this project's dead run