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") +}