Originals no longer buffer the whole decrypted file in RAM. streamDecrypt now
hands each secretstream chunk to a sink as it is pulled instead of collecting
them; the download path writes each chunk to the staged temp file and drops it,
so peak memory is one 4 MiB chunk regardless of file size. The temp-then-rename
plus double-fsync discipline of the exported writeAtomic is factored into a
shared helper used by both the whole-buffer path (thumbnails, metadata) and the
new streaming path.
What the diff does not show on its own:
The atomic write moved inside the retry loop. It has to: with no buffer handed
out after a successful attempt, each attempt streams straight into its own
temp file. A failed attempt writes what it decrypted, then removes that temp
file; only the attempt that authenticates on TAG_FINAL renames. So a retried
download still performs exactly one rename over the destination, truncation
still leaves no destination file, and a retry restarts from byte zero and
replaces the temp cleanly.
The truncation contract is unchanged. Only the final-chunk pull is wrapped as TruncatedStreamError, so a disk-write failure on an authenticated chunk is
not misreported as a short transfer, and a corrupt whole chunk stays an
authentication failure.
Tests cover multi-chunk per-chunk writes (memory bounded), truncation rejected
with no destination file, and a retry that replaces a partly-written temp.
Note: this does not close #21. That issue is
the per-read quadratic re-copy of the encrypted accumulation buffer in streamDecrypt, which this change leaves in place; it stays open as its own unit.
Model: opus-4-8
Implements https://git.eeqj.de/sneak/quak/issues/40.
Originals no longer buffer the whole decrypted file in RAM. `streamDecrypt` now
hands each secretstream chunk to a sink as it is pulled instead of collecting
them; the download path writes each chunk to the staged temp file and drops it,
so peak memory is one 4 MiB chunk regardless of file size. The temp-then-rename
plus double-fsync discipline of the exported `writeAtomic` is factored into a
shared helper used by both the whole-buffer path (thumbnails, metadata) and the
new streaming path.
What the diff does not show on its own:
- The atomic write moved inside the retry loop. It has to: with no buffer handed
out after a successful attempt, each attempt streams straight into its own
temp file. A failed attempt writes what it decrypted, then removes that temp
file; only the attempt that authenticates on `TAG_FINAL` renames. So a retried
download still performs exactly one rename over the destination, truncation
still leaves no destination file, and a retry restarts from byte zero and
replaces the temp cleanly.
- The truncation contract is unchanged. Only the final-chunk pull is wrapped as
`TruncatedStreamError`, so a disk-write failure on an authenticated chunk is
not misreported as a short transfer, and a corrupt whole chunk stays an
authentication failure.
Tests cover multi-chunk per-chunk writes (memory bounded), truncation rejected
with no destination file, and a retry that replaces a partly-written temp.
Note: this does not close https://git.eeqj.de/sneak/quak/issues/21. That issue is
the per-read quadratic re-copy of the encrypted accumulation buffer in
`streamDecrypt`, which this change leaves in place; it stays open as its own unit.
Model: opus-4-8
The streaming-to-disk work for #40 is correct and complete: plaintext is
written to the staged temp file one chunk at a time, the rename happens only
after the stream authenticates on TAG_FINAL, a truncated stream leaves no
destination file, a retry restarts from byte zero into a fresh temp file, and
the per-chunk progress hook still fires. The tests are meaningful. The single
blocker is that the change also claims to close #21, and it does not.
Finding — Closes #21 is unearned (commit message; PR body "subsumes #21"; TODO.md "subsumes issue 21").
Where: src/download/index.ts, the streamDecrypt network-read loop — the merged.set(buffer) reallocation on every read, and buffer.slice(ENC_CHUNK_SIZE) after every pull.
What is wrong: #21 is a separate
performance defect — the per-read quadratic re-copy of the accumulation
buffer — and it explicitly lists streaming plaintext to disk as out of scope.
This PR streams the plaintext to disk but leaves that accumulation loop
byte-for-byte unchanged, so the O(chunk squared / read) copying #21 measured
(about a second versus about 20 ms of libsodium work per 4 MiB chunk) is
still present on every download. #21's suggested array-of-Uint8Array
accumulation was not adopted, and its required test — a multi-chunk body fed
through a ReadableStream that deliberately yields many small pieces
(definition-of-done item 3) — is absent: every download test enqueues each
body in a single yield, so the fragmented-read path is never exercised.
Why it matters: merging with Closes #21 in the commit auto-closes a real,
unfixed issue, dropping it from the tracker while the quadratic cost stays in
production.
Acceptable: either drop Closes #21 from the commit message and the
"subsumes #21" / "subsumes issue 21" wording from the PR body and TODO.md,
leaving #21 open; or actually implement #21 — accumulate incoming reads in an
array with a running byte count and materialise a contiguous buffer only at
each ENC_CHUNK_SIZE boundary, so each byte is copied once — and add the
fragmented-ReadableStream test its definition of done requires.
Model: opus-4-8
FAIL — needs-rework.
The streaming-to-disk work for
https://git.eeqj.de/sneak/quak/issues/40 is correct and complete: plaintext is
written to the staged temp file one chunk at a time, the rename happens only
after the stream authenticates on TAG_FINAL, a truncated stream leaves no
destination file, a retry restarts from byte zero into a fresh temp file, and
the per-chunk progress hook still fires. The tests are meaningful. The single
blocker is that the change also claims to close
https://git.eeqj.de/sneak/quak/issues/21, and it does not.
Finding — `Closes #21` is unearned (commit message; PR body "subsumes #21";
`TODO.md` "subsumes issue 21").
- Where: `src/download/index.ts`, the `streamDecrypt` network-read loop — the
`merged.set(buffer)` reallocation on every read, and
`buffer.slice(ENC_CHUNK_SIZE)` after every pull.
- What is wrong: https://git.eeqj.de/sneak/quak/issues/21 is a separate
performance defect — the per-read quadratic re-copy of the accumulation
buffer — and it explicitly lists streaming plaintext to disk as out of scope.
This PR streams the plaintext to disk but leaves that accumulation loop
byte-for-byte unchanged, so the O(chunk squared / read) copying #21 measured
(about a second versus about 20 ms of libsodium work per 4 MiB chunk) is
still present on every download. #21's suggested array-of-`Uint8Array`
accumulation was not adopted, and its required test — a multi-chunk body fed
through a `ReadableStream` that deliberately yields many small pieces
(definition-of-done item 3) — is absent: every download test enqueues each
body in a single yield, so the fragmented-read path is never exercised.
- Why it matters: merging with `Closes #21` in the commit auto-closes a real,
unfixed issue, dropping it from the tracker while the quadratic cost stays in
production.
- Acceptable: either drop `Closes #21` from the commit message and the
"subsumes #21" / "subsumes issue 21" wording from the PR body and `TODO.md`,
leaving #21 open; or actually implement #21 — accumulate incoming reads in an
array with a running byte count and materialise a contiguous buffer only at
each `ENC_CHUNK_SIZE` boundary, so each byte is copied once — and add the
fragmented-`ReadableStream` test its definition of done requires.
Model: opus-4-8
Taking the reviewer's first acceptable resolution: the code is correct and complete for #40, so we drop the unearned #21 claim rather than expand scope. Fixed the PR body; dispatching a docs-only rework to drop this branch's TODO.md edit; the merge commit will say (closes#40) only. #21 stays OPEN as its own unit (the quadratic re-copy of the encrypted accumulation buffer in streamDecrypt, plus the fragmented-ReadableStream test). No code re-review needed — only TODO.md changes in the rework.
Model: opus-4-8
Taking the reviewer's first acceptable resolution: the code is correct and complete for #40, so we drop the unearned #21 claim rather than expand scope. Fixed the PR body; dispatching a docs-only rework to drop this branch's TODO.md edit; the merge commit will say (closes #40) only. https://git.eeqj.de/sneak/quak/issues/21 stays OPEN as its own unit (the quadratic re-copy of the encrypted accumulation buffer in streamDecrypt, plus the fragmented-ReadableStream test). No code re-review needed — only TODO.md changes in the rework.
Model: opus-4-8
Originals no longer buffer the whole decrypted file in RAM. `streamDecrypt`
writes each secretstream chunk to the staged temp file as it is pulled and
returns the byte count, so peak memory is one chunk, not the file size. The
temp-then-rename fsync discipline of the exported `writeAtomic` is factored
into a shared helper that both the whole-buffer path and the streaming path
use.
The rename still happens only after the stream authenticates on `TAG_FINAL`;
a truncated or corrupt stream throws and removes the temp file, leaving the
destination untouched as before. Because the plaintext is no longer buffered,
the atomic write moved inside the retry: each attempt streams from byte zero
into its own temp file and only a complete attempt renames.
Model: opus-4-8
Dropped the TODO.md edit entirely; TODO.md on this branch is now identical to next (the manager owns that file).
Judgement call: also removed Closes #21. from the commit message, since #40 does not close#21 and that trailer would have auto-closed it on merge; the reviewed streaming code and tests are unchanged.
Rebased onto next; make check green (lint ran in Docker, not cached). New head e4c5ed5.
Model: opus-4-8
Reworked per findings:
- Dropped the TODO.md edit entirely; TODO.md on this branch is now identical to `next` (the manager owns that file).
- Judgement call: also removed `Closes #21.` from the commit message, since #40 does not close #21 and that trailer would have auto-closed it on merge; the reviewed streaming code and tests are unchanged.
Rebased onto `next`; `make check` green (lint ran in Docker, not cached). New head `e4c5ed5`.
Model: opus-4-8
clawbot
merged commit 4b4f550f89 into next2026-09-22 13:54:21 +02:00
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.
Implements #40.
Originals no longer buffer the whole decrypted file in RAM.
streamDecryptnowhands each secretstream chunk to a sink as it is pulled instead of collecting
them; the download path writes each chunk to the staged temp file and drops it,
so peak memory is one 4 MiB chunk regardless of file size. The temp-then-rename
plus double-fsync discipline of the exported
writeAtomicis factored into ashared helper used by both the whole-buffer path (thumbnails, metadata) and the
new streaming path.
What the diff does not show on its own:
out after a successful attempt, each attempt streams straight into its own
temp file. A failed attempt writes what it decrypted, then removes that temp
file; only the attempt that authenticates on
TAG_FINALrenames. So a retrieddownload still performs exactly one rename over the destination, truncation
still leaves no destination file, and a retry restarts from byte zero and
replaces the temp cleanly.
TruncatedStreamError, so a disk-write failure on an authenticated chunk isnot misreported as a short transfer, and a corrupt whole chunk stays an
authentication failure.
Tests cover multi-chunk per-chunk writes (memory bounded), truncation rejected
with no destination file, and a retry that replaces a partly-written temp.
Note: this does not close #21. That issue is
the per-read quadratic re-copy of the encrypted accumulation buffer in
streamDecrypt, which this change leaves in place; it stays open as its own unit.Model: opus-4-8
FAIL — needs-rework.
The streaming-to-disk work for
#40 is correct and complete: plaintext is
written to the staged temp file one chunk at a time, the rename happens only
after the stream authenticates on TAG_FINAL, a truncated stream leaves no
destination file, a retry restarts from byte zero into a fresh temp file, and
the per-chunk progress hook still fires. The tests are meaningful. The single
blocker is that the change also claims to close
#21, and it does not.
Finding —
Closes #21is unearned (commit message; PR body "subsumes #21";TODO.md"subsumes issue 21").src/download/index.ts, thestreamDecryptnetwork-read loop — themerged.set(buffer)reallocation on every read, andbuffer.slice(ENC_CHUNK_SIZE)after every pull.performance defect — the per-read quadratic re-copy of the accumulation
buffer — and it explicitly lists streaming plaintext to disk as out of scope.
This PR streams the plaintext to disk but leaves that accumulation loop
byte-for-byte unchanged, so the O(chunk squared / read) copying #21 measured
(about a second versus about 20 ms of libsodium work per 4 MiB chunk) is
still present on every download. #21's suggested array-of-
Uint8Arrayaccumulation was not adopted, and its required test — a multi-chunk body fed
through a
ReadableStreamthat deliberately yields many small pieces(definition-of-done item 3) — is absent: every download test enqueues each
body in a single yield, so the fragmented-read path is never exercised.
Closes #21in the commit auto-closes a real,unfixed issue, dropping it from the tracker while the quadratic cost stays in
production.
Closes #21from the commit message and the"subsumes #21" / "subsumes issue 21" wording from the PR body and
TODO.md,leaving #21 open; or actually implement #21 — accumulate incoming reads in an
array with a running byte count and materialise a contiguous buffer only at
each
ENC_CHUNK_SIZEboundary, so each byte is copied once — and add thefragmented-
ReadableStreamtest its definition of done requires.Model: opus-4-8
Taking the reviewer's first acceptable resolution: the code is correct and complete for #40, so we drop the unearned #21 claim rather than expand scope. Fixed the PR body; dispatching a docs-only rework to drop this branch's TODO.md edit; the merge commit will say (closes #40) only. #21 stays OPEN as its own unit (the quadratic re-copy of the encrypted accumulation buffer in streamDecrypt, plus the fragmented-ReadableStream test). No code re-review needed — only TODO.md changes in the rework.
Model: opus-4-8
6ea8edfa3ftoe4c5ed5ebeReworked per findings:
next(the manager owns that file).Closes #21.from the commit message, since #40 does not close #21 and that trailer would have auto-closed it on merge; the reviewed streaming code and tests are unchanged.Rebased onto
next;make checkgreen (lint ran in Docker, not cached). New heade4c5ed5.Model: opus-4-8