amadan

carlos / platform Public

fix(provision): protect corrupted-record routes, resolve duplicate hosts loudly

Critical: an unparseable or self-disagreeing record body must never read as
"the bucket retired it" -- the removal phase was deleting live routes for
records that merely failed to parse this pass. A protect set (keyed on the
record's key-derived host) now holds back only that host, narrower than the
pass-wide readFailed veto.

Important: two accounts racing for the same host resolved by silent
last-writer-wins map overwrite. First-seen (listing order) now wins, and the
loser is logged and skipped instead of vanishing without a trace.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 07d431e3686349c05c5b527b06ca6956c0b20b62 parent 116f5a0
2 files changed, +114 −0
  • internal/provision/reconcile.go +26 −0
  • internal/provision/reconcile_test.go +88 −0
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go
index 0eb8bcca6..daa83bd3d 100644
--- a/internal/provision/reconcile.go
+++ b/internal/provision/reconcile.go
@@ -70,6 +70,17 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
return fmt.Errorf("provision: list apps: %w", err)
}
recs := map[string]Record{} // host -> record, in-scope only
+ // protect holds hosts whose record KEY parsed (ParseRecordKey succeeded)
+ // but whose body could not be trusted — unparseable JSON, or a body that
+ // disagrees with its own path. Such a host must NEVER be treated as "the
+ // bucket retired it": a non-additive Record encoding change reaching an
+ // older binary would make EVERY existing record fail exactly this way,
+ // and without this guard one tick would delete every provisioned route
+ // in scope. This is narrower than readFailed (which vetoes removal for
+ // the WHOLE pass): only the specific host with the unusable body is held
+ // back — every other record still converges/removes normally the same
+ // pass.
+ protect := map[string]bool{}
for _, key := range keys {
acct, app, host, ok := ParseRecordKey(key, known)
if !ok {
@@ -87,16 +98,27 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
var rec Record
if err := json.Unmarshal(raw, &rec); err != nil {
log.Printf("provision: skipping record %s: malformed: %v", key, err)
+ protect[host] = true
continue
}
if rec.Host != host || rec.Account != acct || rec.App != app {
log.Printf("provision: skipping record %s: disagrees with its own path (host=%q account=%q app=%q)",
key, rec.Host, rec.Account, rec.App)
+ protect[host] = true
continue
}
if rec.Region != cfg.Region || !poolServed(cfg.Pools, rec.Pool) {
continue // another agent's jurisdiction, not this pass's business
}
+ // Two accounts writing the same host is a collision this pass
+ // cannot arbitrate silently: whichever record this listing reaches
+ // FIRST wins the host, and the loser is skipped loudly rather than
+ // overwriting the winner in the map with no trace of the conflict.
+ if prev, dup := recs[rec.Host]; dup {
+ log.Printf("provision: duplicate host %s: keeping %s, skipping %s", rec.Host,
+ RecordKey(prev.Account, prev.App, prev.Host), RecordKey(rec.Account, rec.App, rec.Host))
+ continue
+ }
recs[rec.Host] = rec
}
@@ -197,6 +219,10 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg
if _, ok := recs[rt.Host]; ok {
continue
}
+ if protect[rt.Host] {
+ log.Printf("provision: not removing %s: its record exists but could not be read cleanly this pass", rt.Host)
+ continue
+ }
if err := reg.Delete(rt.Host); err != nil {
log.Printf("provision: remove retired instance route %s: %v", rt.Host, err)
continue
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go
index 9212d2ea6..088bc4796 100644
--- a/internal/provision/reconcile_test.go
+++ b/internal/provision/reconcile_test.go
@@ -390,3 +390,91 @@ func TestReconcileSkipsMalformedRecord(t *testing.T) {
t.Errorf("minted from a malformed record's body host: %+v %v", rt, err)
}
}
+
+// CRITICAL regression (code review finding): an existing route's record
+// going unparseable — not gone, just unreadable this pass — must never read
+// as "the bucket retired it". Before the fix, json.Unmarshal failure and
+// path-disagreement both log-and-continue without setting readFailed and
+// without the host entering recs, so the removal phase deleted the route.
+// A single non-additive change to Record's wire shape reaching an older
+// binary would make EVERY existing record fail exactly this way, turning
+// one tick into a mass fleet retirement. The fix (protect map, keyed on the
+// record's KEY-derived host) is deliberately narrower than readFailed: it
+// holds back only the specific corrupted host, so a genuinely retired
+// neighbor is still removed in the very same pass — proven below.
+func TestReconcileProtectsRouteWithCorruptedRecordBody(t *testing.T) {
+ s, sqid := setup(t)
+ if err := CreateRecord(ctx, s, acmeRecord(sqid)); err != nil {
+ t.Fatal(err)
+ }
+ reg := regWith(t)
+ cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"}
+ if err := Reconcile(ctx, s, reg, cfg); err != nil {
+ t.Fatal(err)
+ }
+ mustGet(t, reg, "acme.go.tito.io") // minted while the record was good
+
+ // Corrupt the record body IN PLACE (a replace, not a delete) — the
+ // shape a non-additive encoding change would produce on an old binary.
+ putReplace(t, s, RecordKey(sqid, "tito", "acme.go.tito.io"), []byte("not json"))
+
+ // A second, genuinely retired route: its record is deleted outright
+ // (not corrupted), so it must still be removed in the SAME pass — this
+ // is what proves protect is narrow, not a readFailed-style pass-wide
+ // veto.
+ ghost := registry.Route{Host: "ghost.go.tito.io", Kind: "instance", App: "tito",
+ Status: "ready", Provisioned: true, Region: "ie", Pool: "tito", Account: sqid}
+ if err := reg.Upsert(ghost); err != nil {
+ t.Fatal(err)
+ }
+
+ if err := Reconcile(ctx, s, reg, cfg); err != nil {
+ t.Fatal(err)
+ }
+ mustGet(t, reg, "acme.go.tito.io") // protected: an unparseable record is not "gone"
+ if rt, err := reg.Get("ghost.go.tito.io"); err != nil || rt != nil {
+ t.Errorf("a genuinely record-less route was not removed alongside a protected one: %+v %v", rt, err)
+ }
+}
+
+// IMPORTANT regression (code review finding): two accounts holding a record
+// for the same host must not resolve by silent last-writer-wins map
+// overwrite (the losing tenant's record would vanish from the pass with no
+// diagnostic). First-seen (bucket listing order) wins; the loser is skipped
+// loudly and produces no second route, no crash.
+func TestReconcileDuplicateHostAcrossAccountsFirstSeenWins(t *testing.T) {
+ s := dirStore(t)
+ sqid1 := writeAccountManifest(t, s, 2, "Tito")
+ sqid2 := writeAccountManifest(t, s, 3, "Other")
+ rec1 := Record{Schema: 1, Host: "dup.go.tito.io", Slug: "dup", App: "tito",
+ Account: sqid1, Pool: "tito", Region: "ie", Channel: "stable", CreatedAt: time.Now()}
+ rec2 := Record{Schema: 1, Host: "dup.go.tito.io", Slug: "dup", App: "tito",
+ Account: sqid2, Pool: "tito", Region: "ie", Channel: "stable", CreatedAt: time.Now()}
+ if err := CreateRecord(ctx, s, rec1); err != nil {
+ t.Fatal(err)
+ }
+ if err := CreateRecord(ctx, s, rec2); err != nil {
+ t.Fatal(err)
+ }
+ reg := regWith(t)
+ cfg := ReconcileConfig{Region: "ie", Pools: []string{"tito"}, DataDir: "/data"}
+ if err := Reconcile(ctx, s, reg, cfg); err != nil {
+ t.Fatal(err)
+ }
+
+ // The winner is whichever record's KEY sorts first — that is the order
+ // s.List("apps/") actually hands the reconcile loop, so this must be
+ // derived, never assumed.
+ key1, key2 := RecordKey(sqid1, "tito", rec1.Host), RecordKey(sqid2, "tito", rec2.Host)
+ winner := sqid1
+ if key2 < key1 {
+ winner = sqid2
+ }
+ got := mustGet(t, reg, "dup.go.tito.io")
+ if got.Account != winner {
+ t.Errorf("duplicate host resolved to account %q, want first-seen %q", got.Account, winner)
+ }
+ if routes, err := reg.List(); err != nil || len(routes) != 1 {
+ t.Errorf("duplicate host produced %d routes, want exactly 1: %+v %v", len(routes), routes, err)
+ }
+}