diff --git a/scripts/canary-rollout.sh b/scripts/canary-rollout.sh index 2824fc680..849e7589b 100755 --- a/scripts/canary-rollout.sh +++ b/scripts/canary-rollout.sh @@ -414,43 +414,75 @@ _gh_retry_after() { printf '%s' "$secs" } -# _run_json — gh run-list JSON (conclusion,createdAt,databaseId,workflowName) for a -# repo since the given Zulu timestamp. Empty repo/wildcard → []. Never fails the caller. -_run_json() { - local repo="$1" wf="$2" since="$3" out err summary ra delay span expo errfile - if [ -z "$repo" ] || [ "$repo" = '*' ]; then echo '[]'; return 0; fi - # The 4h scheduled tick fans this out across every agent × tier repo, so a single - # transient blip (network / secondary rate-limit / brief 5xx) among dozens of reads - # used to fail the whole fleet sweep (#738). Retry a non-zero `gh` a bounded number of - # times to ride out a momentary hiccup, capturing gh's stderr so the annotations name - # the REAL failure (HTTP status + class) instead of a bare "transient" (#810). Backoff - # is exponential + full-jitter and honors a server Retry-After / x-ratelimit-reset hint - # so a secondary-rate-limit window is respected rather than hammered. A SUSTAINED failure - # still exhausts the retries and returns non-zero: fail CLOSED on a genuine outage - # (returning an empty [] would read as "zero failures" and could green-light a bad - # promotion — a non-zero return halts the run under `set -e` instead). An empty-but- - # successful result (no runs) is still a valid []. Tunable via CANARY_GH_RETRIES / - # CANARY_GH_RETRY_SLEEP (tests set 0). CANARY_GH_RETRY_AFTER_CAP caps server hints (default 900s). +# _repo_wf_runs_cached — ALL recent runs of one workflow on a repo +# (conclusion,createdAt,databaseId,workflowName), UNBOUNDED by date and memoized for the +# process, so the candidate / baseline / downgrade / correctness windows a single agent +# samples collapse from one `gh run list` per (repo × workflow × window) into ONE read +# per (repo × workflow) (#819). This is the fix for the secondary-rate-limit burst AND +# for the retry storm it caused: a workflow that has never run on a repo makes `gh run +# list --workflow` exit non-zero with "could not find any workflows named …" — a PERMANENT +# condition, not a transient one, so retrying it 6× with backoff (as #810's wrapper did +# for every error class) blew a fleet sweep out to ~20 min and got the job cancelled. We +# now classify that message as a legitimate empty result ([]) and never retry it; only a +# genuine transport error (403/5xx/network/auth) is retried and, if sustained, fails CLOSED. +# +# The cache is FILE-backed under $_RUNS_CACHE_DIR (exported by main() before the fan-out), +# because callers invoke this via command substitution — a subshell — so an in-memory array +# populated in one call would not survive to the next. When the dir is unset (a unit test +# calling _run_json directly), every call fetches. Tunable via CANARY_GH_RETRIES / +# CANARY_GH_RETRY_SLEEP (tests set 0). CANARY_GH_RETRY_AFTER_CAP caps server hints (default 900s). +_repo_wf_runs_cached() { + local repo="$1" wf="$2" out err summary ra delay span expo errfile cachef key keyhash local attempts="${CANARY_GH_RETRIES:-6}" base="${CANARY_GH_RETRY_SLEEP:-2}" attempt=1 local ra_cap="${CANARY_GH_RETRY_AFTER_CAP:-900}" case "$ra_cap" in ''|*[!0-9]*) ra_cap=900 ;; esac case "$attempts" in ''|*[!0-9]*) attempts=6 ;; esac case "$base" in ''|*[!0-9]*) base=2 ;; esac - errfile="$(mktemp)" - if ! declare -p tmpfiles &>/dev/null; then - declare -g -a tmpfiles=() - trap 'rm -f "${tmpfiles[@]+"${tmpfiles[@]}"}"' EXIT + # Cache hit? A non-empty cache file for this (repo, workflow) — including a cached "[]" + # for a no-runs / not-found workflow, so it is never re-queried within the sweep. + if [ -n "${_RUNS_CACHE_DIR:-}" ]; then + # Hash the (repo, workflow) key into the filename so distinct pairs can never collide. + # A plain char-substitution (e.g. non-alnum → "_") would map a workflow named "A B" and + # one named "A/B" on the same repo to the same file — cross-contaminating their cached + # run history and so their gate health. Workflow display names carry spaces and em-dashes, + # so this is a real collision surface. sha256 (not sha1/md5 — those trip weak-hash linters + # and are collision-broken) keeps the mapping injective; fall back to substitution only if + # no hasher exists. This is a filename derivation, not a security context. + key="${repo}//${wf}" + # sha256sum on Linux runners, shasum -a 256 on macOS; substitution only if neither + # exists (and then, at worst, the pre-existing collision surface — never a crash). + keyhash="$(printf '%s' "$key" | { sha256sum 2>/dev/null || shasum -a 256 2>/dev/null; } | cut -d' ' -f1)" + [ -n "$keyhash" ] || keyhash="${key//[^A-Za-z0-9._-]/_}" + cachef="$_RUNS_CACHE_DIR/${keyhash}.json" + [ -s "$cachef" ] && { cat "$cachef"; return 0; } fi + errfile="$(mktemp)" + declare -p tmpfiles &>/dev/null || declare -g -a tmpfiles=() tmpfiles+=("$errfile") + # Register the cleanup in THIS (sub)shell: command substitution forks a subshell that + # does NOT inherit the parent's traps, so registering only on first declaration of + # tmpfiles would leave every subshell invocation without one. Re-running `trap … EXIT` + # in the same shell just reinstalls the same idempotent handler. + trap 'rm -f "${tmpfiles[@]+"${tmpfiles[@]}"}"' EXIT while :; do - if out="$(gh run list --repo "$repo" --workflow "$wf" ${since:+--created ">=$since"} \ + if out="$(gh run list --repo "$repo" --workflow "$wf" \ -L 1000 --json conclusion,createdAt,databaseId,workflowName 2>"$errfile")"; then - rm -f "$errfile"; echo "${out:-[]}"; return 0 + rm -f "$errfile"; out="${out:-[]}" + [ -n "${cachef:-}" ] && [ -d "$_RUNS_CACHE_DIR" ] && printf '%s' "$out" > "$cachef" 2>/dev/null || true + printf '%s\n' "$out"; return 0 fi err="" if [ -r "$errfile" ]; then err="$(<"$errfile")" fi + # A workflow with no runs on this repo is NOT a fetch failure — `gh` just can't resolve + # the name. Treat it as an empty result (and cache []) so it is neither retried nor + # counted as an outage; retrying it was the #810→#803 cancellation storm. + if [[ "${err,,}" == *"could not find any workflow"* ]] || [[ "${err,,}" == *"no workflows"* ]]; then + rm -f "$errfile" + [ -n "${cachef:-}" ] && [ -d "$_RUNS_CACHE_DIR" ] && printf '%s' '[]' > "$cachef" 2>/dev/null || true + echo '[]'; return 0 + fi summary="$(_gh_err_summary "$err")" if [ "$attempt" -ge "$attempts" ]; then echo "::error::_run_json: failed to fetch run list for $repo (workflow=$wf) after $attempts attempt(s) — $summary" >&2 @@ -480,6 +512,22 @@ _run_json() { done } +# _run_json — the target workflow's runs on a repo since the +# given Zulu timestamp, filtered LOCALLY from the per-(repo,workflow) cache so the several +# windows an agent samples share one fetch (#819). Empty repo/wildcard → []. A genuine +# fetch failure fails CLOSED (non-zero) so an empty [] never masks real failures and +# green-lights a bad promotion. Empty since = no lower bound. +_run_json() { + local repo="$1" wf="$2" since="$3" raw + if [ -z "$repo" ] || [ "$repo" = '*' ]; then echo '[]'; return 0; fi + raw="$(_repo_wf_runs_cached "$repo" "$wf")" || return 1 + # Local since cut, inclusive — the same window the old server-side `--created ">=$since"` + # produced. jq on an empty/garbage payload falls back to []. + jq -c --arg since "$since" \ + '[ .[]? | select($since == "" or (.createdAt // "") >= $since) ]' \ + <<< "${raw:-[]}" 2>/dev/null || echo '[]' +} + # _tier_sample — EXECUTED runs (success+failure) on the # source tier since the candidate cut. Prints " ". _tier_sample() { @@ -488,7 +536,7 @@ _tier_sample() { wf="$(_agent_field "$agent" run_workflow)" for repo in "$@"; do json="$(_run_json "$repo" "$wf" "$since")" - executed=$(( executed + $(jq '[.[]?|select(.conclusion=="success" or .conclusion=="failure")]|length' 2>/dev/null <<< "$json" || echo 0) )) + executed=$(( executed + $(jq '[.[]?|select(.conclusion=="success" or .conclusion=="failure")]|length' 2>/dev/null <<< "${json:-[]}" || echo 0) )) e="$(jq -r '[.[]?|select(.conclusion=="success" or .conclusion=="failure")|.createdAt?]|min // empty' 2>/dev/null <<< "$json" || echo "")" if [ -n "$e" ] && { [ -z "$earliest" ] || [[ "$e" < "$earliest" ]]; }; then earliest="$e"; fi done @@ -663,11 +711,11 @@ _cumulative_health() { suspect_patterns="$(_suspect_patterns "$agent")" for repo in "$@"; do json="$(_run_json "$repo" "$wf" "$since")" - startup=$(( startup + $(jq '[.[]?|select(.conclusion=="startup_failure")]|length' 2>/dev/null <<< "$json" || echo 0) )) + startup=$(( startup + $(jq '[.[]?|select(.conclusion=="startup_failure")]|length' 2>/dev/null <<< "${json:-[]}" || echo 0) )) if [ -z "$patterns" ] && [ -z "$suspect_patterns" ]; then # No benign or suspect patterns to match — count all failures with one jq pass, # avoiding a gh run view call per failure. - fail=$(( fail + $(jq '[.[]?|select(.conclusion=="failure")]|length' 2>/dev/null <<< "$json" || echo 0) )) + fail=$(( fail + $(jq '[.[]?|select(.conclusion=="failure")]|length' 2>/dev/null <<< "${json:-[]}" || echo 0) )) else while IFS=$'\t' read -r rid rwf; do if [ -n "$patterns" ] && _failure_benign "$repo" "$rid" "$rwf" "$patterns"; then @@ -2118,6 +2166,17 @@ main() { echo "::error::SOAK_WINDOW_DAYS, when set, must be a positive integer" >&2 return 2 fi + # Per-process run-list cache shared across the fan-out's command-substitution subshells + # (#819) — exported so subshells inherit the path, and reaped on exit. This trap lives in + # the PARENT shell; _repo_wf_runs_cached's errfile trap lives in the per-call subshells, so + # the two are in different shells and never clobber each other. + if [ -z "${_RUNS_CACHE_DIR:-}" ]; then + _RUNS_CACHE_DIR="$(mktemp -d 2>/dev/null || true)" + if [ -n "$_RUNS_CACHE_DIR" ]; then + export _RUNS_CACHE_DIR + trap 'rm -rf "${_RUNS_CACHE_DIR:-}"' EXIT + fi + fi local sub="${1:-}"; shift || true case "$sub" in evaluate) [ $# -ge 1 ] || { echo "usage: evaluate " >&2; return 2; }; cmd_evaluate "$@" ;; diff --git a/tests/canary_rollout.bats b/tests/canary_rollout.bats index 5c5e0e6b5..45e162b5c 100644 --- a/tests/canary_rollout.bats +++ b/tests/canary_rollout.bats @@ -602,6 +602,120 @@ GHEOF grep -qx '60' "$SLEEP_LOG" } +# ── _run_json: per-(repo,workflow) cache + not-found-is-empty + local since (#819) ── +# The 4h sweep asks for a workflow's runs once per sample window (candidate / baseline / +# downgrade / correctness). Enumerating per window per workflow tripped secondary rate +# limits, AND a zero-run workflow made `gh run list --workflow` exit non-zero with "could +# not find any workflows named …" — which #810's wrapper retried 6× as a transient, blowing +# a sweep out to ~20 min until the job was cancelled. #819: one UNBOUNDED read per +# (repo,workflow), memoized; the since cut applied locally; a not-found is a cached []. +@test "_run_json: memoizes per (repo,workflow) — two windows share ONE gh call (#819)" { + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + export CALLS="$BATS_TEST_TMPDIR/gh-calls"; : > "$CALLS" + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +echo "call" >> "$CALLS" +echo '[{"conclusion":"success","createdAt":"2026-01-10T00:00:00Z","databaseId":1,"workflowName":"W"}, + {"conclusion":"failure","createdAt":"2026-01-02T00:00:00Z","databaseId":2,"workflowName":"W"}]' +GHEOF + chmod +x "$STUB_BIN/gh" + run env _RUNS_CACHE_DIR="$BATS_TEST_TMPDIR/rc" bash -c ' + mkdir -p "$_RUNS_CACHE_DIR"; source "'"$ORCH"'" + _run_json some/repo W "2026-01-05T00:00:00Z" # candidate window (since = 01-05) + _run_json some/repo W "" # baseline window (no lower bound) + ' + [ "$status" -eq 0 ] + # Both windows served by a single fetch of (some/repo, W). + [ "$(wc -l < "$CALLS")" -eq 1 ] +} + +@test "_run_json: cache key is collision-free — 'A B' vs 'A/B' workflows don't share a file (#835 CodeRabbit)" { + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + # gh echoes back the requested workflow name so we can prove which cache entry served the call. + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +wf=""; prev="" +for a in "$@"; do [ "$prev" = "--workflow" ] && wf="$a"; prev="$a"; done +printf '[{"conclusion":"success","createdAt":"2026-01-10T00:00:00Z","databaseId":1,"workflowName":"%s"}]\n' "$wf" +GHEOF + chmod +x "$STUB_BIN/gh" + run env _RUNS_CACHE_DIR="$BATS_TEST_TMPDIR/rc2" bash -c ' + mkdir -p "$_RUNS_CACHE_DIR"; source "'"$ORCH"'" + _run_json some/repo "A B" "" | jq -r ".[0].workflowName" + _run_json some/repo "A/B" "" | jq -r ".[0].workflowName" + ' + [ "$status" -eq 0 ] + # Under a char-substitution key both names would collapse to "A_B" and the second call + # would be served the first's cached "A B" row. A hashed key keeps them distinct. + [ "$(printf '%s\n' "$output" | sed -n 1p)" = "A B" ] + [ "$(printf '%s\n' "$output" | sed -n 2p)" = "A/B" ] +} + +@test "_run_json: applies the since cut LOCALLY — pre-cut rows excluded, empty since keeps all (#819)" { + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +echo '[{"conclusion":"success","createdAt":"2026-01-10T00:00:00Z","databaseId":1,"workflowName":"W"}, + {"conclusion":"failure","createdAt":"2026-01-02T00:00:00Z","databaseId":2,"workflowName":"W"}]' +GHEOF + chmod +x "$STUB_BIN/gh" + # since = 01-05 → only the 01-10 row survives. + run bash -c "source '$ORCH' && _run_json some/repo W '2026-01-05T00:00:00Z' | jq -c 'map(.databaseId)'" + [ "$status" -eq 0 ]; [ "$output" = "[1]" ] + # empty since → both rows. + run bash -c "source '$ORCH' && _run_json some/repo W '' | jq -c 'map(.databaseId)|sort'" + [ "$status" -eq 0 ]; [ "$output" = "[1,2]" ] +} + +@test "_run_json: fetch is unbounded — passes --workflow but NOT --created (#819)" { + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + export ARGS="$BATS_TEST_TMPDIR/gh-args" + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +echo "$*" >> "$ARGS" +echo '[]' +GHEOF + chmod +x "$STUB_BIN/gh" + run bash -c "source '$ORCH' && _run_json some/repo 'Dev-Lead Agent' '2026-01-05T00:00:00Z'" + [ "$status" -eq 0 ] + grep -q -- '--workflow' "$ARGS" + ! grep -q -- '--created' "$ARGS" +} + +@test "_run_json: a not-found workflow returns [] immediately — no retry, no fail-flag (#819)" { + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + export CALLS="$BATS_TEST_TMPDIR/nf-calls"; : > "$CALLS" + cat > "$STUB_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +echo "call" >> "$CALLS" +echo "could not find any workflows named Persona — Mention Router" >&2 +exit 1 +GHEOF + chmod +x "$STUB_BIN/gh" + export FLAG="$BATS_TEST_TMPDIR/fetch-fail-flag"; : > "$FLAG" + run env CANARY_GH_RETRY_SLEEP=0 CANARY_GH_RETRIES=6 _CANARY_FETCH_FAIL_FLAG="$FLAG" \ + bash -c "source '$ORCH' && _run_json some/repo 'Persona — Mention Router' ''" + [ "$status" -eq 0 ] + [ "$output" = "[]" ] + # Not retried (single gh call) and NOT recorded as a fetch outage (#820 flag empty). + [ "$(wc -l < "$CALLS")" -eq 1 ] + [ ! -s "$FLAG" ] +} + +@test "_cumulative_health: does not crash when a window fails CLOSED to an empty payload (#819 arith guard)" { + # A sustained fetch failure makes _run_json return non-zero → json="" in the caller. The + # `$(( fail + $(jq … <<< "${json:-[]}") ))` guard must count 0, never emit `+ ` (bash + # arithmetic operand-expected syntax error, the second half of the #803 canary regression). + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")"; export PATH="$STUB_BIN:$PATH" + printf '#!/usr/bin/env bash\necho "gh: HTTP 503: server error" >&2\nexit 1\n' > "$STUB_BIN/gh" + chmod +x "$STUB_BIN/gh" + run env CANARY_GH_RETRY_SLEEP=0 CANARY_GH_RETRIES=1 \ + bash -c "source '$ORCH' && _cumulative_health dev-lead '' 0 some/repo" + # Fail-closed path returns non-zero, but NEVER with an arithmetic syntax error. + [[ "$output" != *"syntax error"* ]] + [[ "$output" != *"operand expected"* ]] +} + # ── next_channel_in_order ───────────────────────────────────────────────────── @test "next_channel_in_order: walks the ring order" { [ "$(next_channel_in_order next 'next,ring0,ring1,stable')" = "ring0" ]