From 89862b9e3276473e2a22c2cadd5d3c11b67c9e29 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 18:57:04 +0000 Subject: [PATCH] Map s3 not-found to storage.ErrNotFound in Get and Stat (closes #129) S3Storer.Get and Stat returned the raw AWS SDK error for a missing object, so errors.Is(err, storage.ErrNotFound) was false on the s3 backend while the file and rclone backends honored the Storer contract. Callers that branch on ErrNotFound behaved differently per backend. Both now wrap ErrNotFound when the SDK reports a missing object, leaving every other error intact. The not-found detection is a small exported s3.IsNotFound helper, also used by HeadObject so the two share one definition. A test asserts a missing key maps to ErrNotFound on the s3 backend; it fails without the mapping. model: claude-opus-4-8 --- TODO.md | 9 ++++++ internal/s3/client.go | 18 +++++++---- internal/storage/s3.go | 17 ++++++++++- internal/storage/s3_test.go | 59 +++++++++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 internal/storage/s3_test.go diff --git a/TODO.md b/TODO.md index b3011b5..01faf4f 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ release" is exactly the contradiction # Completed Steps +- 2026-09-21: Made the s3 storage backend report a missing object as + `storage.ErrNotFound`, like the `file` and `rclone` backends and as the + `Storer` interface documents. `S3Storer.Get` and `Stat` returned the raw + AWS SDK error, so `errors.Is(err, storage.ErrNotFound)` was false on s3 + and callers branched differently per backend. Added a small `s3.IsNotFound` + helper (reused by `HeadObject`) and a test that a missing key maps to + `ErrNotFound` + ([issue #129](https://git.eeqj.de/sneak/vaultik/issues/129)). + - 2026-09-21: Fixed `verify --deep` reporting healthy snapshots as corrupt. Its final blob-integrity check hashed the encrypted downloaded bytes with a single SHA256 and compared that to the blob diff --git a/internal/s3/client.go b/internal/s3/client.go index f043ba3..b54d3f4 100644 --- a/internal/s3/client.go +++ b/internal/s3/client.go @@ -219,11 +219,7 @@ func (c *Client) HeadObject(ctx context.Context, key string) (bool, error) { Key: aws.String(fullKey), }) if err != nil { - var ( - notFound *s3types.NotFound - noSuchKey *s3types.NoSuchKey - ) - if errors.As(err, ¬Found) || errors.As(err, &noSuchKey) { + if IsNotFound(err) { return false, nil } @@ -233,6 +229,18 @@ func (c *Client) HeadObject(ctx context.Context, key string) (bool, error) { return true, nil } +// IsNotFound reports whether err indicates that an object does not exist. +// Head and Get requests surface a missing object as different SDK types, +// so both are checked here. +func IsNotFound(err error) bool { + var ( + notFound *s3types.NotFound + noSuchKey *s3types.NoSuchKey + ) + + return errors.As(err, ¬Found) || errors.As(err, &noSuchKey) +} + // ObjectInfo contains information about an S3 object. // It is used by ListObjectsStream to return object metadata // along with any errors encountered during listing. diff --git a/internal/storage/s3.go b/internal/storage/s3.go index 1f4f7c5..648e697 100644 --- a/internal/storage/s3.go +++ b/internal/storage/s3.go @@ -38,14 +38,29 @@ func (s *S3Storer) PutWithProgress( } // Get retrieves data from the specified key. +// Returns ErrNotFound if the object does not exist. func (s *S3Storer) Get(ctx context.Context, key string) (io.ReadCloser, error) { - return s.client.GetObject(ctx, key) + rc, err := s.client.GetObject(ctx, key) + if err != nil { + if s3.IsNotFound(err) { + return nil, fmt.Errorf("get %q: %w", key, ErrNotFound) + } + + return nil, err + } + + return rc, nil } // Stat returns metadata about an object without retrieving its contents. +// Returns ErrNotFound if the object does not exist. func (s *S3Storer) Stat(ctx context.Context, key string) (*ObjectInfo, error) { info, err := s.client.StatObject(ctx, key) if err != nil { + if s3.IsNotFound(err) { + return nil, fmt.Errorf("stat %q: %w", key, ErrNotFound) + } + return nil, err } diff --git a/internal/storage/s3_test.go b/internal/storage/s3_test.go new file mode 100644 index 0000000..3554a43 --- /dev/null +++ b/internal/storage/s3_test.go @@ -0,0 +1,59 @@ +package storage_test + +import ( + "context" + "errors" + "net/http/httptest" + "testing" + + "github.com/johannesboyne/gofakes3" + "github.com/johannesboyne/gofakes3/backend/s3mem" + + "sneak.berlin/go/vaultik/internal/s3" + "sneak.berlin/go/vaultik/internal/storage" +) + +// TestS3StorerMissingKeyMapsToErrNotFound verifies that the s3 backend reports +// a missing object as storage.ErrNotFound, matching the file and rclone +// backends and the Storer contract. Without the mapping, Get and Stat leak the +// raw SDK error and errors.Is(err, storage.ErrNotFound) is false. +// +//nolint:paralleltest // shares an in-process S3 server via t.Cleanup +func TestS3StorerMissingKeyMapsToErrNotFound(t *testing.T) { + const bucket = "test-bucket" + + backend := s3mem.New() + + err := backend.CreateBucket(bucket) + if err != nil { + t.Fatalf("create bucket: %v", err) + } + + srv := httptest.NewServer(gofakes3.New(backend).Server()) + t.Cleanup(srv.Close) + + ctx := context.Background() + + client, err := s3.NewClient(ctx, s3.Config{ + Endpoint: srv.URL, + Bucket: bucket, + AccessKeyID: "test", + SecretAccessKey: "test", + Region: "us-east-1", + }) + if err != nil { + t.Fatalf("new client: %v", err) + } + + storer := storage.NewS3Storer(client) + + _, err = storer.Get(ctx, "does-not-exist") + if !errors.Is(err, storage.ErrNotFound) { + t.Errorf("Get on missing key: got %v, want ErrNotFound", err) + } + + _, err = storer.Stat(ctx, "does-not-exist") + if !errors.Is(err, storage.ErrNotFound) { + t.Errorf("Stat on missing key: got %v, want ErrNotFound", err) + } +}