backup: harden fs/s3 storage backends

Address review findings on the new lib/backup/storage backends:

- s3: distinguish a missing key from a missing bucket. isKeyNotFound
  now matches only NoSuchKey, not any 404 (a missing bucket returns
  404 as NoSuchBucket). Get also probes the object with a one-byte
  read (GET) rather than Stat (HEAD): a GET 404 has a body, so the
  real code is preserved; a HEAD 404 has none and is reported as
  NoSuchKey regardless. The probe also drops the extra HEAD request.
- fs: Put flushes the data and parent directory to disk before
  returning, so a crash cannot leave an object present but truncated.
- fs: Put widens the object mode from CreateTemp's 0600 to 0644, so a
  backup written by one user can be read by another.
- fs: objectPath uses a relative-path escape check; the old
  string-prefix check rejected every key when the root resolved to /.
- fs: Get rejects a key that resolves to a directory with
  ErrKeyNotFound instead of a later, opaque read error.
- fs: Put enforces the size contract (non-negative, exact byte count)
  so a wrong size fails loudly instead of storing a short object; the
  Storage interface now documents the contract for both backends.
- fs: New sweeps stale temp files left by interrupted Puts, which are
  otherwise hidden from List and cannot be removed through the API.
- fs: List returns a non-nil empty slice, matching the s3 backend.
- s3 integration test: tag integration_docker so unit:fullSkipDocker
  excludes it, matching the repo convention.

Part of TNTP-8210

Author: bigbes

lib/backup/storage/fs/fs.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/tarantool/tt/lib/backup/storage"
 )
@@ -20,7 +21,14 @@ const tempFilePrefix = ".tt-backup-"
 // tempFilePattern is the glob pattern passed to os.CreateTemp for temporary files.
 const tempFilePattern = tempFilePrefix + "*"
 
-var errPathRequired = errors.New("fs storage path is required")
+// staleTempFileAge is how old a leftover temp file must be before New sweeps it.
+// The threshold keeps the sweep from racing a concurrent Put in another process.
+const staleTempFileAge = 24 * time.Hour
+
+var (
+	errPathRequired = errors.New("fs storage path is required")
+	errNegativeSize = errors.New("fs object size must be non-negative")
+)
 
 // Config describes local filesystem storage configuration.
 type Config struct {
@@ -58,7 +66,37 @@ func New(cfg Config) (*Storage, error) {
 		return nil, fmt.Errorf("failed to resolve storage root %q: %w", cfg.Path, err)
 	}
 
-	return &Storage{root: root}, nil
+	s := &Storage{root: root}
+	s.sweepStaleTempFiles()
+
+	return s, nil
+}
+
+// sweepStaleTempFiles best-effort removes temp files left behind by interrupted
+// Put calls (process killed between os.CreateTemp and the deferred os.Remove).
+// Such files are hidden from List and cannot be Deleted through the API, so they
+// would otherwise accumulate forever. Errors are ignored: this is an optimization,
+// not a guarantee, and the root may not exist yet.
+func (s *Storage) sweepStaleTempFiles() {
+	cutoff := time.Now().Add(-staleTempFileAge)
+	_ = filepath.WalkDir(s.root, func(path string, d os.DirEntry, err error) error {
+		switch {
+		case err != nil:
+			return nil
+		case d.IsDir() || !isTempFile(d.Name()):
+			return nil
+		}
+
+		info, err := d.Info()
+		switch {
+		case err != nil:
+			return nil
+		case info.ModTime().Before(cutoff):
+			_ = os.Remove(path)
+		}
+
+		return nil
+	})
 }
 
 // List returns objects whose keys start with the given prefix, sorted by key.
@@ -74,7 +112,7 @@ func (s *Storage) List(ctx context.Context, prefix string) ([]storage.ObjectInfo
 
 	root := filepath.Join(s.root, filepath.FromSlash(path.Dir(cleanPrefix)))
 
-	var objects []storage.ObjectInfo
+	objects := make([]storage.ObjectInfo, 0)
 	if _, err := os.Stat(root); err != nil {
 		if errors.Is(err, os.ErrNotExist) {
 			return objects, nil
@@ -165,12 +203,25 @@ func (s *Storage) Get(ctx context.Context, key string) (io.ReadCloser, error) {
 		return nil, fmt.Errorf("failed to open object %q: %w", key, err)
 	}
 
+	// A key that resolves to a directory is not a stored object; report it as
+	// missing here rather than deferring an opaque EISDIR to the caller's Read.
+	info, err := f.Stat()
+	switch {
+	case err != nil:
+		_ = f.Close()
+		return nil, fmt.Errorf("failed to stat object %q: %w", key, err)
+	case !info.Mode().IsRegular():
+		_ = f.Close()
+		return nil, storage.ErrKeyNotFound
+	}
+
 	return f, nil
 }
 
-// Put stores an object to the filesystem. The size parameter is unused;
-// it exists to satisfy the storage.Storage interface.
-func (s *Storage) Put(ctx context.Context, key string, r io.Reader, _ int64) error {
+// Put stores an object to the filesystem via a temp file and atomic rename.
+// size must be the exact, non-negative number of bytes r yields; a mismatch is
+// an error so a wrong size cannot silently store a truncated object.
+func (s *Storage) Put(ctx context.Context, key string, r io.Reader, size int64) error {
 	if err := ctx.Err(); err != nil {
 		return fmt.Errorf("failed to put object %q: %w", key, err)
 	}
@@ -180,6 +231,10 @@ func (s *Storage) Put(ctx context.Context, key string, r io.Reader, _ int64) err
 		return fmt.Errorf("failed to resolve object path %q: %w", key, err)
 	}
 
+	if size < 0 {
+		return fmt.Errorf("failed to put object %q: %w", key, errNegativeSize)
+	}
+
 	dir := filepath.Dir(path)
 	if err := os.MkdirAll(dir, 0o755); err != nil {
 		return fmt.Errorf("failed to create object directory for %q: %w", key, err)
@@ -193,9 +248,29 @@ func (s *Storage) Put(ctx context.Context, key string, r io.Reader, _ int64) err
 	tmpPath := tmp.Name()
 	defer os.Remove(tmpPath)
 
-	if _, err := io.Copy(tmp, ctxReader{ctx, r}); err != nil {
+	written, err := io.Copy(tmp, ctxReader{ctx, r})
+	switch {
+	case err != nil:
 		_ = tmp.Close()
 		return fmt.Errorf("failed to write object %q: %w", key, err)
+	case written != size:
+		_ = tmp.Close()
+		return fmt.Errorf("failed to write object %q: wrote %d bytes, expected %d",
+			key, written, size)
+	}
+
+	// os.CreateTemp makes the file 0600; widen it to match the 0755 directories
+	// so a backup written by one user can be read back by another.
+	if err := tmp.Chmod(0o644); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("failed to set object mode %q: %w", key, err)
+	}
+
+	// Flush the data before the rename so a crash cannot leave the object present
+	// under its final name but truncated or empty.
+	if err := tmp.Sync(); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("failed to sync object %q: %w", key, err)
 	}
 
 	if err := tmp.Close(); err != nil {
@@ -206,6 +281,28 @@ func (s *Storage) Put(ctx context.Context, key string, r io.Reader, _ int64) err
 		return fmt.Errorf("failed to store object %q: %w", key, err)
 	}
 
+	// Flush the directory so the rename itself survives a crash.
+	if err := syncDir(dir); err != nil {
+		return fmt.Errorf("failed to persist object %q: %w", key, err)
+	}
+
+	return nil
+}
+
+// syncDir flushes a directory to disk so a rename within it is durable across a crash.
+func syncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return fmt.Errorf("failed to open directory %q: %w", dir, err)
+	}
+	if err := d.Sync(); err != nil {
+		_ = d.Close()
+		return fmt.Errorf("failed to sync directory %q: %w", dir, err)
+	}
+	if err := d.Close(); err != nil {
+		return fmt.Errorf("failed to close directory %q: %w", dir, err)
+	}
+
 	return nil
 }
 
@@ -243,7 +340,11 @@ func (s *Storage) objectPath(key string) (string, error) {
 	}
 
 	path := filepath.Join(s.root, filepath.FromSlash(cleanKey))
-	if path != s.root && !strings.HasPrefix(path, s.root+string(os.PathSeparator)) {
+	// Use a relative-path check rather than a string prefix: the prefix form
+	// (s.root + separator) is "//" when the root resolves to "/", which no joined
+	// path ever has, so it would reject every key under a root of "/".
+	rel, err := filepath.Rel(s.root, path)
+	if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
 		return "", fmt.Errorf("storage key %q escapes storage root", cleanKey)
 	}
 

lib/backup/storage/fs/fs_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"io"
+	"os"
 	"path/filepath"
 	"sync"
 	"testing"
@@ -74,6 +75,52 @@ func TestGetNotFound(t *testing.T) {
 	require.True(t, errors.Is(err, storage.ErrKeyNotFound))
 }
 
+func TestGetDirectoryKeyNotFound(t *testing.T) {
+	ctx := t.Context()
+	s := newTestStorage(t, t.TempDir())
+
+	// Store an object so that its parent ("data") exists as a directory, then
+	// request that directory as a key: it must read as missing, not EISDIR.
+	require.NoError(t, s.Put(ctx, storage.ArchiveKey("dir", "rs1"),
+		bytes.NewReader([]byte("archive")), int64(len("archive"))))
+
+	_, err := s.Get(ctx, "data")
+	require.True(t, errors.Is(err, storage.ErrKeyNotFound))
+}
+
+func TestPutRejectsNegativeSize(t *testing.T) {
+	s := newTestStorage(t, t.TempDir())
+
+	err := s.Put(t.Context(), storage.ManifestKey("neg"), bytes.NewReader([]byte("x")), -1)
+	require.True(t, errors.Is(err, errNegativeSize))
+}
+
+func TestPutRejectsSizeMismatch(t *testing.T) {
+	ctx := t.Context()
+	s := newTestStorage(t, t.TempDir())
+
+	key := storage.ManifestKey("mismatch")
+	err := s.Put(ctx, key, bytes.NewReader([]byte("four")), 100)
+	require.Error(t, err)
+
+	// The failed Put must not leave a partial object behind.
+	_, err = s.Get(ctx, key)
+	require.True(t, errors.Is(err, storage.ErrKeyNotFound))
+}
+
+func TestPutStoresReadableMode(t *testing.T) {
+	ctx := t.Context()
+	root := t.TempDir()
+	s := newTestStorage(t, root)
+
+	key := storage.ManifestKey("mode")
+	require.NoError(t, s.Put(ctx, key, bytes.NewReader([]byte("x")), 1))
+
+	info, err := os.Stat(filepath.Join(root, "cluster", "production", key))
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o644), info.Mode().Perm())
+}
+
 func TestList(t *testing.T) {
 	ctx := t.Context()
 	s := newTestStorage(t, t.TempDir())

lib/backup/storage/s3/s3.go

@@ -1,11 +1,11 @@
 package s3
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"io"
-	"net/http"
 	"sort"
 	"strings"
 
@@ -120,7 +120,15 @@ func (s *Storage) Get(ctx context.Context, key string) (io.ReadCloser, error) {
 		return nil, fmt.Errorf("failed to get s3 object %q: %w", cleanKey, err)
 	}
 
-	if _, err := object.Stat(); err != nil {
+	// GetObject is lazy: the request is issued on the first read. Force it here
+	// with a one-byte probe so a missing object is detected up front, and use a
+	// read (GET) rather than object.Stat (HEAD): a GET 404 carries a body, so
+	// minio-go reports NoSuchKey vs NoSuchBucket accurately, whereas a bodyless
+	// HEAD 404 is synthesized as NoSuchKey and would hide a missing bucket. The
+	// probed byte is replayed to the caller so no data is lost.
+	var probe [1]byte
+	n, err := object.Read(probe[:])
+	if err != nil && err != io.EOF {
 		_ = object.Close()
 		if isKeyNotFound(err) {
 			return nil, storage.ErrKeyNotFound
@@ -129,7 +137,27 @@ func (s *Storage) Get(ctx context.Context, key string) (io.ReadCloser, error) {
 		return nil, fmt.Errorf("failed to get s3 object %q: %w", cleanKey, err)
 	}
 
-	return object, nil
+	return &objectReadCloser{
+		Reader: io.MultiReader(bytes.NewReader(probe[:n]), object),
+		closer: object,
+	}, nil
+}
+
+// objectReadCloser rejoins the probed first byte with the rest of the object
+// stream (via the embedded Reader) while delegating Close to the underlying
+// object. Read is promoted from the embedded Reader so that io.EOF is passed
+// through unwrapped.
+type objectReadCloser struct {
+	io.Reader
+	closer io.Closer
+}
+
+func (o *objectReadCloser) Close() error {
+	if err := o.closer.Close(); err != nil {
+		return fmt.Errorf("failed to close s3 object: %w", err)
+	}
+
+	return nil
 }
 
 // Put uploads the object; size must be non-negative and is passed through to minio-go.
@@ -196,7 +224,13 @@ func validateConfig(cfg Config) error {
 }
 
 // isKeyNotFound reports whether err means the object key does not exist.
+//
+// It matches only the NoSuchKey code, never a bare 404: a missing bucket also
+// returns 404 (as NoSuchBucket), and treating that as "key not found" would make
+// Get report "not found" and Delete report success against a misconfigured bucket.
+// Callers must pass errors from body-bearing requests (GET, DELETE), where minio-go
+// decodes the real code; a HEAD 404 has no body and is synthesized as NoSuchKey
+// regardless of the actual cause, so Get probes with a read rather than a Stat.
 func isKeyNotFound(err error) bool {
-	resp := minio.ToErrorResponse(err)
-	return resp.Code == "NoSuchKey" || resp.StatusCode == http.StatusNotFound
+	return minio.ToErrorResponse(err).Code == "NoSuchKey"
 }

lib/backup/storage/s3/s3_integration_test.go

@@ -1,4 +1,4 @@
-//go:build integration
+//go:build integration_docker
 
 package s3
 
@@ -82,6 +82,27 @@ func TestGarageDelete(t *testing.T) {
 	require.True(t, errors.Is(err, storage.ErrKeyNotFound))
 }
 
+func TestGarageGetMissingBucketIsNotKeyNotFound(t *testing.T) {
+	ctx := t.Context()
+	testcontainers.SkipIfProviderIsNotHealthy(t)
+
+	garage := startGarage(ctx, t)
+	st, err := New(Config{
+		Endpoint:        garage.endpoint,
+		Bucket:          "no-such-bucket",
+		Region:          garage.region,
+		AccessKeyID:     garage.accessKey,
+		SecretAccessKey: garage.secretKey,
+	})
+	require.NoError(t, err)
+
+	// A missing bucket must surface as a real error, not be flattened into
+	// ErrKeyNotFound the way a bodyless HEAD 404 would be.
+	_, err = st.Get(ctx, storage.ManifestKey("whatever"))
+	require.Error(t, err)
+	require.False(t, errors.Is(err, storage.ErrKeyNotFound))
+}
+
 type garageInstance struct {
 	endpoint  string
 	bucket    string

lib/backup/storage/storage.go

@@ -11,10 +11,22 @@ import (
 )
 
 // Storage describes a backup object storage backend.
+//
+// Keys are canonical slash-separated paths (see CleanKey). Implementations must
+// behave uniformly: Get reports a missing object via ErrKeyNotFound, List reports
+// it via an empty (non-nil) result, Delete is idempotent (a missing key is not an
+// error), List results are sorted by key, and every method honors ctx cancellation.
 type Storage interface {
+	// List returns objects whose key starts with prefix, sorted by key.
 	List(ctx context.Context, prefix string) ([]ObjectInfo, error)
+	// Get opens the object for reading, returning ErrKeyNotFound if it is absent.
 	Get(ctx context.Context, key string) (io.ReadCloser, error)
+	// Put stores the bytes read from r under key. size must be the exact,
+	// non-negative number of bytes r yields; implementations reject a negative
+	// size or a byte-count mismatch. Streaming with an unknown length is not
+	// supported.
 	Put(ctx context.Context, key string, r io.Reader, size int64) error
+	// Delete removes the object; a missing object is not an error.
 	Delete(ctx context.Context, key string) error
 }