From 0cbe338b58d8707a15df9c3c36b31aa0025fba7a Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 05:11:07 +0000 Subject: [PATCH 1/3] Add failing tests for the download retry policy Tests only; the retry module they import does not exist yet, so the branch is red at this commit. New test/retry/retry.test.ts documents the classifier and the backoff: which errors are worth another attempt, which are not, and how the delay before each retry is derived. It asserts on the arguments handed to an injected sleep function rather than on elapsed time, so the suite never waits and the numbers are exact. test/api/client.test.ts gains the request-count contract for each of the six call sites, the deadline behaviour, the ApiError typing that the presigned PUT and the null-body paths need in order to be classified at all, and the replay rule for the two non-idempotent methods. test/download/download.test.ts gains the case that motivates the whole design: a socket reset after the response headers arrived, which happens below ApiClient and can only be caught by retrying the request, the stream consumption and the decryption together. It also pins that a retried download stages exactly one temp file, and that the two retry layers do not compose into a multiplied request budget. The existing truncation tests now assert on the error type rather than its wording, since that type is what the classifier reads. test/thumbnails/thumbnails.test.ts separates a genuine 404 from an exhausted retry, so a failing server can no longer make fix-missing-thumbnails re-upload thumbnails that already exist. --- test/api/client.test.ts | 541 +++++++++++++++++++++++++++- test/cli/backup.test.ts | 13 +- test/download/download.test.ts | 330 ++++++++++++++++- test/retry/retry.test.ts | 557 +++++++++++++++++++++++++++++ test/thumbnails/thumbnails.test.ts | 110 +++++- 5 files changed, 1535 insertions(+), 16 deletions(-) create mode 100644 test/retry/retry.test.ts diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 2d1522c..387da4f 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -29,12 +29,23 @@ * return a `ReadableStream` from the appropriate CDN * (or the self-hosted fallback path). * + * - Retries and timeouts. Every request is issued under a deadline and, + * where it is safe to do so, retried with exponential backoff. The + * policy is `src/retry.ts`; what the last section of this file + * documents is which requests get it and which deliberately do not. + * * All tests inject a fake `fetch` via the constructor so nothing touches * the network. The fake records every call for assertion. */ import { describe, expect, it } from "vitest"; -import { ApiClient, ApiError } from "../../src/api/client.js"; +import { + ApiClient, + ApiError, + DEFAULT_DOWNLOAD_TIMEOUT_MS, + DEFAULT_REQUEST_TIMEOUT_MS, +} from "../../src/api/client.js"; +import type { RetryOptions } from "../../src/retry.js"; // --------------------------------------------------------------------------- // Test helpers @@ -102,6 +113,92 @@ const recordingFetch = ( return { fetch: fake as typeof globalThis.fetch, calls }; }; +/** + * One scripted outcome for a single `fetch` call: + * + * - a `Response`, returned as-is; + * - an `Error`, thrown — this is how `fetch` reports a network failure; + * - `HANG`, a request that never answers until its own deadline aborts it. + * + * `HANG` is what makes the timeout tests honest. A fake that resolved after a + * delay would be testing the clock; this one resolves *only* when the signal + * quak attached fires. If no signal is attached, or the signal is not wired to + * the body, the promise never settles and the test fails on its own timeout + * rather than passing by accident. + */ +const HANG = Symbol("hang until aborted"); +type FetchStep = Response | Error | typeof HANG; + +const scriptedFetch = ( + ...steps: FetchStep[] +): { + fetch: typeof globalThis.fetch; + calls: { url: string; init: RequestInit | undefined }[]; +} => { + const calls: { url: string; init: RequestInit | undefined }[] = []; + let i = 0; + const fake = async ( + input: RequestInfo | URL, + init?: RequestInit, + ): Promise => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.href + : input.url; + calls.push({ url, init }); + const step = steps[i++]; + if (step === undefined) { + throw new Error(`scriptedFetch: no step for call #${i - 1}`); + } + if (step === HANG) { + return new Promise((_resolve, reject) => { + const signal = init?.signal; + if (!signal) return; + if (signal.aborted) { + reject(signal.reason as Error); + return; + } + signal.addEventListener( + "abort", + () => reject(signal.reason as Error), + { once: true }, + ); + }); + } + if (step instanceof Error) throw step; + return step; + }; + return { fetch: fake as typeof globalThis.fetch, calls }; +}; + +/** An error shaped like a Node transport failure: the errno is on `.code`. */ +const errnoError = (code: string, message = code): Error => + Object.assign(new Error(message), { code }); + +/** + * A retry policy with the waiting removed. Backoff arithmetic is covered in + * `test/retry/retry.test.ts`; what the tests below are about is *how many + * requests* each call site issues, so they inject a `sleep` that returns + * immediately. Nothing in this file waits. + */ +const noWait: RetryOptions = { + sleep: () => Promise.resolve(), + random: () => 0, +}; + +/** Drain a stream and return the bytes, so body-level failures surface. */ +const readAll = async (stream: ReadableStream): Promise => { + const reader = stream.getReader(); + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (value) total += value.length; + if (done) return total; + } +}; + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -383,3 +480,445 @@ describe("ApiClient.getFileStream / getThumbnailStream", () => { expect(headers.get("X-Auth-Token")).toBe("tk"); }); }); + +// --------------------------------------------------------------------------- +// Retries +// +// The rule the whole section turns on: a request is repeated only when +// repeating it could produce a different answer, and only when repeating it +// cannot do harm. Those are two separate questions and the second one is why +// `postJSON` and `putJSON` behave differently from everything else here. +// +// Every assertion below counts requests. None of them measures how long +// anything took: the retry policy's `sleep` is injected and returns +// immediately, so a machine under load and an idle one produce identical +// results. +// --------------------------------------------------------------------------- + +describe("ApiClient retries", () => { + it("issues exactly one request for a 404", async () => { + // A 404 is an answer, not a failure to get one. Repeating it wastes + // a round trip and — for `listMissingThumbnails`, which reads a 404 + // as "this thumbnail really is missing" — delays a correct result. + // The script holds five responses; only the first may be consumed. + const { fetch, calls } = scriptedFetch( + textResponse("Not Found", 404), + textResponse("Not Found", 404), + textResponse("Not Found", 404), + textResponse("Not Found", 404), + textResponse("Not Found", 404), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect(client.getJSON("/missing")).rejects.toBeInstanceOf( + ApiError, + ); + expect(calls).toHaveLength(1); + }); + + it("retries a 500 up to the configured attempt count, then throws", async () => { + // `attempts` is a total, not a number of retries: three attempts mean + // three requests. The script offers five responses so that a client + // which ignored the limit would be visible as a count of 4 or 5 + // rather than as a crash. + const { fetch, calls } = scriptedFetch( + textResponse("boom", 500), + textResponse("boom", 500), + textResponse("boom", 500), + textResponse("boom", 500), + textResponse("boom", 500), + ); + const client = new ApiClient({ + fetch, + retry: { ...noWait, attempts: 3 }, + }); + + const err: unknown = await client + .getJSON("/flaky") + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(ApiError); + expect((err as ApiError).status).toBe(500); + expect(calls).toHaveLength(3); + }); + + it("returns the first successful response after a 503", async () => { + const { fetch, calls } = scriptedFetch( + textResponse("unavailable", 503), + jsonResponse({ ok: true }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.getJSON<{ ok: boolean }>("/health"), + ).resolves.toEqual({ ok: true }); + expect(calls).toHaveLength(2); + }); + + it("retries 408 and 429", async () => { + // The two 4xx codes that are about timing rather than about the + // request. Backoff is precisely the right response to both. + for (const status of [408, 429]) { + const { fetch, calls } = scriptedFetch( + textResponse("wait", status), + jsonResponse({ ok: true }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.getJSON("/rate-limited"), + ).resolves.toBeDefined(); + expect(calls).toHaveLength(2); + } + }); + + it("retries a fetch rejection and succeeds on a later attempt", async () => { + // A dropped or refused connection is the failure this policy exists + // for: the request never got an answer, so asking again is free of + // consequence and likely to work. + const { fetch, calls } = scriptedFetch( + new TypeError("fetch failed"), + errnoError("ECONNRESET", "read ECONNRESET"), + jsonResponse({ collections: [] }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect(client.getJSON("/collections/v2")).resolves.toEqual({ + collections: [], + }); + expect(calls).toHaveLength(3); + }); + + it("applies the policy to file and thumbnail streams", async () => { + // Both CDN endpoints go through the same wrapper. A 503 from + // files.ente.io during a large backup is common enough that not + // retrying it would fail files for no reason. + for (const get of ["getFileStream", "getThumbnailStream"] as const) { + const { fetch, calls } = scriptedFetch( + textResponse("unavailable", 503), + streamResponse(new Uint8Array([1, 2, 3])), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + const stream = await client[get](7); + expect(await readAll(stream)).toBe(3); + expect(calls).toHaveLength(2); + } + }); + + it("does not retry when the caller opts out", async () => { + // `{ retry: false }` exists for one caller: the download layer, which + // wraps request *and* body consumption *and* decryption in a single + // retry of its own. Without the opt-out the two budgets would + // multiply — four attempts each becoming sixteen requests for one + // file. + const { fetch, calls } = scriptedFetch( + textResponse("unavailable", 503), + streamResponse(new Uint8Array([1, 2, 3])), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.getFileStream(7, { retry: false }), + ).rejects.toBeInstanceOf(ApiError); + expect(calls).toHaveLength(1); + }); + + it("exposes its resolved retry policy to the download layer", async () => { + // The download layer runs its own `withRetry` and must run it under + // the same policy the client was configured with, not under the + // library defaults. + const { fetch } = scriptedFetch(jsonResponse({})); + const client = new ApiClient({ + fetch, + retry: { ...noWait, attempts: 9, baseDelayMs: 7, maxDelayMs: 11 }, + }); + + const policy = client.getRetryOptions(); + expect(policy.attempts).toBe(9); + expect(policy.baseDelayMs).toBe(7); + expect(policy.maxDelayMs).toBe(11); + }); +}); + +describe("ApiClient timeouts", () => { + it("ships bounded default deadlines", () => { + // Asserted here so the README and the code cannot drift. Two numbers + // rather than one, because a deadline that is sane for a JSON call is + // nowhere near enough for a multi-gigabyte body, and a deadline long + // enough for that body would let a hung API call stall a backup for + // ten minutes. + expect(DEFAULT_REQUEST_TIMEOUT_MS).toBe(30_000); + expect(DEFAULT_DOWNLOAD_TIMEOUT_MS).toBe(600_000); + }); + + it("attaches an abort signal to every request", async () => { + const { fetch, calls } = recordingFetch( + jsonResponse({}), + jsonResponse({}), + new Response(null, { status: 200 }), + jsonResponse({}), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await client.getJSON("/a"); + await client.postJSON("/b", {}); + await client.putFile("https://s3.example/x", new Uint8Array([1])); + await client.putJSON("/c", {}); + + for (const call of calls) { + expect(call.init?.signal).toBeInstanceOf(AbortSignal); + } + }); + + it("gives up on a request that never answers, and retries it", async () => { + // Before this policy existed there was no timeout anywhere in quak: a + // CDN connection that accepted the request and then went quiet would + // hang `quak backup` forever. `HANG` reproduces exactly that — the + // fake never answers, so the only thing that can end the call is the + // deadline quak attached. + const { fetch, calls } = scriptedFetch(HANG, HANG, HANG); + const client = new ApiClient({ + fetch, + requestTimeoutMs: 20, + retry: { ...noWait, attempts: 3 }, + }); + + await expect(client.getJSON("/black-hole")).rejects.toThrow(); + // A timeout is retryable, so all three attempts were spent... + expect(calls).toHaveLength(3); + // ...each under its own fresh deadline, not one shared one that + // expired during the first attempt. + const signals = calls.map((c) => c.init?.signal); + expect(new Set(signals).size).toBe(3); + }, 5000); + + it("recovers when a later attempt answers in time", async () => { + const { fetch, calls } = scriptedFetch(HANG, jsonResponse({ ok: 1 })); + const client = new ApiClient({ + fetch, + requestTimeoutMs: 20, + retry: noWait, + }); + + await expect(client.getJSON("/slow-then-fast")).resolves.toEqual({ + ok: 1, + }); + expect(calls).toHaveLength(2); + }, 5000); + + it("aborts a body that stalls after the headers arrived", async () => { + // The failure mode that a naive timeout misses. `getFileStream` + // returns as soon as headers arrive; the bytes are pulled later, in + // the download layer. A deadline that only guarded the initial fetch + // would leave the identical hang one layer down — which is where + // multi-megabyte photo downloads actually stall. + // + // This fake resolves its headers immediately and then serves a body + // that never produces a chunk and never observes the signal, so the + // only thing that can unblock the read is quak's own enforcement of + // the deadline over the stream it hands out. + const stalling = new Response( + new ReadableStream({ + pull: () => new Promise(() => {}), + }), + { status: 200 }, + ); + const { fetch } = scriptedFetch(stalling); + const client = new ApiClient({ + fetch, + downloadTimeoutMs: 20, + retry: { ...noWait, attempts: 1 }, + }); + + const stream = await client.getFileStream(42); + const err: unknown = await readAll(stream).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).name).toBe("TimeoutError"); + }, 5000); + + it("lets a body that arrives in time through untouched", async () => { + // The counterpart to the previous test: enforcing the deadline over + // the stream must not corrupt or truncate a body that is simply being + // read normally. + const payload = new Uint8Array([9, 8, 7, 6, 5]); + const { fetch } = scriptedFetch(streamResponse(payload)); + const client = new ApiClient({ fetch, retry: noWait }); + + const stream = await client.getFileStream(42); + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + chunks.push(value); + } + const joined = new Uint8Array(chunks.reduce((n, c) => n + c.length, 0)); + let offset = 0; + for (const c of chunks) { + joined.set(c, offset); + offset += c.length; + } + expect(joined).toEqual(payload); + }); +}); + +describe("ApiClient error typing", () => { + it("throws ApiError with the status when a presigned PUT fails", async () => { + // `putFile` used to throw a bare Error with the status baked into a + // string. Nothing downstream could classify it, so a 500 from S3 was + // indistinguishable from a bug and could never be retried. + const { fetch, calls } = scriptedFetch( + new Response("Forbidden", { status: 403 }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + const err: unknown = await client + .putFile("https://s3.example/obj", new Uint8Array([1, 2])) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(ApiError); + expect((err as ApiError).status).toBe(403); + expect(calls).toHaveLength(1); + }); + + it("retries a presigned PUT on a 5xx", async () => { + // A presigned PUT writes the whole object at one key in one request, + // so repeating it either overwrites the same bytes or lands them for + // the first time. There is no partial state to protect. + const { fetch, calls } = scriptedFetch( + new Response("slow down", { status: 503 }), + new Response(null, { status: 200 }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await client.putFile("https://s3.example/obj", new Uint8Array([1, 2])); + expect(calls).toHaveLength(2); + }); + + it("throws ApiError when a download response has no body", async () => { + // Also previously a bare Error. It carries the response status so a + // caller can see what arrived — and it is *not* retried: a 200 with + // no body is a malformed response, and asking again produces the same + // malformed response. + for (const get of ["getFileStream", "getThumbnailStream"] as const) { + const { fetch, calls } = scriptedFetch( + new Response(null, { status: 200 }), + new Response(null, { status: 200 }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + const err: unknown = await client[get](5).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(ApiError); + expect((err as ApiError).status).toBe(200); + expect(calls).toHaveLength(1); + } + }); +}); + +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`. + * + * They are retried only when the failure proves the request never reached + * the server, which in practice means the connection was never + * established. Everything else is ambiguous: a 5xx proves the server did + * process the request, and a reset or a timeout can arrive after it did. + * Replaying under that ambiguity can burn a 2FA attempt or register a + * thumbnail twice, and neither is worth the round trip it saves. + */ + it("does not replay a POST after a 5xx", async () => { + const { fetch, calls } = scriptedFetch( + textResponse("boom", 500), + jsonResponse({ sessionID: "second" }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.postJSON("/users/two-factor/verify", { code: "123456" }), + ).rejects.toBeInstanceOf(ApiError); + expect(calls).toHaveLength(1); + }); + + it("does not replay a POST after a mid-flight connection reset", async () => { + // A reset can happen after the request was fully sent and acted on. + // It is retryable in general — `getJSON` retries it — but it is not + // replay-safe. + const { fetch, calls } = scriptedFetch( + errnoError("ECONNRESET", "socket hang up"), + jsonResponse({ sessionID: "second" }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.postJSON("/users/srp/create-session", {}), + ).rejects.toThrow(/ECONNRESET|socket hang up/); + expect(calls).toHaveLength(1); + }); + + it("does not replay a POST after a timeout", async () => { + // A deadline says nothing about whether the server acted. + const { fetch, calls } = scriptedFetch(HANG, jsonResponse({})); + const client = new ApiClient({ + fetch, + requestTimeoutMs: 20, + retry: noWait, + }); + + await expect(client.postJSON("/users/ott", {})).rejects.toThrow(); + expect(calls).toHaveLength(1); + }, 5000); + + it("replays a POST when the connection was never established", async () => { + // A refused connection or a DNS failure happens before any request + // byte is written, so the server cannot have seen it. This is the one + // case where replaying is provably harmless. + const { fetch, calls } = scriptedFetch( + new TypeError("fetch failed", { + cause: errnoError("ECONNREFUSED", "connect ECONNREFUSED"), + }), + errnoError("EAI_AGAIN", "getaddrinfo EAI_AGAIN api.ente.io"), + jsonResponse({ sessionID: "third" }), + ); + const client = new ApiClient({ fetch, retry: noWait }); + + await expect( + client.postJSON<{ sessionID: string }>( + "/users/srp/create-session", + {}, + ), + ).resolves.toEqual({ sessionID: "third" }); + expect(calls).toHaveLength(3); + }); + + it("applies the same rule to PUT", async () => { + // `/files/thumbnail` is reached through `putJSON`. + const failing = scriptedFetch( + textResponse("boom", 503), + jsonResponse({}), + ); + const failingClient = new ApiClient({ + fetch: failing.fetch, + retry: noWait, + }); + await expect( + failingClient.updateThumbnail(1, "key", "header"), + ).rejects.toBeInstanceOf(ApiError); + expect(failing.calls).toHaveLength(1); + + const refused = scriptedFetch( + errnoError("ECONNREFUSED", "connect ECONNREFUSED"), + jsonResponse({}), + ); + const refusedClient = new ApiClient({ + fetch: refused.fetch, + retry: noWait, + }); + await refusedClient.updateThumbnail(1, "key", "header"); + expect(refused.calls).toHaveLength(2); + }); +}); diff --git a/test/cli/backup.test.ts b/test/cli/backup.test.ts index f559b80..5f2efab 100644 --- a/test/cli/backup.test.ts +++ b/test/cli/backup.test.ts @@ -411,11 +411,22 @@ describe("quak backup", () => { // File 101 (sunset.jpg) will return HTTP 500. The other two // files must still download. The result must report the failure // without throwing. + // + // A 500 is retryable, so this file now costs several requests before + // it is given up on — that is the point of the retry policy, and + // `runBackup`'s own resilience is unchanged by it: the retry lives + // strictly below this loop, and an exhausted file is still logged, + // counted, and stepped over rather than aborting the run. The + // injected `sleep` is what keeps the suite from actually waiting out + // the backoff. const outDir = join(testDir, "partial-failure"); const client = await Client.login({ email: TEST_EMAIL, password: TEST_PASSWORD, - apiOptions: { fetch: buildMockFetch(mock, { failFileID: 101 }) }, + apiOptions: { + fetch: buildMockFetch(mock, { failFileID: 101 }), + retry: { sleep: () => Promise.resolve(), random: () => 0 }, + }, }); const result = await runBackup(client, outDir); diff --git a/test/download/download.test.ts b/test/download/download.test.ts index db50a9d..0608381 100644 --- a/test/download/download.test.ts +++ b/test/download/download.test.ts @@ -34,6 +34,15 @@ * forever after. The staging file and the rename are observed directly (see * the `rename` hook below), not inferred from an empty directory. * + * 3. **A failed transfer is retried as a whole.** A download is a request, a + * stream consumption, and a decryption, and only the first of those three + * happens inside `ApiClient`. A socket reset after the response headers + * have arrived therefore surfaces here, in the download layer — and that is + * the dominant failure mode for multi-megabyte photos over a CDN. So the + * entire sequence is retried as one unit, not just the request. The + * secretstream pull state is not resumable and there is no Range support, + * so a retry starts the file over from byte zero. + * * These tests build synthetic encrypted files using sodium's push API, * serve them from a mock fetch, and verify the decrypted output on disk. */ @@ -61,6 +70,8 @@ import { } from "vitest"; import { init, toBase64, STREAM_CHUNK_SIZE } from "../../src/crypto/index.js"; import { ApiClient } from "../../src/api/client.js"; +import { ApiError, TruncatedStreamError } from "../../src/errors.js"; +import type { RetryOptions } from "../../src/retry.js"; import { downloadFile, downloadThumbnail } from "../../src/download/index.js"; import type { EnteFile, FileMetadata } from "../../src/model/types.js"; @@ -326,18 +337,92 @@ beforeAll(() => { multiChunk = encryptMultiChunkBody(multiChunkKey, 1, 1024); }); +/** An error shaped like a Node transport failure: the errno is on `.code`. */ +const errnoError = (code: string, message = code): Error => + Object.assign(new Error(message), { code }); + +/** + * A retry policy with the waiting removed, used by every fixture in this + * file. Backoff arithmetic belongs to `test/retry/retry.test.ts`; here the + * only interesting quantity is how many requests a download issued, so the + * injected `sleep` returns immediately and nothing in this file waits. + */ +const noWait: RetryOptions = { + sleep: () => Promise.resolve(), + random: () => 0, +}; + +/** + * One scripted outcome for a single request to the CDN. + * + * - `body` — a complete response body. + * - `status` — an HTTP error response. + * - `reset` — a response whose headers arrive, whose body delivers `bytes`, + * and which then dies with a socket reset. This is the failure that + * motivates retrying the download rather than the request: by the time it + * happens `ApiClient` has already returned successfully. + */ +type BodyStep = + | { kind: "body"; bytes: Uint8Array } + | { kind: "status"; status: number } + | { kind: "reset"; bytes: Uint8Array }; + +/** + * A fetch that serves one scripted step per call and counts the calls. It + * deliberately refuses to serve more requests than it was given steps for, so + * a retry loop that ran away is a test failure rather than a silent success. + */ +const scriptedCdnFetch = ( + ...steps: BodyStep[] +): { fetch: typeof globalThis.fetch; requests: () => number } => { + let calls = 0; + const fake = async (): Promise => { + const step = steps[calls++]; + if (step === undefined) { + throw new Error(`scriptedCdnFetch: no step for request #${calls}`); + } + if (step.kind === "status") { + return new Response("error", { status: step.status }); + } + if (step.kind === "body") { + return new Response(step.bytes, { status: 200 }); + } + const bytes = step.bytes; + return new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(bytes); + controller.error( + errnoError("ECONNRESET", "aborted by peer"), + ); + }, + }), + { status: 200 }, + ); + }; + return { fetch: fake as typeof globalThis.fetch, requests: () => calls }; +}; + /** * Build an EnteFile plus ApiClient whose file *and* thumbnail streams both * serve `body` under `header`. The download path under test is otherwise * identical for the two, so every truncation/atomicity case below runs * against both entry points from a single fixture. + * + * The default policy here is a single attempt. The failure-contract tests are + * about what the caller and the filesystem are left with, not about how many + * times quak asked; pinning attempts to one keeps them saying exactly that, + * and keeps them from re-decrypting a 4 MiB fixture four times over. The + * retry counts have their own tests at the bottom of this file, which set the + * attempt count explicitly. */ const fixtureFor = ( key: Uint8Array, header: Uint8Array, body: Uint8Array, + retry: RetryOptions = { ...noWait, attempts: 1 }, ): { api: ApiClient; file: EnteFile } => ({ - api: new ApiClient({ fetch: mockFetchForBody(body) }), + api: new ApiClient({ fetch: mockFetchForBody(body), retry }), file: buildMockEnteFile(key, header, header), }); @@ -502,9 +587,17 @@ describe.each(entryPoints)( ); const outPath = join(freshDir(), "truncated.bin"); - await expect(download(api, file, outPath)).rejects.toThrow( - /truncated/i, + const err: unknown = await download(api, file, outPath).catch( + (e: unknown) => e, ); + + // The type, not the wording, is the contract. The retry policy + // classifies truncation as worth another attempt, and it decides + // that with `instanceof`: matching on message text would make + // rewording a diagnostic silently turn every truncated download + // into a permanent failure. + expect(err).toBeInstanceOf(TruncatedStreamError); + expect((err as Error).message).toMatch(/truncated/i); }); it("rejects a body whose final chunk arrived only in part", async () => { @@ -537,7 +630,7 @@ describe.each(entryPoints)( (e: unknown) => e, ); - expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(TruncatedStreamError); expect((err as Error).message).toMatch(/truncated/i); expect((err as Error).cause).toBeInstanceOf(Error); expect(((err as Error).cause as Error).message).toMatch( @@ -564,8 +657,8 @@ describe.each(entryPoints)( ); const outPath = join(freshDir(), "empty.bin"); - await expect(download(api, file, outPath)).rejects.toThrow( - /truncated/i, + await expect(download(api, file, outPath)).rejects.toBeInstanceOf( + TruncatedStreamError, ); }); @@ -588,8 +681,8 @@ describe.each(entryPoints)( const dir = freshDir(); const outPath = join(dir, "absent.bin"); - await expect(download(api, file, outPath)).rejects.toThrow( - /truncated/i, + await expect(download(api, file, outPath)).rejects.toBeInstanceOf( + TruncatedStreamError, ); expect(existsSync(outPath)).toBe(false); @@ -621,10 +714,18 @@ describe.each(entryPoints)( const dir = freshDir(); const outPath = join(dir, "corrupt.bin"); - await expect(download(api, file, outPath)).rejects.toThrow( - /authentication failed/i, + const err: unknown = await download(api, file, outPath).catch( + (e: unknown) => e, ); + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toMatch(/authentication failed/i); + // And explicitly *not* the truncation type, because that type is + // what the retry policy keys on: mislabelling corruption as + // truncation would spend the whole attempt budget re-downloading + // a file that will never decrypt. + expect(err).not.toBeInstanceOf(TruncatedStreamError); + expect(existsSync(outPath)).toBe(false); expect(readdirSync(dir)).toEqual([]); }); @@ -648,8 +749,8 @@ describe.each(entryPoints)( const outPath = join(dir, "existing.bin"); writeFileSync(outPath, existing); - await expect(download(api, file, outPath)).rejects.toThrow( - /truncated/i, + await expect(download(api, file, outPath)).rejects.toBeInstanceOf( + TruncatedStreamError, ); expect(readFileSync(outPath)).toEqual(Buffer.from(existing)); @@ -755,3 +856,208 @@ describe.each(entryPoints)( }); }, ); + +// --------------------------------------------------------------------------- +// Retries +// +// What is retried here is the whole download — request, stream consumption, +// decryption — because only the first of those three happens inside +// `ApiClient`. Every assertion counts requests; none of them measures time. +// --------------------------------------------------------------------------- + +describe.each(entryPoints)("$name retries", ({ name, download }) => { + const freshDir = (): string => mkdtempSync(join(testDir, `${name}-retry-`)); + + /** A cheap single-chunk fixture: no 4 MiB encryption in the retry tests. */ + const smallFixture = ( + seed: number, + ): { + key: Uint8Array; + header: Uint8Array; + ciphertext: Uint8Array; + plaintext: Uint8Array; + } => { + const plaintext = patternBytes(1024, seed); + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptFileBody(plaintext, key); + return { key, header, ciphertext, plaintext }; + }; + + const clientFor = ( + fetch: typeof globalThis.fetch, + attempts: number, + ): ApiClient => new ApiClient({ fetch, retry: { ...noWait, attempts } }); + + it("retries a connection reset that happened mid-body", async () => { + // The case `ApiClient` cannot see. Its own request succeeded: headers + // arrived, a `ReadableStream` was handed back, and only then did the + // socket die. Retrying the fetch alone would have caught nothing, + // which is why the retry wraps the whole sequence. + const { key, header, ciphertext, plaintext } = smallFixture(41); + const { fetch, requests } = scriptedCdnFetch( + { kind: "reset", bytes: ciphertext.slice(0, 16) }, + { kind: "body", bytes: ciphertext }, + ); + const api = clientFor(fetch, 4); + const file = buildMockEnteFile(key, header, header); + const dir = freshDir(); + const outPath = join(dir, "reset-then-ok.bin"); + + const result = await download(api, file, outPath); + + expect(requests()).toBe(2); + expect(result.bytesWritten).toBe(plaintext.length); + expectSameBytes(readFileSync(outPath), plaintext); + }); + + it("stages one temp file for the attempt that succeeded, not one per attempt", async () => { + // The atomic write stays outside the retry loop. A retried download + // must not leave a trail of half-written scratch files, and the + // destination must be touched exactly once — by the attempt that + // produced a complete, authenticated plaintext. + const { key, header, ciphertext } = smallFixture(42); + const { fetch } = scriptedCdnFetch( + { kind: "reset", bytes: ciphertext.slice(0, 16) }, + { kind: "reset", bytes: ciphertext.slice(0, 16) }, + { kind: "body", bytes: ciphertext }, + ); + const api = clientFor(fetch, 4); + const file = buildMockEnteFile(key, header, header); + const dir = freshDir(); + const outPath = join(dir, "one-stage.bin"); + + await download(api, file, outPath); + + expect(renameHook.calls).toHaveLength(1); + expect(renameHook.calls[0]!.to).toBe(outPath); + expect(readdirSync(dir)).toEqual(["one-stage.bin"]); + }); + + it("retries a truncated body and gives up after the configured attempts", async () => { + // Truncation is retryable — the file on the server is intact, the + // transfer was not — but it is not retryable forever. Three attempts + // configured, three requests, then the caller gets the error. + const key = sodium.crypto_secretstream_xchacha20poly1305_keygen(); + const { header, ciphertext } = encryptNonFinalBody( + patternBytes(256, 43), + key, + ); + const { fetch, requests } = scriptedCdnFetch( + { kind: "body", bytes: ciphertext }, + { kind: "body", bytes: ciphertext }, + { kind: "body", bytes: ciphertext }, + { kind: "body", bytes: ciphertext }, + ); + const api = clientFor(fetch, 3); + const file = buildMockEnteFile(key, header, header); + const dir = freshDir(); + const outPath = join(dir, "always-truncated.bin"); + + await expect(download(api, file, outPath)).rejects.toBeInstanceOf( + TruncatedStreamError, + ); + + expect(requests()).toBe(3); + // Every attempt failed before anything was written, so the directory + // is still empty. + expect(readdirSync(dir)).toEqual([]); + }); + + it("issues exactly one request when the file is gone", async () => { + // A 404 from the CDN is an answer. `runBackup` logs it and moves on; + // spending three more requests and three backoff waits on it would + // slow a large backup down for nothing. + const { key, header } = smallFixture(44); + const { fetch, requests } = scriptedCdnFetch( + { kind: "status", status: 404 }, + { kind: "status", status: 404 }, + { kind: "status", status: 404 }, + { kind: "status", status: 404 }, + ); + const api = clientFor(fetch, 4); + const file = buildMockEnteFile(key, header, header); + const outPath = join(freshDir(), "gone.bin"); + + const err: unknown = await download(api, file, outPath).catch( + (e: unknown) => e, + ); + + expect(err).toBeInstanceOf(ApiError); + expect((err as ApiError).status).toBe(404); + expect(requests()).toBe(1); + }); + + it("retries a 503 from the CDN", async () => { + const { key, header, ciphertext, plaintext } = smallFixture(45); + const { fetch, requests } = scriptedCdnFetch( + { kind: "status", status: 503 }, + { kind: "status", status: 503 }, + { kind: "body", bytes: ciphertext }, + ); + const api = clientFor(fetch, 4); + const file = buildMockEnteFile(key, header, header); + const outPath = join(freshDir(), "flaky-cdn.bin"); + + await download(api, file, outPath); + + expect(requests()).toBe(3); + expectSameBytes(readFileSync(outPath), plaintext); + }); + + it("spends one attempt budget, not one per layer", async () => { + // `ApiClient.getFileStream` retries on its own for direct callers. + // The download layer opts out of that and runs its own retry over the + // whole sequence. If it did not, the two budgets would compose: three + // attempts here would become nine requests to the CDN for a single + // file, and the default four would become sixteen. + const { key, header } = smallFixture(46); + const steps: BodyStep[] = Array.from({ length: 12 }, () => ({ + kind: "status" as const, + status: 503, + })); + const { fetch, requests } = scriptedCdnFetch(...steps); + const api = clientFor(fetch, 3); + const file = buildMockEnteFile(key, header, header); + const outPath = join(freshDir(), "budget.bin"); + + await expect(download(api, file, outPath)).rejects.toBeInstanceOf( + ApiError, + ); + + expect(requests()).toBe(3); + }); +}); + +describe("download retries: corruption is not retried", () => { + it("gives up immediately on a chunk that failed to authenticate", async () => { + // A whole chunk that failed to authenticate while the stream + // continued past it is corruption or a wrong key. Neither is fixed by + // asking again, and a backup run that retried every such file would + // multiply the cost of a genuinely broken file by the attempt count. + // + // This is also the boundary of the single-chunk ambiguity documented + // at the classifier: the split is only achievable because this body + // has more than one chunk. + const corrupted = Uint8Array.from(multiChunk.body); + corrupted[10] ^= 0xff; + const { fetch, requests } = scriptedCdnFetch( + { kind: "body", bytes: corrupted }, + { kind: "body", bytes: corrupted }, + { kind: "body", bytes: corrupted }, + { kind: "body", bytes: corrupted }, + ); + const api = new ApiClient({ fetch, retry: { ...noWait, attempts: 4 } }); + const file = buildMockEnteFile( + multiChunkKey, + multiChunk.header, + multiChunk.header, + ); + const outPath = join(mkdtempSync(join(testDir, "corrupt-")), "c.bin"); + + await expect(downloadFile(api, file, outPath)).rejects.toThrow( + /authentication failed/i, + ); + + expect(requests()).toBe(1); + }); +}); diff --git a/test/retry/retry.test.ts b/test/retry/retry.test.ts new file mode 100644 index 0000000..42eefed --- /dev/null +++ b/test/retry/retry.test.ts @@ -0,0 +1,557 @@ +/** + * Tests for `src/retry.ts` — the retry policy shared by every network + * operation in quak. + * + * Two things live in that module and they are deliberately separate: + * + * - **`isRetryable(err)`**, a pure classifier. Given an error, is trying + * again capable of producing a different answer? Nothing else about the + * error matters: not how it was logged, not where it came from. + * + * - **`withRetry(fn, opts)`**, the loop. It calls `fn`, and while the error + * is classified retryable and attempts remain, it sleeps and calls `fn` + * again. It never inspects errors itself. + * + * The classifier's default answer is *no*. quak is a backup tool: a wrongly + * retried permanent failure costs a user round trips and delays the rest of + * the run, while a wrongly rejected transient failure costs one file that the + * next run picks up. When in doubt, fail fast. + * + * ## Reading the backoff assertions + * + * `withRetry` takes its `sleep` and its `random` as injected functions. Every + * test here passes a `sleep` that records the delay it was asked for and + * returns immediately, so the suite never waits, and a `random` that returns a + * fixed number, so jitter is exact rather than approximate. **No assertion in + * this file (or anywhere else in the suite) is about elapsed wall-clock time.** + * They are about what `withRetry` asked for, and how many times `fn` ran. + * + * The delay before retry number *n* (1-based) is: + * + * random() * min(maxDelayMs, baseDelayMs * 2 ** (n - 1)) + * + * That is exponential backoff with full jitter: the exponential term is the + * *ceiling*, and the actual wait is drawn uniformly below it. Full jitter, + * rather than a fixed delay plus noise, is what stops a client that lost a + * hundred parallel downloads to one CDN blip from re-sending all hundred at + * the same instant. + */ + +import { describe, expect, it } from "vitest"; +import { + DEFAULT_RETRY_OPTIONS, + isRetryable, + isSafeToReplay, + resolveRetryOptions, + withRetry, +} from "../../src/retry.js"; +import { ApiError, TruncatedStreamError } from "../../src/errors.js"; +import { ApiError as ApiErrorFromClient } from "../../src/api/client.js"; + +// --------------------------------------------------------------------------- +// Test helpers +// --------------------------------------------------------------------------- + +/** + * A `sleep` that records what it was asked to wait for and returns + * immediately. This is the whole reason `withRetry` takes an injected sleep: + * the retry policy is exercised in full — every branch, every delay — without + * the suite spending a single millisecond waiting. + */ +const recordingSleep = (): { + sleep: (ms: number) => Promise; + delays: number[]; +} => { + const delays: number[] = []; + return { + sleep: (ms: number): Promise => { + delays.push(ms); + return Promise.resolve(); + }, + delays, + }; +}; + +/** An error shaped like a Node transport failure: the errno is on `.code`. */ +const errnoError = (code: string, message = code): Error => + Object.assign(new Error(message), { code }); + +// --------------------------------------------------------------------------- +// Classification +// --------------------------------------------------------------------------- + +describe("isRetryable: HTTP status codes", () => { + it("does not retry ordinary 4xx responses", () => { + // A 4xx is the server saying the request itself is wrong. Repeating + // it verbatim produces the same answer, so retrying only delays the + // failure the caller has to handle. 404 is the load-bearing case: + // `listMissingThumbnails` depends on a 404 arriving promptly and + // exactly once. + for (const status of [400, 401, 403, 404, 409, 410, 422]) { + expect(isRetryable(new ApiError(`HTTP ${status}`, status))).toBe( + false, + ); + } + }); + + it("retries 408 and 429", () => { + // The two 4xx codes that are statements about timing rather than + // about the request. 408 is the server admitting it gave up waiting; + // 429 is it asking for less traffic — which backoff supplies. + expect(isRetryable(new ApiError("timeout", 408))).toBe(true); + expect(isRetryable(new ApiError("slow down", 429))).toBe(true); + }); + + it("retries every 5xx response", () => { + // A 5xx is the server failing, not the request being wrong. Ente's + // CDN in particular returns 500 and 503 under load. + for (const status of [500, 502, 503, 504, 599]) { + expect(isRetryable(new ApiError(`HTTP ${status}`, status))).toBe( + true, + ); + } + }); + + it("treats a 2xx or 3xx ApiError as not retryable", () => { + // These exist: `getFileStream` raises an ApiError carrying the + // response status when a 200 arrives with a null body. That is a + // malformed response, not a transport failure, and repeating the + // request will produce the same malformed response. + expect(isRetryable(new ApiError("response body is null", 200))).toBe( + false, + ); + expect(isRetryable(new ApiError("redirect", 304))).toBe(false); + }); + + it("uses the same ApiError class that ApiClient exports", () => { + // `ApiError` lives in `src/errors.ts` and is re-exported from + // `src/api/client.ts`, which is where every existing caller and test + // imports it from. If those ever became two separate classes the + // classifier would silently stop recognising errors raised by the + // client, and every 5xx in the wild would be treated as permanent. + expect(ApiErrorFromClient).toBe(ApiError); + expect(new ApiErrorFromClient("boom", 503)).toBeInstanceOf(ApiError); + expect(isRetryable(new ApiErrorFromClient("boom", 503))).toBe(true); + }); +}); + +describe("isRetryable: transport failures", () => { + it("retries a TypeError, which is how fetch reports a failed request", () => { + // Node's fetch rejects with `TypeError: fetch failed` for everything + // below HTTP: DNS failure, refused connection, TLS error, reset + // socket. The real diagnosis is on `cause`, but there is nothing on + // the object that distinguishes it from a TypeError thrown by a bug, + // so this rule is deliberately literal. The cost of the imprecision + // is bounded by the attempt count; the alternative — demanding a + // recognised `cause` — would classify real network failures as + // permanent and fail backups that should have succeeded. + expect(isRetryable(new TypeError("fetch failed"))).toBe(true); + }); + + it("retries an errno carried on the error itself", () => { + for (const code of [ + "ECONNRESET", + "ETIMEDOUT", + "EPIPE", + "ENOTFOUND", + "EAI_AGAIN", + "ECONNREFUSED", + "EHOSTUNREACH", + "ENETUNREACH", + ]) { + expect(isRetryable(errnoError(code))).toBe(true); + } + }); + + it("retries an errno buried in the cause chain", () => { + // undici does not put the errno on the error it throws; it hangs the + // underlying socket error off `cause`, sometimes more than one level + // down. A classifier that only looked at the top-level error would + // see a bare `Error` and call every dropped connection permanent. + const nested = new Error("request to files.ente.io failed", { + cause: new Error("socket hang up", { + cause: errnoError("ECONNRESET", "read ECONNRESET"), + }), + }); + expect(isRetryable(nested)).toBe(true); + }); + + it("accepts a plain object as a cause", () => { + // Not everything in a cause chain is an Error instance. + expect( + isRetryable(new Error("failed", { cause: { code: "ETIMEDOUT" } })), + ).toBe(true); + }); + + it("retries an aborted request", () => { + // `AbortSignal.timeout()` aborts with a `TimeoutError`; an explicit + // `abort()` produces an `AbortError`. quak only ever aborts a request + // on its own deadline, so both mean "this attempt ran out of time", + // which is exactly the condition a later attempt might not hit. + expect(isRetryable(new DOMException("timed out", "TimeoutError"))).toBe( + true, + ); + expect(isRetryable(new DOMException("aborted", "AbortError"))).toBe( + true, + ); + }); + + it("does not confuse an unrelated errno with a transport failure", () => { + // A filesystem error surfaces the same way an errno network error + // does. Retrying a full disk or a missing directory is pointless. + expect(isRetryable(errnoError("ENOSPC", "no space left"))).toBe(false); + expect(isRetryable(errnoError("ENOENT", "no such file"))).toBe(false); + expect(isRetryable(errnoError("EACCES", "permission denied"))).toBe( + false, + ); + }); + + it("terminates on a cause chain that points at itself", () => { + // Defensive: `cause` is an arbitrary user-settable property and + // nothing stops it forming a cycle. Without a bound on the walk this + // classifier would hang the process, which is a worse failure than + // any misclassification. + const looped: Error & { cause?: unknown } = new Error("loop"); + looped.cause = looped; + expect(isRetryable(looped)).toBe(false); + }); +}); + +describe("isRetryable: stream truncation versus corruption", () => { + it("retries a truncated stream", () => { + // Truncation is a transfer that stopped early. The bytes that did + // arrive are useless, but the file on the server is fine, so asking + // again is exactly right. + expect(isRetryable(new TruncatedStreamError("stream truncated"))).toBe( + true, + ); + }); + + it("retries a truncated stream whose cause is an authentication failure", () => { + // The single-chunk ambiguity, recorded on issue #2 and inherited from + // the truncation work: when a body ends part-way through a chunk, + // Poly1305 fails and carries no framing signal, so a cut connection + // and genuinely corrupt bytes are indistinguishable. That case is + // reported as truncation with the authentication failure preserved as + // `cause`, and it is therefore retried. + // + // Retrying is the deliberate choice. For a multi-chunk body the split + // is real — a corrupt chunk mid-stream stays an authentication + // failure, see the next test — but for a single-chunk body (most + // thumbnails, every small file) a wrong key and a cut connection look + // identical. The cost of guessing wrong is bounded: a few extra round + // trips before the same failure. The cost of guessing the other way + // is a silently truncated file kept forever. + const err = new TruncatedStreamError("stream truncated", { + cause: new Error("secretstream chunk authentication failed"), + }); + expect(isRetryable(err)).toBe(true); + }); + + it("does not retry an authentication failure that is not truncation", () => { + // A whole chunk that failed to authenticate while the stream carried + // on past it cannot be a short transfer. It is corruption or a wrong + // key, and no number of retries fixes either. + expect( + isRetryable(new Error("secretstream chunk authentication failed")), + ).toBe(false); + }); +}); + +describe("isRetryable: everything else", () => { + it("does not retry programming errors or unknown values", () => { + expect(isRetryable(new Error("boom"))).toBe(false); + expect(isRetryable(new RangeError("out of range"))).toBe(false); + expect(isRetryable(new SyntaxError("bad JSON"))).toBe(false); + expect(isRetryable("a string")).toBe(false); + expect(isRetryable(undefined)).toBe(false); + expect(isRetryable(null)).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// The replay-safety classifier for non-idempotent requests +// --------------------------------------------------------------------------- + +describe("isSafeToReplay", () => { + /** + * `isRetryable` answers "could a retry succeed?". For a POST or a PUT + * 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 failures that prove no request + * byte ever reached the server — which means the connection was never + * established. + */ + it("replays only failures where the connection was never established", () => { + for (const code of [ + "ENOTFOUND", + "EAI_AGAIN", + "ECONNREFUSED", + "EHOSTUNREACH", + "ENETUNREACH", + ]) { + expect(isSafeToReplay(errnoError(code))).toBe(true); + } + // Also when undici has buried it, which is how it actually arrives. + expect( + isSafeToReplay( + new TypeError("fetch failed", { + cause: errnoError("ECONNREFUSED"), + }), + ), + ).toBe(true); + }); + + it("does not replay a failure that could have happened after the server acted", () => { + // Every one of these is ambiguous about whether the server processed + // the request. A 5xx proves it did. A reset or a broken pipe can + // arrive after the request was fully sent and handled. A timeout says + // nothing at all about the server's state. A bare `fetch failed` with + // no recognisable cause could be any of them. + expect(isSafeToReplay(new ApiError("HTTP 500", 500))).toBe(false); + expect(isSafeToReplay(new ApiError("HTTP 429", 429))).toBe(false); + expect(isSafeToReplay(errnoError("ECONNRESET"))).toBe(false); + expect(isSafeToReplay(errnoError("EPIPE"))).toBe(false); + expect(isSafeToReplay(errnoError("ETIMEDOUT"))).toBe(false); + expect( + isSafeToReplay(new DOMException("timed out", "TimeoutError")), + ).toBe(false); + expect(isSafeToReplay(new TypeError("fetch failed"))).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// The retry loop +// --------------------------------------------------------------------------- + +describe("withRetry", () => { + it("calls the function once and does not sleep when it succeeds", async () => { + const { sleep, delays } = recordingSleep(); + let calls = 0; + const result = await withRetry( + () => { + calls++; + return Promise.resolve("ok"); + }, + { sleep }, + ); + + expect(result).toBe("ok"); + expect(calls).toBe(1); + expect(delays).toEqual([]); + }); + + it("stops at the first success and returns its value", async () => { + const { sleep, delays } = recordingSleep(); + let calls = 0; + const result = await withRetry( + () => { + calls++; + if (calls < 3) { + return Promise.reject(new ApiError("HTTP 503", 503)); + } + return Promise.resolve(calls); + }, + { attempts: 5, sleep }, + ); + + expect(result).toBe(3); + expect(calls).toBe(3); + // Two failures, so two waits — and none after the attempt that + // succeeded. + expect(delays).toHaveLength(2); + }); + + it("gives up after `attempts` calls and throws the last error", async () => { + // `attempts` counts calls, not retries: `attempts: 3` means the + // function runs three times in total. The error that escapes is the + // one from the final attempt, because that is the current state of + // the world; a caller logging it is logging what is true now. + const { sleep, delays } = recordingSleep(); + let calls = 0; + const failure = withRetry( + () => { + calls++; + return Promise.reject(new ApiError(`attempt ${calls}`, 503)); + }, + { attempts: 3, sleep }, + ); + + await expect(failure).rejects.toThrow("attempt 3"); + expect(calls).toBe(3); + // Three attempts, two gaps between them. Sleeping after the last + // attempt would delay the caller's failure for nothing. + expect(delays).toHaveLength(2); + }); + + it("does not retry at all when attempts is 1", async () => { + const { sleep, delays } = recordingSleep(); + let calls = 0; + await expect( + withRetry( + () => { + calls++; + return Promise.reject(new ApiError("HTTP 500", 500)); + }, + { attempts: 1, sleep }, + ), + ).rejects.toThrow("HTTP 500"); + + expect(calls).toBe(1); + expect(delays).toEqual([]); + }); + + it("rethrows a non-retryable error immediately", async () => { + const { sleep, delays } = recordingSleep(); + let calls = 0; + await expect( + withRetry( + () => { + calls++; + return Promise.reject(new ApiError("HTTP 404", 404)); + }, + { attempts: 5, sleep }, + ), + ).rejects.toBeInstanceOf(ApiError); + + expect(calls).toBe(1); + expect(delays).toEqual([]); + }); + + it("preserves the error object, not just its message", async () => { + // Callers classify what escapes: `listMissingThumbnails` needs the + // `ApiError` and its status to tell a genuine 404 from a transient + // failure. Wrapping the error in a "retries exhausted" error would + // break that. + const original = new ApiError("gone", 410, { code: "GONE" }); + const err: unknown = await withRetry(() => Promise.reject(original), { + sleep: () => Promise.resolve(), + }).catch((e: unknown) => e); + + expect(err).toBe(original); + }); + + it("honours a caller-supplied classifier", async () => { + // This is how the non-idempotent call sites narrow the policy: same + // loop, same backoff, stricter question. + const { sleep } = recordingSleep(); + let calls = 0; + await expect( + withRetry( + () => { + calls++; + // Retryable under the default policy... + return Promise.reject(new ApiError("HTTP 503", 503)); + }, + { attempts: 4, sleep, isRetryable: isSafeToReplay }, + ), + ).rejects.toThrow("HTTP 503"); + + // ...but not under `isSafeToReplay`, so it ran exactly once. + expect(calls).toBe(1); + }); +}); + +describe("withRetry backoff", () => { + it("doubles the ceiling on each retry and caps it", async () => { + // `random: () => 1` pins the jitter to the top of its range, which + // makes the ceiling itself observable. The sequence is + // base, base*2, base*4, ... clamped at maxDelayMs — so a long outage + // settles into a steady poll instead of growing to hours. + const { sleep, delays } = recordingSleep(); + await expect( + withRetry(() => Promise.reject(new ApiError("HTTP 500", 500)), { + attempts: 6, + baseDelayMs: 100, + maxDelayMs: 250, + sleep, + random: () => 1, + }), + ).rejects.toThrow(); + + expect(delays).toEqual([100, 200, 250, 250, 250]); + }); + + it("draws each delay uniformly below its ceiling", async () => { + // Full jitter. The exponential value is the maximum wait, not the + // wait itself, so a fleet of clients that failed together does not + // come back in lockstep. + const { sleep, delays } = recordingSleep(); + await expect( + withRetry(() => Promise.reject(new ApiError("HTTP 500", 500)), { + attempts: 4, + baseDelayMs: 100, + maxDelayMs: 10_000, + sleep, + random: () => 0.25, + }), + ).rejects.toThrow(); + + expect(delays).toEqual([25, 50, 100]); + }); + + it("never asks to sleep longer than the cap or less than zero", async () => { + // Whatever `random` returns from its [0, 1) contract, the delay stays + // inside the configured envelope. + const draws = [0, 0.999_999, 0.5, 0.1, 0.9]; + let i = 0; + const { sleep, delays } = recordingSleep(); + await expect( + withRetry(() => Promise.reject(new ApiError("HTTP 500", 500)), { + attempts: 6, + baseDelayMs: 1000, + maxDelayMs: 2000, + sleep, + random: () => draws[i++]!, + }), + ).rejects.toThrow(); + + expect(delays).toHaveLength(5); + for (const d of delays) { + expect(d).toBeGreaterThanOrEqual(0); + expect(d).toBeLessThanOrEqual(2000); + } + expect(delays[0]).toBe(0); + }); +}); + +describe("retry defaults", () => { + it("ships a bounded, documented default policy", () => { + // These are the numbers the README documents. They are asserted here + // so the README and the code cannot drift apart silently: four + // attempts, half a second of base delay, ten seconds of ceiling — + // under 15 seconds of waiting in the worst case, which keeps a + // several-thousand-file backup moving past a bad file rather than + // stalling on it. + expect(DEFAULT_RETRY_OPTIONS.attempts).toBe(4); + expect(DEFAULT_RETRY_OPTIONS.baseDelayMs).toBe(500); + expect(DEFAULT_RETRY_OPTIONS.maxDelayMs).toBe(10_000); + }); + + it("fills in only the fields the caller left out", () => { + const resolved = resolveRetryOptions({ attempts: 2 }); + expect(resolved.attempts).toBe(2); + expect(resolved.baseDelayMs).toBe(DEFAULT_RETRY_OPTIONS.baseDelayMs); + expect(resolved.maxDelayMs).toBe(DEFAULT_RETRY_OPTIONS.maxDelayMs); + expect(typeof resolved.sleep).toBe("function"); + expect(typeof resolved.random).toBe("function"); + }); + + it("resolves to the defaults when given nothing", () => { + expect(resolveRetryOptions()).toEqual(DEFAULT_RETRY_OPTIONS); + expect(resolveRetryOptions({})).toEqual(DEFAULT_RETRY_OPTIONS); + }); + + it("defaults random to a real generator in [0, 1)", () => { + const { random } = resolveRetryOptions(); + for (let i = 0; i < 100; i++) { + const r = random(); + expect(r).toBeGreaterThanOrEqual(0); + expect(r).toBeLessThan(1); + } + }); +}); diff --git a/test/thumbnails/thumbnails.test.ts b/test/thumbnails/thumbnails.test.ts index 314eb3d..2743d17 100644 --- a/test/thumbnails/thumbnails.test.ts +++ b/test/thumbnails/thumbnails.test.ts @@ -32,6 +32,7 @@ import { fixMissingThumbnails, } from "../../src/thumbnails.js"; import type { KeyAttributes } from "../../src/auth/types.js"; +import type { RetryOptions } from "../../src/retry.js"; // --------------------------------------------------------------------------- // Mock server with controllable thumbnail behavior @@ -51,7 +52,7 @@ interface ThumbMockState { filesByCollection: Record[]>; fileCiphertexts: Record; fileKeys: Record; - thumbnailBehavior: Record; + thumbnailBehavior: Record; // Captures from fix operations uploadedThumbnails: { fileID: number; @@ -302,6 +303,9 @@ const buildThumbFetch = (m: ThumbMockState) => { if (behavior === "empty") { return new Response(new Uint8Array(0), { status: 200 }); } + if (behavior === "500") { + return new Response("Internal Server Error", { status: 500 }); + } return new Response("not found", { status: 404 }); } @@ -345,6 +349,38 @@ const buildThumbFetch = (m: ThumbMockState) => { }) as typeof globalThis.fetch; }; +/** + * A retry policy with the waiting removed. `listMissingThumbnails` walks every + * file in the account, so a transient failure is retried; without an injected + * `sleep` these tests would spend real seconds waiting out backoff. + */ +const noWait: RetryOptions = { + sleep: () => Promise.resolve(), + random: () => 0, +}; + +/** Wrap a fetch so the tests can count how often one endpoint was hit. */ +const countingFetch = ( + inner: typeof globalThis.fetch, + match: (url: string) => boolean, +): { fetch: typeof globalThis.fetch; matched: () => number } => { + let matched = 0; + const fake = async ( + input: RequestInfo | URL, + init?: RequestInit, + ): Promise => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.href + : input.url; + if (match(url)) matched++; + return inner(input, init); + }; + return { fetch: fake as typeof globalThis.fetch, matched: () => matched }; +}; + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -378,7 +414,77 @@ describe("listMissingThumbnails", () => { expect(emptyEntry.collection).toBe("Photos"); const notFoundEntry = missing.find((m) => m.fileID === 102)!; - expect(notFoundEntry.reason).toContain("fetch failed"); + // A 404 is the server stating the thumbnail is not there. That is the + // only network answer that means "missing", and the reason says so + // rather than the older catch-all "fetch failed" — which used to + // cover a 500 and a dropped connection too. + expect(notFoundEntry.reason).toContain("not found"); + }); + + it("does not report a thumbnail as missing when the server is failing", async () => { + // The distinction that matters for `helper fix-missing-thumbnails`. + // Reporting a file here leads to downloading the original, + // regenerating a thumbnail, and uploading it over a thumbnail that + // was fine all along — because the server was briefly returning 500s. + // + // File 102 serves 500 on every attempt, so the retries are genuinely + // exhausted. It must still not be reported. + const failingMock = await buildThumbMock(); + failingMock.thumbnailBehavior[102] = "500"; + + const counted = countingFetch( + buildThumbFetch(failingMock), + (url) => url.includes("thumbnails.ente.io") && url.includes("102"), + ); + const client = await Client.login({ + email: TEST_EMAIL, + password: TEST_PASSWORD, + apiOptions: { fetch: counted.fetch, retry: { ...noWait } }, + }); + + const missing = await listMissingThumbnails(client); + + // Only the genuinely empty thumbnail is reported. + expect(missing.map((m) => m.fileID)).toEqual([101]); + // And the 500 was retried rather than accepted as an answer: four + // attempts is the library default. + expect(counted.matched()).toBe(4); + }); + + it("does not report a thumbnail as missing when the connection fails", async () => { + // Same rule for a transport failure, which carries no status at all. + const failingMock = await buildThumbMock(); + const inner = buildThumbFetch(failingMock); + let thumbRequests = 0; + const fetch = (async ( + input: RequestInfo | URL, + init?: RequestInit, + ): Promise => { + const url = + typeof input === "string" + ? input + : input instanceof URL + ? input.href + : input.url; + if (url.includes("thumbnails.ente.io") && url.includes("102")) { + thumbRequests++; + throw Object.assign(new Error("socket hang up"), { + code: "ECONNRESET", + }); + } + return inner(input, init); + }) as typeof globalThis.fetch; + + const client = await Client.login({ + email: TEST_EMAIL, + password: TEST_PASSWORD, + apiOptions: { fetch, retry: { ...noWait } }, + }); + + const missing = await listMissingThumbnails(client); + + expect(missing.map((m) => m.fileID)).toEqual([101]); + expect(thumbRequests).toBe(4); }); it("deduplicates files seen in multiple collections", async () => { -- 2.49.1 From f3cf4af833df97026453d2a2d7966423052fe658 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 05:21:31 +0000 Subject: [PATCH 2/3] Retry transient network failures with exponential backoff (closes #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No retry on 4xx, backoff on 5xx and transport failures, and a deadline on every request. Before this, one transient 503 or TCP reset failed a file for good, and a CDN connection that went quiet after accepting the request blocked `quak backup` forever, because there was no timeout anywhere. src/retry.ts holds the policy: a classifier that decides whether another attempt could produce a different answer, and a loop that acts on it with exponential backoff and full jitter. Retried: 5xx, 408, 429, transport failures (the errno is read out of the cause chain, which is where Node's fetch puts it), deadline aborts, and truncated transfers. Not retried: every other 4xx, and anything unrecognised — a wrongly retried permanent failure delays every remaining file, while a wrongly abandoned transient one costs a single file the next run picks up. Attempt count, delays, sleep and jitter source are all configurable through ApiClientOptions; sleep being injectable is what lets the suite exercise the policy without waiting. Truncation needed a type before it could be classified. streamDecrypt threw plain Errors whose messages began "download: stream truncated", and classifying on message text would mean the next reword silently turned every truncated download into a permanent failure. It now throws TruncatedStreamError, which lives in src/errors.ts alongside ApiError so the classifier can recognise both without importing the modules that import it; api/client.ts re-exports ApiError, so it stays one class and every existing import path still resolves. Downloads retry the request, the stream consumption and the decryption together. Only the first of those happens inside ApiClient: a socket reset after the headers arrived throws in streamDecrypt, and retrying the request alone would never see it. The client's own retry is switched off for those two calls so the budgets do not multiply into sixteen requests per file, and the atomic write stays outside the loop so a download that took three attempts still performs one write and one rename. Non-idempotent requests are not blindly replayed. postJSON and putJSON reach create-session, two-factor/verify — which burns one of a few second-factor attempts — and files/thumbnail, so they retry only when the connection was never established and the server provably never saw the request. putFile is exempt and retries fully: a presigned PUT stores one whole object at one key, with no partial state to damage. It now throws ApiError with the status, as do the two null-body paths, which previously threw bare Errors that nothing could classify. Timeouts come from AbortSignal.timeout(), renewed per attempt: 30s for JSON and upload calls, 10 minutes for file bodies, since a value short enough to keep a hung API call from stalling a backup would cancel a legitimate multi-gigabyte download. The download deadline is enforced over the body rather than only the headers, by racing each read against the signal, so the guarantee does not depend on the fetch implementation tearing down a stream it already handed over. listMissingThumbnails now separates a genuine 404 from an exhausted retry. Its bare catch reported both as missing, which after this change would have let a few minutes of 500s talk fix-missing-thumbnails into regenerating and re-uploading thumbnails that were fine. runBackup and runMetadataBackup are untouched: the retry sits below them and their per-file resilience is unchanged. --- README.md | 90 ++++++++++++- TODO.md | 13 +- src/api/client.ts | 273 +++++++++++++++++++++++++++++---------- src/download/index.ts | 54 +++++++- src/errors.ts | 45 +++++++ src/index.ts | 20 ++- src/retry.ts | 193 +++++++++++++++++++++++++++ src/thumbnails.ts | 33 ++++- test/retry/retry.test.ts | 10 +- 9 files changed, 637 insertions(+), 94 deletions(-) create mode 100644 src/errors.ts create mode 100644 src/retry.ts diff --git a/README.md b/README.md index 0762343..6145269 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,8 @@ quak/ model/ decrypted Collection, File, Metadata types + decrypt fns download/ streaming file/thumbnail download + decryption backup.ts resilient full-account backup with dedup + errors.ts error types shared across layers + retry.ts retry classifier + exponential backoff with jitter thumbnails.ts detect + regenerate missing thumbnails client.ts high-level Client class assembled from the above index.ts public library exports @@ -250,6 +252,87 @@ Endpoints used: - `POST /files/upload-url`: mint a presigned upload URL (for thumbnail repair). - `PUT /files/thumbnail`: register an uploaded thumbnail's object key. +### Retries and timeouts + +Every request in the library goes through one policy, in `src/retry.ts`. A +request is repeated only when repeating it could produce a different answer: + +- `ApiError` with a 5xx status: retried. So are `408` and `429`, the two 4xx + codes that are statements about timing rather than about the request. +- Every other 4xx: not retried. A 404 in particular is an answer, and + `listMissingThumbnails` depends on getting it promptly and once. +- Transport failures — a `fetch` rejection, `ECONNRESET`, `ETIMEDOUT`, a DNS or + TLS failure — and deadline aborts: retried. The errno is looked for in the + error's `cause` chain, because that is where Node's `fetch` puts it. +- A truncated download: retried. +- Anything else, including a secretstream authentication failure that is not + truncation: not retried. The default answer is no. For a backup tool, retrying + a permanent failure spends round trips and delays every remaining file, while + declining to retry a transient one costs a single file that the next run picks + up. + +Backoff is exponential with full jitter: the delay before retry _n_ is +`random() * min(maxDelayMs, baseDelayMs * 2 ** (n - 1))`. The exponential term +is the ceiling and the wait is drawn below it, so a client that lost many +parallel downloads to one CDN blip does not send them all again at the same +instant. Defaults, configurable through `ApiClientOptions.retry`: + +| Option | Default | Meaning | +| ------------- | ------- | ----------------------------------- | +| `attempts` | `4` | total calls, not retries | +| `baseDelayMs` | `500` | ceiling for the first retry's delay | +| `maxDelayMs` | `10000` | upper bound on that ceiling | + +With those defaults a file that is going to fail gives up after at most three +and a half seconds of waiting. `sleep` and `random` are injectable through the +same option, which is how the test suite exercises the whole policy without +waiting. + +Two deadlines, applied with `AbortSignal.timeout()` and renewed for each +attempt: + +| Option | Default | Applies to | +| ------------------- | -------- | ------------------------------------------- | +| `requestTimeoutMs` | `30000` | `getJSON`, `postJSON`, `putJSON`, `putFile` | +| `downloadTimeoutMs` | `600000` | file and thumbnail body transfers | + +They are separate because one number cannot serve both: a value short enough to +keep a hung API call from stalling a backup would cancel a legitimate +multi-gigabyte download. The download deadline covers the body, not just the +headers — `getFileStream` returns as soon as headers arrive, so a deadline that +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 failures that prove no request byte reached the server, +which means the connection was never established (`ECONNREFUSED`, `ENOTFOUND`, +and the like). 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. +`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 +download layer rather than in `ApiClient`, and that is the common failure for +multi-megabyte photos over a CDN. The secretstream pull state is not resumable +and these endpoints have no Range support, so a retry starts the file over. The +atomic write stays outside the retry, so a download that needed three attempts +still performs exactly one write and one rename. `runBackup` and +`runMetadataBackup` are unchanged: the retry sits below them, and a file that +fails after exhausting it is still logged, counted, and stepped over. + +One imprecision is deliberate and worth knowing about. When a body ends part-way +through a secretstream chunk, Poly1305 fails and carries no framing signal, so a +cut connection and genuinely corrupt bytes are indistinguishable. quak reports +that as truncation, which means it is retried. For a body of more than one chunk +the distinction is real — a chunk that failed while the stream carried on past +it stays an authentication failure and is not retried — but for a single-chunk +body, which is most thumbnails and every small file, a wrong key, server-side +corruption and a mid-chunk cutoff all present alike and all get retried. The +cost is bounded by the attempt count, and it buys never silently keeping a +truncated file. + ### Session handling The `Client` class holds the auth token, master key, secret key, and public key @@ -308,7 +391,7 @@ code is non-zero if any files failed. ## TODO -- [ ] Retry policy: no retry on 4xx, exponential backoff on 5xx and network +- [x] Retry policy: no retry on 4xx, exponential backoff on 5xx and network errors - [ ] Update the API reference section below to match the current implementation - [ ] `make docker` green @@ -333,7 +416,10 @@ are correct. The key types and their actual signatures can be found in: - `src/client.ts`: `Client`, `LoginOptions`, `ClientSnapshot` -- `src/api/client.ts`: `ApiClient`, `ApiClientOptions`, `ApiError` +- `src/api/client.ts`: `ApiClient`, `ApiClientOptions`, `ApiError`, + `StreamOptions` +- `src/errors.ts`: `ApiError`, `TruncatedStreamError` +- `src/retry.ts`: `withRetry`, `isRetryable`, `isSafeToReplay`, `RetryOptions` - `src/auth/types.ts`: `KeyAttributes`, `SRPAttributes`, `AuthorizationResponse`, `LoginChallenge` - `src/model/types.ts`: `Collection`, `EnteFile`, `FileMetadata`, `FileBlob`, diff --git a/TODO.md b/TODO.md index f1e961d..2ae41c8 100644 --- a/TODO.md +++ b/TODO.md @@ -14,12 +14,16 @@ pre-1.0 # Next Step -Implement the download retry policy from the README TODO: no retry on 4xx, -exponential backoff on 5xx and network errors. Apply it to file and thumbnail -downloads, cover it with mock-server tests, and update the README TODO checkbox. +Update the README API reference section to match the current implementation. # Completed Steps +- 2026-08-09: Retry policy: no retry on 4xx (except `408` and `429`), + exponential backoff with full jitter on 5xx, transport failures and truncated + transfers, under per-attempt deadlines that cover the response body as well as + the request. Downloads retry request, stream consumption and decryption as one + unit; `postJSON` and `putJSON` are replayed only when the connection was never + established. - 2026-08-09: Downloads verify the secretstream terminated on `TAG_FINAL` and write output atomically: a truncated body is rejected instead of landing on disk as a short file, and plaintext is staged in a sibling temp file and @@ -45,9 +49,6 @@ downloads, cover it with mock-server tests, and update the README TODO checkbox. # Future Steps -- Retry policy: no retry on 4xx, exponential backoff on 5xx and network errors - (the Next Step). -- Update the README API reference section to match the current implementation. - Make `make docker` green. - Tag v1.0.0. - Future desktop client, separate repo: diff --git a/src/api/client.ts b/src/api/client.ts index 32b1ae7..95ad995 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -1,8 +1,33 @@ +import { ApiError } from "../errors.js"; +import { + isSafeToReplay, + resolveRetryOptions, + withRetry, + type ResolvedRetryOptions, + type RetryOptions, +} from "../retry.js"; + +// `ApiError` is defined in `src/errors.ts` so that the retry classifier can +// recognise it without importing this module, which imports the classifier. +// It is re-exported here because this is where callers have always imported it +// from, and it must remain one class: a second copy would make `instanceof` +// fail in the classifier and every 5xx would look permanent. +export { ApiError }; + const DEFAULT_API_ORIGIN = "https://api.ente.io"; const DEFAULT_FILES_ORIGIN = "https://files.ente.io"; const DEFAULT_THUMBS_ORIGIN = "https://thumbnails.ente.io"; const CLIENT_PACKAGE = "berlin.sneak.quak"; +// Two deadlines rather than one, because a single number cannot serve both +// jobs. Thirty seconds is generous for a JSON call and short enough that a +// hung API connection cannot stall a backup for long. A file body is a +// different shape of problem: the deadline has to cover the whole transfer, +// which for a large video on a slow link is minutes, so a value sane for JSON +// would cancel legitimate downloads. +export const DEFAULT_REQUEST_TIMEOUT_MS = 30_000; +export const DEFAULT_DOWNLOAD_TIMEOUT_MS = 600_000; + export interface ApiClientOptions { apiOrigin?: string; filesOrigin?: string; @@ -10,33 +35,78 @@ export interface ApiClientOptions { authToken?: string; fetch?: typeof globalThis.fetch; userAgent?: string; + retry?: RetryOptions; + requestTimeoutMs?: number; + downloadTimeoutMs?: number; } -export class ApiError extends Error { - readonly status: number; - readonly code?: string; - readonly requestID?: string; - readonly body?: unknown; - constructor( - message: string, - status: number, - opts?: { code?: string; requestID?: string; body?: unknown }, - ) { - super(message); - this.name = "ApiError"; - this.status = status; - this.code = opts?.code; - this.requestID = opts?.requestID; - this.body = opts?.body; - } +export interface StreamOptions { + // Opt out of this client's own retry. Exactly one caller wants that: the + // download layer, which retries the request, the stream consumption and + // the decryption as one unit. Leaving both layers enabled would multiply + // the budgets — four attempts each becoming sixteen requests per file. + retry?: boolean; } +// Enforce a deadline over a response body, not merely over its headers. +// +// `getFileStream` returns as soon as headers arrive; the bytes are pulled +// later, in the download layer. Whether the signal passed to `fetch` also +// tears down the body afterwards is up to the fetch implementation, so this +// wrapper makes it a property of quak instead: every read races the signal, +// and an abort errors the stream with the abort reason — which the retry +// classifier recognises. +const deadlineStream = ( + body: ReadableStream, + signal: AbortSignal, +): ReadableStream => { + const reader = body.getReader(); + let rejectOnAbort: (reason: unknown) => void = () => undefined; + const aborted = new Promise((_resolve, reject) => { + rejectOnAbort = reject; + }); + // The abort may fire when nothing is awaiting `aborted` — after the body + // has been read in full, say. Without this, that rejection would surface + // as an unhandled rejection and take the process down. + void aborted.catch(() => undefined); + + const onAbort = (): void => rejectOnAbort(signal.reason); + if (signal.aborted) onAbort(); + else signal.addEventListener("abort", onAbort, { once: true }); + const release = (): void => signal.removeEventListener("abort", onAbort); + + return new ReadableStream({ + async pull(controller) { + try { + const next = await Promise.race([reader.read(), aborted]); + if (next.done) { + release(); + controller.close(); + return; + } + controller.enqueue(next.value); + } catch (err) { + release(); + await reader.cancel(err).catch(() => undefined); + throw err; + } + }, + async cancel(reason) { + release(); + await reader.cancel(reason); + }, + }); +}; + export class ApiClient { private readonly apiOrigin: string; private readonly isCustomOrigin: boolean; private readonly filesOrigin: string; private readonly thumbsOrigin: string; private readonly _fetch: typeof globalThis.fetch; + private readonly retry: ResolvedRetryOptions; + private readonly requestTimeoutMs: number; + private readonly downloadTimeoutMs: number; private token: string | undefined; constructor(opts?: ApiClientOptions) { @@ -53,6 +123,11 @@ export class ApiClient { opts?.thumbsOrigin ?? DEFAULT_THUMBS_ORIGIN ).replace(/\/+$/, ""); this._fetch = opts?.fetch ?? globalThis.fetch; + this.retry = resolveRetryOptions(opts?.retry); + this.requestTimeoutMs = + opts?.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS; + this.downloadTimeoutMs = + opts?.downloadTimeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS; this.token = opts?.authToken; } @@ -64,6 +139,13 @@ export class ApiClient { this.token = undefined; } + // 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. + getRetryOptions(): ResolvedRetryOptions { + return this.retry; + } + private headers(extra?: Record): Record { const h: Record = { "X-Client-Package": CLIENT_PACKAGE, @@ -128,38 +210,53 @@ export class ApiClient { } } } - const resp = await this._fetch(url.href, { - method: "GET", - headers: this.headers(), - }); - await this.throwIfError(resp); - return (await resp.json()) as T; + // A GET changes nothing, so it is retried under the full policy. + return withRetry(async () => { + const resp = await this._fetch(url.href, { + method: "GET", + headers: this.headers(), + signal: AbortSignal.timeout(this.requestTimeoutMs), + }); + await this.throwIfError(resp); + return (await resp.json()) as T; + }, this.retry); } async postJSON(path: string, body: unknown): Promise { const url = `${this.apiOrigin}${path}`; - const resp = await this._fetch(url, { - method: "POST", - headers: this.headers({ "Content-Type": "application/json" }), - body: JSON.stringify(body), - }); - await this.throwIfError(resp); - return (await resp.json()) as T; + // 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 when the failure + // proves the request never reached the server, which in practice means + // the connection was never established. A 5xx, a mid-flight reset and + // a timeout are all left to the caller, because each of them can occur + // after the server has already acted. + return withRetry( + async () => { + const resp = await this._fetch(url, { + method: "POST", + headers: this.headers({ + "Content-Type": "application/json", + }), + body: JSON.stringify(body), + signal: AbortSignal.timeout(this.requestTimeoutMs), + }); + await this.throwIfError(resp); + return (await resp.json()) as T; + }, + { ...this.retry, isRetryable: isSafeToReplay }, + ); } - async getFileStream(fileID: number): Promise> { + async getFileStream( + fileID: number, + opts?: StreamOptions, + ): Promise> { const url = this.isCustomOrigin ? `${this.apiOrigin}/files/download/${fileID}` : `${this.filesOrigin}/?fileID=${fileID}`; - const resp = await this._fetch(url, { - method: "GET", - headers: this.headers(), - }); - await this.throwIfError(resp); - if (!resp.body) { - throw new Error("response body is null"); - } - return resp.body; + return this.streamRequest(url, opts); } async getUploadURL( @@ -173,28 +270,52 @@ export class ApiClient { } async putFile(presignedURL: string, data: Uint8Array): Promise { - const resp = await this._fetch(presignedURL, { - method: "PUT", - headers: { - "Content-Type": "application/octet-stream", - "Content-Length": String(data.length), - }, - body: data, - }); - if (!resp.ok) { - throw new Error(`PUT to presigned URL failed: HTTP ${resp.status}`); - } + // Idempotent despite being a write: a presigned PUT stores one whole + // object at one key in one request, so replaying it either overwrites + // the same bytes or lands them for the first time. There is no partial + // state to protect, hence the full policy rather than the POST rule. + await withRetry(async () => { + const resp = await this._fetch(presignedURL, { + method: "PUT", + headers: { + "Content-Type": "application/octet-stream", + "Content-Length": String(data.length), + }, + body: data, + signal: AbortSignal.timeout(this.requestTimeoutMs), + }); + if (!resp.ok) { + // An ApiError, not a bare Error: without the status on the + // error the upload path cannot be classified at all, and a + // 503 from S3 would be indistinguishable from a bug. + throw new ApiError( + `PUT to presigned URL failed: HTTP ${resp.status}`, + resp.status, + ); + } + }, this.retry); } async putJSON(path: string, body: unknown): Promise { const url = `${this.apiOrigin}${path}`; - const resp = await this._fetch(url, { - method: "PUT", - headers: this.headers({ "Content-Type": "application/json" }), - body: JSON.stringify(body), - }); - await this.throwIfError(resp); - return (await resp.json()) as T; + // Same idempotency rule as `postJSON`, for the same reason: this + // reaches `/files/thumbnail`, which registers an uploaded thumbnail + // against a file. + return withRetry( + async () => { + const resp = await this._fetch(url, { + method: "PUT", + headers: this.headers({ + "Content-Type": "application/json", + }), + body: JSON.stringify(body), + signal: AbortSignal.timeout(this.requestTimeoutMs), + }); + await this.throwIfError(resp); + return (await resp.json()) as T; + }, + { ...this.retry, isRetryable: isSafeToReplay }, + ); } async updateThumbnail( @@ -210,18 +331,36 @@ export class ApiClient { async getThumbnailStream( fileID: number, + opts?: StreamOptions, ): Promise> { const url = this.isCustomOrigin ? `${this.apiOrigin}/files/preview/${fileID}` : `${this.thumbsOrigin}/?fileID=${fileID}`; - const resp = await this._fetch(url, { - method: "GET", - headers: this.headers(), - }); - await this.throwIfError(resp); - if (!resp.body) { - throw new Error("response body is null"); - } - return resp.body; + return this.streamRequest(url, opts); + } + + private async streamRequest( + url: string, + opts?: StreamOptions, + ): Promise> { + const once = async (): Promise> => { + // A fresh deadline per attempt, so a retry gets the whole budget + // rather than the remainder of the one that just expired. + const signal = AbortSignal.timeout(this.downloadTimeoutMs); + const resp = await this._fetch(url, { + method: "GET", + headers: this.headers(), + signal, + }); + await this.throwIfError(resp); + if (!resp.body) { + // Carries the status, and is not retryable: a response that + // arrived without a body is malformed, and asking again + // produces the same malformed response. + throw new ApiError("response body is null", resp.status); + } + return deadlineStream(resp.body, signal); + }; + return opts?.retry === false ? once() : withRetry(once, this.retry); } } diff --git a/src/download/index.ts b/src/download/index.ts index 6e7a6b1..bda3ba7 100644 --- a/src/download/index.ts +++ b/src/download/index.ts @@ -9,6 +9,8 @@ import { STREAM_CHUNK_SIZE, streamTagFinal, } from "../crypto/index.js"; +import { TruncatedStreamError } from "../errors.js"; +import { withRetry } from "../retry.js"; import type { ApiClient } from "../api/client.js"; import type { EnteFile } from "../model/types.js"; @@ -65,7 +67,7 @@ const streamDecrypt = async ( try { pulled = pullStreamChunk(state, buffer); } catch (err) { - throw new Error( + throw new TruncatedStreamError( `download: stream truncated: response body ended with ${buffer.length} trailing bytes that did not authenticate as a final chunk (transfer stopped mid-chunk, or the data is corrupt)`, { cause: err }, ); @@ -85,13 +87,13 @@ const streamDecrypt = async ( // Returning a short plaintext here would put a corrupt file on disk that // later backup runs would treat as complete. if (chunksPulled === 0) { - throw new Error( + throw new TruncatedStreamError( "download: stream truncated: response body contained no secretstream chunks", ); } const tagFinal = streamTagFinal(); if (lastTag !== tagFinal) { - throw new Error( + throw new TruncatedStreamError( `download: stream truncated: last chunk tag ${lastTag}, expected TAG_FINAL (${tagFinal})`, ); } @@ -128,15 +130,49 @@ const writeAtomic = async ( } }; +// Fetch a stream and decrypt it, retrying the whole sequence. +// +// The request is only the first third of a download. `getXStream` returns as +// soon as headers arrive, and the bytes are pulled here, so a socket reset +// mid-body — the dominant failure mode for multi-megabyte photos over a CDN — +// throws in `streamDecrypt` and never reaches `ApiClient` at all. Retrying the +// request alone would miss it entirely. +// +// The client's own retry is therefore switched off for these two calls: with +// both layers active the budgets would multiply, and the library default of +// four attempts would mean sixteen requests for one file. The policy comes +// from the client so a caller that configured one gets it here too. +// +// A retry starts the file over from byte zero: the secretstream pull state is +// not resumable and there is no Range support on these endpoints. +const fetchAndDecrypt = async ( + api: ApiClient, + openStream: () => Promise>, + header: Uint8Array, + key: Uint8Array, +): Promise => + withRetry(async () => { + const stream = await openStream(); + return streamDecrypt(stream, header, key); + }, api.getRetryOptions()); + export const downloadFile = async ( api: ApiClient, file: EnteFile, outPath?: string, ): Promise => { const resolvedPath = outPath ?? file.metadata.title; - const stream = await api.getFileStream(file.id); const header = fromBase64(file.file.decryptionHeader); - const plaintext = await streamDecrypt(stream, header, file.key); + const plaintext = await fetchAndDecrypt( + api, + () => api.getFileStream(file.id, { retry: false }), + header, + file.key, + ); + // Outside the retry, deliberately: only the attempt that produced a + // complete, authenticated plaintext gets to stage a temporary file, so a + // download that needed three tries still performs exactly one write and + // one rename. await writeAtomic(resolvedPath, plaintext); return { path: resolvedPath, bytesWritten: plaintext.length }; }; @@ -147,9 +183,13 @@ export const downloadThumbnail = async ( outPath?: string, ): Promise => { const resolvedPath = outPath ?? `thumb_${file.metadata.title}`; - const stream = await api.getThumbnailStream(file.id); const header = fromBase64(file.thumbnail.decryptionHeader); - const plaintext = await streamDecrypt(stream, header, file.key); + const plaintext = await fetchAndDecrypt( + api, + () => api.getThumbnailStream(file.id, { retry: false }), + header, + file.key, + ); await writeAtomic(resolvedPath, plaintext); return { path: resolvedPath, bytesWritten: plaintext.length }; }; diff --git a/src/errors.ts b/src/errors.ts new file mode 100644 index 0000000..e001092 --- /dev/null +++ b/src/errors.ts @@ -0,0 +1,45 @@ +// Error types that more than one layer of quak needs to recognise. +// +// They live here rather than beside the code that throws them so that the +// retry classifier can identify them without importing the HTTP client or the +// download layer — both of which import the classifier. `ApiError` is +// re-exported from `src/api/client.ts`, which is where callers have always +// imported it from and where it still belongs conceptually. + +export class ApiError extends Error { + readonly status: number; + readonly code?: string; + readonly requestID?: string; + readonly body?: unknown; + constructor( + message: string, + status: number, + opts?: { code?: string; requestID?: string; body?: unknown }, + ) { + super(message); + this.name = "ApiError"; + this.status = status; + this.code = opts?.code; + this.requestID = opts?.requestID; + this.body = opts?.body; + } +} + +// A response body that ended before the secretstream did. +// +// This is a type rather than a message prefix because it is a decision, not a +// diagnostic: the retry policy asks "was this a short transfer?" and acts on +// the answer. Matching on the wording of an error message would make the next +// person to reword a diagnostic silently turn every truncated download into a +// permanent failure, and the failure would look like a corrupt file rather +// than like a bug. +// +// `cause` carries the underlying authentication failure on the one path where +// there is one — a body that stopped part-way through a chunk, which Poly1305 +// cannot distinguish from corruption. +export class TruncatedStreamError extends Error { + constructor(message: string, opts?: ErrorOptions) { + super(message, opts); + this.name = "TruncatedStreamError"; + } +} diff --git a/src/index.ts b/src/index.ts index d1ae701..c53ceb9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,7 +1,25 @@ export const VERSION = "0.0.0"; export { Client, type LoginOptions, type ClientSnapshot } from "./client.js"; -export { ApiClient, ApiError, type ApiClientOptions } from "./api/client.js"; +export { + ApiClient, + ApiError, + DEFAULT_DOWNLOAD_TIMEOUT_MS, + DEFAULT_REQUEST_TIMEOUT_MS, + type ApiClientOptions, + type StreamOptions, +} from "./api/client.js"; +export { TruncatedStreamError } from "./errors.js"; +export { + DEFAULT_RETRY_OPTIONS, + isRetryable, + isSafeToReplay, + resolveRetryOptions, + withRetry, + type ResolvedRetryOptions, + type RetryOptions, + type WithRetryOptions, +} from "./retry.js"; export { unwrapAuth, type UnwrapResult } from "./auth/unwrap.js"; export { beginLogin, diff --git a/src/retry.ts b/src/retry.ts new file mode 100644 index 0000000..c425de8 --- /dev/null +++ b/src/retry.ts @@ -0,0 +1,193 @@ +// The retry policy shared by every network operation in quak. +// +// Two independent pieces: a classifier that decides whether an error is worth +// another attempt, and a loop that acts on that decision with exponential +// backoff. Keeping them apart is what lets the non-idempotent call sites reuse +// the loop under a stricter question (see `isSafeToReplay`). +// +// The classifier's default answer is no. For a backup tool, retrying a +// permanent failure spends round trips and delays every remaining file, while +// declining to retry a transient one costs a single file that the next run +// picks up anyway. When the evidence is ambiguous, fail fast. + +import { ApiError, TruncatedStreamError } from "./errors.js"; + +export interface RetryOptions { + // Total calls, not retries: `attempts: 1` disables retrying. + attempts?: number; + // Ceiling for the first retry's delay; doubles with each retry. + baseDelayMs?: number; + // Upper bound on that ceiling, so a long outage settles into a steady + // poll instead of growing without limit. + maxDelayMs?: number; + // Injected so tests exercise the whole policy without waiting. + sleep?: (ms: number) => Promise; + // Injected so the jitter is reproducible under test. + random?: () => number; +} + +export type ResolvedRetryOptions = Required; + +export const DEFAULT_RETRY_OPTIONS: ResolvedRetryOptions = { + attempts: 4, + baseDelayMs: 500, + maxDelayMs: 10_000, + sleep: (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)), + random: Math.random, +}; + +export const resolveRetryOptions = ( + opts?: RetryOptions, +): ResolvedRetryOptions => ({ + attempts: opts?.attempts ?? DEFAULT_RETRY_OPTIONS.attempts, + baseDelayMs: opts?.baseDelayMs ?? DEFAULT_RETRY_OPTIONS.baseDelayMs, + maxDelayMs: opts?.maxDelayMs ?? DEFAULT_RETRY_OPTIONS.maxDelayMs, + sleep: opts?.sleep ?? DEFAULT_RETRY_OPTIONS.sleep, + random: opts?.random ?? DEFAULT_RETRY_OPTIONS.random, +}); + +// Transport failures: the request did not complete, for reasons below HTTP. +const TRANSPORT_CODES = new Set([ + "ECONNRESET", + "ECONNABORTED", + "ETIMEDOUT", + "EPIPE", + "ENOTFOUND", + "EAI_AGAIN", + "ECONNREFUSED", + "EHOSTUNREACH", + "ENETUNREACH", + "ENETRESET", + "ENETDOWN", +]); + +// The subset of the above that can only happen before any request byte was +// written: name resolution failed, or the connection was refused or never +// routed. See `isSafeToReplay`. +const CONNECT_CODES = new Set([ + "ENOTFOUND", + "EAI_AGAIN", + "ECONNREFUSED", + "EHOSTUNREACH", + "ENETUNREACH", + "ENETDOWN", +]); + +// `cause` is an arbitrary user-settable property and nothing prevents it from +// forming a cycle, so the walk is bounded. Hanging the process would be a +// worse outcome than any misclassification. +const MAX_CAUSE_DEPTH = 8; + +// Collect every `code` in an error's cause chain. undici does not put the +// 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[] => { + const codes: string[] = []; + let current: unknown = err; + for (let depth = 0; depth < MAX_CAUSE_DEPTH; depth++) { + if (current === null || typeof current !== "object") break; + const { code, cause } = current as { code?: unknown; cause?: unknown }; + if (typeof code === "string") codes.push(code); + if (cause === current) break; + current = cause; + } + return codes; +}; + +const isAbort = (err: unknown): boolean => { + if (err === null || typeof err !== "object") return false; + const { name } = err as { name?: unknown }; + return name === "AbortError" || name === "TimeoutError"; +}; + +// Is another attempt capable of producing a different answer? +// +// A note on the truncation case, recorded on issue #2. `TruncatedStreamError` +// is retried, and for a body of more than one chunk that is exactly right: a +// chunk that failed to authenticate while the stream carried on past it is +// corruption, stays an ordinary authentication failure, and is not retried. +// +// For a single-chunk body — most thumbnails, every small file — the split is +// not achievable. A wrong file key, server-side corruption and a connection +// cut mid-chunk are cryptographically identical: Poly1305 fails and carries no +// framing signal. All three are reported as truncation and therefore retried. +// That imprecision is deliberate and bounded by the attempt count: one wasted +// round trip on a genuinely corrupt file is a fair price for never silently +// keeping a truncated one, and the alternative — treating a single-chunk +// authentication failure as permanent — would reintroduce exactly the +// silent-corruption risk the truncation check exists to remove. +export const isRetryable = (err: unknown): boolean => { + if (err instanceof ApiError) { + // 408 and 429 are the two 4xx codes that are statements about timing + // rather than about the request, and backoff is the right answer to + // both. Every other 4xx will answer the same way however often it is + // asked. + if (err.status === 408 || err.status === 429) return true; + return err.status >= 500 && err.status <= 599; + } + if (err instanceof TruncatedStreamError) return true; + // quak only ever aborts a request on its own deadline, so an abort means + // this attempt ran out of time — which a later one may not. + if (isAbort(err)) return true; + // Node's fetch rejects with `TypeError: fetch failed` for everything below + // HTTP: DNS failure, refused connection, TLS error, reset socket. Nothing + // on the object separates it from a TypeError thrown by a bug, so this is + // deliberately literal. Demanding a recognised `cause` instead would + // classify real network failures as permanent and fail backups that should + // 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)); +}; + +// 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 when the failure +// proves no request byte reached the server, which means the connection was +// never established. +// +// 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 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)); + +export interface WithRetryOptions extends RetryOptions { + isRetryable?: (err: unknown) => boolean; +} + +// Exponential backoff with full jitter: the exponential term is the ceiling, +// and the actual wait is drawn uniformly below it. Full jitter, rather than a +// fixed delay plus noise, is what stops a client that lost a hundred parallel +// downloads to one CDN blip from re-sending all hundred at the same instant. +const backoffMs = (retryNumber: number, policy: ResolvedRetryOptions): number => + policy.random() * + Math.min(policy.maxDelayMs, policy.baseDelayMs * 2 ** (retryNumber - 1)); + +export const withRetry = async ( + fn: () => Promise, + opts?: WithRetryOptions, +): Promise => { + const policy = resolveRetryOptions(opts); + const retryable = opts?.isRetryable ?? isRetryable; + for (let attempt = 1; ; attempt++) { + try { + return await fn(); + } catch (err) { + if (attempt >= policy.attempts || !retryable(err)) { + // The error escapes unwrapped, and it is the one from the + // final attempt: callers classify what they catch, and + // `listMissingThumbnails` in particular needs the `ApiError` + // and its status intact. + throw err; + } + await policy.sleep(backoffMs(attempt, policy)); + } + } +}; diff --git a/src/thumbnails.ts b/src/thumbnails.ts index 4176f5e..254f53d 100644 --- a/src/thumbnails.ts +++ b/src/thumbnails.ts @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { readFileSync } from "node:fs"; import * as jpeg from "jpeg-js"; import type { Client } from "./client.js"; +import { ApiError } from "./api/client.js"; import { encryptBlob, toBase64 } from "./crypto/index.js"; import { downloadFile } from "./download/index.js"; import type { EnteFile } from "./model/types.js"; @@ -62,13 +63,31 @@ export const listMissingThumbnails = async ( reason: "empty thumbnail (0 bytes)", }); } - } catch { - missing.push({ - fileID: file.id, - title: file.metadata.title, - collection: col.name, - reason: "thumbnail fetch failed", - }); + } catch (err) { + // A 404 is the server stating the thumbnail is not there: + // that, and an empty body, are the only two answers that mean + // "missing". Anything else reaching this point is a failure + // that already exhausted its retries — a failing server, a + // dropped connection, a deadline — and says nothing about + // whether the thumbnail exists. + // + // The distinction is what stops `helper + // fix-missing-thumbnails` from downloading originals, + // regenerating thumbnails and uploading them over thumbnails + // that were fine all along, because the CDN was briefly + // returning 500s while this ran. + if (err instanceof ApiError && err.status === 404) { + missing.push({ + fileID: file.id, + title: file.metadata.title, + collection: col.name, + reason: "thumbnail not found (HTTP 404)", + }); + } else { + log( + `[${col.name}] Could not check ${file.metadata.title}: ${err instanceof Error ? err.message : String(err)} (not reported as missing)`, + ); + } } } } diff --git a/test/retry/retry.test.ts b/test/retry/retry.test.ts index 42eefed..7ddd24b 100644 --- a/test/retry/retry.test.ts +++ b/test/retry/retry.test.ts @@ -522,11 +522,13 @@ describe("withRetry backoff", () => { describe("retry defaults", () => { it("ships a bounded, documented default policy", () => { // These are the numbers the README documents. They are asserted here - // so the README and the code cannot drift apart silently: four - // attempts, half a second of base delay, ten seconds of ceiling — - // under 15 seconds of waiting in the worst case, which keeps a + // so the README and the code cannot drift apart silently. Four + // attempts at a 500ms base put the three ceilings at 500, 1000 and + // 2000ms, so a file that is going to fail gives up after at most + // three and a half seconds of waiting — which keeps a // several-thousand-file backup moving past a bad file rather than - // stalling on it. + // stalling on it. The 10s cap only comes into play for a caller that + // raises the attempt count. expect(DEFAULT_RETRY_OPTIONS.attempts).toBe(4); expect(DEFAULT_RETRY_OPTIONS.baseDelayMs).toBe(500); expect(DEFAULT_RETRY_OPTIONS.maxDelayMs).toBe(10_000); -- 2.49.1 From 348f23bac9196a838d56ed5037eaea6eaef5d47c Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 9 Aug 2026 05:41:59 +0000 Subject: [PATCH 3/3] Narrow the replay errno set to failures that prove no connection existed `CONNECT_CODES` drives `isSafeToReplay`, which is the only thing standing between a transport failure and a replayed `POST /users/two-factor/verify`. It included `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` on the stated grounds that those errnos can only be reported before any request byte was written. That is not true on Linux: an ICMP destination-unreachable delivered on an already-established connection sets the socket error and the next read or write returns `EHOSTUNREACH` or `ENETUNREACH`, and a local interface going down after the request was fully written surfaces as `ENETDOWN` the same way. In each case the server may already have received and acted on the request -- exactly the ambiguity the rule exists to exclude, on the paths that consume a second-factor attempt or register a thumbnail. The three are dropped from `CONNECT_CODES` and stay in `TRANSPORT_CODES`, so they remain retryable for the idempotent calls; only replay eligibility narrows. What is left -- `ENOTFOUND`, `EAI_AGAIN`, `ECONNREFUSED` -- means no TCP connection to the server ever existed, so no request byte can have been transmitted. The justification is corrected everywhere it was stated: the comment on `CONNECT_CODES`, the one on `isSafeToReplay`, the `postJSON` call site, the README's idempotency section and the `client.test.ts` docblock. All of them now describe what the narrowed set actually establishes rather than claiming a proof it did not support. The narrowing is enforced by the suite rather than asserted in a comment: the three errnos join `ECONNRESET`/`EPIPE`/`ETIMEDOUT` in the `isSafeToReplay`-returns-false test, with companion `isRetryable` assertions so a future edit cannot make them non-retryable by accident. Putting the three back into `CONNECT_CODES` turns that test red (1 failure, verified). --- README.md | 18 ++++++++++++------ src/api/client.ts | 11 ++++++----- src/retry.ts | 38 +++++++++++++++++++++++--------------- test/api/client.test.ts | 9 +++++---- test/retry/retry.test.ts | 31 +++++++++++++++++++++---------- 5 files changed, 67 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 6145269..34ee16e 100644 --- a/README.md +++ b/README.md @@ -305,12 +305,18 @@ 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 failures that prove no request byte reached the server, -which means the connection was never established (`ECONNREFUSED`, `ENOTFOUND`, -and the like). 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. -`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. +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. 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/src/api/client.ts b/src/api/client.ts index 95ad995..f3b9996 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -227,11 +227,12 @@ export class ApiClient { // 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 when the failure - // proves the request never reached the server, which in practice means - // the connection was never established. A 5xx, a mid-flight reset and - // a timeout are all left to the caller, because each of them can occur - // after the server has already acted. + // 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. return withRetry( async () => { const resp = await this._fetch(url, { diff --git a/src/retry.ts b/src/retry.ts index c425de8..4d5eb69 100644 --- a/src/retry.ts +++ b/src/retry.ts @@ -62,17 +62,22 @@ const TRANSPORT_CODES = new Set([ "ENETDOWN", ]); -// The subset of the above that can only happen before any request byte was -// written: name resolution failed, or the connection was refused or never -// routed. See `isSafeToReplay`. -const CONNECT_CODES = new Set([ - "ENOTFOUND", - "EAI_AGAIN", - "ECONNREFUSED", - "EHOSTUNREACH", - "ENETUNREACH", - "ENETDOWN", -]); +// The subset of the above that can only be reported before a TCP connection +// exists, and therefore before any request byte could have been written: name +// resolution produced no address (`ENOTFOUND`, `EAI_AGAIN`) or the peer +// refused the connection with an RST to the SYN (`ECONNREFUSED`). +// +// The routing errnos — `EHOSTUNREACH`, `ENETUNREACH`, `ENETDOWN` — are +// deliberately absent even though they look like connect-time failures. On +// Linux they are also delivered on an already-established socket: an ICMP +// destination-unreachable arriving mid-flight sets the socket error and the +// next read or write returns it, and a local interface going down after the +// request was fully written surfaces the same way. In those cases the server +// may already have received and acted on the request, which is exactly the +// ambiguity this set exists to exclude. They stay in `TRANSPORT_CODES`, so +// they remain retryable for idempotent calls; only replay eligibility is +// narrowed. See `isSafeToReplay`. +const CONNECT_CODES = new Set(["ENOTFOUND", "EAI_AGAIN", "ECONNREFUSED"]); // `cause` is an arbitrary user-settable property and nothing prevents it from // forming a cycle, so the walk is bounded. Hanging the process would be a @@ -148,13 +153,16 @@ export const isRetryable = (err: unknown): boolean => { // `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 when the failure -// proves no request byte reached the server, which means the connection was -// never established. +// attempts — and `/files/thumbnail`. 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 +// server cannot have acted. // // 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 deadline says nothing at all about the server's state. +// 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)); diff --git a/test/api/client.test.ts b/test/api/client.test.ts index 387da4f..2faf019 100644 --- a/test/api/client.test.ts +++ b/test/api/client.test.ts @@ -824,10 +824,11 @@ describe("ApiClient non-idempotent requests", () => { * state: `/users/srp/create-session`, `/users/two-factor/verify` — which * consumes one of a small number of 2FA attempts — and `/files/thumbnail`. * - * They are retried only when the failure proves the request never reached - * the server, which in practice means the connection was never - * established. Everything else is ambiguous: a 5xx proves the server did - * process the request, and a reset or a timeout can arrive after it did. + * 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 + * the connection — so no request byte can have been transmitted. + * Everything else is ambiguous: a 5xx proves the server did process the + * request, and a reset or a timeout can arrive after it did. * Replaying under that ambiguity can burn a 2FA attempt or register a * thumbnail twice, and neither is worth the round trip it saves. */ diff --git a/test/retry/retry.test.ts b/test/retry/retry.test.ts index 7ddd24b..7baf8c0 100644 --- a/test/retry/retry.test.ts +++ b/test/retry/retry.test.ts @@ -282,18 +282,13 @@ describe("isSafeToReplay", () => { * 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 failures that prove no request - * byte ever reached the server — which means the connection was never - * established. + * 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. */ it("replays only failures where the connection was never established", () => { - for (const code of [ - "ENOTFOUND", - "EAI_AGAIN", - "ECONNREFUSED", - "EHOSTUNREACH", - "ENETUNREACH", - ]) { + for (const code of ["ENOTFOUND", "EAI_AGAIN", "ECONNREFUSED"]) { expect(isSafeToReplay(errnoError(code))).toBe(true); } // Also when undici has buried it, which is how it actually arrives. @@ -317,6 +312,22 @@ describe("isSafeToReplay", () => { expect(isSafeToReplay(errnoError("ECONNRESET"))).toBe(false); expect(isSafeToReplay(errnoError("EPIPE"))).toBe(false); expect(isSafeToReplay(errnoError("ETIMEDOUT"))).toBe(false); + // The routing errnos look like connect-time failures but are not. On + // Linux an ICMP destination-unreachable delivered on an established + // connection sets the socket error, and the next read or write returns + // `EHOSTUNREACH` or `ENETUNREACH`; a local interface going down after + // the request was fully written surfaces as `ENETDOWN` the same way. + // In each case the server may already have consumed the request — a + // replayed `/users/two-factor/verify` would burn a second attempt. + // They remain retryable for the idempotent calls; this asserts only + // that they are not replayable. + expect(isSafeToReplay(errnoError("EHOSTUNREACH"))).toBe(false); + expect(isSafeToReplay(errnoError("ENETUNREACH"))).toBe(false); + expect(isSafeToReplay(errnoError("ENETDOWN"))).toBe(false); + // ...and that the narrowing did not make them non-retryable. + expect(isRetryable(errnoError("EHOSTUNREACH"))).toBe(true); + expect(isRetryable(errnoError("ENETUNREACH"))).toBe(true); + expect(isRetryable(errnoError("ENETDOWN"))).toBe(true); expect( isSafeToReplay(new DOMException("timed out", "TimeoutError")), ).toBe(false); -- 2.49.1