amadan
status+console: fix review findings on staged_version (#78)
DryRun now scope-splits a dedicated route_versions needle pair (dryRunRouteVersionHost/Version, mirroring dryRunMetricsNeedles) so the self-check actually exercises RouteVersions instead of passing by fixture accident: required inside status/boxes/*.json, banned everywhere else DryRun publishes. The console's staged_version join now asks status.BoxStale before folding a box's RouteVersions in, so a host whose only reporting box has gone dark drops out of the join instead of serving that box's last-known version as current. Doc comments across apimode.go and apiinstances.go reworded to the true contract: fresh-ish while a box keeps reporting, dropped rather than stale once none does.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+119
−33
apimode.go+8 −4internal/console/apiinstances.go+27 −13internal/console/apiinstances_test.go+23 −10internal/status/dryrun.go+61 −6
diff --git a/apimode.go b/apimode.go| index 875edf27f..e6457ba47 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -861,10 +861,14 @@ type apiInstanceRecord struct { |
| CreatedBy string `json:"created_by"` |
| // StagedVersion is the console's additive staged_version join (Task 8, |
| // issue #78): whichever version a box's adopt pass last reported for |
| - // this host, via status/boxes/<label>.json — empty when no box has |
| - // reported it yet. Best-effort, bound by the ~5-minute status flush |
| - // cadence; empty on a create response, since no box has adopted the |
| - // record before this call returns it. |
| + // this host, via status/boxes/<label>.json. Fresh-ish for as long as |
| + // some box keeps reporting the host, but the console drops it from the |
| + // join the moment that box goes stale (status.BoxStale) rather than |
| + // going on serving a dark box's last-known version as if it were |
| + // current — so empty here means EITHER no box has ever reported this |
| + // host, or the one that did has gone silent. Always empty on a create |
| + // response, since no box has adopted the record before this call |
| + // returns it. |
| StagedVersion string `json:"staged_version"` |
| } |
diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go| index d3a5b03dc..1ceeaa606 100644 |
| --- a/internal/console/apiinstances.go |
| +++ b/internal/console/apiinstances.go |
| @@ -165,18 +165,31 @@ type instanceListRecord struct { |
| // StagedVersion is registry.Route.Version as last reported by whichever |
| // box's adopt pass claimed this host, read from that box's own |
| // status/boxes/<label>.json (internal/status's bucket-private Metrics |
| - // block — see its package doc). Empty when no box has reported this |
| - // host yet: a brand new record, or one whose box hasn't ticked its next |
| - // ~5-minute status flush since create. Best-effort and freshness-bound |
| - // by that flush cadence — the deploy watch does NOT read this field. |
| + // block — see its package doc). Empty both for a host no box has ever |
| + // reported AND for one whose last-reporting box has gone stale (see |
| + // stagedVersions below, which asks status.BoxStale the same question |
| + // the operator fleet page does): fresh-ish for as long as some box keeps |
| + // reporting it, but a box that has gone silent drops OUT of the join |
| + // rather than going on serving that box's last-known version as if it |
| + // were current. The deploy watch does NOT read this field. |
| StagedVersion string `json:"staged_version"` |
| } |
| -// stagedVersions builds a host -> version map from every box's own private |
| -// RouteVersions (status/boxes/*.json), through status.LoadBoxes — the same |
| -// loader and the same skip-corrupt-object rule the operator fleet page's |
| -// HostRows reads through (internal/console/view.go), so this join and that |
| -// page can never disagree about what a box last reported. |
| +// stagedVersions builds a host -> version map from every NON-STALE box's own |
| +// private RouteVersions (status/boxes/*.json), through status.LoadBoxes — |
| +// the same loader the operator fleet page's HostRows reads through |
| +// (internal/console/view.go) — and status.BoxStale, the fleet's one |
| +// staleness rule (internal/status/fleet.go: "Every caller ... asks this |
| +// function, never its own subtraction"). A box that stopped reporting is |
| +// EXCLUDED from the join entirely rather than contributing its last-known |
| +// reading: the alternative would let a route adopted a week ago on a now- |
| +// dark box keep answering staged_version queries as if it were as current |
| +// as a box that reported thirty seconds ago, which is exactly the kind of |
| +// reassuring-but-wrong answer internal/status's whole design refuses to |
| +// give (see BoxStale's own doc). now is passed in, not read internally, so |
| +// this and BuildFleet/HostRows always vote on staleness using the request's |
| +// own clock rather than three independent time.Now() calls that could |
| +// disagree at a period boundary. |
| // |
| // A store that cannot be listed at all — not one corrupt object, the whole |
| // read failing — degrades to "no staged versions known" rather than |
| @@ -186,7 +199,7 @@ type instanceListRecord struct { |
| // today — instanceGate already refuses those with a 503 before this ever |
| // runs — but the nil check is kept here too so this helper degrades safely |
| // for any future caller that reaches it before that gate. |
| -func stagedVersions(ctx context.Context, s release.Store) map[string]string { |
| +func stagedVersions(ctx context.Context, s release.Store, now time.Time) map[string]string { |
| if s == nil { |
| return nil |
| } |
| @@ -197,7 +210,7 @@ func stagedVersions(ctx context.Context, s release.Store) map[string]string { |
| } |
| versions := map[string]string{} |
| for _, b := range boxes { |
| - if b.Metrics == nil { |
| + if b.Metrics == nil || status.BoxStale(b, now, status.SnapshotPeriod) { |
| continue |
| } |
| for host, v := range b.Metrics.RouteVersions { |
| @@ -209,7 +222,8 @@ func stagedVersions(ctx context.Context, s release.Store) map[string]string { |
| // handleAPIInstanceList answers `carlos instances list`: this app's records, |
| // capped and honest about the cap, each carrying its best-effort |
| -// staged_version (Task 8, issue #78). |
| +// staged_version (Task 8, issue #78) — fresh-ish while some box keeps |
| +// reporting the host, dropped rather than stale once none does. |
| func (s *Server) handleAPIInstanceList(w http.ResponseWriter, r *http.Request) { |
| _, acct, app, ok := s.instanceGate(w, r) |
| if !ok { |
| @@ -223,7 +237,7 @@ func (s *Server) handleAPIInstanceList(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusBadGateway, "could not list instances") |
| return |
| } |
| - versions := stagedVersions(ctx, s.cfg.Releases) |
| + versions := stagedVersions(ctx, s.cfg.Releases, time.Now()) |
| out := make([]instanceListRecord, len(recs)) |
| for i, rec := range recs { |
| out[i] = instanceListRecord{Record: rec, StagedVersion: versions[rec.Host]} |
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go| index 9ef699780..94d6c648b 100644 |
| --- a/internal/console/apiinstances_test.go |
| +++ b/internal/console/apiinstances_test.go |
| @@ -433,9 +433,12 @@ func TestAPIInstanceListAndGet(t *testing.T) { |
| } |
| // TestAPIInstanceListStagedVersion (issue #78): the list join. A record |
| -// whose host was named in a box's private status/boxes/<label>.json |
| +// whose host was named in a FRESH box's private status/boxes/<label>.json |
| // RouteVersions carries that box's reported version as staged_version; a |
| -// record no box has reported yet gets the empty string; and neither a |
| +// record no box has ever reported gets the empty string; a record whose |
| +// only reporting box has gone STALE (status.BoxStale — the same rule the |
| +// operator fleet page's HostRows votes with) is EXCLUDED from the join, not |
| +// served that box's last-known reading as if it were current; and neither a |
| // garbage object under status/boxes/ nor an entirely absent one turns the |
| // list into a 500 — the join is best-effort observability layered onto the |
| // record at read time, never load-bearing for the list itself. |
| @@ -443,7 +446,7 @@ func TestAPIInstanceListStagedVersion(t *testing.T) { |
| fx, token := instanceFixture(t, "code-inst-staged") |
| enableInstances(t, fx, "titogo", titoManifest()) |
| u := instancesURL(fx, "bab", "titogo") |
| - for _, h := range []string{"acme.go.tito.io", "beta.go.tito.io"} { |
| + for _, h := range []string{"acme.go.tito.io", "beta.go.tito.io", "gamma.go.tito.io"} { |
| if code, _, out := rawReq(t, http.MethodPost, u, token, |
| fmt.Sprintf(`{"host":%q,"region":"ie"}`, h)); code != http.StatusCreated { |
| t.Fatalf("create %s = %d %s", h, code, out) |
| @@ -471,11 +474,18 @@ func TestAPIInstanceListStagedVersion(t *testing.T) { |
| } |
| } |
| - // Seed a box snapshot naming acme's staged version — beta gets none — |
| - // and a garbage object alongside it that must be skipped, not fatal. |
| - box := status.CollectBox(nil, status.Probes{MachineID: "box1"}, time.Now()) |
| - box.Metrics = &status.BoxMetrics{RouteVersions: map[string]string{"acme.go.tito.io": "abc1234"}} |
| - if err := status.WriteBox(context.Background(), fx.rel, box); err != nil { |
| + // Seed a FRESH box snapshot naming acme's staged version, a STALE one |
| + // (older than status.SnapshotPeriod x 3 — see status.BoxStale) naming |
| + // gamma's, and a garbage object alongside them that must be skipped, not |
| + // fatal. beta gets no box at all. |
| + fresh := status.CollectBox(nil, status.Probes{MachineID: "box1"}, time.Now()) |
| + fresh.Metrics = &status.BoxMetrics{RouteVersions: map[string]string{"acme.go.tito.io": "abc1234"}} |
| + if err := status.WriteBox(context.Background(), fx.rel, fresh); err != nil { |
| + t.Fatal(err) |
| + } |
| + stale := status.CollectBox(nil, status.Probes{MachineID: "box2"}, time.Now().Add(-time.Hour)) |
| + stale.Metrics = &status.BoxMetrics{RouteVersions: map[string]string{"gamma.go.tito.io": "deadbeef"}} |
| + if err := status.WriteBox(context.Background(), fx.rel, stale); err != nil { |
| t.Fatal(err) |
| } |
| if _, err := fx.rel.Put(context.Background(), "status/boxes/broken.json", []byte("{not json"), ""); err != nil { |
| @@ -484,7 +494,7 @@ func TestAPIInstanceListStagedVersion(t *testing.T) { |
| code, _, out = rawReq(t, http.MethodGet, u, token, "") |
| if code != http.StatusOK { |
| - t.Fatalf("list with a seeded box and a garbage object = %d %s, want 200", code, out) |
| + t.Fatalf("list with seeded boxes and a garbage object = %d %s, want 200", code, out) |
| } |
| if err := json.Unmarshal([]byte(out), &list); err != nil { |
| t.Fatalf("list body %s: %v", out, err) |
| @@ -494,11 +504,14 @@ func TestAPIInstanceListStagedVersion(t *testing.T) { |
| got[r.Host] = r.StagedVersion |
| } |
| if got["acme.go.tito.io"] != "abc1234" { |
| - t.Errorf("acme staged_version = %q, want abc1234", got["acme.go.tito.io"]) |
| + t.Errorf("acme staged_version = %q, want abc1234 (its box is fresh)", got["acme.go.tito.io"]) |
| } |
| if got["beta.go.tito.io"] != "" { |
| t.Errorf("beta staged_version = %q, want empty (no box has reported it)", got["beta.go.tito.io"]) |
| } |
| + if got["gamma.go.tito.io"] != "" { |
| + t.Errorf("gamma staged_version = %q, want empty — its only reporting box is stale and must not be joined", got["gamma.go.tito.io"]) |
| + } |
| } |
| // TestAPIInstanceDelete: deletion is idempotent (the caller's goal is "not |
diff --git a/internal/status/dryrun.go b/internal/status/dryrun.go| index 14db89cc1..4143f75a0 100644 |
| --- a/internal/status/dryrun.go |
| +++ b/internal/status/dryrun.go |
| @@ -65,6 +65,30 @@ var dryRunMetricsNeedles = []string{ |
| "reconnecting", // fixture Tunnel, verbatim (a real, valid Tunnel value) |
| } |
| +// dryRunRouteVersionHost and dryRunRouteVersionVersion are Task 8's own |
| +// needle pair (#78): a THIRD un-opted route, distinct from leakyapp and |
| +// svc/internal above, whose only purpose is carrying a Version — the one |
| +// thing that makes CollectBox populate Box.Metrics.RouteVersions, this |
| +// package's one hostname-keyed field. Neither string reuses any existing |
| +// needle (not dryRunNeedles, not dryRunMetricsNeedles, not demoapp's own |
| +// "abc1234"): route_versions gets its own dedicated positive/negative check, |
| +// scope-split exactly like dryRunMetricsNeedles below, rather than riding |
| +// along on an assertion built for something else. |
| +const ( |
| + dryRunRouteVersionHost = "staged-secret.internal" |
| + dryRunRouteVersionVersion = "stagedver9012" |
| +) |
| + |
| +// dryRunRouteVersionNeedles is the scope-split pair for the fixture above: |
| +// REQUIRED inside status/boxes/*.json (proof that RouteVersions actually |
| +// attached — an unexercised field is a field this self-check can't vouch |
| +// for) and BANNED everywhere else DryRun publishes (proof that a |
| +// route_versions hostname, unlike a band word or an aggregate, never |
| +// reaches fleet.json or the rendered page). Same reasoning and the same |
| +// scan-loop shape as dryRunMetricsNeedles, because RouteVersions lives in |
| +// the identical private home: Box.Metrics. |
| +var dryRunRouteVersionNeedles = []string{dryRunRouteVersionHost, dryRunRouteVersionVersion} |
| + |
| // ScanForLeaks reports which of banned's strings appear anywhere in j. It is |
| // the single implementation both DryRun's self-check and |
| // contract_test.go's scanForLeaks test helper run — the test package can't |
| @@ -82,12 +106,13 @@ func ScanForLeaks(j []byte, banned []string) []string { |
| return found |
| } |
| -// dryRunFixtures builds a registry and probe set stuffed with needles: two |
| -// un-opted hosts (one instance, one service) that must vanish entirely, one |
| -// opted-in app that must survive by name, and mid-band fractions — 0.73 and |
| -// 0.95 sit inside the "busy"/"at-capacity" bands per snapshot.go's |
| -// 0.70/0.90 cuts, so banding actually gets exercised rather than trivially |
| -// cleared at an edge. |
| +// dryRunFixtures builds a registry and probe set stuffed with needles: three |
| +// un-opted hosts (one instance, one service, and one instance whose only job |
| +// is carrying a Version — see dryRunRouteVersionNeedles) that must vanish |
| +// from every public surface, one opted-in app that must survive by name, and |
| +// mid-band fractions — 0.73 and 0.95 sit inside the "busy"/"at-capacity" |
| +// bands per snapshot.go's 0.70/0.90 cuts, so banding actually gets exercised |
| +// rather than trivially cleared at an edge. |
| func dryRunFixtures() ([]registry.Route, Probes) { |
| routes := []registry.Route{ |
| { |
| @@ -102,6 +127,14 @@ func dryRunFixtures() ([]registry.Route, Probes) { |
| Host: "svc-secret.example.com", Kind: "service", App: "svc/internal", |
| Status: "ready", |
| }, |
| + { |
| + // Un-opted, so its host must not surface via the apps feed either |
| + // — the ONLY reason this route exists is to give CollectBox a |
| + // Version to fold into Box.Metrics.RouteVersions (see the needle |
| + // pair's own doc comment above). |
| + Host: dryRunRouteVersionHost, Kind: "instance", App: "stagedapp", |
| + Status: "ready", Version: dryRunRouteVersionVersion, |
| + }, |
| } |
| memFrac, diskFrac := 0.73, 0.95 |
| @@ -239,6 +272,11 @@ func DryRun(dir string, out io.Writer) error { |
| // the opt-in app: a leak-scan that finds nothing proves nothing about |
| // whether the thing that SHOULD be there actually is. |
| boxKeySeen := false |
| + // routeVersionKeySeen is boxKeySeen's sibling for Task 8's own scoped |
| + // pair (dryRunRouteVersionNeedles): whether the scan actually reached a |
| + // status/boxes/ object carrying the route_versions fixture, so this |
| + // self-check can't pass by simply never exercising RouteVersions. |
| + routeVersionKeySeen := false |
| for _, key := range keys { |
| body, _, err := s.Get(ctx, key) |
| @@ -270,6 +308,20 @@ func DryRun(dir string, out io.Writer) error { |
| return fmt.Errorf("dry run: %s leaked raw Metrics fixture %v — Box.Metrics must never reach the public feed", key, leaks) |
| } |
| + // dryRunRouteVersionNeedles gets the identical scope split as |
| + // dryRunMetricsNeedles just above, for the identical reason: |
| + // RouteVersions lives in Box.Metrics too, so status/boxes/*.json is |
| + // its one privileged home and every other published key must never |
| + // carry it. |
| + if strings.HasPrefix(key, "status/boxes/") { |
| + if leaks := ScanForLeaks(body, dryRunRouteVersionNeedles); len(leaks) == 0 { |
| + return fmt.Errorf("dry run: %s: expected route_versions fixtures %v, found none — RouteVersions not attached?", key, dryRunRouteVersionNeedles) |
| + } |
| + routeVersionKeySeen = true |
| + } else if leaks := ScanForLeaks(body, dryRunRouteVersionNeedles); len(leaks) > 0 { |
| + return fmt.Errorf("dry run: %s leaked a route_versions fixture %v — RouteVersions must never reach the public feed", key, leaks) |
| + } |
| + |
| // Page assets (index.html, status.css) aren't JSON and simply fail |
| // to decode here — silently skipped, since the band-digit check |
| // only means anything for objects that actually carry Load/Capacity |
| @@ -285,6 +337,9 @@ func DryRun(dir string, out io.Writer) error { |
| if !boxKeySeen { |
| return fmt.Errorf("dry run: no status/boxes/*.json object was scanned — Metrics scope check never ran") |
| } |
| + if !routeVersionKeySeen { |
| + return fmt.Errorf("dry run: no status/boxes/*.json object was scanned — route_versions scope check never ran") |
| + } |
| fmt.Fprintln(out, "dry-run: no leaks") |
| return nil |