Skip to content

fix(consume): make release resolution robust to GitHub API rate limits#3182

Open
danceratopz wants to merge 6 commits into
ethereum:forks/amsterdamfrom
danceratopz:reduce-consume-cache-pagination
Open

fix(consume): make release resolution robust to GitHub API rate limits#3182
danceratopz wants to merge 6 commits into
ethereum:forks/amsterdamfrom
danceratopz:reduce-consume-cache-pagination

Conversation

@danceratopz

Copy link
Copy Markdown
Member

Description

consume commands that resolve fixtures from a release spec crashed whenever the release-information cache was older than 4 hours and the GitHub API request failed:

uv run consume cache --input=tests@v20.0.1
...
INTERNALERROR>   File ".../plugins/consume/releases.py", line 250, in download_release_information
INTERNALERROR>     response.raise_for_status()
INTERNALERROR> requests.exceptions.HTTPError: 403 Client Error: rate limit exceeded for url: https://api.github.com/repos/ethereum/execution-spec-tests/releases?per_page=100

The root cause: get_release_information() deleted the stale cache file before attempting the refresh. A failed download (the unauthenticated GitHub API allows 60 requests/hour per IP, shared by every tool on the machine) crashed the run and left no cache behind, so every subsequent run hit the API cold and the failure became self-perpetuating.

This PR makes release resolution robust and, for pinned versions, offline after the first lookup:

  • Pinned versions (e.g. tests@v20.0.1) resolve from the cache regardless of its age and without any API requests: Release tags are immutable, so a cached entry cannot become outdated.
  • A stale cache is kept as a fallback when the refresh fails (with a logged warning) instead of being deleted upfront; only lookups with no usable cache still hard-fail.
  • A corrupt cache file (e.g. a partially-written download) is re-downloaded instead of wedging every run until manually deleted; the cache is now written atomically via a temporary file.
  • API requests are authenticated with GITHUB_TOKEN when set: 5,000 requests/hour instead of 60.
  • get_release_page_url() now receives the release spec instead of the asset URL, so the release-page lookup shares the pinned-version fast path instead of triggering a second resolution.

The per_page=100 pagination introduced in #3085 was initially suspected but is not the cause: the request count per refresh is unchanged (still at most 2 pages per repo), and larger pages mean fewer, not more, requests.

The first commit adds the tests on their own; the four that cover the new behavior fail without the fix.

Related Issues or PRs

Related: #3085.

Checklist

  • Ran fast static checks to avoid CI fails, see Code Standards & Verifying Changes: just static
  • PR title has the form <type>(<area>): <title>, where <type> and <area> come from an appropriate C-<type>, respectively A-<area>, label. The title should match the target squash commit message.

Cute Animal Picture

403 Forbidden

Add tests for `get_release_information`'s caching behavior around the
GitHub API rate limit:

- A pinned version (immutable tag) that a stale cache already resolves
  must not trigger an API refresh.
- A rate-limited refresh must fall back to the stale cache instead of
  deleting it and crashing.
- A corrupt cache file must be re-downloaded, not crash every run.
- `GITHUB_TOKEN`, when set, must authenticate API requests.

The four tests covering the new behavior fail without the fix.
Previously `get_release_information` deleted a stale (>4 hours old)
release-information cache file before attempting to re-download it, so
a failed refresh (e.g. the unauthenticated GitHub API rate limit of 60
requests/hour per IP was exhausted) crashed the run and left no cache
behind, forcing every subsequent run onto the API.

- Resolve pinned versions (e.g. `tests@v20.0.1`) from the cache
  regardless of its age and without any API requests: Release tags are
  immutable, so a cached entry cannot be outdated.
- Keep the stale cache file as a fallback when the refresh fails,
  instead of deleting it upfront.
- Re-download a corrupt cache file instead of crashing; write the
  cache via a temporary file so concurrent readers never see a partial
  write.
- Authenticate API requests with `GITHUB_TOKEN` when set (5000
  requests/hour instead of 60).
- Pass the release spec instead of the asset URL to
  `get_release_page_url` so the release-page lookup also benefits
  from the pinned-version fast path.
@danceratopz danceratopz added C-bug Category: this is a bug, deviation, or other problem A-test-consume Area: execution_testing.cli.pytest_commands.plugins.consume labels Jul 15, 2026
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.37%. Comparing base (56e615e) to head (ce63a6e).
⚠️ Report is 3 commits behind head on forks/amsterdam.

Additional details and impacted files
@@               Coverage Diff                @@
##           forks/amsterdam    #3182   +/-   ##
================================================
  Coverage            93.37%   93.37%           
================================================
  Files                  624      624           
  Lines                36990    36990           
  Branches              3386     3386           
================================================
  Hits                 34539    34539           
  Misses                1677     1677           
  Partials               774      774           
Flag Coverage Δ
unittests 93.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@danceratopz
danceratopz marked this pull request as draft July 15, 2026 23:38
`FixturesSource.from_release_spec` resolved the same spec twice, once
via `get_release_url` and once via `get_release_page_url`, so the
cached release information was parsed twice and, when the cache was
stale and the GitHub API unavailable, the doomed refresh was attempted
twice (with a duplicate fallback warning).

- Add `resolve_release` and derive both the asset download URL and
  the release page from a single resolution in `from_release_spec`;
  `get_release_url`/`get_release_page_url` remain as thin wrappers.
- Drop `get_release_page_url`'s direct asset-URL branch: It became
  unreachable when its only caller switched to passing the release
  spec, and it duplicated `is_release_url`'s regex.
- De-duplicate the canned API response and the cache-redirection
  monkeypatching in the release tests.
@danceratopz
danceratopz marked this pull request as ready for review July 15, 2026 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-test-consume Area: execution_testing.cli.pytest_commands.plugins.consume C-bug Category: this is a bug, deviation, or other problem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant