Fail snapshot remove when a destination-only snapshot's metadata cannot be deleted (closes #340)

When a snapshot was only on the destination store and deleting its
metadata there failed, snapshot remove warned and exited 0 having
removed nothing. That failure is now an error. A snapshot in the local
index keeps its behaviour: it is removed from the local index and the
failed delete on the destination store is a warning.

The README and the help text said the `vaultik prune` follow-up is
printed on every success; they now say it is printed only when metadata
was removed from the destination store, and never under --json. The
comments on RemoveSnapshot and removeSnapshotRemote describe both
cases; removeSnapshotRemote's also says it returns true only when the
delete finished without an error and removed at least one object.

Model: opus-5-5

README.md

@@ -360,16 +360,20 @@ snapshot from the local index and strips the snapshot's metadata from
 the backup destination store. Blobs are NOT touched — deleting blobs
 requires reading every remaining remote manifest (the destination store
 may hold snapshots this host doesn't know about), which is what
-`vaultik prune` does. On success the command prints the exact `vaultik
+`vaultik prune` does. The command prints the `vaultik prune` invocation
-prune` invocation to run as a follow-up. An ID that names no snapshot in
+to run as a follow-up only when it removed the snapshot's metadata from
-the local index or on the destination store is an error; with
+the destination store, and never under `--json`. An ID that names no
-`--local-only`, the snapshot must be in the local index. Local row
+snapshot in the local index or on the destination store is an error;
+with `--local-only`, the snapshot must be in the local index. Local row
 cleanup (files, chunks, blobs the snapshot was the last referrer for)
-runs automatically. If the destination store is unreachable, the local-DB
+runs automatically. If removing the snapshot's metadata from the
-removal still completes and a warning is emitted; run `vaultik snapshot
+destination store fails (for example because the store is unreachable),
-remove <snapshot-id>` again once the store is reachable to remove the
+the local-DB removal still completes and a warning is emitted; run
-snapshot's metadata from it (`vaultik prune` does not). To wipe everything
+`vaultik snapshot remove <snapshot-id>` again once the store is reachable
-on the destination in one go, use `vaultik remote nuke --force`.
+to remove the snapshot's metadata from it (`vaultik prune` does not). For
+a snapshot that is only on the destination store, that failure is an
+error. To wipe everything on the destination in one go, use `vaultik
+remote nuke --force`.
 * `--local-only`: Skip remote cleanup; only touch the local index
 * `--dry-run`: Show what would be deleted without deleting
 * `--force`: Skip confirmation prompt

config.example.yml

@@ -302,7 +302,8 @@ storage_url: "rclone://myremote/path/to/backups"
 
 # Average chunk size for content-defined chunking
 # Smaller chunks = better deduplication but more metadata
-# Supports: 10MB, 5M, 1GB, 500KB, 64MiB, etc.
+# Minimum 1MB
+# Supports: 10MB, 5M, 1GB, 64MiB, etc.
 # Default: 10MB
 #chunk_size: 10MB
 

internal/cli/config.go

@@ -224,7 +224,7 @@ storage_url: ""
 
 # Average chunk size for content-defined chunking (FastCDC).
 # Smaller = better deduplication but more metadata overhead.
-# Accepts: 1MB, 10M, 64KB, etc.
+# Minimum 1MB. Accepts: 1MB, 10M, 64MiB, etc.
 # Default: 10MB
 # chunk_size: 10MB
 

internal/cli/snapshot.go

@@ -270,8 +270,9 @@ By default, this removes the snapshot from the local index database and
 strips the snapshot's metadata from the backup destination store. Blobs
 are NOT touched: deleting them requires reading every remaining remote
 manifest (the destination store may hold snapshots this host doesn't
-know about), which is what 'vaultik prune' does. On success the command
+know about), which is what 'vaultik prune' does. The command prints the
-prints the exact 'vaultik prune' invocation to run as a follow-up.
+'vaultik prune' invocation to run as a follow-up only when it removed the
+snapshot's metadata from the destination store, and never under --json.
 
 An ID that names no snapshot in the local index or on the destination
 store is an error.
@@ -280,10 +281,12 @@ Use --local-only to skip the remote half (e.g. when you want to forget a
 snapshot locally without touching the destination store); the snapshot
 must then be in the local index.
 
-If the remote is unreachable, the local-database removal still completes
+If removing the snapshot's metadata from the destination store fails (for
-and a warning is emitted; run 'vaultik snapshot remove <snapshot-id>' again
+example because the remote is unreachable), the local-database removal
-once the destination store is reachable to remove the snapshot's metadata
+still completes and a warning is emitted; run 'vaultik snapshot remove
-from it ('vaultik prune' does not).
+<snapshot-id>' again once the destination store is reachable to remove
+the snapshot's metadata from it ('vaultik prune' does not). For a
+snapshot that is only on the destination store, that failure is an error.
 
 To wipe the entire destination store and start over, use 'vaultik remote
 nuke --force' — it is the single supported entry point for that.`,

internal/config/config.go

@@ -30,13 +30,15 @@ const appName = "vaultik"
 //nolint:gosec // G101: marker for detecting a pasted secret key, not a credential
 const secretKeyPrefix = "AGE-SECRET-KEY-"
 
-// Defaults and validation bounds for tunable settings.
+// Defaults and validation bounds for tunable settings. MB and GB are
+// 1000-based, as the size parser reads them; the S3 part sizes are S3's own
+// limits, which are 1024-based (MiB, GiB).
 const (
-	defaultBlobSizeLimit    = Size(10 * 1024 * 1024 * 1024) // 10GB
+	defaultBlobSizeLimit    = Size(10 * 1000 * 1000 * 1000) // 10GB
-	defaultChunkSize        = Size(10 * 1024 * 1024)        // 10MB
+	defaultChunkSize        = Size(10 * 1000 * 1000)        // 10MB
 	defaultS3PartSize       = Size(5 * 1024 * 1024)         // 5MiB
 	defaultCompressionLevel = 3
-	minChunkSize            = 1024 * 1024 // 1MB
+	minChunkSize            = 1000 * 1000 // 1MB
 	minCompressionLevel     = 1
 	maxCompressionLevel     = 19
 	// S3 accepts a multipart upload part from 5MiB to 5GiB.

internal/config/config_test.go

@@ -337,6 +337,81 @@ func TestLoadS3PartSize(t *testing.T) {
 	})
 }
 
+// TestDocumentedSizeDefaults checks that the defaults are the documented
+// chunk_size 10MB and blob_size_limit 10GB as the parser reads those values
+// (1000-based), so writing a default out in the config file changes nothing.
+func TestDocumentedSizeDefaults(t *testing.T) {
+	t.Parallel()
+
+	cfg, err := Parse([]byte("chunk_size: 10MB\nblob_size_limit: 10GB\n"))
+	if err != nil {
+		t.Fatalf("Parse() unexpected error: %v", err)
+	}
+
+	if cfg.ChunkSize != defaultChunkSize {
+		t.Errorf("chunk_size: 10MB is %d bytes, the default is %d",
+			cfg.ChunkSize, defaultChunkSize)
+	}
+
+	if cfg.BlobSizeLimit != defaultBlobSizeLimit {
+		t.Errorf("blob_size_limit: 10GB is %d bytes, the default is %d",
+			cfg.BlobSizeLimit, defaultBlobSizeLimit)
+	}
+}
+
+// TestDocumentedSizeExamples checks the 1MB minimum chunk_size and that
+// every size the README, config.example.yml and the config init template
+// list as an example is accepted.
+func TestDocumentedSizeExamples(t *testing.T) {
+	t.Parallel()
+
+	const withoutSizes = "snapshots:\n" +
+		"  test:\n" +
+		"    paths: [/tmp/vaultik-test-source]\n" +
+		"storage_url: file:///tmp/vaultik-test-storage\n"
+
+	tests := []struct {
+		setting string
+		wantErr error
+	}{
+		{"chunk_size: 1MB", nil},
+		{"chunk_size: 999999", errChunkSizeTooSmall},
+		{"chunk_size: 5M", nil},
+		{"chunk_size: 10M", nil},
+		{"chunk_size: 10MB", nil},
+		{"chunk_size: 64MiB", nil},
+		{"chunk_size: 1GB", nil},
+		// Four times the default chunk_size, the documented minimum.
+		{"blob_size_limit: 40MB", nil},
+		{"blob_size_limit: 500MB", nil},
+		{"blob_size_limit: 1GB", nil},
+		{"blob_size_limit: 1GiB", nil},
+		{"blob_size_limit: 10G", nil},
+		{"blob_size_limit: 10GB", nil},
+		{"s3: {part_size: 5MiB}", nil},
+		{"s3: {part_size: 10MB}", nil},
+		{"s3: {part_size: 16MiB}", nil},
+		{"s3: {part_size: 100MiB}", nil},
+		{"s3: {part_size: 5GiB}", nil},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.setting, func(t *testing.T) {
+			t.Parallel()
+
+			cfg, err := Parse([]byte(withoutSizes + tt.setting + "\n"))
+			if err != nil {
+				t.Fatalf("Parse() unexpected error: %v", err)
+			}
+
+			err = cfg.Validate()
+			if !errors.Is(err, tt.wantErr) {
+				t.Fatalf("Validate() error = %v, want %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
 // 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

internal/snapshot/exclude_test.go

@@ -578,3 +578,54 @@ func TestExcludePatterns_DoubleStarFileAtEveryDepth(t *testing.T) {
 	require.Equal(t, 1, result.FilesScanned,
 		"**/ file pattern should match at the top level and nested")
 }
+
+func TestExcludePatterns_AnchoredDoubleStarAtEveryDepth(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, /**/ 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,
+		"/**/ pattern should match at the top level and nested")
+}
+
+func TestExcludePatterns_DoubleStarInMiddleMatchesZeroDirectories(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, a/**/b matches a/b as well as a/c/b. The last part
+	// is a glob: with a plain name the glob library already matched a/b.
+	fs := &afero.MemMapFs{}
+	files := []string{"/backup/a/x.log", "/backup/a/c/y.log", "/backup/a/c/kept.txt"}
+
+	for _, path := range files {
+		require.NoError(t, fs.MkdirAll(filepath.Dir(path), 0755))
+		require.NoError(t, afero.WriteFile(fs, path, []byte(path), 0644))
+	}
+
+	scanner, repos, cleanup := createTestScanner(t, fs, []string{"a/**/*.log"})
+	defer cleanup()
+
+	ctx := context.Background()
+	createSnapshotRecord(ctx, t, repos, "test-snapshot")
+
+	result, err := scanner.Scan(ctx, "/backup", "test-snapshot")
+	require.NoError(t, err)
+
+	// a/**/*.log should exclude /backup/a/x.log and /backup/a/c/y.log
+	// Total files: 3, excluded: 2
+	require.Equal(t, 1, result.FilesScanned,
+		"a/**/*.log should match a/x.log as well as a/c/y.log")
+}

internal/snapshot/scanner.go

@@ -2035,33 +2035,60 @@ func compileExcludePatterns(patterns []string) []compiledPattern {
 		pattern = strings.TrimSuffix(pattern, "/")
 
 		// A leading "**/" demands a "/" before the name and so misses an
-		// entry directly under the source path. An unanchored pattern
+		// entry directly under the source path. It means "at every depth",
-		// already matches at every depth, so drop the prefix: as in
+		// which is what an unanchored pattern already does, so drop the
-		// gitignore, "**/x" then also matches a top-level "x".
+		// prefix and match unanchored: as in gitignore, "**/x" and
-		if !anchored {
+		// "/**/x" then also match a top-level "x".
-			pattern = strings.TrimPrefix(pattern, "**/")
+		if rest, found := strings.CutPrefix(pattern, "**/"); found {
+			pattern = rest
+			anchored = false
 		}
 
-		// Compile the glob pattern
+		// Likewise "/**/" inside a pattern demands two slashes, so
-		// For patterns without path separators, we need to match them as components
+		// "a/**/*.log" misses "a/x.log". As in gitignore it stands for
-		// e.g., ".git" should match ".git" anywhere in the path
+		// zero or more directories, so also compile the pattern with it
-		g, err := glob.Compile(pattern, '/')
+		// made a single "/". The glob library's {a,b} is no help: with a
-		if err != nil {
+		// "**" inside it in mid-pattern, it misses paths it should match.
-			log.Warn("Invalid exclude pattern, skipping", "pattern", p, "error", err)
+		for _, variant := range doubleStarVariants(pattern) {
+			// For patterns without path separators, we need to match them as components
+			// e.g., ".git" should match ".git" anywhere in the path
+			g, err := glob.Compile(variant, '/')
+			if err != nil {
+				log.Warn("Invalid exclude pattern, skipping", "pattern", p, "error", err)
 
-			continue
+				break
+			}
+
+			compiled = append(compiled, compiledPattern{
+				pattern:  g,
+				anchored: anchored,
+				original: p,
+			})
 		}
-
-		compiled = append(compiled, compiledPattern{
-			pattern:  g,
-			anchored: anchored,
-			original: p,
-		})
 	}
 
 	return compiled
 }
 
+// doubleStarVariants returns pattern once for every way of keeping each
+// "/**/" in it or making it a single "/".
+func doubleStarVariants(pattern string) []string {
+	const doubleStar = "/**/"
+
+	before, after, found := strings.Cut(pattern, doubleStar)
+	if !found {
+		return []string{pattern}
+	}
+
+	var variants []string
+
+	for _, rest := range doubleStarVariants(after) {
+		variants = append(variants, before+doubleStar+rest, before+"/"+rest)
+	}
+
+	return variants
+}
+
 // shouldExclude checks if a path should be excluded based on exclude patterns
 // filePath is the full path to the file
 // rootPath is the root of the backup source directory

internal/vaultik/remove_snapshot_test.go

@@ -17,6 +17,7 @@ import (
 	"sneak.berlin/go/vaultik/internal/log"
 	"sneak.berlin/go/vaultik/internal/snapshot"
 	"sneak.berlin/go/vaultik/internal/storage"
+	"sneak.berlin/go/vaultik/internal/storage/faultstore"
 	"sneak.berlin/go/vaultik/internal/types"
 	"sneak.berlin/go/vaultik/internal/vaultik"
 )
@@ -431,6 +432,40 @@ func TestRemoveSnapshot_NotInLocalIndex(t *testing.T) {
 	assert.Contains(t, out, "Removed snapshot metadata from remote storage")
 }
 
+// TestRemoveSnapshot_NotInLocalIndexDeleteFails checks that removing a
+// snapshot only the destination store holds fails when its metadata
+// cannot be deleted there, under --force and under --json: nothing was
+// removed, so the command must not succeed.
+func TestRemoveSnapshot_NotInLocalIndexDeleteFails(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	for name, opts := range map[string]vaultik.RemoveOptions{
+		"force": {Force: true},
+		"json":  {JSON: true},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			store := newTestStorer()
+			addManifest(t, store, "snapshot-001", nil)
+
+			faulty := faultstore.New(store)
+			faulty.OnDelete = func(string) bool { return true }
+
+			tv := vaultik.NewForTesting(faulty)
+			tv.Repositories = newLocalIndex(t)
+
+			result, err := tv.RemoveSnapshot("snapshot-001", &opts)
+
+			require.ErrorIs(t, err, faultstore.ErrInjectedDelete)
+			assert.False(t, result.RemoteRemoved)
+			assert.True(t, store.hasKey(remoteKeyPath("snapshot-001")))
+			assert.Empty(t, tv.Stdout.String())
+		})
+	}
+}
+
 func TestRemoveAllSnapshots_RequiresForce(t *testing.T) {
 	log.Initialize(log.Config{})
 	t.Parallel()

internal/vaultik/snapshot.go

@@ -1219,9 +1219,11 @@ const snapshotRemoveCommandHint = "vaultik snapshot remove"
 // destination store. Blobs are NOT touched: removing a snapshot's
 // blobs requires reading every remaining remote manifest (the remote
 // may hold snapshots this host doesn't know about), which is what
-// `vaultik prune` is for — the command prints the prune invocation to
+// `vaultik prune` is for — when metadata was removed from the
-// run as a follow-up. When the remote is unreachable the command still
+// destination store, the text output names the prune invocation to run
-// completes the local-DB removal and warns.
+// as a follow-up. When removing the metadata fails, a snapshot in the
+// local index is still removed from it, with a warning; a snapshot
+// only on the destination store is an error.
 func (v *Vaultik) RemoveSnapshot(
 	snapshotID string, opts *RemoveOptions,
 ) (*RemoveResult, error) {
@@ -1256,8 +1258,10 @@ func (v *Vaultik) RemoveSnapshot(
 		}
 	}
 
-	if !opts.LocalOnly {
+	result.RemoteRemoved, err = v.removeSnapshotRemote(
-		result.RemoteRemoved = v.removeSnapshotRemote(snapshotID, opts)
+		snapshotID, inLocalIndex, opts)
+	if err != nil {
+		return result, err
 	}
 
 	if v.SnapshotManager != nil {
@@ -1379,19 +1383,33 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions)
 }
 
 // removeSnapshotRemote strips the snapshot's metadata from the
-// destination store, warning and proceeding on failure: the local-DB
+// destination store. It returns true only when the delete finished
-// removal has already happened, so the user is told the remote half
+// without an error and removed at least one object; under LocalOnly it
-// didn't finish and to run `vaultik snapshot remove` for the snapshot
+// does nothing. When the snapshot is only on the destination store, a
-// again once the destination store is reachable (`vaultik prune` never
+// failure is returned as an error, because that removal is all the
-// removes snapshot metadata). Returns true when the remote removal
+// command does. When the snapshot was in the local
-// succeeded and deleted something.
+// index, its removal there has already happened, so a failure is only a
-func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool {
+// warning telling the user to run `vaultik snapshot remove` for the
+// snapshot again once the destination store is reachable (`vaultik
+// prune` never removes snapshot metadata).
+func (v *Vaultik) removeSnapshotRemote(
+	snapshotID string, inLocalIndex bool, opts *RemoveOptions,
+) (bool, error) {
+	if opts.LocalOnly {
+		return false, nil
+	}
+
 	log.Info("Removing snapshot metadata from remote storage",
 		"snapshot_id", snapshotID)
 
 	remoteKey := snapshot.RemoteSnapshotKey(snapshotID)
 
 	deleted, err := v.deleteRemoteSnapshotByKey(remoteKey)
+	if err != nil && !inLocalIndex {
+		return false, fmt.Errorf(
+			"removing snapshot metadata from destination store: %w", err)
+	}
+
 	if err != nil {
 		log.Warn("Could not remove snapshot metadata from remote storage; "+
 			"run '"+snapshotRemoveCommandHint+"' with the snapshot's ID "+
@@ -1406,10 +1424,10 @@ func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) b
 				err, snapshotRemoveCommandHint, snapshotID)
 		}
 
-		return false
+		return false, nil
 	}
 
-	return deleted > 0
+	return deleted > 0, nil
 }
 
 // RemoveAllSnapshots removes every snapshot known to the local