Under --skip-errors a packing failure can record a file that cannot be restored #161

New Issue

2026-09-22T00:55:14+02:00

clawbot commented

2026-09-22 00:55:14 +02:00

Found in passing by the security review #73; outside its seven scope items. Severity: medium, reliability (a completed snapshot holds an unrestorable file; not reachable by the destination operator).

What is wrong

In processFileStreaming (internal/snapshot/scanner.go:1691-1715) a new chunk is added to the known set, the scanner's pending set and the packer's pending-row list before addChunkToPacker runs. With --skip-errors, scanner.go:1307-1315 skips the file on any error, not only read errors (the comment at :66 says "file read errors"; README.md:119 says "per-file errors").

If starting a new blob fails once (temp file cannot be created, the age header cannot be written because the temp filesystem is full, or the blob row insert fails; internal/blob/packer.go:199-203, :325-342) and a later blob in the same run succeeds, that later finalize inserts the lost chunk's row into chunks with no blob_chunks row (packer.go:427, :552-560). Any later file containing the chunk is recorded in a snapshot that completes normally, and restore fails with "chunk not found in any blob". Because loadKnownChunks reads the chunks table, later runs treat the chunk as already stored.

Upload and write failures do not complete a snapshot: the packer keeps the closed writer and the final Flush fails the run.

Acceptable

An error returned by addChunkToPacker (packer, database, encryption, upload) aborts the run regardless of --skip-errors. Only open and read errors are skipped.
README.md:119, internal/cli/root.go:60 and scanner.go:66 agree on that wording.
Do not fix this by reordering the known/pending bookkeeping: AddPendingChunk and addPendingChunkHash must come before packing, because a finalize triggered inside addChunkToPacker inserts and un-pends the triggering chunk.
Readability only: packer.go:599-602 sets currentBlob to nil after cleanupTempFile.

Definition of done

Test: inject a one-time temp-file creation failure under --skip-errors; the run fails (no snapshot completes with a file whose chunk is in no blob).
Test: a file read error under --skip-errors is still skipped and the run completes.
No existing assertion weakened; make check green.

Related: #148 is the same family (chunk rows that outlive data never stored) with a different trigger, an interrupted upload followed by a re-run. Its repair-at-start option may also clear the state described here on the next run, but it does not stop --skip-errors from swallowing packer errors within a run; both are needed. This cannot go into #72, whose definition of done forbids production changes.

Line numbers are as of next at 6fcd8e1.

model: fable-5-1

Found in passing by the security review https://git.eeqj.de/sneak/vaultik/issues/73; outside its seven scope items. Severity: **medium**, reliability (a completed snapshot holds an unrestorable file; not reachable by the destination operator). ## What is wrong In `processFileStreaming` (`internal/snapshot/scanner.go:1691-1715`) a new chunk is added to the known set, the scanner's pending set and the packer's pending-row list before `addChunkToPacker` runs. With `--skip-errors`, `scanner.go:1307-1315` skips the file on any error, not only read errors (the comment at `:66` says "file read errors"; `README.md:119` says "per-file errors"). If starting a new blob fails once (temp file cannot be created, the age header cannot be written because the temp filesystem is full, or the blob row insert fails; `internal/blob/packer.go:199-203`, `:325-342`) and a later blob in the same run succeeds, that later finalize inserts the lost chunk's row into `chunks` with no `blob_chunks` row (`packer.go:427`, `:552-560`). Any later file containing the chunk is recorded in a snapshot that completes normally, and restore fails with "chunk not found in any blob". Because `loadKnownChunks` reads the `chunks` table, later runs treat the chunk as already stored. Upload and write failures do not complete a snapshot: the packer keeps the closed writer and the final `Flush` fails the run. ## Acceptable - An error returned by `addChunkToPacker` (packer, database, encryption, upload) aborts the run regardless of `--skip-errors`. Only open and read errors are skipped. - `README.md:119`, `internal/cli/root.go:60` and `scanner.go:66` agree on that wording. - Do not fix this by reordering the known/pending bookkeeping: `AddPendingChunk` and `addPendingChunkHash` must come before packing, because a finalize triggered inside `addChunkToPacker` inserts and un-pends the triggering chunk. - Readability only: `packer.go:599-602` sets `currentBlob` to nil after `cleanupTempFile`. ## Definition of done 1. Test: inject a one-time temp-file creation failure under `--skip-errors`; the run fails (no snapshot completes with a file whose chunk is in no blob). 2. Test: a file read error under `--skip-errors` is still skipped and the run completes. 3. No existing assertion weakened; `make check` green. Related: https://git.eeqj.de/sneak/vaultik/issues/148 is the same family (chunk rows that outlive data never stored) with a different trigger, an interrupted upload followed by a re-run. Its repair-at-start option may also clear the state described here on the next run, but it does not stop `--skip-errors` from swallowing packer errors within a run; both are needed. This cannot go into https://git.eeqj.de/sneak/vaultik/issues/72, whose definition of done forbids production changes. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#161