From 99a4df67e69f4cba7c567310c45849623dcb19a0 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 02:36:45 +0000 Subject: [PATCH] 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 --- README.md | 2 +- .../entry_snapshot_create_skip_errors_test.go | 30 ++++ internal/cli/snapshot.go | 7 + internal/snapshot/scanner.go | 76 +++++++--- internal/vaultik/snapshot.go | 47 ++++-- .../vaultik/snapshot_deleted_count_test.go | 8 +- internal/vaultik/snapshot_failed_file_test.go | 13 +- internal/vaultik/snapshot_skip_errors_test.go | 142 ++++++++++++++++++ 8 files changed, 292 insertions(+), 33 deletions(-) create mode 100644 internal/cli/entry_snapshot_create_skip_errors_test.go diff --git a/README.md b/README.md index f1facea..5827e84 100644 --- a/README.md +++ b/README.md @@ -182,7 +182,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 61ac3d0..47a79fd 100644 --- a/internal/snapshot/scanner.go +++ b/internal/snapshot/scanner.go @@ -141,10 +141,12 @@ 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, each entry removed +// after the walk listed its directory 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 +163,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 @@ -890,8 +897,28 @@ func (s *Scanner) scanPhase( log.Debug("Starting directory walk", "path", path) walkFn := func(filePath string, info os.FileInfo, err error) error { + // Check exclude patterns first, so an excluded entry that is gone + // or cannot be looked up is neither counted nor reported. For + // directories, skip the entire subtree. info is nil when the + // walk could not look the entry up. + if s.shouldExclude(filePath, path) { + if info != nil && info.IsDir() { + return filepath.SkipDir + } + + return nil + } + + // The source path is in no directory listing, so its absence is + // an error rather than a removal. + if errors.Is(err, os.ErrNotExist) && filePath != path { + countRemovedDuringWalk(filePath, existingFiles, result) + + return nil + } + if err != nil { - return s.handleWalkError(filePath, err) + return s.handleWalkError(filePath, err, result) } // Check context cancellation @@ -901,15 +928,6 @@ func (s *Scanner) scanPhase( default: } - // Check exclude patterns - for directories, skip the entire subtree - if s.shouldExclude(filePath, path) { - if info.IsDir() { - return filepath.SkipDir - } - - return nil - } - // Handle symlinks and directories handled, err := s.recordSpecialEntry( filePath, info, existingFiles, knownFiles, collector, result) @@ -985,7 +1003,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{}{} @@ -1009,15 +1027,38 @@ func (s *Scanner) recordSpecialEntry( return false, nil } +// countRemovedDuringWalk counts an entry that was in its directory's +// listing but was gone when the walk looked it up, like a file removed +// before phase 2 reads it: as examined and failed, and not as an error, +// with or without --skip-errors. +func countRemovedDuringWalk( + filePath string, existingFiles map[string]struct{}, result *ScanResult, +) { + log.Warn("File was deleted during the directory walk, skipping", + "path", filePath) + + // The walk found it, so it is not also counted as deleted. + existingFiles[filePath] = struct{}{} + result.FilesScanned++ + result.FilesFailed++ +} + // 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 } @@ -1459,6 +1500,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 6d74865..5f533ba 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 @@ -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 diff --git a/internal/vaultik/snapshot_deleted_count_test.go b/internal/vaultik/snapshot_deleted_count_test.go index 80dbd0a..24f78fd 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 2742115..0c906fe 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 af07afe..4864760 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,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.") +} -- 2.54.0