diff --git a/README.md b/README.md index 0762343..34ee16e 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,93 @@ 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 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 +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 +397,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 +422,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..f3b9996 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,54 @@ 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 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, { + 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 +271,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 +332,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..4d5eb69 --- /dev/null +++ b/src/retry.ts @@ -0,0 +1,201 @@ +// 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 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 +// 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 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 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)); + +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/api/client.test.ts b/test/api/client.test.ts index 2d1522c..2faf019 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,446 @@ 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 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. + */ + 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..7baf8c0 --- /dev/null +++ b/test/retry/retry.test.ts @@ -0,0 +1,570 @@ +/** + * 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 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"]) { + 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); + // 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); + 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 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. 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); + }); + + 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 () => {