diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index b49831776e2..81c66c08afa 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -83,10 +83,11 @@ # name a slot a DIFFERENT live task now holds. Cleanup kills every process under # that path and hard-resets it before returning it, so releasing a slot that is # not genuinely this task's destroys another worker's live work. Before the first -# cleanup step, teardown verifies record exclusivity: no OTHER task record in -# this home or any locally registered Firstmate home may name the same live path -# in its worktree= or home=. One live path with two task records is the reuse -# collision itself, whichever record is stale. +# cleanup step, teardown reads the slot claim. If this task still owns the slot +# (or the claim is absent), it verifies record exclusivity: no OTHER task record +# in this home or any locally registered Firstmate home may name the same live +# path in its worktree= or home=. Two records naming this task's slot refuse its +# return; a positively reassigned slot is never returned. # That scan alone cannot prove THIS record is the current owner, because the task # that took the slot next may leave no record it can reach - its own worker may # have exited and its record been cleaned up, or it may live in a home this @@ -2376,8 +2377,9 @@ require_exclusive_task_worktree_slot() { # Positive slot ownership, read from the claim the task that took the slot wrote # into the slot itself (bin/fm-wake-lib.sh owns the claim and its states). # -# The record scan above proves that no OTHER task record names this slot. It -# cannot prove that THIS record is not the stale one, because the task that took +# The record scan, when this task still owns the slot, proves that no OTHER +# task record names it. It cannot prove that THIS record is not the stale one, +# because the task that took # the slot next may leave no record this scan can reach: its own worker may have # exited and its record been cleaned up, or it may belong to a home this machine # does not register. The claim closes that gap from the other side - it names the @@ -2387,8 +2389,9 @@ require_exclusive_task_worktree_slot() { # # A claim naming another task does not refuse: it means the slot is no longer # this task's, so the record's own cleanup proceeds and every slot step is -# skipped (see the script header for why refusing would strand the record and -# why skipping discards nothing). Returns TEARDOWN_SLOT_REASSIGNED_RC for that +# skipped, including the exclusivity scan that would reject the other task's +# legitimate record (see the script header for why refusing would strand the +# record and why skipping discards nothing). Returns TEARDOWN_SLOT_REASSIGNED_RC for that # state so each caller gates its slot steps on one determination; the claimant # stays in FM_TREEHOUSE_SLOT_OWNER_ID and FM_TREEHOUSE_SLOT_OWNER_HOME. # @@ -3308,8 +3311,10 @@ remove_secondmate_registry_entry() { return "$rc" } -require_exclusive_task_worktree_slot || exit 1 require_owned_task_worktree_slot || exit 1 +if teardown_owns_worktree; then + require_exclusive_task_worktree_slot || exit 1 +fi validate_pr_poll_cleanup "$STATE" "$ID" || exit 1 diff --git a/docs/architecture.md b/docs/architecture.md index 1180e6223c9..da8a2e1aee6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -408,7 +408,8 @@ After the forge accepts firstmate's merge request, the merge path persists the r A later merged poll consumes only that matching persisted value; with no match it records the landing as external rather than consulting a live away-posture record that may have been archived or replaced. [`bin/fm-merge-authority-lib.sh`](../bin/fm-merge-authority-lib.sh)'s header owns resolution, private atomic persistence, identity-checked consumption, and retirement, while only the merge path gates on the answer. Teardown is fail-closed for ship worktrees: dirty worktrees refuse, and committed work must be landed before the worktree is returned. -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. +A pool worktree is only returned after teardown passes the slot-ownership proof: when the slot claim names this task or is absent, a contradictory task record or a supported live endpoint refuses without touching either task, and no discard authority relaxes that. +A claim naming another task proves reassignment, so teardown skips every slot operation and cleans up only the stale task record. A slot's own owner claim, written by the spawn that takes it under the allocation lock and owned by [`bin/fm-wake-lib.sh`](../bin/fm-wake-lib.sh), covers a slot reassigned to a task that left no record the scan could reach: a claim naming a different task releases nothing - teardown warns, names the claimant, and finishes only the task's own cleanup - because Treehouse's own live process lease cannot answer ownership once the worker's exit releases it. Allocation and return serialize on one project lock per machine-local Firstmate tree: every home reachable through local parent links shares that lock, and a home seeded from another machine anchors its own, because a lock taken on this filesystem is neither held nor observable across that boundary. Before the worktree is returned, teardown concludes the task's own no-mistakes run when it is parked at a gate, including a run whose head the task copy cannot resolve - the shared runs-ledger continuation proof is the only recognition for that case, so cleanup never orphans a parked run the pipeline advanced past the submitted head. diff --git a/tests/fm-teardown-endpoint-safety.test.sh b/tests/fm-teardown-endpoint-safety.test.sh index 01dc74239e6..ee9150c82bf 100755 --- a/tests/fm-teardown-endpoint-safety.test.sh +++ b/tests/fm-teardown-endpoint-safety.test.sh @@ -550,6 +550,80 @@ test_reused_pool_slot_refuses_before_touching_the_other_task() { pass "fm-teardown: a pool slot named by a second task record is never returned, killed, or reset" } +# Two records can name one slot after reuse. Only the slot's positive claim +# distinguishes the stale record from its current owner; an absent or unsafe +# claim never licenses either record to release the contested slot. +test_shared_slot_claim_distinguishes_stale_from_current_owner() { + local dir id=stale-task other=current-task scenario target rc + for scenario in stale current absent unsafe; do + dir=$(make_case "shared-slot-$scenario") + mark_case_as_treehouse_pool "$dir" + fm_write_meta "$dir/home/state/$id.meta" \ + "window=firstmate:fm-$id" "endpoint_task_id=$id" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=scout" + fm_write_meta "$dir/home/state/$other.meta" \ + "window=firstmate:fm-$other" "endpoint_task_id=$other" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=scout" + target=$id + case "$scenario" in + stale) claim_pool_slot "$dir" "$other" ;; + current) claim_pool_slot "$dir" "$other"; target=$other ;; + absent) ;; + unsafe) printf 'not-a-claim\n' > "$dir/pool/1/.fm-slot-owner" ;; + esac + + set +e + run_case "$dir" "$target" > "$dir/stdout" 2> "$dir/stderr" + rc=$? + set -e + if [ "$scenario" = stale ]; then + [ "$rc" -eq 0 ] || fail "stale record could not finish its own cleanup: $(cat "$dir/stderr")" + assert_reassigned_slot_left_alone "$dir" "$id" "$other" "shared-slot stale record" + assert_present "$dir/home/state/$other.meta" "stale cleanup removed the current owner's record" + assert_present "$dir/worktree/sentinel" "stale cleanup changed the current owner's copy" + assert_contains "$(cat "$dir/runtime.log")" "fm-$id" \ + "stale cleanup did not close its own endpoint" + ! grep -F "kill-window" "$dir/runtime.log" | grep -Fq "fm-$other" \ + || fail "stale cleanup closed the current owner's endpoint: $(cat "$dir/runtime.log")" + else + [ "$rc" -ne 0 ] || fail "contested slot with $scenario claim was returned" + assert_present "$dir/home/state/$id.meta" "refusal erased the stale task record" + assert_present "$dir/home/state/$other.meta" "refusal erased the current task record" + assert_present "$dir/worktree/sentinel" "refusal changed the contested copy" + [ ! -s "$dir/runtime.log" ] \ + || fail "contested slot with $scenario claim reached the runtime: $(cat "$dir/runtime.log")" + if [ "$scenario" = unsafe ]; then + assert_contains "$(cat "$dir/stderr")" "$dir/pool/1/.fm-slot-owner" \ + "unsafe claim refusal did not identify its file" + else + assert_contains "$(cat "$dir/stderr")" "$other" \ + "contested slot refusal did not name the other record" + fi + fi + done + + # Ordinary cleanup of a completed ship must skip the current owner's dirty + # slot even without --force; the two task records still name the same copy. + dir=$(make_case shared-slot-completed-ships) + mark_case_as_treehouse_pool "$dir" + fm_write_meta "$dir/home/state/$id.meta" \ + "window=firstmate:fm-$id" "endpoint_task_id=$id" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=ship" + fm_write_meta "$dir/home/state/$other.meta" \ + "window=firstmate:fm-$other" "endpoint_task_id=$other" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=ship" + claim_pool_slot "$dir" "$other" + FM_HOME="$dir/home" FM_ROOT_OVERRIDE="$ROOT" \ + FM_RUNTIME_LOG="$dir/runtime.log" PATH="$dir/fakebin:$PATH" \ + "$TEARDOWN" "$id" > "$dir/stdout" 2> "$dir/stderr" \ + || fail "completed ship's stale record could not clean up without --force: $(cat "$dir/stderr")" + assert_reassigned_slot_left_alone "$dir" "$id" "$other" "completed ship without --force" + assert_present "$dir/home/state/$other.meta" "completed ship cleanup removed the current task record" + assert_present "$dir/worktree/sentinel" "completed ship cleanup changed the current owner's dirty copy" + + pass "fm-teardown: a positive other-task claim cleans only the stale record; current, absent and unsafe claims cannot release a contested slot" +} + test_cross_home_pool_slot_collision_refuses() { local dir id=stale-task other=secondmate-task second_home second_project rc dir=$(make_case slot-reuse-cross-home) @@ -1400,6 +1474,7 @@ test_orca_close_failure_refuses_even_under_force test_already_gone_endpoint_still_completes_without_a_refusal test_bare_relative_origin_shares_project_lock_with_clone test_reused_pool_slot_refuses_before_touching_the_other_task +test_shared_slot_claim_distinguishes_stale_from_current_owner test_cross_home_pool_slot_collision_refuses test_sole_slot_record_still_tears_down test_reassigned_pool_slot_finishes_own_cleanup_without_touching_the_slot