diff --git a/scripts/remote.sh b/scripts/remote.sh index ddc20f96e..0a5aa512c 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1924,6 +1924,36 @@ _remote_sync_engine_status() { fi } +# How many turns the readiness poll takes before giving up. +# +# THE SHIPPED VALUE IS 1600 AND THIS DOES NOT CHANGE IT. The number is lifted out +# of the loop so a test can ask for it without running it, and so the regression +# suites can reach the give-up path without paying for the full ceiling -- five +# of them were spending 12 minutes of a 25-minute CI shard on a wait whose length +# none of them are about. +# +# `AGMSG_TEST_SYNC_READY_TURNS` is a test seam. A value that is not a positive +# integer falls back to the shipped number rather than to zero: a resolver that +# quietly answered nothing would turn every one of those suites back into a full +# ceiling run, which is the failure this exists to remove. +_remote_sync_ready_turns() { + local want="${AGMSG_TEST_SYNC_READY_TURNS:-}" + # Unset, or not written in digits at all. + case "$want" in + ''|*[!0-9]*) printf '1600'; return ;; + esac + # AND ZERO IS NOT A CEILING. `0` is digits-only, so a guard that only rejects + # non-digits accepts it and the poll ends before it begins -- every suite that + # drives the give-up path would then reach its assertions without the engine + # having been waited on at all, and stay green while measuring nothing. That is + # the failure this fallback is described as preventing, and the description was + # true of `oops` and false of `0` until review caught it. `00` counts too. + case "$want" in + *[1-9]*) printf '%s' "$want" ;; + *) printf '1600' ;; + esac +} + _remote_sync_engine_reap_owned() { local team="$1" owned_pid="$2" state pid signal attempts for signal in TERM KILL; do @@ -2685,7 +2715,9 @@ cmd_sync_start() { # that is late or missing for ANY reason costs this caller its own wait and # not the rest of the machine. agmsg_lock_release - while [ "$i" -lt 1600 ]; do + local ready_turns + ready_turns="$(_remote_sync_ready_turns)" + while [ "$i" -lt "$ready_turns" ]; do IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team") if [ "$engine_state" = "running" ] && [ "$ready_pid" = "$started_pid" ] && tail -c "+$log_offset" "$logfile" 2>/dev/null | diff --git a/tests/test_remote_engine_start_refusal.bats b/tests/test_remote_engine_start_refusal.bats index f5e76731e..bb02b731c 100644 --- a/tests/test_remote_engine_start_refusal.bats +++ b/tests/test_remote_engine_start_refusal.bats @@ -113,7 +113,7 @@ skip_if_root() { printf '%s\n' 2147483647 > "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" chmod a-w "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" - run bash "$SCRIPTS/remote.sh" sync start testteam + run env AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam [ "$status" -ne 0 ] # Captured before the next `run`, which overwrites $output. @@ -133,7 +133,7 @@ skip_if_root() { # command fails on a team literally named "'testteam'" -- measured, that is # what the first version of this test did. A printed route has to be run the # way it is meant to be run. - run bash -c "bash '$SCRIPTS/remote.sh' $args" + run bash -c "AGMSG_TEST_SYNC_READY_TURNS=40 bash '$SCRIPTS/remote.sh' $args" # "not refused" is not enough: a remedy that no longer parses is answered with # a usage line, which is also not a refusal. Measured -- changing only the # printed verb (start -> begin) left this test green until the two assertions @@ -159,7 +159,7 @@ skip_if_root() { @test "sync start: a writable run dir still starts an engine (#730)" { # The control. Without it, every assertion above is satisfied by a # `sync start` that refuses unconditionally. - run bash "$SCRIPTS/remote.sh" sync start testteam + run env AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam # The engine is real here and will fail to reach https://remote.example, so # this does not assert success -- only that the refusal above is not what # happened, and that the pidfile path was reachable. @@ -192,7 +192,7 @@ skip_if_root() { local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" local starter i=0 j=0 freed=0 - bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & starter=$! # The engine existing is what says the START is over and the WAIT has begun. @@ -315,7 +315,7 @@ skip_if_root() { local cycles="$TEST_SKILL_DIR/run/remote-sync.testteam.cycles.json" local starter engine foreign i=0 - bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam >/dev/null 2>&1 & starter=$! while [ ! -f "$pidfile" ] && [ "$i" -lt 400 ]; do i=$((i + 1)); sleep 0.05; done [ -f "$pidfile" ] @@ -364,7 +364,7 @@ skip_if_root() { local cycles="$TEST_SKILL_DIR/run/remote-sync.testteam.cycles.json" local starter engine i=0 err="$TEST_SKILL_DIR/retake.err" - bash "$SCRIPTS/remote.sh" sync start testteam >"$err" 2>&1 & + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam >"$err" 2>&1 & starter=$! while [ ! -f "$pidfile" ] && [ "$i" -lt 400 ]; do i=$((i + 1)); sleep 0.05; done [ -f "$pidfile" ] diff --git a/tests/test_remote_status_liveness.bats b/tests/test_remote_status_liveness.bats index 59b44f956..fcb0ce56d 100644 --- a/tests/test_remote_status_liveness.bats +++ b/tests/test_remote_status_liveness.bats @@ -715,7 +715,7 @@ write_windows_spelling_fixtures() { run env PATH="$fake_bin:$PATH" AGMSG_NODE="$fake_node" \ AGMSG_TEST_CHILD_PID_FILE="$child_pid_file" \ - bash "$SCRIPTS/remote.sh" sync start testteam + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam [ "$status" -ne 0 ] [[ "$output" == *"did not become ready"* ]] [ ! -e "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] @@ -771,7 +771,7 @@ write_unownable_ps_fixture() { run env PATH="$fake_bin:$PATH" AGMSG_NODE="$fake_node" \ AGMSG_TEST_CHILD_PID_FILE="$child_pid_file" \ - bash "$SCRIPTS/remote.sh" sync start testteam + AGMSG_TEST_SYNC_READY_TURNS=40 bash "$SCRIPTS/remote.sh" sync start testteam [ "$status" -ne 0 ] child_pid="$(cat "$child_pid_file")" diff --git a/tests/test_sync_readiness_budget.bats b/tests/test_sync_readiness_budget.bats new file mode 100644 index 000000000..fbf321d63 --- /dev/null +++ b/tests/test_sync_readiness_budget.bats @@ -0,0 +1,95 @@ +#!/usr/bin/env bats + +# The readiness poll's ceiling, and the seam that lets the suites skip it (#831). +# +# WHY THE SEAM EXISTS. Reaching `sync start`'s give-up path means waiting out the +# poll, and five regression cases were doing that at the shipped 1600 turns: +# 12 minutes 24 seconds of a macOS CI shard whose job cap is 25, measured from +# the gaps between consecutive `ok` lines on a run that was cancelled for +# exceeding it. None of those five is about the length of the wait; each needs +# only a state where readiness never arrives, which 40 turns produces just as +# well. +# +# WHAT MUST NOT MOVE is the shipped number, and that is what this file pins -- +# separately from the suites that use the seam, so that lowering the default +# reddens something even if every one of those suites is passing. + +load test_helper + +setup() { setup_test_env; export SKILL_DIR="$TEST_SKILL_DIR"; } +teardown() { teardown_test_env; } + +ask_turns() { + cat > "$TEST_SKILL_DIR/turns.sh" <<'EOF_TURNS' +#!/usr/bin/env bash +. "$SCRIPTS/remote.sh" +printf 'turns=%s\n' "$(_remote_sync_ready_turns)" +EOF_TURNS +} + +@test "the shipped readiness ceiling is 1600 turns (#831)" { + # The number itself, asked for rather than run. Bound by letting the poll reach + # the ceiling it cost 52 seconds in one case; this costs milliseconds and holds + # the same fact. + ask_turns + run env SCRIPTS="$SCRIPTS" bash "$TEST_SKILL_DIR/turns.sh" + [ "$status" -eq 0 ] + grep -qF 'turns=1600' <<<"$output" +} + +@test "the seam is honoured when it is set (#831)" { + # THE NEGATIVE CONTROL FOR THE CASE ABOVE. Without it, a resolver that ignored + # the variable and always answered 1600 would satisfy the shipped-default test + # while silently putting every suite that sets the seam back on the full + # ceiling -- which is the CI failure this change exists to remove, restored + # invisibly. + ask_turns + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=40 bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=40' <<<"$output" +} + +@test "a value that is not a count falls back to shipped, not to zero (#831)" { + # A ceiling of zero would end the poll before it began: every `sync start` + # would report failure instantly, and the suites would still be green because + # they are asserting on the give-up path. Asserted for three shapes, because + # they reach the fallback down different comparisons. + ask_turns + for bad in oops -5 12x; do + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS="$bad" bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=1600' <<<"$output" + done +} + +@test "an explicit zero is not a ceiling: it falls back to shipped (#831)" { + # THE ONE THE OTHER FALLBACK CASE DID NOT COVER. `0` is written in digits, so a + # guard that rejects only non-digits accepts it -- and a ceiling of zero ends + # the poll before it runs once. Every suite that drives the give-up path would + # then reach its assertions without the engine ever being waited on, and stay + # green while measuring nothing. + # + # The prose said "not a positive integer falls back". That was true of `oops` + # and false of `0`, and the cases tested `oops`, `-5`, `12x` -- none of them + # the digits-only shape the sentence was actually about (raised in review). + ask_turns + for zero in 0 00 000; do + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS="$zero" bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=1600' <<<"$output" + done +} + +@test "a positive count with a leading zero is still that count (#831)" { + # THE NEGATIVE CONTROL FOR THE CASE ABOVE. "reject anything containing a zero" + # would satisfy it and quietly send `40` -> 1600, putting every seamed suite + # back on the full ceiling -- the exact regression this file exists to catch. + ask_turns + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS=040 bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=040' <<<"$output" +} + +@test "an unset seam and an empty seam agree (#831)" { + # `export FOO=` is not the same shape as never exporting it, and a case split + # on `-n`/`-z` can tell them apart. Both must mean shipped. + ask_turns + run env SCRIPTS="$SCRIPTS" AGMSG_TEST_SYNC_READY_TURNS= bash "$TEST_SKILL_DIR/turns.sh" + grep -qF 'turns=1600' <<<"$output" +}