test(watcher): de-flake and leak-proof the watcher-lock suite

CONTRIBUTING.md

@@ -102,6 +102,8 @@ Use `bin/fm-test-run.sh --list-lanes` for exact lane names and `--help` for `--j
 Discover tests by listing `tests/*.test.sh`: each is a self-contained bash script named `<subject>.test.sh`, and its header comment describes what it covers, so pass one to `bin/fm-test-run.sh` to focus on a subject with canonical timing output.
 Tests that need a real optional backend or an explicit opt-in (real herdr/zellij/cmux smoke tests, the live Pi regression) skip themselves and print the tool or environment gate needed to enable them, so the portable suite remains safe on machines without those tools.
 The [Herdr backend guide](docs/herdr-backend.md#destructive-lab-safety) owns the lane's isolation boundary, while [runtime backend verification](docs/verification/runtime-backends.md#herdr) owns active empirical evidence; live harness credential tests remain opt-in.
+A test that starts a background process must register it with `fm_test_track_pid` so it is reaped on every exit path, including the abort that `fail` performs mid-case.
+A leaked child keeps the suite's inherited output pipe open, which hangs whatever is reading that pipe to EOF, and it keeps holding its temp home's watcher lock; `tests/lib.sh` owns the registry and reaping rules, including why nothing is ever reaped by process-name pattern.
 
 ## Questions
 

bin/fm-watch-arm.sh

@@ -501,22 +501,24 @@ owned_child_finished() {
   fi
 
   if [ "$rc" -eq 0 ]; then
+    # Surface the child's own words the moment it closes, BEFORE spending the
+    # bounded successor-confirmation window on it. The child explains itself
+    # here ("already running pid N" when it stood down behind a peer), and
+    # whoever is reading this arm's output should not have to wait out that poll
+    # to learn why. The relative order of what this arm prints is unchanged: the
+    # child's output has always preceded the attached/FAILED line that follows.
+    print_watch_output "$child_out"
+    rm -f "$child_out" 2>/dev/null || true
+    child=
+    child_out=
     if wait_for_healthy_successor; then
       cycle_log_append "$rc" "$signal" unexpected-clean-exit "attached:$HEALTHY_PID"
-      print_watch_output "$child_out"
-      rm -f "$child_out" 2>/dev/null || true
-      child=
-      child_out=
       cycle_mark_predecessor_successor "attached:$HEALTHY_PID"
       report_attached
       cycle_begin "$HEALTHY_PID" attached "$HEALTHY_IDENTITY"
       attach_and_wait "$HEALTHY_PID"
       return $?
     fi
-    print_watch_output "$child_out"
-    rm -f "$child_out" 2>/dev/null || true
-    child=
-    child_out=
     if close_unobserved_cycle; then
       cycle_log_append "$rc" "$signal" clean-exit-delivered-wake none
       return 0

tests/fm-afk-pi-herdr-return-e2e.test.sh

@@ -57,6 +57,7 @@ cleanup() {
     rc=1
   fi
   rm -rf "$TMP_ROOT"
+  fm_test_cleanup
   exit "$rc"
 }
 trap cleanup EXIT

tests/fm-send-secondmate-marker-herdr-e2e.test.sh

@@ -51,6 +51,7 @@ cleanup() {
     rc=1
   fi
   rm -rf "$TMP_ROOT"
+  fm_test_cleanup
   exit "$rc"
 }
 trap cleanup EXIT

tests/fm-tmux-submit-busy.test.sh

@@ -10,7 +10,7 @@ set -u
 . "$ROOT/bin/fm-tmux-lib.sh"
 
 TMP_ROOT=$(mktemp -d "${TMPDIR:-/tmp}/fm-tmux-submit-busy.XXXXXX")
-trap 'rm -rf "$TMP_ROOT"' EXIT
+trap 'rm -rf "$TMP_ROOT"; fm_test_cleanup' EXIT
 
 # Override fm_pane_is_busy for testing: FM_FAKE_PANE_BUSY=1 means busy.
 fm_pane_is_busy() {

tests/lib.sh

@@ -50,12 +50,14 @@ pass() {
   printf 'ok - %s\n' "$1"
 }
 
-# --- self-cleaning temp root ------------------------------------------------
+# --- self-cleaning temp root and background-process reaping ------------------
 #
 # fm_test_tmproot <prefix> echoes a fresh temp dir and registers it for removal
-# on EXIT/INT/TERM. A test file that needs extra teardown (e.g. killing a
-# daemon) should define its own EXIT trap and call fm_test_cleanup from inside
-# it so registered dirs are still removed.
+# on EXIT/INT/TERM. fm_test_track_pid <pid> registers a background process for
+# reaping on the same EXIT, on the success path AND on every fail() path. A test
+# file that needs extra teardown (e.g. killing a daemon) should define its own
+# EXIT trap and call fm_test_cleanup from inside it so both registries are still
+# drained.
 #
 # The call site is almost always `TMP_ROOT=$(fm_test_tmproot prefix)`, which
 # forks a subshell to capture stdout. Anything that function does to the
@@ -66,10 +68,25 @@ pass() {
 # subshell's - see `man bash` on `$$`), so fm_test_tmproot records the
 # directory in a `$$`-keyed registry file instead, and the trap that reaps
 # that file is armed once, here, at source time - which always runs in the
-# real caller, never a subshell.
+# real caller, never a subshell. The pid registry is the same shape for the
+# same reason: fm_test_track_pid is often called from a subshell too.
+#
+# Both registries are mktemp'd here in the sourcing shell rather than opened at
+# a predictable $$-derived name in a shared /tmp, where another user could
+# pre-create or symlink one and steer the rm -rf in fm_test_cleanup. Every read
+# is existence-guarded so an absent file reads as empty.
+#
+# Reaping is BY REGISTERED PID ONLY, and only while that pid is still a child of
+# this shell. Never reap by process-name pattern: a pattern like the watcher's
+# script path matches every firstmate home on the machine, including a sibling
+# home's real watcher.
 
 FM_TEST_CLEANUP_DIRS=()
 FM_TEST_CLEANUP_REGISTRY=$(mktemp "${TMPDIR:-/tmp}/.fm-test-cleanup.$$.XXXXXX") || return 1
+FM_TEST_PID_LOG=$(mktemp "${TMPDIR:-/tmp}/.fm-test-pids.$$.XXXXXX") || {
+  rm -f "$FM_TEST_CLEANUP_REGISTRY"
+  return 1
+}
 
 fm_test_pid_identity() {
   local pid=$1
@@ -78,12 +95,66 @@ fm_test_pid_identity() {
 }
 
 FM_TEST_OWNER_IDENTITY=$(fm_test_pid_identity "$$") || {
-  rm -f "$FM_TEST_CLEANUP_REGISTRY"
+  rm -f "$FM_TEST_CLEANUP_REGISTRY" "$FM_TEST_PID_LOG"
   return 1
 }
 
+# fm_test_pid_is_own_child <pid>: true while <pid> is still a direct child of
+# this shell. This guards every signal the reaper sends: once a pid has been
+# waited on the kernel is free to hand that number to an unrelated process, and
+# a test harness must never signal a process it does not own.
+fm_test_pid_is_own_child() {
+  local pid=$1 parent
+  case "$pid" in
+    ''|*[!0-9]*) return 1 ;;
+  esac
+  parent=$(ps -p "$pid" -o ppid= 2>/dev/null | tr -d '[:space:]')
+  [ -n "$parent" ] && [ "$parent" = "$$" ]
+}
+
+fm_test_track_pid() {
+  local pid=$1
+  case "$pid" in
+    ''|*[!0-9]*) return 0 ;;
+  esac
+  printf '%s\n' "$pid" >> "$FM_TEST_PID_LOG"
+}
+
+# fm_test_live_tracked_pids: print every registered pid still running as a child
+# of this shell. Used both by the reaper and by suites that assert they left
+# nothing behind.
+fm_test_live_tracked_pids() {
+  local pid
+  [ -n "${FM_TEST_PID_LOG:-}" ] && [ -s "$FM_TEST_PID_LOG" ] || return 0
+  while read -r pid; do
+    fm_test_pid_is_own_child "$pid" || continue
+    printf '%s\n' "$pid"
+  done < "$FM_TEST_PID_LOG"
+}
+
+fm_test_reap_tracked_pids() {
+  local pid i
+  for pid in $(fm_test_live_tracked_pids); do
+    kill -TERM "$pid" 2>/dev/null || true
+  done
+  # Give a TERM-handling child its own teardown (fm-watch-arm.sh tears down the
+  # watcher it forked) a bounded moment before escalating.
+  i=0
+  while [ "$i" -lt 20 ] && [ -n "$(fm_test_live_tracked_pids)" ]; do
+    sleep 0.1
+    i=$((i + 1))
+  done
+  for pid in $(fm_test_live_tracked_pids); do
+    kill -KILL "$pid" 2>/dev/null || true
+    wait "$pid" 2>/dev/null || true
+  done
+  [ -f "$FM_TEST_PID_LOG" ] && : > "$FM_TEST_PID_LOG"
+  return 0
+}
+
 fm_test_cleanup() {
   local d
+  fm_test_reap_tracked_pids
   for d in "${FM_TEST_CLEANUP_DIRS[@]:-}"; do
     [ -n "$d" ] && rm -rf "$d"
   done
@@ -93,6 +164,8 @@ fm_test_cleanup() {
     done < "$FM_TEST_CLEANUP_REGISTRY"
     rm -f "$FM_TEST_CLEANUP_REGISTRY"
   fi
+  [ -n "${FM_TEST_PID_LOG:-}" ] && rm -f "$FM_TEST_PID_LOG"
+  return 0
 }
 
 fm_test_tmproot() {

tests/wake-helpers.sh

@@ -274,6 +274,99 @@ SH
   printf '%s\n' "$dir"
 }
 
+# fm_wake_start_peer <ready-file> [--ignore-term]: start a long-lived stand-in
+# for a peer watcher process and hand its pid back in FM_WAKE_PEER_PID only once
+# the process is genuinely READY. Readiness matters twice, and both used to be
+# raced:
+#   - the stand-in has exec'd its final program, so bin/fm-wake-lib.sh's
+#     fm_pid_identity records the identity the process will keep, not the
+#     pre-exec fork it happened to catch;
+#   - with --ignore-term the SIGTERM ignore disposition is already installed, so
+#     a restart arm's TERM cannot kill the fixture in its startup window and turn
+#     a "peer survives, arm attaches" case into a confusing "did not attach".
+# It is a plain function, not a command substitution, so $! stays a child of the
+# caller's shell and the caller can still wait on it.
+FM_WAKE_PEER_PID=
+fm_wake_start_peer() {
+  local ready=$1 policy=${2:-} i
+  rm -f "$ready"
+  bash -c '
+    [ "$2" = --ignore-term ] && trap "" TERM
+    : > "$1"
+    # Short sleeps, so a fixture that has to be force-killed cannot orphan a
+    # long-lived grandchild.
+    while :; do sleep 1; done
+  ' _ "$ready" "$policy" &
+  FM_WAKE_PEER_PID=$!
+  fm_test_track_pid "$FM_WAKE_PEER_PID"
+  i=0
+  while [ "$i" -lt 100 ] && [ ! -e "$ready" ]; do
+    sleep 0.1
+    i=$((i + 1))
+  done
+  [ -e "$ready" ] || fail "peer fixture never signalled ready"
+  [ "$policy" = --ignore-term ] || return 0
+  # Fixture self-check: prove the ignore disposition really took, so a broken
+  # peer fails here by name instead of downstream as a flaky assertion.
+  kill -TERM "$FM_WAKE_PEER_PID" 2>/dev/null || true
+  i=0
+  while [ "$i" -lt 5 ]; do
+    is_live_non_zombie "$FM_WAKE_PEER_PID" || fail "term-resistant peer fixture died on SIGTERM"
+    sleep 0.1
+    i=$((i + 1))
+  done
+}
+
+# fm_wake_temp_watcher_pids <root>: print the pid of every watcher still recorded
+# as the live owner of a temp home's singleton lock under <root>. Scoped by path
+# and identity so it can never name a sibling firstmate home's real watcher: only
+# locks reached through the suite's own mktemp glob "$root"/*/state/.watch.lock are
+# considered - no real home occupies that path - and the pid is confirmed through
+# bin/fm-wake-lib.sh's identity check, so a recycled pid is skipped. The fm-home
+# read from each lock is passed straight back to fm_watcher_lock_matches_pid, so
+# that home argument is no longer load-bearing; the identity comparison is.
+fm_wake_temp_watcher_pids() {
+  local root=$1 lock state home pid lib="$ROOT/bin/fm-wake-lib.sh"
+  for lock in "$root"/*/state/.watch.lock; do
+    [ -d "$lock" ] || continue
+    state=$(dirname "$lock")
+    pid=$(cat "$lock/pid" 2>/dev/null || true)
+    home=$(cat "$lock/fm-home" 2>/dev/null || true)
+    case "$pid" in
+      ''|*[!0-9]*) continue ;;
+    esac
+    kill -0 "$pid" 2>/dev/null || continue
+    FM_STATE_OVERRIDE="$state" bash -c '
+      # shellcheck disable=SC1090,SC1091
+      . "$1"
+      fm_watcher_lock_matches_pid "$2" "$3" "$4" "$5"
+    ' _ "$lib" "$state" "$ROOT/bin/fm-watch.sh" "$pid" "$home" || continue
+    printf '%s\n' "$pid"
+  done
+}
+
+# fm_wake_reap_temp_watchers <root>: end the watchers fm_wake_temp_watcher_pids
+# names. These are grandchildren (an arm forks its watcher), so the pid registry
+# in tests/lib.sh cannot reach them; the identity-verified temp-home lock can.
+fm_wake_reap_temp_watchers() {
+  local root=$1 pid i
+  for pid in $(fm_wake_temp_watcher_pids "$root"); do
+    kill -TERM "$pid" 2>/dev/null || true
+  done
+  i=0
+  while [ "$i" -lt 20 ] && [ -n "$(fm_wake_temp_watcher_pids "$root")" ]; do
+    sleep 0.1
+    i=$((i + 1))
+  done
+  # Re-derive the list rather than reusing the first one: a pid only stays
+  # eligible for the harder signal while the temp home's lock still identifies it
+  # as that home's watcher.
+  for pid in $(fm_wake_temp_watcher_pids "$root"); do
+    kill -KILL "$pid" 2>/dev/null || true
+  done
+  return 0
+}
+
 wait_for_exit() {
   local pid=$1 limit=${2:-50} i=0
   while [ "$i" -lt "$limit" ]; do