diff --git a/bin/fm-remote-job-lib.sh b/bin/fm-remote-job-lib.sh index f6ac2ad9b99..d64bbe30f05 100755 --- a/bin/fm-remote-job-lib.sh +++ b/bin/fm-remote-job-lib.sh @@ -956,38 +956,45 @@ fm_remote_job_worker_process_group() { # 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() { # + 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() { # + 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() { # - 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() { diff --git a/bin/fm-test-run.sh b/bin/fm-test-run.sh index 762272b948f..20a1fb2493d 100755 --- a/bin/fm-test-run.sh +++ b/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|\ diff --git a/tests/fm-ci-herdr-timeout.test.sh b/tests/fm-ci-herdr-timeout.test.sh new file mode 100755 index 00000000000..b1718c2e423 --- /dev/null +++ b/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" diff --git a/tests/fm-public-followup.test.sh b/tests/fm-public-followup.test.sh index c009bdcbbe9..b7855821285 100755 --- a/tests/fm-public-followup.test.sh +++ b/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 diff --git a/tests/fm-remote-job-orphan-reap.test.sh b/tests/fm-remote-job-orphan-reap.test.sh index 0c52a4c9012..681740395b4 100755 --- a/tests/fm-remote-job-orphan-reap.test.sh +++ b/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() { # + ( + # 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" diff --git a/tests/fm-test-run.test.sh b/tests/fm-test-run.test.sh index b90dfa2129d..10572fa4ae1 100755 --- a/tests/fm-test-run.test.sh +++ b/tests/fm-test-run.test.sh @@ -99,6 +99,7 @@ init_changed_fixture_repo() { fm-documentation-audiences.test.sh \ fm-test-isolation-proof.test.sh \ fm-test-run.test.sh \ + fm-ci-herdr-timeout.test.sh \ fm-cd-pretool-check.test.sh \ fm-daemon.test.sh \ fm-harness-adapter-instructions-live-e2e.test.sh \ @@ -285,6 +286,22 @@ test_shell_line_ending_policy_selects_runner_contract() { pass "shell line-ending policy selects runner coverage" } +test_workflow_selects_yaml_contract() { + local tmp repo listed + tmp=$(mktemp -d "${TMPDIR:-/tmp}/fm-test-run-workflow.XXXXXX") + repo="$tmp/repo" + init_changed_fixture_repo "$repo" + mkdir -p "$repo/.github/workflows" + printf 'jobs: {}\n' >"$repo/.github/workflows/ci.yml" + listed=$(cd "$repo" && bin/fm-test-run.sh --list --changed --base HEAD) + assert_contains "$listed" "tests/fm-ci-herdr-timeout.test.sh" \ + "workflow changes select the YAML timeout contract" + assert_contains "$listed" "tests/fm-test-run.test.sh" \ + "workflow changes retain unrelated runner coverage" + rm -rf "$tmp" + pass "workflow changes select the focused YAML contract and runner coverage" +} + test_changed_dependency_selection_and_unmapped_failure() { local tmp repo listed rc tmp=$(mktemp -d "${TMPDIR:-/tmp}/fm-test-run-changed.XXXXXX") @@ -1504,40 +1521,6 @@ SH pass "jobs scheduler runs proven scripts; failure propagates; non-proven refused" } -test_herdr_ci_family_run_has_a_step_timeout() { - # The required Herdr lane's hang tripwire is the family-run *step* bound, not - # the 75-minute job cap. Parse the workflow as YAML so nested `with.name` - # artifact keys cannot masquerade as the step contract. - command -v ruby >/dev/null 2>&1 \ - || fail "ruby is required to parse .github/workflows/ci.yml as YAML" - local json job_timeout step_timeout - json=$(ruby -ryaml -rjson -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? -raise "family-run step has no timeout-minutes" unless step.key?("timeout-minutes") -puts JSON.generate( - "job_timeout" => job.fetch("timeout-minutes"), - "step_timeout" => step.fetch("timeout-minutes") -) -' "$ROOT/.github/workflows/ci.yml") \ - || fail "could not parse tests-herdr timeouts from ci.yml" - job_timeout=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["job_timeout"])' <<<"$json") \ - || fail "could not read job timeout from parsed workflow" - step_timeout=$(python3 -c 'import json,sys; print(json.load(sys.stdin)["step_timeout"])' <<<"$json") \ - || fail "could not read step timeout from parsed workflow" - [ "$job_timeout" = 75 ] \ - || fail "tests-herdr job backstop must stay 75 minutes, got $job_timeout" - [ "$step_timeout" = 20 ] \ - || fail "family-run step timeout must be 20 minutes, got $step_timeout" - [ "$step_timeout" -lt "$job_timeout" ] \ - || fail "family-run step timeout must be below the job backstop" - pass "Herdr CI family-run step times out at 20 min under a 75 min job backstop" -} - test_aggregate_json() { local tmp a b tmp=$(mktemp -d "${TMPDIR:-/tmp}/fm-test-run-aggjson.XXXXXX") @@ -1580,6 +1563,19 @@ assert len(doc["scripts"])==3 pass "aggregate-json merges lane timing artifacts" } +test_yaml_assertion_skips_when_ruby_is_absent() { + local out rc + set +e + out=$(hash -r; PATH='' "$BASH" "$ROOT/tests/fm-ci-herdr-timeout.test.sh" 2>&1) + rc=$? + set -e + [ "$rc" -eq 0 ] || fail "the executable test must skip cleanly without Ruby: $out" + [ "$out" = 'skip: ruby absent; YAML assertion not run' ] \ + || fail "the missing Ruby dependency was not declared: $out" + pass "the executable YAML test declares its Ruby dependency when Ruby is absent" +} + +test_yaml_assertion_skips_when_ruby_is_absent test_list_all_exact_suite_coverage test_family_selection test_single_script_selection @@ -1587,6 +1583,7 @@ test_changed_file_selection_is_conservative test_task_marker_refuses_the_primary_checkout test_changed_runner_surfaces_select_their_family test_shell_line_ending_policy_selects_runner_contract +test_workflow_selects_yaml_contract test_changed_dependency_selection_and_unmapped_failure test_changed_bin_reference_selects_per_script_not_per_family test_changed_uses_bounded_automatic_concurrency @@ -1613,5 +1610,4 @@ test_concurrent_runs_are_ordered_longest_first test_per_script_timeout_bounds_a_hang test_max_wall_ms_is_a_result_not_advice test_jobs_parallel_scheduler_and_failure_propagation -test_herdr_ci_family_run_has_a_step_timeout test_aggregate_json