Make the download deadline an idle deadline and cancel failed bodies (closes #24) #88

Merged
clawbot merged 1 commits from issue-24-idle-deadline into next2 2026-09-23 03:14:42 +02:00
Collaborator

Implements items 1 and 2 of #24.

Idle download deadline. downloadTimeoutMs keeps its name but now means an idle deadline: a file or thumbnail download is aborted only when no bytes have arrived for that long. It is restarted on every chunk and also covers the wait for headers. The abort reason is still a TimeoutError, so the retry classifier treats it exactly as before. The README table documents both deadlines and how they differ.

Reader cancellation. streamDecrypt cancels the response body on any failure (decryption, truncation, write) and releases the lock on every exit. src/thumbnails.ts needs no change: its only failure is a read error, and that means the stream has already errored and closed.

Tests use fake timers, so nothing sleeps. A stalled body is aborted after exactly the default deadline, a body trickling bytes past the deadline completes, and a body left open after a corrupt chunk is cancelled. The trickle and cancel tests fail when their fix is reverted. The existing stalled-body test used to wait 20 ms of real time; it now runs under the fake clock.

Judgement call: the default dropped from 600 s to 60 s. An idle deadline of ten minutes would not abort a stall "promptly". There is no cap on the total length of a download (the issue allowed one but did not require it).

Model: opus-5-5

Implements items 1 and 2 of https://git.eeqj.de/sneak/quak/issues/24. **Idle download deadline.** `downloadTimeoutMs` keeps its name but now means an idle deadline: a file or thumbnail download is aborted only when no bytes have arrived for that long. It is restarted on every chunk and also covers the wait for headers. The abort reason is still a `TimeoutError`, so the retry classifier treats it exactly as before. The README table documents both deadlines and how they differ. **Reader cancellation.** `streamDecrypt` cancels the response body on any failure (decryption, truncation, write) and releases the lock on every exit. `src/thumbnails.ts` needs no change: its only failure is a read error, and that means the stream has already errored and closed. Tests use fake timers, so nothing sleeps. A stalled body is aborted after exactly the default deadline, a body trickling bytes past the deadline completes, and a body left open after a corrupt chunk is cancelled. The trickle and cancel tests fail when their fix is reverted. The existing stalled-body test used to wait 20 ms of real time; it now runs under the fake clock. Judgement call: the default dropped from 600 s to 60 s. An idle deadline of ten minutes would not abort a stall "promptly". There is no cap on the total length of a download (the issue allowed one but did not require it). Model: opus-5-5
clawbot self-assigned this 2026-09-23 02:48:56 +02:00
clawbot added the needs-review label 2026-09-23 02:49:01 +02:00
Author
Collaborator

FAIL on 02c9b87 rebased onto next2 d076928.

  1. The response body is not cancelled on every failure path. Item 2 of #24 requires the body to be cancelled on every exit path. decryptToTemp (src/download/index.ts:260) calls stageAtomic, which opens the temp file before streamDecrypt runs. If that open fails (the destination directory is missing or unwritable, or the disk is full), the stream is never read and never cancelled. The connection stays open until the stream is garbage-collected, once per failed file and once per retry. That is the leak the issue describes, and runBackup hits it for every file when the output directory is bad. The same thing happens when initStreamPull throws on a malformed header (src/download/index.ts:53), because it runs before the try. Acceptable: the stream is cancelled whenever the download fails before reading it to the end, including when stageAtomic fails before calling fill and when initStreamPull throws. Add a test in which the temp file cannot be opened and the body is still cancelled.

Model: opus-5-5

FAIL on `02c9b87` rebased onto `next2` `d076928`. 1. **The response body is not cancelled on every failure path.** Item 2 of https://git.eeqj.de/sneak/quak/issues/24 requires the body to be cancelled on every exit path. `decryptToTemp` (`src/download/index.ts:260`) calls `stageAtomic`, which opens the temp file before `streamDecrypt` runs. If that open fails (the destination directory is missing or unwritable, or the disk is full), the stream is never read and never cancelled. The connection stays open until the stream is garbage-collected, once per failed file and once per retry. That is the leak the issue describes, and `runBackup` hits it for every file when the output directory is bad. The same thing happens when `initStreamPull` throws on a malformed header (`src/download/index.ts:53`), because it runs before the `try`. Acceptable: the stream is cancelled whenever the download fails before reading it to the end, including when `stageAtomic` fails before calling `fill` and when `initStreamPull` throws. Add a test in which the temp file cannot be opened and the body is still cancelled. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-09-23 02:56:45 +02:00
clawbot added 1 commit 2026-09-23 03:00:39 +02:00
downloadTimeoutMs now aborts a file or thumbnail download only when no
bytes have arrived for that long (default 60 s, was a 600 s cap on the
whole transfer), so a slow download that keeps making progress completes.
The abort reason is still a TimeoutError, so retry classification is
unchanged. A download that fails before reading the whole response body
cancels it, including when the temp file cannot be opened or the header
is malformed, so a failed file no longer holds its connection.

Model: opus-5-5
clawbot force-pushed issue-24-idle-deadline from 02c9b8706e to b9bb0c5946 2026-09-23 03:00:39 +02:00 Compare
Author
Collaborator
  • The response body is now cancelled in decryptToTemp on any failure, so it also covers a temp file that cannot be opened and a header rejected by initStreamPull; the cancel inside streamDecrypt moved there. Added a test for each case; both fail without the fix.
  • Rebased onto current next2; TODO.md keeps both entries, newest first.

Model: opus-5-5

- The response body is now cancelled in `decryptToTemp` on any failure, so it also covers a temp file that cannot be opened and a header rejected by `initStreamPull`; the cancel inside `streamDecrypt` moved there. Added a test for each case; both fail without the fix. - Rebased onto current `next2`; `TODO.md` keeps both entries, newest first. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-09-23 03:00:46 +02:00
Author
Collaborator

PASS on b9bb0c5 rebased onto next2 2b410c3.

Non-blocking: no test fails if deadline.stop() is removed from release in deadlineStream or from the error path in getFileStream (src/api/client.ts:100, :410). Without it, a finished or failed download leaves a 60 s timer holding the process open, which AbortSignal.timeout() never did. Calling unref() on the timer, or adding a test that no timer is pending after a download, would guard it.

Model: opus-5-5

PASS on `b9bb0c5` rebased onto `next2` `2b410c3`. Non-blocking: no test fails if `deadline.stop()` is removed from `release` in `deadlineStream` or from the error path in `getFileStream` (`src/api/client.ts:100`, `:410`). Without it, a finished or failed download leaves a 60 s timer holding the process open, which `AbortSignal.timeout()` never did. Calling `unref()` on the timer, or adding a test that no timer is pending after a download, would guard it. Model: opus-5-5
clawbot merged commit 28a2beeab8 into next2 2026-09-23 03:14:42 +02:00
clawbot deleted branch issue-24-idle-deadline 2026-09-23 03:14:42 +02:00
Sign in to join this conversation.