From e10d1598e5bf8c090a302cb986ecc2cf4827c289 Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 18 Aug 2026 07:02:31 -0700 Subject: [PATCH 1/2] perf(ci): let the readiness suites skip a wait none of them are about The macOS 1/4 shard finishes in 24.1 minutes against a 25-minute job cap. Every other shard has ten to seventeen minutes spare. The last green run on that shard cleared the cap by 54 seconds, so the next test anyone adds there turns their PR red for a reason that is not their test. Measured from the gaps between consecutive `ok` lines on a run that WAS cancelled for exceeding it: five cases held 12m24s, and sixth place was 18 seconds. All five wait out `sync start`'s readiness poll at the shipped 1600 turns. None of them is about how long that wait is; each needs a state where readiness never arrives, and forty turns produces exactly that state. `_remote_sync_ready_turns` lifts the number out of the loop so it can be asked for instead of run, and `AGMSG_TEST_SYNC_READY_TURNS` lets a suite name a shorter ceiling. THE SHIPPED DEFAULT IS UNCHANGED AT 1600 and is pinned by its own file, independently of every suite that sets the seam -- including that a value which is not a count falls back to shipped rather than to zero, since a ceiling of zero would end the poll before it began and leave those same suites green while measuring nothing. Per case, what the seam changed and what it did not: the command the refusal prints (#730) 40 turns; asserts the printed remedy runs -- both invocations, the second inside `bash -c` a writable run dir still starts an engine (#730) 40 turns; asserts an engine appears the registry lock is free while readiness is still polled (#817) 40 turns; needs the starter alive across a 3s window -- it lives ~2s and the case passed 5 of 5 runs a timed-out starter does not clear another engine's records (#817) 40 turns; asserts the other engine's records survive a cleanup that cannot retake the lock says so and keeps the record (#817) 40 turns; asserts the message and both records says a live engine was left behind when it cannot be reaped (#731) 40 turns; asserts the text and that the child is still running reaps a ready-timeout child before releasing ownership 40 turns; asserts the child is reaped Not one assertion is touched: the whole test diff is those invocation lines and nothing else. The first pass seamed only the five CI named and left three slow cases behind -- one of them a case I had already seamed, whose SECOND invocation went through `bash -c` and did not match the pattern. Derived instead from every test that reaches the poll: 242s -> 25s and 145s -> 44s, with 9/9 and 31/31 unchanged. --- scripts/remote.sh | 23 ++++++- tests/test_remote_engine_start_refusal.bats | 12 ++-- tests/test_remote_status_liveness.bats | 4 +- tests/test_sync_readiness_budget.bats | 69 +++++++++++++++++++++ 4 files changed, 99 insertions(+), 9 deletions(-) create mode 100644 tests/test_sync_readiness_budget.bats diff --git a/scripts/remote.sh b/scripts/remote.sh index ddc20f96e..6a737f7b6 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1924,6 +1924,25 @@ _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() { + case "${AGMSG_TEST_SYNC_READY_TURNS:-}" in + ''|*[!0-9]*) printf '1600' ;; + *) printf '%s' "$AGMSG_TEST_SYNC_READY_TURNS" ;; + esac +} + _remote_sync_engine_reap_owned() { local team="$1" owned_pid="$2" state pid signal attempts for signal in TERM KILL; do @@ -2685,7 +2704,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..fa0dac0f2 --- /dev/null +++ b/tests/test_sync_readiness_budget.bats @@ -0,0 +1,69 @@ +#!/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 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" +} From ac90bd84e2f2c2201bc195fed39077af5a8d61e9 Mon Sep 17 00:00:00 2001 From: fujibee Date: Wed, 19 Aug 2026 09:30:13 -0700 Subject: [PATCH 2/2] fix(remote): a ceiling of zero is not a ceiling `0` is written in digits, so a guard that rejected only non-digits accepted it and the readiness poll ended before running once. Every suite driving the give-up path would then reach its assertions with the engine never waited on, and stay green while measuring nothing -- the failure this fallback is described as preventing. The description was true of `oops` and false of `0`, and the cases covered `oops`, `-5`, `12x`: none of them the digits-only shape the sentence was actually about. Raised in review. `0`, `00`, `000` fall back to the shipped 1600. `040` still resolves to 40, which is the negative control against "reject anything containing a zero" -- that would send every seamed suite back to the full ceiling silently. Mutation: dropping the zero guard reddens the new case and nothing else. --- scripts/remote.sh | 17 ++++++++++++++--- tests/test_sync_readiness_budget.bats | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 6a737f7b6..0a5aa512c 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1937,9 +1937,20 @@ _remote_sync_engine_status() { # 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() { - case "${AGMSG_TEST_SYNC_READY_TURNS:-}" in - ''|*[!0-9]*) printf '1600' ;; - *) printf '%s' "$AGMSG_TEST_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 } diff --git a/tests/test_sync_readiness_budget.bats b/tests/test_sync_readiness_budget.bats index fa0dac0f2..fbf321d63 100644 --- a/tests/test_sync_readiness_budget.bats +++ b/tests/test_sync_readiness_budget.bats @@ -60,6 +60,32 @@ EOF_TURNS 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.