Callback function doesn't trigger after secondary rate limit is hit

[johnmcollier]

Hi,

I have some code that intiializes a go-github client using go-github-ratelimiter, with a custom callback function defined like so:

rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(tc.Transport, github_ratelimit.WithLimitDetectedCallback(func(cbContext *github_ratelimit.CallbackContext) {
		ctx := *cbContext.UserContext
		// Some other steps
}))
client := github.NewClient(rateLimiter)

But despite using a GitHub token that is secondary rate limited (for content creation), I don't ever see the callback function run.

I'm using github.com/gofri/go-github-ratelimit v1.0.2 and github.com/google/go-github/v41 v41.0.0

[johnmcollier]

I set some break points and found that despite a 403 error being returned, with the "You have exceeded a secondary rate limit" error message, there is no Retry-After header set. This is because GitHub may not always set a Retry-After after in the response, and you may need to look at x-ratelimit-reset instead:

https://docs.github.com/en/rest/guides/best-practices-for-integrators?apiVersion=2022-11-28#dealing-with-secondary-rate-limits

Would it be possible for go-github-ratelimit to also check x-ratelimit-reset when waiting?

[gofri]

Hey @johnmcollier, thanks for the detailed report!
Actually, I mentioned this issue in #6, but haven't encountered it myself.
I hope to get to fixing it sometime this week. Meanwhile, could you please elaborate on the context?
e.g. the specific API endpoint, is it on GitHub.com or a private instance (and if so, which version), etc.
Thanks !

It would also be useful to have the full set of response headers; x-ratelimit-reset is used for the primary rate limit too, which is categorized. Treating primary rate limit the same as a secondary may prevent users from using all API endpoints, when some endpoints are still available.

[johnmcollier]

Hey @gofri sure thing. Thanks for the prompt response!

I'm hitting the secondary rate limiting on repository creation. It's on GitHub.com, specifically the https://api.github.com/user/repos endpoint. And I've printed the full list of headers:

map[Access-Control-Allow-Origin:[*] Access-Control-Expose-Headers:[ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset] Content-Security-Policy:[default-src 'none'] Content-Type:[application/json; charset=utf-8] Date:[Fri, 28 Apr 2023 17:14:51 GMT] Github-Authentication-Token-Expiration:[2023-05-28 13:55:16 UTC] Referrer-Policy:[origin-when-cross-origin, strict-origin-when-cross-origin] Server:[GitHub.com] Strict-Transport-Security:[max-age=31536000; includeSubdomains; preload] Vary:[Accept-Encoding, Accept, X-Requested-With] X-Accepted-Oauth-Scopes:[public_repo, repo] X-Content-Type-Options:[nosniff] X-Frame-Options:[deny] X-Github-Api-Version-Selected:[2022-11-28] X-Github-Media-Type:[github.v3; param=baptiste-preview.nebula-preview; format=json] X-Github-Request-Id:[841F:978C:10E779:22A133:644BFF0B] X-Oauth-Scopes:[delete_repo, repo] X-Ratelimit-Limit:[5000] X-Ratelimit-Remaining:[4840] X-Ratelimit-Reset:[1682702662] X-Ratelimit-Resource:[core] X-Ratelimit-Used:[160] X-Xss-Protection:[0]]

[gofri]

Thanks @johnmcollier! Looks like a legit secondary rate limit indeed. Just to be sure, the response body has the message as described in the documentation, right?

p.s. I noticed in your comment the undocumented x-ratelimit-reaource header there. Looks like it'd be very useful for implementing primary rate limit handling in a simple and hermetic way. Thanks for that 💪
@gmlewis I believe you'll find that new header useful too (also, this edge case for setting the secondary rate limit timeout, I guess)

[johnmcollier]

Yup, I can confirm I get the following message in the body:

2023/04/28 13:03:39 POST https://api.github.com/user/repos: 403 You have exceeded a secondary rate limit and have been temporarily blocked from content creation. Please retry your request again later.

[gmlewis]

@gmlewis I believe you'll find that new header useful too (also, this edge case for setting the secondary rate limit timeout, I guess)

Cool! PRs are always welcome. 😂

[gofri]

@gmlewis duly noted 😅

@johnmcollier
I need to add tests for the new case before I merge it, but I think the fix on this branch should work for you:
gofri/handle-x-ratelimit
you can test it by updating the dependency to this specific commit in your repo (I'll create a new tag after the merge):
go get -u github.com/gofri/go-github-ratelimit@a500e14de53fb61009cf390956068109b0f4a7db

[johnmcollier]

It works, thanks a ton! Looking forward to seeing it in main 😃

[gofri]

https://github.com/gofri/go-github-ratelimit/releases/tag/v1.0.3
😄