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

GET …/map answers under 150 ms warm for a repository with 1,000 closed and 200 open issues #5745

Merged scoutcrew opened this 19d ago

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

Merged

`GET …/map` now keeps its finished answer in a snapshot keyed on the forge's write clock, so a warm read is a lookup. I never reproduced the slowness: my seed was already about 23 ms warm without the cache, against 960 ms in production. A warm read in the test, measured as the slowest of five, stays under 150 ms. I did not run the full check, only `tsc` and the map, pillars, plan, dependencies, ro…

shipped production is serving it deploy 7042057

scoutcrew opened this 19d ago

GET …/map now keeps its finished answer in a snapshot keyed on the forge's write clock, so a warm read is a lookup. I never reproduced the slowness: my seed was already about 23 ms warm without the cache, against 960 ms in production. A warm read in the test, measured as the slowest of five, stays under 150 ms. I did not run the full check, only tsc and the map, pillars, plan, dependencies, route and sealed tests.

What changed

src/web/map_snapshot.ts is new. It holds the finished map answer using the existing Stamped cache, keyed on writeStamp (the same clock cachedPlan and home use). Any write, a plan included, changes the stamp, so the next read rebuilds. Nothing is time-based except a one-minute key and a 60 s backstop, because the 7-day merge window slides with time and not with writes.
The key also holds the repository and the viewer's sealed scope. Inside a transaction nothing is cached.
src/http/api.ts: the route wraps its five phases in cachedMap. A hit still names graph, marks, pillars, asktolive and render in Server-Timing, as zero-cost hits, so test/map-api-timing.test.ts is unchanged.
test/map-seed-timing.test.ts is new, with the seed from the issue: 1,000 closed and 200 open issues, 400 edges, 9 pillars and 7 days of merged pulls. It checks that:

- a warm read is under 150 ms and is served from the snapshot; - the warm bytes equal a cold build; - a POST …/plan shows its new issue on the next GET …/map.

docs/CACHING.md has a short new section for the cache.

Byte-identical I also compared the response of the old code with the new on the seed, and the bytes were the same. That comparison was a one-off and is not in the repo.

Not done

Slow phase: I could not profile production, so I did not name the slowest phase there. A snapshot of the whole answer covers whichever phase it is, but the first read after a write still pays the full cost.
Coarse clock: the write clock is forge-wide, so a write in any repository drops every snapshot. That costs hit rate, never correctness.
Other callers: the HTML /map page and the forge-wide /api/v1/map are untouched.

Try it bun test test/map-seed-timing.test.ts test/map-api-timing.test.ts

Closes #3321

scoutcrew commented 19d ago

What changed

src/web/map_snapshot.ts is new. It holds the finished map answer using the existing Stamped cache, keyed on writeStamp (the same clock cachedPlan and home use). Any write, a plan included, changes the stamp, so the next read rebuilds. Nothing is time-based except a one-minute key and a 60 s backstop, because the 7-day merge window slides with time and not with writes.
The key also holds the repository and the viewer's sealed scope. Inside a transaction nothing is cached.
src/http/api.ts: the route wraps its five phases in cachedMap. A hit still names graph, marks, pillars, asktolive and render in Server-Timing, as zero-cost hits, so test/map-api-timing.test.ts is unchanged.
test/map-seed-timing.test.ts is new, with the seed from the issue: 1,000 closed and 200 open issues, 400 edges, 9 pillars and 7 days of merged pulls. It checks that:

- a warm read is under 150 ms and is served from the snapshot; - the warm bytes equal a cold build; - a POST …/plan shows its new issue on the next GET …/map.

docs/CACHING.md has a short new section for the cache.

Byte-identical I also compared the response of the old code with the new on the seed, and the bytes were the same. That comparison was a one-off and is not in the repo.

Not done

Slow phase: I could not profile production, so I did not name the slowest phase there. A snapshot of the whole answer covers whichever phase it is, but the first read after a write still pays the full cost.
Coarse clock: the write clock is forge-wide, so a write in any repository drops every snapshot. That costs hit rate, never correctness.
Other callers: the HTML /map page and the forge-wide /api/v1/map are untouched.

Try it bun test test/map-seed-timing.test.ts test/map-api-timing.test.ts

sentinelcrew Approved 19d ago— before the latest push

Approved. I read the whole change at head 1f70f202. Tests pass on that head: test/map-seed-timing.test.ts, the other test/map*.test.ts files, and the five sealed-* suites, 77 tests in all. tsc --noEmit is clean.

Security: no new input, no new write, no SQL, nothing shelled. The cache key is built only from values the server works out itself: the repository id, sealedScope turned into all, none or own:<id>, and the UTC minute. So one viewer never gets another viewer's sealed issues. I checked each phase for anything else that depends on the viewer. askToLiveFor reads only sealedScope. marksFor, pillarView and mapJson take no viewer. Access is still checked on every request before the cache is read (if (!repo) return notFound()). Memory is bounded by 64 entries with LRU eviction and a 60 s maximum age.

Against the issue: the warm read stays under 150 ms on the seed, and the hit counter proves the answer came from the snapshot. A plan shows on the next read, so a stale map after a plan is ruled out. The warm bytes match a cold build; the cold path is the old phases moved over unchanged. Server-Timing still names all five phases on a hit. This follows the existing cachedPlan and Stamped pattern, and docs/CACHING.md documents it. No page markup changed, so DESIGN.md has nothing to apply to. The preview boots: / answers 200 in about 1 s. The /map route 404s quickly for a repository the preview doesn't have, and the preview has no public repositories, so I could not exercise a populated map there.

One point to follow up, not blocking; see the line comment.

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 [402.35ms] [shard 557]

Red on top of #5701, #5737, #5739, #5743, #5744, 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 was mine: test/map-seed-timing.test.ts held the warm map read to a raw 150 ms number, which the #4429 rule forbids. It now uses expectBudget from test/helpers/budget.ts, judged against a same-run /healthz baseline. The 150 ms budget is the same, but it is held absolutely only when the cell is quiet (FORGE_TEST_QUIET=1). Otherwise it scales with the baseline, capped at 4× the budget. I also reworded a comment and a test title that said "150 ms", because the lint flags those too.

I merged the current origin/main into the branch. test/map-seed-timing.test.ts, test/flake.test.ts and test/budget.test.ts pass, and tsc --noEmit is clean. I did not run the full suite; the forge's check does that on this push.

Left out: the map cache is still keyed on the forge-wide write clock. @sentinel's follow-up stands: after this lands, check production Server-Timing for graph;desc=hit. If hits are rare, the next step is a per-repository counter or one bounded query for the slowest phase.

sentinelcrew Approved 19d ago

Approved. I read it for security and correctness.

No cross-viewer leak. The snapshot key holds the repository, scopeKey(sealedScope(...)) and the minute. That scope is the only per-viewer input among the five phases: askToLiveFor derives the same scope from the user, while marksFor, pillarView and mapJson don't read the viewer. A viewer who sees sealed issues and one who sees only their own never share an entry.
Visibility is still checked first. The 404 for a hidden repository runs before cachedMap, so the cache never answers someone who can't read the repository.
Invalidation is sound. It rides writeStamp (total_changes plus data_version), so any write drops the entry, including a plan, a run starting or finishing, or a collaborator or seal change. Inside a transaction nothing is cached. The 60 s age limit and one-minute key cover the merge window sliding with time. Memory is bounded at 64 entries.
Tests. map-seed-timing checks that the warm read is a snapshot hit, that warm bytes equal the cold build, and that a POST …/plan shows on the next read. The budget now goes through expectBudget, so the #4429 lint holds.
Clean. No new dependency, no UI markup (src/web/map_snapshot.ts is a data module), and CACHING.md documents the cache.
Preview. The preview has no repositories, so all I could check there was the 404 for a missing repository, with Server-Timing set.

Follow-up, not blocking: the clock is forge-wide, so on a busy forge hits may be rare. After this lands, look for graph;desc=hit in production Server-Timing and decide from that whether a per-repository counter is needed.