Author SHA1 Message Date
clawbot 2c4af6d0df Stop snapshot purge when a snapshot cannot leave the local index (closes #337)
check / check (push) Waiting to run
snapshot purge, and the purge step of snapshot create --prune, logged a
failed local index delete, then deleted that snapshot's metadata from
the destination store anyway and exited 0, leaving a local record whose
metadata was gone. It now stops at that snapshot with an error, before
touching its metadata, so the command exits non-zero; snapshots already
purged stay purged. This matches how the same loop already handles a
failed metadata delete.

Model: opus-5-5
2026-10-13 19:35:57 +00:00
6 changed files with 111 additions and 446 deletions
+3 -1
View File
@@ -339,7 +339,9 @@ local index alone, and still exits zero.
**`snapshot purge`**: Remove old snapshots based on criteria. Retention is
per-snapshot-name (`--keep-latest` keeps the latest of each name, not the
latest globally).
latest globally). If a snapshot cannot be removed from the local index, the
command stops there, leaves that snapshot's metadata on the destination store
and exits non-zero; `snapshot create --prune` fails the same way.
* `--keep-latest`: Keep only the most recent snapshot of each name
* `--older-than <duration>`: Remove snapshots older than duration (e.g. `30d`,
`4w`, `6mo`, `1y`; `m` is minutes, `mo` is months)
+90
View File
@@ -0,0 +1,90 @@
package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
import (
"context"
"database/sql"
"os"
"path/filepath"
"testing"
"time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/vaultik/internal/database"
"sneak.berlin/go/vaultik/internal/snapshot"
"sneak.berlin/go/vaultik/internal/types"
)
// The `snapshot purge` command word and flags used below.
const (
cmdPurge = "purge"
flagKeepLatest = "--keep-latest"
flagForce = "--force"
)
// TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails makes
// removing a snapshot from the local index fail and checks that
// `snapshot purge` exits non-zero and leaves that snapshot's metadata on
// the destination store. It used to log the error, delete the metadata
// anyway and exit 0.
//
//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
func TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails(t *testing.T) {
ctx := context.Background()
configPath := writeHermeticPruneConfig(t, false)
// writeHermeticPruneConfig puts the index and the store directory
// beside the config file.
dir := filepath.Dir(configPath)
// Two snapshots of the same name: --keep-latest purges the older.
olderStart := time.Date(2026, 5, 1, 9, 0, 0, 0, time.UTC)
newerStart := olderStart.Add(time.Hour)
olderID := "test-host_test_" + olderStart.Format(time.RFC3339)
newerID := "test-host_test_" + newerStart.Format(time.RFC3339)
db, err := database.New(ctx, filepath.Join(dir, "index.sqlite"))
require.NoError(t, err)
repos := database.NewRepositories(db)
manifestPaths := make(map[string]string)
for id, startedAt := range map[string]time.Time{
olderID: olderStart,
newerID: newerStart,
} {
completedAt := startedAt.Add(time.Minute)
snap := &database.Snapshot{
ID: types.SnapshotID(id),
Hostname: "test-host",
VaultikVersion: "test",
StartedAt: startedAt,
CompletedAt: &completedAt,
}
err = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error {
return repos.Snapshots.Create(ctx, tx, snap)
})
require.NoError(t, err)
manifestPaths[id] = filepath.Join(dir, "store", "metadata",
snapshot.RemoteSnapshotKey(id), "manifest.json.zst")
require.NoError(t, os.MkdirAll(filepath.Dir(manifestPaths[id]), 0o750))
require.NoError(t, os.WriteFile(manifestPaths[id], []byte("stub"), 0o600))
}
_, err = db.Conn().ExecContext(ctx, `
CREATE TRIGGER refuse_snapshot_delete BEFORE DELETE ON snapshots
BEGIN SELECT RAISE(ABORT, 'simulated index error'); END`)
require.NoError(t, err)
require.NoError(t, db.Close())
code, _, stderr := runEntry(t,
flagConfig, configPath, cmdSnapshot, cmdPurge, flagKeepLatest, flagForce)
assert.Equal(t, 1, code)
assert.Contains(t, stderr, "simulated index error")
assert.FileExists(t, manifestPaths[olderID],
"a snapshot still in the local index keeps its metadata")
}
+13 -63
View File
@@ -120,10 +120,9 @@ func (f *FileStorer) Delete(_ context.Context, key string) error {
return nil
}
// List returns all keys with the given prefix, reading through symlinked
// directories. It fails when the destination directory is missing, or
// when the prefix or a directory above it is a symlink whose target is
// missing; a prefix that is truly missing is an empty listing.
// List returns all keys with the given prefix. It fails when the
// destination directory is missing; a missing prefix under it is an
// empty listing.
func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) {
var keys []string
@@ -134,7 +133,8 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
basePath := f.fullPath(prefix)
exists, err := f.prefixExists(prefix)
// Check if the prefix exists
exists, err := afero.Exists(f.fs, basePath)
if err != nil {
return nil, fmt.Errorf("checking path: %w", err)
}
@@ -143,7 +143,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
return keys, nil // Empty list for non-existent prefix
}
err = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
@@ -176,9 +176,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
}
// ListStream returns a channel of ObjectInfo for large result sets. Like
// List, it reads through symlinked directories and sends an error when
// the destination directory is missing, or when the prefix or a directory
// above it is a symlink whose target is missing.
// List, it sends an error when the destination directory is missing.
func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan ObjectInfo {
ch := make(chan ObjectInfo)
@@ -194,7 +192,8 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
basePath := f.fullPath(prefix)
exists, err := f.prefixExists(prefix)
// Check if the prefix exists
exists, err := afero.Exists(f.fs, basePath)
if err != nil {
ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)}
@@ -205,7 +204,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
return // Empty channel for non-existent prefix
}
_ = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
_ = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
// Check context cancellation
select {
case <-ctx.Done():
@@ -244,12 +243,11 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
}
// DeletePartialUploads removes every file under prefix whose name ends in
// tempSuffix. Like List, it reads through symlinked directories and fails
// on one whose target is missing. A missing prefix has none to remove.
// tempSuffix. A missing prefix has none to remove.
func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) error {
basePath := f.fullPath(prefix)
exists, err := f.prefixExists(prefix)
exists, err := afero.Exists(f.fs, basePath)
if err != nil {
return fmt.Errorf("checking path: %w", err)
}
@@ -258,7 +256,7 @@ func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) er
return nil
}
err = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
@@ -354,54 +352,6 @@ func (f *FileStorer) writeAtomic(
return nil
}
// walk is afero.Walk, except that it reads through a symlinked directory
// the way Get and Stat do, where afero.Walk reports the link as one file.
// afero.Walk uses Lstat, which does not follow a link, whenever the
// filesystem has one; the struct below carries only the afero.Fs methods,
// so the walk uses Stat instead. A link back up the tree is followed until
// the operating system refuses the path for too many levels of links, and
// the walk then fails.
func (f *FileStorer) walk(root string, fn filepath.WalkFunc) error {
return afero.Walk(struct{ afero.Fs }{f.fs}, root, fn)
}
// prefixExists reports whether prefix exists under the destination
// directory. It checks each directory on the way down to prefix in turn,
// and the first one Stat cannot find decides: if it is not there at all,
// prefix is missing; if it is a symlink whose target is missing, such as
// a link to a volume that is not mounted, prefixExists fails, since
// listing it as empty would let prune delete every blob.
func (f *FileStorer) prefixExists(prefix string) (bool, error) {
lstater, canLstat := f.fs.(afero.Lstater)
path := f.basePath
for name := range strings.SplitSeq(prefix, "/") {
path = filepath.Join(path, name)
_, err := f.fs.Stat(path)
if err == nil {
continue
}
if !os.IsNotExist(err) {
return false, err
}
// Lstat does not follow a symlink at path, so it finds the link
// that Stat could not follow.
if canLstat {
_, _, lstatErr := lstater.LstatIfPossible(path)
if lstatErr == nil {
return false, err
}
}
return false, nil
}
return true, nil
}
// fullPath returns the full filesystem path for a key.
func (f *FileStorer) fullPath(key string) string {
return filepath.Join(f.basePath, key)
-232
View File
@@ -4,10 +4,7 @@ import (
"context"
"errors"
"io/fs"
"os"
"path/filepath"
"reflect"
"strings"
"testing"
"sneak.berlin/go/vaultik/internal/storage"
@@ -63,232 +60,3 @@ func TestFileStorerListMissingDestination(t *testing.T) {
t.Errorf("ListStream error = %v, want a not-exist error", streamErr)
}
}
// The two directories at the top of a destination, either of which the
// tests below replace with a symlink.
const (
metadataDir = "metadata"
blobsDir = "blobs"
)
// moveBehindSymlink moves base/dir to a directory elsewhere and puts a
// symlink to it in its place.
func moveBehindSymlink(t *testing.T, base, dir string) {
t.Helper()
linkPath := filepath.Join(base, dir)
movedPath := filepath.Join(t.TempDir(), dir)
err := os.Rename(linkPath, movedPath)
if err != nil {
t.Fatalf("moving %s away: %v", dir, err)
}
err = os.Symlink(movedPath, linkPath)
if err != nil {
t.Fatalf("linking %s: %v", dir, err)
}
}
// TestFileStorerListsThroughSymlinkedDirectory checks that List and
// ListStream read through a symlinked metadata/ or blobs/ directory, as Get
// and Stat do, instead of listing the link as one key. Listing the whole
// destination meets the link part-way through the walk; listing its prefix
// starts the walk at the link. A prefix missing behind the link lists as
// empty.
func TestFileStorerListsThroughSymlinkedDirectory(t *testing.T) {
t.Parallel()
for _, dir := range []string{metadataDir, blobsDir} {
t.Run(dir, func(t *testing.T) {
t.Parallel()
base := t.TempDir()
key := dir + "/aa/bb/object"
s, err := storage.NewFileStorer(base)
if err != nil {
t.Fatalf("NewFileStorer: %v", err)
}
err = s.Put(context.Background(), key, strings.NewReader("data"))
if err != nil {
t.Fatalf("Put: %v", err)
}
moveBehindSymlink(t, base, dir)
want := []string{key}
for _, prefix := range []string{"", dir + "/"} {
got := listSorted(t, s, prefix)
if !reflect.DeepEqual(got, want) {
t.Errorf("List(%q) = %v, want %v", prefix, got, want)
}
got = listStreamKeys(t, s, prefix)
if !reflect.DeepEqual(got, want) {
t.Errorf("ListStream(%q) = %v, want %v", prefix, got, want)
}
}
absent := dir + "/absent/"
if got := listSorted(t, s, absent); len(got) != 0 {
t.Errorf("List(%q) = %v, want empty", absent, got)
}
if got := listStreamKeys(t, s, absent); len(got) != 0 {
t.Errorf("ListStream(%q) = %v, want empty", absent, got)
}
})
}
}
// TestFileStorerListFailsOnSymlinkWithMissingTarget checks that List and
// ListStream fail on a symlinked metadata/ or blobs/ directory whose target
// is missing, such as a link to a volume that is not mounted, as they do on
// a missing destination directory, instead of listing it as empty. That
// holds for a prefix below the link too.
func TestFileStorerListFailsOnSymlinkWithMissingTarget(t *testing.T) {
t.Parallel()
ctx := context.Background()
for _, dir := range []string{metadataDir, blobsDir} {
t.Run(dir, func(t *testing.T) {
t.Parallel()
base := t.TempDir()
s, err := storage.NewFileStorer(base)
if err != nil {
t.Fatalf("NewFileStorer: %v", err)
}
unmounted := filepath.Join(t.TempDir(), "unmounted")
err = os.Symlink(unmounted, filepath.Join(base, dir))
if err != nil {
t.Fatalf("Symlink: %v", err)
}
for _, prefix := range []string{"", dir + "/", dir + "/aa/"} {
keys, err := s.List(ctx, prefix)
if !errors.Is(err, fs.ErrNotExist) {
t.Errorf("List(%q) = %v, %v; want a not-exist error",
prefix, keys, err)
}
var streamErr error
for object := range s.ListStream(ctx, prefix) {
if object.Err != nil {
streamErr = object.Err
}
}
if !errors.Is(streamErr, fs.ErrNotExist) {
t.Errorf("ListStream(%q) error = %v, want a not-exist error",
prefix, streamErr)
}
}
})
}
}
// TestFileStorerListFailsOnSymlinkLoop checks that a symlink pointing back
// at the destination makes List and ListStream fail instead of walking it
// forever.
func TestFileStorerListFailsOnSymlinkLoop(t *testing.T) {
t.Parallel()
ctx := context.Background()
base := t.TempDir()
s, err := storage.NewFileStorer(base)
if err != nil {
t.Fatalf("NewFileStorer: %v", err)
}
err = s.Put(ctx, testBlobKey, strings.NewReader("data"))
if err != nil {
t.Fatalf("Put: %v", err)
}
err = os.Symlink(base, filepath.Join(base, "blobs", "loop"))
if err != nil {
t.Fatalf("Symlink: %v", err)
}
keys, err := s.List(ctx, "blobs/")
if err == nil {
t.Errorf("List = %d keys and no error, want an error", len(keys))
}
var streamErr error
for object := range s.ListStream(ctx, "blobs/") {
if object.Err != nil {
streamErr = object.Err
}
}
if streamErr == nil {
t.Error("ListStream sent no error, want one")
}
}
// TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory checks that
// DeletePartialUploads, which remote nuke uses, removes a leftover temp
// file behind a symlinked metadata/ or blobs/ directory and keeps the
// object at the real key.
func TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory(t *testing.T) {
t.Parallel()
ctx := context.Background()
for _, dir := range []string{metadataDir, blobsDir} {
t.Run(dir, func(t *testing.T) {
t.Parallel()
base := t.TempDir()
key := dir + "/aa/bb/object"
s, err := storage.NewFileStorer(base)
if err != nil {
t.Fatalf("NewFileStorer: %v", err)
}
err = s.Put(ctx, key, strings.NewReader("data"))
if err != nil {
t.Fatalf("Put: %v", err)
}
leftover := filepath.Join(base, key+"-123456.partial")
err = os.WriteFile(leftover, []byte("half"), 0o600)
if err != nil {
t.Fatalf("writing leftover temp file: %v", err)
}
moveBehindSymlink(t, base, dir)
err = s.DeletePartialUploads(ctx, dir+"/")
if err != nil {
t.Fatalf("DeletePartialUploads: %v", err)
}
// leftover runs through the link.
_, err = os.Stat(leftover)
if !os.IsNotExist(err) {
t.Errorf("leftover temp file was not removed: %v", err)
}
_, err = s.Stat(ctx, key)
if err != nil {
t.Errorf("Stat of the real key: %v", err)
}
})
}
}
+5 -3
View File
@@ -627,7 +627,8 @@ func selectSnapshotsToPurge(
}
// confirmAndExecutePurge shows deletion candidates, confirms with the
// user, and deletes snapshots.
// user, and deletes snapshots. It stops with an error at the first
// snapshot it cannot delete.
func (v *Vaultik) confirmAndExecutePurge(
toDelete []SnapshotInfo, force, quiet bool,
) error {
@@ -669,10 +670,11 @@ func (v *Vaultik) confirmAndExecutePurge(
snapshotID := snap.ID.String()
log.Info("Deleting snapshot", "id", snapshotID)
// Stop before the remote delete: a snapshot still in the local
// index must keep its metadata on the destination store.
err := v.deleteSnapshotFromLocalDB(snapshotID)
if err != nil {
log.Error("Failed to delete from local database",
"snapshot_id", snapshotID, "error", err)
return fmt.Errorf("deleting snapshot %s from local index: %w", snapshotID, err)
}
_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
@@ -1,147 +0,0 @@
package vaultik_test
import (
"context"
"io/fs"
"os"
"path/filepath"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/vaultik/internal/log"
"sneak.berlin/go/vaultik/internal/vaultik"
)
// TestPruneDeletesNothingThroughSymlinkedDirectory checks that prune keeps
// every blob and the local snapshot record when the metadata/ or blobs/
// directory of a file:// destination is a symlink to a directory elsewhere
// (https://git.eeqj.de/sneak/vaultik/issues/296). A backup writes through
// such a link, so prune has to list through it too.
func TestPruneDeletesNothingThroughSymlinkedDirectory(t *testing.T) {
log.Initialize(log.Config{})
t.Parallel()
for _, dir := range []string{"metadata", "blobs"} {
t.Run(dir, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
storeDir := filepath.Join(t.TempDir(), "store")
v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
blobFiles := blobFilesIn(t, storeDir)
moveBehindSymlink(t, storeDir, dir)
require.NoError(t, v.Prune(&vaultik.PruneOptions{Force: true}))
// When blobs/ is the link, these paths run through it.
for _, blobFile := range blobFiles {
assert.FileExists(t, blobFile, "prune must delete no blob")
}
snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
require.NoError(t, err)
assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
})
}
}
// TestPruneFailsOnSymlinkedDirectoryWithMissingTarget checks that prune
// fails, deleting no blob and keeping the local snapshot record, when the
// metadata/ or blobs/ directory of a file:// destination is a symlink
// whose target is missing, as a link to a volume that is not mounted is.
func TestPruneFailsOnSymlinkedDirectoryWithMissingTarget(t *testing.T) {
log.Initialize(log.Config{})
t.Parallel()
for _, dir := range []string{"metadata", "blobs"} {
t.Run(dir, func(t *testing.T) {
t.Parallel()
ctx := context.Background()
storeDir := filepath.Join(t.TempDir(), "store")
v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
blobFiles := blobFilesIn(t, storeDir)
targetPath := moveBehindSymlink(t, storeDir, dir)
// Unmount the volume the link points to, and mount it again
// once prune has run.
unmountedPath := targetPath + "-unmounted"
require.NoError(t, os.Rename(targetPath, unmountedPath))
err := v.Prune(&vaultik.PruneOptions{Force: true})
require.ErrorIs(t, err, fs.ErrNotExist)
require.NoError(t, os.Rename(unmountedPath, targetPath))
// When blobs/ is the link, these paths run through it.
for _, blobFile := range blobFiles {
assert.FileExists(t, blobFile, "prune must delete no blob")
}
snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
require.NoError(t, err)
assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
})
}
}
// TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget checks that
// snapshot remove warns that the metadata could not be removed from the
// destination when metadata/ is a symlink whose target is missing, as it
// does when the destination directory is missing, instead of finding no
// metadata to remove.
func TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget(t *testing.T) {
log.Initialize(log.Config{})
t.Parallel()
ctx := context.Background()
storeDir := filepath.Join(t.TempDir(), "store")
v, repos, out := backUpToFileDestination(ctx, t, storeDir)
targetPath := moveBehindSymlink(t, storeDir, "metadata")
require.NoError(t, os.Rename(targetPath, targetPath+"-unmounted"))
result, err := v.RemoveSnapshot(localSnapshotID(ctx, t, repos, "first"),
&vaultik.RemoveOptions{Force: true})
require.NoError(t, err)
assert.False(t, result.RemoteRemoved)
assert.Contains(t, out.String(),
"Could not remove snapshot metadata from remote")
assert.NotContains(t, out.String(),
"Removed snapshot metadata from remote storage")
}
// blobFilesIn returns the paths of the blob files under storeDir/blobs, and
// fails the test when there are none.
func blobFilesIn(t *testing.T, storeDir string) []string {
t.Helper()
var blobFiles []string
err := filepath.WalkDir(filepath.Join(storeDir, "blobs"),
func(path string, entry fs.DirEntry, err error) error {
if err == nil && !entry.IsDir() {
blobFiles = append(blobFiles, path)
}
return err
})
require.NoError(t, err)
require.NotEmpty(t, blobFiles)
return blobFiles
}
// moveBehindSymlink moves storeDir/dir to a directory elsewhere, puts a
// symlink to it in its place, and returns where it went.
func moveBehindSymlink(t *testing.T, storeDir, dir string) string {
t.Helper()
linkPath := filepath.Join(storeDir, dir)
targetPath := filepath.Join(t.TempDir(), dir)
require.NoError(t, os.Rename(linkPath, targetPath))
require.NoError(t, os.Symlink(targetPath, linkPath))
return targetPath
}