diff --git a/README.md b/README.md index 355322d..aff6b19 100644 --- a/README.md +++ b/README.md @@ -375,7 +375,9 @@ on the destination in one go, use `vaultik remote nuke --force`. * Optional path arguments to restore specific files/directories (default: all) * Preserves file permissions, timestamps, ownership (ownership requires root), symlinks, and empty directories -* `--verify`: After restoring, verify every file's chunk hashes match +* `--verify`: After restoring, verify every restored file's chunk hashes + match. A file skipped under `--skip-errors` is reported as failed to + restore and is not verified. **`prune`**: Tidy up everything that isn't needed. Runs three passes: (1) reconcile the local index against the destination store — any diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index c5843e8..352a608 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -103,11 +103,12 @@ type RestoreResult struct { BlobsDownloaded int BytesDownloaded int64 Duration time.Duration + FilesFailed int + FailedFiles []string // Paths of files that failed to restore // Verification results (only populated if Verify option is set) - FilesVerified int - BytesVerified int64 - FilesFailed int - FailedFiles []string // Paths of files that failed verification + FilesVerified int + BytesVerified int64 + VerifyFailedFiles []string // Paths of restored files that failed verification } // Restore restores files from a snapshot to the target directory @@ -647,20 +648,34 @@ func (v *Vaultik) handleRestoreVerification( opts *RestoreOptions, result *RestoreResult, ) error { - err := v.verifyRestoredFiles(v.ctx, repos, files, opts.TargetDir, result) + // A file that failed to restore was not written and is already + // reported as such, so it is not verified. + failedToRestore := make(map[string]bool, len(result.FailedFiles)) + for _, path := range result.FailedFiles { + failedToRestore[path] = true + } + + restored := make([]*database.File, 0, len(files)) + for _, file := range files { + if !failedToRestore[file.Path.String()] { + restored = append(restored, file) + } + } + + err := v.verifyRestoredFiles(v.ctx, repos, restored, opts.TargetDir, result) if err != nil { return fmt.Errorf("verification failed: %w", err) } - if result.FilesFailed > 0 { + if len(result.VerifyFailedFiles) > 0 { v.UI.Errorf("Verification failed: %s files did not match expected checksums.", - v.UI.Count(result.FilesFailed)) + v.UI.Count(len(result.VerifyFailedFiles))) - for _, path := range result.FailedFiles { + for _, path := range result.VerifyFailedFiles { v.UI.Detailf("%s", v.UI.Path(path)) } - return fmt.Errorf("%d %w", result.FilesFailed, errFilesFailedVerify) + return fmt.Errorf("%d %w", len(result.VerifyFailedFiles), errFilesFailedVerify) } v.UI.Completef("Verified %s files (%s).", @@ -1420,8 +1435,8 @@ func (v *Vaultik) verifyRestoredFiles( if err != nil { log.Error("File verification failed", "path", file.Path, "error", err) - result.FilesFailed++ - result.FailedFiles = append(result.FailedFiles, file.Path.String()) + result.VerifyFailedFiles = append( + result.VerifyFailedFiles, file.Path.String()) } bytesProcessed += file.Size @@ -1437,7 +1452,7 @@ func (v *Vaultik) verifyRestoredFiles( log.Info("Verification complete", "files_verified", result.FilesVerified, "bytes_verified", ubytes(result.BytesVerified), - "files_failed", result.FilesFailed, + "files_failed", len(result.VerifyFailedFiles), ) return nil diff --git a/internal/vaultik/restore_skip_errors_test.go b/internal/vaultik/restore_skip_errors_test.go index 7cbfeff..79070d3 100644 --- a/internal/vaultik/restore_skip_errors_test.go +++ b/internal/vaultik/restore_skip_errors_test.go @@ -4,7 +4,9 @@ import ( "bytes" "context" "fmt" + "os" "path/filepath" + "strings" "testing" "github.com/spf13/afero" @@ -84,6 +86,130 @@ func TestRestoreSkipErrorsSkipsFilesOfMissingBlob(t *testing.T) { } } +// TestRestoreSkipErrorsVerifyChecksOnlyRestoredFiles restores with +// SkipErrors and Verify after one blob of a two-blob snapshot was +// deleted. Verification must check only the files that were written, so +// a file of the deleted blob is reported as failed to restore and not +// also as failing verification, and Restore must still return an error. +func TestRestoreSkipErrorsVerifyChecksOnlyRestoredFiles(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + backup := backupThenDeleteOneBlob(ctx, t) + + var out bytes.Buffer + + v := newReaderVaultik(ctx, backup.cfg, backup.storer, nil, backup.fs) + v.UI = ui.NewWithColor(&out, false) + + err := v.Restore(&vaultik.RestoreOptions{ + SnapshotID: backup.snapshotID, + TargetDir: backup.restoreDir, + Verify: true, + SkipErrors: true, + }) + + require.Error(t, err, "restore must fail when files were skipped") + assert.Contains(t, err.Error(), + fmt.Sprintf("%d file(s) failed to restore", len(backup.lost))) + + written := len(backup.files) - len(backup.lost) + + assert.Contains(t, out.String(), fmt.Sprintf("Verifying %d files", written)) + assert.Contains(t, out.String(), fmt.Sprintf("Verified %d files", written)) + assert.NotContains(t, out.String(), "Verification failed") +} + +// TestRestoreSkipErrorsVerifyReportsFailuresApart restores with +// SkipErrors and Verify after one blob of a two-blob snapshot was +// deleted, while one file that does restore is written one byte too long. +// The restore warning must list only the files of the deleted blob, the +// verification summary only the long file, each path once, and Restore +// must return an error. +func TestRestoreSkipErrorsVerifyReportsFailuresApart(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + backup := backupThenDeleteOneBlob(ctx, t) + + var long string + + for path := range backup.files { + if !backup.lost[path] { + long = path + + break + } + } + + var out bytes.Buffer + + fs := trailingByteFs{Fs: backup.fs, suffix: long} + v := newReaderVaultik(ctx, backup.cfg, backup.storer, nil, fs) + v.UI = ui.NewWithColor(&out, false) + + err := v.Restore(&vaultik.RestoreOptions{ + SnapshotID: backup.snapshotID, + TargetDir: backup.restoreDir, + Verify: true, + SkipErrors: true, + }) + require.Error(t, err, "restore must fail when files failed") + + _, afterWarning, found := strings.Cut(out.String(), + fmt.Sprintf("%d file(s) failed to restore:", len(backup.lost))) + require.Truef(t, found, "no restore warning in:\n%s", out.String()) + + warning, summary, found := strings.Cut(afterWarning, + "Verification failed: 1 files did not match") + require.Truef(t, found, "no verification summary in:\n%s", out.String()) + + for path := range backup.lost { + assert.Equalf(t, 1, strings.Count(warning, path), + "%s must be listed once as failed to restore", path) + assert.NotContainsf(t, summary, path, + "%s was not restored and must not fail verification", path) + } + + assert.NotContainsf(t, warning, long, + "%s was restored and must not be listed as failed to restore", long) + assert.Equalf(t, 1, strings.Count(summary, long), + "%s must be listed once as failing verification", long) +} + +// trailingByteFs appends one byte, on close, to the file it opens whose +// path ends in suffix, so that file is restored one byte too long and +// fails verification. +type trailingByteFs struct { + afero.Fs + + suffix string +} + +//nolint:ireturn // afero.Fs.OpenFile is defined to return the interface +func (fs trailingByteFs) OpenFile( + name string, flag int, perm os.FileMode, +) (afero.File, error) { + f, err := fs.Fs.OpenFile(name, flag, perm) + if err != nil || !strings.HasSuffix(name, fs.suffix) { + return f, err + } + + return trailingByteFile{File: f}, nil +} + +type trailingByteFile struct { + afero.File +} + +func (f trailingByteFile) Close() error { + _, _ = f.File.Write([]byte{'x'}) + + return f.File.Close() +} + // TestRestoreMissingBlobAbortsWithoutSkipErrors checks that a deleted blob // still ends the restore with an error when SkipErrors is not set. func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) {