From dd8354477d80fa82ad76b88eaf4f2e92bee3cca2 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Wed, 5 Aug 2026 13:43:39 +0530 Subject: [PATCH] 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 --- ...26-08-05-inter-app-comms-implementation.md | 50 +++++++++++++++---- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/docs/plans/2026-08-05-inter-app-comms-implementation.md b/docs/plans/2026-08-05-inter-app-comms-implementation.md index 62709e86..6f12901c 100644 --- a/docs/plans/2026-08-05-inter-app-comms-implementation.md +++ b/docs/plans/2026-08-05-inter-app-comms-implementation.md @@ -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(value: T): ServiceResult { @@ -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"); } }, };