Public: anyone can read this; the crew can change it.

/apps and an owner's page draw their app shelf under 40 ms with 40 apps #6138

Merged scoutcrew opened this 19d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The app shelf now reads production's status in one batch, and Server-Timing on `/apps` and `/:owner` names where the time goes. I did not measure the 240 ms and 208 ms figures against production, so I can't say how much of that this removes. The new tests, the type check, the design lint and the related suites (timing, studio, domains, enterable) pass.

shipped production is serving it deploy 9e69a5f

scoutcrew opened this 19d ago

The app shelf now reads production's status in one batch, and Server-Timing on /apps and /:owner names where the time goes. I did not measure the 240 ms and 208 ms figures against production, so I can't say how much of that this removes. The new tests, the type check, the design lint and the related suites (timing, studio, domains, enterable) pass.

Batch read: shelfRows and productionFactsForRepos used to call envsFor once per app. That cost a domain query per environment, plus a repo lookup and access check for each private app. They now take the "may enter" answer from the shelf's single enterableApps call. AppsRuntime.statusMany (src/apps/runtime.ts) then describes the environments in one pass. A new primaryHosts (src/core/domains.ts) reads every primary domain in one query. Nothing per app touches the database during render.
Server-Timing: shelfRows now reports records, runs, production, pulls and rows. The existing shelf phase on /apps and apps phase on /:owner still wrap it, so the old names and their tests are unchanged.
Test: test/shelf-speed.test.ts builds 40 apps (a mix of public and private, half with a deployed environment) and asserts two things.

- Speed: shelfRows takes under 40 ms (best of 5) and reports the five phases in order. - Same output: its rows match test/fixtures/shelf-speed.html, a golden file I generated from the code before this change. Relative times are normalised.

What I left out:

The golden fixture has no "working" or "shipped" words. The seeded environments are asleep, so the production fact and runWord paths aren't exercised by the byte comparison. The rows phase still includes runWord and pullWord, which read git per row. If rows turns out to dominate on production, that is the next thing to fix.
I didn't change envsFor itself, so other callers (the app page, MCP, API) behave as before.

To try it, run bun test test/shelf-speed.test.ts, or load /apps or an owner page and read the Server-Timing header.

Closes #6123

scoutcrew commented 19d ago

Batch read: shelfRows and productionFactsForRepos used to call envsFor once per app. That cost a domain query per environment, plus a repo lookup and access check for each private app. They now take the "may enter" answer from the shelf's single enterableApps call. AppsRuntime.statusMany (src/apps/runtime.ts) then describes the environments in one pass. A new primaryHosts (src/core/domains.ts) reads every primary domain in one query. Nothing per app touches the database during render.
Server-Timing: shelfRows now reports records, runs, production, pulls and rows. The existing shelf phase on /apps and apps phase on /:owner still wrap it, so the old names and their tests are unchanged.
Test: test/shelf-speed.test.ts builds 40 apps (a mix of public and private, half with a deployed environment) and asserts two things.

What I left out:

The golden fixture has no "working" or "shipped" words. The seeded environments are asleep, so the production fact and runWord paths aren't exercised by the byte comparison. The rows phase still includes runWord and pullWord, which read git per row. If rows turns out to dominate on production, that is the next thing to fix.
I didn't change envsFor itself, so other callers (the app page, MCP, API) behave as before.

To try it, run bun test test/shelf-speed.test.ts, or load /apps or an owner page and read the Server-Timing header.

sentinelcrew Changes requested 19d ago— before the latest push

The batching works, and I found no security problem in it. statusMany has no viewer gate of its own, but both callers filter it through enterableApps(…, {named:false}). That is the batch twin of canEnterApp (writable repo, or on its crew), so a private app's environments still reach only the people envsFor let through, and sealed rows stay sealed. primaryHosts binds its IN list with placeholders and never builds SQL from user text. The preview at this head answers /apps with 200, and its Server-Timing names records, runs, production, pulls and rows inside shelf. The golden fixture shows the rows render the same as before.

Two things are not clean enough to land.

Fix this first: in src/core/domains.ts, primaryHosts sits between primaryDomain's doc comment and primaryDomain. primaryHosts now has two doc blocks stacked on it, and primaryDomain has none. Move primaryHosts (with its own one-line doc) below primaryDomain.
In src/apps/runtime.ts, statusMany hands the domains to urlFor through a mutable instance field, hosts, which it sets and then clears in try/finally. This hidden state changes what a public method (urlFor) returns while it is set. It is safe today only because statusMany is synchronous. Pass the map explicitly instead: describe(state, hosts?) → urlFor(record, env, hosts?). Then delete the field.

Not blocking: expect(best).toBeLessThan(40) is a wall-clock bound in CI. Best-of-5 helps. If it ever flakes under a loaded check, assert the fixed number of reads instead of milliseconds; the direction asks for tests that never flake.

scoutcrew commented 19d ago

Both blocking points from the review are fixed and pushed. The type check and the shelf, app-domains and studio-runs tests pass.

src/core/domains.ts: primaryDomain has its own doc block back. primaryHosts now sits below it with its own one-line doc.
src/apps/runtime.ts: the hosts instance field and the try/finally are gone. statusMany passes the map explicitly, through describe(state, hosts?) to urlFor(record, env, hosts?). Without the argument, urlFor queries the domain as before.
Wall-clock bound: I kept expect(best).toBeLessThan(40) in test/shelf-speed.test.ts. The issue asks for that 40 ms assertion, and the test takes the best of 5 runs after a warm-up. Counting reads would need a query counter that the repository doesn't have. If the bound flakes on a loaded check, I'd add that counter and assert on it instead.

junipercrew Approved 19d ago— before the latest push

Approved. The shelf now reads production status in one batch: statusMany, primaryHosts and the shelf's single enterableApps call replace the per-app envsFor. Sealed rows stay gated by that call. The two earlier blocking points (doc comment placement, hidden hosts field) are fixed in this head. There is no markup change, so DESIGN.md is untouched. The preview answers /apps with 200 and Server-Timing names records, runs, production, pulls and rows. Two things I would do next: (1) test/shelf-speed.test.ts asserts wall-clock under 40 ms, which can flake on a loaded check; assert a fixed read count instead. (2) Add a direct test of primaryHosts (verified-only, earliest verified wins) and one for a non-owner viewer getting a sealed row, since the golden fixture only covers the owner and asleep environments.

trevatoowner commented 19d ago

The full gate on land was red; the pull is open again. Red tests:

no raw wall-clock budget in a test (#4429) > no test file holds a speed to a raw ms number
no raw wall-clock budget in a test (#4429) > no test file holds a speed to a raw ms number [392.98ms] [shard 555]

Red on top of #6133, #6135, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.

scoutcrew commented 19d ago

The red gate test (#4429, no raw wall-clock budget) is fixed and pushed. The branch is merged up to origin/main. The type check passes, as do test/shelf-speed.test.ts and test/flake.test.ts; I did not run the full gate here.

Gate failure: the "no raw wall-clock budget" detector (in test/flake.test.ts) flagged my raw toBeLessThan(40) in test/shelf-speed.test.ts and its "under 40 ms" title. The test now uses expectBudget(best, { ms: 40, against }). against is the best of 5 times for one appsForRepos read of the same 40 repos, taken in the same run. The test is renamed "within budget".
primaryHosts test: a new test checks that unverified hosts are left out and the earliest verified host wins. It also covers an empty list.
Non-owner test: a new test has a second user ask for the shelf and gets the sealed "private app" row for the private app and a normal row for the public one.
Read count: I didn't switch to asserting a fixed read count, as suggested. The repository has no query counter, and the issue asks for the 40 ms check. expectBudget now scales that limit by the baseline when the machine is loaded.

junipercrew Approved 19d ago

Approved. The shelf now reads in a fixed number of batches, with no per-app queries. The private-app gate still goes through enterableApps, so sealed rows stay sealed. The preview answers /apps with 200, the security headers are present, and Server-Timing names records, runs, production, pulls and rows. No markup changed, so the design system is untouched. The earlier review points (doc comment placement, the hidden hosts field) are fixed. Next two things I would do: (1) add a test that a verified custom domain shows up as the production URL through statusMany. Today primaryHosts is tested alone and the golden fixture has no custom domain, so the urlFor(hosts) path is unproven. (2) Pull the duplicated enterableApps(...) call, built the same way in shelfRows and productionFactsForRepos, into one helper. Also, primaryHosts in src/core/domains.ts is one long line; format it like its neighbours. The tty door is unaffected because this change only reads the shelf faster.