runsetup.make_adapters raises SystemExit at five sites, every one of them after save_state
has published a run directory carrying finished=False / crashed=False and no run-start.
Nothing reconciles that shape: runs.reconcile_stale_worktrees is finished-ONLY by design (a
stopped run is still resumable, so its worktree is left for resume/clean rather than stranded
out from under the operator). So the directory lingers in bmad-loop list looking resumable — for a
run that never started and has nothing to resume.
Both composers are affected (compose_run and compose_sweep), and the escape need not be a
SystemExit: an adapter family's lazy load thunk is first imported here, and it is deliberately
never invoked by validate or bmad-loop adapters, so an ImportError from an optional dependency
has had no earlier gate and strands the run the same way.
Fix
Make composition atomic from the first published artifact: on any escape, remove the run directory
and its out-of-tree state, then re-raise the original error unchanged.
Two things the fix has to get right, both of which only surfaced under review:
-
runs.delete_run, not a bare rmtree. delete_run also drops the run's out-of-tree state
dir, which is what covers the config-digest stamp the composers write between the state and the
pid.
-
The run id must be claimed, not probed. delete_run's force=False guard is
liveness-scoped — it refuses a live session, and the dangerous case is precisely the run that is
not live (paused, stopped, finished). Since Journal.__init__ mkdirs with exist_ok=True, the
hidden --run-id flag (on both run and sweep) pointed at an existing run silently adopted
that run's directory and published its own state.json over it. Adding an unwind on top of that
would have deleted a pre-existing run outright — journal, logs and tasks. The id is now claimed
with mkdir(parents=True, exist_ok=False), which makes creation and collision-refusal one atomic
step, and the claim sits outside the composers' try so a refusal can never reach the unwind
arm and delete the very run it protects.
A removal that itself fails is reported — stderr warning: plus a composition-unwind-failed
journal entry — rather than passing silently. It stays best-effort (force=False, under
suppress(Exception)) so a cleanup failure never replaces the launch error, but not raising is
not the same as not reporting.
Found while fixing #501 and fixed on that branch (9c7a284, aab2b19, 8e35247): #501 moves the
sweep trigger's latch onto compose_sweep's success boundary, which is only a meaningful boundary
once a failed composition leaves nothing behind.
runsetup.make_adaptersraisesSystemExitat five sites, every one of them aftersave_statehas published a run directory carrying
finished=False/crashed=Falseand norun-start.Nothing reconciles that shape:
runs.reconcile_stale_worktreesis finished-ONLY by design (astopped run is still resumable, so its worktree is left for
resume/cleanrather than strandedout from under the operator). So the directory lingers in
bmad-loop listlooking resumable — for arun that never started and has nothing to resume.
Both composers are affected (
compose_runandcompose_sweep), and the escape need not be aSystemExit: an adapter family's lazy load thunk is first imported here, and it is deliberatelynever invoked by
validateorbmad-loop adapters, so anImportErrorfrom an optional dependencyhas had no earlier gate and strands the run the same way.
Fix
Make composition atomic from the first published artifact: on any escape, remove the run directory
and its out-of-tree state, then re-raise the original error unchanged.
Two things the fix has to get right, both of which only surfaced under review:
runs.delete_run, not a barermtree.delete_runalso drops the run's out-of-tree statedir, which is what covers the config-digest stamp the composers write between the state and the
pid.
The run id must be claimed, not probed.
delete_run'sforce=Falseguard isliveness-scoped — it refuses a live session, and the dangerous case is precisely the run that is
not live (paused, stopped, finished). Since
Journal.__init__mkdirs withexist_ok=True, thehidden
--run-idflag (on bothrunandsweep) pointed at an existing run silently adoptedthat run's directory and published its own
state.jsonover it. Adding an unwind on top of thatwould have deleted a pre-existing run outright — journal, logs and tasks. The id is now claimed
with
mkdir(parents=True, exist_ok=False), which makes creation and collision-refusal one atomicstep, and the claim sits outside the composers'
tryso a refusal can never reach the unwindarm and delete the very run it protects.
A removal that itself fails is reported — stderr
warning:plus acomposition-unwind-failedjournal entry — rather than passing silently. It stays best-effort (
force=False, undersuppress(Exception)) so a cleanup failure never replaces the launch error, but not raising isnot the same as not reporting.
Found while fixing #501 and fixed on that branch (
9c7a284,aab2b19,8e35247): #501 moves thesweep trigger's latch onto
compose_sweep's success boundary, which is only a meaningful boundaryonce a failed composition leaves nothing behind.