carlos / platform Public

objstore: every other grant clears a seed's mark, and RevertIn deletes by etag (Plan 5 Task 4 re-review)

The mark was cleared only on a first grant. A seed that wrote its grant
block and then aborted left a marked store that an ordinary create
sharing the environment, or an operator grant or provision re-run, could
adopt with the mark kept; the seed's finish would then tear down a store
another host uses. Every grant whose seed differs from the mark now
clears it in the walk's first write, re-runs included. An aborted seed's
bucket can leak that way, the safe direction. The same seed's re-run
keeps its mark.

RevertIn's delete is now release.DeleteIfMatch on the etag, so a
concurrent store create is never lost. A lost CAS on rollback is logged
and the record left alone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 941e6db4bdc28840d984b2e9e925d5e7f2a9733a parent 70ba167

6 files changed, +105 −59

internal/console/instanceprivatestore.go +6 −3
internal/console/instanceprivatestore_test.go +26 −0
internal/objstore/objstore.go +6 −20
internal/objstore/objstore_test.go +7 −5
internal/objstore/walk.go +18 −10
internal/objstore/walk_seed_test.go +42 −21

CI passed — run details

diff --git a/internal/console/instanceprivatestore.go b/internal/console/instanceprivatestore.go

index 6400fb6fe..4f6475c33 100644

--- a/internal/console/instanceprivatestore.go

+++ b/internal/console/instanceprivatestore.go

@@ -316,12 +316,15 @@ func (s *Server) instanceStoreProvisionFor(ctx context.Context, w http.ResponseW

// another path could grant that record later and keep the mark, and

// the seed's own finish would then tear down a store it never made

// (spec §4 Storage). Still under the lock, the record goes back to

- // what it was. A grant that did write its block has moved the record

- // past declaredTag, and that estate is this seed's to finish.

+ // what it was, conditionally on declaredTag. A grant that did write

+ // its block has moved the record past declaredTag, and that estate

+ // is this seed's to finish.

rbCtx, rbCancel := context.WithTimeout(context.WithoutCancel(ctx), storeTimeout)

rerr := objstore.RevertIn(rbCtx, effectStore, acct.Sqid, app, env, prevRaw, declaredTag)

rbCancel()

- if rerr != nil && !errors.Is(rerr, release.ErrCASConflict) {

+ if errors.Is(rerr, release.ErrCASConflict) {

+ log.Printf("console: instance store %s/%s/%s: seed %s declaration not rolled back: the record was written since (its grant block or another writer), left as it is", acct.Sqid, app, env, seedID)

+ } else if rerr != nil {

log.Printf("console: instance store %s/%s/%s: roll back seed %s declaration: %v", acct.Sqid, app, env, seedID, rerr)

}

diff --git a/internal/console/instanceprivatestore_test.go b/internal/console/instanceprivatestore_test.go

index 9737f7308..854a04466 100644

--- a/internal/console/instanceprivatestore_test.go

+++ b/internal/console/instanceprivatestore_test.go

@@ -244,6 +244,32 @@ func TestInstanceStoreProvisionForKeepsAGrantItWrote(t *testing.T) {

}

+// A seed that wrote its grant block and then aborted leaves a store marked

+// with its id. An ordinary create sharing that environment adopts the

+// store, and the mark goes in the same write, so the seed's finish cannot

+// tear down a store another host now uses.

+func TestInstanceStoreProvisionAdoptingASeedsStoreClearsTheMark(t *testing.T) {

+ fx, fake, acct := seededStoreFixture(t, "code-seed-adopted")

+ fake.FailNext = map[string]error{"EnsureUser": errors.New("iam is down")}

+ w := httptest.NewRecorder()

+ if _, ok, _ := fx.srv.instanceStoreProvisionFor(context.Background(), w, acct, "shop", "canary", "alice@keymail.dev", seedA); ok {

+ t.Fatal("the seeded provision succeeded, want its walk to fail after the grant block")

+ }

+ if rec := canaryStore(t, fx); rec == nil || rec.Grant == nil || rec.Declared.Seed != seedA {

+ t.Fatalf("canary record = %+v, want the seed's grant block and mark", rec)

+ }

+ w = httptest.NewRecorder()

+ out, ok := fx.srv.instanceStoreProvision(context.Background(), w, acct, "shop", "canary", "bob@keymail.dev")

+ if !ok {

+ t.Fatalf("ordinary provision refused: %d %s", w.Code, w.Body.String())

+ }

+ for _, rec := range []*objstore.Record{out, canaryStore(t, fx)} {

+ if rec.Declared == nil || rec.Declared.Seed != "" || rec.Provisioned == nil {

+ t.Fatalf("store after adoption = %+v / %+v, want provisioned with no seed mark", rec, rec.Declared)

+ }

// Without a seed nothing is rolled back: a failed grant leaves the

// declaration standing, as it always has, for a retry to resume.

func TestInstanceStoreProvisionLeavesItsDeclarationOnAFailedGrant(t *testing.T) {

diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go

index ad6fec2d8..b77c4ee27 100644

--- a/internal/objstore/objstore.go

+++ b/internal/objstore/objstore.go

@@ -322,14 +322,10 @@ func PutIn(ctx context.Context, s release.Store, acct, app, env string, rec *Rec

// RevertIn puts one store record back the way it was before a write the

// caller made, which left the record at etag: prev's exact bytes, or no

-// record at all when prev is nil. A record no longer at etag has been

-// written since, by a grant or by another writer, and that write is not

-// the caller's to undo: RevertIn leaves it and answers

-// release.ErrCASConflict. A record already gone is left gone.

-//

-// The store has no conditional delete, so the etag is read just before the

-// delete. Callers hold the store's lock (the console's storeWalks), which

-// every grant takes too.

+// record at all when prev is nil. Both are conditional on etag: a record

+// written since, by a grant or by another writer (the declare door takes

+// no store lock), is not the caller's to undo, and RevertIn leaves it and

+// answers release.ErrCASConflict. A record already gone counts as deleted.

func RevertIn(ctx context.Context, s release.Store, acct, app, env string, prev []byte, etag string) error {

return release.WithAppMutation(ctx, s, acct, app, "objstore.RevertIn", func(claimContext context.Context) error {

return claimRawRevertIn(claimContext, s, acct, app, env, prev, etag)

@@ -341,20 +337,10 @@ func claimRawRevertIn(ctx context.Context, s release.Store, acct, app, env strin

return fmt.Errorf("objstore: invalid environment %q", env)

}

key := KeyIn(acct, app, env)

- _, cur, err := s.Get(ctx, key)

- if errors.Is(err, os.ErrNotExist) {

- return nil

- }

- if err != nil {

- return err

- }

- if cur != etag {

- return fmt.Errorf("objstore: %s changed since it was written: %w", key, release.ErrCASConflict)

- }

if prev == nil {

- return s.Delete(ctx, key)

+ return release.DeleteIfMatch(ctx, s, key, etag)

}

- _, err = s.Put(ctx, key, prev, etag)

+ _, err := s.Put(ctx, key, prev, etag)

return err

}

diff --git a/internal/objstore/objstore_test.go b/internal/objstore/objstore_test.go

index c48efcde9..66c020862 100644

--- a/internal/objstore/objstore_test.go

+++ b/internal/objstore/objstore_test.go

@@ -128,10 +128,12 @@ func TestRevertIn(t *testing.T) {

t.Fatal(err)

}

before, _, _ := s.Get(ctx, key)

- if err := RevertIn(ctx, s, "bab", "shop", "canary", nil, tag); !errors.Is(err, release.ErrCASConflict) {

- t.Fatalf("revert past another write = %v, want ErrCASConflict", err)

- }

- if after, _, _ := s.Get(ctx, key); !bytes.Equal(after, before) {

- t.Fatalf("record after a refused revert = %s, want %s", after, before)

+ for _, prev := range [][]byte{nil, prior} {

+ if err := RevertIn(ctx, s, "bab", "shop", "canary", prev, tag); !errors.Is(err, release.ErrCASConflict) {

+ t.Fatalf("revert (prev %q) past another write = %v, want ErrCASConflict", prev, err)

+ }

+ if after, _, _ := s.Get(ctx, key); !bytes.Equal(after, before) {

+ t.Fatalf("record after a refused revert = %s, want %s", after, before)

+ }

}

diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go

index a56051502..15e8619c5 100644

--- a/internal/objstore/walk.go

+++ b/internal/objstore/walk.go

@@ -200,12 +200,14 @@ func claimRawGrant(ctx context.Context, s release.Store, p Provisioner, d Deploy

// coordinate of WHICH estate, never a change to how the walk behaves.

// seed is the seed id this grant is for (GrantInFor), "" for every other

-// grant. The FIRST grant of a record whose declaration carries another

-// seed's mark (Declared.Seed) clears the mark in the same write as the

-// grant block: the store is then this grant's, not the seed's, and the

-// seed's finish must never tear it down (seeded-instances spec §4

-// Storage). A mark therefore survives only on a store its own seed

-// granted. A record with no mark is written exactly as before.

+// grant. Any grant of a record whose declaration carries another seed's

+// mark (Declared.Seed), a first grant or a re-run, clears the mark in the

+// walk's first write (with the grant block, on a first grant): the store

+// is then held by this grant, not the seed, and the seed's finish must

+// never tear it down (seeded-instances spec §4 Storage). An aborted

+// seed's bucket can leak that way, which is the safe direction. A mark

+// therefore survives only while its own seed is the only one granting.

+// A record with no mark is written exactly as before.

func claimRawGrantIn(ctx context.Context, s release.Store, p Provisioner, d Deployment,

sealPub *ecdh.PublicKey, acct, app, env, actor, source, adoptBucket, seed string, now time.Time) (*Record, error) {

// 1. resolve + write the grant block

@@ -256,11 +258,17 @@ func claimRawGrantIn(ctx context.Context, s release.Store, p Provisioner, d Depl

if err := claimStoreNames(ctx, s, acct, app, env, bucket, user, rec.Grant == nil, now); err != nil {

return nil, err

}

- if rec.Grant == nil {

- if rec.Declared.Seed != "" && rec.Declared.Seed != seed {

- rec.Declared.Seed = ""

+ // A grant for any seed but the one that marked the record clears the

+ // mark, first grant or re-run, in this same write: whoever grants the

+ // store now holds it, and the seed's finish must not tear it down.

+ clearMark := rec.Declared.Seed != "" && rec.Declared.Seed != seed

+ if clearMark {

+ rec.Declared.Seed = ""

+ }

+ if rec.Grant == nil || clearMark {

+ if rec.Grant == nil {

+ rec.Grant = &GrantBlock{Backend: d.Backend, Bucket: bucket, Source: source, At: now, By: actor}

}

- rec.Grant = &GrantBlock{Backend: d.Backend, Bucket: bucket, Source: source, At: now, By: actor}

if etag, err = PutIn(ctx, s, acct, app, env, rec, etag); err != nil {

return nil, err

}

diff --git a/internal/objstore/walk_seed_test.go b/internal/objstore/walk_seed_test.go

index 03b666432..38b03300e 100644

--- a/internal/objstore/walk_seed_test.go

+++ b/internal/objstore/walk_seed_test.go

@@ -53,27 +53,48 @@ func TestFirstGrantClearsAnotherSeedsMark(t *testing.T) {

}

-// A re-run over a store its seed already granted is a repair of that

-// seed's estate, not an adoption: the mark stays.

-func TestGrantRerunKeepsTheGrantingSeedsMark(t *testing.T) {

- const seedB = "sd_bbbbbbbbbbbbbbbbbbbbbbbbbb"

- ctx := context.Background()

- s := release.NewDirStore(t.TempDir())

- _, pub := testSealKeypair(t)

- p := NewMemProvisioner("eu-west-1")

- decl := &Declared{EnvPrefix: DefaultEnvPrefix, At: time.Now().UTC(), By: "member@example.com", Seed: seedB}

- if _, err := PutIn(ctx, s, "bab", "shop", "canary", &Record{Declared: decl}, ""); err != nil {

- t.Fatal(err)

- }

- if _, err := GrantInFor(ctx, s, p, testDeployment(), pub, "bab", "shop", "canary", "op", GrantSourceOperator, "", seedB, time.Now().UTC()); err != nil {

- t.Fatal(err)

- }

- rec, err := GrantIn(ctx, s, p, testDeployment(), pub, "bab", "shop", "canary", "op", GrantSourceOperator, "", time.Now().UTC())

- if err != nil {

- t.Fatal(err)

- }

- if rec.Declared.Seed != seedB {

- t.Fatalf("Declared.Seed after a re-run = %q, want %q", rec.Declared.Seed, seedB)

+// A re-run over a store its seed already granted keeps the mark only when

+// it is that seed's own re-run. An unseeded re-run (an operator grant, a

+// provision, an ordinary create sharing the environment) or another seed's

+// clears it, so the aborted seed's finish cannot tear down a store someone

+// else now holds; its bucket leaks instead, the safe direction.

+func TestGrantRerunClearsAMarkThatIsNotItsOwn(t *testing.T) {

+ const seedA, seedB = "sd_aaaaaaaaaaaaaaaaaaaaaaaaaa", "sd_bbbbbbbbbbbbbbbbbbbbbbbbbb"

+ for name, c := range map[string]struct{ rerunFor, want string }{

+ "the same seed's re-run": {seedB, seedB},

+ "an unseeded re-run": {"", ""},

+ "another seed's re-run": {seedA, ""},

+ } {

+ t.Run(name, func(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ decl := &Declared{EnvPrefix: DefaultEnvPrefix, At: time.Now().UTC(), By: "member@example.com", Seed: seedB}

+ if _, err := PutIn(ctx, s, "bab", "shop", "canary", &Record{Declared: decl}, ""); err != nil {

+ t.Fatal(err)

+ }

+ first, err := GrantInFor(ctx, s, p, testDeployment(), pub, "bab", "shop", "canary", "op", GrantSourceOperator, "", seedB, time.Now().UTC())

+ if err != nil {

+ t.Fatal(err)

+ }

+ rec, err := GrantInFor(ctx, s, p, testDeployment(), pub, "bab", "shop", "canary", "op", GrantSourceOperator, "", c.rerunFor, time.Now().UTC())

+ if err != nil {

+ t.Fatal(err)

+ }

+ stored, _, err := GetIn(ctx, s, "bab", "shop", "canary")

+ if err != nil {

+ t.Fatal(err)

+ }

+ for _, r := range []*Record{rec, stored} {

+ if r.Declared.Seed != c.want {

+ t.Fatalf("Declared.Seed after the re-run = %q, want %q", r.Declared.Seed, c.want)

+ }

+ if r.Grant == nil || *r.Grant != *first.Grant || r.Provisioned == nil || r.Provisioned.KeyID != first.Provisioned.KeyID {

+ t.Fatalf("re-run changed the estate: grant %+v provisioned %+v, want %+v %+v", r.Grant, r.Provisioned, first.Grant, first.Provisioned)

+ }

+ })

}