Exit non-zero when snapshot create --skip-errors skips anything it cannot read (closes #310)

A backup under --skip-errors printed its skip errors, then "Finished
successfully.", and exited 0, so a cron wrapper could not tell it from
a complete backup.

Each skipped entry, including a directory the walk could not list, is
now counted as examined and failed. If --skip-errors skipped anything
it could not read or list, the run still keeps its snapshots and
prunes as before, then fails with "Backup completed, but N file(s)
could not be read". A file removed during the run, even before the
walk reaches it, counts once as failed and leaves the exit status
alone; without the flag it no longer stops the backup. Excluded
entries are never counted.

Model: opus-5-5

internal/vaultik/snapshot.go

@@ -39,6 +39,11 @@ var (
 // caller can report it apart from a failed backup.
 var ErrPostBackupPruneFailed = errors.New("post-backup prune failed")
 
+// ErrFilesUnreadable is returned when --skip-errors skipped files or
+// directories it could not read or list. The snapshot is complete and
+// kept, so the caller can report it apart from a failed backup.
+var ErrFilesUnreadable = errors.New("file(s) could not be read")
+
 // listRecentLimit caps how many snapshot rows are fetched from the
 // local index for listing and purge operations.
 const listRecentLimit = 10000
@@ -130,12 +135,16 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
 	}
 
 	// Process each named snapshot
+	filesUnreadable := 0
+
 	for snapIdx, snapName := range snapshotNames {
-		err = v.createNamedSnapshot(
+		unreadable, err := v.createNamedSnapshot(
 			opts, hostname, snapName, snapIdx+1, len(snapshotNames))
 		if err != nil {
 			return err
 		}
+
+		filesUnreadable += unreadable
 	}
 
 	// Print overall summary if multiple snapshots
@@ -151,6 +160,19 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
 		}
 	}
 
+	return v.finishCreateSnapshot(filesUnreadable)
+}
+
+// finishCreateSnapshot ends a run whose snapshots are all complete and
+// kept. Like finishRestore, it returns an error when --skip-errors
+// skipped anything it could not read or list; otherwise it prints the
+// closing line. A file removed during the run is counted as failed in
+// the summary but is not an error, with or without --skip-errors.
+func (v *Vaultik) finishCreateSnapshot(filesUnreadable int) error {
+	if filesUnreadable > 0 {
+		return fmt.Errorf("%d %w", filesUnreadable, ErrFilesUnreadable)
+	}
+
 	// Terminus must obey the --cron invariant: silent on total
 	// success only. UI.Complete is dropped in cron/quiet mode (that's
 	// the success path), but if any warnings fired during the run we
@@ -240,16 +262,22 @@ type snapshotStats struct {
 	totalBlobsUploaded int
 	uploadDuration     time.Duration
 
+	// The part of totalFilesFailed that --skip-errors skipped because it
+	// could not be read or listed.
+	totalFilesUnreadable int
+
 	// The sizes of all blobs the snapshot references, set by
 	// finalizeSnapshotMetadata once snapshot_blobs is populated.
 	blobSize             int64
 	blobUncompressedSize int64
 }
 
-// createNamedSnapshot creates a single named snapshot
+// createNamedSnapshot creates a single named snapshot and returns the
+// number of entries --skip-errors skipped because it could not read or
+// list them.
 func (v *Vaultik) createNamedSnapshot(
 	opts *SnapshotCreateOptions, hostname, snapName string, idx, total int,
-) error {
+) (int, error) {
 	snapshotStartTime := time.Now()
 
 	if total > 1 {
@@ -258,12 +286,12 @@ func (v *Vaultik) createNamedSnapshot(
 
 	resolvedDirs, err := v.resolveSnapshotPaths(snapName)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	previousSnapshotID, err := v.previousSnapshotID(snapName)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	scanner := v.ScannerFactory(snapshot.ScannerParams{
@@ -278,7 +306,7 @@ func (v *Vaultik) createNamedSnapshot(
 	snapshotID, err := v.SnapshotManager.CreateSnapshotWithName(
 		v.ctx, hostname, snapName, v.Globals.Version, v.Globals.Commit)
 	if err != nil {
-		return fmt.Errorf("creating snapshot: %w", err)
+		return 0, fmt.Errorf("creating snapshot: %w", err)
 	}
 
 	log.Info("Beginning snapshot", "snapshot_id", snapshotID, "name", snapName)
@@ -286,12 +314,12 @@ func (v *Vaultik) createNamedSnapshot(
 
 	stats, err := v.scanAllDirectories(scanner, resolvedDirs, snapshotID)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	err = v.finalizeSnapshotMetadata(snapshotID, stats)
 	if err != nil {
-		return err
+		return 0, err
 	}
 
 	log.Info("Snapshot complete",
@@ -304,7 +332,7 @@ func (v *Vaultik) createNamedSnapshot(
 
 	v.printSnapshotSummary(snapshotID, snapshotStartTime, stats)
 
-	return nil
+	return stats.totalFilesUnreadable, nil
 }
 
 // previousSnapshotID returns the ID of the most recent completed snapshot
@@ -390,6 +418,7 @@ func (v *Vaultik) scanAllDirectories(
 		stats.totalBlobs += result.BlobsCreated
 		stats.totalFilesSkipped += result.FilesSkipped
 		stats.totalFilesFailed += result.FilesFailed
+		stats.totalFilesUnreadable += result.FilesUnreadable
 		stats.totalBytesSkipped += result.BytesSkipped
 		stats.totalFilesDeleted += result.FilesDeleted
 		stats.totalBytesDeleted += result.BytesDeleted

internal/vaultik/snapshot_deleted_count_test.go

@@ -178,11 +178,13 @@ func TestBackupDoesNotCountFileInUnopenableDirectoryAsDeleted(t *testing.T) {
 	}
 
 	env.out.Reset()
-	require.NoError(t, env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+	err := env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
 		SkipErrors: true,
 		Snapshots:  []string{"work"},
-	}))
-	assert.Contains(t, env.out.String(), filesLine(1, 0, 1))
+	})
+	require.ErrorIs(t, err, vaultik.ErrFilesUnreadable)
+	assert.Contains(t, env.out.String(),
+		"Files: 2 examined, 0 backed up, 1 unchanged, 1 failed.")
 }
 
 // A backup counts only files that the previous snapshot of its own name

internal/vaultik/snapshot_failed_file_test.go

@@ -49,13 +49,19 @@ func TestSnapshotSummaryCountsFileNotStoredAsFailed(t *testing.T) {
 		name       string
 		openErr    error
 		skipErrors bool
+		wantErr    error // nil for none
 	}{
 		// What a normal user gets opening a file with mode 000.
 		{name: "unopenable under skip-errors",
-			openErr: os.ErrPermission, skipErrors: true},
+			openErr: os.ErrPermission, skipErrors: true,
+			wantErr: vaultik.ErrFilesUnreadable},
 		// What opening a file removed after phase 1 gives.
 		{name: "removed between the phases",
 			openErr: os.ErrNotExist, skipErrors: false},
+		// A removal is not an error, so --skip-errors does not make the
+		// run fail. See https://git.eeqj.de/sneak/vaultik/issues/310.
+		{name: "removed between the phases under skip-errors",
+			openErr: os.ErrNotExist, skipErrors: true},
 	}
 
 	for _, tt := range tests {
@@ -99,10 +105,11 @@ func TestSnapshotSummaryCountsFileNotStoredAsFailed(t *testing.T) {
 			v := newBackupVaultik(ctx, cfg, store, repos, db, fs)
 			v.UI = ui.NewWithColor(out, false)
 
-			require.NoError(t, v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+			err = v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
 				SkipErrors: tt.skipErrors,
 				Snapshots:  []string{"src"},
-			}))
+			})
+			require.ErrorIs(t, err, tt.wantErr)
 
 			summary := out.String()
 			assert.Contains(t, summary,

internal/vaultik/snapshot_skip_errors_test.go

@@ -3,6 +3,7 @@ package vaultik_test
 import (
 	"context"
 	"fmt"
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -73,3 +74,144 @@ func TestSkipErrorsBackupStopsOnIndexErrorRecordingDirectory(t *testing.T) {
 	require.Len(t, snapshots, 1)
 	assert.Nil(t, snapshots[0].CompletedAt)
 }
+
+// A backup under --skip-errors that skips a file it cannot open, or a
+// directory it cannot list, keeps the snapshot, counts what it skipped
+// as failed and returns an error instead of finishing successfully. See
+// https://git.eeqj.de/sneak/vaultik/issues/310.
+func TestSkipErrorsBackupKeepsSnapshotAndFailsWhenItSkips(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		skipped string // relative to the source directory
+	}{
+		{name: "file it cannot open", skipped: "skipdir/file.txt"},
+		{name: "directory it cannot list", skipped: "skipdir"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			env := newDeletedCountEnv(t, map[string][]byte{
+				"stored.txt":       []byte("stored"),
+				"skipdir/file.txt": []byte("in skipdir"),
+			})
+			env.setSnapshot("work", "")
+
+			// A mode does not lock out root, who runs the tests, so
+			// opening the skipped path is made to fail instead.
+			env.v.Fs = &openFailFs{
+				path: filepath.Join(env.src, tt.skipped),
+				err:  os.ErrPermission,
+			}
+
+			err := env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+				SkipErrors: true,
+				Snapshots:  []string{"work"},
+			})
+			require.ErrorIs(t, err, vaultik.ErrFilesUnreadable)
+
+			assert.Contains(t, env.out.String(),
+				"Files: 2 examined, 1 backed up, 0 unchanged, 1 failed.")
+			assert.NotContains(t, env.out.String(), "Finished successfully.")
+
+			snapshots, err := env.v.Repositories.Snapshots.ListRecent(
+				context.Background(), listRecentTestLimit)
+			require.NoError(t, err)
+			require.Len(t, snapshots, 1)
+			assert.NotNil(t, snapshots[0].CompletedAt, "the snapshot is kept")
+		})
+	}
+}
+
+// removeOnLookupFs is the real filesystem, except that path is removed
+// just before it is looked up. The walk looks an entry up after listing
+// its directory, so path is listed but gone when the walk reaches it.
+type removeOnLookupFs struct {
+	afero.OsFs
+
+	path string
+}
+
+func (f *removeOnLookupFs) LstatIfPossible(
+	name string,
+) (os.FileInfo, bool, error) {
+	if name == f.path {
+		_ = os.Remove(name)
+	}
+
+	return f.OsFs.LstatIfPossible(name)
+}
+
+// A file removed before the walk reaches it is counted once, as failed,
+// like a file removed before its data is read, and the backup finishes
+// successfully, with or without --skip-errors. See
+// https://git.eeqj.de/sneak/vaultik/issues/310.
+func TestBackupCountsFileRemovedBeforeWalkReachesItAsFailed(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		skipErrors bool
+	}{
+		{name: "under skip-errors", skipErrors: true},
+		{name: "without skip-errors", skipErrors: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			env := newDeletedCountEnv(t, map[string][]byte{
+				"stays.txt":   []byte("stays"),
+				"removed.txt": []byte("removed"),
+			})
+			env.setSnapshot("walk", "")
+
+			// The previous snapshot has the file, so a removal the walk
+			// also counted as deleted would show.
+			env.backUp(t, "walk")
+
+			env.v.Fs = &removeOnLookupFs{
+				path: filepath.Join(env.src, "removed.txt"),
+			}
+
+			env.out.Reset()
+			require.NoError(t, env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+				SkipErrors: tt.skipErrors,
+				Snapshots:  []string{"walk"},
+			}))
+			assert.Contains(t, env.out.String(),
+				"Files: 2 examined, 0 backed up, 1 unchanged, 1 failed.")
+			assert.Contains(t, env.out.String(), "Finished successfully.")
+		})
+	}
+}
+
+// A file an exclude pattern matches is not counted when it is removed
+// before the walk reaches it. See
+// https://git.eeqj.de/sneak/vaultik/issues/310.
+func TestBackupIgnoresExcludedFileRemovedBeforeWalkReachesIt(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	env := newDeletedCountEnv(t, map[string][]byte{
+		"stays.txt":   []byte("stays"),
+		"removed.tmp": []byte("removed"),
+	})
+	env.setSnapshot("walk", "", "*.tmp")
+	env.v.Fs = &removeOnLookupFs{
+		path: filepath.Join(env.src, "removed.tmp"),
+	}
+
+	require.NoError(t, env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+		SkipErrors: true,
+		Snapshots:  []string{"walk"},
+	}))
+	assert.Contains(t, env.out.String(), filesLine(1, 1, 0))
+	assert.Contains(t, env.out.String(), "Finished successfully.")
+}