From c7890953b5ab0d28199ffad96c111b8c40b91772 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 6 Oct 2026 09:41:45 +0000 Subject: [PATCH] Skip files of an unreadable blob under restore --skip-errors (closes #218) A blob that failed to download ended `snapshot restore` even with --skip-errors, after restoring whichever files came first. The download error now goes through the same per-file handling as any other restore error, once for every pending file that references the blob. With --skip-errors those files are reported as failed, the rest are restored, and the command still exits non-zero. Without the flag the restore still aborts; the error now also names one affected file and suggests --skip-errors. A cancelled restore still ends at once. The --skip-errors help text and README now limit the packing and storage caveat to snapshot creation. Model: opus-5-5 --- README.md | 2 +- TODO.md | 8 + internal/cli/root.go | 3 +- internal/vaultik/restore.go | 30 ++- internal/vaultik/restore_plan.go | 14 ++ internal/vaultik/restore_skip_errors_test.go | 194 +++++++++++++++++++ 6 files changed, 246 insertions(+), 5 deletions(-) create mode 100644 internal/vaultik/restore_skip_errors_test.go diff --git a/README.md b/README.md index ecd7622..ed3f984 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 (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. ### locking diff --git a/TODO.md b/TODO.md index 2336169..911ee8b 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,14 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-06: Made `snapshot restore --skip-errors` skip the files that + need a blob it cannot download + ([issue #218](https://git.eeqj.de/sneak/vaultik/issues/218)). A missing + or damaged blob ended the restore even with the flag, after restoring + whichever files happened to come first. Every file that needs such a + blob is now reported as failed, the rest are restored, and the command + still exits non-zero. Without the flag the blob error still aborts. + - 2026-10-06: Made the local index actually run in WAL mode with a busy timeout ([issue #217](https://git.eeqj.de/sneak/vaultik/issues/217)). The connection settings were written in a form the SQLite driver diff --git a/internal/cli/root.go b/internal/cli/root.go index 7f24423..463eb1d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -60,7 +60,8 @@ on the source system.`, cmd.PersistentFlags().BoolVar(&rootFlags.SkipErrors, "skip-errors", false, "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)") + "(packing and storage errors while creating a snapshot still "+ + "abort)") // Add subcommands cmd.AddCommand( diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index 118a68b..f8372d6 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -355,7 +355,7 @@ func (v *Vaultik) runRestoreLoop( fileID, ready := plan.popReady() if !ready { - downloaded, err := session.downloadNextBlobSet(plan) + downloaded, err := session.downloadNextBlobSet(plan, filesByID) if err != nil { return err } @@ -411,7 +411,13 @@ func (v *Vaultik) runRestoreLoop( // blob set and downloads its blobs; after each blob lands, the plan // moves any pending file whose set just emptied onto the ready queue. // Returns false when nothing is pending download (the caller stops). -func (s *restoreSession) downloadNextBlobSet(plan *restorePlan) (bool, error) { +// +// A blob that cannot be downloaded is reported through +// handleRestoreFileError for every pending file that references it, so +// it aborts the restore unless --skip-errors is set. +func (s *restoreSession) downloadNextBlobSet( + plan *restorePlan, filesByID map[types.FileID]*database.File, +) (bool, error) { s.sweeper.sweep() next, ok := plan.pickNextDownload() @@ -433,7 +439,25 @@ func (s *restoreSession) downloadNextBlobSet(plan *restorePlan) (bool, error) { err := s.downloadBlobToCache(hash, blob.CompressedSize, blob.UncompressedSize) if err != nil { - return false, fmt.Errorf("downloading blob %s: %w", shortHash(hash), err) + err = fmt.Errorf("downloading blob %s: %w", shortHash(hash), err) + + // On cancel the error says nothing about the blob, so it ends + // the restore instead of failing the files that need it. + if s.ctx.Err() != nil { + return false, err + } + + for _, fileID := range plan.filesReferencingBlob(hash) { + fileErr := s.v.handleRestoreFileError( + plan, s.opts, s.result, filesByID[fileID], fileID, err) + if fileErr != nil { + return false, fileErr + } + } + + // next is among the failed files, so the rest of its blob set + // is left for any other file that still needs it. + return true, nil } s.result.BlobsDownloaded++ diff --git a/internal/vaultik/restore_plan.go b/internal/vaultik/restore_plan.go index 8ec0f1f..d9805fa 100644 --- a/internal/vaultik/restore_plan.go +++ b/internal/vaultik/restore_plan.go @@ -214,6 +214,20 @@ func (p *restorePlan) blobsNeeded(fileID types.FileID) []string { return out } +// filesReferencingBlob returns the pending files that reference the +// named blob, in any order. The result is a copy, so the caller may +// finishFile each of them while ranging over it. +func (p *restorePlan) filesReferencingBlob(blobHash string) []types.FileID { + files := p.blobFiles[blobHash] + + out := make([]types.FileID, 0, len(files)) + for id := range files { + out = append(out, id) + } + + return out +} + // hasPending reports whether any unfinished files remain. func (p *restorePlan) hasPending() bool { return len(p.fileBlobs) > 0 diff --git a/internal/vaultik/restore_skip_errors_test.go b/internal/vaultik/restore_skip_errors_test.go new file mode 100644 index 0000000..bb19ff4 --- /dev/null +++ b/internal/vaultik/restore_skip_errors_test.go @@ -0,0 +1,194 @@ +package vaultik_test + +import ( + "bytes" + "context" + "fmt" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/config" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/storage" + "sneak.berlin/go/vaultik/internal/ui" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// A file no larger than the chunker's minimum chunk size (a quarter of +// faultChunkSize) is stored as one chunk, so it lives in exactly one blob. +// More of them than fit in faultMaxBlobSize make the snapshot span two +// blobs. +const ( + missingBlobFileBytes = int(faultChunkSize / 4) + missingBlobFileCount = 20 +) + +// missingBlobBackup is a snapshot of single-chunk files spread over two +// blobs, with one of those blobs deleted from the store. +type missingBlobBackup struct { + fs afero.Fs + cfg *config.Config + storer storage.Storer + snapshotID string + restoreDir string + // files holds the original content by source path. + files map[string][]byte + // lost holds the source paths whose chunk is in the deleted blob. + lost map[string]bool +} + +// TestRestoreSkipErrorsSkipsFilesOfMissingBlob restores with SkipErrors +// after one blob of a two-blob snapshot was deleted. Every file stored in +// that blob must be reported as failed and left absent, every other file +// must be restored intact, and Restore must still return an error. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestRestoreSkipErrorsSkipsFilesOfMissingBlob(t *testing.T) { + 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, + 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))) + + for path, content := range backup.files { + restored := filepath.Join(backup.restoreDir, path) + + if backup.lost[path] { + assert.Containsf(t, out.String(), path, + "%s needs the deleted blob and must be reported", path) + assert.NoFileExists(t, restored) + + continue + } + + got, err := afero.ReadFile(backup.fs, restored) + require.NoErrorf(t, err, "%s does not need the deleted blob", path) + assert.Equalf(t, content, got, "%s restored with wrong content", path) + } +} + +// TestRestoreMissingBlobAbortsWithoutSkipErrors checks that a deleted blob +// still ends the restore with an error when SkipErrors is not set. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) { + ctx := context.Background() + backup := backupThenDeleteOneBlob(ctx, t) + + v := newReaderVaultik(ctx, backup.cfg, backup.storer, nil, backup.fs) + + err := v.Restore(&vaultik.RestoreOptions{ + SnapshotID: backup.snapshotID, + TargetDir: backup.restoreDir, + }) + + require.ErrorIs(t, err, storage.ErrNotFound) +} + +// backupThenDeleteOneBlob backs up missingBlobFileCount single-chunk +// files, then deletes from the store the blob holding the first of them. +func backupThenDeleteOneBlob( + ctx context.Context, t *testing.T, +) *missingBlobBackup { + t.Helper() + log.Initialize(log.Config{}) + + fs := afero.NewOsFs() + tempDir := t.TempDir() + dataDir := filepath.Join(tempDir, "src") + dbPath := filepath.Join(tempDir, "index.sqlite") + cfg := faultTestConfig() + + require.NoError(t, fs.MkdirAll(dataDir, 0o755)) + + files := make(map[string][]byte, missingBlobFileCount) + + for i := range missingBlobFileCount { + path := filepath.Join(dataDir, fmt.Sprintf("file-%02d.bin", i)) + files[path] = bytesPattern( + fmt.Sprintf("file-%02d-", i), missingBlobFileBytes) + require.NoError(t, afero.WriteFile(fs, path, files[path], 0o644)) + } + + storer, err := storage.NewFileStorer(filepath.Join(tempDir, "remote")) + require.NoError(t, err) + + db, err := database.New(ctx, dbPath) + require.NoError(t, err) + + repos := database.NewRepositories(db) + + id := fullFaultBackup( + ctx, t, fs, storer, cfg, repos, dataDir, dbPath, "missingblob") + + blobOfFile := make(map[string]string, len(files)) + for path := range files { + blobOfFile[path] = blobHashOfSingleChunkFile(ctx, t, repos, path) + } + + require.NoError(t, db.Close()) + + deleted := blobOfFile[filepath.Join(dataDir, "file-00.bin")] + require.NoError(t, storer.Delete(ctx, fmt.Sprintf( + "blobs/%s/%s/%s", deleted[:2], deleted[2:4], deleted))) + + lost := make(map[string]bool) + + for path, hash := range blobOfFile { + if hash == deleted { + lost[path] = true + } + } + + require.Less(t, len(lost), len(files), + "the snapshot must span more than one blob") + + return &missingBlobBackup{ + fs: fs, + cfg: cfg, + storer: storer, + snapshotID: id, + restoreDir: filepath.Join(tempDir, "restored"), + files: files, + lost: lost, + } +} + +// blobHashOfSingleChunkFile returns the hash of the blob holding the one +// chunk of the file at path, as recorded in the local index. +func blobHashOfSingleChunkFile( + ctx context.Context, t *testing.T, + repos *database.Repositories, path string, +) string { + t.Helper() + + chunks, err := repos.FileChunks.GetByPath(ctx, path) + require.NoError(t, err) + require.Lenf(t, chunks, 1, "%s must be a single chunk", path) + + blobChunk, err := repos.BlobChunks.GetByChunkHash( + ctx, chunks[0].ChunkHash.String()) + require.NoError(t, err) + require.NotNilf(t, blobChunk, "chunk of %s is in no blob", path) + + blob, err := repos.Blobs.GetByID(ctx, blobChunk.BlobID.String()) + require.NoError(t, err) + + return blob.Hash.String() +}