fix(rpc): bound retryable status range and add malformed-response code
- isRetryableStatus now fails closed for out-of-range values (600+, negative, NaN) by bounding the 5xx check on both sides (>= 500 && <= 599), instead of an unbounded >= 500 that classified garbage statuses like 1000 as retryable. - 408 Request Timeout is now retryable, matching the RPC_TRANSPORT doc comment (connection, timeout, 5xx) — a timeout surfaced as 408 is no longer treated differently from the same timeout surfaced as 504. - Add RPC_MALFORMED: the callee answered, but not with a ServiceResult (HTML error page, truncated body, unexpected shape). Distinct from RPC_TRANSPORT since something DID respond; non-retryable via the existing retryableFor, no new branch needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -13,6 +13,12 @@ export const RPC_ERROR_CODES = {
|
|||||||
handler: "RPC_HANDLER",
|
handler: "RPC_HANDLER",
|
||||||
/** Identity token missing, malformed, expired, or for another audience. */
|
/** Identity token missing, malformed, expired, or for another audience. */
|
||||||
identity: "RPC_IDENTITY",
|
identity: "RPC_IDENTITY",
|
||||||
|
/**
|
||||||
|
* The callee answered, but not with a ServiceResult — a proxy's HTML error
|
||||||
|
* page, a truncated body, an unexpected shape. Distinct from `transport`:
|
||||||
|
* something DID respond, so retrying returns the same thing.
|
||||||
|
*/
|
||||||
|
malformed: "RPC_MALFORMED",
|
||||||
} as const;
|
} as const;
|
||||||
|
|
||||||
export type RpcErrorCode = (typeof RPC_ERROR_CODES)[keyof typeof RPC_ERROR_CODES];
|
export type RpcErrorCode = (typeof RPC_ERROR_CODES)[keyof typeof RPC_ERROR_CODES];
|
||||||
@@ -23,11 +29,17 @@ function retryableFor(code: string): boolean {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* 5xx and 429 mean "the callee could not answer, try later". A 4xx is the
|
* 5xx, 429 and 408 mean "the callee could not answer, try later". Any other
|
||||||
* callee saying no — retrying it just repeats the same rejection.
|
* 4xx is the callee saying no — retrying just repeats the same rejection.
|
||||||
|
*
|
||||||
|
* The range is bounded on BOTH sides deliberately: an unbounded `>= 500`
|
||||||
|
* puts a garbage status like 1000 in the retryable bucket, and this function
|
||||||
|
* is the sole gate the client and HTTP transport trust for retry safety.
|
||||||
|
* An out-of-range value must fail closed, i.e. not retryable.
|
||||||
*/
|
*/
|
||||||
export function isRetryableStatus(status: number): boolean {
|
export function isRetryableStatus(status: number): boolean {
|
||||||
return status >= 500 || status === 429;
|
if (status === 408 || status === 429) return true;
|
||||||
|
return status >= 500 && status <= 599;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function success<T>(value: T): ServiceResult<T> {
|
export function success<T>(value: T): ServiceResult<T> {
|
||||||
|
|||||||
@@ -23,12 +23,30 @@ describe("rpc errors", () => {
|
|||||||
expect(isRetryableStatus(500)).toBe(true);
|
expect(isRetryableStatus(500)).toBe(true);
|
||||||
expect(isRetryableStatus(503)).toBe(true);
|
expect(isRetryableStatus(503)).toBe(true);
|
||||||
expect(isRetryableStatus(429)).toBe(true);
|
expect(isRetryableStatus(429)).toBe(true);
|
||||||
|
expect(isRetryableStatus(599)).toBe(true);
|
||||||
|
expect(isRetryableStatus(408)).toBe(true);
|
||||||
expect(isRetryableStatus(400)).toBe(false);
|
expect(isRetryableStatus(400)).toBe(false);
|
||||||
expect(isRetryableStatus(403)).toBe(false);
|
expect(isRetryableStatus(403)).toBe(false);
|
||||||
expect(isRetryableStatus(404)).toBe(false);
|
expect(isRetryableStatus(404)).toBe(false);
|
||||||
|
expect(isRetryableStatus(409)).toBe(false);
|
||||||
expect(isRetryableStatus(200)).toBe(false);
|
expect(isRetryableStatus(200)).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("an out-of-range status fails closed", () => {
|
||||||
|
for (const status of [600, 1000, 0, -1, Number.NaN]) {
|
||||||
|
expect(isRetryableStatus(status)).toBe(false);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test("a malformed response is never retryable", () => {
|
||||||
|
const f = failure(RPC_ERROR_CODES.malformed, "Unexpected response shape");
|
||||||
|
expect(f.ok).toBe(false);
|
||||||
|
if (!f.ok) {
|
||||||
|
expect(f.code).toBe("RPC_MALFORMED");
|
||||||
|
expect(f.retryable).toBe(false);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
test("a denial is never retryable", () => {
|
test("a denial is never retryable", () => {
|
||||||
const f = failure(RPC_ERROR_CODES.denied, "Forbidden");
|
const f = failure(RPC_ERROR_CODES.denied, "Forbidden");
|
||||||
if (!f.ok) expect(f.retryable).toBe(false);
|
if (!f.ok) expect(f.retryable).toBe(false);
|
||||||
|
|||||||
Reference in New Issue
Block a user