From 435c84fc35faaf7deba1c1c7cb88c58163731dd9 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 23:58:27 +0000 Subject: [PATCH] Harden the retry classifier and pin per-attempt deadlines (closes #80) A POST or PUT is replayed only when every errno in the cause chain is a connect errno and the walk reached the end of the chain, and postJSON/putJSON no longer follow redirects, so a redirect is an ApiError that is not retried. getRetryOptions() returns a copy. New tests pin every errno the classifier names, the cause-chain depth limit, a chain deeper than the limit, a two-error cycle, and a fresh deadline per attempt for every retrying entry point. The README endpoint list is now the one place naming the requests the replay rule covers; code comments point to it. Model: opus-5-5 --- README.md | 32 ++++++++------ TODO.md | 7 +++ src/api/client.ts | 27 ++++++------ src/retry.ts | 37 +++++++++++----- test/api/client.test.ts | 94 ++++++++++++++++++++++++++++++++++++++-- test/retry/retry.test.ts | 79 +++++++++++++++++++++++++++++++-- 6 files changed, 230 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 1b11afc..a97e7c6 100644 --- a/README.md +++ b/README.md @@ -323,6 +323,7 @@ Endpoints used: - `GET /collections/v2/diff?collectionID=&sinceTime=`: list files in a collection; paginate while `hasMore` is true. - `GET https://files.ente.io/?fileID=`: download encrypted file bytes. +- `POST /files/data/fetch`: fetch encrypted ML data for a batch of files. - `POST /files/upload-url`: mint a presigned upload URL (for thumbnail repair). - `PUT /files/thumbnail`: register an uploaded thumbnail's object key. @@ -377,20 +378,23 @@ headers — `getFileStream` returns as soon as headers arrive, so a deadline tha only guarded the initial request would leave the same hang one layer down. **Non-idempotent requests are not blindly replayed.** `postJSON` and `putJSON` -reach `/users/srp/create-session`, `/users/two-factor/verify` — which consumes -one of a small number of second-factor attempts — and `/files/thumbnail`. They -are retried only on the three failures that establish no TCP connection to the -server ever existed, so no request byte can have been transmitted: `ENOTFOUND` -and `EAI_AGAIN` (name resolution produced no address) and `ECONNREFUSED` (the -peer refused the connection). A 5xx, a mid-flight reset and a deadline are all -left to the caller, because each of them can happen after the server has already -acted. The routing errnos `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` are -excluded for the same reason, despite looking like connect-time failures: on -Linux an ICMP unreachable arriving mid-flight, or a local interface going down -after the request was written, delivers them on an already-established socket. -They stay retryable for the idempotent calls. `putFile` is exempt: a presigned -PUT stores one whole object at one key in one request, so replaying it has no -partial state to damage. +send every `POST` and `PUT` in the endpoint list above; some of them change +server state, and `/users/two-factor/verify` consumes one of a small number of +second-factor attempts. They are retried only when every errno in the error's +`cause` chain is one of the three that establish no TCP connection to the server +ever existed, so no request byte can have been transmitted: `ENOTFOUND` and +`EAI_AGAIN` (name resolution produced no address) and `ECONNREFUSED` (the peer +refused the connection). A 5xx, a mid-flight reset and a deadline are all left +to the caller, because each of them can happen after the server has already +acted. These two do not follow redirects either: a redirect means the server +already received the request, so it is reported as an error and not retried. The +routing errnos `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` are excluded for the +same reason, despite looking like connect-time failures: on Linux an ICMP +unreachable arriving mid-flight, or a local interface going down after the +request was written, delivers them on an already-established socket. They stay +retryable for the idempotent calls. `putFile` is exempt: a presigned PUT stores +one whole object at one key in one request, so replaying it has no partial state +to damage. A download is retried as a whole — request, stream consumption, and decryption — because a socket reset after the response headers have arrived surfaces in the diff --git a/TODO.md b/TODO.md index 4316a42..92f26ad 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Tag v1.0.0. `imageMetadata.exifError`, and a failure to read the original as `imageMetadataError` in the per-file JSON, instead of the field being left out. +- 2026-09-22: Hardened the retry classifier (issue 80). A `POST` or `PUT` is + replayed only when every errno in the cause chain is a connect errno, and it + no longer follows redirects. `getRetryOptions()` returns a copy. Tests pin + every errno the classifier names, the cause-chain depth limit, cycle + termination, and a fresh deadline per attempt for every retrying entry point. + The README's endpoint list is the one place that names the requests the replay + rule covers. - 2026-09-22: Stopped `make test` collecting tests from checkouts nested under `.claude/` (issue 25). vitest ignores `.gitignore` when finding tests, so a nested checkout ran the whole suite again; `vitest.config.ts` now adds diff --git a/src/api/client.ts b/src/api/client.ts index 61f874f..b7bc281 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -146,8 +146,9 @@ export class ApiClient { // The policy this client was configured with, so that a caller wrapping a // whole operation in its own `withRetry` — the download layer — runs under // the same settings rather than under the library defaults. + // A copy, so the caller cannot change this client's settings through it. getRetryOptions(): ResolvedRetryOptions { - return this.retry; + return { ...this.retry }; } private headers(extra?: Record): Record { @@ -228,15 +229,15 @@ export class ApiClient { async postJSON(path: string, body: unknown): Promise { const url = `${this.apiOrigin}${path}`; - // Idempotency: this reaches `/users/srp/create-session`, - // `/users/two-factor/verify` and `/users/ott`, all of which change - // server state — verifying a second factor consumes one of a small - // number of attempts. So a POST is replayed only on a failure that - // establishes no TCP connection to the server ever existed: DNS - // produced no address, or the peer refused the connection. A 5xx, a - // mid-flight reset, a routing errno (which Linux also delivers on an - // established socket) and a timeout are all left to the caller, - // because each of them can occur after the server has already acted. + // Not idempotent: a POST is replayed only when `isSafeToReplay` + // says no request byte can have reached the server. The endpoints + // this covers are listed in the README under "Endpoints used". + // + // Redirects are not followed. The origin has already received the + // request when it answers with one, so a connection refused by the + // redirect target would look replay-safe when it is not. The API has + // no legitimate redirect, so one surfaces as an `ApiError` with its + // 3xx status, which is not retried. return withRetry( async () => { const resp = await this._fetch(url, { @@ -245,6 +246,7 @@ export class ApiClient { "Content-Type": "application/json", }), body: JSON.stringify(body), + redirect: "manual", signal: AbortSignal.timeout(this.requestTimeoutMs), }); await this.throwIfError(resp); @@ -303,9 +305,7 @@ export class ApiClient { async putJSON(path: string, body: unknown): Promise { const url = `${this.apiOrigin}${path}`; - // Same idempotency rule as `postJSON`, for the same reason: this - // reaches `/files/thumbnail`, which registers an uploaded thumbnail - // against a file. + // Same replay and redirect rules as `postJSON`, for the same reasons. return withRetry( async () => { const resp = await this._fetch(url, { @@ -314,6 +314,7 @@ export class ApiClient { "Content-Type": "application/json", }), body: JSON.stringify(body), + redirect: "manual", signal: AbortSignal.timeout(this.requestTimeoutMs), }); await this.throwIfError(resp); diff --git a/src/retry.ts b/src/retry.ts index 4d5eb69..1198a79 100644 --- a/src/retry.ts +++ b/src/retry.ts @@ -88,18 +88,21 @@ const MAX_CAUSE_DEPTH = 8; // errno on the error it throws — it hangs the underlying socket error off // `cause`, sometimes more than one level down — so a classifier that only read // the top-level error would see a bare `Error` and call every dropped -// connection permanent. -const causeCodes = (err: unknown): string[] => { +// connection permanent. `complete` is false when the walk stopped at the +// depth limit with more of the chain still below it. +const causeCodes = (err: unknown): { codes: string[]; complete: boolean } => { const codes: string[] = []; let current: unknown = err; for (let depth = 0; depth < MAX_CAUSE_DEPTH; depth++) { - if (current === null || typeof current !== "object") break; + if (current === null || typeof current !== "object") { + return { codes, complete: true }; + } const { code, cause } = current as { code?: unknown; cause?: unknown }; if (typeof code === "string") codes.push(code); - if (cause === current) break; + if (cause === current) return { codes, complete: true }; current = cause; } - return codes; + return { codes, complete: current === null || typeof current !== "object" }; }; const isAbort = (err: unknown): boolean => { @@ -145,15 +148,15 @@ export const isRetryable = (err: unknown): boolean => { // have succeeded; the cost of the imprecision is bounded by the attempt // count. if (err instanceof TypeError) return true; - return causeCodes(err).some((code) => TRANSPORT_CODES.has(code)); + return causeCodes(err).codes.some((code) => TRANSPORT_CODES.has(code)); }; // Could the first attempt already have taken effect on the server? // // `isRetryable` is the wrong question for a request that changes state. -// quak's non-idempotent calls are `/users/srp/create-session`, -// `/users/two-factor/verify` — which consumes one of a small number of 2FA -// attempts — and `/files/thumbnail`. They are replayed only on the failures in +// `postJSON` and `putJSON` use this for every `POST` and `PUT` listed in the +// README under "Endpoints used"; verifying a second factor, for one, consumes +// one of a small number of attempts. They are replayed only on the failures in // `CONNECT_CODES`, which establish that no TCP connection to the server ever // existed: there was no address to connect to, or the peer refused the // connection outright. A request byte cannot have been transmitted, so the @@ -162,9 +165,19 @@ export const isRetryable = (err: unknown): boolean => { // Everything else is ambiguous. A 5xx proves the server did process the // request. A reset or a broken pipe can arrive after it was fully sent and // acted on. A routing errno can be delivered on an established socket. A -// deadline says nothing at all about the server's state. -export const isSafeToReplay = (err: unknown): boolean => - isRetryable(err) && causeCodes(err).some((code) => CONNECT_CODES.has(code)); +// deadline says nothing at all about the server's state. So every errno in the +// cause chain must be a connect errno: one other errno anywhere in the chain +// is doubt, and doubt is not replayed. A chain longer than the walk is doubt +// too: the links below the limit were never read. +export const isSafeToReplay = (err: unknown): boolean => { + const { codes, complete } = causeCodes(err); + return ( + isRetryable(err) && + complete && + codes.length > 0 && + codes.every((code) => CONNECT_CODES.has(code)) + ); +}; export interface WithRetryOptions extends RetryOptions { isRetryable?: (err: unknown) => boolean; diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 2faf019..9ed6094 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -639,6 +639,24 @@ describe("ApiClient retries", () => { expect(policy.baseDelayMs).toBe(7); expect(policy.maxDelayMs).toBe(11); }); + + it("does not let a caller change its settings through that policy", async () => { + const { fetch, calls } = scriptedFetch( + textResponse("boom", 500), + textResponse("boom", 500), + textResponse("boom", 500), + ); + const client = new ApiClient({ + fetch, + retry: { ...noWait, attempts: 2 }, + }); + + client.getRetryOptions().attempts = 3; + + expect(client.getRetryOptions().attempts).toBe(2); + await expect(client.getJSON("/x")).rejects.toBeInstanceOf(ApiError); + expect(calls).toHaveLength(2); + }); }); describe("ApiClient timeouts", () => { @@ -693,6 +711,51 @@ describe("ApiClient timeouts", () => { expect(new Set(signals).size).toBe(3); }, 5000); + it("gives every retrying entry point a fresh deadline per attempt", async () => { + // A refused connection is retried by every entry point, the + // non-idempotent ones included. If the deadline were created once, + // outside the retry, both attempts would carry the same signal. + const entryPoints: [ + string, + () => Response, + (c: ApiClient) => unknown, + ][] = [ + ["getJSON", () => jsonResponse({}), (c) => c.getJSON("/a")], + ["postJSON", () => jsonResponse({}), (c) => c.postJSON("/b", {})], + ["putJSON", () => jsonResponse({}), (c) => c.putJSON("/c", {})], + [ + "putFile", + () => new Response(null, { status: 200 }), + (c) => c.putFile("https://s3.example/x", new Uint8Array([1])), + ], + [ + "getFileStream", + () => streamResponse(new Uint8Array([1])), + (c) => c.getFileStream(1), + ], + [ + "getThumbnailStream", + () => streamResponse(new Uint8Array([1])), + (c) => c.getThumbnailStream(1), + ], + ]; + for (const [name, success, call] of entryPoints) { + const { fetch, calls } = scriptedFetch( + errnoError("ECONNREFUSED", "connect ECONNREFUSED"), + success(), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await call(client); + + expect(calls, name).toHaveLength(2); + const [first, second] = calls.map((c) => c.init?.signal); + expect(first, name).toBeInstanceOf(AbortSignal); + expect(second, name).toBeInstanceOf(AbortSignal); + expect(second, name).not.toBe(first); + } + }); + it("recovers when a later attempt answers in time", async () => { const { fetch, calls } = scriptedFetch(HANG, jsonResponse({ ok: 1 })); const client = new ApiClient({ @@ -820,9 +883,8 @@ describe("ApiClient error typing", () => { describe("ApiClient non-idempotent requests", () => { /** - * `postJSON` and `putJSON` carry quak's only requests that change server - * state: `/users/srp/create-session`, `/users/two-factor/verify` — which - * consumes one of a small number of 2FA attempts — and `/files/thumbnail`. + * `postJSON` and `putJSON` carry quak's requests that can change server + * state; the README lists them under "Endpoints used". * * They are retried only on a failure that establishes no TCP connection to * the server ever existed — DNS produced no address, or the peer refused @@ -922,4 +984,30 @@ describe("ApiClient non-idempotent requests", () => { await refusedClient.updateThumbnail(1, "key", "header"); expect(refused.calls).toHaveLength(2); }); + + it("does not follow or replay a redirect on POST or PUT", async () => { + // The origin has already received a request it answers with a + // redirect, so following it would let a refused connection to the + // redirect target pass for a request that never went out. + for (const send of [ + (c: ApiClient) => c.postJSON("/users/ott", {}), + (c: ApiClient) => c.putJSON("/files/thumbnail", {}), + ]) { + const { fetch, calls } = scriptedFetch( + new Response(null, { + status: 307, + headers: { location: "https://elsewhere.example/" }, + }), + jsonResponse({}), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + const err: unknown = await send(client).catch((e: unknown) => e); + + expect(calls[0]?.init?.redirect).toBe("manual"); + expect(err).toBeInstanceOf(ApiError); + expect((err as ApiError).status).toBe(307); + expect(calls).toHaveLength(1); + } + }); }); diff --git a/test/retry/retry.test.ts b/test/retry/retry.test.ts index 7baf8c0..394d96e 100644 --- a/test/retry/retry.test.ts +++ b/test/retry/retry.test.ts @@ -149,8 +149,11 @@ describe("isRetryable: transport failures", () => { }); it("retries an errno carried on the error itself", () => { + // Every errno the classifier names, so none can be reclassified + // unnoticed. for (const code of [ "ECONNRESET", + "ECONNABORTED", "ETIMEDOUT", "EPIPE", "ENOTFOUND", @@ -158,6 +161,8 @@ describe("isRetryable: transport failures", () => { "ECONNREFUSED", "EHOSTUNREACH", "ENETUNREACH", + "ENETRESET", + "ENETDOWN", ]) { expect(isRetryable(errnoError(code))).toBe(true); } @@ -215,6 +220,27 @@ describe("isRetryable: transport failures", () => { looped.cause = looped; expect(isRetryable(looped)).toBe(false); }); + + it("terminates on a cause chain that loops through two errors", () => { + const first: Error & { cause?: unknown } = new Error("first"); + const second = new Error("second", { cause: first }); + first.cause = second; + expect(isRetryable(first)).toBe(false); + }); + + it("reads the error and at most seven causes below it", () => { + // The walk is bounded at eight links. An errno at the eighth link is + // found; one at the ninth is not. + const buried = (causes: number): Error => { + let err = errnoError("ECONNRESET"); + for (let i = 0; i < causes; i++) { + err = new Error(`wrapper ${i}`, { cause: err }); + } + return err; + }; + expect(isRetryable(buried(7))).toBe(true); + expect(isRetryable(buried(8))).toBe(false); + }); }); describe("isRetryable: stream truncation versus corruption", () => { @@ -279,10 +305,9 @@ describe("isSafeToReplay", () => { * that is not the whole question: the other half is "could the first * attempt already have taken effect on the server?". * - * quak's non-idempotent calls are `/users/srp/create-session`, - * `/users/two-factor/verify` (which consumes one of a limited number of - * 2FA attempts) and `/files/thumbnail`. A blind replay of any of them can - * do real damage, so they retry only on the failures that establish no TCP + * The calls this guards are the `POST` and `PUT` requests listed in the + * README under "Endpoints used". A blind replay of some of them can do + * real damage, so they retry only on the failures that establish no TCP * connection to the server ever existed — DNS produced no address, or the * peer refused the connection — and therefore that no request byte can * have been transmitted. @@ -333,6 +358,52 @@ describe("isSafeToReplay", () => { ).toBe(false); expect(isSafeToReplay(new TypeError("fetch failed"))).toBe(false); }); + + it("does not replay any other errno the classifier names", () => { + for (const code of [ + "ECONNRESET", + "ECONNABORTED", + "ETIMEDOUT", + "EPIPE", + "EHOSTUNREACH", + "ENETUNREACH", + "ENETRESET", + "ENETDOWN", + ]) { + expect(isSafeToReplay(errnoError(code))).toBe(false); + } + }); + + it("does not replay a chain that also shows the request may have gone out", () => { + // A connect errno somewhere in the chain is not enough: any other + // errno beside it is doubt, and doubt is not replayed. + const reset = Object.assign( + new Error("read ECONNRESET", { cause: errnoError("ECONNREFUSED") }), + { code: "ECONNRESET" }, + ); + const mixed = new TypeError("fetch failed", { cause: reset }); + expect(isRetryable(mixed)).toBe(true); + expect(isSafeToReplay(mixed)).toBe(false); + }); + + it("does not replay a chain longer than the walk reads", () => { + // Eight connect errnos, then a reset at the ninth link, below the + // limit. The walk never sees the reset, so it cannot rule it out. + const refusedChain = (below: Error | undefined): Error => { + let err = below; + for (let i = 0; i < 8; i++) { + err = Object.assign(new Error(`refused ${i}`, { cause: err }), { + code: "ECONNREFUSED", + }); + } + return err as Error; + }; + expect(isSafeToReplay(refusedChain(errnoError("ECONNRESET")))).toBe( + false, + ); + // The same eight links with nothing below them are replayable. + expect(isSafeToReplay(refusedChain(undefined))).toBe(true); + }); }); // ---------------------------------------------------------------------------