Harden the retry classifier and pin per-attempt deadlines (closes #80)

A POST or PUT is replayed only when every errno in the cause chain is a
connect errno and the walk reached the end of the chain, and
postJSON/putJSON no longer follow redirects, so a redirect is an
ApiError that is not retried. getRetryOptions() returns a copy. New
tests pin every errno the classifier names, the cause-chain depth
limit, a chain deeper than the limit, a two-error cycle, and a fresh
deadline per attempt for every retrying entry point. The README
endpoint list is now the one place naming the requests the replay rule
covers; code comments point to it.

Model: opus-5-5

README.md

@@ -323,6 +323,7 @@ Endpoints used:
 - `GET /collections/v2/diff?collectionID=<id>&sinceTime=<usec>`: list files in a
   collection; paginate while `hasMore` is true.
 - `GET https://files.ente.io/?fileID=<id>`: download encrypted file bytes.
+- `POST /files/data/fetch`: fetch encrypted ML data for a batch of files.
 - `POST /files/upload-url`: mint a presigned upload URL (for thumbnail repair).
 - `PUT /files/thumbnail`: register an uploaded thumbnail's object key.
 
@@ -377,20 +378,23 @@ headers — `getFileStream` returns as soon as headers arrive, so a deadline tha
 only guarded the initial request would leave the same hang one layer down.
 
 **Non-idempotent requests are not blindly replayed.** `postJSON` and `putJSON`
-reach `/users/srp/create-session`, `/users/two-factor/verify` — which consumes
-one of a small number of second-factor attempts — and `/files/thumbnail`. They
-are retried only on the three failures that establish no TCP connection to the
-server ever existed, so no request byte can have been transmitted: `ENOTFOUND`
-and `EAI_AGAIN` (name resolution produced no address) and `ECONNREFUSED` (the
-peer refused the connection). A 5xx, a mid-flight reset and a deadline are all
-left to the caller, because each of them can happen after the server has already
-acted. The routing errnos `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` are
-excluded for the same reason, despite looking like connect-time failures: on
-Linux an ICMP unreachable arriving mid-flight, or a local interface going down
-after the request was written, delivers them on an already-established socket.
-They stay retryable for the idempotent calls. `putFile` is exempt: a presigned
-PUT stores one whole object at one key in one request, so replaying it has no
-partial state to damage.
+send every `POST` and `PUT` in the endpoint list above; some of them change
+server state, and `/users/two-factor/verify` consumes one of a small number of
+second-factor attempts. They are retried only when every errno in the error's
+`cause` chain is one of the three that establish no TCP connection to the server
+ever existed, so no request byte can have been transmitted: `ENOTFOUND` and
+`EAI_AGAIN` (name resolution produced no address) and `ECONNREFUSED` (the peer
+refused the connection). A 5xx, a mid-flight reset and a deadline are all left
+to the caller, because each of them can happen after the server has already
+acted. These two do not follow redirects either: a redirect means the server
+already received the request, so it is reported as an error and not retried. The
+routing errnos `EHOSTUNREACH`, `ENETUNREACH` and `ENETDOWN` are excluded for the
+same reason, despite looking like connect-time failures: on Linux an ICMP
+unreachable arriving mid-flight, or a local interface going down after the
+request was written, delivers them on an already-established socket. They stay
+retryable for the idempotent calls. `putFile` is exempt: a presigned PUT stores
+one whole object at one key in one request, so replaying it has no partial state
+to damage.
 
 A download is retried as a whole — request, stream consumption, and decryption —
 because a socket reset after the response headers have arrived surfaces in the

TODO.md

@@ -18,6 +18,19 @@ Tag v1.0.0.
 
 # Completed Steps
 
+- 2026-09-22: Hardened the retry classifier (issue 80). A `POST` or `PUT` is
+  replayed only when every errno in the cause chain is a connect errno, and it
+  no longer follows redirects. `getRetryOptions()` returns a copy. Tests pin
+  every errno the classifier names, the cause-chain depth limit, cycle
+  termination, and a fresh deadline per attempt for every retrying entry point.
+  The README's endpoint list is the one place that names the requests the replay
+  rule covers.
+- 2026-09-22: Stopped `make test` collecting tests from checkouts nested under
+  `.claude/` (issue 25). vitest ignores `.gitignore` when finding tests, so a
+  nested checkout ran the whole suite again; `vitest.config.ts` now adds
+  `.claude/**` to vitest's default excludes, and
+  `test/packaging/nested-checkout.test.ts` plants a nested checkout in a temp
+  directory and fails if vitest would collect it.
 - 2026-09-22: Dropped the deprecated `@types/libsodium-wrappers-sumo` stub from
   `devDependencies` (issue 27). It shipped no declarations; the types come from
   `libsodium-wrappers-sumo` itself. `yarn.lock` regenerated by `yarn remove`.

src/api/client.ts

@@ -146,8 +146,9 @@ export class ApiClient {
     // The policy this client was configured with, so that a caller wrapping a
     // whole operation in its own `withRetry` — the download layer — runs under
     // the same settings rather than under the library defaults.
+    // A copy, so the caller cannot change this client's settings through it.
     getRetryOptions(): ResolvedRetryOptions {
-        return this.retry;
+        return { ...this.retry };
     }
 
     private headers(extra?: Record<string, string>): Record<string, string> {
@@ -228,15 +229,15 @@ export class ApiClient {
 
     async postJSON<T>(path: string, body: unknown): Promise<T> {
         const url = `${this.apiOrigin}${path}`;
-        // Idempotency: this reaches `/users/srp/create-session`,
-        // `/users/two-factor/verify` and `/users/ott`, all of which change
-        // server state — verifying a second factor consumes one of a small
-        // number of attempts. So a POST is replayed only on a failure that
-        // establishes no TCP connection to the server ever existed: DNS
-        // produced no address, or the peer refused the connection. A 5xx, a
-        // mid-flight reset, a routing errno (which Linux also delivers on an
-        // established socket) and a timeout are all left to the caller,
-        // because each of them can occur after the server has already acted.
+        // Not idempotent: a POST is replayed only when `isSafeToReplay`
+        // says no request byte can have reached the server. The endpoints
+        // this covers are listed in the README under "Endpoints used".
+        //
+        // Redirects are not followed. The origin has already received the
+        // request when it answers with one, so a connection refused by the
+        // redirect target would look replay-safe when it is not. The API has
+        // no legitimate redirect, so one surfaces as an `ApiError` with its
+        // 3xx status, which is not retried.
         return withRetry(
             async () => {
                 const resp = await this._fetch(url, {
@@ -245,6 +246,7 @@ export class ApiClient {
                         "Content-Type": "application/json",
                     }),
                     body: JSON.stringify(body),
+                    redirect: "manual",
                     signal: AbortSignal.timeout(this.requestTimeoutMs),
                 });
                 await this.throwIfError(resp);
@@ -303,9 +305,7 @@ export class ApiClient {
 
     async putJSON<T>(path: string, body: unknown): Promise<T> {
         const url = `${this.apiOrigin}${path}`;
-        // Same idempotency rule as `postJSON`, for the same reason: this
-        // reaches `/files/thumbnail`, which registers an uploaded thumbnail
-        // against a file.
+        // Same replay and redirect rules as `postJSON`, for the same reasons.
         return withRetry(
             async () => {
                 const resp = await this._fetch(url, {
@@ -314,6 +314,7 @@ export class ApiClient {
                         "Content-Type": "application/json",
                     }),
                     body: JSON.stringify(body),
+                    redirect: "manual",
                     signal: AbortSignal.timeout(this.requestTimeoutMs),
                 });
                 await this.throwIfError(resp);

src/retry.ts

@@ -88,18 +88,21 @@ const MAX_CAUSE_DEPTH = 8;
 // errno on the error it throws — it hangs the underlying socket error off
 // `cause`, sometimes more than one level down — so a classifier that only read
 // the top-level error would see a bare `Error` and call every dropped
-// connection permanent.
-const causeCodes = (err: unknown): string[] => {
+// connection permanent. `complete` is false when the walk stopped at the
+// depth limit with more of the chain still below it.
+const causeCodes = (err: unknown): { codes: string[]; complete: boolean } => {
     const codes: string[] = [];
     let current: unknown = err;
     for (let depth = 0; depth < MAX_CAUSE_DEPTH; depth++) {
-        if (current === null || typeof current !== "object") break;
+        if (current === null || typeof current !== "object") {
+            return { codes, complete: true };
+        }
         const { code, cause } = current as { code?: unknown; cause?: unknown };
         if (typeof code === "string") codes.push(code);
-        if (cause === current) break;
+        if (cause === current) return { codes, complete: true };
         current = cause;
     }
-    return codes;
+    return { codes, complete: current === null || typeof current !== "object" };
 };
 
 const isAbort = (err: unknown): boolean => {
@@ -145,15 +148,15 @@ export const isRetryable = (err: unknown): boolean => {
     // have succeeded; the cost of the imprecision is bounded by the attempt
     // count.
     if (err instanceof TypeError) return true;
-    return causeCodes(err).some((code) => TRANSPORT_CODES.has(code));
+    return causeCodes(err).codes.some((code) => TRANSPORT_CODES.has(code));
 };
 
 // Could the first attempt already have taken effect on the server?
 //
 // `isRetryable` is the wrong question for a request that changes state.
-// quak's non-idempotent calls are `/users/srp/create-session`,
-// `/users/two-factor/verify` — which consumes one of a small number of 2FA
-// attempts — and `/files/thumbnail`. They are replayed only on the failures in
+// `postJSON` and `putJSON` use this for every `POST` and `PUT` listed in the
+// README under "Endpoints used"; verifying a second factor, for one, consumes
+// one of a small number of attempts. They are replayed only on the failures in
 // `CONNECT_CODES`, which establish that no TCP connection to the server ever
 // existed: there was no address to connect to, or the peer refused the
 // connection outright. A request byte cannot have been transmitted, so the
@@ -162,9 +165,19 @@ export const isRetryable = (err: unknown): boolean => {
 // Everything else is ambiguous. A 5xx proves the server did process the
 // request. A reset or a broken pipe can arrive after it was fully sent and
 // acted on. A routing errno can be delivered on an established socket. A
-// deadline says nothing at all about the server's state.
-export const isSafeToReplay = (err: unknown): boolean =>
-    isRetryable(err) && causeCodes(err).some((code) => CONNECT_CODES.has(code));
+// deadline says nothing at all about the server's state. So every errno in the
+// cause chain must be a connect errno: one other errno anywhere in the chain
+// is doubt, and doubt is not replayed. A chain longer than the walk is doubt
+// too: the links below the limit were never read.
+export const isSafeToReplay = (err: unknown): boolean => {
+    const { codes, complete } = causeCodes(err);
+    return (
+        isRetryable(err) &&
+        complete &&
+        codes.length > 0 &&
+        codes.every((code) => CONNECT_CODES.has(code))
+    );
+};
 
 export interface WithRetryOptions extends RetryOptions {
     isRetryable?: (err: unknown) => boolean;

test/api/client.test.ts

@@ -639,6 +639,24 @@ describe("ApiClient retries", () => {
         expect(policy.baseDelayMs).toBe(7);
         expect(policy.maxDelayMs).toBe(11);
     });
+
+    it("does not let a caller change its settings through that policy", async () => {
+        const { fetch, calls } = scriptedFetch(
+            textResponse("boom", 500),
+            textResponse("boom", 500),
+            textResponse("boom", 500),
+        );
+        const client = new ApiClient({
+            fetch,
+            retry: { ...noWait, attempts: 2 },
+        });
+
+        client.getRetryOptions().attempts = 3;
+
+        expect(client.getRetryOptions().attempts).toBe(2);
+        await expect(client.getJSON("/x")).rejects.toBeInstanceOf(ApiError);
+        expect(calls).toHaveLength(2);
+    });
 });
 
 describe("ApiClient timeouts", () => {
@@ -693,6 +711,51 @@ describe("ApiClient timeouts", () => {
         expect(new Set(signals).size).toBe(3);
     }, 5000);
 
+    it("gives every retrying entry point a fresh deadline per attempt", async () => {
+        // A refused connection is retried by every entry point, the
+        // non-idempotent ones included. If the deadline were created once,
+        // outside the retry, both attempts would carry the same signal.
+        const entryPoints: [
+            string,
+            () => Response,
+            (c: ApiClient) => unknown,
+        ][] = [
+            ["getJSON", () => jsonResponse({}), (c) => c.getJSON("/a")],
+            ["postJSON", () => jsonResponse({}), (c) => c.postJSON("/b", {})],
+            ["putJSON", () => jsonResponse({}), (c) => c.putJSON("/c", {})],
+            [
+                "putFile",
+                () => new Response(null, { status: 200 }),
+                (c) => c.putFile("https://s3.example/x", new Uint8Array([1])),
+            ],
+            [
+                "getFileStream",
+                () => streamResponse(new Uint8Array([1])),
+                (c) => c.getFileStream(1),
+            ],
+            [
+                "getThumbnailStream",
+                () => streamResponse(new Uint8Array([1])),
+                (c) => c.getThumbnailStream(1),
+            ],
+        ];
+        for (const [name, success, call] of entryPoints) {
+            const { fetch, calls } = scriptedFetch(
+                errnoError("ECONNREFUSED", "connect ECONNREFUSED"),
+                success(),
+            );
+            const client = new ApiClient({ fetch, retry: noWait });
+
+            await call(client);
+
+            expect(calls, name).toHaveLength(2);
+            const [first, second] = calls.map((c) => c.init?.signal);
+            expect(first, name).toBeInstanceOf(AbortSignal);
+            expect(second, name).toBeInstanceOf(AbortSignal);
+            expect(second, name).not.toBe(first);
+        }
+    });
+
     it("recovers when a later attempt answers in time", async () => {
         const { fetch, calls } = scriptedFetch(HANG, jsonResponse({ ok: 1 }));
         const client = new ApiClient({
@@ -820,9 +883,8 @@ describe("ApiClient error typing", () => {
 
 describe("ApiClient non-idempotent requests", () => {
     /**
-     * `postJSON` and `putJSON` carry quak's only requests that change server
-     * state: `/users/srp/create-session`, `/users/two-factor/verify` — which
-     * consumes one of a small number of 2FA attempts — and `/files/thumbnail`.
+     * `postJSON` and `putJSON` carry quak's requests that can change server
+     * state; the README lists them under "Endpoints used".
      *
      * They are retried only on a failure that establishes no TCP connection to
      * the server ever existed — DNS produced no address, or the peer refused
@@ -922,4 +984,30 @@ describe("ApiClient non-idempotent requests", () => {
         await refusedClient.updateThumbnail(1, "key", "header");
         expect(refused.calls).toHaveLength(2);
     });
+
+    it("does not follow or replay a redirect on POST or PUT", async () => {
+        // The origin has already received a request it answers with a
+        // redirect, so following it would let a refused connection to the
+        // redirect target pass for a request that never went out.
+        for (const send of [
+            (c: ApiClient) => c.postJSON("/users/ott", {}),
+            (c: ApiClient) => c.putJSON("/files/thumbnail", {}),
+        ]) {
+            const { fetch, calls } = scriptedFetch(
+                new Response(null, {
+                    status: 307,
+                    headers: { location: "https://elsewhere.example/" },
+                }),
+                jsonResponse({}),
+            );
+            const client = new ApiClient({ fetch, retry: noWait });
+
+            const err: unknown = await send(client).catch((e: unknown) => e);
+
+            expect(calls[0]?.init?.redirect).toBe("manual");
+            expect(err).toBeInstanceOf(ApiError);
+            expect((err as ApiError).status).toBe(307);
+            expect(calls).toHaveLength(1);
+        }
+    });
 });

test/packaging/nested-checkout.test.ts

@@ -0,0 +1,50 @@
+// A checkout nested under `.claude/` must not add its tests to this suite.
+// The test plants one in a temporary directory next to a real test file and
+// asks vitest, with this repo's config, which test files it would run.
+import { afterEach, describe, expect, it } from "vitest";
+import { execFileSync } from "node:child_process";
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { fileURLToPath } from "node:url";
+
+const repoRoot = fileURLToPath(new URL("../../", import.meta.url));
+
+let root = "";
+
+afterEach(() => {
+    rmSync(root, { recursive: true, force: true });
+});
+
+const writeTest = (path: string): void => {
+    mkdirSync(join(root, path, ".."), { recursive: true });
+    writeFileSync(
+        join(root, path),
+        'import { it } from "vitest";\nit("runs", () => {});\n',
+    );
+};
+
+describe("vitest.config.ts", () => {
+    it("does not collect tests from a checkout nested under .claude/", () => {
+        root = mkdtempSync(join(tmpdir(), "quak-nested-checkout-"));
+        writeTest("test/real.test.ts");
+        writeTest(".claude/worktrees/other/test/real.test.ts");
+
+        const output = execFileSync(
+            process.execPath,
+            [
+                join(repoRoot, "node_modules/vitest/vitest.mjs"),
+                "list",
+                "--filesOnly",
+                "--config",
+                join(repoRoot, "vitest.config.ts"),
+                "--root",
+                root,
+            ],
+            { cwd: root, encoding: "utf-8" },
+        );
+
+        const files = output.split("\n").filter((line) => line !== "");
+        expect(files).toEqual(["test/real.test.ts"]);
+    });
+});

test/retry/retry.test.ts

@@ -149,8 +149,11 @@ describe("isRetryable: transport failures", () => {
     });
 
     it("retries an errno carried on the error itself", () => {
+        // Every errno the classifier names, so none can be reclassified
+        // unnoticed.
         for (const code of [
             "ECONNRESET",
+            "ECONNABORTED",
             "ETIMEDOUT",
             "EPIPE",
             "ENOTFOUND",
@@ -158,6 +161,8 @@ describe("isRetryable: transport failures", () => {
             "ECONNREFUSED",
             "EHOSTUNREACH",
             "ENETUNREACH",
+            "ENETRESET",
+            "ENETDOWN",
         ]) {
             expect(isRetryable(errnoError(code))).toBe(true);
         }
@@ -215,6 +220,27 @@ describe("isRetryable: transport failures", () => {
         looped.cause = looped;
         expect(isRetryable(looped)).toBe(false);
     });
+
+    it("terminates on a cause chain that loops through two errors", () => {
+        const first: Error & { cause?: unknown } = new Error("first");
+        const second = new Error("second", { cause: first });
+        first.cause = second;
+        expect(isRetryable(first)).toBe(false);
+    });
+
+    it("reads the error and at most seven causes below it", () => {
+        // The walk is bounded at eight links. An errno at the eighth link is
+        // found; one at the ninth is not.
+        const buried = (causes: number): Error => {
+            let err = errnoError("ECONNRESET");
+            for (let i = 0; i < causes; i++) {
+                err = new Error(`wrapper ${i}`, { cause: err });
+            }
+            return err;
+        };
+        expect(isRetryable(buried(7))).toBe(true);
+        expect(isRetryable(buried(8))).toBe(false);
+    });
 });
 
 describe("isRetryable: stream truncation versus corruption", () => {
@@ -279,10 +305,9 @@ describe("isSafeToReplay", () => {
      * that is not the whole question: the other half is "could the first
      * attempt already have taken effect on the server?".
      *
-     * quak's non-idempotent calls are `/users/srp/create-session`,
-     * `/users/two-factor/verify` (which consumes one of a limited number of
-     * 2FA attempts) and `/files/thumbnail`. A blind replay of any of them can
-     * do real damage, so they retry only on the failures that establish no TCP
+     * The calls this guards are the `POST` and `PUT` requests listed in the
+     * README under "Endpoints used". A blind replay of some of them can do
+     * real damage, so they retry only on the failures that establish no TCP
      * connection to the server ever existed — DNS produced no address, or the
      * peer refused the connection — and therefore that no request byte can
      * have been transmitted.
@@ -333,6 +358,52 @@ describe("isSafeToReplay", () => {
         ).toBe(false);
         expect(isSafeToReplay(new TypeError("fetch failed"))).toBe(false);
     });
+
+    it("does not replay any other errno the classifier names", () => {
+        for (const code of [
+            "ECONNRESET",
+            "ECONNABORTED",
+            "ETIMEDOUT",
+            "EPIPE",
+            "EHOSTUNREACH",
+            "ENETUNREACH",
+            "ENETRESET",
+            "ENETDOWN",
+        ]) {
+            expect(isSafeToReplay(errnoError(code))).toBe(false);
+        }
+    });
+
+    it("does not replay a chain that also shows the request may have gone out", () => {
+        // A connect errno somewhere in the chain is not enough: any other
+        // errno beside it is doubt, and doubt is not replayed.
+        const reset = Object.assign(
+            new Error("read ECONNRESET", { cause: errnoError("ECONNREFUSED") }),
+            { code: "ECONNRESET" },
+        );
+        const mixed = new TypeError("fetch failed", { cause: reset });
+        expect(isRetryable(mixed)).toBe(true);
+        expect(isSafeToReplay(mixed)).toBe(false);
+    });
+
+    it("does not replay a chain longer than the walk reads", () => {
+        // Eight connect errnos, then a reset at the ninth link, below the
+        // limit. The walk never sees the reset, so it cannot rule it out.
+        const refusedChain = (below: Error | undefined): Error => {
+            let err = below;
+            for (let i = 0; i < 8; i++) {
+                err = Object.assign(new Error(`refused ${i}`, { cause: err }), {
+                    code: "ECONNREFUSED",
+                });
+            }
+            return err as Error;
+        };
+        expect(isSafeToReplay(refusedChain(errnoError("ECONNRESET")))).toBe(
+            false,
+        );
+        // The same eight links with nothing below them are replayable.
+        expect(isSafeToReplay(refusedChain(undefined))).toBe(true);
+    });
 });
 
 // ---------------------------------------------------------------------------

vitest.config.ts

@@ -0,0 +1,10 @@
+import { configDefaults, defineConfig } from "vitest/config";
+
+// vitest does not read .gitignore when looking for tests. A checkout nested
+// under .claude/ has its own test/ tree, and without this exclude the suite
+// runs once per nested checkout and still reports success.
+export default defineConfig({
+    test: {
+        exclude: [...configDefaults.exclude, ".claude/**"],
+    },
+});