amadan

carlos / platform Public

console: stop tag-sweeping every page, propagate API tag-read errors

Review fixes on the releases-page tag work:

- assembleChannels no longer calls tagByRelease — that builder is shared by
  every app-page tab and the account home page's whole app list, so a full
  ListTags sweep (List + GetTag per tag) was paid by every render and used
  by none except the releases tab. tagByRelease is split into a pure fold
  (tagIndex, no read) and a fetch-and-fold (tagByRelease, now (map, error)).
  handleAppReleases does ONE ListTags read and backfills ChannelView.Tag
  itself after Assemble returns.
- handleAPIReleasesList restores its 15s context timeout and now propagates
  a broken tags read as the same 502 every other store failure in the file
  gets, instead of silently answering 200 with every tag missing.
- Added tagsCap (20) so the Tags section renders only the newest tags,
  matching versionsCap's convention.
- New tests: TestPromoteFlashNamesTagWarning (a non-iteration stable
  promote's flash carries the TagWarning parenthetical, the other half of
  the carry-forward binding), TestReleasesPageNoTagsSection (no tags cut ->
  no Tags heading), TestAPIReleasesListTagsStoreFailure (broken tags store
  -> 502, not a wrong 200).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 05f5158b1e083858b82b779e7342357f8c5f4516 parent 28d90ed
5 files changed, +168 −32
  • internal/console/apiversion.go +14 −5
  • internal/console/apiversion_test.go +44 −0
  • internal/console/server.go +21 −9
  • internal/console/server_test.go +46 −0
  • internal/console/view.go +43 −18
diff --git a/internal/console/apiversion.go b/internal/console/apiversion.go
index 1a96b73db..f1c4fc3dd 100644
--- a/internal/console/apiversion.go
+++ b/internal/console/apiversion.go
@@ -195,17 +195,26 @@ func (s *Server) handleAPIReleasesList(w http.ResponseWriter, r *http.Request) {
if !ok {
return
}
- // Neither Versions nor tagByRelease takes a context (both read via
- // context.Background() internally, view.go's existing convention for
- // this kind of best-effort page-render read) — so, unlike this file's
- // other handlers, there is no request-scoped deadline to wire through.
+ ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second)
+ defer cancel()
versions, err := Versions(s.cfg.Releases, acct.Sqid, app)
if err != nil {
log.Printf("console: api releases list %s/%s: %v", acct.Sqid, app, err)
jsonError(w, http.StatusBadGateway, "could not read the releases")
return
}
- byRelease := tagByRelease(s.cfg.Releases, acct.Sqid, app)
+ // Unlike the console's own releases page (handleAppReleases, server.go),
+ // which degrades a broken tags read to a tag-less page, an API caller
+ // that asked for tags and got a wrong 200 back with every tag silently
+ // missing would have no way to tell "no tags exist" from "the read
+ // failed" — so this propagates the error to the same 502 every other
+ // store failure in this file gets.
+ byRelease, err := tagByRelease(ctx, s.cfg.Releases, acct.Sqid, app)
+ if err != nil {
+ log.Printf("console: api releases list %s/%s: tags: %v", acct.Sqid, app, err)
+ jsonError(w, http.StatusBadGateway, "could not read the tags")
+ return
+ }
out := make([]map[string]any, 0, len(versions))
for _, v := range versions {
out = append(out, map[string]any{
diff --git a/internal/console/apiversion_test.go b/internal/console/apiversion_test.go
index d4364a256..336f94ccb 100644
--- a/internal/console/apiversion_test.go
+++ b/internal/console/apiversion_test.go
@@ -262,3 +262,47 @@ func TestAPIReleasesList(t *testing.T) {
t.Fatalf("row tag = %v, want 0.5.0", row["tag"])
}
}
+
+// brokenTagsListStore fails every List against a tags/ prefix — a bucket
+// outage on the tag read specifically, not on the releases read ListTags's
+// caller already succeeded at.
+type brokenTagsListStore struct{ release.Store }
+
+const brokenTagsListStoreErr = "AccessDenied: s3://carlos-prod-secret-bucket/tags request id DEF456"
+
+func (b brokenTagsListStore) List(ctx context.Context, prefix string) ([]string, error) {
+ if strings.HasSuffix(prefix, "/tags/") {
+ return nil, errors.New(brokenTagsListStoreErr)
+ }
+ return b.Store.List(ctx, prefix)
+}
+
+// TestAPIReleasesListTagsStoreFailure: view.go's tagByRelease (the fetch
+// form, used here rather than the pure tagIndex fold since this handler has
+// no tag slice of its own) propagates a ListTags failure as an error rather
+// than an empty map — this pins that handleAPIReleasesList turns that error
+// into the same 502 every other broken-store read in this file gets,
+// rather than a 200 whose every row silently claims no tag exists.
+func TestAPIReleasesListTagsStoreFailure(t *testing.T) {
+ fx := newConsoleFixture(t)
+ now := time.Now()
+ if _, err := fx.st.CreateAppIn(OpsAccountID, "km", "paul@keymail.dev", now); err != nil {
+ t.Fatal(err)
+ }
+ token := cliToken(t, fx, "paul@keymail.dev", "code-releases-list-tagsfail")
+ if err := release.Ship(context.Background(), fx.rel, release.OpsAccount, release.Manifest{
+ Schema: 1, App: "km", Version: "0.5.0-1", Kind: "binary", BuiltAt: "2026-08-08T10:00:00Z",
+ Artifacts: map[string]release.Artifact{"linux-amd64": {File: "km"}},
+ }, map[string][]byte{"linux-amd64": []byte("bin")}); err != nil {
+ t.Fatal(err)
+ }
+ fx.srv.cfg.Releases = brokenTagsListStore{fx.rel}
+
+ status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/bab/km/releases", token)
+ if status != http.StatusBadGateway {
+ t.Fatalf("releases list with a broken tags read = %d %v, want 502", status, out)
+ }
+ if msg := str(t, out, "error"); strings.Contains(msg, "AccessDenied") || strings.Contains(msg, "carlos-prod-secret-bucket") {
+ t.Fatalf("502 body leaks the store's own error text: %q", msg)
+ }
+}
diff --git a/internal/console/server.go b/internal/console/server.go
index 623be2101..31ee93dcd 100644
--- a/internal/console/server.go
+++ b/internal/console/server.go
@@ -1415,21 +1415,33 @@ func (s *Server) handleAppReleases(w http.ResponseWriter, r *http.Request) {
log.Printf("console: versions %s: %v", p.AppName, err)
p.FlashErr = "Could not list this app's releases."
}
- // Versions itself never reads ListTags (view.go), so this tab does the
- // join its own template needs: the tag pill on the Releases table, and
- // the Tags section's changelogs below it (release versioning spec,
- // 2026-08-08). tagByRelease is the one place the release->tag map gets
- // built; the raw tag list for the changelog section is a second read of
- // the same tags/ prefix, since a pill needs only the name but the Tags
- // section needs the whole record.
+ // Versions itself never reads ListTags (view.go), and neither does
+ // assembleChannels any more — a full tag sweep thrown away by every
+ // OTHER app-page tab and the account home page's whole app list would
+ // be a needless read on every one of those renders (release versioning
+ // spec, 2026-08-08; view.go's tagByRelease doc comment has the full
+ // story). This tab is the one place that sweep pays for itself: ONE
+ // ListTags read serves the Tags section below (raw records, changelog
+ // and all, capped at tagsCap — ListTags' own semver-descending order),
+ // the Releases table's pill (view.go's tagIndex, the pure fold with no
+ // read of its own), and the Channels table's pill, backfilled onto
+ // Assemble's already-built ChannelView rows since assembleChannels
+ // itself leaves Tag "". A failed read degrades this tab's tag
+ // decoration only; the page still renders.
if tags, terr := release.ListTags(context.Background(), s.cfg.Releases, acct.Sqid, p.AppName); terr != nil {
log.Printf("console: tags %s: %v", p.AppName, terr)
} else {
- p.Tags = tags
- byRelease := tagByRelease(s.cfg.Releases, acct.Sqid, p.AppName)
+ byRelease := tagIndex(tags)
for i := range versions {
versions[i].Tag = byRelease[versions[i].Version]
}
+ for i := range p.Detail.Channels {
+ p.Detail.Channels[i].Tag = byRelease[p.Detail.Channels[i].Version]
+ }
+ if len(tags) > tagsCap {
+ tags = tags[:tagsCap]
+ }
+ p.Tags = tags
}
p.Versions = versions
// The canary form's "how do I give this a URL" copy — this tab's data, not
diff --git a/internal/console/server_test.go b/internal/console/server_test.go
index 901ded9da..37c322fb7 100644
--- a/internal/console/server_test.go
+++ b/internal/console/server_test.go
@@ -2083,3 +2083,49 @@ func TestPromoteFlashNamesTag(t *testing.T) {
t.Fatalf("no-op stable promote flash missing tag name:\n%s", body)
}
}
+
+// TestPromoteFlashNamesTagWarning is the other half of the binding
+// constraint: a version that reaches stable but is NOT a "<target>-<n>"
+// iteration (a sha build, an explicit -version ship) carries no tag —
+// stableTag sets res.TagWarning instead of res.Tag (internal/release's
+// release.go), and the flash must say why, in parentheses, exactly as
+// tagSuffix builds it, not go silent about it.
+func TestPromoteFlashNamesTagWarning(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+ csrf := sessionToken(t, fx, c)
+
+ shipVersion(t, fx, "kass", "v9")
+ postForm(t, c, fx.u("/apps/kass/promote"),
+ url.Values{"csrf": {csrf}, "version": {"v9"}, "new_channel": {"edge"}, "confirmed": {"1"}})
+ postForm(t, c, fx.u("/apps/kass/promote"),
+ url.Values{"csrf": {csrf}, "version": {"v9"}, "new_channel": {"beta"}, "confirmed": {"1"}})
+ postGated(t, fx, c, fx.u("/apps/kass/promote"),
+ url.Values{"csrf": {csrf}, "version": {"v9"}, "channel": {"stable"}, "confirmed": {"1"}})
+
+ _, _, body := get(t, c, fx.u("/apps/kass"))
+ // html/template escapes the flash text, so the literal angle brackets in
+ // TagWarning's "<target>-<n>" arrive as entities.
+ if !strings.Contains(body, "(no tag cut: v9 is not a &lt;target&gt;-&lt;n&gt; iteration)") {
+ t.Fatalf("promote-to-stable flash missing tag warning:\n%s", body)
+ }
+ if strings.Contains(body, " — tagged ") {
+ t.Fatalf("flash claims a tag was cut for a non-iteration version:\n%s", body)
+ }
+}
+
+// TestReleasesPageNoTagsSection: an app with no tags cut renders 200 and
+// omits the Tags section entirely — {{if .Tags}} must not print an empty
+// heading, and a nil/empty p.Tags must not be confused with a read failure.
+func TestReleasesPageNoTagsSection(t *testing.T) {
+ fx := newConsoleFixture(t)
+ c := signInPaul(t, fx)
+
+ status, _, body := get(t, c, fx.u("/apps/kass/releases"))
+ if status != http.StatusOK {
+ t.Fatalf("/apps/kass/releases = %d, want 200", status)
+ }
+ if strings.Contains(body, "<h2>Tags</h2>") {
+ t.Fatalf("Tags section rendered with no tags ever cut:\n%s", body)
+ }
+}
diff --git a/internal/console/view.go b/internal/console/view.go
index b1eafe102..3bc79e6ca 100644
--- a/internal/console/view.go
+++ b/internal/console/view.go
@@ -174,9 +174,14 @@ type ChannelView struct {
// badge is permanent, like the hotfix badge.
FastTrack bool
// Tag is the semver tag pointing at this channel's current release, if
- // any (release versioning spec, 2026-08-08) — set by joining against
- // release.ListTags via tagByRelease. "" for a release nobody tagged, and
- // for every channel below stable, which no tag ever names.
+ // any (release versioning spec, 2026-08-08). assembleChannels — the one
+ // builder every app-page tab AND the account home page's app list share
+ // — deliberately leaves this "": a full ListTags sweep is one List plus
+ // one GetTag per tag, and every render except the releases tab throws
+ // that read away unread. handleAppReleases fills it in, per row, after
+ // Assemble returns, from the one tag read that render already needs for
+ // its own Tags section. "" for a release nobody tagged, for every
+ // channel below stable (no tag ever names one), and on every OTHER tab.
Tag string
// Err degrades a missing or invalid channel pointer to one row's error
// text instead of a failed page.
@@ -445,19 +450,14 @@ func assembleCanaries(channels []ChannelView, routes []RouteView) []CanaryView {
return out
}
-// tagByRelease maps release version -> tag version, one bucket list
-// (release versioning spec, 2026-08-08). This is the ONE place the join
-// between a shipped release and the tag that names it lives — every caller
-// that wants to know "does this release carry a tag" (assembleChannels
-// below, and apiversion.go's handleAPIReleasesList) reads this map rather
-// than building its own from release.ListTags. A listing failure degrades
-// to a nil map (every lookup then reports "no tag"), matching the rest of
-// this file's read-degrades-gracefully posture for a page render.
-func tagByRelease(s release.Store, acct, app string) map[string]string {
- tags, err := release.ListTags(context.Background(), s, acct, app)
- if err != nil {
- return nil
- }
+// tagIndex folds an already-read tag list into a release version -> tag
+// version map (release versioning spec, 2026-08-08) — the pure join, with
+// no store read of its own. This is the ONE place the fold lives: a caller
+// that already has a []release.Tag in hand (handleAppReleases, which reads
+// tags once for both its Tags section and this map) calls this directly
+// rather than re-listing just to build the same map tagByRelease below
+// would.
+func tagIndex(tags []release.Tag) map[string]string {
m := make(map[string]string, len(tags))
for _, t := range tags {
m[t.Release] = t.Version
@@ -465,6 +465,27 @@ func tagByRelease(s release.Store, acct, app string) map[string]string {
return m
}
+// tagByRelease is tagIndex plus the read, for a caller with no tag slice of
+// its own yet (apiversion.go's handleAPIReleasesList). Unlike a page
+// render's usual degrade-and-continue posture, a listing failure here is
+// RETURNED rather than swallowed into an empty map: an API caller that
+// asked for tags and silently got none back would see a wrong 200 instead
+// of the 502 a broken store deserves.
+//
+// Deliberately NOT called from assembleChannels: that builder is shared by
+// every app-page tab and the account home page's whole app list, and a full
+// ListTags sweep (one List, one GetTag per tag) thrown away by every
+// caller except the releases tab would be a full tag-history read on every
+// page view. handleAppReleases fills ChannelView.Tag itself, after Assemble
+// returns, from the one read that render already needs.
+func tagByRelease(ctx context.Context, s release.Store, acct, app string) (map[string]string, error) {
+ tags, err := release.ListTags(ctx, s, acct, app)
+ if err != nil {
+ return nil, err
+ }
+ return tagIndex(tags), nil
+}
+
// assembleChannels lists the app's account-qualified channel pointers and
// resolves each one. An app whose objects are still at the legacy unqualified
// keys lists as empty here, exactly as the fleet sees it — the console showing
@@ -476,7 +497,6 @@ func assembleChannels(ctx context.Context, s release.Store, pub ed25519.PublicKe
if err != nil {
return []ChannelView{{Name: "(all)", Err: "listing channels: " + err.Error()}}
}
- tags := tagByRelease(s, acct, app)
var names []string
for _, k := range keys {
name := strings.TrimPrefix(k, prefix)
@@ -497,7 +517,6 @@ func assembleChannels(ctx context.Context, s release.Store, pub ed25519.PublicKe
Name: name, Version: p.Version, PromotedAt: p.PromotedAt,
HasHistory: len(p.History) > 0,
FastTrack: p.FastTrack,
- Tag: tags[p.Version],
}
// The trail is newest first, but a rollback carries it forward
// without appending: the entry that explains the CURRENT version is
@@ -676,6 +695,12 @@ func humanAge(d time.Duration) string {
// versionsCap bounds the app page's release list.
const versionsCap = 20
+// tagsCap bounds the releases tab's Tags section the same way versionsCap
+// bounds the table above it. release.ListTags is already semver-descending,
+// so this is a plain slice truncate — the newest tagsCap tags, not an
+// arbitrary subset.
+const tagsCap = 20
+
// Versions lists an app's shipped releases — the distinct version dirs under
// apps/<app>/releases/ with each manifest's kind and build stamp — newest
// first by built_at (best effort: RFC3339 parses win, then version string),