fix(bin): bound watcher shutdown and improve stop diagnostics

bin/fm-wake-lib.sh

@@ -533,17 +533,18 @@ 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=
-  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
@@ -644,6 +645,30 @@ _fm_recovery_marker_write_locked() {
   fi
 }
 
+# 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=
+
+# 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() {  # <lockdir>
+  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 +678,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 +894,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
@@ -920,7 +945,13 @@ fm_lock_try_acquire() {
 
   if fm_lock_try_create "$lockdir"; then
     return 0
+  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
   pid=$(cat "$lockdir/pid" 2>/dev/null || true)

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

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.

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"

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=
@@ -1114,7 +1109,8 @@ 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")
     state="$dir/home/state"
@@ -1165,26 +1161,68 @@ 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
+    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
+      # 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
+      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
       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"
-    alive=0
-    process_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"