docs: close three retryability gaps in the Task 2 plan snippet
isRetryableStatus used an unbounded status >= 500, so a garbage status like 1000 landed in the retryable bucket. This function is the sole gate the client and HTTP transport trust for retry safety, and an out-of-range value must fail closed. Bounded on both sides. 408 Request Timeout was non-retryable while the same file documented transport as covering "connection, timeout, 5xx" - a genuine timeout surfaced as 408 was classified differently from the identical timeout surfaced as 504. Now retryable. There was no code for "the callee answered but not with a ServiceResult" - a proxy's HTML error page, a truncated body. Task 8 was already papering over it by hand-setting retryable: false beside a transport code that retryableFor says is always retryable, which is exactly how the two drift apart. Added RPC_MALFORMED and made that path use failure() so retryability is derived from the code rather than written next to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -285,16 +285,34 @@ describe("rpc errors", () => {
|
||||
});
|
||||
|
||||
test("only transport failures are retryable", () => {
|
||||
// 5xx and 429 are the callee saying "try again"; everything else is final.
|
||||
// 5xx, 429 and 408 are the callee saying "try again"; everything else is final.
|
||||
expect(isRetryableStatus(500)).toBe(true);
|
||||
expect(isRetryableStatus(503)).toBe(true);
|
||||
expect(isRetryableStatus(599)).toBe(true);
|
||||
expect(isRetryableStatus(429)).toBe(true);
|
||||
expect(isRetryableStatus(408)).toBe(true);
|
||||
expect(isRetryableStatus(400)).toBe(false);
|
||||
expect(isRetryableStatus(403)).toBe(false);
|
||||
expect(isRetryableStatus(404)).toBe(false);
|
||||
expect(isRetryableStatus(409)).toBe(false);
|
||||
expect(isRetryableStatus(200)).toBe(false);
|
||||
});
|
||||
|
||||
test("an out-of-range status fails closed rather than landing in the retry bucket", () => {
|
||||
for (const status of [600, 1000, 0, -1, Number.NaN]) {
|
||||
expect(isRetryableStatus(status)).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
test("a malformed callee response is its own non-retryable code", () => {
|
||||
const f = failure(RPC_ERROR_CODES.malformed, "Malformed service response");
|
||||
if (!f.ok) {
|
||||
expect(f.code).toBe("RPC_MALFORMED");
|
||||
// Something answered; asking again returns the same thing.
|
||||
expect(f.retryable).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
test("a denial is never retryable", () => {
|
||||
const f = failure(RPC_ERROR_CODES.denied, "Forbidden");
|
||||
if (!f.ok) expect(f.retryable).toBe(false);
|
||||
@@ -344,6 +362,12 @@ export const RPC_ERROR_CODES = {
|
||||
handler: "RPC_HANDLER",
|
||||
/** Identity token missing, malformed, expired, or for another audience. */
|
||||
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;
|
||||
|
||||
export type RpcErrorCode = (typeof RPC_ERROR_CODES)[keyof typeof RPC_ERROR_CODES];
|
||||
@@ -354,11 +378,17 @@ function retryableFor(code: string): boolean {
|
||||
}
|
||||
|
||||
/**
|
||||
* 5xx and 429 mean "the callee could not answer, try later". A 4xx is the
|
||||
* callee saying no — retrying it just repeats the same rejection.
|
||||
* 5xx, 429 and 408 mean "the callee could not answer, try later". Any other
|
||||
* 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 {
|
||||
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> {
|
||||
@@ -1772,6 +1802,8 @@ export function httpTransport(options: HttpTransportOptions = {}): Transport {
|
||||
}
|
||||
|
||||
if (!response.ok) {
|
||||
// The one place retryability is status-derived rather than code-derived:
|
||||
// the callee answered, and its status says whether asking again helps.
|
||||
return {
|
||||
ok: false,
|
||||
code: RPC_ERROR_CODES.transport,
|
||||
@@ -1784,13 +1816,9 @@ export function httpTransport(options: HttpTransportOptions = {}): Transport {
|
||||
return (await response.json()) as ServiceResult;
|
||||
} catch {
|
||||
// A 200 that is not a ServiceResult means something else answered —
|
||||
// a proxy, an error page. Retrying will not change that.
|
||||
return {
|
||||
ok: false,
|
||||
code: RPC_ERROR_CODES.transport,
|
||||
message: "Malformed service response",
|
||||
retryable: false,
|
||||
};
|
||||
// a proxy, an error page. Retrying will not change that. Use failure()
|
||||
// so retryability is DERIVED from the code, never hand-set beside it.
|
||||
return failure(RPC_ERROR_CODES.malformed, "Malformed service response");
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user