fix(utils): honor retry-after for translation retries#2279
Open
g9420 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds support for respecting HTTP ChangesRetry-After header support
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/utils/src/translate/index.ts (1)
122-125: ⚡ Quick winConsider supporting Retry-After for 503 responses.
The current implementation only honors
Retry-Afterfor429rate-limit responses. The HTTP specification also allowsRetry-Afterwith503 Service Unavailableresponses, which is commonly used during maintenance windows or temporary outages.♻️ Proposed enhancement to support 503
const retryDelay = calculateRetryDelay( attempt, - response.status === 429 ? response.headers.get('Retry-After') : null, + response.status === 429 || response.status === 503 + ? response.headers.get('Retry-After') + : null, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/src/translate/index.ts` around lines 122 - 125, The retry delay calculation currently only passes the 'Retry-After' header when response.status === 429; update the conditional in the call to calculateRetryDelay so it also passes the header when response.status === 503 (i.e., check for 429 || 503 or otherwise include 503). Locate the call to calculateRetryDelay in translate/index.ts and ensure the second argument uses response.headers.get('Retry-After') for both 429 and 503 responses so maintenance/outage 503 responses honor Retry-After.packages/utils/src/translate.test.ts (1)
17-45: Test correctly validates retry timing; consider adding HTTP-date format coverage.The test properly verifies that a
429response withRetry-After: 60waits the full 60 seconds before retrying. The use of fake timers and the check at 1 second confirms no early retry occurs.Consider adding a second test case to verify the HTTP-date format for
Retry-After, sinceparseRetryAfterDelaysupports both formats:📋 Example test for HTTP-date format
it('should wait for Retry-After HTTP-date before retrying a 429 response', async () => { const futureDate = new Date(Date.now() + 30000).toUTCString(); const fetchMock = vi .fn() .mockResolvedValueOnce({ ok: false, status: 429, headers: new Headers({ 'Retry-After': futureDate }), }) .mockResolvedValueOnce({ ok: true, status: 200, json: async () => [[['hola']]], }); vi.stubGlobal('fetch', fetchMock); const result = translateText('hello', 'es'); await Promise.resolve(); expect(fetchMock).toHaveBeenCalledTimes(1); await vi.advanceTimersByTimeAsync(29000); expect(fetchMock).toHaveBeenCalledTimes(1); await vi.advanceTimersByTimeAsync(1000); await expect(result).resolves.toBe('hola'); expect(fetchMock).toHaveBeenCalledTimes(2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/utils/src/translate.test.ts` around lines 17 - 45, Add a second unit test that mirrors the existing 429/Retry-After behavior but supplies an HTTP-date string in the Retry-After header to exercise parseRetryAfterDelay's date parsing; in the test create a futureDate via new Date(Date.now() + 30000).toUTCString(), mock the first fetch to return status 429 with that header and the second to return ok 200 with the expected JSON, stubGlobal('fetch'), call translateText('hello', 'es'), advance fake timers until just before the date then past it to assert no early retry and that the final result resolves to 'hola' and fetch was called twice (referencing parseRetryAfterDelay and translateText to locate the logic to cover).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/utils/src/translate/index.ts`:
- Around line 17-43: The parseRetryAfterDelay function currently returns parsed
Retry-After delays unbounded, allowing values larger than the configured cap;
update parseRetryAfterDelay (used by calculateRetryDelay) to clamp returned
delays to RETRY_CONFIG.maxDelay by returning Math.min(parsedDelay,
RETRY_CONFIG.maxDelay) for both numeric-second and HTTP-date branches so
Retry-After values cannot exceed the global maxDelay.
---
Nitpick comments:
In `@packages/utils/src/translate.test.ts`:
- Around line 17-45: Add a second unit test that mirrors the existing
429/Retry-After behavior but supplies an HTTP-date string in the Retry-After
header to exercise parseRetryAfterDelay's date parsing; in the test create a
futureDate via new Date(Date.now() + 30000).toUTCString(), mock the first fetch
to return status 429 with that header and the second to return ok 200 with the
expected JSON, stubGlobal('fetch'), call translateText('hello', 'es'), advance
fake timers until just before the date then past it to assert no early retry and
that the final result resolves to 'hola' and fetch was called twice (referencing
parseRetryAfterDelay and translateText to locate the logic to cover).
In `@packages/utils/src/translate/index.ts`:
- Around line 122-125: The retry delay calculation currently only passes the
'Retry-After' header when response.status === 429; update the conditional in the
call to calculateRetryDelay so it also passes the header when response.status
=== 503 (i.e., check for 429 || 503 or otherwise include 503). Locate the call
to calculateRetryDelay in translate/index.ts and ensure the second argument uses
response.headers.get('Retry-After') for both 429 and 503 responses so
maintenance/outage 503 responses honor Retry-After.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d2add22-96a1-41ed-90ee-da7ef2b6486b
📒 Files selected for processing (2)
packages/utils/src/translate.test.tspackages/utils/src/translate/index.ts
Comment on lines
17
to
+43
| /** | ||
| * Calculate retry delay time with exponential backoff | ||
| * Parse Retry-After header value. | ||
| * @param retryAfter Retry-After header value | ||
| * @returns Delay time in milliseconds, or null if the header is invalid | ||
| */ | ||
| const parseRetryAfterDelay = (retryAfter: string | null): number | null => { | ||
| if (!retryAfter) { | ||
| return null; | ||
| } | ||
|
|
||
| const retryAfterValue = retryAfter.trim(); | ||
| if (!retryAfterValue) { | ||
| return null; | ||
| } | ||
|
|
||
| const retryAfterSeconds = Number(retryAfterValue); | ||
| if (Number.isFinite(retryAfterSeconds)) { | ||
| return retryAfterSeconds >= 0 ? retryAfterSeconds * 1000 : null; | ||
| } | ||
|
|
||
| const retryAfterDate = Date.parse(retryAfterValue); | ||
| if (!Number.isNaN(retryAfterDate)) { | ||
| return Math.max(retryAfterDate - Date.now(), 0); | ||
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for timeout configurations or retry budget limits that might bound this behavior
# Search for timeout configurations in the codebase
rg -n -C3 'timeout|TimeoutError|AbortSignal' --type=ts --glob '!node_modules'
# Search for any retry budget or circuit breaker patterns
rg -n -C3 'retry.*budget|circuit.*breaker|max.*wait|timeout.*config' --type=ts --glob '!node_modules'Repository: refly-ai/refly
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "parseRetryAfterDelay|RETRY_CONFIG|maxDelay|Retry-After" packages/utils/src/translate/index.ts
echo "---- context around RETRY_CONFIG ----"
rg -n "RETRY_CONFIG" packages/utils/src/translate/index.ts -n
echo "---- printing translate index.ts head (1-200) ----"
sed -n '1,220p' packages/utils/src/translate/index.tsRepository: refly-ai/refly
Length of output: 8596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "parseRetryAfterDelay\(" packages/utils/src/translate/index.ts
rg -n "calculateRetryDelay\(" packages/utils/src/translate/index.ts
rg -n "maxDelay" packages/utils/src/translate/index.tsRepository: refly-ai/refly
Length of output: 348
Cap parsed Retry-After delay to RETRY_CONFIG.maxDelay
calculateRetryDelay returns the parsed Retry-After value as-is when valid, bypassing the Math.min(..., RETRY_CONFIG.maxDelay) cap that’s applied only to the exponential-backoff path—so Retry-After: 3600 (or a far-future HTTP date) can produce waits far longer than the configured 10s. Consider return Math.min(retryAfterDelay, RETRY_CONFIG.maxDelay) (or document intentional unbounded behavior).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/utils/src/translate/index.ts` around lines 17 - 43, The
parseRetryAfterDelay function currently returns parsed Retry-After delays
unbounded, allowing values larger than the configured cap; update
parseRetryAfterDelay (used by calculateRetryDelay) to clamp returned delays to
RETRY_CONFIG.maxDelay by returning Math.min(parsedDelay, RETRY_CONFIG.maxDelay)
for both numeric-second and HTTP-date branches so Retry-After values cannot
exceed the global maxDelay.
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
2 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.
Fixes #2278
Summary
Retry-Afterfor 429 translation retry responsesRetry-AftervaluesRetry-Afteris missing or invalidTests
pnpm --filter @refly/utils exec vitest run src/translate.test.tspnpm exec biome check packages/utils/src/translate/index.ts packages/utils/src/translate.test.tsSummary by CodeRabbit
Retry-Afterheaders from rate-limit and server error responses, implementing intelligent retry delays. Falls back to exponential backoff when headers are unavailable, improving reliability during high-load scenarios.