Skip to content

fix(github-graphql): lower shrinkPageSize floor from 25 to 5 - #45460

Merged
secustor merged 2 commits into
renovatebot:mainfrom
NickAnge:fix/45248-shrink-page-size-floor
Aug 25, 2026
Merged

fix(github-graphql): lower shrinkPageSize floor from 25 to 5#45460
secustor merged 2 commits into
renovatebot:mainfrom
NickAnge:fix/45248-shrink-page-size-floor

Conversation

@NickAnge

@NickAnge NickAnge commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Changes

shrinkPageSize() walked 100 → 50 → 25 and then gave up. For repositories with many tags, refs(first: 25) with nested tag / annotated-tag target resolution still exceeds GitHub's 10-second GraphQL execution limit, so the query is terminated server-side (502 / 504). The resulting ExternalHostError was re-thrown as an unrecoverable failure instead of being retried with a smaller page.

GitHub Support confirmed these are query execution timeouts — the query runs for exactly 10 seconds and is killed by their GraphQL backend — and recommended reducing the refs(first: ...) page size.

This PR extends the ladder to 100 → 50 → 25 → 10 → 5. The step list is now a single pageSizes constant, and shrinkPageSize() advances through it by index rather than via a chain of if branches, so adding or removing a step is a one-line change.

Behaviour is unchanged for every repository that already succeeds at 25 or above — the extra steps are only reached after 25 has failed.

Note for reviewers

hasReachedQueryLimit() is a flat queryCount >= 100, which is an item ceiling that varies with page size: 2,500 items at first: 25, but only 500 at first: 5. So a repository large enough to need the smallest page is also the one most likely to be truncated by the query cap.

I deliberately left that out of this PR to keep the change minimal, since it is a separate design decision. Happy to fold in either option if you have a preference:

  • scale the query limit with the page size so the item budget stays roughly constant, or
  • keep the flat cap and log a warning when it is hit.

Context

Please select one of the following:

  • This closes an existing Issue, Closes: #
  • This doesn't close an Issue, but I accept the risk that this PR may be closed if maintainers disagree with its opening or implementation

Discussion: #45248 (maintainer reply: "PR welcome")

Related: #16343, which introduced the shrink mechanism in 2022 with 25 as the floor.

AI assistance disclosure

Did you use AI tools to create any part of this pull request?

  • No — I did not use AI for this contribution.
  • Yes — minimal assistance (e.g., IDE autocomplete, small code completions, grammar fixes).
  • Yes — substantive assistance (AI-generated non-trivial portions of code, tests, or documentation).
  • Yes — other (please describe):

Claude Opus 5, via Claude Code, wrote the code change, the test suite, and this description. Every test was verified to fail on main and pass with the change (see below), and the diff was reviewed by @NickAnge before pushing.

Use of AI in replying to PR comments

Who answers review comments:

  • @NickAnge will read and reply directly.
  • An agent will draft replies and @username will read them before they are posted.
  • Nobody has explicitly committed to replying.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests, but ran on a real repository, or
  • Both unit tests + ran on a real repository

The Page shrinking suite now covers every rung of the ladder instead of just the first two. It is table-driven over attempts → resulting page size, plus two behavioural cases:

  • shrinks page size when a later page fails — new. Page 1 succeeds at 100, page 2 fails, and the retry reuses the same cursor at 50, so shrinking works mid-pagination too.
  • re-throws if shrinking did not help — now derives the attempt count from the ladder instead of hard-coding three.

The three new/changed cases fail on main and pass with the fix:

# datasource-fetcher.ts reverted to main
× shrinks page size to 10 after 3 failed attempt(s)
× shrinks page size to 5 after 4 failed attempt(s)
× re-throws if shrinking did not help
Tests  3 failed | 77 passed (80)

# with the fix
Tests  80 passed (80)

pnpm check --all lib/util/github/graphql and pnpm type-check are clean.

These tests mock the HTTP layer, so they cover the retry logic rather than the premise that first: 5 fits inside the 10s limit. The repository where we hit this is private, so I could not reproduce the timeout publicly. Happy to find a large enough public repo if you want that confirmed before merging.

@cla-assistant

cla-assistant Bot commented Aug 25, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@NickAnge
NickAnge marked this pull request as ready for review August 25, 2026 16:16
@github-actions
github-actions Bot requested a review from viceice August 25, 2026 16:16
@viceice
viceice requested a review from a team August 25, 2026 18:53
@secustor
secustor added this pull request to the merge queue Aug 25, 2026
Merged via the queue into renovatebot:main with commit d4c29b1 Aug 25, 2026
71 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 44.45.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants