fix(canary): cache run-list per (repo,workflow) + not-found→empty (#819) — restores the cancelled Canary Rollout job

scripts/canary-rollout.sh

@@ -414,43 +414,75 @@ _gh_retry_after() {
   printf '%s' "$secs"
 }
 
-# _run_json <repo> <workflow> <since_z> — 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 <repo> <workflow> — 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 <repo> <workflow> <since_z> — 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 <agent> <since_z> <repo...> — EXECUTED runs (success+failure) on the
 # source tier since the candidate cut. Prints "<executed> <earliest_createdAt|->".
 _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 <agent>" >&2; return 2; }; cmd_evaluate "$@" ;;

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" ]