Skip to content

fix(bin): identify a task record by path identity in the slot-collision scan #5487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
taimurrabuske wants to merge 1 commit into from
Closed

fix(bin): identify a task record by path identity in the slot-collision scan #5487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 53 additions & 9 deletions bin/fm-teardown.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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/<user>/work next to its physical /nobackup/<user>/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() { # <state-dir>
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
Expand All @@ -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" ] || {
Expand Down Expand Up @@ -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() { # <meta-path>
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")
Expand Down
98 changes: 98 additions & 0 deletions tests/fm-teardown-endpoint-safety.test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() { # <case> <id> <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 <return>" "$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

Expand Down Expand Up @@ -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
Expand Down