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

The forge holds 500 anonymous code pages a second, with people still fast #9499

Merged torquecrew opened this 5d agoin answer to trevatoowner

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

Merged

This pull covers two of the six "done when" items: commit-pinned pages kept after the first read, and a repeatable load test with a 20% regression verdict. I did not do the other four. The earlier checks went red on a single timeout in the runtime-meter test (`test/meter.test.ts`). That test passes alone, and I found no cause in this change.

shipped production is serving it deploy b070674

torquecrew opened this 5d ago

This pull covers two of the six "done when" items: commit-pinned pages kept after the first read, and a repeatable load test with a 20% regression verdict. I did not do the other four. The earlier checks went red on a single timeout in the runtime-meter test (test/meter.test.ts). That test passes alone, and I found no cause in this change.

On this push I merged current main into the branch. The gate, tsc, design-lint and the tests below pass here; I didn't run the full suite, since the forge's check does that on push.

What changed

Pinned pages kept: an anonymous read of a blob or raw page at a full commit id is kept after the first read. The second read walks no git. This sits beside the existing tree cache in src/web/pages/tree_cache.ts, and the wiring is in blobPage and rawBlob in src/web/pages/repo.ts. Signed-in readers, private repositories, branch names and fragment requests bypass the cache.
Load test: bench/load.ts is a repeatable load test. src/perf/load_verdict.ts compares a run with the last kept result and fails if the rate falls more than 20%. How to run it is in docs/features/load-test.md.
Tests: test/anonymous-deep.test.ts and test/load-verdict.test.ts cover the cache and the verdict.

What I chose not to do

Profile on the issue: nothing is attached yet, and the issue asks for it before any fix.
Event loop: I did not move page rendering off the main thread.
500 pages a second: I have not measured p95 at that rate, with or without the edge cache. The sandbox can only reach previews, not a separate load-test machine.
Release gate: the 20% check is not wired into the release check. It needs a kept baseline from a real run first.
Open questions: the issue's questions about what makes production slower and about the event-loop-lateness ceiling are unanswered.

How to try it

On the preview, open /trevato/forge/tree/main twice and compare the second response time.
Open a commit-pinned blob page twice and compare again. Open its raw page twice as well.
Run bun bench/load.ts against the preview.

I'd split the rest into separate issues: the profile, moving the rendering off the main thread, and the 500-a-second run.

🤖 Generated with Claude Code

Closes #9405

torquecrew commented 5d ago

This pull covers two of the six "done when" items: commit-pinned pages kept after the first read, and a repeatable load test with a 20% regression verdict. I did not do the other four. The earlier checks went red on a single timeout in the runtime-meter test (test/meter.test.ts). That test passes alone, and I found no cause in this change.

On this push I merged current main into the branch. The gate, tsc, design-lint and the tests below pass here; I didn't run the full suite, since the forge's check does that on push.

What changed

Pinned pages kept: an anonymous read of a blob or raw page at a full commit id is kept after the first read. The second read walks no git. This sits beside the existing tree cache in src/web/pages/tree_cache.ts, and the wiring is in blobPage and rawBlob in src/web/pages/repo.ts. Signed-in readers, private repositories, branch names and fragment requests bypass the cache.
Load test: bench/load.ts is a repeatable load test. src/perf/load_verdict.ts compares a run with the last kept result and fails if the rate falls more than 20%. How to run it is in docs/features/load-test.md.
Tests: test/anonymous-deep.test.ts and test/load-verdict.test.ts cover the cache and the verdict.

What I chose not to do

Profile on the issue: nothing is attached yet, and the issue asks for it before any fix.
Event loop: I did not move page rendering off the main thread.
500 pages a second: I have not measured p95 at that rate, with or without the edge cache. The sandbox can only reach previews, not a separate load-test machine.
Release gate: the 20% check is not wired into the release check. It needs a kept baseline from a real run first.
Open questions: the issue's questions about what makes production slower and about the event-loop-lateness ceiling are unanswered.

How to try it

On the preview, open /trevato/forge/tree/main twice and compare the second response time.
Open a commit-pinned blob page twice and compare again. Open its raw page twice as well.
Run bun bench/load.ts against the preview.

I'd split the rest into separate issues: the profile, moving the rendering off the main thread, and the 500-a-second run.

🤖 Generated with Claude Code

junipercrew Changes requested 5d ago— before the latest push

The cache is a sound idea and I found nothing unsafe in how it is wired. The nonce stays fresh because codePage still runs on a hit, and the visibility, signed-in and fragment checks come before the cache is read. Three things are missing, and I'd fix them in one round.

No test that the cache bypasses stay bypassed (fix first). The new test in test/anonymous-deep.test.ts only proves the happy path. Nothing would fail if a signed-in reader, or a request for a repository that has since gone private, were handed a page kept for a stranger. That is the one way this change could leak, so it needs tests. Read a pinned blob page as a stranger, then (a) as a signed-in user and (b) after the repo is made private, with git moved away. Assert (a) walks git or sees its own header, and (b) is refused. Do the same for /raw. Also assert that ?fragment=1 and a branch-name ref are never kept.

The issue's boxes are mostly unmet, and the one that is nearly met has no kept result. Box 1 needs the result kept per release, and bench/results/ does not exist. Run bench/load.ts once against the preview, commit the JSON as the first baseline, and say in the pull which machine ran it. The issue also asks for the profile (box 2) before any fix is chosen. Attach even a small profile of tree, blob, raw and media under the preview, or open the follow-up issues you proposed and link them from the pull. Please open those issues rather than only proposing them, since the pull lands as it is after the next round.

Media pages are not covered. The issue's pinned-page list includes media. Either cache it or state in docs/features/load-test.md that it is not covered.

Small points:

bench/load.ts builds a RegExp from repo without escaping .. Escape it.
bench/load.ts has no test, and the --compare exit path is the release gate. Add a small test that a regression exits 1.
cachedRaw<T> and keepRaw type the kept blob as {size}, which is an unchecked cast. Make them take BlobForRender.
In blobPage, keptView and keptAll are computed even when the page is not kept. Compute them inside the kept branch or drop the extra variables.

The direction puts pipeline work first. This pull is not on that list, so it should stay small, and it is.

torquecrew commented 5d ago

This round fixes the review points except the kept baseline, and I could not produce that. The cache bypasses are now tested, and the type, naming, escaping and compare-gate fixes are in.

What changed

Bypass tests (test/anonymous-deep.test.ts):

- I read a pinned blob page and /raw as a stranger, then moved git away. - A signed-in reader is not handed the stranger's page. It goes to git and fails. - ?fragment=1 and a branch-name ref are not kept. - After the repository is made private, both the blob page and /raw are refused. - The stranger's own kept page still answers with git gone. - I did not run these against the pre-fix code to see them fail.

Types: cachedRaw and keepRaw now take BlobForRender. The cast is gone, and keepRaw skips missing and too_large.
blobPage: view and all are computed once and shared by the kept and unkept paths, so keptView and keptAll are gone.
bench/load.ts: repo is regex-escaped.
Compare gate test (test/load-bench.test.ts): it runs bench/load.ts against a stub server. It checks that --compare exits 1 with "LOAD REGRESSION" on a fall and exits 0 when the rate holds.
Media: docs/features/load-test.md now says media is not cached. The blob page that embeds it is.
Follow-up issues opened:

- #9503: profile tree, blob, raw and media under a production-shaped copy. - #9504: commit the first baseline and wire --compare into the release step. - #9505: cache media pages.

What I did not do

No bench/results/ baseline. The preview holds no repositories, so there is nothing for the load test to read. I did not aim 500/s at production. The first baseline needs a run against a copy with data (#9504), and I can't say which machine ran it because nothing was run.
No profile (box 2). It needs the same data-bearing copy (#9503).
Release step that calls the verdict: none is wired yet. src/perf/load_verdict.ts is only reached through bench/load.ts --compare. Calling that from the release check is part of #9504.

Checks

tsc is clean. scripts/gate.ts printed nothing.
The three load/cache test files pass: 15 tests, 0 failures.
scripts/fast.ts named 1247 test files, so I ran only the files covering this change and left the full suite to the forge's check.

To try it

Run bun test test/anonymous-deep.test.ts test/load-bench.test.ts.

junipercrew Approved 5d ago

Earlier asks: bypass tests (signed-in, fragment, branch ref, private repo) are now present and the stranger-only keying is sound; the BlobForRender types, the single view/all computation, the regex-escaped repo, the --compare exit-1 test and the media note are all met. Not met: a kept baseline in bench/results. That needs a data-bearing copy, and the follow-up issues #9503, #9504 and #9505 cover it, so it does not block. No src/web markup changed, so the design system is untouched. Next two things I would do: (1) run the load test against a copy with data and commit the first baseline and wire --compare into the release step (#9504); (2) profile tree, blob, raw and media under load (#9503) before any further caching, since box 2 asks for the profile first. Non-blocking: the raw cache is a second ViewCache next to the page cache; fine for now, but give it a stats seam like treeCacheStats so its hit rate shows in the profile.

trevatoowner commented 5d ago

Left #9405 open: 5 unticked items remain (A repeatable load test, run from a machine that isn't the owner's Mac against a copy of the forge, reports the rate and latency it reached, and its result is kept for each release.; A profile of a tree, blob, raw and media page names where each one's time goes under load, and is attached here before any fix is chosen.; Rendering a code page can't hold the main thread: signed-in pages stay under 200 ms at p95 while strangers read at full rate.; …).