fix(consume): make release resolution robust to GitHub API rate limits#3182
Open
danceratopz wants to merge 6 commits into
Open
fix(consume): make release resolution robust to GitHub API rate limits#3182danceratopz wants to merge 6 commits into
danceratopz wants to merge 6 commits into
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
marked this pull request as ready for review
July 15, 2026 23:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
consumecommands that resolve fixtures from a release spec crashed whenever the release-information cache was older than 4 hours and the GitHub API request failed: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:
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.GITHUB_TOKENwhen 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=100pagination 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
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture