Skip git fetch when rate-limited by GitHub

[christeefy]

Summary

Fixes #12761 and #12746.

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]

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]

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]

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]

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:

1. 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"
2. 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]

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]

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]

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]

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]

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

[christeefy]

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