#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
Already done on next2 by #39: the download writer in src/download/index.ts:169-202 syncs the temp file before the rename and the directory after it. src/library/content.ts cleans up leftover .quak-*.tmp files in the content cache when the library opens. Do not redo either.
What is left:
The backup tree's copyAtomic in src/backup.ts:157 copies to a .quak-backup-<name>-<pid>-<random>.tmp file and renames it, with no fsync. Give it the same sync-before-rename and sync-directory-after as the download writer, reusing that code rather than copying it.
At the start of a backup run, delete leftover backup temp files in the backup tree. The name already carries the process ID. Delete only files whose process is no longer running, so a backup running at the same time is never touched. Test both cases.
A code comment at the rename site, and a line in the README backup layout section, covering what rename does to an existing destination: a symlink is replaced rather than followed, and the new file carries the temp file's permissions.
Tests for a destination directory that cannot be written and one that does not exist, for both downloadFile and downloadThumbnail. Check first whether test/download/download.test.ts around lines 802 and 959 already covers any of these, and add only what is missing.
README backup layout: name the temp files a user may find after an interrupted run and say what they are.
make check green; TODO.md updated in the same commit.
Model: opus-5-5
## Implementer brief (branch `next2`)
Already done on `next2` by https://git.eeqj.de/sneak/quak/issues/39: the download writer in `src/download/index.ts:169-202` syncs the temp file before the rename and the directory after it. `src/library/content.ts` cleans up leftover `.quak-*.tmp` files in the content cache when the library opens. Do not redo either.
What is left:
1. The backup tree's `copyAtomic` in `src/backup.ts:157` copies to a `.quak-backup-<name>-<pid>-<random>.tmp` file and renames it, with no fsync. Give it the same sync-before-rename and sync-directory-after as the download writer, reusing that code rather than copying it.
2. At the start of a backup run, delete leftover backup temp files in the backup tree. The name already carries the process ID. Delete only files whose process is no longer running, so a backup running at the same time is never touched. Test both cases.
3. A code comment at the rename site, and a line in the README backup layout section, covering what rename does to an existing destination: a symlink is replaced rather than followed, and the new file carries the temp file's permissions.
4. Tests for a destination directory that cannot be written and one that does not exist, for both `downloadFile` and `downloadThumbnail`. Check first whether `test/download/download.test.ts` around lines 802 and 959 already covers any of these, and add only what is missing.
5. README backup layout: name the temp files a user may find after an interrupted run and say what they are.
`make check` green; `TODO.md` updated in the same commit.
Model: opus-5-5
Implemented in #85: the backup copy now syncs the temp file before the rename and the directory after it; each backup run deletes leftover backup temp files whose process is no longer running; the rename behaviour and the temp file names are documented; and there are tests for a missing and an unwritable destination directory.
Model: opus-5-5
Implemented in https://git.eeqj.de/sneak/quak/pulls/85: the backup copy now syncs the temp file before the rename and the directory after it; each backup run deletes leftover backup temp files whose process is no longer running; the rename behaviour and the temp file names are documented; and there are tests for a missing and an unwritable destination directory.
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
#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.
clawbot referenced this issue2026-09-23 00:30:14 +02:00
Implementer brief (branch
next2)Already done on
next2by #39: the download writer insrc/download/index.ts:169-202syncs the temp file before the rename and the directory after it.src/library/content.tscleans up leftover.quak-*.tmpfiles in the content cache when the library opens. Do not redo either.What is left:
copyAtomicinsrc/backup.ts:157copies to a.quak-backup-<name>-<pid>-<random>.tmpfile and renames it, with no fsync. Give it the same sync-before-rename and sync-directory-after as the download writer, reusing that code rather than copying it.downloadFileanddownloadThumbnail. Check first whethertest/download/download.test.tsaround lines 802 and 959 already covers any of these, and add only what is missing.make checkgreen;TODO.mdupdated in the same commit.Model: opus-5-5
Implemented in #85: the backup copy now syncs the temp file before the rename and the directory after it; each backup run deletes leftover backup temp files whose process is no longer running; the rename behaviour and the temp file names are documented; and there are tests for a missing and an unwritable destination directory.
Model: opus-5-5