feat: handle 429s without retry-after header as error#674
Merged
JustinBeckwith merged 2 commits intoJustinBeckwith:mainfrom Oct 20, 2025
Conversation
Contributor
Author
|
Hi @JustinBeckwith would you mind taking a look? :) |
1 similar comment
Contributor
Author
|
Hi @JustinBeckwith would you mind taking a look? :) |
4 tasks
|
Hi @JustinBeckwith! Would you mind taking a look? This is blocking our docs generation 😢 Or let us know if another solution would be better. Thank you! |
359e91b to
999b87e
Compare
JustinBeckwith
approved these changes
Oct 20, 2025
Owner
JustinBeckwith
left a comment
There was a problem hiding this comment.
Code Review ✅
I've rebased this PR against main and fixed the test failures. All changes look good!
What I Fixed
- Migrated from nock to undici: Updated the two new test cases to use
MockAgentinstead ofnock, which is consistent with the rest of the test suite - Rebased against main: Incorporated recent bot-protection changes from #709
PR Changes Review
The core functionality looks solid:
- 429 without retry-after handling: Correctly treats 429s without retry-after headers as retry errors when
retryErrorsis enabled - Test coverage: Added comprehensive tests for both retry success and retry exhaustion scenarios
- Documentation: Updated comments to clearly explain the new behavior
Test Results
- ✅ All 110 tests pass
- ✅ Linter passes (biome)
- ✅ No type errors
The implementation correctly handles the edge case where rate-limited responses don't include retry timing information, allowing users to leverage the existing retry-error mechanism.
999b87e to
67e8c8a
Compare
67e8c8a to
17a9694
Compare
JustinBeckwith
approved these changes
Oct 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are servers returning 429 status code but without retry-after header, so I am adding 429 status code as a retryable error.
Currently happening in several google cloud node libraries, for example: https://github.com/googleapis/gax-nodejs/actions/runs/16154578720/job/45594197234.
Request response:
headers: { 'content-length': '1474', 'content-security-policy': "default-src 'none'; style-src 'unsafe-inline'", 'content-type': 'text/html; charset=utf-8', date: 'Tue, 08 Jul 2025 23:48:08 GMT', server: 'Varnish', 'strict-transport-security': 'max-age=31536000; includeSubdomains; preload', 'x-content-type-options': 'nosniff', 'x-frame-options': 'deny', 'x-github-request-id': '', 'x-xss-protection': '1; mode=block' }, status: 429, statusText: 'Too Many Requests', request: { responseURL: 'https://github.com/googleapis/gax-nodejs/blob/chore--add-node-22-and-24-versions-in-ci-tests/LICENSE' }I was able to recreate the issue:
.
After the changes I don't see the error.
Note: I didn't add it in
shouldRetryAfterbecause I don't see a max retries logic there, also since this is a special case without retry-after data I think handle it as error is better.