Abort the run when packing fails, even under --skip-errors (closes #161)

A chunk is registered as pending (known, scanner-pending, packer
pending-row) before it is packed. Under --skip-errors the scanner
skipped a file on any processing error, including a failure inside
addChunkToPacker (packing, database, encryption, upload). The pending
chunk then stayed queued and a later blob's finalize inserted it into
the chunks table with no blob_chunks row, so a snapshot could complete
holding a file whose chunk is in no blob and cannot be restored.

Errors from addChunkToPacker are now marked and abort the run
regardless of --skip-errors; only open and read errors are skipped.
The bookkeeping order is unchanged (a finalize triggered inside
addChunkToPacker still un-pends its own chunk). Flag help and comments
now say only unreadable files are skipped.

Model: opus-4-8

README.md

@@ -167,7 +167,7 @@ vaultik version
 * `--verbose`, `-v`: Enable verbose output (on stderr — see below)
 * `--debug`: Enable debug output (on stderr — see below)
 * `--quiet`, `-q`: Suppress non-error output (also suppresses startup banner)
-* `--skip-errors`: Continue past per-file errors instead of aborting (applies to `snapshot create` and `restore`)
+* `--skip-errors`: Skip files that cannot be read when creating a snapshot, or that cannot be restored when restoring, instead of aborting. Packing and storage errors (which would leave a chunk recorded but not stored) still abort the run.
 
 ### locking
 

internal/cli/root.go

@@ -57,8 +57,9 @@ on the source system.`,
 	cmd.PersistentFlags().BoolVarP(&rootFlags.Quiet, "quiet", "q", false,
 		"Suppress non-error output")
 	cmd.PersistentFlags().BoolVar(&rootFlags.SkipErrors, "skip-errors", false,
-		"Continue past per-file errors instead of aborting "+
+		"Skip files that cannot be read when creating a snapshot, or "+
-			"(applies to snapshot create and restore)")
+			"that cannot be restored when restoring, instead of aborting "+
+			"(packing and storage errors still abort)")
 
 	// Add subcommands
 	cmd.AddCommand(

internal/snapshot/scanner.go

@@ -63,7 +63,9 @@ type Scanner struct {
 	exclude           []string          // Glob patterns for files/directories to exclude
 	compiledExclude   []compiledPattern // Compiled glob patterns
 	progress          *ProgressReporter
-	skipErrors        bool // Skip file read errors (log loudly but continue)
+	// skipErrors skips files that cannot be opened or read (logged loudly);
+	// packer, database, encryption, and upload errors still abort the run.
+	skipErrors bool
 	// ui is the user-facing output; never nil (defaults to a discarding writer).
 	ui *ui.Writer
 
@@ -121,7 +123,9 @@ type ScannerConfig struct {
 	EnableProgress   bool       // Enable the live progress reporter (ETAs, throughput)
 	UI               *ui.Writer // Where user-facing scanner messages go; nil = discard
 	Exclude          []string   // Glob patterns for files/directories to exclude
-	SkipErrors       bool       // Skip file read errors (log loudly but continue)
+	// SkipErrors skips files that cannot be opened or read (log loudly but
+	// continue); packer, database, encryption, and upload errors still abort.
+	SkipErrors bool
 }
 
 // ScanResult contains the results of a scan operation
@@ -1336,6 +1340,15 @@ func (s *Scanner) processFileWithErrorHandling(
 ) (bool, error) {
 	err := s.processFileStreaming(ctx, fileToProcess, result)
 	if err != nil {
+		// A packer/database/encryption/upload failure means the chunk's data
+		// may not have been stored. Skipping the file would let the snapshot
+		// record a file whose chunk is in no blob and cannot be restored, so
+		// abort the run even under --skip-errors. Only open and read errors
+		// are skipped below.
+		var pErr *packerError
+		if errors.As(err, &pErr) {
+			return false, fmt.Errorf("processing file %s: %w", fileToProcess.Path, err)
+		}
 		// Handle files that were deleted between scan and process phases
 		if errors.Is(err, os.ErrNotExist) {
 			log.Warn("File was deleted during backup, skipping",
@@ -1345,7 +1358,7 @@ func (s *Scanner) processFileWithErrorHandling(
 
 			return true, nil
 		}
-		// Skip file read errors if --skip-errors is enabled
+		// Skip open/read errors if --skip-errors is enabled
 		if s.skipErrors {
 			log.Error("Failed to process file (skipping due to --skip-errors)",
 				"path", fileToProcess.Path, "error", err)
@@ -1712,6 +1725,20 @@ type streamingChunkInfo struct {
 	size      int64
 }
 
+// packerError marks an error that came from adding a chunk to the packer
+// (packing, database, encryption, or upload). Such an error means the chunk's
+// data may not have been stored, so the run must abort even under --skip-errors:
+// skipping the file would leave the chunk recorded as backed up while it lives
+// in no blob, and a later snapshot could record a file that cannot be restored.
+// Only open and read errors are safe to skip.
+type packerError struct {
+	err error
+}
+
+func (e *packerError) Error() string { return e.err.Error() }
+
+func (e *packerError) Unwrap() error { return e.err }
+
 // processFileStreaming processes a file by streaming chunks directly to the packer
 func (s *Scanner) processFileStreaming(
 	ctx context.Context, fileToProcess *FileToProcess, result *ScanResult,
@@ -1762,7 +1789,11 @@ func (s *Scanner) processFileStreaming(
 		if !chunkExists {
 			err := s.addChunkToPacker(ctx, chunk)
 			if err != nil {
-				return err
+				// Mark as a packer error so --skip-errors cannot swallow it:
+				// the chunk was registered as pending before packing, so a
+				// skipped file here would be recorded as backed up while its
+				// data was never stored.
+				return &packerError{err: err}
 			}
 		}
 

internal/snapshot/skip_errors_test.go

@@ -0,0 +1,216 @@
+package snapshot_test
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/spf13/afero"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+)
+
+// errSimTempFail is the one-time temp-file creation failure blobTempFailFs
+// injects, mirroring a full temp filesystem.
+var errSimTempFail = errors.New("simulated temp-file creation failure")
+
+// errSimRead is the read failure readFailFile injects for a file that opens
+// but cannot be read.
+var errSimRead = errors.New("simulated read failure")
+
+// blobTempFailFs fails the first temp-file creation for a packer blob, then
+// behaves normally, simulating a one-time failure to start a new blob.
+type blobTempFailFs struct {
+	afero.Fs
+
+	mu     sync.Mutex
+	failed bool
+}
+
+//nolint:ireturn // afero.Fs.OpenFile is defined to return the interface.
+func (f *blobTempFailFs) OpenFile(
+	name string, flag int, perm os.FileMode,
+) (afero.File, error) {
+	if strings.Contains(name, "vaultik-blob-") {
+		f.mu.Lock()
+		firstTime := !f.failed
+		f.failed = true
+		f.mu.Unlock()
+
+		if firstTime {
+			return nil, errSimTempFail
+		}
+	}
+
+	return f.Fs.OpenFile(name, flag, perm)
+}
+
+// readFailFile wraps an afero.File whose Read always fails.
+type readFailFile struct {
+	afero.File
+}
+
+func (readFailFile) Read([]byte) (int, error) {
+	return 0, errSimRead
+}
+
+// readFailFs fails reads of one target path after a successful open.
+type readFailFs struct {
+	afero.Fs
+
+	target string
+}
+
+//nolint:ireturn // afero.Fs.Open is defined to return the interface.
+func (f *readFailFs) Open(name string) (afero.File, error) {
+	file, err := f.Fs.Open(name)
+	if err != nil {
+		return nil, err
+	}
+
+	if name == f.target {
+		return readFailFile{File: file}, nil
+	}
+
+	return file, nil
+}
+
+// writeSkipErrorTestFile writes one file into fs with a fixed mtime.
+func writeSkipErrorTestFile(t *testing.T, fs afero.Fs, path, content string) {
+	t.Helper()
+
+	err := fs.MkdirAll(filepath.Dir(path), 0755)
+	if err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+
+	err = afero.WriteFile(fs, path, []byte(content), 0644)
+	if err != nil {
+		t.Fatalf("write %s: %v", path, err)
+	}
+
+	when := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC)
+
+	err = fs.Chtimes(path, when, when)
+	if err != nil {
+		t.Fatalf("chtimes %s: %v", path, err)
+	}
+}
+
+// runSkipErrorScan scans /source on fs with the given skip-errors setting and
+// returns the repositories (for inspection) and the scan error.
+func runSkipErrorScan(
+	t *testing.T, fs afero.Fs, skipErrors bool,
+) (*database.Repositories, error) {
+	t.Helper()
+
+	db, err := database.NewTestDB()
+	if err != nil {
+		t.Fatalf("create test db: %v", err)
+	}
+
+	t.Cleanup(func() {
+		cerr := db.Close()
+		if cerr != nil {
+			t.Errorf("close db: %v", cerr)
+		}
+	})
+
+	repos := database.NewRepositories(db)
+
+	scanner := snapshot.NewScanner(snapshot.ScannerConfig{
+		FS:               fs,
+		ChunkSize:        int64(1024 * 16),
+		Repositories:     repos,
+		MaxBlobSize:      int64(1024 * 1024),
+		CompressionLevel: 3,
+		AgeRecipients:    []string{testAgePublicKey},
+		SkipErrors:       skipErrors,
+	})
+
+	ctx := context.Background()
+	snapshotID := "test-snapshot-skip-errors"
+	createTestSnapshotRecord(ctx, t, repos, snapshotID)
+
+	_, err = scanner.Scan(ctx, "/source", snapshotID)
+
+	return repos, err
+}
+
+// TestScannerPackingFailureAbortsUnderSkipErrors checks that a failure to start
+// a new blob aborts the run even with --skip-errors. Otherwise the file would
+// be skipped while its chunk had already been registered as pending, letting a
+// later blob record that chunk in the chunks table with no blob to back it —
+// a snapshot that completes with a file that cannot be restored.
+func TestScannerPackingFailureAbortsUnderSkipErrors(t *testing.T) {
+	t.Parallel()
+
+	// Two files with distinct content so each yields a distinct chunk: the
+	// first fails to start a blob, and without the fix the second's blob would
+	// commit the first's orphaned chunk row.
+	fs := &blobTempFailFs{Fs: afero.NewMemMapFs()}
+	writeSkipErrorTestFile(t, fs, "/source/file1.txt", "first file content")
+	writeSkipErrorTestFile(t, fs, "/source/file2.txt", "second file content")
+
+	repos, err := runSkipErrorScan(t, fs, true)
+	if err == nil {
+		t.Fatal("expected scan to abort on the packer error, got nil")
+	}
+
+	// ListUnpacked returns chunks recorded with no blob_chunks row: exactly the
+	// unrestorable state this fix prevents.
+	unpacked, err := repos.Chunks.ListUnpacked(context.Background(), 10)
+	if err != nil {
+		t.Fatalf("listing unpacked chunks: %v", err)
+	}
+
+	if len(unpacked) != 0 {
+		t.Fatalf("expected no chunk recorded without a blob, got %d", len(unpacked))
+	}
+}
+
+// TestScannerReadErrorAbortsWithoutSkipErrors checks that a file read error
+// aborts the run when --skip-errors is not set.
+func TestScannerReadErrorAbortsWithoutSkipErrors(t *testing.T) {
+	t.Parallel()
+
+	const target = "/source/unreadable.txt"
+
+	fs := &readFailFs{Fs: afero.NewMemMapFs(), target: target}
+	writeSkipErrorTestFile(t, fs, target, "content that cannot be read")
+
+	_, err := runSkipErrorScan(t, fs, false)
+	if err == nil {
+		t.Fatal("expected scan to fail on the read error, got nil")
+	}
+}
+
+// TestScannerReadErrorSkippedWithSkipErrors checks that a file read error is
+// skipped and the run completes when --skip-errors is set.
+func TestScannerReadErrorSkippedWithSkipErrors(t *testing.T) {
+	t.Parallel()
+
+	const target = "/source/unreadable.txt"
+
+	fs := &readFailFs{Fs: afero.NewMemMapFs(), target: target}
+	writeSkipErrorTestFile(t, fs, target, "content that cannot be read")
+
+	repos, err := runSkipErrorScan(t, fs, true)
+	if err != nil {
+		t.Fatalf("expected scan to complete with --skip-errors, got %v", err)
+	}
+
+	chunks, err := repos.FileChunks.GetByFile(context.Background(), target)
+	if err != nil {
+		t.Fatalf("getting file chunks: %v", err)
+	}
+
+	if len(chunks) != 0 {
+		t.Fatalf("expected unreadable file skipped, got %d chunks", len(chunks))
+	}
+}