fix: correct worker shutdown waits and test portability

bin/fm-remote-job-lib.sh

@@ -956,38 +956,45 @@ fm_remote_job_worker_process_group() { # <pid>
   printf '%s\n' "$pgid"
 }
 
+# The single liveness question every stop step asks: is the authoritative
+# target still alive? That target is the isolated worker group when one is
+# provable, and the lone process otherwise - never both. The serving child
+# recorded in worker.pid dies before its supervisor, so a condition that also
+# required that pid to be alive would call a group that is still shutting down
+# already gone.
+fm_remote_job_worker_tree_alive() { # <pgid> <pid>
+  local pgid=$1 pid=$2
+  if [ -n "$pgid" ]; then
+    kill -0 -- "-$pgid" 2>/dev/null
+  else
+    kill -0 "$pid" 2>/dev/null
+  fi
+}
+
+# Wait up to five seconds for the authoritative target to disappear; 0 when it
+# did. The bound is what turns "still visible right now" into a confirmation.
+fm_remote_job_wait_worker_tree_gone() { # <pgid> <pid>
+  local pgid=$1 pid=$2 i=0
+  while fm_remote_job_worker_tree_alive "$pgid" "$pid" && [ "$i" -lt 50 ]; do
+    i=$((i + 1))
+    sleep 0.1
+  done
+  ! fm_remote_job_worker_tree_alive "$pgid" "$pid"
+}
+
 # Stop a worker and every descendant it leaked, TERM first and KILL only for a
 # survivor. Signals the isolated worker group when one is provable and the lone
 # process otherwise. Returns non-zero when any verified worker-group member is
 # still alive afterwards.
 fm_remote_job_stop_worker_tree() { # <pid>
-  local pid=$1 pgid i=0
+  local pid=$1 pgid
   case "$pid" in ''|*[!0-9]*) return 1 ;; esac
   [ "$pid" -gt 1 ] || return 1
   pgid=$(fm_remote_job_worker_process_group "$pid" 2>/dev/null || true)
   if [ -n "$pgid" ]; then kill -TERM -- "-$pgid" 2>/dev/null || true; else kill -TERM "$pid" 2>/dev/null || true; fi
-  while { [ -n "$pgid" ] && kill -0 -- "-$pgid" 2>/dev/null || [ -z "$pgid" ] && kill -0 "$pid" 2>/dev/null; } \
-    && [ "$i" -lt 50 ]; do
-    i=$((i + 1))
-    sleep 0.1
-  done
-  if [ -n "$pgid" ]; then
-    kill -0 -- "-$pgid" 2>/dev/null || return 0
-  else
-    kill -0 "$pid" 2>/dev/null || return 0
-  fi
+  fm_remote_job_wait_worker_tree_gone "$pgid" "$pid" && return 0
   if [ -n "$pgid" ]; then kill -KILL -- "-$pgid" 2>/dev/null || true; else kill -KILL "$pid" 2>/dev/null || true; fi
-  i=0
-  while { [ -n "$pgid" ] && kill -0 -- "-$pgid" 2>/dev/null || [ -z "$pgid" ] && kill -0 "$pid" 2>/dev/null; } \
-    && [ "$i" -lt 50 ]; do
-    i=$((i + 1))
-    sleep 0.1
-  done
-  if [ -n "$pgid" ]; then
-    ! kill -0 -- "-$pgid" 2>/dev/null
-  else
-    ! kill -0 "$pid" 2>/dev/null
-  fi
+  fm_remote_job_wait_worker_tree_gone "$pgid" "$pid"
 }
 
 fm_remote_job_read_single_line() {

bin/fm-test-run.sh

@@ -272,7 +272,7 @@ family_for_basename() {
     fm-supervision-instructions.test.sh|fm-task-delivery.test.sh|\
     fm-tmux-submit-busy.test.sh|fm-trace-context-lib.test.sh|\
     fm-transition-lib.test.sh|\
-    fm-test-run.test.sh|fm-test-isolation-proof.test.sh)
+    fm-test-run.test.sh|fm-ci-herdr-timeout.test.sh|fm-test-isolation-proof.test.sh)
       printf '%s\n' pure-contract-unit
       ;;
     fm-daemon.test.sh|fm-guard-stale-banner.test.sh|fm-pi-watch-extension.test.sh|\

tests/fm-ci-herdr-timeout.test.sh

@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+# The required Herdr lane's hang tripwire is the family-run step bound, not
+# the job cap. Parse YAML so nested with.name artifact keys cannot masquerade
+# as the step contract.
+set -u
+
+if ! command -v ruby >/dev/null 2>&1; then
+  printf 'skip: ruby absent; YAML assertion not run\n'
+  exit 0
+fi
+
+# shellcheck source=tests/lib.sh
+. "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
+
+ruby -ryaml -e '
+doc = YAML.load_file(ARGV[0])
+job = doc.fetch("jobs").fetch("tests-herdr")
+step = job.fetch("steps").find { |s|
+  s.is_a?(Hash) && s["name"] == "Run real-Herdr family (serial, required)"
+}
+raise "missing family-run step" if step.nil?
+job_timeout = job.fetch("timeout-minutes")
+step_timeout = step.fetch("timeout-minutes")
+raise "tests-herdr job backstop must stay 75 minutes" unless job_timeout == 75
+raise "family-run step timeout must be 20 minutes" unless step_timeout == 20
+raise "family-run step timeout must be below the job backstop" unless step_timeout < job_timeout
+' "$ROOT/.github/workflows/ci.yml" || fail "invalid tests-herdr timeout contract"
+pass "Herdr CI family-run step times out at 20 min under a 75 min job backstop"

tests/fm-public-followup.test.sh

@@ -17,6 +17,8 @@ set -u
 . "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
 # shellcheck source=bin/fm-timeout-lib.sh
 . "$ROOT/bin/fm-timeout-lib.sh"
+# shellcheck source=bin/fm-remote-job-lib.sh
+. "$ROOT/bin/fm-remote-job-lib.sh"
 
 PF="$ROOT/bin/fm-public-followup.sh"
 EMIT="$ROOT/bin/fm-public-followup-emit.sh"
@@ -42,20 +44,35 @@ EOF
 }
 
 # The remote-route cases drive the real remote job worker, which outlives the
-# command that staged its job. Stop it before the shared fixture cleanup runs,
-# and keep that cleanup (tests/lib.sh owns it) rather than replacing the trap.
+# command that staged its job. Stop the whole bounded worker process group before
+# the shared fixture cleanup runs, and keep that cleanup (tests/lib.sh owns it)
+# rather than replacing the trap.
+pf_test_stop_remote_worker() {
+  local pid_file="${REMOTE_FIXTURE_JOBS:-$TMP_ROOT/remote-jobs}/worker.pid" pid command
+  [ -f "$pid_file" ] || return 0
+  pid=$(cat "$pid_file" 2>/dev/null) || return 0
+  case "$pid" in ''|*[!0-9]*) return 0 ;; esac
+  command=$(ps -p "$pid" -o command= 2>/dev/null || true)
+  case "$command" in
+    *fm-remote-job-worker.sh*) fm_remote_job_stop_worker_tree "$pid" || return 1 ;;
+    *) return 0 ;;
+  esac
+}
+
 pf_test_cleanup() {
-  local pid_file="${REMOTE_FIXTURE_JOBS:-$TMP_ROOT/remote-jobs}/worker.pid" pid
+  local exit_status=$? worker_rc=0
   if [ -n "$PF_TEST_LOCK_HOLDER" ]; then
     kill "$PF_TEST_LOCK_HOLDER" 2>/dev/null || true
     wait "$PF_TEST_LOCK_HOLDER" 2>/dev/null || true
     PF_TEST_LOCK_HOLDER=
   fi
-  if [ -f "$pid_file" ]; then
-    pid=$(cat "$pid_file" 2>/dev/null) || pid=
-    [ -z "$pid" ] || kill "$pid" 2>/dev/null || true
+  pf_test_stop_remote_worker || worker_rc=1
+  fm_test_cleanup || true
+  if [ "$worker_rc" -ne 0 ]; then
+    printf 'not ok - the remote job worker survived suite cleanup\n' >&2
+    [ "$exit_status" -ne 0 ] || exit_status=1
+    exit "$exit_status"
   fi
-  fm_test_cleanup
 }
 trap pf_test_cleanup EXIT
 trap 'pf_test_cleanup; exit 130' INT
@@ -3088,6 +3105,63 @@ test_local_work_home_emit_path_is_unchanged() {
   pass "a local work home's emit path is unchanged"
 }
 
+# Run one remote collection in a child invocation so the parent can assert the
+# executable suite's real exit status and verify that its trap reaps the worker
+# before removing the fixture root.
+test_remote_worker_cleanup_fixture() {
+  local home remote pid_file pid pgid
+  remote_fixture_prepare
+  home=$(make_home cleanup-child)
+  remote=$(make_remote_route "$home" cleanup-mate)
+  seed_repro_commitment "$home" pf-cleanup-child req-cleanup-child \
+    secondmate:cleanup-mate work-cleanup-child
+  run_pf_remote "$home" consume >/dev/null \
+    || fail "the cleanup fixture could not start its remote worker"
+  pid_file="$REMOTE_FIXTURE_JOBS/worker.pid"
+  [ -s "$pid_file" ] || fail "the cleanup fixture did not publish its worker PID"
+  pid=$(cat "$pid_file")
+  # worker.pid records the serving child, which a leaking teardown kills and the
+  # supervisor above it simply replaces. The group leader is that supervisor, so
+  # it is the target the parent has to watch.
+  pgid=$(fm_remote_job_process_pgid "$pid") \
+    || fail "the cleanup fixture could not resolve its worker process group"
+  [ "$pgid" != "$pid" ] \
+    || fail "the cleanup fixture's worker has no supervisor above its serving child"
+  [ -n "${PF_TEST_ROOT_MARKER:-}" ] || fail "the cleanup fixture has no root marker"
+  [ -n "${PF_TEST_SUPERVISOR_MARKER:-}" ] || fail "the cleanup fixture has no supervisor marker"
+  printf '%s\n' "$TMP_ROOT" > "$PF_TEST_ROOT_MARKER"
+  printf '%s\n' "$pgid" > "$PF_TEST_SUPERVISOR_MARKER"
+  pass "remote worker cleanup fixture completed"
+}
+
+test_suite_exit_status_and_remote_cleanup() {
+  local tmp out rc root supervisor survived=0
+  tmp=$(mktemp -d "${TMPDIR:-/tmp}/fm-public-followup-cleanup.XXXXXX")
+  set +e
+  PF_TEST_ROOT_MARKER="$tmp/root" PF_TEST_SUPERVISOR_MARKER="$tmp/supervisor" \
+    FM_TEST_ONLY=test_remote_worker_cleanup_fixture \
+    "$BASH" "${BASH_SOURCE[0]}" >"$tmp/out" 2>"$tmp/err"
+  rc=$?
+  set -e
+  out=$(cat "$tmp/out" "$tmp/err")
+  root=$(cat "$tmp/root" 2>/dev/null || true)
+  supervisor=$(cat "$tmp/supervisor" 2>/dev/null || true)
+  case "$supervisor" in ''|*[!0-9]*) supervisor= ;; esac
+  if [ -n "$supervisor" ] && kill -0 -- "-$supervisor" 2>/dev/null; then
+    survived=1
+    kill -KILL -- "-$supervisor" 2>/dev/null || true
+  fi
+  [ "$rc" -eq 0 ] || { rm -rf "$tmp"; fail "a green executable suite must exit 0: $out"; }
+  [ -n "$root" ] || { rm -rf "$tmp"; fail "the cleanup fixture did not record its temporary root: $out"; }
+  [ -n "$supervisor" ] \
+    || { rm -rf "$tmp"; fail "the cleanup fixture did not record its worker process group: $out"; }
+  [ ! -e "$root" ] || { rm -rf "$tmp"; fail "the green executable suite leaked fixture files: $root"; }
+  [ "$survived" -eq 0 ] \
+    || { rm -rf "$tmp"; fail "the green executable suite leaked members of its remote worker group"; }
+  rm -rf "$tmp"
+  pass "a green executable suite returns 0 and leaves no remote worker tree or fixture files"
+}
+
 # CI's stock macOS Bash lane sets FM_TEST_ONLY to run just the bash-3.2 empty-lock
 # register regression. The rest of this file is not a 3.2 snapshot suite.
 if [ -n "${FM_TEST_ONLY:-}" ]; then
@@ -3167,4 +3241,5 @@ test_empty_remote_collection_is_healthy
 test_remote_brief_rejects_traversal_route_paths
 test_local_work_home_emit_path_is_unchanged
 test_remote_collection_is_idempotent
+test_suite_exit_status_and_remote_cleanup
 test_stage_in_refuses_ambiguous_or_unusable_homes

tests/fm-remote-job-orphan-reap.test.sh

@@ -90,7 +90,7 @@ start_worker() {
     export FM_REMOTE_JOB_STATE_ROOT="$state_root"
     export FM_REMOTE_JOB_PLATFORM_OVERRIDE=Linux
     export FM_REMOTE_JOB_ORPHAN_GRACE_SECONDS=1
-    # shellcheck source=bin/fm-remote-job-lib.sh
+    # shellcheck source=/dev/null
     . "$ROOT/bin/fm-remote-job-lib.sh"
     fm_remote_job_start_linux_worker "$root" "$account_home" >&2 || exit 1
     deadline=$(( $(date +%s) + 10 ))
@@ -203,3 +203,97 @@ pass "the reaper stops an abandoned worker's whole tree"
 out=$("$REAPER" 2>&1) || fail "a repeat reaper run failed: $out"
 assert_not_contains "$out" "$STALE" "the reaper reported an already-stopped worker"
 pass "the reaper is idempotent"
+
+# --- the shared tree stop the reaper and every teardown route through --------
+#
+# fm_remote_job_stop_worker_tree decides both "stop it" and "it is still
+# alive", so a stop that mistakes a shutting-down tree for a stopped one KILLs
+# it mid-shutdown, and one that mistakes a stopped tree for a survivor reports
+# a leak that is not there.
+
+stop_worker_tree() { # <pid>
+  (
+    # shellcheck source=/dev/null
+    . "$ROOT/bin/fm-remote-job-lib.sh"
+    fm_remote_job_stop_worker_tree "$1"
+  )
+}
+
+# The real supervisor's shutdown shape: worker.pid records the serving child,
+# which exits first, while the group leader still has its own shutdown to
+# finish.
+CASE3="$TMP_ROOT/case3"
+mkdir -p "$CASE3/bin"
+CASE3_MARKER="$CASE3/leader-shutdown"
+cat > "$CASE3/bin/fm-remote-job-worker.sh" <<'SH'
+#!/bin/bash
+set -u
+if [ "${1:-}" = --serve ]; then
+  trap 'exit 0' TERM
+  while :; do sleep 0.2; done
+fi
+leader_shutdown() {
+  kill -TERM "${CHILD:-}" 2>/dev/null || true
+  wait "${CHILD:-}" 2>/dev/null || true
+  sleep 1
+  printf 'graceful\n' > "$LEADER_MARKER"
+  exit 0
+}
+trap leader_shutdown TERM
+"$0" --serve &
+CHILD=$!
+while :; do sleep 0.2; done
+SH
+chmod +x "$CASE3/bin/fm-remote-job-worker.sh"
+
+set -m
+LEADER_MARKER="$CASE3_MARKER" "$CASE3/bin/fm-remote-job-worker.sh" >/dev/null 2>&1 &
+GRACEFUL=$!
+set +m
+track "$GRACEFUL"
+wait_child "$GRACEFUL" 10 || fail "the shutdown fixture never started its serving child"
+GRACEFUL_SERVE=$(pgrep -P "$GRACEFUL" | head -n 1)
+[ "$(pgid_of "$GRACEFUL_SERVE")" = "$GRACEFUL" ] ||
+  fail "the shutdown fixture's serving child is outside its process group"
+
+GRACEFUL_RC=0
+stop_worker_tree "$GRACEFUL_SERVE" || GRACEFUL_RC=$?
+[ "$GRACEFUL_RC" -eq 0 ] ||
+  fail "stopping a tree whose serving child exits first reported a survivor"
+[ "$(cat "$CASE3_MARKER" 2>/dev/null || true)" = graceful ] ||
+  fail "the tree stop escalated to KILL while the leader was still shutting down"
+wait_gone "$GRACEFUL_SERVE" 10 || fail "the serving child survived the tree stop"
+wait_gone "$GRACEFUL" 10 || fail "the group leader survived the tree stop"
+pass "a tree whose serving child exits before its leader is stopped gracefully and reported stopped"
+
+# A tree that ignores TERM is a real survivor of the graceful phase: it must be
+# escalated to KILL and only then reported stopped.
+CASE4="$TMP_ROOT/case4"
+mkdir -p "$CASE4/bin"
+cat > "$CASE4/bin/fm-remote-job-worker.sh" <<'SH'
+#!/bin/bash
+set -u
+trap '' TERM
+if [ "${1:-}" = --serve ]; then
+  while :; do sleep 0.2; done
+fi
+"$0" --serve &
+while :; do sleep 0.2; done
+SH
+chmod +x "$CASE4/bin/fm-remote-job-worker.sh"
+
+set -m
+"$CASE4/bin/fm-remote-job-worker.sh" >/dev/null 2>&1 &
+STUBBORN=$!
+set +m
+track "$STUBBORN"
+wait_child "$STUBBORN" 10 || fail "the TERM-ignoring fixture never started its serving child"
+STUBBORN_SERVE=$(pgrep -P "$STUBBORN" | head -n 1)
+
+STUBBORN_RC=0
+stop_worker_tree "$STUBBORN_SERVE" || STUBBORN_RC=$?
+[ "$STUBBORN_RC" -eq 0 ] ||
+  fail "a TERM-ignoring worker tree was not reported stopped after the KILL escalation"
+wait_gone "$STUBBORN_SERVE" 10 || fail "a TERM-ignoring serving child survived the tree stop"
+wait_gone "$STUBBORN" 10 || fail "a TERM-ignoring group leader survived the tree stop"
+pass "a worker tree that ignores TERM is escalated to KILL and reported stopped"