diff --git a/README.md b/README.md index 06fcb65..fa5e3d4 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,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`: Continue past per-file errors instead of aborting (applies to `snapshot create` and `restore`) +* `--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 (which would leave a chunk recorded but not stored) still abort the run. ### locking diff --git a/internal/cli/root.go b/internal/cli/root.go index 0c8c6ce..fcfa60e 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -57,8 +57,9 @@ on the source system.`, cmd.PersistentFlags().BoolVarP(&rootFlags.Quiet, "quiet", "q", false, "Suppress non-error output") cmd.PersistentFlags().BoolVar(&rootFlags.SkipErrors, "skip-errors", false, - "Continue past per-file errors instead of aborting "+ - "(applies to snapshot create and restore)") + "Skip files that cannot be read when creating a snapshot, or "+ + "that cannot be restored when restoring, instead of aborting "+ + "(packing and storage errors still abort)") // Add subcommands cmd.AddCommand( diff --git a/internal/snapshot/scanner.go b/internal/snapshot/scanner.go index bd85feb..319e866 100644 --- a/internal/snapshot/scanner.go +++ b/internal/snapshot/scanner.go @@ -63,7 +63,9 @@ type Scanner struct { exclude []string // Glob patterns for files/directories to exclude compiledExclude []compiledPattern // Compiled glob patterns progress *ProgressReporter - skipErrors bool // Skip file read errors (log loudly but continue) + // skipErrors skips files that cannot be opened or read (logged loudly); + // packer, database, encryption, and upload errors still abort the run. + skipErrors bool // ui is the user-facing output; never nil (defaults to a discarding writer). ui *ui.Writer @@ -121,7 +123,9 @@ type ScannerConfig struct { EnableProgress bool // Enable the live progress reporter (ETAs, throughput) UI *ui.Writer // Where user-facing scanner messages go; nil = discard Exclude []string // Glob patterns for files/directories to exclude - SkipErrors bool // Skip file read errors (log loudly but continue) + // SkipErrors skips files that cannot be opened or read (log loudly but + // continue); packer, database, encryption, and upload errors still abort. + SkipErrors bool } // ScanResult contains the results of a scan operation @@ -1336,6 +1340,15 @@ func (s *Scanner) processFileWithErrorHandling( ) (bool, error) { err := s.processFileStreaming(ctx, fileToProcess, result) if err != nil { + // A packer/database/encryption/upload failure means the chunk's data + // may not have been stored. Skipping the file would let the snapshot + // record a file whose chunk is in no blob and cannot be restored, so + // abort the run even under --skip-errors. Only open and read errors + // are skipped below. + var pErr *packerError + if errors.As(err, &pErr) { + return false, fmt.Errorf("processing file %s: %w", fileToProcess.Path, err) + } // Handle files that were deleted between scan and process phases if errors.Is(err, os.ErrNotExist) { log.Warn("File was deleted during backup, skipping", @@ -1345,7 +1358,7 @@ func (s *Scanner) processFileWithErrorHandling( return true, nil } - // Skip file read errors if --skip-errors is enabled + // Skip open/read errors if --skip-errors is enabled if s.skipErrors { log.Error("Failed to process file (skipping due to --skip-errors)", "path", fileToProcess.Path, "error", err) @@ -1712,6 +1725,20 @@ type streamingChunkInfo struct { size int64 } +// packerError marks an error that came from adding a chunk to the packer +// (packing, database, encryption, or upload). Such an error means the chunk's +// data may not have been stored, so the run must abort even under --skip-errors: +// skipping the file would leave the chunk recorded as backed up while it lives +// in no blob, and a later snapshot could record a file that cannot be restored. +// Only open and read errors are safe to skip. +type packerError struct { + err error +} + +func (e *packerError) Error() string { return e.err.Error() } + +func (e *packerError) Unwrap() error { return e.err } + // processFileStreaming processes a file by streaming chunks directly to the packer func (s *Scanner) processFileStreaming( ctx context.Context, fileToProcess *FileToProcess, result *ScanResult, @@ -1762,7 +1789,11 @@ func (s *Scanner) processFileStreaming( if !chunkExists { err := s.addChunkToPacker(ctx, chunk) if err != nil { - return err + // Mark as a packer error so --skip-errors cannot swallow it: + // the chunk was registered as pending before packing, so a + // skipped file here would be recorded as backed up while its + // data was never stored. + return &packerError{err: err} } } diff --git a/internal/snapshot/skip_errors_test.go b/internal/snapshot/skip_errors_test.go new file mode 100644 index 0000000..9ae128a --- /dev/null +++ b/internal/snapshot/skip_errors_test.go @@ -0,0 +1,216 @@ +package snapshot_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/spf13/afero" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/snapshot" +) + +// errSimTempFail is the one-time temp-file creation failure blobTempFailFs +// injects, mirroring a full temp filesystem. +var errSimTempFail = errors.New("simulated temp-file creation failure") + +// errSimRead is the read failure readFailFile injects for a file that opens +// but cannot be read. +var errSimRead = errors.New("simulated read failure") + +// blobTempFailFs fails the first temp-file creation for a packer blob, then +// behaves normally, simulating a one-time failure to start a new blob. +type blobTempFailFs struct { + afero.Fs + + mu sync.Mutex + failed bool +} + +//nolint:ireturn // afero.Fs.OpenFile is defined to return the interface. +func (f *blobTempFailFs) OpenFile( + name string, flag int, perm os.FileMode, +) (afero.File, error) { + if strings.Contains(name, "vaultik-blob-") { + f.mu.Lock() + firstTime := !f.failed + f.failed = true + f.mu.Unlock() + + if firstTime { + return nil, errSimTempFail + } + } + + return f.Fs.OpenFile(name, flag, perm) +} + +// readFailFile wraps an afero.File whose Read always fails. +type readFailFile struct { + afero.File +} + +func (readFailFile) Read([]byte) (int, error) { + return 0, errSimRead +} + +// readFailFs fails reads of one target path after a successful open. +type readFailFs struct { + afero.Fs + + target string +} + +//nolint:ireturn // afero.Fs.Open is defined to return the interface. +func (f *readFailFs) Open(name string) (afero.File, error) { + file, err := f.Fs.Open(name) + if err != nil { + return nil, err + } + + if name == f.target { + return readFailFile{File: file}, nil + } + + return file, nil +} + +// writeSkipErrorTestFile writes one file into fs with a fixed mtime. +func writeSkipErrorTestFile(t *testing.T, fs afero.Fs, path, content string) { + t.Helper() + + err := fs.MkdirAll(filepath.Dir(path), 0755) + if err != nil { + t.Fatalf("mkdir: %v", err) + } + + err = afero.WriteFile(fs, path, []byte(content), 0644) + if err != nil { + t.Fatalf("write %s: %v", path, err) + } + + when := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC) + + err = fs.Chtimes(path, when, when) + if err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } +} + +// runSkipErrorScan scans /source on fs with the given skip-errors setting and +// returns the repositories (for inspection) and the scan error. +func runSkipErrorScan( + t *testing.T, fs afero.Fs, skipErrors bool, +) (*database.Repositories, error) { + t.Helper() + + db, err := database.NewTestDB() + if err != nil { + t.Fatalf("create test db: %v", err) + } + + t.Cleanup(func() { + cerr := db.Close() + if cerr != nil { + t.Errorf("close db: %v", cerr) + } + }) + + repos := database.NewRepositories(db) + + scanner := snapshot.NewScanner(snapshot.ScannerConfig{ + FS: fs, + ChunkSize: int64(1024 * 16), + Repositories: repos, + MaxBlobSize: int64(1024 * 1024), + CompressionLevel: 3, + AgeRecipients: []string{testAgePublicKey}, + SkipErrors: skipErrors, + }) + + ctx := context.Background() + snapshotID := "test-snapshot-skip-errors" + createTestSnapshotRecord(ctx, t, repos, snapshotID) + + _, err = scanner.Scan(ctx, "/source", snapshotID) + + return repos, err +} + +// TestScannerPackingFailureAbortsUnderSkipErrors checks that a failure to start +// a new blob aborts the run even with --skip-errors. Otherwise the file would +// be skipped while its chunk had already been registered as pending, letting a +// later blob record that chunk in the chunks table with no blob to back it — +// a snapshot that completes with a file that cannot be restored. +func TestScannerPackingFailureAbortsUnderSkipErrors(t *testing.T) { + t.Parallel() + + // Two files with distinct content so each yields a distinct chunk: the + // first fails to start a blob, and without the fix the second's blob would + // commit the first's orphaned chunk row. + fs := &blobTempFailFs{Fs: afero.NewMemMapFs()} + writeSkipErrorTestFile(t, fs, "/source/file1.txt", "first file content") + writeSkipErrorTestFile(t, fs, "/source/file2.txt", "second file content") + + repos, err := runSkipErrorScan(t, fs, true) + if err == nil { + t.Fatal("expected scan to abort on the packer error, got nil") + } + + // ListUnpacked returns chunks recorded with no blob_chunks row: exactly the + // unrestorable state this fix prevents. + unpacked, err := repos.Chunks.ListUnpacked(context.Background(), 10) + if err != nil { + t.Fatalf("listing unpacked chunks: %v", err) + } + + if len(unpacked) != 0 { + t.Fatalf("expected no chunk recorded without a blob, got %d", len(unpacked)) + } +} + +// TestScannerReadErrorAbortsWithoutSkipErrors checks that a file read error +// aborts the run when --skip-errors is not set. +func TestScannerReadErrorAbortsWithoutSkipErrors(t *testing.T) { + t.Parallel() + + const target = "/source/unreadable.txt" + + fs := &readFailFs{Fs: afero.NewMemMapFs(), target: target} + writeSkipErrorTestFile(t, fs, target, "content that cannot be read") + + _, err := runSkipErrorScan(t, fs, false) + if err == nil { + t.Fatal("expected scan to fail on the read error, got nil") + } +} + +// TestScannerReadErrorSkippedWithSkipErrors checks that a file read error is +// skipped and the run completes when --skip-errors is set. +func TestScannerReadErrorSkippedWithSkipErrors(t *testing.T) { + t.Parallel() + + const target = "/source/unreadable.txt" + + fs := &readFailFs{Fs: afero.NewMemMapFs(), target: target} + writeSkipErrorTestFile(t, fs, target, "content that cannot be read") + + repos, err := runSkipErrorScan(t, fs, true) + if err != nil { + t.Fatalf("expected scan to complete with --skip-errors, got %v", err) + } + + chunks, err := repos.FileChunks.GetByFile(context.Background(), target) + if err != nil { + t.Fatalf("getting file chunks: %v", err) + } + + if len(chunks) != 0 { + t.Fatalf("expected unreadable file skipped, got %d chunks", len(chunks)) + } +}