amadan
console: an environment binding takes a seed's store under the store lock (final fix F3)
finishSeed checked that no other instance named the seed's environment, then tore the store down; the update door could bind another instance to that environment in between, and the teardown emptied the store it had just adopted. Binding an instance to a new environment (the update door, and a create that provisions no store of its own) now takes that environment's store walk lock and, under it, clears a seed mark that is not the binder's own seed's: GrantInFor's rule for a binding that grants nothing. The update door holds the lock across its record write. finishSeedStore takes a first look without the lock, then re-reads the store record and the sharing check under the lock immediately before the teardown, so a binding made since keeps the store. The lock is process-local: one console per deployment is a rollout invariant, not built here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 files changed,
+340
−24
internal/console/apiinstances.go+12 −0internal/console/seedenvbind.go+84 −0internal/console/seedenvbind_test.go+195 −0internal/console/seedfinish.go+49 −24
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index e9c51261d..3849655de 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -1208,6 +1208,11 @@ func (s *Server) handleAPIInstanceCreate(w http.ResponseWriter, r *http.Request) |
| cancel() |
| ctx, cancel = context.WithTimeout(r.Context(), instanceStoreTimeout) |
| defer cancel() |
| + // An environment named without a store walk of its own takes the store |
| + // from a seed that made it (final review F3, seedenvbind.go). |
| + if !m.PrivateStorage && !s.bindEnvironmentCreate(ctx, w, rec) { |
| + return |
| + } |
| var claimRequest *claimCreateRequest |
| createContext := r.Context() |
| if s.cfg.HostClaim { |
| @@ -1723,6 +1728,13 @@ func (s *Server) handleAPIInstanceUpdate(w http.ResponseWriter, r *http.Request) |
| "in place; delete "+host+" and create it again with the shape you want") |
| return |
| } |
| + // A new environment binding is serialised with a seed's store teardown, |
| + // and takes the store from the seed (final review F3, seedenvbind.go). |
| + unlockEnv, ok := s.lockEnvironmentUpdate(ctx, w, stored, *rec) |
| + if !ok { |
| + return |
| + } |
| + defer unlockEnv() |
| // provision.UpdateRecord marshals the whole record through the canonical |
| // writer: nothing here hand-builds an object body, so upstream ORDER — |
| // which the reconcile pass's shapeMismatch compares verbatim against the |
diff --git a/internal/console/seedenvbind.go b/internal/console/seedenvbind.go| new file mode 100644 |
| index 000000000..3712c3f84 |
| --- /dev/null |
| +++ b/internal/console/seedenvbind.go |
| @@ -0,0 +1,84 @@ |
| +// An instance's environment binding and a seed's store (final review F3). |
| +// A seed marks the store it makes (objstore.Declared.Seed), and its finish |
| +// tears that store down while the mark stands and no other instance names |
| +// the environment (seedfinish.go). An instance that comes to name the |
| +// environment afterwards holds the store from then on, so the binding |
| +// clears the mark under the store's walk lock, the lock the finish holds |
| +// from its last check through the teardown. The lock is this process's: |
| +// one console per deployment is a rollout invariant of the seeds series. |
| + |
| +package console |
| + |
| +import ( |
| + "context" |
| + "log" |
| + "net/http" |
| + |
| + "github.com/carlosframework/platform/internal/objstore" |
| + "github.com/carlosframework/platform/internal/provision" |
| +) |
| + |
| +// lockEnvironmentUpdate is the update door's binding: when the patch moves |
| +// rec to another environment, it takes that environment's store walk lock |
| +// and moves the store to rec (takeSeedStore). The caller holds unlock |
| +// across its record write. A store that cannot be read or written answers |
| +// main's update retry sentence, and ok is false. |
| +func (s *Server) lockEnvironmentUpdate(ctx context.Context, w http.ResponseWriter, stored, rec provision.Record) (unlock func(), ok bool) { |
| + if rec.Environment == stored.Environment { |
| + return func() {}, true |
| + } |
| + unlock = s.storeWalks.lock(rec.Account, rec.App, rec.Environment) |
| + if err := s.takeSeedStore(ctx, rec); err != nil { |
| + unlock() |
| + log.Printf("console: api instances update %s/%s/%s: environment %q: %v", rec.Account, rec.App, rec.Host, rec.Environment, err) |
| + jsonError(w, http.StatusBadGateway, "could not update the instance — try again") |
| + return nil, false |
| + } |
| + return unlock, true |
| +} |
| + |
| +// bindEnvironmentCreate is the create door's binding for an instance that |
| +// names an environment and provisions no store of its own (a private |
| +// storage create's grant clears the mark itself, by GrantInFor's rule). It |
| +// moves the store to rec under the lock. A failure answers main's create |
| +// retry sentence, and the answer is false. |
| +func (s *Server) bindEnvironmentCreate(ctx context.Context, w http.ResponseWriter, rec provision.Record) bool { |
| + if rec.Environment == "" { |
| + return true |
| + } |
| + defer s.storeWalks.lock(rec.Account, rec.App, rec.Environment)() |
| + if err := s.takeSeedStore(ctx, rec); err != nil { |
| + log.Printf("console: api instances create %s/%s/%s: environment %q: %v", rec.Account, rec.App, rec.Host, rec.Environment, err) |
| + jsonError(w, http.StatusBadGateway, "could not create the instance — try again") |
| + return false |
| + } |
| + return true |
| +} |
| + |
| +// takeSeedStore clears the seed mark on the store of rec's environment |
| +// when it is not rec's own seed's: GrantInFor's rule (objstore |
| +// claimRawGrantIn) for a binding that grants nothing. Whoever binds the |
| +// store now holds it, and the seed's finish must never tear it down; an |
| +// aborted seed's bucket can leak that way, which is the safe direction. |
| +// The caller holds the environment's store walk lock. |
| +func (s *Server) takeSeedStore(ctx context.Context, rec provision.Record) error { |
| + store, etag, err := objstore.GetIn(ctx, s.cfg.Releases, rec.Account, rec.App, rec.Environment) |
| + if err != nil { |
| + return err |
| + } |
| + mine := "" |
| + if rec.Seed != nil { |
| + mine = rec.Seed.ID |
| + } |
| + if store == nil || store.Declared == nil || store.Declared.Seed == "" || store.Declared.Seed == mine { |
| + return nil |
| + } |
| + seed := store.Declared.Seed |
| + store.Declared.Seed = "" |
| + if _, err := objstore.PutIn(ctx, s.cfg.Releases, rec.Account, rec.App, rec.Environment, store, etag); err != nil { |
| + return err |
| + } |
| + log.Printf("console: instance %s/%s/%s binds environment %q: the store seed %s made (bucket %s) is no longer that seed's, and its finish leaves it standing", |
| + rec.Account, rec.App, rec.Host, rec.Environment, seed, storeBucketName(store)) |
| + return nil |
| +} |
diff --git a/internal/console/seedenvbind_test.go b/internal/console/seedenvbind_test.go| new file mode 100644 |
| index 000000000..755ed0975 |
| --- /dev/null |
| +++ b/internal/console/seedenvbind_test.go |
| @@ -0,0 +1,195 @@ |
| +package console |
| + |
| +import ( |
| + "context" |
| + "net/http" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/objstore" |
| + "github.com/carlosframework/platform/internal/provision" |
| + "github.com/carlosframework/platform/internal/seeds" |
| +) |
| + |
| +const bindHost = "other.shop.example.com" |
| + |
| +// plainInstance writes a plain record of shop on host in env. |
| +func (f fromFixture) plainInstance(t *testing.T, host, env string) { |
| + t.Helper() |
| + if err := provision.CreateRecord(context.Background(), f.fx.rel, provision.Record{Schema: 1, Host: host, Slug: "other", |
| + App: fromApp, Account: "bab", Channel: "stable", Environment: env, CreatedAt: time.Now().UTC()}); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// bindEnvironment patches host's environment through the update door. |
| +func (f fromFixture) bindEnvironment(t *testing.T, host, env string) (int, string) { |
| + t.Helper() |
| + code, _, out := rawReq(t, http.MethodPatch, f.url+"/"+host, f.token, `{"environment":"`+env+`"}`) |
| + return code, out |
| +} |
| + |
| +// Final review F3: the finish's first look passes (the store is the |
| +// seed's, and no other instance names canary), then another instance binds |
| +// canary through the update door, then the finish goes on to its teardown. |
| +// The binding took the store from the seed under the store's lock, so the |
| +// teardown's locked re-read keeps it: the store and its bucket survive, |
| +// with the mark cleared, and the record still goes. |
| +func TestFinishSeedSparesAStoreBoundAfterItsFirstLook(t *testing.T) { |
| + f := finishFixture(t) |
| + id := newSeedID(t) |
| + f.seedRecord(t, fromHost, "canary", id, seeds.KindImport, seeds.StateRolledBack) |
| + f.markedStore(t, id) |
| + f.plainInstance(t, bindHost, "staging") |
| + bucket := storeBucketName(f.canaryStore(t)) |
| + if f.fake.Buckets[bucket] == nil { |
| + f.fake.Buckets[bucket] = map[string][]byte{} |
| + } |
| + bound := false |
| + finishSeedCheckedHook = func(host string) { |
| + if host != fromHost || bound { |
| + return |
| + } |
| + bound = true |
| + if code, out := f.bindEnvironment(t, bindHost, "canary"); code != http.StatusOK { |
| + t.Errorf("bind canary = %d %s", code, out) |
| + } |
| + } |
| + t.Cleanup(func() { finishSeedCheckedHook = nil }) |
| + |
| + if err := f.fx.srv.finishSeed(context.Background(), "bab", fromApp, fromHost, id); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !bound { |
| + t.Fatal("the finish never reached its teardown checks") |
| + } |
| + store := f.canaryStore(t) |
| + if store == nil || store.Grant == nil || store.Declared == nil || store.Declared.Seed != "" { |
| + t.Fatalf("canary store after the finish = %+v; want it standing, its mark cleared", store) |
| + } |
| + if _, ok := f.fake.Buckets[bucket]; !ok { |
| + t.Fatalf("bucket %s was torn down under an instance that bound it", bucket) |
| + } |
| + if rec, err := f.record(t, bindHost); err != nil || rec.Environment != "canary" { |
| + t.Fatalf("the binder's record = %+v, %v", rec, err) |
| + } |
| + f.requireRecordGone(t, fromHost) |
| +} |
| + |
| +// The update door's binding waits for the store's walk lock, which the |
| +// finish's teardown holds, and clears another seed's mark in the same |
| +// locked stretch as its record write. |
| +func TestEnvironmentBindingWaitsForTheStoreLock(t *testing.T) { |
| + f := finishFixture(t) |
| + id := newSeedID(t) |
| + f.seedRecord(t, fromHost, "canary", id, seeds.KindImport, seeds.StatePlaced) |
| + f.markedStore(t, id) |
| + f.plainInstance(t, bindHost, "staging") |
| + unlock := f.fx.srv.storeWalks.lock("bab", fromApp, "canary") |
| + done := make(chan int, 1) |
| + go func() { |
| + code, _, _ := rawReq(t, http.MethodPatch, f.url+"/"+bindHost, f.token, `{"environment":"canary"}`) |
| + done <- code |
| + }() |
| + waitForStoreLockQueue(t, f, "canary", 2, unlock) |
| + if store := f.canaryStore(t); store.Declared.Seed != id { |
| + t.Fatalf("the binding cleared the mark without the lock: %+v", store.Declared) |
| + } |
| + if rec, _ := f.record(t, bindHost); rec.Environment != "staging" { |
| + t.Fatalf("the binding landed without the lock: %+v", rec) |
| + } |
| + unlock() |
| + if code := <-done; code != http.StatusOK { |
| + t.Fatalf("bind canary = %d", code) |
| + } |
| + if store := f.canaryStore(t); store.Declared.Seed != "" { |
| + t.Fatalf("canary store after the binding = %+v; want the mark cleared", store.Declared) |
| + } |
| +} |
| + |
| +// waitForStoreLockQueue waits until env's store lock has refs holders and |
| +// waiters. On a timeout it unlocks and fails. |
| +func waitForStoreLockQueue(t *testing.T, f fromFixture, env string, refs int, unlock func()) { |
| + t.Helper() |
| + deadline := time.Now().Add(3 * time.Second) |
| + for { |
| + f.fx.srv.storeWalks.mu.Lock() |
| + n := 0 |
| + if e := f.fx.srv.storeWalks.m[storeWalkKey("bab", fromApp, env)]; e != nil { |
| + n = e.refs |
| + } |
| + f.fx.srv.storeWalks.mu.Unlock() |
| + if n == refs { |
| + return |
| + } |
| + if time.Now().After(deadline) { |
| + unlock() |
| + t.Fatalf("the store lock of %s has %d holders and waiters, want %d", env, n, refs) |
| + } |
| + time.Sleep(time.Millisecond) |
| + } |
| +} |
| + |
| +// A binding leaves alone: a store with no mark, the binder's own seed's |
| +// store, and an unchanged environment. Only another seed's mark is cleared. |
| +func TestEnvironmentBindingClearsOnlyAnotherSeedsMark(t *testing.T) { |
| + for _, c := range []struct { |
| + name string |
| + mark func(own string) string |
| + binder func(f fromFixture, t *testing.T, own string) |
| + cleared bool |
| + }{ |
| + {"another seed's", func(string) string { return newSeedID(t) }, |
| + func(f fromFixture, t *testing.T, _ string) { f.plainInstance(t, bindHost, "staging") }, true}, |
| + {"no mark", func(string) string { return "" }, |
| + func(f fromFixture, t *testing.T, _ string) { f.plainInstance(t, bindHost, "staging") }, false}, |
| + {"the binder's own seed's", func(own string) string { return own }, |
| + func(f fromFixture, t *testing.T, own string) { |
| + f.seedRecord(t, bindHost, "staging", own, seeds.KindImport, seeds.StateCommitted) |
| + }, false}, |
| + } { |
| + t.Run(c.name, func(t *testing.T) { |
| + f := finishFixture(t) |
| + own := newSeedID(t) |
| + c.binder(f, t, own) |
| + mark := c.mark(own) |
| + f.markedStore(t, mark) |
| + if code, out := f.bindEnvironment(t, bindHost, "canary"); code != http.StatusOK { |
| + t.Fatalf("bind canary = %d %s", code, out) |
| + } |
| + got := f.canaryStore(t).Declared.Seed |
| + if want := map[bool]string{true: "", false: mark}[c.cleared]; got != want { |
| + t.Fatalf("mark after the binding = %q, want %q", got, want) |
| + } |
| + }) |
| + } |
| + // Re-sending the environment an instance already names takes nothing. |
| + f := finishFixture(t) |
| + f.plainInstance(t, bindHost, "canary") |
| + mark := newSeedID(t) |
| + f.markedStore(t, mark) |
| + if code, out := f.bindEnvironment(t, bindHost, "canary"); code != http.StatusOK { |
| + t.Fatalf("re-bind canary = %d %s", code, out) |
| + } |
| + if got := f.canaryStore(t).Declared.Seed; got != mark { |
| + t.Fatalf("mark after an unchanged binding = %q, want %q", got, mark) |
| + } |
| +} |
| + |
| +// A create naming an environment, on an app that provisions no store per |
| +// instance, takes the store from the seed that made it. |
| +func TestEnvironmentCreateTakesASeedsStore(t *testing.T) { |
| + f := newImportFixture(t) |
| + mark := newSeedID(t) |
| + at := time.Now().UTC() |
| + putCanaryStoreOf(t, f.fx, fromApp, &objstore.Record{ |
| + Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "bob@keymail.dev", Seed: mark}, |
| + Grant: &objstore.GrantBlock{Backend: "mem", Bucket: "carlos-bab-shop-canary", Source: objstore.GrantSourceAccountApproval, At: at, By: "operator@keymail.dev"}, |
| + }) |
| + if code, out := f.create(t, `{"host":"`+bindHost+`","environment":"canary"}`); code != http.StatusCreated { |
| + t.Fatalf("create = %d %v", code, out) |
| + } |
| + if got := f.canaryStore(t).Declared.Seed; got != "" { |
| + t.Fatalf("mark after the create = %q, want it cleared", got) |
| + } |
| +} |
diff --git a/internal/console/seedfinish.go b/internal/console/seedfinish.go| index f1cea9c43..f79a4f18c 100644 |
| --- a/internal/console/seedfinish.go |
| +++ b/internal/console/seedfinish.go |
| @@ -28,11 +28,11 @@ import ( |
| // another state, none) is left alone and answers nil. In order: |
| // |
| // 1. The store of the record's environment, under the store's walk lock |
| -// and re-read there, is torn down only while its Declared.Seed is |
| -// seedID (preflight M9), and only when no other instance names that |
| -// environment (M10), read strictly: a record of the app that will |
| -// not decode counts as one (Plan 8 M7). A store with another seed, or |
| -// none, is never touched. |
| +// and re-read there with the sharing check (final review F3), is torn |
| +// down only while its Declared.Seed is seedID (preflight M9), and only |
| +// when no other instance names that environment (M10), read strictly: |
| +// a record of the app that will not decode counts as one (Plan 8 M7). |
| +// A store with another seed, or none, is never touched. |
| // 2. The seed's snapshot, seeds/<host>/<id>/snapshot.db. |
| // 3. For a --from seed, every key under the host's replica prefix in the |
| // data bucket (M11). It is this seed's: the record is create-only and |
| @@ -114,43 +114,68 @@ func (s *Server) finishSeedOnce(ctx context.Context, sqid, app, host, seedID str |
| } |
| // finishSeedStore is the finish's first step: the store of env, torn down |
| -// only while seedID made it and no other instance uses it. It holds the |
| -// store's walk lock throughout, so a grant (a begin's, or carlos store |
| -// create --environment) cannot change the store between the check and the |
| -// teardown. |
| +// only while seedID made it and no other instance uses it. A first look |
| +// runs without the lock, and a store it keeps ends the step there. The |
| +// teardown itself runs under the store's walk lock, after the store record |
| +// and the sharing check are read again there (final review F3): a grant (a |
| +// begin's, or carlos store create --environment) and an instance's |
| +// environment binding (seedenvbind.go) take that lock too, and a binding |
| +// clears the seed's mark, so whatever bound since the first look keeps the |
| +// store. |
| func (s *Server) finishSeedStore(ctx context.Context, sqid, app, host, seedID, env string) error { |
| + store, err := s.seedStoreToTear(ctx, sqid, app, host, seedID, env) |
| + if store == nil || err != nil { |
| + return err |
| + } |
| + if finishSeedCheckedHook != nil { |
| + finishSeedCheckedHook(host) |
| + } |
| defer s.storeWalks.lock(sqid, app, env)() |
| + if store, err = s.seedStoreToTear(ctx, sqid, app, host, seedID, env); store == nil || err != nil { |
| + return err |
| + } |
| + if s.cfg.StoreProvisioner == nil { |
| + return fmt.Errorf("finish seed %s of %s: this console has no storage backend to tear its store down with", seedID, host) |
| + } |
| + tctx, cancel := context.WithTimeout(ctx, storeTeardownTimeout) |
| + defer cancel() |
| + if err := objstore.TeardownIn(tctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreScanner, sqid, app, env); err != nil { |
| + return fmt.Errorf("tear down the store of %s: %w", host, err) |
| + } |
| + log.Printf("console: finish seed %s of %s/%s/%s: tore down its store (bucket %s)", seedID, sqid, app, host, storeBucketName(store)) |
| + return nil |
| +} |
| + |
| +// finishSeedCheckedHook, when set (tests), runs between the finish's first |
| +// look at the store and its locked re-read. |
| +var finishSeedCheckedHook func(host string) |
| + |
| +// seedStoreToTear reads env's store and answers it when seedID made it and |
| +// no other instance names env; nil when the store is to be kept (logged |
| +// for an operator when another instance may use it). |
| +func (s *Server) seedStoreToTear(ctx context.Context, sqid, app, host, seedID, env string) (*objstore.Record, error) { |
| store, _, err := objstore.GetIn(ctx, s.cfg.Releases, sqid, app, env) |
| if err != nil { |
| - return fmt.Errorf("read the store of %s: %w", host, err) |
| + return nil, fmt.Errorf("read the store of %s: %w", host, err) |
| } |
| if store == nil || store.Declared == nil || store.Declared.Seed != seedID { |
| - return nil |
| + return nil, nil |
| } |
| other, unread, err := s.otherInstanceInEnvironment(ctx, sqid, app, host, env) |
| if err != nil { |
| - return err |
| + return nil, err |
| } |
| if unread { |
| log.Printf("console: finish seed %s of %s/%s/%s: OPERATOR: the record of instance %s could not be read, so it may use environment %q, and the store seed %s made (bucket %s) is left standing; tear it down by hand if it is not wanted", |
| seedID, sqid, app, host, other, env, seedID, storeBucketName(store)) |
| - return nil |
| + return nil, nil |
| } |
| if other != "" { |
| log.Printf("console: finish seed %s of %s/%s/%s: OPERATOR: instance %s also uses environment %q, so the store seed %s made (bucket %s) is left standing; tear it down by hand if it is not wanted", |
| seedID, sqid, app, host, other, env, seedID, storeBucketName(store)) |
| - return nil |
| - } |
| - if s.cfg.StoreProvisioner == nil { |
| - return fmt.Errorf("finish seed %s of %s: this console has no storage backend to tear its store down with", seedID, host) |
| - } |
| - tctx, cancel := context.WithTimeout(ctx, storeTeardownTimeout) |
| - defer cancel() |
| - if err := objstore.TeardownIn(tctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreScanner, sqid, app, env); err != nil { |
| - return fmt.Errorf("tear down the store of %s: %w", host, err) |
| + return nil, nil |
| } |
| - log.Printf("console: finish seed %s of %s/%s/%s: tore down its store (bucket %s)", seedID, sqid, app, host, storeBucketName(store)) |
| - return nil |
| + return store, nil |
| } |
| // otherInstanceInEnvironment names an instance of app other than host |