From 14b4fee2149d9883f7e53df6c018693bcf926562 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 5 Sep 2026 12:22:44 +0000 Subject: [PATCH] Add idempotent object deletion --- docs/internal/storage.md | 3 ++ docs/roadmap/implementation.md | 2 +- internal/adapters/storage/fake.go | 21 ++++++++ internal/adapters/storage/fake_test.go | 37 ++++++++++++- internal/adapters/storage/object_store.go | 1 + internal/adapters/storage/s3_backend.go | 20 +++++++ internal/adapters/storage/s3_backend_test.go | 55 +++++++++++++++++--- internal/app/post_publish_cleanup_test.go | 4 ++ internal/stage/analyze_test.go | 5 ++ internal/stage/prepare_previous_test.go | 4 ++ 10 files changed, 143 insertions(+), 9 deletions(-) diff --git a/docs/internal/storage.md b/docs/internal/storage.md index dfc86f5..f3a771d 100644 --- a/docs/internal/storage.md +++ b/docs/internal/storage.md @@ -16,6 +16,7 @@ operator-selected storage fields and credential mechanisms belong in - `Download(ctx, key, localPath)` - `Upload(ctx, localPath, key, opts)` - `UploadConditional(ctx, source, key, opts, condition)` +- `Delete(ctx, key)` removes one object and succeeds when it is already absent - `Exists(ctx, key)` Key invariant: @@ -47,6 +48,8 @@ not own discovery, defaults, or configuration validation. - `Read` binds a returned body to its S3 ETag. `UploadConditional` maps an ETag match or absence precondition directly to the provider request and reports a failed precondition without performing a local check-then-write replacement. +- `Delete` maps one normalized key to the provider delete-object operation and + treats an absent object as success. - `Exists` maps not-found responses to `false`. ## Invariants diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index ad12598..33c6bc2 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -84,7 +84,7 @@ message. Update documentation alongside observable changes. ## Stage 1 — Add Idempotent Object Deletion -**Status:** Pending +**Status:** Completed ### Goal diff --git a/internal/adapters/storage/fake.go b/internal/adapters/storage/fake.go index b767380..5a4c39b 100644 --- a/internal/adapters/storage/fake.go +++ b/internal/adapters/storage/fake.go @@ -23,11 +23,13 @@ type FakeBackend struct { Uploads []FakeUploadCall Downloads []FakeDownloadCall Reads []FakeReadCall + Deletes []string ExistsKeys []string ListErr error DownloadErr error UploadErr error + DeleteErr error ExistsErr error UploadHook func(FakeUploadCall) error DownloadHook func(FakeDownloadCall) error @@ -316,6 +318,25 @@ func fakeObjectETag(data []byte) string { return hex.EncodeToString(sum[:]) } +// Delete removes one object. Removing an absent object succeeds. +func (f *FakeBackend) Delete(ctx context.Context, key string) error { + if err := ctx.Err(); err != nil { + return err + } + if f.DeleteErr != nil { + return f.DeleteErr + } + normalizedKey := normalizeObjectKey(key) + if normalizedKey == "" { + return fmt.Errorf("delete object: key is required") + } + f.mu.Lock() + defer f.mu.Unlock() + f.Deletes = append(f.Deletes, normalizedKey) + delete(f.Objects, normalizedKey) + return nil +} + // Exists checks object presence. func (f *FakeBackend) Exists(ctx context.Context, key string) (bool, error) { if err := ctx.Err(); err != nil { diff --git a/internal/adapters/storage/fake_test.go b/internal/adapters/storage/fake_test.go index ed637aa..25394f2 100644 --- a/internal/adapters/storage/fake_test.go +++ b/internal/adapters/storage/fake_test.go @@ -71,6 +71,38 @@ func TestFakeBackendUploadAndExists(t *testing.T) { } } +func TestFakeBackendDeleteIsIdempotent(t *testing.T) { + fake := &FakeBackend{} + fake.SeedObject(FakeObject{Key: "locks/source.lock", Data: []byte("lock")}) + + for range 2 { + if err := fake.Delete(context.Background(), `locks\source.lock`); err != nil { + t.Fatalf("Delete() error = %v", err) + } + } + if _, ok := fake.Objects["locks/source.lock"]; ok { + t.Fatal("Delete() left the object present") + } + if len(fake.Deletes) != 2 || fake.Deletes[0] != "locks/source.lock" || fake.Deletes[1] != "locks/source.lock" { + t.Fatalf("Delete() calls = %#v, want two normalized keys", fake.Deletes) + } +} + +func TestFakeBackendDeleteHonorsContextAndInjectedError(t *testing.T) { + canceled, cancel := context.WithCancel(context.Background()) + cancel() + fake := &FakeBackend{} + if err := fake.Delete(canceled, "locks/source.lock"); !errors.Is(err, context.Canceled) { + t.Fatalf("Delete() canceled error = %v, want context.Canceled", err) + } + + wantErr := errors.New("delete fail") + fake.DeleteErr = wantErr + if err := fake.Delete(context.Background(), "locks/source.lock"); !errors.Is(err, wantErr) { + t.Fatalf("Delete() injected error = %v, want %v", err, wantErr) + } +} + func TestFakeBackendConditionalUploadRejectsStaleGeneration(t *testing.T) { fake := &FakeBackend{} fake.SeedObject(FakeObject{Key: "locks.yml", Data: []byte("old")}) @@ -87,7 +119,7 @@ func TestFakeBackendConditionalUploadRejectsStaleGeneration(t *testing.T) { } func TestFakeBackendObjectErrors(t *testing.T) { - fake := &FakeBackend{DownloadErr: errors.New("download fail"), UploadErr: errors.New("upload fail"), ListErr: errors.New("list fail"), ExistsErr: errors.New("exists fail")} + fake := &FakeBackend{DownloadErr: errors.New("download fail"), UploadErr: errors.New("upload fail"), ListErr: errors.New("list fail"), DeleteErr: errors.New("delete fail"), ExistsErr: errors.New("exists fail")} if _, err := fake.List(context.Background(), "x"); err == nil || !strings.Contains(err.Error(), "list fail") { t.Fatalf("List() error = %v, want list fail", err) @@ -100,6 +132,9 @@ func TestFakeBackendObjectErrors(t *testing.T) { if _, err := fake.Upload(context.Background(), local, "x", UploadOptions{}); err == nil || !strings.Contains(err.Error(), "upload fail") { t.Fatalf("Upload() error = %v, want upload fail", err) } + if err := fake.Delete(context.Background(), "x"); err == nil || !strings.Contains(err.Error(), "delete fail") { + t.Fatalf("Delete() error = %v, want delete fail", err) + } if _, err := fake.Exists(context.Background(), "x"); err == nil || !strings.Contains(err.Error(), "exists fail") { t.Fatalf("Exists() error = %v, want exists fail", err) } diff --git a/internal/adapters/storage/object_store.go b/internal/adapters/storage/object_store.go index d8f9863..f56aff6 100644 --- a/internal/adapters/storage/object_store.go +++ b/internal/adapters/storage/object_store.go @@ -28,6 +28,7 @@ type ObjectStore interface { Download(ctx context.Context, key, localPath string) error Upload(ctx context.Context, localPath, key string, opts UploadOptions) (ObjectInfo, error) UploadConditional(ctx context.Context, source io.Reader, key string, opts UploadOptions, condition WriteCondition) (ObjectInfo, error) + Delete(ctx context.Context, key string) error Exists(ctx context.Context, key string) (bool, error) } diff --git a/internal/adapters/storage/s3_backend.go b/internal/adapters/storage/s3_backend.go index f7bc64f..5a102d9 100644 --- a/internal/adapters/storage/s3_backend.go +++ b/internal/adapters/storage/s3_backend.go @@ -23,6 +23,7 @@ type s3API interface { ListObjectsV2(ctx context.Context, params *s3.ListObjectsV2Input, optFns ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) GetObject(ctx context.Context, params *s3.GetObjectInput, optFns ...func(*s3.Options)) (*s3.GetObjectOutput, error) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) + DeleteObject(ctx context.Context, params *s3.DeleteObjectInput, optFns ...func(*s3.Options)) (*s3.DeleteObjectOutput, error) HeadObject(ctx context.Context, params *s3.HeadObjectInput, optFns ...func(*s3.Options)) (*s3.HeadObjectOutput, error) } @@ -300,6 +301,25 @@ func (b *S3Backend) uploadReader(ctx context.Context, source io.Reader, key stri return info, nil } +// Delete removes one object. S3 deletion is idempotent when the key is absent. +func (b *S3Backend) Delete(ctx context.Context, key string) error { + if err := ctx.Err(); err != nil { + return err + } + normalizedKey := normalizeObjectKey(key) + if normalizedKey == "" { + return fmt.Errorf("delete object: key is required") + } + _, err := b.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &b.bucket, + Key: &normalizedKey, + }) + if err != nil { + return fmt.Errorf("delete object %q: %w", normalizedKey, err) + } + return nil +} + // Exists checks whether one object key exists. func (b *S3Backend) Exists(ctx context.Context, key string) (bool, error) { normalizedKey := normalizeObjectKey(key) diff --git a/internal/adapters/storage/s3_backend_test.go b/internal/adapters/storage/s3_backend_test.go index 0fe7397..c748f4b 100644 --- a/internal/adapters/storage/s3_backend_test.go +++ b/internal/adapters/storage/s3_backend_test.go @@ -29,16 +29,18 @@ type fakeS3API struct { getETag *string getLastModified *time.Time - putOut *s3.PutObjectOutput - putErr error + putOut *s3.PutObjectOutput + putErr error + deleteErr error headErr error - lastList *s3.ListObjectsV2Input - lastLists []*s3.ListObjectsV2Input - lastGet *s3.GetObjectInput - lastPut *s3.PutObjectInput - lastHead *s3.HeadObjectInput + lastList *s3.ListObjectsV2Input + lastLists []*s3.ListObjectsV2Input + lastGet *s3.GetObjectInput + lastPut *s3.PutObjectInput + lastDelete *s3.DeleteObjectInput + lastHead *s3.HeadObjectInput } func (f *fakeS3API) ListObjectsV2(_ context.Context, params *s3.ListObjectsV2Input, _ ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { @@ -116,6 +118,14 @@ func (f *fakeS3API) PutObject(_ context.Context, params *s3.PutObjectInput, _ .. return f.putOut, nil } +func (f *fakeS3API) DeleteObject(_ context.Context, params *s3.DeleteObjectInput, _ ...func(*s3.Options)) (*s3.DeleteObjectOutput, error) { + f.lastDelete = params + if f.deleteErr != nil { + return nil, f.deleteErr + } + return &s3.DeleteObjectOutput{}, nil +} + func (f *fakeS3API) HeadObject(_ context.Context, params *s3.HeadObjectInput, _ ...func(*s3.Options)) (*s3.HeadObjectOutput, error) { f.lastHead = params if f.headErr != nil { @@ -275,6 +285,37 @@ func TestS3BackendExistsNotFound(t *testing.T) { } } +func TestS3BackendDeleteMapsRequestAndErrors(t *testing.T) { + client := &fakeS3API{} + backend := &S3Backend{bucket: "bucket-1", client: client} + if err := backend.Delete(context.Background(), `locks\source.lock`); err != nil { + t.Fatalf("Delete() error = %v", err) + } + if client.lastDelete == nil || *client.lastDelete.Bucket != "bucket-1" || *client.lastDelete.Key != "locks/source.lock" { + t.Fatalf("DeleteObject input = %#v, want normalized bucket and key", client.lastDelete) + } + + wantErr := errors.New("permission denied") + client.deleteErr = wantErr + err := backend.Delete(context.Background(), "locks/source.lock") + if !errors.Is(err, wantErr) || !strings.Contains(err.Error(), "locks/source.lock") { + t.Fatalf("Delete() error = %v, want contextual backend error", err) + } +} + +func TestS3BackendDeleteHonorsCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + client := &fakeS3API{} + err := (&S3Backend{bucket: "bucket-1", client: client}).Delete(ctx, "locks/source.lock") + if !errors.Is(err, context.Canceled) { + t.Fatalf("Delete() error = %v, want context.Canceled", err) + } + if client.lastDelete != nil { + t.Fatal("Delete() called S3 after cancellation") + } +} + func TestNewS3BackendFromConfigUsesClientOptions(t *testing.T) { original := newS3Client t.Cleanup(func() { newS3Client = original }) diff --git a/internal/app/post_publish_cleanup_test.go b/internal/app/post_publish_cleanup_test.go index c8f3390..92cb5ba 100644 --- a/internal/app/post_publish_cleanup_test.go +++ b/internal/app/post_publish_cleanup_test.go @@ -610,6 +610,10 @@ func (s *failKeyStore) UploadConditional(ctx context.Context, source io.Reader, return s.delegate.UploadConditional(ctx, source, key, opts, condition) } +func (s *failKeyStore) Delete(ctx context.Context, key string) error { + return s.delegate.Delete(ctx, key) +} + func (s *failKeyStore) Exists(ctx context.Context, key string) (bool, error) { return s.delegate.Exists(ctx, key) } diff --git a/internal/stage/analyze_test.go b/internal/stage/analyze_test.go index 9185a52..b5aac95 100644 --- a/internal/stage/analyze_test.go +++ b/internal/stage/analyze_test.go @@ -1853,6 +1853,11 @@ func (s *analyzeObjectStoreTracker) UploadConditional(context.Context, io.Reader return storage.ObjectInfo{}, errors.New("unexpected conditional object store upload call") } +func (s *analyzeObjectStoreTracker) Delete(context.Context, string) error { + s.called = true + return errors.New("unexpected object store delete call") +} + func (s *analyzeObjectStoreTracker) Exists(context.Context, string) (bool, error) { s.called = true return false, errors.New("unexpected object store exists call") diff --git a/internal/stage/prepare_previous_test.go b/internal/stage/prepare_previous_test.go index 90eb657..94c3342 100644 --- a/internal/stage/prepare_previous_test.go +++ b/internal/stage/prepare_previous_test.go @@ -425,6 +425,10 @@ func (s *preparePreviousCaptureStore) UploadConditional(ctx context.Context, sou return s.delegate.UploadConditional(ctx, source, key, opts, condition) } +func (s *preparePreviousCaptureStore) Delete(ctx context.Context, key string) error { + return s.delegate.Delete(ctx, key) +} + func (s *preparePreviousCaptureStore) Exists(ctx context.Context, key string) (bool, error) { s.existsKeys = append(s.existsKeys, key) return s.delegate.Exists(ctx, key)