From 38543b2259e0bff17b0df8baf69f6c2474c10118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Tue, 1 Sep 2026 09:17:18 +0200 Subject: [PATCH 1/2] docs(runs,cleanup): record the measured ceiling on the removal guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `live_session_may_be_ours` reads a listing that omits a live session as "no session", so `delete`, `archive` and `clean` can remove a run directory the session still depends on. The degrade was already documented as a decision; what was missing was its cost. Behavior is unchanged — this records the ceiling and gives it a retirement path. Two ways to meet it, both measured on psmux 3.3.8 (66cf613) under an isolated registry: - A `has-session` whose 500 ms connect does not land reaps a live session's registry entry. `ls` then exits 0 without that session, nothing on stderr, until the server's registry maintenance re-writes it — 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 server pid was alive throughout). - A process whose PATH lacks the binary reads every session as absent, for as long as that PATH lasts. The binary is per-process; the server is not. The two do not even look alike — a reap takes one entry, so the 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 exactly what a genuinely dead session produces. Why no stronger guard. The listing does not fail, it answers wrongly: exit 0, empty, clean stderr. The discrimination added for the window-listing seam reads exit codes and stderr, and neither is on this path, so there is nothing to condition on. Retry-with-delay pays more than the window on every removal that finds no session, which is the common case, and `clean` pays it per run. A tag-first read draws on the same listing and is blind in the same window. Warning whenever the multiplexer is unusable fires on every box that simply has none. A proof discipline was built and withdrawn earlier over four review rounds, each refuting its newest proof source; the transports offer no proof of absence. The psmux half is a defect, not a design: four sibling reap sites in that binary gate on `ConnectionRefused` because "a timeout means busy-but-alive and must not be deleted" (their own comment), the `has-session` site does not, and tmux 3.4 blocks rather than reaps under the same treatment. Reported as psmux/psmux#622. When a release carries the fix and the supported floor moves to it, only the PATH case remains. No version gate is possible meanwhile — there is no detectable condition to gate on. All three cited mechanisms were relocated by symbol on psmux master 80362fa and are unchanged since v3.3.8; 42 commits touch those paths, none of them these. Verified on both platforms: Windows 7197 passed, Linux 7397 passed, failures only in the known local environmental categories. Two claims from an earlier draft were refuted in review and corrected. Closes #732 --- CHANGELOG.md | 10 +++++++ docs/FEATURES.md | 2 +- src/bmad_loop/runs.py | 68 ++++++++++++++++++++++++++++++++++++++++--- tests/test_runs.py | 35 ++++++++++++++++++++++ 4 files changed, 110 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b25d079ab..37bc05250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,16 @@ breaking changes may land in a minor release. ### 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; the cost is now + measured rather than open. On psmux 3.3.8 a `has-session` whose 500 ms connect misses reaps a + live session's registry entry, and `ls` then exits 0 without it until the server's next registry + tick; and a process whose PATH lacks the binary reads every session as absent. Both reach the + guard as the same verdict a dead session produces — this run's name is not in the listing — so it + keeps accepting them. The psmux reap is an upstream defect, reported as psmux/psmux#622; the + window closes once a release fixes it and the supported floor moves to that release or later. + Behavior is unchanged. + - **psmux sessions now live in a per-project registry** (#537). bmad-loop points `PSMUX_DATA_DIR` at `//_mux`, so a prune in one project cannot address another's servers at all. A bare `psmux ls` no longer shows them — `bmad-loop mux` prints the diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 31735b187..e9d795955 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -288,7 +288,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w - `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). - `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/`. 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`). `--json` emits what was reclaimed (or would be) as a 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 0404ba412..e716f2719 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -2350,10 +2350,70 @@ 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 fails, that last one being + the raise — and says nothing about which (the + diagnostic #525 added, ``_warn_unproven_listing``, is on ``session_options`` + and the window path, not here). The reap does not even reach those + branches: the 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 fb0af4cee..7213871e7 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -4770,6 +4770,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 From dc3b1fd87e7edb4323d8c622ea2e502e172bb4cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Tue, 1 Sep 2026 09:33:08 +0200 Subject: [PATCH 2/2] docs(changelog,runs): address review on entry length and a wrong failure claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, two of three accepted: - The CHANGELOG entry ran nine lines where AGENTS.md asks for terse and scannable. Condensed to one, keeping the user-visible outcome, the affected commands and the retirement path; the measurement stays in docs/FEATURES.md and in the issue, which is where a reader who wants it will look. - `live_session_may_be_ours` claimed the query-failure branch of `list_sessions` "is the raise". It is not: the bundled backend folds `SubprocessError` and `OSError` into the same `[]` as a missing binary and a dead server, and only an out-of-tree backend raises here. The passage now says so, which strengthens rather than weakens the argument around it — three indistinguishable sentinels and no raise is a worse read than two. Declined: expanding the docs/FEATURES.md bullet with `--force` and `engine_liveness` semantics. Verified those claims hold, but they describe behavior this change does not touch, and the bullet already carries the load-bearing parts — `--force` removes anyway and kills nothing, `clean` has no force bypass and reports such runs as protected. Worth its own issue rather than widening this one. --- CHANGELOG.md | 10 +--------- src/bmad_loop/runs.py | 14 ++++++++------ 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37bc05250..39fb00069 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,15 +45,7 @@ breaking changes may land in a minor release. ### 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; the cost is now - measured rather than open. On psmux 3.3.8 a `has-session` whose 500 ms connect misses reaps a - live session's registry entry, and `ls` then exits 0 without it until the server's next registry - tick; and a process whose PATH lacks the binary reads every session as absent. Both reach the - guard as the same verdict a dead session produces — this run's name is not in the listing — so it - keeps accepting them. The psmux reap is an upstream defect, reported as psmux/psmux#622; the - window closes once a release fixes it and the supported floor moves to that release or later. - Behavior is unchanged. +- 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. - **psmux sessions now live in a per-project registry** (#537). bmad-loop points `PSMUX_DATA_DIR` at `//_mux`, so a prune in one project cannot address diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index e716f2719..4f9ffe57f 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -2366,12 +2366,14 @@ def live_session_may_be_ours(project: Path, run_id: str) -> bool: 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 fails, that last one being - the raise — and says nothing about which (the - diagnostic #525 added, ``_warn_unproven_listing``, is on ``session_options`` - and the window path, not here). The reap does not even reach those - branches: the exit is 0. So - there is neither a signal to condition on nor a word on stderr about it. + 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