amadan

carlos / platform Public

Merge branch 'fix-console-registry-208' into main

console: protect shared registry lifetime (#208)
codex 78fc8d40133a9c1ab538c5566ce6fa6b5149127f parent fe2088eparent 2bd3b74
9 files changed, +151 −14
  • internal/console/apistatusinstances.go +0 −1
  • internal/console/apistatusinstances_test.go +27 −0
  • internal/console/claimlogging_test.go +75 −0
  • internal/console/economicspass.go +1 −1
  • internal/console/fleets.go +7 −7
  • internal/console/instancepage_test.go +2 −1
  • internal/console/server.go +3 −0
  • internal/console/sharedregistry_test.go +17 −0
  • internal/console/view.go +19 −4

CI passed — run details

landed from fix-console-registry-208

diff --git a/internal/console/apistatusinstances.go b/internal/console/apistatusinstances.go
index 27b71e455..b4c9bb1f9 100644
--- a/internal/console/apistatusinstances.go
+++ b/internal/console/apistatusinstances.go
@@ -114,7 +114,6 @@ func (s *Server) handleAPIStatusInstances(w http.ResponseWriter, r *http.Request
"this console serves no route registry, so it cannot say which account an instance belongs to")
return
}
- defer reg.Close()
routes, err := reg.List()
if err != nil {
log.Printf("console: api status instances: registry list: %v", err)
diff --git a/internal/console/apistatusinstances_test.go b/internal/console/apistatusinstances_test.go
index 9d9081b1a..489bf37c8 100644
--- a/internal/console/apistatusinstances_test.go
+++ b/internal/console/apistatusinstances_test.go
@@ -53,6 +53,33 @@ func TestAPIStatusInstancesGate(t *testing.T) {
}
}
+func TestAPIStatusInstancesKeepsSharedRegistryOpen(t *testing.T) {
+ fx := newConsoleFixture(t)
+ token := cliToken(t, fx, "paul@keymail.dev", "code-status-registry-lifetime")
+ fx.srv.cfg.AppsDomain = "oncarlos.test"
+ if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+ tenancyFixture(t, fx)
+ statusInstanceRows(t, fx, token, "")
+
+ reg, err := openRegistry(fx.regPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, err = reg.ListPaths()
+ if err != nil {
+ t.Fatalf("registry read after status request: %v", err)
+ }
+ if rows := statusInstanceRows(t, fx, token, ""); len(rows) != 3 {
+ t.Fatalf("second status request = %d rows, want 3", len(rows))
+ }
+ status, body := postJSON(t, fx.ts.URL+"/api/cli/apps/"+fx.sqid+"/new", map[string]string{"name": "afterstatus"}, token)
+ if status != http.StatusOK {
+ t.Fatalf("app creation after status request = %d %v", status, body)
+ }
+}
+
// tenancyFixture puts real readings behind the door: one box, three sampled
// units, two of them owned by routes belonging to DIFFERENT accounts and one
// owned by no route at all.
diff --git a/internal/console/claimlogging_test.go b/internal/console/claimlogging_test.go
new file mode 100644
index 000000000..82703b6f0
--- /dev/null
+++ b/internal/console/claimlogging_test.go
@@ -0,0 +1,75 @@
+package console
+
+import (
+ "bytes"
+ "context"
+ "log"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+)
+
+// captureLog redirects the global logger for one test. Callers must not be
+// parallel.
+func captureLog(t *testing.T) *bytes.Buffer {
+ t.Helper()
+ var buf bytes.Buffer
+ prev := log.Writer()
+ log.SetOutput(&buf)
+ t.Cleanup(func() { log.SetOutput(prev) })
+ return &buf
+}
+
+func TestClaimCheckFailureIsLogged(t *testing.T) {
+ cases := []struct {
+ op string
+ breakStore func(t *testing.T, fx *consoleFixture)
+ }{
+ {"HasAppIn", func(t *testing.T, fx *consoleFixture) {
+ if _, err := fx.st.db.Exec(`DROP TABLE apps`); err != nil {
+ t.Fatal(err)
+ }
+ }},
+ {"HasOwners", func(t *testing.T, fx *consoleFixture) {
+ if _, err := fx.st.db.Exec(`DROP TABLE owners`); err != nil {
+ t.Fatal(err)
+ }
+ }},
+ {"apexHolder", func(t *testing.T, fx *consoleFixture) {
+ // A registry path whose parent is a regular file cannot be opened.
+ file := filepath.Join(t.TempDir(), "file")
+ if err := os.WriteFile(file, nil, 0o600); err != nil {
+ t.Fatal(err)
+ }
+ fx.srv.cfg.AppsDomain = "apps.test"
+ fx.srv.cfg.Registry = filepath.Join(file, "registry.db")
+ }},
+ }
+ for _, tc := range cases {
+ t.Run(tc.op, func(t *testing.T) {
+ fx := newConsoleFixture(t)
+ acct := opsAccount(t, fx)
+ tc.breakStore(t, fx)
+ buf := captureLog(t)
+ outcome, placed, held := fx.srv.claimApp(context.Background(), acct, "logged-site", "paul@keymail.dev", "", true)
+ if outcome != claimCheckFailed || placed != "" || held != nil {
+ t.Fatalf("claimApp = %v, %q, %v; want claimCheckFailed", outcome, placed, held)
+ }
+ out := buf.String()
+ for _, want := range []string{
+ `account="` + acct.Sqid + `"`,
+ `app="logged-site"`,
+ `op="` + tc.op + `"`,
+ "err=",
+ } {
+ if !strings.Contains(out, want) {
+ t.Errorf("log %q lacks %s", out, want)
+ }
+ }
+ if n := strings.Count(out, "claim check failed"); n != 1 {
+ t.Errorf("claim check failed logged %d times: %q", n, out)
+ }
+ })
+ }
+}
diff --git a/internal/console/economicspass.go b/internal/console/economicspass.go
index ab8f31261..db4dd18ca 100644
--- a/internal/console/economicspass.go
+++ b/internal/console/economicspass.go
@@ -80,7 +80,7 @@ func (s *Server) economicsPass(ctx context.Context, now time.Time) error {
// every pass that reads routes (censusPass, statementsPass); mirrors
// instanceHealth's same-shaped guard for the empty-string case
// (apiinstances.go:207).
-func (s *Server) openPassRegistry() (*registry.Registry, error) {
+func (s *Server) openPassRegistry() (consoleRegistry, error) {
if s.cfg.Registry == "" {
return nil, fmt.Errorf("no registry configured")
}
diff --git a/internal/console/fleets.go b/internal/console/fleets.go
index cafa9cc09..0985be1f1 100644
--- a/internal/console/fleets.go
+++ b/internal/console/fleets.go
@@ -68,7 +68,7 @@ type fleetView struct {
// exact same 404, mirroring apiAccountAppBy's "unknown and not-yours are one
// answer" rule for apps. It also hands back the registry handle it opened,
// since every caller needs one for whatever it does next.
-func (s *Server) apiFleet(w http.ResponseWriter, r *http.Request) (addr string, acct *Account, fleet *registry.Fleet, reg *registry.Registry, ok bool) {
+func (s *Server) apiFleet(w http.ResponseWriter, r *http.Request) (addr string, acct *Account, fleet *registry.Fleet, reg consoleRegistry, ok bool) {
addr, acct, ok = s.apiAccount(w, r)
if !ok {
return "", nil, nil, nil, false
@@ -100,7 +100,7 @@ func (s *Server) apiFleet(w http.ResponseWriter, r *http.Request) (addr string,
// its own refusal (404, "no such box") rather than the fleet-ownership 404
// above, because within an ALREADY-OWNED fleet a bad label is an ordinary
// not-found, not a cross-tenant probe.
-func findFleetBox(reg *registry.Registry, fleet, label string) (*registry.FleetBox, error) {
+func findFleetBox(reg consoleRegistry, fleet, label string) (*registry.FleetBox, error) {
boxes, err := reg.ListFleetBoxes(fleet)
if err != nil {
return nil, err
@@ -476,7 +476,7 @@ func (s *Server) handleAPIFleetDetachBox(w http.ResponseWriter, r *http.Request)
// happens to send — see placeAppTarget's doc comment for why that
// inheritance has to be a separate mechanism from the route/record stamping
// below, not the same one.
-func (s *Server) appPlacement(reg *registry.Registry, acct *Account, app string) (string, error) {
+func (s *Server) appPlacement(reg consoleRegistry, acct *Account, app string) (string, error) {
stored, err := s.store.AppPlacement(acct.ID, app)
if err != nil {
return "", err
@@ -920,7 +920,7 @@ type fleetPagePlacedApp struct {
// distinguishable "no such fleet" message that would turn this into a
// cross-account oracle. Every browser door onto a {sqid}/{fleet} URL calls
// this AFTER its own membership gate (account or owner), never before.
-func (s *Server) resolveOwnedFleet(w http.ResponseWriter, r *http.Request, acct *Account) (*registry.Fleet, *registry.Registry, bool) {
+func (s *Server) resolveOwnedFleet(w http.ResponseWriter, r *http.Request, acct *Account) (*registry.Fleet, consoleRegistry, bool) {
reg, err := openRegistry(s.cfg.Registry)
if err != nil {
log.Printf("console: fleet page: open registry: %v", err)
@@ -944,7 +944,7 @@ func (s *Server) resolveOwnedFleet(w http.ResponseWriter, r *http.Request, acct
// accountFleet gates GET .../fleets/{fleet}: session membership, any role —
// a member reads the same page an owner does, just without its forms (the
// template's own {{if .IsOwner}} checks, not a second gate here).
-func (s *Server) accountFleet(w http.ResponseWriter, r *http.Request) (addr, raw string, acct *Account, role string, fleet *registry.Fleet, reg *registry.Registry, ok bool) {
+func (s *Server) accountFleet(w http.ResponseWriter, r *http.Request) (addr, raw string, acct *Account, role string, fleet *registry.Fleet, reg consoleRegistry, ok bool) {
addr, raw, acct, role, ok = s.account(w, r)
if !ok {
return "", "", nil, "", nil, nil, false
@@ -958,7 +958,7 @@ func (s *Server) accountFleet(w http.ResponseWriter, r *http.Request) (addr, raw
// ownerFleet gates the two mutation doors (rotate, detach): owner only,
// same byte-identical-404 fleet resolution accountFleet uses.
-func (s *Server) ownerFleet(w http.ResponseWriter, r *http.Request) (addr, raw string, acct *Account, fleet *registry.Fleet, reg *registry.Registry, ok bool) {
+func (s *Server) ownerFleet(w http.ResponseWriter, r *http.Request) (addr, raw string, acct *Account, fleet *registry.Fleet, reg consoleRegistry, ok bool) {
addr, raw, acct, ok = s.owner(w, r)
if !ok {
return "", "", nil, nil, nil, false
@@ -983,7 +983,7 @@ func (s *Server) ownerFleet(w http.ResponseWriter, r *http.Request) (addr, raw s
// (warn carries the first one, for a flash banner) — this page must never
// 500 because one of three independent reads had a bad moment, the same
// posture HostRows (view.go) and handleFleet already take.
-func (s *Server) loadFleetPage(ctx context.Context, reg *registry.Registry, acct *Account, fleet *registry.Fleet) (boxes []fleetPageBox, apps []fleetPagePlacedApp, warn string) {
+func (s *Server) loadFleetPage(ctx context.Context, reg consoleRegistry, acct *Account, fleet *registry.Fleet) (boxes []fleetPageBox, apps []fleetPagePlacedApp, warn string) {
regBoxes, err := reg.ListFleetBoxes(fleet.Name)
if err != nil {
log.Printf("console: fleet page %s/%s: list boxes: %v", acct.Sqid, fleet.Name, err)
diff --git a/internal/console/instancepage_test.go b/internal/console/instancepage_test.go
index a3dee4db4..cf59e7b82 100644
--- a/internal/console/instancepage_test.go
+++ b/internal/console/instancepage_test.go
@@ -119,10 +119,11 @@ func TestOwnedInstanceRoutesDoesNotPassOtherAccountsToSelfReport(t *testing.T) {
func TestInstanceDetailOwnsRouteAndAliases(t *testing.T) {
fx := newConsoleFixture(t)
c := signInPaul(t, fx)
- reg, err := openRegistry(fx.regPath)
+ reg, err := registry.Open(fx.regPath)
if err != nil {
t.Fatal(err)
}
+ t.Cleanup(func() { reg.Close() })
if err := reg.Upsert(registry.Route{Host: "foreign.example.com", App: "kass", Account: "bac", Kind: "instance", Version: "foreign-version"}); err != nil {
t.Fatal(err)
}
diff --git a/internal/console/server.go b/internal/console/server.go
index 4da840494..072bb3a45 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -3525,6 +3525,7 @@ func (s *Server) claimApp(ctx context.Context, acct *Account, name, addr, placem
// Taken means taken IN THIS ACCOUNT — app names are unique per account,
// never globally, so another account's "blog" is not a conflict.
if has, err := s.store.HasAppIn(acct.ID, name); err != nil {
+ log.Printf("console: claim check failed: account=%q app=%q op=%q err=%v", acct.Sqid, name, "HasAppIn", err)
return claimCheckFailed, "", nil
} else if has {
return claimTaken, "", nil
@@ -3534,6 +3535,7 @@ func (s *Server) claimApp(ctx context.Context, acct *Account, name, addr, placem
// objects there stays claim-proof.
if acct.ID == OpsAccountID {
if has, err := s.store.HasOwners(name); err != nil {
+ log.Printf("console: claim check failed: account=%q app=%q op=%q err=%v", acct.Sqid, name, "HasOwners", err)
return claimCheckFailed, "", nil
} else if has {
return claimTaken, "", nil
@@ -3565,6 +3567,7 @@ func (s *Server) claimApp(ctx context.Context, acct *Account, name, addr, placem
if apexBucketFailure(err) {
return claimStoreFailed, "", nil
}
+ log.Printf("console: claim check failed: account=%q app=%q op=%q err=%v", acct.Sqid, name, "apexHolder", err)
return claimCheckFailed, "", nil
} else if hold != nil {
return claimApexHeld, "", hold
diff --git a/internal/console/sharedregistry_test.go b/internal/console/sharedregistry_test.go
new file mode 100644
index 000000000..6cb31b89d
--- /dev/null
+++ b/internal/console/sharedregistry_test.go
@@ -0,0 +1,17 @@
+package console
+
+import (
+ "reflect"
+ "testing"
+)
+
+func TestBorrowedRegistryDoesNotExposeClose(t *testing.T) {
+ for name, fn := range map[string]any{
+ "openRegistry": openRegistry,
+ "openPassRegistry": (*Server).openPassRegistry,
+ } {
+ if _, ok := reflect.TypeOf(fn).Out(0).MethodByName("Close"); ok {
+ t.Errorf("%s lets callers close the shared registry", name)
+ }
+ }
+}
diff --git a/internal/console/view.go b/internal/console/view.go
index b3ea2c477..999b9b64a 100644
--- a/internal/console/view.go
+++ b/internal/console/view.go
@@ -495,15 +495,30 @@ func TrashedApps(ctx context.Context, s release.Store, acct string, rows []Trash
return out
}
-// registry.Registry has no Close, so opening per page would leak a handle
-// each request; one cached handle per path instead. Freshness is not
-// sacrificed: Registry.List reads over a fresh connection every call.
+// consoleRegistry lets requests and passes borrow a process-owned handle.
+// Close is deliberately absent: closing it would break every later caller.
+type consoleRegistry interface {
+ Get(string) (*registry.Route, error)
+ List() ([]registry.Route, error)
+ ListAliases() ([]registry.RouteAlias, error)
+ ListPaths() ([]registry.RoutePath, error)
+ SetPlacement(string, string) error
+ CreateFleet(registry.Fleet) (bool, error)
+ GetFleet(string) (*registry.Fleet, error)
+ ListFleets() ([]registry.Fleet, error)
+ AddFleetBox(registry.FleetBox) error
+ ListFleetBoxes(string) ([]registry.FleetBox, error)
+ SetFleetBoxToken(string, string, string) error
+ SetFleetBoxTags(string, string, string) error
+ SetFleetBoxStatus(string, string, string) error
+}
+
var (
regMu sync.Mutex
regCache = map[string]*registry.Registry{}
)
-func openRegistry(path string) (*registry.Registry, error) {
+func openRegistry(path string) (consoleRegistry, error) {
regMu.Lock()
defer regMu.Unlock()
if r, ok := regCache[path]; ok {