From ea726979922ec3ef34793805212451a4fcd41b48 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 6 Oct 2026 08:46:17 +0200 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. Three tests listed a file:// destination nothing had created; they now create it. Model: opus-5-5 --- TODO.md | 10 ++ internal/cli/entry_banner_test.go | 15 +- internal/cli/entry_prune_json_test.go | 15 +- internal/storage/file.go | 31 +++- internal/storage/file_backend_test.go | 35 +++++ internal/vaultik/fault_injection_test.go | 4 + internal/vaultik/missing_destination_test.go | 155 +++++++++++++++++++ 7 files changed, 246 insertions(+), 19 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_banner_test.go b/internal/cli/entry_banner_test.go index cbb51e3..df7321b 100644 --- a/internal/cli/entry_banner_test.go +++ b/internal/cli/entry_banner_test.go @@ -172,9 +172,9 @@ func TestBannerSuppressedInArgs(t *testing.T) { // hermeticConfig is a complete, valid config that needs no network and // no credentials: file:// storage is exempt from the S3 credential -// checks, and FileStorer over a directory that does not exist lists -// zero objects without erroring. Chunk, blob and compression settings -// are filled in by config.Load. +// checks. A test that lists the destination must create its directory +// first, because listing a directory that does not exist is an error. +// Chunk, blob and compression settings are filled in by config.Load. const hermeticConfig = `age_recipients: - age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj snapshots: @@ -196,22 +196,25 @@ hostname: test-host // `snapshot list` is the command chosen because it is the only --json // command that reaches its document without a populated destination // store: it reads the local index, streams `metadata/` (empty here), -// and treats a barren destination as an empty list rather than a -// failure. +// and treats an empty destination directory as an empty list rather +// than a failure. // // Not parallel: it replaces os.Args, os.Stdout and the xdg globals. func TestEntryJSONStdoutIsExactlyOneDocument(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.yml") + storeDir := filepath.Join(dir, "store") contents := fmt.Sprintf(hermeticConfig, filepath.Join(dir, "source"), - filepath.Join(dir, "store"), + storeDir, filepath.Join(dir, "index.sqlite")) require.NoError(t, os.WriteFile(configPath, []byte(contents), configFileMode)) + require.NoError(t, os.Mkdir(storeDir, 0o750)) + // 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/cli/entry_prune_json_test.go b/internal/cli/entry_prune_json_test.go index 7f4d6e9..b220df4 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, 0o750)) + // 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..bcf2c3d 100644 --- a/internal/storage/file.go +++ b/internal/storage/file.go @@ -22,11 +22,11 @@ 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, because 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 +119,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 +174,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..f92331e 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, 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..594c284 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, 0o750)) + 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..3869bc7 --- /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) + require.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") +}