Warn about restored ownership only when an entry's owner or group differs (closes #316)

A restore by a normal user always warned that file ownership was not
preserved, even when every entry came out with its recorded owner and
group. The owner and group each restored entry has on disk are now
compared with the recorded ones, and the warning is printed once if any
differ. The comparison is made on disk because on macOS, and in a setgid
directory on Linux, a new entry takes the group of its directory, not
the user's.

Judgement call: an entry that is not on disk after the restore is not compared.
Unverified: not run on a Mac; the directory-group test runs only as root.

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. 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.
+* `--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.
 
 ### locking
 

internal/cli/entry_snapshot_create_skip_errors_test.go

@@ -1,30 +0,0 @@
-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,13 +115,6 @@ 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,11 +141,10 @@ 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 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.
+// 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.
 type ScanResult struct {
 	FilesScanned   int
 	FilesSkipped   int
@@ -162,11 +161,6 @@ 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
@@ -889,7 +883,7 @@ func (s *Scanner) scanPhase(
 
 	walkFn := func(filePath string, info os.FileInfo, err error) error {
 		if err != nil {
-			return s.handleWalkError(filePath, err, result)
+			return s.handleWalkError(filePath, err)
 		}
 
 		// Check context cancellation
@@ -965,7 +959,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, result)
+			return true, s.handleWalkError(filePath, err)
 		}
 
 		existingFiles[filePath] = struct{}{}
@@ -989,21 +983,14 @@ func (s *Scanner) recordSpecialEntry(
 }
 
 // handleWalkError deals with a filesystem error surfaced by the walk:
-// 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 {
+// skipped (with a UI notice) under --skip-errors, fatal otherwise.
+func (s *Scanner) handleWalkError(filePath string, err error) 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
 	}
 
@@ -1435,7 +1422,6 @@ func (s *Scanner) processFileWithErrorHandling(
 				s.ui.Path(fileToProcess.Path), err)
 
 			countFailedFile(fileToProcess, result)
-			result.FilesUnreadable++
 
 			return true, nil
 		}

internal/vaultik/restore.go

@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"slices"
 	"strings"
+	"syscall"
 	"time"
 
 	"filippo.io/age"
@@ -219,9 +220,7 @@ func (v *Vaultik) finishRestore(
 	)
 
 	if os.Geteuid() != 0 {
-		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.")
+		v.warnIfOwnershipNotPreserved(files, opts.TargetDir)
 	}
 
 	if result.FilesFailed > 0 {
@@ -247,6 +246,43 @@ 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.
@@ -965,8 +1001,9 @@ 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 and emit one warning at the end
-	// of the restore explaining that ownership was not preserved.
+	// 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.
 	runningAsRoot bool
 	// directories holds every restored directory, for
 	// applyDirectoryMetadata to finish after the restore loop.

internal/vaultik/restore_ownership_warning_test.go

@@ -0,0 +1,131 @@
+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,11 +39,6 @@ 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
@@ -140,16 +135,12 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
 	}
 
 	// Process each named snapshot
-	filesUnreadable := 0
-
 	for snapIdx, snapName := range snapshotNames {
-		unreadable, err := v.createNamedSnapshot(
+		err = v.createNamedSnapshot(
 			opts, hostname, snapName, snapIdx+1, len(snapshotNames))
 		if err != nil {
 			return err
 		}
-
-		filesUnreadable += unreadable
 	}
 
 	// Print overall summary if multiple snapshots
@@ -165,19 +156,6 @@ 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
@@ -245,22 +223,16 @@ 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 and returns the
-// number of entries --skip-errors skipped because it could not read or
-// list them.
+// createNamedSnapshot creates a single named snapshot
 func (v *Vaultik) createNamedSnapshot(
 	opts *SnapshotCreateOptions, hostname, snapName string, idx, total int,
-) (int, error) {
+) error {
 	snapshotStartTime := time.Now()
 
 	if total > 1 {
@@ -269,12 +241,12 @@ func (v *Vaultik) createNamedSnapshot(
 
 	resolvedDirs, err := v.resolveSnapshotPaths(snapName)
 	if err != nil {
-		return 0, err
+		return err
 	}
 
 	previousSnapshotID, err := v.previousSnapshotID(snapName)
 	if err != nil {
-		return 0, err
+		return err
 	}
 
 	scanner := v.ScannerFactory(snapshot.ScannerParams{
@@ -289,7 +261,7 @@ func (v *Vaultik) createNamedSnapshot(
 	snapshotID, err := v.SnapshotManager.CreateSnapshotWithName(
 		v.ctx, hostname, snapName, v.Globals.Version, v.Globals.Commit)
 	if err != nil {
-		return 0, fmt.Errorf("creating snapshot: %w", err)
+		return fmt.Errorf("creating snapshot: %w", err)
 	}
 
 	log.Info("Beginning snapshot", "snapshot_id", snapshotID, "name", snapName)
@@ -297,12 +269,12 @@ func (v *Vaultik) createNamedSnapshot(
 
 	stats, err := v.scanAllDirectories(scanner, resolvedDirs, snapshotID)
 	if err != nil {
-		return 0, err
+		return err
 	}
 
 	err = v.finalizeSnapshotMetadata(snapshotID, stats)
 	if err != nil {
-		return 0, err
+		return err
 	}
 
 	log.Info("Snapshot complete",
@@ -315,7 +287,7 @@ func (v *Vaultik) createNamedSnapshot(
 
 	v.printSnapshotSummary(snapshotID, snapshotStartTime, stats)
 
-	return stats.totalFilesUnreadable, nil
+	return nil
 }
 
 // previousSnapshotID returns the ID of the most recent completed snapshot
@@ -401,7 +373,6 @@ 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,13 +178,11 @@ func TestBackupDoesNotCountFileInUnopenableDirectoryAsDeleted(t *testing.T) {
 	}
 
 	env.out.Reset()
-	err := env.v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+	require.NoError(t, 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, 0 backed up, 1 unchanged, 1 failed.")
+	}))
+	assert.Contains(t, env.out.String(), filesLine(1, 0, 1))
 }
 
 // A backup counts only files that the previous snapshot of its own name

internal/vaultik/snapshot_failed_file_test.go

@@ -49,19 +49,13 @@ 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,
-			wantErr: vaultik.ErrFilesUnreadable},
+			openErr: os.ErrPermission, skipErrors: true},
 		// 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 {
@@ -105,11 +99,10 @@ func TestSnapshotSummaryCountsFileNotStoredAsFailed(t *testing.T) {
 			v := newBackupVaultik(ctx, cfg, store, repos, db, fs)
 			v.UI = ui.NewWithColor(out, false)
 
-			err = v.CreateSnapshot(&vaultik.SnapshotCreateOptions{
+			require.NoError(t, 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,7 +3,6 @@ package vaultik_test
 import (
 	"context"
 	"fmt"
-	"os"
 	"path/filepath"
 	"testing"
 
@@ -74,55 +73,3 @@ 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")
-		})
-	}
-}