Catalog: fetch browse data and install payloads separately - #436
Conversation
The box pulled one bulk snapshot carrying every published app's verbatim manifest, compose and resolved images map. On a 43-app catalog that was 614KB, of which 77% was install payload for apps the box would never install; the browse data the store renders was 106KB. Browse is now GET /catalog?env=<environment>, filtered server-side to the box's own surface. An app's manifest and compose are fetched per app at install time by following the manifest_url / compose_url its record carries, so Load takes a context. Drop the index digest. It was recomputed by re-marshalling the parsed app index, which made field order load-bearing and turned any new published field into a flag day: the digest stopped matching, verify() refused the snapshot, and the box showed an empty store. Measured on external_costs, not theoretical. It closed no threat TLS and HTTP framing do not already close, so `version` replaces it as an opaque token the box never recomputes, and unknown keys are dropped like encoding/json drops them everywhere else. The schema_version refusal stays. Read an installed app's manifest from its own persisted copy. Load was never install-only: the detail page's mail picker and the mail rebind pre-check called it for apps already installed, and a live fetch there would put a routine page load behind the catalog service. Both now read life.InstanceManifest, which also fixes an installed app's manifest vanishing the moment the app is unpublished. Treat every published URL as opaque, so artwork and documents can move to another origin. The asset cache name is derived from the URL, and assetCachePath contains the app id, which safeJoin did not. Closes #434.
The three docs disagreed about when a self-review runs. CLAUDE.md and code-review.md put it after the PR opens and make Greptile mandatory; contributing.md put it before and never mentioned Greptile at all. Greptile only runs on an open PR, so a review done before pushing cannot include it — a contributor following Step 6 got the agent half and stopped, which is a self-review code-review.md calls unfinished. Swap the two steps and say why. Step 7 now names both halves, carries the "empty comment list means not posted yet" warning and the #435 example that previously lived only in code-review.md, and puts the fixup-commits-on-the-same-branch rule where it bites. Fix the two stale cross-references: code-review.md said Step 6, the PR template said Step 7.
The per-app-field checkbox told contributors that adding a field inside an app rejects the whole snapshot on every box that does not model it, so the data may only be published after a release ships. The index digest that caused that is gone, so the warning now describes behavior the box no longer has.
Confidence Score: 5/5The changes since the previous review appear safe to merge, with the outstanding Docker-live compilation issue fully fixed. The non-tagged
|
| Filename | Overview |
|---|---|
| internal/lifecycle/dockerlive_test.go | All six tagged test callers now load and pass a CatalogApp, fully resolving the previous compilation finding. |
| Makefile | The vet target now type-checks each supported tagged test variant without adding a new environment dependency. |
| docs/progress/catalog-split-browse-and-install.md | Records the resolved review findings and the new tagged-vet safeguard. |
Reviews (3): Last reviewed commit: "Fix the dockerlive Install callers, and ..." | Re-trigger Greptile
Greptile P1 on #436. installApp loaded the manifest to validate the folder, mail and config elections, and the async install job then called Manager.Install, which loaded again. While both documents lived in one bulk snapshot that re-read was cheap and near-enough consistent. Now each is its own fetch, so a republish between the two reads means the job installs a payload the request never validated: a folder mount the elections still name but the new manifest dropped, a newly required config field nobody was asked for, or one publication's manifest paired with another's compose. Install now takes a lifecycle.CatalogApp — the manifest and compose as one value — instead of a manifest id, and LoadCatalogApp is called exactly once, in the request that validates against it. What this does not close: the two documents of a single payload are still two fetches, so one publication's manifest can pair with another's compose. That needs the payload addressable as a unit from the control plane, which is a two-repo change.
|
Greptile P1 — catalog payload loaded twice — is a real bug and is fixed in 0e5f192. You are right about the consequence, and it is worse than the wasted round trip I had recorded it as. While the manifest and compose lived in one bulk snapshot, the job's re-read was cheap and near-enough consistent. Now each is its own fetch, so a republish landing between the request's load and the job's load means the job installs a payload nothing validated: a folder mount the elections still name but the new manifest dropped, a newly required config field the installer was never asked for, or one publication's manifest paired with another's compose. The fix keeps one version-consistent pair for the whole install:
One part I have not closed, deliberately. The two documents of a single payload are still two HTTP fetches, so a publish landing between them can still pair one publication's manifest with another's compose. Closing the request/job window does not close that one — it needs the payload addressable as a unit from the control plane (a version on the document routes, or an ETag the box requires to match across both fetches). That is a contract change in the cloud repo, so it is recorded as a two-repo follow-up in the progress entry rather than patched here. For the record: I had seen this exact code path during an inner-loop run, counted the four fetches, and filed it as an efficiency follow-up. Good catch on what it actually was. |
| // Install runs the install transaction for an already-loaded catalog (Door-1) | ||
| // app. It takes the payload rather than an id so the manifest it installs is | ||
| // byte-for-byte the one the caller validated the elections against. | ||
| func (m *Manager) Install(ctx context.Context, app CatalogApp, owner Owner, scope string, mounts []FolderMount, mailProviderID string, config []store.InstanceConfig, progress func(step string)) (store.Instance, error) { |
There was a problem hiding this comment.
Dockerlive Tests No Longer Compile
Install now requires a CatalogApp, but all six callers in dockerlive_test.go still pass string IDs. As a result, the documented go test -tags dockerlive ./internal/lifecycle/ workflow fails to compile, preventing the live Docker provisioning suite from running. Update those callers to load and pass a CatalogApp, as the other lifecycle tests now do.
Greptile, second pass on #436. The CatalogApp signature change left the six Install callers in dockerlive_test.go passing a string id. `go vet ./...` does not compile a file behind a build tag, so `make check` was green the whole time. The sweep that updated the other ~55 call sites missed this one too: it matched on `context.Background()` and this file passes a ctx variable. Fixing only the file would leave the hole open, so `make vet` now also vets the tagged variants. vet type-checks without running anything, so it needs none of the hardware those tags name — running the tests still does, per TESTING.md.
|
Second-pass finding is right, and it is a regression my own fix introduced — fixed in cffaa78. The
Fixing just the file would leave the hole open for the next signature change, so VET_TAGS := dockerlive usermgrtest avahitest nmtest pamtestThat is cheap and belongs in the gate: vet only type-checks, so it needs none of the hardware those tags name. Actually running those tests still needs the real system each tag stands for, per Verified locally with |
What & why
The box pulled one bulk snapshot (
GET /catalog/sync) carrying every published app's verbatimmanifest.yml,compose.ymland resolved images map. On a 43-app catalog that is 614KB, of which 77% is install payload for apps the box will never install; the browse data the store actually renders is 106KB. Every box paid the full 614KB on first sync and on every catalog change.Browse and install are now two fetches:
GET /catalog?env=<environment>— display records, landing page, category vocabulary, and an opaqueversiontoken. Filtered server-side to the box's surface, so the box applies no visibility pass of its own.GET /catalog/apps/{id}/manifestand/compose— the verbatim file asapplication/yaml, fetched only when the box actually installs that app.Loadtakes a context now.The index digest is gone. It was recomputed by re-marshalling the parsed app index, which made field order load-bearing and turned any new published field into a flag day: the digest stopped matching,
verify()refused the snapshot, and the box showed an empty store. That was measured onexternal_costs, not theoretical. It closed no threat TLS (origin) and HTTP framing (truncation) do not already close, soversionreplaces it as a token the box stores, echoes, and never recomputes, and unknown keys are dropped the wayencoding/jsondrops them everywhere else. Theschema_versionrefusal stays.Installed apps read their own manifest. This is the load-bearing half.
Loadwas never install-only — the detail page's mail picker and the mail rebind pre-check called it for apps already installed — and a live fetch there would put a routine page load behind the catalog service. Both now readlife.InstanceManifest, the copywriteInstanceDiralready persists. That also fixes a pre-existing bug: an installed app's manifest used to disappear the moment the app was unpublished.Published URLs are opaque.
icon_file/screenshotsbecameicon_url/screenshot_urls, joiningmanifest_url/compose_url. The box follows what it is given (absolute URLs on another origin included) and assembles nothing, so artwork or documents can move to object storage without a box-side change.Beyond the issue
Two doc changes ride along at the maintainer's request, rather than as a separate PR:
contributing.mdStep 6/7 swapped. The three docs disagreed:CLAUDE.mdandcode-review.mdput the self-review after the PR opens and make Greptile mandatory;contributing.mdput it before and never mentioned Greptile. Greptile only runs on an open PR, so a pre-push review structurally cannot include it. Step 7 now names both halves.Spec(s) touched
docs/specs/APP_STORE.md— banners, # Failure modes, # What we run, # Landing page, # Category labels, # What the box models (rewritten), # Locked decisions.docs/specs/DECISIONS.md— new entry 2026-09-04. The 2026-07-02 entry's "TLS + integrity digest" trust story loses its second half.docs/specs/NEXT.md,docs/architecture.md— kept in step.docs/progress/catalog-split-browse-and-install.md.What was tested
make checkgreen, minus the pre-existinglibpam0g-devgap on my machine (pamverifier/cmd/host-agent-realdo not build there; neither is touched by this change). Everything else ingo test ./...passes.make dev— not unit tests alone:mkcatalogseed file (the air-gapped lane's path): installedwhoamiend-to-end, served onwhoami.local.catalog: synced browse payload, endpoint loggedBROWSE env='appliance'plusDOC /catalog/apps/whoami/manifestand/composeonly at install time, install completed, app served through Caddy.Load, absolute document URLs on another origin, 404-vs-500 document failures, seed-file inline payload, opaque-URL asset caching and containment, empty screenshot slots, unknown-key tolerance, opaque version token.bash -nclean on both changed bootstrap scripts.Known gaps & deviations
APP_STORE.md# Failure modes.dev/test-qemu/dev/cloud/testneed root + KVM and are not in the normal loop. Both scripts changed only by dropping one flag. Validated on the nextCI / Cloud imagerun.installApploads to validate elections, thenManager.Installloads again. Found in the live run. Correct but wasteful; it is a change to the install call chain, so it is logged as a follow-up rather than widened into this PR.assetCachePathdid not contain the app id — a hole the oldsafeJoinhad too;detailOfAppemitted URLs for empty screenshot slots). Greptile's half is still pending on this PR and I will work it next.Platform gaps
None.
Closes #434