-
Notifications
You must be signed in to change notification settings - Fork 53
feat(js-sdk): support retry-after header #504
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -128,8 +128,47 @@ function isAxiosError(err: any): boolean { | |||||||
function randomTime(loopCount: number, minWaitInMs: number): number { | ||||||||
const min = Math.ceil(2 ** loopCount * minWaitInMs); | ||||||||
const max = Math.ceil(2 ** (loopCount + 1) * minWaitInMs); | ||||||||
return Math.floor(Math.random() * (max - min) + min); //The maximum is exclusive and the minimum is inclusive | ||||||||
const calculatedTime = Math.floor(Math.random() * (max - min) + min); | ||||||||
return Math.min(calculatedTime, 120000); | ||||||||
} | ||||||||
function parseRetryAfterHeader(headers: Record<string, string | string[] | undefined>): number | undefined { | ||||||||
const retryAfter = headers["retry-after"] || headers["Retry-After"]; | ||||||||
|
||||||||
if (!retryAfter) { | ||||||||
return undefined; | ||||||||
} | ||||||||
|
||||||||
const retryAfterValue = Array.isArray(retryAfter) ? retryAfter[0] : retryAfter; | ||||||||
|
||||||||
if (!retryAfterValue) { | ||||||||
return undefined; | ||||||||
} | ||||||||
|
||||||||
// Try to parse as integer (seconds) | ||||||||
const secondsValue = parseInt(retryAfterValue, 10); | ||||||||
if (!isNaN(secondsValue)) { | ||||||||
const msValue = secondsValue * 1000; | ||||||||
if (msValue >= 1000 && msValue <= 1800000) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than these being hard-coded, now that the go-sdk changes have been merged, you can use the following variables:
Suggested change
|
||||||||
return msValue; | ||||||||
} | ||||||||
return undefined; | ||||||||
} | ||||||||
|
||||||||
try { | ||||||||
const dateValue = new Date(retryAfterValue); | ||||||||
const now = new Date(); | ||||||||
const delayMs = dateValue.getTime() - now.getTime(); | ||||||||
|
||||||||
if (delayMs >= 1000 && delayMs <= 1800000) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic looks repeated, can you extract it to it's own function and reference it above and here? |
||||||||
return delayMs; | ||||||||
} | ||||||||
} catch (e) { | ||||||||
// Invalid date format | ||||||||
} | ||||||||
|
||||||||
return undefined; | ||||||||
} | ||||||||
|
||||||||
|
||||||||
interface WrappedAxiosResponse<R> { | ||||||||
response?: AxiosResponse<R>; | ||||||||
|
@@ -164,18 +203,40 @@ export async function attemptHttpRequest<B, R>( | |||||||
throw new FgaApiAuthenticationError(err); | ||||||||
} else if (status === 404) { | ||||||||
throw new FgaApiNotFoundError(err); | ||||||||
} else if (status === 429 || status >= 500) { | ||||||||
if (iterationCount >= config.maxRetry) { | ||||||||
} else if (status === 429 || (status >= 500 && status !== 501)) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also make sure we are retrying on network errors? Currently we do not retry on them: https://github.com/openfga/js-sdk/blob/8fb95c80ff57d10616c0fd60ca1dfc991ce41675/common.ts#L177 |
||||||||
if (iterationCount >= config.maxRetry) { | ||||||||
// We have reached the max retry limit | ||||||||
// Thus, we have no choice but to throw | ||||||||
if (status === 429) { | ||||||||
throw new FgaApiRateLimitExceededError(err); | ||||||||
} else { | ||||||||
throw new FgaApiInternalError(err); | ||||||||
} | ||||||||
if (status === 429) { | ||||||||
throw new FgaApiRateLimitExceededError(err); | ||||||||
} else { | ||||||||
throw new FgaApiInternalError(err); | ||||||||
} | ||||||||
await new Promise(r => setTimeout(r, randomTime(iterationCount, config.minWaitInMs))); | ||||||||
} else { | ||||||||
} | ||||||||
|
||||||||
let retryDelayMs: number | undefined; | ||||||||
|
||||||||
// Check for Retry-After header | ||||||||
if (err.response?.headers) { | ||||||||
retryDelayMs = parseRetryAfterHeader(err.response.headers); | ||||||||
} | ||||||||
|
||||||||
// If Retry-After header isn't valid or not present, use exponential backoff | ||||||||
if (retryDelayMs === undefined) { | ||||||||
// For 429s, we always retry with exponential backoff | ||||||||
if (status === 429) { | ||||||||
retryDelayMs = randomTime(iterationCount, config.minWaitInMs); | ||||||||
} else if (status >= 500 && status !== 501) { | ||||||||
// For 5xx (except 501), we only retry if Retry-After was present | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's change this so that we fall back to exponential backoff here too |
||||||||
// Since we already checked and it wasn't, we throw | ||||||||
throw new FgaApiInternalError(err); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
if (retryDelayMs !== undefined) { | ||||||||
await new Promise(r => setTimeout(r, retryDelayMs)); | ||||||||
} | ||||||||
} else { | ||||||||
throw new FgaApiError(err); | ||||||||
} | ||||||||
} | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,7 +202,9 @@ describe("{{appTitleCaseName}} SDK", function () { | |
}); | ||
|
||
it("should retry a failed attempt to request to exchange the credentials", async () => { | ||
const scope1 = nocks.tokenExchange(OPENFGA_API_TOKEN_ISSUER, "test-token", 300, 500); | ||
const scope1 = nocks.tokenExchange(OPENFGA_API_TOKEN_ISSUER, "test-token", 300, 500, { | ||
'Retry-After': '1' // Add Retry-After header | ||
}); | ||
const scope2 = nocks.tokenExchange(OPENFGA_API_TOKEN_ISSUER); | ||
nocks.readAuthorizationModels(baseConfig.storeId!); | ||
|
||
|
@@ -486,6 +488,24 @@ describe("{{appTitleCaseName}} SDK", function () { | |
fgaApi.check(baseConfig.storeId!, { tuple_key: tupleKey }, { retryParams: { maxRetry: 0 }}) | ||
).rejects.toThrow(FgaApiInternalError); | ||
}); | ||
it("should throw FgaApiInternalError for 500 error without Retry-After header", async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without the retry header, it should fall back to exponential backoff |
||
nock(basePath) | ||
.post( | ||
`/stores/${storeId}/check`, | ||
{ | ||
tuple_key: tupleKey, | ||
}, | ||
expect.objectContaining({ Authorization: "Bearer test-token" }) | ||
) | ||
.reply(500, { | ||
code: "internal_error", | ||
message: "nock error", | ||
}); | ||
|
||
await expect( | ||
fgaApi.check(baseConfig.storeId!, { tuple_key: tupleKey }) | ||
).rejects.toThrow(FgaApiInternalError); | ||
}); | ||
|
||
it("should not throw FgaApiInternalError with default retries", async () => { | ||
nock(basePath) | ||
|
@@ -499,6 +519,8 @@ describe("{{appTitleCaseName}} SDK", function () { | |
.reply(500, { | ||
code: "internal_error", | ||
message: "nock error", | ||
}, { | ||
'Retry-After': '1' | ||
}); | ||
|
||
nock(basePath) | ||
|
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.
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.
Defined here:
sdk-generator/config/common/config.base.json
Lines 88 to 89 in 885bdb7