-
-
Notifications
You must be signed in to change notification settings - Fork 20
Description
Background:
PR Monitor builds a ThrottledOctokit and specifies an onAbuseLimit retry method: https://github.com/fwouts/prmonitor/blob/master/src/github-api/implementation.ts#L17
Github updated their "abuse" rate limits to be "secondary" rate limits at some point in the recent past. Current documentation: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits
In octokit/plugin-throttling v3.5.2, the message-matching for onAbuseLimit was updated to match the new error message, which no longer contains the word "abuse": octokit/plugin-throttling.js@6c361d4
octokit/plugin-throttling updated to completely replace abuse-handling with secondary-rate-limit handling and indicate the onAbuseLimit method is deprecated in release v3.6: octokit/plugin-throttling.js#458 on March 2, 2022.
When a user encounters the secondary rate limit, they receive a 403 response and a JSON error:
{
"message": "You have exceeded a secondary rate limit. Please wait a few minutes before you try again."
}
Issue:
PR Monitor is currently built using octokit/plugin-throttling v3.5.1, which still looks for "abuse" in the error message response from Github: https://github.com/octokit/plugin-throttling.js/blob/c9c3bf68c663889e36389d170746db6dfe86cbb8/src/index.ts#L124
As a result, PR Monitor is not handling the secondary-rate-limits using the onAbuseLimit method as expected. Instead, errors appear in the console - and I believe PR Monitor will continue to retry, although I need to confirm exactly how that works.
Our team uses PR Monitor widely (thank you!) and yesterday encountered the secondary rate limit across most of the team. I suspect this is due to the number of users/repos/PRs/comments causing a large number of requests, but I think the error was exacerbated by the unhandled secondary-limit and PR Monitor struggled to get out of the secondary-limit's retry window.
Recommended Solution:
Upgrade octokit/plugin-throttling to at least v3.6 and implement the new onSecondaryRateLimit() handler.