This is the TODO.md Next Step and the first unchecked box in the README TODO.
Every network request in the library goes through ApiClient (src/api/client.ts); there are
exactly six fetch call sites (:131, :141, :154, :176, :191, :217). None of them
has any retry, backoff, or timeout. A single transient 503 or TCP reset fails the file, and
a hung CDN connection blocks quak backup forever with no timeout at all.
The subtlety that makes this more than a one-line wrapper: getFileStream and getThumbnailStream return a ReadableStream as soon as headers arrive. The bytes are
pulled later, inside streamDecrypt in src/download/index.ts. A socket reset after
headers therefore throws in the download layer, never in ApiClient. Retrying only the fetch call would miss mid-stream failures, which are the dominant failure mode for
multi-megabyte photo downloads over a CDN.
Depends on #1 (truncation detection and atomic writes), which must land first.
Definition of done
A shared retry helper exists (e.g. src/retry.ts) exporting a withRetry wrapper and an isRetryable classifier with this policy:
ApiError with status 400-499: never retried, except 408 and 429, which are
retried.
Anything else (programming errors, decryption/authentication failures): never
retried.
Backoff is exponential with jitter and a cap, and the number of attempts, base delay and
cap are configurable through ApiClientOptions. Defaults are documented in the README.
A per-request timeout is applied via AbortSignal.timeout(), configurable through ApiClientOptions, so no request can hang forever. Timeout aborts count as retryable.
Retry covers request establishment for all six ApiClient call sites, and separately
covers the entire file and thumbnail download — request plus stream consumption plus
decryption — in downloadFile and downloadThumbnail.
ApiClient.putFile throws ApiError (carrying the status) instead of the bare new Error it throws today at src/api/client.ts:184-186, so the upload path can be
classified. Same for the two "response body is null" throws at :160 and :223.
Non-idempotent requests are not blindly replayed: postJSON and putJSON retry only on
network/timeout failures where the request provably did not reach the server, or retry is
opt-in per call. Whichever you choose, it is documented in a comment at the call site and
in the README.
Sleeping is injectable so tests never actually wait. The suite stays inside the 30s cap in script/test.
listMissingThumbnails (src/thumbnails.ts:32-76) still reports a genuine 404 as a
missing thumbnail, and does not report a thumbnail as missing merely because retries
were exhausted on a 5xx or network error. Its bare catch { } must distinguish the two.
runBackup and runMetadataBackup keep their existing per-file resilience semantics: the
retry lives below them, and a file that fails after exhausting retries is still logged and
skipped rather than aborting the run.
Tests cover, at minimum: a 404 causes exactly one request; a 500 is retried up to the
configured attempt count and then throws; a network rejection is retried; a mid-stream
abort on a file download is retried and the retry succeeds; a timeout fires and is
retried; postJSON follows whatever rule item 6 settled on.
README TODO checkbox ticked, README documents the retry and timeout defaults, TODO.md
Next Step moved to Completed Steps and the next Future Step promoted — all in the same
commit as the implementation.
make check green.
## Problem
This is the `TODO.md` Next Step and the first unchecked box in the README TODO.
Every network request in the library goes through `ApiClient` (`src/api/client.ts`); there are
exactly six `fetch` call sites (`:131`, `:141`, `:154`, `:176`, `:191`, `:217`). **None of them
has any retry, backoff, or timeout.** A single transient 503 or TCP reset fails the file, and
a hung CDN connection blocks `quak backup` forever with no timeout at all.
The subtlety that makes this more than a one-line wrapper: `getFileStream` and
`getThumbnailStream` return a `ReadableStream` as soon as *headers* arrive. The bytes are
pulled later, inside `streamDecrypt` in `src/download/index.ts`. A socket reset **after**
headers therefore throws in the download layer, never in `ApiClient`. Retrying only the
`fetch` call would miss mid-stream failures, which are the dominant failure mode for
multi-megabyte photo downloads over a CDN.
Depends on #1 (truncation detection and atomic writes), which must land first.
## Definition of done
1. A shared retry helper exists (e.g. `src/retry.ts`) exporting a `withRetry` wrapper and an
`isRetryable` classifier with this policy:
- `ApiError` with status 400-499: **never retried**, except `408` and `429`, which are
retried.
- `ApiError` with status 500-599: retried.
- Network-layer failures (`fetch` rejection, `ECONNRESET`, `ETIMEDOUT`, DNS/TLS failure)
and timeout aborts: retried.
- Truncation errors from #1: retried.
- Anything else (programming errors, decryption/authentication failures): **never**
retried.
2. Backoff is exponential with jitter and a cap, and the number of attempts, base delay and
cap are configurable through `ApiClientOptions`. Defaults are documented in the README.
3. A per-request timeout is applied via `AbortSignal.timeout()`, configurable through
`ApiClientOptions`, so no request can hang forever. Timeout aborts count as retryable.
4. Retry covers **request establishment** for all six `ApiClient` call sites, and separately
covers the **entire file and thumbnail download** — request plus stream consumption plus
decryption — in `downloadFile` and `downloadThumbnail`.
5. `ApiClient.putFile` throws `ApiError` (carrying the status) instead of the bare
`new Error` it throws today at `src/api/client.ts:184-186`, so the upload path can be
classified. Same for the two `"response body is null"` throws at `:160` and `:223`.
6. Non-idempotent requests are not blindly replayed: `postJSON` and `putJSON` retry only on
network/timeout failures where the request provably did not reach the server, or retry is
opt-in per call. Whichever you choose, it is documented in a comment at the call site and
in the README.
7. Sleeping is injectable so tests never actually wait. The suite stays inside the 30s cap in
`script/test`.
8. `listMissingThumbnails` (`src/thumbnails.ts:32-76`) still reports a genuine 404 as a
missing thumbnail, and does **not** report a thumbnail as missing merely because retries
were exhausted on a 5xx or network error. Its bare `catch { }` must distinguish the two.
9. `runBackup` and `runMetadataBackup` keep their existing per-file resilience semantics: the
retry lives below them, and a file that fails after exhausting retries is still logged and
skipped rather than aborting the run.
10. Tests cover, at minimum: a 404 causes exactly one request; a 500 is retried up to the
configured attempt count and then throws; a network rejection is retried; a mid-stream
abort on a file download is retried and the retry succeeds; a timeout fires and is
retried; `postJSON` follows whatever rule item 6 settled on.
11. README TODO checkbox ticked, README documents the retry and timeout defaults, `TODO.md`
Next Step moved to Completed Steps and the next Future Step promoted — all in the same
commit as the implementation.
12. `make check` green.
clawbot
added this to the 1.0.0 milestone 2026-08-09 03:43:12 +02:00
clawbot
self-assigned this 2026-08-09 03:43:12 +02:00
Note from the review of #20 — affects this issue's retry classification
#20 makes truncation detectable, which this issue depends on. In doing so it had to resolve an
ambiguity that has a direct consequence for the retry policy specified here, and the reviewer
asked for it to be recorded against this issue before implementation starts.
The situation. When a secretstream body ends with leftover bytes that do not form a complete
chunk, Poly1305 authentication fails. That failure carries no length or framing signal, so it is
impossible to distinguish "the transfer stopped mid-chunk" from "these bytes are corrupt". #20 resolves it in favour of truncation, preserving the underlying authentication error as cause, on the reasoning that for a backup tool a false "truncated" is cheap (re-download) and a
false "complete" is expensive (silent corruption kept forever).
What that means for the retry policy in this issue. The definition of done above lists
truncation errors as retryable and decryption/authentication failures as never retryable. For multi-chunk files that split still works: a corrupt chunk in mid-stream is unambiguous,
because the stream continued past it, and it still surfaces as an authentication failure.
For single-chunk files — which is most thumbnails and every small file — the split is not
achievable as written. A wrong file key, server-side corruption, and a connection cut mid-chunk
all now present identically as truncation. Retrying will not fix the first two; it will just cost
one extra round trip before failing again.
How to handle it here. Do not treat this as a blocker; treat it as a known and bounded
imprecision, and make the choice explicitly rather than inheriting it by accident:
Accept the extra retry on single-chunk files. It is bounded by the attempt count and one
wasted round trip on a genuinely corrupt file is a fair price for never silently keeping a
truncated one.
Do not add a special case that reclassifies single-chunk authentication failures as
non-retryable. That would reintroduce exactly the silent-corruption risk #20 exists to remove.
The reviewer suggested a possible later refinement: a chunksPulled > 0 signal could narrow
the ambiguity for some shapes. Out of scope for this issue; file it separately if it proves
worth doing.
Whatever is chosen, document it in a comment at the classifier and in the README section that
documents the retry defaults, so the imprecision is visible to anyone reading the policy rather
than buried in the crypto layer.
## Note from the review of #20 — affects this issue's retry classification
#20 makes truncation detectable, which this issue depends on. In doing so it had to resolve an
ambiguity that has a direct consequence for the retry policy specified here, and the reviewer
asked for it to be recorded against this issue before implementation starts.
**The situation.** When a secretstream body ends with leftover bytes that do not form a complete
chunk, Poly1305 authentication fails. That failure carries no length or framing signal, so it is
impossible to distinguish "the transfer stopped mid-chunk" from "these bytes are corrupt".
#20 resolves it in favour of truncation, preserving the underlying authentication error as
`cause`, on the reasoning that for a backup tool a false "truncated" is cheap (re-download) and a
false "complete" is expensive (silent corruption kept forever).
**What that means for the retry policy in this issue.** The definition of done above lists
truncation errors as retryable and decryption/authentication failures as never retryable. For
**multi-chunk** files that split still works: a corrupt chunk in mid-stream is unambiguous,
because the stream continued past it, and it still surfaces as an authentication failure.
For **single-chunk** files — which is most thumbnails and every small file — the split is not
achievable as written. A wrong file key, server-side corruption, and a connection cut mid-chunk
all now present identically as truncation. Retrying will not fix the first two; it will just cost
one extra round trip before failing again.
**How to handle it here.** Do not treat this as a blocker; treat it as a known and bounded
imprecision, and make the choice explicitly rather than inheriting it by accident:
- Accept the extra retry on single-chunk files. It is bounded by the attempt count and one
wasted round trip on a genuinely corrupt file is a fair price for never silently keeping a
truncated one.
- Do not add a special case that reclassifies single-chunk authentication failures as
non-retryable. That would reintroduce exactly the silent-corruption risk #20 exists to remove.
- The reviewer suggested a possible later refinement: a `chunksPulled > 0` signal could narrow
the ambiguity for some shapes. Out of scope for this issue; file it separately if it proves
worth doing.
Whatever is chosen, document it in a comment at the classifier and in the README section that
documents the retry defaults, so the imprecision is visible to anyone reading the policy rather
than buried in the crypto layer.
Written against main at 937bcb7, i.e. after #1 landed. Line references are to that tree.
Prerequisite you will hit immediately: truncation errors are not identifiable
streamDecrypt in src/download/index.ts now throws plain Errors whose messages begin download: stream truncated: (three distinct sites: no chunks, wrong final tag, trailing bytes
that failed to authenticate). The definition of done above requires truncation to be retryable,
and you must not classify on message text. String matching on error messages is exactly the
kind of coupling that breaks silently the next time someone rewords a message.
Introduce a typed error — an exported class such as TruncatedStreamError extends Error, or a readonly kind discriminant — and throw it from all three sites, preserving the existing cause
on the trailing-bytes path. Update the existing truncation tests to assert on the type rather than
only on the message. This is a small change to #1's code, but it is in service of this issue and
is in scope here.
The retry helper
New module, e.g. src/retry.ts. Two exports: a withRetry(fn, opts) wrapper and an isRetryable(err) classifier. Classification, exhaustively — every branch needs a test:
ApiError 400-499: not retried, except 408 and 429, which are.
ApiError 500-599: retried.
TruncatedStreamError: retried.
fetch rejections (TypeError), ECONNRESET / ETIMEDOUT (which arrive as cause, not on the
error itself — unwrap), and AbortError from a timeout: retried.
Everything else, including secretstream authentication failures that are not truncation: not
retried. Default to not retrying when unsure; a wrong "retryable" wastes round trips on a
permanent failure, and for a backup tool that is worse than failing fast.
Backoff is exponential with jitter and a cap. Attempt count, base delay and cap come from ApiClientOptions. The sleep function must be injectable — the suite currently runs in about
5s and must not grow by real waiting. Do not use fake timers as the primary mechanism; inject.
ApiClient (src/api/client.ts)
Six fetch sites: :131getJSON, :141postJSON, :154getFileStream, :176putFile, :191putJSON, :217getThumbnailStream. this._fetch is already injectable, which is how
every existing test drives it.
putFile currently throws a bare new Error and loses the status. Fix it to throw ApiError.
Same for the two "response body is null" throws. Without this the upload path cannot be
classified at all.
Add requestTimeoutMs via AbortSignal.timeout(). There is no timeout anywhere today, so a
hung CDN connection blocks quak backup forever. Note that a timeout on getFileStream must
cover the body read, not just the headers — a signal that only guards the initial fetch leaves
the same hang in place one layer down.
Idempotency:postJSON/putJSON reach /users/srp/create-session, /users/two-factor/verify and /files/thumbnail. Do not blindly replay them. Either retry only
on failures where the request provably never reached the server, or make retry opt-in per call.
Whichever you pick, say so in a comment at the call site and in the README.
Downloads (src/download/index.ts)
downloadFile and downloadThumbnail are each getXStream → streamDecrypt → writeAtomic.
A mid-stream reset throws inside streamDecrypt, not inside ApiClient, so wrap the whole
sequence — request, stream consumption, decryption — in withRetry. The secretstream pull state
is not resumable and there is no Range support, so a retry re-issues the request and starts over.
Keep writeAtomic as the last step; do not stage a file per attempt.
Thumbnails (src/thumbnails.ts)
listMissingThumbnails wraps getThumbnailStream in a bare catch { } that turns every failure
into "thumbnail fetch failed". Once retries exist it must distinguish a genuine 404 (thumbnail
really is missing → report it) from exhausted retries on a 5xx or network error (transient → do
not report). Otherwise helper fix-missing-thumbnails will regenerate and re-upload thumbnails
that already exist on the server.
Callers that must not double-retry
src/backup.ts:97 and src/metadata-backup.ts:159 already swallow per-file errors and continue.
The retry belongs strictly below them; their resilience semantics and the documented non-zero exit
code must not change.
Single-chunk ambiguity
See my earlier comment on this issue. For single-chunk files, a wrong key, server corruption, and
a mid-chunk cutoff are indistinguishable and all present as truncation, so they will be retried.
Accept that, do not add a special case reclassifying single-chunk auth failures as non-retryable,
and document the imprecision at the classifier and in the README.
Tests
test/api/client.test.ts:78-102 has a recordingFetch(...responses) harness that pops one
canned response per call — ideal for "exactly one request on 404, N on 500". Everything is fetch
injection; no new dependency is needed and none should be added.
test/download/download.test.ts has seeded-LCG fixtures (patternBytes) and a vi.mock of node:fs/promisesrename. Follow those patterns; do not reintroduce sodium.randombytes_buf
in fixtures — it cost ~20s and failed the suite once already.
Required cases: 404 issues exactly one request; 500 retried to the configured limit then throws;
408 and 429 retried; fetch rejection retried; timeout fires and is retried; a mid-stream abort
on a file download retried and the retry succeeds; a truncated body retried; a non-truncation
authentication failure not retried; whatever rule you chose for postJSON.
Assert on request counts, not elapsed time. No test may depend on wall-clock duration.
Process
Branch off main. First commit is the failing tests; implementation in a later commit.
Verify only via make targets / script/ entrypoints. Never invoke vitest, eslint, prettier,
tsc or yarn scripts directly.
Report the measured make test time. Baseline is about 5s at 141 tests; the 20s budget and the
30s hard cap in script/test both still apply.
make build is broken on main (#3) and is not in scope here.
README: tick the retry TODO checkbox, document the retry and timeout defaults and the
idempotency rule. TODO.md: move the Next Step into Completed Steps and promote the next Future
Step, in the same commit as the implementation. Run make fmt.
Do not touch #8 (runBackup skip heuristic), #9 (filename sanitization), #21 (quadratic
buffering) or #22 (fsync/temp-file reaping).
## Implementation requirements
Written against `main` at `937bcb7`, i.e. after #1 landed. Line references are to that tree.
### Prerequisite you will hit immediately: truncation errors are not identifiable
`streamDecrypt` in `src/download/index.ts` now throws plain `Error`s whose messages begin
`download: stream truncated:` (three distinct sites: no chunks, wrong final tag, trailing bytes
that failed to authenticate). The definition of done above requires truncation to be retryable,
and **you must not classify on message text.** String matching on error messages is exactly the
kind of coupling that breaks silently the next time someone rewords a message.
Introduce a typed error — an exported class such as `TruncatedStreamError extends Error`, or a
`readonly kind` discriminant — and throw it from all three sites, preserving the existing `cause`
on the trailing-bytes path. Update the existing truncation tests to assert on the type rather than
only on the message. This is a small change to #1's code, but it is in service of this issue and
is in scope here.
### The retry helper
New module, e.g. `src/retry.ts`. Two exports: a `withRetry(fn, opts)` wrapper and an
`isRetryable(err)` classifier. Classification, exhaustively — every branch needs a test:
- `ApiError` 400-499: not retried, **except** 408 and 429, which are.
- `ApiError` 500-599: retried.
- `TruncatedStreamError`: retried.
- `fetch` rejections (`TypeError`), `ECONNRESET` / `ETIMEDOUT` (which arrive as `cause`, not on the
error itself — unwrap), and `AbortError` from a timeout: retried.
- Everything else, including secretstream authentication failures that are not truncation: **not**
retried. Default to not retrying when unsure; a wrong "retryable" wastes round trips on a
permanent failure, and for a backup tool that is worse than failing fast.
Backoff is exponential with jitter and a cap. Attempt count, base delay and cap come from
`ApiClientOptions`. **The sleep function must be injectable** — the suite currently runs in about
5s and must not grow by real waiting. Do not use fake timers as the primary mechanism; inject.
### `ApiClient` (`src/api/client.ts`)
- Six fetch sites: `:131` `getJSON`, `:141` `postJSON`, `:154` `getFileStream`, `:176` `putFile`,
`:191` `putJSON`, `:217` `getThumbnailStream`. `this._fetch` is already injectable, which is how
every existing test drives it.
- `putFile` currently throws a bare `new Error` and loses the status. Fix it to throw `ApiError`.
Same for the two `"response body is null"` throws. Without this the upload path cannot be
classified at all.
- Add `requestTimeoutMs` via `AbortSignal.timeout()`. There is no timeout anywhere today, so a
hung CDN connection blocks `quak backup` forever. Note that a timeout on `getFileStream` must
cover the *body read*, not just the headers — a signal that only guards the initial fetch leaves
the same hang in place one layer down.
- **Idempotency:** `postJSON`/`putJSON` reach `/users/srp/create-session`,
`/users/two-factor/verify` and `/files/thumbnail`. Do not blindly replay them. Either retry only
on failures where the request provably never reached the server, or make retry opt-in per call.
Whichever you pick, say so in a comment at the call site and in the README.
### Downloads (`src/download/index.ts`)
`downloadFile` and `downloadThumbnail` are each `getXStream` → `streamDecrypt` → `writeAtomic`.
A mid-stream reset throws inside `streamDecrypt`, not inside `ApiClient`, so **wrap the whole
sequence** — request, stream consumption, decryption — in `withRetry`. The secretstream pull state
is not resumable and there is no Range support, so a retry re-issues the request and starts over.
Keep `writeAtomic` as the last step; do not stage a file per attempt.
### Thumbnails (`src/thumbnails.ts`)
`listMissingThumbnails` wraps `getThumbnailStream` in a bare `catch { }` that turns every failure
into `"thumbnail fetch failed"`. Once retries exist it must distinguish a genuine 404 (thumbnail
really is missing → report it) from exhausted retries on a 5xx or network error (transient → do
not report). Otherwise `helper fix-missing-thumbnails` will regenerate and re-upload thumbnails
that already exist on the server.
### Callers that must not double-retry
`src/backup.ts:97` and `src/metadata-backup.ts:159` already swallow per-file errors and continue.
The retry belongs strictly below them; their resilience semantics and the documented non-zero exit
code must not change.
### Single-chunk ambiguity
See my earlier comment on this issue. For single-chunk files, a wrong key, server corruption, and
a mid-chunk cutoff are indistinguishable and all present as truncation, so they will be retried.
Accept that, do not add a special case reclassifying single-chunk auth failures as non-retryable,
and document the imprecision at the classifier and in the README.
### Tests
- `test/api/client.test.ts:78-102` has a `recordingFetch(...responses)` harness that pops one
canned response per call — ideal for "exactly one request on 404, N on 500". Everything is fetch
injection; **no new dependency is needed** and none should be added.
- `test/download/download.test.ts` has seeded-LCG fixtures (`patternBytes`) and a `vi.mock` of
`node:fs/promises` `rename`. Follow those patterns; do not reintroduce `sodium.randombytes_buf`
in fixtures — it cost ~20s and failed the suite once already.
- Required cases: 404 issues exactly one request; 500 retried to the configured limit then throws;
408 and 429 retried; `fetch` rejection retried; timeout fires and is retried; a mid-stream abort
on a file download retried and the retry succeeds; a truncated body retried; a non-truncation
authentication failure **not** retried; whatever rule you chose for `postJSON`.
- Assert on **request counts**, not elapsed time. No test may depend on wall-clock duration.
### Process
- Branch off `main`. First commit is the failing tests; implementation in a later commit.
- Verify only via `make` targets / `script/` entrypoints. Never invoke vitest, eslint, prettier,
tsc or yarn scripts directly.
- Report the measured `make test` time. Baseline is about 5s at 141 tests; the 20s budget and the
30s hard cap in `script/test` both still apply.
- `make build` is broken on `main` (#3) and is not in scope here.
- README: tick the retry TODO checkbox, document the retry and timeout defaults and the
idempotency rule. `TODO.md`: move the Next Step into Completed Steps and promote the next Future
Step, in the same commit as the implementation. Run `make fmt`.
- Do not touch #8 (`runBackup` skip heuristic), #9 (filename sanitization), #21 (quadratic
buffering) or #22 (fsync/temp-file reaping).
Branch retry-policy off main at 937bcb7. First commit is the tests, red; implementation follows.
Module layout
src/errors.ts (new) — holds ApiError (moved here verbatim) and the new TruncatedStreamError. src/api/client.ts re-exports ApiError so every existing import path and instanceof check keeps working. The move exists only to break the import cycle that would otherwise appear: the classifier needs ApiError, and ApiClient needs the classifier.
src/retry.ts (new) — isRetryable(err), withRetry(fn, opts), resolveRetryOptions(opts), and the exported defaults.
Classification
ApiError 408 / 429 -> retry; other 4xx -> no retry; 5xx -> retry.
TruncatedStreamError -> retry.
TypeError (how undici surfaces every fetch failure) -> retry.
AbortError / TimeoutError -> retry.
Any error carrying a transport errno anywhere in its cause chain (ECONNRESET, ETIMEDOUT, EPIPE, ENOTFOUND, EAI_AGAIN, ECONNREFUSED, EHOSTUNREACH, ENETUNREACH) -> retry. The chain is walked because undici hangs the real errno off cause, not off the error itself.
Everything else, including secretstream chunk authentication failed that is not truncation -> no retry. Unknown means no retry.
The single-chunk ambiguity recorded in the first comment is accepted as-is: no special case reclassifies a single-chunk authentication failure. It will be documented in a comment at the classifier and in the README.
Backoff and timeouts
Exponential with full jitter and a cap: attempt n waits random() * min(maxDelayMs, baseDelayMs * 2^(n-1)). Defaults attempts: 4, baseDelayMs: 500, maxDelayMs: 10000. Both sleep and random are injectable through ApiClientOptions.retry, so no test waits and no test depends on jitter.
Two timeouts, because one number cannot serve both: requestTimeoutMs (default 30000) for the four JSON/upload calls, and downloadTimeoutMs (default 600000) for the two stream endpoints, where the deadline has to cover a multi-gigabyte body. Both via AbortSignal.timeout(), a fresh signal per attempt.
For the stream endpoints the signal alone is not enough to make the guarantee ours: it only aborts the body if the fetch implementation honours it after headers. So getFileStream / getThumbnailStream wrap the returned ReadableStream in a reader that races each read() against the signal and errors the stream with the abort reason. That is what makes "the timeout covers the body read" a property of this repo rather than of undici, and it is testable against an injected fetch that ignores the signal.
Call sites
All six fetch sites retry by default.
putFile and the two "response body is null" throws become ApiError carrying the status.
Idempotency:postJSON and putJSON retry only on failures that prove no request bytes reached the server, i.e. the connection was never established (ENOTFOUND, EAI_AGAIN, ECONNREFUSED, EHOSTUNREACH, ENETUNREACH). A 5xx, a mid-flight ECONNRESET, and a timeout are all explicitly not replayed, because each can occur after the server has already processed the request — and these paths reach /users/srp/create-session, /users/two-factor/verify (which burns a 2FA attempt) and /files/thumbnail. Documented at the call site and in the README.
getFileStream / getThumbnailStream take { retry: false } so the download layer can wrap the whole sequence in its own withRetry without the two budgets multiplying (4 x 4 = 16 requests).
Downloads and thumbnails
downloadFile / downloadThumbnail wrap request + stream consumption + decryption in one withRetry. writeAtomic stays outside it, so a retried download stages exactly one temp file and only on the attempt that succeeded.
listMissingThumbnails reports a thumbnail as missing only on ApiError 404 (or an empty body). Anything else — exhausted retries on 5xx, network, timeout — is logged through the progress callback and not reported, so helper fix-missing-thumbnails cannot be talked into re-uploading thumbnails that already exist.
runBackup / runMetadataBackup are untouched; the retry sits strictly below them.
Tests
New test/retry/retry.test.ts for the classifier and the backoff (asserting on the recorded arguments passed to the injected sleep, never on elapsed time), plus additions to test/api/client.test.ts, test/download/download.test.ts and test/thumbnails/thumbnails.test.ts covering: 404 issues exactly one request; 500 issues exactly the configured attempt count; 408 and 429 retried; fetch rejection retried then succeeding; a timeout firing and being retried; a stalled body aborted by the download timeout; a mid-stream reset retried with the retry succeeding; a truncated body retried; a corrupt whole chunk not retried; postJSON not replayed on 500 / ECONNRESET / timeout but replayed on ECONNREFUSED; and exactly one rename after a retried download. Every assertion is on request counts. Existing fixtures (recordingFetch, patternBytes, the rename hook) are reused; no new dependency, and no sodium.randombytes_buf in fixtures.
test/cli/backup.test.ts's existing 500 case gets an injected no-op sleep so it does not start waiting for real.
README TODO checkbox, retry/timeout/idempotency documentation, and the TODO.md Next Step rotation all land in the implementation commit.
## Implementation plan
Branch `retry-policy` off `main` at `937bcb7`. First commit is the tests, red; implementation follows.
### Module layout
- **`src/errors.ts` (new)** — holds `ApiError` (moved here verbatim) and the new `TruncatedStreamError`. `src/api/client.ts` re-exports `ApiError` so every existing import path and `instanceof` check keeps working. The move exists only to break the import cycle that would otherwise appear: the classifier needs `ApiError`, and `ApiClient` needs the classifier.
- **`src/retry.ts` (new)** — `isRetryable(err)`, `withRetry(fn, opts)`, `resolveRetryOptions(opts)`, and the exported defaults.
### Classification
- `ApiError` 408 / 429 -> retry; other 4xx -> no retry; 5xx -> retry.
- `TruncatedStreamError` -> retry.
- `TypeError` (how undici surfaces every `fetch` failure) -> retry.
- `AbortError` / `TimeoutError` -> retry.
- Any error carrying a transport errno anywhere in its `cause` chain (`ECONNRESET`, `ETIMEDOUT`, `EPIPE`, `ENOTFOUND`, `EAI_AGAIN`, `ECONNREFUSED`, `EHOSTUNREACH`, `ENETUNREACH`) -> retry. The chain is walked because undici hangs the real errno off `cause`, not off the error itself.
- Everything else, including `secretstream chunk authentication failed` that is not truncation -> no retry. Unknown means no retry.
The single-chunk ambiguity recorded in the first comment is accepted as-is: no special case reclassifies a single-chunk authentication failure. It will be documented in a comment at the classifier and in the README.
### Backoff and timeouts
Exponential with full jitter and a cap: attempt *n* waits `random() * min(maxDelayMs, baseDelayMs * 2^(n-1))`. Defaults `attempts: 4`, `baseDelayMs: 500`, `maxDelayMs: 10000`. Both `sleep` and `random` are injectable through `ApiClientOptions.retry`, so no test waits and no test depends on jitter.
Two timeouts, because one number cannot serve both: `requestTimeoutMs` (default 30000) for the four JSON/upload calls, and `downloadTimeoutMs` (default 600000) for the two stream endpoints, where the deadline has to cover a multi-gigabyte body. Both via `AbortSignal.timeout()`, a fresh signal per attempt.
For the stream endpoints the signal alone is not enough to make the guarantee ours: it only aborts the body if the `fetch` implementation honours it after headers. So `getFileStream` / `getThumbnailStream` wrap the returned `ReadableStream` in a reader that races each `read()` against the signal and errors the stream with the abort reason. That is what makes "the timeout covers the body read" a property of this repo rather than of undici, and it is testable against an injected `fetch` that ignores the signal.
### Call sites
- All six `fetch` sites retry by default.
- `putFile` and the two `"response body is null"` throws become `ApiError` carrying the status.
- **Idempotency:** `postJSON` and `putJSON` retry *only* on failures that prove no request bytes reached the server, i.e. the connection was never established (`ENOTFOUND`, `EAI_AGAIN`, `ECONNREFUSED`, `EHOSTUNREACH`, `ENETUNREACH`). A 5xx, a mid-flight `ECONNRESET`, and a timeout are all explicitly *not* replayed, because each can occur after the server has already processed the request — and these paths reach `/users/srp/create-session`, `/users/two-factor/verify` (which burns a 2FA attempt) and `/files/thumbnail`. Documented at the call site and in the README.
- `getFileStream` / `getThumbnailStream` take `{ retry: false }` so the download layer can wrap the whole sequence in its own `withRetry` without the two budgets multiplying (4 x 4 = 16 requests).
### Downloads and thumbnails
- `downloadFile` / `downloadThumbnail` wrap request + stream consumption + decryption in one `withRetry`. `writeAtomic` stays outside it, so a retried download stages exactly one temp file and only on the attempt that succeeded.
- `listMissingThumbnails` reports a thumbnail as missing only on `ApiError` 404 (or an empty body). Anything else — exhausted retries on 5xx, network, timeout — is logged through the progress callback and *not* reported, so `helper fix-missing-thumbnails` cannot be talked into re-uploading thumbnails that already exist.
- `runBackup` / `runMetadataBackup` are untouched; the retry sits strictly below them.
### Tests
New `test/retry/retry.test.ts` for the classifier and the backoff (asserting on the recorded arguments passed to the injected `sleep`, never on elapsed time), plus additions to `test/api/client.test.ts`, `test/download/download.test.ts` and `test/thumbnails/thumbnails.test.ts` covering: 404 issues exactly one request; 500 issues exactly the configured attempt count; 408 and 429 retried; `fetch` rejection retried then succeeding; a timeout firing and being retried; a stalled body aborted by the download timeout; a mid-stream reset retried with the retry succeeding; a truncated body retried; a corrupt whole chunk not retried; `postJSON` not replayed on 500 / `ECONNRESET` / timeout but replayed on `ECONNREFUSED`; and exactly one `rename` after a retried download. Every assertion is on request counts. Existing fixtures (`recordingFetch`, `patternBytes`, the `rename` hook) are reused; no new dependency, and no `sodium.randombytes_buf` in fixtures.
`test/cli/backup.test.ts`'s existing 500 case gets an injected no-op `sleep` so it does not start waiting for real.
README TODO checkbox, retry/timeout/idempotency documentation, and the `TODO.md` Next Step rotation all land in the implementation commit.
Implemented in #23 — #23 (branch retry-policy, awaiting review).
All twelve items of the definition of done are addressed there. The two decisions the issue left open:
Idempotency (item 6):postJSON and putJSON are replayed only on failures that prove no request byte reached the server, i.e. the connection was never established. Not opt-in per call. A 5xx, a mid-flight ECONNRESET and a deadline are all explicitly not replayed. Documented at both call sites and in the README.
Single-chunk ambiguity: accepted as the manager note asked. No special case reclassifies a single-chunk authentication failure as non-retryable, and the imprecision is documented at the classifier and in the README.
make check green; make test 7.48s at 210 tests, up from ~5s at 141.
Implemented in #23 — https://git.eeqj.de/sneak/quak/pulls/23 (branch `retry-policy`, awaiting review).
All twelve items of the definition of done are addressed there. The two decisions the issue left open:
- **Idempotency (item 6):** `postJSON` and `putJSON` are replayed only on failures that prove no request byte reached the server, i.e. the connection was never established. Not opt-in per call. A 5xx, a mid-flight `ECONNRESET` and a deadline are all explicitly not replayed. Documented at both call sites and in the README.
- **Single-chunk ambiguity:** accepted as the manager note asked. No special case reclassifies a single-chunk authentication failure as non-retryable, and the imprecision is documented at the classifier and in the README.
`make check` green; `make test` 7.48s at 210 tests, up from ~5s at 141.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Problem
This is the
TODO.mdNext Step and the first unchecked box in the README TODO.Every network request in the library goes through
ApiClient(src/api/client.ts); there areexactly six
fetchcall sites (:131,:141,:154,:176,:191,:217). None of themhas any retry, backoff, or timeout. A single transient 503 or TCP reset fails the file, and
a hung CDN connection blocks
quak backupforever with no timeout at all.The subtlety that makes this more than a one-line wrapper:
getFileStreamandgetThumbnailStreamreturn aReadableStreamas soon as headers arrive. The bytes arepulled later, inside
streamDecryptinsrc/download/index.ts. A socket reset afterheaders therefore throws in the download layer, never in
ApiClient. Retrying only thefetchcall would miss mid-stream failures, which are the dominant failure mode formulti-megabyte photo downloads over a CDN.
Depends on #1 (truncation detection and atomic writes), which must land first.
Definition of done
src/retry.ts) exporting awithRetrywrapper and anisRetryableclassifier with this policy:ApiErrorwith status 400-499: never retried, except408and429, which areretried.
ApiErrorwith status 500-599: retried.fetchrejection,ECONNRESET,ETIMEDOUT, DNS/TLS failure)and timeout aborts: retried.
retried.
cap are configurable through
ApiClientOptions. Defaults are documented in the README.AbortSignal.timeout(), configurable throughApiClientOptions, so no request can hang forever. Timeout aborts count as retryable.ApiClientcall sites, and separatelycovers the entire file and thumbnail download — request plus stream consumption plus
decryption — in
downloadFileanddownloadThumbnail.ApiClient.putFilethrowsApiError(carrying the status) instead of the barenew Errorit throws today atsrc/api/client.ts:184-186, so the upload path can beclassified. Same for the two
"response body is null"throws at:160and:223.postJSONandputJSONretry only onnetwork/timeout failures where the request provably did not reach the server, or retry is
opt-in per call. Whichever you choose, it is documented in a comment at the call site and
in the README.
script/test.listMissingThumbnails(src/thumbnails.ts:32-76) still reports a genuine 404 as amissing thumbnail, and does not report a thumbnail as missing merely because retries
were exhausted on a 5xx or network error. Its bare
catch { }must distinguish the two.runBackupandrunMetadataBackupkeep their existing per-file resilience semantics: theretry lives below them, and a file that fails after exhausting retries is still logged and
skipped rather than aborting the run.
configured attempt count and then throws; a network rejection is retried; a mid-stream
abort on a file download is retried and the retry succeeds; a timeout fires and is
retried;
postJSONfollows whatever rule item 6 settled on.TODO.mdNext Step moved to Completed Steps and the next Future Step promoted — all in the same
commit as the implementation.
make checkgreen.Note from the review of #20 — affects this issue's retry classification
#20 makes truncation detectable, which this issue depends on. In doing so it had to resolve an
ambiguity that has a direct consequence for the retry policy specified here, and the reviewer
asked for it to be recorded against this issue before implementation starts.
The situation. When a secretstream body ends with leftover bytes that do not form a complete
chunk, Poly1305 authentication fails. That failure carries no length or framing signal, so it is
impossible to distinguish "the transfer stopped mid-chunk" from "these bytes are corrupt".
#20 resolves it in favour of truncation, preserving the underlying authentication error as
cause, on the reasoning that for a backup tool a false "truncated" is cheap (re-download) and afalse "complete" is expensive (silent corruption kept forever).
What that means for the retry policy in this issue. The definition of done above lists
truncation errors as retryable and decryption/authentication failures as never retryable. For
multi-chunk files that split still works: a corrupt chunk in mid-stream is unambiguous,
because the stream continued past it, and it still surfaces as an authentication failure.
For single-chunk files — which is most thumbnails and every small file — the split is not
achievable as written. A wrong file key, server-side corruption, and a connection cut mid-chunk
all now present identically as truncation. Retrying will not fix the first two; it will just cost
one extra round trip before failing again.
How to handle it here. Do not treat this as a blocker; treat it as a known and bounded
imprecision, and make the choice explicitly rather than inheriting it by accident:
wasted round trip on a genuinely corrupt file is a fair price for never silently keeping a
truncated one.
non-retryable. That would reintroduce exactly the silent-corruption risk #20 exists to remove.
chunksPulled > 0signal could narrowthe ambiguity for some shapes. Out of scope for this issue; file it separately if it proves
worth doing.
Whatever is chosen, document it in a comment at the classifier and in the README section that
documents the retry defaults, so the imprecision is visible to anyone reading the policy rather
than buried in the crypto layer.
Implementation requirements
Written against
mainat937bcb7, i.e. after #1 landed. Line references are to that tree.Prerequisite you will hit immediately: truncation errors are not identifiable
streamDecryptinsrc/download/index.tsnow throws plainErrors whose messages begindownload: stream truncated:(three distinct sites: no chunks, wrong final tag, trailing bytesthat failed to authenticate). The definition of done above requires truncation to be retryable,
and you must not classify on message text. String matching on error messages is exactly the
kind of coupling that breaks silently the next time someone rewords a message.
Introduce a typed error — an exported class such as
TruncatedStreamError extends Error, or areadonly kinddiscriminant — and throw it from all three sites, preserving the existingcauseon the trailing-bytes path. Update the existing truncation tests to assert on the type rather than
only on the message. This is a small change to #1's code, but it is in service of this issue and
is in scope here.
The retry helper
New module, e.g.
src/retry.ts. Two exports: awithRetry(fn, opts)wrapper and anisRetryable(err)classifier. Classification, exhaustively — every branch needs a test:ApiError400-499: not retried, except 408 and 429, which are.ApiError500-599: retried.TruncatedStreamError: retried.fetchrejections (TypeError),ECONNRESET/ETIMEDOUT(which arrive ascause, not on theerror itself — unwrap), and
AbortErrorfrom a timeout: retried.retried. Default to not retrying when unsure; a wrong "retryable" wastes round trips on a
permanent failure, and for a backup tool that is worse than failing fast.
Backoff is exponential with jitter and a cap. Attempt count, base delay and cap come from
ApiClientOptions. The sleep function must be injectable — the suite currently runs in about5s and must not grow by real waiting. Do not use fake timers as the primary mechanism; inject.
ApiClient(src/api/client.ts):131getJSON,:141postJSON,:154getFileStream,:176putFile,:191putJSON,:217getThumbnailStream.this._fetchis already injectable, which is howevery existing test drives it.
putFilecurrently throws a barenew Errorand loses the status. Fix it to throwApiError.Same for the two
"response body is null"throws. Without this the upload path cannot beclassified at all.
requestTimeoutMsviaAbortSignal.timeout(). There is no timeout anywhere today, so ahung CDN connection blocks
quak backupforever. Note that a timeout ongetFileStreammustcover the body read, not just the headers — a signal that only guards the initial fetch leaves
the same hang in place one layer down.
postJSON/putJSONreach/users/srp/create-session,/users/two-factor/verifyand/files/thumbnail. Do not blindly replay them. Either retry onlyon failures where the request provably never reached the server, or make retry opt-in per call.
Whichever you pick, say so in a comment at the call site and in the README.
Downloads (
src/download/index.ts)downloadFileanddownloadThumbnailare eachgetXStream→streamDecrypt→writeAtomic.A mid-stream reset throws inside
streamDecrypt, not insideApiClient, so wrap the wholesequence — request, stream consumption, decryption — in
withRetry. The secretstream pull stateis not resumable and there is no Range support, so a retry re-issues the request and starts over.
Keep
writeAtomicas the last step; do not stage a file per attempt.Thumbnails (
src/thumbnails.ts)listMissingThumbnailswrapsgetThumbnailStreamin a barecatch { }that turns every failureinto
"thumbnail fetch failed". Once retries exist it must distinguish a genuine 404 (thumbnailreally is missing → report it) from exhausted retries on a 5xx or network error (transient → do
not report). Otherwise
helper fix-missing-thumbnailswill regenerate and re-upload thumbnailsthat already exist on the server.
Callers that must not double-retry
src/backup.ts:97andsrc/metadata-backup.ts:159already swallow per-file errors and continue.The retry belongs strictly below them; their resilience semantics and the documented non-zero exit
code must not change.
Single-chunk ambiguity
See my earlier comment on this issue. For single-chunk files, a wrong key, server corruption, and
a mid-chunk cutoff are indistinguishable and all present as truncation, so they will be retried.
Accept that, do not add a special case reclassifying single-chunk auth failures as non-retryable,
and document the imprecision at the classifier and in the README.
Tests
test/api/client.test.ts:78-102has arecordingFetch(...responses)harness that pops onecanned response per call — ideal for "exactly one request on 404, N on 500". Everything is fetch
injection; no new dependency is needed and none should be added.
test/download/download.test.tshas seeded-LCG fixtures (patternBytes) and avi.mockofnode:fs/promisesrename. Follow those patterns; do not reintroducesodium.randombytes_bufin fixtures — it cost ~20s and failed the suite once already.
408 and 429 retried;
fetchrejection retried; timeout fires and is retried; a mid-stream aborton a file download retried and the retry succeeds; a truncated body retried; a non-truncation
authentication failure not retried; whatever rule you chose for
postJSON.Process
main. First commit is the failing tests; implementation in a later commit.maketargets /script/entrypoints. Never invoke vitest, eslint, prettier,tsc or yarn scripts directly.
make testtime. Baseline is about 5s at 141 tests; the 20s budget and the30s hard cap in
script/testboth still apply.make buildis broken onmain(#3) and is not in scope here.idempotency rule.
TODO.md: move the Next Step into Completed Steps and promote the next FutureStep, in the same commit as the implementation. Run
make fmt.runBackupskip heuristic), #9 (filename sanitization), #21 (quadraticbuffering) or #22 (fsync/temp-file reaping).
Implementation plan
Branch
retry-policyoffmainat937bcb7. First commit is the tests, red; implementation follows.Module layout
src/errors.ts(new) — holdsApiError(moved here verbatim) and the newTruncatedStreamError.src/api/client.tsre-exportsApiErrorso every existing import path andinstanceofcheck keeps working. The move exists only to break the import cycle that would otherwise appear: the classifier needsApiError, andApiClientneeds the classifier.src/retry.ts(new) —isRetryable(err),withRetry(fn, opts),resolveRetryOptions(opts), and the exported defaults.Classification
ApiError408 / 429 -> retry; other 4xx -> no retry; 5xx -> retry.TruncatedStreamError-> retry.TypeError(how undici surfaces everyfetchfailure) -> retry.AbortError/TimeoutError-> retry.causechain (ECONNRESET,ETIMEDOUT,EPIPE,ENOTFOUND,EAI_AGAIN,ECONNREFUSED,EHOSTUNREACH,ENETUNREACH) -> retry. The chain is walked because undici hangs the real errno offcause, not off the error itself.secretstream chunk authentication failedthat is not truncation -> no retry. Unknown means no retry.The single-chunk ambiguity recorded in the first comment is accepted as-is: no special case reclassifies a single-chunk authentication failure. It will be documented in a comment at the classifier and in the README.
Backoff and timeouts
Exponential with full jitter and a cap: attempt n waits
random() * min(maxDelayMs, baseDelayMs * 2^(n-1)). Defaultsattempts: 4,baseDelayMs: 500,maxDelayMs: 10000. Bothsleepandrandomare injectable throughApiClientOptions.retry, so no test waits and no test depends on jitter.Two timeouts, because one number cannot serve both:
requestTimeoutMs(default 30000) for the four JSON/upload calls, anddownloadTimeoutMs(default 600000) for the two stream endpoints, where the deadline has to cover a multi-gigabyte body. Both viaAbortSignal.timeout(), a fresh signal per attempt.For the stream endpoints the signal alone is not enough to make the guarantee ours: it only aborts the body if the
fetchimplementation honours it after headers. SogetFileStream/getThumbnailStreamwrap the returnedReadableStreamin a reader that races eachread()against the signal and errors the stream with the abort reason. That is what makes "the timeout covers the body read" a property of this repo rather than of undici, and it is testable against an injectedfetchthat ignores the signal.Call sites
fetchsites retry by default.putFileand the two"response body is null"throws becomeApiErrorcarrying the status.postJSONandputJSONretry only on failures that prove no request bytes reached the server, i.e. the connection was never established (ENOTFOUND,EAI_AGAIN,ECONNREFUSED,EHOSTUNREACH,ENETUNREACH). A 5xx, a mid-flightECONNRESET, and a timeout are all explicitly not replayed, because each can occur after the server has already processed the request — and these paths reach/users/srp/create-session,/users/two-factor/verify(which burns a 2FA attempt) and/files/thumbnail. Documented at the call site and in the README.getFileStream/getThumbnailStreamtake{ retry: false }so the download layer can wrap the whole sequence in its ownwithRetrywithout the two budgets multiplying (4 x 4 = 16 requests).Downloads and thumbnails
downloadFile/downloadThumbnailwrap request + stream consumption + decryption in onewithRetry.writeAtomicstays outside it, so a retried download stages exactly one temp file and only on the attempt that succeeded.listMissingThumbnailsreports a thumbnail as missing only onApiError404 (or an empty body). Anything else — exhausted retries on 5xx, network, timeout — is logged through the progress callback and not reported, sohelper fix-missing-thumbnailscannot be talked into re-uploading thumbnails that already exist.runBackup/runMetadataBackupare untouched; the retry sits strictly below them.Tests
New
test/retry/retry.test.tsfor the classifier and the backoff (asserting on the recorded arguments passed to the injectedsleep, never on elapsed time), plus additions totest/api/client.test.ts,test/download/download.test.tsandtest/thumbnails/thumbnails.test.tscovering: 404 issues exactly one request; 500 issues exactly the configured attempt count; 408 and 429 retried;fetchrejection retried then succeeding; a timeout firing and being retried; a stalled body aborted by the download timeout; a mid-stream reset retried with the retry succeeding; a truncated body retried; a corrupt whole chunk not retried;postJSONnot replayed on 500 /ECONNRESET/ timeout but replayed onECONNREFUSED; and exactly onerenameafter a retried download. Every assertion is on request counts. Existing fixtures (recordingFetch,patternBytes, therenamehook) are reused; no new dependency, and nosodium.randombytes_bufin fixtures.test/cli/backup.test.ts's existing 500 case gets an injected no-opsleepso it does not start waiting for real.README TODO checkbox, retry/timeout/idempotency documentation, and the
TODO.mdNext Step rotation all land in the implementation commit.Implemented in #23 — #23 (branch
retry-policy, awaiting review).All twelve items of the definition of done are addressed there. The two decisions the issue left open:
postJSONandputJSONare replayed only on failures that prove no request byte reached the server, i.e. the connection was never established. Not opt-in per call. A 5xx, a mid-flightECONNRESETand a deadline are all explicitly not replayed. Documented at both call sites and in the README.make checkgreen;make test7.48s at 210 tests, up from ~5s at 141.