Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adjust GitHub rate limiting behavior. #1820

Merged
merged 2 commits into from
Jun 6, 2024
Merged

Adjust GitHub rate limiting behavior. #1820

merged 2 commits into from
Jun 6, 2024

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jun 2, 2024

This introduces two changes around GitHub rate limiting:

  • Retries are only performed with 403 or 429 responses. GitHub's docs state that rate-limits will only return those responses. Previously, the client would retry any non-successful request (like a 404), which was frequently causing several needless requests to GitHub.
  • Disable retries on the server. Since a rate-limit retry could take several minutes, that would almost certainly mean it would get cancelled due to the 10 second webhook rate limit. I don't think the other endpoints really need it either (a retry can sleep for a very long time). It is still enabled for the prioritization CLI tool.

ehuss added 2 commits June 1, 2024 17:27
@ehuss
Only retry on 403 FORBIDDEN or 429 TOO_MANY_REQUESTS
46af494 
GitHub's docs clearly state that it will only respond with 403 or 429
if you exceed the rate limit.
https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit

The current logic was triggering a retry for all non-success responses.
In some situations (like expected 404 responses), this was causing
triagebot to send 3 extra needless requests.
@ehuss
Disable GitHub retry on the server.
e5e4c92 
This disables the GitHub rate limit retry logic on the server. Generally
for webhooks, the server must respond within 10 seconds. If it ever
hits the rate limit, it will likely sleep for several minutes, meaning
it will be guaranteed to be killed.

Whether or not retries are enabled is now a property of `GithubClient`.
It is enabled for the CLI prioritization tool (which added this retry
logic in the first place in
rust-lang#579).
@ehuss ehuss merged commit 4fe2183 into rust-lang:master Jun 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ehuss @Mark-Simulacrum