diff --git a/README.md b/README.md index f0cc9ba..8febcf9 100644 --- a/README.md +++ b/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 diff --git a/internal/cli/entry_snapshot_create_skip_errors_test.go b/internal/cli/entry_snapshot_create_skip_errors_test.go new file mode 100644 index 0000000..e52491d --- /dev/null +++ b/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") +} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 7fd5b73..7f7742c 100644 --- a/internal/cli/snapshot.go +++ b/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) }) diff --git a/internal/snapshot/scanner.go b/internal/snapshot/scanner.go index 10a5541..79cc0e7 100644 --- a/internal/snapshot/scanner.go +++ b/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 } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 3adeff9..0f5b66c 100644 --- a/internal/vaultik/snapshot.go +++ b/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 diff --git a/internal/vaultik/snapshot_deleted_count_test.go b/internal/vaultik/snapshot_deleted_count_test.go index bce2a4c..9cb4941 100644 --- a/internal/vaultik/snapshot_deleted_count_test.go +++ b/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 diff --git a/internal/vaultik/snapshot_failed_file_test.go b/internal/vaultik/snapshot_failed_file_test.go index a5f8bc4..ff8f9b8 100644 --- a/internal/vaultik/snapshot_failed_file_test.go +++ b/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, diff --git a/internal/vaultik/snapshot_skip_errors_test.go b/internal/vaultik/snapshot_skip_errors_test.go index 0f88515..b27861d 100644 --- a/internal/vaultik/snapshot_skip_errors_test.go +++ b/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") + }) + } +}