From e7286b5851a8f91d108f5cbd97b2513a1dc00ba7 Mon Sep 17 00:00:00 2001 From: Tomer Weller Date: Tue, 25 Aug 2026 03:03:03 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Regression=20test=20for=20#3823=20=E2=80=94?= =?UTF-8?q?=20fails=20on=20current=20main?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add classify_ci_cancel TAP cases to scripts/test-monitor-skill-snippets.sh asserting a no-successor CI cancel at its job timeout-minutes cap classifies as hang-cancel (CI-not-green), while supersede/manual cancels and the failure path stay unchanged. Fails on current main: the function is absent, so the harness aborts with "classify_ci_cancel: command not found". Refs #3823 Co-authored-by: Claude Code --- scripts/test-monitor-skill-snippets.sh | 85 +++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/scripts/test-monitor-skill-snippets.sh b/scripts/test-monitor-skill-snippets.sh index 07088aa8..c0d7d74d 100755 --- a/scripts/test-monitor-skill-snippets.sh +++ b/scripts/test-monitor-skill-snippets.sh @@ -55,7 +55,7 @@ cleanup() { trap cleanup EXIT # ── TAP state ──────────────────────────────────────────────────────────────── -TAP_PLAN=479 +TAP_PLAN=486 TAP_CURRENT=0 TAP_FAILURES=0 @@ -2577,6 +2577,89 @@ PYEOF "check (9) not wired to the streak escalator" fi + # ════════════════════════════════════════════════════════════════════════════ + # classify_ci_cancel (T63aa–T63ag) — #3823 + # Source: scripts/lib/monitor-decisions.sh — monitor-tick check (11) CI check. + # + # classify_ci_cancel CONCLUSION HAS_SUCCESSOR DURATION_SEC TIMEOUT_SEC + # prints exactly one of four stable literals on stdout (and exports the same + # value in CI_CANCEL_CLASS): + # not-cancel | supersede-cancel | hang-cancel | manual-cancel + # Only `hang-cancel` is CI-not-green. It fires when a job was cancelled + # (conclusion=cancelled) with NO superseding run and its wall-clock duration + # reached its job-level `timeout-minutes` cap — i.e. it was killed at the + # cap (#3768's 55m-job / timeout-minutes:45 fixture), the mode where + # `if: failure()` never runs so the #3289 diagnostics dump is destroyed. + # A missing/unreadable timeout on a no-successor cancel falls back to + # `hang-cancel` (fail toward surfacing, never hiding). + # + # RED on origin/main: the function is absent there, so any call aborts the + # harness with "command not found" (set -euo pipefail), and check (11b)'s + # `select(.conclusion == "failure")` returns empty for a cancelled job (scored + # green). GREEN after #3823. + # ════════════════════════════════════════════════════════════════════════════ + + # ── Test 63aa: no-successor cancel at its cap → hang-cancel (#3768 fixture) ── + # 55m job (3300s) vs timeout-minutes:45 (2700s), no superseding run. + local cc + cc=$(classify_ci_cancel cancelled no 3300 2700) + if [[ "$cc" == "hang-cancel" ]]; then + tap_ok "ci-cancel-classify: cancelled/no-successor/3300>=2700 → hang-cancel" + else + tap_not_ok "ci-cancel-classify: cancelled/no-successor/3300>=2700 → hang-cancel" "got '$cc'" + fi + + # ── Test 63ab: routine concurrency supersede-cancel stays green (#3653) ────── + # A newer run for the SAME workflow+ref exists → intentional cancel, not a hang. + cc=$(classify_ci_cancel cancelled yes 600 2700) + if [[ "$cc" == "supersede-cancel" ]]; then + tap_ok "ci-cancel-classify: cancelled/successor → supersede-cancel (stays green)" + else + tap_not_ok "ci-cancel-classify: cancelled/successor → supersede-cancel" "got '$cc'" + fi + + # ── Test 63ac: cancelled under cap, no successor → manual-cancel (not a hang) ─ + cc=$(classify_ci_cancel cancelled no 120 2700) + if [[ "$cc" == "manual-cancel" ]]; then + tap_ok "ci-cancel-classify: cancelled/no-successor/120<2700 → manual-cancel" + else + tap_not_ok "ci-cancel-classify: cancelled/no-successor/120<2700 → manual-cancel" "got '$cc'" + fi + + # ── Test 63ad: failure conclusion is not a cancel → not-cancel (path intact) ─ + cc=$(classify_ci_cancel failure no 1560 2700) + if [[ "$cc" == "not-cancel" ]]; then + tap_ok "ci-cancel-classify: failure → not-cancel (failure/success handling unchanged)" + else + tap_not_ok "ci-cancel-classify: failure → not-cancel" "got '$cc'" + fi + + # ── Test 63ae: missing/unreadable timeout, no successor → hang-cancel ──────── + # Fail toward surfacing: if the job's timeout-minutes can't be read, a + # no-successor cancel MUST NOT be hidden as manual-cancel. + cc=$(classify_ci_cancel cancelled no 3300 "") + if [[ "$cc" == "hang-cancel" ]]; then + tap_ok "ci-cancel-classify: unreadable timeout + no successor → hang-cancel (fail-open)" + else + tap_not_ok "ci-cancel-classify: unreadable timeout + no successor → hang-cancel" "got '$cc'" + fi + + # ── Test 63af: duration exactly at the cap → hang-cancel (>= boundary) ─────── + cc=$(classify_ci_cancel cancelled no 2700 2700) + if [[ "$cc" == "hang-cancel" ]]; then + tap_ok "ci-cancel-classify: duration == timeout → hang-cancel (>= boundary)" + else + tap_not_ok "ci-cancel-classify: duration == timeout → hang-cancel" "got '$cc'" + fi + + # ── Test 63ag: consistency — SKILL.md check (11) references classify_ci_cancel + if grep -q 'classify_ci_cancel' "$tick_file_ob"; then + tap_ok "consistency: monitor-tick/SKILL.md references classify_ci_cancel" + else + tap_not_ok "consistency: monitor-tick/SKILL.md references classify_ci_cancel" \ + "check (11) not wired to the shared cancel classifier" + fi + # ── Test 64: Tick history capture invokes the single-writer helper ────────── # #3791: the hand-rolled `<<'PY'` / json.dumps heredoc is replaced by a call # to scripts/lib/monitor-tick-artifacts.py emit-row so there is exactly one From c27b892638b67bba86fc84b7920d826876ac3e2b Mon Sep 17 00:00:00 2001 From: Tomer Weller Date: Tue, 25 Aug 2026 03:08:03 +0000 Subject: [PATCH 2/2] Surface CI hang-cancels in monitor-tick check (11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CI job wall-clock-killed at its timeout-minutes cap resolves conclusion `cancelled`, not `failure`, so monitor-tick check (11) — which keyed only on `failure` — scored it `ci: all green`. Per #3768 that is the worse of the two testnet-shard modes: `if: failure()` never fires, the #3289 diagnostics dump is destroyed, and `--log-failed` is empty, so the monitor was blind exactly where the evidence is thinnest. Add a pure, unit-tested `classify_ci_cancel` to monitor-decisions.sh that maps a job outcome to not-cancel / supersede-cancel / hang-cancel / manual-cancel, distinguishing a reportable hang (no successor, duration >= job timeout-minutes) from a routine concurrency supersede (avoiding the #3653 alarm-fatigue regression). Rewire check (11a)/(11b)/REPORTING RULE and the `ci:` status line around it, read the immutable per-job conclusion (closing the run-level non-monotonicity a retry would otherwise cause), and add the no-auto-rerun rule for hang-cancels. Refs #3823 Co-authored-by: Claude Code --- .claude/skills/monitor-tick/SKILL.md | 53 +++++++++++++--- scripts/lib/monitor-decisions.sh | 94 ++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 8 deletions(-) diff --git a/.claude/skills/monitor-tick/SKILL.md b/.claude/skills/monitor-tick/SKILL.md index 0a924cb3..e45d36a8 100644 --- a/.claude/skills/monitor-tick/SKILL.md +++ b/.claude/skills/monitor-tick/SKILL.md @@ -2199,7 +2199,8 @@ Otherwise enter the deploy path: **(11a) Scope**: only inspect workflows that run on branch main. `gh run list --branch main --limit 10 --json databaseId,name,status,conclusion,headSha,createdAt --jq '.[] | "\(.name)|\(.status)|\(.conclusion)|\(.headSha[:8])|\(.databaseId)|\(.createdAt)"'`. Ignore runs triggered by PRs on other branches. Scan for completed runs with -conclusion `failure`. +conclusion `failure` **or `cancelled`** — a `cancelled` run is NOT automatically +green (see the hang-cancel handling below, #3823). **(11b) Job-level** (CRITICAL — catches continue-on-error failures): For the latest completed run of EACH distinct workflow name (enumerate dynamically @@ -2207,16 +2208,49 @@ from 11a, do NOT hard-code names — and take the run **IDs** from THIS tick's 11a output, never carry an ID list over from a previous tick: a workflow that ran again since then has a new ID, so a reused ID reports a stale conclusion, the same wrong-conclusion class as #3883), check individual jobs: -`gh run view --json jobs --jq '.jobs[] | select(.conclusion == "failure") | "\(.name)|\(.conclusion)"'`. +`gh run view --json jobs --jq '.jobs[] | select(.conclusion == "failure" or .conclusion == "cancelled") | "\(.name)|\(.conclusion)|\(.startedAt)|\(.completedAt)"'`. Workflows with continue-on-error jobs report run-level conclusion `success` even when jobs fail — you MUST check job-level conclusions. If any jobs have -conclusion `failure`, treat it the same as a run-level failure. +conclusion `failure`, treat it the same as a run-level failure. Use the +**immutable per-job conclusion** here (not the run-level one): a retry resets a +run's `conclusion` to `null`, but a concluded job's value is fixed, so reading +per-job also closes the run-level non-monotonicity that would otherwise let a +retry hide a `cancelled` result on the next tick (#3823 comment 6). + +**Classifying a `cancelled` job (#3823)** — a job wall-clock-killed at its +`timeout-minutes` cap resolves `cancelled`, not `failure`. That is the WORSE of +the two testnet-shard modes (#3768): `if: failure()` never fires, so the #3289 +watchdog diagnostics dump is destroyed and `--log-failed` is empty. But a +routine concurrency supersede also resolves `cancelled`, and surfacing those +would recreate the #3653 alarm fatigue. Discriminate with the shared pure +function `classify_ci_cancel` (`scripts/lib/monitor-decisions.sh`): + +```bash +source scripts/lib/monitor-decisions.sh +# For a cancelled job, gather its inputs: +# - HAS_SUCCESSOR: `yes` iff a NEWER run of the SAME workflow AND same head ref +# (main) exists in the 11a listing — matches the concurrency group keyed on +# github.ref, so a concurrent PR run of the same workflow is NOT a supersede. +# - DURATION_SEC : the job's completedAt − startedAt, in seconds. +# - TIMEOUT_SEC : the job-level `timeout-minutes` from the workflow YAML, in +# seconds (quickstart.yml=45m=2700, history-publish.yml=40m=2400). NOT the +# step-level `step_timeout_minutes: 25` — the step timeout is only +# corroborating evidence in the report. If unreadable, the function fails +# toward surfacing (treats a no-successor cancel as a hang). +classify_ci_cancel "$conclusion" "$has_successor" "$duration_sec" "$timeout_sec" +# CI_CANCEL_CLASS ∈ not-cancel | supersede-cancel | hang-cancel | manual-cancel +``` + +Only `hang-cancel` is CI-not-green. `supersede-cancel` and `manual-cancel` stay +green; `not-cancel` defers to the failure/success handling above. **REPORTING RULE** — NEVER report `ci: all green` if ANY job has conclusion -`failure`, even if the run-level conclusion is `success`. The `ci:` line in -the status report MUST reflect the WORST job-level result across all -workflows. A continue-on-error job failure is NOT green — it is RED. Do not -qualify failures as "known", "pre-existing", or "cosmetic". +`failure`, OR is classified `hang-cancel` by `classify_ci_cancel`, even if the +run-level conclusion is `success` (or `cancelled`). The `ci:` line in the status +report MUST reflect the WORST job-level result across all workflows, ranked: +**job `failure` > `hang-cancel` > `supersede-cancel`/`manual-cancel`/green**. A +continue-on-error job failure is NOT green — it is RED. A `hang-cancel` is NOT +green either. Do not qualify failures as "known", "pre-existing", or "cosmetic". Only act on failures from the last 2 hours (compare `createdAt` with `date -u +%Y-%m-%dT%H:%M:%SZ`). For each failure: @@ -2242,6 +2276,9 @@ Only act on failures from the last 2 hours (compare `createdAt` with If the failure is clearly NOT automation (build error from real code, test assertion mismatch, hash mismatch, panic from production code), skip the rerun and proceed to step 4. + **Never auto-rerun a `hang-cancel`** (#3823): #3768 shows the rerun burns + another full-length runner slot and lands identically at the same + `timeout-minutes` wall. File/comment instead (step 4/5); do not `gh run rerun`. 4. Check for an existing open issue: `gh issue list --search "" --state open`. If one matches, `gh issue comment ` with the new evidence (sha, log @@ -2519,7 +2556,7 @@ MONITOR — L — metrics_ratio: scp , apply 50% (N ticks) — investigating>, pending 50% (N ticks)> | collecting baseline recovery_stalled: ) | collecting baseline> deploy: reachable from origin/main — see ~/data/deploy_quarantine.txt) | BLOCKED (quarantine file unreadable — fail-closed) | BLOCKED (quarantine ancestry check failed for — fail-closed) | DEFERRED (cool-down: ...) | SYNCED (no-binary-impact: ...) | pulled N commits (old..new) | SKIPPED (dirty-tree|ci-red|build-failed, filed/commented #)> - ci: | WORKFLOW jobs FAILED (continue-on-error) — NAME|conclusion listed, filed/commented #> + ci: | WORKFLOW jobs FAILED (continue-on-error) — NAME|conclusion listed, filed/commented # | WORKFLOW job CANCELLED-HANG () — run , filed/commented #> self_reflect: : ) | filed # (urgent: ) | filed # (no-label: ) | filed # (not-ready: )> ``` diff --git a/scripts/lib/monitor-decisions.sh b/scripts/lib/monitor-decisions.sh index a74a7395..812428e5 100755 --- a/scripts/lib/monitor-decisions.sh +++ b/scripts/lib/monitor-decisions.sh @@ -1597,6 +1597,100 @@ eval_obsrvr_not_indexed_streak() { return 0 } +# ───────────────────────────────────────────────────────────────────────────── +# classify_ci_cancel CONCLUSION HAS_SUCCESSOR DURATION_SEC TIMEOUT_SEC +# +# Pure decision function for monitor-tick check (11) CI check (issue #3823). +# +# check (11a)/(11b) historically keyed EXCLUSIVELY on conclusion `failure`, so a +# CI job wall-clock-killed at its `timeout-minutes` cap — which resolves as +# conclusion `cancelled`, NOT `failure` — was scored `ci: all green`. Per the +# #3768 / #3741 thread that is the WORSE of the two testnet-shard failure modes: +# because the job ends `cancelled`, `if: failure()` never fires, the #3289 +# watchdog diagnostics dump is destroyed, and `gh run view --log-failed` returns +# nothing. So the monitor was blind exactly where the evidence is thinnest. +# +# This function maps a run/job outcome to ONE of four stable literals so the tick +# can distinguish a reportable hang-cancel from a routine supersede-cancel +# (avoiding the #3653 alarm-fatigue regression that a blanket "match cancelled" +# would cause): +# +# not-cancel - conclusion != cancelled. Defer to the existing +# failure/success handling; this function has no opinion. +# supersede-cancel - a newer run for the SAME workflow AND same head ref exists +# (routine concurrency cancel, `cancel-in-progress: true`). +# Intentionally obsolete → stays green. +# hang-cancel - no superseding run AND the run's wall-clock duration +# reached its job-level `timeout-minutes` cap → killed at the +# cap. CI-NOT-GREEN and REPORTABLE. +# manual-cancel - no superseding run and duration is under the cap (an +# early cancel for some other reason). Low-tier, stays green. +# +# Arguments: +# CONCLUSION - the IMMUTABLE per-job/per-attempt conclusion (from +# `gh run view --json jobs`), NOT the run-level conclusion. +# Using the per-job value also closes the run-level +# non-monotonicity (thread comment 6): a retry resets run-level +# `conclusion` to `null`, but a concluded job's value is fixed. +# HAS_SUCCESSOR - literal `yes`/`no`: whether a newer run of the SAME workflow +# AND same head ref (`main`) exists. Ref-scoped to match the +# concurrency group keyed on `github.ref` — a concurrent PR run +# of the same workflow is NOT a supersede and must not hide a +# real main hang. Anything other than `yes` is treated as `no`. +# DURATION_SEC - the job's wall-clock seconds (from its `startedAt`/ +# `completedAt`). Non-numeric / empty ⇒ unreadable. +# TIMEOUT_SEC - the job-level `timeout-minutes` from the workflow YAML, in +# seconds (quickstart.yml=45m=2700, history-publish.yml=40m= +# 2400). NOT the step-level `step_timeout_minutes` — the step +# timeout is only corroborating evidence in the report. +# Non-numeric / empty ⇒ unreadable. +# +# Fail-open rule: when TIMEOUT_SEC or DURATION_SEC is missing/unreadable and the +# cancel has no successor, classify `hang-cancel` — fail toward surfacing, never +# toward hiding a possible hang. GitHub's ~5m force-kill grace means a real hang +# overshoots the job cap comfortably, so the coarse `>=` floor is safe. +# +# Prints exactly one literal on stdout AND sets global CI_CANCEL_CLASS to the +# same value (mirrors the classify_* siblings' echo-plus-global contract). +# Returns: 0 always. Does no process/network I/O — the caller gathers the inputs. +# Portability: POSIX-ish Bash/zsh; no external processes, no bashisms. +# ───────────────────────────────────────────────────────────────────────────── +classify_ci_cancel() { + local conclusion="$1" + local has_successor="$2" + local duration_sec="$3" + local timeout_sec="$4" + + # Not a cancel → the existing failure/success path owns it. + if [[ "$conclusion" != "cancelled" ]]; then + CI_CANCEL_CLASS="not-cancel" + printf '%s' "$CI_CANCEL_CLASS" + return 0 + fi + + # A newer run for the same workflow+ref superseded this one → routine cancel. + if [[ "$has_successor" == "yes" ]]; then + CI_CANCEL_CLASS="supersede-cancel" + printf '%s' "$CI_CANCEL_CLASS" + return 0 + fi + + # No successor. Decide hang vs early cancel by the job's own timeout cap. + # Fail-open: any unreadable input surfaces as a hang rather than hiding it. + if [[ "$duration_sec" =~ ^[0-9]+$ ]] && [[ "$timeout_sec" =~ ^[0-9]+$ ]]; then + if [[ "$duration_sec" -ge "$timeout_sec" ]]; then + CI_CANCEL_CLASS="hang-cancel" + else + CI_CANCEL_CLASS="manual-cancel" + fi + else + CI_CANCEL_CLASS="hang-cancel" + fi + + printf '%s' "$CI_CANCEL_CLASS" + return 0 +} + # ───────────────────────────────────────────────────────────────────────────── # prune_rotated_logs LOGS_DIR [KEEP_PER_CATEGORY] #