amadan

carlos / platform Public

Merge branch 'feat-release-conditional-delete' into main

claude 8746297d5d9cb6dd9cdebe6ebb25285bb7696d0e parent d43d971parent 35cc65d
2 files changed, +290 −0
  • internal/release/conditional_delete.go +103 −0
  • internal/release/conditional_delete_test.go +187 −0

CI passed — run details

diff --git a/internal/release/conditional_delete.go b/internal/release/conditional_delete.go
new file mode 100644
index 000000000..f3233809a
--- /dev/null
+++ b/internal/release/conditional_delete.go
@@ -0,0 +1,103 @@
+package release
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "os"
+ "strings"
+
+ "github.com/aws/aws-sdk-go-v2/aws"
+ awss3 "github.com/aws/aws-sdk-go-v2/service/s3"
+)
+
+var (
+ ErrConditionalDeleteUnsupported = errors.New("store: conditional delete unsupported")
+ ErrInvalidETag = errors.New("store: conditional delete requires one exact etag")
+)
+
+// ConditionalDeleter is optional. Callers must not replace a refused conditional
+// delete with an ordinary Delete: that would discard the observed revision.
+type ConditionalDeleter interface {
+ DeleteIfMatch(ctx context.Context, key, etag string) error
+}
+
+// DeleteIfMatch deletes only the observed revision, or refuses if the backend
+// cannot enforce the condition. Known absence is success, not proof that an object
+// previously existed. It never falls back to an unconditional delete.
+func DeleteIfMatch(ctx context.Context, s Store, key, etag string) error {
+ if !exactDeleteETag(etag) {
+ return ErrInvalidETag
+ }
+ conditional, ok := s.(ConditionalDeleter)
+ if !ok {
+ return ErrConditionalDeleteUnsupported
+ }
+ return conditional.DeleteIfMatch(ctx, key, etag)
+}
+
+func exactDeleteETag(etag string) bool {
+ if len(etag) == 0 || strings.HasPrefix(etag, "W/") {
+ return false
+ }
+ quoted := etag[0] == '"'
+ if quoted {
+ if len(etag) < 3 || etag[len(etag)-1] != '"' {
+ return false
+ }
+ etag = etag[1 : len(etag)-1]
+ }
+ for _, c := range []byte(etag) {
+ if c <= 32 || c == 127 || c == '"' || c == '*' || (!quoted && c == ',') {
+ return false
+ }
+ }
+ return true
+}
+
+func (p prefixStore) DeleteIfMatch(ctx context.Context, key, etag string) error {
+ return DeleteIfMatch(ctx, p.Store, p.full(key), etag)
+}
+
+func (d *DirStore) DeleteIfMatch(_ context.Context, key, etag string) error {
+ if !exactDeleteETag(etag) {
+ return ErrInvalidETag
+ }
+ path, err := d.path(key)
+ if err != nil {
+ return err
+ }
+ d.mu.Lock()
+ defer d.mu.Unlock()
+ body, err := os.ReadFile(path)
+ if errors.Is(err, os.ErrNotExist) {
+ return nil
+ }
+ if err != nil {
+ return err
+ }
+ if etagOf(body) != etag {
+ return fmt.Errorf("dirstore: %s: %w", key, ErrCASConflict)
+ }
+ return os.Remove(path)
+}
+
+func (s *S3Store) DeleteIfMatch(ctx context.Context, key, etag string) error {
+ if !exactDeleteETag(etag) {
+ return ErrInvalidETag
+ }
+ _, err := s.Client.DeleteObject(ctx, &awss3.DeleteObjectInput{Bucket: &s.Bucket, Key: &key, IfMatch: aws.String(etag)})
+ if s3NotFound(err) {
+ return nil
+ }
+ if s3CASConflict(err) {
+ return fmt.Errorf("s3store: %s: %w", key, ErrCASConflict)
+ }
+ return err
+}
+
+var (
+ _ ConditionalDeleter = (*DirStore)(nil)
+ _ ConditionalDeleter = (*S3Store)(nil)
+ _ ConditionalDeleter = prefixStore{}
+)
diff --git a/internal/release/conditional_delete_test.go b/internal/release/conditional_delete_test.go
new file mode 100644
index 000000000..b6405dfb2
--- /dev/null
+++ b/internal/release/conditional_delete_test.go
@@ -0,0 +1,187 @@
+package release
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "os"
+ "sync"
+ "testing"
+
+ "github.com/aws/aws-sdk-go-v2/aws"
+ "github.com/aws/aws-sdk-go-v2/credentials"
+ awss3 "github.com/aws/aws-sdk-go-v2/service/s3"
+)
+
+type deleteTrap struct {
+ Store
+ called bool
+}
+
+func (s *deleteTrap) Delete(context.Context, string) error { s.called = true; return nil }
+func TestConditionalDeleteUnsupportedAndInvalidTags(t *testing.T) {
+ ctx := context.Background()
+ trap := &deleteTrap{}
+ for _, s := range []Store{trap, NewPrefixStore(trap, "scope"), NewPrefixStore(NewPrefixStore(trap, "outer"), "inner")} {
+ if e := DeleteIfMatch(ctx, s, "key", "etag"); !errors.Is(e, ErrConditionalDeleteUnsupported) {
+ t.Fatal(e)
+ }
+ }
+ if trap.called {
+ t.Fatal("unconditional fallback")
+ }
+ d := NewDirStore(t.TempDir())
+ etag, e := d.Put(ctx, "key", []byte("keep"), "")
+ if e != nil {
+ t.Fatal(e)
+ }
+ for _, bad := range []string{"", " ", "*", `"*"`, `""`, `W/"etag"`, `"one", "two"`, "one,two", "etag\r\nX-Test: bad", " etag", "etag ", "\tetag", `"broken`, `broken"`} {
+ for _, fn := range []func() error{func() error { return DeleteIfMatch(ctx, d, "key", bad) }, func() error { return d.DeleteIfMatch(ctx, "key", bad) }, func() error { return (&S3Store{}).DeleteIfMatch(ctx, "key", bad) }} {
+ if e := fn(); !errors.Is(e, ErrInvalidETag) {
+ t.Fatalf("invalid tag accepted: %q: %v", bad, e)
+ }
+ }
+ }
+ b, got, e := d.Get(ctx, "key")
+ if e != nil || string(b) != "keep" || got != etag {
+ t.Fatal("invalid tag changed object")
+ }
+}
+func TestConditionalDeleteDirRevisionAndPrefix(t *testing.T) {
+ ctx := context.Background()
+ d := NewDirStore(t.TempDir())
+ scoped := NewPrefixStore(NewPrefixStore(d, "outer"), "inner")
+ old, e := scoped.Put(ctx, "key", []byte("old"), "")
+ if e != nil {
+ t.Fatal(e)
+ }
+ current, e := scoped.Put(ctx, "key", []byte("new"), old)
+ if e != nil {
+ t.Fatal(e)
+ }
+ if _, e = d.Put(ctx, "key", []byte("outside"), ""); e != nil {
+ t.Fatal(e)
+ }
+ if e = DeleteIfMatch(ctx, scoped, "key", old); !errors.Is(e, ErrCASConflict) {
+ t.Fatal(e)
+ }
+ b, _, e := d.Get(ctx, "outer/inner/key")
+ if e != nil || string(b) != "new" {
+ t.Fatal("stale tag deleted current object")
+ }
+ if e = DeleteIfMatch(ctx, scoped, "key", current); e != nil {
+ t.Fatal(e)
+ }
+ if _, _, e = d.Get(ctx, "outer/inner/key"); !errors.Is(e, os.ErrNotExist) {
+ t.Fatal(e)
+ }
+ if b, _, e = d.Get(ctx, "key"); e != nil || string(b) != "outside" {
+ t.Fatal("scope escaped")
+ }
+ if e = DeleteIfMatch(ctx, scoped, "key", current); e != nil {
+ t.Fatal("absence not idempotent", e)
+ }
+ if e = DeleteIfMatch(ctx, d, "../escape", current); e == nil {
+ t.Fatal("traversal accepted")
+ }
+}
+func TestConditionalDeleteDirRacesPut(t *testing.T) {
+ ctx := context.Background()
+ d := NewDirStore(t.TempDir())
+ for i := 0; i < 100; i++ {
+ key := fmt.Sprint(i)
+ old, e := d.Put(ctx, key, []byte("old"), "")
+ if e != nil {
+ t.Fatal(e)
+ }
+ start := make(chan struct{})
+ results := make(chan error, 2)
+ go func() { <-start; _, e := d.Put(ctx, key, []byte("new"), old); results <- e }()
+ go func() { <-start; results <- d.DeleteIfMatch(ctx, key, old) }()
+ close(start)
+ a, b := <-results, <-results
+ if (a == nil) == (b == nil) {
+ t.Fatal("both operations won or lost", a, b)
+ }
+ if a != nil && !errors.Is(a, ErrCASConflict) || b != nil && !errors.Is(b, ErrCASConflict) {
+ t.Fatal(a, b)
+ }
+ body, _, e := d.Get(ctx, key)
+ if e != nil && !errors.Is(e, os.ErrNotExist) {
+ t.Fatal(e)
+ }
+ if e == nil && string(body) != "new" {
+ t.Fatal("unexpected surviving revision")
+ }
+ }
+}
+
+func TestConditionalDeleteS3ActualWire(t *testing.T) {
+ for _, absentStatus := range []int{204, 404} {
+ t.Run(fmt.Sprint(absentStatus), func(t *testing.T) {
+ var mu sync.Mutex
+ objects := map[string]string{"/bucket/scope/key": "current", "/bucket/key": "outside"}
+ calls := 0
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ mu.Lock()
+ defer mu.Unlock()
+ calls++
+ if r.Method != "DELETE" {
+ t.Error("unexpected pre-read or method", r.Method)
+ w.WriteHeader(405)
+ return
+ }
+ if len(r.Header.Values("If-Match")) != 1 || r.Header.Get("If-Match") == "" || r.Header.Get("If-None-Match") != "" {
+ t.Error("missing exact condition")
+ w.WriteHeader(400)
+ return
+ }
+ etag, exists := objects[r.URL.Path]
+ if !exists {
+ if absentStatus == 404 {
+ w.Header().Set("Content-Type", "application/xml")
+ w.WriteHeader(404)
+ fmt.Fprint(w, `<Error><Code>NoSuchKey</Code></Error>`)
+ } else {
+ w.WriteHeader(204)
+ }
+ return
+ }
+ if r.Header.Get("If-Match") != `"`+etag+`"` {
+ w.Header().Set("Content-Type", "application/xml")
+ w.WriteHeader(412)
+ fmt.Fprint(w, `<Error><Code>PreconditionFailed</Code></Error>`)
+ return
+ }
+ delete(objects, r.URL.Path)
+ w.WriteHeader(204)
+ }))
+ defer srv.Close()
+ s := &S3Store{Bucket: "bucket", Client: awss3.New(awss3.Options{Region: "us-east-1", Credentials: credentials.NewStaticCredentialsProvider("fixture", "fixture", ""), BaseEndpoint: aws.String(srv.URL), UsePathStyle: true, RetryMaxAttempts: 1})}
+ ctx := context.Background()
+ scoped := NewPrefixStore(s, "scope")
+ if e := DeleteIfMatch(ctx, scoped, "key", `"stale"`); !errors.Is(e, ErrCASConflict) {
+ t.Fatal(e)
+ }
+ mu.Lock()
+ retained := objects["/bucket/scope/key"]
+ mu.Unlock()
+ if retained != "current" {
+ t.Fatal("wrong tag deleted object")
+ }
+ if e := DeleteIfMatch(ctx, scoped, "key", `"current"`); e != nil {
+ t.Fatal(e)
+ }
+ if e := DeleteIfMatch(ctx, scoped, "key", `"current"`); e != nil {
+ t.Fatal("absence not accepted", e)
+ }
+ mu.Lock()
+ defer mu.Unlock()
+ if _, ok := objects["/bucket/scope/key"]; ok || objects["/bucket/key"] != "outside" || calls != 3 {
+ t.Fatal("conditional scope or request count", objects, calls)
+ }
+ })
+ }
+}