amadan
provision+registry+cli: fix steering review findings — re-add preservation, adoption gate, Fingerprint exclusion
Three review findings against 76c4482, per the amended plan (b485ec5): 1. carlos add's re-add path blanked steering (Upsert overwrites it like addrs/health); the Upsert doc paragraph wrongly claimed no such caller existed. Fixed by preserving existing.Steering in runAdd's existing-route block, same as Addrs/Health/Addr, and correcting the doc comment. 2. shapeMismatch no longer compares Steering: it's a record-owned column (Channel/Environment/Hibernate's category), and comparing it could refuse adoption of a hand-added route forever for a case adoption exists to serve. convergeOwned already stamps it unconditionally post-adoption. 3. Fingerprint() no longer hashes steering — reverted to its pre-Task-2 column set. The plan's "automatic via routeCols" premise only holds for SnapshotFingerprint (which needs no change and keeps its test); Fingerprint's column list is hand-curated and steering has no edge consumer to reload for. Route.Steering's doc comment now carries the deliberate-exclusion note in the Caps/Unit idiom. Tests: TestAddPreservesUpstreamsOnReAdd extended with steering assertions; TestShapeMismatchComparesSteering inverted to TestShapeMismatchIgnoresSteering; TestFingerprintSeesSteeringChange replaced with TestFingerprintIgnoresSteering; TestSnapshotFingerprintSeesSteeringChange unchanged.
6 files changed,
+101
−47
cli.go+7 −0cli_test.go+15 −0internal/provision/reconcile.go+11 −6internal/provision/reconcile_test.go+18 −7internal/registry/registry.go+32 −22internal/registry/registry_test.go+18 −12
diff --git a/cli.go b/cli.go| index 102ffb7df..0c98e76f1 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -228,6 +228,13 @@ func runAdd(args []string) { |
| if *addr == "" { |
| route.Addr = existing.Addr |
| } |
| + // Steering (multi-region ingress design Part 4) carries no -steering |
| + // flag on this box verb either, and Upsert overwrites it exactly like |
| + // Addrs/Health (registry.go's Upsert doc comment) — so the same gap |
| + // applies: a bare re-add would silently clear a route's Route53 |
| + // steering opt-in the moment the steering housekeeper duty (Task 6) |
| + // starts converging DNS from it. Preserve it the same way. |
| + route.Steering = existing.Steering |
| } |
| if err := r.Upsert(route); err != nil { |
| log.Fatal(err) |
diff --git a/cli_test.go b/cli_test.go| index b86b37ef7..6a012e263 100644 |
| --- a/cli_test.go |
| +++ b/cli_test.go |
| @@ -747,6 +747,15 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| if err := reg.SetUpstreams(host, "", wantAddrs, wantHealth); err != nil { |
| t.Fatalf("SetUpstreams: %v", err) |
| } |
| + // Steering (multi-region ingress design Part 4) has the identical gap: |
| + // no -steering flag on this verb, Upsert's steering column is a plain |
| + // overwrite like addrs/health, so a bare re-add must not blank a route's |
| + // Route53 opt-in either (review finding: a re-add landing inside the |
| + // steering housekeeper's 5-minute converge window would otherwise delete |
| + // the host's live latency records). |
| + if err := reg.SetSteering(host, "latency"); err != nil { |
| + t.Fatalf("SetSteering: %v", err) |
| + } |
| // A re-add naming -addr again, once the row carries an established |
| // multi-upstream set, is refused rather than silently clearing it — the |
| @@ -767,6 +776,9 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| t.Fatalf("row after a refused re-add = addr %q addrs %q health %q, want it untouched (addr \"\", addrs %q, health %q)", |
| rt.Addr, rt.Addrs, rt.Health, wantAddrs, wantHealth) |
| } |
| + if rt.Steering != "latency" { |
| + t.Fatalf("steering after a refused re-add = %q, want it untouched (latency)", rt.Steering) |
| + } |
| // A re-add naming NO -addr at all must not blank Addrs/Health either — |
| // this box-local verb only carries what its own flags set, and the |
| @@ -784,6 +796,9 @@ func TestAddPreservesUpstreamsOnReAdd(t *testing.T) { |
| if rt.Addr != "" { |
| t.Errorf("Addr after a flagless re-add = %q, want it to stay preserved as the establishing SetUpstreams left it", rt.Addr) |
| } |
| + if rt.Steering != "latency" { |
| + t.Errorf("steering after a flagless re-add = %q, want it still preserved as the establishing SetSteering left it", rt.Steering) |
| + } |
| // The refusal is Addrs-specific: an ordinary re-add with -addr on a host |
| // that has no established multi-upstream set is unaffected and still |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 3a47a42dd..b8c98e4af 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -309,10 +309,17 @@ func upstreamCols(rec Record) (addr, addrs, health string) { |
| // record describes — the gate on adoption (design §4.2), and so the gate on |
| // this pass acquiring removal authority over a row it did not mint. |
| // |
| -// Channel, Environment and Hibernate are deliberately absent: those are the |
| -// columns the record is taking ownership OF, and a hand-added route on the |
| -// wrong channel is the whole reason to adopt one. Requiring them to agree |
| -// would make adoption impossible in exactly the case it exists for. |
| +// Channel, Environment, Hibernate and Steering are deliberately absent: those |
| +// are the columns the record is taking ownership OF, and a hand-added route |
| +// on the wrong channel is the whole reason to adopt one. Requiring them to |
| +// agree would make adoption impossible in exactly the case it exists for — |
| +// Steering named explicitly here because a record carrying |
| +// steering:"latency" over an unprovisioned row is a real deployment shape |
| +// (an operator hand-added the route before the console's steering PATCH |
| +// door existed to touch it), and refusing that adoption forever would buy |
| +// nothing: convergeOwned stamps Steering onto the row unconditionally the |
| +// moment adoption succeeds (ruled 2026-08-23, plan amendment b485ec5, |
| +// superseding this task's own first pass, which compared it here). |
| // |
| // Backing, Tunnel and Caps are deliberately required to be empty (unset) on |
| // the existing row — registry.Route's own doc comments name these three as |
| @@ -388,8 +395,6 @@ func shapeMismatch(existing, want registry.Route) string { |
| return fmt.Sprintf("addrs %q/%q", existing.Addrs, want.Addrs) |
| case existing.Health != want.Health: |
| return fmt.Sprintf("health %q/%q", existing.Health, want.Health) |
| - case existing.Steering != want.Steering: |
| - return fmt.Sprintf("steering %q/%q", existing.Steering, want.Steering) |
| case existing.Unit != want.Unit: |
| return fmt.Sprintf("unit %q/%q", existing.Unit, want.Unit) |
| case existing.Slug != want.Slug: |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 2e0478cd8..dc73f2e16 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -2248,17 +2248,28 @@ func TestRouteForCarriesSteering(t *testing.T) { |
| } |
| } |
| -// TestShapeMismatchComparesSteering is the adopt-side proof for Steering, the |
| -// same guard TestShapeMismatchComparesAddrsAndHealth already applies to the |
| -// upstream columns: a hand-added route whose steering disagrees with what |
| -// the record describes must not be silently adopted. |
| -func TestShapeMismatchComparesSteering(t *testing.T) { |
| +// TestShapeMismatchIgnoresSteering is the re-ruled adopt-side proof (plan |
| +// amendment b485ec5, review finding 2): Steering is RECORD-OWNED, the same |
| +// category Channel/Environment/Hibernate are — a hand-added route whose |
| +// steering disagrees with what the record describes must still be ADOPTED, |
| +// not refused, because comparing it here would permanently block adoption |
| +// for exactly the case adoption exists to serve (an operator's hand-added |
| +// route predating the console's steering PATCH door). convergeOwned stamps |
| +// the record's Steering onto the row unconditionally once adoption succeeds |
| +// — this test's first assertion is what proves that stamp isn't blocked. |
| +func TestShapeMismatchIgnoresSteering(t *testing.T) { |
| a := registry.Route{Kind: "instance", Steering: "latency"} |
| b := registry.Route{Kind: "instance", Steering: ""} |
| - if shapeMismatch(a, b) == "" { |
| - t.Fatal("differing steering adopted silently") |
| + if shapeMismatch(a, b) != "" { |
| + t.Fatalf("differing steering blocked adoption: %q, want a match — steering is record-owned", shapeMismatch(a, b)) |
| } |
| if shapeMismatch(a, registry.Route{Kind: "instance", Steering: "latency"}) != "" { |
| t.Fatal("equal steering must match") |
| } |
| + // A steering-only difference must not mask a REAL mismatch on another |
| + // column — shapeMismatch still has to walk past it to the next case. |
| + c := registry.Route{Kind: "service", Steering: "latency"} |
| + if reason := shapeMismatch(a, c); reason == "" { |
| + t.Fatal("a genuine kind mismatch was hidden behind the ignored steering column") |
| + } |
| } |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index 218d89c63..08ac9554b 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -216,6 +216,13 @@ type Route struct { |
| // SINGLE-BOX INERT as of this column's introduction: nothing reads it |
| // yet. See provision.Record.Steering's doc comment for the task-by-task |
| // rollout that gives it effect. |
| + // |
| + // Deliberately NOT in Fingerprint(): steering changes what DNS answers, |
| + // not what the edge serves; the converger is a host housekeeper duty |
| + // reading the registry directly (Tasks 4-6), never gated on the edge's |
| + // reload poll. SnapshotFingerprint still covers it, automatically, via |
| + // routeCols — the restore surface and the reload surface answer |
| + // different questions (SnapshotFingerprint's own doc comment). |
| Steering string |
| CreatedAt int64 |
| @@ -711,12 +718,17 @@ func (r *Registry) Delete(host string) error { |
| // by the caller instead of the ON CONFLICT clause. |
| // |
| // `steering` (multi-region ingress design Part 4) joins `addrs`/`health` as a |
| -// PLAIN overwrite, for the identical reason: provision.routeFor now carries |
| -// Record.Steering onto every route it mints, the owned-route branch converges |
| -// it through SetSteering rather than a full-row Upsert, and `carlos add` |
| -// carries no `-steering` flag at all — there is no caller that could reach |
| -// this Upsert with an unrelated Route literal and a stale zero value for a |
| -// column an operator meant to keep. |
| +// PLAIN overwrite: provision.routeFor carries Record.Steering onto every |
| +// route it mints, and the owned-route branch converges it through |
| +// SetSteering rather than a full-row Upsert. `carlos add` carries no |
| +// `-steering` flag, but it IS a caller that can reach this Upsert with an |
| +// unrelated Route literal — the same gap Addrs/Health had — so it is closed |
| +// the same way, at the CLI's own door: a re-add over an existing host copies |
| +// existing.Steering onto the literal before this call, beside Addrs/Health. |
| +// Without that copy, a bare re-add during the self-heal window after the |
| +// steering housekeeper duty (Task 6) is armed would silently clear a route's |
| +// Route53 latency records — a real incident this repo caught in review before |
| +// it shipped, not a hypothetical. |
| func (r *Registry) Upsert(t Route) error { |
| if t.CreatedAt == 0 { |
| t.CreatedAt = time.Now().Unix() |
| @@ -1580,17 +1592,15 @@ func (r *Registry) DeleteWildcardDomain(host string) error { |
| // "box-a"/"box-b"), which is precisely the blind spot hashing the values |
| // closes. |
| // |
| -// `steering` is hashed too (multi-region ingress design Part 4, plan Task 2): |
| -// it is a route's own member-facing routing opt-in, set through the same |
| -// console PATCH door channel/environment go through, and it must move the |
| -// digest for the identical reason those two do — a value that reaches the |
| -// bucket and the registry row but never trips the edge's own reload poll |
| -// stays invisible until an unrelated column happens to change too, exactly |
| -// the failure class db_path (#69) and addrs/health (#152) above were added |
| -// to close. Task 2 wires no edge consumer of this column yet — the field is |
| -// SINGLE-BOX INERT until Tasks 3-6 — but the reload signal is cheap to carry |
| -// now rather than teach Fingerprint about a column after something depends |
| -// on it moving. |
| +// `steering` (multi-region ingress design Part 4) is deliberately NOT hashed |
| +// here — see Route.Steering's own doc comment for the exclusion, stated in |
| +// the same place `unit`'s exclusion is (registry.go:93's rule, restated): |
| +// steering changes what DNS answers, not what the edge serves, and the |
| +// converger that acts on it is a host housekeeper duty reading the registry |
| +// directly, never gated on this poll (plan amendment b485ec5, re-ruling this |
| +// task's own first pass, which hashed it here on the wrong premise that |
| +// SnapshotFingerprint's routeCols-derived coverage extended to this function |
| +// too — it does not; this function's column list is hand-curated). |
| // |
| // The fleets and fleet_boxes tables are hashed into the SAME digest, each |
| // under its own section marker so a value can never migrate across sections |
| @@ -1629,7 +1639,7 @@ func (r *Registry) Fingerprint() (string, error) { |
| } |
| rows, err := db.Query(` |
| SELECT rowid, status, socket, addr, version, hibernate, channel, backing, tunnel, |
| - db_path, region, fleet_box, addrs, health, steering |
| + db_path, region, fleet_box, addrs, health |
| FROM routes ORDER BY rowid`) |
| if err != nil { |
| return "", err |
| @@ -1639,14 +1649,14 @@ FROM routes ORDER BY rowid`) |
| n := 0 |
| for rows.Next() { |
| var rowid, hibernate int64 |
| - var status, socket, addr, version, channel, backing, tunnel, dbPath, region, fleetBox, addrs, health, steering string |
| + var status, socket, addr, version, channel, backing, tunnel, dbPath, region, fleetBox, addrs, health string |
| if err := rows.Scan(&rowid, &status, &socket, &addr, &version, &hibernate, |
| - &channel, &backing, &tunnel, &dbPath, ®ion, &fleetBox, &addrs, &health, &steering); err != nil { |
| + &channel, &backing, &tunnel, &dbPath, ®ion, &fleetBox, &addrs, &health); err != nil { |
| return "", err |
| } |
| - fmt.Fprintf(h, "%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1e", |
| + fmt.Fprintf(h, "%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1e", |
| rowid, status, socket, addr, version, hibernate, channel, backing, tunnel, |
| - dbPath, region, fleetBox, addrs, health, steering) |
| + dbPath, region, fleetBox, addrs, health) |
| n++ |
| } |
| if err := rows.Err(); err != nil { |
diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go| index dbf68f355..8a48955e3 100644 |
| --- a/internal/registry/registry_test.go |
| +++ b/internal/registry/registry_test.go |
| @@ -242,34 +242,40 @@ func TestSetSteeringErrorsOnUnmatchedHost(t *testing.T) { |
| } |
| } |
| -// TestFingerprintSeesSteeringChange and TestSnapshotFingerprintSeesSteeringChange |
| -// are the plan's own demand: "Fingerprint AND SnapshotFingerprint change when |
| -// steering changes (pin, don't assume routeCols coverage)". SnapshotFingerprint |
| -// hashes routeCols generically (its own doc comment: "a column added to |
| -// routeCols is covered here automatically"), but Fingerprint's SELECT is a |
| -// hand-typed subset — nothing enforces that steering ever lands in it besides |
| -// this test. |
| -func TestFingerprintSeesSteeringChange(t *testing.T) { |
| +// TestFingerprintIgnoresSteering is the re-ruled pin (plan amendment |
| +// b485ec5, review finding 3): steering is deliberately OUT of Fingerprint, |
| +// the same idiom as caps/unit (Route.Steering's own doc comment) — it |
| +// changes what DNS answers, not what the edge serves, and the converger |
| +// that acts on it is a host housekeeper duty reading the registry directly, |
| +// never gated on the edge's reload poll. This task's own first pass hashed |
| +// it here; that was wrong, and this test is what would have caught it. |
| +func TestFingerprintIgnoresSteering(t *testing.T) { |
| reg := openTestRegistry(t) |
| if err := reg.Upsert(Route{Host: "m.example.com", Kind: "instance", Status: "ready"}); err != nil { |
| t.Fatal(err) |
| } |
| - fp1, err := reg.Fingerprint() |
| + before, err := reg.Fingerprint() |
| if err != nil { |
| t.Fatal(err) |
| } |
| if err := reg.SetSteering("m.example.com", "latency"); err != nil { |
| t.Fatal(err) |
| } |
| - fp2, err := reg.Fingerprint() |
| + after, err := reg.Fingerprint() |
| if err != nil { |
| t.Fatal(err) |
| } |
| - if fp1 == fp2 { |
| - t.Fatal("steering change invisible to Fingerprint — the edge would never notice a steering opt-in move") |
| + if after != before { |
| + t.Fatalf("Fingerprint moved on a steering change (%s -> %s) — the edge would reload for a change that does not touch what it serves", before, after) |
| } |
| } |
| +// TestSnapshotFingerprintSeesSteeringChange is UNCHANGED by the re-ruling: |
| +// SnapshotFingerprint hashes routeCols generically (its own doc comment: "a |
| +// column added to routeCols is covered here automatically"), and the |
| +// restore surface must reproduce steering even though the reload surface |
| +// (Fingerprint, above) deliberately ignores it — the two answer different |
| +// questions, per SnapshotFingerprint's own doc comment. |
| func TestSnapshotFingerprintSeesSteeringChange(t *testing.T) { |
| reg := openTestRegistry(t) |
| if err := reg.Upsert(Route{Host: "m.example.com", Kind: "instance", Status: "ready"}); err != nil { |