Accept 429 response codes as an indication of the secondary rate limit being exceeded #1895

holly-cummins · 2024-07-04T18:11:17Z

Resolves #1842.
Resolves #1805.

Description

If a user uses this library to drive the GitHub API hard, there will quite often be failures caused by secondary rate limits being breached (#1842 and #1805). There’s been some discussion of headers on #1842, and also of pluggable 'handle the rate limit' modules, but I think there’s a more fundamental problem with the handling of rate limits. (The good news is this means there's a simpler solution, too.)

The AbuseLimitHandler checks for a rate limit violation by looking for a 403 header and some headers:  

@Override
boolean isError(@Nonnull GitHubConnectorResponse connectorResponse) {
    return isForbidden(connectorResponse) && hasRetryOrLimitHeader(connectorResponse);
}

HTTP_FORBIDDEN is a 403. The GitHub docs indicate that limits from GitHub are signalled with either 403 or TOO_MANY_REQUESTS responses (429). In my experience, I've only seen 429s.

What about headers?

The GitHub docs docs suggest that any headers are optional, and that in the absence of headers the code should just wait a minute (which, luckily, is what the current implementation already does):

If the retry-after response header is present, you should not retry your request until after that many seconds has elapsed. If the x-ratelimit-remaining header is 0, you should not retry your request until after the time, in UTC epoch seconds, specified by the x-ratelimit-reset header. Otherwise, wait for at least one minute before retrying. If your request continues to fail due to a secondary rate limit, wait for an exponentially increasing amount of time between retries, and throw an error after a specific number of retries.

Based on these docs, I think it's sufficient to count a 429 as a rate limit response, even if headers are missing. Semantically, a 429 is an indication of an abuse detection/rate limit scenario, and should be handled as such.

What about the rate limit handler?

GitHubRateLimitHandler also requires a 403 to 'count' a response as rate limited:

@Override
boolean isError(@NotNull GitHubConnectorResponse connectorResponse) throws IOException {
connectorResponse.statusCode() == HttpURLConnection.HTTP_FORBIDDEN
            && "0".equals(connectorResponse.header("X-RateLimit-Remaining"));

I didn't update this one, since the secondary rate limit checks are now correctly being handled on the abuse path with my changes. It might be worth updating it, or also even considering whether the distinction between the abuse and rate limit paths is so fuzzy that having both is just maintenance overhead. But that’s a different discussion.

Before submitting a PR:

[ X] Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
[ X] Add JavaDocs and other comments explaining the behavior.
When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
[ X] Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

Fill in the "Description" above with clear summary of the changes. This includes:
- [X ] If this PR fixes one or more issues, include "Fixes #" lines for each issue.
- [ X] Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
[X ] All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
Enable "Allow edits from maintainers".

bitwiseman

This is a definite improvement.

It still needs tests. Look at the existing abuse limit tests, which use manually constructed test data. Maybe just add one more step to the existing test rather than adding a whole new test. Up to you.

src/main/java/org/kohsuke/github/GitHubAbuseLimitHandler.java

bitwiseman · 2024-07-07T17:16:36Z

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

@@ -89,7 +89,7 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException {
        private long parseWaitTime(HttpURLConnection uc) {
            String v = uc.getHeaderField("Retry-After");
            if (v == null)
-                return 60 * 1000; // can't tell, return 1 min
+                return 61 * 1000; // can't tell, return 1 min plus 1 second for luck


Is there a functional reason for this?

The GitHub docs say "Otherwise, wait for at least one minute before retrying." I never know if the check is < 60s or <=60s, but it seems like waiting something that is unambiguously more than a minute is safest.

codecov · 2024-07-07T17:23:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.36%. Comparing base (7db2ec0) to head (caa5167).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1895      +/-   ##
============================================
+ Coverage     81.32%   81.36%   +0.03%     
- Complexity     2458     2463       +5     
============================================
  Files           239      239              
  Lines          7464     7469       +5     
  Branches        400      401       +1     
============================================
+ Hits           6070     6077       +7     
+ Misses         1147     1146       -1     
+ Partials        247      246       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

holly-cummins · 2024-07-09T08:34:49Z

Oh good, I'm glad it looks sensible. I've added some tests and squashed.

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

Co-Authored-By: Liam Newman <[email protected]>

bitwiseman · 2024-07-16T17:04:30Z

Please don't squash/force-push. We can squash when merging the PR.

bitwiseman

Test failure needs fixing.

… wiremock

holly-cummins · 2024-07-17T21:24:57Z

Test failure needs fixing.

Oops, sorry about that. And noted about the squashing - different orgs have different requirements, so I have a squash habit, but I'll keep the commits separate. I agree it does make subsequent reviews easier!

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java

…ere retry-after is a date, and light refactoring

holly-cummins · 2024-07-30T20:01:31Z

The CI's failing because I didn't have enough coverage, even with the new test. To get the parseWaitTime method to be testable, I had to do a bit of refactoring to make it visible to the test code. I also added a new test to cover both paths, and then I realised that the original code could fail with a NumberFormatException, because Retry-After can be a date. So I added new code and tests to handle that case. I also did a bit of refactoring of the test code and removing assertions about the mock that I was pretty sure we definitely didn't care about.

bitwiseman

Sorry for the slow response cycle.

bitwiseman · 2024-08-05T16:53:43Z

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

+        } catch (NumberFormatException nfe) {
+            // The retry-after header could be a number in seconds, or an http-date
+            ZonedDateTime zdt = ZonedDateTime.parse(v, DateTimeFormatter.RFC_1123_DATE_TIME);
+            return ChronoUnit.MILLIS.between(ZonedDateTime.now(), zdt);


GitHubClient has a method to get Date (or Instant).
https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubClient.java#L916

Unfortunately, diff from local machine time is not reliable. The local machine may not be synchronized with the server time. We have to use the diff from the response time.
https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GHRateLimit.java#L543-L571

However, this PR is a huge step forward and I'd rather have this less than perfect code added than wait for the next release.

I'll approve and file an issue to fix this.

Thanks! (And thanks for explaining about the diff to the response time, that makes a lot of sense.)

Sure thing. If you have time to circle back and address #1909 , that would be awesome. Thanks for the great contribution!

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java

holly-cummins changed the title ~~Accept 429 as an indication of the rate limit~~ Accept 429 as an indication of the secondary rate limit being exceeded Jul 4, 2024

holly-cummins changed the title ~~Accept 429 as an indication of the secondary rate limit being exceeded~~ Accept 429 response codes as an indication of the secondary rate limit being exceeded Jul 4, 2024

bitwiseman requested changes Jul 7, 2024

View reviewed changes

holly-cummins force-pushed the accept-429 branch from 0683320 to abee4fe Compare July 8, 2024 20:50

holly-cummins requested a review from bitwiseman July 9, 2024 08:35

bitwiseman approved these changes Jul 10, 2024

View reviewed changes

src/main/java/org/kohsuke/github/AbuseLimitHandler.java Outdated Show resolved Hide resolved

Accept 429 as an indication of the rate limit

e6fe5c9

Co-Authored-By: Liam Newman <[email protected]>

holly-cummins force-pushed the accept-429 branch from 1c182a9 to e6fe5c9 Compare July 10, 2024 16:45

bitwiseman self-requested a review July 17, 2024 06:41

bitwiseman requested changes Jul 17, 2024

View reviewed changes

Remove assertion which is mostly asserting about the configuration of…

f17f206

… wiremock

bitwiseman reviewed Jul 21, 2024

View reviewed changes

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java Outdated Show resolved Hide resolved

Add extra test to bring coverage above threshold, also handle case wh…

34c4a8e

…ere retry-after is a date, and light refactoring

Reformat header to resolve warning

ee276ad

holly-cummins requested a review from bitwiseman August 12, 2024 16:13

bitwiseman approved these changes Aug 18, 2024

View reviewed changes

Update src/main/java/org/kohsuke/github/AbuseLimitHandler.java

8d57cd4

bitwiseman reviewed Aug 18, 2024

View reviewed changes

src/main/java/org/kohsuke/github/AbuseLimitHandler.java Outdated Show resolved Hide resolved

Update src/main/java/org/kohsuke/github/AbuseLimitHandler.java

c6075e6

bitwiseman reviewed Aug 18, 2024

View reviewed changes

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java Show resolved Hide resolved

bitwiseman reviewed Aug 18, 2024

View reviewed changes

src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java Show resolved Hide resolved

bitwiseman added 2 commits August 18, 2024 13:09

Apply suggestions from code review

098fc48

Merge branch 'main' into accept-429

caa5167

bitwiseman merged commit 314917e into hub4j:main Aug 18, 2024
12 checks passed

bitwiseman mentioned this pull request Aug 19, 2024

AbuseLimitHandler does not handle scenarios where the local time is not synchronized with GitHub server time #1909

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept 429 response codes as an indication of the secondary rate limit being exceeded #1895

Accept 429 response codes as an indication of the secondary rate limit being exceeded #1895

holly-cummins commented Jul 4, 2024

bitwiseman left a comment

bitwiseman Jul 7, 2024

holly-cummins Jul 8, 2024

codecov bot commented Jul 7, 2024 •

edited

Loading

holly-cummins commented Jul 9, 2024

bitwiseman commented Jul 16, 2024

bitwiseman left a comment

holly-cummins commented Jul 17, 2024

holly-cummins commented Jul 30, 2024

bitwiseman left a comment

bitwiseman Aug 5, 2024

holly-cummins Aug 29, 2024

bitwiseman Aug 30, 2024

Accept 429 response codes as an indication of the secondary rate limit being exceeded #1895

Accept 429 response codes as an indication of the secondary rate limit being exceeded #1895

Conversation

holly-cummins commented Jul 4, 2024

Description

What about headers?

What about the rate limit handler?

Before submitting a PR:

When creating a PR:

bitwiseman left a comment

Choose a reason for hiding this comment

bitwiseman Jul 7, 2024

Choose a reason for hiding this comment

holly-cummins Jul 8, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2024 • edited Loading

Codecov Report

holly-cummins commented Jul 9, 2024

bitwiseman commented Jul 16, 2024

bitwiseman left a comment

Choose a reason for hiding this comment

holly-cummins commented Jul 17, 2024

holly-cummins commented Jul 30, 2024

bitwiseman left a comment

Choose a reason for hiding this comment

bitwiseman Aug 5, 2024

Choose a reason for hiding this comment

holly-cummins Aug 29, 2024

Choose a reason for hiding this comment

bitwiseman Aug 30, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2024 •

edited

Loading