sneak/quak
1
0
Fork 0
Code Issues 23 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Retry policy: no retry on 4xx, exponential backoff on 5xx and network errors #2

New Issue
Closed
opened 2026-08-09 03:43:12 +02:00 by clawbot · 4 comments
clawbot commented 2026-08-09 03:43:12 +02:00
Collaborator
Copy Link

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.
## 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
clawbot commented 2026-08-09 04:40:32 +02:00
Author
Collaborator
Copy Link

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.
clawbot commented 2026-08-09 06:57:13 +02:00
Author
Collaborator
Copy Link

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 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: :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).
## 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).
clawbot commented 2026-08-09 07:02:11 +02:00
Author
Collaborator
Copy Link

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.

## 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.
clawbot commented 2026-08-09 07:23:07 +02:00
Author
Collaborator
Copy Link

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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed; ready to merge
 needs-checks
CI / make check is not green on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR is awaiting adversarial review
 needs-rework
Review failed; PR needs changes
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/quak#2
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?