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

A backup under --skip-errors printed its skip errors and failed count,
then "Finished successfully.", and exited 0, so a cron wrapper could
not tell it from a complete backup. A directory the walk could not list
was not counted at all.

Each entry the walk skips is now counted as examined and failed. When
--skip-errors skipped anything it could not read or list, the run keeps
its snapshots, runs the post-backup prune as before, and then returns
an error the command reports as "Backup completed, but N file(s) could
not be read". A file removed during the run is still counted as failed
in the summary but leaves the exit status at 0, with or without the
flag.

Model: opus-5-5

README.md

@@ -178,7 +178,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`: Skip files that cannot be read when creating a snapshot, or that cannot be restored when restoring, instead of aborting. Packing and storage errors while creating a snapshot (which would leave a chunk recorded but not stored) still abort the run.
+* `--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 while creating a snapshot (which would leave a chunk recorded but not stored) still abort the run. A run that skipped anything still finishes but exits non-zero: `snapshot create` keeps the snapshot and counts each file or directory it skipped as failed. A file removed during the backup is also counted as failed but does not change the exit status.
 
 ### locking
 

internal/cli/entry_snapshot_create_skip_errors_test.go

@@ -0,0 +1,30 @@
+package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestEntrySnapshotCreateSkipErrorsExitsNonZeroWhenItSkips backs up,
+// under --skip-errors, a source directory that does not exist, which the
+// walk skips. The command exits non-zero and reports the backup as
+// completed, not as a failed snapshot creation. See
+// https://git.eeqj.de/sneak/vaultik/issues/310.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
+func TestEntrySnapshotCreateSkipErrorsExitsNonZeroWhenItSkips(t *testing.T) {
+	// writeBackupConfig would create the source directory.
+	configPath := writeHermeticPruneConfig(t, false)
+
+	code, stdout, stderr := runEntry(t,
+		flagConfig, configPath, "--skip-errors", cmdSnapshot, cmdCreate)
+
+	assert.Equal(t, 1, code)
+	assert.Contains(t, stdout,
+		"Files: 1 examined, 0 backed up, 0 unchanged, 1 failed.")
+	assert.NotContains(t, stdout, "Finished successfully.")
+	assert.Contains(t, stderr,
+		"Backup completed, but 1 file(s) could not be read")
+	assert.NotContains(t, stderr, "Snapshot creation failed")
+}

internal/cli/snapshot.go

@@ -115,6 +115,13 @@ on Linux), then /etc/vaultik/config.yml.`,
 					return
 				}
 
+				if errors.Is(err, vaultik.ErrFilesUnreadable) {
+					log.Error("Backup completed with unreadable files", "error", err)
+					ReportErrorf("Backup completed, but %v", err)
+
+					return
+				}
+
 				log.Error("Snapshot creation failed", "error", err)
 				ReportErrorf("Snapshot creation failed: %v", err)
 			})

internal/snapshot/scanner.go

@@ -141,10 +141,11 @@ type ScannerConfig struct {
 // ScanResult contains the results of a scan operation. Files and bytes
 // are counted per file: BytesScanned is the size of the new and changed
 // files, BytesSkipped that of the unchanged ones. FilesFailed counts the
-// new and changed files that phase 2 could not store; FilesScanned
-// includes them and BytesScanned does not. ChunksCreated counts the
-// chunks that were not already stored, and NewChunkSize is their total
-// size.
+// new and changed files that phase 2 could not store and, under
+// --skip-errors, each entry the walk could not read, such as a directory
+// it could not list; FilesScanned includes them and BytesScanned does
+// not. ChunksCreated counts the chunks that were not already stored, and
+// NewChunkSize is their total size.
 type ScanResult struct {
 	FilesScanned   int
 	FilesSkipped   int
@@ -161,6 +162,11 @@ type ScanResult struct {
 	UploadDuration time.Duration
 	StartTime      time.Time
 	EndTime        time.Time
+
+	// FilesUnreadable is the part of FilesFailed that --skip-errors
+	// skipped because it could not be read or listed; the rest are files
+	// removed after the walk listed them.
+	FilesUnreadable int
 }
 
 // NewScanner creates a new scanner instance
@@ -883,7 +889,7 @@ func (s *Scanner) scanPhase(
 
 	walkFn := func(filePath string, info os.FileInfo, err error) error {
 		if err != nil {
-			return s.handleWalkError(filePath, err)
+			return s.handleWalkError(filePath, err, result)
 		}
 
 		// Check context cancellation
@@ -959,7 +965,7 @@ func (s *Scanner) recordSpecialEntry(
 	if info.Mode()&os.ModeSymlink != 0 {
 		file, err := s.buildSymlinkEntry(filePath, info)
 		if err != nil {
-			return true, s.handleWalkError(filePath, err)
+			return true, s.handleWalkError(filePath, err, result)
 		}
 
 		existingFiles[filePath] = struct{}{}
@@ -983,14 +989,21 @@ func (s *Scanner) recordSpecialEntry(
 }
 
 // handleWalkError deals with a filesystem error surfaced by the walk:
-// skipped (with a UI notice) under --skip-errors, fatal otherwise.
-func (s *Scanner) handleWalkError(filePath string, err error) error {
+// under --skip-errors the entry is skipped with a UI notice and counted
+// as examined, failed and unreadable, otherwise the error is fatal.
+func (s *Scanner) handleWalkError(
+	filePath string, err error, result *ScanResult,
+) error {
 	if s.skipErrors {
 		log.Error("Failed to access file (skipping due to --skip-errors)",
 			"path", filePath, "error", err)
 		s.ui.Errorf("Failed to access %s: %v. Skipping (--skip-errors).",
 			s.ui.Path(filePath), err)
 
+		result.FilesScanned++
+		result.FilesFailed++
+		result.FilesUnreadable++
+
 		return nil // Continue scanning
 	}
 
@@ -1422,6 +1435,7 @@ func (s *Scanner) processFileWithErrorHandling(
 				s.ui.Path(fileToProcess.Path), err)
 
 			countFailedFile(fileToProcess, result)
+			result.FilesUnreadable++
 
 			return true, nil
 		}

internal/vaultik/restore.go

@@ -12,7 +12,6 @@ import (
 	"path/filepath"
 	"slices"
 	"strings"
-	"syscall"
 	"time"
 
 	"filippo.io/age"
@@ -220,7 +219,9 @@ func (v *Vaultik) finishRestore(
 	)
 
 	if os.Geteuid() != 0 {
-		v.warnIfOwnershipNotPreserved(files, opts.TargetDir)
+		v.UI.Warningf("Restore did not preserve file ownership: chown(2) " +
+			"requires root. Re-run as root (e.g. with sudo) if you need " +
+			"original UID/GID preserved.")
 	}
 
 	if result.FilesFailed > 0 {
@@ -246,43 +247,6 @@ func (v *Vaultik) finishRestore(
 	return unmatchedErr
 }
 
-// warnIfOwnershipNotPreserved warns once if some entry restored under
-// targetDir has another owner or group on disk than the snapshot
-// records. It is meant for a restore by a normal user, which changes no
-// owner or group: a new entry is owned by that user, and its group is
-// that user's, or the group of the directory it is created in on macOS
-// and in a setgid directory on Linux. An entry that is not on disk, or
-// whose filesystem reports no owner, is skipped.
-func (v *Vaultik) warnIfOwnershipNotPreserved(
-	files []*database.File, targetDir string,
-) {
-	for _, file := range files {
-		targetPath, err := containedRestorePath(
-			v.Fs, targetDir, file.Path.String())
-		if err != nil {
-			continue
-		}
-
-		info, err := lstatIfPossible(v.Fs, targetPath)
-		if err != nil {
-			continue
-		}
-
-		owner, ok := info.Sys().(*syscall.Stat_t)
-		if !ok {
-			continue
-		}
-
-		if owner.Uid != file.UID || owner.Gid != file.GID {
-			v.UI.Warningf("Restore did not preserve file ownership: chown(2) " +
-				"requires root. Re-run as root (e.g. with sudo) if you need " +
-				"original UID/GID preserved.")
-
-			return
-		}
-	}
-}
-
 // reportUnmatchedPaths warns, by name, about each requested path that
 // matched nothing in the snapshot, and returns an error counting them,
 // or nil when there are none.
@@ -1001,9 +965,8 @@ type restoreSession struct {
 	// root can chown a file to an arbitrary UID/GID — non-root chown
 	// always fails with EPERM. Attempting it anyway produces N
 	// guaranteed-failed syscalls + N noisy debug lines, so we skip
-	// the call entirely as non-root; warnIfOwnershipNotPreserved warns
-	// once at the end of the restore if that left any owner or group
-	// other than recorded.
+	// the call entirely as non-root and emit one warning at the end
+	// of the restore explaining that ownership was not preserved.
 	runningAsRoot bool
 	// directories holds every restored directory, for
 	// applyDirectoryMetadata to finish after the restore loop.

internal/vaultik/restore_ownership_warning_test.go

@@ -1,131 +0,0 @@
-package vaultik //nolint:testpackage // calls the unexported ownership check
-
-import (
-	"bytes"
-	"context"
-	"os"
-	"strings"
-	"syscall"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/database"
-	"sneak.berlin/go/vaultik/internal/log"
-	"sneak.berlin/go/vaultik/internal/ui"
-)
-
-// These tests cover https://git.eeqj.de/sneak/vaultik/issues/316: a
-// restore by a normal user warns that ownership was not preserved only
-// when some restored entry has another owner or group on disk than the
-// snapshot records.
-
-const ownershipWarning = "did not preserve file ownership"
-
-// TestOwnershipWarningSilentWhenEntriesMatch checks that a restore whose
-// entries all end up with their recorded owner and group prints no
-// warning.
-func TestOwnershipWarningSilentWhenEntriesMatch(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	targetDir := t.TempDir()
-	owner := ownerOf(t, targetDir)
-
-	out := restoreAsNormalUser(t, targetDir, []*database.File{
-		{Path: "/a", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid},
-		{Path: "/b", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid},
-	})
-
-	assert.Empty(t, out)
-}
-
-// TestOwnershipWarningWhenOwnerDiffers checks that the warning is printed
-// when an entry recorded with another owner is restored owned by the
-// restoring user, even though its group matches the recorded one.
-func TestOwnershipWarningWhenOwnerDiffers(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	targetDir := t.TempDir()
-	owner := ownerOf(t, targetDir)
-
-	out := restoreAsNormalUser(t, targetDir, []*database.File{
-		{Path: "/a", Mode: plainFileMode, UID: otherOwnerID, GID: owner.Gid},
-	})
-
-	assert.Equal(t, 1, strings.Count(out, ownershipWarning), out)
-}
-
-// TestOwnershipWarningWhenDirectoryGivesItsGroup checks that the warning
-// is printed, once, when entries recorded with the restoring user's
-// owner and group take another group from the setgid directory they are
-// restored into, as every new entry takes its directory's group on
-// macOS.
-func TestOwnershipWarningWhenDirectoryGivesItsGroup(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	if os.Geteuid() != 0 {
-		t.Skip("giving a directory to another group needs root")
-	}
-
-	targetDir := t.TempDir()
-	user := ownerOf(t, targetDir)
-
-	require.NoError(t, os.Chown(targetDir, -1, int(otherOwnerID)))
-	require.NoError(t, os.Chmod(targetDir, os.ModeSetgid|writableTestMode))
-
-	out := restoreAsNormalUser(t, targetDir, []*database.File{
-		{Path: "/a", Mode: plainFileMode, UID: user.Uid, GID: user.Gid},
-		{Path: "/b", Mode: plainFileMode, UID: user.Uid, GID: user.Gid},
-	})
-
-	assert.Equal(t, 1, strings.Count(out, ownershipWarning), out)
-}
-
-// ownerOf returns the owner and group path has on disk.
-func ownerOf(t *testing.T, path string) *syscall.Stat_t {
-	t.Helper()
-
-	info, err := os.Lstat(path)
-	require.NoError(t, err)
-
-	owner, ok := info.Sys().(*syscall.Stat_t)
-	require.True(t, ok)
-
-	return owner
-}
-
-// restoreAsNormalUser restores rows, all empty regular files, into
-// targetDir without changing any owner or group, as restore does for a
-// normal user, and returns what warnIfOwnershipNotPreserved prints.
-func restoreAsNormalUser(
-	t *testing.T, targetDir string, rows []*database.File,
-) string {
-	t.Helper()
-
-	ctx := context.Background()
-	rows, repos := makeFiles(ctx, t, rows)
-
-	var out bytes.Buffer
-
-	v := newContainmentVaultik(ctx, afero.NewOsFs())
-	v.UI = ui.NewWithColor(&out, false)
-
-	session := &restoreSession{
-		v:      v,
-		ctx:    ctx,
-		repos:  repos,
-		opts:   &RestoreOptions{TargetDir: targetDir},
-		result: &RestoreResult{},
-	}
-	for _, row := range rows {
-		require.NoError(t, session.restoreFile(row))
-	}
-
-	v.warnIfOwnershipNotPreserved(rows, targetDir)
-
-	return out.String()
-}

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
@@ -135,12 +140,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
@@ -156,6 +165,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
@@ -223,16 +245,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 {
@@ -241,12 +269,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{
@@ -261,7 +289,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)
@@ -269,12 +297,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",
@@ -287,7 +315,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
@@ -373,6 +401,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,55 @@ 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")
+		})
+	}
+}