carlos / platform Public

registry: SetCaps is a compare-and-swap — a stale read cannot merge over a revoke

Task 4's review (conducted in-session after the subagent hit the usage
limit): runRouteGrant computes the next column value FROM the row it
read, so two operators racing the door — or one racing a slow shell —
could re-assert a token the other just revoked, on the column that hands
out :25 and the box's IAM role. The write now carries the prior value in
its WHERE clause; a lost race refuses naming the current grants and the
operator re-runs against reality.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell 2 months ago 350dae82914e58c61cf82126643a19bcd5cde4e4 parent bd68622

3 files changed, +47 −11

cli.go +4 −1
internal/registry/registry.go +28 −3
internal/registry/registry_test.go +15 −7

diff --git a/cli.go b/cli.go

index 1770b8bc5..1086d1948 100644

--- a/cli.go

+++ b/cli.go

@@ -827,7 +827,10 @@ func runRouteGrant(r *registry.Registry, rt registry.Route, grantTok, revokeTok

}

if !dryRun {

- if err := r.SetCaps(rt.Host, next); err != nil {

+ // Compare-and-swap on the value this command computed `next` FROM:

+ // losing a race against another operator's grant/revoke refuses with

+ // the current value rather than merging a stale read over it.

+ if err := r.SetCaps(rt.Host, rt.Caps, next); err != nil {

log.Fatalf("route: write: %v", err)

}

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

index cefa35757..9f7c29f64 100644

--- a/internal/registry/registry.go

+++ b/internal/registry/registry.go

@@ -543,8 +543,16 @@ func (r *Registry) SetChannel(host, channel string) error {

// cannot clobber a concurrent SetVersions or SetNextDue), and an error when

// no row matched, because a human types a hostname into this one directly

// and a silent no-op on a typo would be worse than a refusal.

-func (r *Registry) SetCaps(host, caps string) error {

- res, err := r.sql.Exec(`UPDATE routes SET caps = ? WHERE host = ?`, caps, host)

+//

+// The write is a compare-and-swap on the column's prior value, because the

+// caller computed `to` FROM `from`: two operators racing this door (or one

+// racing a slow shell) would otherwise merge stale reads — B's grant built on

+// a read taken before A's revoke would silently re-assert the very token A

+// just removed, on the column that hands out :25 and the box's IAM role. A

+// lost race is a refusal that names the current value; the operator re-runs

+// against reality.

+func (r *Registry) SetCaps(host, from, to string) error {

+ res, err := r.sql.Exec(`UPDATE routes SET caps = ? WHERE host = ? AND caps = ?`, to, host, from)

if err != nil {

return err

}

@@ -553,11 +561,28 @@ func (r *Registry) SetCaps(host, caps string) error {

return err

}

if n == 0 {

- return fmt.Errorf("no route for host %q", host)

+ var cur string

+ serr := r.sql.QueryRow(`SELECT caps FROM routes WHERE host = ?`, host).Scan(&cur)

+ if serr == sql.ErrNoRows {

+ return fmt.Errorf("no route for host %q", host)

+ }

+ if serr != nil {

+ return serr

+ }

+ return fmt.Errorf("grants for %q changed underneath this command (now %q) — re-run it", host, capsOrNone(cur))

}

return nil

}

+// capsOrNone renders a caps column value for an error message — "none"

+// rather than "" so a lost race against a revoke reads as an answer.

+func capsOrNone(caps string) string {

+ if caps == "" {

+ return "none"

+ }

+ return caps

// SetDBPath stamps a route's database path narrowly. The converge pass uses

// it to teach ops.FleetDBs (the box-wide litestream unit's config generator)

// where a `data.replicate: platform` app's databases actually live, instead

diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go

index 4a7da1d0f..c331c00e9 100644

--- a/internal/registry/registry_test.go

+++ b/internal/registry/registry_test.go

@@ -4,6 +4,7 @@ import (

"database/sql"

"os"

"path/filepath"

+ "strings"

"testing"

)

@@ -835,7 +836,7 @@ VALUES ('legacy.example.com', 'instance', 'ready', 1700000000)`); err != nil {

t.Fatalf("the migrated row shifted columns: %+v", *got)

}

// And the new column is writable on that same row.

- if err := reg.SetCaps("legacy.example.com", "port:25"); err != nil {

+ if err := reg.SetCaps("legacy.example.com", "", "port:25"); err != nil {

t.Fatal(err)

}

got, err = reg.Get("legacy.example.com")

@@ -864,7 +865,7 @@ func TestSetCapsIsNarrowAndErrorsOnAnUnknownHost(t *testing.T) {

if err := reg.Upsert(base); err != nil {

t.Fatal(err)

}

- if err := reg.SetCaps("keymail.dev", "port:25"); err != nil {

+ if err := reg.SetCaps("keymail.dev", "", "port:25"); err != nil {

t.Fatal(err)

}

got, err := reg.Get("keymail.dev")

@@ -877,9 +878,16 @@ func TestSetCapsIsNarrowAndErrorsOnAnUnknownHost(t *testing.T) {

if *got != want {

t.Fatalf("SetCaps was not narrow:\n got %+v\n want %+v", *got, want)

}

- if err := reg.SetCaps("nope.example.com", "port:25"); err == nil {

+ if err := reg.SetCaps("nope.example.com", "", "port:25"); err == nil {

t.Fatal("SetCaps on an unknown host must error, not silently write nothing")

}

+ // The compare-and-swap: a write built on a stale read refuses, naming the

+ // current value, instead of merging over a concurrent grant/revoke.

+ if err := reg.SetCaps("keymail.dev", "stale-read", "host-role"); err == nil {

+ t.Fatal("SetCaps with a stale prior value must refuse, not overwrite")

+ } else if !strings.Contains(err.Error(), "changed underneath") || !strings.Contains(err.Error(), "port:25") {

+ t.Fatalf("a lost race should name the condition and the current value, got: %v", err)

+ }

}

// SetDBPath is the converge pass's narrow stamp (it teaches ops.FleetDBs

@@ -897,7 +905,7 @@ func TestSetDBPathIsNarrowAndErrorsOnAnUnknownHost(t *testing.T) {

if err := reg.Upsert(base); err != nil {

t.Fatal(err)

}

- if err := reg.SetCaps("console.example.com", "host-role"); err != nil {

+ if err := reg.SetCaps("console.example.com", "", "host-role"); err != nil {

t.Fatal(err)

}

if err := reg.SetDBPath("console.example.com", "/var/lib/carlos-app/console.example.com/console.db"); err != nil {

@@ -933,7 +941,7 @@ func TestUpsertPreservesAnExistingGrant(t *testing.T) {

if err := reg.Upsert(base); err != nil {

t.Fatal(err)

}

- if err := reg.SetCaps("keymail.dev", "host-role,port:25"); err != nil {

+ if err := reg.SetCaps("keymail.dev", "", "host-role,port:25"); err != nil {

t.Fatal(err)

}

// The re-add / the re-mint: same row, no grants in hand.

@@ -961,7 +969,7 @@ func TestUpsertPreservesAnExistingGrant(t *testing.T) {

t.Fatalf("Caps = %q, want an explicit Upsert value to win", got.Caps)

}

// Revoking to nothing has exactly one door, and it is not Upsert.

- if err := reg.SetCaps("keymail.dev", ""); err != nil {

+ if err := reg.SetCaps("keymail.dev", "port:25", ""); err != nil {

t.Fatal(err)

}

got, err = reg.Get("keymail.dev")

@@ -989,7 +997,7 @@ func TestFingerprintIgnoresAGrant(t *testing.T) {

if err != nil {

t.Fatal(err)

}

- if err := reg.SetCaps("keymail.dev", "host-role,port:25"); err != nil {

+ if err := reg.SetCaps("keymail.dev", "", "host-role,port:25"); err != nil {

t.Fatal(err)

}

after, err := reg.Fingerprint()