From 34cc5ad7e673fbe6edb7ab505feac3afca2fcf56 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 14:22:18 +0800 Subject: [PATCH 01/13] fix(lock): bound stale recovery mutex acquisition --- bin/fm-wake-lib.sh | 99 +++++++++++--- tests/fm-grok-continuity-live-e2e.test.sh | 46 ++++++- tests/fm-watcher-lock.test.sh | 155 +++++++++++++++++++++- 3 files changed, 272 insertions(+), 28 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 28249b661f3..ed3f04a9029 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -331,7 +331,7 @@ fm_lock_owner_dir() { fm_lock_prepare_owner() { local ownerdir=$1 mypid back mypid=${BASHPID:-$$} - printf '%s\n' "$mypid" > "$ownerdir/pid" 2>/dev/null || return 1 + printf '%s\n' "$mypid" 2>/dev/null > "$ownerdir/pid" || return 1 back=$(cat "$ownerdir/pid" 2>/dev/null || true) [ "$back" = "$mypid" ] } @@ -404,17 +404,18 @@ fm_lock_claim() { } fm_lock_try_create() { - local lockdir=$1 allowed_steal_owner=${2:-} ownerdir + local lockdir=$1 allowed_steal_owner=${2:-} ownerdir rc FM_LOCK_OWNER_DIR= - ownerdir=$(fm_lock_owner_dir "$lockdir") || return 1 + ownerdir=$(fm_lock_owner_dir "$lockdir") || return 2 if [ -e "$lockdir" ] || [ -L "$lockdir" ]; then fm_lock_discard_owner "$ownerdir" return 1 fi if ! fm_lock_prepare_owner "$ownerdir"; then fm_lock_discard_owner "$ownerdir" - return 1 + return 2 fi + rc=2 if ln -s "$ownerdir" "$lockdir" 2>/dev/null && fm_lock_points_to_owner "$lockdir" "$ownerdir"; then if fm_lock_claim "$lockdir" "$ownerdir" "$allowed_steal_owner"; then FM_LOCK_OWNER_DIR=$ownerdir @@ -426,8 +427,49 @@ fm_lock_try_create() { else fm_lock_remove_stray_owner_link "$lockdir" "$ownerdir" fi + if [ -e "$lockdir" ] || [ -L "$lockdir" ] \ + || [ -e "$lockdir.steal" ] || [ -L "$lockdir.steal" ]; then + rc=1 + fi fm_lock_discard_owner "$ownerdir" - return 1 + return "$rc" +} + +# Acquire a stale-recovery mutex without requesting a second mutex. Status 1 +# means contended; status 2 means the parent or owner record could not be +# created. A dead symlink owner is serialized inside its unique owner directory, +# so interrupted recovery remains reclaimable without creating .steal.steal. +fm_lock_try_acquire_steal_mutex() { + local steal=$1 rc pid ownerdir reclaim + case "$steal" in + *.steal) : ;; + *) return 2 ;; + esac + rc=0 + fm_lock_try_create "$steal" || rc=$? + [ "$rc" -ne 0 ] || return 0 + [ "$rc" -ne 2 ] || return 2 + [ ! -e "$steal.steal" ] && [ ! -L "$steal.steal" ] || return 1 + pid=$(cat "$steal/pid" 2>/dev/null || true) + fm_pid_alive "$pid" && return 1 + fm_lock_mid_acquire_is_fresh "$steal" "$pid" && return 1 + [ -L "$steal" ] || return 1 + ownerdir=$(fm_lock_link_owner "$steal" 2>/dev/null) || return 1 + reclaim="$ownerdir/reclaim" + mkdir "$reclaim" 2>/dev/null || return 1 + if [ -e "$steal.steal" ] || [ -L "$steal.steal" ] \ + || ! fm_lock_recheck_stale_owner "$steal" "$ownerdir" "$pid"; then + rmdir "$reclaim" 2>/dev/null || true + return 1 + fi + if ! rm -f "$steal" 2>/dev/null; then + rmdir "$reclaim" 2>/dev/null || true + return 1 + fi + fm_lock_clean_known_files "$ownerdir" + rmdir "$reclaim" 2>/dev/null || true + rmdir "$ownerdir" 2>/dev/null || true + fm_lock_try_create "$steal" } fm_lock_remove_path() { @@ -788,14 +830,18 @@ fm_lock_try_acquire() { FM_LOCK_OWNER_DIR= FM_LOCK_RECOVERED_PID= - if fm_lock_try_create "$lockdir"; then - return 0 - fi + rc=0 + fm_lock_try_create "$lockdir" || rc=$? + [ "$rc" -ne 0 ] || return 0 + [ "$rc" -ne 2 ] || return 2 # Compare against ${BASHPID:-$$} inline, never via a command substitution: # $() forks a subshell whose BASHPID is not this frame's pid. + # Bash 3 lacks BASHPID and preserves $$ in subshells, so BASH_SUBSHELL keeps + # a child frame from being mistaken for the parent that owns the lock. pid=$(cat "$lockdir/pid" 2>/dev/null || true) - if [ -n "$pid" ] && [ "$pid" = "${BASHPID:-$$}" ]; then + if [ -n "$pid" ] && [ "$pid" = "${BASHPID:-$$}" ] \ + && { [ -n "${BASHPID:-}" ] || [ "${BASH_SUBSHELL:-0}" -eq 0 ]; }; then # The recorded holder is THIS very process. Single-threaded bash can only # observe that when an interrupting trap abandoned the frame that held the # lock mid-critical-section (e.g. TERM inside a recovery-marker section, @@ -805,11 +851,11 @@ fm_lock_try_acquire() { # - the hang reproduced by the self-held reclaim regression in # tests/fm-wake-queue.test.sh - so reclaim the abandoned hold instead. fm_lock_remove_path "$lockdir" || true - if fm_lock_try_create "$lockdir"; then - return 0 - fi + rc=0 + fm_lock_try_create "$lockdir" || rc=$? + [ "$rc" -ne 0 ] || return 0 FM_LOCK_HELD_PID=$(cat "$lockdir/pid" 2>/dev/null || true) - return 1 + return "$rc" fi if fm_pid_alive "$pid"; then FM_LOCK_HELD_PID=$pid @@ -820,11 +866,20 @@ fm_lock_try_acquire() { return 1 fi + case "$lockdir" in + *.steal) + FM_LOCK_HELD_PID=$pid + return 1 + ;; + esac + steal="$lockdir.steal" - if ! fm_lock_try_acquire "$steal"; then + rc=0 + fm_lock_try_acquire_steal_mutex "$steal" || rc=$? + if [ "$rc" -ne 0 ]; then FM_LOCK_HELD_PID=$(cat "$lockdir/pid" 2>/dev/null || true) FM_LOCK_OWNER_DIR= - return 1 + return "$rc" fi steal_owner=${FM_LOCK_OWNER_DIR:-} @@ -868,9 +923,9 @@ fm_lock_try_acquire() { return 1 fi fm_lock_remove_path "$lockdir" || true - rc=1 - if fm_lock_try_create "$lockdir" "$steal_owner"; then - rc=0 + rc=0 + fm_lock_try_create "$lockdir" "$steal_owner" || rc=$? + if [ "$rc" -eq 0 ]; then # shellcheck disable=SC2034 # Read by sourcing callers after lock acquisition. FM_LOCK_RECOVERED_PID=$cur fi @@ -884,8 +939,12 @@ fm_lock_try_acquire() { } fm_lock_acquire_wait() { - local lockdir=$1 - while ! fm_lock_try_acquire "$lockdir"; do + local lockdir=$1 rc + while :; do + rc=0 + fm_lock_try_acquire "$lockdir" || rc=$? + [ "$rc" -ne 0 ] || return 0 + [ "$rc" -ne 2 ] || return 2 sleep 0.1 done } diff --git a/tests/fm-grok-continuity-live-e2e.test.sh b/tests/fm-grok-continuity-live-e2e.test.sh index 2caf9ab74f8..442a31491d2 100755 --- a/tests/fm-grok-continuity-live-e2e.test.sh +++ b/tests/fm-grok-continuity-live-e2e.test.sh @@ -50,19 +50,51 @@ lab_pid_is_safe() { esac } +wait_lab_pid_exit() { + local pid=$1 i=0 + [ -n "$pid" ] || return 0 + while lab_pid_is_safe "$pid" && [ "$i" -lt 50 ]; do + sleep 0.1 + i=$((i + 1)) + done + if lab_pid_is_safe "$pid"; then + kill -KILL "$pid" 2>/dev/null || true + kill -CONT "$pid" 2>/dev/null || true + fi + i=0 + while lab_pid_is_safe "$pid" && [ "$i" -lt 50 ]; do + sleep 0.1 + i=$((i + 1)) + done + ! lab_pid_is_safe "$pid" +} + cleanup() { - local watcher_pid arm_pid + local coordinator_pid watcher_pid arm_pid pid cleanup_failed=0 + coordinator_pid=$(sed -n 's/^pid=//p' "$HOME_DIR/state/.grok-watch-coordinator" 2>/dev/null || true) watcher_pid=$(cat "$HOME_DIR/state/.watch.lock/pid" 2>/dev/null || true) arm_pid=$(ps -p "$watcher_pid" -o ppid= 2>/dev/null | tr -d ' ' || true) "$TMUX" -L "$SOCKET" kill-server 2>/dev/null || true - sleep 0.1 - if [ -n "$watcher_pid" ] && lab_pid_is_safe "$watcher_pid"; then - kill -TERM "$watcher_pid" 2>/dev/null || true - fi - if [ -n "$arm_pid" ] && lab_pid_is_safe "$arm_pid"; then - kill -TERM "$arm_pid" 2>/dev/null || true + for pid in "$coordinator_pid" "$watcher_pid" "$arm_pid"; do + if [ -n "$pid" ] && lab_pid_is_safe "$pid"; then + kill -TERM "$pid" 2>/dev/null || true + kill -CONT "$pid" 2>/dev/null || true + fi + done + for pid in "$coordinator_pid" "$watcher_pid" "$arm_pid"; do + wait_lab_pid_exit "$pid" || cleanup_failed=1 + done + if [ "$cleanup_failed" -ne 0 ]; then + trap - EXIT + printf 'not ok - Grok live E2E cleanup could not retire every lab-owned coordinator, watcher, and arm\n' >&2 + exit 1 fi rm -rf "$LAB" + if [ -e "$LAB" ]; then + trap - EXIT + printf 'not ok - Grok live E2E cleanup could not remove the retired lab\n' >&2 + exit 1 + fi } trap cleanup EXIT diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index a3628b1694f..dc0f439ec26 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -220,6 +220,155 @@ test_lock_single_winner_under_concurrency() { pass "concurrent fm_lock_try_acquire yields exactly one winner" } +test_lock_missing_parent_returns_typed_failure_with_bounded_launches() { + local dir state missing fakebin count pidfile command_name real_command out rc elapsed result wait_result launches attempt_pid + dir=$(make_case lock-missing-parent) + state="$dir/state" + missing="$dir/absent/demo.lock" + fakebin="$dir/countbin" + count="$dir/helper-launches" + pidfile="$dir/attempt-pid" + mkdir -p "$fakebin" + for command_name in basename cat date dirname ln mkdir mktemp readlink rm rmdir stat uname; do + real_command=$(command -v "$command_name") + cat > "$fakebin/$command_name" </dev/null || true +count=\$((count + 1)) +printf '%s\n' "\$count" > "\$FM_TEST_LAUNCH_COUNT" +if [ "\$count" -gt "\${FM_TEST_LAUNCH_BUDGET:?}" ]; then + kill -TERM "\${FM_TEST_ROOT_PID:?}" 2>/dev/null || true + exit 97 +fi +exec "$real_command" "\$@" +SH + chmod +x "$fakebin/$command_name" + done + + rc=0 + SECONDS=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_LAUNCH_COUNT="$count" \ + FM_TEST_LAUNCH_BUDGET=18 bash -c ' + FM_TEST_ROOT_PID=${BASHPID:-$$} + export FM_TEST_ROOT_PID + . "$1" + printf "0\n" > "$3" + printf "%s\n" "$FM_TEST_ROOT_PID" > "$4" + trap "exit 97" TERM + i=0 + while [ "$i" -lt 5 ]; do + fm_lock_try_acquire "$2" + result=$? + [ "$result" -eq 2 ] || exit 20 + i=$((i + 1)) + done + fm_lock_acquire_wait "$2" + wait_result=$? + [ "$wait_result" -eq 2 ] || exit 21 + read -r launches < "$3" + printf "result=%s wait_result=%s launches=%s\n" "$result" "$wait_result" "$launches" + ' _ "$LIB" "$missing" "$count" "$pidfile" 2>&1) || rc=$? + elapsed=$SECONDS + + [ "$rc" -eq 0 ] || fail "missing-parent lock attempt did not return typed invalid-path status within its launch fuse (rc=$rc): $out" + result=${out#*result=}; result=${result%% *} + wait_result=${out#*wait_result=}; wait_result=${wait_result%% *} + launches=${out#*launches=}; launches=${launches%%[!0-9]*} + [ "$result" -eq 2 ] || fail "missing-parent lock attempt returned '$result' instead of typed invalid-path status 2: $out" + [ "$wait_result" -eq 2 ] || fail "missing-parent lock wait returned '$wait_result' instead of propagating status 2: $out" + [ "$launches" -le 18 ] || fail "five missing-parent failures plus one wait exceeded the helper-launch budget ($launches): $out" + [ "$elapsed" -lt 3 ] || fail "missing-parent lock attempts did not return promptly (${elapsed}s): $out" + attempt_pid=$(cat "$pidfile") + [ -z "$(pgrep -P "$attempt_pid" 2>/dev/null || true)" ] \ + || fail "missing-parent lock attempt left a spawned descendant alive" + pass "missing-parent lock failure is typed, prompt, descendant-free, and launch-bounded" +} + +test_lock_owner_record_failure_returns_typed_failure() { + local dir state lockdir fakebin count real_mktemp out rc + dir=$(make_case lock-owner-record-failure) + state="$dir/state" + lockdir="$state/.owner-failure.lock" + fakebin="$dir/countbin" + count="$dir/mktemp-launches" + real_mktemp=$(command -v mktemp) + mkdir -p "$fakebin" + cat > "$fakebin/mktemp" </dev/null || true +count=\$((count + 1)) +printf '%s\n' "\$count" > "\$FM_TEST_LAUNCH_COUNT" +if [ "\$count" -gt 4 ]; then + kill -TERM "\${FM_TEST_ROOT_PID:?}" 2>/dev/null || true + exit 97 +fi +ownerdir=\$("$real_mktemp" "\$@") || exit \$? +chmod 0500 "\$ownerdir" || exit 1 +printf '%s\n' "\$ownerdir" +SH + chmod +x "$fakebin/mktemp" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_LAUNCH_COUNT="$count" bash -c ' + . "$1" + printf "0\n" > "$3" + FM_TEST_ROOT_PID=${BASHPID:-$$} + export FM_TEST_ROOT_PID + trap "exit 97" TERM + fm_lock_try_acquire "$2" + rc=$? + printf "rc=%s\n" "$rc" + [ "$rc" -eq 2 ] + ' _ "$LIB" "$lockdir" "$count" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "owner-record creation failure did not return typed status 2 promptly (rc=$rc): $out" + [ "$out" = "rc=2" ] || fail "owner-record creation failure returned an unexpected result: $out" + pass "owner-record creation failure returns typed invalid-create status promptly" +} + +test_stale_or_malformed_steal_mutex_never_claims_nested_mutex() { + local kind dir state lockdir steal ownerdir fakebin log real_ln dead out rc + for kind in stale malformed; do + dir=$(make_case "lock-no-nested-steal-$kind") + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + fakebin="$dir/logbin" + log="$dir/ln-targets" + real_ln=$(command -v ln) + dead=$(dead_pid) + mkdir "$lockdir" "$fakebin" + printf '%s\n' "$dead" > "$lockdir/pid" + if [ "$kind" = stale ]; then + ownerdir="$state/.stale-steal-owner" + mkdir "$ownerdir" + printf '%s\n' "$dead" > "$ownerdir/pid" + ln -s "$ownerdir" "$steal" + else + ln -s "$state/.missing-steal-owner" "$steal" + fi + cat > "$fakebin/ln" <> "\${FM_TEST_LN_LOG:?}" +exec "$real_ln" "\$@" +SH + chmod +x "$fakebin/ln" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_LN_LOG="$log" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "$kind steal-mutex fixture shell failed (rc=$rc): $out" + ! grep -F "$lockdir.steal.steal" "$log" >/dev/null 2>&1 \ + || fail "$kind steal mutex attempted a nested .steal.steal claim: $(cat "$log")" + done + pass "stale and malformed steal mutexes never claim .steal.steal" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -321,7 +470,8 @@ test_lock_does_not_steal_live_lock() { printf '%s\n' "$live" > "$lockdir/pid" out=$(FM_STATE_OVERRIDE="$state" bash -c ' . "$1" - if fm_lock_try_acquire "$2"; then rc=0; else rc=1; fi + rc=0 + fm_lock_try_acquire "$2" || rc=$? printf "rc=%s held=%s\n" "$rc" "${FM_LOCK_HELD_PID:-}" ' _ "$LIB" "$lockdir") kill "$live" 2>/dev/null || true @@ -1107,6 +1257,9 @@ test_stale_watch_reclaim_publishes_before_clear test_live_stale_watch_lock_is_actionable test_guard_warnings test_lock_single_winner_under_concurrency +test_lock_missing_parent_returns_typed_failure_with_bounded_launches +test_lock_owner_record_failure_returns_typed_failure +test_stale_or_malformed_steal_mutex_never_claims_nested_mutex test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From db7bdcd7f21146d2312a7c1bfd4dbe95e0c7eea2 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 14:22:23 +0800 Subject: [PATCH 02/13] test(tool-updates): compare line counts numerically --- tests/fm-tool-update-check.test.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/fm-tool-update-check.test.sh b/tests/fm-tool-update-check.test.sh index 2f8843a821d..a0a7a4e0da4 100755 --- a/tests/fm-tool-update-check.test.sh +++ b/tests/fm-tool-update-check.test.sh @@ -131,7 +131,7 @@ test_path_skew_is_reported_from_every_copy() { assert_contains "$report" "0.8.2 is installed at $fresh/$TOOL" "the report does not name the newer installed copy, so no other PATH copy was asked for its version" assert_not_contains "$report" "update available" "PATH skew must not be reported as a published update" assert_contains "$report" "$(printf 'tool updates:')" "the report is missing its one-line prefix" - [ "$(wc -l < "$out")" = 1 ] || fail "the report must be exactly one line for the wake record" + [ "$(wc -l < "$out")" -eq 1 ] || fail "the report must be exactly one line for the wake record" pass "PATH skew is reported by asking every copy on PATH for its own version" } @@ -311,7 +311,7 @@ test_one_broken_pattern_does_not_blind_the_rest_of_the_sweep() { report=$(cat "$out") assert_contains "$report" "herdr update not in effect: PATH resolves 0.8.0 at $stale/$TOOL" "a broken pattern on another tool suppressed the PATH skew report" assert_contains "$report" "no-mistakes check failed: announce_pattern is not a usable extended regular expression" "the tool whose pattern cannot be used was not named" - [ "$(wc -l < "$out")" = 1 ] || fail "the report must stay exactly one line" + [ "$(wc -l < "$out")" -eq 1 ] || fail "the report must stay exactly one line" pass "a broken pattern is reported for its own tool and the rest of the sweep still reports" } @@ -690,7 +690,7 @@ test_an_overlong_report_says_it_was_cut() { run_check "$home" "$PATH" "$out" report=$(cat "$out") assert_contains "$report" "[truncated]" "an over-long report was cut without saying so" - [ "$(wc -l < "$out")" = 1 ] || fail "the cut report must still be exactly one line" + [ "$(wc -l < "$out")" -eq 1 ] || fail "the cut report must still be exactly one line" pass "an over-long report is cut with the shared truncation marker" } From c5aa4355f6efeadc7a3fa24ad6571bd99414b951 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 16:42:50 +0800 Subject: [PATCH 03/13] test(lock): tolerate loaded-host timing --- tests/fm-watcher-lock.test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index dc0f439ec26..16d4114b2dc 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -278,7 +278,7 @@ SH [ "$result" -eq 2 ] || fail "missing-parent lock attempt returned '$result' instead of typed invalid-path status 2: $out" [ "$wait_result" -eq 2 ] || fail "missing-parent lock wait returned '$wait_result' instead of propagating status 2: $out" [ "$launches" -le 18 ] || fail "five missing-parent failures plus one wait exceeded the helper-launch budget ($launches): $out" - [ "$elapsed" -lt 3 ] || fail "missing-parent lock attempts did not return promptly (${elapsed}s): $out" + [ "$elapsed" -lt 10 ] || fail "missing-parent lock attempts did not return promptly (${elapsed}s): $out" attempt_pid=$(cat "$pidfile") [ -z "$(pgrep -P "$attempt_pid" 2>/dev/null || true)" ] \ || fail "missing-parent lock attempt left a spawned descendant alive" From 2dd2bb7199048ef5d306fa0fdd54859595bc7672 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 17:03:27 +0800 Subject: [PATCH 04/13] no-mistakes(review): fix lock status typing, steal reclaim, and E2E cleanup --- bin/fm-wake-lib.sh | 55 ++++++++++++++++++----- tests/fm-grok-continuity-live-e2e.test.sh | 20 ++++++++- tests/fm-watcher-lock.test.sh | 34 +++++++++++++- 3 files changed, 97 insertions(+), 12 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index ed3f04a9029..c248c38fb81 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -415,7 +415,7 @@ fm_lock_try_create() { fm_lock_discard_owner "$ownerdir" return 2 fi - rc=2 + rc=1 if ln -s "$ownerdir" "$lockdir" 2>/dev/null && fm_lock_points_to_owner "$lockdir" "$ownerdir"; then if fm_lock_claim "$lockdir" "$ownerdir" "$allowed_steal_owner"; then FM_LOCK_OWNER_DIR=$ownerdir @@ -427,10 +427,6 @@ fm_lock_try_create() { else fm_lock_remove_stray_owner_link "$lockdir" "$ownerdir" fi - if [ -e "$lockdir" ] || [ -L "$lockdir" ] \ - || [ -e "$lockdir.steal" ] || [ -L "$lockdir.steal" ]; then - rc=1 - fi fm_lock_discard_owner "$ownerdir" return "$rc" } @@ -455,23 +451,62 @@ fm_lock_try_acquire_steal_mutex() { fm_lock_mid_acquire_is_fresh "$steal" "$pid" && return 1 [ -L "$steal" ] || return 1 ownerdir=$(fm_lock_link_owner "$steal" 2>/dev/null) || return 1 + if [ ! -d "$ownerdir" ]; then + fm_lock_points_to_owner "$steal" "$ownerdir" || return 1 + rm -f "$steal" 2>/dev/null || return 1 + rc=0 + fm_lock_try_create "$steal" || rc=$? + return "$rc" + fi reclaim="$ownerdir/reclaim" - mkdir "$reclaim" 2>/dev/null || return 1 + fm_lock_reclaim_marker_claim "$reclaim" || return 1 if [ -e "$steal.steal" ] || [ -L "$steal.steal" ] \ || ! fm_lock_recheck_stale_owner "$steal" "$ownerdir" "$pid"; then - rmdir "$reclaim" 2>/dev/null || true + fm_lock_reclaim_marker_release "$reclaim" return 1 fi if ! rm -f "$steal" 2>/dev/null; then - rmdir "$reclaim" 2>/dev/null || true + fm_lock_reclaim_marker_release "$reclaim" return 1 fi fm_lock_clean_known_files "$ownerdir" - rmdir "$reclaim" 2>/dev/null || true + fm_lock_reclaim_marker_release "$reclaim" rmdir "$ownerdir" 2>/dev/null || true fm_lock_try_create "$steal" } +# The reclaim marker serializes stale steal-mutex recovery. It records the +# reclaimer's pid so a reclaimer killed mid-recovery cannot wedge every later +# reclaimer: a marker whose pid is dead and whose age passed the stale window is +# itself reclaimable, exactly like every other stale record in this file. +fm_lock_reclaim_marker_claim() { + local reclaim=$1 mypid + mypid=${BASHPID:-$$} + if ! mkdir "$reclaim" 2>/dev/null; then + fm_lock_reclaim_marker_is_abandoned "$reclaim" || return 1 + fm_lock_reclaim_marker_release "$reclaim" + mkdir "$reclaim" 2>/dev/null || return 1 + fi + printf '%s\n' "$mypid" > "$reclaim/pid" 2>/dev/null || true + return 0 +} + +fm_lock_reclaim_marker_is_abandoned() { + local reclaim=$1 pid stale + [ -d "$reclaim" ] || return 1 + pid=$(cat "$reclaim/pid" 2>/dev/null || true) + fm_pid_alive "$pid" && return 1 + stale=$FM_LOCK_STALE_AFTER + [ "$stale" -lt 2 ] && stale=2 + [ "$(fm_path_age "$reclaim")" -ge "$stale" ] +} + +fm_lock_reclaim_marker_release() { + local reclaim=$1 + rm -f "$reclaim/pid" 2>/dev/null || true + rmdir "$reclaim" 2>/dev/null || true +} + fm_lock_remove_path() { local lockdir=$1 ownerdir if [ -L "$lockdir" ]; then @@ -1165,7 +1200,7 @@ fm_autoarm_release_abandoned() { # lock="$state/.claude-autoarm.lock" steal="$lock.steal" fm_autoarm_claim_abandoned "$state" || return 1 - fm_lock_try_acquire "$steal" || return 1 + fm_lock_try_acquire_steal_mutex "$steal" || return 1 if ! fm_autoarm_claim_abandoned "$state"; then fm_lock_release "$steal" return 1 diff --git a/tests/fm-grok-continuity-live-e2e.test.sh b/tests/fm-grok-continuity-live-e2e.test.sh index 442a31491d2..46143cf8512 100755 --- a/tests/fm-grok-continuity-live-e2e.test.sh +++ b/tests/fm-grok-continuity-live-e2e.test.sh @@ -72,6 +72,9 @@ wait_lab_pid_exit() { cleanup() { local coordinator_pid watcher_pid arm_pid pid cleanup_failed=0 coordinator_pid=$(sed -n 's/^pid=//p' "$HOME_DIR/state/.grok-watch-coordinator" 2>/dev/null || true) + if [ "${COORDINATOR_RECORDED:-0}" = 1 ] && [ -z "$coordinator_pid" ]; then + cleanup_failed=1 + fi watcher_pid=$(cat "$HOME_DIR/state/.watch.lock/pid" 2>/dev/null || true) arm_pid=$(ps -p "$watcher_pid" -o ppid= 2>/dev/null | tr -d ' ' || true) "$TMUX" -L "$SOCKET" kill-server 2>/dev/null || true @@ -105,9 +108,24 @@ mkdir -p "$HOME_DIR/state" "$HOME_DIR/config" printf 'project=fixture\n' > "$HOME_DIR/state/grok-e2e.meta" "$TMUX" -L "$SOCKET" new-session -d -s "$SESSION" -c "$PROJECT" \ - "env FM_HOME='$HOME_DIR' FM_ROOT_OVERRIDE='$PROJECT' FM_POLL=1 FM_SIGNAL_GRACE=0 FM_HEARTBEAT=600 bash -lc 'printf \"%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.lock\"; grok --trust --always-approve --reasoning-effort low; rc=\$?; printf \"GROK_EXIT=%s\\n\" \"\$rc\"; sleep 300'" + "env FM_HOME='$HOME_DIR' FM_ROOT_OVERRIDE='$PROJECT' FM_POLL=1 FM_SIGNAL_GRACE=0 FM_HEARTBEAT=600 bash -lc 'printf \"pid=%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.grok-watch-coordinator\"; printf \"%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.lock\"; grok --trust --always-approve --reasoning-effort low; rc=\$?; printf \"GROK_EXIT=%s\\n\" \"\$rc\"; sleep 300'" wait_for_text "Grok Build" 180 || fail "Grok did not reach its ready composer" + +i=0 +coordinator_pid= +while [ "$i" -lt 60 ]; do + coordinator_pid=$(sed -n 's/^pid=//p' "$HOME_DIR/state/.grok-watch-coordinator" 2>/dev/null || true) + [ -n "$coordinator_pid" ] && break + sleep 0.5 + i=$((i + 1)) +done +[ -n "$coordinator_pid" ] \ + || fail "the lab coordinator did not record its pid, so cleanup cannot retire it before removing the lab" +lab_pid_is_safe "$coordinator_pid" \ + || fail "the recorded coordinator pid is not a lab-owned process" +COORDINATOR_RECORDED=1 + sleep 1 # shellcheck disable=SC2016 # Backticks are literal prompt markup. PROMPT='Use run_terminal_command with background=true to run exactly `bin/fm-watch-arm.sh`. Never use a shell ampersand. Once it reports started, respond briefly.' diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 16d4114b2dc..06a3e0d7083 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -347,6 +347,7 @@ test_stale_or_malformed_steal_mutex_never_claims_nested_mutex() { ln -s "$ownerdir" "$steal" else ln -s "$state/.missing-steal-owner" "$steal" + touch -h -t 202001010000 "$steal" fi cat > "$fakebin/ln" <&1) || rc=$? [ "$rc" -eq 0 ] || fail "$kind steal-mutex fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "$kind steal mutex was never reclaimed, so the dead-owner lock stayed unacquirable: $out" ! grep -F "$lockdir.steal.steal" "$log" >/dev/null 2>&1 \ || fail "$kind steal mutex attempted a nested .steal.steal claim: $(cat "$log")" done - pass "stale and malformed steal mutexes never claim .steal.steal" + pass "stale and malformed steal mutexes are reclaimed without ever claiming .steal.steal" +} + +test_abandoned_reclaim_marker_does_not_wedge_stale_steal_recovery() { + local dir state lockdir steal ownerdir dead out rc + dir=$(make_case lock-abandoned-reclaim-marker) + state="$dir/state" + lockdir="$state/.wedged.lock" + steal="$lockdir.steal" + ownerdir="$state/.stale-steal-owner" + dead=$(dead_pid) + mkdir "$lockdir" "$ownerdir" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$ownerdir/pid" + ln -s "$ownerdir" "$steal" + mkdir "$ownerdir/reclaim" + touch -t 202001010000 "$ownerdir/reclaim" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "abandoned-reclaim-marker fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "a reclaim marker left behind by a killed reclaimer permanently blocked stale steal recovery: $out" + [ ! -d "$ownerdir/reclaim" ] || fail "the reclaimed marker was not released" + pass "a reclaim marker abandoned by a killed reclaimer does not wedge stale steal recovery" } test_lock_steals_dead_pid_lock() { @@ -1260,6 +1291,7 @@ test_lock_single_winner_under_concurrency test_lock_missing_parent_returns_typed_failure_with_bounded_launches test_lock_owner_record_failure_returns_typed_failure test_stale_or_malformed_steal_mutex_never_claims_nested_mutex +test_abandoned_reclaim_marker_does_not_wedge_stale_steal_recovery test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From d95a4d7f9dc4f7fcc0489dc3f387a135ce3ab283 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 17:19:08 +0800 Subject: [PATCH 05/13] no-mistakes(review): serialize steal reclaim, retire nested-steal debris, fail closed --- bin/fm-afk-return.sh | 2 +- bin/fm-lock.sh | 2 +- bin/fm-startup-network.sh | 16 ++-- bin/fm-wake-drain.sh | 4 +- bin/fm-wake-lib.sh | 57 +++++++++++--- bin/fm-watch.sh | 2 +- bin/fm-x-lib.sh | 6 +- tests/fm-watcher-lock.test.sh | 140 +++++++++++++++++++++++++++++++++- 8 files changed, 200 insertions(+), 29 deletions(-) diff --git a/bin/fm-afk-return.sh b/bin/fm-afk-return.sh index cf5addb24cf..88fb27792c4 100755 --- a/bin/fm-afk-return.sh +++ b/bin/fm-afk-return.sh @@ -228,7 +228,7 @@ main() { . "$SCRIPT_DIR/fm-classify-lib.sh" mkdir -p "$STATE" || return 1 - fm_lock_acquire_wait "$LOCK" + fm_lock_acquire_wait "$LOCK" || return 1 trap 'fm_lock_release "$LOCK"' EXIT write_pending_seed || { fm_lock_release "$LOCK"; trap - EXIT; return 1; } return_reconcile diff --git a/bin/fm-lock.sh b/bin/fm-lock.sh index 52d7c8aee4b..207b8a099ad 100755 --- a/bin/fm-lock.sh +++ b/bin/fm-lock.sh @@ -73,7 +73,7 @@ if ! fm_lock_try_acquire "$CLAIM_LOCK"; then echo "error: the prior session's bounded startup sweep is finishing; operate read-only until it releases the fleet lock" >&2 exit 1 fi - fm_lock_acquire_wait "$CLAIM_LOCK" + fm_lock_acquire_wait "$CLAIM_LOCK" || exit 1 fi CLAIM_LOCK_HELD=1 diff --git a/bin/fm-startup-network.sh b/bin/fm-startup-network.sh index 3cc9097b739..2efe656a229 100755 --- a/bin/fm-startup-network.sh +++ b/bin/fm-startup-network.sh @@ -200,7 +200,7 @@ cmd_start() { # return 1 fi - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get state)" = running ] && worker_alive \ && { [ "$locked" != 1 ] || [ "$(status_get lock_pid)" = "$lock_pid" ]; }; then # A worker from this or a previous session is still going. Starting a second @@ -299,7 +299,7 @@ await_delivery() { # limit=$(( $(delivery_budget) * 10 )) while [ "$waited" -lt "$limit" ]; do claim_live=0 - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get generation)" != "$generation" ]; then fm_lock_release "$PUBLISH_LOCK" return 0 @@ -332,7 +332,7 @@ EOF sleep 0.1 waited=$((waited + 1)) done - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get generation)" != "$generation" ] || [ -f "$DELIVERED_FILE" ]; then fm_lock_release "$PUBLISH_LOCK" return 0 @@ -345,7 +345,7 @@ EOF publish() { # local generation=$1 state=$2 phases=$3 locked=$4 started=$5 rc=$6 out=$7 timings=${8:-} report_published=1 - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get generation)" != "$generation" ]; then fm_lock_release "$PUBLISH_LOCK" return 0 @@ -387,7 +387,7 @@ cmd_run() { # budget=$(stage_budget) phases=probe if [ -n "$generation" ]; then - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get generation)" = "$generation" ] && [ "$(status_get pid)" = "$$" ]; then internal=1 started=$(status_get started) @@ -410,7 +410,7 @@ cmd_run() { # if [ "$internal" -eq 0 ]; then generation="$(now).$$.manual" - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 if [ "$(status_get state)" = running ] && worker_alive; then fm_lock_release "$PUBLISH_LOCK" return 1 @@ -438,7 +438,7 @@ EOF stage_started=$(fm_timing_now_ms) rc=0 if [ "$sweep_locked" -eq 1 ]; then - fm_lock_acquire_wait "$STATE/.lock.acquire" + fm_lock_acquire_wait "$STATE/.lock.acquire" || return 1 lease_held=1 if ! lock_unchanged "$lock_pid"; then sweep_locked=0 @@ -544,7 +544,7 @@ print_state() { cmd_harvest() { # local pid=$1 generation state claim_record claim_generation claim_pid - fm_lock_acquire_wait "$PUBLISH_LOCK" + fm_lock_acquire_wait "$PUBLISH_LOCK" || return 1 generation=$(status_get generation) # Another session's live claim is left alone; the worker reaps a dead one. if [ -f "$CLAIM_FILE" ]; then diff --git a/bin/fm-wake-drain.sh b/bin/fm-wake-drain.sh index 203765be80f..80e969ad705 100755 --- a/bin/fm-wake-drain.sh +++ b/bin/fm-wake-drain.sh @@ -278,7 +278,7 @@ trap cleanup EXIT trap 'exit 130' INT trap 'exit 143' TERM -fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" +fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" || exit 1 DRAIN_LOCK_HELD=true if [ -n "$ACK_THROUGH" ]; then @@ -291,7 +291,7 @@ if [ -n "$ACK_THROUGH" ]; then echo "wake drain: inactive outcome receipt could not be recorded safely" >&2 exit 1 fi - fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" + fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" || exit 1 DRAIN_LOCK_HELD=true DRAIN_TMP=$(mktemp "$STATE/.wake-queue.ack.XXXXXX") || exit 1 chmod 0600 "$DRAIN_TMP" || exit 1 diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index c248c38fb81..a30ac8c337e 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -445,22 +445,28 @@ fm_lock_try_acquire_steal_mutex() { fm_lock_try_create "$steal" || rc=$? [ "$rc" -ne 0 ] || return 0 [ "$rc" -ne 2 ] || return 2 - [ ! -e "$steal.steal" ] && [ ! -L "$steal.steal" ] || return 1 + ! fm_lock_legacy_nested_steal_blocks "$steal.steal" || return 1 pid=$(cat "$steal/pid" 2>/dev/null || true) fm_pid_alive "$pid" && return 1 fm_lock_mid_acquire_is_fresh "$steal" "$pid" && return 1 [ -L "$steal" ] || return 1 ownerdir=$(fm_lock_link_owner "$steal" 2>/dev/null) || return 1 if [ ! -d "$ownerdir" ]; then - fm_lock_points_to_owner "$steal" "$ownerdir" || return 1 - rm -f "$steal" 2>/dev/null || return 1 - rc=0 - fm_lock_try_create "$steal" || rc=$? - return "$rc" + case "${ownerdir##*/}" in + "${steal##*/}".owner.*) : ;; + *) return 1 ;; + esac + if ! mkdir "$ownerdir" 2>/dev/null; then + [ -d "$ownerdir" ] || return 1 + fi + if ! fm_lock_points_to_owner "$steal" "$ownerdir"; then + rmdir "$ownerdir" 2>/dev/null || true + return 1 + fi fi reclaim="$ownerdir/reclaim" fm_lock_reclaim_marker_claim "$reclaim" || return 1 - if [ -e "$steal.steal" ] || [ -L "$steal.steal" ] \ + if fm_lock_legacy_nested_steal_blocks "$steal.steal" \ || ! fm_lock_recheck_stale_owner "$steal" "$ownerdir" "$pid"; then fm_lock_reclaim_marker_release "$reclaim" return 1 @@ -480,14 +486,22 @@ fm_lock_try_acquire_steal_mutex() { # reclaimer: a marker whose pid is dead and whose age passed the stale window is # itself reclaimable, exactly like every other stale record in this file. fm_lock_reclaim_marker_claim() { - local reclaim=$1 mypid + local reclaim=$1 mypid retired mypid=${BASHPID:-$$} if ! mkdir "$reclaim" 2>/dev/null; then fm_lock_reclaim_marker_is_abandoned "$reclaim" || return 1 - fm_lock_reclaim_marker_release "$reclaim" + retired="$reclaim.dead.$mypid" + rm -rf "$retired" 2>/dev/null || true + mv "$reclaim" "$retired" 2>/dev/null || return 1 + rm -rf "$retired" 2>/dev/null || true mkdir "$reclaim" 2>/dev/null || return 1 fi - printf '%s\n' "$mypid" > "$reclaim/pid" 2>/dev/null || true + if ! { printf '%s\n' "$mypid" 2>/dev/null > "$reclaim/pid"; } \ + || [ "$(cat "$reclaim/pid" 2>/dev/null || true)" != "$mypid" ]; then + rm -f "$reclaim/pid" 2>/dev/null || true + rmdir "$reclaim" 2>/dev/null || true + return 1 + fi return 0 } @@ -503,8 +517,27 @@ fm_lock_reclaim_marker_is_abandoned() { fm_lock_reclaim_marker_release() { local reclaim=$1 + [ "$(cat "$reclaim/pid" 2>/dev/null || true)" = "${BASHPID:-$$}" ] || return 1 rm -f "$reclaim/pid" 2>/dev/null || true rmdir "$reclaim" 2>/dev/null || true + return 0 +} + +# A leftover .steal.steal can only come from a pre-upgrade recursive reclaimer. +# It still blocks while its owner is live or too fresh to judge; once the owner +# is dead and the residue passed the stale window it is retired here so the +# non-recursive path can proceed without ever creating a nested mutex. +fm_lock_legacy_nested_steal_blocks() { + local residue=$1 pid stale + [ -e "$residue" ] || [ -L "$residue" ] || return 1 + pid=$(cat "$residue/pid" 2>/dev/null || true) + fm_pid_alive "$pid" && return 0 + stale=$FM_LOCK_STALE_AFTER + [ "$stale" -lt 2 ] && stale=2 + [ "$(fm_path_age "$residue")" -lt "$stale" ] && return 0 + fm_lock_remove_path "$residue" >/dev/null 2>&1 || true + [ -e "$residue" ] || [ -L "$residue" ] || return 1 + return 0 } fm_lock_remove_path() { @@ -1230,7 +1263,7 @@ fm_wake_append() { recovery_marker="$STATE/.watcher-down" status=0 - fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" + fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" || return 1 _fm_recovery_marker_publish "$recovery_marker" downtime || status=$? if [ "$status" -eq 0 ]; then seq=$(cat "$seq_file" 2>/dev/null || echo 0) @@ -1259,7 +1292,7 @@ fm_wake_queued_keys() { signal|stale|check|heartbeat) ;; *) printf 'fm_wake_queued_keys: invalid wake kind: %s\n' "$kind" >&2; return 2 ;; esac - fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" + fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" || return 1 fm_wake_queued_keys_locked "$kind" fm_lock_release "$FM_WAKE_QUEUE_LOCK" } diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index a01af557e27..7c45cf8ccae 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -645,7 +645,7 @@ procevent_surface_queued() { local key reason PROCEVENT_SURFACED= [ -s "$FM_WAKE_QUEUE" ] || return 0 - fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" + fm_lock_acquire_wait "$FM_WAKE_QUEUE_LOCK" || return 1 while IFS= read -r key; do case "$key" in procevent:*) ;; *) continue ;; esac [ -e "$(procevent_surfaced_marker "$key")" ] && continue diff --git a/bin/fm-x-lib.sh b/bin/fm-x-lib.sh index 447d7cd4400..83742ec749a 100644 --- a/bin/fm-x-lib.sh +++ b/bin/fm-x-lib.sh @@ -939,7 +939,7 @@ fmx_meta_link_set() { local meta=$1 rid=$2 ts=$3 followups=${4:-0} platform=${5:-} reply_max=${6:-} tmp lock [ -f "$meta" ] || return 1 lock=$(fm_meta_lock_path "$meta") || return 1 - fm_lock_acquire_wait "$lock" + fm_lock_acquire_wait "$lock" || return 1 [ -f "$meta" ] || { fm_lock_release "$lock"; return 1; } tmp=$(fmx_meta_tmp "$meta") || { fm_lock_release "$lock"; return 1; } if ! { grep -vE '^x_request=|^x_request_ts=|^x_followups=|^x_platform=|^x_reply_max_chars=' "$meta" || true; } > "$tmp"; then @@ -966,7 +966,7 @@ fmx_meta_followups_set() { local meta=$1 n=$2 tmp lock [ -f "$meta" ] || return 1 lock=$(fm_meta_lock_path "$meta") || return 1 - fm_lock_acquire_wait "$lock" + fm_lock_acquire_wait "$lock" || return 1 [ -f "$meta" ] || { fm_lock_release "$lock"; return 1; } tmp=$(fmx_meta_tmp "$meta") || { fm_lock_release "$lock"; return 1; } if ! { grep -vE '^x_followups=' "$meta" || true; } > "$tmp"; then @@ -985,7 +985,7 @@ fmx_meta_link_clear() { local meta=$1 tmp lock [ -f "$meta" ] || return 0 lock=$(fm_meta_lock_path "$meta") || return 1 - fm_lock_acquire_wait "$lock" + fm_lock_acquire_wait "$lock" || return 1 [ -f "$meta" ] || { fm_lock_release "$lock"; return 0; } tmp=$(fmx_meta_tmp "$meta") || { fm_lock_release "$lock"; return 1; } if ! { grep -vE '^x_request=|^x_request_ts=|^x_followups=|^x_platform=|^x_reply_max_chars=' "$meta" || true; } > "$tmp"; then diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 06a3e0d7083..6f562a77eb2 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -346,7 +346,7 @@ test_stale_or_malformed_steal_mutex_never_claims_nested_mutex() { printf '%s\n' "$dead" > "$ownerdir/pid" ln -s "$ownerdir" "$steal" else - ln -s "$state/.missing-steal-owner" "$steal" + ln -s "$steal.owner.gone01" "$steal" touch -h -t 202001010000 "$steal" fi cat > "$fakebin/ln" < "$lockdir/pid" + printf '%s\n' "$dead" > "$ownerdir/pid" + ln -s "$ownerdir" "$steal" + ln -s "$state/.retired-nested-owner" "$residue" + touch -h -t 202001010000 "$residue" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "stale nested-steal residue fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "a pre-upgrade .steal.steal residue permanently blocked stale primary reclaim: $out" + [ ! -e "$residue" ] && [ ! -L "$residue" ] \ + || fail "the retired nested-steal residue was left behind" + + dir=$(make_case lock-legacy-nested-live) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + residue="$steal.steal" + ownerdir="$steal.owner.legacy" + mkdir "$lockdir" "$ownerdir" "$residue" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$ownerdir/pid" + printf '%s\n' "$$" > "$residue/pid" + ln -s "$ownerdir" "$steal" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "live nested-steal residue fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=1" ] \ + || fail "a live pre-upgrade nested-steal holder was overrun instead of being waited out: $out" + [ -d "$residue" ] || fail "a live nested-steal residue was destroyed by the upgrade path" + [ -L "$steal" ] || fail "the steal mutex was reclaimed while a live nested holder still owned it" + pass "a pre-upgrade .steal.steal residue is retired only once its owner is dead and stale" +} + +test_reclaim_marker_is_not_stolen_from_a_live_owner() { + local dir state ownerdir reclaim out rc + dir=$(make_case lock-reclaim-marker-ownership) + state="$dir/state" + ownerdir="$state/.owner" + reclaim="$ownerdir/reclaim" + mkdir -p "$reclaim" + printf '%s\n' "$$" > "$reclaim/pid" + touch -t 202001010000 "$reclaim" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_reclaim_marker_release "$2" + printf "release=%s " "$?" + fm_lock_reclaim_marker_claim "$2" + printf "claim=%s\n" "$?" + ' _ "$LIB" "$reclaim" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "reclaim-marker ownership fixture shell failed (rc=$rc): $out" + [ "$out" = "release=1 claim=1" ] \ + || fail "another process released or took over a reclaim marker held by a live owner: $out" + [ -d "$reclaim" ] || fail "the live owner's reclaim marker was removed by a non-owner" + [ "$(cat "$reclaim/pid")" = "$$" ] || fail "the live owner's reclaim-marker pid was overwritten" + pass "a reclaim marker held by a live owner is neither released nor taken over by another process" +} + +test_dangling_steal_owner_reclaim_yields_one_winner() { + local dir state lockdir steal foreign marker dead i pids pid wins out rc + + dir=$(make_case lock-dangling-steal-concurrency) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + marker="$dir/wins" + dead=$(dead_pid) + mkdir "$lockdir" + printf '%s\n' "$dead" > "$lockdir/pid" + ln -s "$steal.owner.gone01" "$steal" + touch -h -t 202001010000 "$steal" + : > "$marker" + pids= + i=1 + while [ "$i" -le 40 ]; do + FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + if fm_lock_try_acquire "$2"; then + printf "%s\n" "${BASHPID:-$$}" >> "$3" + sleep 1 + fi + ' _ "$LIB" "$lockdir" "$marker" & + pids="$pids $!" + i=$((i + 1)) + done + for pid in $pids; do + wait "$pid" 2>/dev/null || true + done + wins=$(awk 'NF { c++ } END { print c + 0 }' "$marker") + [ "$wins" -eq 1 ] || fail "expected exactly one dangling-steal reclaimer to win, got $wins" + + dir=$(make_case lock-dangling-steal-foreign) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + foreign="$state/.not-an-owner-record" + mkdir "$lockdir" + printf '%s\n' "$dead" > "$lockdir/pid" + ln -s "$foreign" "$steal" + touch -h -t 202001010000 "$steal" + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "foreign steal-target fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=1" ] || fail "a steal mutex pointing outside the owner-record shape was reclaimed: $out" + [ ! -e "$foreign" ] || fail "reclaim created a directory at a path that is not an owner record" + pass "dangling steal-owner reclaim is serialized to one winner and refuses foreign targets" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1292,6 +1427,9 @@ test_lock_missing_parent_returns_typed_failure_with_bounded_launches test_lock_owner_record_failure_returns_typed_failure test_stale_or_malformed_steal_mutex_never_claims_nested_mutex test_abandoned_reclaim_marker_does_not_wedge_stale_steal_recovery +test_legacy_nested_steal_residue_is_retired_only_when_stale +test_reclaim_marker_is_not_stolen_from_a_live_owner +test_dangling_steal_owner_reclaim_yields_one_winner test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 21d05b2b758cef40003e7be3205f61805ebd0943 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 17:33:41 +0800 Subject: [PATCH 06/13] no-mistakes(review): bind reclaim takeover to inspected marker, reclaim legacy steal dirs --- bin/fm-wake-lib.sh | 79 +++++++++++++++++++++++++---------- tests/fm-watcher-lock.test.sh | 71 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 23 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index a30ac8c337e..dcd541f47e2 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -436,7 +436,7 @@ fm_lock_try_create() { # created. A dead symlink owner is serialized inside its unique owner directory, # so interrupted recovery remains reclaimable without creating .steal.steal. fm_lock_try_acquire_steal_mutex() { - local steal=$1 rc pid ownerdir reclaim + local steal=$1 rc pid ownerdir expected_owner reclaim case "$steal" in *.steal) : ;; *) return 2 ;; @@ -449,35 +449,51 @@ fm_lock_try_acquire_steal_mutex() { pid=$(cat "$steal/pid" 2>/dev/null || true) fm_pid_alive "$pid" && return 1 fm_lock_mid_acquire_is_fresh "$steal" "$pid" && return 1 - [ -L "$steal" ] || return 1 - ownerdir=$(fm_lock_link_owner "$steal" 2>/dev/null) || return 1 - if [ ! -d "$ownerdir" ]; then - case "${ownerdir##*/}" in - "${steal##*/}".owner.*) : ;; - *) return 1 ;; - esac - if ! mkdir "$ownerdir" 2>/dev/null; then - [ -d "$ownerdir" ] || return 1 - fi - if ! fm_lock_points_to_owner "$steal" "$ownerdir"; then - rmdir "$ownerdir" 2>/dev/null || true - return 1 + if [ -L "$steal" ]; then + ownerdir=$(fm_lock_link_owner "$steal" 2>/dev/null) || return 1 + expected_owner=$ownerdir + if [ ! -d "$ownerdir" ]; then + case "${ownerdir##*/}" in + "${steal##*/}".owner.*) : ;; + *) return 1 ;; + esac + if ! mkdir "$ownerdir" 2>/dev/null; then + [ -d "$ownerdir" ] || return 1 + fi + if ! fm_lock_points_to_owner "$steal" "$ownerdir"; then + rmdir "$ownerdir" 2>/dev/null || true + return 1 + fi fi + elif [ -d "$steal" ]; then + case "$pid" in + ''|*[!0-9]*) return 1 ;; + esac + ownerdir=$steal + expected_owner= + else + return 1 fi reclaim="$ownerdir/reclaim" fm_lock_reclaim_marker_claim "$reclaim" || return 1 if fm_lock_legacy_nested_steal_blocks "$steal.steal" \ - || ! fm_lock_recheck_stale_owner "$steal" "$ownerdir" "$pid"; then + || ! fm_lock_recheck_stale_owner "$steal" "$expected_owner" "$pid"; then fm_lock_reclaim_marker_release "$reclaim" return 1 fi - if ! rm -f "$steal" 2>/dev/null; then + fm_lock_reclaim_marker_held "$reclaim" || return 1 + if [ "$ownerdir" = "$steal" ]; then + fm_lock_reclaim_marker_release "$reclaim" || return 1 + fm_lock_remove_path "$steal" || return 1 + else + if ! rm -f "$steal" 2>/dev/null; then + fm_lock_reclaim_marker_release "$reclaim" + return 1 + fi + fm_lock_clean_known_files "$ownerdir" fm_lock_reclaim_marker_release "$reclaim" - return 1 + rmdir "$ownerdir" 2>/dev/null || true fi - fm_lock_clean_known_files "$ownerdir" - fm_lock_reclaim_marker_release "$reclaim" - rmdir "$ownerdir" 2>/dev/null || true fm_lock_try_create "$steal" } @@ -486,13 +502,21 @@ fm_lock_try_acquire_steal_mutex() { # reclaimer: a marker whose pid is dead and whose age passed the stale window is # itself reclaimable, exactly like every other stale record in this file. fm_lock_reclaim_marker_claim() { - local reclaim=$1 mypid retired + local reclaim=$1 mypid retired abandoned_pid retired_pid mypid=${BASHPID:-$$} if ! mkdir "$reclaim" 2>/dev/null; then fm_lock_reclaim_marker_is_abandoned "$reclaim" || return 1 + abandoned_pid=$FM_LOCK_RECLAIM_OBSERVED_PID retired="$reclaim.dead.$mypid" rm -rf "$retired" 2>/dev/null || true mv "$reclaim" "$retired" 2>/dev/null || return 1 + retired_pid=$(cat "$retired/pid" 2>/dev/null || true) + if [ "$retired_pid" != "$abandoned_pid" ] || fm_pid_alive "$retired_pid"; then + if [ -e "$reclaim" ] || [ -L "$reclaim" ] || ! mv "$retired" "$reclaim" 2>/dev/null; then + rm -rf "$retired" 2>/dev/null || true + fi + return 1 + fi rm -rf "$retired" 2>/dev/null || true mkdir "$reclaim" 2>/dev/null || return 1 fi @@ -505,19 +529,28 @@ fm_lock_reclaim_marker_claim() { return 0 } +FM_LOCK_RECLAIM_OBSERVED_PID= + fm_lock_reclaim_marker_is_abandoned() { local reclaim=$1 pid stale + FM_LOCK_RECLAIM_OBSERVED_PID= [ -d "$reclaim" ] || return 1 pid=$(cat "$reclaim/pid" 2>/dev/null || true) fm_pid_alive "$pid" && return 1 stale=$FM_LOCK_STALE_AFTER [ "$stale" -lt 2 ] && stale=2 - [ "$(fm_path_age "$reclaim")" -ge "$stale" ] + [ "$(fm_path_age "$reclaim")" -ge "$stale" ] || return 1 + FM_LOCK_RECLAIM_OBSERVED_PID=$pid + return 0 +} + +fm_lock_reclaim_marker_held() { + [ "$(cat "$1/pid" 2>/dev/null || true)" = "${BASHPID:-$$}" ] } fm_lock_reclaim_marker_release() { local reclaim=$1 - [ "$(cat "$reclaim/pid" 2>/dev/null || true)" = "${BASHPID:-$$}" ] || return 1 + fm_lock_reclaim_marker_held "$reclaim" || return 1 rm -f "$reclaim/pid" 2>/dev/null || true rmdir "$reclaim" 2>/dev/null || true return 0 diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 6f562a77eb2..5df17b241b0 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -535,6 +535,75 @@ test_dangling_steal_owner_reclaim_yields_one_winner() { pass "dangling steal-owner reclaim is serialized to one winner and refuses foreign targets" } +test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected() { + local dir state ownerdir reclaim fakebin real_mv dead out rc + dir=$(make_case lock-reclaim-marker-takeover-identity) + state="$dir/state" + ownerdir="$state/.owner" + reclaim="$ownerdir/reclaim" + fakebin="$dir/racebin" + real_mv=$(command -v mv) + dead=$(dead_pid) + mkdir -p "$reclaim" "$fakebin" + printf '%s\n' "$dead" > "$reclaim/pid" + touch -t 202001010000 "$reclaim" + cat > "$fakebin/mv" < "\$1/pid" 2>/dev/null || true +exec "$real_mv" "\$@" +SH + chmod +x "$fakebin/mv" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_RACER_PID="$$" bash -c ' + . "$1" + fm_lock_reclaim_marker_claim "$2" + printf "claim=%s\n" "$?" + ' _ "$LIB" "$reclaim" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "reclaim-marker takeover fixture shell failed (rc=$rc): $out" + [ "$out" = "claim=1" ] \ + || fail "a reclaimer retired a marker another racer had already replaced with its own live one: $out" + [ -d "$reclaim" ] || fail "the racer's live reclaim marker was destroyed by the losing takeover" + [ "$(cat "$reclaim/pid" 2>/dev/null || true)" = "$$" ] \ + || fail "the racer's live reclaim-marker pid did not survive the losing takeover" + pass "a reclaim-marker takeover only retires the abandoned marker it actually inspected" +} + +test_legacy_directory_steal_mutex_is_reclaimed_without_recursion() { + local dir state lockdir steal fakebin log real_ln dead out rc + dir=$(make_case lock-legacy-steal-dir) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + fakebin="$dir/logbin" + log="$dir/ln-targets" + real_ln=$(command -v ln) + dead=$(dead_pid) + mkdir "$lockdir" "$steal" "$fakebin" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$steal/pid" + cat > "$fakebin/ln" <> "\${FM_TEST_LN_LOG:?}" +exec "$real_ln" "\$@" +SH + chmod +x "$fakebin/ln" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_LN_LOG="$log" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "legacy directory steal-mutex fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "a legacy directory-shaped steal mutex with a dead owner was never reclaimed: $out" + ! grep -F "$steal.steal" "$log" >/dev/null 2>&1 \ + || fail "reclaiming a legacy directory steal mutex created a nested .steal.steal: $(cat "$log")" + pass "a legacy directory-shaped steal mutex is reclaimed without any nested steal transition" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1430,6 +1499,8 @@ test_abandoned_reclaim_marker_does_not_wedge_stale_steal_recovery test_legacy_nested_steal_residue_is_retired_only_when_stale test_reclaim_marker_is_not_stolen_from_a_live_owner test_dangling_steal_owner_reclaim_yields_one_winner +test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected +test_legacy_directory_steal_mutex_is_reclaimed_without_recursion test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 6a0914c24aa16741899e9f0caa548e1e177b331a Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 17:46:40 +0800 Subject: [PATCH 07/13] no-mistakes(review): fix coordinator argv ownership and marker takeover gap --- bin/fm-wake-lib.sh | 25 +++++++++----- tests/fm-grok-continuity-live-e2e.test.sh | 2 +- tests/fm-watcher-lock.test.sh | 42 +++++++++++++++++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index dcd541f47e2..82497af2ace 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -502,7 +502,7 @@ fm_lock_try_acquire_steal_mutex() { # reclaimer: a marker whose pid is dead and whose age passed the stale window is # itself reclaimable, exactly like every other stale record in this file. fm_lock_reclaim_marker_claim() { - local reclaim=$1 mypid retired abandoned_pid retired_pid + local reclaim=$1 mypid retired abandoned_pid retired_pid took_over=0 mypid=${BASHPID:-$$} if ! mkdir "$reclaim" 2>/dev/null; then fm_lock_reclaim_marker_is_abandoned "$reclaim" || return 1 @@ -510,22 +510,31 @@ fm_lock_reclaim_marker_claim() { retired="$reclaim.dead.$mypid" rm -rf "$retired" 2>/dev/null || true mv "$reclaim" "$retired" 2>/dev/null || return 1 - retired_pid=$(cat "$retired/pid" 2>/dev/null || true) - if [ "$retired_pid" != "$abandoned_pid" ] || fm_pid_alive "$retired_pid"; then - if [ -e "$reclaim" ] || [ -L "$reclaim" ] || ! mv "$retired" "$reclaim" 2>/dev/null; then - rm -rf "$retired" 2>/dev/null || true - fi + if ! mkdir "$reclaim" 2>/dev/null; then + rm -rf "$retired" 2>/dev/null || true return 1 fi - rm -rf "$retired" 2>/dev/null || true - mkdir "$reclaim" 2>/dev/null || return 1 + took_over=1 fi if ! { printf '%s\n' "$mypid" 2>/dev/null > "$reclaim/pid"; } \ || [ "$(cat "$reclaim/pid" 2>/dev/null || true)" != "$mypid" ]; then rm -f "$reclaim/pid" 2>/dev/null || true rmdir "$reclaim" 2>/dev/null || true + [ "$took_over" -eq 0 ] || rm -rf "$retired" 2>/dev/null || true return 1 fi + if [ "$took_over" -eq 1 ]; then + retired_pid=$(cat "$retired/pid" 2>/dev/null || true) + rm -rf "$retired" 2>/dev/null || true + if [ "$retired_pid" != "$abandoned_pid" ] || fm_pid_alive "$retired_pid"; then + if fm_pid_alive "$retired_pid"; then + printf '%s\n' "$retired_pid" 2>/dev/null > "$reclaim/pid" || true + else + fm_lock_reclaim_marker_release "$reclaim" + fi + return 1 + fi + fi return 0 } diff --git a/tests/fm-grok-continuity-live-e2e.test.sh b/tests/fm-grok-continuity-live-e2e.test.sh index 46143cf8512..99debc5b155 100755 --- a/tests/fm-grok-continuity-live-e2e.test.sh +++ b/tests/fm-grok-continuity-live-e2e.test.sh @@ -108,7 +108,7 @@ mkdir -p "$HOME_DIR/state" "$HOME_DIR/config" printf 'project=fixture\n' > "$HOME_DIR/state/grok-e2e.meta" "$TMUX" -L "$SOCKET" new-session -d -s "$SESSION" -c "$PROJECT" \ - "env FM_HOME='$HOME_DIR' FM_ROOT_OVERRIDE='$PROJECT' FM_POLL=1 FM_SIGNAL_GRACE=0 FM_HEARTBEAT=600 bash -lc 'printf \"pid=%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.grok-watch-coordinator\"; printf \"%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.lock\"; grok --trust --always-approve --reasoning-effort low; rc=\$?; printf \"GROK_EXIT=%s\\n\" \"\$rc\"; sleep 300'" + "env FM_HOME='$HOME_DIR' FM_ROOT_OVERRIDE='$PROJECT' FM_POLL=1 FM_SIGNAL_GRACE=0 FM_HEARTBEAT=600 bash -lc 'printf \"pid=%s\\n\" \"\$\$\" > \"$HOME_DIR/state/.grok-watch-coordinator\"; printf \"%s\\n\" \"\$\$\" > \"\$FM_HOME/state/.lock\"; grok --trust --always-approve --reasoning-effort low; rc=\$?; printf \"GROK_EXIT=%s\\n\" \"\$rc\"; sleep 300'" wait_for_text "Grok Build" 180 || fail "Grok did not reach its ready composer" diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 5df17b241b0..f2be2006218 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -604,6 +604,47 @@ SH pass "a legacy directory-shaped steal mutex is reclaimed without any nested steal transition" } +test_reclaim_marker_takeover_never_vacates_the_marker_slot() { + local dir state ownerdir reclaim fakebin real_cat gaps dead out rc + dir=$(make_case lock-reclaim-marker-no-gap) + state="$dir/state" + ownerdir="$state/.owner" + reclaim="$ownerdir/reclaim" + fakebin="$dir/gapbin" + gaps="$dir/gap-log" + real_cat=$(command -v cat) + dead=$(dead_pid) + mkdir -p "$reclaim" "$fakebin" + printf '%s\n' "$dead" > "$reclaim/pid" + touch -t 202001010000 "$reclaim" + : > "$gaps" + cat > "$fakebin/cat" </dev/null; then + printf '%s\n' "\${FM_TEST_BYSTANDER_PID:?}" > "\$FM_TEST_RECLAIM/pid" 2>/dev/null || true + printf 'claimed\n' >> "\${FM_TEST_GAP_LOG:?}" +fi +exec "$real_cat" "\$@" +SH + chmod +x "$fakebin/cat" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_RECLAIM="$reclaim" \ + FM_TEST_GAP_LOG="$gaps" FM_TEST_BYSTANDER_PID="$$" bash -c ' + . "$1" + fm_lock_reclaim_marker_claim "$2" + printf "claim=%s\n" "$?" + ' _ "$LIB" "$reclaim" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "reclaim-marker gap fixture shell failed (rc=$rc): $out" + [ ! -s "$gaps" ] \ + || fail "a bystander claimed the reclaim marker while a takeover had vacated the slot: $(cat "$gaps")" + [ "$out" = "claim=0" ] \ + || fail "the takeover of a genuinely abandoned marker did not succeed: $out" + [ "$(cat "$reclaim/pid" 2>/dev/null || true)" != "$$" ] \ + || fail "the bystander's pid ended up owning the reclaim marker" + pass "a reclaim-marker takeover never leaves the marker slot claimable by a bystander" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1500,6 +1541,7 @@ test_legacy_nested_steal_residue_is_retired_only_when_stale test_reclaim_marker_is_not_stolen_from_a_live_owner test_dangling_steal_owner_reclaim_yields_one_winner test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected +test_reclaim_marker_takeover_never_vacates_the_marker_slot test_legacy_directory_steal_mutex_is_reclaimed_without_recursion test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency From 620d507caf3cda5964152b7ac9b2d4bc2b36182b Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 18:00:18 +0800 Subject: [PATCH 08/13] no-mistakes(review): guard conditional-context lock waits, retire legacy steal debris --- bin/fm-backlog-handoff.sh | 14 +++++++++-- bin/fm-captain-hold.sh | 2 +- bin/fm-spawn.sh | 2 +- bin/fm-wake-lib.sh | 19 +++++++++++++++ tests/fm-watcher-lock.test.sh | 46 +++++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 4 deletions(-) diff --git a/bin/fm-backlog-handoff.sh b/bin/fm-backlog-handoff.sh index f7736553348..28cc9ea861a 100755 --- a/bin/fm-backlog-handoff.sh +++ b/bin/fm-backlog-handoff.sh @@ -444,14 +444,24 @@ with_remote_route_locks() { # shift 2 case "$id" in ''|*[!A-Za-z0-9._-]*) echo "error: unsafe remote handoff id: $id" >&2; return 1 ;; esac ACTIVE_REGISTRY_LOCK=$(secondmate_registry_lock_path "$STATE") - fm_lock_acquire_wait "$ACTIVE_REGISTRY_LOCK" + if ! fm_lock_acquire_wait "$ACTIVE_REGISTRY_LOCK"; then + echo "error: could not lock the secondmate registry for $id" >&2 + ACTIVE_REGISTRY_LOCK= + release_remote_locks + return 1 + fi if [ "$(secondmate_registry_field "$REG" "$id" remote 2>/dev/null || true)" != 1 ]; then echo "error: pending outbox has no matching remote secondmate route: $id" >&2 release_remote_locks return 1 fi ACTIVE_HANDOFF_LOCK="$STATE/.backlog-handoff-$id.lock" - fm_lock_acquire_wait "$ACTIVE_HANDOFF_LOCK" + if ! fm_lock_acquire_wait "$ACTIVE_HANDOFF_LOCK"; then + echo "error: could not lock the pending handoff outbox for $id" >&2 + ACTIVE_HANDOFF_LOCK= + release_remote_locks + return 1 + fi if "$operation" "$@"; then rc=0; else rc=$?; fi release_remote_locks return "$rc" diff --git a/bin/fm-captain-hold.sh b/bin/fm-captain-hold.sh index cb429d95238..ed56246ce87 100755 --- a/bin/fm-captain-hold.sh +++ b/bin/fm-captain-hold.sh @@ -775,7 +775,7 @@ command_complete() { [ -f "$meta" ] && has_meta=1 if [ "$has_meta" = 1 ]; then CAPTAIN_META_LOCK=$(fm_meta_lock_path "$meta") || fail "could not resolve task metadata lock" - fm_lock_acquire_wait "$CAPTAIN_META_LOCK" + fm_lock_acquire_wait "$CAPTAIN_META_LOCK" || fail "could not lock task metadata" CAPTAIN_META_LOCK_HELD=1 [ -f "$meta" ] || fail "task metadata disappeared while recording completion" fi diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index b03d429dd66..d5ea0fcf2b0 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -2820,7 +2820,7 @@ fi spawn_record_traceparent() { local meta="$STATE/$ID.meta" tmp status=0 SPAWN_META_LOCK=$(fm_meta_lock_path "$meta") || return 1 - fm_lock_acquire_wait "$SPAWN_META_LOCK" + fm_lock_acquire_wait "$SPAWN_META_LOCK" || return 1 SPAWN_META_LOCK_HELD=1 SPAWN_META_TMP="$STATE/.$ID.meta.trace.${BASHPID:-$$}" if [ ! -f "$meta" ] || [ ! -w "$meta" ] \ diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 82497af2ace..830d7dca632 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -484,6 +484,7 @@ fm_lock_try_acquire_steal_mutex() { fm_lock_reclaim_marker_held "$reclaim" || return 1 if [ "$ownerdir" = "$steal" ]; then fm_lock_reclaim_marker_release "$reclaim" || return 1 + fm_lock_clean_known_debris "$steal" fm_lock_remove_path "$steal" || return 1 else if ! rm -f "$steal" 2>/dev/null; then @@ -582,6 +583,24 @@ fm_lock_legacy_nested_steal_blocks() { return 0 } +# Retire only debris this lock implementation is known to create inside a lock +# directory: an interrupted reclaim takeover copy, and a stray owner symlink a +# racer left behind. Anything else is left in place so the caller's rmdir still +# fails closed on state this code does not own. +fm_lock_clean_known_debris() { + local lockdir=$1 base entry + base=${lockdir##*/} + for entry in "$lockdir"/reclaim.dead.*; do + [ -d "$entry" ] && [ ! -L "$entry" ] || continue + rm -f "$entry/pid" 2>/dev/null || true + rmdir "$entry" 2>/dev/null || true + done + for entry in "$lockdir/$base".owner.*; do + [ -L "$entry" ] || continue + rm -f "$entry" 2>/dev/null || true + done +} + fm_lock_remove_path() { local lockdir=$1 ownerdir if [ -L "$lockdir" ]; then diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index f2be2006218..cdf9f11722e 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -645,6 +645,51 @@ SH pass "a reclaim-marker takeover never leaves the marker slot claimable by a bystander" } +test_legacy_directory_steal_mutex_survives_known_recovery_debris() { + local dir state lockdir steal dead out rc + dir=$(make_case lock-legacy-steal-dir-debris) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + dead=$(dead_pid) + mkdir "$lockdir" "$steal" "$steal/reclaim.dead.$dead" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$steal/pid" + printf '%s\n' "$dead" > "$steal/reclaim.dead.$dead/pid" + ln -s "$state/.gone-owner" "$steal/.contend.lock.steal.owner.abc123" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "legacy steal-dir debris fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "known reclaim debris left the legacy directory steal mutex permanently unreclaimable: $out" + + dir=$(make_case lock-legacy-steal-dir-unknown) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + mkdir "$lockdir" "$steal" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$steal/pid" + printf 'unowned\n' > "$steal/not-a-lock-record" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "unknown steal-dir content fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=1" ] \ + || fail "reclaim destroyed a steal directory holding content this lock code does not own: $out" + [ -f "$steal/not-a-lock-record" ] || fail "unowned content inside the steal directory was deleted" + pass "legacy steal-dir reclaim retires known debris and stays fail-closed on unowned content" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1543,6 +1588,7 @@ test_dangling_steal_owner_reclaim_yields_one_winner test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected test_reclaim_marker_takeover_never_vacates_the_marker_slot test_legacy_directory_steal_mutex_is_reclaimed_without_recursion +test_legacy_directory_steal_mutex_survives_known_recovery_debris test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 904aa603fe0debba939e53902abe60dc0d43436c Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 18:15:18 +0800 Subject: [PATCH 09/13] no-mistakes(review): harden legacy steal reclaim, marker self-recovery, owner residue --- bin/fm-remote-secondmate-control.sh | 33 +++++++-- bin/fm-wake-lib.sh | 21 ++++-- ...fm-remote-secondmate-lifecycle-e2e.test.sh | 4 ++ tests/fm-watcher-lock.test.sh | 72 +++++++++++++++++++ 4 files changed, 120 insertions(+), 10 deletions(-) diff --git a/bin/fm-remote-secondmate-control.sh b/bin/fm-remote-secondmate-control.sh index de25832d8b8..d3b9bcff3ca 100755 --- a/bin/fm-remote-secondmate-control.sh +++ b/bin/fm-remote-secondmate-control.sh @@ -94,16 +94,41 @@ retired_mktemp_suffix_valid() { # return 1 } -# A lock owner directory holds only the files bin/fm-wake-lib.sh's owner -# contract writes and cleans. -retired_lock_owner_dir_valid() { # +# A stale-recovery serialization marker bin/fm-wake-lib.sh leaves inside a lock +# owner directory: a directory whose only entry is the reclaimer's pid file. +retired_lock_reclaim_marker_valid() { # local dir=$1 entry [ -d "$dir" ] && [ ! -L "$dir" ] || return 1 for entry in "$dir"/* "$dir"/.[!.]* "$dir"/..?*; do [ -e "$entry" ] || [ -L "$entry" ] || continue + [ "${entry##*/}" = pid ] || return 1 retired_plain_file "$entry" || return 1 + done + return 0 +} + +# A lock owner directory holds only the files bin/fm-wake-lib.sh's owner +# contract writes and cleans, plus the reclaim markers its stale-recovery path +# creates there. +retired_lock_owner_dir_valid() { # + local dir=$1 entry suffix + [ -d "$dir" ] && [ ! -L "$dir" ] || return 1 + for entry in "$dir"/* "$dir"/.[!.]* "$dir"/..?*; do + [ -e "$entry" ] || [ -L "$entry" ] || continue case "${entry##*/}" in - pid|fm-home|pid-identity|role|watcher-path) ;; + pid|fm-home|pid-identity|role|watcher-path) + retired_plain_file "$entry" || return 1 + ;; + reclaim) + retired_lock_reclaim_marker_valid "$entry" || return 1 + ;; + reclaim.dead.*) + suffix=${entry##*.} + case "$suffix" in + ''|*[!0-9]*) return 1 ;; + esac + retired_lock_reclaim_marker_valid "$entry" || return 1 + ;; *) return 1 ;; esac done diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 830d7dca632..482da2b1092 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -404,7 +404,7 @@ fm_lock_claim() { } fm_lock_try_create() { - local lockdir=$1 allowed_steal_owner=${2:-} ownerdir rc + local lockdir=$1 allowed_steal_owner=${2:-} ownerdir FM_LOCK_OWNER_DIR= ownerdir=$(fm_lock_owner_dir "$lockdir") || return 2 if [ -e "$lockdir" ] || [ -L "$lockdir" ]; then @@ -415,7 +415,6 @@ fm_lock_try_create() { fm_lock_discard_owner "$ownerdir" return 2 fi - rc=1 if ln -s "$ownerdir" "$lockdir" 2>/dev/null && fm_lock_points_to_owner "$lockdir" "$ownerdir"; then if fm_lock_claim "$lockdir" "$ownerdir" "$allowed_steal_owner"; then FM_LOCK_OWNER_DIR=$ownerdir @@ -428,7 +427,7 @@ fm_lock_try_create() { fm_lock_remove_stray_owner_link "$lockdir" "$ownerdir" fi fm_lock_discard_owner "$ownerdir" - return "$rc" + return 1 } # Acquire a stale-recovery mutex without requesting a second mutex. Status 1 @@ -483,9 +482,14 @@ fm_lock_try_acquire_steal_mutex() { fi fm_lock_reclaim_marker_held "$reclaim" || return 1 if [ "$ownerdir" = "$steal" ]; then - fm_lock_reclaim_marker_release "$reclaim" || return 1 + if [ ! -d "$steal" ] || [ -L "$steal" ]; then + fm_lock_reclaim_marker_release "$reclaim" + return 1 + fi + fm_lock_clean_known_files "$steal" fm_lock_clean_known_debris "$steal" - fm_lock_remove_path "$steal" || return 1 + fm_lock_reclaim_marker_release "$reclaim" || return 1 + rmdir "$steal" 2>/dev/null || return 1 else if ! rm -f "$steal" 2>/dev/null; then fm_lock_reclaim_marker_release "$reclaim" @@ -527,7 +531,8 @@ fm_lock_reclaim_marker_claim() { if [ "$took_over" -eq 1 ]; then retired_pid=$(cat "$retired/pid" 2>/dev/null || true) rm -rf "$retired" 2>/dev/null || true - if [ "$retired_pid" != "$abandoned_pid" ] || fm_pid_alive "$retired_pid"; then + if [ "$retired_pid" != "$abandoned_pid" ] \ + || { [ "$retired_pid" != "$mypid" ] && fm_pid_alive "$retired_pid"; }; then if fm_pid_alive "$retired_pid"; then printf '%s\n' "$retired_pid" 2>/dev/null > "$reclaim/pid" || true else @@ -546,6 +551,10 @@ fm_lock_reclaim_marker_is_abandoned() { FM_LOCK_RECLAIM_OBSERVED_PID= [ -d "$reclaim" ] || return 1 pid=$(cat "$reclaim/pid" 2>/dev/null || true) + if [ -n "$pid" ] && [ "$pid" = "${BASHPID:-$$}" ]; then + FM_LOCK_RECLAIM_OBSERVED_PID=$pid + return 0 + fi fm_pid_alive "$pid" && return 1 stale=$FM_LOCK_STALE_AFTER [ "$stale" -lt 2 ] && stale=2 diff --git a/tests/fm-remote-secondmate-lifecycle-e2e.test.sh b/tests/fm-remote-secondmate-lifecycle-e2e.test.sh index 37077528259..6324a1f49cf 100755 --- a/tests/fm-remote-secondmate-lifecycle-e2e.test.sh +++ b/tests/fm-remote-secondmate-lifecycle-e2e.test.sh @@ -1290,6 +1290,10 @@ printf 'diverged preferences\n' \ # its lock symlink, and an atomic staging file behind in the same directories. mkdir "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z" printf '4242\n' > "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z/pid" +mkdir "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z/reclaim" +printf '4242\n' > "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z/reclaim/pid" +mkdir "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z/reclaim.dead.4242" +printf '4242\n' > "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z/reclaim.dead.4242/pid" ln -s "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock.owner.aB3d9Z" \ "$REMOTE_HOME/config/.fm-inherit-crew-harness.lock" printf 'partial payload\n' > "$REMOTE_HOME/data/.inherit.Qz71xW" diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index cdf9f11722e..78f3b372f4a 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -690,6 +690,76 @@ test_legacy_directory_steal_mutex_survives_known_recovery_debris() { pass "legacy steal-dir reclaim retires known debris and stays fail-closed on unowned content" } +test_legacy_directory_reclaim_never_deletes_a_racer_mutex() { + local dir state lockdir steal reclaim racer_owner fakebin real_rmdir dead out rc + dir=$(make_case lock-legacy-steal-dir-racer) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + reclaim="$steal/reclaim" + racer_owner="$state/.racer-owner" + fakebin="$dir/racebin" + real_rmdir=$(command -v rmdir) + dead=$(dead_pid) + mkdir "$lockdir" "$steal" "$fakebin" + printf '%s\n' "$dead" > "$lockdir/pid" + printf '%s\n' "$dead" > "$steal/pid" + cat > "$fakebin/rmdir" </dev/null || true + mkdir -p "\${FM_TEST_RACER_OWNER:?}" + printf '%s\n' "\${FM_TEST_RACER_PID:?}" > "\$FM_TEST_RACER_OWNER/pid" + ln -s "\$FM_TEST_RACER_OWNER" "\$FM_TEST_STEAL" 2>/dev/null || true + exit \$rc +fi +exec "$real_rmdir" "\$@" +SH + chmod +x "$fakebin/rmdir" + + rc=0 + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_RECLAIM="$reclaim" \ + FM_TEST_STEAL="$steal" FM_TEST_RACER_OWNER="$racer_owner" FM_TEST_RACER_PID="$$" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "legacy steal-dir racer fixture shell failed (rc=$rc): $out" + [ -L "$steal" ] \ + || fail "the legacy directory reclaim deleted the live steal mutex a racer created: $out" + [ "$(readlink "$steal")" = "$racer_owner" ] \ + || fail "the racer's steal mutex no longer points at its own owner record: $(readlink "$steal")" + [ "$out" = "rc=1" ] \ + || fail "the losing reclaimer reported success after a racer took the steal mutex: $out" + pass "legacy directory reclaim never deletes a steal mutex another reclaimer created" +} + +test_self_orphaned_reclaim_marker_is_reclaimable_by_its_owner() { + local dir state ownerdir reclaim out rc + dir=$(make_case lock-reclaim-marker-self-orphan) + state="$dir/state" + ownerdir="$state/.owner" + reclaim="$ownerdir/reclaim" + mkdir -p "$ownerdir" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + mkdir "$2" + printf "%s\n" "${BASHPID:-$$}" > "$2/pid" + fm_lock_reclaim_marker_claim "$2" + claim=$? + if [ "$(cat "$2/pid" 2>/dev/null || true)" = "${BASHPID:-$$}" ]; then owned=self; else owned=other; fi + printf "claim=%s owned=%s\n" "$claim" "$owned" + ' _ "$LIB" "$reclaim" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "self-orphaned marker fixture shell failed (rc=$rc): $out" + [ "$out" = "claim=0 owned=self" ] \ + || fail "a process could not reclaim the marker it orphaned itself, so it would spin forever: $out" + pass "a reclaim marker orphaned by the caller itself stays reclaimable by that caller" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1589,6 +1659,8 @@ test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected test_reclaim_marker_takeover_never_vacates_the_marker_slot test_legacy_directory_steal_mutex_is_reclaimed_without_recursion test_legacy_directory_steal_mutex_survives_known_recovery_debris +test_legacy_directory_reclaim_never_deletes_a_racer_mutex +test_self_orphaned_reclaim_marker_is_reclaimable_by_its_owner test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 3329efb85eb382b3e8558d6cbf144cf5b21e7729 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 18:29:36 +0800 Subject: [PATCH 10/13] no-mistakes(review): preserve steal-dir owner record, prove descendant-free attempts --- bin/fm-wake-lib.sh | 63 ++++++++++++++++++++++----- tests/fm-watcher-lock.test.sh | 80 ++++++++++++++++++++++++++++------- 2 files changed, 117 insertions(+), 26 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 482da2b1092..37b3d58cc91 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -465,9 +465,6 @@ fm_lock_try_acquire_steal_mutex() { fi fi elif [ -d "$steal" ]; then - case "$pid" in - ''|*[!0-9]*) return 1 ;; - esac ownerdir=$steal expected_owner= else @@ -475,21 +472,25 @@ fm_lock_try_acquire_steal_mutex() { fi reclaim="$ownerdir/reclaim" fm_lock_reclaim_marker_claim "$reclaim" || return 1 - if fm_lock_legacy_nested_steal_blocks "$steal.steal" \ - || ! fm_lock_recheck_stale_owner "$steal" "$expected_owner" "$pid"; then + if fm_lock_legacy_nested_steal_blocks "$steal.steal"; then + fm_lock_reclaim_marker_release "$reclaim" + return 1 + fi + if [ "$ownerdir" = "$steal" ]; then + if ! fm_lock_legacy_steal_dir_still_stale "$steal" "$pid"; then + fm_lock_reclaim_marker_release "$reclaim" + return 1 + fi + elif ! fm_lock_recheck_stale_owner "$steal" "$expected_owner" "$pid"; then fm_lock_reclaim_marker_release "$reclaim" return 1 fi fm_lock_reclaim_marker_held "$reclaim" || return 1 if [ "$ownerdir" = "$steal" ]; then - if [ ! -d "$steal" ] || [ -L "$steal" ]; then + if ! fm_lock_retire_legacy_steal_dir "$steal"; then fm_lock_reclaim_marker_release "$reclaim" return 1 fi - fm_lock_clean_known_files "$steal" - fm_lock_clean_known_debris "$steal" - fm_lock_reclaim_marker_release "$reclaim" || return 1 - rmdir "$steal" 2>/dev/null || return 1 else if ! rm -f "$steal" 2>/dev/null; then fm_lock_reclaim_marker_release "$reclaim" @@ -502,6 +503,48 @@ fm_lock_try_acquire_steal_mutex() { fm_lock_try_create "$steal" } +# The staleness of a legacy directory-shaped steal mutex was already judged on +# the untouched directory before its reclaim marker was created, so the marker's +# own mtime must not be re-read here; only the owner record is re-verified. +fm_lock_legacy_steal_dir_still_stale() { + local steal=$1 expected_pid=$2 actual_pid + [ -d "$steal" ] && [ ! -L "$steal" ] || return 1 + actual_pid=$(cat "$steal/pid" 2>/dev/null || true) + [ "$actual_pid" = "$expected_pid" ] || return 1 + ! fm_pid_alive "$actual_pid" +} + +# Replace a proven-stale legacy directory steal mutex by renaming it into a +# fresh owner-record name and removing it there. The rename is the removal, so +# the directory's own pid record survives every failure path and the mutex stays +# reclaimable; unknown content refuses the whole retirement untouched. +fm_lock_retire_legacy_steal_dir() { + local steal=$1 aside entry + [ -d "$steal" ] && [ ! -L "$steal" ] || return 1 + fm_lock_clean_known_debris "$steal" + for entry in "$steal"/* "$steal"/.[!.]* "$steal"/..?*; do + [ -e "$entry" ] || [ -L "$entry" ] || continue + case "${entry##*/}" in + pid|reclaim) ;; + *) return 1 ;; + esac + done + aside=$(fm_lock_owner_dir "$steal") || return 1 + if ! rmdir "$aside" 2>/dev/null; then + fm_lock_discard_owner "$aside" + return 1 + fi + if [ ! -d "$steal" ] || [ -L "$steal" ]; then + rmdir "$aside" 2>/dev/null || true + return 1 + fi + mv "$steal" "$aside" 2>/dev/null || return 1 + rm -f "$aside/pid" "$aside/reclaim/pid" 2>/dev/null || true + rmdir "$aside/reclaim" 2>/dev/null || true + rmdir "$aside" 2>/dev/null || true + return 0 +} + # The reclaim marker serializes stale steal-mutex recovery. It records the # reclaimer's pid so a reclaimer killed mid-recovery cannot wedge every later # reclaimer: a marker whose pid is dead and whose age passed the stale window is diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 78f3b372f4a..6153d9556da 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -222,12 +222,15 @@ test_lock_single_winner_under_concurrency() { test_lock_missing_parent_returns_typed_failure_with_bounded_launches() { local dir state missing fakebin count pidfile command_name real_command out rc elapsed result wait_result launches attempt_pid + local proctable snapfile snapshot_pid leaked dir=$(make_case lock-missing-parent) state="$dir/state" missing="$dir/absent/demo.lock" fakebin="$dir/countbin" count="$dir/helper-launches" pidfile="$dir/attempt-pid" + proctable="$dir/live-process-table" + snapfile="$dir/snapshot-pid" mkdir -p "$fakebin" for command_name in basename cat date dirname ln mkdir mktemp readlink rm rmdir stat uname; do real_command=$(command -v "$command_name") @@ -267,8 +270,12 @@ SH wait_result=$? [ "$wait_result" -eq 2 ] || exit 21 read -r launches < "$3" + ps -eo pid=,ppid= > "$5" 2>/dev/null & + snapshot_pid=$! + wait "$snapshot_pid" 2>/dev/null || true + printf "%s\n" "$snapshot_pid" > "$6" printf "result=%s wait_result=%s launches=%s\n" "$result" "$wait_result" "$launches" - ' _ "$LIB" "$missing" "$count" "$pidfile" 2>&1) || rc=$? + ' _ "$LIB" "$missing" "$count" "$pidfile" "$proctable" "$snapfile" 2>&1) || rc=$? elapsed=$SECONDS [ "$rc" -eq 0 ] || fail "missing-parent lock attempt did not return typed invalid-path status within its launch fuse (rc=$rc): $out" @@ -280,8 +287,12 @@ SH [ "$launches" -le 18 ] || fail "five missing-parent failures plus one wait exceeded the helper-launch budget ($launches): $out" [ "$elapsed" -lt 10 ] || fail "missing-parent lock attempts did not return promptly (${elapsed}s): $out" attempt_pid=$(cat "$pidfile") - [ -z "$(pgrep -P "$attempt_pid" 2>/dev/null || true)" ] \ - || fail "missing-parent lock attempt left a spawned descendant alive" + snapshot_pid=$(cat "$snapfile") + [ -s "$proctable" ] || fail "the live process table was never captured while the lock attempt was running" + leaked=$(awk -v parent="$attempt_pid" -v self="$snapshot_pid" \ + '$2 == parent && $1 != self { print $1 }' "$proctable" | tr '\n' ' ') + [ -z "$leaked" ] \ + || fail "missing-parent lock attempt left a spawned descendant alive: $leaked" pass "missing-parent lock failure is typed, prompt, descendant-free, and launch-bounded" } @@ -687,16 +698,51 @@ test_legacy_directory_steal_mutex_survives_known_recovery_debris() { [ "$out" = "rc=1" ] \ || fail "reclaim destroyed a steal directory holding content this lock code does not own: $out" [ -f "$steal/not-a-lock-record" ] || fail "unowned content inside the steal directory was deleted" - pass "legacy steal-dir reclaim retires known debris and stays fail-closed on unowned content" + [ "$(cat "$steal/pid" 2>/dev/null || true)" = "$dead" ] \ + || fail "a refused retirement destroyed the steal mutex's own owner record" + + rm -f "$steal/not-a-lock-record" + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "retry-after-cleanup fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "a legacy steal mutex stayed unreclaimable after its unowned content was removed: $out" + pass "legacy steal-dir reclaim retires known debris, fails closed on unowned content, and stays retryable" +} + +test_legacy_directory_steal_mutex_without_owner_record_is_reclaimable_when_aged() { + local dir state lockdir steal dead out rc + dir=$(make_case lock-legacy-steal-dir-no-pid) + state="$dir/state" + lockdir="$state/.contend.lock" + steal="$lockdir.steal" + dead=$(dead_pid) + mkdir "$lockdir" "$steal" + printf '%s\n' "$dead" > "$lockdir/pid" + touch -t 202001010000 "$steal" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2" + printf "rc=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "pid-less legacy steal-dir fixture shell failed (rc=$rc): $out" + [ "$out" = "rc=0" ] \ + || fail "an aged legacy steal directory carrying no owner record was permanently unreclaimable: $out" + pass "an aged legacy steal directory with no owner record is still reclaimable" } test_legacy_directory_reclaim_never_deletes_a_racer_mutex() { - local dir state lockdir steal reclaim racer_owner fakebin real_rmdir dead out rc + local dir state lockdir steal racer_owner fakebin real_rmdir dead out rc dir=$(make_case lock-legacy-steal-dir-racer) state="$dir/state" lockdir="$state/.contend.lock" steal="$lockdir.steal" - reclaim="$steal/reclaim" racer_owner="$state/.racer-owner" fakebin="$dir/racebin" real_rmdir=$(command -v rmdir) @@ -706,21 +752,22 @@ test_legacy_directory_reclaim_never_deletes_a_racer_mutex() { printf '%s\n' "$dead" > "$steal/pid" cat > "$fakebin/rmdir" </dev/null || true - mkdir -p "\${FM_TEST_RACER_OWNER:?}" - printf '%s\n' "\${FM_TEST_RACER_PID:?}" > "\$FM_TEST_RACER_OWNER/pid" - ln -s "\$FM_TEST_RACER_OWNER" "\$FM_TEST_STEAL" 2>/dev/null || true - exit \$rc -fi +case "\$1" in + */reclaim) + "$real_rmdir" "\$@" + rc=\$? + mkdir -p "\${FM_TEST_RACER_OWNER:?}" + printf '%s\n' "\${FM_TEST_RACER_PID:?}" > "\$FM_TEST_RACER_OWNER/pid" + ln -s "\$FM_TEST_RACER_OWNER" "\${FM_TEST_STEAL:?}" 2>/dev/null || true + exit \$rc + ;; +esac exec "$real_rmdir" "\$@" SH chmod +x "$fakebin/rmdir" rc=0 - out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_TEST_RECLAIM="$reclaim" \ + out=$(PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" \ FM_TEST_STEAL="$steal" FM_TEST_RACER_OWNER="$racer_owner" FM_TEST_RACER_PID="$$" bash -c ' . "$1" fm_lock_try_acquire "$2" @@ -1659,6 +1706,7 @@ test_reclaim_marker_takeover_is_bound_to_the_marker_it_inspected test_reclaim_marker_takeover_never_vacates_the_marker_slot test_legacy_directory_steal_mutex_is_reclaimed_without_recursion test_legacy_directory_steal_mutex_survives_known_recovery_debris +test_legacy_directory_steal_mutex_without_owner_record_is_reclaimable_when_aged test_legacy_directory_reclaim_never_deletes_a_racer_mutex test_self_orphaned_reclaim_marker_is_reclaimable_by_its_owner test_lock_steals_dead_pid_lock From 1ebeaf4a9a60497b971f3d9cedd8c163b4bf68ba Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 18:43:13 +0800 Subject: [PATCH 11/13] no-mistakes(review): restore self-abandoned steal-mutex reclaim in non-recursive helper --- bin/fm-wake-lib.sh | 11 +++++++++++ tests/fm-watcher-lock.test.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 37b3d58cc91..f416668871b 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -445,7 +445,18 @@ fm_lock_try_acquire_steal_mutex() { [ "$rc" -ne 0 ] || return 0 [ "$rc" -ne 2 ] || return 2 ! fm_lock_legacy_nested_steal_blocks "$steal.steal" || return 1 + # Compare against ${BASHPID:-$$} inline, never via a command substitution, + # and keep the Bash 3 subshell guard the primary path uses: a trap that + # abandoned the frame holding this mutex must not deadlock the exit path + # against itself, but a child frame must never reclaim its parent's hold. pid=$(cat "$steal/pid" 2>/dev/null || true) + if [ -n "$pid" ] && [ "$pid" = "${BASHPID:-$$}" ] \ + && { [ -n "${BASHPID:-}" ] || [ "${BASH_SUBSHELL:-0}" -eq 0 ]; }; then + fm_lock_remove_path "$steal" || true + rc=0 + fm_lock_try_create "$steal" || rc=$? + return "$rc" + fi fm_pid_alive "$pid" && return 1 fm_lock_mid_acquire_is_fresh "$steal" "$pid" && return 1 if [ -L "$steal" ]; then diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 6153d9556da..22e9a425e9a 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -807,6 +807,33 @@ test_self_orphaned_reclaim_marker_is_reclaimable_by_its_owner() { pass "a reclaim marker orphaned by the caller itself stays reclaimable by that caller" } +test_self_abandoned_steal_mutex_is_reclaimed_only_by_its_own_frame() { + local dir state lockdir dead out rc + dir=$(make_case lock-self-abandoned-steal) + state="$dir/state" + lockdir="$state/.contend.lock" + dead=$(dead_pid) + mkdir "$lockdir" + printf '%s\n' "$dead" > "$lockdir/pid" + + rc=0 + out=$(FM_STATE_OVERRIDE="$state" bash -c ' + . "$1" + fm_lock_try_acquire "$2.steal" || exit 7 + ( fm_lock_try_acquire "$2" >/dev/null 2>&1; printf "sub=%s " "$?" ) + fm_lock_try_acquire "$2" + printf "self=%s\n" "$?" + ' _ "$LIB" "$lockdir" 2>&1) || rc=$? + [ "$rc" -eq 0 ] || fail "self-abandoned steal-mutex fixture shell failed (rc=$rc): $out" + case "$out" in + "sub=1 self="*) : ;; + *) fail "a child frame reclaimed the steal mutex its live parent still holds: $out" ;; + esac + [ "$out" = "sub=1 self=0" ] \ + || fail "a process could not reclaim the steal mutex its own interrupted frame abandoned, so the exit path would spin forever: $out" + pass "a self-abandoned steal mutex is reclaimed by its own frame and never by a child frame" +} + test_lock_steals_dead_pid_lock() { local dir state lockdir dead rc newpid dir=$(make_case lock-dead-steal) @@ -1709,6 +1736,7 @@ test_legacy_directory_steal_mutex_survives_known_recovery_debris test_legacy_directory_steal_mutex_without_owner_record_is_reclaimable_when_aged test_legacy_directory_reclaim_never_deletes_a_racer_mutex test_self_orphaned_reclaim_marker_is_reclaimable_by_its_owner +test_self_abandoned_steal_mutex_is_reclaimed_only_by_its_own_frame test_lock_steals_dead_pid_lock test_lock_stale_steal_single_winner_under_concurrency test_lock_live_steal_mutex_is_not_reclaimed From 07bdc4cc1b0a1cc741b5aee6b221917580cfd702 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 19:05:58 +0800 Subject: [PATCH 12/13] no-mistakes(document): document typed lock statuses and non-recursive steal reclaim --- bin/fm-wake-lib.sh | 12 ++++++++++++ docs/watcher-continuity.md | 1 + 2 files changed, 13 insertions(+) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index f416668871b..7c9e0b22ffd 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -403,6 +403,10 @@ fm_lock_claim() { return 0 } +# Status 1 means the lock is held or was lost to a racer and retrying can win it; +# status 2 means this process could not create its own owner record at all +# (missing or unwritable parent directory), which no amount of waiting fixes. +# Every caller in this file propagates that distinction rather than spinning. fm_lock_try_create() { local lockdir=$1 allowed_steal_owner=${2:-} ownerdir FM_LOCK_OWNER_DIR= @@ -1016,6 +1020,11 @@ fm_recovery_marker_reopen_announced() { fm_recovery_transition "$1" reopen-announced } +# Returns 0 on acquisition, 1 while the lock is legitimately contended, and 2 +# when acquisition can never succeed here (see fm_lock_try_create). A path that +# already ends in .steal is never stale-recovered through a second mutex: at +# most one primary-lock-to-.steal transition exists, so no .steal.steal is ever +# created and stale recovery cannot recurse. fm_lock_try_acquire() { local lockdir=$1 pid steal cur rc steal_owner primary_owner FM_LOCK_HELD_PID= @@ -1130,6 +1139,9 @@ fm_lock_try_acquire() { return "$rc" } +# Waits out ordinary contention but is NOT unconditional: a status-2 failure from +# fm_lock_try_acquire is returned to the caller, so callers must check the result +# and must not assume the lock is held after this returns. fm_lock_acquire_wait() { local lockdir=$1 rc while :; do diff --git a/docs/watcher-continuity.md b/docs/watcher-continuity.md index cf458e2640d..df16f486532 100644 --- a/docs/watcher-continuity.md +++ b/docs/watcher-continuity.md @@ -84,6 +84,7 @@ The same suite covers ordinary same-process session replacement for `/new`, `/re `tests/fm-watch-arm.test.sh` covers durable queue replay, real remote parent-replies ingestion into the authoritative status log, decision-only OPEN DECISIONS recovery, interrupted handling replay, generation-bound acknowledgement, a persistent live successor after recovery, a watcher close inside the handling window that must leave the printed acknowledgement valid, and the self-healing moved-generation acknowledgement that consumes its handled rows and names its remedy. `tests/fm-watch-recovery-loop.test.sh` covers the once-per-generation announcement bound with the real Pi extension against a refused handling handshake, and a handling successor that must surface a real crew event instead of going blind. `tests/fm-watcher-lock.test.sh` covers verified-successor attach, recovery publication before stale-lock removal, the typed self-eviction failure, bounded and successor-linked lifecycle rows, and a SIGSTOP counterfactual that distinguishes a live PID from a stale beacon before classifying termination. +It also covers the portable lock's stale-recovery boundary: at most one primary-lock-to-`.steal` transition exists, so a stale, malformed, or legacy directory-shaped steal mutex is reclaimed without ever creating a nested `.steal.steal` mutex, and a lock whose parent directory or owner record cannot be created fails promptly with a typed status and a bounded process-launch budget instead of spinning or recursing. `tests/fm-subagent-pretool-check.test.sh` proves Claude retains only the non-status Bash seatbelts. `tests/fm-claude-stop-autoarm.test.sh` covers the auto-arm's scope, stale and live session owners, unchanged AFK and need boundaries, single-flight, bounded failure retries, benign live-watcher cycle ends, one-notice failure episodes, and exit-2 translation. It also covers abandoned single-flight claims: a claim the ledger shows already finished, and one whose recorded pid-identity no longer matches its live pid while the ledger still reads arming or is absent entirely, are both reclaimed so a lapsed home re-arms, while an identity-matched claim still arming, one the ledger does not name, and the guard's own terminal check keep the gate closed ([`turnend-guard.md`](turnend-guard.md) owns that boundary). From 5bdbcccf2f5834873fd25790ff8873f95568e2b7 Mon Sep 17 00:00:00 2001 From: Ivan Li Date: Mon, 24 Aug 2026 19:07:34 +0800 Subject: [PATCH 13/13] no-mistakes(lint): quote stale-beacon verdict literal to fix SC2100 --- bin/fm-wake-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 7c9e0b22ffd..40f78555c11 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -251,13 +251,13 @@ fm_pi_extension_owns_supervision() { # shellcheck disable=SC2034 # Read by callers after the function returns. FM_WATCHER_VERDICT_OK=false # shellcheck disable=SC2034 # Read by callers after the function returns. -FM_WATCHER_VERDICT_REASON=stale-beacon +FM_WATCHER_VERDICT_REASON='stale-beacon' fm_watcher_supervision_verdict() { local state=$1 watch=$2 grace=${3:-${FM_GUARD_GRACE:-300}} home=${4:-$FM_HOME} local root=${5:-$FM_ROOT} local beat age fresh=false model FM_WATCHER_VERDICT_OK=false - FM_WATCHER_VERDICT_REASON=stale-beacon + FM_WATCHER_VERDICT_REASON='stale-beacon' beat="$state/.last-watcher-beat" age=$(fm_path_age "$beat") case "$age" in