s3: split bucket delete into two-phase dispatch (Codex P1 #695)

Codex P1 on PR #695 caught that the original single-OperationGroup
shape would be rejected by the production coordinator on every
bucket delete. The fix lands here together with a test-coordinator
tightening that catches this class of bug in unit tests.

Background:
  kv/sharded_coordinator.go:dispatchDelPrefixBroadcast rejects any
  OperationGroup that mixes DEL_PREFIX with other ops AND any
  OperationGroup that uses DEL_PREFIX inside a transaction. The
  earlier shape ([Del BucketMetaKey, DelPrefix×6] with IsTxn=true)
  hit both rejection rules, so production would have failed every
  bucket delete with ErrInvalidRequest. The local test coordinator
  bypassed those checks, so the regression test passed despite
  the production-breaking shape.

Fix is two-part:

1. **Split AdminDeleteBucket and s3.go:deleteBucket into two
   Dispatch calls**:

   Phase 1 — `Del BucketMetaKey` in a txn (OCC-protected against
             a concurrent AdminCreateBucket racing the delete).
             Lives inside retryS3Mutation so an OCC conflict
             retries the whole closure.
   Phase 2 — DEL_PREFIX broadcast over every per-bucket key
             family in a non-txn OperationGroup. Lives outside
             retryS3Mutation because Phase 1 is the point of no
             return: a Phase-2 retry would 404 at loadBucketMetaAt.

   Phase-2 failure is logged via slog.WarnContext and not
   propagated. The bucket meta is already gone from the operator's
   POV; orphan keys (if any) are no worse than the pre-fix state
   on main and can be recovered by a future sweep tool. Surfacing
   a 500 to the operator after a successful delete would be a
   worse UX.

   Phase-1-first ordering is deliberate: a Phase-2-first ordering
   could leave the bucket meta extant while per-bucket data was
   wiped if Phase 1 then failed (concurrent recreate). Phase-1-
   first localises any partial failure to "bucket gone, orphan
   data may persist", which has a clean audit trail.

2. **Tighten localAdapterCoordinator validation** to mirror the
   production coordinator's dispatch-time rejection rules:
   reject IsTxn=true with any DelPrefix, reject mixed
   Del+DelPrefix groups. Without this, a future regression that
   ships the rejected shape would silently pass tests while
   breaking production. The existing
   TestS3Server_AdminDeleteBucket_SweepsOrphansAcrossAllPerBucketPrefixes
   test now exercises the production-realistic dispatch path and
   would have caught Codex P1 directly.

Refactor: split the single `bucketDeleteOperationGroupElems`
helper into `bucketDeleteSafetyNetElems` (DEL_PREFIX-only, used
by Phase 2) and a new `runBucketDeleteSafetyNet` method that
shares the dispatch-and-log logic between Admin and SigV4
paths. The Phase-1 Del op shape is small enough to inline at
each call site.

Design doc §6.2 rewritten with the two-phase rationale, the
Codex P1 finding, the Phase-2 failure semantics, and why Phase-1-
first ordering is correct.

Tests:
  go test -count=1 -run TestS3Server_AdminDeleteBucket ./adapter/ — passes
  go test -race -count=1 -run TestS3 ./adapter/ — passes (1.455s)
  golangci-lint run ./adapter/... — 0 issues

Author: bootjp

adapter/dynamodb_migration_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/bootjp/elastickv/kv"
 	"github.com/bootjp/elastickv/store"
+	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
 
@@ -27,6 +28,9 @@ func (c *localAdapterCoordinator) Dispatch(ctx context.Context, req *kv.Operatio
 	if req == nil {
 		return &kv.CoordinateResponse{}, nil
 	}
+	if err := c.validateDispatchShape(req); err != nil {
+		return nil, err
+	}
 	commitTS, err := c.commitTSForRequest(req)
 	if err != nil {
 		return nil, err
@@ -37,6 +41,45 @@ func (c *localAdapterCoordinator) Dispatch(ctx context.Context, req *kv.Operatio
 	return &kv.CoordinateResponse{}, nil
 }
 
+// validateDispatchShape mirrors the production coordinator's
+// dispatch-time rejection rules so tests catch the same class of
+// bug that real clusters would reject. Specifically:
+//
+//   - DEL_PREFIX cannot be in a transactional OperationGroup
+//     (kv/sharded_coordinator.go: dispatchDelPrefixBroadcast
+//     refuses IsTxn=true).
+//   - DEL_PREFIX cannot be mixed with Put or Del in the same
+//     OperationGroup (validateDelPrefixOnly enforces all-or-none).
+//
+// Without these checks, a regression that ships
+// `IsTxn:true with [Del, DelPrefix...]` (Codex P1 on PR #695)
+// would silently pass the local coordinator while production
+// rejected every bucket delete with ErrInvalidRequest.
+func (c *localAdapterCoordinator) validateDispatchShape(req *kv.OperationGroup[kv.OP]) error {
+	hasDelPrefix := false
+	hasOther := false
+	for _, elem := range req.Elems {
+		if elem == nil {
+			continue
+		}
+		if elem.Op == kv.DelPrefix {
+			hasDelPrefix = true
+		} else {
+			hasOther = true
+		}
+	}
+	if !hasDelPrefix {
+		return nil
+	}
+	if req.IsTxn {
+		return errors.Wrap(kv.ErrInvalidRequest, "DEL_PREFIX not supported in transactions")
+	}
+	if hasOther {
+		return errors.Wrap(kv.ErrInvalidRequest, "DEL_PREFIX cannot be mixed with other operations")
+	}
+	return nil
+}
+
 func (c *localAdapterCoordinator) commitTSForRequest(req *kv.OperationGroup[kv.OP]) (uint64, error) {
 	if req == nil {
 		return 0, nil

adapter/s3.go

@@ -636,6 +636,7 @@ func (s *S3Server) headBucket(w http.ResponseWriter, r *http.Request, bucket str
 }
 
 func (s *S3Server) deleteBucket(w http.ResponseWriter, r *http.Request, bucket string) {
+	var deletedGeneration uint64
 	err := s.retryS3Mutation(r.Context(), func() error {
 		readTS := s.readTS()
 		startTS := s.txnStartTS(readTS)
@@ -669,24 +670,32 @@ func (s *S3Server) deleteBucket(w http.ResponseWriter, r *http.Request, bucket s
 			}
 		}
 
-		// Same DEL_PREFIX safety net as AdminDeleteBucket — see
-		// design doc 2026_04_28_proposed_admin_delete_bucket_safety_net.md
-		// for the race analysis. The empty-probe above is racy
-		// against concurrent PutObject; the DEL_PREFIX ops in the
-		// shared OperationGroup tombstone every per-bucket prefix
-		// at the commit timestamp so anything that snuck in after
-		// readTS is swept along with BucketMetaKey.
+		// Phase 1: Del BucketMetaKey in a txn (OCC-protected
+		// against concurrent createBucket racing this delete).
+		// Phase 2 (DEL_PREFIX safety net) runs outside the txn
+		// because the production coordinator rejects DEL_PREFIX
+		// inside transactions and rejects mixed Del+DelPrefix
+		// groups (kv/sharded_coordinator.go: dispatchDelPrefixBroadcast).
+		// See AdminDeleteBucket's doc comment for the full
+		// rationale.
 		_, err = s.coordinator.Dispatch(r.Context(), &kv.OperationGroup[kv.OP]{
 			IsTxn:   true,
 			StartTS: startTS,
-			Elems:   bucketDeleteOperationGroupElems(bucket, meta.Generation),
+			Elems:   []*kv.Elem[kv.OP]{{Op: kv.Del, Key: s3keys.BucketMetaKey(bucket)}},
 		})
-		return errors.WithStack(err)
+		if err != nil {
+			return errors.WithStack(err)
+		}
+		deletedGeneration = meta.Generation
+		return nil
 	})
 	if err != nil {
 		writeS3MutationError(w, err, bucket, "")
 		return
 	}
+	// Phase 2: best-effort DEL_PREFIX safety net. See
+	// AdminDeleteBucket / runBucketDeleteSafetyNet for the contract.
+	s.runBucketDeleteSafetyNet(r.Context(), bucket, deletedGeneration)
 	w.WriteHeader(http.StatusNoContent)
 }
 

adapter/s3_admin.go

@@ -3,6 +3,7 @@ package adapter
 import (
 	"bytes"
 	"context"
+	"log/slog"
 	"sort"
 	"strings"
 
@@ -359,34 +360,46 @@ func (s *S3Server) AdminPutBucketAcl(ctx context.Context, principal AdminPrincip
 // bucket-must-be-empty rule mirrors the SigV4 deleteBucket path —
 // the dashboard cannot force a recursive delete, by design.
 //
-// The empty-probe (ScanAt with limit=1 on the manifest prefix) is
-// the operator-facing UX: a non-empty bucket returns 409 with
-// ErrAdminBucketNotEmpty. The probe is racy on its own — a
-// PutObject that commits between readTS and the delete's commitTS
-// is not visible to the probe and would have left orphan keys
-// under a deleted bucket meta. See design doc
-// 2026_04_28_proposed_admin_delete_bucket_safety_net.md for the
-// race analysis. The DEL_PREFIX ops appended to the dispatch
-// below close that window: the same OperationGroup wipes every
-// per-bucket prefix at the shared commitTS, so anything that
-// snuck in during the race is tombstoned together with the
-// bucket meta.
+// The dispatch happens in two phases because the production
+// coordinator (kv/sharded_coordinator.go: dispatchDelPrefixBroadcast)
+// rejects DEL_PREFIX inside a transaction and rejects DEL_PREFIX
+// mixed with Del or Put in the same OperationGroup:
+//
+//	Phase 1: Del BucketMetaKey in a txn (OCC-protected against
+//	         a concurrent AdminCreateBucket landing between our
+//	         readTS and commitTS).
+//	Phase 2: DEL_PREFIX over every per-bucket key family in a
+//	         non-txn broadcast — the safety net that sweeps
+//	         orphans left by any PutObject that committed
+//	         chunks/manifest between the empty-probe and the
+//	         Phase-1 commit. See design doc
+//	         2026_04_28_proposed_admin_delete_bucket_safety_net.md
+//	         §6.2 for the original single-OperationGroup design
+//	         and the dispatch-shape rejection that forced the
+//	         two-phase split.
+//
+// Phase 2 is best-effort: a Phase-2 failure leaves the bucket meta
+// already deleted (Phase 1 succeeded) but per-bucket prefixes
+// possibly still containing orphans. That state is no worse than
+// the pre-fix behaviour on main and recovers on operator-driven
+// re-cleanup. We log a warning rather than propagate the error so
+// the operator-visible delete reports success — the bucket really
+// is gone from the API surface, and a retry would 404 because
+// loadBucketMetaAt no longer finds the meta.
 //
 // BucketGenerationKey is intentionally NOT deleted. Re-creating
-// the bucket bumps the generation; orphan blobs that escaped
-// this delete (e.g. on an older generation from a previous
-// delete-recreate cycle) stay isolated under the old generation
-// prefix and never surface in the new bucket. Removing the
-// generation key would lose this property — pinned by
-// TestS3Server_AdminDeleteBucket_BucketGenerationKeySurvives.
+// the bucket bumps the generation; orphan blobs that escaped this
+// delete (e.g. on an older generation) stay isolated under the
+// old generation prefix and never surface in the new bucket.
+// Pinned by TestS3Server_AdminDeleteBucket_BucketGenerationKeySurvives.
 //
-// The contract change for clients: a PutObject that returned
-// 200 OK during the race window can have its data swept by the
-// concurrent delete. Operators are advised to pause writes
-// before AdminDeleteBucket; the alternative (orphan objects
-// that no API can enumerate or remove) is strictly worse.
+// The contract change for clients: a PutObject that returned 200
+// OK during the race window can have its data swept by the
+// concurrent delete. Operators are advised to pause writes before
+// AdminDeleteBucket; the alternative (orphan objects that no API
+// can enumerate or remove) is strictly worse.
 //
-// The same DEL_PREFIX shape is mirrored on the SigV4 path
+// The same shape is mirrored on the SigV4 path
 // (adapter/s3.go:deleteBucket) so both delete entrypoints share
 // the same race-window guarantees.
 func (s *S3Server) AdminDeleteBucket(ctx context.Context, principal AdminPrincipal, name string) error {
@@ -397,6 +410,7 @@ func (s *S3Server) AdminDeleteBucket(ctx context.Context, principal AdminPrincip
 		return ErrAdminNotLeader
 	}
 
+	var deletedGeneration uint64
 	err := s.retryS3Mutation(ctx, func() error {
 		readTS := s.readTS()
 		startTS := s.txnStartTS(readTS)
@@ -418,39 +432,49 @@ func (s *S3Server) AdminDeleteBucket(ctx context.Context, principal AdminPrincip
 		if len(kvs) > 0 {
 			return ErrAdminBucketNotEmpty
 		}
+		// Phase 1: Del BucketMetaKey in a txn so a concurrent
+		// AdminCreateBucket racing the delete is rejected by OCC.
+		// retryS3Mutation handles ErrWriteConflict / ErrTxnLocked
+		// by re-running this whole closure.
 		_, err = s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
 			IsTxn:   true,
 			StartTS: startTS,
-			Elems:   bucketDeleteOperationGroupElems(name, meta.Generation),
+			Elems:   []*kv.Elem[kv.OP]{{Op: kv.Del, Key: s3keys.BucketMetaKey(name)}},
 		})
-		return errors.WithStack(err)
+		if err != nil {
+			return errors.WithStack(err)
+		}
+		deletedGeneration = meta.Generation
+		return nil
 	})
 	if err != nil {
 		return err //nolint:wrapcheck // sentinel errors propagate as-is.
 	}
+	// Phase 2: best-effort safety-net DEL_PREFIX. Outside the
+	// retryS3Mutation closure because retrying after Phase 1
+	// committed would 404 at loadBucketMetaAt; we want the error
+	// (if any) logged but not propagated to the operator.
+	s.runBucketDeleteSafetyNet(ctx, name, deletedGeneration)
 	return nil
 }
 
-// bucketDeleteOperationGroupElems returns the OperationGroup elem
-// list for a bucket delete: a Del on BucketMetaKey followed by
-// DEL_PREFIX over each per-bucket key family. Shared between
-// AdminDeleteBucket and the SigV4 deleteBucket path so both
-// entrypoints sweep the same set of orphan-prone prefixes; if a
-// future per-bucket key family is added, updating this one helper
-// covers both delete paths in lockstep.
+// bucketDeleteSafetyNetElems returns the DEL_PREFIX elem list for
+// the Phase-2 safety-net dispatch shared between AdminDeleteBucket
+// and the SigV4 deleteBucket path. One helper so a future
+// per-bucket key family added to the data plane covers both delete
+// entrypoints in lockstep.
 //
 // BucketGenerationKey is intentionally not in the list — see the
 // AdminDeleteBucket doc comment for the orphan-isolation rationale.
 //
 // The 6 DEL_PREFIX ops broadcast across every shard
 // (kv/sharded_coordinator.go: DEL_PREFIX cannot be routed to a
-// single shard). This is acceptable because (a) the empty-probe
-// already confirmed the manifest prefix is empty in the common
-// case, so per-shard scans return 0 keys, (b) AdminDeleteBucket /
-// SigV4 deleteBucket are operator-frequency, not data-plane.
-func bucketDeleteOperationGroupElems(bucket string, generation uint64) []*kv.Elem[kv.OP] {
+// single shard). Acceptable because (a) the empty-probe already
+// confirmed the manifest prefix is empty in the common case, so
+// per-shard scans return 0 keys, (b) bucket delete is operator-
+// frequency, not data-plane.
+func bucketDeleteSafetyNetElems(bucket string, generation uint64) []*kv.Elem[kv.OP] {
 	return []*kv.Elem[kv.OP]{
-		{Op: kv.Del, Key: s3keys.BucketMetaKey(bucket)},
 		{Op: kv.DelPrefix, Key: s3keys.ObjectManifestPrefixForBucket(bucket, generation)},
 		{Op: kv.DelPrefix, Key: s3keys.UploadMetaPrefixForBucket(bucket, generation)},
 		{Op: kv.DelPrefix, Key: s3keys.UploadPartPrefixForBucket(bucket, generation)},
@@ -460,6 +484,24 @@ func bucketDeleteOperationGroupElems(bucket string, generation uint64) []*kv.Ele
 	}
 }
 
+// runBucketDeleteSafetyNet runs the Phase-2 DEL_PREFIX dispatch
+// and swallows transport / cluster errors after logging — the
+// caller has already deleted the bucket meta and the operator-
+// visible state is consistent with that. Shared between admin and
+// SigV4 paths.
+func (s *S3Server) runBucketDeleteSafetyNet(ctx context.Context, bucket string, generation uint64) {
+	if _, err := s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
+		Elems: bucketDeleteSafetyNetElems(bucket, generation),
+	}); err != nil {
+		slog.WarnContext(ctx,
+			"bucket delete safety-net DEL_PREFIX failed; bucket meta is gone but orphan sweep incomplete",
+			slog.String("bucket", bucket),
+			slog.Uint64("generation", generation),
+			slog.String("error", err.Error()),
+		)
+	}
+}
+
 // adminCanonicalACL normalises an empty input to the canned
 // "private" default. The SigV4 createBucket / putBucketAcl paths
 // apply the same default after trimming the x-amz-acl header.

docs/design/2026_04_28_proposed_admin_delete_bucket_safety_net.md

@@ -170,21 +170,75 @@ identical to the existing `ObjectManifestPrefixForBucket`.
 
 ### 6.2 `AdminDeleteBucket` (and `s3.go:deleteBucket`) commit shape
 
+The original revision of this design proposed a **single
+OperationGroup** carrying both the `Del BucketMetaKey` and the
+six `DelPrefix` ops, relying on the FSM to apply them all at one
+commitTS. That shape is rejected by the production coordinator
+(Codex P1 on PR #695):
+
+- `kv/sharded_coordinator.go:dispatchDelPrefixBroadcast` rejects
+  any `OperationGroup` containing `DelPrefix` when `IsTxn` is
+  true: *"DEL_PREFIX not supported in transactions"*.
+- The same broadcast path runs `validateDelPrefixOnly` and rejects
+  mixed `Del` / `Put` + `DelPrefix` groups: *"DEL_PREFIX cannot be
+  mixed with other operations"*.
+
+Together those two rules forbid the single-group shape. The
+implementation splits into two `Dispatch` calls:
+
 ```go
-elems := []*kv.Elem[kv.OP]{
-    {Op: kv.Del,       Key: BucketMetaKey(name)},
-    {Op: kv.DelPrefix, Key: ObjectManifestPrefixForBucket(name, gen)},
-    {Op: kv.DelPrefix, Key: UploadMetaPrefixForBucket(name, gen)},
-    {Op: kv.DelPrefix, Key: UploadPartPrefixForBucket(name, gen)},
-    {Op: kv.DelPrefix, Key: BlobPrefixForBucket(name, gen)},
-    {Op: kv.DelPrefix, Key: GCUploadPrefixForBucket(name, gen)},
-    {Op: kv.DelPrefix, Key: RoutePrefixForBucket(name, gen)},
-}
+// Phase 1: Del BucketMetaKey in a txn (OCC-protected against
+// concurrent AdminCreateBucket racing this delete).
+s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
+    IsTxn:   true,
+    StartTS: startTS,
+    Elems:   []*kv.Elem[kv.OP]{{Op: kv.Del, Key: BucketMetaKey(name)}},
+})
+
+// Phase 2: DEL_PREFIX safety net broadcast (non-txn). Only runs
+// after Phase 1 commits; failure here is best-effort and logged
+// rather than propagated.
+s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
+    Elems: []*kv.Elem[kv.OP]{
+        {Op: kv.DelPrefix, Key: ObjectManifestPrefixForBucket(name, gen)},
+        {Op: kv.DelPrefix, Key: UploadMetaPrefixForBucket(name, gen)},
+        {Op: kv.DelPrefix, Key: UploadPartPrefixForBucket(name, gen)},
+        {Op: kv.DelPrefix, Key: BlobPrefixForBucket(name, gen)},
+        {Op: kv.DelPrefix, Key: GCUploadPrefixForBucket(name, gen)},
+        {Op: kv.DelPrefix, Key: RoutePrefixForBucket(name, gen)},
+    },
+})
 ```
 
 The empty-probe stays as the operator-facing UX: when the bucket
 is genuinely non-empty (no race), the operator still sees 409 and
-the `DEL_PREFIX` ops never run.
+neither phase runs.
+
+**Phase-2 failure semantics**: Phase 1 is the point of no return.
+If Phase 2's `Dispatch` returns an error (transport blip,
+cluster transient, etc.), the bucket meta is already gone but
+the per-bucket prefixes may still contain orphans. The operator-
+visible delete reports success (the bucket really is gone from
+the API surface, and a retry would 404 at `loadBucketMetaAt`) and
+the failure is recorded via `slog.WarnContext`. The resulting
+state is no worse than the pre-fix behaviour on main — orphans
+were already the failure mode the original race produced. A
+future cluster-wide sweep tool (when one exists) can recover the
+disk space.
+
+**Why not Phase 2 first?** A "DEL_PREFIX before Del" ordering
+would wipe per-bucket data while the bucket meta still exists. If
+Phase 1 then fails (concurrent recreate races the OCC), readers
+see "bucket exists" but their chunks/manifests don't — confusing
+state with no clean recovery. Phase-1-first localises any partial
+failure to "bucket gone, orphan data may persist", which has a
+well-defined audit trail in slog.
+
+**Test-coordinator parity**: `adapter/dynamodb_migration_test.go`
+mirrors the production rejection rules in `localAdapterCoordinator.
+validateDispatchShape`. Without that, the original single-group
+shape passed local tests while production rejected every bucket
+delete with `ErrInvalidRequest` — exactly the gap Codex P1 caught.
 
 ### 6.3 Apply-time cost