Retry/timeout follow-ups: idle deadline, reader cancellation, and classifier hardening #24

New Issue

clawbot · 2026-08-09T07:38:34+02:00

clawbot commented

2026-08-09 07:38:34 +02:00

Problem

Review of #23 (the retry policy, issue #2) produced seven findings that were deliberately kept
out of that PR to avoid a fourth review cycle on a change that was otherwise sound. Collecting
them here so they are tracked rather than lost.

1. `downloadTimeoutMs` is a whole-transfer deadline, not an idle deadline

This is the substantive one and the only behaviour regression in the set. The 600s default covers
1 GB only at a sustained ~13.7 Mbps. A large video downloading slowly but making steady progress
will now be aborted at ten minutes where it previously succeeded, and — because a timeout abort is
classified retryable — it will be retried and abort again, burning the full attempt budget without
ever completing.

An idle deadline (abort only when no bytes have arrived for N seconds) is the correct shape for a
streaming download: it kills genuinely hung connections without punishing slow ones. A
whole-transfer cap can stay as an outer bound if desired, but it must be large enough that a
realistically slow link completes.

Done when: a download that makes continuous slow progress is not aborted; a download that
stalls with no bytes arriving is aborted promptly; both are tested without any dependence on
wall-clock time; the defaults and the distinction are documented in the README.

2. `streamDecrypt` never cancels its reader on failure

When decryption throws — truncation, authentication failure, an aborted body — the
ReadableStream reader is abandoned without cancel(). The underlying socket may be held until
GC. Under runBackup, which continues past per-file failures, this leaks once per failed file
across a long run.

Done when: the reader is cancelled on every exit path, success and failure; a test asserts
cancellation happens on the failure path.

3. Three errnos in the retry sets are untested

Some members of TRANSPORT_CODES / CONNECT_CODES have no test pinning their classification, so
a future edit could silently reclassify them. Every errno the classifier names should have a test
asserting which side of the line it falls on.

4. Per-attempt deadline coverage is pinned only for `getJSON`

The requirement that each retry attempt gets a fresh AbortSignal.timeout() — rather than one
deadline spanning all attempts — is tested for getJSON only. postJSON, putJSON, putFile,
getFileStream and getThumbnailStream are unverified on this property.

Done when: every retrying entry point has a test proving attempt N+1 gets a fresh deadline.

5. `getRetryOptions()` returns its internal object by reference

A caller can mutate the client's retry configuration in place. Return a copy, or freeze it.

Definition of done

Each of the five items above is either fixed with a test that fails if the fix is reverted, or
closed with a comment explaining why it is not worth fixing. No item is silently dropped.

make check green. No test depends on wall-clock duration — sleeping is injected.

Not a 1.0.0 blocker

Deliberately outside the 1.0.0 milestone, with one caveat: item 1 should be promoted into the
milestone if quak is expected to back up large video files over ordinary domestic connections
before the tag. It is a regression against current behaviour, not merely a missing improvement.
Flagging that for a judgement call rather than deciding it here.

Context

All five were found during review of #23. See that PR's review comment for the original wording.

## Problem Review of #23 (the retry policy, issue #2) produced seven findings that were deliberately kept out of that PR to avoid a fourth review cycle on a change that was otherwise sound. Collecting them here so they are tracked rather than lost. ### 1. `downloadTimeoutMs` is a whole-transfer deadline, not an idle deadline This is the substantive one and the only behaviour regression in the set. The 600s default covers 1 GB only at a sustained ~13.7 Mbps. A large video downloading slowly but making steady progress will now be aborted at ten minutes where it previously succeeded, and — because a timeout abort is classified retryable — it will be retried and abort again, burning the full attempt budget without ever completing. An idle deadline (abort only when no bytes have arrived for N seconds) is the correct shape for a streaming download: it kills genuinely hung connections without punishing slow ones. A whole-transfer cap can stay as an outer bound if desired, but it must be large enough that a realistically slow link completes. **Done when:** a download that makes continuous slow progress is not aborted; a download that stalls with no bytes arriving is aborted promptly; both are tested without any dependence on wall-clock time; the defaults and the distinction are documented in the README. ### 2. `streamDecrypt` never cancels its reader on failure When decryption throws — truncation, authentication failure, an aborted body — the `ReadableStream` reader is abandoned without `cancel()`. The underlying socket may be held until GC. Under `runBackup`, which continues past per-file failures, this leaks once per failed file across a long run. **Done when:** the reader is cancelled on every exit path, success and failure; a test asserts cancellation happens on the failure path. ### 3. Three errnos in the retry sets are untested Some members of `TRANSPORT_CODES` / `CONNECT_CODES` have no test pinning their classification, so a future edit could silently reclassify them. Every errno the classifier names should have a test asserting which side of the line it falls on. ### 4. Per-attempt deadline coverage is pinned only for `getJSON` The requirement that each retry attempt gets a fresh `AbortSignal.timeout()` — rather than one deadline spanning all attempts — is tested for `getJSON` only. `postJSON`, `putJSON`, `putFile`, `getFileStream` and `getThumbnailStream` are unverified on this property. **Done when:** every retrying entry point has a test proving attempt N+1 gets a fresh deadline. ### 5. `getRetryOptions()` returns its internal object by reference A caller can mutate the client's retry configuration in place. Return a copy, or freeze it. ## Definition of done Each of the five items above is either fixed with a test that fails if the fix is reverted, or closed with a comment explaining why it is not worth fixing. No item is silently dropped. `make check` green. No test depends on wall-clock duration — sleeping is injected. ## Not a 1.0.0 blocker Deliberately outside the `1.0.0` milestone, with one caveat: **item 1 should be promoted into the milestone if quak is expected to back up large video files over ordinary domestic connections before the tag.** It is a regression against current behaviour, not merely a missing improvement. Flagging that for a judgement call rather than deciding it here. ## Context All five were found during review of #23. See that PR's review comment for the original wording.

clawbot self-assigned this 2026-08-09 07:38:34 +02:00

clawbot commented

2026-08-09 07:50:58 +02:00

Two more items for this issue

Surfaced during the rework of #23, after this issue was filed. Adding rather than losing them.

6. `MAX_CAUSE_DEPTH` is untested

The classifier walks the cause chain to a bounded depth when looking for an errno. That bound
has no test, so neither the depth limit nor the termination behaviour is pinned. A cyclic or
unusually deep cause chain is exactly the sort of input that produces a hang or a silent
misclassification, and neither would be caught today.

Done when: a test pins the depth limit, and a test proves a cyclic cause chain terminates
rather than looping.

7. The retried-endpoint list is stated in two places and they disagree

The call-site comment in src/api/client.ts and the README's idempotency section each enumerate
which endpoints are affected by the replay rule, and the two lists have already drifted apart.
Two hand-maintained copies of the same list is how the next inaccuracy gets introduced.

Done when: there is one authoritative statement of the rule and the other location points at
it rather than restating it.

Both fold into this issue's existing definition of done: each item is either fixed with a test
that fails when the fix is reverted, or closed with a comment explaining why it is not worth
fixing. Nothing silently dropped.

## Two more items for this issue Surfaced during the rework of #23, after this issue was filed. Adding rather than losing them. ### 6. `MAX_CAUSE_DEPTH` is untested The classifier walks the `cause` chain to a bounded depth when looking for an errno. That bound has no test, so neither the depth limit nor the termination behaviour is pinned. A cyclic or unusually deep `cause` chain is exactly the sort of input that produces a hang or a silent misclassification, and neither would be caught today. **Done when:** a test pins the depth limit, and a test proves a cyclic `cause` chain terminates rather than looping. ### 7. The retried-endpoint list is stated in two places and they disagree The call-site comment in `src/api/client.ts` and the README's idempotency section each enumerate which endpoints are affected by the replay rule, and the two lists have already drifted apart. Two hand-maintained copies of the same list is how the next inaccuracy gets introduced. **Done when:** there is one authoritative statement of the rule and the other location points at it rather than restating it. Both fold into this issue's existing definition of done: each item is either fixed with a test that fails when the fix is reverted, or closed with a comment explaining why it is not worth fixing. Nothing silently dropped.

clawbot referenced this issue

2026-08-09 08:02:33 +02:00

Retry transient network failures with exponential backoff (closes #2) #23

clawbot referenced this issue

2026-08-09 08:05:06 +02:00

Retry transient network failures with exponential backoff (closes #2) #23

clawbot commented

2026-08-09 08:05:21 +02:00

Two final items, from the third review of #23

That review passed the PR but left two more nits. Adding them so this list is complete before
anyone picks the issue up.

8. Redirects defeat the "no request byte transmitted" proof

fetch defaults to redirect: "follow". If a POST is answered with a redirect and the
connection to the redirect target is then refused, the failure surfaces as ECONNREFUSED — a
member of CONNECT_CODES — and the request is replayed. But the origin already received and
answered the first POST. The proof the replay rule rests on does not survive a redirect hop.

Narrow, and it does not apply to any endpoint quak currently calls, which is why the reviewer
recorded it rather than blocking on it. The suggested fix is redirect: "manual" on postJSON
and putJSON: quak's API surface has no legitimate redirect, so following one silently is not a
behaviour worth preserving on a non-idempotent request.

Done when: either the redirect path can no longer produce a replay, or there is a comment at
CONNECT_CODES stating the limitation explicitly, so the next reader does not over-trust the
proof.

9. `isSafeToReplay` uses `.some()` over the cause chain

src/retry.ts around lines 166-167. .some() returns true if any link in the chain carries a
connect-time errno — an optimistic reduction inside a function whose entire doctrine is "when in
doubt, do not replay". A chain that pairs a genuine connect failure with a later error indicating
the request did go out would be judged replayable.

Done when: the reduction matches the function's doctrine, or a comment explains why .some()
is correct here — with a test pinning whichever answer is chosen.

Also noted

The previous review's finding 6 never made it into this issue: POST /users/verify-email
(src/auth/login.ts:137) is state-changing but appears in neither the call-site comment nor the
README's endpoint list. That is the same drift described in item 7 above, so fixing 7 properly
should subsume it — but it needs to be on the list explicitly rather than assumed.

That brings this issue to nine items. The definition of done is unchanged: each is fixed with a
test that fails when the fix is reverted, or closed with a comment explaining why it is not worth
fixing. Nothing silently dropped.

## Two final items, from the third review of #23 That review passed the PR but left two more nits. Adding them so this list is complete before anyone picks the issue up. ### 8. Redirects defeat the "no request byte transmitted" proof `fetch` defaults to `redirect: "follow"`. If a `POST` is answered with a redirect and the connection to the *redirect target* is then refused, the failure surfaces as `ECONNREFUSED` — a member of `CONNECT_CODES` — and the request is replayed. But the origin already received and answered the first POST. The proof the replay rule rests on does not survive a redirect hop. Narrow, and it does not apply to any endpoint quak currently calls, which is why the reviewer recorded it rather than blocking on it. The suggested fix is `redirect: "manual"` on `postJSON` and `putJSON`: quak's API surface has no legitimate redirect, so following one silently is not a behaviour worth preserving on a non-idempotent request. **Done when:** either the redirect path can no longer produce a replay, or there is a comment at `CONNECT_CODES` stating the limitation explicitly, so the next reader does not over-trust the proof. ### 9. `isSafeToReplay` uses `.some()` over the cause chain `src/retry.ts` around lines 166-167. `.some()` returns true if *any* link in the chain carries a connect-time errno — an optimistic reduction inside a function whose entire doctrine is "when in doubt, do not replay". A chain that pairs a genuine connect failure with a later error indicating the request did go out would be judged replayable. **Done when:** the reduction matches the function's doctrine, or a comment explains why `.some()` is correct here — with a test pinning whichever answer is chosen. ### Also noted The previous review's finding 6 never made it into this issue: `POST /users/verify-email` (`src/auth/login.ts:137`) is state-changing but appears in neither the call-site comment nor the README's endpoint list. That is the same drift described in item 7 above, so fixing 7 properly should subsume it — but it needs to be on the list explicitly rather than assumed. That brings this issue to nine items. The definition of done is unchanged: each is fixed with a test that fails when the fix is reverted, or closed with a comment explaining why it is not worth fixing. Nothing silently dropped.

clawbot referenced this issue

2026-08-09 11:56:00 +02:00

Fix the TypeScript build: `make build` fails with TS6059 on main #3

clawbot referenced this issue

2026-08-09 11:59:09 +02:00

Fix the TypeScript build: `make build` fails with TS6059 on main #3

clawbot referenced this issue

2026-08-09 12:08:04 +02:00

Compile bin/ alongside src/ so the package can be built (closes #3) #26

clawbot referenced this issue

2026-08-09 12:23:18 +02:00

Make `make docker` green and policy-conformant #4