carlos / platform Public

provision, registry: rewrite stale SetUpstreams hazard comments, pin mint hand-off

Task 3's own commit closed the hazard these two registry.go comment
blocks (Upsert doc comment, SetUpstreams doc comment) still described as
open. Rewrite both to state the current truth: provision.routeFor now
carries Record.Addrs/Health onto every route it mints, and the reconcile
pass's owned-route branch never calls Upsert at all (narrow setters
only), so a reconcile tick cannot blank a SetUpstreams write. The one
surviving plain-overwrite blanker is `carlos add` re-run over an
existing host (cli.go:194-198), which Upserts a Route literal with no
Addrs/Health — named as the remaining hazard for a future CLI task, not
routeFor.

Also: comment the json.Marshal(Health) err==nil branch in routeFor as
provably unreachable (struct of strings/ints) rather than an unexplained
swallow, and add an end-to-end assertion (TestReconcileMintsEveryShape)
proving a minted multi-addr record's Addrs/Health reach the actual
registry row through a real Reconcile pass, not just routeFor in
isolation.

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

Paul Campbell 2 months ago 6154a7ccacdfda6cea8284ea234a0cc4d4fd3e61 parent f4ac9a4

3 files changed, +54 −19

internal/provision/reconcile.go +5 −0
internal/provision/reconcile_test.go +18 −0
internal/registry/registry.go +31 −19

diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go

index a93e5aed7..2d79e7f37 100644

--- a/internal/provision/reconcile.go

+++ b/internal/provision/reconcile.go

@@ -95,6 +95,11 @@ func routeFor(rec Record, dataDir, seed string) registry.Route {

// upstream.

if h := rec.HealthOrDefault(); h != nil {

withDefaults := h.WithDefaults()

+ // Health is a struct of strings and ints — json.Marshal cannot fail

+ // on it (no channels, funcs, cyclic pointers, unsupported types).

+ // The err == nil guard exists to fail CLOSED on the impossible case

+ // rather than panic: leaving rt.Health == "" would mean the route

+ // mints unprobed, not that a bad value reaches the edge.

if b, err := json.Marshal(withDefaults); err == nil {

rt.Health = string(b)

}

diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go

index f8eaf88a3..6e42af20e 100644

--- a/internal/provision/reconcile_test.go

+++ b/internal/provision/reconcile_test.go

@@ -159,6 +159,9 @@ func TestReconcileMintsEveryShape(t *testing.T) {

{Schema: 1, Host: "svc.go.tito.io", Slug: "svc", App: "tito", Account: sqid,

Pool: "tito", Region: "ie", Channel: "stable", Kind: "service",

Addr: "127.0.0.1:7000", Environment: "staging", CreatedAt: time.Now()},

+ {Schema: 1, Host: "ha.go.tito.io", Slug: "ha", App: "tito", Account: sqid,

+ Pool: "tito", Region: "ie", Channel: "stable", Kind: "service",

+ Addrs: []string{"127.0.0.1:7001", "127.0.0.1:7002"}, CreatedAt: time.Now()},

}

for _, rec := range recs {

if err := CreateRecord(ctx, s, rec); err != nil {

@@ -220,6 +223,21 @@ func TestReconcileMintsEveryShape(t *testing.T) {

if svc != wantSvc {

t.Errorf("service route = %+v, want %+v", svc, wantSvc)

}

+ // The routeFor -> reg.Upsert hand-off, pinned end-to-end through a real

+ // mint pass: a multi-addr record's Addrs (and its implied default

+ // Health) must reach the row Reconcile actually wrote, not just the

+ // value routeFor returns in isolation (TestRouteForCarriesAddrsAndHealth

+ // covers that half). This is the fix for the hazard Task 2 flagged —

+ // SetUpstreams was safe in isolation, but every mint-path Upsert would

+ // have blanked it, had routeFor not been taught to carry the columns.

+ ha := *mustGet(t, reg, "ha.go.tito.io")

+ if ha.Addrs != "127.0.0.1:7001,127.0.0.1:7002" {

+ t.Errorf("multi-addr record's Addrs did not reach the registry row: %q", ha.Addrs)

+ }

+ if ha.Health == "" {

+ t.Error("multi-addr record's default Health did not reach the registry row")

+ }

}

// 2. Scope.

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

index 8c85f7358..6b957e762 100644

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

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

@@ -646,12 +646,20 @@ func (r *Registry) Delete(host string) error {

// `addrs` and `health` (multi-upstream routing, issue #152) are, by

// contrast, PLAIN overwrites — see their ON CONFLICT clause below — same as

-// `addr`, `status` and the rest of the un-preserved majority. That makes

-// SetUpstreams (registry.go, next to SetUnit) unsafe to rely on until

-// provision.routeFor is taught to carry Record.Addrs/Health onto the Route

-// it mints: until then, every reconcile tick's Upsert on that host blanks

-// whatever SetUpstreams last wrote, the same way a plain excluded.environment

-// once silently unbound a route.

+// `addr`, `status` and the rest of the un-preserved majority. That is safe

+// against the provision reconcile pass specifically: provision.routeFor

+// (reconcile.go, func routeFor) now carries Record.Addrs/Health onto every

+// route it mints, and the mint branch (reconcile.go, routeFor → reg.Upsert)

+// is the only reconcile branch that calls Upsert at all — the owned-route

+// branch (reconcile.go, the `existing.Provisioned && ...` case) converges

+// through narrow setters (convergeOwned, SetHibernate, SetSeed), never a

+// full-row Upsert, so a reconcile tick on an already-owned host cannot blank

+// what SetUpstreams wrote. The one remaining plain-overwrite blanker is

+// `carlos add` re-run over an existing host (cli.go:194-198): its Upsert

+// builds a Route literal with no Addrs/Health fields, so re-adding a host

+// with an established multi-upstream set still blanks it, the same way a

+// plain excluded.environment once silently unbound a route. That gap is

+// this CLI verb's, not routeFor's, and is the next task's job to close.

func (r *Registry) Upsert(t Route) error {

if t.CreatedAt == 0 {

t.CreatedAt = time.Now().Unix()

@@ -904,19 +912,23 @@ func (r *Registry) SetUnit(host, unit string) error {

// SetUpstreams repoints a route's multi-upstream set as a narrow UPDATE —

// same rationale as SetChannel/SetUnit: a read-modify-write Upsert racing a

// deploy's SetVersions or the activator's SetNextDue would clobber those

-// columns, and this is meant to be how the provision reconcile pass converges

-// them independently of everything else a full-row Upsert would touch. But

-// unlike environment/caps/tunnel/provisioned/seed/unit/fleet_box, addrs and

-// health are PLAIN overwrites in Upsert (registry.go:684), not

-// preserve-when-empty — so today, until provision.routeFor is taught to carry

-// Addrs/Health onto the Route it builds, every reconcile tick's Upsert blanks

-// whatever a SetUpstreams call just wrote. SetUpstreams is not yet safe to

-// rely on in the presence of Upsert traffic on the same host; that wiring is

-// the next task's job. addrs is the caller's already comma-joined list ("" =

-// none); health is the probe-config JSON verbatim ("" = unprobed). Neither

-// argument is validated here — the rails live at the provision pass's own

-// door (ValidateShape), which runs before a record ever reaches this column,

-// exactly as SetCaps's callers validate before calling it.

+// columns, and this is how the provision reconcile pass converges them

+// independently of everything else a full-row Upsert would touch. Unlike

+// environment/caps/tunnel/provisioned/seed/unit/fleet_box, addrs and health

+// are PLAIN overwrites in Upsert (registry.go:684), not preserve-when-empty

+// — but provision.routeFor now carries Record.Addrs/Health onto every route

+// it mints, and the reconcile pass's owned-route branch never calls Upsert

+// (it converges through narrow setters, this one included), so a reconcile

+// tick cannot blank what a prior SetUpstreams call wrote. The surviving

+// plain-overwrite hazard is `carlos add` re-run over an existing host

+// (cli.go:194-198): its Upsert carries no Addrs/Health, so re-adding a host

+// still blanks an established multi-upstream set — a CLI-side gap, not a

+// SetUpstreams or reconcile one, and the next task's job to close. addrs is

+// the caller's already comma-joined list ("" = none); health is the

+// probe-config JSON verbatim ("" = unprobed). Neither argument is validated

+// here — the rails live at the provision pass's own door (ValidateShape),

+// which runs before a record ever reaches this column, exactly as SetCaps's

+// callers validate before calling it.

// Errors on an unmatched host, the SetChannel/SetUnit contract: a silent

// no-op on a host that does not exist would hide a reconcile pass writing to