Atomic download writes: durability, orphan reaping, and untested failure paths #22

New Issue

clawbot · 2026-08-09T05:00:52+02:00

clawbot commented

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

Problem

#20 introduced writeAtomic in src/download/index.ts: downloads are staged in a temporary
sibling of the destination and renamed into place only after the whole stream has decrypted and
verified. That is exactly what #1 specified, and it closed the silent-corruption hole.

Three reviewers deferred a set of adjacent items rather than expand that PR's scope. Collecting
them here so they are tracked rather than lost.

No fsync, so the guarantee does not survive power loss. writeFile then rename is
atomic with respect to a crashing process, but not with respect to a crashing machine: the
rename can reach disk before the data does, leaving a destination file that is correctly named
and silently short. For a backup tool this is the same class of failure #1 existed to close,
just with a smaller window. Fixing it means syncing the temp file before the rename, and
syncing the containing directory after it.
Orphaned temp files are never reaped. Cleanup runs on the error path, but a process killed
between the write and the rename leaves a .quak-*.tmp sibling behind forever. Over many
interrupted backup runs these accumulate silently inside the backup tree.
Rename-over-existing changes semantics that nothing documents or tests. rename replaces
the destination inode rather than writing through it, so an existing destination that is a
symlink is replaced rather than followed, and the resulting file carries the temp file's mode
rather than the previous file's. Neither behaviour is wrong, but neither is stated anywhere,
and the backup-layout section of the README describes a tree built out of symlinks.
Failure paths involving the destination directory are untested. An unwritable destination
directory, and a destination directory that does not exist, are both plausible in real use and
neither has coverage.
The .quak-*.tmp naming is undocumented. The README's backup layout section enumerates
what appears in a backup tree; transient temp files should be mentioned so a user who
interrupts a run and finds one knows what it is.

Definition of done

A completed download is durable across power loss: the staged file's contents are synced
before the rename, and the containing directory is synced after it. If the cost of syncing is
judged unacceptable for the common case, make it an option and document the default and the
trade-off — but make the choice explicitly rather than by omission.
Orphaned temp files from previously killed runs are cleaned up, with a rule that cannot delete
a temp file belonging to a concurrently running download.
The symlink-replacement and file-mode consequences of rename are documented in a code
comment and, where user-visible, in the README backup layout section.
Tests cover an unwritable destination directory and a nonexistent destination directory, for
both downloadFile and downloadThumbnail.
The .quak-*.tmp name is documented in the README backup layout section.
make check green.

Not a 1.0.0 blocker

Deliberately outside the 1.0.0 milestone. The main correctness hole is closed; these harden the
edges. Promote it if the durability item turns out to matter more than it currently appears to.

Context

Deferred by review during #20. See the three review comments on that PR for the original wording.

## Problem #20 introduced `writeAtomic` in `src/download/index.ts`: downloads are staged in a temporary sibling of the destination and renamed into place only after the whole stream has decrypted and verified. That is exactly what #1 specified, and it closed the silent-corruption hole. Three reviewers deferred a set of adjacent items rather than expand that PR's scope. Collecting them here so they are tracked rather than lost. 1. **No `fsync`, so the guarantee does not survive power loss.** `writeFile` then `rename` is atomic with respect to a crashing *process*, but not with respect to a crashing *machine*: the rename can reach disk before the data does, leaving a destination file that is correctly named and silently short. For a backup tool this is the same class of failure #1 existed to close, just with a smaller window. Fixing it means syncing the temp file before the rename, and syncing the containing directory after it. 2. **Orphaned temp files are never reaped.** Cleanup runs on the error path, but a process killed between the write and the rename leaves a `.quak-*.tmp` sibling behind forever. Over many interrupted backup runs these accumulate silently inside the backup tree. 3. **Rename-over-existing changes semantics that nothing documents or tests.** `rename` replaces the destination inode rather than writing through it, so an existing destination that is a symlink is replaced rather than followed, and the resulting file carries the temp file's mode rather than the previous file's. Neither behaviour is wrong, but neither is stated anywhere, and the backup-layout section of the README describes a tree built out of symlinks. 4. **Failure paths involving the destination directory are untested.** An unwritable destination directory, and a destination directory that does not exist, are both plausible in real use and neither has coverage. 5. **The `.quak-*.tmp` naming is undocumented.** The README's backup layout section enumerates what appears in a backup tree; transient temp files should be mentioned so a user who interrupts a run and finds one knows what it is. ## Definition of done 1. A completed download is durable across power loss: the staged file's contents are synced before the rename, and the containing directory is synced after it. If the cost of syncing is judged unacceptable for the common case, make it an option and document the default and the trade-off — but make the choice explicitly rather than by omission. 2. Orphaned temp files from previously killed runs are cleaned up, with a rule that cannot delete a temp file belonging to a concurrently running download. 3. The symlink-replacement and file-mode consequences of `rename` are documented in a code comment and, where user-visible, in the README backup layout section. 4. Tests cover an unwritable destination directory and a nonexistent destination directory, for both `downloadFile` and `downloadThumbnail`. 5. The `.quak-*.tmp` name is documented in the README backup layout section. 6. `make check` green. ## Not a 1.0.0 blocker Deliberately outside the `1.0.0` milestone. The main correctness hole is closed; these harden the edges. Promote it if the durability item turns out to matter more than it currently appears to. ## Context Deferred by review during #20. See the three review comments on that PR for the original wording.

clawbot self-assigned this 2026-08-09 05:00:52 +02:00

clawbot referenced this issue

2026-08-09 05:01:11 +02:00

Verify secretstream TAG_FINAL and write downloads atomically (closes #1) #20

clawbot referenced this issue

2026-08-09 06:57:13 +02:00

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

clawbot referenced this issue

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

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

clawbot referenced this issue

2026-08-09 07:36:26 +02:00