-
-
Notifications
You must be signed in to change notification settings - Fork 13
Fix: throttle requests to Github #94
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
base: main
Are you sure you want to change the base?
Conversation
722c8ee to
3e84716
Compare
bb769d9 to
a436012
Compare
|
Sorry about the thrash on this PR, I struggled to get Octokit to play nicely with tests and Typescript. All of Octokit is ESM and this repo is commonjs. |
f814c84 to
44d8130
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements request throttling and retry logic for GitHub API calls to prevent rate limit errors when performing large operations. The change replaces the bare-bones @octokit/core with custom throttling and retry functionality while maintaining compatibility with the existing tsconfig.
- Replaced direct
@octokit/coreusage with a customOctokitClientwrapper that includes throttling and retry plugins - Added comprehensive Jest mocks for the new Octokit dependencies to support testing
- Updated package dependencies to include the required Octokit plugins
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/repositoryConnection/RepositoryConnection.ts | Updated import to use custom OctokitClient wrapper |
| src/repositoryConnection/QuartzSyncerSiteManager.ts | Added mode property to ContentTreeItem type |
| src/repositoryConnection/OctokitClient.ts | New custom Octokit client with throttling and retry configuration |
| package.json | Updated dependencies to include Octokit plugins and core library |
| mocks/* | Added Jest mocks for new Octokit dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| timeout: 5000, | ||
| }, | ||
| retry: { | ||
| doNotRetry: ["429"], |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retry configuration excludes 429 (rate limit) errors from retries, but the throttling plugin is designed to handle rate limits by waiting and retrying. This configuration conflicts with the throttling behavior and may prevent proper rate limit handling.
| doNotRetry: ["429"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Copilot is correct here, this was probably a debugging error. I'll have to test this again.
| console.warn( | ||
| `Rate limit hit for ${options.method} ${options.url}, retrying in ${retryAfter}s`, | ||
| ); |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using console.warn directly may not integrate well with the existing logging system. Consider using the Logger import from js-logger that's used elsewhere in the codebase for consistent logging.
| console.warn( | ||
| `Abuse limit hit for ${options.method} ${options.url}, retrying in ${retryAfter}s`, | ||
| ); |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using console.warn directly may not integrate well with the existing logging system. Consider using the Logger import from js-logger that's used elsewhere in the codebase for consistent logging.
|
Hey, Thank you very much for this PR, and apologies for the delayed response. This PR looks very promising. I have some small remarks regarding logging, but I'm inclined to just merge this after testing and do some minor changes before I publish the next release. I'll run some tests tomorrow. |
This is something I eventually want to resolve properly, so I can just target ESM fully. As you have experienced, the current testing setup causes issues in that regard. |
|
I ran a quick test where I had to update 387 files. It was successful, but took 8 minutes. Definitely a bit of a chore, but at least I didn't have to deal with errors. |
Closes #88
@octokit/coreis a bare bones Github API library, whereasoctokitis batteries included, meaning that retry logic and throttling is baked-in. I would love to use that, but it causes issues with your tsconfig and how your modules are setup. Instead, I just replicated the throttling and retry logic inside ofOctokitClient.These changes will prevent the the plugin from hitting rate limits when performing large actions and allow for request retrying if things fail.
I think there will be a bit of a performance hit here when uploading a small number of files. Github has built this to abide by their own rate limit, so they seem more conservative.
For a small number of files, 1 write per second is going to be slower than the "try them all at once" approach that the plugin currently takes, but also means rate limit errors should pretty much never occur.