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

README.md

@@ -375,9 +375,7 @@ 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 restored file's chunk hashes
-  match. A file skipped under `--skip-errors` is reported as failed to
-  restore and is not verified.
+* `--verify`: After restoring, verify every file's chunk hashes match
 
 **`prune`**: Tidy up everything that isn't needed. Runs three passes:
 (1) reconcile the local index against the destination store — any
@@ -435,7 +433,10 @@ object store. Credentials are read from `s3.access_key_id` and
 `s3.secret_access_key` in the config file.
 
 **Local filesystem** (`file:///path/to/backup`): Stores blobs and metadata on
-a local or mounted filesystem. Useful for testing or backing up to a NAS.
+a local or mounted filesystem. Useful for testing or backing up to a NAS. The
+path must be absolute; a relative one is refused when the config loads.
+Percent-escapes in the path are decoded, so `my%20stick` and `my stick` name
+the same directory.
 
 **Rclone** (`rclone://remote/path`): Uses rclone's 70+ supported cloud
 providers. Requires rclone to be configured separately (`rclone config`).

internal/blob/packer.go

@@ -245,6 +245,17 @@ func (p *Packer) Flush(ctx context.Context) error {
 	return nil
 }
 
+// DiscardCurrentBlob closes and removes the temp file of the blob being
+// packed, if any, and drops that blob. A caller that stops before Flush
+// must call it, or the temp file is left behind. Thread-safe.
+func (p *Packer) DiscardCurrentBlob() {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	p.cleanupTempFile()
+	p.currentBlob = nil
+}
+
 // FinalizeBlob finalizes the current blob being assembled.
 // This compresses the accumulated chunks, encrypts the result, and computes
 // the content-addressed hash. The finalized blob is either passed to the

internal/cli/config.go

@@ -251,6 +251,12 @@ func NewConfigCommand() *cobra.Command {
 		Use:   "config",
 		Short: "Manage the configuration file",
 		Long:  "Commands for creating, editing, and querying the vaultik config file.",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newConfigInitCommand())

internal/cli/database.go

@@ -15,6 +15,12 @@ func NewDatabaseCommand() *cobra.Command {
 		Use:   "database",
 		Short: "Manage the local state database",
 		Long:  `Commands for managing the local SQLite state database.`,
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(

internal/cli/entry_unknown_subcommand_test.go

@@ -0,0 +1,43 @@
+package cli //nolint:testpackage // shares runEntry and the command-word constants
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestEntryGroupRejectsUnknownSubcommand runs each group command with a
+// mistyped subcommand, as a crontab line might carry one. It must fail
+// with exit 1 and name the word, as `vaultik bogus` does at the top
+// level, instead of printing the group's help and exiting 0 having done
+// nothing. The group name alone still prints its help.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout and os.Stderr
+func TestEntryGroupRejectsUnknownSubcommand(t *testing.T) {
+	for _, testCase := range []struct {
+		group    string
+		mistyped string
+	}{
+		{group: cmdSnapshot, mistyped: "craete"},
+		{group: "config", mistyped: "inti"},
+		{group: cmdRemote, mistyped: "ifno"},
+		{group: "database", mistyped: "delte"},
+		{group: "completion", mistyped: "nosuchshell"},
+	} {
+		t.Run(testCase.group, func(t *testing.T) {
+			code, stdout, stderr := runEntry(t,
+				testCase.group, testCase.mistyped)
+
+			assert.Equal(t, 1, code)
+			assert.Empty(t, stdout)
+			assert.Contains(t, stderr,
+				`unknown command "`+testCase.mistyped+`"`)
+
+			code, stdout, _ = runEntry(t, testCase.group)
+
+			assert.Equal(t, 0, code)
+			assert.Contains(t, stdout,
+				programName+" "+testCase.group+" [command]")
+		})
+	}
+}

internal/cli/remote.go

@@ -18,6 +18,12 @@ func NewRemoteCommand() *cobra.Command {
 		Use:   "remote",
 		Short: "Remote storage management commands",
 		Long:  "Commands for inspecting and managing remote storage",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	// Add subcommands

internal/cli/root.go

@@ -74,6 +74,21 @@ on the source system.`,
 		NewDatabaseCommand(),
 	)
 
+	// cobra adds the completion command only when Execute runs, and with
+	// no Run, so it skipped its own cobra.NoArgs and a mistyped shell name
+	// printed help and exited 0. Adding it here lets it have a Run. Its
+	// scripts go to os.Stdout as it is now: a later SetOut does not
+	// redirect them.
+	cmd.InitDefaultCompletionCmd()
+
+	for _, sub := range cmd.Commands() {
+		if sub.Name() == "completion" {
+			sub.Run = func(completion *cobra.Command, _ []string) {
+				_ = completion.Help()
+			}
+		}
+	}
+
 	return cmd
 }
 

internal/cli/snapshot.go

@@ -41,6 +41,12 @@ func NewSnapshotCommand() *cobra.Command {
 		Use:   "snapshot",
 		Short: "Snapshot management commands",
 		Long:  "Commands for creating, listing, and managing snapshots",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	// Add subcommands

internal/config/config.go

@@ -61,6 +61,9 @@ var (
 	errBadS3PartSize    = errors.New("s3.part_size must be between 5MiB and 5GiB")
 	errBadStorageScheme = errors.New(
 		"storage_url must start with s3://, file://, or rclone://")
+	errFileURLNotAbsolute = errors.New(
+		"a file:// storage_url needs an absolute path, " +
+			"as in file:///path/to/backup")
 	errStorageNotConfigured = errors.New(
 		"storage not configured; set storage_url or provide s3.endpoint + " +
 			"s3.bucket + credentials")
@@ -461,11 +464,18 @@ func (c *Config) validateStorage() error {
 
 // validateStorageURL validates URL-based storage configuration. File and
 // rclone URLs need no credentials; S3 URLs require the legacy s3.*
-// credential fields.
+// credential fields. A file URL must name an absolute path: a relative one
+// would resolve against the working directory, so the destination would
+// move with it.
 func (c *Config) validateStorageURL() error {
 	switch {
 	case strings.HasPrefix(c.StorageURL, "file://"):
-		// File storage doesn't need S3 credentials
+		// storage.ParseStorageURL applies the same rule, but config
+		// cannot import storage, which imports config.
+		if !strings.HasPrefix(c.StorageURL, "file:///") {
+			return errFileURLNotAbsolute
+		}
+
 		return nil
 	case strings.HasPrefix(c.StorageURL, "rclone://"):
 		// Rclone storage uses rclone's own config

internal/config/config_test.go

@@ -337,6 +337,35 @@ func TestLoadS3PartSize(t *testing.T) {
 	})
 }
 
+// TestLoadRejectsRelativeFileURL checks that a file:// storage_url with a
+// relative path fails at load. Accepted, it would resolve against the
+// working directory, so a backup run from another directory would write
+// to a different destination and still succeed.
+func TestLoadRejectsRelativeFileURL(t *testing.T) {
+	t.Parallel()
+
+	const relativeConfig = "snapshots:\n" +
+		"  test:\n" +
+		"    paths: [/tmp/vaultik-test-source]\n" +
+		"storage_url: file://dest\n"
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(relativeConfig), 0o600)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	_, err = Load(path)
+	if !errors.Is(err, errFileURLNotAbsolute) {
+		t.Fatalf("Load() error = %v, want errFileURLNotAbsolute", err)
+	}
+
+	if !strings.Contains(err.Error(), "absolute path") {
+		t.Errorf("error %q does not say to use an absolute path", err.Error())
+	}
+}
+
 // TestValidateAgeRecipients checks that recipients are parsed at config load
 // (a bad entry fails immediately, not mid-backup) and that no invalid entry —
 // least of all a pasted secret key — is echoed in the error. An empty list

internal/snapshot/exclude_test.go

@@ -531,3 +531,50 @@ func TestExcludePatterns_UnanchoredPatternFile(t *testing.T) {
 	require.Equal(t, 1, result.FilesScanned,
 		"Unanchored pattern for file should match anywhere")
 }
+
+func TestExcludePatterns_DoubleStarDirectoryAtEveryDepth(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, a leading **/ also matches directly under the source dir
+	fs := setupAnchoredTestFS(t)
+
+	scanner, repos, cleanup := createTestScanner(t, fs, []string{"**/projectname"})
+	defer cleanup()
+
+	ctx := context.Background()
+	createSnapshotRecord(ctx, t, repos, "test-snapshot")
+
+	result, err := scanner.Scan(ctx, "/backup", "test-snapshot")
+	require.NoError(t, err)
+
+	// **/projectname should exclude BOTH:
+	// - /backup/projectname/file.txt
+	// - /backup/otherproject/projectname/file.txt
+	// Total files: 4, excluded: 2
+	require.Equal(t, 2, result.FilesScanned,
+		"**/ directory pattern should match at the top level and nested")
+}
+
+func TestExcludePatterns_DoubleStarFileAtEveryDepth(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, a leading **/ also matches directly under the source dir
+	fs := setupAnchoredTestFS(t)
+
+	scanner, repos, cleanup := createTestScanner(t, fs, []string{"**/*.txt"})
+	defer cleanup()
+
+	ctx := context.Background()
+	createSnapshotRecord(ctx, t, repos, "test-snapshot")
+
+	result, err := scanner.Scan(ctx, "/backup", "test-snapshot")
+	require.NoError(t, err)
+
+	// **/*.txt should exclude ALL .txt files:
+	// - /backup/file.txt
+	// - /backup/projectname/file.txt
+	// - /backup/otherproject/projectname/file.txt
+	// Total files: 4, excluded: 3
+	require.Equal(t, 1, result.FilesScanned,
+		"**/ file pattern should match at the top level and nested")
+}

internal/snapshot/scanner.go

@@ -1295,6 +1295,11 @@ func (s *Scanner) batchAddFilesToSnapshot(
 func (s *Scanner) processPhase(
 	ctx context.Context, filesToProcess []*FileToProcess, result *ScanResult,
 ) error {
+	// On an error or cancellation the blob being packed is never
+	// finalized, so its temp file must be removed here. After a
+	// successful flush there is no such blob and this does nothing.
+	defer s.packer.DiscardCurrentBlob()
+
 	// Calculate total bytes to process
 	var totalBytes int64
 	for _, f := range filesToProcess {
@@ -2013,6 +2018,14 @@ func compileExcludePatterns(patterns []string) []compiledPattern {
 		// Remove trailing slash if present (directory indicator)
 		pattern = strings.TrimSuffix(pattern, "/")
 
+		// A leading "**/" demands a "/" before the name and so misses an
+		// entry directly under the source path. An unanchored pattern
+		// already matches at every depth, so drop the prefix: as in
+		// gitignore, "**/x" then also matches a top-level "x".
+		if !anchored {
+			pattern = strings.TrimPrefix(pattern, "**/")
+		}
+
 		// Compile the glob pattern
 		// For patterns without path separators, we need to match them as components
 		// e.g., ".git" should match ".git" anywhere in the path

internal/snapshot/skip_errors_test.go

@@ -381,3 +381,60 @@ func TestScannerCancelStopsSkipErrorsRun(t *testing.T) {
 		})
 	}
 }
+
+// TestScannerRemovesBlobTempFileWhenRunStops checks that a run stopped
+// part-way, by a file it cannot read or by a cancellation, removes the
+// temp file of the blob it was packing.
+func TestScannerRemovesBlobTempFileWhenRunStops(t *testing.T) {
+	t.Parallel()
+
+	// a.txt is backed up first and starts a blob; the run stops at b.txt.
+	const target = "/source/b.txt"
+
+	tests := []struct {
+		name  string
+		newFs func(cancel context.CancelFunc) afero.Fs
+	}{
+		{
+			name: "file cannot be read",
+			newFs: func(context.CancelFunc) afero.Fs {
+				return &readFailFs{Fs: afero.NewMemMapFs(), target: target}
+			},
+		},
+		{
+			name: "cancelled",
+			newFs: func(cancel context.CancelFunc) afero.Fs {
+				return &cancelOnOpenFs{
+					Fs: afero.NewMemMapFs(), target: target, cancel: cancel,
+				}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			fs := tt.newFs(cancel)
+			writeSkipErrorTestFile(t, fs, "/source/a.txt", "first file content")
+			writeSkipErrorTestFile(t, fs, target, "second file content")
+
+			_, err := runSkipErrorScan(ctx, t, fs, "/source", false, nil)
+			if err == nil {
+				t.Fatal("expected the scan to stop with an error, got nil")
+			}
+
+			left, err := afero.Glob(fs, filepath.Join(os.TempDir(), "vaultik-blob-*"))
+			if err != nil {
+				t.Fatalf("listing blob temp files: %v", err)
+			}
+
+			if len(left) != 0 {
+				t.Fatalf("expected no blob temp file left, got %v", left)
+			}
+		})
+	}
+}

internal/storage/url.go

@@ -17,8 +17,10 @@ const (
 
 // Sentinel errors for storage URL parsing.
 var (
-	ErrEmptyStorageURL   = errors.New("storage URL is empty")
-	ErrEmptyFilePath     = errors.New("file URL path is empty")
+	ErrEmptyStorageURL  = errors.New("storage URL is empty")
+	ErrEmptyFilePath    = errors.New("file URL path is empty")
+	ErrRelativeFilePath = errors.New(
+		"file URL needs an absolute path, as in file:///path/to/backup")
 	ErrMissingBucket     = errors.New("s3 URL missing bucket name")
 	ErrMissingRemote     = errors.New("rclone URL missing remote name")
 	ErrUnsupportedScheme = errors.New(
@@ -51,17 +53,8 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 		return nil, ErrEmptyStorageURL
 	}
 
-	// Handle file:// URLs
 	if after, ok := strings.CutPrefix(rawURL, "file://"); ok {
-		path := after
-		if path == "" {
-			return nil, ErrEmptyFilePath
-		}
-
-		return &URL{
-			Scheme: schemeFile,
-			Prefix: path,
-		}, nil
+		return parseFileURL(after)
 	}
 
 	if strings.HasPrefix(rawURL, "s3://") {
@@ -75,6 +68,27 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 	return nil, ErrUnsupportedScheme
 }
 
+// parseFileURL parses the path that follows file://. The path must be
+// absolute: a relative one would resolve against the working directory,
+// so the destination would move with it. Percent-escapes are decoded, so
+// my%20stick names the directory "my stick"; a literal space is kept.
+func parseFileURL(rawPath string) (*URL, error) {
+	if rawPath == "" {
+		return nil, ErrEmptyFilePath
+	}
+
+	if !strings.HasPrefix(rawPath, "/") {
+		return nil, ErrRelativeFilePath
+	}
+
+	path, err := url.PathUnescape(rawPath)
+	if err != nil {
+		return nil, fmt.Errorf("invalid file URL path: %w", err)
+	}
+
+	return &URL{Scheme: schemeFile, Prefix: path}, nil
+}
+
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
 // the userinfo and any query parameter other than endpoint, region and
 // ssl, so a credential-bearing URL is never stored or echoed.

internal/storage/url_parse_test.go

@@ -14,7 +14,10 @@ import (
 func TestParseStorageURLValid(t *testing.T) {
 	t.Parallel()
 
-	const bucket = "mybucket"
+	const (
+		bucket     = "mybucket"
+		fileScheme = "file"
+	)
 
 	cases := []struct {
 		name string
@@ -24,7 +27,17 @@ func TestParseStorageURLValid(t *testing.T) {
 		{
 			name: "file absolute path",
 			raw:  "file:///var/backups/vaultik",
-			want: &storage.URL{Scheme: "file", Prefix: "/var/backups/vaultik"},
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/var/backups/vaultik"},
+		},
+		{
+			name: "file percent-escaped path is decoded",
+			raw:  "file:///Volumes/my%20stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
+		},
+		{
+			name: "file path with a literal space",
+			raw:  "file:///Volumes/my stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
 		},
 		{
 			name: "s3 bucket and prefix, ssl defaults on",
@@ -90,6 +103,7 @@ func TestParseStorageURLErrors(t *testing.T) {
 	}{
 		{"empty url", "", storage.ErrEmptyStorageURL},
 		{"file empty path", "file://", storage.ErrEmptyFilePath},
+		{"file relative path", "file://dest", storage.ErrRelativeFilePath},
 		{"s3 missing bucket", "s3://", storage.ErrMissingBucket},
 		{"s3 missing bucket with path", "s3:///justprefix", storage.ErrMissingBucket},
 		{"rclone missing remote", "rclone://", storage.ErrMissingRemote},

internal/vaultik/restore.go

@@ -103,12 +103,11 @@ 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
-	VerifyFailedFiles []string // Paths of restored files that failed verification
+	FilesVerified int
+	BytesVerified int64
+	FilesFailed   int
+	FailedFiles   []string // Paths of files that failed verification
 }
 
 // Restore restores files from a snapshot to the target directory
@@ -310,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.",
@@ -648,34 +658,20 @@ func (v *Vaultik) handleRestoreVerification(
 	opts *RestoreOptions,
 	result *RestoreResult,
 ) error {
-	// 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)
+	err := v.verifyRestoredFiles(v.ctx, repos, files, opts.TargetDir, result)
 	if err != nil {
 		return fmt.Errorf("verification failed: %w", err)
 	}
 
-	if len(result.VerifyFailedFiles) > 0 {
+	if result.FilesFailed > 0 {
 		v.UI.Errorf("Verification failed: %s files did not match expected checksums.",
-			v.UI.Count(len(result.VerifyFailedFiles)))
+			v.UI.Count(result.FilesFailed))
 
-		for _, path := range result.VerifyFailedFiles {
+		for _, path := range result.FailedFiles {
 			v.UI.Detailf("%s", v.UI.Path(path))
 		}
 
-		return fmt.Errorf("%d %w", len(result.VerifyFailedFiles), errFilesFailedVerify)
+		return fmt.Errorf("%d %w", result.FilesFailed, errFilesFailedVerify)
 	}
 
 	v.UI.Completef("Verified %s files (%s).",
@@ -1435,8 +1431,8 @@ func (v *Vaultik) verifyRestoredFiles(
 		if err != nil {
 			log.Error("File verification failed", "path", file.Path, "error", err)
 
-			result.VerifyFailedFiles = append(
-				result.VerifyFailedFiles, file.Path.String())
+			result.FilesFailed++
+			result.FailedFiles = append(result.FailedFiles, file.Path.String())
 		}
 
 		bytesProcessed += file.Size
@@ -1452,7 +1448,7 @@ func (v *Vaultik) verifyRestoredFiles(
 	log.Info("Verification complete",
 		"files_verified", result.FilesVerified,
 		"bytes_verified", ubytes(result.BytesVerified),
-		"files_failed", len(result.VerifyFailedFiles),
+		"files_failed", result.FilesFailed,
 	)
 
 	return nil

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)

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

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,53 +92,18 @@ 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)
 	}
 }
 
-// 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")
-}
-
 // TestRestoreMissingBlobAbortsWithoutSkipErrors checks that a deleted blob
 // still ends the restore with an error when SkipErrors is not set.
 func TestRestoreMissingBlobAbortsWithoutSkipErrors(t *testing.T) {
@@ -138,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(
@@ -145,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")
@@ -178,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
 		}
 	}
@@ -203,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