amadan
steering: fix review findings — records are the deployment's truth
FINDING 1 (Important): the duty derived its desired set from the LEASE HOLDER'S LOCAL REGISTRY, but routes are not uniformly provisioned — an everywhereClass record (exec/unit/socket kinds) is homed to ONE box, so that host is absent from every other box's registry. A housekeeper lease migration therefore made a home-boxed steered host's records managed-but-unwanted, and the pass deleted them: NXDOMAIN for a live host until the lease came back, with the shrink guard silent at 2-of-8. DNS bound to housekeeper elections. The plan's own Task 4 Consumes line already said where steered hosts come from: BUCKET RECORDS. New provision.ListAllRecords does the deployment-global, account-qualified walk (accounts/ for the trusted sqid set, apps/, ParseRecordKey, Get, decode) with FAIL-CLOSED semantics — the deliberate difference from Reconcile's own walk, which tolerates a partial read because it must still mint. steeringPass now reads records, not the registry, and the registry is gone from the steering path entirely (steeringLoop takes no argument). The mapping is minimal and verified field by field against the fold: Host, Steering, Pool are all desiredState reads. H2's split applies unchanged, one layer deeper: an accounts/apps listing failure, an unreadable account manifest (which silently hides that account's whole record set) and any record Get failure are all FATAL; a record that reads but will not decode is skipped and logged. FINDING 2 (Minor): unexport Converge -> converge. ConvergeDeployment is the seam; the exported form was surface with no caller. Stale "nothing calls it" headers corrected in the same pass. FINDING 3 (Minor): steeringDuty declines on a nil pass as well as an empty zone id — one token of totality on a production DNS write path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9 files changed,
+410
−106
internal/host/agent.go+1 −1internal/host/steering_loop.go+67 −17internal/host/steering_loop_test.go+239 −61internal/provision/reconcile.go+72 −0internal/steering/deployment.go+4 −4internal/steering/deployment_test.go+2 −2internal/steering/route53.go+12 −9internal/steering/route53_test.go+8 −8internal/steering/steering.go+5 −4
diff --git a/internal/host/agent.go b/internal/host/agent.go| index f1ba38497..f8bd49872 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -333,7 +333,7 @@ func RunAgent(args []string) { |
| // own loop for mintMeshKeyLoop's exact reason, and dormant on every box |
| // today: CARLOS_STEERING_ZONE_ID unset means one log line at arm and |
| // nothing else, ever. See steering_loop.go. |
| - go steeringLoop(reg) |
| + go steeringLoop() |
| // The sweep reads the registry only through this closure — the stamped |
| // bespoke units to sample beside the carlos- ones, re-read each tick. |
| go metricsLoop(cgm.Root(), func() []string { return stampedUnitNames(reg) }) |
diff --git a/internal/host/steering_loop.go b/internal/host/steering_loop.go| index 743f6a43f..6b2379d99 100644 |
| --- a/internal/host/steering_loop.go |
| +++ b/internal/host/steering_loop.go |
| @@ -15,6 +15,7 @@ import ( |
| "strings" |
| "time" |
| + "github.com/carlosframework/platform/internal/provision" |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| "github.com/carlosframework/platform/internal/steering" |
| @@ -67,24 +68,67 @@ func loadEdgesFileBodies(ctx context.Context, s release.Store) ([][]byte, error) |
| return out, nil |
| } |
| -// steeringPass is one converge attempt: read both inputs, then converge. |
| +// steeredRoutesFromRecords reads the deployment's steered hosts from the |
| +// BUCKET RECORDS — provision.ListAllRecords — and never from this box's |
| +// registry. |
| // |
| -// Both reads fail closed (see loadEdgesFileBodies) and BOTH must succeed |
| -// before converge is called even once — a routes query that failed is not |
| -// "no steered hosts" any more than a failed listing is "no edges". converge |
| -// is injected rather than called directly so this contract is testable |
| -// without an AWS client: the real one is the closure steeringLoop builds |
| -// around steering.ConvergeDeployment. |
| +// The registry is the wrong source and the bug is not theoretical (review |
| +// finding, 2026-08-23). Routes are NOT uniformly provisioned: an |
| +// everywhereClass record (exec/unit/socket kinds) is homed to ONE box, so |
| +// that host is absent from every other box's registry. This duty runs on |
| +// whichever box holds the housekeeper lease — so a steered, home-boxed host |
| +// read from the local registry would be present in the desired set while the |
| +// lease sat on its home box and ABSENT the moment the lease moved. Absent |
| +// means managed-but-unwanted, which means DELETED: a live host goes NXDOMAIN |
| +// until the lease comes back, and the shrink guard does not fire at 2-of-8. |
| +// DNS bound to housekeeper elections is not a system anyone can operate. |
| +// The records are the deployment's truth, identical from every box. |
| // |
| -// listRoutes is the registry's List, injected for the same reason. |
| -func steeringPass(ctx context.Context, s release.Store, listRoutes func() ([]registry.Route, error), converge func(context.Context, []registry.Route, [][]byte) error) error { |
| +// The mapping is deliberately MINIMAL: desiredState (internal/steering) |
| +// reads exactly three fields off a registry.Route — Steering (the "latency" |
| +// opt-in), Host (the DNS name, and the wildcard check) and Pool (steering |
| +// requires the shared pool) — verified field by field against that fold, so |
| +// carrying more would be inventing agreement the fold does not need. |
| +// |
| +// Records with Steering == "" are dropped HERE rather than passed down: the |
| +// fold skips them SILENTLY anyway (steeredHosts' `case "": continue`), so |
| +// nothing an operator would see is lost. Every other value — including |
| +// invalid ones, wildcard hosts and pool-scoped rows — is passed through |
| +// untouched so the fold's own loud, documented refusals still fire. This |
| +// loader must not become a second, quieter policy gate. |
| +func steeredRoutesFromRecords(ctx context.Context, s release.Store) ([]registry.Route, error) { |
| + recs, err := provision.ListAllRecords(ctx, s) |
| + if err != nil { |
| + return nil, err |
| + } |
| + out := make([]registry.Route, 0, len(recs)) |
| + for _, rec := range recs { |
| + if rec.Steering == "" { |
| + continue |
| + } |
| + out = append(out, registry.Route{Host: rec.Host, Steering: rec.Steering, Pool: rec.Pool}) |
| + } |
| + return out, nil |
| +} |
| + |
| +// steeringPass is one converge attempt: read both inputs from the STORE, |
| +// then converge. The registry is not consulted at all — see |
| +// steeredRoutesFromRecords. |
| +// |
| +// Both reads fail closed (see loadEdgesFileBodies and ListAllRecords) and |
| +// BOTH must succeed before converge is called even once — a records read |
| +// that failed is not "no steered hosts" any more than a failed edges listing |
| +// is "no edges". converge is injected rather than called directly so this |
| +// contract is testable without an AWS client: the real one is the closure |
| +// steeringLoop builds around steering.ConvergeDeployment. |
| +func steeringPass(ctx context.Context, s release.Store, converge func(context.Context, []registry.Route, [][]byte) error) error { |
| edges, err := loadEdgesFileBodies(ctx, s) |
| if err != nil { |
| return err |
| } |
| - routes, err := listRoutes() |
| + routes, err := steeredRoutesFromRecords(ctx, s) |
| if err != nil { |
| - return fmt.Errorf("steering: list routes: %w", err) |
| + return err |
| } |
| return converge(ctx, routes, edges) |
| } |
| @@ -93,9 +137,9 @@ func steeringPass(ctx context.Context, s release.Store, listRoutes func() ([]reg |
| // |
| // DORMANCY (zoneID == "") is enforced here, at the top, and it is a |
| // structural claim rather than a promise: pass is the closure holding the |
| -// store handle, the registry and the AWS client, so a dormant duty that never |
| -// calls it makes zero reads and zero API calls by construction. Every box in |
| -// production today is in exactly this state. |
| +// store handle and the AWS client — every read and every API call this duty |
| +// can make — so a dormant duty that never calls it does neither, by |
| +// construction. Every box in production today is in exactly this state. |
| // |
| // DUTY-STATUS HONESTY — read this before trusting a green tick. A nil return |
| // from this duty does NOT mean the zone converged. internal/steering's two |
| @@ -115,9 +159,15 @@ func steeringPass(ctx context.Context, s release.Store, listRoutes func() ([]reg |
| // duty was considered and left out of this task on purpose — it changes the |
| // housekeeperDuty contract for every duty, and the journal already carries |
| // the truth. |
| +// The pass == nil half of the guard is one token of totality on a path that |
| +// writes production DNS: steeringLoop leaves pass nil in exactly the branch |
| +// that leaves zoneID empty, so the two conditions are the same condition |
| +// today — but "today" is doing load-bearing work in that sentence, and the |
| +// failure mode if it ever stopped being true is a nil call that panics the |
| +// agent's goroutine rather than a duty that declines to run. |
| func steeringDuty(zoneID string, pass func(context.Context) error) housekeeperDuty { |
| return housekeeperDuty{"steering converge", func(ctx context.Context) error { |
| - if zoneID == "" { |
| + if zoneID == "" || pass == nil { |
| return nil |
| } |
| return pass(ctx) |
| @@ -151,7 +201,7 @@ func steeringDuty(zoneID string, pass func(context.Context) error) housekeeperDu |
| // plus one Route53 health check per EIP, which is what adds rung-2 |
| // withdrawal for steered hosts even single-box. Nothing about it needs a |
| // second region to be correct. |
| -func steeringLoop(reg *registry.Registry) { |
| +func steeringLoop() { |
| ctx := context.Background() |
| if shouldSkipPublisherElection(os.Getenv) { |
| log.Printf("agent: steering loop is a platform-box job — skipped on a fleet box") |
| @@ -179,7 +229,7 @@ func steeringLoop(reg *registry.Registry) { |
| } |
| appsDomain := strings.TrimSpace(os.Getenv("PLATFORM_APPS_DOMAIN")) |
| pass = func(ctx context.Context) error { |
| - return steeringPass(ctx, store, reg.List, func(ctx context.Context, routes []registry.Route, edges [][]byte) error { |
| + return steeringPass(ctx, store, func(ctx context.Context, routes []registry.Route, edges [][]byte) error { |
| return steering.ConvergeDeployment(ctx, client, zoneID, routes, edges, appsDomain) |
| }) |
| } |
diff --git a/internal/host/steering_loop_test.go b/internal/host/steering_loop_test.go| index 60c923620..3cf390200 100644 |
| --- a/internal/host/steering_loop_test.go |
| +++ b/internal/host/steering_loop_test.go |
| @@ -2,24 +2,36 @@ package host |
| import ( |
| "context" |
| + "encoding/json" |
| "errors" |
| + "strconv" |
| "strings" |
| "testing" |
| + "github.com/carlosframework/platform/internal/domains" |
| + "github.com/carlosframework/platform/internal/provision" |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| // ---- fakes ----------------------------------------------------------------- |
| -// failingListStore is a store whose control/edges/ LISTING is broken — the |
| -// control-bucket blip that must never reach the converger as "no edges". |
| +// failingListStore breaks the LISTING of one prefix — the control-bucket |
| +// blip that must never reach the converger as "no edges" or "no records". |
| +// Prefix-scoped because the pass now lists three of them (control/edges/, |
| +// accounts/, apps/) and each is a separate fail-closed direction. |
| type failingListStore struct { |
| release.Store |
| - err error |
| + prefix string |
| + err error |
| } |
| -func (f failingListStore) List(context.Context, string) ([]string, error) { return nil, f.err } |
| +func (f failingListStore) List(ctx context.Context, prefix string) ([]string, error) { |
| + if prefix == f.prefix { |
| + return nil, f.err |
| + } |
| + return f.Store.List(ctx, prefix) |
| +} |
| // failingGetStore lists fine but refuses ONE named key — the half-read that |
| // would otherwise fold to a short desired set and delete the missing box's |
| @@ -64,23 +76,61 @@ func steeringTestStore(t *testing.T, files map[string][]byte) release.Store { |
| return s |
| } |
| -func okRoutes(routes ...registry.Route) func() ([]registry.Route, error) { |
| - return func() ([]registry.Route, error) { return routes, nil } |
| +// seedAccount writes the account manifest ListAllRecords' known-account |
| +// guard (provision.ParseRecordKey) requires, and returns its sqid — the |
| +// frozen encoding of the id, via domains.Sqid, never a literal. |
| +func seedAccount(t *testing.T, s release.Store, id int64, name string) string { |
| + t.Helper() |
| + sqid := domains.Sqid(id) |
| + body := []byte(`{"id":` + strconv.FormatInt(id, 10) + `,"sqid":"` + sqid + |
| + `","name":"` + name + `","created_at":"2026-08-02T09:00:00Z"}`) |
| + if _, err := s.Put(context.Background(), "accounts/"+sqid+"/account.json", body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + return sqid |
| +} |
| + |
| +// seedRecord writes one instance record at its canonical key. |
| +func seedRecord(t *testing.T, s release.Store, sqid, app, host, steering, pool string) { |
| + t.Helper() |
| + rec := provision.Record{ |
| + Schema: 1, Host: host, Slug: app, App: app, Account: sqid, |
| + Pool: pool, Channel: "canary/main", Steering: steering, |
| + } |
| + body, err := json.Marshal(rec) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := s.Put(context.Background(), provision.RecordKey(sqid, app, host), body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| } |
| -// ---- the pass: fail-closed reads (H2) -------------------------------------- |
| +// ---- the pass: what it reads, and failing closed (H2) ---------------------- |
| -// A healthy pass hands the converger every edges file body under the prefix, |
| -// in listing order, plus the registry's routes. |
| -func TestSteeringPassReadsEdgesFilesAndRoutes(t *testing.T) { |
| - quietLog(t) |
| +// steeringWorld seeds a whole small deployment: two boxes' edges files, one |
| +// account, and three records — one steered, one unsteered, one steered under |
| +// a pool. |
| +func steeringWorld(t *testing.T) (release.Store, string) { |
| + t.Helper() |
| s := steeringTestStore(t, map[string][]byte{ |
| "control/edges/dublin.json": []byte(`{"v":1,"box":"dublin"}`), |
| "control/edges/oregon.json": []byte(`{"v":1,"box":"oregon"}`), |
| }) |
| + sqid := seedAccount(t, s, 12, "Tito") |
| + seedRecord(t, s, sqid, "app", "app.example.com", "latency", "") |
| + seedRecord(t, s, sqid, "app", "plain.example.com", "", "") |
| + return s, sqid |
| +} |
| + |
| +// The healthy pass: edges bodies in sorted listing order, and routes derived |
| +// from the BUCKET RECORDS — not from any registry. |
| +func TestSteeringPassReadsEdgesFilesAndRecords(t *testing.T) { |
| + quietLog(t) |
| + s, _ := steeringWorld(t) |
| rec := &convergeRecorder{} |
| - if err := steeringPass(context.Background(), s, okRoutes(registry.Route{Host: "app.example.com"}), rec.converge); err != nil { |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| t.Fatalf("steeringPass: %v", err) |
| } |
| if rec.calls != 1 { |
| @@ -93,22 +143,77 @@ func TestSteeringPassReadsEdgesFilesAndRoutes(t *testing.T) { |
| if !strings.Contains(string(rec.edges[0]), "dublin") || !strings.Contains(string(rec.edges[1]), "oregon") { |
| t.Fatalf("edges = %q, want dublin then oregon (sorted)", rec.edges) |
| } |
| - if len(rec.routes) != 1 || rec.routes[0].Host != "app.example.com" { |
| - t.Fatalf("routes = %+v, want the registry's one row", rec.routes) |
| + // Only the steered record becomes a route; the unsteered one is dropped |
| + // here because the fold would skip it silently anyway. |
| + if len(rec.routes) != 1 { |
| + t.Fatalf("routes = %+v, want only the steered record", rec.routes) |
| + } |
| + got := rec.routes[0] |
| + if got.Host != "app.example.com" || got.Steering != "latency" || got.Pool != "" { |
| + t.Fatalf("route = %+v, want the three fields the fold reads", got) |
| } |
| } |
| -// H2, half one: a broken LISTING must not converge. A converger handed "no |
| -// edges" folds to an empty desired set; Converge's own refusal would catch |
| -// that particular case, but a listing that came back SHORT would not be |
| -// caught at all — so the read fails the duty instead, before any of it. |
| -func TestSteeringPassFailsClosedOnListError(t *testing.T) { |
| +// FINDING 1's pin (review, 2026-08-23). The steered host here is the |
| +// HOME-BOXED class — an instance record with no Addr, which everywhereClass |
| +// materializes on ONE box only, so it is absent from every other box's |
| +// registry. It must still produce a record from whichever box holds the |
| +// housekeeper lease, because the lease moves and DNS must not. |
| +// |
| +// The proof is structural as much as behavioural: steeringPass takes no |
| +// registry at all, so there is no local view for the answer to depend on. |
| +// Before the fix this test could not even have been written — the pass would |
| +// have needed a registry, and the host would have been missing from it. |
| +func TestSteeringPassSteersHomeBoxedRecordWithNoLocalRegistry(t *testing.T) { |
| + quietLog(t) |
| + s := steeringTestStore(t, map[string][]byte{ |
| + "control/edges/dublin.json": []byte(`{"v":1,"box":"dublin"}`), |
| + }) |
| + sqid := seedAccount(t, s, 12, "Tito") |
| + // No Addr, no Unit, default hibernation: home-boxed, exactly the class |
| + // the rollout's stage-5 checklist steers. |
| + seedRecord(t, s, sqid, "exec-app", "home.example.com", "latency", "") |
| + rec := &convergeRecorder{} |
| + |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| + t.Fatalf("steeringPass: %v", err) |
| + } |
| + if len(rec.routes) != 1 || rec.routes[0].Host != "home.example.com" { |
| + t.Fatalf("routes = %+v, want the home-boxed steered host — a lease migration must not withdraw it", rec.routes) |
| + } |
| +} |
| + |
| +// The loader drops Steering=="" (the fold skips those silently anyway) but |
| +// must NOT become a second, quieter policy gate: an invalid value, a |
| +// wildcard host and a pool-scoped row all go DOWN to the fold, whose loud |
| +// documented refusals are the operator's only signal that they exist. |
| +func TestSteeringPassPassesNonLatencySteeringValuesDown(t *testing.T) { |
| + quietLog(t) |
| + s := steeringTestStore(t, map[string][]byte{ |
| + "control/edges/dublin.json": []byte(`{"v":1,"box":"dublin"}`), |
| + }) |
| + sqid := seedAccount(t, s, 12, "Tito") |
| + seedRecord(t, s, sqid, "app", "weird.example.com", "weighted", "") |
| + seedRecord(t, s, sqid, "app", "pooled.example.com", "latency", "eu") |
| + rec := &convergeRecorder{} |
| + |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| + t.Fatalf("steeringPass: %v", err) |
| + } |
| + if len(rec.routes) != 2 { |
| + t.Fatalf("routes = %+v, want both rows delivered for the fold to refuse loudly", rec.routes) |
| + } |
| +} |
| + |
| +// H2, direction one: a broken edges LISTING must not converge. |
| +func TestSteeringPassFailsClosedOnEdgesListError(t *testing.T) { |
| quietLog(t) |
| boom := errors.New("bucket down") |
| - s := failingListStore{Store: steeringTestStore(t, nil), err: boom} |
| + base, _ := steeringWorld(t) |
| + s := failingListStore{Store: base, prefix: "control/edges/", err: boom} |
| rec := &convergeRecorder{} |
| - err := steeringPass(context.Background(), s, okRoutes(), rec.converge) |
| + err := steeringPass(context.Background(), s, rec.converge) |
| if err == nil { |
| t.Fatal("steeringPass returned nil on a failed edges listing") |
| } |
| @@ -120,24 +225,17 @@ func TestSteeringPassFailsClosedOnListError(t *testing.T) { |
| } |
| } |
| -// H2, half two, and the one the shrink guard was written for: the listing |
| -// succeeded, so the pass KNOWS how many boxes there are — and one of their |
| -// files would not come back. Converging on the rest is exactly the short read |
| -// that deletes a live edge's records. |
| -func TestSteeringPassFailsClosedOnGetError(t *testing.T) { |
| +// H2, direction two, and the one the shrink guard was written for: the |
| +// listing succeeded, so the pass KNOWS how many boxes there are — and one of |
| +// their files would not come back. |
| +func TestSteeringPassFailsClosedOnEdgesGetError(t *testing.T) { |
| quietLog(t) |
| boom := errors.New("object unreadable") |
| - s := failingGetStore{ |
| - Store: steeringTestStore(t, map[string][]byte{ |
| - "control/edges/dublin.json": []byte(`{"v":1,"box":"dublin"}`), |
| - "control/edges/oregon.json": []byte(`{"v":1,"box":"oregon"}`), |
| - }), |
| - bad: "control/edges/oregon.json", |
| - err: boom, |
| - } |
| + base, _ := steeringWorld(t) |
| + s := failingGetStore{Store: base, bad: "control/edges/oregon.json", err: boom} |
| rec := &convergeRecorder{} |
| - err := steeringPass(context.Background(), s, okRoutes(), rec.converge) |
| + err := steeringPass(context.Background(), s, rec.converge) |
| if err == nil { |
| t.Fatal("steeringPass returned nil with one edges file unreadable") |
| } |
| @@ -152,38 +250,104 @@ func TestSteeringPassFailsClosedOnGetError(t *testing.T) { |
| } |
| } |
| -// H2, half three: the registry read is the other input, and the same rule |
| -// holds — a routes query that failed is not "no steered hosts". |
| -func TestSteeringPassFailsClosedOnRegistryError(t *testing.T) { |
| - quietLog(t) |
| - boom := errors.New("registry locked") |
| - s := steeringTestStore(t, map[string][]byte{"control/edges/dublin.json": []byte(`{"v":1,"box":"dublin"}`)}) |
| +// H2, direction three, re-pointed at the RECORDS read (FINDING 1): the |
| +// records are now the routes half of the fold, so every way that read can |
| +// come back short is a way a live steered host gets deleted. Four of them, |
| +// each fatal. |
| +func TestSteeringPassFailsClosedOnRecordsRead(t *testing.T) { |
| + boom := errors.New("bucket down") |
| + cases := []struct { |
| + name string |
| + wrap func(release.Store) release.Store |
| + named string |
| + }{ |
| + { |
| + // Without accounts/, ParseRecordKey's known-account guard rejects |
| + // EVERY record key — the fold would see no steered hosts at all. |
| + name: "accounts listing fails", |
| + wrap: func(s release.Store) release.Store { |
| + return failingListStore{Store: s, prefix: "accounts/", err: boom} |
| + }, |
| + }, |
| + { |
| + // The subtle one: ONE account's manifest is unreadable, so only |
| + // THAT account's records vanish. Silent, partial, and exactly the |
| + // shape that deletes one tenant's DNS. |
| + name: "an account manifest is unreadable", |
| + wrap: func(s release.Store) release.Store { |
| + return failingGetStore{Store: s, bad: "accounts/" + domains.Sqid(12) + "/account.json", err: boom} |
| + }, |
| + }, |
| + { |
| + name: "apps listing fails", |
| + wrap: func(s release.Store) release.Store { |
| + return failingListStore{Store: s, prefix: "apps/", err: boom} |
| + }, |
| + }, |
| + { |
| + name: "a record object will not Get", |
| + wrap: func(s release.Store) release.Store { |
| + return failingGetStore{Store: s, bad: provision.RecordKey(domains.Sqid(12), "app", "app.example.com"), err: boom} |
| + }, |
| + named: "app.example.com", |
| + }, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + quietLog(t) |
| + base, _ := steeringWorld(t) |
| + rec := &convergeRecorder{} |
| + |
| + err := steeringPass(context.Background(), c.wrap(base), rec.converge) |
| + if err == nil { |
| + t.Fatalf("steeringPass returned nil when %s", c.name) |
| + } |
| + if rec.calls != 0 { |
| + t.Fatalf("converge ran %d times, want 0 — a short records read must never converge", rec.calls) |
| + } |
| + if c.named != "" && !strings.Contains(err.Error(), c.named) { |
| + t.Fatalf("err = %v, want the unreadable record named", err) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// The content-vs-visibility split, records half: a record that READS but does |
| +// not decode is skipped and logged, and the pass converges on the rest. One |
| +// malformed body must not blind the deployment to every other steered host. |
| +func TestSteeringPassSkipsUnparseableRecord(t *testing.T) { |
| + logs := captureLog(t) |
| + s, sqid := steeringWorld(t) |
| + if _, err := s.Put(context.Background(), provision.RecordKey(sqid, "app", "broken.example.com"), []byte("{not json"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| rec := &convergeRecorder{} |
| - err := steeringPass(context.Background(), s, func() ([]registry.Route, error) { return nil, boom }, rec.converge) |
| - if err == nil { |
| - t.Fatal("steeringPass returned nil on a failed registry read") |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| + t.Fatalf("steeringPass: %v — an unparseable record is not a read failure", err) |
| } |
| - if !errors.Is(err, boom) { |
| - t.Fatalf("err = %v, want it to wrap the registry error", err) |
| + if rec.calls != 1 { |
| + t.Fatalf("converge calls = %d, want 1", rec.calls) |
| } |
| - if rec.calls != 0 { |
| - t.Fatalf("converge ran %d times after a failed registry read, want 0", rec.calls) |
| + if len(rec.routes) != 1 || rec.routes[0].Host != "app.example.com" { |
| + t.Fatalf("routes = %+v, want the healthy steered record still folded", rec.routes) |
| + } |
| + if !strings.Contains(logs(), "broken.example.com") { |
| + t.Fatalf("log = %q, want the malformed record named", logs()) |
| } |
| } |
| -// The distinction H2 turns on: a file that READS but does not PARSE is a |
| -// CONTENT problem, and the host loader must not pre-judge it — the body goes |
| -// down to the fold, which skips it and says so (steering's own test). Only |
| -// READ failures fail the duty. |
| +// The same split, edges half: the host loader must NOT pre-judge content — |
| +// a body that will not parse is delivered verbatim and the fold decides. |
| func TestSteeringPassPassesUnparseableBodiesDown(t *testing.T) { |
| quietLog(t) |
| s := steeringTestStore(t, map[string][]byte{ |
| "control/edges/dublin.json": []byte("{not json"), |
| }) |
| + seedAccount(t, s, 12, "Tito") |
| rec := &convergeRecorder{} |
| - if err := steeringPass(context.Background(), s, okRoutes(), rec.converge); err != nil { |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| t.Fatalf("steeringPass: %v — an unparseable body is not a read failure", err) |
| } |
| if rec.calls != 1 { |
| @@ -194,9 +358,9 @@ func TestSteeringPassPassesUnparseableBodiesDown(t *testing.T) { |
| } |
| } |
| -// Objects under the prefix that this deployment's edges loop did not write |
| -// are not read at all — the same key-shape filter internal/edge's homeMap |
| -// applies. A stray object must not be able to fail the pass forever. |
| +// Objects under the edges prefix that this deployment's edges loop did not |
| +// write are not read at all — the same key-shape filter internal/edge's |
| +// homeMap applies. A stray object must not be able to fail the pass forever. |
| func TestSteeringPassIgnoresForeignKeysUnderThePrefix(t *testing.T) { |
| quietLog(t) |
| s := steeringTestStore(t, map[string][]byte{ |
| @@ -204,9 +368,10 @@ func TestSteeringPassIgnoresForeignKeysUnderThePrefix(t *testing.T) { |
| "control/edges/README.txt": []byte("hello"), |
| "control/edges/nested/inner.json": []byte(`{"v":1,"box":"nested"}`), |
| }) |
| + seedAccount(t, s, 12, "Tito") |
| rec := &convergeRecorder{} |
| - if err := steeringPass(context.Background(), s, okRoutes(), rec.converge); err != nil { |
| + if err := steeringPass(context.Background(), s, rec.converge); err != nil { |
| t.Fatalf("steeringPass: %v", err) |
| } |
| if len(rec.edges) != 1 { |
| @@ -232,6 +397,19 @@ func TestSteeringDutyDormantDoesNoWork(t *testing.T) { |
| } |
| } |
| +// FINDING 3 (review, 2026-08-23): totality on a production DNS write path. |
| +// A zone id with no pass is unreachable today — steeringLoop leaves pass nil |
| +// in exactly the branch that leaves zoneID empty — but if that ever stopped |
| +// being true, the duty must decline rather than panic the agent's goroutine |
| +// on a nil call. |
| +func TestSteeringDutyWithNoPassDeclines(t *testing.T) { |
| + quietLog(t) |
| + duty := steeringDuty("Z0ABCDEF1234", nil) |
| + if err := duty.run(context.Background()); err != nil { |
| + t.Fatalf("duty with a nil pass returned %v, want nil (and no panic)", err) |
| + } |
| +} |
| + |
| func TestSteeringDutyRunsThePassWhenZoneSet(t *testing.T) { |
| quietLog(t) |
| ran := 0 |
| @@ -278,7 +456,7 @@ func TestSteeringLoopIsAPlatformBoxJob(t *testing.T) { |
| t.Setenv("CARLOS_DEPLOYMENT_BUCKET", "") |
| t.Setenv("CARLOS_DEPLOYMENT_DIR", "") |
| - steeringLoop(nil) // must return; a fleet box never reaches the loop |
| + steeringLoop() // must return; a fleet box never reaches the loop |
| if got := logs(); !strings.Contains(got, skipLine) { |
| t.Fatalf("log = %q, want the skip line verbatim", got) |
| @@ -300,7 +478,7 @@ func TestSteeringLoopArmsWithoutReleasePubkey(t *testing.T) { |
| t.Setenv("CARLOS_STEERING_ZONE_ID", "") |
| buf := captureRacyLog(t) |
| - go steeringLoop(nil) |
| + go steeringLoop() |
| waitFor(t, "the steering loop to arm despite no CARLOS_RELEASE_PUBKEY", func() bool { |
| return strings.Contains(buf.String(), "agent: steering loop armed") |
| @@ -317,7 +495,7 @@ func TestSteeringLoopDormantWhenZoneUnset(t *testing.T) { |
| t.Setenv("CARLOS_STEERING_ZONE_ID", "") |
| buf := captureRacyLog(t) |
| - go steeringLoop(nil) |
| + go steeringLoop() |
| waitFor(t, "the dormant line", func() bool { |
| return strings.Contains(buf.String(), "steering: zone unset — dormant") |
| @@ -336,7 +514,7 @@ func TestSteeringLoopDormantWhenZoneMalformed(t *testing.T) { |
| t.Setenv("CARLOS_STEERING_ZONE_ID", "/hostedzone/Z0ABCDEF1234") |
| buf := captureRacyLog(t) |
| - go steeringLoop(nil) |
| + go steeringLoop() |
| waitFor(t, "the dormant line for a malformed zone id", func() bool { |
| return strings.Contains(buf.String(), "steering: zone unset — dormant") |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index b8c98e4af..f7706bb18 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -927,6 +927,78 @@ func knownAccounts(ctx context.Context, s release.Store) (known map[string]bool, |
| return known, readFailed, nil |
| } |
| +// ListAllRecords enumerates EVERY instance record in the deployment — the |
| +// same account-qualified walk Reconcile does (list accounts/ for the trusted |
| +// sqid set, list apps/, ParseRecordKey, Get, decode), exported for callers |
| +// that need deployment-GLOBAL route facts rather than one app's. |
| +// |
| +// It exists because the registry is not deployment-global truth. Routes are |
| +// provisioned per box: an everywhereClass record (exec/unit/socket kinds) is |
| +// homed to ONE box, so that host is simply absent from every other box's |
| +// registry. Any deployment-wide decision derived from one box's registry |
| +// therefore changes meaning when the work moves to a different box — which, |
| +// for DNS steering (internal/steering), meant a housekeeper lease migration |
| +// could delete a live steered host's records and NXDOMAIN it until the lease |
| +// came back. The records ARE the deployment's truth; this is how to read all |
| +// of them. |
| +// |
| +// FAIL-CLOSED, and that is the deliberate difference from Reconcile's own |
| +// walk. Reconcile tolerates a partial read (it logs, sets readFailed, and |
| +// vetoes only its removal phase) because it must still MINT on a degraded |
| +// pass. A caller deriving global state has no such split: a short list is |
| +// indistinguishable from records having been deleted, so every read failure |
| +// is an error and no records are returned — |
| +// |
| +// - accounts/ listing fails, or ANY account manifest is unreadable, |
| +// malformed, or disagrees with its own sqid: that account's records all |
| +// fail ParseRecordKey's known-account guard and would vanish silently |
| +// from the result. Fatal. |
| +// - apps/ listing fails, or ANY record object will not Get. Fatal. |
| +// |
| +// A record that READS but does not decode — or whose body disagrees with its |
| +// own path — is the other class: a content problem in one object, which is |
| +// skipped and logged (Reconcile's `protect` treatment, minus the registry |
| +// bookkeeping). One malformed record must not blind a caller to every other. |
| +func ListAllRecords(ctx context.Context, s release.Store) ([]Record, error) { |
| + known, readFailed, err := knownAccounts(ctx, s) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if readFailed { |
| + return nil, errors.New("provision: an account manifest could not be read cleanly — refusing to report a partial record set") |
| + } |
| + keys, err := s.List(ctx, "apps/") |
| + if err != nil { |
| + return nil, fmt.Errorf("provision: list apps: %w", err) |
| + } |
| + // Sorted so the result — and anything folded from it — is stable |
| + // call to call, whatever order the store lists in. |
| + slices.Sort(keys) |
| + var recs []Record |
| + for _, key := range keys { |
| + acct, app, host, ok := ParseRecordKey(key, known) |
| + if !ok { |
| + continue |
| + } |
| + raw, _, err := s.Get(ctx, key) |
| + if err != nil { |
| + return nil, fmt.Errorf("provision: read record %s: %w", key, err) |
| + } |
| + var rec Record |
| + if err := json.Unmarshal(raw, &rec); err != nil { |
| + log.Printf("provision: skipping record %s: malformed: %v", key, err) |
| + continue |
| + } |
| + if rec.Host != host || rec.Account != acct || rec.App != app { |
| + log.Printf("provision: skipping record %s: disagrees with its own path (host=%q account=%q app=%q)", |
| + key, rec.Host, rec.Account, rec.App) |
| + continue |
| + } |
| + recs = append(recs, rec) |
| + } |
| + return recs, nil |
| +} |
| + |
| // poolServed reports whether pool is one this box serves. An empty |
| // cfg.Pools means the shared "" pool ONLY, never "every pool" — the same |
| // convention domains.Config's AppsDomain=="" carries: unconfigured means the |
diff --git a/internal/steering/deployment.go b/internal/steering/deployment.go| index 9a7a74198..a6f92b48c 100644 |
| --- a/internal/steering/deployment.go |
| +++ b/internal/steering/deployment.go |
| @@ -3,9 +3,9 @@ package steering |
| // deployment.go — this package's ONE exported converge entry point. |
| // |
| // Everything else here is unexported by design: desiredState is a pure fold |
| -// (steering.go) and Converge takes a set that only that fold can build |
| +// (steering.go) and converge takes a set that only that fold can build |
| // (route53.go), so a caller outside this package cannot assemble the argument |
| -// Converge needs. ConvergeDeployment is the seam that closes that gap for the |
| +// converge needs. ConvergeDeployment is the seam that closes that gap for the |
| // host housekeeper (internal/host/steering_loop.go) — it takes what a BOX has |
| // (registry rows and raw edges-file bodies), folds, and converges, in one |
| // call that cannot be told to converge a set the fold did not produce. |
| @@ -51,7 +51,7 @@ import ( |
| // |
| // The distinction is not cosmetic. Both end in "one fewer edge in the fold", |
| // but only the read failure means the deployment does not KNOW what it has — |
| -// and Converge's own guards (the empty-set refusal and the proportional |
| +// and converge's own guards (the empty-set refusal and the proportional |
| // shrink limit) are the last line, not the first. This is the first. |
| func ConvergeDeployment(ctx context.Context, client route53API, zoneID string, routes []registry.Route, edgesFiles [][]byte, appsDomain string) error { |
| edges := make([]edgesFileView, 0, len(edgesFiles)) |
| @@ -69,5 +69,5 @@ func ConvergeDeployment(ctx context.Context, client route53API, zoneID string, r |
| } |
| edges = append(edges, f) |
| } |
| - return Converge(ctx, client, zoneID, desiredState(routes, edges, appsDomain)) |
| + return converge(ctx, client, zoneID, desiredState(routes, edges, appsDomain)) |
| } |
diff --git a/internal/steering/deployment_test.go b/internal/steering/deployment_test.go| index 313a3fad4..d1c82d933 100644 |
| --- a/internal/steering/deployment_test.go |
| +++ b/internal/steering/deployment_test.go |
| @@ -103,7 +103,7 @@ func TestConvergeDeploymentSkipsUnparseableEdgesFiles(t *testing.T) { |
| // TestConvergeDeploymentAllEdgesUnparseableRefusesTeardown is the composition |
| // that matters most: every edges file unreadable-as-content folds to an EMPTY |
| -// desired set, and Converge's empty-set refusal must catch it. A pass that |
| +// desired set, and converge's empty-set refusal must catch it. A pass that |
| // obeyed it would delete the deployment's entire managed record set because |
| // of a schema problem. |
| func TestConvergeDeploymentAllEdgesUnparseableRefusesTeardown(t *testing.T) { |
| @@ -132,7 +132,7 @@ func TestConvergeDeploymentAllEdgesUnparseableRefusesTeardown(t *testing.T) { |
| } |
| } |
| -// TestConvergeDeploymentRefusesBadZoneID: the zone-id gate is Converge's, and |
| +// TestConvergeDeploymentRefusesBadZoneID: the zone-id gate is converge's, and |
| // the entry point must not paper over it — a malformed id returns an error |
| // (which the duty logs) rather than converging against something else. |
| func TestConvergeDeploymentRefusesBadZoneID(t *testing.T) { |
diff --git a/internal/steering/route53.go b/internal/steering/route53.go| index 5985d5d2c..ebff7d946 100644 |
| --- a/internal/steering/route53.go |
| +++ b/internal/steering/route53.go |
| @@ -6,9 +6,12 @@ package steering |
| // health-check ownership rule (ownedHealthChecks) before changing anything |
| // here: everything else is bookkeeping around those two boundaries. |
| // |
| -// SINGLE-BOX INERT as of this task: nothing calls Converge. Task 6 wires it |
| -// into the host housekeeper, where CARLOS_STEERING_ZONE_ID unset (every box |
| -// today) keeps the duty dormant — zero API calls, zero writes. |
| +// converge is UNEXPORTED: ConvergeDeployment (deployment.go) is this |
| +// package's only entry point, so no caller outside it can converge a desired |
| +// set the fold did not build. It is reached, from Task 6's housekeeper duty |
| +// (internal/host/steering_loop.go) — but only on a deployment that sets |
| +// CARLOS_STEERING_ZONE_ID, which no box does today: unset keeps the duty |
| +// dormant, zero API calls, zero writes. |
| import ( |
| "context" |
| @@ -125,7 +128,7 @@ type ownedCheck struct { |
| eip string |
| } |
| -// Converge makes the zone's carlos-managed latency records match want. |
| +// converge makes the zone's carlos-managed latency records match want. |
| // |
| // The order is deliberate and is a safety property, not an implementation |
| // detail: |
| @@ -146,7 +149,7 @@ type ownedCheck struct { |
| // undone; the housekeeper's next tick re-runs the whole thing from a fresh |
| // read. Every step is individually idempotent, so a half-applied pass is a |
| // state this function converges from, not a state it gets stuck in. |
| -func Converge(ctx context.Context, client route53API, zoneID string, want []desiredRecord) error { |
| +func converge(ctx context.Context, client route53API, zoneID string, want []desiredRecord) error { |
| if !steeringZonePattern.MatchString(strings.TrimSpace(zoneID)) { |
| return fmt.Errorf("steering: refusing to converge: %q is not a bare hosted-zone id", zoneID) |
| } |
| @@ -563,7 +566,7 @@ func wantKey(rec desiredRecord) string { |
| // - it is bound to a health check WE own (tag-derived, ownedHealthChecks). |
| // This is the strongest of the five: every record this code writes is |
| // bound to one of our tagged checks, so the binding is a creation |
| -// signature. It is what makes the answer to "can Converge delete a |
| +// signature. It is what makes the answer to "can converge delete a |
| // record it did not create?" a structural no rather than a naming |
| // convention. |
| // |
| @@ -755,7 +758,7 @@ func dropNames(want []desiredRecord, names map[string]bool) []desiredRecord { |
| // partitionManaged counts the zone's carlos-managed records and collects the |
| // ones no desired key wants — the pass's would-be deletions. It is separated |
| -// from recordChanges so Converge can weigh the deletions BEFORE it creates a |
| +// from recordChanges so converge can weigh the deletions BEFORE it creates a |
| // single health check, which is what makes the shrink guard's refusal |
| // genuinely zero-write. |
| // |
| @@ -837,7 +840,7 @@ func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet, |
| for _, rec := range want { |
| checkID, ok := checkByEIP[rec.EIP] |
| if !ok || checkID == "" { |
| - // Unreachable: Converge creates a check for every distinct EIP |
| + // Unreachable: converge creates a check for every distinct EIP |
| // before calling this. Loud rather than silently minting a |
| // record with no health check, which would never withdraw. |
| return nil, fmt.Errorf("steering: no health check for %s (record %s / %s)", rec.EIP, rec.Name, rec.Box) |
| @@ -869,7 +872,7 @@ func recordChanges(want []desiredRecord, existing []r53types.ResourceRecordSet, |
| // independently valid — an UPSERT is idempotent and a DELETE is of a record |
| // nothing wants — so the next housekeeper tick re-reads the zone and issues |
| // exactly the changes still outstanding. The one thing that must NOT happen |
| -// after a failed batch is the health-check GC, and Converge returns before |
| +// after a failed batch is the health-check GC, and converge returns before |
| // reaching it. |
| func applyChanges(ctx context.Context, client route53API, zoneID string, changes []r53types.Change) error { |
| if len(changes) == 0 { |
diff --git a/internal/steering/route53_test.go b/internal/steering/route53_test.go| index 1bbb92a31..21a594956 100644 |
| --- a/internal/steering/route53_test.go |
| +++ b/internal/steering/route53_test.go |
| @@ -19,7 +19,7 @@ import ( |
| // --------------------------------------------------------------------------- |
| // fakeR53 is an in-memory Route53 that records every call and APPLIES the |
| -// changes it is handed, so a second Converge against the same fake sees the |
| +// changes it is handed, so a second converge against the same fake sees the |
| // state the first one left. No network, ever — the whole point of route53API |
| // being an interface. |
| // |
| @@ -372,8 +372,8 @@ func find(t *testing.T, f *fakeR53, name, setID string) r53types.ResourceRecordS |
| func mustConverge(t *testing.T, f *fakeR53, want []desiredRecord) { |
| t.Helper() |
| - if err := Converge(context.Background(), f, testZone, want); err != nil { |
| - t.Fatalf("Converge: %v", err) |
| + if err := converge(context.Background(), f, testZone, want); err != nil { |
| + t.Fatalf("converge: %v", err) |
| } |
| } |
| @@ -598,7 +598,7 @@ func TestConvergeRemoveSteering(t *testing.T) { |
| } |
| // TestConvergeStaticRecordsUntouched is the deletion-safety test. The zone is |
| -// seeded with everything an operator's zone really holds; Converge must |
| +// seeded with everything an operator's zone really holds; converge must |
| // submit no change that touches any of it. |
| func TestConvergeStaticRecordsUntouched(t *testing.T) { |
| f := newFake() |
| @@ -992,7 +992,7 @@ func TestConvergeEmptyWantRefusesTeardown(t *testing.T) { |
| recs := len(f.records) |
| buf := captureLog(t) |
| - if err := Converge(context.Background(), f, testZone, nil); err != nil { |
| + if err := converge(context.Background(), f, testZone, nil); err != nil { |
| t.Fatalf("empty want should not error: %v", err) |
| } |
| if f.writes() != before { |
| @@ -1014,7 +1014,7 @@ func TestConvergeEmptyWantRefusesTeardown(t *testing.T) { |
| func TestConvergeRejectsBadZoneID(t *testing.T) { |
| for _, zone := range []string{"", " ", "z0abc", "/hostedzone/Z0ABC", "Z0abc", "NOTAZONE"} { |
| f := newFake() |
| - err := Converge(context.Background(), f, zone, []desiredRecord{dublinRec}) |
| + err := converge(context.Background(), f, zone, []desiredRecord{dublinRec}) |
| if err == nil { |
| t.Errorf("zone %q accepted", zone) |
| } |
| @@ -1064,7 +1064,7 @@ func TestConvergePagination(t *testing.T) { |
| func TestConvergeTagFailureRemovesTheOrphan(t *testing.T) { |
| f := newFake() |
| f.failTags = errors.New("boom") |
| - err := Converge(context.Background(), f, testZone, []desiredRecord{dublinRec}) |
| + err := converge(context.Background(), f, testZone, []desiredRecord{dublinRec}) |
| if err == nil { |
| t.Fatalf("tagging failure did not fail the pass") |
| } |
| @@ -1081,7 +1081,7 @@ func TestConvergeTagFailureRemovesTheOrphan(t *testing.T) { |
| func TestConvergeChangeFailureIsLoud(t *testing.T) { |
| f := newFake() |
| f.failChange = errors.New("InvalidChangeBatch") |
| - err := Converge(context.Background(), f, testZone, []desiredRecord{dublinRec}) |
| + err := converge(context.Background(), f, testZone, []desiredRecord{dublinRec}) |
| if err == nil { |
| t.Fatalf("a failed change batch did not fail the pass") |
| } |
diff --git a/internal/steering/steering.go b/internal/steering/steering.go| index 7d09bf781..b5d50aa46 100644 |
| --- a/internal/steering/steering.go |
| +++ b/internal/steering/steering.go |
| @@ -8,10 +8,11 @@ |
| // Everything in this file is a PURE function: no AWS calls, no |
| // release.Store I/O, no registry writes. desiredState is table-testable |
| // with plain literals, by design — Task 5 (internal/steering/route53.go) |
| -// is the only place an AWS SDK call is allowed to appear, and Task 6 wires |
| -// this package into the host housekeeper. As of this task, nothing calls |
| -// desiredState: it is SINGLE-BOX INERT, exactly like every other stage of |
| -// this plan until its own wiring task lands. |
| +// is the only place an AWS SDK call is allowed to appear, and Task 6 wired |
| +// this package into the host housekeeper (internal/host/steering_loop.go). |
| +// desiredState is reached only through ConvergeDeployment, and only on a |
| +// deployment that sets CARLOS_STEERING_ZONE_ID — no box does today, so this |
| +// fold is still SINGLE-BOX DORMANT in production. |
| package steering |
| import ( |