feat: add support for handling Retry-After header
#267
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
- Coverage 88.43% 87.79% -0.64%
==========================================
Files 23 23
Lines 1219 1254 +35
Branches 198 230 +32
==========================================
+ Hits 1078 1101 +23
- Misses 85 96 +11
- Partials 56 57 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is generated using the existing sdk-generator PR openfga/sdk-generator#504 |
SoulPancake
changed the title
Retry-After header
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
common.ts (5)
145-147: Allow Retry-After: 0 and handle small negative skewRFC allows Retry-After of 0 (immediate retry). Also clamp small negative delays due to clock skew to 0.
Apply this diff:
-function isValidRetryDelay(delayMs: number): boolean { - return delayMs > 0 && delayMs <= 1800 * 1000; -} +function isValidRetryDelay(delayMs: number): boolean { + return delayMs >= 0 && delayMs <= 1800 * 1000; +}- const dateValue = new Date(retryAfterValue); - const now = new Date(); - const delayMs = dateValue.getTime() - now.getTime(); + const dateValue = new Date(retryAfterValue); + const now = new Date(); + const delayMs = Math.max(0, dateValue.getTime() - now.getTime()); - if (isValidRetryDelay(delayMs)) { + if (isValidRetryDelay(delayMs)) { return delayMs; }Also applies to: 172-179
139-144: Avoid 0ms backoff when minWaitInMs is unset (burst risk)With minWaitInMs=0, randomTime returns 0ms. Use a small floor to prevent tight loops.
Apply this diff:
-function randomTime(loopCount: number, minWaitInMs: number): number { - const min = Math.ceil(2 ** loopCount * minWaitInMs); - const max = Math.ceil(2 ** (loopCount + 1) * minWaitInMs); +function randomTime(loopCount: number, minWaitInMs: number): number { + const base = Math.max(minWaitInMs, 50); // 50ms floor + const min = Math.ceil(2 ** loopCount * base); + const max = Math.ceil(2 ** (loopCount + 1) * base); const calculatedTime = Math.floor(Math.random() * (max - min) + min); return Math.min(calculatedTime, 120 * 1000); }
276-279: Default retry params can yield 0ms backoffIf configuration.retryParams is undefined and options omit retryParams, minWaitInMs becomes 0. Ensure sane defaults when maxRetry > 0.
Apply this diff:
- const retryParams = axiosArgs.options?.retryParams ? axiosArgs.options?.retryParams : configuration.retryParams; - const maxRetry:number = retryParams ? retryParams.maxRetry : 0; - const minWaitInMs:number = retryParams ? retryParams.minWaitInMs : 0; + const retryParams = axiosArgs.options?.retryParams ?? configuration.retryParams; + const maxRetry: number = retryParams?.maxRetry ?? 0; + const minWaitInMs: number = maxRetry > 0 ? Math.max(50, retryParams?.minWaitInMs ?? 200) : 0;
230-266: Optional: include 408 (Request Timeout) in retryable statuses408 is commonly safe to retry with backoff.
Apply this diff:
- } else if (status === 429 || (status >= 500 && status !== 501)) { + } else if (status === 408 || status === 429 || (status >= 500 && status !== 501)) {
200-201: Optional: narrow return type (never returns undefined after fixes)Control flow always returns a WrappedAxiosResponse or throws. Simplify the signature.
Apply this diff:
-): Promise<WrappedAxiosResponse<R> | undefined> { +): Promise<WrappedAxiosResponse<R>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)common.ts(7 hunks)tests/helpers/nocks.ts(1 hunks)tests/index.test.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/index.test.ts (3)
tests/helpers/default-config.ts (2)
OPENFGA_API_TOKEN_ISSUER(20-20)baseConfig(54-67)configuration.ts (1)
GetDefaultRetryParams(48-53)api.ts (1)
OpenFgaApi(1284-1508)
common.ts (2)
base.ts (1)
RequestArgs(29-32)errors.ts (5)
FgaError(27-40)FgaApiValidationError(120-134)FgaApiNotFoundError(142-155)FgaApiRateLimitExceededError(165-179)FgaApiInternalError(187-203)
🔇 Additional comments (1)
common.ts (1)
250-263: Good: honors Retry-After header with sensible fallbackParsing Retry-After and falling back to capped exponential backoff is correct and improves resilience.
Consider adding tests for:
- Retry-After: "0", very large value (clamped), HTTP-date in past (treated as 0), invalid string (fallback to backoff).
- Network error retry path.
2 participants
Description
For #208
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests