From 5c0a098a620314c13b8f465b1ae3827b6a15061f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 11:50:15 +0200 Subject: [PATCH 1/6] fix(watch): bound watcher shutdown on the recovery marker lock The watcher's EXIT trap took the recovery-marker lock with an unbounded wait, so a live holder made SIGTERM a no-op: the watcher stayed up until SIGKILL. Measured on the real script - with a live holder the signalled watcher was still running after 20s, where an uncontended shutdown takes 0.15s. A supervisor that cannot stop its watcher by signalling it has no supervision, so shutdown now refuses instead of blocking. Three changes, one theme: a stop that cannot be observed is not a stop. 1. bin/fm-wake-lib.sh routes every recovery-marker lock acquisition through one helper, and bin/fm-watch.sh bounds only the shutdown transition (FM_WATCHER_SHUTDOWN_LOCK_SECS, default 5). Every other caller keeps the unbounded wait it relies on, because the default is empty. On the deadline watcher_cleanup's existing failure branch fires and names the holder, so the watcher stops and says what it could not persist, and it retains the stale lock evidence the next arm reclaims exactly as that branch already did for an unwritable marker. 2. tests/fm-pr-check-security.test.sh's watcher-stop assertion now prints what it saw - elapsed time, poll count, ps state and wchan, live descendants, and the watcher's stderr tail - on the failing path only. A CI sighting of that assertion was unexplainable after the fact because it reported a verdict with no evidence. The 150-poll budget is deliberately unchanged: the measured margin over a healthy shutdown is 15-20x, so raising it would only hide a real failure. 3. tests/lib.sh now owns one liveness helper with three states: live, gone, and ps could not answer. The two former copies read an empty ps answer as LIVE, so a process reaped between the kill -0 and the ps read - the likeliest moment for a parent shell to reap it - was reported running, and a ps hiccup and a genuinely stuck process produced the same verdict and the same message. They are now different messages. SUBSTITUTION, FLAGGED FOR REVIEW. The obvious tool for (1) was fm_lock_acquire_wait_bounded, already in this file. I built it that way first and it was WRONG, measured rather than argued: tests/fm-pr-check-security.test.sh then failed "signaled watcher left its singleton lock", deterministically on Linux 2/2 and macOS 2/2 in a whole-file run, where the unmodified tree passes 2/2 on both. Instrumented, the watcher reported status=1 with the marker lock held by its own pid. That helper delegates acquisition to a CHILD process, and from that child the caller's own abandoned hold is indistinguishable from a live foreign holder, so it cannot acquire a lock the caller itself already holds. The exit path is exactly that case: a signal can land inside a recovery-marker critical section and the EXIT trap then re-enters it, which is why fm_lock_try_acquire carries an in-process self-held reclaim. The bound is therefore a deadline around the ordinary in-process acquire. Same fm_lock_try_acquire, same stale recovery and self-held reclaim, one added outcome (124 on the deadline). Its dependency footprint is smaller, not larger: no helper process, no external timeout binary, and nothing new to source from a dying shell. ERREXIT, and why the new call sites are written the way they are. tests/fm-pr-check-security.test.sh turns errexit on and off around individual commands and leaves it ON for every case that follows, so in a whole-file run a bare command returning non-zero ends the script with status 1, no assertion and no message. My first version of the stop loop called the liveness helper bare and read $?, which is exactly that shape: the suite printed 25 oks and stopped, silently, in the case under change. Every three-state call is therefore written `|| state=$?` - a condition context, exempt from errexit - and the call sites say why. Worth a maintainer's eye on its own: any bare command added to a later case in that file truncates the suite the same way. CONTRACT CLASS, with the case against it. I read (1) as restoring intended behavior. watcher_cleanup already had a "could not be persisted, retaining stale lock evidence" branch and a non-zero cleanup status, so the design already contemplated this transition not completing; the unbounded wait simply made that branch unreachable for a held lock. Against that reading: observable behavior does change. A watcher that used to wait, and would have published the marker once the holder let go, now gives up after 5s and leaves the downtime marker unpublished and the singleton lock stale, and 5 is a new policy constant with no prior art in this file. That is a fair reading. If the maintainer classes this as new behavior we take the human gate rather than repackaging it. I read (3) as new behavior in test infrastructure rather than a pure fix: a third return code is genuinely new, and about 45 existing call sites now read an unreadable ps differently than before. The case for calling it a fix is that the old direction was simply wrong. (2) changes no product behavior and prints only on an already-failing path. PROVEN BY MUTATION, each restored afterwards. - Bound removed in watcher_cleanup: the new case goes red with "signaled watcher did not stop while the recovery marker lock was held". - Deadline removed from the shared acquire helper: same red. Both transitions route through that one helper, so there is no second site to miss; that is the structural answer to the trap-site hazard below rather than a second test. - Both TERM traps in bin/fm-watch.sh disabled: the drain assertion goes red and now prints the state that makes it readable. - Only the file-level TERM trap disabled: the drain case stays GREEN, because run_check_capture re-installs the disposition on every check. Both trap sites now carry a comment saying so, since a mutation applied to one alone proves nothing. - Empty-ps direction reverted to LIVE: the liveness contract case goes red. DELIBERATELY NOT CHANGED, and worth a maintainer's eye. bin/fm-watch.sh takes the same marker lock unbounded at STARTUP (fm_recovery_marker_reopen_announced and fm_recovery_marker_arm_check, before the first beacon touch), and _fm_recovery_marker_arm_check holds the wake-queue lock while it waits, so a held marker lock wedges a starting watcher before it publishes any liveness at all. Blocking there is defensible, because a watcher that cannot read its recovery state arguably should not start, and the scope here is the exit trap. It is left alone and flagged rather than widened. --- bin/fm-wake-lib.sh | 44 +++++++++++++-- bin/fm-watch.sh | 28 ++++++++-- tests/fm-pr-check-security.test.sh | 52 +++++++++++++----- tests/fm-test-fixtures.test.sh | 72 +++++++++++++++++++++++++ tests/fm-watcher-lock.test.sh | 87 ++++++++++++++++++++++++++++++ tests/lib.sh | 40 ++++++++++++++ tests/wake-helpers.sh | 22 ++++---- 7 files changed, 313 insertions(+), 32 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index e3582cbc4da..b659e3fbdf4 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -644,6 +644,44 @@ _fm_recovery_marker_write_locked() { fi } +# Seconds a recovery-marker transition may wait for the marker lock before it +# refuses instead of blocking. Empty - the default - keeps the ordinary +# unbounded wait every mutation-critical caller relies on, so no existing caller +# changes behavior. A caller whose whole job is to STOP sets it for its own +# call: bin/fm-watch.sh's EXIT trap, where blocking on this lock turns "the +# watcher stops" into "the watcher never stops" and makes a supervisor's signal +# a no-op. On the deadline the transition returns 124 with FM_LOCK_HELD_PID +# naming whoever still holds the lock. +FM_RECOVERY_MARKER_LOCK_TIMEOUT="${FM_RECOVERY_MARKER_LOCK_TIMEOUT:-}" + +# The one place a recovery-marker transition takes the marker lock, so a bound +# cannot be added to one acquisition and silently missed by the other. +# +# The bound is a deadline around the ORDINARY acquire, deliberately NOT +# fm_lock_acquire_wait_bounded. That helper delegates acquisition to a child +# process, and from that child the caller's OWN abandoned hold is +# indistinguishable from a live foreign holder - so it cannot acquire a lock the +# caller itself already holds. The exit path is exactly that case: a signal can +# land inside a recovery-marker critical section and the EXIT trap then re-enters +# it, which is why fm_lock_try_acquire carries an in-process self-held reclaim. +# Measured: routing shutdown through the helper left the singleton lock behind +# where the plain wait released it. Keeping fm_lock_try_acquire keeps that +# reclaim, the stale-owner recovery, and every other acquisition rule identical; +# the only added outcome is 124 on the deadline, with FM_LOCK_HELD_PID naming +# whoever still holds it. +_fm_recovery_marker_lock_acquire() { # + local started + if [ -z "$FM_RECOVERY_MARKER_LOCK_TIMEOUT" ]; then + fm_lock_acquire_wait "$1" + return + fi + started=$SECONDS + while ! fm_lock_try_acquire "$1"; do + [ "$(( SECONDS - started ))" -lt "$FM_RECOVERY_MARKER_LOCK_TIMEOUT" ] || return 124 + sleep 0.1 + done +} + # Preserve a pending or announced episode's generation across downtime # republication so its outstanding acknowledgement remains usable, and keep an # already-announced generation announced so it cannot be re-presented until a @@ -653,7 +691,7 @@ _fm_recovery_marker_publish() { local marker=$1 kind=${2:-downtime} lock saved_token generation='' status=pending case "$kind" in handling|downtime) ;; *) return 1 ;; esac lock="${marker}.lock" - fm_lock_acquire_wait "$lock" || return 1 + _fm_recovery_marker_lock_acquire "$lock" || return $? if [ -d "$marker" ] && [ ! -L "$marker" ]; then fm_lock_release "$lock" return 1 @@ -869,13 +907,13 @@ fm_recovery_transition() { ;; release-lock) [ -n "$target" ] || return 1 - _fm_recovery_marker_publish "$marker" "${value:-downtime}" || return 1 + _fm_recovery_marker_publish "$marker" "${value:-downtime}" || return $? fm_lock_release "$target" ;; release-lock-existing) [ -n "$target" ] || return 1 local lock="${marker}.lock" - fm_lock_acquire_wait "$lock" || return 1 + _fm_recovery_marker_lock_acquire "$lock" || return $? if ! fm_recovery_marker_read "$marker"; then fm_lock_release "$lock" return 1 diff --git a/bin/fm-watch.sh b/bin/fm-watch.sh index bdd720a800d..640ccfe55dc 100755 --- a/bin/fm-watch.sh +++ b/bin/fm-watch.sh @@ -223,6 +223,8 @@ esac SIGNAL_GRACE=${FM_SIGNAL_GRACE:-30} # seconds to linger after a signal so trailing # signals (a status write, then the same turn's # turn-end hook) coalesce into one wake +# Internal cleanup deadline; docs/watcher-continuity.md owns recovery behavior. +WATCHER_SHUTDOWN_LOCK_SECS=5 TURNEND_CHURN_ABSORB_SECS=${FM_TURNEND_CHURN_ABSORB_SECS:-900} # longest a task's # bare turn-ends may be deferred on pane-churn # evidence alone (signal_turnend_panes_churned) @@ -1529,6 +1531,11 @@ run_check_capture() { FM_ACTIVE_CHECK_PGID=$FM_ACTIVE_CHECK_PID set +m pgid=$(ps -o pgid= -p "$FM_ACTIVE_CHECK_PID" 2>/dev/null | tr -d '[:space:]') + # This restores the file-level disposition, and it runs on EVERY check, so it + # is the site that outlives the other one. BOTH must change together: a change + # - or a mutation meant to prove the handler is load-bearing - applied only to + # the file-level `trap 'exit 1' HUP INT TERM` is silently undone here, and + # proves nothing about a watcher that has run at least one check. trap 'exit 1' HUP INT TERM if [ -n "$pgid" ] && [ "$pgid" != "$FM_ACTIVE_CHECK_PGID" ]; then fm_active_check_stop || true @@ -1853,7 +1860,7 @@ pr_poll_control_release() { } watcher_cleanup() { - local cleanup_status=0 owns_lock=0 transition=release-lock + local cleanup_status=0 owns_lock=0 transition=release-lock transition_status=0 pr_poll_control_release || cleanup_status=1 if [ "$(cat "$WATCH_LOCK/pid" 2>/dev/null || true)" = "${WATCHER_PID:-}" ]; then owns_lock=1 @@ -1865,14 +1872,25 @@ watcher_cleanup() { fm_active_check_stop || cleanup_status=1 fm_check_output_cleanup fm_custom_check_snapshot_cleanup - if [ "$owns_lock" -eq 1 ] \ - && ! fm_recovery_transition "$WATCHER_DOWNTIME_MARKER" "$transition" "$WATCH_LOCK" downtime; then - echo "watcher: recovery state could not be persisted; retaining stale lock evidence" >&2 - cleanup_status=1 + if [ "$owns_lock" -eq 1 ]; then + # _fm_recovery_marker_lock_acquire owns the in-process deadline semantics. + FM_RECOVERY_MARKER_LOCK_TIMEOUT=$WATCHER_SHUTDOWN_LOCK_SECS + transition_status=0 + fm_recovery_transition "$WATCHER_DOWNTIME_MARKER" "$transition" "$WATCH_LOCK" downtime \ + || transition_status=$? + FM_RECOVERY_MARKER_LOCK_TIMEOUT= + if [ "$transition_status" -eq 124 ]; then + echo "watcher: recovery state could not be persisted within ${WATCHER_SHUTDOWN_LOCK_SECS}s (marker lock held by pid ${FM_LOCK_HELD_PID:-unknown}); stopping and retaining stale lock evidence" >&2 + cleanup_status=1 + elif [ "$transition_status" -ne 0 ]; then + echo "watcher: recovery state could not be persisted; retaining stale lock evidence" >&2 + cleanup_status=1 + fi fi return "$cleanup_status" } trap watcher_cleanup EXIT +# See run_check_capture's trap-restoration comment before changing this handler. trap 'exit 1' HUP INT TERM # This watcher's own pid, as recorded in the lock by fm_lock_claim (which writes # ${BASHPID:-$$} from this same main shell). Read directly, never via a command diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index b38435781bc..769b816802a 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -47,15 +47,10 @@ file_mode() { fi } -process_is_live_non_zombie() { - local pid=$1 stat - kill -0 "$pid" 2>/dev/null || return 1 - stat=$(ps -p "$pid" -o stat= 2>/dev/null || true) - case "$stat" in - Z*) return 1 ;; - esac - return 0 -} +# Process liveness comes from tests/lib.sh's is_live_non_zombie (0 live, 1 gone, +# 2 ps could not answer). This file used to carry its own copy that read an +# empty ps answer as LIVE, which is what let a ps hiccup and a genuinely stuck +# watcher reach the same assertion with the same message. LINK_KIND= LINK_TARGET= @@ -1115,6 +1110,7 @@ SH test_returned_custom_check_descendants_are_drained() { local backend dir state fakebin ready direct_done child_pid_file sentinel watcher_pid child_pid i rc alive force_fallback + local watcher_state signaled_at for backend in installed-timeout fallback-timeout; do dir=$(make_case "returned-custom-descendant-$backend") state="$dir/home/state" @@ -1165,23 +1161,55 @@ SH && [ -e "$state/.last-check" ] \ || fail "$backend watcher did not complete the direct custom check" child_pid=$(cat "$child_pid_file") + signaled_at=$(date +%s) kill -TERM "$watcher_pid" 2>/dev/null || fail "could not stop $backend watcher" + # The budget is 150 POLLS, not 3 seconds: each iteration is a 0.02s sleep + # plus a ps, so its wall cost grows with machine load and the watcher gets + # proportionally MORE time exactly when it needs it. Do not raise it to make + # a red go away - the measured margin over a healthy shutdown is 15-20x, so + # a failure here is a real one and raising the budget only hides it. + # `|| watcher_state=$?` rather than a bare call: cases above leave errexit + # ON, and a bare command returning non-zero would end this script silently + # with no assertion and no message. i=0 - while process_is_live_non_zombie "$watcher_pid" && [ "$i" -lt 150 ]; do + watcher_state=0 + while [ "$i" -lt 150 ]; do + watcher_state=0 + is_live_non_zombie "$watcher_pid" || watcher_state=$? + [ "$watcher_state" -eq 1 ] && break sleep 0.02 i=$((i + 1)) done - if process_is_live_non_zombie "$watcher_pid"; then + if [ "$watcher_state" -ne 1 ]; then + # Say what was seen. This costs nothing on the green path - it runs only + # when the case is already failing - and without it a failure here reports + # a verdict with no evidence, which is exactly what left one CI sighting + # unexplainable after the fact. + printf '# %s watcher still %s %ss after TERM (%s polls); state below\n' \ + "$backend" \ + "$([ "$watcher_state" -eq 2 ] && printf 'unreadable' || printf 'live')" \ + "$(( $(date +%s) - signaled_at ))" "$i" >&2 + ps -o pid=,ppid=,pgid=,stat=,wchan=,args= -p "$watcher_pid" >&2 2>/dev/null || true + ps -eo pid=,ppid=,stat=,args= 2>/dev/null | awk -v w="$watcher_pid" '$2 == w' >&2 || true + printf '# %s watcher stderr tail:\n' "$backend" >&2 + tail -20 "$dir/watch.err" >&2 2>/dev/null || true kill -KILL "$watcher_pid" 2>/dev/null || true wait "$watcher_pid" 2>/dev/null || true kill -KILL "$child_pid" 2>/dev/null || true + # A ps that cannot answer and a watcher that will not stop are different + # failures and must not share a message. + [ "$watcher_state" -ne 2 ] \ + || fail "$backend watcher liveness was unreadable after the direct check returned" fail "$backend watcher did not stop after the direct check returned" fi rc=0 wait "$watcher_pid" || rc=$? [ "$rc" -ne 0 ] || fail "$backend signaled watcher exited successfully" + # Only a definite LIVE counts as a surviving descendant; an unreadable ps is + # not evidence of one, and the sentinel assertion below proves the drain + # independently of this reading. alive=0 - process_is_live_non_zombie "$child_pid" && alive=1 + is_live_non_zombie "$child_pid" && alive=1 [ "$alive" -eq 0 ] || kill -KILL "$child_pid" 2>/dev/null || true wait "$child_pid" 2>/dev/null || true [ "$alive" -eq 0 ] || fail "$backend watcher left a returned check descendant alive" diff --git a/tests/fm-test-fixtures.test.sh b/tests/fm-test-fixtures.test.sh index 5d9c67c8b1a..c549c2a0c95 100755 --- a/tests/fm-test-fixtures.test.sh +++ b/tests/fm-test-fixtures.test.sh @@ -279,8 +279,80 @@ test_spawn_home_layout() { pass "spawn-home layout writes harness pin, beat, and brief" } +test_is_live_non_zombie_separates_gone_from_unreadable() { + local dir fakebin live zombie_file zombie zombie_pid zombie_state rc note i + dir="$TMP_ROOT/liveness" + mkdir -p "$dir" + fakebin=$(fm_fakebin "$dir") + + sleep 30 & + live=$! + rc=0 + is_live_non_zombie "$live" || rc=$? + [ "$rc" -eq 0 ] || fail "a running process was not reported live (rc=$rc)" + + # A real zombie: the perl parent forks a child that exits at once and never + # reaps it, so the pid stays present and ps describes it as Z (Linux) or ZN + # (macOS). kill -0 succeeds on a zombie, so this is exactly the case a bare + # kill -0 gets wrong. + zombie_file="$dir/zombie.pid" + perl -e '$| = 1; my $p = fork; exit 0 unless $p; + open my $f, ">", $ARGV[0] or die $!; print {$f} "$p\n"; close $f; sleep 30' \ + "$zombie_file" & + zombie=$! + i=0 + while [ "$i" -lt 200 ] && [ ! -s "$zombie_file" ]; do + sleep 0.05 + i=$((i + 1)) + done + [ -s "$zombie_file" ] || fail "zombie fixture never reported its child pid" + zombie_pid=$(cat "$zombie_file") + zombie_state= + i=0 + while [ "$i" -lt 200 ]; do + zombie_state=$(ps -p "$zombie_pid" -o stat= 2>/dev/null | tr -d '[:space:]') || zombie_state= + case "$zombie_state" in Z*) break ;; esac + sleep 0.05 + i=$((i + 1)) + done + case "$zombie_state" in + Z*) ;; + *) + kill -KILL "$live" "$zombie" 2>/dev/null || true + wait "$live" 2>/dev/null || true + wait "$zombie" 2>/dev/null || true + fail "zombie fixture child $zombie_pid never became a zombie (last state: ${zombie_state:-unreadable})" + ;; + esac + rc=0 + is_live_non_zombie "$zombie_pid" || rc=$? + [ "$rc" -eq 1 ] || fail "an unreaped zombie was not reported gone (rc=$rc)" + + # ps that answers nothing for a pid that is still present. The empty answer + # must NOT be read as live: that direction is what made a ps hiccup and a + # genuinely stuck process indistinguishable. + fm_fake_exit0 "$fakebin" ps + note="$dir/unknown.err" + rc=0 + PATH="$fakebin:$PATH" is_live_non_zombie "$live" 2> "$note" || rc=$? + [ "$rc" -eq 2 ] || fail "a silent ps for a present pid was not reported unknown (rc=$rc)" + assert_grep 'UNKNOWN, not live' "$note" "unknown liveness was not announced" + + # Same silent ps, but the pid is gone: that is a definite answer, not unknown. + kill -KILL "$live" 2>/dev/null || true + wait "$live" 2>/dev/null || true + rc=0 + PATH="$fakebin:$PATH" is_live_non_zombie "$live" || rc=$? + [ "$rc" -eq 1 ] || fail "a departed pid was not reported gone under a silent ps (rc=$rc)" + + kill -KILL "$zombie" 2>/dev/null || true + wait "$zombie" 2>/dev/null || true + pass "is_live_non_zombie separates live, gone, and an unreadable ps" +} + test_git_config_isolation || fail "Git fixture config isolation" test_touch_epoch_preserves_repeated_dst_hour +test_is_live_non_zombie_separates_gone_from_unreadable test_no_mistakes_version_constant test_no_mistakes_init_doctor_markers test_fake_gh_and_gh_axi diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index c0d37f0cf61..28fadda975d 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -531,6 +531,92 @@ test_watcher_self_evicts_on_lock_takeover() { pass "watcher self-evicts when the lock pid no longer names it" } +# Shutdown must be bounded. Every recovery-marker transition takes the marker +# lock, and the one the EXIT trap runs used to wait for it forever: with a live +# holder, SIGTERM became a no-op and the only way to stop the watcher was +# SIGKILL. A supervisor that cannot stop its watcher by signalling it has no +# supervision, so this pins that the signalled watcher exits anyway and says +# what it could not persist. +test_shutdown_is_bounded_when_marker_lock_is_held() { + local dir state fakebin out err pid holder holder_pid holder_survived i rc seeded lock_kept + dir=$(make_case bounded-shutdown) + state="$dir/state" + fakebin="$dir/fakebin" + out="$dir/watch.out" + err="$dir/watch.err" + PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_POLL=0.2 FM_SIGNAL_GRACE=1 \ + FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 "$WATCH" > "$out" 2> "$err" & + pid=$! + i=0 + while [ "$i" -lt 80 ]; do + [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$pid" ] \ + && [ -e "$state/.last-watcher-beat" ] \ + && break + sleep 0.1 + i=$((i + 1)) + done + [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$pid" ] \ + && [ -e "$state/.last-watcher-beat" ] \ + || fail "watcher did not publish its singleton lock and beacon" + + # Seed a LIVE holder on the recovery marker lock. It has to be seeded after + # the watcher is up: the watcher takes this same lock at startup, and seeding + # first would wedge it there instead of at the shutdown path under test. Its + # stdio goes to /dev/null so a holder outliving a failed assertion can never + # hold a caller's output pipe open. + sleep 300 >/dev/null 2>&1 & + holder=$! + seeded=0 + i=0 + while [ "$i" -lt 200 ]; do + if mkdir "$state/.watcher-down.lock" 2>/dev/null; then + printf '%s\n' "$holder" > "$state/.watcher-down.lock/pid" + seeded=1 + break + fi + sleep 0.05 + i=$((i + 1)) + done + if [ "$seeded" -ne 1 ]; then + kill "$holder" 2>/dev/null || true + wait "$holder" 2>/dev/null || true + kill -KILL "$pid" 2>/dev/null || true + wait "$pid" 2>/dev/null || true + fail "could not seed a live holder on the recovery marker lock" + fi + + kill -TERM "$pid" 2>/dev/null || fail "could not signal the watcher" + # 20s ceiling against a 5s shutdown bound. Without the bound the watcher never + # exits at all, so this is a wide margin on a bounded path, not a tight race. + rc=0 + wait_for_exit "$pid" 200 || rc=$? + # Read everything the holder is needed for, then retire it, so no assertion + # below can leave a 300s sleeper behind. + holder_pid=$(cat "$state/.watcher-down.lock/pid" 2>/dev/null || true) + holder_survived=0 + is_live_non_zombie "$holder" && holder_survived=1 + lock_kept=0 + [ -e "$state/.watch.lock" ] && lock_kept=1 + kill "$holder" 2>/dev/null || true + wait "$holder" 2>/dev/null || true + + [ "$rc" -ne 124 ] \ + || fail "signaled watcher did not stop while the recovery marker lock was held" + [ "$rc" -ne 0 ] || fail "signaled watcher exited successfully" + assert_grep 'recovery state could not be persisted within' "$err" \ + "bounded shutdown did not report what it could not persist" + assert_grep "marker lock held by pid $holder" "$err" \ + "bounded shutdown did not name the live holder it gave up on" + # It gave up on the lock rather than stealing it, and left the stale singleton + # evidence the next arm reclaims - the same outcome an unwritable marker has. + [ "$holder_pid" = "$holder" ] \ + || fail "bounded shutdown clobbered the live marker-lock holder (got '$holder_pid')" + [ "$holder_survived" -eq 1 ] || fail "bounded shutdown killed the marker-lock holder" + [ "$lock_kept" -eq 1 ] \ + || fail "bounded shutdown released the stale lock evidence it reported retaining" + pass "signaled watcher stops on a held recovery marker lock and reports what it could not persist" +} + test_arm_self_eviction_is_loud_without_successor() { local dir state fakebin armout armpid watcher_pid status i dir=$(make_case arm-self-evict) @@ -1127,6 +1213,7 @@ test_lock_paused_mid_acquire_claim_fails_during_steal test_watch_restart_rejects_reused_pid test_watch_restart_attaches_to_healthy_peer test_watcher_self_evicts_on_lock_takeover +test_shutdown_is_bounded_when_marker_lock_is_held test_arm_self_eviction_is_loud_without_successor test_arm_attaches_and_waits_for_live_fresh_watcher test_attached_arm_signal_is_recorded_in_cycle_ledger diff --git a/tests/lib.sh b/tests/lib.sh index 4429362ae55..d1df520ac24 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -111,6 +111,46 @@ FM_TEST_OWNER_IDENTITY=$(fm_test_pid_identity "$$") || { return 1 } +# --- process liveness ------------------------------------------------------- +# +# is_live_non_zombie - THREE states, because two cannot express what a +# reader actually needs to know: +# +# 0 the pid names a live, non-zombie process +# 1 the pid is gone, or is a zombie waiting to be reaped +# 2 ps could not answer for a pid that is still present +# +# The three-state contract exists because the two-state version got the empty +# case backwards. `ps` returning nothing was read as "still running", so a +# process reaped between the `kill -0` below and the `ps` read - the likeliest +# moment for a parent shell to reap it - was reported LIVE, and an assertion +# built on that could not tell a genuinely stuck process from a `ps` that simply +# did not answer. Both produced the same verdict and therefore the same message. +# +# So an empty answer is never LIVE here. It is re-read: a pid that has since +# vanished is gone (1), and only a pid still present after a retry is UNKNOWN +# (2), which is announced on stderr so a caller that folds it into a boolean +# still leaves a reader the reason. +is_live_non_zombie() { # + local pid=$1 stat + kill -0 "$pid" 2>/dev/null || return 1 + stat=$(ps -p "$pid" -o stat= 2>/dev/null || true) + if [ -z "$stat" ]; then + kill -0 "$pid" 2>/dev/null || return 1 + stat=$(ps -p "$pid" -o stat= 2>/dev/null || true) + if [ -z "$stat" ]; then + kill -0 "$pid" 2>/dev/null || return 1 + printf '# is_live_non_zombie: ps reported no state for present pid %s; UNKNOWN, not live\n' \ + "$pid" >&2 + return 2 + fi + fi + case "$stat" in + Z*) return 1 ;; + esac + return 0 +} + # --- process-event runner reaping ------------------------------------------- # # A process-event runner is detached into its own process group and reparents to diff --git a/tests/wake-helpers.sh b/tests/wake-helpers.sh index da83bb3dc91..89a29dfd23f 100644 --- a/tests/wake-helpers.sh +++ b/tests/wake-helpers.sh @@ -297,9 +297,17 @@ SH } wait_for_exit() { - local pid=$1 limit=${2:-50} i=0 + local pid=$1 limit=${2:-50} i=0 state while [ "$i" -lt "$limit" ]; do - if ! is_live_non_zombie "$pid"; then + # Only a definite "gone" reaps. tests/lib.sh's is_live_non_zombie also + # reports UNKNOWN (2) for a present pid ps could not describe, and waiting + # on one of those would block here until it really exited - unbounded, which + # is the opposite of what this helper is for. Keep polling instead. + # `|| state=$?` rather than a bare call, so this stays safe in a suite + # running under errexit. + state=0 + is_live_non_zombie "$pid" || state=$? + if [ "$state" -eq 1 ]; then wait "$pid" return "$?" fi @@ -311,16 +319,6 @@ wait_for_exit() { return 124 } -is_live_non_zombie() { - local pid=$1 stat - kill -0 "$pid" 2>/dev/null || return 1 - stat=$(ps -p "$pid" -o stat= 2>/dev/null || true) - case "$stat" in - Z*) return 1 ;; - esac - return 0 -} - hash_text() { if command -v md5 >/dev/null 2>&1; then printf '%s' "$1" | md5 -q From 947cc1a27c3c2b7aa1c3be78c3cc02c49546dd17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 12:06:48 +0200 Subject: [PATCH 2/6] fix(review): Fix watcher diagnostics, liveness races, and timeout configuration scope --- tests/fm-pr-check-security.test.sh | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index 769b816802a..72dd210852c 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -1180,6 +1180,8 @@ SH sleep 0.02 i=$((i + 1)) done + watcher_state=0 + is_live_non_zombie "$watcher_pid" || watcher_state=$? if [ "$watcher_state" -ne 1 ]; then # Say what was seen. This costs nothing on the green path - it runs only # when the case is already failing - and without it a failure here reports @@ -1189,8 +1191,18 @@ SH "$backend" \ "$([ "$watcher_state" -eq 2 ] && printf 'unreadable' || printf 'live')" \ "$(( $(date +%s) - signaled_at ))" "$i" >&2 - ps -o pid=,ppid=,pgid=,stat=,wchan=,args= -p "$watcher_pid" >&2 2>/dev/null || true - ps -eo pid=,ppid=,stat=,args= 2>/dev/null | awk -v w="$watcher_pid" '$2 == w' >&2 || true + printf '# descendant tree (watcher pid %s; recorded child pid %s): pid ppid pgid stat wchan args\n' \ + "$watcher_pid" "$child_pid" >&2 + ps -eo pid=,ppid=,pgid=,stat=,wchan=,args= 2>/dev/null | awk -v w="$watcher_pid" -v c="$child_pid" ' + function tree(pid, indent, child) { + if (seen[pid]++) return + if (pid in rows) print indent rows[pid] + for (child in parents) + if (parents[child] == pid) tree(child, indent " ") + } + { rows[$1] = $0; parents[$1] = $2 } + END { tree(w, ""); tree(c, "") } + ' >&2 || true printf '# %s watcher stderr tail:\n' "$backend" >&2 tail -20 "$dir/watch.err" >&2 2>/dev/null || true kill -KILL "$watcher_pid" 2>/dev/null || true From 9393a81988c8e999e660627f02d6f959da1929a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 12:35:52 +0200 Subject: [PATCH 3/6] fix(review): Reject unknown liveness and remove last ambient timeout override --- bin/fm-wake-lib.sh | 2 +- tests/fm-daemon.test.sh | 7 ++-- tests/fm-pr-check-security.test.sh | 14 ++++---- tests/fm-watch-arm.test.sh | 14 +++++--- tests/fm-watcher-lock.test.sh | 53 +++++++++++++++++++++--------- 5 files changed, 58 insertions(+), 32 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index b659e3fbdf4..82b7f967039 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -652,7 +652,7 @@ _fm_recovery_marker_write_locked() { # watcher stops" into "the watcher never stops" and makes a supervisor's signal # a no-op. On the deadline the transition returns 124 with FM_LOCK_HELD_PID # naming whoever still holds the lock. -FM_RECOVERY_MARKER_LOCK_TIMEOUT="${FM_RECOVERY_MARKER_LOCK_TIMEOUT:-}" +FM_RECOVERY_MARKER_LOCK_TIMEOUT= # The one place a recovery-marker transition takes the marker lock, so a bound # cannot be added to one acquisition and silently missed by the other. diff --git a/tests/fm-daemon.test.sh b/tests/fm-daemon.test.sh index 510a4320988..6d64764daa2 100755 --- a/tests/fm-daemon.test.sh +++ b/tests/fm-daemon.test.sh @@ -2410,7 +2410,7 @@ test_wedge_alarm_hung_channel_times_out_and_falls_through() { } test_wedge_alarm_backgrounded_command_times_out_and_reaps_descendant() { - local dir daemon_log child_file child command + local dir daemon_log child_file child command child_state dir=$(make_wedge_case wedge-backgrounded-timeout) daemon_log="$dir/daemon.log" child_file="$dir/notifier-child" @@ -2421,8 +2421,11 @@ test_wedge_alarm_backgrounded_command_times_out_and_reaps_descendant() { child=$(cat "$child_file") grep -F 'command notifier timed out' "$daemon_log" >/dev/null \ || fail "a backgrounded command notifier bypassed its timeout: $(cat "$daemon_log" 2>/dev/null)" - if is_live_non_zombie "$child"; then + child_state=0 + is_live_non_zombie "$child" || child_state=$? + if [ "$child_state" -ne 1 ]; then kill -TERM "$child" 2>/dev/null || true + [ "$child_state" -ne 2 ] || fail "timed-out command notifier descendant liveness was unreadable (pid $child)" fail "a timed-out command notifier left its descendant running (pid $child)" fi pass "a backgrounded command notifier remains bounded until its process group is reaped" diff --git a/tests/fm-pr-check-security.test.sh b/tests/fm-pr-check-security.test.sh index 72dd210852c..9d5803ffefd 100755 --- a/tests/fm-pr-check-security.test.sh +++ b/tests/fm-pr-check-security.test.sh @@ -1109,7 +1109,7 @@ SH } test_returned_custom_check_descendants_are_drained() { - local backend dir state fakebin ready direct_done child_pid_file sentinel watcher_pid child_pid i rc alive force_fallback + local backend dir state fakebin ready direct_done child_pid_file sentinel watcher_pid child_pid i rc child_state force_fallback local watcher_state signaled_at for backend in installed-timeout fallback-timeout; do dir=$(make_case "returned-custom-descendant-$backend") @@ -1217,14 +1217,12 @@ SH rc=0 wait "$watcher_pid" || rc=$? [ "$rc" -ne 0 ] || fail "$backend signaled watcher exited successfully" - # Only a definite LIVE counts as a surviving descendant; an unreadable ps is - # not evidence of one, and the sentinel assertion below proves the drain - # independently of this reading. - alive=0 - is_live_non_zombie "$child_pid" && alive=1 - [ "$alive" -eq 0 ] || kill -KILL "$child_pid" 2>/dev/null || true + child_state=0 + is_live_non_zombie "$child_pid" || child_state=$? + [ "$child_state" -eq 1 ] || kill -KILL "$child_pid" 2>/dev/null || true wait "$child_pid" 2>/dev/null || true - [ "$alive" -eq 0 ] || fail "$backend watcher left a returned check descendant alive" + [ "$child_state" -ne 2 ] || fail "$backend returned check descendant liveness was unreadable" + [ "$child_state" -eq 1 ] || fail "$backend watcher left a returned check descendant alive" [ ! -e "$sentinel" ] || fail "$backend returned check descendant reached its sentinel" ! find "$state" -maxdepth 1 -name '.fm-custom-check.*' -print | grep . >/dev/null \ || fail "$backend watcher left a private custom check snapshot" diff --git a/tests/fm-watch-arm.test.sh b/tests/fm-watch-arm.test.sh index 33cd245700a..8810cf0b3e5 100755 --- a/tests/fm-watch-arm.test.sh +++ b/tests/fm-watch-arm.test.sh @@ -141,7 +141,7 @@ drain_ack_pair() { # } start_rearm_arm() { # [predecessor-arm-pid] - local home=$1 state=$2 fakebin=$3 armout=$4 predecessor=${5:-} i + local home=$1 state=$2 fakebin=$3 armout=$4 predecessor=${5:-} i process_state PATH="$fakebin:$PATH" FM_HOME="$home" FM_STATE_OVERRIDE="$state" \ FM_POLL=1 FM_SIGNAL_GRACE=0 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 \ FM_WATCH_PREDECESSOR_ARM_PID="$predecessor" \ @@ -150,7 +150,9 @@ start_rearm_arm() { # [predecessor-arm-pid] i=0 while [ "$i" -lt 80 ]; do grep -q '^watcher: started ' "$armout" 2>/dev/null && return 0 - is_live_non_zombie "$ARM_PID" || return 0 + process_state=0 + is_live_non_zombie "$ARM_PID" || process_state=$? + [ "$process_state" -eq 1 ] && return 0 sleep 0.05 i=$((i + 1)) done @@ -365,7 +367,7 @@ test_rearm_resurfaces_durable_queue_and_remote_open_decision() { } test_marker_publish_failure_retains_recovery_evidence() { - local dir home state fakebin first_arm watcher_pid armout + local dir home state fakebin first_arm watcher_pid armout process_state dir=$(make_case downtime-marker-publish-failure) home="$dir/home" state="$dir/state" @@ -382,8 +384,10 @@ test_marker_publish_failure_retains_recovery_evidence() { [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$watcher_pid" ] \ || fail "marker publication failure discarded stale-lock recovery evidence" - ! is_live_non_zombie "$watcher_pid" \ - || fail "marker-failure fixture watcher remained live" + process_state=0 + is_live_non_zombie "$watcher_pid" || process_state=$? + [ "$process_state" -ne 2 ] || fail "marker-failure fixture watcher liveness was unreadable" + [ "$process_state" -eq 1 ] || fail "marker-failure fixture watcher remained live" rmdir "$state/.watcher-down" armout="$dir/recovery-arm.out" diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 28fadda975d..3c989750721 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -35,7 +35,7 @@ drain_and_ack() { # } test_singleton_start() { - local dir state fakebin out1 out2 pid1 pid2 live i + local dir state fakebin out1 out2 pid1 pid2 live i pid1_state pid2_state dir=$(make_case singleton) state="$dir/state" fakebin="$dir/fakebin" @@ -47,13 +47,17 @@ test_singleton_start() { pid2=$! i=0 while [ "$i" -lt 50 ]; do - live=0 - is_live_non_zombie "$pid1" && live=$((live + 1)) - is_live_non_zombie "$pid2" && live=$((live + 1)) - [ "$live" -eq 1 ] && break + pid1_state=0 + pid2_state=0 + is_live_non_zombie "$pid1" || pid1_state=$? + is_live_non_zombie "$pid2" || pid2_state=$? + live=$(( (pid1_state == 0) + (pid2_state == 0) )) + [ "$pid1_state" -ne 2 ] && [ "$pid2_state" -ne 2 ] && [ "$live" -eq 1 ] && break sleep 0.1 i=$((i + 1)) done + [ "$pid1_state" -ne 2 ] && [ "$pid2_state" -ne 2 ] \ + || fail "singleton watcher liveness was unreadable" [ "$live" -eq 1 ] || fail "expected exactly one live watcher, got $live" i=0 while [ "$i" -lt 50 ] && ! grep -h 'watcher: already running pid ' "$out1" "$out2" >/dev/null 2>&1; do @@ -425,7 +429,7 @@ test_lock_paused_mid_acquire_claim_fails_during_steal() { } test_watch_restart_rejects_reused_pid() { - local dir state fakebin out live pid i + local dir state fakebin out live pid i process_state dir=$(make_case restart-reused-pid) state="$dir/state" fakebin="$dir/fakebin" @@ -440,12 +444,18 @@ test_watch_restart_rejects_reused_pid() { PATH="$fakebin:$PATH" FM_HOME="$dir" FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 "$WATCH_ARM" --restart > "$out" & pid=$! i=0 - while [ "$i" -lt 80 ] && is_live_non_zombie "$pid"; do + while [ "$i" -lt 80 ]; do + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + [ "$process_state" -eq 1 ] && break sleep 0.1 i=$((i + 1)) done - is_live_non_zombie "$pid" \ - && fail "restart did not surface recovery after replacing a reused-pid lock" + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + [ "$process_state" -ne 2 ] || fail "restart arm liveness was unreadable after replacing a reused-pid lock" + [ "$process_state" -eq 1 ] \ + || fail "restart did not surface recovery after replacing a reused-pid lock" wait "$pid" 2>/dev/null || true grep -F 'check: rearm-resurface' "$out" >/dev/null \ || fail "restart replaced reused-pid lock without surfacing recovery: $(cat "$out")" @@ -737,7 +747,7 @@ test_arm_starts_and_self_heals() { # before reporting 'started' - whether the lock is empty (clean start) or held # by a dead pid with a fresh-looking leftover beacon (self-heal). It must never # report 'healthy' off a dead pid. One row per pre-state, one assertion block. - local row dir state fakebin armout armpid i lock_pid dead_pid + local row dir state fakebin armout armpid i lock_pid dead_pid process_state for row in clean dead-pid; do dir=$(make_case "arm-$row") state="$dir/state" @@ -759,15 +769,20 @@ test_arm_starts_and_self_heals() { i=0 while [ "$i" -lt 80 ]; do if [ "$row" = dead-pid ]; then - is_live_non_zombie "$armpid" || break + process_state=0 + is_live_non_zombie "$armpid" || process_state=$? + [ "$process_state" -eq 1 ] && break else grep -qF 'watcher: started pid=' "$armout" 2>/dev/null && break fi sleep 0.1; i=$((i + 1)) done if [ "$row" = dead-pid ]; then - is_live_non_zombie "$armpid" \ - && fail "arm did not surface recovery after reclaiming a dead-pid lock" + process_state=0 + is_live_non_zombie "$armpid" || process_state=$? + [ "$process_state" -ne 2 ] || fail "arm liveness was unreadable after reclaiming a dead-pid lock" + [ "$process_state" -eq 1 ] \ + || fail "arm did not surface recovery after reclaiming a dead-pid lock" wait "$armpid" 2>/dev/null || true grep -F 'check: rearm-resurface' "$armout" >/dev/null \ || fail "arm reclaimed dead-pid lock without surfacing recovery: $(cat "$armout")" @@ -788,7 +803,7 @@ test_arm_starts_and_self_heals() { } test_arm_hup_cleans_child_and_temp_output() { - local dir state fakebin armout i armpid lock_pid status + local dir state fakebin armout i armpid lock_pid status process_state dir=$(make_case arm-hup-cleanup) state="$dir/state" fakebin="$dir/fakebin" @@ -808,11 +823,17 @@ test_arm_hup_cleans_child_and_temp_output() { status=$? [ "$status" -eq 129 ] || fail "arm did not exit with HUP status (got $status)" i=0 - while [ "$i" -lt 80 ] && is_live_non_zombie "$lock_pid"; do + while [ "$i" -lt 80 ]; do + process_state=0 + is_live_non_zombie "$lock_pid" || process_state=$? + [ "$process_state" -eq 1 ] && break sleep 0.1 i=$((i + 1)) done - ! is_live_non_zombie "$lock_pid" || fail "HUP cleanup left watcher child running" + process_state=0 + is_live_non_zombie "$lock_pid" || process_state=$? + [ "$process_state" -ne 2 ] || fail "HUP cleanup watcher child liveness was unreadable" + [ "$process_state" -eq 1 ] || fail "HUP cleanup left watcher child running" ! ls "$state"/.watch-arm-output.* >/dev/null 2>&1 || fail "HUP cleanup left temp output behind" pass "arm cleans child watcher and temp output on HUP" } From 4e547d559f91374e086465217af479b79e4c5d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 13:51:35 +0200 Subject: [PATCH 4/6] fix(review): Stop recursive lock recovery on resource failures --- bin/fm-wake-lib.sh | 8 ++- tests/fm-watcher-lock.test.sh | 113 ++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 82b7f967039..a8f5f305ac4 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -536,14 +536,14 @@ fm_lock_claim() { fm_lock_try_create() { local lockdir=$1 allowed_steal_owner=${2:-} ownerdir 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 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 @@ -958,7 +958,11 @@ fm_lock_try_acquire() { if fm_lock_try_create "$lockdir"; then return 0 + else + rc=$? fi + [ "$rc" -eq 1 ] || return 1 + [ -e "$lockdir" ] || [ -L "$lockdir" ] || return 1 fm_current_pid current || return 1 pid=$(cat "$lockdir/pid" 2>/dev/null || true) diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index 3c989750721..8dc9735a9d5 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -627,6 +627,117 @@ test_shutdown_is_bounded_when_marker_lock_is_held() { pass "signaled watcher stops on a held recovery marker lock and reports what it could not persist" } +test_lock_resource_failure_returns() ( + local dir state pid='' i process_state rc result + if [ "$(id -u)" -eq 0 ]; then + pass "lock owner-directory failure returns # SKIP root bypasses directory permissions" + return + fi + dir=$(make_case lock-resource-failure) + state="$dir/state" + result="$dir/acquire.rc" + trap 'if [ -n "$pid" ]; then kill -KILL "$pid" 2>/dev/null || true; wait "$pid" 2>/dev/null || true; fi; chmod u+w "$state"' EXIT + chmod a-w "$state" || fail "could not make lock fixture state unwritable" + + FM_STATE_OVERRIDE="$state" bash -eu -c ' + . "$1" + if fm_lock_owner_dir "$2"; then + exit 10 + fi + printf "ready\n" > "$3" + if fm_lock_try_acquire "$2"; then rc=0; else rc=$?; fi + printf "%s\n" "$rc" > "$4" + exit "$rc" + ' _ "$LIB" "$state/.resource.lock" "$dir/fault.ready" "$result" \ + > "$dir/acquire.out" 2> "$dir/acquire.err" & + pid=$! + i=0 + while [ "$i" -lt 50 ]; do + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + [ "$process_state" -eq 1 ] && break + sleep 0.1 + i=$((i + 1)) + done + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + [ -s "$dir/fault.ready" ] || fail "fixture could not induce owner-directory creation failure" + [ "$process_state" -ne 2 ] || fail "resource-fault acquirer liveness was unreadable" + [ "$process_state" -eq 1 ] && [ -s "$result" ] \ + || fail "lock acquisition did not return on owner-directory creation failure" + rc=0 + wait "$pid" || rc=$? + pid= + [ "$rc" -ne 0 ] && [ "$rc" = "$(cat "$result")" ] \ + || fail "lock acquisition did not return nonzero on owner-directory creation failure" + [ ! -e "$state/.resource.lock" ] && [ ! -L "$state/.resource.lock" ] \ + || fail "failed acquisition published a lock" + pass "lock acquisition returns nonzero when owner-directory creation fails" +) + +test_shutdown_is_bounded_when_state_is_unwritable() ( + local dir state fakebin err pid='' i process_state rc started elapsed + if [ "$(id -u)" -eq 0 ]; then + pass "watcher shutdown on unwritable state # SKIP root bypasses directory permissions" + return + fi + dir=$(make_case unwritable-state-shutdown) + state="$dir/state" + fakebin="$dir/fakebin" + err="$dir/watch.err" + trap 'if [ -n "$pid" ]; then kill -KILL "$pid" 2>/dev/null || true; wait "$pid" 2>/dev/null || true; fi; chmod u+w "$state"' EXIT + PATH="$fakebin:$PATH" FM_HOME="$dir" FM_STATE_OVERRIDE="$state" FM_POLL=0.2 FM_SIGNAL_GRACE=1 \ + FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 "$WATCH" > "$dir/watch.out" 2> "$err" & + pid=$! + i=0 + while [ "$i" -lt 80 ]; do + [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$pid" ] \ + && [ -e "$state/.last-watcher-beat" ] \ + && [ ! -e "$state/.watcher-down.lock" ] && [ ! -L "$state/.watcher-down.lock" ] \ + && break + sleep 0.1 + i=$((i + 1)) + done + [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$pid" ] \ + && [ -e "$state/.last-watcher-beat" ] \ + || fail "watcher did not publish its singleton lock and beacon before the resource fault" + chmod a-w "$state" || fail "could not make watcher state unwritable" + [ ! -e "$state/.watcher-down.lock" ] && [ ! -L "$state/.watcher-down.lock" ] \ + || fail "resource-fault fixture encountered marker-lock contention" + if FM_STATE_OVERRIDE="$state" bash -eu -c '. "$1"; fm_lock_owner_dir "$2"' \ + _ "$LIB" "$state/.watcher-down.lock" >/dev/null 2>&1; then + fail "fixture could not induce watcher owner-directory creation failure" + fi + is_live_non_zombie "$pid" || fail "watcher exited before the resource-fault shutdown signal" + started=$SECONDS + kill -TERM "$pid" || fail "could not signal the watcher with unwritable state" + i=0 + while [ "$i" -lt 200 ]; do + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + [ "$process_state" -eq 1 ] && break + sleep 0.1 + i=$((i + 1)) + done + process_state=0 + is_live_non_zombie "$pid" || process_state=$? + elapsed=$((SECONDS - started)) + [ "$process_state" -ne 2 ] || fail "resource-fault watcher liveness was unreadable" + [ "$process_state" -eq 1 ] \ + || fail "signaled watcher did not stop after owner-directory creation failed" + rc=0 + wait "$pid" || rc=$? + [ "$rc" -ne 0 ] || fail "signaled watcher exited successfully with unwritable state" + assert_grep 'recovery state could not be persisted within 5s' "$err" \ + "resource-fault shutdown did not reach its deadline reporting branch" + assert_grep 'stopping and retaining stale lock evidence' "$err" \ + "resource-fault shutdown did not report retained lock evidence" + [ "$(cat "$state/.watch.lock/pid" 2>/dev/null || true)" = "$pid" ] \ + || fail "resource-fault shutdown removed the singleton lock evidence" + pid= + pass "signaled watcher reaches its deadline on owner-directory creation failure (${elapsed}s)" +) + test_arm_self_eviction_is_loud_without_successor() { local dir state fakebin armout armpid watcher_pid status i dir=$(make_case arm-self-evict) @@ -1235,6 +1346,8 @@ test_watch_restart_rejects_reused_pid test_watch_restart_attaches_to_healthy_peer test_watcher_self_evicts_on_lock_takeover test_shutdown_is_bounded_when_marker_lock_is_held +test_lock_resource_failure_returns || exit $? +test_shutdown_is_bounded_when_state_is_unwritable || exit $? test_arm_self_eviction_is_loud_without_successor test_arm_attaches_and_waits_for_live_fresh_watcher test_attached_arm_signal_is_recorded_in_cycle_ledger From 62a12dc8d7fd2ca85ebaf1d29c48c7eb52c9361b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 14:02:33 +0200 Subject: [PATCH 5/6] fix(review): Fix watcher startup regression caught during shutdown hardening review --- bin/fm-wake-lib.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index a8f5f305ac4..49875a5a29b 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -962,7 +962,6 @@ fm_lock_try_acquire() { rc=$? fi [ "$rc" -eq 1 ] || return 1 - [ -e "$lockdir" ] || [ -L "$lockdir" ] || return 1 fm_current_pid current || return 1 pid=$(cat "$lockdir/pid" 2>/dev/null || true) From 31e4d35bec947717151b92359a2f04a542c05dd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 9 Sep 2026 14:25:44 +0200 Subject: [PATCH 6/6] fix(document): Document bounded watcher shutdown and lock recovery --- bin/fm-wake-lib.sh | 36 +++++++++++++----------------------- docs/watcher-continuity.md | 8 ++++++-- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/bin/fm-wake-lib.sh b/bin/fm-wake-lib.sh index 49875a5a29b..b9ad6de4d5d 100755 --- a/bin/fm-wake-lib.sh +++ b/bin/fm-wake-lib.sh @@ -533,6 +533,7 @@ fm_lock_claim() { return 0 } +# Return 2 for owner creation/preparation failures; 1 permits contention recovery. fm_lock_try_create() { local lockdir=$1 allowed_steal_owner=${2:-} ownerdir FM_LOCK_OWNER_DIR= @@ -644,31 +645,17 @@ _fm_recovery_marker_write_locked() { fi } -# Seconds a recovery-marker transition may wait for the marker lock before it -# refuses instead of blocking. Empty - the default - keeps the ordinary -# unbounded wait every mutation-critical caller relies on, so no existing caller -# changes behavior. A caller whose whole job is to STOP sets it for its own -# call: bin/fm-watch.sh's EXIT trap, where blocking on this lock turns "the -# watcher stops" into "the watcher never stops" and makes a supervisor's signal -# a no-op. On the deadline the transition returns 124 with FM_LOCK_HELD_PID -# naming whoever still holds the lock. +# Internal timeout in seconds for the shutdown-reachable transitions below. +# Empty keeps ordinary recovery-marker operations on their blocking path; +# watcher_cleanup in bin/fm-watch.sh supplies its deadline and clears it after use. +# On timeout, return 124 with FM_LOCK_HELD_PID identifying a holder when known. FM_RECOVERY_MARKER_LOCK_TIMEOUT= -# The one place a recovery-marker transition takes the marker lock, so a bound -# cannot be added to one acquisition and silently missed by the other. -# -# The bound is a deadline around the ORDINARY acquire, deliberately NOT -# fm_lock_acquire_wait_bounded. That helper delegates acquisition to a child -# process, and from that child the caller's OWN abandoned hold is -# indistinguishable from a live foreign holder - so it cannot acquire a lock the -# caller itself already holds. The exit path is exactly that case: a signal can -# land inside a recovery-marker critical section and the EXIT trap then re-enters -# it, which is why fm_lock_try_acquire carries an in-process self-held reclaim. -# Measured: routing shutdown through the helper left the singleton lock behind -# where the plain wait released it. Keeping fm_lock_try_acquire keeps that -# reclaim, the stale-owner recovery, and every other acquisition rule identical; -# the only added outcome is 124 on the deadline, with FM_LOCK_HELD_PID naming -# whoever still holds it. +# Both cleanup transitions must use this acquisition path to share the deadline. +# Keep acquisition in the exiting process: fm_lock_acquire_wait_bounded delegates +# to a child, which cannot reclaim a hold abandoned by the caller's interrupted +# critical section. fm_lock_try_acquire preserves that self-held reclaim and +# stale-owner recovery while returning to the deadline check after failures. _fm_recovery_marker_lock_acquire() { # local started if [ -z "$FM_RECOVERY_MARKER_LOCK_TIMEOUT" ]; then @@ -961,6 +948,9 @@ fm_lock_try_acquire() { else rc=$? fi + # Resource failures must return without recursive stealing. Contention can + # leave lockdir absent after a .steal-blocked claim; keep that path eligible + # to recover an abandoned stealer. [ "$rc" -eq 1 ] || return 1 fm_current_pid current || return 1 diff --git a/docs/watcher-continuity.md b/docs/watcher-continuity.md index a5a4554f5d3..c28937e20f7 100644 --- a/docs/watcher-continuity.md +++ b/docs/watcher-continuity.md @@ -52,7 +52,10 @@ The turn-end guard remains the final backstop rather than the normal continuity A recovery episode is one generation of `state/.watcher-down`, and it is retired only by the generation-bound acknowledgement the drain prints as `WAKE_ACK_REQUIRED`. An unacknowledged downtime generation is announced at most once: the first recovery marks that generation announced, and later arms wait until a new down stretch mints a new generation. A non-successor watcher start after an announced-but-unacked episode is a new down stretch and mints a fresh generation so buried decisions still resurface once. -Every watcher close and every durable queue append publishes downtime, so a downtime republication of any pending episode reuses its generation instead of minting a new one, and an already-announced generation stays announced. +When it owns the singleton lock, watcher cleanup preserves an existing recovery episode or publishes downtime before releasing that lock; every durable queue append publishes downtime. +Cleanup waits for the recovery-marker lock only until the internal deadline owned by `WATCHER_SHUTDOWN_LOCK_SECS` in [`bin/fm-watch.sh`](../bin/fm-watch.sh); ordinary recovery-marker operations retain blocking semantics. +On timeout or another persistence failure, the watcher reports the failure and stops while retaining its singleton lock as stale recovery evidence for the next arm; the timeout diagnostic also names the marker-lock holder PID when known. +A downtime republication of any pending episode reuses its generation instead of minting a new one, and an already-announced generation stays announced. That reuse keeps a watcher close inside the handling window from orphaning the acknowledgement already presented and trapping later arms in repeated recovery presentation. An acknowledgement carries two separable facts: queue-row consumption is bound to the monotonic `--ack-through` sequence (further scoped per actor - see "Per-actor acknowledgement" below), while only retiring the episode is bound to `--recovery-generation`. A generation mismatch therefore does not block consumption of rows through that sequence; it is a non-fatal result that names its own remedy - re-drain, then acknowledge the newer episode. @@ -116,7 +119,8 @@ Only the watcher process touches `state/.last-watcher-beat`; no helper process c The same suite covers ordinary same-process session replacement for `/new`, `/resume`, `/fork`, and reload, same-instance shutdown-plus-start, automatic re-arm before any model turn, a fresh extension-module rebind carrying all in-flight actionable closes exactly once, stale prior-generation callbacks, repeated transitions with exactly one live cycle, disappearance of the shutting-down refusal after a valid replacement activates, and terminal quit still refusing late rearm. `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. +`tests/fm-watcher-lock.test.sh` covers verified-successor attach, recovery publication before stale-lock removal, bounded shutdown with a held marker lock or unwritable state, acquisition returning on owner-directory creation failure, 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. +The unwritable-state cases explicitly skip as root because root bypasses the directory permissions needed to induce the resource fault. `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, exit-2 translation, and host-timeout HUP/TERM/INT translation into the same durable failure handoff. It also covers generation-claim single-flight, stuck-claim supersession, superseded-owner silence, notice-marker refusal and retry, ownership-atomic episode reset, and the legacy upgrade shim; [`turnend-guard.md`](turnend-guard.md) owns those behavior contracts.