-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Skip GitHub fast path when rate-limited #13033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
0b9e8d1
Error and skip on 429
christeefy 3d0ea05
🎨 Apply cargo fmt
christeefy e11c0ea
📝 Improve comment
christeefy 7dd16fd
Re-trigger CI
christeefy 6eeb463
Merge branch 'main' into gh-fast-path/429
christeefy 1ff93df
Update implementation based on feedback
christeefy 8ff2112
🔥 Remove previous implementation
christeefy eab2d32
Format docs
christeefy 066169d
Merge branch 'main' into gh-fast-path/429
christeefy c14c21b
🎨 Reformat code
christeefy a2a9dac
🔧 Add `UV_GITHUB_FAST_PATH_URL` env var
christeefy 5e04f88
Apply env var change
christeefy 1fb7bca
✅ Add test for GitResolver
christeefy 29ce14c
✅ Add unit test for `github_fast_path` fn
christeefy 7a7fd11
Do not retry on HTTP 429
christeefy f3f8355
Revert "Do not retry on HTTP 429"
christeefy 0431722
Merge branch 'main' into gh-fast-path/429
christeefy 67f9a65
Remove use of LazyLock
christeefy dc85d68
Change memory ordering
christeefy a093666
Check rate-limited status from response header
christeefy 912dbaa
Determine rate limit via status codes
christeefy 872d6ee
✅ Update tests
christeefy 8875411
Limit visibility for GitHubRateLimitStatus::new
christeefy e9b0a90
Update tests
christeefy 3a55cf6
Merge branch 'main' into gh-fast-path/429
christeefy ab45228
Fix lint
christeefy 9d94d9b
Retrigger CI
christeefy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| use reqwest::{Response, StatusCode}; | ||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||
|
|
||
| /// A global state on whether we are being rate-limited by GitHub's REST API. | ||
| /// If we are, avoid "fast-path" attempts. | ||
| pub(crate) static GITHUB_RATE_LIMIT_STATUS: GitHubRateLimitStatus = GitHubRateLimitStatus::new(); | ||
|
|
||
| /// GitHub REST API rate limit status tracker. | ||
| /// | ||
| /// ## Assumptions | ||
| /// | ||
| /// The rate limit timeout duration is much longer than the runtime of a `uv` command. | ||
| /// And so we do not need to invalidate this state based on `x-ratelimit-reset`. | ||
| #[derive(Debug)] | ||
| pub(crate) struct GitHubRateLimitStatus(AtomicBool); | ||
|
|
||
| impl GitHubRateLimitStatus { | ||
| const fn new() -> Self { | ||
| Self(AtomicBool::new(false)) | ||
| } | ||
|
|
||
| pub(crate) fn activate(&self) { | ||
| self.0.store(true, Ordering::Relaxed); | ||
| } | ||
|
|
||
| pub(crate) fn is_active(&self) -> bool { | ||
| self.0.load(Ordering::Relaxed) | ||
| } | ||
| } | ||
|
|
||
| /// Determine if GitHub is applying rate-limiting based on the response | ||
| pub(crate) fn is_github_rate_limited(response: &Response) -> bool { | ||
| // HTTP 403 and 429 are possible status codes in the event of a primary or secondary rate limit. | ||
| // Source: https://docs.github.com/en/rest/using-the-rest-api/troubleshooting-the-rest-api?apiVersion=2022-11-28#rate-limit-errors | ||
| let status_code = response.status(); | ||
| status_code == StatusCode::FORBIDDEN || status_code == StatusCode::TOO_MANY_REQUESTS | ||
| } |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2066,6 +2066,64 @@ fn install_git_public_https_missing_branch_or_tag() { | |
| "###); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[cfg(feature = "git")] | ||
| async fn install_git_public_rate_limited_by_github_rest_api_403_response() { | ||
| let context = TestContext::new("3.12"); | ||
|
|
||
| let server = MockServer::start().await; | ||
| Mock::given(method("GET")) | ||
| .respond_with(ResponseTemplate::new(403)) | ||
| .expect(1) | ||
| .mount(&server) | ||
| .await; | ||
|
|
||
| uv_snapshot!(context.filters(), context | ||
| .pip_install() | ||
| .arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage") | ||
| .env("UV_GITHUB_FAST_PATH_URL", server.uri()), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
|
|
||
| ----- stderr ----- | ||
| Resolved 1 package in [TIME] | ||
| Prepared 1 package in [TIME] | ||
| Installed 1 package in [TIME] | ||
| + uv-public-pypackage==0.1.0 (from git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389) | ||
| "); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[cfg(feature = "git")] | ||
| async fn install_git_public_rate_limited_by_github_rest_api_429_response() { | ||
| use uv_client::DEFAULT_RETRIES; | ||
|
|
||
| let context = TestContext::new("3.12"); | ||
|
|
||
| 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; | ||
|
Comment on lines
+2104
to
+2109
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work on the testing strategy here! |
||
|
|
||
| uv_snapshot!(context.filters(), context | ||
| .pip_install() | ||
| .arg("uv-public-pypackage @ git+https://github.com/astral-test/uv-public-pypackage") | ||
| .env("UV_GITHUB_FAST_PATH_URL", server.uri()), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
|
|
||
| ----- stderr ----- | ||
| Resolved 1 package in [TIME] | ||
| Prepared 1 package in [TIME] | ||
| Installed 1 package in [TIME] | ||
| + uv-public-pypackage==0.1.0 (from git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389) | ||
| "); | ||
| } | ||
|
|
||
| /// Install a package from a public GitHub repository at a ref that does not exist | ||
| #[test] | ||
| #[cfg(feature = "git")] | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should document what shape of URL is expected, is there something that GitHub considers as API root that users could exchange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_URLto be even more explicit, assuming we are okay withUV_TEST_*appearing in non-test files.Please let me know what you think.