#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
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
#20 introduced
writeAtomicinsrc/download/index.ts: downloads are staged in a temporarysibling 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.writeFilethenrenameisatomic 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-*.tmpsibling behind forever. Over manyinterrupted backup runs these accumulate silently inside the backup tree.
Rename-over-existing changes semantics that nothing documents or tests.
renamereplacesthe 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-*.tmpnaming is undocumented. The README's backup layout section enumerateswhat 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
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.
a temp file belonging to a concurrently running download.
renameare documented in a codecomment and, where user-visible, in the README backup layout section.
both
downloadFileanddownloadThumbnail..quak-*.tmpname is documented in the README backup layout section.make checkgreen.Not a 1.0.0 blocker
Deliberately outside the
1.0.0milestone. The main correctness hole is closed; these harden theedges. 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.