amadan
canary-ttl: review fix round — honest age, one formatter, and the console pin
The nag printed a canary's TOTAL age labelled as time-expired — false by exactly CanaryTTL, so a one-hour lapse read as a seven-day-old problem and the two surfaces of a visibility feature disagreed by a constant. CanaryAge subtracts the lease; FormatCanaryAge is the ONE formatter both the nag and the console badge call, "<1h" for the first hour because "0h" reads as not-actually-expired. The visibility-only contract now has its pin at the one surface the release-layer tests cannot reach: TestLadderPromotesOntoAnExpiredCanary drives the console's promote handler onto a hand-signed expired pointer and asserts the promotion lands — a mutation gating handlePromote on expiry failed zero tests before this existed. Plus: the once-per-route nag proven with three routes on one channel (awake and hibernating), loud never-add-this comments on the fast loops, the status tile's hidden-when-null choice documented as an assertion-of-the-positive, and the per-box feed field's comment no longer implying a table column that does not exist. Started by the reviewing agent before its session limit hit; finished inline. Rulings and findings in canary-ttl-review.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8 files changed,
+244
−35
internal/console/ladder.go+9 −14internal/console/ladder_test.go+56 −0internal/host/agent.go+13 −0internal/release/adopt.go+25 −10internal/release/adopt_test.go+56 −10internal/release/canary_ttl.go+44 −0internal/release/canary_ttl_test.go+36 −0internal/status/fleet.go+5 −1
diff --git a/internal/console/ladder.go b/internal/console/ladder.go| index 562699e8f..ceadd0ab7 100644 |
| --- a/internal/console/ladder.go |
| +++ b/internal/console/ladder.go |
| @@ -26,7 +26,6 @@ package console |
| // remembers deploying. Visibility only — an expired canary keeps serving. |
| import ( |
| - "fmt" |
| "sort" |
| "strings" |
| "time" |
| @@ -281,6 +280,14 @@ func prodWiredCanaryHosts(routes []RouteView) map[string]bool { |
| // because expiry is a property of the CHANNEL's PromotedAt — every route |
| // following the same canary shares one answer, computed once rather than |
| // re-derived per host. |
| +// |
| +// Formatting goes through release.CanaryAge + release.FormatCanaryAge — |
| +// the SAME pair the adopt pass's nag calls (internal/release/adopt.go) — |
| +// rather than computing a duration here and rendering it locally. That used |
| +// to be two independent implementations that quietly disagreed by exactly |
| +// CanaryTTL (2026-08-03 review, IMPORTANT 2): sharing the formatter is what |
| +// makes "the nag and the badge cannot drift apart" a structural fact |
| +// instead of a coincidence two people have to keep re-verifying. |
| func canaryExpiry(channels []ChannelView, now time.Time) map[string]string { |
| out := map[string]string{} |
| for _, cv := range channels { |
| @@ -295,19 +302,7 @@ func canaryExpiry(channels []ChannelView, now time.Time) map[string]string { |
| if !release.CanaryExpired(ptr, now) { |
| continue |
| } |
| - out[cv.Name] = formatAge(now.Sub(cv.PromotedAt) - release.CanaryTTL) |
| + out[cv.Name] = release.FormatCanaryAge(release.CanaryAge(ptr, now)) |
| } |
| return out |
| } |
| - |
| -// formatAge renders a duration coarsely — days once it's at least one full |
| -// day, hours below that — because "expired 3d ago" is what a reader acts |
| -// on; nobody needs minute-level precision on a guard rail whose whole point |
| -// is that nobody was watching the clock. |
| -func formatAge(d time.Duration) string { |
| - d = d.Round(time.Hour) |
| - if d < 24*time.Hour { |
| - return fmt.Sprintf("%dh", int(d.Hours())) |
| - } |
| - return fmt.Sprintf("%dd", int(d.Hours())/24) |
| -} |
diff --git a/internal/console/ladder_test.go b/internal/console/ladder_test.go| index 05e7cc388..2194c2768 100644 |
| --- a/internal/console/ladder_test.go |
| +++ b/internal/console/ladder_test.go |
| @@ -8,6 +8,9 @@ package console |
| import ( |
| "bytes" |
| "context" |
| + "crypto/ed25519" |
| + "encoding/base64" |
| + "encoding/json" |
| "net/http" |
| "net/url" |
| "strings" |
| @@ -480,3 +483,56 @@ func TestCanaryExpiryOmitsUnexpiredChannels(t *testing.T) { |
| t.Error("an old canary must appear in the expiry map") |
| } |
| } |
| + |
| +// seedExpiredCanary hand-signs a canary pointer whose lease ran out long |
| +// ago — every public promotion path stamps the real clock, so forging |
| +// PromotedAt in the wrapper is the only way to put an EXPIRED canary in |
| +// front of a console under test (the same technique, for the same reason, |
| +// as internal/release's writeBackdatedPointer, which is deliberately not |
| +// exported). |
| +func seedExpiredCanary(t *testing.T, fx *consoleFixture, app, channel, version string) { |
| + t.Helper() |
| + p := &release.ChannelPointer{Schema: 1, App: app, Channel: channel, |
| + Version: version, PromotedAt: time.Now().Add(-(release.CanaryTTL + 30*24*time.Hour))} |
| + payload, err := json.Marshal(p) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + b64 := base64.StdEncoding.EncodeToString(payload) |
| + wrapper, err := json.Marshal(struct { |
| + Payload string `json:"payload"` |
| + Sig string `json:"sig"` |
| + }{b64, base64.StdEncoding.EncodeToString(ed25519.Sign(fx.priv, []byte(b64)))}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := fx.rel.Put(context.Background(), |
| + release.ChannelKey(release.OpsAccount, app, channel), wrapper, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// TestLadderPromotesOntoAnExpiredCanary pins spec §5's visibility-only |
| +// contract at the one surface the release-layer pins cannot reach: the |
| +// console's promote handler. An expired canary is exactly where the next |
| +// build lands when work on that branch resumes, and a "this canary expired" |
| +// gate here — however well-meant a confirmation might look — would turn a |
| +// guard rail into an obstacle at the moment it is least wanted. (2026-08-03 |
| +// review, IMPORTANT 1: a mutation gating handlePromote on expiry failed |
| +// zero tests before this one existed.) |
| +func TestLadderPromotesOntoAnExpiredCanary(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + shipVersion(t, fx, "kass", "v8") |
| + seedExpiredCanary(t, fx, "kass", "canary/old-branch", "v1") |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + |
| + status, _, body := postForm(t, c, fx.u("/apps/kass/promote"), |
| + url.Values{"csrf": {csrf}, "channel": {"canary/old-branch"}, "version": {"v8"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("promotion onto an expired canary = %d, want 303:\n%s", status, body) |
| + } |
| + if p := fx.channelVersion(t, "kass", "canary/old-branch"); p.Version != "v8" { |
| + t.Fatalf("expired canary still holds %s — expiry has become a gate", p.Version) |
| + } |
| +} |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index d6b3da2b4..18b51b3bc 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -205,6 +205,13 @@ func adoptLoop(reg *registry.Registry) { |
| for { |
| time.Sleep(delay) |
| delay = time.Hour + rand.N(10*time.Minute) |
| + // DO NOT add release.NagExpiredCanaries(...) to this call. This loop |
| + // runs hourly, but it is NOT "the hourly root adopt pass" the option's |
| + // own doc comment names — that's RunAdopt (carlos-adopt.service) |
| + // below, root-privileged and the one place config sync also runs. Two |
| + // hourly callers both nagging would double the log line every hour; |
| + // worse, if this loop's cadence ever changes, a nag wired here changes |
| + // frequency with it silently. One caller, one nag. |
| if err := release.Pass(ctx, reg, store, pub, optDir, restartRoute); err != nil { |
| log.Printf("agent: adopt pass: %v", err) |
| } |
| @@ -237,6 +244,12 @@ func canaryAdoptLoop(reg *registry.Registry) { |
| log.Printf("agent: canary adopt loop armed (2s + jitter, canary/* routes only)") |
| for { |
| time.Sleep(2*time.Second + rand.N(time.Second)) |
| + // DO NOT add release.NagExpiredCanaries(...) to this call, ever. This |
| + // is THE loop release.NagExpiredCanaries' doc comment warns about by |
| + // name: every 2-3 seconds, canary/* only. An expired canary sitting |
| + // still would nag on every single tick — a journal-flooding poll storm |
| + // out of what is supposed to be an hourly, easy-to-skim log line. The |
| + // nag belongs to RunAdopt (carlos-adopt.service) alone. |
| if err := release.PassFilter(ctx, reg, store, pub, optDir, restartRoute, isCanaryRoute); err != nil { |
| log.Printf("agent: canary adopt pass: %v", err) |
| } |
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index d01ca2262..5f6ef1228 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -38,15 +38,22 @@ type passConfig struct { |
| // NagExpiredCanaries opts a Pass/PassFilter call into spec §5's nag: one |
| // line per expired canary/* route the call sees, written to w. |
| // |
| -// This belongs to the HOURLY ROOT adopt pass alone — carlos-adopt.service's |
| -// RunAdopt, which is the only caller that sets it (2026-08-03 review). The |
| -// same split PR #9's config sync uses, for the same reason: the in-process |
| -// adoptLoop (hourly + jitter, inside the sandboxed carlos-edge.service) and |
| -// canaryAdoptLoop (every 2-3 seconds, canary/* routes only — the whole |
| -// point of which is FAST pickup of a freshly-promoted canary) never set |
| -// this. An expired canary sitting still on the fast loop would otherwise |
| -// nag every 2-3 seconds forever; gating it to the hourly root pass keeps |
| -// the nag journal-friendly — once an hour, not a poll-storm of log lines. |
| +// DO NOT pass this to a fast/frequent adoption loop. It belongs to the |
| +// HOURLY ROOT adopt pass ALONE — carlos-adopt.service's RunAdopt |
| +// (internal/host/agent.go), the only caller that sets it (2026-08-03 |
| +// review), and every future caller must keep it that way. The same split |
| +// PR #9's config sync uses, for the same reason: the in-process adoptLoop |
| +// (hourly + jitter, inside the sandboxed carlos-edge.service) and |
| +// canaryAdoptLoop (every 2-3 SECONDS, canary/* routes only — the whole |
| +// point of which is fast pickup of a freshly-promoted canary) must NEVER |
| +// set this. An expired canary sitting still on that fast loop would |
| +// otherwise nag every 2-3 seconds forever, turning a visibility feature |
| +// into a journal-flooding one; gating it to the hourly root pass keeps the |
| +// nag journal-friendly — once an hour, not a poll-storm of log lines. This |
| +// is enforced by convention (which call sites pass the option), not by the |
| +// type system — see agent.go's adoptLoop/canaryAdoptLoop for the loud |
| +// "never add this" comments at the two places it would be easiest to add |
| +// by accident. |
| func NagExpiredCanaries(w io.Writer) PassOption { |
| return func(c *passConfig) { c.nag = w } |
| } |
| @@ -235,8 +242,16 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| // the hourly root adopt pass, or an expired canary sitting still would |
| // nag every few seconds forever instead of once an hour. |
| if nag != nil && CanaryExpired(ptr, now) { |
| + // FormatCanaryAge(CanaryAge(...)), NOT a raw now.Sub(ptr.PromotedAt): |
| + // that would print the pointer's TOTAL age (CanaryAge + CanaryTTL), |
| + // overstating how long it's been expired by exactly one lease length |
| + // (2026-08-03 review, IMPORTANT 2) — and disagreeing with the |
| + // console's badge, which renders the same CanaryAge/FormatCanaryAge |
| + // pair (internal/console/ladder.go's canaryExpiry). One formatter, |
| + // shared, so the two operator-facing surfaces of this feature cannot |
| + // drift apart on what a duration means. |
| fmt.Fprintf(nag, " %s: canary %s (%s) has been expired %s — still serving, nothing blocked\n", |
| - rt.Host, ptr.Channel, ptr.Version, now.Sub(ptr.PromotedAt).Round(time.Hour)) |
| + rt.Host, ptr.Channel, ptr.Version, FormatCanaryAge(CanaryAge(ptr, now))) |
| } |
| channel := routeChannel(rt) |
| ver := ptr.Version |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index cab85113b..ab541f310 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -490,12 +490,20 @@ func TestPassFilterDoesNotNagWithoutTheOption(t *testing.T) { |
| } |
| } |
| -// TestPassNagsExactlyOnceWithTheOption pins the root path's half: RunAdopt |
| +// TestPassNagsOncePerExpiredCanaryRoute pins the root path's half: RunAdopt |
| // (carlos-adopt.service, hourly) passes NagExpiredCanaries(os.Stdout) to |
| -// Pass, and that call must print EXACTLY ONE nag line for one expired |
| -// canary route — journal-friendly, once per hourly pass, not once per host |
| -// revisit. |
| -func TestPassNagsExactlyOnceWithTheOption(t *testing.T) { |
| +// Pass, and that call must print ONE nag line PER EXPIRED CANARY ROUTE it |
| +// sees — not one per hourly pass regardless of how many routes follow the |
| +// expired channel. Three routes (two awake, one hibernating — the awake and |
| +// hibernating lists are walked separately in Pass/PassFilter, so this is |
| +// also the regression test for "hibernating routes get skipped" and |
| +// "a route is visited twice") on ONE expired canary channel must produce |
| +// exactly three lines, each naming its own host. |
| +// |
| +// (2026-08-03 review, MINOR 6: this test used to be named |
| +// TestPassNagsExactlyOnceWithTheOption and only ever exercised one route, |
| +// so "not once per host revisit" was asserted by name only, never proven.) |
| +func TestPassNagsOncePerExpiredCanaryRoute(t *testing.T) { |
| s := NewDirStore(t.TempDir()) |
| ctx := context.Background() |
| priv, pub := testKeys(t) |
| @@ -505,18 +513,56 @@ func TestPassNagsExactlyOnceWithTheOption(t *testing.T) { |
| shipRelease(t, s, "bab", "myapp", "v1", adoptPlatform, []byte("build one")) |
| writeBackdatedPointer(t, s, priv, "bab", "myapp", "canary/x", "v1", |
| time.Now().Add(-(CanaryTTL + 30*24*time.Hour))) |
| + addRoute(t, reg, "awake-a.example.com", "myapp", "canary/x", false) |
| + addRoute(t, reg, "awake-b.example.com", "myapp", "canary/x", false) |
| + addRoute(t, reg, "sleepy.example.com", "myapp", "canary/x", true) |
| + |
| + var buf bytes.Buffer |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }, NagExpiredCanaries(&buf)); err != nil { |
| + t.Fatalf("Pass: %v", err) |
| + } |
| + out := buf.String() |
| + if got := strings.Count(out, "expired"); got != 3 { |
| + t.Errorf("nag lines containing \"expired\" = %d, want exactly 3 (one per route, awake and hibernating alike):\n%s", got, out) |
| + } |
| + for _, host := range []string{"awake-a.example.com", "awake-b.example.com", "sleepy.example.com"} { |
| + if !strings.Contains(out, host) { |
| + t.Errorf("nag output does not name %s:\n%s", host, out) |
| + } |
| + } |
| +} |
| + |
| +// TestNagReportsTimePastTTLNotTotalAge pins the 2026-08-03 review's |
| +// IMPORTANT 2 fix: the nag must report how long PAST CanaryTTL a canary is, |
| +// not its total age. A pointer promoted CanaryTTL+1h ago has been EXPIRED |
| +// for about an hour, not 169 hours (7 days 1 hour, its total age) — the |
| +// second number is real but answers a different question, and printing it |
| +// under "has been expired" is the bug the review caught. It must also |
| +// disagree with nothing the console shows for the same pointer |
| +// (internal/console/ladder_test.go's TestLadderExpiredCanaryIsBadgedWithAge |
| +// exercises the same release.CanaryAge/FormatCanaryAge pair). |
| +func TestNagReportsTimePastTTLNotTotalAge(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + opt := t.TempDir() |
| + reg := testRegistry(t) |
| + |
| + shipRelease(t, s, "bab", "myapp", "v1", adoptPlatform, []byte("build one")) |
| + writeBackdatedPointer(t, s, priv, "bab", "myapp", "canary/x", "v1", |
| + time.Now().Add(-(CanaryTTL + time.Hour))) |
| addRoute(t, reg, "old-canary.example.com", "myapp", "canary/x", false) |
| var buf bytes.Buffer |
| if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }, NagExpiredCanaries(&buf)); err != nil { |
| t.Fatalf("Pass: %v", err) |
| } |
| - got := strings.Count(buf.String(), "expired") |
| - if got != 1 { |
| - t.Errorf("nag lines containing \"expired\" = %d, want exactly 1:\n%s", got, buf.String()) |
| + out := buf.String() |
| + if !strings.Contains(out, "1h") { |
| + t.Errorf("nag does not say 1h (time past TTL) for a canary expired ~1h ago:\n%s", out) |
| } |
| - if !strings.Contains(buf.String(), "old-canary.example.com") { |
| - t.Errorf("nag line does not name the route:\n%s", buf.String()) |
| + if strings.Contains(out, "169h") { |
| + t.Errorf("nag reports total age (169h) instead of time past TTL:\n%s", out) |
| } |
| } |
diff --git a/internal/release/canary_ttl.go b/internal/release/canary_ttl.go| index 2481a4aed..9a3425300 100644 |
| --- a/internal/release/canary_ttl.go |
| +++ b/internal/release/canary_ttl.go |
| @@ -16,6 +16,7 @@ |
| package release |
| import ( |
| + "fmt" |
| "strings" |
| "time" |
| ) |
| @@ -39,3 +40,46 @@ func CanaryExpired(p *ChannelPointer, now time.Time) bool { |
| } |
| return now.Sub(p.PromotedAt) > CanaryTTL |
| } |
| + |
| +// CanaryAge is how long PAST CanaryTTL an expired canary pointer is — the |
| +// number both operator-facing surfaces (the adopt pass's nag and the |
| +// console's badge) render, NOT the pointer's total age. Total age |
| +// (now.Sub(p.PromotedAt)) is always exactly CanaryAge + CanaryTTL, and |
| +// reporting IT as "how long has this been expired" overstates the lapse by |
| +// one full lease length — a canary that missed its lease by an hour would |
| +// read as a seven-day-old problem (2026-08-03 review, IMPORTANT 2). |
| +// |
| +// Meaningful only when CanaryExpired(p, now) is already true; call that |
| +// first. A caller that skips the check gets a negative or otherwise |
| +// nonsensical duration back for a fresh or non-canary pointer, the same as |
| +// any bare Sub on the wrong side of zero. |
| +func CanaryAge(p *ChannelPointer, now time.Time) time.Duration { |
| + return now.Sub(p.PromotedAt) - CanaryTTL |
| +} |
| + |
| +// FormatCanaryAge renders a CanaryAge duration for a human. It is the ONE |
| +// formatter both the adopt pass's nag (internal/release/adopt.go) and the |
| +// console's badge (internal/console/ladder.go) call, so the two |
| +// operator-facing surfaces of this feature can never drift into disagreeing |
| +// about what a duration means (2026-08-03 review, IMPORTANT 2) — the way |
| +// they briefly did when the console rounded-and-bucketed a duration the nag |
| +// printed raw and un-subtracted. |
| +// |
| +// "<1h" for anything under a full hour past the TTL — never "0h", which |
| +// reads as "not actually expired" to whoever sees it first (2026-08-03 |
| +// review, MINOR 7). "Nh" rounded to the nearest hour from one hour up to a |
| +// day. "Nd" TRUNCATED (not rounded) at a day and beyond: nobody needs |
| +// minute-level precision on a guard rail whose whole point is that nobody |
| +// was watching the clock, and losing up to 23 hours off the day count is |
| +// the deliberate coarseness that buys — nothing here mistakes a 47-hour |
| +// lapse for two days. |
| +func FormatCanaryAge(d time.Duration) string { |
| + if d < time.Hour { |
| + return "<1h" |
| + } |
| + d = d.Round(time.Hour) |
| + if d < 24*time.Hour { |
| + return fmt.Sprintf("%dh", int(d.Hours())) |
| + } |
| + return fmt.Sprintf("%dd", int(d.Hours())/24) |
| +} |
diff --git a/internal/release/canary_ttl_test.go b/internal/release/canary_ttl_test.go| index 29c4a4e66..f598a079d 100644 |
| --- a/internal/release/canary_ttl_test.go |
| +++ b/internal/release/canary_ttl_test.go |
| @@ -78,3 +78,39 @@ func TestCanaryExpiredNilPointer(t *testing.T) { |
| t.Error("a nil pointer must not be flagged expired") |
| } |
| } |
| + |
| +// ---- CanaryAge / FormatCanaryAge (2026-08-03 review, IMPORTANT 2) --------- |
| + |
| +// CanaryAge must report time PAST the TTL, not total age — a pointer |
| +// CanaryTTL+1h old has been expired for about an hour, not 7 days 1 hour. |
| +func TestCanaryAgeIsTimePastTTLNotTotalAge(t *testing.T) { |
| + now := time.Now() |
| + p := &ChannelPointer{Channel: "canary/x", PromotedAt: now.Add(-(CanaryTTL + time.Hour))} |
| + got := CanaryAge(p, now) |
| + if got < 59*time.Minute || got > 61*time.Minute { |
| + t.Errorf("CanaryAge = %s, want ~1h (total age minus CanaryTTL, not the 169h total age)", got) |
| + } |
| +} |
| + |
| +func TestFormatCanaryAge(t *testing.T) { |
| + cases := []struct { |
| + d time.Duration |
| + want string |
| + }{ |
| + {0, "<1h"}, // the instant it expires |
| + {24 * time.Minute, "<1h"}, // MINOR 7: must not read "0h" |
| + {59 * time.Minute, "<1h"}, |
| + {time.Hour, "1h"}, // review's own example: 7d1h old -> "1h" |
| + {90 * time.Minute, "2h"}, // rounds to nearest hour |
| + {23 * time.Hour, "23h"}, |
| + {24 * time.Hour, "1d"}, |
| + {47 * time.Hour, "1d"}, // deliberate coarseness: truncates, not rounds |
| + {8 * 24 * time.Hour, "8d"}, |
| + {30 * 24 * time.Hour, "30d"}, |
| + } |
| + for _, c := range cases { |
| + if got := FormatCanaryAge(c.d); got != c.want { |
| + t.Errorf("FormatCanaryAge(%s) = %q, want %q", c.d, got, c.want) |
| + } |
| + } |
| +} |
diff --git a/internal/status/fleet.go b/internal/status/fleet.go| index f5d6387cb..7f69d8e92 100644 |
| --- a/internal/status/fleet.go |
| +++ b/internal/status/fleet.go |
| @@ -44,7 +44,11 @@ type FleetBox struct { |
| AdoptionRunning *bool `json:"adoption_running"` |
| // ExpiredCanaries appears per-box for the same reason AdoptionRunning |
| // does: the fleet-wide sum says something is expired somewhere, this |
| - // says on which box, without naming which route. |
| + // says on which box, without naming which route. Published in the FEED |
| + // only for now — the rendered boxes table deliberately has no column |
| + // for it yet (2026-08-03 review, MINOR 4: this comment used to imply |
| + // the table showed it; it does not, and adding the column is deferred |
| + // until an expired canary has actually been hunted via the feed once). |
| ExpiredCanaries *int `json:"expired_canaries"` |
| } |