amadan
edge+host: cap staleHopN, re-point region source to AWS region via IMDS
Review fixes on Task 1's stage-5 gates commit (8bd84c6):
Important 1 — staleHopN was keyed by client-controlled host (a wildcard-
homed route lets a sending box mint a signed hop for any invented
subdomain, edge.go's backhaul doc), so it grew without bound for the life
of the process. Capped at staleHopKeyCap (1024) distinct keys; anything
past the cap folds into one "<box> (other)" overflow bucket rather than
minting a new key — the same reasoning already applied to the
backhaul-error logThrottled key in the same commit.
Plan-defect (ruled, docs/superpowers/plans/2026-08-23-multi-region-steering.md
89b891b) — CARLOS_REGION is the CARLOS placement code ("ie"), not an AWS
region; Task 5's ChangeResourceRecordSets would reject it batch-wide, and
production boxes (which don't set it) would silently never arm. Re-pointed
the edges-file region field to IMDS placement/region at loop arm
(readPlacementRegion, sharing the token/leaf idiom with readPublicIP via
new imdsToken/imdsLeaf helpers), overridden by CARLOS_AWS_REGION, resolved
through a pure edgesFileRegion(getenv, probed) — domainsTickConfig's own
injected-probe shape — so the override/unset/trim contract is unit-pinned
without a live IMDS server.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 files changed,
+244
−36
internal/edge/homemap.go+29 −4internal/edge/homemap_test.go+39 −0internal/host/agent.go+56 −16internal/host/edgesfile.go+41 −11internal/host/edgesfile_test.go+79 −5
diff --git a/internal/edge/homemap.go b/internal/edge/homemap.go| index cea4be36a..044709a88 100644 |
| --- a/internal/edge/homemap.go |
| +++ b/internal/edge/homemap.go |
| @@ -173,10 +173,22 @@ type homeMap struct { |
| // for the first-and-every-tenth log cadence. |
| partialPolls int |
| // staleHopN counts staleHopRefusal occurrences per "box host" key (box is |
| - // always m.self — see staleHopRefusal's doc for why it is still named in |
| - // the key and the log line), for the SAME first-and-every-tenth cadence |
| - // partialPolls drives. Guarded by mu, following logPartialPoll's own |
| - // choice to reuse the one lock rather than mint a second. |
| + // always m.self — despite the name this is a PER-HOST counter in |
| + // practice, not cross-sender isolation; see staleHopRefusal's doc for |
| + // why box is still named in the key and the log line), for the SAME |
| + // first-and-every-tenth cadence partialPolls drives. Guarded by mu, |
| + // following logPartialPoll's own choice to reuse the one lock rather |
| + // than mint a second. |
| + // |
| + // Capped at staleHopKeyCap distinct keys — host here is CLIENT- |
| + // CONTROLLED (Home()'s one-label wildcard match means a single |
| + // catch-all edges-file entry lets a sending box mint a signed hop for |
| + // any invented subdomain, edge.go's backhaul doc), so an unbounded map |
| + // would be unbounded memory for the life of the process. Review catch |
| + // on this same commit: the identical reasoning already gated the |
| + // backhaul-error logThrottled key (edge.go) and is logThrottled's own |
| + // documented rule (never key from remote input) — this map just has no |
| + // logThrottled to inherit the cap from, so it gets its own. |
| staleHopN map[string]int |
| // The mirrored mesh keys, read on the same refresh. Replaced only on a |
| @@ -589,6 +601,16 @@ func (m *homeMap) staleHopRefusal(w http.ResponseWriter, host string) { |
| m.logStaleHop(host) |
| } |
| +// staleHopKeyCap bounds staleHopN's key set (review catch, 2026-08-23): |
| +// host is client-controlled (staleHopN's doc, above — a wildcard-homed |
| +// route lets a sending box mint a signed hop for any invented subdomain), |
| +// so an uncapped map would be unbounded memory, and an unbounded run of |
| +// distinct first-time log lines, for the life of the process. Once the cap |
| +// is reached, every NEW key folds into one "<box> (other)" overflow bucket |
| +// instead of growing the map further; keys already tracked keep their own |
| +// counts and cadence. |
| +const staleHopKeyCap = 1024 |
| + |
| // logStaleHop is staleHopRefusal's counting half, split out so the HTTP |
| // response is written before anything touches mu. |
| func (m *homeMap) logStaleHop(host string) { |
| @@ -597,6 +619,9 @@ func (m *homeMap) logStaleHop(host string) { |
| if m.staleHopN == nil { |
| m.staleHopN = make(map[string]int) |
| } |
| + if _, tracked := m.staleHopN[key]; !tracked && len(m.staleHopN) >= staleHopKeyCap { |
| + key = m.self + " (other)" |
| + } |
| m.staleHopN[key]++ |
| n := m.staleHopN[key] |
| m.mu.Unlock() |
diff --git a/internal/edge/homemap_test.go b/internal/edge/homemap_test.go| index 56b61245b..8854054b2 100644 |
| --- a/internal/edge/homemap_test.go |
| +++ b/internal/edge/homemap_test.go |
| @@ -422,6 +422,45 @@ func TestStaleHopRefusalLogsFirstAndEveryTenth(t *testing.T) { |
| } |
| } |
| +// Review catch on this task: host is client-controlled (a wildcard-homed |
| +// route lets a sending box mint a signed hop for any invented subdomain, |
| +// staleHopN's own doc), so staleHopN must not grow without bound. Drives |
| +// logStaleHop directly rather than through HTTP, so cap-plus-a-few distinct |
| +// hosts stays a cheap, focused test of the invariant: the map's key count |
| +// stops at cap+1 (every tracked key plus the one shared overflow bucket), |
| +// and hosts past the cap accumulate under that bucket instead of growing |
| +// the map further. |
| +func TestStaleHopNIsCappedWithAnOverflowBucket(t *testing.T) { |
| + _, m, _ := hopEdge(t, "annie", "local.test") |
| + |
| + for i := 0; i < staleHopKeyCap; i++ { |
| + m.logStaleHop(fmt.Sprintf("host-%d.example.com", i)) |
| + } |
| + if got := len(m.staleHopN); got != staleHopKeyCap { |
| + t.Fatalf("staleHopN has %d key(s) at the cap, want exactly %d", got, staleHopKeyCap) |
| + } |
| + |
| + // Three MORE distinct hosts, past the cap. |
| + for i := 0; i < 3; i++ { |
| + m.logStaleHop(fmt.Sprintf("overflow-%d.example.com", i)) |
| + } |
| + if got, want := len(m.staleHopN), staleHopKeyCap+1; got != want { |
| + t.Fatalf("staleHopN has %d key(s) past the cap, want %d (cap distinct keys + one overflow bucket, not %d+3)", got, want, staleHopKeyCap) |
| + } |
| + const overflowKey = "annie (other)" |
| + if n := m.staleHopN[overflowKey]; n != 3 { |
| + t.Fatalf("overflow key %q count = %d, want 3 — each new host past the cap must fold into the shared bucket, not mint its own", overflowKey, n) |
| + } |
| + |
| + // A key that was already tracked BEFORE the cap keeps its own identity |
| + // and cadence — the cap only refuses NEW keys, never demotes an |
| + // existing one. |
| + m.logStaleHop("host-0.example.com") |
| + if n := m.staleHopN["annie host-0.example.com"]; n != 2 { |
| + t.Fatalf("a pre-cap key's count = %d, want 2 — it must keep incrementing under its own key, not fold into overflow", n) |
| + } |
| +} |
| + |
| // Verification failure is not an error path: it is "you are an ordinary |
| // client", with every forwarding claim you made discarded. |
| func TestUnverifiedHopIsServedAsAnOrdinaryClientWithForwardedHeadersStripped(t *testing.T) { |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 8a609ae33..3f9e5dda4 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -1433,13 +1433,12 @@ func resolveHost(host string) ([]string, error) { |
| return net.DefaultResolver.LookupHost(ctx, host) |
| } |
| -// readPublicIP best-effort reads this box's public IPv4 from EC2 IMDSv2 |
| -// (token PUT, then the public-ipv4 leaf). Any failure — not on EC2, IMDS |
| -// disabled, no public address — returns "", and the caller treats that as |
| -// "unknown", never as an error: a box without a discoverable public IP is a |
| -// valid deployment whose DNS checks simply stand down. |
| -func readPublicIP() string { |
| - client := &http.Client{Timeout: 2 * time.Second} |
| +// imdsToken fetches an EC2 IMDSv2 session token (60s TTL) — the shared |
| +// first step of every IMDS read in this file. Any failure (not on EC2, |
| +// IMDS disabled, a transient metadata-endpoint hiccup) returns "", and |
| +// every caller already treats a blank token as "no answer", never an |
| +// error. |
| +func imdsToken(client *http.Client) string { |
| req, err := http.NewRequest(http.MethodPut, "http://169.254.169.254/latest/api/token", nil) |
| if err != nil { |
| return "" |
| @@ -1449,32 +1448,73 @@ func readPublicIP() string { |
| if err != nil { |
| return "" |
| } |
| - token, _ := io.ReadAll(resp.Body) |
| - resp.Body.Close() |
| + defer resp.Body.Close() |
| if resp.StatusCode != http.StatusOK { |
| return "" |
| } |
| - req, err = http.NewRequest(http.MethodGet, "http://169.254.169.254/latest/meta-data/public-ipv4", nil) |
| + body, _ := io.ReadAll(resp.Body) |
| + return strings.TrimSpace(string(body)) |
| +} |
| + |
| +// imdsLeaf reads one IMDS leaf under the given token, trimmed. The status |
| +// code travels back with the body so a caller can tell "empty leaf" from |
| +// "leaf did not answer 200" without a second round trip. |
| +func imdsLeaf(client *http.Client, token, path string) (string, int) { |
| + req, err := http.NewRequest(http.MethodGet, "http://169.254.169.254"+path, nil) |
| if err != nil { |
| - return "" |
| + return "", 0 |
| } |
| - req.Header.Set("X-aws-ec2-metadata-token", strings.TrimSpace(string(token))) |
| - resp, err = client.Do(req) |
| + req.Header.Set("X-aws-ec2-metadata-token", token) |
| + resp, err := client.Do(req) |
| if err != nil { |
| - return "" |
| + return "", 0 |
| } |
| defer resp.Body.Close() |
| body, _ := io.ReadAll(resp.Body) |
| - if resp.StatusCode != http.StatusOK { |
| + return strings.TrimSpace(string(body)), resp.StatusCode |
| +} |
| + |
| +// readPublicIP best-effort reads this box's public IPv4 from EC2 IMDSv2 |
| +// (token PUT, then the public-ipv4 leaf). Any failure — not on EC2, IMDS |
| +// disabled, no public address — returns "", and the caller treats that as |
| +// "unknown", never as an error: a box without a discoverable public IP is a |
| +// valid deployment whose DNS checks simply stand down. |
| +func readPublicIP() string { |
| + client := &http.Client{Timeout: 2 * time.Second} |
| + token := imdsToken(client) |
| + if token == "" { |
| + return "" |
| + } |
| + ip, status := imdsLeaf(client, token, "/latest/meta-data/public-ipv4") |
| + if status != http.StatusOK { |
| return "" |
| } |
| - ip := strings.TrimSpace(string(body)) |
| if net.ParseIP(ip) == nil { |
| return "" |
| } |
| return ip |
| } |
| +// readPlacementRegion best-effort reads this box's AWS region from EC2 |
| +// IMDSv2 (token PUT, then the placement/region leaf) — the same idiom and |
| +// the same "" on any failure contract as readPublicIP: not on EC2, IMDS |
| +// disabled, or a metadata-endpoint hiccup all read as "unknown" here, and |
| +// the caller (edgesFileRegion) treats that identically to an unset |
| +// CARLOS_AWS_REGION override — an edge with neither is simply unarmed for |
| +// steering, not an error. |
| +func readPlacementRegion() string { |
| + client := &http.Client{Timeout: 2 * time.Second} |
| + token := imdsToken(client) |
| + if token == "" { |
| + return "" |
| + } |
| + region, status := imdsLeaf(client, token, "/latest/meta-data/placement/region") |
| + if status != http.StatusOK { |
| + return "" |
| + } |
| + return region |
| +} |
| + |
| // domainsLoop runs the domains reconcile pass forever on the STATUS tick's |
| // cadence (5min + jitter), not the adopt loop's hour: the console's |
| // custom-domain guidance promises "Checks run every few minutes", and a DNS |
diff --git a/internal/host/edgesfile.go b/internal/host/edgesfile.go| index fbe7d5523..1ff616d8a 100644 |
| --- a/internal/host/edgesfile.go |
| +++ b/internal/host/edgesfile.go |
| @@ -35,10 +35,16 @@ type edgeHost struct { |
| // {"v":1,"box":"annie-welding","eip":"99.81.104.219","region":"eu-west-1","generation":"2026-08-22T09:00:00Z","hosts":{"app.example.com":{"role":"home"}}} |
| // |
| // Region is additive (Part 3, Task 4's steering converger needs it for |
| -// latency records): omitempty, value = CARLOS_REGION verbatim, absent |
| -// entirely when the env is unset — a pre-region body written by an older |
| -// build still parses, and this build's own body for an unset-region box is |
| -// byte-identical to what it always published. |
| +// latency records): omitempty, absent entirely when it could not be |
| +// resolved (see edgesFileRegion) — a pre-region body written by an older |
| +// build still parses, and this build's own body for an unresolved-region |
| +// box is byte-identical to what it always published. |
| +// |
| +// The value is the AWS region (e.g. "eu-west-1"), NOT CARLOS_REGION — that |
| +// env var is the CARLOS placement code ("ie", "fleet/<name>"), a DIFFERENT |
| +// vocabulary that Task 5's ChangeResourceRecordSets calls would reject |
| +// batch-wide if it ever reached here (plan review catch, 2026-08-23). |
| +// edgesFileRegion resolves the real thing. |
| type edgesFile struct { |
| V int `json:"v"` |
| Box string `json:"box"` |
| @@ -72,10 +78,11 @@ type edgesFile struct { |
| // NEVER had a discoverable IP — the DR runbook's hosts list still gets |
| // published in that case, just without a dial address. |
| // |
| -// region is CARLOS_REGION verbatim (read once at loop arm, the domains |
| -// loop's precedent — see edgesFileLoop) and is part of the change gate like |
| -// every other field except Generation: a region relabel is a real change and |
| -// must write, not be swallowed as a timestamp-only refresh would be. |
| +// region is the AWS region resolved once at loop arm by edgesFileRegion |
| +// (the domains loop's read-once precedent — see edgesFileLoop) and is part |
| +// of the change gate like every other field except Generation: a region |
| +// relabel is a real change and must write, not be swallowed as a |
| +// timestamp-only refresh would be. |
| func edgesFilePass(ctx context.Context, s release.Store, reg *registry.Registry, box, eip, region string, last *[]byte) error { |
| routes, err := reg.List() |
| if err != nil { |
| @@ -171,6 +178,27 @@ func seedEdgesFileGate(ctx context.Context, s release.Store, box string) []byte |
| return gate |
| } |
| +// edgesFileRegion resolves this box's AWS region, pure and getenv/probe- |
| +// injectable — domainsTickConfig's own shape (agent.go): the network probe |
| +// (readPlacementRegion, an IMDS call) is read ONCE by the caller and handed |
| +// in as probedRegion, so this function itself never touches the network and |
| +// its override/unset/trim contract is unit-pinned without a live IMDS |
| +// server in the test. |
| +// |
| +// CARLOS_AWS_REGION, when set, wins outright — the escape hatch for testing |
| +// and for any box this build ever runs on OFF EC2. Otherwise the IMDS |
| +// probe's answer stands, trimmed. Neither set means "" — a box that is |
| +// off EC2 with no override is correctly UNARMED for steering (Task 4's |
| +// converger gates on this field parsing as an AWS region, never just |
| +// non-empty), not an error: DNS health checks target EIPs, which never |
| +// depended on this. |
| +func edgesFileRegion(getenv func(string) string, probedRegion string) string { |
| + if override := strings.TrimSpace(getenv("CARLOS_AWS_REGION")); override != "" { |
| + return override |
| + } |
| + return strings.TrimSpace(probedRegion) |
| +} |
| + |
| // edgesFileLoop publishes this box's edges file every 5 minutes + jitter — |
| // the same cadence and arming site as registrySnapshotLoop. Home moves ARE |
| // placement events, not per-request events, but the comment this replaces |
| @@ -195,10 +223,12 @@ func edgesFileLoop(reg *registry.Registry) { |
| return |
| } |
| box := metricsBoxLabel(os.Getenv, readMachineID()) |
| - // CARLOS_REGION, read once here at arm — the domains loop's read-once |
| + // Region resolved once here at arm — the domains loop's read-once |
| // precedent (domainsLoop reads its env once before the loop and threads |
| - // the result through every tick via cfg, never re-reading per pass). |
| - region := strings.TrimSpace(os.Getenv("CARLOS_REGION")) |
| + // the result through every tick via cfg, never re-reading or re-probing |
| + // per pass). readPlacementRegion is the one IMDS call; edgesFileRegion |
| + // applies the CARLOS_AWS_REGION override on top, pure. |
| + region := edgesFileRegion(os.Getenv, readPlacementRegion()) |
| log.Printf("agent: edges file loop armed (5min + jitter, %s)", edgesFileKey(box)) |
| last := seedEdgesFileGate(ctx, store, box) |
| delay := time.Minute |
diff --git a/internal/host/edgesfile_test.go b/internal/host/edgesfile_test.go| index 43fd5a19c..fbffb76fe 100644 |
| --- a/internal/host/edgesfile_test.go |
| +++ b/internal/host/edgesfile_test.go |
| @@ -271,11 +271,14 @@ func TestEdgesFileRawWireBodyMatchesSchema(t *testing.T) { |
| // FINDING 5 (task 1, 2026-08-22): region is additive to the pinned schema — |
| // this extends FINDING 4's raw-body pin rather than replacing it. Two |
| -// bodies, byte for byte: CARLOS_REGION unset must produce the IDENTICAL |
| -// bytes FINDING 4 already pins (no "region" key at all, omitempty), and |
| -// CARLOS_REGION set must insert exactly `"region":"eu-west-1"` between |
| -// "eip" and "generation" — the struct field order — with nothing else |
| -// disturbed. |
| +// bodies, byte for byte: an unresolved region (edgesFileRegion's "" case — |
| +// off EC2, no CARLOS_AWS_REGION override) must produce the IDENTICAL bytes |
| +// FINDING 4 already pins (no "region" key at all, omitempty), and a |
| +// resolved region must insert exactly `"region":"eu-west-1"` between "eip" |
| +// and "generation" — the struct field order — with nothing else disturbed. |
| +// This test drives edgesFilePass directly with the resolved value; the |
| +// resolution contract itself (override wins, IMDS otherwise, trimmed) is |
| +// pinned separately in TestEdgesFileRegionResolution. |
| func TestEdgesFileRawWireBodyRegionField(t *testing.T) { |
| s, reg := release.NewDirStore(t.TempDir()), edgesTestRegistry(t) |
| seedRoute(t, reg, "app.example.com") |
| @@ -328,6 +331,77 @@ func TestEdgesFileRawWireBodyRegionField(t *testing.T) { |
| }) |
| } |
| +// Plan re-ruling (2026-08-23, review catch): CARLOS_REGION is the CARLOS |
| +// placement code ("ie"), NOT an AWS region — Task 5's |
| +// ChangeResourceRecordSets would reject it batch-wide, so the field's |
| +// value must come from CARLOS_AWS_REGION / IMDS instead, never |
| +// CARLOS_REGION. edgesFileRegion is the pure half of that contract |
| +// (getenv/probe-injectable, domainsTickConfig's own shape) — this pins |
| +// override-wins, unset-and-no-probe-answer → "", and trimming on both |
| +// inputs, all without a live IMDS server. |
| +func TestEdgesFileRegionResolution(t *testing.T) { |
| + env := func(m map[string]string) func(string) string { |
| + return func(k string) string { return m[k] } |
| + } |
| + |
| + cases := []struct { |
| + name string |
| + env map[string]string |
| + probed string |
| + want string |
| + }{ |
| + { |
| + name: "override set wins over the probed value", |
| + env: map[string]string{"CARLOS_AWS_REGION": "eu-west-1"}, |
| + probed: "us-east-1", |
| + want: "eu-west-1", |
| + }, |
| + { |
| + name: "override set, untrimmed, wins and is trimmed", |
| + env: map[string]string{"CARLOS_AWS_REGION": " eu-west-1 "}, |
| + probed: "us-east-1", |
| + want: "eu-west-1", |
| + }, |
| + { |
| + name: "override unset falls back to the probed value", |
| + env: map[string]string{}, |
| + probed: "eu-west-1", |
| + want: "eu-west-1", |
| + }, |
| + { |
| + name: "override unset, probed value untrimmed, is trimmed", |
| + env: map[string]string{}, |
| + probed: " eu-west-1 ", |
| + want: "eu-west-1", |
| + }, |
| + { |
| + name: "CARLOS_REGION (the placement code) is never consulted", |
| + env: map[string]string{"CARLOS_REGION": "ie"}, |
| + probed: "", |
| + want: "", |
| + }, |
| + { |
| + name: "unset override and no probe answer (off EC2) is empty, not an error", |
| + env: map[string]string{}, |
| + probed: "", |
| + want: "", |
| + }, |
| + { |
| + name: "blank override (set but empty) does not shadow the probed value", |
| + env: map[string]string{"CARLOS_AWS_REGION": ""}, |
| + probed: "eu-west-1", |
| + want: "eu-west-1", |
| + }, |
| + } |
| + for _, c := range cases { |
| + t.Run(c.name, func(t *testing.T) { |
| + if got := edgesFileRegion(env(c.env), c.probed); got != c.want { |
| + t.Fatalf("edgesFileRegion() = %q, want %q", got, c.want) |
| + } |
| + }) |
| + } |
| +} |
| + |
| // FINDING 6 (task 1, 2026-08-22, the plan-2 final review's Important 1): a |
| // freshly ARMED loop must seed its change-gate from whatever this box last |
| // published, or two bugs follow — see seedEdgesFileGate's doc. This test |