From 6488c54560c1b472cc2db36047a10034b759969c Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 6 Oct 2026 04:22:54 +0000 Subject: [PATCH] List a missing file:// destination directory as an error (closes #220) The file backend listed a destination directory that does not exist as an empty store. With the volume unplugged, snapshot list reported every local snapshot as missing from the store, snapshot remove said it had removed metadata it never reached, and prune dropped every local snapshot record. List and ListStream now fail when the destination directory is missing, so those commands take their existing path for a store that cannot be listed. A missing prefix under an existing directory is still an empty listing, and a first backup still creates the directory. Two test fixtures listed a file:// destination nothing had created; they now create it. Model: opus-5-5 --- TODO.md | 10 ++ internal/cli/entry_prune_json_test.go | 15 +- internal/storage/file.go | 32 +++- internal/storage/file_backend_test.go | 35 +++++ internal/vaultik/fault_injection_test.go | 4 + internal/vaultik/missing_destination_test.go | 155 +++++++++++++++++++ 6 files changed, 238 insertions(+), 13 deletions(-) create mode 100644 internal/vaultik/missing_destination_test.go diff --git a/TODO.md b/TODO.md index dbf60b3..d8432df 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,16 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-06: Made a `file://` destination whose directory is missing + count as one that cannot be listed + ([issue #220](https://git.eeqj.de/sneak/vaultik/issues/220)). The file + backend listed a missing directory as an empty store, so with the + quickstart's USB stick unplugged `snapshot list` reported every local + snapshot as missing from the store, `snapshot remove` claimed to have + removed metadata it never reached, and `prune` dropped every local + snapshot record. Listing a missing destination directory is now an + error; a first backup still creates the directory. + - 2026-10-06: Made `--older-than` and `--keep-newer-than` reject a duration with characters outside its number-and-unit parts ([issue #215](https://git.eeqj.de/sneak/vaultik/issues/215)). The diff --git a/internal/cli/entry_prune_json_test.go b/internal/cli/entry_prune_json_test.go index 7f4d6e9..7ef8628 100644 --- a/internal/cli/entry_prune_json_test.go +++ b/internal/cli/entry_prune_json_test.go @@ -98,25 +98,30 @@ func TestEntryPruneJSONStdoutIsExactlyOneDocument(t *testing.T) { } } -// writeHermeticPruneConfig builds a config over a temp directory and, if -// seedStale is set, creates the index database up front with one -// snapshot record that has no counterpart on the destination store. -// Returns the config path. +// writeHermeticPruneConfig builds a config over a temp directory with an +// empty destination directory and, if seedStale is set, creates the +// index database up front with one snapshot record that has no +// counterpart on the destination store. Returns the config path. func writeHermeticPruneConfig(t *testing.T, seedStale bool) string { t.Helper() dir := t.TempDir() configPath := filepath.Join(dir, "config.yml") indexPath := filepath.Join(dir, "index.sqlite") + storeDir := filepath.Join(dir, "store") contents := fmt.Sprintf(hermeticConfig, filepath.Join(dir, "source"), - filepath.Join(dir, "store"), + storeDir, indexPath) require.NoError(t, os.WriteFile(configPath, []byte(contents), configFileMode)) + // prune fails on a destination directory that does not exist, so the + // empty store is created here rather than left to a first backup. + require.NoError(t, os.Mkdir(storeDir, 0o755)) + // The PID lock lives under xdg.DataHome, which xdg resolves at // package init; point it at the temp dir so the test neither // touches nor collides with the real one. diff --git a/internal/storage/file.go b/internal/storage/file.go index 5c31898..f47ea0b 100644 --- a/internal/storage/file.go +++ b/internal/storage/file.go @@ -22,11 +22,12 @@ type FileStorer struct { // // Construction is intentionally cheap and does not touch the filesystem. // The basePath is recorded; the directory is created lazily on first -// write. Reads (Get/Stat/List) tolerate a missing basePath — a missing -// or unmounted destination during `snapshot list` should NOT block the -// command, it should degrade to "no remote snapshots reachable" with a -// warning. Write operations (Put/PutWithProgress) call MkdirAll for the +// write. Write operations (Put/PutWithProgress) call MkdirAll for the // per-blob parent directory, which also covers basePath on first use. +// Get and Stat report a key under a missing basePath as ErrNotFound. +// List and ListStream fail on a missing basePath: a missing or unmounted +// destination cannot be listed, and listing it as an empty store would +// make `prune` drop every local snapshot record. // // Uses the real OS filesystem by default; call SetFilesystem to // override for testing. @@ -119,13 +120,20 @@ func (f *FileStorer) Delete(_ context.Context, key string) error { return nil } -// List returns all keys with the given prefix. +// 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 + _, err := f.fs.Stat(f.basePath) + if err != nil { + return nil, fmt.Errorf("checking destination directory: %w", err) + } + basePath := f.fullPath(prefix) - // Check if base path exists + // Check if the prefix exists exists, err := afero.Exists(f.fs, basePath) if err != nil { return nil, fmt.Errorf("checking path: %w", err) @@ -167,16 +175,24 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) return keys, nil } -// ListStream returns a channel of ObjectInfo for large result sets. +// ListStream returns a channel of ObjectInfo for large result sets. Like +// 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) go func() { defer close(ch) + _, err := f.fs.Stat(f.basePath) + if err != nil { + ch <- ObjectInfo{Err: fmt.Errorf("checking destination directory: %w", err)} + + return + } + basePath := f.fullPath(prefix) - // Check if base path exists + // Check if the prefix exists exists, err := afero.Exists(f.fs, basePath) if err != nil { ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)} diff --git a/internal/storage/file_backend_test.go b/internal/storage/file_backend_test.go index de5ef31..40a7d26 100644 --- a/internal/storage/file_backend_test.go +++ b/internal/storage/file_backend_test.go @@ -1,6 +1,10 @@ package storage_test import ( + "context" + "errors" + "io/fs" + "path/filepath" "testing" "sneak.berlin/go/vaultik/internal/storage" @@ -25,3 +29,34 @@ func TestFileStorer(t *testing.T) { t.Parallel() runStorerConformance(t, newFileStorer) } + +// TestFileStorerListMissingDestination checks that List and ListStream +// fail when the destination directory does not exist, as on an unmounted +// volume, instead of reporting an empty store. +func TestFileStorerListMissingDestination(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + s, err := storage.NewFileStorer(filepath.Join(t.TempDir(), "unmounted")) + if err != nil { + t.Fatalf("NewFileStorer: %v", err) + } + + keys, err := s.List(ctx, "metadata/") + if !errors.Is(err, fs.ErrNotExist) { + t.Errorf("List = %v, %v; want a not-exist error", keys, err) + } + + var streamErr error + + for object := range s.ListStream(ctx, "metadata/") { + if object.Err != nil { + streamErr = object.Err + } + } + + if !errors.Is(streamErr, fs.ErrNotExist) { + t.Errorf("ListStream error = %v, want a not-exist error", streamErr) + } +} diff --git a/internal/vaultik/fault_injection_test.go b/internal/vaultik/fault_injection_test.go index 055fcfd..0576a89 100644 --- a/internal/vaultik/fault_injection_test.go +++ b/internal/vaultik/fault_injection_test.go @@ -301,6 +301,10 @@ func TestInterruptedBlobUploadRecordsNoUploadedBlob(t *testing.T) { writeFaultSourceTree(t, fs, dataDir) + // No upload succeeds, so nothing creates the destination directory. + // It must exist for the listing below to show that no blob survived. + require.NoError(t, os.Mkdir(storeDir, 0o755)) + inner, err := storage.NewFileStorer(storeDir) require.NoError(t, err) diff --git a/internal/vaultik/missing_destination_test.go b/internal/vaultik/missing_destination_test.go new file mode 100644 index 0000000..d9d35d2 --- /dev/null +++ b/internal/vaultik/missing_destination_test.go @@ -0,0 +1,155 @@ +package vaultik_test + +import ( + "bytes" + "context" + "io/fs" + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "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" +) + +// These tests cover https://git.eeqj.de/sneak/vaultik/issues/220: a +// file:// destination whose directory is missing, such as a USB stick +// that is not plugged in, cannot be listed. It is not an empty store, so +// no command may conclude from it that the local snapshots are gone. + +// backUpToFileDestination backs up the snapshot named "first" to a +// file:// destination at storeDir, which need not exist yet. Everything +// the returned Vaultik prints after the backup goes to the returned +// buffer. +func backUpToFileDestination( + ctx context.Context, t *testing.T, storeDir string, +) (*vaultik.Vaultik, *database.Repositories, *bytes.Buffer) { + t.Helper() + + osFs := afero.NewOsFs() + tempDir := t.TempDir() + dataDir := filepath.Join(tempDir, "src") + dbPath := filepath.Join(tempDir, "index.sqlite") + + writeFaultSourceTree(t, osFs, dataDir) + + store, err := storage.NewFileStorer(storeDir) + require.NoError(t, err) + + db, err := database.New(ctx, dbPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + repos := database.NewRepositories(db) + cfg := changedFileConfig(dataDir, dbPath) + v := newBackupVaultik(ctx, cfg, store, repos, db, osFs) + + require.NoError(t, backUp(v, "first")) + + out := &bytes.Buffer{} + v.Stdout = out + v.UI = ui.NewWithColor(out, false) + + return v, repos, out +} + +// backUpThenUnplug backs up to a file:// destination, then moves the +// destination directory away, as unplugging the volume it lives on would. +func backUpThenUnplug( + ctx context.Context, t *testing.T, +) (*vaultik.Vaultik, *database.Repositories, *bytes.Buffer) { + t.Helper() + + storeDir := filepath.Join(t.TempDir(), "usbstick") + v, repos, out := backUpToFileDestination(ctx, t, storeDir) + + require.NoError(t, os.Rename(storeDir, storeDir+"-unplugged")) + + return v, repos, out +} + +// TestFirstBackupCreatesDestinationDirectory checks that a first backup +// to a destination directory that does not exist yet creates it, and +// that the destination can be listed afterwards. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestFirstBackupCreatesDestinationDirectory(t *testing.T) { + log.Initialize(log.Config{}) + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "volume", "backup") + v, _, out := backUpToFileDestination(ctx, t, storeDir) + + require.NoError(t, v.ListSnapshots(false)) + + assert.NotContains(t, out.String(), "Could not list backup destination store") + assert.NotContains(t, out.String(), "not found in backup destination store") +} + +// TestListSnapshotsWarnsWhenDestinationMissing checks that snapshot list +// warns and shows the local index alone, without reporting the local +// snapshot as missing from the destination. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestListSnapshotsWarnsWhenDestinationMissing(t *testing.T) { + log.Initialize(log.Config{}) + + ctx := context.Background() + v, repos, out := backUpThenUnplug(ctx, t) + id := localSnapshotID(ctx, t, repos, "first") + + require.NoError(t, v.ListSnapshots(false)) + + assert.Contains(t, out.String(), "Could not list backup destination store") + assert.Contains(t, out.String(), "Showing snapshots from the local index only.") + assert.Contains(t, out.String(), id) + assert.NotContains(t, out.String(), "not found in backup destination store") +} + +// TestRemoveSnapshotWarnsWhenDestinationMissing checks that snapshot +// remove warns that the metadata could not be removed from the +// destination, instead of reporting that it was. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestRemoveSnapshotWarnsWhenDestinationMissing(t *testing.T) { + log.Initialize(log.Config{}) + + ctx := context.Background() + v, repos, out := backUpThenUnplug(ctx, t) + + 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") +} + +// TestPruneKeepsLocalRecordsWhenDestinationMissing checks that prune +// fails on a destination it cannot list and deletes no local snapshot +// record. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestPruneKeepsLocalRecordsWhenDestinationMissing(t *testing.T) { + log.Initialize(log.Config{}) + + ctx := context.Background() + v, repos, _ := backUpThenUnplug(ctx, t) + + err := v.Prune(&vaultik.PruneOptions{Force: true}) + require.ErrorIs(t, err, fs.ErrNotExist) + assert.ErrorContains(t, err, "listing remote snapshots") + + snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + assert.Len(t, snapshots, 1, "prune must delete no local snapshot record") +}