amadan
provision: fix shape-drift message and close empty-channel mint gap
Review findings on Task 3 (#130): - Shape-drift log now warns that `carlos remove` purges capability grants (registry.Delete's contract) instead of suggesting it as a routine fix, and prints the socket/db pair so a socket- or DBPath-only drift is distinguishable from a kind/addr/unit one. - Unit is only compared when the record states one, so a route stamped out-of-band via `carlos route -unit` (issue #110) no longer trips drift on every pass forever. - The read-path gate (beside the ValidateShape skip) now also refuses a record with an empty Channel: previously only the converge branch guarded against blanking a channel, so a channel-less record could still mint a route pinned to no channel — the #130 failure mode through the mint door instead of the converge door. Design: docs/superpowers/specs/2026-08-10-route-records-design.md §4.1
2 files changed,
+100
−3
internal/provision/reconcile.go+23 −3internal/provision/reconcile_test.go+77 −0
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 8aaa40341..9847fb897 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -159,6 +159,17 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| protect[host] = true |
| continue |
| } |
| + // ValidateShape deliberately does not inspect Channel (it validates |
| + // shape fields, and Channel is not one), but a channel-less record |
| + // must not become — or converge — a route pinned to no channel: |
| + // that is issue #130's failure mode arriving through a different |
| + // door than the one Task 3 closed. Protected like a corrupted body: |
| + // the record is there, it just cannot be used. |
| + if rec.Channel == "" { |
| + log.Printf("provision: skipping record %s: empty channel", key) |
| + protect[host] = true |
| + continue |
| + } |
| if rec.Region != cfg.Region || !poolServed(cfg.Pools, rec.Pool) { |
| continue // another agent's jurisdiction, not this pass's business |
| } |
| @@ -301,12 +312,21 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| log.Printf("provision: converge seed on %s: %v", rec.Host, err) |
| } |
| } |
| + // Unit is compared only when the RECORD states one: `carlos |
| + // route -unit` (issue #110) is the supported way to stamp the |
| + // unit owning a route's process out of band, and a record that |
| + // never spoke to Unit must not treat that stamp as drift — or |
| + // this would fire on a healthy, correctly-stamped route every |
| + // pass, forever. |
| if want := routeFor(rec, cfg.DataDir, seed); want.Kind != existing.Kind || |
| - want.Addr != existing.Addr || want.Unit != existing.Unit || |
| + want.Addr != existing.Addr || (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) — not converged in place; `carlos remove %s` and let the next pass re-mint", |
| - rec.Host, want.Kind, existing.Kind, want.Addr, existing.Addr, want.Unit, existing.Unit, rec.Host) |
| + "(kind %q/%q addr %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, |
| + want.Socket, existing.Socket, want.DBPath, existing.DBPath, rec.Host) |
| } |
| default: |
| log.Printf("provision: %s exists but was not minted by this pass (app %q acct %q) — leaving it alone", |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index d94fc7378..27609993b 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -343,6 +343,38 @@ func TestReconcileConvergesChannel(t *testing.T) { |
| } |
| } |
| +// The converge guard is load-bearing: an empty Channel means a malformed |
| +// record, not "clear the channel" — an empty channel would pin the route to |
| +// nothing, exactly the #130 failure mode. A route already provisioned on a |
| +// real channel must keep it when its record goes channel-less, not have it |
| +// blanked. |
| +func TestReconcileConvergeGuardEmptyChannelLeavesRouteAlone(t *testing.T) { |
| + s, sqid := setup(t) |
| + rec := acmeRecord(sqid) |
| + 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) |
| + } |
| + before := *mustGet(t, reg, "acme.go.tito.io") |
| + if before.Channel != "stable" { |
| + t.Fatalf("fixture's route didn't channel as expected: %+v", before) |
| + } |
| + |
| + rec.Channel = "" |
| + 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, "acme.go.tito.io") |
| + if after != before { |
| + t.Errorf("channel-less record changed the row: before %+v after %+v", before, after) |
| + } |
| +} |
| + |
| func TestReconcileConvergesEnvironment(t *testing.T) { |
| s, sqid := setup(t) |
| rec := acmeRecord(sqid) |
| @@ -674,6 +706,51 @@ func TestReconcileSkipsRecordWithInvalidShape(t *testing.T) { |
| mustGet(t, reg, bad.Host) |
| } |
| +// ValidateShape deliberately does not inspect Channel (it validates shape |
| +// fields, Channel is not one), but a channel-less record must not become a |
| +// route pinned to no channel — that is issue #130's failure mode (a route |
| +// whose channel resolves to nothing, waking with "no adoptable head") |
| +// arriving through the MINT door instead of the converge door Task 3 |
| +// closed. Skipped loudly at the read-path gate, and protected from removal |
| +// for the same reason a corrupted body is — an unusable record is not a |
| +// retired one. |
| +func TestReconcileSkipsRecordWithEmptyChannel(t *testing.T) { |
| + s, sqid := setup(t) |
| + bad := acmeRecord(sqid) |
| + bad.Host, bad.Slug, bad.Channel = "no-channel.go.tito.io", "no-channel", "" |
| + body, err := json.Marshal(bad) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := s.Put(ctx, RecordKey(sqid, "tito", bad.Host), body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + 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) |
| + } |
| + if rt, err := reg.Get(bad.Host); err != nil || rt != nil { |
| + t.Errorf("minted a route from a channel-less record: %+v %v", rt, err) |
| + } |
| + // The good neighbour in the same pass is untouched by the skip. |
| + mustGet(t, reg, "acme.go.tito.io") |
| + |
| + // And a route that somehow already carries that host is not treated as |
| + // retired: the record is there, it just cannot be used. |
| + if err := reg.Upsert(registry.Route{Host: bad.Host, Kind: "instance", App: "tito", |
| + Status: "ready", Provisioned: true, Region: "ie", Pool: "tito", Account: sqid}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, bad.Host) |
| +} |
| + |
| // 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 |