carlos / platform Public

registry: fix SetUpstreams doc overclaim, add SetUpstreams value assertion

Review fix: SetUpstreams' doc claimed the provision reconcile pass is
"the only writer" of addrs/health, but Upsert also writes both,
unconditionally, as plain overwrites — until provision.routeFor is
taught to carry Record.Addrs/Health onto the Route it mints, every
reconcile tick's Upsert blanks whatever SetUpstreams wrote. Corrected
the SetUpstreams doc and added a matching note to Upsert's per-column
doc block. Also strengthened TestFingerprintSeesAddrsAndHealthChanges
with a Get() + per-field assertion after the second SetUpstreams call,
since a fingerprint move alone can't catch an addrs<->health argument
swap inside SetUpstreams.

Paul Campbell 2 months ago a3e02f5bce25ac0c24dc8c1e4cb5f861569666b7 parent 74a2339

2 files changed, +32 −3

internal/registry/registry.go +18 −3
internal/registry/registry_test.go +14 −0

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

index 2e2e17469..8c85f7358 100644

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

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

@@ -643,6 +643,15 @@ func (r *Registry) Delete(host string) error {

// -revoke`), SetTunnel (`carlos route -tunnel -`), SetUnit (`carlos route

// -unit -`), SetSeed, SetFleetBox, and for `provisioned` the row's own

// deletion.

+//

+// `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.

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

if t.CreatedAt == 0 {

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

@@ -895,9 +904,15 @@ 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 the provision reconcile pass (the only writer of these two

-// columns today) converges them independently of everything else a full-row

-// Upsert would touch. addrs is the caller's already comma-joined list ("" =

+// 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,

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

index 2916c52f0..bfd703aac 100644

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

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

@@ -125,6 +125,20 @@ func TestFingerprintSeesAddrsAndHealthChanges(t *testing.T) {

if fp2 == fp3 {

t.Fatal("health change invisible to Fingerprint")

}

+ // The fingerprint moves on ANY column change, so the two moves above don't

+ // prove SetUpstreams wrote the right value into the right column — an

+ // addrs<->health argument swap inside SetUpstreams would move the

+ // fingerprint too. Check the values landed where asked.

+ got, err := reg.Get("m.example.com")

+ if err != nil {

+ t.Fatal(err)

+ }

+ if got.Addrs != "10.0.0.1:3000,10.0.0.3:3000" {

+ t.Fatalf("Addrs = %q, want the second SetUpstreams' addrs arg", got.Addrs)

+ }

+ if got.Health != `{"path":"/up","status":200}` {

+ t.Fatalf("Health = %q, want the second SetUpstreams' health arg", got.Health)

+ }

}

// AddrList must fold the legacy single Addr into a one-element list when