amadan

carlos / platform Public

provision: trash before manifest in the app prefetch; chunk account manifests

Astra round 2 (accept with fixes):

- P2: the app prefetch read every app's manifest, where the record loop
  had always asked the trash marker first and skipped the manifest of a
  trashed app. It reads the marker first and the manifest only when the
  app is not trashed and its marker read.
- P2: the prefetch kept each decoded manifest whole. It keeps only the
  seed the lookup always reduced it to.
- P2: account manifests are read a chunk at a time too.
- P3: tests walk 130 records across three chunks (each minted, listed,
  read once), read ten apps' state once each under the eight-reader
  ceiling, and show a trashed app's manifest is not read.
Agent notes
Mutants checked: manifest read before the trash marker, and chunks returning only the first chunk. Each fails the test written for it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 43e143897cf6f4495879f9070460b60e73426b47 parent c54e770
2 files changed, +142 −40
  • internal/provision/parallelreads_test.go +92 −6
  • internal/provision/reconcile.go +50 −34

CI passed — run details

diff --git a/internal/provision/parallelreads_test.go b/internal/provision/parallelreads_test.go
index 199ef3793..0c01a24a8 100644
--- a/internal/provision/parallelreads_test.go
+++ b/internal/provision/parallelreads_test.go
@@ -207,8 +207,11 @@ func (s *appOverlapStore) Get(ctx context.Context, key string) ([]byte, string,
// time, and each one once.
func TestReconcileReadsAppStateConcurrentlyAndOnce(t *testing.T) {
inner, sqid := setup(t)
- for i := 0; i < 6; i++ {
+ want := map[string]bool{}
+ for i := 0; i < 10; i++ {
app := "app" + string(rune('a'+i))
+ want[release.AppPrefix(sqid, app)+"instances.json"] = true
+ want[release.AppTombstoneKey(sqid, app)] = true
if err := PutManifest(ctx, inner, sqid, app, Manifest{Schema: 1, App: app, Domain: app + ".example", Pool: "tito"}); err != nil {
t.Fatal(err)
}
@@ -223,12 +226,95 @@ func TestReconcileReadsAppStateConcurrentlyAndOnce(t *testing.T) {
if err := Reconcile(ctx, s, regWith(t), cfg); err != nil {
t.Fatal(err)
}
- if s.maxFlight < 2 {
- t.Fatalf("app state read at most %d at a time, want several", s.maxFlight)
+ if s.maxFlight < 2 || s.maxFlight > 8 {
+ t.Fatalf("app state read up to %d at a time, want several and never more than 8", s.maxFlight)
}
- for key, n := range s.reads {
- if n != 1 {
- t.Fatalf("%s read %d times in one pass, want once", key, n)
+ for key := range want {
+ if s.reads[key] != 1 {
+ t.Fatalf("%s read %d times in one pass, want once", key, s.reads[key])
}
}
}
+
+// An app in the trash is never minted for, so its manifest is not read: the
+// order the record loop always asked in, trash first.
+func TestReconcileDoesNotReadATrashedAppsManifest(t *testing.T) {
+ inner, sqid := setup(t)
+ tomb := fmt.Sprintf(`{"schema":1,"account":%q,"app":"tito","deleted_at":"2026-10-05T00:00:00Z","actor":"test"}`, sqid)
+ if _, err := inner.Put(ctx, release.AppTombstoneKey(sqid, "tito"), []byte(tomb), ""); err != nil {
+ t.Fatal(err)
+ }
+ if got, err := release.ReadAppTombstone(ctx, inner, sqid, "tito"); err != nil || got == nil {
+ t.Fatalf("fixture: the trash marker does not read as one: %+v, %v", got, err)
+ }
+ if err := CreateRecord(ctx, inner, acmeRecord(sqid)); err != nil {
+ t.Fatal(err)
+ }
+ s := &appOverlapStore{Store: inner}
+ cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"}
+ if err := Reconcile(ctx, s, regWith(t), cfg); err != nil {
+ t.Fatal(err)
+ }
+ if n := s.reads[release.AppPrefix(sqid, "tito")+"instances.json"]; n != 0 {
+ t.Fatalf("a trashed app's manifest was read %d times", n)
+ }
+}
+
+// Every chunk is walked: 130 records, three chunks, each record minted and
+// listed, each read once.
+func TestRecordReadersWalkEveryChunk(t *testing.T) {
+ inner, sqid := setup(t)
+ const total = 130
+ hosts := map[string]bool{}
+ for i := 0; i < total; i++ {
+ rec := acmeRecord(sqid)
+ rec.Host, rec.Slug = fmt.Sprintf("r%04d.go.tito.io", i), fmt.Sprintf("r%04d", i)
+ hosts[rec.Host] = true
+ if err := CreateRecord(ctx, inner, rec); err != nil {
+ t.Fatal(err)
+ }
+ }
+ s := &perKeyGets{Store: inner}
+ reg := regWith(t)
+ cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"}
+ if err := Reconcile(ctx, s, reg, cfg); err != nil {
+ t.Fatal(err)
+ }
+ for h := range hosts {
+ if rt, err := reg.Get(h); err != nil || rt == nil {
+ t.Fatalf("%s not minted: %+v, %v", h, rt, err)
+ }
+ if n := s.count(RecordKey(sqid, "tito", h)); n != 1 {
+ t.Fatalf("%s read %d times by Reconcile, want once", h, n)
+ }
+ }
+ recs, err := ListAllRecords(ctx, s)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(recs) != total {
+ t.Fatalf("ListAllRecords = %d records, want %d", len(recs), total)
+ }
+}
+
+type perKeyGets struct {
+ release.Store
+ mu sync.Mutex
+ n map[string]int
+}
+
+func (s *perKeyGets) Get(ctx context.Context, key string) ([]byte, string, error) {
+ s.mu.Lock()
+ if s.n == nil {
+ s.n = map[string]int{}
+ }
+ s.n[key]++
+ s.mu.Unlock()
+ return s.Store.Get(ctx, key)
+}
+
+func (s *perKeyGets) count(key string) int {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ return s.n[key]
+}
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go
index 0de893f51..de93d8b14 100644
--- a/internal/provision/reconcile.go
+++ b/internal/provision/reconcile.go
@@ -846,9 +846,11 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
// lookups below then judge exactly what they judged before, from these
// results, and read for themselves only for an app not prefetched (a
// route's app with no record, at removal time).
+ // The manifest is kept only as the seed it renders to, never whole: the
+ // lookup below has always reduced it to that at once.
type manifestRead struct {
- m *Manifest
- err error
+ seed string
+ err error
}
type tombRead struct {
tomb *release.AppTombstone
@@ -870,12 +872,24 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
var prefetchMu sync.Mutex
forEach(recApps, func(id string) {
k := appOf[id]
- m, merr := GetManifest(ctx, s, k.acct, k.app)
+ // Trash first, as the record loop asks: an app in the trash, or one
+ // whose marker will not read, is never minted for, so its manifest
+ // was never read and is not read here either.
tomb, terr := release.ReadAppTombstone(ctx, s, k.acct, k.app)
prefetchMu.Lock()
- manifests[k] = manifestRead{m, merr}
tombs[k] = tombRead{tomb, terr}
prefetchMu.Unlock()
+ if tomb != nil || terr != nil {
+ return
+ }
+ m, merr := GetManifest(ctx, s, k.acct, k.app)
+ r := manifestRead{err: merr}
+ if merr == nil {
+ r.seed = seedJSON(m)
+ }
+ prefetchMu.Lock()
+ manifests[k] = r
+ prefetchMu.Unlock()
})
seedCache := map[acctApp]string{}
@@ -887,12 +901,15 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
return seedCache[k], seedKnown[k]
}
seenManifest[k] = true
- var m *Manifest
+ var rendered string
var err error
if r, ok := manifests[k]; ok {
- m, err = r.m, r.err
+ rendered, err = r.seed, r.err
} else {
- m, err = GetManifest(ctx, s, acct, app)
+ var m *Manifest
+ if m, err = GetManifest(ctx, s, acct, app); err == nil {
+ rendered = seedJSON(m)
+ }
}
if err != nil {
if errors.Is(err, os.ErrNotExist) {
@@ -907,7 +924,7 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
readFailed = true
return "", false
}
- seedCache[k], seedKnown[k] = seedJSON(m), true
+ seedCache[k], seedKnown[k] = rendered, true
return seedCache[k], seedKnown[k]
}
@@ -1278,33 +1295,32 @@ func knownAccounts(ctx context.Context, s release.Store) (known map[string]bool,
manifestKeys = append(manifestKeys, key)
}
}
- bodies := getEach(ctx, s, manifestKeys)
- for _, key := range keys {
- sqid, ok := domains.AccountManifestSqid(key)
- if !ok {
- continue
- }
- body, err := bodies[key].body, bodies[key].err
- if err != nil {
- log.Printf("provision: skipping %s: read failed: %v", key, err)
- readFailed = true
- continue
- }
- var m domains.AccountManifest
- if err := json.Unmarshal(body, &m); err != nil || m.ID <= 0 || m.Sqid == "" {
- log.Printf("provision: skipping %s: malformed account manifest: %v", key, err)
- readFailed = true
- continue
- }
- if m.Sqid != sqid || domains.Sqid(m.ID) != sqid {
- log.Printf("provision: skipping %s: sqid %q/%q is not Sqid(%d)=%q", key, sqid, m.Sqid, m.ID, domains.Sqid(m.ID))
- readFailed = true
- continue
- }
- if m.Deleted() {
- continue
+ for _, part := range chunks(manifestKeys) {
+ bodies := getEach(ctx, s, part)
+ for _, key := range part {
+ sqid, _ := domains.AccountManifestSqid(key)
+ body, err := bodies[key].body, bodies[key].err
+ if err != nil {
+ log.Printf("provision: skipping %s: read failed: %v", key, err)
+ readFailed = true
+ continue
+ }
+ var m domains.AccountManifest
+ if err := json.Unmarshal(body, &m); err != nil || m.ID <= 0 || m.Sqid == "" {
+ log.Printf("provision: skipping %s: malformed account manifest: %v", key, err)
+ readFailed = true
+ continue
+ }
+ if m.Sqid != sqid || domains.Sqid(m.ID) != sqid {
+ log.Printf("provision: skipping %s: sqid %q/%q is not Sqid(%d)=%q", key, sqid, m.Sqid, m.ID, domains.Sqid(m.ID))
+ readFailed = true
+ continue
+ }
+ if m.Deleted() {
+ continue
+ }
+ known[sqid] = true
}
- known[sqid] = true
}
return known, readFailed, nil
}