Skip to content

fix(pipeline): retry the hand-off API calls that a GitHub brown-out killed#9772

Merged
MarkusNeusinger merged 6 commits into
mainfrom
fix/pipeline-api-retry-hardening
Jul 24, 2026
Merged

fix(pipeline): retry the hand-off API calls that a GitHub brown-out killed#9772
MarkusNeusinger merged 6 commits into
mainfrom
fix/pipeline-api-retry-hardening

Conversation

@MarkusNeusinger

@MarkusNeusinger MarkusNeusinger commented Jul 24, 2026

Copy link
Copy Markdown
Owner

What happened

On 2026-07-24, ~16:05–16:26 UTC GitHub's API browned out — HTTP 502 on the
workflow-dispatch endpoint, HTTP 504 on GraphQL, HTTP 429 on codeload. Four radar-basic
PRs fell out of the pipeline and stayed out, because the calls that hand work to the next
workflow had no retry:

PR Library State it was left in Call that died
#9742 seaborn ai-approved q90 impl-mergegh pr view504
#9744 pygal ai-rejected q87 repair dispatch → 504; ai-attempt-1 also lost
#9749 makie ai-rejected q88 repair dispatch → 502
#9751 highcharts ai-rejected q88 repair dispatch → 502

impl-review's verdict step is the pipeline's only hand-off point — every downstream
workflow starts from a call made there. So an unretried blip doesn't merely fail a step; it
leaves a PR carrying a verdict label with nothing listening.

All four have since been recovered manually and merged; this PR stops it recurring.

Changes

impl-review.yml — the verdict step

  • every API call retries 3× with linear backoff (same shape as the existing Extract PR info
    retry, added 2026-05-06 for this same class of failure)
  • the repair is dispatched before the attempt label is added, and the label can no longer
    gate it. Losing the label costs one over-strict review; losing the dispatch costs the whole PR
  • if the dispatch still exhausts its retries, the step drops ai-rejected on the way out:
    ai-rejected + ai-attempt-N matches no watchdog case (Case 2 excludes any verdict
    label, Case 4 excludes PRs that already have an attempt label), while the label-less state is
    exactly Case 2 — which re-dispatches the repair at the attempt number the remaining label
    still encodes
  • the label read tracks success explicitly instead of inferring it from empty output, and a
    successful read with no verdict label now fails loudly instead of ending the step green

Two bugs found while verifying the above

  • ai-attempt-4 never existed as a label. The attempt label encodes the cascading
    threshold (90 → 80 → 70 → 60 → 50), but the repo only has ai-attempt-1..3, so on the 4th
    repair the add failed every time and || true swallowed it — feat(pygal): implement scatter-matrix-interactive #7268 and feat(pygal): implement boxen-basic #7039 both reached
    "Repair Attempt 4/4" carrying only ai-attempt-1..3. Every 4th review therefore re-applied
    the ≥ 90 attempt-0 bar, and the attempts-exhausted branch (close PR, remove stale
    implementation) could never be reached. Labels are now created on demand.
    This also means my first draft of this PR was a hard blocker: with the label add fatal, the
    4th repair would never have been dispatched at all.
  • That newly-reachable branch rendered a literal $SCORE/100 — quoted heredoc. Now
    expands, with the markdown backticks escaped so they can't become command substitution.

impl-merge.yml

  • the 5× merge retry re-reads PR state first. A merge that succeeded server-side but lost its
    HTTP response used to fail four more times with "already merged" and exit 1 — and because
    every post-merge step is gated on should_run == 'true' (implicit success()), that skipped
    GCS promotion, the impl:{lib}:done label, closing the issue and the Postgres sync. A merged
    PR with none of its bookkeeping: exactly the silent partial completion CLAUDE.md warns about

auto-update-pr-branches.yml — Dependabot PRs could never merge while the pipeline ran

  • the workflow called update-branch on Dependabot PRs. That push is authored by
    github-actions[bot], so GitHub gates the resulting runs behind manual approval
    (action_required) — and because main advances every few minutes during impl merges, each
    branch was re-updated long before anyone could approve. PR deps(python): Bump the python-minor group across 1 directory with 7 updates #9674 accumulated 174 runs in
    22 h: 162 action_required against only 4 green
    (the original dependabot[bot] push)
  • Dependabot branches are now skipped. They merge fine while behind — the main ruleset is
    not strict (strict_required_status_checks_policy: false); the file's header claimed the
    opposite and is corrected
  • /dependabot's playbook told operators to run that same harmful update-branch by hand; it
    now says the opposite and documents the @dependabot recreate remedy

Verification

.github/workflows/ has no verification loop (CLAUDE.md), so this was reviewed by a 4-lens
adversarial pass with an independent refutation round (regression / shell-under-bash -e / retry idempotency / the Dependabot
premise), and every claim was re-checked against the live repo before acting. That pass is what
caught the ai-attempt-4 blocker and the false Dependabot-rebase premise in my first draft.

  • YAML parses; bash -n clean on the extracted steps
  • gh_retry semantics exercised under bash -e: succeeds first try / recovers after 2
    failures / multi-arg passthrough / hard failure aborts the step
  • the corrected control flow tested for all three cases — normal, attempt 4 with a
    permanently failing label add (dispatch must still fire)
    , and dispatch exhaustion
    (ai-rejected dropped, exit 1)
  • confirmed the CI runner's gh adds labels fine; only the locally-installed gh 2.45.0 hits
    the projectCards GraphQL deprecation, so hard-failing on --add-label is safe in CI
  • ruleset strictness, the ai-attempt-* label set, and the poisoned Dependabot head commits
    were all read from the live repo, not assumed

Follow-up (not in this PR)

🤖 Generated with Claude Code

…illed

On 2026-07-24 (~16:05-16:26 UTC) GitHub returned HTTP 502 on the
workflow-dispatch endpoint and HTTP 504 on GraphQL. Four radar-basic PRs
fell out of the pipeline and stayed there: #9742 was already ai-approved
but impl-merge's condition check died on an unretried `gh pr view`, and
#9744/#9749/#9751 were ai-rejected but their impl-repair dispatch never
landed.

impl-review's verdict step is the pipeline's only hand-off point - every
downstream workflow starts from a call made there - so an unretried blip
does not merely fail a step, it leaves a PR carrying a verdict label with
nothing listening.

- retry every API call in that step and in impl-merge's condition check
  3x with linear backoff
- dispatch impl-repair *before* adding the attempt label, so a label
  problem can never suppress the dispatch (losing the label costs one
  over-strict review; losing the dispatch costs the whole PR)
- on dispatch exhaustion, drop `ai-rejected` on the way out: the
  combination ai-rejected + ai-attempt-N matches no watchdog case, while
  the label-less state is exactly Case 2, which re-dispatches the repair
  at the attempt number the remaining label still encodes
- create `ai-attempt-N` on demand. `ai-attempt-4` never existed, so the
  4th repair's label add always failed and `|| true` hid it (#7268/#7039
  reached "Repair Attempt 4/4" carrying only ai-attempt-1..3). Every 4th
  review therefore re-applied the >= 90 attempt-0 bar, and the
  attempts-exhausted branch could never be reached
- that branch's PR comment used a quoted heredoc and rendered a literal
  "$SCORE/100"; it now expands, with the markdown backticks escaped so
  they cannot become command substitution
- impl-merge's merge retry re-reads state first: a merge that succeeded
  server-side but lost its response used to fail four more times with
  "already merged" and exit 1, skipping GCS promotion, the
  impl:{lib}:done label, closing the issue and the Postgres sync

Also stop auto-updating Dependabot branches. That push is authored by
github-actions[bot], which makes GitHub gate the resulting runs behind
manual approval; because main advances every few minutes, each branch was
re-updated long before anyone could approve. PR #9674 burned 174 runs in
22 h - 162 action_required against 4 green. They merge fine while behind
(the main ruleset is not strict; the file's header claimed otherwise and
is corrected), and /dependabot's playbook told operators to run that same
harmful update-branch by hand.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 24, 2026 20:43
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Approving the gated run once is enough — verified that the required
contexts (Run Linting / Run Tests / Run Frontend Tests) do report as
SUCCESS on a github-actions[bot]-authored head once the run is approved
(#9674, commit 5954507). The head only kept getting re-gated because
this workflow kept pushing to it; with that stopped, one approval sticks.
`@dependabot recreate` is an alternative, not a requirement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves the resilience of the implementation pipeline’s workflow hand-offs during transient GitHub API failures (e.g., 5xx/429), and fixes related pipeline reliability issues uncovered during verification (missing ai-attempt-4 label, merge idempotency, Dependabot branch updates).

Changes:

  • Add consistent 3× retry + linear backoff around critical gh/dispatch calls in impl-review.yml (verdict hand-off) and impl-merge.yml (pre-merge condition/state reads).
  • Make repair dispatch non-blockable by attempt-label writes and ensure ai-attempt-4 is created on demand to keep the cascading threshold logic intact.
  • Skip Dependabot branches in auto-update-pr-branches.yml and update the /dependabot operator playbook accordingly; add a detailed changelog entry.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
CHANGELOG.md Adds detailed Fixed entries documenting the brown-out impact and the pipeline/workflow corrections.
agentic/commands/dependabot.md Updates the operator playbook to avoid harmful update-branch operations on Dependabot PRs and document recovery (@dependabot recreate).
.github/workflows/impl-review.yml Adds gh retry helper and adjusts dispatch/label ordering to prevent stranded PRs; creates missing attempt labels on demand.
.github/workflows/impl-merge.yml Retries PR condition reads and adds an idempotency guard to avoid failing after a server-side merge succeeds.
.github/workflows/auto-update-pr-branches.yml Skips Dependabot branches to prevent action_required gating loops; corrects ruleset assumptions in header.

Comment thread .github/workflows/impl-review.yml Outdated
Comment on lines +947 to +949
gh pr edit "$PR_NUM" --remove-label "ai-rejected" 2>/dev/null || true
echo "::error::impl-repair dispatch failed after 3 attempts — verdict label dropped so watchdog Case 2 picks PR #${PR_NUM} up"
exit 1
Comment thread .github/workflows/impl-merge.yml Outdated
PR_DATA=$(cat /tmp/pr.json)
break
fi
echo "::warning::gh pr view failed (attempt ${attempt}/3): $(cat /tmp/pr.err)"
Comment thread .github/workflows/impl-merge.yml Outdated
Comment on lines +255 to +259
STATE=$(gh pr view "$PR_NUM" --repo "$REPOSITORY" --json state -q .state 2>/dev/null || echo "")
if [ "$STATE" = "MERGED" ]; then
echo "::notice::PR #${PR_NUM} is already merged — continuing to post-merge steps"
exit 0
fi
] | length
')
if [[ "$SKIPPED" -gt 0 ]]; then
echo "::notice::Skipping $SKIPPED Dependabot PR(s) — Dependabot rebases those itself (see comment above)"
Copilot AI review requested due to automatic review settings July 24, 2026 20:47
… the state probe

- impl-review: the `--remove-label ai-rejected` on the dispatch-failure path
  IS the hand-off to the watchdog, so it can't be an unretried `|| true` —
  the same brown-out would swallow it and leave the PR in the stranded
  state it exists to escape. Retried, and loud when it still fails.
- impl-merge: retry the new state probe too. A blip there fell through to
  another `gh pr merge` on an already-merged PR, re-creating the
  merged-but-bookkeeping-skipped mode the probe was added to prevent.
- impl-merge: flatten multi-line gh stderr before interpolating it into
  `::warning::`, matching impl-review.
- auto-update-pr-branches: the ::notice:: still claimed "Dependabot
  rebases those itself", contradicting the corrected comment above it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MarkusNeusinger

Copy link
Copy Markdown
Owner Author

Applied all four review comments:

  • impl-review.yml:949 — right, and it was the sharpest of the four: that --remove-label ai-rejected is the hand-off to the watchdog, so leaving it unretried under || true meant the same brown-out could swallow the rescue and leave the PR in precisely the stranded state the surrounding comment warns about. Now retried, and it emits an explicit ::error:: naming the PR if it still fails.
  • impl-merge.yml:259 — same class of problem, applied to the new idempotency probe: a blip on the state read fell through to another gh pr merge on an already-merged PR, re-creating the merged-but-bookkeeping-skipped mode the probe exists to prevent. The probe is retried 3×, and an unreadable state now warns before proceeding rather than silently looking like OPEN.
  • impl-merge.yml:61 — flattened, matching what impl-review.yml already does.
  • auto-update-pr-branches.yml:103 — correct catch. I had rewritten the block comment after verifying the ruleset is non-strict, but left the stale claim in the ::notice:: string; it now states the actual reason.

Verified the merge loop's four paths against a stubbed gh under bash -e: normal merge, lost-response merge (state MERGED, merge command errors) → exit 0 so the post-merge steps still run, state read blipping while the merge succeeds, and a genuinely unmergeable PR → exit 1.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

.github/workflows/impl-review.yml:893

  • Same pipefail/SIGPIPE issue as above: tr ... | head -c 500 can cause the step to exit under bash -eo pipefail, defeating the label-read retry loop. Use head | tr instead.
              break
            fi
            LERR=$(tr '\n' ' ' < /tmp/labels.err | head -c 500)
            echo "::warning::gh pr view (labels) failed (attempt ${attempt}/3): ${LERR}"
            if [ "$attempt" -lt 3 ]; then

.github/workflows/impl-merge.yml:63

  • ::warning::... $(cat /tmp/pr.err) can emit multi-line output, which breaks the annotation format and makes logs harder to parse. Consider flattening + truncating stderr to one line (same pattern used elsewhere in this PR).
              if gh pr view "$PR_NUM" --repo "$GH_REPO" --json headRefName,labels > /tmp/pr.json 2> /tmp/pr.err; then
                PR_DATA=$(cat /tmp/pr.json)
                break
              fi
              # Flattened: `::warning::` is line-oriented, and gh's HTTP errors
              # are routinely multi-line — the tail would spill into the raw log.
              PERR=$(tr '\n' ' ' < /tmp/pr.err | head -c 500)

.github/workflows/auto-update-pr-branches.yml:105

  • The notice says "Dependabot rebases those itself", but the preceding comment explicitly notes Dependabot does not rebase merely because a branch is behind. Reword this notice to avoid giving operators the wrong mental model.
          if [[ "$SKIPPED" -gt 0 ]]; then
            echo "::notice::Skipping $SKIPPED Dependabot PR(s) — updating them gates their CI behind manual approval, and the non-strict ruleset means being behind does not block the merge (see comment above)"
          fi

Comment on lines +866 to +870
if "$@" 2> /tmp/gh_retry.err; then
return 0
fi
err=$(tr '\n' ' ' < /tmp/gh_retry.err | head -c 500)
echo "::warning::${what} failed (attempt ${attempt}/3): ${err}"
Copilot AI review requested due to automatic review settings July 24, 2026 20:51
`tr ... | head -c 500` aborts the assignment under `set -o pipefail` once
the input exceeds the 64 KB pipe buffer (verified: 200 KB input +
pipefail -> abort; without pipefail -> rc 0). These steps run under plain
`bash -e`, so it is not reachable today, but other steps in this repo do
set `-eo pipefail` and gh's stderr is attacker-independent but unbounded.
`head -c 500 | tr` is byte-identical output with no exposure either way.

Applied to all four call sites, plus the pre-existing `$(cat /tmp/pr.err)`
in "Extract PR info" which had the same multi-line annotation problem.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MarkusNeusinger

Copy link
Copy Markdown
Owner Author

Applied — with a correction to the stated premise.

The SIGPIPE mechanism is real but not reachable in these steps as written, for two independent reasons:

  1. pipefail is not set here. All three edited steps use the default shell (/usr/bin/bash -e {0}, confirmed in the real run logs) and none of them run set -o pipefail. The comment says "the runner's bash -eo pipefail" — that's the shape GitHub uses for shell: bash, not for a bare run:.
  2. It needs >64 KB of stderr. tr only takes SIGPIPE if it's still writing after head exits, so the input has to exceed the pipe buffer. Measured:
input set -e set -eo pipefail
3 KB rc 0 rc 0
200 KB rc 0 aborted (SIGPIPE)

So today it is safe on both counts. I applied the flip anyway, because it costs nothing and removes the dependency on both of those staying true — auto-update-pr-branches.yml in this same PR does use set -eo pipefail, so "nobody adds pipefail to this step later" isn't a property worth relying on. Since tr '\n' ' ' is 1:1 byte-preserving, head -c 500 | tr yields identical output.

Done at all four call sites, plus the pre-existing $(cat /tmp/pr.err) in Extract PR info (impl-review.yml:65) which wasn't in the diff but had the same multi-line annotation problem.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +280 to 284
fi

# Update branch before merge attempt
gh pr update-branch "$PR_NUM" --repo "$REPOSITORY" 2>/dev/null || true
sleep 2
Copilot AI review requested due to automatic review settings July 24, 2026 20:55
The state probe narrows the lost-response window but doesn't close it: if
the probe is blind for the same reason the merge response was lost, every
retry reports "already merged" until the loop exits 1 — and the post-merge
bookkeeping (GCS promotion, impl:{lib}:done, closing the issue, Postgres
sync) is skipped on a PR that is in fact merged.

gh saying the PR is already merged IS the success signal, so match it on
stderr and continue. Genuine failures still retry and still exit 1, and
the retry warning now carries the actual gh error instead of nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@MarkusNeusinger

Copy link
Copy Markdown
Owner Author

Good catch — applied. You're right that the state probe narrows the window without closing it: if the probe is blind for the same reason the merge response was lost, every retry reports "already merged" until the loop exits 1, and the bookkeeping is skipped on a PR that is merged.

gh pr merge's stderr is now captured and an already-merged report is treated as the success signal it actually is. Genuine failures still retry and still exit 1 — and the retry warning now carries the real gh error, which it previously discarded entirely.

Exercised against a stubbed gh under bash -e:

scenario result
state probe blind + merge already done (your case) ::notice::already merged (stderr)exit 0, post-merge steps run
state probe blind + genuine failure (Base branch was modified) 3 retries, error surfaced in each warning → exit 1
normal merge exit 0 on attempt 1

That's now three layers on the same failure: state probe → stderr match → loud exit.

@MarkusNeusinger
MarkusNeusinger merged commit f71a9e4 into main Jul 24, 2026
7 checks passed
@MarkusNeusinger
MarkusNeusinger deleted the fix/pipeline-api-retry-hardening branch July 24, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/workflows/impl-review.yml:862

  • The comment says “Every API call below is retried 3x”, but in this block several gh API calls are still not retried (e.g., gh pr close, gh pr comment, gh pr edit --add-label quality-poor). Either wrap the remaining calls with gh_retry or narrow the comment to “hand-off/critical calls” so it matches the actual behavior.
          # exactly what the 2026-07-24 GitHub API brown-out did to four
          # radar-basic PRs (HTTP 502 on the dispatch endpoint, HTTP 504 on
          # GraphQL). Every API call below is retried 3x with linear backoff.
          # `::warning::` is line-oriented, so multi-line gh stderr would leak
          # everything after the first newline into the raw log — flatten it.

Copilot AI review requested due to automatic review settings July 24, 2026 21:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

# until the loop exits 1 and the post-merge bookkeeping is skipped.
# gh saying the PR is already merged IS the success signal.
MERR=$(head -c 500 /tmp/merge.err | tr '\n' ' ')
if grep -qiE 'already merged|pull request is (already )?(merged|closed)' /tmp/merge.err; then
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants