-
Notifications
You must be signed in to change notification settings - Fork 430
Configure the API client to retry more often when in test mode #3135
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
Conversation
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 configures the API client to use different retry configurations based on whether the action is running in test mode. In test mode, the retry count increases from 3 to 10 attempts to help mitigate rate limiting issues during PR checks.
- Adds a new
getRetryConfig()function that returns different retry configurations based on test mode - Updates
createApiClientWithDetailsto use the dynamic retry configuration - Updates test to verify the retry configuration is properly applied
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/api-client.ts | Adds getRetryConfig() function and applies retry config to API client |
| src/api-client.test.ts | Updates test to verify retry configuration is applied |
| lib/*.js | Generated JavaScript files reflecting the TypeScript changes |
src/api-client.ts
Outdated
| } | ||
|
|
||
| export function getRetryConfig(): { retries: number } { | ||
| return isInTestMode() ? { retries: 10 } : { retries: 3 }; |
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.
This should have some explicit, and significant backoff numbers, or at least a comment about the backoff strategy. As is, we easily risk making the rate limit situation worse IMO.
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.
Since the change here only matters for our CI, I figured we'd ship-to-learn to see how this goes with the default retryAfterBaseValue, rather than trying to nail down a good configuration.
However, I have now pushed a commit to increase the retryAfterBaseValue for test mode, made it explicit for both configurations, and added a comment explaining how the plugin uses it.
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.
OK. Depending on +/-1 retry attempts, the total wait can span just over one hour, so this is guaranteed or at least likely to see a reset.
Alternatively we could recognize the failure as being rate-limit related and act accordingly.
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.
Alternatively we could recognize the failure as being rate-limit related and act accordingly.
That seems like a more involved change. In the long run, we probably want a more robust solution for our CI here (e.g. a token with higher rate limits).
At the same time, we need to be careful not to affect "normal" CodeQL runs, because we don't want them to be stuck retrying API calls for long.
henrymercer
left a comment
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'm unsure how much this will help, and have some concerns about increased Actions usage, but happy to give it a go.
One area that might be worth investigating is whether we can combine more of our existing PR checks into the same jobs so that we only need to download the CodeQL distribution once.
Or, as a more involved step, we could investigate caching the nightly CodeQL distribution in the Actions cache. But this probably isn't worth it.
|
It doesn't seem like any of us are really convinced that this would really be a useful change to help with the rate limiting issues. I'll close this PR for now since we have merged other improvements, and still have ideas for other things we can try. |
Configures
createApiClientWithDetailswith aretryconfig that will attempt more retries, when in test mode. This is to try and work around rate limits on this repository when running a lot of PR checks.Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist