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.

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

When retrying based on x-ratelimit-reset, take local time and server time into account #599

Open
gr2m opened this issue May 30, 2023 · 4 comments
Assignees
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects

Comments

@gr2m
Copy link
Contributor

gr2m commented May 30, 2023

I've seen the plugin handle the primary rate limit correctly, wait until the provided x-ratelimit-reset time, and then immediately hit the rate limit again. My guess is that local time and server time are out-of-sync. It's something we had to take into account for the timeouts of installation access tokens as well in https://github.com/octokit/auth-app.js.

@ghost ghost added this to Inbox in JS May 30, 2023
@kfcampbell kfcampbell added Priority: Normal Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented labels Jun 1, 2023
@ghost ghost moved this from Inbox to Bugs in JS Jun 1, 2023
@gr2m gr2m self-assigned this Aug 7, 2023
@ghost ghost moved this from Bugs to In progress in JS Aug 7, 2023
@gr2m
Copy link
Contributor Author

gr2m commented Aug 7, 2023

A mentee of mine ran into the problem using code that I haven't changed for a long time. It sure feels like something changed: Rutasd/hearted_contributions#1 (comment)

I'll take care of the date sync but also add a few seconds extra buffer

@gr2m
Copy link
Contributor Author

gr2m commented Aug 11, 2023

for reference, this is how we account for time skew in @octokit/auth-app: https://github.com/octokit/auth-app.js/blob/fdfdcc294bc5fc27eeed86d4a3d0915826fdd553/src/hook.ts#L48-L80

@jyasskin
Copy link
Contributor

I also saw the identical Date and x-ratelimit-reset headers (Mon, 23 Oct 2023 21:48:42 GMT and 1698097722) in https://github.com/jyasskin/spec-maintenance/actions/runs/6618851077/job/17978147853. Both clocks are on Github infrastructure, so it's unlikely they're out of sync, although it's possible. https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#exceeding-the-rate-limit says "you should not try your request until after the time specified by the x-ratelimit-reset time" (my emphasis). So we probably ought to add a second to the time. We could also use the date header instead of Date.now(), but that doesn't seem like it was the cause of either of these examples.

@gr2m
Copy link
Contributor Author

gr2m commented Oct 24, 2023

you should not try your request until after the time specified by the x-ratelimit-reset time

I think we should totally add a second. Does not implement the fix described in this issue, but it's much simpler and I think we should do it. 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
No open projects
JS
  
In progress
Development

No branches or pull requests

4 participants