From 0d73e0e309fd845f55d16fc4ba9107ed0cf86039 Mon Sep 17 00:00:00 2001 From: Christopher McKay Date: Sun, 23 Aug 2026 11:23:33 -0400 Subject: [PATCH] test(watcher): stop fixture confirmation budgets racing real child startup tests/fm-watcher-lock.test.sh passed in isolation but failed intermittently under full-suite and ambient concurrent load. bin/fm-watch-arm.sh computes its confirmation deadline immediately after forking the real child watcher, so the child's entire fork, exec, lock acquisition and beacon publication has to land inside that wall clock. Two cases shrank that budget to one second, leaving a two-second window for work measured at 3.1-4.9s under CPU oversubscription, so the arm honestly reported "FAILED - no live watcher with a fresh beacon" and their premises collapsed. A third case ran on the production budget, but its child must also execute a registered check before exiting: measured at 1.9-2.3s idle and 9.1-13.1s under load, against an 11s budget. The two cases that must confirm a real child now hold the arm to production's own budget instead of a shrunken fixture one, the immediate-wake case gets an explicit budget with headroom over its measured loaded cost, and the two waits for the arm's typed failure are sized off the largest production default rather than a fixed eight seconds. No bin/ change and no default behavior change: the lock's fail-closed semantics, SIGSTOP handling, stale-heartbeat detection and the arm's typed failures are untouched. Verified 4/4 green at 3x CPU oversubscription (loadavg 75-80) after 3/3 red before the change, and CONTRIBUTING.md records the convention. --- CONTRIBUTING.md | 2 ++ tests/fm-watcher-lock.test.sh | 39 ++++++++++++++++++++++++++++------- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 98cc88a5f68..aa53fa2efb5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -103,6 +103,8 @@ Family selection is the ordinary local path; `--all` is deliberate full regressi CI owns broad regression across required portable parallel shards, the portable serial lane's separate-runner shards, the Herdr lane, lint, invariants, the coverage guard, and stock macOS Bash compatibility in [`.github/workflows/ci.yml`](.github/workflows/ci.yml). Use `bin/fm-test-run.sh --list-lanes` for exact lane names and `--help` for `--jobs` rules and required gate-skip flags when reproducing a lane locally. Discover tests by listing `tests/*.test.sh`: each is a self-contained bash script named `.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. +A fixture may shorten a production timeout to keep a failure path prompt, but never below what the real work inside that window costs on a loaded machine: a fork, an exec, a lock acquisition, a beacon publication, or a first-poll check. +Where a case's assertion is not about the timeout itself, give that window headroom over the measured loaded cost, and bound the test's own waiting with iteration-counted poll loops, which stretch under load where a wall-clock budget does not. 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. diff --git a/tests/fm-watcher-lock.test.sh b/tests/fm-watcher-lock.test.sh index a3628b1694f..482e425a9f5 100755 --- a/tests/fm-watcher-lock.test.sh +++ b/tests/fm-watcher-lock.test.sh @@ -13,6 +13,13 @@ WATCH_ARM="$ROOT/bin/fm-watch-arm.sh" DRAIN="$ROOT/bin/fm-wake-drain.sh" LIB="$ROOT/bin/fm-wake-lib.sh" +# An arm only reports its typed failure after wait_for_healthy_successor has +# spent the whole confirmation budget, so cases that wait for that failure must +# outlast the largest production default (30s on MSYS, 10s elsewhere - see +# ARM_CONFIRM_DEFAULT in bin/fm-watch-arm.sh). This is a ceiling spent only when +# an arm genuinely fails to exit; a passing case returns as soon as it does. +ARM_FAIL_EXIT_POLLS=400 + TMP_ROOT=$(fm_test_tmproot fm-watcher-lock-tests) mark_pr_check_migration_complete() { @@ -536,7 +543,14 @@ test_arm_self_eviction_is_loud_without_successor() { fakebin="$dir/fakebin" armout="$dir/arm.out" mark_pr_check_migration_complete "$state" - PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_POLL=0.2 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 FM_ARM_CONFIRM_TIMEOUT=1 "$WATCH_ARM" > "$armout" & + # The arm's confirmation budget bounds a REAL child startup (fork, exec, lock + # acquisition, beacon publication), so this case holds the arm to production's + # own budget rather than a shrunken fixture one: a one-second budget turned + # ordinary CPU contention into an honest "FAILED - no live watcher with a fresh + # beacon" and broke this case's premise under full-suite load (issue #2844). + # It stays at the production default rather than something roomier because the + # same budget bounds the successor wait this case deliberately spends below. + PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_POLL=0.2 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 "$WATCH_ARM" > "$armout" & armpid=$! i=0 while [ "$i" -lt 80 ]; do @@ -551,7 +565,7 @@ test_arm_self_eviction_is_loud_without_successor() { # self-evict normally. With no verified successor, the arm must turn that # otherwise clean empty close into the typed nonzero failure. printf '%s\n' "$$" > "$state/.watch.lock/pid" - wait_for_exit "$armpid" 80 + wait_for_exit "$armpid" "$ARM_FAIL_EXIT_POLLS" status=$? [ "$status" -ne 0 ] && [ "$status" -ne 124 ] || fail "self-evicted arm did not fail nonzero (status $status)" grep -qF 'watcher: FAILED - cycle ended without an actionable reason' "$armout" || fail "self-evicted arm omitted the typed cycle-end failure" @@ -742,7 +756,13 @@ SH FM_STATE_OVERRIDE="$state" "$ROOT/bin/fm-check-register.sh" task >/dev/null \ || fail "could not register immediate-wake custom check" rc=0 - PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_GUARD_GRACE=0 FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=0 FM_HEARTBEAT=999999 "$WATCH_ARM" > "$armout" || rc=$? + # This case asserts wake propagation, not the confirmation deadline, and its + # child must also run the registered check before exiting: measured at 1.9-2.3s + # idle but 9.1-13.1s at 3x CPU oversubscription, against an 11s production + # budget. An explicit budget takes the deadline out of the assertion and costs + # nothing on a passing run, because the arm returns as soon as the child + # settles (issue #2844). + PATH="$fakebin:$PATH" FM_STATE_OVERRIDE="$state" FM_GUARD_GRACE=0 FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=0 FM_HEARTBEAT=999999 FM_ARM_CONFIRM_TIMEOUT=60 "$WATCH_ARM" > "$armout" || rc=$? [ "$rc" -eq 0 ] || fail "arm returned non-zero for an immediate wake (status $rc): $(cat "$armout")" grep -F "check: $check_file: merged: https://example.test/pr/7" "$armout" >/dev/null || fail "arm did not propagate the immediate check wake" ! grep -qF 'watcher: FAILED' "$armout" || fail "arm printed FAILED after a valid immediate wake" @@ -766,12 +786,15 @@ test_arm_waits_for_peer_beacon_after_child_stands_down() { printf '%s\n' "$dir" > "$state/.watch.lock/fm-home" printf '%s\n' "$WATCH" > "$state/.watch.lock/watcher-path" printf '%s\n' "$identity" > "$state/.watch.lock/pid-identity" - PATH="$fakebin:$PATH" FM_HOME="$dir" FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 FM_ARM_CONFIRM_TIMEOUT=1 FM_ARM_ATTACH_POLL=0.1 "$WATCH_ARM" > "$armout" & + # Same budget contract as the self-eviction case: the owned child's real + # startup and stand-down happen inside the arm's confirmation window, so the + # window stays production-sized (issue #2844). + PATH="$fakebin:$PATH" FM_HOME="$dir" FM_POLL=5 FM_SIGNAL_GRACE=1 FM_CHECK_INTERVAL=999999 FM_HEARTBEAT=999999 FM_ARM_ATTACH_POLL=0.1 "$WATCH_ARM" > "$armout" & armpid=$! # Synchronize on the owned child declining the live peer lock before making - # the peer healthy. Sleeping for the same one-second budget as the arm made - # this regression fixture race the confirmation deadline under full-suite - # load, rather than testing the intended successor-handshake boundary. + # the peer healthy. Sleeping for the same budget the arm spends made this + # regression fixture race the confirmation deadline under full-suite load, + # rather than testing the intended successor-handshake boundary. i=0 while [ "$i" -lt 80 ]; do grep -qF "watcher: already running pid $peer" "$state"/.watch-arm-output.* 2>/dev/null && break @@ -793,7 +816,7 @@ test_arm_waits_for_peer_beacon_after_child_stands_down() { # After the peer dies without a successor, the attached arm must fail loudly. kill "$peer" 2>/dev/null || true wait "$peer" 2>/dev/null || true - wait_for_exit "$armpid" 80 + wait_for_exit "$armpid" "$ARM_FAIL_EXIT_POLLS" status=$? [ "$status" -ne 0 ] && [ "$status" -ne 124 ] || fail "attached arm did not fail after peer died (status $status): $(cat "$armout")" grep -qF 'watcher: FAILED - cycle ended without an actionable reason' "$armout" || fail "peer-attached arm did not emit the typed cycle-end failure"