-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Don't retry 429s returned from the cache service #2243
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 modifies the cache service client to stop retrying 429 (Too Many Requests) responses from the cache service. Previously, 429s were treated as retryable errors, but this change makes them fail immediately to prevent slowing down runs since cache is a best-effort service.
Changes:
- Removed
HttpCodes.TooManyRequestsfrom the list of retryable status codes - Added special handling for 429 responses to warn users about rate limits and throw immediately
- Added comprehensive test coverage for the new 429 handling behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cache/src/internal/shared/cacheTwirpClient.ts | Removed 429 from retryable status codes and added special handling to detect retry-after headers and warn users before throwing |
| packages/cache/tests/cacheTwirpClient.test.ts | Added comprehensive tests validating that 429s fail immediately without retries and properly handle retry-after headers |
| packages/cache/package.json | Bumped version from 5.0.2 to 5.0.3 |
| packages/cache/RELEASES.md | Added release notes for 5.0.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Bassem Dghaidi <568794+Link-@users.noreply.github.com>
Added a comment regarding rate limiting and retry behavior.
|
For posterity, we're not retrying on the retry after schedule to prevent workflow run delays. It's best to fail fast in this case. We might want to make this configurable though. |
These are only returned in exceptional situations, it's better to not slow down the run retrying these when cache is best effort.