carlos / platform Public

console: a seed's failed grant rolls back its declaration (Plan 5 Task 4 review)

A seeded instanceStoreProvisionFor whose grant fails before writing its
block now puts the store record back while it still holds the store lock:
it deletes a record it created, or restores the exact prior bytes of one
it re-declared (objstore.RevertIn, CAS on the declaration's etag). A
declaration marked with the seed could otherwise outlive the seed, be
granted by another path with the mark kept, and be torn down by the
seed's finish (spec §4 Storage). A grant that wrote its block is this
seed's estate and is left for its finish. The unseeded path is unchanged.

Also from the review: a seeded grant whose declaration lost its mark to
the unlocked declare door is logged, and the redeclare comment names the
prefix it replaces.

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

claude 2 weeks ago 1434d9d73e1a953a214dcb213b4fa3b59f5c116b parent f20cbab

4 files changed, +256 −2

internal/console/instanceprivatestore.go +48 −2
internal/console/instanceprivatestore_test.go +105 −0
internal/objstore/objstore.go +38 −0
internal/objstore/objstore_test.go +65 −0

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

index 4a9331df6..5f4216cb3 100644

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

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

@@ -199,6 +199,9 @@ var errSeedStoreTaken = errors.New("console: the environment's store is not this

// A seed's declaration carries its id (objstore.Declared.Seed). A record

// that is declared and holds no estate is declared again for this seed.

+// When the grant then fails before writing its block, the declaration is

+// rolled back under the lock: deleted if this call created the record,

+// else restored to its previous bytes.

func (s *Server) instanceStoreProvisionFor(ctx context.Context, w http.ResponseWriter, acct *Account, app, env, actor, seedID string) (*objstore.Record, bool, error) {

if s.refuseTemporary(w, acct, app, "give it object storage") {

return nil, false, nil

@@ -256,6 +259,7 @@ func (s *Server) instanceStoreProvisionFor(ctx context.Context, w http.ResponseW

if seedID != "" && !seedOwnsStore(rec, seedID) {

return rec, false, errSeedStoreTaken

}

+ existed := rec != nil

if rec == nil {

rec = &objstore.Record{}

}

@@ -266,14 +270,35 @@ func (s *Server) instanceStoreProvisionFor(ctx context.Context, w http.ResponseW

// A seed is the exception: a declared-only record holds no estate, and

// one not marked with this seed is declared again, for this seed, so

- // the store this seed makes is marked as its own.

+ // the store this seed makes is marked as its own. That replaces the

+ // other writer's EnvPrefix, By and At: a member's chosen --env-prefix

+ // on a store with no estate yet is lost, and the app's inherited prefix

+ // is delivered instead.

redeclare := rec.Declared == nil

if seedID != "" && rec.Declared != nil && rec.Grant == nil && rec.Provisioned == nil && rec.Declared.Seed != seedID {

redeclare = true

}

+ // What a seed's failed grant rolls back to (below): the record's exact

+ // bytes before this call declared it, or nil when there was no record.

+ var prevRaw []byte

+ if seedID != "" && redeclare && existed {

+ raw, rawTag, err := effectStore.Get(walkCtx, objstore.KeyIn(acct.Sqid, app, env))

+ if err != nil {

+ log.Printf("console: instance store read %s/%s/%s: %v", acct.Sqid, app, env, err)

+ jsonError(w, http.StatusBadGateway, "could not read this instance's store record — try again")

+ return nil, false, nil

+ }

+ if rawTag != etag {

+ jsonError(w, http.StatusConflict,

+ "this instance's store record changed while it was being created — try again")

+ return nil, false, nil

+ }

+ prevRaw = raw

+ }

+ declaredTag := ""

if redeclare {

rec.Declared = &objstore.Declared{EnvPrefix: prefix, At: time.Now().UTC(), By: actor, Seed: seedID}

- if _, err := objstore.PutIn(walkCtx, effectStore, acct.Sqid, app, env, rec, etag); err != nil {

+ if declaredTag, err = objstore.PutIn(walkCtx, effectStore, acct.Sqid, app, env, rec, etag); err != nil {

if errors.Is(err, release.ErrCASConflict) {

jsonError(w, http.StatusConflict,

"this instance's store record changed while it was being created — try again")

@@ -286,6 +311,27 @@ func (s *Server) instanceStoreProvisionFor(ctx context.Context, w http.ResponseW

}

out, err := objstore.GrantIn(walkCtx, effectStore, effectProvisioner, s.cfg.StoreDeployment,

pub, acct.Sqid, app, env, e.Decision.By, objstore.GrantSourceAccountApproval, "", time.Now())

+ if err != nil && seedID != "" && redeclare {

+ // A seed's declaration must not outlive a grant that wrote nothing:

+ // 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.

+ 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) {

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

+ }

+ if err == nil && seedID != "" && (out.Declared == nil || out.Declared.Seed != seedID) {

+ // The declare door takes no store lock, so a `carlos store create

+ // --environment` between this declaration and the grant can rewrite

+ // it without the mark. The store is still new, but this seed's

+ // abort can no longer tear it down: said in the log, for an operator.

+ log.Printf("console: instance store %s/%s/%s: granted for seed %s but its declaration no longer names the seed", acct.Sqid, app, env, seedID)

+ }

if errors.Is(err, objstore.ErrStoreNameTaken) {

log.Printf("console: instance store provision %s/%s/%s: %v", acct.Sqid, app, env, err)

jsonError(w, http.StatusConflict,

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

index 49efa5adf..dc57ed082 100644

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

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

@@ -1,9 +1,12 @@

package console

import (

+ "bytes"

"context"

"errors"

+ "net/http"

"net/http/httptest"

+ "os"

"testing"

"time"

@@ -154,6 +157,108 @@ func TestInstanceStoreProvisionForRedeclaresADeclaredOnlyRecord(t *testing.T) {

}

+// takeCanaryBucketName plants another store of shop that names canary's

+// bucket, so canary's first grant fails with ErrStoreNameTaken before it

+// writes a grant block.

+func takeCanaryBucketName(t *testing.T, fx *consoleFixture) {

+ t.Helper()

+ at := time.Now().UTC()

+ rec := &objstore.Record{

+ Declared: &objstore.Declared{EnvPrefix: "CARLOS_STORE", At: at, By: "alice@keymail.dev"},

+ Grant: &objstore.GrantBlock{Backend: "mem", Bucket: "carlos-bab-shop-canary", Source: objstore.GrantSourceAccountApproval, At: at, By: "operator@keymail.dev"},

+ }

+ if _, err := objstore.PutIn(context.Background(), fx.rel, release.OpsAccount, "shop", "staging", rec, ""); err != nil {

+ t.Fatal(err)

+ }

+func canaryRaw(t *testing.T, fx *consoleFixture) []byte {

+ t.Helper()

+ raw, _, err := fx.rel.Get(context.Background(), objstore.KeyIn(release.OpsAccount, "shop", "canary"))

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

+ return nil

+ }

+ if err != nil {

+ t.Fatal(err)

+ }

+ return raw

+// A seed whose grant fails before writing its block leaves no record

+// marked with its id. Another path could otherwise grant that record and

+// keep the mark, and the seed's finish would tear down a store it never

+// made. Here the seed created the record, so the record is gone.

+func TestInstanceStoreProvisionForRollsBackAFreshDeclaration(t *testing.T) {

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

+ takeCanaryBucketName(t, fx)

+ w := httptest.NewRecorder()

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

+ if ok || err != nil || w.Code != http.StatusConflict {

+ t.Fatalf("provision = %v, %v (%d %s); want the name-taken refusal answered", ok, err, w.Code, w.Body.String())

+ }

+ if raw := canaryRaw(t, fx); raw != nil {

+ t.Fatalf("canary record = %s, want none: the seed's declaration must not outlive its failed grant", raw)

+ }

+ if len(fake.Buckets) != 0 {

+ t.Fatalf("backend buckets = %v, want none", fake.Buckets)

+ }

+// Here the seed re-declared a record that was there before it, so the

+// record goes back to exactly the bytes it had.

+func TestInstanceStoreProvisionForRestoresARedeclaredRecord(t *testing.T) {

+ for name, decl := range map[string]*objstore.Declared{

+ "another seed's": {EnvPrefix: "SHOP_FILES", At: time.Now().UTC().Add(-time.Hour), By: "alice@keymail.dev", Seed: seedB},

+ "an ordinary": {EnvPrefix: "SHOP_FILES", At: time.Now().UTC().Add(-time.Hour), By: "alice@keymail.dev"},

+ } {

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

+ fx, _, acct := seededStoreFixture(t, "code-seed-rollback-redeclare")

+ putCanaryStore(t, fx, &objstore.Record{Declared: decl})

+ takeCanaryBucketName(t, fx)

+ before := canaryRaw(t, fx)

+ w := httptest.NewRecorder()

+ _, ok, err := fx.srv.instanceStoreProvisionFor(context.Background(), w, acct, "shop", "canary", "bob@keymail.dev", seedA)

+ if ok || err != nil || w.Code != http.StatusConflict {

+ t.Fatalf("provision = %v, %v (%d %s); want the name-taken refusal answered", ok, err, w.Code, w.Body.String())

+ }

+ if after := canaryRaw(t, fx); !bytes.Equal(after, before) {

+ t.Fatalf("canary record after the failed seed:\n%s\nwant byte-identical to before:\n%s", after, before)

+ }

+ })

+ }

+// A grant that wrote its block before failing has made estate for this

+// seed: the record keeps the block and the mark, for the seed's finish.

+func TestInstanceStoreProvisionForKeepsAGrantItWrote(t *testing.T) {

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

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

+ w := httptest.NewRecorder()

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

+ if ok || err != nil || w.Code != http.StatusBadGateway {

+ t.Fatalf("provision = %v, %v (%d %s); want the walk failure answered", ok, err, w.Code, w.Body.String())

+ }

+ rec := canaryStore(t, fx)

+ if rec == nil || rec.Grant == nil || rec.Declared == nil || rec.Declared.Seed != seedA {

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

+ }

+// 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) {

+ fx, _, acct := seededStoreFixture(t, "code-unseeded-failed-grant")

+ takeCanaryBucketName(t, fx)

+ w := httptest.NewRecorder()

+ if _, ok := fx.srv.instanceStoreProvision(context.Background(), w, acct, "shop", "canary", "alice@keymail.dev"); ok || w.Code != http.StatusConflict {

+ t.Fatalf("provision = %v (%d %s); want the name-taken refusal", ok, w.Code, w.Body.String())

+ }

+ rec := canaryStore(t, fx)

+ if rec == nil || rec.Declared == nil || rec.Declared.Seed != "" || rec.Declared.By != "alice@keymail.dev" || rec.Grant != nil {

+ t.Fatalf("canary record = %+v, want the unmarked declaration left standing", rec)

+ }

// Without a seed, a declared-only record keeps its author and timestamp,

// exactly as before: only a seeded create re-declares.

func TestInstanceStoreProvisionKeepsAnExistingDeclaration(t *testing.T) {

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

index bdc957092..ad6fec2d8 100644

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

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

@@ -320,6 +320,44 @@ func PutIn(ctx context.Context, s release.Store, acct, app, env string, rec *Rec

return

}

+// 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.

+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)

+ })

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

+ if env != "" && !appconfig.ValidEnvironmentName(env) {

+ 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)

+ }

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

+ return err

// ErrStoreInventory means a store listing or body cannot establish its estate.

var ErrStoreInventory = errors.New("objstore: invalid store inventory")

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

index 10f0c4444..c48efcde9 100644

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

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

@@ -1,9 +1,12 @@

package objstore

import (

+ "bytes"

"context"

"errors"

+ "os"

"testing"

+ "time"

"github.com/carlosframework/platform/internal/release"

)

@@ -70,3 +73,65 @@ func TestPutGetRoundTripAndCAS(t *testing.T) {

t.Fatalf("stale etag: %v", err)

}

+// RevertIn undoes the caller's own write and nothing else: it deletes a

+// record the caller created, restores the exact prior bytes of one it

+// rewrote, and leaves a record that moved past the caller's etag.

+func TestRevertIn(t *testing.T) {

+ ctx := context.Background()

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

+ key := KeyIn("bab", "shop", "canary")

+ decl := func(by string) *Record {

+ return &Record{Declared: &Declared{EnvPrefix: DefaultEnvPrefix, At: time.Now().UTC(), By: by}}

+ }

+ // Created by the caller: deleted.

+ tag, err := PutIn(ctx, s, "bab", "shop", "canary", decl("seed"), "")

+ if err != nil {

+ t.Fatal(err)

+ }

+ if err := RevertIn(ctx, s, "bab", "shop", "canary", nil, tag); err != nil {

+ t.Fatal(err)

+ }

+ if _, _, err := s.Get(ctx, key); !errors.Is(err, os.ErrNotExist) {

+ t.Fatalf("after revert of a created record: %v, want it gone", err)

+ }

+ // Already gone: left gone.

+ if err := RevertIn(ctx, s, "bab", "shop", "canary", nil, tag); err != nil {

+ t.Fatalf("revert of a missing record = %v, want nil", err)

+ }

+ // Rewritten by the caller: the prior bytes, exactly.

+ prior := []byte("{\"schema\":1,\"app\":\"shop\",\"environment\":\"canary\",\"declared\":{\"env_prefix\":\"SHOP_FILES\",\"at\":\"2026-10-01T00:00:00Z\",\"by\":\"alice@example.com\"}}\n")

+ prevTag, err := s.Put(ctx, key, prior, "")

+ if err != nil {

+ t.Fatal(err)

+ }

+ tag, err = PutIn(ctx, s, "bab", "shop", "canary", decl("seed"), prevTag)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if err := RevertIn(ctx, s, "bab", "shop", "canary", prior, tag); err != nil {

+ t.Fatal(err)

+ }

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

+ t.Fatalf("after revert = %s, want %s", got, prior)

+ }

+ // Moved past the caller's write: left as it is.

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

+ tag, err = PutIn(ctx, s, "bab", "shop", "canary", decl("seed"), cur)

+ if err != nil {

+ t.Fatal(err)

+ }

+ if _, err := PutIn(ctx, s, "bab", "shop", "canary", decl("someone-else"), tag); err != nil {

+ 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)

+ }