fix(runs,cli): refuse to remove a run dir under a live session (#526)

CHANGELOG.md

@@ -188,6 +188,14 @@ whose seams had diverged enough that several ports needed a different fix, and t
   truncated tag that reads as another project's. The prune scan then skipped the project's own
   parked control windows. The last requested field now keeps its delimiters.
 
+- **Removing a run directory no longer strands a session that outlived its engine (#526).**
+  `delete`, `archive` and `clean` gated on engine-pid liveness, which an orphan — engine dead,
+  agent session still alive — passes. For an untagged session the run dir is the last ownership
+  proof a prune can read, so removing it leaked the session for the life of the machine. Removal
+  now refuses while a `bmad-loop-<run-id>` session the project cannot prove foreign is live, and
+  `clean` leaves the run untouched; `--force` overrides the refusal and kills nothing, since a
+  session name carries no project.
+
 - **A project path the multiplexer cannot carry no longer strands the scans over it (#419).** The
   ownership tag held the resolved path, and two transports mangled it: psmux's control line refuses
   a spaced UNC share, and a listing row splits on any separator `splitlines()` knows (LF, CR, VT,

docs/FEATURES.md

@@ -206,6 +206,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se
 - `bmad-loop stop <run-id>` — 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 delete <run-id>` — delete a run directory (`--force` stops it first if live).
 - `bmad-loop archive <run-id>` — compress a run into `.bmad-loop/archive` and remove it (`--force` stops it first if live).
+- 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.
 - `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 (`--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).

src/bmad_loop/cli.py

@@ -2894,7 +2894,11 @@ def cmd_delete(args: argparse.Namespace) -> int:
     rc = _stop_or_block_live_engine(run_dir, args.run_id, args.force)
     if rc is not None:
         return rc
-    runs.delete_run(run_dir)
+    try:
+        runs.delete_run(project, run_dir, force=args.force)
+    except runs.LiveSessionError as e:
+        print(f"{e} (or pass --force)", file=sys.stderr)
+        return 1
     print(f"run {args.run_id} deleted")
     return 0
 
@@ -2910,7 +2914,11 @@ def cmd_archive(args: argparse.Namespace) -> int:
     rc = _stop_or_block_live_engine(run_dir, args.run_id, args.force)
     if rc is not None:
         return rc
-    dest = runs.archive_run(project, run_dir)
+    try:
+        dest = runs.archive_run(project, run_dir, force=args.force)
+    except runs.LiveSessionError as e:
+        print(f"{e} (or pass --force)", file=sys.stderr)
+        return 1
     print(f"run {args.run_id} archived to {dest}")
     return 0
 
@@ -3015,6 +3023,24 @@ def cmd_clean(args: argparse.Namespace) -> int:
     deleted: list[str] = []
     unverifiable: list[str] = []
     for run_dir in reclaimable:
+        if runs.live_session_may_be_ours(project, run_dir.name):
+            # `reclaimable` is keyed on engine pid liveness, so an orphan — engine
+            # dead, agent session still live — passes it, and everything below this
+            # point mutates: the worktree the session may still be working in, the
+            # trimmed artifacts, and the run dir itself, which for an untagged
+            # session is the only ownership proof a later prune can read (#419).
+            # So the guard is the first thing in the loop, ahead of every mutation,
+            # and the run is reported untouched rather than half-reclaimed.
+            # `cleanup` clears the session — but for an untagged one it proves
+            # ownership by this same run dir, so the operator confirms first
+            # (`bmad-loop attach <id>`); the next `clean` then reclaims the run.
+            protected.append(run_dir.name)
+            if not args.json:
+                print(
+                    f"run {run_dir.name}: agent session still live — left untouched",
+                    file=sys.stderr,
+                )
+            continue
         if runs.engine_liveness(run_dir) == "unknown":
             # warn-only: unknown never blocks cleanup, but say so before removal.
             # In JSON mode this lives in the document instead (unverifiable_pid),
@@ -3031,21 +3057,44 @@ def cmd_clean(args: argparse.Namespace) -> int:
         run_bytes = _dir_size(run_dir)
         # collect, never print-as-you-mutate: the document is emitted once at the
         # end, so every per-item line has to survive the loop as data
-        for wt in runs.reconcile_orphan_worktrees(repo, run_dir, dry_run=dry):
+        run_worktrees = runs.reconcile_orphan_worktrees(repo, run_dir, dry_run=dry)
+        for wt in run_worktrees:
             worktrees.append(str(wt))
             if not args.json:
                 print(f"{'would remove' if dry else 'removed'} worktree {wt}")
         if run_dir.name in past:
             freed += run_bytes
-            runs.trim_run_dir(run_dir, dry_run=dry)  # shrink before archiving
-            if args.hard or not pol.cleanup.archive_old:
-                if not dry:
-                    runs.delete_run(run_dir)
-                deleted.append(run_dir.name)
-            else:
-                if not dry:
-                    runs.archive_run(project, run_dir)
-                archived.append(run_dir.name)
+            shrunk = runs.trim_run_dir(run_dir, dry_run=dry)  # shrink before archiving
+            try:
+                if args.hard or not pol.cleanup.archive_old:
+                    if not dry:
+                        runs.delete_run(project, run_dir)
+                    deleted.append(run_dir.name)
+                else:
+                    if not dry:
+                        runs.archive_run(project, run_dir)
+                    archived.append(run_dir.name)
+            except runs.LiveSessionError:
+                # A session appeared between the loop-top guard and here — a resume
+                # of a stopped run, racing this clean. The chokepoint refused the
+                # removal; record the run instead of letting one racing run abort
+                # the whole invocation. Correct the estimate down to what actually
+                # went. The wider race — every mutation in this loop against a
+                # concurrent resume — is older than this guard (`reclaimable` is
+                # sampled in the loop above and never re-read) and is tracked in
+                # issue #533.
+                freed += wt_bytes - run_bytes
+                # Classify by what happened, not by what was intended: the steps
+                # above may already have taken this run's worktree and artifacts,
+                # and `protected` means "left untouched" in the --json contract.
+                # Only a run nothing reached is protected; one already shrunk is
+                # trimmed, which is exactly the state it ends in.
+                (trimmed if run_worktrees or shrunk else protected).append(run_dir.name)
+                if not args.json:
+                    print(
+                        f"run {run_dir.name}: agent session appeared mid-clean — not removed",
+                        file=sys.stderr,
+                    )
         elif pol.cleanup.trim_artifacts:
             if runs.trim_run_dir(run_dir, dry_run=dry):
                 freed += wt_bytes

src/bmad_loop/documents.py

@@ -435,11 +435,13 @@ def clean_document(
 
     Every list names items the text enumerates or counts: `worktrees` holds
     absolute worktree paths, the rest hold run ids. `protected` is the runs left
-    untouched — `--keep`-listed or non-terminal — which the text reports only as
-    a count. `unverifiable_pid` is the subset of touched runs whose engine
-    liveness could not be proven; it is the text mode's stderr warning, carried
-    in the document so JSON mode leaves stderr empty, and it never blocks
-    reclamation.
+    untouched — `--keep`-listed, non-terminal, or carrying a live agent session,
+    which protects a run wherever it sits relative to the retention window
+    (reclaiming it would strand the session, #419) — which the text reports only
+    as a count. `unverifiable_pid` is the subset
+    of touched runs whose engine liveness could not be proven; it is the text
+    mode's stderr warning, carried in the document so JSON mode leaves stderr
+    empty, and it never blocks reclamation.
 
     `policy.retain` is the *effective* window — `--retain` when given, else
     `[cleanup] run_retention`. The other three are the configured policy as

src/bmad_loop/runs.py

@@ -14,7 +14,7 @@
 from pathlib import Path
 
 from . import devcontract, verify
-from .adapters.multiplexer import get_multiplexer
+from .adapters.multiplexer import MultiplexerError, get_multiplexer
 from .journal import STATE_FILE, Journal, load_state, save_state
 from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask
 from .platform_util import (
@@ -51,6 +51,12 @@ class GracefulStopError(Exception):
     the operator-facing message the CLI/TUI surface verbatim."""
 
 
+class LiveSessionError(Exception):
+    """A run directory was not removed because the run's agent session is still
+    live (see :func:`live_session_may_be_ours`). ``str()`` is the operator-facing
+    message the CLI/TUI surface verbatim."""
+
+
 # How long stop_run waits for a signalled engine to exit before falling back to
 # marking the run stopped itself.
 _STOP_WAIT_S = 10.0
@@ -552,15 +558,106 @@ def stop_run(run_dir: Path) -> bool:
     return True
 
 
-def delete_run(run_dir: Path) -> None:
-    """Permanently remove a run directory. Callers enforce the live guard."""
+def live_session_may_be_ours(project: Path, run_id: str) -> bool:
+    """True when a live ``bmad-loop-<id>`` session exists that this project cannot
+    prove belongs to another one — the precondition of the removal guard below.
+
+    Ownership is read exactly as :func:`prunable_sessions` reads it. A tag outside
+    :func:`accepted_tags` proves the session foreign, and a *tagged* session carries
+    its own ownership proof, so it does not need this project's run dir at all:
+    answering False there keeps the guard off a removal that provably strands
+    nothing. Untagged, or tagged as ours, answers True — neither can be ruled out
+    as depending on this run dir, and only the untagged case is load-bearing.
+
+    An observation, so it degrades rather than raising, and each read degrades in
+    its own direction. A listing that cannot answer reads as "no session": that is
+    already what the bundled backend returns for a missing multiplexer, a dead
+    server or a failed query, and a guard that varied by backend would be worse
+    than no guard. A tag that cannot be read is *not* proof the session is foreign,
+    so it reads as untagged and the refusal stands — by then the listing has
+    already established that a session is live.
+
+    Both reads are caught explicitly because the seam permits a raise: only
+    `pipe_pane` and `kill_session` are contractually best-effort, so an
+    out-of-tree backend raises :class:`MultiplexerError` here where the bundled
+    one returns empty (docs/adapter-authoring-guide.md). The listing is checked
+    first, so the tag query only runs on a name collision."""
+    name = session_name(run_id)
+    try:
+        if name not in mux_sessions():
+            return False
+    except MultiplexerError:
+        return False
+    try:
+        tag = session_project_tags().get(name, "")
+    except MultiplexerError:
+        tag = ""  # unread is not proof of foreign
+    return not tag or tag in accepted_tags(project)
+
+
+def _refuse_live_session(project: Path, run_id: str, verb: str) -> None:
+    """Backstop for #419: refuse to remove a run dir out from under a live session.
+
+    Every caller's live guard is keyed on *engine pid* liveness, so an orphan —
+    engine dead, agent session still alive in the multiplexer — passes all of them.
+    That is the one state where the run dir is load-bearing: for an untagged
+    session it is the only ownership proof :func:`prunable_sessions` can read, so
+    removing it leaks the session (and its server) for the life of the machine.
+    Refusing is a repair-path write failing loudly, per the module doctrine.
+
+    Scoped to what it can justify: a session this project can prove is another
+    one's does not block anything (see :func:`live_session_may_be_ours`). Refusing
+    there would strand nothing and wedge every removal path — including `clean`,
+    which has no override — for as long as the other project's run lives.
+
+    Never a kill from here: a session name carries no project, so killing
+    `bmad-loop-<id>` by name would tear down another project's live run whenever the
+    two share a run id (reachable — `--run-id` is caller-supplied).
+
+    The message names `bmad-loop cleanup` as the remedy but does not call it sound.
+    `prune_sessions` proves ownership from the tag when there is one and falls back
+    to *this same run dir* when there is not — the weak proof this guard exists to
+    protect, so on the untagged case it can prune another project's session on a
+    shared run id (#419's second edge, pinned by
+    `test_prunable_sessions_claims_an_untagged_session_on_a_run_id_collision`).
+    Hence the message asks the operator to confirm first: nothing available here can
+    prove the session ours, and minting a proof that outlives the run dir is #419
+    direction (2), not this guard."""
+    if live_session_may_be_ours(project, run_id):
+        raise LiveSessionError(
+            f"run {run_id}: refusing to {verb} its directory while its agent session is "
+            f"still live — for an untagged session this directory is the only ownership "
+            f"proof a later prune has. Clear the session with `bmad-loop cleanup` first, "
+            f"having confirmed it is this project's (`bmad-loop attach {run_id}`): an "
+            f"untagged session is proven ours by this same directory, so a run id shared "
+            f"with another project would prune theirs"
+        )
+
+
+def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None:
+    """Permanently remove a run directory. Callers enforce the engine-liveness
+    guard; the session guard is enforced here (see :func:`_refuse_live_session`),
+    which raises :class:`LiveSessionError` instead of removing.
+
+    ``force`` is the operator's explicit override and skips that guard, accepting
+    the leak on their own say-so. It deliberately does not kill the session
+    instead — that would be unscoped, and this project cannot prove the session is
+    its own (which is the whole defect). Trading a possible leak of our own session
+    for a possible kill of someone else's is the wrong direction for an override."""
+    if not force:
+        _refuse_live_session(project, run_dir.name, "delete")
     shutil.rmtree(run_dir)
 
 
-def archive_run(project: Path, run_dir: Path) -> Path:
+def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path:
     """Compress a run dir into .bmad-loop/archive/<id>.tar.gz and remove the
     original. The tarball is written to a temp path then atomically replaced into
-    place so a partial archive never appears. Callers enforce the live guard."""
+    place so a partial archive never appears. Callers enforce the engine-liveness
+    guard; the session guard is enforced here (see :func:`_refuse_live_session`,
+    and :func:`delete_run` for ``force``) and runs before the tarball is written,
+    so a refusal leaves nothing behind."""
+    if not force:
+        _refuse_live_session(project, run_dir.name, "archive")
     archive_dir = project / ARCHIVE_DIR
     archive_dir.mkdir(parents=True, exist_ok=True)
     dest = archive_dir / f"{run_dir.name}.tar.gz"

src/bmad_loop/tui/app.py

@@ -1000,8 +1000,11 @@ def done(ok: bool | None) -> None:
     @work(thread=True, group="lifecycle")
     def _delete_run_worker(self, run_id: str, run_dir: Path) -> None:
         try:
-            runs.delete_run(run_dir)
-        except OSError as e:
+            runs.delete_run(self.project, run_dir)
+        except (OSError, runs.LiveSessionError) as e:
+            # LiveSessionError is the #419 backstop: the confirm above gates on engine
+            # liveness, which an orphaned session passes. Surface it like any other
+            # failed removal rather than letting it kill the worker thread.
             self.call_from_thread(self.notify, f"delete failed: {e}", severity="error")
             return
         self.call_from_thread(self._dashboard.forget_run, run_id)
@@ -1037,7 +1040,9 @@ def done(ok: bool | None) -> None:
     def _archive_run_worker(self, run_id: str, run_dir: Path) -> None:
         try:
             dest = runs.archive_run(self.project, run_dir)
-        except OSError as e:
+        except (OSError, runs.LiveSessionError) as e:
+            # see _delete_run_worker: the confirm's guard is engine-keyed, this one
+            # is session-keyed (#419).
             self.call_from_thread(self.notify, f"archive failed: {e}", severity="error")
             return
         self.call_from_thread(self._dashboard.forget_run, run_id)