diff --git a/TODO.md b/TODO.md index 9160574..59037e2 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,17 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-13: Made a `file://` destination list through a symlinked + directory ([issue #296](https://git.eeqj.de/sneak/vaultik/issues/296)). + A `metadata/` or `blobs/` directory that was a symlink to a directory + elsewhere was listed as one file, so the destination store listed as + empty while backups and restores still worked through the link. + `prune` then removed the local snapshot records and deleted every blob, + and `remote info` and `remote nuke` found no blobs. The listings now + read through the link as `Get` and `Stat` do. A link whose target is + missing, such as one to a volume that is not mounted, or a link + pointing back up the tree makes them fail. + - 2026-10-13: Stopped `remote info` counting a file under `blobs/` that is not a blob as an orphaned blob ([issue #295](https://git.eeqj.de/sneak/vaultik/issues/295)). A stray diff --git a/internal/storage/file.go b/internal/storage/file.go index 7bb74d1..9fc6268 100644 --- a/internal/storage/file.go +++ b/internal/storage/file.go @@ -120,9 +120,10 @@ func (f *FileStorer) Delete(_ context.Context, key string) error { return nil } -// 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. +// List returns all keys with the given prefix, reading through symlinked +// directories. It fails when the destination directory is missing or the +// prefix is a symlink whose target is missing; a missing prefix under the +// destination directory is an empty listing. func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) { var keys []string @@ -133,8 +134,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) basePath := f.fullPath(prefix) - // Check if the prefix exists - exists, err := afero.Exists(f.fs, basePath) + exists, err := f.prefixExists(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 = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error { + err = f.walk(basePath, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -176,7 +176,9 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) } // ListStream returns a channel of ObjectInfo for large result sets. Like -// List, it sends an error when the destination directory is missing. +// List, it reads through symlinked directories and sends an error when +// the destination directory is missing or the prefix is a symlink whose +// target is missing. func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan ObjectInfo { ch := make(chan ObjectInfo) @@ -192,8 +194,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec basePath := f.fullPath(prefix) - // Check if the prefix exists - exists, err := afero.Exists(f.fs, basePath) + exists, err := f.prefixExists(basePath) if err != nil { ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)} @@ -204,7 +205,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec return // Empty channel for non-existent prefix } - _ = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error { + _ = f.walk(basePath, func(path string, info os.FileInfo, err error) error { // Check context cancellation select { case <-ctx.Done(): @@ -352,6 +353,40 @@ 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 is afero.Exists, except that it does not follow a symlink +// at path. A symlinked prefix whose target is missing, such as a link to +// a volume that is not mounted, therefore exists, and walk then fails on +// it; with afero.Exists it would list as empty, and prune would delete +// every blob. +func (f *FileStorer) prefixExists(path string) (bool, error) { + lstater, ok := f.fs.(afero.Lstater) + if !ok { + return afero.Exists(f.fs, path) + } + + _, _, err := lstater.LstatIfPossible(path) + if err == nil { + return true, nil + } + + if os.IsNotExist(err) { + return false, nil + } + + return false, err +} + // fullPath returns the full filesystem path for a key. func (f *FileStorer) fullPath(key string) string { return filepath.Join(f.basePath, key) diff --git a/internal/storage/file_backend_test.go b/internal/storage/file_backend_test.go index f92331e..818e589 100644 --- a/internal/storage/file_backend_test.go +++ b/internal/storage/file_backend_test.go @@ -4,7 +4,10 @@ import ( "context" "errors" "io/fs" + "os" "path/filepath" + "reflect" + "strings" "testing" "sneak.berlin/go/vaultik/internal/storage" @@ -60,3 +63,151 @@ func TestFileStorerListMissingDestination(t *testing.T) { t.Errorf("ListStream error = %v, want a not-exist error", streamErr) } } + +// 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. +func TestFileStorerListsThroughSymlinkedDirectory(t *testing.T) { + t.Parallel() + + for _, dir := range []string{"metadata", "blobs"} { + 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) + } + + 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) + } + + 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) + } + } + }) + } +} + +// 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. +func TestFileStorerListFailsOnSymlinkWithMissingTarget(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + for _, dir := range []string{"metadata", "blobs"} { + 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 + "/"} { + 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") + } +} diff --git a/internal/vaultik/symlinked_destination_test.go b/internal/vaultik/symlinked_destination_test.go new file mode 100644 index 0000000..3dc1192 --- /dev/null +++ b/internal/vaultik/symlinked_destination_test.go @@ -0,0 +1,121 @@ +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") + }) + } +} + +// 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 +}