Raised by the codex review gate on PR #532 and split out rather than folded in: the hazard
predates that PR, and closing it is a concurrency-model change, not a guard fix.
The race
cmd_clean classifies runs once, then mutates:
for run_dir in runs.list_run_dirs(project):
...
elif runs.reclaimable(run_dir): # sampled here
reclaimable.append(run_dir)
...
for run_dir in reclaimable: # mutated here, never re-classified
...reconcile_orphan_worktrees / trim_run_dir / archive_run / delete_run...reclaimable(run_dir) is not engine_alive(run_dir) plus a terminal state. A stopped run
satisfies it and is also resumable, so bmad-loop resume <id> may start an engine and an agent
session in that run's worktree at any point after the sample. clean then force-removes the
worktree (reconcile_orphan_worktrees) and deletes worktrees/ (trim_run_dir) beneath a live
agent.
Nothing serializes the two. There is no lock, and resume does not consult a clean in progress.
What PR #532 does and does not change
#532 adds a live-session guard at the top of the reclaim loop and at the delete_run /
archive_run chokepoint. That narrows the window — a session already live when clean
reaches the run now protects it — but it does not close it, because both reads are samples too.
Its own scope note says so. Specifically, after #532:
- a session appearing between the loop-top guard and the removal is refused at the chokepoint,
and clean records the run and continues (agent session appeared mid-clean — not removed) —
but the worktree reconcile and trim above it have already run;
- a run inside the retention window reaches
trim_run_dir with only the loop-top sample, so
a session appearing after it loses worktrees/ with no second check at all.
Both are the same one-sample-then-mutate shape, and both exist identically on main at
814f0a2 — where the loop has no session check whatsoever.
Why it is not just "add another check"
Another sample immediately before each mutation shrinks the window again without closing it;
the operation is not atomic with respect to session creation. Closing it needs one of:
- a per-run lock that
resume / run start and clean both take (and that survives a crash
holding it, which is why this is not a one-liner);
- or making the destructive steps refuse on a fresh read taken inside the same operation that
removes — i.e. moving the guard into the mutation rather than before it;
- or accepting the race explicitly and documenting
clean as unsafe to run concurrently with
resume, which is at least honest and is roughly today's contract, undocumented.
The third is cheap and may be the right answer; it is a decision, not a patch, which is why this
was split out of #532 instead of being folded into it.
Reproduction shape
Not yet reproduced live — the window is small and needs a timing wedge. A deterministic version
is available at the seam: make runs.reclaimable (or live_session_may_be_ours) answer
differently on successive calls, as test_cmd_clean_survives_a_session_appearing_mid_clean does
in #532 for the narrower chokepoint case.
Refs: #532, #526, #419
Raised by the codex review gate on PR #532 and split out rather than folded in: the hazard
predates that PR, and closing it is a concurrency-model change, not a guard fix.
The race
cmd_cleanclassifies runs once, then mutates:reclaimable(run_dir)isnot engine_alive(run_dir)plus a terminal state. A stopped runsatisfies it and is also resumable, so
bmad-loop resume <id>may start an engine and an agentsession in that run's worktree at any point after the sample.
cleanthen force-removes theworktree (
reconcile_orphan_worktrees) and deletesworktrees/(trim_run_dir) beneath a liveagent.
Nothing serializes the two. There is no lock, and
resumedoes not consult a clean in progress.What PR #532 does and does not change
#532 adds a live-session guard at the top of the reclaim loop and at the
delete_run/archive_runchokepoint. That narrows the window — a session already live whencleanreaches the run now protects it — but it does not close it, because both reads are samples too.
Its own scope note says so. Specifically, after #532:
and
cleanrecords the run and continues (agent session appeared mid-clean — not removed) —but the worktree reconcile and trim above it have already run;
trim_run_dirwith only the loop-top sample, soa session appearing after it loses
worktrees/with no second check at all.Both are the same one-sample-then-mutate shape, and both exist identically on
mainat814f0a2— where the loop has no session check whatsoever.Why it is not just "add another check"
Another sample immediately before each mutation shrinks the window again without closing it;
the operation is not atomic with respect to session creation. Closing it needs one of:
resume/ run start andcleanboth take (and that survives a crashholding it, which is why this is not a one-liner);
removes — i.e. moving the guard into the mutation rather than before it;
cleanas unsafe to run concurrently withresume, which is at least honest and is roughly today's contract, undocumented.The third is cheap and may be the right answer; it is a decision, not a patch, which is why this
was split out of #532 instead of being folded into it.
Reproduction shape
Not yet reproduced live — the window is small and needs a timing wedge. A deterministic version
is available at the seam: make
runs.reclaimable(orlive_session_may_be_ours) answerdifferently on successive calls, as
test_cmd_clean_survives_a_session_appearing_mid_cleandoesin #532 for the narrower chokepoint case.
Refs: #532, #526, #419