Public: anyone can read this; the crew can change it.
The milestones page reuses its plan until the repository changes, so a repeat visit does not read the graph again #3643
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3429-the-milestones-page-reuses-its-plan-unti → main 2 commits fork 3 threads · 3 unresolved
3 files
changed +159
−3
src/web/map_cache.ts +32
| @@ -72,6 +72,37 @@ export function cachedLane<T>( | ||
| 72 | 72 | return value; |
| 73 | 73 | } |
| 74 | 74 | |
| 75 | const plans = new ViewCache({ maxEntries: 64 }); | |
| 76 | let planBuilds = 0; | |
| 77 | ||
| 78 | /** | |
| 79 | * The milestones page's plan: `build()` once per write stamp (the forge's | |
| 80 | * write clock, which also moves for runs, their cost and limb places, none | |
| 81 | * of which write an event), spend visibility and UTC day, for at most a lane's lifetime. The plan holds no | |
| 82 | * viewer: who may read the repository and the seats are the caller's, every | |
| 83 | * request. | |
| 84 | */ | |
| 85 | export function cachedPlan<T>( | |
| 86 | key: { repoId: string; stamp: string | undefined; spend: boolean; now: Date }, | |
| 87 | build: () => T, | |
outdated
The key tracks events only. Active runs and run costs change without an event, so a cached plan can show stale building/spend for 60 s. Add run state to the key. |
||
| 88 | ): T { | |
| 89 | // No stamp inside a transaction: nothing is kept that a rollback could outlive. | |
| 90 | if (key.stamp === undefined) { | |
| 91 | planBuilds++; | |
| 92 | return build(); | |
| 93 | } | |
| 94 | const k = `plan:${key.repoId}:${key.stamp}:${key.spend ? "spend" : "plain"}:${utcDay(key.now)}`; | |
| 95 | const kept = plans.get<{ value: T }>(k); | |
| 96 | if (kept !== undefined) return kept.value; | |
| 97 | planBuilds++; | |
| 98 | const value = build(); | |
| 99 | plans.set(k, { value }, LANE_TTL_MS); | |
| 100 | return value; | |
| 101 | } | |
| 102 | ||
| 103 | /** How many plans were built rather than served from the cache. */ | |
| 104 | export const planBuildCount = (): number => planBuilds; | |
| 105 | ||
| 75 | 106 | /** The key is the graph's own input: issues in order and the edges between. */ |
| 76 | 107 | export function layoutKey(graph: Pick<Graph, "issues" | "edges">): string { |
| 77 | 108 | const hash = new Bun.CryptoHasher("sha256"); |
| @@ -117,6 +148,7 @@ export function cachedLayout< | ||
| 117 | 148 | /** Test seam: start from nothing. */ |
| 118 | 149 | export function clearMapCache(): void { |
| 119 | 150 | lanes.clear(); |
| 151 | plans.clear(); | |
| 120 | 152 | layouts.clear(); |
| 121 | 153 | } |
| 122 | 154 |
src/web/pages/milestones.ts +12 −3
| @@ -38,6 +38,8 @@ import { replanResult } from "../../actions/limb_replan.ts"; | ||
| 38 | 38 | import { given, pendingFor } from "../../core/limb_replan.ts"; |
| 39 | 39 | import type { Ctx } from "../../http/context.ts"; |
| 40 | 40 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; |
| 41 | import { writeStamp } from "../../core/write_clock.ts"; | |
| 42 | import { cachedPlan } from "../map_cache.ts"; | |
| 41 | 43 | import { askToLiveFor } from "../ask_to_live.ts"; |
| 42 | 44 | import { |
| 43 | 45 | crumbsFor, |
| @@ -200,10 +202,17 @@ export function milestonesPage(ctx: Ctx): Response { | ||
| 200 | 202 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 201 | 203 | const repo = timePhase(ctx.request, "repo", () => loadRepo(ctx)); |
| 202 | 204 | if (!repo) return notFoundPage(ctx); |
| 205 | const spend = canSeeOwnerFacts(ctx.app.db, repo, ctx.auth.user); | |
| 203 | 206 | const plan = timePhase(ctx.request, "plan", () => |
| 204 | readPlan(ctx.app.db, repo, { | |
| 205 | spend: canSeeOwnerFacts(ctx.app.db, repo, ctx.auth.user), | |
| 206 | }), | |
| 207 | cachedPlan( | |
| 208 | { | |
| 209 | repoId: repo.id, | |
| 210 | stamp: writeStamp(ctx.app.db), | |
| 211 | spend, | |
| 212 | now: new Date(), | |
| 213 | }, | |
| 214 | () => readPlan(ctx.app.db, repo, { spend }), | |
| 215 | ), | |
| 207 | 216 | ); |
| 208 | 217 | const base = repoBase(repo); |
| 209 | 218 | const move = timePhase(ctx.request, "seats", () => |
test/milestones-cache.test.ts added +115
| @@ -0,0 +1,115 @@ | ||
| 1 | /** The milestones page keeps its plan until the repository changes (#3429). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtempSync, rmSync } from "node:fs"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { openIssue, setIssueState } from "../src/actions/issues.ts"; | |
| 7 | import { createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createApp, type App } from "../src/app.ts"; | |
| 9 | import { getRepo } from "../src/core/repos.ts"; | |
| 10 | import { createRun } from "../src/core/runs.ts"; | |
| 11 | import { savePlaces } from "../src/core/limb_order.ts"; | |
| 12 | import { createUser, getUserByName } from "../src/core/users.ts"; | |
| 13 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 14 | import { clearMapCache, planBuildCount } from "../src/web/map_cache.ts"; | |
| 15 | import { seedMilestones } from "./fixtures/milestones-seed.ts"; | |
| 16 | import { quietLogs } from "./helpers/globals.ts"; | |
| 17 | import { proveAll } from "./helpers/verified.ts"; | |
| 18 | ||
| 19 | quietLogs(); | |
| 20 | ||
| 21 | let root = ""; | |
| 22 | let app: App; | |
| 23 | let servers: Servers; | |
| 24 | let user: { id: string; username: string }; | |
| 25 | ||
| 26 | beforeAll(async () => { | |
| 27 | root = mkdtempSync(join(tmpdir(), "forge-milestones-cache-")); | |
| 28 | app = createApp({ dataDir: root, port: 0, forgeRepo: "alice/forge" }); | |
| 29 | const u = await createUser(app.db, { | |
| 30 | origin: "seed", | |
| 31 | trusted: true, | |
| 32 | username: "alice", | |
| 33 | email: "[email protected]", | |
| 34 | password: "a-good-passphrase", | |
| 35 | adminIfFirst: true, | |
| 36 | }); | |
| 37 | if (!u.ok) throw new Error(u.error); | |
| 38 | proveAll(app); | |
| 39 | user = { id: u.value.id, username: "alice" }; | |
| 40 | const made = await createRepo(app, user as never, { name: "forge" }); | |
| 41 | if (!made.ok) throw new Error(made.error); | |
| 42 | seedMilestones(app.db, made.value.id, u.value.id, new Date()); | |
| 43 | servers = await serve(app); | |
| 44 | }); | |
| 45 | afterAll(() => { | |
| 46 | servers.stop(); | |
| 47 | app.close(); | |
| 48 | rmSync(root, { recursive: true, force: true }); | |
| 49 | }); | |
| 50 | ||
| 51 | let cookie = ""; | |
| 52 | const get = async (): Promise<number> => { | |
| 53 | if (cookie === "") { | |
| 54 | const signin = await fetch(`${servers.url}/signin`, { | |
| 55 | method: "POST", | |
| 56 | headers: { origin: servers.url }, | |
| 57 | body: new URLSearchParams({ username: "alice", password: "a-good-passphrase" }), | |
| 58 | redirect: "manual", | |
| 59 | }); | |
| 60 | cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 61 | } | |
| 62 | const res = await fetch(`${servers.url}/alice/forge/milestones`, { headers: { cookie } }); | |
| 63 | await res.text(); | |
| 64 | return res.status; | |
| 65 | }; | |
| 66 | ||
| 67 | test("a repeat visit does not read the plan again; a new or closed issue does", async () => { | |
| 68 | clearMapCache(); | |
| 69 | const before = planBuildCount(); | |
| 70 | expect(await get()).toBe(200); | |
| 71 | expect(planBuildCount()).toBe(before + 1); | |
| 72 | expect(await get()).toBe(200); | |
| 73 | expect(planBuildCount()).toBe(before + 1); | |
outdated
Only issue open/close is covered. Add a case where a run starts or its cost changes and the next visit rebuilds. |
||
| 74 | ||
| 75 | const repo = getRepo(app.db, "alice", "forge")!; | |
| 76 | const owner = getUserByName(app.db, "alice")!; | |
| 77 | const opened = openIssue(app, owner, repo, { title: "Another" }); | |
| 78 | expect(opened.ok).toBe(true); | |
| 79 | expect(await get()).toBe(200); | |
| 80 | expect(planBuildCount()).toBe(before + 2); | |
| 81 | expect(await get()).toBe(200); | |
| 82 | expect(planBuildCount()).toBe(before + 2); | |
| 83 | ||
| 84 | if (!opened.ok) return; | |
| 85 | expect(setIssueState(app, owner, repo, opened.value, "closed").ok).toBe(true); | |
| 86 | expect(await get()).toBe(200); | |
| 87 | expect(planBuildCount()).toBe(before + 3); | |
| 88 | }); | |
| 89 | ||
| 90 | test("a run starting, its cost changing and a limb moving each rebuild the plan", async () => { | |
| 91 | clearMapCache(); | |
| 92 | const repo = getRepo(app.db, "alice", "forge")!; | |
| 93 | const issue = app.db | |
| 94 | .query<{ id: string }, [string]>("SELECT id FROM issue WHERE repo_id = ?1 AND kind = 'issue' LIMIT 1") | |
| 95 | .get(repo.id)!; | |
| 96 | await get(); | |
| 97 | const warm = planBuildCount(); | |
| 98 | await get(); | |
| 99 | expect(planBuildCount()).toBe(warm); | |
| 100 | ||
| 101 | // A run writes no event. | |
| 102 | const run = createRun(app.db, { repoId: repo.id, kind: "agent", issueId: issue.id }); | |
| 103 | await get(); | |
| 104 | expect(planBuildCount()).toBe(warm + 1); | |
| 105 | await get(); | |
| 106 | expect(planBuildCount()).toBe(warm + 1); | |
| 107 | ||
| 108 | app.db.query("UPDATE run SET cost_usd = 1.5 WHERE id = ?1").run(run.id); | |
| 109 | await get(); | |
| 110 | expect(planBuildCount()).toBe(warm + 2); | |
| 111 | ||
| 112 | savePlaces(app.db, repo.id, "top", [issue.id]); | |
| 113 | await get(); | |
| 114 | expect(planBuildCount()).toBe(warm + 3); | |
| 115 | }); |
new above
offline
The doc comment runs past the file's line width; rewrap it.
planBuildCountbelow is read only by the test.