From a3f3f4e94d44f7cdbe51678f19bd0f0b9cf075f1 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 13 Oct 2026 08:59:40 +0000 Subject: [PATCH] Restore the other files when a chunk is in no blob under --skip-errors (closes #326) Restore planning stopped at the first file needing a chunk that no blob holds, so --skip-errors never saw that file and nothing was restored. The planner now leaves such a file out of the plan and returns it with the reason, and each one goes through handleRestoreFileError like any other failed file: skipped and counted under --skip-errors, fatal without it. The fatal error now suggests --skip-errors, as other restore failures do. The skip-errors tests now share one backup helper; the new case removes a blob from the snapshot metadata and exports it again. Model: opus-5-5 --- internal/vaultik/restore.go | 13 +- internal/vaultik/restore_malformed_db_test.go | 10 +- internal/vaultik/restore_plan.go | 25 ++-- internal/vaultik/restore_skip_errors_test.go | 119 +++++++++++++++--- 4 files changed, 138 insertions(+), 29 deletions(-) diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index c5843e8..08e71c2 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -309,11 +309,22 @@ func (v *Vaultik) restoreAllFiles( return nil, err } - plan, err := newRestorePlan(v.ctx, repos, files, chunkToBlobMap, blobIDToHash) + plan, unplanned, err := newRestorePlan( + v.ctx, repos, files, chunkToBlobMap, blobIDToHash) if err != nil { return nil, fmt.Errorf("building restore plan: %w", err) } + // Ranging over files rather than the map reports them in a stable order. + for _, file := range files { + if planErr, ok := unplanned[file.ID]; ok { + err = v.handleRestoreFileError(plan, opts, result, file, file.ID, planErr) + if err != nil { + return nil, err + } + } + } + filesByID, totalBytesExpected := indexRestoreFiles(files) v.UI.Beginf("Restoring %s files (%s) to %s.", diff --git a/internal/vaultik/restore_malformed_db_test.go b/internal/vaultik/restore_malformed_db_test.go index b6c884d..661d18b 100644 --- a/internal/vaultik/restore_malformed_db_test.go +++ b/internal/vaultik/restore_malformed_db_test.go @@ -61,8 +61,8 @@ func craftedFile( } // TestRestoreShortChunkHashInNoBlob proves a file whose short chunk hash -// has no blob_chunks row fails restore planning and the chunk write with -// an error. +// has no blob_chunks row is left out of the restore plan with an error, +// and fails the chunk write with an error. func TestRestoreShortChunkHashInNoBlob(t *testing.T) { t.Parallel() @@ -77,9 +77,11 @@ func TestRestoreShortChunkHashInNoBlob(t *testing.T) { chunkToBlobMap, err := v.buildChunkToBlobMap(ctx, repos) require.NoError(t, err) - _, err = newRestorePlan(ctx, repos, []*database.File{file}, + plan, unplanned, err := newRestorePlan(ctx, repos, []*database.File{file}, chunkToBlobMap, map[string]string{}) - require.ErrorIs(t, err, errPlanChunkMissing) + require.NoError(t, err) + require.ErrorIs(t, unplanned[file.ID], errPlanChunkMissing) + require.False(t, plan.hasPending()) fileChunks, err := repos.FileChunks.GetByFileID(ctx, file.ID) require.NoError(t, err) diff --git a/internal/vaultik/restore_plan.go b/internal/vaultik/restore_plan.go index c018b79..2615c52 100644 --- a/internal/vaultik/restore_plan.go +++ b/internal/vaultik/restore_plan.go @@ -41,20 +41,25 @@ type restorePlan struct { // newRestorePlan builds the file→blob index for the given files. Files // whose chunks reference no blobs (symlinks, directories) start in the -// ready queue immediately. +// ready queue immediately. A file with a chunk that no blob holds cannot +// be restored: it is left out of the plan and returned, by ID, with the +// reason. func newRestorePlan( ctx context.Context, repos *database.Repositories, files []*database.File, chunkToBlobMap map[string]*database.BlobChunk, blobIDToHash map[string]string, -) (*restorePlan, error) { +) (*restorePlan, map[types.FileID]error, error) { p := &restorePlan{ fileBlobs: make(map[types.FileID]map[string]struct{}, len(files)), blobFiles: make(map[string]map[types.FileID]struct{}), ready: make([]types.FileID, 0, len(files)), cached: make(map[string]struct{}), } + unplanned := make(map[types.FileID]error) + +nextFile: for _, f := range files { if f.IsSymlink() || f.Mode&uint32(os.ModeDir) != 0 { // No chunks to fetch — restore can run immediately. @@ -66,7 +71,7 @@ func newRestorePlan( fileChunks, err := repos.FileChunks.GetByFileID(ctx, f.ID) if err != nil { - return nil, fmt.Errorf("planning %s: %w", f.Path, err) + return nil, nil, fmt.Errorf("planning %s: %w", f.Path, err) } blobs := make(map[string]struct{}) @@ -74,14 +79,18 @@ func newRestorePlan( for _, fc := range fileChunks { bc, ok := chunkToBlobMap[fc.ChunkHash.String()] if !ok { - return nil, fmt.Errorf("planning %s: %w: %s", - f.Path, errPlanChunkMissing, shortHash(fc.ChunkHash.String())) + unplanned[f.ID] = fmt.Errorf("%w: %s", + errPlanChunkMissing, shortHash(fc.ChunkHash.String())) + + continue nextFile } hash, ok := blobIDToHash[bc.BlobID.String()] if !ok { - return nil, fmt.Errorf("planning %s: %w: %s", - f.Path, errPlanBlobIDMissing, bc.BlobID) + unplanned[f.ID] = fmt.Errorf("%w: %s", + errPlanBlobIDMissing, bc.BlobID) + + continue nextFile } blobs[hash] = struct{}{} @@ -103,7 +112,7 @@ func newRestorePlan( } } - return p, nil + return p, unplanned, nil } // markBlobCached records that the named blob is now resident in the diff --git a/internal/vaultik/restore_skip_errors_test.go b/internal/vaultik/restore_skip_errors_test.go index 7cbfeff..c74b691 100644 --- a/internal/vaultik/restore_skip_errors_test.go +++ b/internal/vaultik/restore_skip_errors_test.go @@ -28,7 +28,8 @@ const ( ) // missingBlobBackup is a snapshot of single-chunk files spread over two -// blobs, with one of those blobs deleted from the store. +// blobs, with one of those blobs lost: deleted from the store, or missing +// from the snapshot metadata. type missingBlobBackup struct { fs afero.Fs cfg *config.Config @@ -37,20 +38,39 @@ type missingBlobBackup struct { 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 holds the source paths whose chunk is in the lost 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. +// after one blob of a two-blob snapshot was deleted from the store. func TestRestoreSkipErrorsSkipsFilesOfMissingBlob(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() ctx := context.Background() - backup := backupThenDeleteOneBlob(ctx, t) + assertRestoreSkipsLostFiles(ctx, t, backupThenDeleteOneBlob(ctx, t)) +} + +// TestRestoreSkipErrorsSkipsFilesWithChunkInNoBlob restores with +// SkipErrors a snapshot whose metadata lists chunks that no blob holds, as +// earlier backup defects left behind. +func TestRestoreSkipErrorsSkipsFilesWithChunkInNoBlob(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + assertRestoreSkipsLostFiles(ctx, t, backupThenDropOneBlobFromMetadata(ctx, t)) +} + +// assertRestoreSkipsLostFiles restores backup with SkipErrors. Every file +// stored in the lost blob must be reported as failed and left absent, +// every other file must be restored intact, and Restore must still return +// an error. +func assertRestoreSkipsLostFiles( + ctx context.Context, t *testing.T, backup *missingBlobBackup, +) { + t.Helper() var out bytes.Buffer @@ -72,14 +92,14 @@ func TestRestoreSkipErrorsSkipsFilesOfMissingBlob(t *testing.T) { if backup.lost[path] { assert.Containsf(t, out.String(), path, - "%s needs the deleted blob and must be reported", path) + "%s needs the lost 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) + require.NoErrorf(t, err, "%s does not need the lost blob", path) assert.Equalf(t, content, got, "%s restored with wrong content", path) } } @@ -103,6 +123,30 @@ func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) { require.ErrorIs(t, err, storage.ErrNotFound) } +// TestRestoreChunkInNoBlobAbortsWithoutSkipErrors checks that a chunk held +// by no blob still ends the restore, before any file is written, when +// SkipErrors is not set. +func TestRestoreChunkInNoBlobAbortsWithoutSkipErrors(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + backup := backupThenDropOneBlobFromMetadata(ctx, t) + + v := newReaderVaultik(ctx, backup.cfg, backup.storer, nil, backup.fs) + + err := v.Restore(&vaultik.RestoreOptions{ + SnapshotID: backup.snapshotID, + TargetDir: backup.restoreDir, + }) + + require.ErrorContains(t, err, "chunk missing from blob map") + + for path := range backup.files { + assert.NoFileExists(t, filepath.Join(backup.restoreDir, path)) + } +} + // backupThenDeleteOneBlob backs up missingBlobFileCount single-chunk // files, then deletes from the store the blob holding the first of them. func backupThenDeleteOneBlob( @@ -110,6 +154,54 @@ func backupThenDeleteOneBlob( ) *missingBlobBackup { t.Helper() + backup, db, lostBlob := backupSingleChunkFiles(ctx, t) + require.NoError(t, db.Close()) + + require.NoError(t, backup.storer.Delete(ctx, fmt.Sprintf( + "blobs/%s/%s/%s", lostBlob[:2], lostBlob[2:4], lostBlob))) + + return backup +} + +// backupThenDropOneBlobFromMetadata backs up missingBlobFileCount +// single-chunk files, then removes the blob holding the first of them from +// the snapshot and exports the snapshot metadata again. The exported +// metadata then lists chunks that no blob holds. +func backupThenDropOneBlobFromMetadata( + ctx context.Context, t *testing.T, +) *missingBlobBackup { + t.Helper() + + backup, db, lostBlob := backupSingleChunkFiles(ctx, t) + + // The uploads row refers to the blob row, so it would keep the export + // from deleting it. + for _, query := range []string{ + "DELETE FROM snapshot_blobs WHERE blob_hash = ?", + "DELETE FROM uploads WHERE blob_hash = ?", + } { + _, err := db.Conn().ExecContext(ctx, query, lostBlob) + require.NoError(t, err) + } + + sm := newFaultSnapshotManager( + backup.fs, backup.storer, backup.cfg, database.NewRepositories(db)) + require.NoError(t, + sm.ExportSnapshotMetadata(ctx, db.Path(), backup.snapshotID)) + require.NoError(t, db.Close()) + + return backup +} + +// backupSingleChunkFiles backs up missingBlobFileCount single-chunk files. +// It returns the backup, with lost naming the files stored in the same +// blob as the first of them, the still open local index, and the hash of +// that blob. +func backupSingleChunkFiles( + ctx context.Context, t *testing.T, +) (*missingBlobBackup, *database.DB, string) { + t.Helper() + fs := afero.NewOsFs() tempDir := t.TempDir() dataDir := filepath.Join(tempDir, "src") @@ -143,16 +235,11 @@ func backupThenDeleteOneBlob( 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))) - + lostBlob := blobOfFile[filepath.Join(dataDir, "file-00.bin")] lost := make(map[string]bool) for path, hash := range blobOfFile { - if hash == deleted { + if hash == lostBlob { lost[path] = true } } @@ -168,7 +255,7 @@ func backupThenDeleteOneBlob( restoreDir: filepath.Join(tempDir, "restored"), files: files, lost: lost, - } + }, db, lostBlob } // blobHashOfSingleChunkFile returns the hash of the blob holding the one