fix(bin): consult the slot owner claim before teardown's record scan - #5315
Closed
ammar00sheikh wants to merge 3 commits into
Closed
ammar00sheikh wants to merge 3 commits into
ammar00sheikh wants to merge 3 commits into
Conversation
added 3 commits
September 22, 2026 20:31
Two task records naming one reused treehouse pool slot could never be torn down: the record-exclusivity scan ran ahead of the slot-owner claim and refused both, including under --force, so the stale record kept its endpoint alive in the fleet view and the watcher re-fired a stale wake on every poll. Read the slot's own claim first and let it settle ownership. A claim that positively names a different task proves this record owns nothing in the slot, so the already-documented reassignment carve-out runs - this task's endpoint, status, records, checks, and backlog are cleaned up while every step that reads or touches the slot is skipped - instead of refusing. Every other claim state keeps exactly the protection it had: an absent claim and a claim naming this record itself still run the record scan and still refuse a contested slot, and an unreadable claim still refuses. --force is unchanged and still authorizes discarding only this task's own unlanded work. The forced-secondmate descendant preflight takes the same order, so a child slot the claim proves was reassigned skips the scan along with its other slot steps.
…unsettled-claim case
Author
|
Solved upstream by #6213 (merged 2026-09-30), which fixes the same two-record slot deadlock with the same claim-first approach: when the slot's owner claim names another task, the stale record's teardown skips the record-exclusivity scan and retires records-only. Closing this PR as superseded. |
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
1 participant
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
Two Firstmate task records that name the same reused treehouse pool slot deadlock each other: neither can ever be torn down, the stale one keeps its endpoint alive in the fleet view, and the watcher re-fires a stale wake for it on every poll.
Reproduced live on 2026-09-22 in the main home, twice:
Tearing down either member refuses with "REFUSED: task X's recorded worktree is also task Y's recorded worktree", including under --force, so the pair is permanently stuck. In the first pair the slot-owner claim file at /Users/cto/.treehouse/waselni-backend-89119c/2/.fm-slot-owner reads task=ts-parity-s1, which proves cache-tags-guard is the stale record and owns nothing in that slot.
bin/fm-teardown.sh's own header (lines 81-105) already describes the intended behaviour for exactly this case: a claim naming another task is proof of reassignment, so teardown should warn, name the claimant, and finish only that task's own cleanup - endpoint, status, records, checks, backlog - while skipping every step that would read or touch the slot. That carve-out never gets a chance to run, because require_exclusive_worktree_slot_record() refuses first.
What Changed
bin/fm-teardown.shnow gates a task's own pool slot through a singlerequire_task_worktree_slot_ownership()that reads the slot's owner claim first and only falls through torequire_exclusive_task_worktree_slotwhen the claim does not settle ownership (absent, or naming this very record). A claim naming a different task now reaches the existing reassignment carve-out — warn, name the claimant, finish only this record's endpoint/status/records/checks/backlog cleanup — instead of being refused by the record-exclusivity scan that previously ran first and rejected both records of a reused-slot pair.preflight_descendant_treehouse_slots()applies the same order for secondmate children: a child slot the claim proves was reassigned is skipped before the record scan runs, so the rest of the forced teardown proceeds without touching that slot.bin/fm-teardown.shheader, therequire_owned_worktree_slot_record()comment, anddocs/architecture.md; addedtest_claim_breaks_the_two_record_slot_deadlock_only_for_the_non_owner(covering the disowned record, the real owner's subsequent normal teardown, and the unclaimed / self-claimed / unreadable-claim cases that must still refuse) andtest_secondmate_force_teardown_clears_reassigned_duplicated_child_slotfor the descendant path.Risk Assessment
🚨 High: The reordering itself is sound, well-documented, and strictly safer than what it replaces, but source and on-disk evidence prove one of the two live reproductions the authoritative intent names remains permanently un-tearable with no supported escape, so shipping a partial fix needs the author's explicit call.
Testing
I read the change, ran the two suites that own this boundary (fm-teardown-endpoint-safety and fm-secondmate-safety) - both green - and proved the commit's new test is a genuine regression test by temporarily reverting bin/fm-teardown.sh to base f5735dc, where it fails with the exact reported "REFUSED ... not even with --force" refusal, then restoring it. For end-user evidence I built a live reproduction of the 2026-09-22 report (cache-tags-guard stale vs ts-parity-s1 owner, both records naming one reused pool slot, the slot claim naming ts-parity-s1, and a real worker process running inside the slot) and captured CLI transcripts of the real fm-teardown.sh before and after the fix: before, both members refuse under --force and the stale endpoint stays in the fleet view; after, the stale record warns, names the claimant, clears only its own records, and leaves the worker alive, the slot copy intact, the claim untouched and the slot un-returned, after which the real owner tears down and returns the slot. I also found that the commit reordered the same two guards in the descendant/secondmate preflight with no test covering it, so I added a focused test there that drives fm-teardown.sh domain --force over a secondmate home whose two child records name one reused slot claimed by a third task - it fails at base and passes at target. The second reported pair (unclaimed slot) still refuses, which matches the recorded declined decision and is pinned by the slot-reuse-unclaimed case. No screenshot applies: this change is a CLI/state-machine fix with no rendered UI surface, so the reviewer-visible artifacts are the CLI transcripts and the persisted record/slot state they print. The worktree is left with only the intentional new test.
Evidence: Before vs after summary of the deadlock repro
Source: Before vs after summary of the deadlock repro
Evidence: CLI transcript - base f5735dc (both records permanently stuck)
Source: CLI transcript - base f5735dc (both records permanently stuck)
$ fm-teardown.sh cache-tags-guard --force REFUSED: task cache-tags-guard's recorded worktree .../2/waselni-backend is also task ts-parity-s1's recorded worktree. Returning that pool slot would kill ts-parity-s1's processes and reset its copy, so nothing was changed - not even with --force. [exit 1] $ fm-teardown.sh ts-parity-s1 --force REFUSED: task ts-parity-s1's recorded worktree .../2/waselni-backend is also task cache-tags-guard's recorded worktree. [exit 1] state/cache-tags-guard.meta (stale record) PRESENT state/ts-parity-s1.meta (live owner record) PRESENT slot returned to the treehouse pool noEvidence: CLI transcript - target b7049e7 (claim breaks the tie)
Source: CLI transcript - target b7049e7 (claim breaks the tie)
$ fm-teardown.sh cache-tags-guard --force warning: task cache-tags-guard's recorded worktree .../2/waselni-backend was reassigned to task ts-parity-s1 (home ...), which claimed that pool slot after this record was written; that slot is no longer cache-tags-guard's, so its processes, copy, and claim are left untouched and only cache-tags-guard's own cleanup runs. teardown cache-tags-guard complete (window firstmate:fm-cache-tags-guard; pool slot .../2/waselni-backend left to task ts-parity-s1 ...) [exit 0] state/cache-tags-guard.meta (stale record) GONE state/ts-parity-s1.meta (live owner record) PRESENT pool/2/.fm-slot-owner (slot claim) PRESENT ts-parity-s1 worker in the slot ALIVE slot returned to the treehouse pool no $ fm-teardown.sh ts-parity-s1 --force teardown ts-parity-s1 complete (window firstmate:fm-ts-parity-s1, worktree .../2/waselni-backend) [exit 0] state/ts-parity-s1.meta (live owner record) GONE pool/2/.fm-slot-owner (slot claim) GONE slot returned to the treehouse pool YESEvidence: Reproduction driver used for both transcripts
Source: Reproduction driver used for both transcripts
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:2394- The fix only breaks the deadlock when the slot carries a claim naming a third task; the second of the two reproductions the intent names is still permanently stuck. Intent (required): "Reproduced live on 2026-09-22 in the main home, twice: ... underpay-ts1 (stale, superseded) and api-driver-suspension both record worktree=/Users/cto/.treehouse/waselni-backend-ts-ad71ac/2/waselni-backend-ts. Tearing down either member refuses ... including under --force, so the pair is permanently stuck." Verified on disk: that slot has NO .fm-slot-owner file (pair 1's does, reading task=ts-parity-s1), both metas still record the path with kind=ship, and the path is a real pool slot (treehouse-state.json present; project and slot git-common-dir both resolve to .../projects/waselni-backend-ts/.git). Concrete trace forfm-teardown.sh underpay-ts1 --force: teardown_live_slot_path returns the slot -> fm_treehouse_slot_owner_state sets FM_TREEHOUSE_SLOT_OWNER=absent -> require_owned_worktree_slot_record returns 0 -> require_owned_task_worktree_slot returns 0 with TEARDOWN_SLOT_REASSIGNED=0 -> line 2393 teardown_owns_worktree returns 0, so it does NOT short-circuit -> line 2394 require_exclusive_task_worktree_slot finds api-driver-suspension.meta naming the same canonical path -> "REFUSED: ... not even with --force". Symmetric for api-driver-suspension. The contradicting hunk is the deliberate design stated in the header: "An absent claim keeps exactly the record-scan protection it had before, because refusing it would strand every task in flight across that change on no evidence at all." The refusal directs the operator tobin/fm-crew-state.sh, which only reports state and cannot rewrite worktree=, so there is no supported escape. Decide with the author whether the absent-claim pair needs its own tiebreaker at the same shared boundary (require_task_worktree_slot_ownership) - e.g. treating a record whose recorded endpoint is provably gone as the stale one, or an explicit operator-supplied disambiguation - or whether shipping half the reported fix is acceptable for now.docs/architecture.md:386- "A pool worktree is only returned after teardown passes the slot-ownership proof: a contradictory task record or a supported live endpoint refuses without touching either task, and no discard authority relaxes that." The clause about a contradictory task record was unconditionally true while require_exclusive_task_worktree_slot ran first; after this change a contradictory record no longer refuses when the claim names another task - teardown exits 0 and removes this task's record via the carve-out. Line 387 already describes the carve-out, so line 386 now overstates the refusal breadth. Narrow it to: a contradictory record refuses only when the claim does not settle ownership (absent, or naming this very record).✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-teardown-endpoint-safety.test.sh tests/fm-secondmate-safety.test.sh(both suites green)tests/fm-teardown-endpoint-safety.test.sh::test_claim_breaks_the_two_record_slot_deadlock_only_for_the_non_owner- run at target (pass) and against basebin/fm-teardown.shfrom f5735dc (fails with the reported REFUSED message), proving the regressiontests/fm-secondmate-safety.test.sh::test_secondmate_force_teardown_clears_reassigned_duplicated_child_slot- new test added this pass for the descendant slot-guard reorder; fails at base, passes at targettests/fm-secondmate-safety.test.sh::test_secondmate_force_teardown_refuses_duplicated_child_slot- existing descendant refusal still holdsManual end-to-end repro of the live report:/Users/cto/.no-mistakes/evidence/01M352W181S0NQ0EDAAGC8VCTR/repro-two-record-slot-deadlock.sh <fm-teardown.sh> <repo root>run once against base f5735dc and once against b7049e7, driving the realbin/fm-teardown.sh cache-tags-guard --forcethents-parity-s1 --forceover a real treehouse pool slot (git worktree + treehouse-state.json + .fm-slot-owner) with a live worker process inside the contested slot🔧 **Document** - 1 issue found → auto-fixed ✅
docs/architecture.md:386- docs/architecture.md:386-387 still describes the pre-change guard order: line 386 states unconditionally that "a contradictory task record ... refuses without touching either task, and no discard authority relaxes that", and line 387 scopes the slot claim to "a slot reassigned to a task that left no record the scan could reach". After this change the claim is read first and settles ownership even when the other record IS reachable, so a contradictory record refuses only when the claim is absent or names this very record. Narrowing that clause was reported in review round 1 as 'architecture-doc-refusal-claim-stale' and declined, so I deliberately made no edit here rather than implementing the declined fix at the adjacent sentence. No action requested; raised only so the deliberate non-edit is visible if the author later wants that paragraph re-scoped. The authoritative owner (bin/fm-teardown.sh's header, which line 390 points to) is accurate.🔧 Fix: narrow architecture slot-ownership refusal to unsettled-claim case
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.