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
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.
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.
Scope narrowed: items 3 to 9 (retry classifier, deadlines per attempt, redirects) moved to #80. This issue keeps item 1 (an idle deadline for downloads instead of a whole-transfer deadline) and item 2 (cancel the stream reader on every exit path). It runs after #78 and #22, which change the same download code.
Model: opus-5-5
Scope narrowed: items 3 to 9 (retry classifier, deadlines per attempt, redirects) moved to https://git.eeqj.de/sneak/quak/issues/80. This issue keeps item 1 (an idle deadline for downloads instead of a whole-transfer deadline) and item 2 (cancel the stream reader on every exit path). It runs after https://git.eeqj.de/sneak/quak/pulls/78 and https://git.eeqj.de/sneak/quak/issues/22, which change the same download code.
Model: opus-5-5
Item 1: idle deadline.src/api/client.ts:355 still applies AbortSignal.timeout(this.downloadTimeoutMs), with a default of 600 s (DEFAULT_DOWNLOAD_TIMEOUT_MS, line 29), over the whole body transfer. The wrapper deadlineStream (around line 57) already races every read against a signal, so that is the place for the change. The deadline restarts whenever a chunk arrives, and it aborts only when no bytes have arrived for that long. Either rename the option to say what it now means, or keep downloadTimeoutMs with its meaning documented; pick one and state it in the PR body. Tests use an injected clock or timer, never real sleeps:
A body that trickles bytes steadily for longer than the deadline completes.
A body that stalls is aborted, and the abort is classified exactly as today.
Update the README table at README.md:366-372.
Item 2: cancel the reader.streamDecrypt in src/download/index.ts (the reader from line 54) never calls reader.cancel() when decryption, truncation or the sink throws. Cancel it on every failure path. On success, release the lock or cancel it. A test checks that the source stream is cancelled when decryption fails. src/thumbnails.ts:65 reads a stream the same way. Give it the same treatment if an error there can leave the reader open.
make check green; TODO.md updated in the same commit.
Model: opus-5-5
## Implementer brief for items 1 and 2 (branch `next2`)
Items 3 to 9 are done in https://git.eeqj.de/sneak/quak/issues/80.
**Item 1: idle deadline.** `src/api/client.ts:355` still applies `AbortSignal.timeout(this.downloadTimeoutMs)`, with a default of 600 s (`DEFAULT_DOWNLOAD_TIMEOUT_MS`, line 29), over the whole body transfer. The wrapper `deadlineStream` (around line 57) already races every read against a signal, so that is the place for the change. The deadline restarts whenever a chunk arrives, and it aborts only when no bytes have arrived for that long. Either rename the option to say what it now means, or keep `downloadTimeoutMs` with its meaning documented; pick one and state it in the PR body. Tests use an injected clock or timer, never real sleeps:
- A body that trickles bytes steadily for longer than the deadline completes.
- A body that stalls is aborted, and the abort is classified exactly as today.
Update the README table at `README.md:366-372`.
**Item 2: cancel the reader.** `streamDecrypt` in `src/download/index.ts` (the reader from line 54) never calls `reader.cancel()` when decryption, truncation or the sink throws. Cancel it on every failure path. On success, release the lock or cancel it. A test checks that the source stream is cancelled when decryption fails. `src/thumbnails.ts:65` reads a stream the same way. Give it the same treatment if an error there can leave the reader open.
`make check` green; `TODO.md` updated in the same commit.
Model: opus-5-5
Items 1 and 2 are in #88: downloadTimeoutMs is now an idle deadline (default 60 s, was a 600 s cap on the whole transfer), and streamDecrypt cancels the response body on failure.
Model: opus-5-5
Items 1 and 2 are in https://git.eeqj.de/sneak/quak/pulls/88: `downloadTimeoutMs` is now an idle deadline (default 60 s, was a 600 s cap on the whole transfer), and `streamDecrypt` cancels the response body on failure.
Model: opus-5-5
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
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.
downloadTimeoutMsis a whole-transfer deadline, not an idle deadlineThis 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.
streamDecryptnever cancels its reader on failureWhen decryption throws — truncation, authentication failure, an aborted body — the
ReadableStreamreader is abandoned withoutcancel(). The underlying socket may be held untilGC. Under
runBackup, which continues past per-file failures, this leaks once per failed fileacross 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_CODEShave no test pinning their classification, soa 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
getJSONThe requirement that each retry attempt gets a fresh
AbortSignal.timeout()— rather than onedeadline spanning all attempts — is tested for
getJSONonly.postJSON,putJSON,putFile,getFileStreamandgetThumbnailStreamare 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 referenceA 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 checkgreen. No test depends on wall-clock duration — sleeping is injected.Not a 1.0.0 blocker
Deliberately outside the
1.0.0milestone, with one caveat: item 1 should be promoted into themilestone 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.
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_DEPTHis untestedThe classifier walks the
causechain to a bounded depth when looking for an errno. That boundhas no test, so neither the depth limit nor the termination behaviour is pinned. A cyclic or
unusually deep
causechain is exactly the sort of input that produces a hang or a silentmisclassification, and neither would be caught today.
Done when: a test pins the depth limit, and a test proves a cyclic
causechain terminatesrather than looping.
7. The retried-endpoint list is stated in two places and they disagree
The call-site comment in
src/api/client.tsand the README's idempotency section each enumeratewhich 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 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
fetchdefaults toredirect: "follow". If aPOSTis answered with a redirect and theconnection to the redirect target is then refused, the failure surfaces as
ECONNREFUSED— amember of
CONNECT_CODES— and the request is replayed. But the origin already received andanswered 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"onpostJSONand
putJSON: quak's API surface has no legitimate redirect, so following one silently is not abehaviour 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_CODESstating the limitation explicitly, so the next reader does not over-trust theproof.
9.
isSafeToReplayuses.some()over the cause chainsrc/retry.tsaround lines 166-167..some()returns true if any link in the chain carries aconnect-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 theREADME'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 issue2026-09-23 00:30:14 +02:00
Scope narrowed: items 3 to 9 (retry classifier, deadlines per attempt, redirects) moved to #80. This issue keeps item 1 (an idle deadline for downloads instead of a whole-transfer deadline) and item 2 (cancel the stream reader on every exit path). It runs after #78 and #22, which change the same download code.
Model: opus-5-5
Implementer brief for items 1 and 2 (branch
next2)Items 3 to 9 are done in #80.
Item 1: idle deadline.
src/api/client.ts:355still appliesAbortSignal.timeout(this.downloadTimeoutMs), with a default of 600 s (DEFAULT_DOWNLOAD_TIMEOUT_MS, line 29), over the whole body transfer. The wrapperdeadlineStream(around line 57) already races every read against a signal, so that is the place for the change. The deadline restarts whenever a chunk arrives, and it aborts only when no bytes have arrived for that long. Either rename the option to say what it now means, or keepdownloadTimeoutMswith its meaning documented; pick one and state it in the PR body. Tests use an injected clock or timer, never real sleeps:Update the README table at
README.md:366-372.Item 2: cancel the reader.
streamDecryptinsrc/download/index.ts(the reader from line 54) never callsreader.cancel()when decryption, truncation or the sink throws. Cancel it on every failure path. On success, release the lock or cancel it. A test checks that the source stream is cancelled when decryption fails.src/thumbnails.ts:65reads a stream the same way. Give it the same treatment if an error there can leave the reader open.make checkgreen;TODO.mdupdated in the same commit.Model: opus-5-5
Items 1 and 2 are in #88:
downloadTimeoutMsis now an idle deadline (default 60 s, was a 600 s cap on the whole transfer), andstreamDecryptcancels the response body on failure.Model: opus-5-5