diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index 0544a9c004b..ea752a144c4 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -87,6 +87,11 @@ # 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. +# Records are matched by identity - the state directory's physical path plus the +# record's own name - never by the spelling each was reached through, so one home +# reached through two equivalent paths (a symlinked /home spelling beside its +# physical target) stays ONE record instead of reading as a task colliding with +# itself; require_exclusive_worktree_slot_record owns that comparison. # 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 @@ -2224,10 +2229,30 @@ teardown_live_slot_path() { canonical_existing_dir "$WT" } +# One entry per state DIRECTORY, keyed by that directory's physical path so a +# home reached through two equivalent spellings - a symlinked path such as +# /home//work next to its physical /nobackup//work target - is +# collected once instead of twice. TREEHOUSE_OWNER_STATE_KEYS[i] is the +# canonical key of TREEHOUSE_OWNER_STATES[i], so the record scan below can name +# each record it finds by identity rather than by the spelling it was reached +# through. A directory that cannot be resolved keys as its literal path: it has +# no records to scan anyway, and dropping it would silently narrow the scan. +add_treehouse_owner_state() { # + local state_dir=$1 key existing + key=$(canonical_existing_dir "$state_dir") || key=$state_dir + for existing in "${TREEHOUSE_OWNER_STATE_KEYS[@]+"${TREEHOUSE_OWNER_STATE_KEYS[@]}"}"; do + [ "$existing" != "$key" ] || return 0 + done + TREEHOUSE_OWNER_STATES+=("$state_dir") + TREEHOUSE_OWNER_STATE_KEYS+=("$key") +} + collect_local_firstmate_states() { local record_state=$1 root home reg line child known existing i=0 local -a homes - TREEHOUSE_OWNER_STATES=("$record_state") + TREEHOUSE_OWNER_STATES=() + TREEHOUSE_OWNER_STATE_KEYS=() + add_treehouse_owner_state "$record_state" root=$(fm_firstmate_root_home "$FM_HOME") || { echo "REFUSED: cannot resolve the root Firstmate home; nothing was changed" >&2 return 1 @@ -2236,11 +2261,7 @@ collect_local_firstmate_states() { while [ "$i" -lt "${#homes[@]}" ]; do home=${homes[$i]} i=$((i + 1)) - known=0 - for existing in "${TREEHOUSE_OWNER_STATES[@]}"; do - [ "$existing" != "$home/state" ] || known=1 - done - [ "$known" = 1 ] || TREEHOUSE_OWNER_STATES+=("$home/state") + add_treehouse_owner_state "$home/state" reg="$home/data/secondmates.md" [ ! -e "$reg" ] && [ ! -L "$reg" ] && continue [ -f "$reg" ] && [ ! -L "$reg" ] || { @@ -2270,15 +2291,38 @@ collect_local_firstmate_states() { done } +# The identity of a task record: its state directory's physical path plus the +# record's own file name. Only the directory is resolved, so two records in one +# directory, and one record name in two genuinely different directories, stay +# distinct identities - the reuse collision this scan exists to catch. A record +# whose directory cannot be resolved keys as its literal path, which can only +# make the comparison stricter. +task_record_identity() { # + local record=$1 dir key + [ -n "$record" ] || return 1 + dir=${record%/*} + [ "$dir" != "$record" ] || dir=. + key=$(canonical_existing_dir "$dir") || key=$dir + printf '%s/%s\n' "$key" "${record##*/}" +} + require_exclusive_worktree_slot_record() { local record_meta=$1 record_id=$2 record_state=$3 worktree=$4 - local slot state_dir other other_id field other_path other_slot + local slot state_dir state_key other other_id field other_path other_slot + local record_identity i slot=$(canonical_existing_dir "$worktree") || return 0 + # Compare records by identity, not by the path spelling each was reached + # through: a home reached through a symlinked spelling would otherwise read + # its own single record as a second task holding the same slot and refuse the + # task as a collision with itself. + record_identity=$(task_record_identity "$record_meta") || record_identity=$record_meta collect_local_firstmate_states "$record_state" || return 1 - for state_dir in "${TREEHOUSE_OWNER_STATES[@]}"; do + for ((i = 0; i < ${#TREEHOUSE_OWNER_STATES[@]}; i++)); do + state_dir=${TREEHOUSE_OWNER_STATES[$i]} + state_key=${TREEHOUSE_OWNER_STATE_KEYS[$i]} for other in "$state_dir"/*.meta; do [ -f "$other" ] && [ ! -L "$other" ] || continue - [ "$other" != "$record_meta" ] || continue + [ "$state_key/${other##*/}" != "$record_identity" ] || continue other_id=$(basename "$other" .meta) for field in worktree home; do other_path=$(fm_meta_get "$other" "$field") diff --git a/tests/fm-teardown-endpoint-safety.test.sh b/tests/fm-teardown-endpoint-safety.test.sh index 4002cf4df3e..82e209cf739 100755 --- a/tests/fm-teardown-endpoint-safety.test.sh +++ b/tests/fm-teardown-endpoint-safety.test.sh @@ -596,6 +596,101 @@ test_sole_slot_record_still_tears_down() { pass "fm-teardown: a task that solely holds its slot still returns it" } +# A home reached through a symlinked spelling (the /home -> /nobackup NFS +# layout) must resolve to the same state directory as its physical path, so a +# task's single record is never read twice and reported as its own collision. +run_case_through_home() { # + local dir=$1 id=$2 home=$3 + FM_HOME="$home" FM_ROOT_OVERRIDE="$ROOT" \ + FM_RUNTIME_LOG="$dir/runtime.log" PATH="$dir/fakebin:$PATH" \ + "$TEARDOWN" "$id" --force +} + +test_symlinked_home_spelling_still_tears_down_its_sole_slot() { + local dir id=symlink-home-task + + dir=$(make_case slot-symlinked-home) + mark_case_as_treehouse_pool "$dir" + ln -s "$dir/home" "$dir/home-alias" + fm_write_meta "$dir/home/state/$id.meta" \ + "window=firstmate:fm-$id" "endpoint_task_id=$id" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=scout" + + run_case_through_home "$dir" "$id" "$dir/home-alias" \ + > "$dir/stdout" 2> "$dir/stderr" \ + || fail "teardown through a symlinked home spelling failed: $(cat "$dir/stderr")" + assert_absent "$dir/home/state/$id.meta" "symlinked-home teardown left the task record" + grep -Fq "treehouse " "$dir/runtime.log" \ + || fail "symlinked-home teardown did not return its own pool slot: $(cat "$dir/runtime.log")" + pass "fm-teardown: a home reached through a symlinked path is not its own slot collision" +} + +test_symlinked_home_spelling_still_refuses_a_real_collision() { + local dir id=symlink-stale other=symlink-live rc + + dir=$(make_case slot-symlinked-home-collision) + mark_case_as_treehouse_pool "$dir" + ln -s "$dir/home" "$dir/home-alias" + 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" + + set +e + run_case_through_home "$dir" "$id" "$dir/home-alias" \ + > "$dir/stdout" 2> "$dir/stderr" + rc=$? + set -e + [ "$rc" -ne 0 ] \ + || fail "teardown through a symlinked home spelling returned a contested slot" + assert_present "$dir/home/state/$id.meta" "contested symlinked-home teardown removed the stale record" + assert_present "$dir/home/state/$other.meta" "contested symlinked-home teardown removed the live record" + assert_present "$dir/worktree/sentinel" "contested symlinked-home teardown reset the shared slot" + [ ! -s "$dir/runtime.log" ] \ + || fail "contested symlinked-home teardown reached the runtime: $(cat "$dir/runtime.log")" + assert_contains "$(cat "$dir/stderr")" "$other" \ + "contested symlinked-home refusal should name the other task" + pass "fm-teardown: a genuine slot collision still refuses through a symlinked home spelling" +} + +# Resolving spellings must collapse only the SAME directory: a registered +# Firstmate home reached through a symlinked path is a different home, and its +# records must still be scanned for the slot this task would return. +test_symlinked_registered_home_still_refuses_a_cross_home_collision() { + local dir id=symlink-reg-stale other=symlink-reg-live second_home rc + + dir=$(make_case slot-symlinked-registration) + mark_case_as_treehouse_pool "$dir" + second_home="$dir/secondmate-home" + mkdir -p "$second_home/state" "$second_home/data" + ln -s "$second_home" "$dir/secondmate-alias" + printf '%s\n' "- mate - fixture (home: $dir/secondmate-alias; scope: test; projects: project; added 2026-01-01)" \ + > "$dir/home/data/secondmates.md" + 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 "$second_home/state/$other.meta" \ + "window=firstmate:fm-$other" "endpoint_task_id=$other" \ + "worktree=$dir/worktree" "project=$dir/project" "kind=scout" + + set +e + run_case "$dir" "$id" > "$dir/stdout" 2> "$dir/stderr" + rc=$? + set -e + [ "$rc" -ne 0 ] \ + || fail "teardown returned a slot held by a home registered through a symlinked path" + assert_present "$dir/home/state/$id.meta" "symlinked-registration collision removed the stale record" + assert_present "$second_home/state/$other.meta" "symlinked-registration collision removed the live record" + assert_present "$dir/worktree/sentinel" "symlinked-registration collision reset the shared slot" + [ ! -s "$dir/runtime.log" ] \ + || fail "symlinked-registration collision reached the runtime: $(cat "$dir/runtime.log")" + assert_contains "$(cat "$dir/stderr")" "$other" \ + "symlinked-registration refusal should name the task holding the slot" + pass "fm-teardown: a home registered through a symlinked path is still scanned for slot collisions" +} + test_recorded_endpoint_that_changed_directory_still_tears_down() { local dir id=moved-task @@ -1385,6 +1480,9 @@ test_bare_relative_origin_shares_project_lock_with_clone test_reused_pool_slot_refuses_before_touching_the_other_task test_cross_home_pool_slot_collision_refuses test_sole_slot_record_still_tears_down +test_symlinked_home_spelling_still_tears_down_its_sole_slot +test_symlinked_home_spelling_still_refuses_a_real_collision +test_symlinked_registered_home_still_refuses_a_cross_home_collision test_reassigned_pool_slot_finishes_own_cleanup_without_touching_the_slot test_own_and_absent_slot_claims_still_tear_down test_recorded_endpoint_that_changed_directory_still_tears_down