feat: add llm retry helper#173
Conversation
📝 WalkthroughWalkthroughAdds LLM-aware retry enhancements: a new exported function 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Simplifies the implementation by using `String()` factory.
Reorganizes code for clarity, adds regex for status parsing, and uses `ms` for delay parsing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/retry.ts (1)
16-41: Avoid sleeping after the final attempt.The loop still sleeps (including long retry-after delays) even when
maxRetriesis exhausted, which unnecessarily stalls failure. Guard against delays once the last attempt has failed.🐛 Proposed fix
- if (shouldRetry === false) { - throw lastError; - } else if (typeof shouldRetry === "number" && Number.isFinite(shouldRetry)) { - await sleep(shouldRetry); - } else { - await sleep(delay); - delay *= 2; - } + if (shouldRetry === false || i >= options.maxRetries) { + throw lastError; + } + if (typeof shouldRetry === "number" && Number.isFinite(shouldRetry)) { + await sleep(shouldRetry); + } else { + await sleep(delay); + delay *= 2; + }
Ensures retries stop when max attempts are reached.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3df76f0cef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const maybeError = source as { status?: unknown; statusCode?: unknown; response?: { status?: unknown } }; | ||
| const candidates: unknown[] = [maybeError.status, maybeError.statusCode, maybeError.response?.status]; | ||
| for (const candidate of candidates) { | ||
| if (typeof candidate === "number") return candidate; | ||
| } |
There was a problem hiding this comment.
In getStatusFromSource, only numeric status/statusCode/response.status values are recognized. If a client surfaces these as strings (e.g. { status: "429" } or { statusCode: "503" }), checkLlmRetryableState will treat the error as non-retryable unless the message also contains a code, so transient 429/5xx responses won’t be retried. Consider accepting numeric strings (similar to getErrorStatus in src/error.ts) or normalizing these fields before the retry decision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid point — getStatusFromSource was only checking for numeric status values while getErrorStatus in src/error.ts already handled string status codes. Fixed in 29c4d32 by parsing string candidates with Number.parseInt, matching the existing pattern.
Align getStatusFromSource with getErrorStatus by also parsing string
status/statusCode values so that errors like { status: "429" } are
correctly recognized as retryable.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| // eslint-disable-next-line sonarjs/function-return-type | ||
| export function checkLlmRetryableState(error: unknown): boolean | number { | ||
| if (error instanceof SyntaxError || error instanceof TypeBoxError || error instanceof LogReturn) { | ||
| return true; | ||
| } | ||
|
|
||
| const status = extractStatus(error) ?? extractStatusFromMessage(error); | ||
| if (!status) return false; | ||
|
|
||
| if (status === 429) { | ||
| const headers = extractHeaders(error); | ||
| const tokenDelay = parseRetryAfter(headers, "x-ratelimit-reset-tokens"); | ||
| const requestDelay = parseRetryAfter(headers, "x-ratelimit-reset-requests"); | ||
| const retryAfter = parseRetryAfter(headers, "retry-after"); | ||
| const delays = [tokenDelay, requestDelay, retryAfter].filter((value): value is number => Number.isFinite(value)); | ||
| if (!delays.length) return true; | ||
| return Math.max(...delays); | ||
| } | ||
|
|
||
| return status >= 500; | ||
| } |
There was a problem hiding this comment.
Avoid treating status 0 as “missing.”
if (!status) will treat 0 as absent; some clients use 0 for network errors, which may deserve a retry. Consider a null/undefined check instead.
💡 Proposed fix
- if (!status) return false;
+ if (status == null) return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line sonarjs/function-return-type | |
| export function checkLlmRetryableState(error: unknown): boolean | number { | |
| if (error instanceof SyntaxError || error instanceof TypeBoxError || error instanceof LogReturn) { | |
| return true; | |
| } | |
| const status = extractStatus(error) ?? extractStatusFromMessage(error); | |
| if (!status) return false; | |
| if (status === 429) { | |
| const headers = extractHeaders(error); | |
| const tokenDelay = parseRetryAfter(headers, "x-ratelimit-reset-tokens"); | |
| const requestDelay = parseRetryAfter(headers, "x-ratelimit-reset-requests"); | |
| const retryAfter = parseRetryAfter(headers, "retry-after"); | |
| const delays = [tokenDelay, requestDelay, retryAfter].filter((value): value is number => Number.isFinite(value)); | |
| if (!delays.length) return true; | |
| return Math.max(...delays); | |
| } | |
| return status >= 500; | |
| } | |
| // eslint-disable-next-line sonarjs/function-return-type | |
| export function checkLlmRetryableState(error: unknown): boolean | number { | |
| if (error instanceof SyntaxError || error instanceof TypeBoxError || error instanceof LogReturn) { | |
| return true; | |
| } | |
| const status = extractStatus(error) ?? extractStatusFromMessage(error); | |
| if (status == null) return false; | |
| if (status === 429) { | |
| const headers = extractHeaders(error); | |
| const tokenDelay = parseRetryAfter(headers, "x-ratelimit-reset-tokens"); | |
| const requestDelay = parseRetryAfter(headers, "x-ratelimit-reset-requests"); | |
| const retryAfter = parseRetryAfter(headers, "retry-after"); | |
| const delays = [tokenDelay, requestDelay, retryAfter].filter((value): value is number => Number.isFinite(value)); | |
| if (!delays.length) return true; | |
| return Math.max(...delays); | |
| } | |
| return status >= 500; | |
| } |
0x4007
left a comment
0x4007
left a comment
There was a problem hiding this comment.
Cool seems good assuming this works
|
@0x4007 Once this is published, I already drafted all the pull-requests to update the plugins and use this, so you'll be able to test it. |
|
| View | Contribution | Count | Reward |
|---|---|---|---|
| Review | Specification | 1 | 5.37 |
| Review | Comment | 3 | 0 |
Conversation Incentives
| Comment | Formatting | Relevance | Priority | Reward |
|---|---|---|---|---|
## Summary- add shared LLM retryable detection with retry-afte… | 2.21content:
content:
h2:
score: 1
elementCount: 1
ul:
score: 0
elementCount: 1
li:
score: 0.1
elementCount: 2
p:
score: 0
elementCount: 1
result: 1.2
regex:
wordCount: 19
wordValue: 0.1
result: 1.01
authorship: 1
| Relevance: 1 Readability: 31.7 | 1 | 5.37 |
@codex review | 0content:
content:
p:
score: 0
elementCount: 1
result: 0
regex:
wordCount: 2
wordValue: 0
result: 0
authorship: 1
| Relevance: 0 Readability: 35.6 | 2 | 0 |
@0x4007 Once this is published, I already drafted all the pull-r… | 0content:
content:
p:
score: 0
elementCount: 1
result: 0
regex:
wordCount: 27
wordValue: 0
result: 0
authorship: 1
| Relevance: 0.5 Readability: 72.9 | 2 | 0 |
Valid point — `getStatusFromSource` was only checking fo… | 0content:
content:
p:
score: 0
elementCount: 1
result: 0
regex:
wordCount: 28
wordValue: 0
result: 0
authorship: 1
| Relevance: 1 Readability: 53.6 | 2 | 0 |
[ 3.96 UUSD ]
@0x4007
Contributions Overview
| View | Contribution | Count | Reward |
|---|---|---|---|
| Review | Code Review | 1 | 3.96 |
| Review | Comment | 1 | 0 |
Review Details for #173#issue-3172551780
| Changes | Priority | Reward |
|---|---|---|
| +180 -18 | 2 | 3.96 |
Conversation Incentives
| Comment | Formatting | Relevance | Priority | Reward |
|---|---|---|---|---|
Cool seems good assuming this works | 0.43content:
content:
p:
score: 0
elementCount: 1
result: 0
regex:
wordCount: 6
wordValue: 0.1
result: 0.43
authorship: 1
| Relevance: 0 Readability: 87.9 | 2 | 0 |
2 participants
Summary
Resolves #170