amadan
console,provision,registry,release: converge the row's legacy addr, refuse shape transitions at PATCH, refuse link-local upstreams
Review fixes on top of c86a30e.
IMPORTANT 1 — the converge could not finish what the PATCH door starts.
registry.SetUpstreams wrote addrs and health but left the row's legacy addr
column standing, so a record migrated from Addr to Addrs (exactly what the
new door does to kass/keymail/console-shaped tenants) left the row saying two
things: the reconcile pass logged "record describes a different route SHAPE
... carlos remove" every tick forever, shapeMismatch refused re-adoption, and
a later clear of the set resurrected the stale addr through AddrList's fold —
live traffic to a decommissioned upstream. SetUpstreams now takes and writes
all three columns (host, addr, addrs, health) as one narrow UPDATE,
upstreamCols derives all three, and convergeOwned drives it. Addr drops out
of the owned-row drift log, since it is now a converged column and a log that
named it would fire forever on a healthy row.
IMPORTANT 2 — PATCH accepted shape changes reconcile refuses to converge in
place: a socket-backed instance handed addrs answered 200 while the box went
on serving the socket, and an addrs-backed instance-kind route emptied to
nothing became a route serving nothing. Refused with a 400 naming the way
through (delete + create) whenever DerivesPaths would flip.
IMPORTANT 3 — ValidateShape now refuses upstreams in 169.254.0.0/16 (the
cloud metadata service), in both the Addr and Addrs spellings. Deliberately
minimal per the 2026-08-12 ruling: loopback and RFC1918 stay allowed, and the
wider question (manifest opt-in for member-stated upstreams, tenant
isolation) is escalated separately.
MINOR 4 — S3Store.Put classifies 404/NoSuchKey under If-Match as
ErrCASConflict: a replace whose object was deleted under it has no etag left
to fail against, and it was surfacing as a permanent 502 on a door that
already answers conflicts. Scoped to the ifMatch branch so a create-only 404
is never masked.
MINOR 6 — documented the null-vs-absent asymmetry on the PATCH body
({"probe":null} clears; {"addrs":null} reads as absent, [] clears).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
11 files changed,
+389
−34
internal/console/apiinstances.go+31 −0internal/console/apiinstances_test.go+74 −0internal/edge/edge_test.go+1 −1internal/provision/provision.go+17 −0internal/provision/provision_test.go+32 −0internal/provision/reconcile.go+43 −21internal/provision/reconcile_test.go+76 −0internal/registry/registry.go+18 −8internal/registry/registry_test.go+45 −3internal/release/store.go+11 −1internal/release/store_rgw_test.go+41 −0
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index aaa6a1ca3..98f5243cb 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -686,6 +686,14 @@ func (s *Server) handleAPIInstanceUpdate(w http.ResponseWriter, r *http.Request) |
| // omitted field leaves what the record carries, `"addrs":[]` clears the |
| // upstream set, and `"probe":null` clears the probe while `"probe":{}` |
| // asks for the platform defaults. |
| + // |
| + // The two are deliberately NOT symmetric about null, and the asymmetry is |
| + // worth stating because it is invisible from the types: `"probe":null` |
| + // CLEARS the probe (a RawMessage records that the key was present), while |
| + // `"addrs":null` is indistinguishable from an absent key to a *[]string |
| + // and therefore means "leave the upstream set alone". Clearing the set is |
| + // spelled `"addrs":[]`, which is also the only spelling that says what it |
| + // means. |
| var req struct { |
| Addrs *[]string `json:"addrs"` |
| Probe json.RawMessage `json:"probe"` |
| @@ -718,6 +726,10 @@ func (s *Server) handleAPIInstanceUpdate(w http.ResponseWriter, r *http.Request) |
| jsonError(w, http.StatusBadGateway, "could not read the instance") |
| return |
| } |
| + // The record as it stands, kept whole before anything below mutates it: |
| + // the shape rail past ValidateShape is a comparison BETWEEN the stored |
| + // route and the patched one, not a property of either alone. |
| + stored := *rec |
| if req.Addrs != nil { |
| // This door states the upstream set WHOLE, so it also retires the |
| // legacy single-addr spelling of the same fact. Leaving Addr standing |
| @@ -740,6 +752,25 @@ func (s *Server) handleAPIInstanceUpdate(w http.ResponseWriter, r *http.Request) |
| jsonError(w, http.StatusBadRequest, err.Error()) |
| return |
| } |
| + // The rail this door needs beyond ValidateShape: a patch may repoint a |
| + // route's upstreams, never change what KIND of thing the route is. |
| + // DerivesPaths is that line exactly — it decides whether the route owns a |
| + // <host>.sock and a <host>.db — and the reconcile pass refuses to converge |
| + // a row across it in place (the "record describes a different route SHAPE" |
| + // log). Allowing the patch anyway would answer 200 for a change that can |
| + // never take effect: a socket-backed instance handed addrs would keep |
| + // serving its socket while the box logged the same complaint every pass, |
| + // and an addrs-backed route emptied to nothing would become a route that |
| + // serves nothing at all. Transitions go through delete + create, which |
| + // this API already supports, and which lets the box retire the old route's |
| + // derived paths honestly. |
| + if stored.DerivesPaths() != rec.DerivesPaths() { |
| + jsonError(w, http.StatusBadRequest, |
| + "that changes the instance's route SHAPE, not just its upstreams — a socket-backed instance "+ |
| + "and an upstream-backed one are different routes, and a box cannot convert one into the other "+ |
| + "in place; delete "+host+" and create it again with the shape you want") |
| + return |
| + } |
| // provision.UpdateRecord marshals the whole record through the canonical |
| // writer: nothing here hand-builds an object body, so upstream ORDER — |
| // which the reconcile pass's shapeMismatch compares verbatim against the |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index d8161768d..633e25472 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -1129,6 +1129,80 @@ func TestAPIInstanceUpdateUpstreams(t *testing.T) { |
| assertNotFound(t, "patch of a traversal host", want, status, h, out) |
| } |
| +// TestAPIInstanceUpdateRefusesShapeTransitions: the patch door repoints |
| +// upstreams; it does not convert one KIND of route into another. Both |
| +// directions across provision.Record.DerivesPaths are refused with a 400 that |
| +// names the way through (delete + create), because a box cannot make either |
| +// change in place: the socket-backed route would go on serving its socket |
| +// while the reconcile pass logged the same shape complaint every tick, and the |
| +// upstream-backed one emptied to nothing would become a route that serves |
| +// nothing at all. A 200 for a change that can never take effect is the worst |
| +// of the three answers. |
| +func TestAPIInstanceUpdateRefusesShapeTransitions(t *testing.T) { |
| + fx, token := instanceFixture(t, "code-inst-patch-shape") |
| + enableInstances(t, fx, "titogo", titoManifest()) |
| + u := instancesURL(fx, "bab", "titogo") |
| + // A socket-backed instance (the ordinary provisioned tenant) and an |
| + // upstream-backed service, side by side. |
| + if code, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"sock.go.tito.io","region":"ie"}`); code != http.StatusCreated { |
| + t.Fatalf("create socket-backed = %d %s", code, out) |
| + } |
| + if code, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"svc.go.tito.io","region":"ie","kind":"service","addrs":["127.0.0.1:7001"]}`); code != http.StatusCreated { |
| + t.Fatalf("create upstream-backed = %d %s", code, out) |
| + } |
| + before := serial(t, fx) |
| + |
| + // Socket-backed -> upstream-backed. |
| + code, _, out := rawReq(t, http.MethodPatch, updateURL(fx, "bab", "titogo", "sock.go.tito.io"), |
| + token, `{"addrs":["127.0.0.1:7001"]}`) |
| + if code != http.StatusBadRequest { |
| + t.Fatalf("giving a socket-backed instance upstreams = %d %s, want 400", code, out) |
| + } |
| + if !strings.Contains(out, "delete") { |
| + t.Errorf("refusal = %s, want it to name the way through (delete and create)", out) |
| + } |
| + // Upstream-backed -> socket-backed (an emptied set on an instance-kind |
| + // record; kind "" derives paths again the moment it has no upstream). |
| + if code, _, out := rawReq(t, http.MethodPost, u, token, |
| + `{"host":"back.go.tito.io","region":"ie","addrs":["127.0.0.1:7002"]}`); code != http.StatusCreated { |
| + t.Fatalf("create instance-kind upstream-backed = %d %s", code, out) |
| + } |
| + code, _, out = rawReq(t, http.MethodPatch, updateURL(fx, "bab", "titogo", "back.go.tito.io"), |
| + token, `{"addrs":[]}`) |
| + if code != http.StatusBadRequest { |
| + t.Fatalf("emptying an upstream-backed instance's set = %d %s, want 400", code, out) |
| + } |
| + |
| + // Neither refusal wrote or announced anything. |
| + sock, err := provision.GetRecord(context.Background(), fx.rel, "bab", "titogo", "sock.go.tito.io") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(sock.Addrs) != 0 { |
| + t.Errorf("a refused shape transition reached the record: %+v", sock) |
| + } |
| + back, err := provision.GetRecord(context.Background(), fx.rel, "bab", "titogo", "back.go.tito.io") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if !slices.Equal(back.Addrs, []string{"127.0.0.1:7002"}) { |
| + t.Errorf("a refused clear reached the record: %+v", back) |
| + } |
| + // One create happened in between, so the serial moved exactly once. |
| + if got := serial(t, fx); got != before+1 { |
| + t.Errorf("serial = %d, want %d — a refused patch must not announce", got, before+1) |
| + } |
| + |
| + // The same-shape patch these guards must NOT catch: an upstream-backed |
| + // route swapping its upstreams stays upstream-backed and still works. |
| + if code, _, out := rawReq(t, http.MethodPatch, updateURL(fx, "bab", "titogo", "svc.go.tito.io"), |
| + token, `{"addrs":["127.0.0.1:7003","127.0.0.1:7004"]}`); code != http.StatusOK { |
| + t.Fatalf("same-shape repoint = %d %s, want 200", code, out) |
| + } |
| +} |
| + |
| // TestAPIInstanceUpdateRefusalMatrix: the NEW verb joins the family's refusal |
| // discipline — every way of not being allowed to touch this record answers |
| // with the bytes http.NotFound writes, so a probe cannot learn that an |
diff --git a/internal/edge/edge_test.go b/internal/edge/edge_test.go| index e5102eaef..74c669b70 100644 |
| --- a/internal/edge/edge_test.go |
| +++ b/internal/edge/edge_test.go |
| @@ -1885,7 +1885,7 @@ func TestReloadSwapsUpstreamSet(t *testing.T) { |
| if err != nil { |
| t.Fatalf("open second handle: %v", err) |
| } |
| - if err := reg2.SetUpstreams("ha.example.com", addrB, health); err != nil { |
| + if err := reg2.SetUpstreams("ha.example.com", "", addrB, health); err != nil { |
| t.Fatalf("SetUpstreams: %v", err) |
| } |
| if err := e.Reload(); err != nil { |
diff --git a/internal/provision/provision.go b/internal/provision/provision.go| index 53bd36893..a3cf664ae 100644 |
| --- a/internal/provision/provision.go |
| +++ b/internal/provision/provision.go |
| @@ -402,6 +402,23 @@ func validAddr(a string) error { |
| if err != nil || portNum == 0 { |
| return fmt.Errorf("addr %q: port must be an integer in the range 1..65535", a) |
| } |
| + // 169.254.0.0/16 — IPv4 link-local, which on every cloud box the platform |
| + // runs on is the instance metadata service (169.254.169.254) and its |
| + // neighbours. An upstream is a URL the edge fetches on the caller's |
| + // behalf, so a route pointed there is an SSRF into the box's own |
| + // credential endpoint, dressed as an ordinary proxy. Refused at the same |
| + // door the port range is, so the record can never reach a box. |
| + // |
| + // This rail is DELIBERATELY minimal (ruling 2026-08-12): loopback and |
| + // RFC1918 stay allowed, because that is where every legitimate addr-backed |
| + // route on the fleet points today (127.0.0.1:8090 consoles, private-subnet |
| + // tenants). The wider question — whether a MEMBER should be able to state |
| + // an upstream at all without a manifest opt-in, and what tenant isolation |
| + // means for one — is escalated separately; do not widen this check into an |
| + // answer for it. |
| + if ip := net.ParseIP(h); ip != nil && ip.To4() != nil && ip.IsLinkLocalUnicast() { |
| + return fmt.Errorf("addr %q: 169.254.0.0/16 is link-local (the cloud metadata service lives there) — an upstream must be a real backend address", a) |
| + } |
| return nil |
| } |
diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go| index fe31bef8a..a8213852c 100644 |
| --- a/internal/provision/provision_test.go |
| +++ b/internal/provision/provision_test.go |
| @@ -696,6 +696,38 @@ func TestValidateShapeMultiAddr(t *testing.T) { |
| } |
| } |
| +// TestValidateShapeRefusesLinkLocalUpstream (ruling 2026-08-12): an upstream |
| +// is an address the EDGE connects to on a caller's behalf, so 169.254.0.0/16 |
| +// — the cloud instance metadata service and its neighbours — is refused at |
| +// the same door the port range is, in both the legacy Addr and the Addrs |
| +// spelling. |
| +// |
| +// The rail is deliberately MINIMAL: loopback and RFC1918 stay allowed, |
| +// because that is where every legitimate addr-backed route on the fleet |
| +// points today (127.0.0.1:8090 consoles, private-subnet tenants). Widening it |
| +// is a separate ruling, not a tidy-up. |
| +func TestValidateShapeRefusesLinkLocalUpstream(t *testing.T) { |
| + for _, addr := range []string{"169.254.169.254:80", "169.254.0.1:8080"} { |
| + if err := ValidateShape(Record{Host: "x", Kind: "service", Addr: addr}); err == nil { |
| + t.Errorf("legacy addr %s accepted, want it refused as link-local", addr) |
| + } else if !strings.Contains(err.Error(), "link-local") { |
| + t.Errorf("refusal for %s = %v, want it to say link-local", addr, err) |
| + } |
| + if err := ValidateShape(Record{Host: "x", Kind: "service", |
| + Addrs: []string{"10.0.0.1:3000", addr}}); err == nil { |
| + t.Errorf("addrs element %s accepted, want it refused as link-local", addr) |
| + } |
| + } |
| + // The allowed neighbours, stated so a later widening has to break a test |
| + // rather than slip through: these are live production shapes. |
| + for _, addr := range []string{"127.0.0.1:8090", "10.0.0.1:3000", "192.168.1.5:8080", "172.16.0.9:443", |
| + "169.254.169.254.example.com:80", "backend.internal:8080"} { |
| + if err := ValidateShape(Record{Host: "x", Kind: "service", Addr: addr}); err != nil { |
| + t.Errorf("addr %s refused (%v), want it allowed — this rail is metadata-service only", addr, err) |
| + } |
| + } |
| +} |
| + |
| func TestLegacyRecordStillParses(t *testing.T) { |
| // A record written before addrs/health existed — byte-for-byte old wire shape. |
| raw := []byte(`{"schema":1,"host":"a.example.com","slug":"a","app":"web","account":"bab","channel":"stable","created_at":"2026-08-01T00:00:00Z","kind":"service","addr":"10.0.0.1:3000"}`) |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 96bb668a3..71ee5a3b7 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -68,8 +68,7 @@ type acctApp struct{ acct, app string } |
| // replicated box-wide as if it never slept. |
| func routeFor(rec Record, dataDir, seed string) registry.Route { |
| rt := registry.Route{ |
| - Host: rec.Host, Slug: rec.Slug, Kind: rec.KindOrDefault(), |
| - Addr: rec.Addr, Unit: rec.Unit, |
| + Host: rec.Host, Slug: rec.Slug, Kind: rec.KindOrDefault(), Unit: rec.Unit, |
| App: rec.App, Account: rec.Account, Pool: rec.Pool, Region: rec.Region, |
| Channel: rec.Channel, Environment: rec.Environment, |
| Hibernate: rec.HibernateOrDefault(), StatusNamed: rec.StatusNamedOrDefault(), |
| @@ -79,12 +78,13 @@ func routeFor(rec Record, dataDir, seed string) registry.Route { |
| rt.Socket = DefaultSocket(rec.Host) |
| rt.DBPath = DefaultDBPath(dataDir, rec.Host) |
| } |
| - rt.Addrs, rt.Health = upstreamCols(rec) |
| + rt.Addr, rt.Addrs, rt.Health = upstreamCols(rec) |
| return rt |
| } |
| -// upstreamCols renders a record's two upstream columns exactly as a minted |
| -// route carries them: the comma-joined addr set and the probe-config JSON. |
| +// upstreamCols renders a record's THREE upstream columns exactly as a minted |
| +// route carries them: the legacy single addr, the comma-joined addr set, and |
| +// the probe-config JSON. |
| // |
| // It exists as its own function because TWO paths now write these columns — |
| // the mint (routeFor, above) and the owned-row converge (convergeOwned, which |
| @@ -92,6 +92,15 @@ func routeFor(rec Record, dataDir, seed string) registry.Route { |
| // a route that was minted correctly comes to be "converged" onto a different |
| // value every pass, forever. One derivation, two callers. |
| // |
| +// All three move together because they are one fact seen three ways: addr and |
| +// addrs are two spellings of "where this route proxies" (registry's AddrList |
| +// folds addr in whenever addrs is empty), and health decides which of those |
| +// upstreams the edge will use. A record that migrates from the legacy addr to |
| +// an addrs set — which is exactly what the console's PATCH door does to a |
| +// pre-existing single-upstream tenant — must clear the row's addr in the same |
| +// write, or the row keeps a decommissioned upstream that resurfaces through |
| +// the fold the moment the set is cleared again. |
| +// |
| // Addrs is read from the record's EXPLICIT field only, never |
| // AddrsOrDefault's Addr-folding — a legacy Addr-only record must mint (and |
| // converge to) a byte-identical route to before these fields existed |
| @@ -99,7 +108,8 @@ func routeFor(rec Record, dataDir, seed string) registry.Route { |
| // callers working off Addr alone). Health, by contrast, goes through |
| // HealthOrDefault: a multi-upstream route with no stated probe MUST be |
| // probed, or the edge has no way to tell which of its upstreams is dark. |
| -func upstreamCols(rec Record) (addrs, health string) { |
| +func upstreamCols(rec Record) (addr, addrs, health string) { |
| + addr = rec.Addr |
| if len(rec.Addrs) > 0 { |
| addrs = strings.Join(rec.Addrs, ",") |
| } |
| @@ -120,7 +130,7 @@ func upstreamCols(rec Record) (addrs, health string) { |
| health = string(b) |
| } |
| } |
| - return addrs, health |
| + return addr, addrs, health |
| } |
| // shapeMatches reports whether an existing route is the same ROUTE the |
| @@ -286,20 +296,24 @@ func convergeOwned(reg *registry.Registry, existing registry.Route, rec Record, |
| log.Printf("provision: converged %s status-named %v -> %v", rec.Host, existing.StatusNamed, *rec.StatusNamed) |
| } |
| } |
| - // One UPDATE for both columns, because they are one fact: an addr set and |
| - // the probe that decides which of those addrs the edge will send traffic |
| - // to. Writing them separately would leave a window in which a route's new |
| - // upstreams are live under the OLD probe's path/expected-status — a probe |
| - // that can mark every new upstream dark. |
| - if addrs, health := upstreamCols(rec); existing.Addrs != addrs || existing.Health != health { |
| + // One UPDATE for all three columns, because they are one fact: where the |
| + // route proxies (addr, addrs — two spellings of the same thing, which |
| + // registry.AddrList folds together) and the probe that decides which of |
| + // those upstreams the edge will send traffic to. Writing them separately |
| + // would leave a window in which a route's new upstreams are live under the |
| + // OLD probe's path/expected-status — a probe that can mark every new |
| + // upstream dark — and leaving addr out entirely would leave the row saying |
| + // two contradictory things forever (see upstreamCols). |
| + if addr, addrs, health := upstreamCols(rec); existing.Addr != addr || |
| + existing.Addrs != addrs || existing.Health != health { |
| if dryRun { |
| - log.Printf("provision: dry-run: would converge %s upstreams %q -> %q (probe %q -> %q)", |
| - rec.Host, existing.Addrs, addrs, existing.Health, health) |
| - } else if err := reg.SetUpstreams(rec.Host, addrs, health); err != nil { |
| + log.Printf("provision: dry-run: would converge %s upstreams addr %q -> %q, addrs %q -> %q (probe %q -> %q)", |
| + rec.Host, existing.Addr, addr, existing.Addrs, addrs, existing.Health, health) |
| + } else if err := reg.SetUpstreams(rec.Host, addr, addrs, health); err != nil { |
| log.Printf("provision: converge upstreams on %s: %v", rec.Host, err) |
| } else { |
| - log.Printf("provision: converged %s upstreams %q -> %q (probe %q -> %q)", |
| - rec.Host, existing.Addrs, addrs, existing.Health, health) |
| + log.Printf("provision: converged %s upstreams addr %q -> %q, addrs %q -> %q (probe %q -> %q)", |
| + rec.Host, existing.Addr, addr, existing.Addrs, addrs, existing.Health, health) |
| } |
| } |
| } |
| @@ -556,14 +570,22 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| // never spoke to Unit must not treat that stamp as drift — or |
| // this would fire on a healthy, correctly-stamped route every |
| // pass, forever. |
| + // Addr is deliberately NOT compared here any more: it is a |
| + // CONVERGED column now (convergeOwned writes it beside Addrs and |
| + // Health through SetUpstreams), and a drift log that kept naming a |
| + // column the same pass had just fixed would fire forever on a |
| + // healthy row — the exact "undiagnosable refusal" this message was |
| + // widened to prevent. What remains are the columns nothing |
| + // converges in place: the route CLASS and the two filesystem paths |
| + // derived from it. |
| if want := routeFor(rec, cfg.DataDir, seed); want.Kind != existing.Kind || |
| - want.Addr != existing.Addr || (rec.Unit != "" && want.Unit != existing.Unit) || |
| + (rec.Unit != "" && want.Unit != existing.Unit) || |
| want.Socket != existing.Socket || want.DBPath != existing.DBPath { |
| log.Printf("provision: %s: record describes a different route SHAPE than the row "+ |
| - "(kind %q/%q addr %q/%q unit %q/%q socket %q/%q db %q/%q) — not converged in place; "+ |
| + "(kind %q/%q unit %q/%q socket %q/%q db %q/%q) — not converged in place; "+ |
| "`carlos remove %s` purges this route's capability grants (registry.Delete's contract, "+ |
| "not a bug) before the next pass re-mints it, so treat that as a last resort, not a routine fix", |
| - rec.Host, want.Kind, existing.Kind, want.Addr, existing.Addr, want.Unit, existing.Unit, |
| + rec.Host, want.Kind, existing.Kind, want.Unit, existing.Unit, |
| want.Socket, existing.Socket, want.DBPath, existing.DBPath, rec.Host) |
| } |
| default: |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 59eeb6ca0..5a5ad8404 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -678,6 +678,82 @@ func TestReconcileConvergesUpstreams(t *testing.T) { |
| } |
| } |
| +// TestReconcileConvergesLegacyAddrToAddrs is the migration this whole feature |
| +// exists to make possible: a tenant that has been serving on ONE addr since |
| +// before Addrs existed (kass, keymail, the consoles) gains a second upstream |
| +// through the console's PATCH door, which rewrites the record from Addr to |
| +// Addrs. The row must end up agreeing with the record on ALL THREE upstream |
| +// columns. |
| +// |
| +// Found in review, 2026-08-12: converging addrs while leaving the row's addr |
| +// standing cost three separate things — the pass logged "record describes a |
| +// different route SHAPE ... carlos remove" every tick forever, the row could |
| +// never be re-adopted (shapeMismatch compares addr), and clearing the set |
| +// later would resurrect the decommissioned upstream through AddrList's fold. |
| +func TestReconcileConvergesLegacyAddrToAddrs(t *testing.T) { |
| + s, sqid := setup(t) |
| + rec := Record{Schema: 1, Host: "whole.go.tito.io", Slug: "whole", App: "tito", Account: sqid, |
| + Pool: "tito", Region: "ie", Channel: "stable", Kind: "service", |
| + Addr: "127.0.0.1:9443", CreatedAt: time.Now()} |
| + if err := CreateRecord(ctx, s, rec); 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) |
| + } |
| + if got := mustGet(t, reg, "whole.go.tito.io"); got.Addr != "127.0.0.1:9443" || got.Addrs != "" { |
| + t.Fatalf("legacy mint = addr %q addrs %q, want the single addr alone", got.Addr, got.Addrs) |
| + } |
| + |
| + // What the PATCH door writes: the set stated whole, the legacy spelling |
| + // retired. |
| + rec.Addr = "" |
| + rec.Addrs = []string{"127.0.0.1:9443", "127.0.0.1:9444"} |
| + putReplace(t, s, RecordKey(sqid, "tito", rec.Host), mustJSON(t, rec)) |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + after := *mustGet(t, reg, "whole.go.tito.io") |
| + if after.Addr != "" { |
| + t.Errorf("row Addr = %q after the migration, want it retired — AddrList would fold it back in", after.Addr) |
| + } |
| + if after.Addrs != "127.0.0.1:9443,127.0.0.1:9444" { |
| + t.Errorf("row Addrs = %q, want the record's set", after.Addrs) |
| + } |
| + if after.Health == "" { |
| + t.Error("a multi-upstream row converged without a probe — the edge cannot tell which upstream is dark") |
| + } |
| + if list := after.AddrList(); len(list) != 2 { |
| + t.Errorf("AddrList = %v, want exactly the two upstreams (a stale addr folded back in is live traffic to a dead backend)", list) |
| + } |
| + |
| + // The row now IS the route the record describes: no drift, and adoptable |
| + // (shapeMismatch is the adoption gate AND the drift test — the two failures |
| + // the un-cleared addr caused, asserted at their source). |
| + seed := seedJSON(&Manifest{Seed: []string{"seed"}}) // setup()'s manifest |
| + if reason := shapeMismatch(after, routeFor(rec, cfg.DataDir, seed)); reason != "" { |
| + t.Errorf("converged row still diverges from its record (%s) — the pass would log this every tick and refuse re-adoption", reason) |
| + } |
| + |
| + // Clearing the set on a route that no longer has a legacy addr to fall |
| + // back on: the columns empty out together, and nothing resurrects. |
| + rec.Addrs = nil |
| + rec.Kind = "instance" // a service with no upstream is refused by ValidateShape |
| + putReplace(t, s, RecordKey(sqid, "tito", rec.Host), mustJSON(t, rec)) |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + cleared := *mustGet(t, reg, "whole.go.tito.io") |
| + if cleared.Addr != "" || cleared.Addrs != "" || cleared.Health != "" { |
| + t.Errorf("cleared row = addr %q addrs %q health %q, want all three empty", cleared.Addr, cleared.Addrs, cleared.Health) |
| + } |
| + if cleared.AddrList() != nil { |
| + t.Errorf("AddrList = %v after clearing, want nil — a resurrected upstream is traffic to a decommissioned backend", cleared.AddrList()) |
| + } |
| +} |
| + |
| // TestReconcileLeavesLegacySingleAddrUpstreamsAlone is the additivity guard |
| // on the converge above: every record in a production bucket today is |
| // single-Addr or socket-backed, carries no addrs and no probe, and must |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index 6b957e762..18ccb5aa2 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -923,18 +923,28 @@ func (r *Registry) SetUnit(host, unit string) error { |
| // 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. |
| +// SetUpstreams or reconcile one, and the next task's job to close. |
| +// |
| +// All THREE upstream columns move together, and that is the point: addr is |
| +// the legacy single-upstream spelling of the same fact addrs carries, and |
| +// AddrList folds addr in whenever addrs is empty. A setter that wrote addrs |
| +// and left addr standing would leave the row saying two different things — |
| +// the drift log firing every pass forever on a row nothing can converge, and, |
| +// the moment the addrs set is cleared again, AddrList quietly folding the |
| +// STALE addr back in and sending live traffic to a decommissioned upstream |
| +// (found in review, 2026-08-12). Callers hand over the whole upstream shape: |
| +// addr is the single legacy upstream ("" = none), addrs the already |
| +// comma-joined set ("" = none), health the probe-config JSON verbatim ("" = |
| +// unprobed). None is validated here — the rails live at the provision pass's |
| +// own door (ValidateShape), which runs before a record ever reaches these |
| +// columns, 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 |
| // the wrong row. |
| -func (r *Registry) SetUpstreams(host, addrs, health string) error { |
| - res, err := r.sql.Exec(`UPDATE routes SET addrs = ?, health = ? WHERE host = ?`, addrs, health, host) |
| +func (r *Registry) SetUpstreams(host, addr, addrs, health string) error { |
| + res, err := r.sql.Exec(`UPDATE routes SET addr = ?, addrs = ?, health = ? WHERE host = ?`, |
| + addr, addrs, health, host) |
| if err != nil { |
| return err |
| } |
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go| index bfd703aac..9697ff41f 100644 |
| --- a/internal/registry/registry_test.go |
| +++ b/internal/registry/registry_test.go |
| @@ -105,7 +105,7 @@ func TestFingerprintSeesAddrsAndHealthChanges(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - if err := reg.SetUpstreams("m.example.com", "10.0.0.1:3000,10.0.0.3:3000", ""); err != nil { |
| + if err := reg.SetUpstreams("m.example.com", "", "10.0.0.1:3000,10.0.0.3:3000", ""); err != nil { |
| t.Fatal(err) |
| } |
| fp2, err := reg.Fingerprint() |
| @@ -115,7 +115,7 @@ func TestFingerprintSeesAddrsAndHealthChanges(t *testing.T) { |
| if fp1 == fp2 { |
| t.Fatal("addrs change invisible to Fingerprint — edge would serve a stale upstream set forever") |
| } |
| - if err := reg.SetUpstreams("m.example.com", "10.0.0.1:3000,10.0.0.3:3000", `{"path":"/up","status":200}`); err != nil { |
| + if err := reg.SetUpstreams("m.example.com", "", "10.0.0.1:3000,10.0.0.3:3000", `{"path":"/up","status":200}`); err != nil { |
| t.Fatal(err) |
| } |
| fp3, err := reg.Fingerprint() |
| @@ -156,9 +156,51 @@ func TestAddrListFoldsLegacyAddr(t *testing.T) { |
| } |
| } |
| +// TestSetUpstreamsRetiresLegacyAddr: the setter owns ALL THREE upstream |
| +// columns, in both directions. A route migrating from the legacy single addr |
| +// to a set must come out of the write with addr EMPTY — AddrList folds addr |
| +// in whenever addrs is empty, so an addr left standing is a decommissioned |
| +// upstream lying in wait for the next time the set is cleared, and the |
| +// provision pass's shape comparison would call the row permanently divergent |
| +// in the meantime. |
| +func TestSetUpstreamsRetiresLegacyAddr(t *testing.T) { |
| + reg := openTestRegistry(t) |
| + if err := reg.Upsert(Route{Host: "m.example.com", Kind: "service", Addr: "10.0.0.9:9443", Status: "ready"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := reg.SetUpstreams("m.example.com", "", "10.0.0.1:3000,10.0.0.2:3000", `{"path":"/up"}`); err != nil { |
| + t.Fatal(err) |
| + } |
| + got, err := reg.Get("m.example.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Addr != "" { |
| + t.Errorf("Addr = %q after migrating to an addrs set, want it retired", got.Addr) |
| + } |
| + if list := got.AddrList(); len(list) != 2 || list[0] != "10.0.0.1:3000" { |
| + t.Errorf("AddrList = %v, want exactly the new set", list) |
| + } |
| + |
| + // And back the other way: a route returning to a single upstream must not |
| + // keep the set beside it. |
| + if err := reg.SetUpstreams("m.example.com", "10.0.0.9:9443", "", ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got, err = reg.Get("m.example.com"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Addrs != "" || got.Health != "" { |
| + t.Errorf("Addrs/Health = %q/%q, want both retired", got.Addrs, got.Health) |
| + } |
| + if list := got.AddrList(); len(list) != 1 || list[0] != "10.0.0.9:9443" { |
| + t.Errorf("AddrList = %v, want the single upstream", list) |
| + } |
| +} |
| + |
| func TestSetUpstreamsErrorsOnUnmatchedHost(t *testing.T) { |
| reg := openTestRegistry(t) |
| - if err := reg.SetUpstreams("nope.example.com", "a:1", ""); err == nil { |
| + if err := reg.SetUpstreams("nope.example.com", "", "a:1", ""); err == nil { |
| t.Fatal("SetUpstreams on an unmatched host: want error, got nil") |
| } |
| } |
diff --git a/internal/release/store.go b/internal/release/store.go| index 3b57fe196..f3dd1d3c3 100644 |
| --- a/internal/release/store.go |
| +++ b/internal/release/store.go |
| @@ -457,7 +457,17 @@ func (s *S3Store) Put(ctx context.Context, key string, body []byte, ifMatch stri |
| } |
| out, err := s.Client.PutObject(ctx, in) |
| if err != nil { |
| - if s3CASConflict(err) { |
| + // A replace-only write whose object has been DELETED under it comes |
| + // back 404/NoSuchKey, not 412: there is no etag left to fail the |
| + // precondition against. That is still a lost conditional write — the |
| + // caller decided this body from bytes that no longer exist — so it |
| + // classifies as a CAS conflict rather than escaping as an opaque |
| + // transport error, which is what made a concurrently-deleted instance |
| + // record answer 502 ("could not update — try again", forever) instead |
| + // of the 409 the door already handles. Scoped to the ifMatch branch on |
| + // purpose: on a create-only write (IfNoneMatch:*) a 404 is not an |
| + // expected answer at all and must not be masked into a conflict. |
| + if s3CASConflict(err) || (ifMatch != "" && s3NotFound(err)) { |
| return "", fmt.Errorf("s3store: %s: %w", key, ErrCASConflict) |
| } |
| return "", err |
diff --git a/internal/release/store_rgw_test.go b/internal/release/store_rgw_test.go| index 78ef98533..bf5cbf933 100644 |
| --- a/internal/release/store_rgw_test.go |
| +++ b/internal/release/store_rgw_test.go |
| @@ -137,6 +137,47 @@ func TestS3Store_CASReplaceAgainstRGW(t *testing.T) { |
| }) |
| } |
| +// TestS3Store_IfMatchOnDeletedObjectIsCASConflict: a replace-only write whose |
| +// object was DELETED under it comes back 404/NoSuchKey rather than 412 — |
| +// there is no etag left to fail the precondition against. It is still a lost |
| +// conditional write (the caller decided this body from bytes that no longer |
| +// exist), and classifying it as anything else surfaces as a 502 "could not |
| +// update, try again" that will never succeed, on a door that already knows how |
| +// to answer a conflict. Found reviewing the instance-record PATCH, 2026-08-12. |
| +// |
| +// The create-only half is the guard on the guard: on an If-None-Match:* write |
| +// a 404 is not an expected answer at all, and masking it into a conflict would |
| +// hide a broken store behind a retry-shaped error. |
| +func TestS3Store_IfMatchOnDeletedObjectIsCASConflict(t *testing.T) { |
| + ctx := context.Background() |
| + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + if r.Method != http.MethodPut { |
| + w.WriteHeader(http.StatusMethodNotAllowed) |
| + return |
| + } |
| + w.Header().Set("Content-Type", "application/xml") |
| + w.WriteHeader(http.StatusNotFound) |
| + _, _ = w.Write([]byte(`<?xml version="1.0" encoding="UTF-8"?>` + |
| + `<Error><Code>NoSuchKey</Code><Message>the object was deleted</Message></Error>`)) |
| + })) |
| + t.Cleanup(srv.Close) |
| + rgwEnv(t, srv.URL) |
| + s, err := NewS3Store(ctx, "b") |
| + if err != nil { |
| + t.Fatalf("NewS3Store: %v", err) |
| + } |
| + |
| + _, err = s.Put(ctx, "apps/bab/titogo/instances/ha.go.tito.io.json", []byte(`{"schema":1}`), `"abc123"`) |
| + if !errors.Is(err, ErrCASConflict) { |
| + t.Fatalf("If-Match Put against a deleted object = %v, want ErrCASConflict", err) |
| + } |
| + if _, err := s.Put(ctx, "apps/bab/titogo/instances/ha.go.tito.io.json", []byte(`{"schema":1}`), ""); errors.Is(err, ErrCASConflict) { |
| + t.Fatalf("create-only Put reported a 404 as a CAS conflict: %v", err) |
| + } else if err == nil { |
| + t.Fatal("create-only Put against a 404-ing store returned no error at all") |
| + } |
| +} |
| + |
| // TestS3Store_CASStillRejectsAStaleEtag is the safety half. Stripping quotes |
| // must FIX compare-and-swap, not remove it: a genuinely stale etag has to keep |
| // losing, or every CAS in CARLOS silently becomes a blind overwrite. |