fix(teardown): skip worktree steps on rerun when the pool slot was returned or reissued - #2387
Open
Kallas95 wants to merge 3 commits into
Open
fix(teardown): skip worktree steps on rerun when the pool slot was returned or reissued#2387Kallas95 wants to merge 3 commits into
Kallas95 wants to merge 3 commits into
Conversation
A teardown rerun after a post-return failure (e.g. a refused focus-unsafe pane close) re-executed every worktree-scoped step, so a slot the pool had meanwhile reissued to a newer task was reset under the live tenant, killing its session (observed 2026-08-08). Ownership is now decided from two provable signals before any worktree step: a durable state/<id>.worktree-returned marker touched immediately after this task's own successful return, and the current fleet bindings - another task's meta recording the same worktree path proves reissue even when the marker is lost. When either shows the worktree is no longer this task's, the safety inspection, run-abort, worktree process reap, branch delete, hook removal, and the return itself are skipped with one line, while tasktmp reaping, the pane close retry, and durable-record cleanup continue unchanged. The marker is removed with the rest of the volatile state. Tests: an end-to-end replay of the incident (return ok, close refused, slot reissued, rerun converges without re-returning), a plain first-run return, a rerun with the worktree still bound and un-returned, and the lost-marker reissue caught by fleet bindings alone. All four fail against the unpatched script, so they pin the guard rather than passing vacuously. Verification: bin/fm-lint.sh clean under pinned ShellCheck 0.11.0; bin/fm-doc-audience-check.sh ok; tests/fm-teardown.test.sh 62/62 and tests/fm-teardown-endpoint-safety.test.sh 7/7 pass with one pre-existing, unrelated local-environment failure (herdr-preflight-missing-adapter) disabled: on this host's bash, sourcing a deliberately removed backends/herdr.sh from fm-backend.sh kills the shell before the graceful refusal. That failure reproduces identically at origin/main and is unrelated to this change.
…tenant-safety guard
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Port the local-only teardown ownership guard onto the current upstream head and ship it as an upstream PR against kunchenguid/firstmate:main.
Background: commit f603232 existed only on this firstmate home's local main and never reached upstream. Upstream has since heavily reworked bin/fm-teardown.sh and both teardown test suites. Verified that upstream does NOT already carry this guard, so it is not redundant. Because a cherry-pick was unavailable in this environment, the guard's intent was RE-FITTED into the current upstream structure of bin/fm-teardown.sh rather than forcing the old hunks in; f603232's commit message body is the deliberate basis for the new commit message.
Behavior contract to preserve exactly: a teardown rerun after a post-return failure must never reset or reap a pool worktree slot that (a) this task already returned - proven by a durable state/.worktree-returned marker touched right after its own successful return and removed by teardown's final cleanup - or (b) another task's meta now records, which proves reissue even when the marker is lost. When either signal shows the slot is no longer this task's, teardown skips the safety inspection, run-abort, worktree process reap, branch delete, hook removal, and the return itself, while tasktmp reaping, the pane close retry, and durable-record cleanup continue unchanged.
Deliberate decisions a reviewer reading only the diff would not know:
Constraints: this changes firstmate's shared tracked material, so .agents/skills/firstmate-coding-guidelines/SKILL.md applies - one sentence per line in Markdown, plain dash never an em dash, no agent co-author, shellcheck-clean bin scripts under the pinned version, and colocated tests extending the existing suite. Delivery route: no write access to origin, so the branch fm/fm-main-rebase-amont must go to the fork Kallas95/firstmate and the PR must open against kunchenguid/firstmate:main. Terminal state is local validation green plus the upstream PR open.
What Changed
bin/fm-teardown.shadds a rerun tenant-safety guard: a durablestate/<id>.worktree-returnedmarker is touched immediately after this task's own successful pool-worktree return (removed by teardown's final cleanup), and ownership is re-checked on rerun from that marker plus live fleet bindings — another task's meta recording the same worktree path proves reissue even when the marker is lost, while bindings whose task carries its own returned marker are ignored as past tenancies. When either signal shows the slot is no longer this task's, the safety inspection, run-abort, worktree process reap, branch delete, hook removal, and the return itself are skipped; tasktmp reaping, the pane close retry, and durable-record cleanup continue unchanged. A failed marker write warns instead of failing teardown, leaving fleet bindings as the fallback signal.tests/fm-teardown.test.shgains four cases pinning the guard: an end-to-end replay of the 2026-08-08 teardown-rerun-reissue incident, a plain first-run return, a rerun whose worktree is still bound and un-returned (the return must still happen), and a lost-marker reissue caught by fleet bindings alone, including ignoring a predecessor's stale past-tenancy binding.AGENTS.mddocuments the new<id>.worktree-returnedstate entry, anddocs/architecture.mdqualifies the teardown worktree checks as applying only while the slot is provably still this task's.Risk Assessment
✅ Low: The fix round implements the captain-authorized refinement exactly as prescribed (marker-bearing bindings skipped with the same presence test, header contract updated, scan semantics otherwise untouched), adds a colocated behavior-observable regression test that is non-vacuous by inspection against the pre-fix code, leaves no test disabled, and the only remaining observation is an explicitly authorized residual corner that never harms a live tenant.
Testing
Exercised the teardown ownership guard end-to-end via the colocated suites and swapped-script trees: the endpoint-safety suite passes 7/7, the full teardown suite passes 62/62 at the target commit once the one proven pre-existing local herdr-preflight environment failure (reproduced identically on the pure base commit) is excluded, the new tests were shown to fail against the unpatched and pre-review-fix scripts so they genuinely pin the guard, and an archived CLI transcript of the 2026-08-08 incident replay shows the rerun skipping the re-return while pane close and record cleanup complete and the reissued slot's tenant survives untouched; no UI surface is involved, so CLI transcripts are the product-level evidence.
Evidence: Incident replay CLI transcript (attempt 1 fails after the return, rerun skips every worktree step, tenant untouched)
Evidence: Fail-before/pass-after regression-pinning matrix (base 6789876, mid 1097082, target 7213b9b)
Evidence: Rerun stdout (the guard's one-line skip followed by clean completion)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:2310- Signal (b) of the tenant-ownership guard can false-positive on a PREDECESSOR's stale meta: worktree_reissued_to_other_task (loop at bin/fm-teardown.sh:2310) treats any other task's meta binding the same worktree path as proof of reissue, without excluding a task that has provably relinquished the slot via its own state/<id>.worktree-returned marker. Concrete reachable sequence, the mirror image of the pinned 2026-08-08 incident: task C returns slot S, touches C's marker, fails on the refused pane close and awaits its sanctioned rerun (C's meta and marker persist by design); the pool reissues S to task A; A finishes and its FIRST teardown runs before C's rerun converges; the scan finds C's meta binding S and sets WORKTREE_OWNED_BY_TASK=0, so A's teardown skips its own safety inspection (the unlanded/dirty-work refusal is silently bypassed), run-abort, worktree reap, branch delete, hook removal, and the pool return itself, then reports complete and erases A's durable records - leaking A's pool lease until manual intervention and leaving A's branch and hook files in the slot for the next tenant. The failure direction is fail-safe (nothing is destroyed, and the skip line names C), but the window is exactly the operational state this fix targets, so every new tenant of an incident slot that tears down before the old rerun converges hits it. A contract-compatible refinement: in the scan, skip any other task whose own <other_id>.worktree-returned marker exists, since that marker proves its binding is a stale record of an already-returned tenancy rather than a live claim; the intent's clause (b) ('another task's meta now records') still holds for marker-less bindings, and the accepted marker-lost fallback is unchanged. Classified ask-user because the two-signal semantics were deliberately ported 'exactly as in the original fix'.🔧 Fix: ignore returned predecessors when detecting pool slot reissue
1 info still open:
bin/fm-teardown.sh:2324- Residual authorized corner of the captain-approved refinement (documentation only, no action needed): a marker-lost rerun whose successor has ALSO already returned the slot (successor's own worktree-returned marker present while it awaits its sanctioned rerun) no longer sees that marker-bearing binding as proof of reissue, so the rerunning task re-executes the full worktree path and re-returns an idle pool slot. This requires two overlapping degraded states (this task's marker lost - a warn-only failure mode - plus the successor inside the post-return-failure window), never touches a live occupant (any marker-bearing binding provably returned the slot; any live tenant's marker-less meta is still detected), matches the pre-guard baseline behavior for idle slots, and stays behind the safety inspection which re-runs on that path. It is exactly the semantics the captain's fix instruction authorized ('only a task binding the same worktree path WITHOUT its own return marker proves the slot was reissued'), so it is an accepted tradeoff, not a defect.tests/fm-teardown.test.sh- The intent states all four ported tests were verified to FAIL against the unpatched script, but empirically only two do against the upstream base script (the incident replay and the lost-marker reissue case). test_first_run_still_returns_worktree and test_rerun_still_bound_unreturned_returns pass both before and after the patch: they pin the non-skip side of the contract (the guard must not over-trigger), which only the patched script could violate. The guard is still non-vacuously pinned; only the narrative claim is imprecise.tests/fm-teardown.test.sh- tests/fm-teardown.test.sh aborts locally at test_herdr_flat_teardown_preflight_refuses_before_changes (herdr-preflight-missing-adapter) before reaching the new tests. I reproduced the identical failure on the pure base commit 6789876 on this host, confirming it is a pre-existing local-environment issue not caused by this change, exactly as the intent discloses. The targeted run excluded only this case; remote CI remains the authority for it.bin/fm-test-run.sh tests/fm-teardown.test.sh tests/fm-teardown-endpoint-safety.test.shat target (endpoint-safety 7/7 ok; main suite aborted at the known herdr-preflight-missing-adapter environment failure before reaching the new tests)single-test run oftest_herdr_flat_teardown_preflight_refuses_before_changeson a pristine base-commit (6789876) tree, reproducing the identical failure and proving it pre-existing on this hostfulltests/fm-teardown.test.shat target with only the known-bad preflight case excluded: 62/62 ok including all five new guard testseach of the five new tests individually against the unpatched basebin/fm-teardown.sh(6789876): incident replay and bindings-only reissue FAIL as designed; first-run, still-bound-rerun, and returned-predecessor pass (they pin the non-skip side)test_returned_predecessor_binding_does_not_skip_current_returnagainst the pre-review-fix script (1097082): FAILS, pinning the review fix; the incident replay passes there, confirming the ported guard itselfmanual evidence capture: re-ran the incident replay with fixture cleanup disabled and archived the real fm-teardown.sh transcripts (attempt-1 stderr, rerun stdout, treehouse return log, tenant worktree state, state-dir contents)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.