Skip GitHub fast path when rate-limited #13033

christeefy · 2025-04-21T23:19:11Z

Summary

I'm interpreting the "special handling" required to mean erroring so that we skip the subsequent Git fetch operation in .git_metadata and .resolve_revision of SourceDistributionBuilder. Please advise if you meant something else instead.

Also let me know if you'd like me to generalize this to other HTTP status codes (i.e. 400 and 422, as mentioned in the comments).

Test Plan

None so far.

What's the best way to create new (snapshot?) test cases for this?
Should I use wiremock to mock a 429 response?

christeefy · 2025-05-02T17:49:00Z

Hi @zanieb, I like to follow up on this PR as it has been awaiting review for a while. Do you have any feedback on this work? Let me know if this is still relevant. Thanks!

charliermarsh · 2025-05-04T17:05:37Z

I think this might not quite be the right approach. I think what we want is: if we detect a rate limit, skip these "fast-path" attempts in any subsequent operations. So we might need to track some kind of "Are we rate-limited?" global state, and read-from + update it in these locations.

zanieb · 2025-05-06T16:04:00Z

Indeed, that's what I had in mind. GitHub also returns a time to wait until attempting another request, so we can stash that in the global state and invalidate it after that much time (though I think in practice, skipping it until the process exits would be fine).

christeefy · 2025-05-09T17:07:38Z

Thank you both for the feedback; I understand things better now after some studying.

I'm assuming that we want the global state to apply to everything within uv-git, including GitResolver and git.rs which both have similar fast-path logic.

Just to ensure I'm interpreting things correctly, here's the steps I'll implement:

Keep track of an "Are we rate-limited?" global state
- If we get a HTTP-429, mark this state to true
- The state can revert to false based on a "time to wait" derived from GitHub's response header, and may be a nice-to-have assuming the process runtime is typically much shorter than this "time to wait"
Subsequent calls to github_fast_path would avoid pinging the GitHub API if/while this global state is true, returning the appropriate value:
- GitResolver::github_fast_path returns Ok(None), leading to a git fetch
- git.rs::github_fast_path returns FastPathRev::Indeterminate leading to all branches and tags potentially being fetched in git.rs::fetch based on RefspecStrategy 🤔

Please let me know if I've misinterpreted anything here.

Thanks!

christeefy · 2025-05-20T14:44:52Z

Hi @zanieb, I've updated the PR based on your feedback.
Appreciate you taking a look at it!

I found adding integration tests to not be easy. The base urls for the GitHub fast-paths are hardcoded and not exposed via the CLI / env vars, so I don't know how to mock their response via wiremock. I can go down the route of exposing them, unless you have better suggestions.

Thanks!

zanieb · 2025-05-20T15:07:23Z

The base urls for the GitHub fast-paths are hardcoded and not exposed via the CLI / env vars,

It'd be fine to add this as an environment variable, imo.

christeefy · 2025-05-20T15:11:58Z

I can add that env var (tentatively UV_GITHUB_FAST_PATH_URL).
In the meantime, let me know if I'm missing anything major in this PR.

christeefy · 2025-05-20T21:08:00Z

I've made the changes and included integration tests.

I manually did some mutation testing (changing the status code from the mock response, and commenting out this PR's functionalities) to verify the tests aren't dummy tests.

christeefy · 2025-05-20T21:09:22Z

crates/uv/tests/it/edit.rs

+    use uv_client::DEFAULT_RETRIES;
+
+    let context = TestContext::new("3.12");
+    let token = decode_token(READ_ONLY_GITHUB_TOKEN);
+
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .respond_with(ResponseTemplate::new(429))
+        .expect(1 + u64::from(DEFAULT_RETRIES))


uv makes 4 attempts (first try + 3 retries) because a HTTP 429 is considered transient in uv-client.

If there are other concurrent fast paths attempt before the first rate-limited request finishes retrying, the skip functionality won't kick in yet. I'm testing the implications of changing the retry semantics for HTTP 429 to Retryable::Fatal.

Local test suite is passing, so I can update these tests to expect zero retries.

Full test suite fails, so not going through with that. See comment below as well.

zanieb · 2025-05-21T01:53:02Z

crates/uv-client/src/base_client.rs

+        "Considering skipping retry of response HTTP {status} for {}",
+        response.url()
+    );
+    if status == StatusCode::TOO_MANY_REQUESTS {


I think this makes a 429 fatal everywhere? We probably don't want that.

Sounds good. Reverting.

This reverts commit 7a7fd11.

christeefy · 2025-05-21T14:28:30Z

Good morning @zanieb, the PR is ready for review.
Please take a look when you get the chance, thanks 😊

konstin · 2025-06-10T09:13:34Z

crates/uv-static/src/env_vars.rs

    #[attr_hidden]
    pub const UV_TEST_INDEX_URL: &'static str = "UV_TEST_INDEX_URL";

+    /// Used to set the GitHub fast-path url for tests.


This should document what shape of URL is expected, is there something that GitHub considers as API root that users could exchange?

This env var's intended use is to mock the GitHub API root during tests. It's not meant for users to override, and is hidden from user-facing docs with #[attr_hidden].

I can rename this to UV_TEST_GITHUB_FAST_PATH_URL to be even more explicit, assuming we are okay with UV_TEST_* appearing in non-test files.

Please let me know what you think.

konstin

Two things that should be easy to fix, otherwise it looks good!

konstin · 2025-06-24T09:42:45Z

crates/uv-git/src/rate_limit.rs

+pub(crate) struct GitHubRateLimitStatus(AtomicBool);
+
+impl GitHubRateLimitStatus {
+    pub(crate) const fn new() -> Self {


Does this method need to be pub(crate)?

No it doesn't. Updated to just const.

konstin · 2025-06-24T10:01:22Z

crates/uv/tests/it/pip_install.rs

+
+    uv_snapshot!(context.filters(), context
+        .pip_install()
+        .arg("pip-test-package @ git+https://github.com/pypa/pip-test-package@5547fa909e83df8bd743d3978d6667497983a4b7")


For consistency, can we use https://github.com/astral-test/uv-public-pypackage here too?

konstin · 2025-06-24T10:01:41Z

crates/uv/tests/it/pip_install.rs

+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .respond_with(ResponseTemplate::new(429))
+        .expect(1 + u64::from(DEFAULT_RETRIES)) // Middleware retries on 429 by default
+        .mount(&server)
+        .await;


Nice work on the testing strategy here!

christeefy · 2025-06-24T13:30:02Z

@konstin changes have been made and CI is passing. Thanks for your quick PR review 😊

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.7.14` -> `0.7.16` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.7.16`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0716) [Compare Source](astral-sh/uv@0.7.15...0.7.16) ##### Python - Add Python 3.14.0b3 See the [`python-build-standalone` release notes](https://github.com/astral-sh/python-build-standalone/releases/tag/20250626) for more details. ##### Enhancements - Include path or URL when failing to convert in lockfile ([#14292](astral-sh/uv#14292)) - Warn when `~=` is used as a Python version specifier without a patch version ([#14008](astral-sh/uv#14008)) ##### Preview features - Ensure preview default Python installs are upgradeable ([#14261](astral-sh/uv#14261)) ##### Performance - Share workspace cache between lock and sync operations ([#14321](astral-sh/uv#14321)) ##### Bug fixes - Allow local indexes to reference remote files ([#14294](astral-sh/uv#14294)) - Avoid rendering desugared prefix matches in error messages ([#14195](astral-sh/uv#14195)) - Avoid using path URL for workspace Git dependencies in `requirements.txt` ([#14288](astral-sh/uv#14288)) - Normalize index URLs to remove trailing slash ([#14245](astral-sh/uv#14245)) - Respect URL-encoded credentials in redirect location ([#14315](astral-sh/uv#14315)) - Lock the source tree when running setuptools, to protect concurrent builds ([#14174](astral-sh/uv#14174)) ##### Documentation - Note that GCP Artifact Registry download URLs must have `/simple` component ([#14251](astral-sh/uv#14251)) ### [`v0.7.15`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0715) [Compare Source](astral-sh/uv@0.7.14...0.7.15) ##### Enhancements - Consistently use `Ordering::Relaxed` for standalone atomic use cases ([#14190](astral-sh/uv#14190)) - Warn on ambiguous relative paths for `--index` ([#14152](astral-sh/uv#14152)) - Skip GitHub fast path when rate-limited ([#13033](astral-sh/uv#13033)) - Preserve newlines in `schema.json` descriptions ([#13693](astral-sh/uv#13693)) ##### Bug fixes - Add check for using minor version link when creating a venv on Windows ([#14252](astral-sh/uv#14252)) - Strip query parameters when parsing source URL ([#14224](astral-sh/uv#14224)) ##### Documentation - Add a link to PyPI FAQ to clarify what per-project token is ([#14242](astral-sh/uv#14242)) ##### Preview features - Allow symlinks in the build backend ([#14212](astral-sh/uv#14212)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [ghcr.io/astral-sh/uv](https://github.com/astral-sh/uv) | final | patch | `0.7.13` -> `0.7.15` | --- ### Release Notes <details> <summary>astral-sh/uv (ghcr.io/astral-sh/uv)</summary> ### [`v0.7.15`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0715) [Compare Source](astral-sh/uv@0.7.14...0.7.15) ##### Enhancements - Consistently use `Ordering::Relaxed` for standalone atomic use cases ([#14190](astral-sh/uv#14190)) - Warn on ambiguous relative paths for `--index` ([#14152](astral-sh/uv#14152)) - Skip GitHub fast path when rate-limited ([#13033](astral-sh/uv#13033)) - Preserve newlines in `schema.json` descriptions ([#13693](astral-sh/uv#13693)) ##### Bug fixes - Add check for using minor version link when creating a venv on Windows ([#14252](astral-sh/uv#14252)) - Strip query parameters when parsing source URL ([#14224](astral-sh/uv#14224)) ##### Documentation - Add a link to PyPI FAQ to clarify what per-project token is ([#14242](astral-sh/uv#14242)) ##### Preview features - Allow symlinks in the build backend ([#14212](astral-sh/uv#14212)) ### [`v0.7.14`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0714) [Compare Source](astral-sh/uv@0.7.13...0.7.14) ##### Enhancements - Add XPU to `--torch-backend` ([#14172](astral-sh/uv#14172)) - Add ROCm backends to `--torch-backend` ([#14120](astral-sh/uv#14120)) - Remove preview label from `--torch-backend` ([#14119](astral-sh/uv#14119)) - Add `[tool.uv.dependency-groups].mygroup.requires-python` ([#13735](astral-sh/uv#13735)) - Add auto-detection for AMD GPUs ([#14176](astral-sh/uv#14176)) - Show retries for HTTP status code errors ([#13897](astral-sh/uv#13897)) - Support transparent Python patch version upgrades ([#13954](astral-sh/uv#13954)) - Warn on empty index directory ([#13940](astral-sh/uv#13940)) - Publish to DockerHub ([#14088](astral-sh/uv#14088)) ##### Performance - Make cold resolves about 10% faster ([#14035](astral-sh/uv#14035)) ##### Bug fixes - Don't use walrus operator in interpreter query script ([#14108](astral-sh/uv#14108)) - Fix handling of changes to `requires-python` ([#14076](astral-sh/uv#14076)) - Fix implied `platform_machine` marker for `...

christeefy added 4 commits April 21, 2025 19:07

Error and skip on 429

0b9e8d1

🎨 Apply cargo fmt

3d0ea05

📝 Improve comment

e11c0ea

Re-trigger CI

7dd16fd

christeefy added 6 commits May 17, 2025 14:17

Merge branch 'main' into gh-fast-path/429

6eeb463

Update implementation based on feedback

1ff93df

🔥 Remove previous implementation

8ff2112

Format docs

eab2d32

Merge branch 'main' into gh-fast-path/429

066169d

🎨 Reformat code

c14c21b

christeefy added 4 commits May 20, 2025 16:22

🔧 Add UV_GITHUB_FAST_PATH_URL env var

a2a9dac

Apply env var change

5e04f88

✅ Add test for GitResolver

1fb7bca

✅ Add unit test for github_fast_path fn

29ce14c

christeefy force-pushed the gh-fast-path/429 branch from 78fd5da to 29ce14c Compare May 20, 2025 20:56

christeefy commented May 20, 2025

View reviewed changes

zanieb mentioned this pull request May 21, 2025

Requests should retry with backoff on 429 #13545

Closed

Do not retry on HTTP 429

7a7fd11

zanieb reviewed May 21, 2025

View reviewed changes

Revert "Do not retry on HTTP 429"

f3f8355

This reverts commit 7a7fd11.

zanieb assigned oconnor663 Jun 9, 2025

konstin reviewed Jun 10, 2025

View reviewed changes

oconnor663 removed their assignment Jun 18, 2025

christeefy added 6 commits June 21, 2025 08:18

Merge branch 'main' into gh-fast-path/429

0431722

Remove use of LazyLock

67f9a65

Change memory ordering

dc85d68

Check rate-limited status from response header

a093666

Determine rate limit via status codes

912dbaa

✅ Update tests

872d6ee

christeefy requested a review from oconnor663 June 21, 2025 14:44

christeefy mentioned this pull request Jun 21, 2025

Consistently use Ordering::Relaxed for standalone atomic use cases #14190

Merged

1 task

oconnor663 approved these changes Jun 23, 2025

View reviewed changes

oconnor663 assigned oconnor663 and konstin and unassigned oconnor663 Jun 23, 2025

konstin approved these changes Jun 24, 2025

View reviewed changes

konstin changed the title ~~Skip git fetch when rate-limited by GitHub~~ Skip GitHub fast path when rate-limited Jun 24, 2025

konstin added the enhancement New feature or improvement to existing functionality label Jun 24, 2025

christeefy added 2 commits June 24, 2025 08:59

Limit visibility for GitHubRateLimitStatus::new

8875411

Update tests

e9b0a90

christeefy requested a review from konstin June 24, 2025 13:29

Merge branch 'main' into gh-fast-path/429

3a55cf6

christeefy force-pushed the gh-fast-path/429 branch from aa91bde to 3a55cf6 Compare June 24, 2025 17:19

christeefy added 2 commits June 24, 2025 13:22

Fix lint

ab45228

Retrigger CI

9d94d9b

oconnor663 merged commit fe11cee into astral-sh:main Jun 24, 2025
87 checks passed

christeefy deleted the gh-fast-path/429 branch June 24, 2025 19:20

BrewTestBot mentioned this pull request Jun 25, 2025

uv 0.7.15 Homebrew/homebrew-core#228105

Merged

Skip GitHub fast path when rate-limited #13033

Skip GitHub fast path when rate-limited #13033

Uh oh!

Conversation

christeefy commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

christeefy commented May 2, 2025

Uh oh!

charliermarsh commented May 4, 2025

Uh oh!

zanieb commented May 6, 2025

Uh oh!

christeefy commented May 9, 2025

Uh oh!

christeefy commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented May 20, 2025

Uh oh!

christeefy commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christeefy commented May 20, 2025

Uh oh!

christeefy May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christeefy May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christeefy commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christeefy commented Jun 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christeefy commented Apr 21, 2025 •

edited

Loading

christeefy commented May 20, 2025 •

edited

Loading

christeefy commented May 20, 2025 •

edited

Loading

christeefy May 20, 2025 •

edited

Loading

christeefy May 20, 2025 •

edited

Loading