Skip to content

perf(pr-feedback-dispatch): skip ci-status re-runs that can only pass again - #12932

Merged
kyle-sexton merged 1 commit into
mainfrom
perf/rerun-ci-status-dedup
Nov 3, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
perf/rerun-ci-status-dedup

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: hub lever L114 (GitHub call budget)

Summary

Measured over 6 h (2026-11-03 13:10Z to 19:10Z):

Measure Value
rerun-ci-status job runs / GITHUB_TOKEN calls 416 / 1370
Job runs that re-ran ci-status 141 (74 after a review lane's attempt 1, 67 after attempt 2 or later)
ci-status re-run attempts (pr-require-checks runs created in the window) 117 on 94 heads (75 heads once, 15 twice, 4 three times)
Attempt N-1 to N failure to success 64, failure to failure 27, success to success 20, success to cancelled 1, other 5
Re-runs whose previous attempt had already passed 21

A ci-status that passed had counted both review lanes, so re-running it can only pass again. The re-run still starts another pr-verify on the head (pr-verify.yml runs on every completed pr-require-checks run, .github/workflows/pr-verify.yml:69-71, and its gate accepts this job's record, line 187). That pr-verify costs about 37 calls (check-intent plus subscription, the attempt-2 median). Most of these passes began reading the reviews within the job's 60 s margin after the last lane finished. In some, the job started early and held before its review-lanes step, so the read came after the lanes finished but the job's start fell before.

Fix

rerun-ci-status's find step now times ci-status's read of the reviews from its Require both review lanes on this head step. When the jobs API lists no steps, it falls back to the job's start, which is earlier. ci-status is left alone in two cases:

  • it began that read more than 60 s after both lanes finished (the existing rule, now with the step's time);
  • it passed on a read at or after both lanes finished (new).

A failed or cancelled ci-status inside the margin is still re-run. Both checks use the job list the step already reads, so they add no API call. The kill-switch, trusted-actor, reconciler-record and record-before-re-run rules are unchanged.

Replaying the rule over the 117 re-runs skips 19: 17 success to success, 1 success to cancelled, 1 failure to failure. It skips none of the 64 failure to success re-runs. The replay times the read against the trigger lane's finish, which is never later than the job's done_at, so 19 is an upper bound.

Expected saving is about 160 to 200 GITHUB_TOKEN calls an hour (estimate). A skipped re-run spends about 50 calls: this job 3, a ci-status attempt about 8 (attempt-1 median), pr-verify about 37, post-verify-notes about 2. At 19 per 6 h that comes to about 160 an hour. All 19 skipped rows come after #12797 merged at 14:31Z (the earliest at 14:59Z), so over the 4.65 h since then it comes to about 200 an hour. Each skip also saves one ci-status job and one pr-verify run of runner time.

On the two rules the hub asked for:

  • At most one re-run per head per cause: this job made no re-run twice for the same head, lane and lane attempt. The heads re-run two or three times got them from different lane attempts or from other re-runners.
  • No re-run without a changed input: 5 failure to failure re-runs had the trigger lane finish before the earlier attempt read the reviews. The rule skips 1 of them. The other 4 fall inside the 60 s lagging-read margin and are kept on purpose.

The data covers 102 of the 103 re-run pr-require-checks runs: one jobs read (run 41657424065) returned HTTP 502.

Not in this PR, reported to the hub:

  • Of the 117 re-runs, 31 were not made by this job. Most were started by github-actions[bot], plausibly the aggregate composite re-running a red contract-only sibling (UNVERIFIED). That is how head 93e737d got two attempt-2 pr-verify runs (41678810148, 41679518660).
  • 3 failure to failure re-runs followed a failed needed job. Skipping those depends on the external aggregate-results composite.

Verification

  • scripts/ci-rerun-ci-status.test.sh: PASS=273 FAIL=0. New cases cover:
    • a late read by step time;
    • a pass read at and after the lanes' finish, skipped with 3 API calls;
    • a pass read before the last lane finished, re-run;
    • a pass with no steps listed, which falls back to the job start;
    • failed and cancelled reads inside the margin, re-run.
  • The other tests that read this workflow (check-trusted-trigger, log-api-budget, reconcile, feedback-dispatch, workflow-gates): 880 of 881 pass. The one failure, ownerAdoption ... idleHours in feedback-dispatch.test.mjs:3184, reads repository config this diff does not touch, so it comes from main.
  • zizmor --offline: no findings. shellcheck on the test passes. Local actionlint reports only concurrency.queue, which is already on main and which this local actionlint does not know.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01MWmASPDSoP4Ps23JPiGyZV

… again

rerun-ci-status re-ran a ci-status that had already passed after reading
both finished review lanes, which only started pr-verify on the head again.
It now leaves a ci-status alone when it passed on a read at or after both
lanes finished, and times the read from ci-status's review-lanes step
rather than the job start, so a job held before that step counts as a late
read. Both use the job list the step already reads: no new API call.

Over 6 h (117 re-runs) the rule skips 19 (17 success->success, 1
success->cancelled, 1 failure->failure) and none of the 64 failure->success re-runs.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01MWmASPDSoP4Ps23JPiGyZV
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-03T20:12:03.070410Z 487e2d7 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 487e2d798eccc1d190111bcced0663321c011874: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #12932

No security issues found.

The PR changes rerun-ci-status in .github/workflows/pr-feedback-dispatch.yml, a trusted path, so I read its whole change and the gate it feeds. The other changed file is the test scripts/ci-rerun-ci-status.test.sh.

What I checked:

  • The new skip can't hide a red result. A ci-status job is now skipped only when it passed (.conclusion == "success") and began reading the reviews at or after done_at. done_at is the latest updated_at among the trigger lane and the other lane's newest finished run (L780-L806). Such a run already counted both finished reviews, so a re-run would read the same review input. A failed, cancelled or in-progress job never takes the passed branch, so it is still re-run as on the base (L831-L841). The skip only drops a green re-run; it can't turn a red ci-status green.
  • The timing source is conservative. read_at is the start of the Require both review lanes on this head step (pr-require-checks.yml L6716), which comes before that step's API reads. When the jobs API lists no steps, it falls back to the job's start, which is earlier still. Both choices make the late and passed skips harder to reach, not easier. A malformed or unreadable timestamp falls into catch "", which means a re-run, as before.
  • Step names come from the PR head, but nothing new is exposed. A same-repository head can rewrite pr-require-checks.yml and so its step names. The job's if: already rejects forked heads (head_repository.full_name == github.repository). A head that can rename that step can already forge ci-status as a whole, as the base comments at L6712-L6714 note. Matching on the step name gives that head nothing more.
  • Other controls are untouched. The diff leaves alone the token permissions: block, the kill-switch read, the trusted-actor and approval checks, the reconciler-record path, record-before-re-run, and the moved-head and closed-PR checks. The merge gate's own holds (merge-holds.mjs and the rest) are not in the diff.
  • No instruction surface or plugin manifest changes. Prose changes are limited to comments that describe the new rule.

The PR description contains no embedded directives aimed at this lane.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed 487e2d798eccc1d190111bcced0663321c011874: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Code review: PR #12932, perf(pr-feedback-dispatch): skip ci-status re-runs that can only pass again

I found no blocking or advisory issues. I read every hunk of both changed files: .github/workflows/pr-feedback-dispatch.yml and scripts/ci-rerun-ci-status.test.sh.

What I checked

  • Step name. The new read_at selector looks for Require both review lanes on this head. That name exactly matches the ci-status step in pr-require-checks.yml#L6716.
  • Fallback when the step is missing. If the jobs API omits the step, lists it with a null started_at, or cuts the list short, strings drops the value and first(...) falls back to the job's started_at. That time is earlier, so the fallback is conservative. With no job at all, ${job:-null} produces "", the try fails, and the job is re-run. The old code behaved the same way.
  • The new "passed" branch. It needs .conclusion == "success", so a ci-status that is still in progress (conclusion null) never takes it. A job that failed or was cancelled inside the 60 s margin is still re-run, and the tests cover both cases.
  • jq context. The two as bindings keep . bound to the job object, so .conclusion is read from the job, as intended.
  • Tests.
    • Late read by step time, with 3 calls pinned.
    • A pass read exactly at the lanes' finish and inside the margin, both skipped with 3 calls.
    • A pass read one second before the finish, re-run.
    • A pass with no steps listed, decided by the job's start in both directions.
    • Failed and cancelled jobs inside the margin, re-run.
  • API calls. The change makes no new API call; it reuses the existing jobs read.

Notes (not findings)

  • If ci-status is still in progress when this job first reads it, the job polls the run, not the jobs list, so the job value is never refreshed. A ci-status that then passes on a read at or after the lanes' finish is still re-run. This doesn't make anything wrong; it is a narrower case the saving doesn't reach.
  • I did not run the tests. The PR's PASS=273 for ci-rerun-ci-status.test.sh and its replay counts (19 of 117 re-runs skipped) are the author's claims and I have not verified them.

Spend

The change cuts GITHUB_TOKEN calls and the runner time of skipped ci-status and pr-verify runs. It adds no charge.

No closing issue (the PR cites hub lever L114), so there are no acceptance criteria to check. No plugin manifests changed.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 431f923 Nov 3, 2026
62 of 63 checks passed
@kyle-sexton
kyle-sexton deleted the perf/rerun-ci-status-dedup branch November 3, 2026 20:41
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.

1 participant