From e56e2dadfa205d8d59f87c5f4f32482997efc8f1 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Wed, 30 Sep 2026 09:19:24 -0400 Subject: [PATCH] fix(ci-status): wait for the re-run of a failed full run before carrying its failure In carry-forward mode a settled failure or error ended the wait at once, so a contract-only event during a re-run of the failed full run read the stale failure and went red. The wait now holds a failure open while the run its target_url names is in flight with an attempt started after the status was created. Only a full run writes the status, so contract-only siblings on a failed SHA still stop at once. A success still ends the wait immediately, and every path stays fail-closed. Every carry-forward red now also says to re-run this run once ci-lanes is success, where a new commit would re-run every lane. Closes #646 Co-Authored-By: Claude Opus 5.5 --- .github/actions/ci-status/action.yml | 17 ++-- .github/actions/ci-status/run.sh | 87 +++++++++++++++----- .github/actions/ci-status/run.test.sh | 109 +++++++++++++++++++++++++- README.md | 36 ++++++--- 4 files changed, 211 insertions(+), 38 deletions(-) diff --git a/.github/actions/ci-status/action.yml b/.github/actions/ci-status/action.yml index 9578396..4407c2d 100644 --- a/.github/actions/ci-status/action.yml +++ b/.github/actions/ci-status/action.yml @@ -55,8 +55,11 @@ inputs: Ceiling, in seconds, on the carry-forward wait. In carry-forward mode this action polls every 15 seconds. Each poll lists the runs of this same workflow on this same head SHA that are still incomplete, excluding this - run, and then reads the `status-context` status list. A `success` or - `failure` is a settled verdict and ends the poll; otherwise the run stops + run, and then reads the `status-context` status list. A `success` is a + settled verdict and ends the poll. A `failure` or `error` ends it too, + unless the full run that wrote it (the run its `target_url` names) is + re-running with an attempt that started after the status was created; + then the poll waits on that run alone. Otherwise the run stops when nothing is in flight, which is the immediate fail on an absent status that this action has always had when no full run is coming, and sleeps when something is. @@ -72,10 +75,12 @@ inputs: status and flips to `completed` moments later, and the other order lets that completion land between the two calls and report both no status and nothing in flight. - Reaching the ceiling fails the run; there is no pass-on-timeout path. The - one case that runs to the ceiling is two or more contract-only runs on a - SHA with no full run in flight to write a verdict for them, and it fails - closed. `0` disables the wait and goes straight to a single status read. + Reaching the ceiling fails the run; there is no pass-on-timeout path. Two + cases run to the ceiling and fail closed: two or more contract-only runs + on a SHA with no status and no full run in flight to write one, and a + writer re-run slower than the ceiling. Every red names both remedies: + re-run the full workflow, or, once the status is `success`, re-run this + run. `0` disables the wait and goes straight to a single status read. The calling job needs `actions: read`: under an explicit `permissions:` block the default is none, and a 403 warns naming the scope and degrades to the status read alone. diff --git a/.github/actions/ci-status/run.sh b/.github/actions/ci-status/run.sh index f17bc07..70a774d 100755 --- a/.github/actions/ci-status/run.sh +++ b/.github/actions/ci-status/run.sh @@ -24,8 +24,11 @@ # 1. List the runs of this same workflow on this same head SHA that are still # incomplete, excluding this run. Any run id; see below. # 2. Read the newest `github-actions[bot]` status for `status-context`. A -# `success` or `failure` is a settled verdict, so stop polling and apply -# it under the usual rules. +# `success` is a settled verdict, so stop polling and apply it. A +# `failure` or `error` is settled too, unless the full run that wrote it +# (the run its `target_url` names) is in flight again with a current +# attempt that started after the status was created: that is a re-run +# about to replace the verdict, so the wait narrows to that one run. # 3. Otherwise, if step 1 found nothing in flight, stop: no run that could # still write a verdict exists, and an absent status fails the run as it # always has. If something IS in flight, sleep and poll again. @@ -36,6 +39,15 @@ # breaks a mutual wait: once the full run writes its verdict, both contract-only # runs read it and stop. # +# Only the writer's re-run holds a settled failure open. Only a full run writes +# the status, so a contract-only sibling is never that run, and two contract-only +# runs on a failed SHA still stop at once instead of waiting on each other. A +# `run_attempt > 1` test would also catch a re-run contract-only run and bring +# that mutual wait back. The start-time test keeps the writer's own first +# attempt, which writes the status a moment before it completes, out of the +# wait. The known limit: a re-run of a DIFFERENT full run on the same SHA does +# not hold a failure open, and the run fails at once as it did before. +# # Step 1 before step 2 WITHIN a poll. A sibling writes the status and flips to # `completed` a moment later. Listing after reading would let that completion # land in the gap between the two calls and report both "no status" and "nothing @@ -43,16 +55,20 @@ # gap, because a status written before the listing is still read after it. # # The loop never turns a verdict green. Reaching the ceiling exits 1 rather than -# passing on an unsettled status; there is no pass-on-timeout path. The one case -# that runs to the ceiling is two or more contract-only runs on a SHA with no -# full run in flight to write a verdict for them: they wait on each other and -# then both fail closed. +# passing on an unsettled status; there is no pass-on-timeout path. Two cases +# run to the ceiling: two or more contract-only runs on a SHA with no status and +# no full run in flight to write one (they wait on each other and then both fail +# closed), and a writer re-run that outlasts the ceiling. # -# Reading a settled status before the wait set empties is a deliberate trade: it -# releases the mutual wait above, and it can carry an older `success` forward +# Reading a settled `success` before the wait set empties is a deliberate trade: +# it releases the mutual wait above, and it can carry an older `success` forward # while a re-run of the same SHA is in flight to overwrite it. The defenses # against a forged status (context, creator login, Bot type, newest id) hold. # +# Every red names both remedies. The run cannot see a later verdict, so the +# message says to re-run the full workflow, and to re-run this run instead once +# `status-context` on the SHA is `success`. +# # `same-repo` false is a fork pull request. Its token is read-only on # `pull_request` whatever `permissions:` requests, so it cannot record lane # state; it aggregates, reports the lanes verdict, and writes nothing. The @@ -193,12 +209,20 @@ require_pattern carry-forward-wait-seconds "$CARRY_FORWARD_WAIT_SECONDS" '^[0-9] # echoing it so the caller can distinguish "read failed" from "read empty" # through the return status. carried_state="" +# When that entry was created, and the run id its `target_url` names (the full +# run that wrote it). Empty when absent; either one empty means the wait never +# treats a run as the writer's re-run. +carried_created_at="" +carried_writer_run_id="" # Whether `carried_state` holds a completed read. The poll loop reads the status # itself, so the caller below must not read a second time and overwrite what the # loop decided on. carried_state_read=false read_carried_state() { + local entry carried_state="" + carried_created_at="" + carried_writer_run_id="" carried_state_read=false # The LIST endpoint, not the combined one: `commits//status` collapses to # one entry per context and exposes no author, so any collaborator with write @@ -221,9 +245,13 @@ read_carried_state() { # explicit check a malformed or truncated body would make jq exit nonzero, # that status would be discarded, and the function would report a completed # read of an empty state. A body this function cannot parse is a failed read. - carried_state="$(jq -r --arg context "$STATUS_CONTEXT" --arg creator "$STATUS_CREATOR" \ - '[ .[] | select(.context == $context and (.creator.login // "") == $creator and (.creator.type // "") == "Bot") ] | (max_by(.id).state // "")' \ + # One pass, `|`-joined: tab is IFS whitespace, so `read` would collapse an + # empty middle field and shift the ones after it. + entry="$(jq -r --arg context "$STATUS_CONTEXT" --arg creator "$STATUS_CREATOR" \ + '[ .[] | select(.context == $context and (.creator.login // "") == $creator and (.creator.type // "") == "Bot") ] | (max_by(.id) // {}) + | [(.state // ""), (.created_at // ""), ((.target_url // "") | capture("/actions/runs/(?[0-9]+)").id // "")] | join("|")' \ <"$gh_stdout")" || return 1 + IFS='|' read -r carried_state carried_created_at carried_writer_run_id <<<"$entry" carried_state_read=true } @@ -307,6 +335,9 @@ wait_for_sibling_runs() { ids="$(jq -r --argjson incomplete "$INCOMPLETE_RUN_STATUSES" --argjson self "$run_id" \ '[ .workflow_runs[]? | select(.status as $s | $incomplete | index($s)) | select((.id // $self) != $self) | .id ] | sort | join(" ")' \ <"$gh_stdout")" + # Kept for the settled-failure check below, which runs after the status + # read has overwritten gh_stdout. + cp -- "$gh_stdout" "$scratch/runs.json" # The status read comes AFTER the listing above, never before it: a sibling # writes the status and flips to `completed` moments later, and the other # order lets that completion land between the two calls. @@ -329,11 +360,19 @@ wait_for_sibling_runs() { cat "$gh_stderr" >&2 break fi - # A settled verdict ends the wait whatever is still in flight. This is what - # releases two contract-only runs that would otherwise wait on each other. - # `error` is settled alongside `failure`: both are terminal states the API - # accepts, and neither passes below, so waiting on one only delays a red. - if [[ "$carried_state" == success || "$carried_state" == failure || "$carried_state" == error ]]; then + # A failure is stale while the full run that wrote it is re-running, so the + # wait narrows to that one run; see the header for why only the writer. + # `error` goes with `failure`: both are terminal states the API accepts, and + # neither passes below. + if [[ "$carried_state" == failure || "$carried_state" == error ]]; then + ids="$(jq -r --argjson incomplete "$INCOMPLETE_RUN_STATUSES" --arg writer "$carried_writer_run_id" --arg since "$carried_created_at" \ + '[ .workflow_runs[]? | select(.status as $s | $incomplete | index($s)) | select($since != "" and (.id | tostring) == $writer and (.run_started_at // "") > $since) | .id ] | join(" ")' \ + <"$scratch/runs.json")" + fi + # A settled verdict with no writer re-run in flight ends the wait whatever + # else is in flight. This is what releases two contract-only runs that + # would otherwise wait on each other. + if [[ "$carried_state" == success || ("$carried_state" =~ ^(failure|error)$ && -z "$ids") ]]; then if [[ "$carry_forward_waited" -gt 0 ]]; then echo "The ${STATUS_CONTEXT} status on ${SHA} settled after ${carry_forward_waited}s." fi @@ -366,6 +405,15 @@ wait_for_sibling_runs() { set_wait_note "$all_ids" } +# Every carry-forward red. The run cannot see a verdict recorded after it, so +# the message names both remedies: a failed or missing full run needs the full +# workflow again, and once the status is `success` re-running this run is +# enough, where a new commit would re-run every lane. +fail_carry_forward() { + echo "::error::no successful ${STATUS_CONTEXT} status on ${SHA}; re-run the full workflow${carry_forward_wait_note}. Once ${STATUS_CONTEXT} on ${SHA} is success, re-run this run instead." + exit 1 +} + if [[ "$contract_only" == true ]]; then echo "Contract-only event: reading the ${STATUS_CONTEXT} status on ${SHA} instead of aggregating skipped lanes." if [[ "$CARRY_FORWARD_WAIT_SECONDS" -gt 0 ]]; then @@ -375,8 +423,7 @@ if [[ "$contract_only" == true ]]; then if [[ "$carry_forward_wait_status" -ne 0 ]]; then # Never pass on timeout: a run that could still write this SHA's verdict # is in flight, so whatever is on the SHA right now is not settled. - echo "::error::no successful ${STATUS_CONTEXT} status on ${SHA}; re-run the full workflow${carry_forward_wait_note}" - exit 1 + fail_carry_forward fi fi # Only when the loop did not read it: a failed Actions read or a mid-loop @@ -385,16 +432,14 @@ if [[ "$contract_only" == true ]]; then # shellcheck disable=SC2310 # read_carried_state handles its own errexit; the caller classifies the status. if ! read_carried_state; then cat "$gh_stderr" >&2 - echo "::error::no successful ${STATUS_CONTEXT} status on ${SHA}; re-run the full workflow${carry_forward_wait_note}" - exit 1 + fail_carry_forward fi fi if [[ "$carried_state" == success ]]; then echo "Carried forward: ${STATUS_CONTEXT} is success on ${SHA} (recorded by ${STATUS_CREATOR})." exit 0 fi - echo "::error::no successful ${STATUS_CONTEXT} status on ${SHA}; re-run the full workflow${carry_forward_wait_note}" - exit 1 + fail_carry_forward fi # --------------------------------------------------------------------------- diff --git a/.github/actions/ci-status/run.test.sh b/.github/actions/ci-status/run.test.sh index 5dd9c62..39e7fa5 100755 --- a/.github/actions/ci-status/run.test.sh +++ b/.github/actions/ci-status/run.test.sh @@ -266,6 +266,14 @@ user_status() { printf '{"id":%s,"context":"ci-lanes","state":"%s","creator":{"login":"a-collaborator","type":"User"}}' "$1" "$2" } +# writer_status +# A bot status as a full run records it: `target_url` names the run that wrote +# it, which is how the wait recognizes that run's re-run. +writer_status() { + printf '{"id":%s,"context":"ci-lanes","state":"%s","created_at":"%s","target_url":"https://github.com/%s/actions/runs/%s","creator":{"login":"github-actions[bot]","type":"Bot"}}' \ + "$1" "$2" "$3" "$repository" "$4" +} + # --- carry-forward wait fixtures ------------------------------------------- statuses_key="GET_repos_melodic-software_ci-workflows_commits_${sha}_statuses" @@ -313,6 +321,13 @@ run_entry() { printf '{"id":%s,"status":"%s","created_at":"%s"}' "$1" "$2" "$3" } +# attempt_entry +# `run_started_at` is when the CURRENT attempt started; a re-run moves it and +# leaves `created_at` alone. +attempt_entry() { + printf '{"id":%s,"status":"%s","created_at":"%s","run_attempt":%s,"run_started_at":"%s"}' "$1" "$2" "$3" "$4" "$5" +} + earlier_full_run="$(run_entry 4000 in_progress 2026-09-05T12:00:00Z)" # --- full mode ------------------------------------------------------------- @@ -588,8 +603,8 @@ run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 expect_log 'Waiting 15s for in-flight run(s) 4000' expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow (waited 15s of 60s on in-flight run(s): 4000)" -# A deliberate trade: a settled verdict ends the wait even with a sibling -# incomplete, so an older verdict can carry forward while a re-run is in flight. +# A settled failure ends the wait even with a sibling incomplete, when that +# sibling is not the writer re-running (this status names no writer at all). # An absent or `pending` status still waits, as the case above shows. echo 'case: a settled status ends the wait even with a sibling still incomplete' clear_status_fixtures @@ -760,6 +775,96 @@ run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 expect_no_log 'Waiting ' expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow" +# --- carry-forward: a stale failure while its writer re-runs ---------------- +# +# Full run 4000 fails and records ci-lanes=failure at 12:00:10Z. Someone re-runs +# it: attempt 2 starts at 12:00:20Z. A contract-only run arrives meanwhile. + +stale_failure="$(writer_status 100 failure 2026-09-05T12:00:10Z 4000)" +writer_rerun="$(attempt_entry 4000 in_progress 2026-09-05T11:59:00Z 2 2026-09-05T12:00:20Z)" + +# Without holding a settled failure open for the writer's re-run, this reads +# the stale failure on the first poll and goes red while the re-run that +# replaces it is still in flight. +echo 'case: a stale failure waits for the in-flight re-run of its writer and carries its success' +clear_status_fixtures +clear_run_fixtures +current_run +status_list_on_call 1 "[${stale_failure}]" +status_list_on_call 2 "[$(writer_status 200 success 2026-09-05T12:04:00Z 4000),${stale_failure}]" +workflow_runs_on_call 1 "[${writer_rerun}]" +workflow_runs_on_call 2 '[]' +run_case 0 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 +expect_log "Waiting 15s for in-flight run(s) 4000 on ${sha} to finish (waited 0s of 60s)." +expect_log "The ci-lanes status on ${sha} settled after 15s." +expect_log "Carried forward: ci-lanes is success on ${sha}" + +# Without the ceiling a re-run slower than it holds the run forever, and +# without "never pass on timeout" it would have to resolve to something. +echo 'case: a writer re-run that outlasts the ceiling fails closed naming it' +clear_status_fixtures +clear_run_fixtures +current_run +status_list "[${stale_failure}]" +workflow_runs "[${writer_rerun}]" +run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=30 +expect_log '::warning::reached the 30s carry-forward-wait-seconds ceiling with in-flight run(s) 4000' +expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow (waited 30s of 30s on in-flight run(s): 4000). Once ci-lanes on ${sha} is success, re-run this run instead." + +# Without the start-time term this waits on the writer's own first attempt, +# which records the status a moment before it completes, and turns a prompt +# red into a wait. +echo 'case: the writer attempt that recorded the failure is not waited on' +clear_status_fixtures +clear_run_fixtures +current_run +status_list "[${stale_failure}]" +workflow_runs "[$(attempt_entry 4000 in_progress 2026-09-05T11:59:00Z 1 2026-09-05T11:59:00Z)]" +run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 +expect_no_log 'Waiting ' +expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow" + +# Without narrowing the wait to the writer, a failed SHA brings back the mutual +# wait: each contract-only run holds the failure open for the other until the +# ceiling. 4300 is itself a re-run started after the failure, so a +# `run_attempt > 1` test in place of the writer test waits on it too. +echo 'case: two contract-only siblings on a failed SHA do not wait on each other' +clear_status_fixtures +clear_run_fixtures +current_run +status_list "[${stale_failure}]" +workflow_runs "[$(attempt_entry 4100 in_progress "$current_run_created_at" 1 "$current_run_created_at"),$(attempt_entry 4300 in_progress 2026-09-05T12:00:00Z 2 2026-09-05T12:01:00Z)]" +run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 +expect_no_log 'Waiting ' +expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow." + +# Without the unsettled-state fail a re-run of the full run that ends without +# recording anything would leave an absent status to pass. No status names no +# writer, so this is the ordinary wait on any sibling, and it still fails. +echo 'case: a missing status still fails after the re-run ends without recording one' +clear_status_fixtures +clear_run_fixtures +current_run +status_list '[]' +workflow_runs_on_call 1 "[${writer_rerun}]" +workflow_runs_on_call 2 '[]' +run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 +expect_log "Waiting 15s for in-flight run(s) 4000 on ${sha} to finish (waited 0s of 60s)." +expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow (waited 15s of 60s on in-flight run(s): 4000)." +expect_no_log 'Carried forward' + +# Without the second remedy the message only ever says to re-run the full +# workflow, and a red that outlived its failure gets a new commit (every lane +# again) where re-running this run is enough. +echo 'case: the failure message names the re-run-this-run remedy for a later success' +clear_status_fixtures +clear_run_fixtures +current_run +status_list "[${stale_failure}]" +workflow_runs '[]' +run_case 1 'skipped skipped' pass true true CARRY_FORWARD_WAIT_SECONDS=60 +expect_log "::error::no successful ci-lanes status on ${sha}; re-run the full workflow. Once ci-lanes on ${sha} is success, re-run this run instead." + # The documented trade, pinned deliberately from the passing side. A recorded # success ends the wait even with a sibling in flight, so a re-run of this SHA # that is about to overwrite it does not hold the run. Without that the loop diff --git a/README.md b/README.md index 7a598b6..f977a65 100644 --- a/README.md +++ b/README.md @@ -237,11 +237,13 @@ consumer to audit it. the `status-context` list, read as above: newest entry by `github-actions[bot]`. - A `success` or `failure` on that read is a settled verdict, so the poll stops - and the verdict applies. Otherwise, if the first call found nothing in flight - the poll stops too, because nothing that could still write a verdict exists - and an absent status fails as it always has. If something is in flight, it - sleeps and polls again. Four properties are load-bearing: + A `success` on that read is a settled verdict, so the poll stops and the + verdict applies. A `failure` or `error` is settled too, unless the full run + that wrote it is being re-run: then the poll waits on that one run (see + below). Otherwise, if the first call found nothing in flight the poll stops + too, because nothing that could still write a verdict exists and an absent + status fails as it always has. If something is in flight, it sleeps and polls + again. Five properties are load-bearing: - **Any in-flight sibling is waited on, at any run id.** Waiting only on lower run ids, as v0.22.1 did, assumed a full run always outranks the @@ -255,7 +257,20 @@ consumer to audit it. a pair waits on nothing. Two contract-only runs on one SHA now wait on each other until the full run writes its verdict, which releases both. With no full run in flight to write one, they run to the ceiling and both fail - closed. That is the only case that waits to the ceiling in normal operation. + closed. + - **Only the writer's re-run holds a failure open.** After a full run fails + and is re-run, a contract-only run that reads the old `failure` waits for + the re-run instead of going red at once. The re-run is recognized as the + run the status's `target_url` names, in flight with a `run_started_at` + later than the status's `created_at`. Only a full run writes the status, so + a contract-only sibling is never that run, and two contract-only runs on a + failed SHA still stop at once. A `run_attempt > 1` test would also match a + re-run contract-only run and bring the mutual wait back. The start-time + test keeps the writer's own first attempt, which writes the status a moment + before it completes, out of the wait. A re-run of a different full run on + the same SHA does not hold the failure open; that run fails at once, as it + did before. A writer re-run slower than the ceiling runs to it and fails + closed. - **Listing before reading, within a poll.** A sibling writes the status and flips to `completed` moments later. Reading first would let that completion land between the two calls and report both no status and nothing in flight, @@ -263,7 +278,10 @@ consumer to audit it. - **The wait never turns a verdict green.** Reaching the ceiling prints `::error::no successful status on ; re-run the full workflow`, extended with how long it waited and on which run ids, and exits 1. There is - no pass-on-timeout path. + no pass-on-timeout path. Every carry-forward red ends with `Once + on is success, re-run this run instead.`: the run cannot see a verdict + recorded after it, and once the lanes pass, re-running the red run replaces + its check run where a new commit would re-run every lane. **The calling job needs `actions: read`.** Under an explicit `permissions:` block the default for every scope is none, so both Actions calls 403 without @@ -271,8 +289,8 @@ consumer to audit it. a single status read, which is the pre-6b contract: loud, and never a pass on an absent status. Any other read failure warns the same way. - **The trade this makes.** Ending the wait on a settled status is what releases - the mutual wait, and it gives up v0.22.1's guard against carrying an older + **The trade this makes.** Ending the wait on a settled `success` is what + releases the mutual wait on a green SHA, and it gives up v0.22.1's guard against carrying an older `success` forward while a re-run of the same SHA is in flight to overwrite it. That guard bound only when a full run had already written a verdict for this exact SHA and another full run was in flight on it again, and it cost a false