Public: anyone can read this; the crew can change it.
A pulls list reads every row's status word in one batch: `words` under 5 ms for a page of pulls #6170
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6151-a-pulls-list-reads-every-row-s-status-wo → main 6 commits fork 1 thread · 1 unresolved
4 files
changed +246
−16
src/web/studio/lists.ts +2 −4
| @@ -60,7 +60,7 @@ import { changeHref, PAGE_TITLES, pageTitle, tellHref } from "./nouns.ts"; | ||
| 60 | 60 | import { tellLine, tellsAnswered } from "./branch_tell.ts"; |
| 61 | 61 | import { knowMergeClosings, tellWords } from "./tell.ts"; |
| 62 | 62 | import { |
| 63 | pullWord, | |
| 63 | pullWords, | |
| 64 | 64 | runWord, |
| 65 | 65 | wordBadge, |
| 66 | 66 | type Word, |
| @@ -503,9 +503,7 @@ export async function changesList(ctx: Ctx): Promise<Response> { | ||
| 503 | 503 | ...sealedScope(db, repo, ctx.auth.user), |
| 504 | 504 | }); |
| 505 | 505 | lap("pulls"); |
| 506 | const words = await Promise.all( | |
| 507 | fetched.map((pull) => pullWord(wordCtx, pull)), | |
| 508 | ); | |
| 506 | const words = await pullWords(wordCtx, fetched); | |
| 509 | 507 | lap("words"); |
| 510 | 508 | const answered = tellsAnswered(db, repo.id, fetched); |
| 511 | 509 | lap("tells"); |
src/web/studio/words.ts +59 −11
| @@ -106,6 +106,9 @@ interface Ahead { | ||
| 106 | 106 | readonly checks: Map<string, CheckSummary>; |
| 107 | 107 | readonly reviewers: Map<string, string | undefined>; |
| 108 | 108 | readonly runPulls: Map<string, Pull | undefined>; |
| 109 | readonly parked: Map<string, boolean>; | |
| 110 | /** Production as a merged pull sees it; `null` when the viewer may not see it. */ | |
| 111 | production?: { env: EnvStatus | undefined } | null; | |
| 109 | 112 | } |
| 110 | 113 | // Keyed by the page's `WordCtx`, which is made fresh for each request and |
| 111 | 114 | // dropped with it, so nothing here outlives the render it was read for. |
| @@ -113,7 +116,7 @@ const ahead = new WeakMap<WordCtx, Ahead>(); | ||
| 113 | 116 | const aheadOf = (ctx: WordCtx): Ahead => { |
| 114 | 117 | let a = ahead.get(ctx); |
| 115 | 118 | if (a === undefined) |
| 116 | ahead.set(ctx, (a = { checks: new Map(), reviewers: new Map(), runPulls: new Map() })); | |
| 119 | ahead.set(ctx, (a = { checks: new Map(), reviewers: new Map(), runPulls: new Map(), parked: new Map() })); | |
| 117 | 120 | return a; |
| 118 | 121 | }; |
| 119 | 122 | |
| @@ -149,6 +152,44 @@ export function knowPulls(ctx: WordCtx, pulls: readonly Pull[]): void { | ||
| 149 | 152 | for (const p of green) a.reviewers.set(p.id, waits.get(p.id)?.reviewer); |
| 150 | 153 | } |
| 151 | 154 | |
| 155 | /** | |
| 156 | * `pullWord` for a page of pulls: checks, reviewers, close reasons and | |
| 157 | * production's status are read together first, so the page costs a handful | |
| 158 | * of queries rather than several per row. Each word is the one `pullWord` | |
| 159 | * gives, in the order of `pulls`. | |
| 160 | */ | |
| 161 | export async function pullWords( | |
| 162 | ctx: WordCtx, | |
| 163 | pulls: readonly Pull[], | |
| 164 | ): Promise<{ word: Word; note: string }[]> { | |
| 165 | knowPulls(ctx, pulls); | |
| 166 | const a = aheadOf(ctx); | |
| 167 | const closed = pulls.filter((p) => p.mergedAt === null && p.state === "closed"); | |
| 168 | if (closed.length > 0) { | |
| 169 | const issues = getIssuesByIds(ctx.db, closed.map((p) => p.id)); | |
| 170 | for (const p of closed) | |
| 171 | a.parked.set(p.id, issues.get(p.id)?.closeReason === "parked"); | |
| 172 | } | |
| 173 | if ( | |
| 174 | a.production === undefined && | |
| 175 | pulls.some((p) => p.mergedAt !== null) && | |
| 176 | ctx.runtime !== undefined && | |
| 177 | ctx.record !== undefined | |
| 178 | ) { | |
| 179 | const repo = getRepoById(ctx.db, ctx.record.repoId); | |
| 180 | a.production = | |
| 181 | repo === undefined || | |
| 182 | !canEnterApp(ctx.db, repo, ctx.record, ctx.viewer, { named: false }) | |
| 183 | ? null | |
| 184 | : { | |
| 185 | env: ctx.runtime | |
| 186 | .status(ctx.record) | |
| 187 | .find((e) => e.env === "production"), | |
| 188 | }; | |
| 189 | } | |
| 190 | return await Promise.all(pulls.map((p) => pullWord(ctx, p))); | |
| 191 | } | |
| 192 | ||
| 152 | 193 | /** `knowPulls` for the pull each of these finished runs produced (`pullFromRun`). */ |
| 153 | 194 | export function knowRunPulls(ctx: WordCtx, runs: readonly Run[]): void { |
| 154 | 195 | const a = aheadOf(ctx); |
| @@ -308,15 +349,21 @@ export async function pullWord( | ||
| 308 | 349 | return { word: "shipped", note: "merged" }; |
| 309 | 350 | // A private app's production — its state, its deploy's error — is for |
| 310 | 351 | // those who may enter it (`canEnterApp`); anyone else reads "merged". |
| 311 | const repo = getRepoById(ctx.db, ctx.record.repoId); | |
| 312 | if ( | |
| 313 | repo === undefined || | |
| 314 | !canEnterApp(ctx.db, repo, ctx.record, ctx.viewer, { named: false }) | |
| 315 | ) | |
| 316 | return { word: "shipped", note: "merged" }; | |
| 317 | const production = ctx.runtime | |
| 318 | .status(ctx.record) | |
| 319 | .find((e) => e.env === "production"); | |
| 352 | const read = ahead.get(ctx)?.production; | |
| 353 | if (read === null) return { word: "shipped", note: "merged" }; | |
| 354 | let production: EnvStatus | undefined; | |
| 355 | if (read !== undefined) production = read.env; | |
| 356 | else { | |
| 357 | const repo = getRepoById(ctx.db, ctx.record.repoId); | |
| 358 | if ( | |
| 359 | repo === undefined || | |
| 360 | !canEnterApp(ctx.db, repo, ctx.record, ctx.viewer, { named: false }) | |
| 361 | ) | |
| 362 | return { word: "shipped", note: "merged" }; | |
| 363 | production = ctx.runtime | |
| 364 | .status(ctx.record) | |
| 365 | .find((e) => e.env === "production"); | |
| 366 | } | |
| 320 | 367 | if (await caughtUp(ctx, production, commitOid)) |
| 321 | 368 | return { word: "shipped", note: "production is serving it" }; |
| 322 | 369 | return mergedPullWord(production, commitOid); |
| @@ -325,7 +372,8 @@ export async function pullWord( | ||
| 325 | 372 | return { |
| 326 | 373 | word: "stopped", |
| 327 | 374 | note: |
| 328 | getIssueById(ctx.db, pull.id)?.closeReason === "parked" | |
| 375 | (ahead.get(ctx)?.parked.get(pull.id) ?? | |
| 376 | getIssueById(ctx.db, pull.id)?.closeReason === "parked") | |
| 329 | 377 | ? "parked by the crew" |
| 330 | 378 | : "closed without shipping", |
| 331 | 379 | }; |
test/private-app.test.ts +42 −1
| @@ -24,7 +24,7 @@ import { audienceFilter } from "../src/core/event_audience.ts"; | ||
| 24 | 24 | import { createRun, listRuns } from "../src/core/runs.ts"; |
| 25 | 25 | import { openIssue } from "../src/actions/issues.ts"; |
| 26 | 26 | import { createToken } from "../src/core/tokens.ts"; |
| 27 | import { pullWord } from "../src/web/studio/words.ts"; | |
| 27 | import { pullWord, pullWords } from "../src/web/studio/words.ts"; | |
| 28 | 28 | import { servingHistory } from "../src/web/studio/words.ts"; |
| 29 | 29 | import { createUser, type User } from "../src/core/users.ts"; |
| 30 | 30 | import { serve, type Servers } from "../src/http/server.ts"; |
| @@ -345,6 +345,47 @@ describe("a private app on a public repository stays private", () => { | ||
| 345 | 345 | expect((await pullWord(ctxFor(alice), merged)).note).toBe("production is serving it"); |
| 346 | 346 | }); |
| 347 | 347 | |
| 348 | test("a page of merged pulls is worded as pullWord words them: the gate held once per page", async () => { | |
| 349 | const commit = "a".repeat(40); | |
| 350 | const other = "b".repeat(40); | |
| 351 | const pull = (id: string, oid: string) => | |
| 352 | ({ id, mergedAt: new Date().toISOString(), mergedOid: oid, headOid: oid, state: "closed" }) as never; | |
| 353 | const pulls = [pull("m1", commit), pull("m2", other)]; | |
| 354 | const runtimeAt = (state: string, serving: string) => | |
| 355 | ({ status: () => [{ env: "production", state, commitOid: serving, error: "SECRET-ERROR" }] }) as never; | |
| 356 | const ctxFor = (viewer: User | undefined, runtime: never) => ({ | |
| 357 | db: app.db, | |
| 358 | dir: "", | |
| 359 | runtime, | |
| 360 | record, | |
| 361 | history: servingHistory(), | |
| 362 | viewer, | |
| 363 | }); | |
| 364 | // Production serves `commit` and has failed to bring `other` up. | |
| 365 | const caught = runtimeAt("running", commit); | |
| 366 | const failed = runtimeAt("failed", other); | |
| 367 | for (const runtime of [caught, failed]) { | |
| 368 | for (const viewer of [alice, bot, carol, undefined]) { | |
| 369 | const batched = await pullWords(ctxFor(viewer, runtime), pulls); | |
| 370 | const single = [ | |
| 371 | await pullWord(ctxFor(viewer, runtime), pulls[0]!), | |
| 372 | await pullWord(ctxFor(viewer, runtime), pulls[1]!), | |
| 373 | ]; | |
| 374 | expect(batched).toEqual(single); | |
| 375 | if (viewer === carol || viewer === undefined) | |
| 376 | for (const w of batched) { | |
| 377 | expect(w).toEqual({ word: "shipped", note: "merged" }); | |
| 378 | expect(JSON.stringify(w)).not.toContain("SECRET-ERROR"); | |
| 379 | } | |
| 380 | } | |
| 381 | } | |
| 382 | // The owner reads production: caught up for one, not for the other. | |
| 383 | const owner = await pullWords(ctxFor(alice, caught), pulls); | |
| 384 | expect(owner[0]!.note).toBe("production is serving it"); | |
| 385 | expect(owner[1]!.note).not.toBe("merged"); | |
| 386 | expect(owner[1]!.note).not.toBe("production is serving it"); | |
| 387 | }); | |
| 388 | ||
| 348 | 389 | test("the MCP's now and the tell page show a private app's runs only to those who may enter it", async () => { |
| 349 | 390 | const issue = openIssue(app, alice, repo(), { title: "Count the books" }); |
| 350 | 391 | if (!issue.ok) throw new Error(String(issue.error)); |
test/pulls-words.test.ts added +143
| @@ -0,0 +1,143 @@ | ||
| 1 | /** | |
| 2 | * A page of pulls is worded from batched reads (#6151): `pullWords` gives the | |
| 3 | * word `pullWord` gives for every state, in few queries and well under 5 ms. | |
| 4 | */ | |
| 5 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 6 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 7 | import { tmpdir } from "node:os"; | |
| 8 | import { join } from "node:path"; | |
| 9 | import { createApp, type App } from "../src/app.ts"; | |
| 10 | import { createRepo } from "../src/actions/repos.ts"; | |
| 11 | import { createIssue } from "../src/core/issues.ts"; | |
| 12 | import { listPulls, type Pull } from "../src/core/pulls.ts"; | |
| 13 | import type { Repo } from "../src/core/repos.ts"; | |
| 14 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 15 | import { getUserByName } from "../src/core/users.ts"; | |
| 16 | import { repoDir } from "../src/git/repos.ts"; | |
| 17 | import { pullWord, pullWords, servingHistory, type WordCtx } from "../src/web/studio/words.ts"; | |
| 18 | import { quietLogs } from "./helpers/globals.ts"; | |
| 19 | import { adminCookie } from "./helpers/admin.ts"; | |
| 20 | import { expectBudget } from "./helpers/budget.ts"; | |
| 21 | ||
| 22 | quietLogs(); | |
| 23 | ||
| 24 | describe("a page of pulls, worded in one batch", () => { | |
| 25 | let dataDir = ""; | |
| 26 | let app: App; | |
| 27 | let repo: Repo; | |
| 28 | ||
| 29 | const ctx = (): WordCtx => ({ | |
| 30 | db: app.db, | |
| 31 | dir: repoDir(app.reposRoot, repo.ownerName, repo.name), | |
| 32 | runtime: undefined, | |
| 33 | record: undefined, | |
| 34 | history: servingHistory(), | |
| 35 | viewer: undefined, | |
| 36 | }); | |
| 37 | const page = (): Pull[] => listPulls(app.db, repo.id, { state: "all", limit: 100 }); | |
| 38 | ||
| 39 | beforeAll(async () => { | |
| 40 | dataDir = await mkdtemp(join(tmpdir(), "forge-pulls-words-")); | |
| 41 | app = createApp({ dataDir, port: 0 }); | |
| 42 | await adminCookie(app, { username: "alice", email: "[email protected]", password: "a-long-words-passphrase" }); | |
| 43 | const alice = getUserByName(app.db, "alice")!; | |
| 44 | const made = await createRepo(app, alice, { name: "garden", visibility: "public" }); | |
| 45 | if (!made.ok) throw new Error(String(made.error)); | |
| 46 | repo = made.value; | |
| 47 | ||
| 48 | // Rows go in directly: the words read the rows and the runs, not git. | |
| 49 | for (let i = 0; i < 50; i++) { | |
| 50 | const branch = `feature/p${i}`; | |
| 51 | const oid = String(i).padStart(2, "0").repeat(20); | |
| 52 | const issue = createIssue(app.db, { repoId: repo.id, authorId: alice.id, title: branch, body: "", kind: "pull" }); | |
| 53 | if (!issue.ok) throw new Error(String(issue.error)); | |
| 54 | const id = issue.value.id; | |
| 55 | app.db | |
| 56 | .query( | |
| 57 | `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id) | |
| 58 | VALUES (?1, 'main', ?2, ?3, ?3, ?4)`, | |
| 59 | ) | |
| 60 | .run(id, branch, "f".repeat(40), repo.id); | |
| 61 | app.db.query("UPDATE pull SET head_oid = ?2 WHERE issue_id = ?1").run(id, oid); | |
| 62 | const check = (outcome: "ok" | "failed" | "running" | "queued") => { | |
| 63 | const c = createRun(app.db, { repoId: repo.id, kind: "check", issueId: id, env: `pull-${issue.value.number}`, commitOid: oid, reason: "pull" }); | |
| 64 | if (outcome === "queued") return; | |
| 65 | claimRun(app.db, c.id); | |
| 66 | if (outcome !== "running") finishRun(app.db, c.id, { state: outcome, summary: outcome }); | |
| 67 | }; | |
| 68 | const state = i % 8; | |
| 69 | if (state === 1) check("ok"); | |
| 70 | if (state === 2) check("failed"); | |
| 71 | if (state === 3) check("running"); | |
| 72 | if (state === 4) check("queued"); | |
| 73 | if (state === 5) app.db.query("UPDATE issue SET state = 'closed', closed_at = datetime('now') WHERE id = ?1").run(id); | |
| 74 | if (state === 6) app.db.query("UPDATE issue SET state = 'closed', closed_at = datetime('now'), parked = 1 WHERE id = ?1").run(id); | |
| 75 | if (state === 7) { | |
| 76 | app.db.query("UPDATE issue SET state = 'closed', closed_at = datetime('now') WHERE id = ?1").run(id); | |
| 77 | app.db.query("UPDATE pull SET merged_at = datetime('now'), merged_oid = ?2 WHERE issue_id = ?1").run(id, oid); | |
| 78 | } | |
| 79 | } | |
| 80 | }); | |
| 81 | ||
| 82 | afterAll(async () => { | |
| 83 | await rm(dataDir, { recursive: true, force: true }); | |
| 84 | }); | |
| 85 | ||
| 86 | test("every word is the one pullWord gives, for every state", async () => { | |
| 87 | const pulls = page(); | |
| 88 | expect(pulls.length).toBe(50); | |
| 89 | const batched = await pullWords(ctx(), pulls); | |
| 90 | for (const [i, pull] of pulls.entries()) | |
| 91 | expect(batched[i]).toEqual(await pullWord(ctx(), pull)); | |
| 92 | const seen = new Set<string>(batched.map((w) => w.word)); | |
| 93 | for (const w of ["waiting", "look", "stopped", "working", "shipped"]) expect(seen.has(w)).toBe(true); | |
| 94 | expect(batched.some((w) => w.note === "parked by the crew")).toBe(true); | |
| 95 | expect(batched.some((w) => w.note === "closed without shipping")).toBe(true); | |
| 96 | }); | |
| 97 | ||
| 98 | test("a public app's production is read once for the page, and anyone may see it", async () => { | |
| 99 | const merged = page().filter((p) => p.mergedAt !== null); | |
| 100 | expect(merged.length).toBeGreaterThan(1); | |
| 101 | const [first, second] = merged as [Pull, Pull]; | |
| 102 | let reads = 0; | |
| 103 | const runtime = (state: string) => | |
| 104 | ({ | |
| 105 | status: () => { | |
| 106 | reads++; | |
| 107 | return [{ env: "production", state, commitOid: first.mergedOid, error: "deploy broke" }]; | |
| 108 | }, | |
| 109 | }) as never; | |
| 110 | const record = { id: "app1", repoId: repo.id, access: "public" } as never; | |
| 111 | const at = (state: string): WordCtx => ({ ...ctx(), runtime: runtime(state), record }); | |
| 112 | const caught = await pullWords(at("running"), [first]); | |
| 113 | expect(caught[0]).toEqual({ word: "shipped", note: "production is serving it" }); | |
| 114 | const failing = at("failed"); | |
| 115 | reads = 0; | |
| 116 | const batched = await pullWords(failing, merged); | |
| 117 | expect(reads).toBe(1); | |
| 118 | expect(batched[0]).toEqual({ word: "shipped", note: "production is serving it" }); | |
| 119 | for (const [i, pull] of merged.entries()) expect(batched[i]).toEqual(await pullWord(at("failed"), pull)); | |
| 120 | expect(second.mergedOid).not.toBe(first.mergedOid); | |
| 121 | expect(batched[1]).toEqual({ word: "waiting", note: "merged; waiting for production to redeploy" }); | |
| 122 | }); | |
| 123 | ||
| 124 | test("the words phase for 50 pulls is under 5 ms", async () => { | |
| 125 | const pulls = page(); | |
| 126 | await pullWords(ctx(), pulls); // warm statement caches | |
| 127 | let best = Infinity; | |
| 128 | for (let n = 0; n < 5; n++) { | |
| 129 | const c = ctx(); | |
| 130 | const at = performance.now(); | |
| 131 | await pullWords(c, pulls); | |
| 132 | best = Math.min(best, performance.now() - at); | |
| 133 | } | |
| 134 | // The same-run baseline: the best of a few trivial reads on this machine, right now. | |
| 135 | let against = Infinity; | |
| 136 | for (let n = 0; n < 20; n++) { | |
| 137 | const at = performance.now(); | |
| 138 | app.db.query("SELECT count(*) FROM issue").get(); | |
| 139 | against = Math.min(against, performance.now() - at); | |
| 140 | } | |
| 141 | expectBudget(best, { ms: 5, against }); | |
| 142 | }); | |
| 143 | }); |
new above
offline
This is where canEnterApp is now evaluated once per page and cached as null. Untested; a regression here leaks a private app's production status to guests. Cover it (guest on private app, owner on same).