amadan

carlos / platform Public

objstore: teardown routes crashed grants, guards the final delete

Review fixes.

1. A grant block with no `provisioned` block is NOT "nothing to tear
down". Grant creates bucket, user and key and delivers the credentials
BEFORE its step-6 record Put, so a lost CAS there leaves exactly that
shape over a live, billing, credentialed estate — and the old
declared-only fast path deleted the only record naming it. Teardown now
routes on the GRANT block: coordinates fall back to Grant.Bucket and
UserName(acct, app) (the deterministic name EnsureUser was called with),
and only a record with no grant at all dies trivially.

2. The final record Delete was blind. A grant landing between the walk's
read and its delete has re-provisioned and recorded a live estate; the
delete would erase that record — the same §9 failure from the other end.
The delete now re-reads first, Rotate-shaped, and refuses unless the
record still names what this walk destroyed (same Provisioned.KeyID,
same Grant.Bucket). The refusal wraps release.ErrCASConflict so the
console door can say "state changed, re-run" rather than retrying a
delete that will never be right. Store.Delete has no CAS, so this
narrows the window rather than closing it; documented at the site.

Also wraps the two bare error returns with objstore: context.

Both fixes are mutation-checked: reverting the routing strands the
crashed grant's bucket and key, and defeating the guard erases the
competing grant's record.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 81805983ee1cb2aa2bf5e6db7c77f95f6f03086c parent e14ff8c
2 files changed, +241 −19
  • internal/objstore/walk.go +97 −19
  • internal/objstore/walk_test.go +144 −0
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index ad7b58929..e2ec5e4b6 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -377,9 +377,16 @@ func RotateFinish(ctx context.Context, s release.Store, p Provisioner, acct, app
// rather than sweeping the prefix over an estate that is still standing.
//
// An absent record is nil, not an error: "no store" is the state teardown is
-// trying to reach, and both callers race other operators to it. A record with
-// no Provisioned block — declared, or granted and never walked — has no
-// backend to touch, so it dies trivially.
+// trying to reach, and both callers race other operators to it.
+//
+// What decides the path is the GRANT block, not the provisioned one. A record
+// carrying a grant but no `provisioned` is not "nothing to tear down": Grant
+// creates the bucket, the user and a key, and delivers the credentials, all
+// BEFORE its step-6 Put — so a lost CAS (or any crash) at that last write
+// leaves exactly this shape over a live, billing, credentialed estate. Reading
+// it as trivially deletable would erase the only record naming that estate,
+// which is precisely the failure §9's ordering rule exists to prevent. Only a
+// record with NO grant at all — declared, never acted on — dies trivially.
func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app string) error {
rec, _, err := Get(ctx, s, acct, app)
if err != nil {
@@ -388,24 +395,50 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str
if rec == nil {
return nil
}
- if rec.Provisioned == nil {
- return s.Delete(ctx, Key(acct, app))
- }
- pr := rec.Provisioned
- // Empty before delete: a real backend refuses to delete a bucket with
- // objects in it, and EmptyBucket is paginated and resumable for exactly
- // the bucket too big to clear in one call.
- if err := p.EmptyBucket(ctx, pr.Bucket); err != nil {
- return fmt.Errorf("objstore: empty bucket %s: %w", pr.Bucket, err)
- }
- if err := p.DeleteBucket(ctx, pr.Bucket); err != nil {
- return fmt.Errorf("objstore: delete bucket %s: %w", pr.Bucket, err)
+ // What this walk believes it is destroying — the guard on the final delete.
+ startKeyID, startBucket := "", ""
+ if rec.Provisioned != nil {
+ startKeyID = rec.Provisioned.KeyID
+ }
+ if rec.Grant != nil {
+ startBucket = rec.Grant.Bucket
+ }
+ if rec.Grant == nil && rec.Provisioned == nil {
+ return deleteRecordIfUnchanged(ctx, s, acct, app, startKeyID, startBucket)
+ }
+ // Coordinates: the provisioned block when there is one, since the bucket
+ // an app's data is in is whatever was recorded at provision time. Falling
+ // back to the grant covers the crashed-grant shape above — and the user
+ // name there is not a guess: EnsureUser is called with exactly
+ // UserName(acct, app), so a principal created by a grant that never
+ // recorded itself is still findable by name.
+ bucket, user := "", UserName(acct, app)
+ switch {
+ case rec.Provisioned != nil:
+ bucket, user = rec.Provisioned.Bucket, rec.Provisioned.IAMUser
+ default:
+ bucket = rec.Grant.Bucket
+ }
+ // A grant block with no bucket cannot come from the walk (Grant resolves
+ // and validates a name before writing the block), so there is nothing
+ // nameable to destroy — but the deterministic user might exist, and the
+ // delivered vars might too, so only the bucket steps are skipped.
+ if bucket != "" {
+ // Empty before delete: a real backend refuses to delete a bucket with
+ // objects in it, and EmptyBucket is paginated and resumable for exactly
+ // the bucket too big to clear in one call.
+ if err := p.EmptyBucket(ctx, bucket); err != nil {
+ return fmt.Errorf("objstore: empty bucket %s: %w", bucket, err)
+ }
+ if err := p.DeleteBucket(ctx, bucket); err != nil {
+ return fmt.Errorf("objstore: delete bucket %s: %w", bucket, err)
+ }
}
// DeleteUser reaps the keys and the inline policy, so the credential dies
// with the estate it was scoped to — no orphan principal keeps a key that
// names a bucket that no longer exists.
- if err := p.DeleteUser(ctx, pr.IAMUser); err != nil {
- return fmt.Errorf("objstore: delete user %s: %w", pr.IAMUser, err)
+ if err := p.DeleteUser(ctx, user); err != nil {
+ return fmt.Errorf("objstore: delete user %s: %w", user, err)
}
// The delivered set goes even though the credentials behind it are already
// dead: leaving them would hand the app's next render a bucket name and a
@@ -433,7 +466,7 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str
// of the names are present, this step is already converged.
m, err := appconfig.Get(ctx, s, acct, app)
if err != nil {
- return err
+ return fmt.Errorf("objstore: read config for %s/%s: %w", acct, app, err)
}
present := false
for _, name := range unsets {
@@ -448,7 +481,52 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str
}
}
// Last (§9). Everything it named is gone.
- return s.Delete(ctx, Key(acct, app))
+ return deleteRecordIfUnchanged(ctx, s, acct, app, startKeyID, startBucket)
+}
+
+// deleteRecordIfUnchanged is the final step of Teardown: delete the record,
+// but only if it still describes the estate the walk just destroyed.
+//
+// Store.Delete has no compare-and-swap, so this is the same shape Rotate's
+// retry uses — re-read, and act only on a record that still names what we
+// started from. The race it closes is a Grant landing mid-teardown: the
+// console's grant door writes this same object, and a grant that completes
+// after our unset has minted a NEW bucket, user and key and recorded them. A
+// blind delete there erases the record of a live estate — the §9 failure,
+// arrived at from the other end.
+//
+// The refusal wraps release.ErrCASConflict so a caller can tell it apart from
+// a backend failure: the estate the walk was aimed at IS gone, and the right
+// response is to re-run against the record as it now stands, not to retry the
+// same delete. A record that has vanished under us is success, not conflict —
+// another teardown got there first, which is the state we wanted.
+//
+// This narrows the window; it does not close it. A grant landing between the
+// re-read and the Delete still loses its record, and only a CAS delete in the
+// Store interface could fix that. The console door serialises these verbs per
+// app, so the remaining window is a fraction of one round trip.
+func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, startKeyID, startBucket string) error {
+ fresh, _, err := Get(ctx, s, acct, app)
+ if err != nil {
+ return fmt.Errorf("objstore: re-read store record for %s/%s: %w", acct, app, err)
+ }
+ if fresh == nil {
+ return nil
+ }
+ keyID, bucket := "", ""
+ if fresh.Provisioned != nil {
+ keyID = fresh.Provisioned.KeyID
+ }
+ if fresh.Grant != nil {
+ bucket = fresh.Grant.Bucket
+ }
+ if keyID != startKeyID || bucket != startBucket {
+ return fmt.Errorf("objstore: store record for %s/%s changed during teardown — re-run: %w", acct, app, release.ErrCASConflict)
+ }
+ if err := s.Delete(ctx, Key(acct, app)); err != nil {
+ return fmt.Errorf("objstore: delete store record for %s/%s: %w", acct, app, err)
+ }
+ return nil
}
// writeEnv delivers the whole coordinate set in ONE appconfig.Apply — one CAS
diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go
index 24f91218d..95f194ca4 100644
--- a/internal/objstore/walk_test.go
+++ b/internal/objstore/walk_test.go
@@ -1148,3 +1148,147 @@ func TestTeardownConfigObjectAbsent(t *testing.T) {
t.Fatalf("teardown minted a config object: %v", err)
}
}
+
+// crashedGrant reproduces a grant that created and delivered everything and
+// then lost its step-6 record Put: bucket, user, key and delivered vars all
+// live, record carrying Declared+Grant and NO provisioned block.
+func crashedGrant(t *testing.T, s release.Store, p *MemProvisioner) (bucket, user string) {
+ t.Helper()
+ ctx := context.Background()
+ bucket, user = "carlos-store-tito-bab-titogo", UserName("bab", "titogo")
+ if _, err := p.EnsureBucket(ctx, bucket); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket); err != nil {
+ t.Fatal(err)
+ }
+ id, _, err := p.MintKey(ctx, user)
+ if err != nil {
+ t.Fatal(err)
+ }
+ // writeEnv had already landed when the record write lost — the credentials
+ // are in config.json under names no record now explains.
+ if _, err := appconfig.Apply(ctx, s, "bab", "titogo", map[string]appconfig.Var{
+ "TITO_S3_BUCKET": {Kind: "env", Value: bucket},
+ "TITO_S3_REGION": {Kind: "env", Value: "eu-west-1"},
+ "TITO_S3_ACCESS_KEY": {Kind: "env", Value: id},
+ }, nil); err != nil {
+ t.Fatal(err)
+ }
+ rec := &Record{
+ Declared: &Declared{EnvPrefix: "TITO_S3", At: time.Now().UTC(), By: "member@example.com"},
+ Grant: &GrantBlock{Backend: "aws", Bucket: bucket, At: time.Now().UTC(), By: "op"},
+ }
+ if _, err := Put(ctx, s, "bab", "titogo", rec, ""); err != nil {
+ t.Fatal(err)
+ }
+ return bucket, user
+}
+
+// A grant block with no provisioned block is NOT "nothing to tear down" — it
+// is the shape a crash at Grant's final record write leaves over a live
+// estate. Deleting the record alone would strand a billing bucket and a live
+// credential nothing names.
+func TestTeardownCrashedGrantSweep(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ bucket, _ := crashedGrant(t, s, p)
+ p.Buckets[bucket]["uploads/one.txt"] = []byte("payload")
+
+ if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {
+ t.Fatal(err)
+ }
+ if len(p.Buckets) != 0 || len(p.Users) != 0 {
+ t.Fatalf("crashed grant left standing: buckets %v users %v", p.Buckets, p.Users)
+ }
+ for name := range mustVars(t, s, "bab", "titogo") {
+ if strings.HasPrefix(name, "TITO_S3_") {
+ t.Fatalf("delivered var %q survived teardown", name)
+ }
+ }
+ stored, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || stored != nil {
+ t.Fatalf("record after teardown = %+v, %v", stored, err)
+ }
+}
+
+func TestTeardownCrashedGrantResumable(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ bucket, _ := crashedGrant(t, s, p)
+ p.Buckets[bucket]["uploads/one.txt"] = []byte("payload")
+
+ p.FailNext["DeleteBucket"] = errors.New("injected: DeleteBucket failed")
+ if err := Teardown(ctx, s, p, "bab", "titogo"); err == nil {
+ t.Fatal("DeleteBucket failure: Teardown returned no error")
+ }
+ half, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || half == nil || half.Grant == nil || half.Grant.Bucket != bucket {
+ t.Fatalf("record after DeleteBucket failure = %+v, %v", half, err)
+ }
+
+ if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {
+ t.Fatalf("resume after DeleteBucket failure: %v", err)
+ }
+ if len(p.Buckets) != 0 || len(p.Users) != 0 {
+ t.Fatalf("estate after resume: buckets %v users %v", p.Buckets, p.Users)
+ }
+ stored, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || stored != nil {
+ t.Fatalf("record after resume = %+v, %v", stored, err)
+ }
+}
+
+// raceOnReadStore runs race() the FIRST time onGet is read, before the read
+// returns — a competing writer landing at a chosen point in a walk.
+type raceOnReadStore struct {
+ release.Store
+ onGet string
+ race func(release.Store)
+}
+
+func (r *raceOnReadStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if r.onGet != "" && key == r.onGet {
+ r.onGet = ""
+ r.race(r.Store)
+ }
+ return r.Store.Get(ctx, key)
+}
+
+// A grant that lands mid-teardown has re-provisioned a live estate and
+// recorded it. The final delete must not erase that record just because this
+// walk read an older one.
+func TestTeardownRefusesConcurrentRegrant(t *testing.T) {
+ ctx := context.Background()
+ inner := release.NewDirStore(t.TempDir())
+ _, pub := testSealKeypair(t)
+ p := NewMemProvisioner("eu-west-1")
+ s := &raceOnReadStore{Store: inner}
+ teardownFixture(t, s, pub, p)
+
+ // Fire on the config probe — the last read Teardown does before its
+ // re-read guard, so the competing record is in place by the time the
+ // guard looks.
+ s.onGet = appconfig.Key("bab", "titogo")
+ s.race = func(in release.Store) {
+ other, etag, err := Get(ctx, in, "bab", "titogo")
+ if err != nil {
+ t.Error(err)
+ return
+ }
+ other.Provisioned.KeyID = "MEMKEY99" // a fresh grant landed and recorded
+ if _, err := Put(ctx, in, "bab", "titogo", other, etag); err != nil {
+ t.Error(err)
+ }
+ }
+
+ if err := Teardown(ctx, s, p, "bab", "titogo"); !errors.Is(err, release.ErrCASConflict) {
+ t.Fatalf("teardown over a concurrent grant: %v, want ErrCASConflict", err)
+ }
+ stored, _, err := Get(ctx, inner, "bab", "titogo")
+ if err != nil || stored == nil || stored.Provisioned == nil || stored.Provisioned.KeyID != "MEMKEY99" {
+ t.Fatalf("the competing grant's record was erased: %+v, %v", stored, err)
+ }
+}