Skip to content

Keep the staging branch sweep alive when a pull request is deleted - #9332

Merged
taladrane merged 2 commits into
mainfrom
fix-staging-branch-cleanup-sweep
Sep 3, 2026
Merged

Keep the staging branch sweep alive when a pull request is deleted#9332
taladrane merged 2 commits into
mainfrom
fix-staging-branch-cleanup-sweep

Conversation

@taladrane

Copy link
Copy Markdown
Collaborator

Problem

The scheduled staging-branch cleanup sweep has failed on every run since 19 Aug — 40/40 of the most recent scheduled runs, and 978 of the last 1,000. Latest example: run 33769892084.

process_pr assumed every <login>/advisory-improvement-<N> branch still has a pull request N. When it doesn't, the unguarded gh api repos/${REPOSITORY}/pulls/${pr_number} returns 404 and set -euo pipefail kills the entire step.

Branches are processed in alphabetical order, so the sweep aborted at the same branch every single time — number 57 of 1,608, AnonymousSnest/advisory-improvement-5641 — and never reconciled the remaining ~97%.

Runs triggered by workflow_run were unaffected, which is why per-PR cleanup kept working and the breakage went unnoticed for weeks. The failure signature is identical on every run:

Pull request 5658 head repo is AnonySE26/advisory-database, not github/advisory-database; skipping.
gh: Not Found (HTTP 404)
##[error]Process completed with exit code 1.

What this changes

1. A deleted pull request is no longer fatal. fetch_pr_json distinguishes 404/410 ("the PR is gone") from a genuine transport failure. The former is skipped with a warning; the latter still fails.

2. Per-target failure isolation. One unreconcilable PR increments a counter and the sweep continues. Failures are still reported and the step exits non-zero at the end, so a real problem stays visible — this is not a blanket || true.

3. Batched GraphQL triage. Resolving all 1,608 branches over REST would cost ~1,770 requests and ~43 minutes on a 10-minute cron. Triaging in batches of 50 over GraphQL first brings a full sweep to roughly 55 API calls and ~90 seconds.

4. A concurrency group so sweeps cannot overlap, keyed per-run for the per-PR triggers so those are never queued behind a sweep.

5. Explicit failure paths. set -e is disabled inside process_pr because it's invoked from an if ! context. Rather than rely on it, the response parsing is guarded, a failed head-branch deletion propagates so the staging branch is never deleted after it, and trailing rm cleanups no longer mask the exit status of the command they follow.

That last point mattered more than it looks: without it, a failed triage batch would have been silently dropped without being counted, turning "always red and does nothing" into "always green and does nothing".

Why the triage is safe

The GraphQL triage only ever narrows the candidate set — process_pr still re-verifies every target over REST before deleting anything. The new target set is provably a subset of the old one, which bounds the blast radius of any bug in the triage itself.

The join deliberately keeps the branch name observed in the branch listing rather than the one GraphQL reported, so a PR retargeted between the listing and the deletion is still caught by the existing base_ref check.

No existing safety gate was removed.

Validation

  • actionlint, bash -n, shellcheck — all clean
  • 19/19 regression assertions, plus targeted tests for each hardening fix (empty response body, malformed body, GraphQL auth failure, masked-exit-code reproductions)
  • Live dry run against this repo: 1,608 branches → 140 actionable targets in 1m25s, 0 triage failures, no deletions performed
  • Independent security review: no vulnerabilities found
  • Independent correctness review: 3 issues found, all fixed in this branch
  • Confirmed the exact run that previously aborted now completes

Not exercised: a live branch deletion.

Reviewer notes

⚠️ The first real sweep will delete ~140 branch pairs. Worth watching that run.

Two things this PR deliberately does not address, both pre-existing:

  • ~1,300 of the 1,608 staging branches still won't be cleaned. Their PRs are fork-headed, and process_pr bails when head_repo != REPOSITORY. Not a regression — but if the goal is "drain the backlog", that gate needs a separate, deliberate decision.
  • 63 branches whose PR was deleted outright are intentionally left in place. There's no PR left to verify against, so cleaning them safely would mean dropping the safety check. Better as a one-off.

The scheduled reconciliation sweep has failed on every run since 19 Aug.
`process_pr` assumed every `<login>/advisory-improvement-<N>` branch still
has a pull request `N`, so the unguarded `gh api repos/.../pulls/N` returned
404 and `set -euo pipefail` killed the whole step. Branches are processed in
alphabetical order, so the sweep aborted at the same branch every time --
57 of 1,608 -- and never reconciled the remaining 97%. Runs triggered by
`workflow_run` were unaffected, which is why per-pull-request cleanup kept
working and the breakage went unnoticed.

Distinguish a deleted pull request from a transport failure, and isolate
per-target failures so one unreconcilable pull request no longer stops the
sweep. Failures are still counted and reported, and the step exits non-zero
at the end, so a real problem stays visible.

Resolving all 1,608 branches over REST would have cost ~1,770 requests and
~43 minutes on a 10-minute cron, so triage the branches in batches of 50
over GraphQL first. That brings a full sweep to roughly 55 API calls and
about 90 seconds. The triage only ever narrows the candidate set --
`process_pr` still re-verifies every target over REST before deleting -- and
the join deliberately keeps the branch name observed in the branch listing
rather than the one GraphQL reported, so a pull request retargeted between
the listing and the deletion is still caught.

Add a `concurrency` group so sweeps cannot overlap, keyed per-run for the
per-pull-request triggers so those are never queued behind a sweep.

`set -e` is disabled inside `process_pr` because it is invoked from an
`if !` context. Make the failure paths explicit rather than relying on it:
guard the response parsing, propagate a failed head-branch deletion so the
staging branch is never deleted after it, and stop trailing `rm` cleanups
from masking the exit status of the command they follow. Without that last
one a failed triage batch would have been silently dropped without being
counted, turning "always red and does nothing" into "always green and does
nothing".

Verified against the live repository: 1,608 branches triage to 140
actionable targets in 1m25s with no triage failures, and the run that
previously aborted now completes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3
Copilot AI balanced review requested due to automatic review settings September 3, 2026 15:10

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.

Copilot review overview

🟡 Changes recommended

Partial GraphQL errors can still be silently ignored, allowing incomplete sweeps to report success.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity .github/​workflows/​delete_staging_and_head_branches_writer.yaml — This accepts any partial GraphQL response whenever .data.repository exists. GraphQL can return…
What changed in this PR

Hardens scheduled staging-branch cleanup so missing pull requests and individual failures do not abort the sweep.

Changes:

  • Handles deleted pull requests safely.
  • Adds batched GraphQL triage and failure isolation.
  • Prevents overlapping scheduled sweeps.
File Description
.github/​workflows/​delete_staging_and_head_branches_writer.yaml Adds resilient cleanup, batching, and concurrency controls.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

-F name="${REPOSITORY#*/}" \
-f query="${query}" > "${response}" 2>"${errors}" || true

if ! jq -e '.data.repository' "${response}" >/dev/null 2>&1; then

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.

Validated GraphQL errors so only NOT_FOUND errors on pull-request alias paths are allowed; all other partial-response errors now fail triage. Fixed in 32e681c.

Co-authored-by: taladrane <63199643+taladrane@users.noreply.github.com>
@taladrane
taladrane merged commit a92e708 into main Sep 3, 2026
2 of 3 checks passed
@taladrane
taladrane deleted the fix-staging-branch-cleanup-sweep branch September 3, 2026 15:45
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.

4 participants