amadan

carlos / platform Public

console: actually serialise the store walks per app

objstore's deleteRecordIfUnchanged narrows the grant-vs-teardown window and
then accepts what is left, for a stated reason: "The console door serialises
these verbs per app, so the remaining window is a fraction of one round trip."

No such serialisation existed. There was no lock of any kind around Grant,
Rotate, RotateFinish or Teardown anywhere in the console, so the premise the
walks reason from was untrue and the window was a whole walk wide.

Two concurrent walks on one app both pass their read; the second sees a grant
block with no provisioned block and runs the full walk in parallel; ReapKeys
deletes the other's fresh key; and the loser's record write loses its CAS. The
record then names a key the app is not running on, and the next rotation reaps
the one it is — the outcome Rotate's doc comment calls NOT survivable.

Reaching it used to need two operators running store verbs against one app at
the same instant. Self-serve object storage (#381) puts a grant walk behind a
member's own `carlos store create`, which the create door says people re-run
from second terminals and deploy scripts.

The lock is keyed on (account, app) — two apps share no estate, so serialising
across them would cost concurrency and buy nothing — and entries are
refcounted and dropped at zero. Both teardown paths take it before reading the
record, not just around the walk: the confirm word and the estate check are
derived from that read, and a walk landing in between makes both stale.

In-process only, and the comment says so. Two consoles fronting one deployment
still interleave; that residue is what the walks' CAS guards are for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 8e3482f8033122e12e590fb6e6bc1376e5ecc424 parent 09d18f5
7 files changed, +218 −1
  • internal/console/apistore.go +3 −0
  • internal/console/appsettings.go +6 −0
  • internal/console/ownership.go +6 −0
  • internal/console/server.go +6 −0
  • internal/console/storelock.go +79 −0
  • internal/console/storelock_test.go +111 −0
  • internal/objstore/walk.go +7 −1
diff --git a/internal/console/apistore.go b/internal/console/apistore.go
index feae53e4f..a5c951b21 100644
--- a/internal/console/apistore.go
+++ b/internal/console/apistore.go
@@ -536,6 +536,7 @@ func (s *Server) handleAPIStoreGrant(w http.ResponseWriter, r *http.Request) {
}
ctx, cancel := context.WithTimeout(r.Context(), storeGrantTimeout)
defer cancel()
+ defer s.storeWalks.lock(acct.Sqid, app)()
rec, err := objstore.Grant(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreDeployment,
pub, acct.Sqid, app, addr, bucket, time.Now())
switch {
@@ -598,6 +599,7 @@ func (s *Server) handleAPIStoreRotate(w http.ResponseWriter, r *http.Request) {
// either, so a CLI can POST with nothing at all.
ctx, cancel := context.WithTimeout(r.Context(), storeRotateTimeout)
defer cancel()
+ defer s.storeWalks.lock(acct.Sqid, app)()
rec, err := objstore.Rotate(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, pub, acct.Sqid, app, time.Now())
switch {
case errors.Is(err, objstore.ErrNotProvisioned), errors.Is(err, objstore.ErrNotDeclared):
@@ -661,6 +663,7 @@ func (s *Server) handleAPIStoreRotateFinish(w http.ResponseWriter, r *http.Reque
}
ctx, cancel := context.WithTimeout(r.Context(), storeRotateTimeout)
defer cancel()
+ defer s.storeWalks.lock(acct.Sqid, app)()
rec, err := objstore.RotateFinish(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, acct.Sqid, app)
switch {
case errors.Is(err, objstore.ErrNotProvisioned):
diff --git a/internal/console/appsettings.go b/internal/console/appsettings.go
index 88a16d976..901175e8b 100644
--- a/internal/console/appsettings.go
+++ b/internal/console/appsettings.go
@@ -805,6 +805,12 @@ func (s *Server) handleStoreDestroy(w http.ResponseWriter, r *http.Request) {
// backend.
ctx, cancel := context.WithTimeout(r.Context(), storeTeardownTimeout)
defer cancel()
+ // Taken BEFORE the read, not just around the walk: the confirm word and
+ // the estate check below are both derived from this record, and a walk
+ // landing between reading it and tearing it down would make both stale —
+ // the shape objstore's deleteRecordIfUnchanged is guarding against from
+ // the other end (objstore/walk.go:560).
+ defer s.storeWalks.lock(acct.Sqid, app)()
rec, _, err := objstore.Get(ctx, s.cfg.Releases, acct.Sqid, app)
if err != nil {
log.Printf("console: store destroy read %s/%s: %v", acct.Sqid, app, err)
diff --git a/internal/console/ownership.go b/internal/console/ownership.go
index cb8f847ee..a4d2314c1 100644
--- a/internal/console/ownership.go
+++ b/internal/console/ownership.go
@@ -404,6 +404,12 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) {
// Guard on Provisioned OR Grant (controller ruling, Task 5 review): a
// grant block without a provisioned block is a crashed grant whose
// backend estate is real.
+ // Held across the read and the walk both: the estate check below is made
+ // from this record, and a grant landing in between would leave the purge
+ // sweeping a prefix whose bucket and IAM user were just re-created
+ // (objstore/walk.go:560's window, from the other end).
+ unlockStoreWalk := s.storeWalks.lock(acct.Sqid, app)
+ defer unlockStoreWalk()
if rec, _, rerr := objstore.Get(r.Context(), s.cfg.Releases, acct.Sqid, app); rerr == nil && rec != nil && (rec.Provisioned != nil || rec.Grant != nil) {
if s.cfg.StoreProvisioner == nil {
fail("“" + app + "” has a provisioned object store, and this console has no storage backend configured (CARLOS_STORE_BACKEND) — it cannot be deleted here until the store is torn down.")
diff --git a/internal/console/server.go b/internal/console/server.go
index 1acdcf8bd..0214895fa 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -616,6 +616,11 @@ type Server struct {
// notification attempt. Tests only — the send is off the request
// goroutine, and a test that asserted on it without this would be racing.
cliNotified chan struct{}
+ // storeWalks serialises the object-storage walks per app (storelock.go).
+ // objstore's own teardown reasoning assumes this exists — "The console
+ // door serialises these verbs per app" (objstore/walk.go:560) — and until
+ // #381 nothing did.
+ storeWalks *storeWalkLocks
// sendApp is how the alert watcher (alertwatch.go) delivers keymail:
// km.SendApp in production, a capture in tests. A field rather than a
// direct call so an alertPass assertion needs no keymail server.
@@ -917,6 +922,7 @@ func NewServer(cfg Config, st *Store) (*Server, error) {
delegationZone: normalizeDelegationZone(cfg.DelegationZone),
steeringHost: normalizeSteeringHost(cfg.SteeringHost),
lookupCNAME: net.DefaultResolver.LookupCNAME,
+ storeWalks: newStoreWalkLocks(),
}
if s.km != nil {
s.sendApp = s.km.SendApp
diff --git a/internal/console/storelock.go b/internal/console/storelock.go
new file mode 100644
index 000000000..aaa350622
--- /dev/null
+++ b/internal/console/storelock.go
@@ -0,0 +1,79 @@
+package console
+
+import "sync"
+
+// storeWalkLocks serialises the object-storage walks per app, which is what
+// objstore has always said this package does.
+//
+// `deleteRecordIfUnchanged` narrows the grant-vs-teardown window and then
+// accepts what is left of it, for a stated reason (`walk.go:560`):
+//
+// 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.
+//
+// Until this type there was no such serialisation anywhere in the console —
+// no lock of any kind around Grant, Rotate, RotateFinish or Teardown. The
+// premise the walks reason from was simply untrue, and the window was a whole
+// walk wide rather than a fraction of a round trip.
+//
+// It mattered less than it might have because reaching it took two operators
+// running store verbs against one app simultaneously. Self-serve object
+// storage (#381) removes that accident: a member's own `carlos store create`
+// drives a grant walk, and the create door's own comment says people re-run it
+// "from a second terminal, from a deploy script" (apistore.go:176-179).
+//
+// IN-PROCESS ONLY, and deliberately not sold as more. One console process
+// holds one of these, so two consoles fronting one deployment would still
+// interleave. That residue is what the walks' CAS guards are for — Grant's
+// step-six retry, Rotate's step-five retry, and this delete's re-read. The
+// lock removes the ordinary race; CAS survives the exotic one.
+//
+// Entries are reference-counted and dropped at zero, so an app that is walked
+// once does not hold a mutex for the life of the process.
+type storeWalkLocks struct {
+ mu sync.Mutex
+ m map[string]*storeWalkLock
+}
+
+type storeWalkLock struct {
+ mu sync.Mutex
+ refs int
+}
+
+func newStoreWalkLocks() *storeWalkLocks {
+ return &storeWalkLocks{m: make(map[string]*storeWalkLock)}
+}
+
+// lock blocks until this app's walk lock is held, and returns the release.
+// The caller defers it:
+//
+// defer s.storeWalks.lock(acct.Sqid, app)()
+//
+// The key is the account AND the app: two apps in one account share no estate
+// (one bucket and one IAM user each, both named for the pair — objstore's
+// MintedBucket and UserName), so serialising across them would cost
+// concurrency and buy nothing.
+func (l *storeWalkLocks) lock(acct, app string) func() {
+ key := acct + "/" + app
+ l.mu.Lock()
+ e := l.m[key]
+ if e == nil {
+ e = &storeWalkLock{}
+ l.m[key] = e
+ }
+ e.refs++
+ l.mu.Unlock()
+
+ e.mu.Lock()
+ return func() {
+ e.mu.Unlock()
+ l.mu.Lock()
+ e.refs--
+ if e.refs == 0 {
+ delete(l.m, key)
+ }
+ l.mu.Unlock()
+ }
+}
diff --git a/internal/console/storelock_test.go b/internal/console/storelock_test.go
new file mode 100644
index 000000000..c04d75e24
--- /dev/null
+++ b/internal/console/storelock_test.go
@@ -0,0 +1,111 @@
+package console
+
+import (
+ "sync"
+ "testing"
+ "time"
+)
+
+// The property the walks reason from: two holders of one app's lock never run
+// at once. Asserted by counting concurrent occupancy rather than by timing,
+// so the test fails on a broken lock rather than on a slow machine.
+func TestStoreWalkLocksExcludePerApp(t *testing.T) {
+ l := newStoreWalkLocks()
+ var mu sync.Mutex
+ live, maxLive := 0, 0
+
+ var wg sync.WaitGroup
+ for i := 0; i < 8; i++ {
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ defer l.lock("bab", "titogo")()
+ mu.Lock()
+ live++
+ if live > maxLive {
+ maxLive = live
+ }
+ mu.Unlock()
+ time.Sleep(time.Millisecond)
+ mu.Lock()
+ live--
+ mu.Unlock()
+ }()
+ }
+ wg.Wait()
+ if maxLive != 1 {
+ t.Fatalf("max concurrent holders of one app's walk lock = %d, want 1", maxLive)
+ }
+}
+
+// Two apps share no estate — one bucket and one IAM user each, both named for
+// the (account, app) pair — so their walks must not queue behind each other.
+func TestStoreWalkLocksDoNotSerialiseAcrossApps(t *testing.T) {
+ l := newStoreWalkLocks()
+ release := l.lock("bab", "titogo")
+ defer release()
+
+ done := make(chan struct{})
+ go func() {
+ l.lock("bab", "other")()
+ close(done)
+ }()
+ select {
+ case <-done:
+ case <-time.After(2 * time.Second):
+ t.Fatal("a walk on another app blocked behind this one's lock")
+ }
+}
+
+// An app walked once must not hold a mutex for the life of the process.
+func TestStoreWalkLocksDropEntriesAtZero(t *testing.T) {
+ l := newStoreWalkLocks()
+ l.lock("bab", "titogo")()
+
+ l.mu.Lock()
+ n := len(l.m)
+ l.mu.Unlock()
+ if n != 0 {
+ t.Fatalf("locks map holds %d entries after the last release, want 0", n)
+ }
+}
+
+// Holding one app's lock while a second waiter is queued must keep the entry
+// alive — dropping it at the wrong moment would hand the waiter a different
+// mutex and silently defeat the exclusion above.
+func TestStoreWalkLocksKeepEntryWhileWaitersQueue(t *testing.T) {
+ l := newStoreWalkLocks()
+ first := l.lock("bab", "titogo")
+
+ queued := make(chan func(), 1)
+ go func() { queued <- l.lock("bab", "titogo") }()
+
+ // Give the waiter time to register its reference before we look.
+ deadline := time.Now().Add(2 * time.Second)
+ for {
+ l.mu.Lock()
+ e := l.m["bab/titogo"]
+ refs := 0
+ if e != nil {
+ refs = e.refs
+ }
+ l.mu.Unlock()
+ if refs == 2 {
+ break
+ }
+ if time.Now().After(deadline) {
+ t.Fatalf("waiter never registered: refs = %d", refs)
+ }
+ time.Sleep(time.Millisecond)
+ }
+
+ first()
+ (<-queued)()
+
+ l.mu.Lock()
+ n := len(l.m)
+ l.mu.Unlock()
+ if n != 0 {
+ t.Fatalf("locks map holds %d entries after both released, want 0", n)
+ }
+}
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index 4dc62e139..38b9db52a 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -558,7 +558,13 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str
// 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.
+// app (internal/console/storelock.go), so the remaining window is a fraction
+// of one round trip.
+//
+// That serialisation was asserted here long before it existed — no lock of any
+// kind guarded these walks until #381 — so if this reference ever stops
+// resolving, the sentence above is a claim about nothing and this window is a
+// whole walk wide again.
func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, startKeyID, startBucket string) error {
fresh, _, err := Get(ctx, s, acct, app)
if err != nil {