amadan

carlos / platform Public

console: make the architecture diagram readable, and stop the page claiming an empty estate

Review round on Task 7. Two Important, three folded-in fixes.

The diagram was unreadable. RenderArchSVG emitted a viewBox and no
width/height, so the SVG had no intrinsic size: a browser sized it at 100%
of its container and scaled a 1918-unit-wide drawing into the console's
~1024px column — 13px node titles at ~7px — and .archwrap's overflow-x
never fired, because an element sized to its container never overflows it.
It now carries width/height beside the viewBox and renders 1:1, with the
wrapper scrolling when the viewport is narrower. Crossing into
archlayout.go was authorised: a page whose headline artifact cannot be read
has not delivered. The edges are restroked off --muted rather than --rule,
the 1px table-hairline token — the edges carry the whole meaning of the
diagram and were drawn in the console's faintest line.

The empty-box line made exactly the confident negative claim the handler's
own comment forbids: when the fleet or host read failed, Boxes was empty
and the page said "No box has reported to this deployment yet", with the
correction buried far below under "Reads that failed". The two causes of an
empty list are now distinguished — page.ArchBoxesUnread carries which one
it is, because len(Boxes)==0 cannot.

Also here:

  - GET /fleet/architecture was registered with a method prefix, so a POST
    got 405 + Allow and distinguished the route from an unrouted path — a
    hole in this task's own byte-identity promise, in the page the door was
    already registered bare to protect. Both are bare now, method checked
    in-handler. (The same pre-existing hole on /fleet, /fleet/metrics and
    /fleet/economics is deliberately left alone.)
  - StartInfraPass had no test at all, including last round's exit-promptly
    fix. TestStartInfraPassStopsWhenLatched sits for the full hour ticker if
    that check is removed.
  - No committed test rendered a NON-EMPTY box card, leaving every field
    inside {{range .Boxes}} unexercised. TestArchitectureBoxCardShowsEstate
    AndDrift seeds a reporting box, an explicit rate-card entry and a 30-vs-
    40 GB disk disagreement, and asserts the card's real content down to the
    drift chip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 7dc9b80b3f3a2dc0accdb782e1478f35be5334d6 parent 6d60315
8 files changed, +277 −13
  • internal/console/archlayout.go +13 −1
  • internal/console/archlayout_test.go +23 −0
  • internal/console/archpage.go +15 −2
  • internal/console/archpage_test.go +161 −0
  • internal/console/infrapass_test.go +33 −0
  • internal/console/server.go +15 −7
  • internal/console/templates/architecture.html +6 −1
  • internal/console/templates/layout.html +11 −2
diff --git a/internal/console/archlayout.go b/internal/console/archlayout.go
index fa5ddb166..e12c1b503 100644
--- a/internal/console/archlayout.go
+++ b/internal/console/archlayout.go
@@ -157,9 +157,21 @@ func layout(v *ArchView) {
// a tag or a label goes through template.HTMLEscapeString — these are
// operator-supplied values from a registry and a cloud account, and the one
// thing this page must never do is execute one of them.
+//
+// width/height are emitted ALONGSIDE the viewBox, and they are what make the
+// picture readable. An <svg> carrying only a viewBox has no intrinsic size,
+// so a browser sizes it at 100% of its container and scales the drawing to
+// fit: a full estate is 1918 units wide and the console's main column is
+// ~1024px, which renders 13px node titles at ~7px and no scrollbar ever
+// appears, because the SVG never overflows anything. With an intrinsic size
+// the drawing renders 1:1 and .archwrap (overflow-x: auto) scrolls when the
+// viewport is narrower — legible everywhere, at the cost of a sideways
+// scroll on the widest graphs, which is the right trade for a diagram whose
+// labels are the content.
func RenderArchSVG(v ArchView) template.HTML {
var b strings.Builder
- fmt.Fprintf(&b, `<svg class="archsvg" viewBox="0 0 %d %d" role="img" aria-label="Deployment architecture">`, v.Width, v.Height)
+ fmt.Fprintf(&b, `<svg class="archsvg" width="%d" height="%d" viewBox="0 0 %d %d" role="img" aria-label="Deployment architecture">`,
+ v.Width, v.Height, v.Width, v.Height)
pos := map[string]Node{}
for _, n := range v.Nodes {
pos[n.ID] = n
diff --git a/internal/console/archlayout_test.go b/internal/console/archlayout_test.go
index 579be79fc..bc9c5d895 100644
--- a/internal/console/archlayout_test.go
+++ b/internal/console/archlayout_test.go
@@ -1,6 +1,7 @@
package console
import (
+ "fmt"
"strings"
"testing"
@@ -90,3 +91,25 @@ func TestLayoutEmptyGraphHasValidViewBox(t *testing.T) {
t.Error("viewBox must never carry a negative dimension")
}
}
+
+// The SVG carries an intrinsic size, not a viewBox alone. Without width and
+// height a browser has nothing to size the element by, so it fills its
+// container and scales the drawing down to fit: a full estate is ~1918 units
+// wide against a ~1024px column, which renders 13px node titles at ~7px and
+// never overflows, so the wrapper's scrollbar never appears either. The
+// labels ARE the content of this diagram, so the size attributes are what
+// make it a diagram rather than a texture.
+func TestSVGCarriesIntrinsicSize(t *testing.T) {
+ v := BuildArch(archFixture())
+ out := string(RenderArchSVG(v))
+ if want := fmt.Sprintf(`width="%d" height="%d"`, v.Width, v.Height); !strings.Contains(out, want) {
+ t.Errorf("SVG must carry %s so it renders 1:1 and .archwrap scrolls; got:\n%s", want, out)
+ }
+ if !strings.Contains(out, fmt.Sprintf(`viewBox="0 0 %d %d"`, v.Width, v.Height)) {
+ t.Error("the viewBox must still match the intrinsic size")
+ }
+ // Deterministic with the attributes as it was without them.
+ if out != string(RenderArchSVG(BuildArch(archFixture()))) {
+ t.Error("two renders of the same input differ")
+ }
+}
diff --git a/internal/console/archpage.go b/internal/console/archpage.go
index b561d06b6..ff809358f 100644
--- a/internal/console/archpage.go
+++ b/internal/console/archpage.go
@@ -21,6 +21,14 @@ import (
)
func (s *Server) handleArchitecture(w http.ResponseWriter, r *http.Request) {
+ // Method-checked here rather than by a "GET /path" mux pattern: that
+ // pattern answers a POST with 405 + Allow, which tells an unauthenticated
+ // prober this route exists — the one thing operatorOnly's bare 404 is
+ // there to withhold. HEAD is GET without a body and is served as such.
+ if r.Method != http.MethodGet && r.Method != http.MethodHead {
+ http.NotFound(w, r)
+ return
+ }
addr, raw, ok := s.operatorOnly(w, r)
if !ok {
return
@@ -32,8 +40,10 @@ func (s *Server) handleArchitecture(w http.ResponseWriter, r *http.Request) {
// A read that failed is SAID, not only logged. An empty Boxes section
// with nothing beside it reads as "this deployment has no boxes", which
// is the one claim a page whose bucket read just failed has no business
- // making.
+ // making. failed names them at the foot; boxesUnread is the same fact
+ // carried to the place the false claim would otherwise be printed.
var failed []string
+ var boxesUnread bool
// Reads two, three and five all go through the deployment bucket; a
// console configured without one (CARLOS_DEPLOYMENT_DIR-less, legal per
@@ -43,12 +53,14 @@ func (s *Server) handleArchitecture(w http.ResponseWriter, r *http.Request) {
if f, err := FleetView(r.Context(), s.cfg.Releases); err != nil {
log.Printf("console: architecture: fleet view: %v", err)
failed = append(failed, "the published fleet status could not be read")
+ boxesUnread = true
} else {
in.Fleet = f
}
if hosts, err := HostRows(r.Context(), s.cfg.Releases, time.Now()); err != nil {
log.Printf("console: architecture: hosts: %v", err)
failed = append(failed, "the per-box snapshots could not be read")
+ boxesUnread = true
} else {
in.Hosts = hosts
}
@@ -82,7 +94,8 @@ func (s *Server) handleArchitecture(w http.ResponseWriter, r *http.Request) {
if len(failed) > 0 {
v.Partial = append(append([]string{}, v.Partial...), failed...)
}
- p := page{Title: "Architecture", Address: addr, CSRF: raw, Arch: &v, ArchSVG: RenderArchSVG(v)}
+ p := page{Title: "Architecture", Address: addr, CSRF: raw,
+ Arch: &v, ArchSVG: RenderArchSVG(v), ArchBoxesUnread: boxesUnread}
p.Flash, p.FlashErr = s.takeFlash(w, r)
s.render(w, http.StatusOK, "architecture", p)
}
diff --git a/internal/console/archpage_test.go b/internal/console/archpage_test.go
index f6cd4f217..c11420353 100644
--- a/internal/console/archpage_test.go
+++ b/internal/console/archpage_test.go
@@ -9,6 +9,7 @@ package console
import (
"context"
+ "errors"
"net/http"
"net/url"
"path/filepath"
@@ -16,7 +17,11 @@ import (
"testing"
"time"
+ "github.com/carlosframework/platform/internal/economics"
"github.com/carlosframework/platform/internal/infraview"
+ "github.com/carlosframework/platform/internal/registry"
+ "github.com/carlosframework/platform/internal/release"
+ "github.com/carlosframework/platform/internal/status"
)
// TestFleetOperatorOnly's shape against /fleet/architecture: signed out,
@@ -238,3 +243,159 @@ func TestArchitectureNamesAFailedRead(t *testing.T) {
t.Errorf("the failed registry read is not named on the page:\n%s", body)
}
}
+
+// A POST to the PAGE is the same 404 an unrouted path gives. A
+// "GET /fleet/architecture" mux pattern would answer 405 + Allow, which
+// distinguishes this route from a missing one to anyone who guesses the URL
+// — the same hole the refresh door is registered bare to avoid.
+func TestArchitecturePageRefusesOtherMethods(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+ if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+ _, _, ghost := postForm(t, newBrowser(t), fx.ts.URL+"/this-route-does-not-exist", url.Values{})
+
+ st, _, body := postForm(t, c, fx.ts.URL+"/fleet/architecture", url.Values{})
+ if st != http.StatusNotFound || body != ghost {
+ t.Fatalf("POST on the page = %d %q, want the same 404 as a missing route", st, body)
+ }
+}
+
+// archBrokenFleetStore fails exactly the one read the box list comes from,
+// leaving every other read on the page working — the shape of a real bad
+// moment, not a dead bucket.
+type archBrokenFleetStore struct{ release.Store }
+
+func (s archBrokenFleetStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if key == "status/fleet.json" {
+ return nil, "", errors.New("bucket having a bad moment")
+ }
+ return s.Store.Get(ctx, key)
+}
+
+// An empty box list has two causes and the page must not confuse them. With
+// the fleet read broken, "No box has reported" would be a confident negative
+// claim about an estate nobody managed to look at.
+func TestArchitectureUnreadBoxesIsNotAnEmptyEstate(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+ if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+
+ // First, the honest empty state: nothing broken, no box reporting.
+ if st, _, body := get(t, c, fx.ts.URL+"/fleet/architecture"); st != 200 ||
+ !strings.Contains(body, "No box has reported to this deployment yet") {
+ t.Fatalf("a genuinely empty fleet = %d, want the empty-state sentence:\n%s", st, body)
+ }
+
+ fx.srv.cfg.Releases = archBrokenFleetStore{fx.rel}
+ st, _, body := get(t, c, fx.ts.URL+"/fleet/architecture")
+ if st != 200 {
+ t.Fatalf("= %d, want 200 — a failed fleet read costs its section, not the page", st)
+ }
+ if strings.Contains(body, "No box has reported to this deployment yet") {
+ t.Errorf("the page claims an empty estate it failed to read:\n%s", body)
+ }
+ if !strings.Contains(body, "The box list could not be read") {
+ t.Errorf("the page must say the box list is unread:\n%s", body)
+ }
+ if !strings.Contains(body, "published fleet status could not be read") {
+ t.Errorf("the failed read must also be named at the foot:\n%s", body)
+ }
+}
+
+// A box card with real content in it: what the estate says the machine is,
+// what the rate card says it costs, the apps and counts CARLOS reports, and
+// the drift chip where the two disagree. Without this every field inside
+// {{range .Boxes}} is unexercised — the fixture's fleet is empty.
+func TestArchitectureBoxCardShowsEstateAndDrift(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+ if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil {
+ t.Fatal(err)
+ }
+ ctx := context.Background()
+
+ // The box CARLOS reports: two static routes, one resident, running a
+ // known build, carrying the fixture's own kass route.
+ if err := status.WriteBox(ctx, fx.rel, status.Box{
+ GeneratedAt: time.Now(),
+ Label: "ada-stargazing",
+ Region: "eu-west-1",
+ Routes: status.RouteCounts{Static: 2, Resident: 1},
+ Metrics: &status.BoxMetrics{
+ Release: "v9.9.9-test",
+ RouteVersions: map[string]string{"kass.carlosapp.dev": "abc123"},
+ },
+ }); err != nil {
+ t.Fatal(err)
+ }
+ reg, err := registry.Open(fx.regPath)
+ if err != nil {
+ t.Fatal(err)
+ }
+ routes, err := reg.List()
+ if err != nil {
+ t.Fatal(err)
+ }
+ f, err := status.BuildFleet(ctx, fx.rel, routes, time.Now(), 5*time.Minute)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := status.PublishFleet(ctx, fx.rel, f); err != nil {
+ t.Fatal(err)
+ }
+
+ // The rate card prices this box explicitly at 30 GB of EBS...
+ sc := economics.DefaultScenario()
+ sc.Name = "test"
+ sc.RateCard.Boxes["ada-stargazing"] = economics.BoxSpec{HourUSD: 0.02, Cores: 2, MemGB: 4, EBSGB: 30}
+ putStoreJSON(t, fx.rel, economics.ScenarioKey("test"), sc)
+ putStoreJSON(t, fx.rel, economics.ActiveKey, economics.Active{Schema: 1, Name: "test"})
+
+ // ...while the estate has 40 GB attached to the machine behind it.
+ fx.srv.inventory = func(context.Context) (infraview.Inventory, error) {
+ return infraview.Inventory{Provider: "aws", Account: "1234", TakenAt: time.Now().UTC(),
+ Resources: []infraview.Resource{
+ {Provider: "aws", Kind: infraview.KindMachine, ID: "i-123", Name: "ada-stargazing", Region: "eu-west-1",
+ Attrs: map[string]string{"type": "t4g.medium", "state": "running"}},
+ {Provider: "aws", Kind: infraview.KindDisk, ID: "vol-1",
+ Attrs: map[string]string{"size_gb": "40", "attached_to": "i-123"}},
+ }}, nil
+ }
+ if err := fx.srv.infraPass(ctx); err != nil {
+ t.Fatal(err)
+ }
+
+ st, _, body := get(t, c, fx.ts.URL+"/fleet/architecture")
+ if st != 200 {
+ t.Fatalf("= %d, want 200", st)
+ }
+ for _, want := range []string{
+ "ada-stargazing", // the card is there at all
+ "eu-west-1", // region
+ "t4g.medium", // discovered instance type
+ "running", // discovered state
+ "4 GB", // rate-card memory
+ "40 GB disk", // discovered disk
+ "/mo", // rate-card price
+ "<code>kass</code>", // the app, joined through the registry
+ "static 2", // fleet counts
+ "resident 1", //
+ "<code>v9.9.9-test</code>", // the release the box reports
+ "chip chip-stale", // drift wears the fact-to-watch chip
+ "rate card says 30 GB of EB", // and says what disagrees
+ "the estate has 40 GB", //
+ } {
+ if !strings.Contains(body, want) {
+ t.Errorf("box card is missing %q in:\n%s", want, body)
+ }
+ }
+ // A box the rate card names explicitly must NOT also report the
+ // priced-by-the-default-spec drift.
+ if strings.Contains(body, "priced by the default spec") {
+ t.Errorf("an explicitly priced box reported default-spec drift:\n%s", body)
+ }
+}
diff --git a/internal/console/infrapass_test.go b/internal/console/infrapass_test.go
index 13e57ddd3..702bc5686 100644
--- a/internal/console/infrapass_test.go
+++ b/internal/console/infrapass_test.go
@@ -107,3 +107,36 @@ func TestLoadInventoryNeverTaken(t *testing.T) {
t.Fatalf("loadInventory = %+v, %q; want nil, \"never\"", inv, why)
}
}
+
+// StartInfraPass leaves as soon as the latch is set, not an hour later. The
+// pass that FLIPS the latch returns its own wrapped error rather than
+// errInfraDisabled — only the NEXT call hits the top-of-function guard — so
+// without the post-pass latch check the goroutine parks on a one-hour ticker
+// with provably nothing left to do. This test would sit for infraPassInterval
+// if that check were removed, which is exactly the regression it exists to
+// catch.
+func TestStartInfraPassStopsWhenLatched(t *testing.T) {
+ fx := newConsoleFixture(t)
+ calls := 0
+ fx.srv.inventory = func(context.Context) (infraview.Inventory, error) {
+ calls++
+ return infraview.Inventory{}, awsinv.ErrNoGrant
+ }
+
+ done := make(chan struct{})
+ go func() {
+ fx.srv.StartInfraPass(context.Background())
+ close(done)
+ }()
+ select {
+ case <-done:
+ case <-time.After(5 * time.Second):
+ t.Fatal("StartInfraPass is still ticking after the latch was set")
+ }
+ if calls != 1 {
+ t.Errorf("provider called %d times, want exactly 1 — the latch is permanent for the process", calls)
+ }
+ if !fx.srv.infraDisabled.Load() {
+ t.Error("the latch was not set")
+ }
+}
diff --git a/internal/console/server.go b/internal/console/server.go
index ccf25ddf2..27cd5fadb 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -803,11 +803,13 @@ func NewServer(cfg Config, st *Store) (*Server, error) {
s.mux.HandleFunc("GET /fleet", s.handleFleet)
s.mux.HandleFunc("GET /fleet/metrics", s.handleFleetMetrics)
s.mux.HandleFunc("GET /fleet/economics", s.handleEconomics)
- // The deployment's map (archpage.go), same operatorOnly gate. The
- // refresh door is registered WITHOUT a method prefix for the reason
- // spelled out below the economics doors: "POST /path" answers a GET
- // with 405 + Allow, which leaks that the route exists.
- s.mux.HandleFunc("GET /fleet/architecture", s.handleArchitecture)
+ // The deployment's map (archpage.go), same operatorOnly gate. BOTH are
+ // registered WITHOUT a method prefix, for the reason spelled out below
+ // the economics doors: a "GET /path" pattern answers a POST with 405 +
+ // Allow, which distinguishes this route from an unrouted one to anyone
+ // who merely guessed the URL — the exact probe the gate exists to
+ // prevent. Each handler checks r.Method itself, first thing.
+ s.mux.HandleFunc("/fleet/architecture", s.handleArchitecture)
s.mux.HandleFunc("/fleet/architecture/refresh", s.handleArchitectureRefresh)
// Per-account table (Task 14): same gate, its own search/filter/cursor
// controls (econlistquery.go) over the same LoadEconomics rows.
@@ -1102,8 +1104,14 @@ type page struct {
// only on paths this handler never takes; ArchSVG is pre-rendered in Go
// rather than assembled in the template because the layout has to be
// deterministic and testable as a whole.
- Arch *ArchView
- ArchSVG template.HTML
+ //
+ // ArchBoxesUnread is why the box list is empty: an empty Boxes with a
+ // failed fleet/host read behind it must say "could not be read", never
+ // "no box has reported". The distinction cannot be made from Boxes
+ // alone — both cases are len 0 — so the handler carries it here.
+ Arch *ArchView
+ ArchSVG template.HTML
+ ArchBoxesUnread bool
// /fleet/economics: the operator unit-economics dashboard (Task 13),
// from LoadEconomics (economicsview.go). nil only on a rendering path
diff --git a/internal/console/templates/architecture.html b/internal/console/templates/architecture.html
index b2e9147b0..ab983677b 100644
--- a/internal/console/templates/architecture.html
+++ b/internal/console/templates/architecture.html
@@ -58,7 +58,12 @@
{{range .Drift}}<p><span class="chip chip-stale">{{.}}</span></p>{{end}}
</section>
{{else}}
-<p>No box has reported to this deployment yet.</p>
+{{/* An empty box list has two causes and they are not the same fact: no box
+ has reported, or the read that would have listed them failed. Saying the
+ first when the second happened is the confident negative claim this page
+ exists to avoid — so the handler carries which one it is. */}}
+{{if $.ArchBoxesUnread}}<p>The box list could not be read — this is not a claim that the deployment has no boxes.</p>
+{{else}}<p>No box has reported to this deployment yet.</p>{{end}}
{{end}}
{{range .Groups}}
diff --git a/internal/console/templates/layout.html b/internal/console/templates/layout.html
index dcd965c98..2a36a9fa8 100644
--- a/internal/console/templates/layout.html
+++ b/internal/console/templates/layout.html
@@ -414,12 +414,21 @@
own wrapper rather than making the page scroll sideways — the drawing
has a minimum readable width the viewport may not have. */
.archwrap { overflow-x: auto; margin: 1rem 0 2rem; }
- .archsvg { width: 100%; min-width: 720px; height: auto; }
+ /* No width rule: the SVG carries its own width/height (RenderArchSVG), so
+ it renders 1:1 and .archwrap scrolls when the viewport is narrower.
+ Sizing it to the container instead would scale a 1918-unit-wide drawing
+ into a ~1024px column and take the 13px labels to ~7px. */
+ .archsvg { display: block; }
.archsvg rect { fill: var(--accent-soft); stroke: var(--rule); }
.archsvg .archnode-box rect { stroke: var(--accent); }
.archsvg .archtitle { fill: var(--ink); font: 600 13px var(--sans); }
.archsvg .archsub { fill: var(--muted); font: 400 11px var(--sans); }
- .archsvg .archedge { fill: none; stroke: var(--rule); stroke-width: 1.5; }
+ /* The edges carry the whole meaning of the diagram — which name reaches
+ which box — so they are stroked off --muted, not --rule. --rule is the
+ 1px table-hairline token (#eae7f0 light, #2c2836 dark): correct for a
+ node's outline sitting on a filled body, invisible for a line that has
+ to be followed across the page. */
+ .archsvg .archedge { fill: none; stroke: var(--muted); stroke-width: 1.5; }
.archbox {
border: 1px solid var(--rule); border-radius: 0.6rem;
padding: 0.9rem 1.1rem; margin: 0 0 0.6rem; max-width: 44rem;