From 5565434a51d69d3d98c2b0c6ed62c60dbca10415 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Sat, 10 Oct 2026 22:47:59 -0500 Subject: [PATCH 1/2] fix(dev-lead): a bot-comment retry cancelled before it starts no longer holds the pending window or an attempt (#2262) A fix-bot-comment retry joins the PR's concurrency lane, where GitHub keeps one pending run, so a newer PR event cancels it before a step runs. The sweep only saw its own marker: the cancelled retry stayed "pending" for 150 minutes and used up one of the two attempts. The sweep now records the run each dispatch created on the retry marker (run=, by editing the marker) and reads recorded runs back on later scans: - a run cancelled with no job step run is lost: not pending, and not counted toward BOT_COMMENT_RETRY_MAX_ATTEMPTS. It still counts toward BOT_COMMENT_RETRY_MAX_TOTAL, and the PR's next retry waits out a short backoff, so a lane that keeps cancelling stays bounded; - a run still queued or running holds every comment on the PR, so one comment's retry no longer cancels another's queued run (the PR #2205 sequence); - a run that cannot be paired with its dispatch (none, or more than one candidate) or cannot be read keeps the time-based behaviour. An API error is never read as "cancelled". bcr_retry_decisions stays pure: run states are passed in as data. Markers without run= parse as before, and an older parser reads a marker with run= unchanged. Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 144 ++++- scripts/lib/bot-comment-retry.sh | 187 +++++- .../dev-lead/unit/test_bot_comment_retry.bats | 555 ++++++++++++++++++ tests/dev-lead/unit/test_dev_lead_retry.bats | 47 ++ .../unit/test_intent_bot_comment_retry.bats | 37 ++ 5 files changed, 953 insertions(+), 17 deletions(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 33c912d59..aa304a3cc 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -50,6 +50,12 @@ set -euo pipefail # bot-comment retry marker before re-listing markers # to pick the earliest (default: 5), so a concurrent # scan's marker is visible to the listing +# BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC — seconds to wait before each look +# for the workflow run a bot-comment retry dispatch +# created (default: the claim settle above), so the run +# is visible to the Actions listing (#2262) +# BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES — how many times to look for that run +# before recording none (default: 3) # # Retryable intents: fix-reviews, review-changes, rebase # These intents fetch all needed context (open threads, PR metadata) fresh @@ -78,6 +84,14 @@ set -euo pipefail # dispatches one deduplicated fix-bot-comment pass (lib/bot-comment-retry.sh). # This recovers the run GitHub drops when a burst of PR events supersedes the # pending fix-bot-comment run in the per-PR concurrency lane (#2009). +# The retry's own run joins that lane and can be dropped the same way (#2262). +# So the scan records the run each dispatch created on its retry marker +# (record_bot_comment_retry_run) and reads those runs back on later scans +# (fetch_bot_comment_retry_run_states): a retry cancelled before it started is +# re-dispatched without waiting out the pending window and uses up none of +# BOT_COMMENT_RETRY_MAX_ATTEMPTS, while a retry still queued or running holds +# the PR's other retries. A run that cannot be identified or read keeps the +# time-based behaviour. # # Unreplied bot review threads are retried too (#2046). A trusted reviewer bot's # review thread is processed only by a fix-reviews pass. When that pass is lost, @@ -124,6 +138,13 @@ DISPATCH_DELAY_SEC="${DISPATCH_DELAY_SEC:-30}" DRY_RUN="${DRY_RUN:-false}" BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC="${BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC:-5}" [[ "$BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC" =~ ^[0-9]+$ ]] || BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC=5 +# Both waits let an eventually consistent listing catch up with a write this scan +# just made, so the run lookup (#2262) defaults to the claim settle. +BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC="${BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC:-$BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC}" +[[ "$BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC" =~ ^[0-9]+$ ]] \ + || BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC="$BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC" +BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES="${BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES:-3}" +[[ "$BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES" =~ ^[0-9]+$ ]] || BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES=3 # CI_MARKER_PREFIX / REVIEWS_MARKER_PREFIX / ISSUE_MARKER_PREFIX: one definition # shared with the budget breaker's marker reader (#2221), values unchanged. @@ -834,6 +855,97 @@ dispatch_bot_comment_retry() { fi } +# record_bot_comment_retry_run +# Finds the workflow run the dispatch just created and records it on the retry +# marker as `run=` (#2262), so a later scan can tell a retry that was +# cancelled before it started from one that is queued or running. +# repository_dispatch returns no run id, and a run that has not started exposes +# no payload, so the run is paired by what the API does expose: the dev-lead +# caller stub's repository_dispatch runs, titled with the retry's event type, +# created at/after (bcr_pick_dispatched_run). It is recorded ONLY when +# exactly one such run exists. None after the tries, more than one (another retry +# reached this repo in the same seconds) or any API failure records nothing, and +# the marker keeps the time-based behaviour. +# The marker is EDITED, never re-posted: the caller stub subscribes to +# issue_comment `created` only, so an edit starts no run in the PR's lane. A new +# comment would, and that run would cancel the retry just dispatched. +# Best effort: it never fails the scan. All logging goes to stderr. +record_bot_comment_retry_run() { + local repo="$1" pr_number="$2" marker_id="$3" cid="$4" version="$5" attempt="$6" + local marker_at="$7" since="$8" + local try listing pick count run_id="" + [[ "$marker_id" =~ ^[0-9]+$ ]] || return 0 + for (( try = 1; try <= BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES; try++ )); do + if [ "$BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC" -gt 0 ]; then + sleep "$BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC" + fi + # An unreadable listing is not "no run": stop, and say which read failed. + if ! listing=$(gh api --method GET "repos/${repo}/actions/runs" \ + -f event=repository_dispatch -f "created=>=${since}" -f per_page=100 2>/dev/null) \ + || ! pick=$(bcr_pick_dispatched_run "$listing" "$since") \ + || ! count=$(jq -er '.count | numbers' <<< "$pick" 2>/dev/null); then + echo " ::warning::bot-comment retry: could not read the Actions runs of ${repo} to find the run dispatched for PR ${pr_number} — the retry marker records no run and keeps the time-based pending window" >&2 + return 0 + fi + if [ "$count" -eq 1 ]; then + run_id=$(jq -r '.id // empty' <<< "$pick" 2>/dev/null || true) + break + fi + if [ "$count" -gt 1 ]; then + echo " [warn] bot-comment retry: ${count} ${BCR_RETRY_EVENT_TYPE} runs started in ${repo} since ${since} — cannot tell which is PR ${pr_number}'s, so the retry marker records none" >&2 + return 0 + fi + done + if [[ ! "$run_id" =~ ^[0-9]+$ ]]; then + echo " [warn] bot-comment retry: no ${BCR_RETRY_EVENT_TYPE} run appeared in ${repo} for PR ${pr_number} after ${BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES} look(s) — the retry marker records no run" >&2 + return 0 + fi + if ! gh api --method PATCH "repos/${repo}/issues/comments/${marker_id}" \ + -f body="$(bcr_retry_marker "$cid" "$version" "$attempt" "$marker_at" "$run_id")" >/dev/null 2>&1; then + echo " ::warning::bot-comment retry: could not record Actions run ${run_id} on retry marker ${marker_id} in ${repo} — the marker keeps the time-based pending window" >&2 + return 0 + fi + echo " [run] bot-comment ${cid} on PR ${pr_number}: retry attempt ${attempt} is Actions run ${run_id}" >&2 +} + +# fetch_bot_comment_retry_run_states +# Reads from the Actions API the state of each run a retry marker recorded, and +# echoes the {"": {status, conclusion, started}} map bcr_retry_decisions +# takes (#2262). A cancelled run's jobs are read too: only a run in which no job +# ran a step counts as lost. A run that cannot be read is LEFT OUT of the map and +# named in a ::warning::, so its retry keeps the time-based pending window and +# its attempt — an API error is never read as "cancelled". +# Always returns 0 and echoes a JSON object. All logging goes to stderr. +fetch_bot_comment_retry_run_states() { + local repo="$1" ids_json="$2" out='{}' id run jobs state next + while IFS= read -r id; do + [[ "$id" =~ ^[0-9]+$ ]] || continue + if ! run=$(gh api "repos/${repo}/actions/runs/${id}" 2>/dev/null &2 + continue + fi + jobs="" + if jq -e '.status == "completed" and .conclusion == "cancelled"' <<< "$run" >/dev/null 2>&1; then + if ! jobs=$(gh api "repos/${repo}/actions/runs/${id}/jobs?per_page=100" 2>/dev/null &2 + jobs="" + fi + fi + if ! state=$(bcr_run_state "$id" "$run" "$jobs"); then + echo " ::warning::bot-comment retry: Actions run ${id} in ${repo} did not read back as a repository_dispatch run — its retry keeps the time-based pending window" >&2 + continue + fi + if jq -e '.status == "completed" and .conclusion == "cancelled" and .started == false' \ + <<< "$state" >/dev/null 2>&1; then + echo " [lost] bot-comment retry: Actions run ${id} in ${repo} was cancelled before it started" >&2 + fi + if next=$(jq -ce --arg id "$id" --argjson s "$state" '. + {($id): $s}' <<< "$out" 2>/dev/null); then + out="$next" + fi + done < <(jq -r '.[]? | strings' <<< "$ids_json" 2>/dev/null || true) + printf '%s\n' "$out" +} + # dev_lead_identity: dev-lead's acting login, from its persona manifest — NOT # BOT_USER, which is pr-review's identity when pr-review calls this scan (#2017). # DEV_LEAD_USER overrides; don-petry is the same fail-safe default the dev-lead @@ -879,7 +991,10 @@ bcr_automation_logins() { # where a second dispatch would supersede the first while it is still pending — # the exact loss this scan exists to recover. Remaining comments are picked up on # later scans. Dedup (pending / completed / edited / attempt cap) lives in the -# pure bcr_retry_decisions. Prints only the number of dispatches to stdout; all +# pure bcr_retry_decisions, which is also told what the Actions API says about +# the runs earlier retries recorded on their markers (#2262): a run cancelled +# before it started is not a pending retry, and a run still queued or running +# holds every comment on the PR. Prints only the number of dispatches to stdout; all # other output goes to stderr. Fails closed (0 dispatches) on any read failure. # It never minimizes anything: the pass posts a disposition, which the harness # verifies as usual. @@ -943,9 +1058,21 @@ scan_pr_for_undispositioned_bot_comments() { local automation automation=$(bcr_automation_logins) + local info_patterns now_epoch + info_patterns=$(_maintainer_gate_info_patterns_json) || true + now_epoch=$(get_now_epoch) || true + # #2262: an earlier retry's run can be cancelled in the PR's lane before it + # starts. Read the runs recorded on the retry markers whose state could change + # a decision. Every unreadable step leaves the map without that run, which is + # the time-based behaviour. + local run_ids run_states='{}' + if run_ids=$(bcr_retry_run_ids "$comments" "$trusted" "$info_patterns" "$now_epoch" "$automation") \ + && jq -e 'length > 0' <<< "$run_ids" >/dev/null 2>&1; then + run_states=$(fetch_bot_comment_retry_run_states "$repo" "$run_ids") || run_states='{}' + fi local decisions if ! decisions=$(bcr_retry_decisions "$comments" "$trusted" \ - "$(_maintainer_gate_info_patterns_json)" "$(get_now_epoch)" "$automation"); then + "$info_patterns" "$now_epoch" "$automation" "$run_states"); then echo " [warn] bot-comment retry: could not evaluate PR ${pr_number} comments — skipping (fail closed)" >&2 echo "0"; return 0 fi @@ -1064,12 +1191,25 @@ scan_pr_for_undispositioned_bot_comments() { echo "0"; return 0 fi fi + # The run this dispatch creates cannot predate it. Two seconds of allowance + # cover a runner clock slightly ahead of GitHub's (run times have 1 s + # resolution); a wider window would only add other PRs' runs as candidates. + local dispatch_since="" + dispatch_since=$(date -u -d "@$(( $(get_now_epoch) - 2 ))" +%Y-%m-%dT%H:%M:%SZ 2>/dev/null) || dispatch_since="" if ! dispatch_bot_comment_retry "$repo" "$pr_number" "$head_sha" "$cid"; then # Nothing was dispatched, so withdraw the marker — left in place it would read # as a pending retry and block the next attempt for the whole pending window. withdraw_bot_comment_retry_marker "$repo" "$marker_id" echo "0"; return 0 fi + if [ "$DRY_RUN" != "true" ]; then + if [ -n "$dispatch_since" ]; then + record_bot_comment_retry_run "$repo" "$pr_number" "$marker_id" "$cid" "$version" \ + "$attempt" "$now_iso" "$dispatch_since" || true + else + echo " ::warning::bot-comment retry: could not compute the dispatch time for PR ${pr_number} in ${repo} — the retry marker records no run and keeps the time-based pending window" >&2 + fi + fi echo "1" } diff --git a/scripts/lib/bot-comment-retry.sh b/scripts/lib/bot-comment-retry.sh index b2560f3b3..1569cb1c8 100644 --- a/scripts/lib/bot-comment-retry.sh +++ b/scripts/lib/bot-comment-retry.sh @@ -31,16 +31,41 @@ # not loop; only an edit re-opens # retry-pending a retry marker for this version is younger than the # pending window → that retry may still be queued or -# running; never duplicate it +# running; never duplicate it. A retry whose recorded +# run was cancelled before it started is LOST and does +# not hold (#2262, see RUN STATES below). A retry on +# this PR whose recorded run is known to be queued or +# running holds every comment: all retries share the +# per-PR lane, and a second dispatch would cancel a +# queued one # rate-limited a fix-bot-comment pass on this PR ended rate-limited # (or blocked) and its reset= time is still ahead # retry-attempts-exhausted this version already had MAX retry attempts since the # last rate-limited end (an attempt that ended rate- -# limited does not count), or MAX_TOTAL in all +# limited does not count, nor does a LOST one), or +# MAX_TOTAL in all — rate-limited and lost included, +# so a lane that keeps cancelling is still bounded # grace-period the comment is too new — the original event-driven # run may still be pending or running +# retry-cancelled-backoff a retry on this PR was lost less than the backoff +# ago: the event that cancelled it has just taken the +# lane's pending slot, and dispatching now would +# cancel that run in turn # (dispatch) otherwise, as attempt # +# RUN STATES (#2262) +# A dispatched retry joins the PR's concurrency lane, where GitHub keeps one +# pending run: any newer PR event cancels it before a step runs. The sweep +# records the run it dispatched on the marker (`run=`) and, on later scans, +# passes what the Actions API says about those runs as the optional +# : {"": {status, conclusion, started}} (bcr_run_state). +# lost status=completed, conclusion=cancelled, started=false (no job ran a +# step). Not pending, and not one of MAX_ATTEMPTS. +# live any status other than completed (queued, pending, in_progress, …). +# Everything else keeps the time-based behaviour: a marker with no `run=`, a run +# that is absent from the map (the API could not be read), a run cancelled after +# it started, a run that completed any other way. Unknown is never read as lost. +# # Only markers posted by our own automation count: the author's login must be # one of (dev-lead's and pr-review's acting identities) # AND carry a trusted association (OWNER/MEMBER/COLLABORATOR). Neither an outside @@ -61,7 +86,11 @@ # BOT_COMMENT_RETRY_MAX_ATTEMPTS retries per comment version since the last # rate-limited end (default 2) # BOT_COMMENT_RETRY_MAX_TOTAL hard ceiling on retries per comment version, -# rate-limited ones included (default 6) +# rate-limited and lost ones included (default 6) +# BOT_COMMENT_RETRY_LOST_BACKOFF_SEC how long after a lost retry's dispatch the +# PR's next retry waits (default: the MIN_AGE +# grace, for the same reason — the run that took +# the lane may still be pending or running) # BOT_COMMENT_RETRY_VERSION_SKEW_SEC tolerance when matching a pass's stamped # version to the comment's version (default 2): # an edited comment's version comes from the @@ -80,8 +109,13 @@ if [ -z "${_MAINTAINER_GATE_AGENT_MARKERS:-}" ]; then fi BCR_RETRY_MARKER_NAME="dev-lead-bot-comment-retry" +# The repository_dispatch the retry sends, and the caller stub that subscribes to +# it in every target repo. bcr_pick_dispatched_run pairs a dispatch with its run +# by these (a repository_dispatch run's title is its event type). +BCR_RETRY_EVENT_TYPE="dev-lead-reviews-retry" +BCR_RETRY_WORKFLOW_PATH=".github/workflows/dev-lead.yml" -# bcr_retry_decisions +# bcr_retry_decisions [run_states_json] # the PR's issue-comment nodes, each {id, author{login, # __typename}, authorAssociation, body, createdAt, # lastEditedAt, isMinimized, minimizedReason} @@ -92,14 +126,42 @@ BCR_RETRY_MARKER_NAME="dev-lead-bot-comment-retry" # current time, unix seconds # the logins our own automation posts markers as # ("don-petry,donpetry-bot"); required — empty fails closed +# optional {"": {status, conclusion, started}} for +# the runs recorded on retry markers (RUN STATES above). +# Omitted, empty or not a JSON object reads as "no run is +# known", which is the time-based behaviour. # Echoes a compact JSON array, oldest comment first, of # {id, login, version, attempt, decision: "dispatch"|"skip", reason} # Returns non-zero (and echoes nothing) when the input cannot be evaluated, so a # caller fails closed rather than dispatching on a misread. bcr_retry_decisions() { + _bcr_evaluate decisions "$@" +} + +# bcr_retry_run_ids +# PURE. Echoes a compact JSON array of the run ids (strings) whose state could +# change a decision, so the sweep reads only those from the Actions API: +# • every trusted retry marker on the PR still inside the pending window (its +# run may be lost, or live and holding the lane), and +# • the markers of a comment version that is out of MAX_ATTEMPTS but under +# MAX_TOTAL (a lost one gives an attempt back). +# Same arguments and failure behaviour as bcr_retry_decisions. +bcr_retry_run_ids() { + _bcr_evaluate run-ids "${1:-}" "${2:-}" "${3:-}" "${4:-}" "${5:-}" +} + +# _bcr_evaluate +# The one evaluation both public functions read, so the run ids the sweep +# fetches are exactly the ones the decision would use. +_bcr_evaluate() { + local mode="$1"; shift local comments="${1:-}" trusted_csv="${2:-}" info_patterns="${3:-}" now_epoch="${4:-}" - local automation_csv="${5:-}" + local automation_csv="${5:-}" run_states="${6:-}" [ -n "$info_patterns" ] || info_patterns='{}' + # An unreadable run-state map is "no run is known" — never a reason to treat a + # retry as lost. + run_states=$(jq -ces 'select(length == 1) | .[0] | objects' <<< "$run_states" 2>/dev/null) \ + || run_states='{}' [[ "$now_epoch" =~ ^[0-9]+$ ]] || return 1 # Without our own logins no marker can be trusted, and an untrusted marker set # would read as "no retry yet" on every scan — fail closed instead. @@ -114,8 +176,13 @@ bcr_retry_decisions() { [[ "$max_attempts" =~ ^[0-9]+$ ]] || max_attempts=2 [[ "$max_total" =~ ^[0-9]+$ ]] || max_total=6 [[ "$skew" =~ ^[0-9]+$ ]] || skew=2 + local lost_backoff="${BOT_COMMENT_RETRY_LOST_BACKOFF_SEC:-$min_age}" + [[ "$lost_backoff" =~ ^[0-9]+$ ]] || lost_backoff="$min_age" printf '%s' "$comments" | jq -ce \ + --arg mode "$mode" \ + --argjson runs "$run_states" \ + --argjson lost_backoff "$lost_backoff" \ --arg markers "$_MAINTAINER_GATE_AGENT_MARKERS" \ --arg trusted "$trusted_csv" \ --arg automation "$automation_csv" \ @@ -163,6 +230,22 @@ bcr_retry_decisions() { # A pending bot-thread retry holds the per-PR lane: a second dispatch would # supersede it. | ([ $notes[] | select(.sibling > 0) | .t ] | max) as $sibling_retry + # Every trusted retry marker on the PR, with what is known about the run it + # recorded (#2262). `lost` and `live` are false unless the run state says so: + # a marker without run=, or a run missing from $runs, is neither. + | [ $notes[] | .t as $t | .retry[] + | (attr("run") | if type == "string" and test("^[0-9]+$") then . else null end) as $rid + | (if $rid == null then null else ($runs[$rid] // null) end) as $st + | {t: $t, id: attr("id"), v: (attr("version") | vepoch), + attempt: ((attr("attempt") // "0") | tonumber? // 0), run: $rid, + lost: (($st | type) == "object" and $st.status == "completed" + and $st.conclusion == "cancelled" and $st.started == false), + live: (($st | type) == "object" and ($st.status | type) == "string" + and $st.status != "completed")} ] as $all_retries + # A retry whose run is known to be queued or running holds the whole PR: the + # lane keeps one pending run, so another dispatch would cancel a queued one. + | ([ $all_retries[] | select(.live) | .t ] | max) as $live_retry + | ([ $all_retries[] | select(.lost) | .t ] | max) as $lost_retry | [ .[] | objects | (.author?.login // "" | tostring | bare) as $l | select($l != "" and ($tb | index($l)) != null) @@ -194,27 +277,99 @@ bcr_retry_decisions() { else ($pv + $skew) >= $vt end) ] | length > 0) as $ran # Retry markers match this version by VALUE (epoch), not by string, so a # timestamp format difference between fetches cannot hide a pending retry. - | [ $notes[] | .t as $t | .retry[] - | select(attr("id") == $id and (attr("version") | vepoch) == $vt) - | {t: $t, attempt: ((attr("attempt") // "0") | tonumber? // 0)} ] as $retries + | [ $all_retries[] | select(.id == $id and .v == $vt) ] as $retries + # Every marker counts here, lost ones too: the attempt number stays + # monotonic (the claim is scoped to it) and MAX_TOTAL bounds a lane that + # keeps cancelling. | ([ $retries[].attempt ] + [ ($retries | length) ] | max) as $attempts - | ([ $retries[] | select($last_hold == null or .t > $last_hold) ] | length) as $counted - | ([ $retries[].t ] | max) as $last_retry - | {id: $id, login: $l, version: $ver, created: .createdAt, attempt: ($attempts + 1)} + # A lost retry did no work: it neither uses up one of MAX_ATTEMPTS nor + # holds the comment as pending. + | ([ $retries[] | select(.lost | not) + | select($last_hold == null or .t > $last_hold) ] | length) as $counted + | ([ $retries[] | select(.lost | not) | .t ] | max) as $last_retry + | {id: $id, login: $l, version: $ver, created: .createdAt, attempt: ($attempts + 1), + # Runs worth reading for this comment (bcr_retry_run_ids): only when a + # lost one could give an attempt back. + runs: (if (($covered or $ran) | not) + and $counted >= $max_attempts and $attempts < $max_total + then [ $retries[].run | select(. != null) ] else [] end)} + ( if $covered then {decision: "skip", reason: "dispositioned"} elif $ran then {decision: "skip", reason: "pass-completed"} elif ($last_retry != null and ($now - $last_retry) < $pending) then {decision: "skip", reason: "retry-pending"} elif ($sibling_retry != null and ($now - $sibling_retry) < $pending) then {decision: "skip", reason: "retry-pending"} + elif ($live_retry != null and ($now - $live_retry) < $pending) + then {decision: "skip", reason: "retry-pending"} elif ($hold_until != null and $hold_until > $now) then {decision: "skip", reason: "rate-limited"} elif ($counted >= $max_attempts or $attempts >= $max_total) then {decision: "skip", reason: "retry-attempts-exhausted"} elif ($now - $vt) < $min_age then {decision: "skip", reason: "grace-period"} + elif ($lost_retry != null and ($now - $lost_retry) < $lost_backoff) + then {decision: "skip", reason: "retry-cancelled-backoff"} else {decision: "dispatch", reason: "undispositioned"} end ) ] - | sort_by(.created) | map(del(.created)) + | sort_by(.created) + | if $mode == "run-ids" + then ([ $all_retries[] | select(.run != null and ($now - .t) < $pending) | .run ] + + [ .[].runs[] ] | unique) + else map(del(.created, .runs)) + end + ' 2>/dev/null +} + +# bcr_run_state [jobs_json] +# PURE. Reduce the Actions API's answers about one run to the state +# bcr_retry_decisions reads: {status, conclusion, started}. +# GET /repos/{repo}/actions/runs/{id} +# GET /repos/{repo}/actions/runs/{id}/jobs — only needed for a +# cancelled run; pass "" when it was not (or could not be) read +# `started` is false only when the job list is complete and no job ran a step +# or ended any way but cancelled/skipped, true when one did, and null when the +# job list is missing, unreadable or truncated. Returns non-zero (echoing +# nothing) unless is the repository_dispatch run — the +# caller then leaves the run out of the map (unknown). +bcr_run_state() { + local run_id="${1:-}" run="${2:-}" jobs="${3:-}" + [[ "$run_id" =~ ^[0-9]+$ ]] || return 1 + jobs=$(jq -ces 'select(length == 1) | .[0] | objects' <<< "$jobs" 2>/dev/null) || jobs='null' + printf '%s' "$run" | jq -ce --arg id "$run_id" --argjson jobs "$jobs" ' + select(type == "object" and (.id | tostring) == $id + and .event == "repository_dispatch" and (.status | type) == "string") + | {status, conclusion: (.conclusion // null), + started: (if ($jobs | type) == "object" and ($jobs.jobs | type) == "array" + and ($jobs.total_count | type) == "number" + and $jobs.total_count == ($jobs.jobs | length) + then any($jobs.jobs[]; + ((.steps // []) | length) > 0 + or ((.conclusion // "cancelled") as $c + | ["cancelled", "skipped"] | index($c) == null)) + else null end)} + ' 2>/dev/null +} + +# bcr_pick_dispatched_run +# PURE. From GET /repos/{repo}/actions/runs (event=repository_dispatch), count +# the runs a retry dispatch sent at/after could have created: the +# dev-lead caller stub's, titled with the retry's event type. Echoes +# {count, id} — id is the run id (a string) only when count is exactly 1, since +# a run that has not started exposes no payload to tell two candidates apart. +# Returns non-zero (echoing nothing) when the listing is unreadable or truncated. +bcr_pick_dispatched_run() { + local listing="${1:-}" since="${2:-}" + printf '%s' "$listing" | jq -ce --arg since "$since" \ + --arg title "$BCR_RETRY_EVENT_TYPE" --arg path "$BCR_RETRY_WORKFLOW_PATH" ' + def epoch: (sub("\\.[0-9]+Z$"; "Z") | fromdateiso8601); + ($since | epoch) as $s + | select(type == "object" and (.workflow_runs | type) == "array" + and (.total_count | type) == "number" + and .total_count <= (.workflow_runs | length)) + | [ .workflow_runs[] | objects + | select(.event == "repository_dispatch" and .display_title == $title and .path == $path) + | select((.id | type) == "number" and (.created_at | type) == "string") + | select((.created_at | epoch) >= $s) | .id ] as $ids + | {count: ($ids | length), id: (if ($ids | length) == 1 then ($ids[0] | tostring) else null end)} ' 2>/dev/null } @@ -297,10 +452,12 @@ _bcr_fetch_pr_comments_into() { echo "${warn} could not merge comment pages" >&2; return 1; } } -# bcr_retry_marker +# bcr_retry_marker [run_id] # The hidden dedup marker the sweep posts BEFORE dispatching. It carries the # `dev-lead` prefix, so the maintainer-comment gate never counts it as a finding. +# is appended as a trailing `run=` once the dispatched run is known +# (#2262); every reader treats a marker without it as before. bcr_retry_marker() { - printf '' \ - "$BCR_RETRY_MARKER_NAME" "$1" "$2" "$3" "$4" + printf '' \ + "$BCR_RETRY_MARKER_NAME" "$1" "$2" "$3" "$4" "${5:+ run=$5}" } diff --git a/tests/dev-lead/unit/test_bot_comment_retry.bats b/tests/dev-lead/unit/test_bot_comment_retry.bats index a68bed70c..de96f9121 100644 --- a/tests/dev-lead/unit/test_bot_comment_retry.bats +++ b/tests/dev-lead/unit/test_bot_comment_retry.bats @@ -906,3 +906,558 @@ GHEOF [ "$status" -eq 0 ] [[ "$output" == *scan_pr_for_undispositioned_bot_comments* ]] } + +# ── #2262: a retry cancelled before it starts ──────────────────────────────── +# The retry's own run joins the PR's concurrency lane, where a newer event +# cancels it before a step runs. The sweep records the run it dispatched on the +# marker (run=) and passes what the Actions API says about recorded runs to +# the pure decision. NOW is 2026-10-02T01:00:00Z; the pending window is 150 min +# and the lost-retry backoff defaults to the 15-min grace. + +RUN_LOST='{"status":"completed","conclusion":"cancelled","started":false}' +RUN_QUEUED='{"status":"queued","conclusion":null,"started":null}' +RUN_RUNNING='{"status":"in_progress","conclusion":null,"started":null}' + +# _retry [run id] [association] [login] +# A retry marker for the default comment version, from our automation unless +# an association/login says otherwise. +_retry() { + _ours "" \ + "$3" "${5:-OWNER}" "${6:-don-petry}" +} + +# _decide_runs … — the decision, given run states. +_decide_runs() { + local states="$1"; shift + bcr_retry_decisions "$(jq -sc '.' <<< "$*")" "$TRUSTED" "$INFO" "$NOW_EPOCH" "$AUTOMATION" "$states" +} + +# _run_ids … — the run ids the sweep would read. +_run_ids() { + bcr_retry_run_ids "$(jq -sc '.' <<< "$*")" "$TRUSTED" "$INFO" "$NOW_EPOCH" "$AUTOMATION" +} + +@test "bcr(#2262): a retry whose run was cancelled before it started re-dispatches without waiting out the pending window" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + # Posted 30 min ago: well inside the 150-min pending window. + m="$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)" + out="$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$m")" + [ "$(_dispatches "$out")" = "1" ] + # The attempt number stays monotonic: the sweep's claim is scoped to it. + [ "$(jq -r '.[0].attempt' <<< "$out")" = "2" ] + # Without the run state the same marker is still a pending retry. + out="$(_decide "$cr" "$m")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] +} + +@test "bcr(#2262): cancelled retries do not exhaust BOT_COMMENT_RETRY_MAX_ATTEMPTS" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + # Two retries (MAX_ATTEMPTS is 2), both cancelled before they started. + out="$(_decide_runs "{\"501\":${RUN_LOST},\"502\":${RUN_LOST}}" "$cr" \ + "$(_retry IC_cr 1 2026-10-01T20:00:00Z 501)" \ + "$(_retry IC_cr 2 2026-10-01T21:00:00Z 502)")" + [ "$(_dispatches "$out")" = "1" ] + [ "$(jq -r '.[0].attempt' <<< "$out")" = "3" ] + # A lost retry gives back only its own attempt. Lost, ran, lost: one attempt used. + out="$(_decide_runs "{\"501\":${RUN_LOST},\"503\":${RUN_LOST}}" "$cr" \ + "$(_retry IC_cr 1 2026-10-01T19:00:00Z 501)" \ + "$(_retry IC_cr 2 2026-10-01T20:00:00Z 502)" \ + "$(_retry IC_cr 3 2026-10-01T21:00:00Z 503)")" + [ "$(_dispatches "$out")" = "1" ] + [ "$(jq -r '.[0].attempt' <<< "$out")" = "4" ] + # Lost, ran, lost, ran: both attempts used. + out="$(_decide_runs "{\"501\":${RUN_LOST},\"503\":${RUN_LOST}}" "$cr" \ + "$(_retry IC_cr 1 2026-10-01T19:00:00Z 501)" \ + "$(_retry IC_cr 2 2026-10-01T20:00:00Z 502)" \ + "$(_retry IC_cr 3 2026-10-01T21:00:00Z 503)" \ + "$(_retry IC_cr 4 2026-10-01T22:00:00Z 504)")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-attempts-exhausted" ] +} + +@test "bcr(#2262): the hard ceiling still stops a lane that keeps cancelling" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + states="{\"501\":${RUN_LOST},\"502\":${RUN_LOST},\"503\":${RUN_LOST},\"504\":${RUN_LOST},\"505\":${RUN_LOST},\"506\":${RUN_LOST}}" + five=("$(_retry IC_cr 1 2026-10-01T17:00:00Z 501)" "$(_retry IC_cr 2 2026-10-01T18:00:00Z 502)" \ + "$(_retry IC_cr 3 2026-10-01T19:00:00Z 503)" "$(_retry IC_cr 4 2026-10-01T20:00:00Z 504)" \ + "$(_retry IC_cr 5 2026-10-01T21:00:00Z 505)") + # Five cancelled retries: none used an attempt, and the default ceiling is 6. + out="$(_decide_runs "$states" "$cr" "${five[@]}")" + [ "$(_dispatches "$out")" = "1" ] + [ "$(jq -r '.[0].attempt' <<< "$out")" = "6" ] + # The sixth is the last, however it ended: lost retries count toward MAX_TOTAL. + out="$(_decide_runs "$states" "$cr" "${five[@]}" "$(_retry IC_cr 6 2026-10-01T22:00:00Z 506)")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-attempts-exhausted" ] + # The ceiling is the tunable. + export BOT_COMMENT_RETRY_MAX_TOTAL=3 + out="$(_decide_runs "$states" "$cr" "${five[@]:0:2}")" + [ "$(_dispatches "$out")" = "1" ] + out="$(_decide_runs "$states" "$cr" "${five[@]:0:3}")" + [ "$(_reason_for "$out" IC_cr)" = "retry-attempts-exhausted" ] +} + +@test "bcr(#2262): a retry whose run is queued or running is never duplicated, and holds the PR's other comments" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + ca="$(_bot IC_ca codeant-ai 'Nitpicks' 2026-10-01T23:08:08Z)" + m="$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)" + local st + for st in "$RUN_QUEUED" "$RUN_RUNNING" '{"status":"pending"}' '{"status":"waiting"}'; do + out="$(_decide_runs "{\"501\":${st}}" "$cr" "$ca" "$m")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] + # Every retry shares the per-PR lane: dispatching for the other comment would + # cancel the queued run (the PR #2205 sequence). + [ "$(_reason_for "$out" IC_ca)" = "retry-pending" ] + done + # A queued retry for an OLDER version of an edited comment holds too: when it + # runs it re-reads the comment's current body. + edited="$(_bot IC_cr coderabbitai 'Walkthrough v2' 2026-10-01T23:05:43Z 2026-10-02T00:40:00Z)" + out="$(_decide_runs "{\"501\":${RUN_QUEUED}}" "$edited" "$m")" + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] + # The hold is bounded by the pending window, like every pending retry. + out="$(_decide_runs "{\"501\":${RUN_QUEUED}}" "$cr" "$ca" "$(_retry IC_cr 1 2026-10-01T22:00:00Z 501)")" + [ "$(_dispatches "$out")" = "2" ] +} + +@test "bcr(#2262): only a run KNOWN to be cancelled before it started is lost — anything else keeps the pending window" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + m="$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)" + # Control: the known-lost state does release the retry. + [ "$(_dispatches "$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$m")")" = "1" ] + local st + for st in \ + '{}' '' 'not json' '[]' 'null' \ + "{\"999\":${RUN_LOST}}" \ + "{\"501\":${RUN_LOST}} {\"501\":${RUN_LOST}}" \ + '{"501":{"status":"completed","conclusion":"cancelled","started":null}}' \ + '{"501":{"status":"completed","conclusion":"cancelled","started":true}}' \ + '{"501":{"status":"completed","conclusion":"cancelled"}}' \ + '{"501":{"status":"completed","conclusion":"cancelled","started":"false"}}' \ + '{"501":{"status":"completed","conclusion":"failure","started":false}}' \ + '{"501":{"status":"completed","conclusion":"success","started":null}}' \ + '{"501":{"status":"completed","conclusion":"skipped","started":false}}' \ + '{"501":{"conclusion":"cancelled","started":false}}' \ + '{"501":null}' '{"501":"cancelled"}' '{"501":[]}'; do + out="$(_decide_runs "$st" "$cr" "$m")" || { echo "state <$st> failed the decision"; return 1; } + [ "$(_dispatches "$out")" = "0" ] || { echo "state <$st> dispatched: $out"; return 1; } + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] || { echo "state <$st>: $out"; return 1; } + done + # A marker whose run= is not a run id is a marker without a run. + bad="$(_ours '' 2026-10-02T00:30:00Z)" + out="$(_decide_runs "{\"abc\":${RUN_LOST}}" "$cr" "$bad")" + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] + [ "$(_run_ids "$cr" "$bad")" = "[]" ] +} + +@test "bcr(#2262): a retry lost moments ago backs off before the PR's next retry" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + ca="$(_bot IC_ca codeant-ai 'Nitpicks' 2026-10-01T23:08:08Z)" + # Dispatched 10 min ago and already cancelled: whatever cancelled it has just + # taken the lane's pending slot, and a dispatch now would cancel that in turn. + m="$(_retry IC_cr 1 2026-10-02T00:50:00Z 501)" + out="$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$ca" "$m")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-cancelled-backoff" ] + [ "$(_reason_for "$out" IC_ca)" = "retry-cancelled-backoff" ] + # Past the backoff (it defaults to the 15-min grace) both are dispatchable. + out="$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$ca" "$(_retry IC_cr 1 2026-10-02T00:44:00Z 501)")" + [ "$(_dispatches "$out")" = "2" ] + # The backoff is tunable; 0 turns it off. + export BOT_COMMENT_RETRY_LOST_BACKOFF_SEC=0 + out="$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$ca" "$m")" + [ "$(_dispatches "$out")" = "2" ] + [ "$(jq -r '.[0].attempt' <<< "$out")" = "2" ] +} + +@test "bcr(#2262): only our own automation's markers name a run, and bcr_retry_run_ids lists just the runs worth reading" { + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + ours="$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)" + member="$(_retry IC_cr 1 2026-10-02T00:40:00Z 998 MEMBER mallory)" + outsider="$(_retry IC_cr 1 2026-10-02T00:41:00Z 999 NONE don-petry)" + # Markers anyone else posted name no run to read (CWE-863)… + [ "$(_run_ids "$cr" "$ours" "$member" "$outsider")" = '["501"]' ] + # …cannot release our pending retry by naming a cancelled run… + out="$(_decide_runs "{\"998\":${RUN_LOST},\"999\":${RUN_LOST}}" "$cr" "$ours" "$member" "$outsider")" + [ "$(_reason_for "$out" IC_cr)" = "retry-pending" ] + # …and cannot hold the PR by naming a live one. + out="$(_decide_runs "{\"998\":${RUN_QUEUED},\"999\":${RUN_QUEUED}}" "$cr" "$member" "$outsider")" + [ "$(_dispatches "$out")" = "1" ] + + # pr-review's scan posts markers too (donpetry-bot): its run is read. + [ "$(_run_ids "$cr" "$(_retry IC_cr 1 2026-10-02T00:30:00Z 601 MEMBER donpetry-bot)")" = '["601"]' ] + # Outside the pending window a run matters only when a lost one could give an + # attempt back: out of MAX_ATTEMPTS, under MAX_TOTAL. + old1="$(_retry IC_cr 1 2026-10-01T20:00:00Z 701)" + old2="$(_retry IC_cr 2 2026-10-01T21:00:00Z 702)" + [ "$(_run_ids "$cr" "$old1")" = '[]' ] + [ "$(_run_ids "$cr" "$old1" "$old2")" = '["701","702"]' ] + # A dispositioned comment needs no run read. + [ "$(_run_ids "$cr" "$old1" "$old2" "$(_ours ' +ok')")" = '[]' ] + # At MAX_TOTAL nothing can change the answer. + export BOT_COMMENT_RETRY_MAX_TOTAL=2 + [ "$(_run_ids "$cr" "$old1" "$old2")" = '[]' ] + # Unevaluable input fails closed, as the decision does. + run bcr_retry_run_ids '{"not":"an array"}' "$TRUSTED" "$INFO" "$NOW_EPOCH" "$AUTOMATION" + [ "$status" -ne 0 ] + [ -z "$output" ] +} + +@test "bcr_run_state(#2262): only a complete job list with no step run reads as not started" { + run_json='{"id":501,"event":"repository_dispatch","status":"completed","conclusion":"cancelled"}' + # The shape the Actions API returned for the cancelled PR #2205 retries. + unstarted='{"total_count":3,"jobs":[ + {"name":"dev-lead / dispatch","status":"completed","conclusion":"cancelled","steps":[]}, + {"name":"dev-lead / ci-relay","status":"completed","conclusion":"cancelled","steps":[]}, + {"name":"dev-lead / resume","status":"completed","conclusion":"cancelled"}]}' + [ "$(bcr_run_state 501 "$run_json" "$unstarted")" = "$RUN_LOST" ] + # A run superseded before any job was created. + [ "$(bcr_run_state 501 "$run_json" '{"total_count":0,"jobs":[]}' | jq -r '.started')" = "false" ] + # A job that ran a step, or ended any way but cancelled/skipped: it started. + ran='{"total_count":2,"jobs":[ + {"conclusion":"cancelled","steps":[{"name":"Set up job","conclusion":"success"}]}, + {"conclusion":"cancelled","steps":[]}]}' + [ "$(bcr_run_state 501 "$run_json" "$ran" | jq -r '.started')" = "true" ] + [ "$(bcr_run_state 501 "$run_json" '{"total_count":1,"jobs":[{"conclusion":"success","steps":[]}]}' | jq -r '.started')" = "true" ] + # Missing, unreadable or truncated job list: unknown, never "not started". + local j + for j in '' 'not json' '[]' '{"jobs":[]}' '{"total_count":5,"jobs":[{"conclusion":"cancelled","steps":[]}]}'; do + [ "$(bcr_run_state 501 "$run_json" "$j" | jq -r '.started')" = "null" ] || { echo "jobs <$j>"; return 1; } + done + # A live run keeps its status; no conclusion yet. + [ "$(bcr_run_state 501 '{"id":501,"event":"repository_dispatch","status":"queued","conclusion":null}')" \ + = '{"status":"queued","conclusion":null,"started":null}' ] + # Not the run asked for, not a repository_dispatch run, or not a run at all. + run bcr_run_state 502 "$run_json" "$unstarted" + [ "$status" -ne 0 ] + run bcr_run_state 501 '{"id":501,"event":"issue_comment","status":"completed","conclusion":"cancelled"}' "$unstarted" + [ "$status" -ne 0 ] + run bcr_run_state 501 '{"message":"Not Found"}' "$unstarted" + [ "$status" -ne 0 ] + run bcr_run_state abc "$run_json" "$unstarted" + [ "$status" -ne 0 ] +} + +# _listed_run [display_title] [path] [event] — one workflow_runs entry. +_listed_run() { + jq -nc --argjson id "$1" --arg c "$2" --arg t "${3:-dev-lead-reviews-retry}" \ + --arg p "${4:-.github/workflows/dev-lead.yml}" --arg e "${5:-repository_dispatch}" \ + '{id:$id, created_at:$c, display_title:$t, path:$p, event:$e, status:"queued", conclusion:null}' +} + +# _runs_listing … — a GET /actions/runs response holding exactly these runs. +_runs_listing() { + jq -sc '{total_count: length, workflow_runs: .}' <<< "$*" +} + +@test "bcr_pick_dispatched_run(#2262): a dispatch is paired only with exactly one candidate run" { + since=2026-10-02T00:59:58Z + ours="$(_listed_run 9001 2026-10-02T01:00:02Z)" + noise=("$(_listed_run 8000 2026-10-02T00:59:40Z)" \ + "$(_listed_run 9002 2026-10-02T01:00:03Z pr-review-mention .github/workflows/pr-review-trigger.yml)" \ + "$(_listed_run 9003 2026-10-02T01:00:03Z dev-lead-ci-failure)" \ + "$(_listed_run 9004 2026-10-02T01:00:03Z dev-lead-reviews-retry .github/workflows/other.yml)" \ + "$(_listed_run 9005 2026-10-02T01:00:03Z dev-lead-reviews-retry .github/workflows/dev-lead.yml issue_comment)") + # An earlier retry's run, another workflow, another event type: not candidates. + [ "$(bcr_pick_dispatched_run "$(_runs_listing "$ours" "${noise[@]}")" "$since")" = '{"count":1,"id":"9001"}' ] + # A run created in the same second as the dispatch is a candidate. + [ "$(bcr_pick_dispatched_run "$(_runs_listing "$(_listed_run 9001 "$since")")" "$since" | jq -r '.id')" = "9001" ] + # Two candidates cannot be told apart (a run that has not started has no payload). + [ "$(bcr_pick_dispatched_run "$(_runs_listing "$ours" "$(_listed_run 9010 2026-10-02T01:00:04Z)")" "$since")" \ + = '{"count":2,"id":null}' ] + # None yet. + [ "$(bcr_pick_dispatched_run "$(_runs_listing "${noise[@]}")" "$since")" = '{"count":0,"id":null}' ] + # An unreadable or truncated listing is not "no run". + local l + for l in '[]' '' 'not json' '{"message":"Not Found"}' \ + "$(jq -c '.total_count = 250' <<< "$(_runs_listing "$ours")")"; do + run bcr_pick_dispatched_run "$l" "$since" + [ "$status" -ne 0 ] || { echo "listing <$l> was read as an answer: $output"; return 1; } + [ -z "$output" ] + done + run bcr_pick_dispatched_run "$(_runs_listing "$ours")" "not a time" + [ "$status" -ne 0 ] +} + +@test "bcr_retry_marker(#2262): the run id is a trailing attribute, and a marker without one is unchanged" { + [ "$(bcr_retry_marker IC_cr 2026-10-01T23:05:43Z 1 2026-10-02T01:00:00Z)" \ + = '' ] + with_run="$(bcr_retry_marker IC_cr 2026-10-01T23:05:43Z 1 2026-10-02T01:00:00Z 9001)" + [ "$with_run" = '' ] + # The sweep's claim listing and the thread-retry arbitration match on these + # prefixes; the maintainer-comment gate must still see an agent marker. + [[ "$with_run" == *'dev-lead-bot-comment-retry id=IC_cr version=2026-10-01T23:05:43Z attempt=1 '* ]] + [[ "$with_run" == '' "$GH_LOG" + # …after the dispatch… + dispatch_line=$(grep -n '/dispatches' "$GH_LOG" | head -1 | cut -d: -f1) + patch_line=$(grep -n -- '--method PATCH' "$GH_LOG" | head -1 | cut -d: -f1) + [ "$dispatch_line" -lt "$patch_line" ] + # …and never re-posted: a new comment would start a run in the PR's lane and + # cancel the retry just dispatched. + [ "$(grep -c -- '--method POST.*/comments' "$GH_LOG")" -eq 1 ] + [ "$(grep -c '/dispatches' "$GH_LOG")" -eq 1 ] + [[ "$output" == *"[run]"*"9001"* ]] + + # A failed edit is surfaced, and the dispatch still counts. + : > "$GH_LOG" + export PATCH_RC=1 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"::warning::"*"could not record Actions run 9001"* ]] + + # DRY_RUN dispatches nothing, so it looks up and edits nothing. + : > "$GH_LOG" + export DRY_RUN="true" PATCH_RC=0 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + if grep -q -e 'actions/runs' -e '--method PATCH' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi +} + +@test "sweep(#2262): a dispatch that cannot be paired with exactly one run records none" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE RUNS_LISTING + GRAPHQL_RESPONSE="$(_graphql_page "$(_bot IC_cr coderabbitai 'Walkthrough')")" + + # Two retry runs since the dispatch (another PR's retry reached this repo in + # the same seconds): they cannot be told apart. + RUNS_LISTING="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)" "$(_listed_run 9010 2026-10-02T01:00:03Z)")" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"cannot tell which"* ]] + [ "$(_patches)" -eq 0 ] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 1 ] + + # No run yet: it looks BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES times, then gives up. + : > "$GH_LOG" + RUNS_LISTING="$(_runs_listing "$(_listed_run 8000 2026-10-02T00:59:40Z)")" + BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES=2 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 2 ] + [[ "$output" == *"no dev-lead-reviews-retry run appeared"* ]] + [ "$(_patches)" -eq 0 ] + + # An unreadable listing is named, not read as "no run": one look, no edit. + : > "$GH_LOG" + RUNS_LISTING="FAIL" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"::warning::"*"could not read the Actions runs"* ]] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 1 ] + [ "$(_patches)" -eq 0 ] + + # A truncated listing could hide a second candidate: unreadable too. + : > "$GH_LOG" + RUNS_LISTING="$(jq -c '.total_count = 250' <<< "$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")")" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"::warning::"*"could not read the Actions runs"* ]] + [ "$(_patches)" -eq 0 ] +} + +@test "sweep(#2262): a retry cancelled before it started is re-dispatched on the next scan, as the next attempt" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE RUNS_LISTING + # Attempt 1 was dispatched 30 min ago (inside the 150-min pending window) and + # its run was cancelled in the PR's lane before any step ran. + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)")" + _recorded_run 501 completed cancelled + _unstarted_jobs 501 + RUNS_LISTING="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"[lost]"*"run 501"* ]] + [ "$(grep -c '/dispatches' "$GH_LOG")" -eq 1 ] + [ "$(jq -r '.client_payload.comment_node_id' "$GH_LOG.payload")" = "IC_cr" ] + # A fresh attempt-2 marker claims the retry, and its own run is recorded. + grep -qF -- '--method POST repos/petry-projects/.github-private/issues/2009/comments -f body=' "$GH_LOG" + grep -qF -- 'attempt=2 at=2026-10-02T01:00:00Z run=9001 -->' "$GH_LOG" + # The state came from the run and its jobs, read once each. + [ "$(grep -c 'actions/runs/501$' "$GH_LOG")" -eq 1 ] + [ "$(grep -c 'actions/runs/501/jobs' "$GH_LOG")" -eq 1 ] +} + +@test "sweep(#2262): a retry whose run is still queued is not duplicated, and no other comment is dispatched over it" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_bot IC_ca codeant-ai 'Nitpicks' 2026-10-01T23:08:08Z)" \ + "$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)")" + local status_now + for status_now in queued in_progress; do + : > "$GH_LOG" + _recorded_run 501 "$status_now" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "0" ] + # It read the run, and found it live: nothing is posted or dispatched — + # not for this comment, and not for the PR's other comment either. + grep -q 'actions/runs/501$' "$GH_LOG" + [[ "$output" == *"bot-comment IC_cr "*"retry-pending"* ]] + [[ "$output" == *"bot-comment IC_ca "*"retry-pending"* ]] + if grep -q -e '/dispatches' -e '--method POST' -e '--method PATCH' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + # Only a cancelled run's jobs are read. + if grep -q 'actions/runs/501/jobs' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + done +} + +@test "sweep(#2262): a run that cannot be read, or that started before it was cancelled, is never treated as lost" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)")" + + # The run read fails (API error, or a token that cannot read Actions). + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "0" ] + [[ "$output" == *"::warning::"*"could not read Actions run 501"* ]] + [[ "$output" == *"bot-comment IC_cr "*"retry-pending"* ]] + if grep -q '/dispatches' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + + # Cancelled, but its jobs cannot be read: whether it started is unknown. + : > "$GH_LOG" + _recorded_run 501 completed cancelled + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "0" ] + [[ "$output" == *"::warning::"*"could not read the jobs of cancelled Actions run 501"* ]] + if grep -q '/dispatches' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + + # Cancelled after a job ran a step: it did work, so it stays a pending attempt. + : > "$GH_LOG" + printf '%s' '{"total_count":1,"jobs":[{"name":"dev-lead / dispatch","conclusion":"cancelled", + "steps":[{"name":"Set up job","conclusion":"success"}]}]}' > "$MOCK_BIN/jobs_501.json" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "0" ] + grep -q 'actions/runs/501/jobs' "$GH_LOG" + [[ "$output" != *"[lost]"* ]] + [[ "$output" == *"bot-comment IC_cr "*"retry-pending"* ]] + if grep -q '/dispatches' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + + # The id reads back as some other kind of run: not ours to judge. + : > "$GH_LOG" + jq -c '.event = "issue_comment"' "$MOCK_BIN/run_501.json" > "$MOCK_BIN/run_501.tmp" + mv "$MOCK_BIN/run_501.tmp" "$MOCK_BIN/run_501.json" + _unstarted_jobs 501 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "0" ] + [[ "$output" == *"::warning::"*"did not read back as a repository_dispatch run"* ]] + if grep -q '/dispatches' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi +} + +@test "sweep(#2262): a lane that keeps cancelling stops at BOT_COMMENT_RETRY_MAX_TOTAL" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE RUNS_LISTING + export BOT_COMMENT_RETRY_MAX_TOTAL=3 + local n + for n in 501 502 503; do _recorded_run "$n" completed cancelled; _unstarted_jobs "$n"; done + RUNS_LISTING="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + + # Two cancelled retries: the third (and last) is dispatched. + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_retry IC_cr 1 2026-10-01T23:30:00Z 501)" \ + "$(_retry IC_cr 2 2026-10-02T00:00:00Z 502)")" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + grep -q 'dev-lead-bot-comment-retry id=IC_cr version=2026-10-01T23:05:43Z attempt=3 ' "$GH_LOG" + + # Three cancelled retries: the ceiling holds, nothing is posted or dispatched. + : > "$GH_LOG" + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_retry IC_cr 1 2026-10-01T23:30:00Z 501)" \ + "$(_retry IC_cr 2 2026-10-02T00:00:00Z 502)" \ + "$(_retry IC_cr 3 2026-10-02T00:30:00Z 503)")" + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "0" ] + [[ "$output" == *"bot-comment IC_cr "*"retry-attempts-exhausted"* ]] + if grep -q -e '/dispatches' -e '--method POST' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi +} diff --git a/tests/dev-lead/unit/test_dev_lead_retry.bats b/tests/dev-lead/unit/test_dev_lead_retry.bats index afa0e83ad..c8a55731d 100644 --- a/tests/dev-lead/unit/test_dev_lead_retry.bats +++ b/tests/dev-lead/unit/test_dev_lead_retry.bats @@ -438,3 +438,50 @@ _scan_with_markers() { _scan_with_markers '["",""]' [[ "$output" != *"DISPATCH"* ]] } + +# ── #2262: the run a bot-comment retry dispatched ──────────────────────────── +# The decision matrix and the sweep wiring are covered in +# test_bot_comment_retry.bats; these pin the two properties the recording rests on. + +@test "retry(#2262): the retry marker records its run by an EDIT, and the caller stub starts no run on a comment edit" { + local body types + body="$(declare -f record_bot_comment_retry_run)" + [ -n "$body" ] + # The run is recorded on the marker the scan already posted… + [[ "$body" == *'--method PATCH "repos/${repo}/issues/comments/${marker_id}"'* ]] + # …never by posting another comment: an issue_comment `created` event starts a + # dev-lead run in the PR's lane, and that run would cancel the retry just sent. + if grep -q -- '--method POST' <<< "$body"; then echo "$body"; return 1; fi + # The caller stub (copied verbatim into every repo) subscribes to `created` + # only. If it ever subscribes to `edited`, the edit above cancels the retry it + # describes, and the run has to be recorded some other way. + types="$(awk '/^ issue_comment:/ {found=1; next} + found && /^ types:/ {print; exit} + found && /^ [a-z_]+:/ {exit}' "$BATS_TEST_DIRNAME/../../../.github/workflows/dev-lead.yml")" + [ "$(tr -d '[:space:]' <<< "$types")" = "types:[created]" ] +} + +@test "retry(#2262): an Actions run that cannot be read is left out of the run states, never guessed" { + local out="$MOCK_BIN/out" err="$MOCK_BIN/err" + # Every read fails: an empty map (all retries keep the time-based window), rc 0. + gh() { return 1; } + fetch_bot_comment_retry_run_states "petry-projects/.github" '["501","502"]' > "$out" 2> "$err" + [ "$(cat "$out")" = "{}" ] + [ "$(grep -c '::warning::.*could not read Actions run 50[12] ' "$err")" -eq 2 ] + # One readable live run and one unreadable: only the readable one is reported. + gh() { + case "$*" in + *"/actions/runs/501") echo '{"id":501,"event":"repository_dispatch","status":"in_progress","conclusion":null}' ;; + *) return 1 ;; + esac + } + fetch_bot_comment_retry_run_states "petry-projects/.github" '["501","502"]' > "$out" 2> "$err" + [ "$(cat "$out")" = '{"501":{"status":"in_progress","conclusion":null,"started":null}}' ] + grep -q '::warning::.*could not read Actions run 502 ' "$err" + # Ids that are not run ids are never sent to the API. + gh() { echo "CALLED $*" >&2; return 1; } + run fetch_bot_comment_retry_run_states "petry-projects/.github" '["../x","5 01",7,null]' + [ "$status" -eq 0 ] + [[ "$output" != *"CALLED"* ]] + [ "${lines[-1]}" = "{}" ] +} diff --git a/tests/dev-lead/unit/test_intent_bot_comment_retry.bats b/tests/dev-lead/unit/test_intent_bot_comment_retry.bats index 48f3499e2..57e35def0 100644 --- a/tests/dev-lead/unit/test_intent_bot_comment_retry.bats +++ b/tests/dev-lead/unit/test_intent_bot_comment_retry.bats @@ -328,3 +328,40 @@ _resolved_comments() { run bash "$INTENT_SCRIPT" [ "$(_get_env INTENT_TYPE)" = "fix-bot-comment" ] } + +# _marker_comment — a retry marker comment as the sweep leaves it: posted +# by dev-lead's identity, then edited to record the run it dispatched (#2262). +_marker_comment() { + jq -nc --arg b "$1" '{id:"IC_m", author:{login:"don-petry", __typename:"User"}, authorAssociation:"OWNER", + body:$b, createdAt:"2026-10-02T00:30:00Z", lastEditedAt:"2026-10-02T00:30:09Z", + isMinimized:false, minimizedReason:null}' +} + +@test "bot-comment retry(#2262): a retry marker that records its run changes nothing for the dispatched pass" { + # The marker exactly as the sweep writes it once the dispatched run is known. + local marker + marker="$(source "$SCRIPT_DIR/scripts/lib/bot-comment-retry.sh" \ + && bcr_retry_marker IC_cr 2026-10-01T23:05:43Z 1 2026-10-02T00:30:00Z 501)" + [[ "$marker" == *" at=2026-10-02T00:30:00Z run=501 -->" ]] + + # The pass this very marker's dispatch started still routes to fix-bot-comment… + _event IC_cr + _node coderabbitai "Walkthrough" + export COMMENTS_JSON="[${CR_NODE},$(_marker_comment "$marker")]" + run bash "$INTENT_SCRIPT" + [ "$status" -eq 0 ] + [ "$(_get_env INTENT_TYPE)" = "fix-bot-comment" ] + [ "$(_get_env INTENT_REASON)" = "bot-comment-retry-dispatch" ] + # …and the classifier reads no Actions run: only the sweep does. + if grep -q 'actions/runs' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi + + # A disposition still covers the comment with the marker present. + : > "$GITHUB_ENV" + export COMMENTS_JSON="[${CR_NODE},$(_marker_comment "$marker"), + {\"id\":\"IC_d\",\"author\":{\"login\":\"don-petry\",\"__typename\":\"User\"},\"authorAssociation\":\"OWNER\", + \"body\":\"\", + \"createdAt\":\"2026-10-02T00:45:00Z\",\"lastEditedAt\":null,\"isMinimized\":false,\"minimizedReason\":null}]" + run bash "$INTENT_SCRIPT" + [ "$(_get_env INTENT_TYPE)" = "skip" ] + [ "$(_get_env INTENT_REASON)" = "bot-comment-already-dispositioned" ] +} From b816e34ba9ec78455e54e1b7e8d7239b8cc92c5a Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 09:59:17 -0500 Subject: [PATCH 2/2] fix(dev-lead): close the retry-run gaps review found (#2262) - bcr_retry_run_ids reads a retry's run inside the lost-retry backoff as well as the pending window. With a backoff longer than the pending window a lost run past the pending window was never read, so the backoff could not hold. - bcr_run_state no longer counts a job whose only steps were skipped as started. A cancelled step, or one with no conclusion, still counts as run. - record_bot_comment_retry_run records a run only when two looks in a row show the same single candidate. One look could record another PR's retry run that was listed before this dispatch's own. The second look narrows that race; a run of ours listed later than both looks still leaves the other as the only candidate. - BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC (default 1200): past it a dispatch records no run instead of waiting, so lookup waits cannot use up the retry workflow's 30-minute limit. - Tests: 7 new, 5 of which fail on the previous scripts. One runs the pairing and the run-state reader against the Actions API's real answers for PR #2205's cancelled retry (tests/fixtures/bot-comment-retry/). Co-Authored-By: Claude Opus 5.5 --- scripts/dev-lead-retry.sh | 47 ++++- scripts/lib/bot-comment-retry.sh | 20 ++- .../dev-lead/unit/test_bot_comment_retry.bats | 165 ++++++++++++++++++ .../run-38073395880-jobs.json | 23 +++ .../bot-comment-retry/run-38073395880.json | 9 + .../runs-listing-pr2205.json | 23 +++ 6 files changed, 274 insertions(+), 13 deletions(-) create mode 100644 tests/fixtures/bot-comment-retry/run-38073395880-jobs.json create mode 100644 tests/fixtures/bot-comment-retry/run-38073395880.json create mode 100644 tests/fixtures/bot-comment-retry/runs-listing-pr2205.json diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index aa304a3cc..8fe62e329 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -55,7 +55,13 @@ set -euo pipefail # created (default: the claim settle above), so the run # is visible to the Actions listing (#2262) # BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES — how many times to look for that run -# before recording none (default: 3) +# before recording none (default: 3). A run is recorded +# only when two looks in a row show it as the single +# candidate, so fewer than 2 records none +# BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC — once this process has run this +# many seconds, stop looking for dispatched runs +# (default: 1200), so the lookup waits cannot use up +# the workflow's time limit on a busy scan # # Retryable intents: fix-reviews, review-changes, rebase # These intents fetch all needed context (open threads, PR metadata) fresh @@ -145,6 +151,10 @@ BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC="${BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC:- || BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC="$BOT_COMMENT_RETRY_CLAIM_SETTLE_SEC" BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES="${BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES:-3}" [[ "$BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES" =~ ^[0-9]+$ ]] || BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES=3 +# Each lookup waits up to TRIES x WAIT seconds. The retry workflow has a 30-minute +# limit, so past 20 minutes a dispatch records no run instead of waiting. +BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC="${BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC:-1200}" +[[ "$BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC" =~ ^[0-9]+$ ]] || BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC=1200 # CI_MARKER_PREFIX / REVIEWS_MARKER_PREFIX / ISSUE_MARKER_PREFIX: one definition # shared with the budget breaker's marker reader (#2221), values unchanged. @@ -863,9 +873,14 @@ dispatch_bot_comment_retry() { # no payload, so the run is paired by what the API does expose: the dev-lead # caller stub's repository_dispatch runs, titled with the retry's event type, # created at/after (bcr_pick_dispatched_run). It is recorded ONLY when -# exactly one such run exists. None after the tries, more than one (another retry -# reached this repo in the same seconds) or any API failure records nothing, and -# the marker keeps the time-based behaviour. +# two looks in a row show the same single such run: another PR's retry can be +# listed before this dispatch's own run is, and one look would record that run. +# None after the tries, a candidate seen only on the last look, more than one +# (another retry reached this repo in the same seconds) or any API failure +# records nothing, and the marker keeps the time-based behaviour. The second +# look narrows that race, it does not close it: a run of ours that is listed +# later than both looks, or never, still leaves the other run as the only one. +# Past BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC it does not look at all. # The marker is EDITED, never re-posted: the caller stub subscribes to # issue_comment `created` only, so an edit starts no run in the PR's lane. A new # comment would, and that run would cancel the retry just dispatched. @@ -873,8 +888,12 @@ dispatch_bot_comment_retry() { record_bot_comment_retry_run() { local repo="$1" pr_number="$2" marker_id="$3" cid="$4" version="$5" attempt="$6" local marker_at="$7" since="$8" - local try listing pick count run_id="" + local try listing pick count run_id="" candidate="" seen="" [[ "$marker_id" =~ ^[0-9]+$ ]] || return 0 + if [ "$SECONDS" -ge "$BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC" ]; then + echo " [warn] bot-comment retry: this scan has run ${SECONDS}s, past the ${BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC}s limit for run lookups — not waiting for PR ${pr_number}'s retry run in ${repo}, so the retry marker records none" >&2 + return 0 + fi for (( try = 1; try <= BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES; try++ )); do if [ "$BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC" -gt 0 ]; then sleep "$BOT_COMMENT_RETRY_RUN_LOOKUP_WAIT_SEC" @@ -887,15 +906,25 @@ record_bot_comment_retry_run() { echo " ::warning::bot-comment retry: could not read the Actions runs of ${repo} to find the run dispatched for PR ${pr_number} — the retry marker records no run and keeps the time-based pending window" >&2 return 0 fi - if [ "$count" -eq 1 ]; then - run_id=$(jq -r '.id // empty' <<< "$pick" 2>/dev/null || true) - break - fi if [ "$count" -gt 1 ]; then echo " [warn] bot-comment retry: ${count} ${BCR_RETRY_EVENT_TYPE} runs started in ${repo} since ${since} — cannot tell which is PR ${pr_number}'s, so the retry marker records none" >&2 return 0 fi + candidate="" + if [ "$count" -eq 1 ]; then + candidate=$(jq -r '.id // empty' <<< "$pick" 2>/dev/null || true) + fi + # The same single candidate on two looks in a row is the run to record. + if [ -n "$candidate" ] && [ "$candidate" = "$seen" ]; then + run_id="$candidate" + break + fi + seen="$candidate" done + if [[ ! "$run_id" =~ ^[0-9]+$ ]] && [ -n "$seen" ]; then + echo " [warn] bot-comment retry: ${BCR_RETRY_EVENT_TYPE} run ${seen} in ${repo} was the only candidate for PR ${pr_number} on the last of ${BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES} look(s) and could not be confirmed by a second — the retry marker records none" >&2 + return 0 + fi if [[ ! "$run_id" =~ ^[0-9]+$ ]]; then echo " [warn] bot-comment retry: no ${BCR_RETRY_EVENT_TYPE} run appeared in ${repo} for PR ${pr_number} after ${BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES} look(s) — the retry marker records no run" >&2 return 0 diff --git a/scripts/lib/bot-comment-retry.sh b/scripts/lib/bot-comment-retry.sh index 1569cb1c8..c5c1c8d3e 100644 --- a/scripts/lib/bot-comment-retry.sh +++ b/scripts/lib/bot-comment-retry.sh @@ -142,7 +142,9 @@ bcr_retry_decisions() { # PURE. Echoes a compact JSON array of the run ids (strings) whose state could # change a decision, so the sweep reads only those from the Actions API: # • every trusted retry marker on the PR still inside the pending window (its -# run may be lost, or live and holding the lane), and +# run may be lost, or live and holding the lane) or inside the lost-retry +# backoff when that is longer (a lost run there still holds the backoff), +# and # • the markers of a comment version that is out of MAX_ATTEMPTS but under # MAX_TOTAL (a lost one gives an attempt back). # Same arguments and failure behaviour as bcr_retry_decisions. @@ -312,7 +314,10 @@ _bcr_evaluate() { ] | sort_by(.created) | if $mode == "run-ids" - then ([ $all_retries[] | select(.run != null and ($now - .t) < $pending) | .run ] + # The longer of the two windows a run state is read in: a live run holds + # for $pending, and a lost one starts a backoff of $lost_backoff. + then ([ $all_retries[] + | select(.run != null and ($now - .t) < ([$pending, $lost_backoff] | max)) | .run ] + [ .[].runs[] ] | unique) else map(del(.created, .runs)) end @@ -327,7 +332,9 @@ _bcr_evaluate() { # cancelled run; pass "" when it was not (or could not be) read # `started` is false only when the job list is complete and no job ran a step # or ended any way but cancelled/skipped, true when one did, and null when the -# job list is missing, unreadable or truncated. Returns non-zero (echoing +# job list is missing, unreadable or truncated. A step whose conclusion is +# `skipped` did not run; any other listed step counts as run, a cancelled one +# included, since it may have begun. Returns non-zero (echoing # nothing) unless is the repository_dispatch run — the # caller then leaves the run out of the map (unknown). bcr_run_state() { @@ -342,7 +349,12 @@ bcr_run_state() { and ($jobs.total_count | type) == "number" and $jobs.total_count == ($jobs.jobs | length) then any($jobs.jobs[]; - ((.steps // []) | length) > 0 + ((.steps // []) + | if type == "array" + then any(.[]; if type == "object" + then (.conclusion // "") != "skipped" + else true end) + else true end) or ((.conclusion // "cancelled") as $c | ["cancelled", "skipped"] | index($c) == null)) else null end)} diff --git a/tests/dev-lead/unit/test_bot_comment_retry.bats b/tests/dev-lead/unit/test_bot_comment_retry.bats index de96f9121..9d724b430 100644 --- a/tests/dev-lead/unit/test_bot_comment_retry.bats +++ b/tests/dev-lead/unit/test_bot_comment_retry.bats @@ -1461,3 +1461,168 @@ _patches() { grep -c -- '--method PATCH' "$GH_LOG" || true; } [[ "$output" == *"bot-comment IC_cr "*"retry-attempts-exhausted"* ]] if grep -q -e '/dispatches' -e '--method POST' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi } + +# ── #2262 review: backoff window, skipped steps, pairing race, lookup deadline ─ + +@test "bcr(#2262): a lost-retry backoff longer than the pending window still has the retry's run read" { + export BOT_COMMENT_RETRY_PENDING_SEC=1200 BOT_COMMENT_RETRY_LOST_BACKOFF_SEC=3600 + cr="$(_bot IC_cr coderabbitai 'Walkthrough')" + ca="$(_bot IC_ca codeant-ai 'Nitpicks' 2026-10-01T23:08:08Z)" + # Dispatched 30 min ago: past the 20-min pending window, inside the 60-min + # backoff. Unread, its run could not be known lost and the backoff would not hold. + m="$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)" + [ "$(_run_ids "$cr" "$ca" "$m")" = '["501"]' ] + out="$(_decide_runs "{\"501\":${RUN_LOST}}" "$cr" "$ca" "$m")" + [ "$(_dispatches "$out")" = "0" ] + [ "$(_reason_for "$out" IC_cr)" = "retry-cancelled-backoff" ] + [ "$(_reason_for "$out" IC_ca)" = "retry-cancelled-backoff" ] + # Past both windows the run cannot change a decision, so it is not read. + [ "$(_run_ids "$cr" "$ca" "$(_retry IC_cr 1 2026-10-01T23:50:00Z 501)")" = '[]' ] +} + +@test "bcr_run_state(#2262): a job whose only steps were skipped did not start" { + run_json='{"id":501,"event":"repository_dispatch","status":"completed","conclusion":"cancelled"}' + skipped='{"total_count":1,"jobs":[{"conclusion":"cancelled","steps":[ + {"name":"Set up job","status":"completed","conclusion":"skipped"}, + {"name":"Run dev-lead","status":"completed","conclusion":"skipped"}]}]}' + [ "$(bcr_run_state 501 "$run_json" "$skipped")" = "$RUN_LOST" ] + # One step that ran among skipped ones: it started. + mixed='{"total_count":1,"jobs":[{"conclusion":"cancelled","steps":[ + {"name":"Set up job","conclusion":"success"},{"name":"Run dev-lead","conclusion":"skipped"}]}]}' + [ "$(bcr_run_state 501 "$run_json" "$mixed" | jq -r '.started')" = "true" ] + # A cancelled step may have begun, and a step with no conclusion is unknown: + # neither is read as "did not run". + local steps + for steps in '[{"name":"Set up job","conclusion":"cancelled"}]' '[{"name":"Set up job"}]' '["Set up job"]' '"not a list"'; do + [ "$(bcr_run_state 501 "$run_json" "{\"total_count\":1,\"jobs\":[{\"conclusion\":\"cancelled\",\"steps\":${steps}}]}" | jq -r '.started')" = "true" ] \ + || { echo "steps <$steps>"; return 1; } + done +} + +@test "bcr(#2262): the pairing and the run state hold on the Actions API's own answers for PR #2205's cancelled retry" { + # tests/fixtures/bot-comment-retry/ holds what GitHub returned on 2026-10-11 + # for the retry run of 2026-10-10T17:50:45Z, cut down to the fields read here: + # the run listing for event=repository_dispatch around it (the other entry is + # pr-review's dispatch nine seconds later), the run, and its jobs. + local fx="$SCRIPT_DIR/tests/fixtures/bot-comment-retry" + [ "$(bcr_pick_dispatched_run "$(cat "$fx/runs-listing-pr2205.json")" 2026-10-10T17:50:43Z)" \ + = '{"count":1,"id":"38073395880"}' ] + [ "$(bcr_run_state 38073395880 "$(cat "$fx/run-38073395880.json")" "$(cat "$fx/run-38073395880-jobs.json")")" \ + = "$RUN_LOST" ] +} + +# _listing_sequence … — call after _run_gh. The Actions run listing +# answers with each argument in turn, then repeats the last: what a listing that +# catches up between looks returns. +_listing_sequence() { + local n=0 l + for l in "$@"; do + n=$((n + 1)) + printf '%s' "$l" > "$MOCK_BIN/listing_${n}.json" + done + : > "$MOCK_BIN/listing.calls" + cat > "$MOCK_BIN/gh" <<'GHEOF' +#!/usr/bin/env bash +printf '%s\n' "$*" >> "$GH_LOG" +dir="$(dirname "$0")" +args="$*" +case "$args" in + *"/dispatches"*) cat > "$GH_LOG.payload"; exit 0 ;; + *"api graphql"*) printf '%s' "$GRAPHQL_RESPONSE" ;; + *"/actions/runs -f"*) + echo look >> "$dir/listing.calls" + n=$(wc -l < "$dir/listing.calls" | tr -d ' ') + while [ ! -f "$dir/listing_${n}.json" ] && [ "$n" -gt 1 ]; do n=$((n - 1)); done + cat "$dir/listing_${n}.json" ;; + *"--method PATCH"*"/issues/comments/"*) exit 0 ;; + *"--method POST"*"/comments"*) echo "777" ;; + *"comments?per_page"*"dev-lead-bot-comment-retry id="*) echo "777" ;; + *"/pulls/"*) printf '%s' "$PR_JSON" ;; + *) echo "[]" ;; +esac +GHEOF + chmod +x "$MOCK_BIN/gh" +} + +@test "sweep(#2262): another PR's retry run listed before ours is not recorded as ours" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE + GRAPHQL_RESPONSE="$(_graphql_page "$(_bot IC_cr coderabbitai 'Walkthrough')")" + # First look: only another PR's retry run (9010) is listed. Second look: ours + # (9001) has appeared beside it. One look would have recorded 9010. + _listing_sequence \ + "$(_runs_listing "$(_listed_run 9010 2026-10-02T01:00:01Z)")" \ + "$(_runs_listing "$(_listed_run 9010 2026-10-02T01:00:01Z)" "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "1" ] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 2 ] + [[ "$output" == *"cannot tell which"* ]] + [ "$(_patches)" -eq 0 ] +} + +@test "sweep(#2262): a run is recorded only once a second look shows the same single candidate" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE + GRAPHQL_RESPONSE="$(_graphql_page "$(_bot IC_cr coderabbitai 'Walkthrough')")" + local none one + none="$(_runs_listing "$(_listed_run 8000 2026-10-02T00:59:40Z)")" + one="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + + # Seen on the last look only: nothing confirms it, so nothing is recorded. + _listing_sequence "$none" "$one" + BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES=2 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 2 ] + [[ "$output" == *"run 9001"*"could not be confirmed"* ]] + [ "$(_patches)" -eq 0 ] + + # With a look to spare the same candidate is confirmed and recorded. + : > "$GH_LOG" + _listing_sequence "$none" "$one" + BOT_COMMENT_RETRY_RUN_LOOKUP_TRIES=3 + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "${lines[-1]}" = "1" ] + [ "$(grep -c 'actions/runs -f' "$GH_LOG")" -eq 3 ] + grep -qF -- 'attempt=1 at=2026-10-02T01:00:00Z run=9001 -->' "$GH_LOG" + [ "$(_patches)" -eq 1 ] +} + +@test "sweep(#2262): past the lookup deadline a dispatch still counts and records no run" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE RUNS_LISTING + GRAPHQL_RESPONSE="$(_graphql_page "$(_bot IC_cr coderabbitai 'Walkthrough')")" + RUNS_LISTING="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + # The scan has already run longer than the deadline allows. + BOT_COMMENT_RETRY_RUN_LOOKUP_DEADLINE_SEC=0 + + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "1" ] + [ "$(grep -c '/dispatches' "$GH_LOG")" -eq 1 ] + [[ "$output" == *"limit for run lookups"* ]] + if grep -q -e 'actions/runs' -e '--method PATCH' "$GH_LOG"; then cat "$GH_LOG"; return 1; fi +} + +@test "sweep(#2262): a retry whose run was cancelled before any job was created is re-dispatched" { + _setup_sweep + _run_gh + export TRUSTED_BOTS="$TRUSTED" GRAPHQL_RESPONSE RUNS_LISTING + GRAPHQL_RESPONSE="$(_graphql_page \ + "$(_bot IC_cr coderabbitai 'Walkthrough')" \ + "$(_retry IC_cr 1 2026-10-02T00:30:00Z 501)")" + _recorded_run 501 completed cancelled + printf '%s' '{"total_count":0,"jobs":[]}' > "$MOCK_BIN/jobs_501.json" + RUNS_LISTING="$(_runs_listing "$(_listed_run 9001 2026-10-02T01:00:02Z)")" + + run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2009 + [ "$status" -eq 0 ] + [ "${lines[-1]}" = "1" ] + [[ "$output" == *"[lost]"*"run 501"* ]] + grep -q 'dev-lead-bot-comment-retry id=IC_cr version=2026-10-01T23:05:43Z attempt=2 ' "$GH_LOG" +} diff --git a/tests/fixtures/bot-comment-retry/run-38073395880-jobs.json b/tests/fixtures/bot-comment-retry/run-38073395880-jobs.json new file mode 100644 index 000000000..3ebfdd398 --- /dev/null +++ b/tests/fixtures/bot-comment-retry/run-38073395880-jobs.json @@ -0,0 +1,23 @@ +{ + "total_count": 3, + "jobs": [ + { + "name": "dev-lead / dispatch", + "status": "completed", + "conclusion": "cancelled", + "steps": [] + }, + { + "name": "dev-lead / ci-relay", + "status": "completed", + "conclusion": "cancelled", + "steps": [] + }, + { + "name": "dev-lead / resume", + "status": "completed", + "conclusion": "cancelled", + "steps": [] + } + ] +} diff --git a/tests/fixtures/bot-comment-retry/run-38073395880.json b/tests/fixtures/bot-comment-retry/run-38073395880.json new file mode 100644 index 000000000..2247f1df7 --- /dev/null +++ b/tests/fixtures/bot-comment-retry/run-38073395880.json @@ -0,0 +1,9 @@ +{ + "id": 38073395880, + "event": "repository_dispatch", + "display_title": "dev-lead-reviews-retry", + "path": ".github/workflows/dev-lead.yml", + "created_at": "2026-10-10T17:50:45Z", + "status": "completed", + "conclusion": "cancelled" +} diff --git a/tests/fixtures/bot-comment-retry/runs-listing-pr2205.json b/tests/fixtures/bot-comment-retry/runs-listing-pr2205.json new file mode 100644 index 000000000..e8bd24056 --- /dev/null +++ b/tests/fixtures/bot-comment-retry/runs-listing-pr2205.json @@ -0,0 +1,23 @@ +{ + "total_count": 2, + "workflow_runs": [ + { + "id": 38073405366, + "event": "repository_dispatch", + "display_title": "pr-review-mention", + "path": ".github/workflows/pr-review-trigger.yml", + "created_at": "2026-10-10T17:50:54Z", + "status": "completed", + "conclusion": "success" + }, + { + "id": 38073395880, + "event": "repository_dispatch", + "display_title": "dev-lead-reviews-retry", + "path": ".github/workflows/dev-lead.yml", + "created_at": "2026-10-10T17:50:45Z", + "status": "completed", + "conclusion": "cancelled" + } + ] +}