Skip to content

fix(source-control): give the verify reader's artifact listing its own timeout - #10465

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/verify-listing-budget
Oct 27, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/verify-listing-budget

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Refs: #10125

Summary

The pr-verify verdict reader (babysit_verify_evidence.py) lists every repository artifact through babysit_gh._run_pages, and every page shares one timeout: gh's 60 s per-call default. The repository now holds 7,522 artifacts, which is 76 pages at 0.7 to 1 s a page. On 2026-10-27 ci-status on #10125 (run 40451037349) ran out of time on page 66 and refused the PR as verify:unreadable: gh api -i repositories/1277265216/actions/artifacts?per_page=100&page=66 timed out after 0.281523s.

pr-merge reads the same listing. On the 13:08Z tick (run 40508067887) its "Skip refusals whose inputs have not changed" step read 75 pages in about 51 s. Artifact uploads were 1,878 on 10-26 and 1,267 on 10-27 by 13:20Z. At that rate, pr-merge's cold listing should pass 60 s within about one to two days (estimate), and from then on every merge candidate in a tick would refuse.

Fix

  • The listing read gets its own timeout, LISTING_TIMEOUT_SECONDS = 300, for that one call; every other gh call keeps the 60 s default. At 1 s a page, 300 s reads about 300 pages, and that fits inside every reader's limit: ci-status's 20-minute job, pr-merge's 18-minute arm step and 45-minute merge job, and pr-feedback-dispatch's 10-minute job.
  • The 5xx retries still share that one deadline, so a retry adds no time to the worst case.

Options considered:

  • The name= filter. It matches one exact name. The reader has to see FAIL verdicts from other pull requests, and it cannot know those names in advance.
  • Shorter retention. Verdicts have to last 90 days (TOO_OLD_DAYS is 80), so this would only slow the growth. The 90-day non-verdict artifacts are a separate question.
  • An incremental, carried listing. It would cut about 76 calls from each cold read. It changes what the reader trusts, so it belongs in its own change.

Verification

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 27, 2026 09:34
…n timeout

The listing's pages share one timeout, gh's 60 s per-call default. At
7,522 artifacts (76 pages at 0.7 to 1 s each) a cold read ran out on page
66 and refused #10125 as verify:unreadable (run 40451037349). The listing
now gets 300 s, and its 5xx retries still fit inside it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 27, 2026 13:57
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 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-10-27T13:58:54.276461Z 47e5049 Draft marked ready
ℹ️ 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

Copy link
Copy Markdown
Contributor

claude-security-review reviewed 47e50492bf2c4f77c7a760362b98360f1669dab8: 0 finding(s), 0 as inline comments.

I found no security issues in this pull request.

critical-findings: 0

What changed: babysit_verify_evidence.py is on the trusted-path list. It is the pr-verify verdict reader, which feeds a merge hold. The change gives the repository artifact listing its own 300 s timeout (LISTING_TIMEOUT_SECONDS) in place of gh's 60 s default. It also adds a regression test and a changelog entry.

What I checked (_read_listing, _actions):

  • Failure still refuses. A timeout, a non-5xx failure, a 429 or a malformed payload still raises _Unreadable, and the PR is refused as verify:unreadable. A 5xx or fast network error still raises _Transient. The new timeout only lets a large listing finish reading, so a hold that refused for a slow read can now judge on the full listing. A partial or truncated listing is not treated as complete, because the read is a single gh api --paginate --slurp call that either returns whole or errors.
  • Retries stay bounded. Retries still share one deadline (deadline = _listing_clock() + LISTING_TIMEOUT_SECONDS). Each retry's timeout is recomputed after its sleep, and a retry is skipped when the wait would end past the deadline. The retry path adds no time beyond the 300 s total.
  • No cache or trust widening. The cache and "a failed listing is kept" behaviour are untouched. The gh_timeout_seconds import is dropped, but nothing else in the file used it. The Actions token (_env) and the verdict-judging logic are unchanged.
  • Tests only. The test changes patch the new constant and add a 76-page read. They touch no production trust logic.

Trust-control lens: the change does not weaken, narrow or remove a merge hold. It does not widen who is trusted, token scope, the kill switch or the broker's inputs.

Manifests: no plugin.json or marketplace.json change, so no manifest <field> lines apply.

Spend: the 300 s ceiling lets one listing read use more Actions runner time. That is subscription or prepaid usage, not a billing change, so it is not a hold.

@github-actions

Copy link
Copy Markdown
Contributor

claude-review reviewed 47e50492bf2c4f77c7a760362b98360f1669dab8: 0 finding(s), 0 as inline comments.

No findings on #10465 (head 47e5049).

What I checked. I read the whole diff and the PR context. I also read _read_listing and _actions in babysit_verify_evidence.py, and _run_pages and _send in babysit_gh.py. I did not run the tests; their results are the author's claim and I have not verified them.

  • Behaviour. _run_pages gives the whole paginated listing one shared timeout, so the old 60 s default ran out on page 66 of 76. _read_listing now passes LISTING_TIMEOUT_SECONDS (300 s) as the first call's timeout. The 5xx retry deadline is also computed from that constant. The retry arithmetic is unchanged: each retry gets what is left of the same deadline after its wait. The first call's timeout was None and is now 300.0, which _run_pages honours, so the rest of the logic is the same.
  • Scope. The 300 s timeout applies to this one listing read only. Other gh calls keep the process default. The only other actions/artifacts read in the scripts is the name=-filtered one in babysit_postmerge.py:1557, which this change deliberately does not touch.
  • Tests. The four timeout tests now assert the explicit first timeout (60.0 under the patched constant) instead of None, and still cover the deadline arithmetic. The new AListingOfManyPagesReadsWhole test fakes _send and the clock so 76 pages at 1 s each must read whole. Its fake _send signature matches the real one, and it exercises the real _run_pages path.
  • Docs and changelog. The module docstring, the _read_listing docstring and the .changes fragment (bump: patch) match the change. A patch bump fits a bug fix with no interface change.

Manifest items. The diff touches no plugin.json or marketplace.json, so none apply.

Closing issues. The PR says Refs: #10125 and closes nothing, so there are no acceptance criteria to check.

Cost. The change makes one listing read allowed to run longer, up to 300 s, which uses more prepaid runner time and API calls. It is not a billing change, so I set no hold.

Notes, not findings.

  • The PR claims 300 s fits inside every reader's step timeout (ci-status 20 min, pr-merge 18 min arm step, pr-feedback-dispatch 10 min). I did not check this against the workflow files, which are outside this diff.
  • LISTING_TIMEOUT_SECONDS now ignores a process-wide --gh-timeout-seconds for this call. pr_queue_snapshot.py is the only caller of set_gh_timeout_seconds, and the constant says so as a judgment call, so I accept it.
  • The new blank line in TheConfigKey in the test file is an unrelated whitespace change and harmless.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit 32ed0a9 Oct 27, 2026
77 checks passed
@kyle-sexton
kyle-sexton deleted the fix/verify-listing-budget branch October 27, 2026 14: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.

1 participant