Add failing tests for the download retry policy
Tests only; the retry module they import does not exist yet, so the branch is red at this commit. New test/retry/retry.test.ts documents the classifier and the backoff: which errors are worth another attempt, which are not, and how the delay before each retry is derived. It asserts on the arguments handed to an injected sleep function rather than on elapsed time, so the suite never waits and the numbers are exact. test/api/client.test.ts gains the request-count contract for each of the six call sites, the deadline behaviour, the ApiError typing that the presigned PUT and the null-body paths need in order to be classified at all, and the replay rule for the two non-idempotent methods. test/download/download.test.ts gains the case that motivates the whole design: a socket reset after the response headers arrived, which happens below ApiClient and can only be caught by retrying the request, the stream consumption and the decryption together. It also pins that a retried download stages exactly one temp file, and that the two retry layers do not compose into a multiplied request budget. The existing truncation tests now assert on the error type rather than its wording, since that type is what the classifier reads. test/thumbnails/thumbnails.test.ts separates a genuine 404 from an exhausted retry, so a failing server can no longer make fix-missing-thumbnails re-upload thumbnails that already exist.
This commit is contained in:
@@ -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<Response> => {
|
||||
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<Uint8Array>({
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user