Public: anyone can read this; the crew can change it.
A repository's settings, danger, hooks, labels and prompts pages name their Server-Timing phases and answer under 50 ms #4436
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4318-a-repository-s-settings-danger-hooks-lab → main 6 commits fork 2 threads · 2 unresolved
5 files
changed +184
−38
src/core/route_timing.ts +25
| @@ -273,6 +273,31 @@ export function takePhases(request: Request): Phase[] { | ||
| 273 | 273 | return list; |
| 274 | 274 | } |
| 275 | 275 | |
| 276 | const startTimes = new WeakMap<Request, number>(); | |
| 277 | ||
| 278 | /** The server stamps when it took a request, so the work before the handler can be named `route`. */ | |
| 279 | export function noteStart(request: Request, at: number): void { | |
| 280 | startTimes.set(request, at); | |
| 281 | } | |
| 282 | ||
| 283 | const framed = new WeakSet<Request>(); | |
| 284 | ||
| 285 | /** | |
| 286 | * A page that sums its phases to `app` calls this first: it names `route`, the | |
| 287 | * server's work before the handler less the session, and asks the server to | |
| 288 | * name `compress`, the work after it. Pages that don't call it are unchanged. | |
| 289 | */ | |
| 290 | export function nameFrame(request: Request): void { | |
| 291 | const at = startTimes.get(request); | |
| 292 | if (at !== undefined) notePhase(request, "route", Math.max(0, performance.now() - at - (sessionTimes.get(request) ?? 0))); | |
| 293 | framed.add(request); | |
| 294 | } | |
| 295 | ||
| 296 | /** Whether the page asked for `compress` to be named. */ | |
| 297 | export function namesFrame(request: Request): boolean { | |
| 298 | return framed.has(request); | |
| 299 | } | |
| 300 | ||
| 276 | 301 | const sessionTimes = new WeakMap<Request, number>(); |
| 277 | 302 | |
| 278 | 303 | /** The server says how long resolving the session took; a page may name it. */ |
src/http/server.ts +10 −1
| @@ -469,6 +469,9 @@ import { | ||
| 469 | 469 | createRouteTiming, |
| 470 | 470 | noteDbTime, |
| 471 | 471 | noteSessionTime, |
| 472 | notePhase, | |
| 473 | namesFrame, | |
| 474 | noteStart, | |
| 472 | 475 | takePhases, |
| 473 | 476 | serverTiming, |
| 474 | 477 | takeDbTime, |
| @@ -2207,7 +2210,12 @@ export async function serve(app: App): Promise<Servers> { | ||
| 2207 | 2210 | if (request.signal.aborted) onAbort(); |
| 2208 | 2211 | else request.signal.addEventListener("abort", onAbort, { once: true }); |
| 2209 | 2212 | const work = run().then( |
| 2210 | (response) => compress(request, response).then(securityFloor), | |
| 2213 | async (response) => { | |
| 2214 | const compressing = performance.now(); | |
| 2215 | const done = await compress(request, response).then(securityFloor); | |
| 2216 | if (namesFrame(request)) notePhase(request, "compress", performance.now() - compressing); | |
| 2217 | return done; | |
| 2218 | }, | |
| 2211 | 2219 | (error) => failure(owner, error), |
| 2212 | 2220 | ); |
| 2213 | 2221 | try { |
| @@ -2225,6 +2233,7 @@ export async function serve(app: App): Promise<Servers> { | ||
| 2225 | 2233 | server: Bun.Server<AppSocket>, |
| 2226 | 2234 | ): Promise<Response | undefined> => { |
| 2227 | 2235 | const started = performance.now(); |
| 2236 | noteStart(request, started); | |
| 2228 | 2237 | // An app's hostname is answered before any route: `hello.<domain>` is |
| 2229 | 2238 | // the app, whatever its path, and the forge's own routes never apply. |
| 2230 | 2239 | const runtime = appsRuntime(app); |
src/web/pages/settings.ts +69 −32
| @@ -59,7 +59,7 @@ import { | ||
| 59 | 59 | updateRepoSettings, |
| 60 | 60 | } from "../../actions/repos.ts"; |
| 61 | 61 | import { appsRuntime } from "../../apps/runtime.ts"; |
| 62 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; | |
| 62 | import { nameFrame, notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts"; | |
| 63 | 63 | import { appVitals, shiftText, speedText as ms, type AppVitals } from "../../apps/vitals.ts"; |
| 64 | 64 | import type { PreviewData } from "../../apps/data.ts"; |
| 65 | 65 | import { |
| @@ -138,6 +138,13 @@ function loadAdminRepo(ctx: Ctx): Repo | undefined { | ||
| 138 | 138 | return repo; |
| 139 | 139 | } |
| 140 | 140 | |
| 141 | /** `loadAdminRepo` for a GET page: names the session, then the repo read as `repo`. */ | |
| 142 | function timedAdminRepo(ctx: Ctx): Repo | undefined { | |
| 143 | nameFrame(ctx.request); | |
| 144 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); | |
| 145 | return timePhase(ctx.request, "repo", () => loadAdminRepo(ctx)); | |
| 146 | } | |
| 147 | ||
| 141 | 148 | function sections(base: string, active: Section) { |
| 142 | 149 | return [ |
| 143 | 150 | { |
| @@ -225,31 +232,41 @@ async function adminForm( | ||
| 225 | 232 | * webhooks, the danger zone — one quiet section over in the same frame. |
| 226 | 233 | */ |
| 227 | 234 | export async function settingsPage(ctx: Ctx): Promise<Response> { |
| 228 | const readable = loadRepo(ctx); | |
| 235 | nameFrame(ctx.request); | |
| 236 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); | |
| 237 | const [readable, repo] = timePhase(ctx.request, "repo", () => { | |
| 238 | const r = loadRepo(ctx); | |
| 239 | return [r, r ? loadAdminRepo(ctx) : undefined] as const; | |
| 240 | }); | |
| 229 | 241 | if (!readable) return repoNotFound(ctx); |
| 230 | const lead = settingsLead(ctx, readable); | |
| 231 | const repo = loadAdminRepo(ctx); | |
| 242 | const lead = timePhase(ctx.request, "lead", () => settingsLead(ctx, readable)); | |
| 232 | 243 | if (!repo) { |
| 233 | const proposals = proposalTokensSection(ctx, readable); | |
| 234 | return page({ | |
| 235 | ctx, | |
| 236 | title: pageTitle("settings", readable, true), | |
| 237 | crumbs: crumbsFor(readable), | |
| 238 | palette: repoPalette(ctx, readable), | |
| 239 | body: html`${repoHeader(ctx, readable, "settings", { compact: true })} | |
| 244 | return timePhase(ctx.request, "render", () => { | |
| 245 | const proposals = proposalTokensSection(ctx, readable); | |
| 246 | return page({ | |
| 247 | ctx, | |
| 248 | title: pageTitle("settings", readable, true), | |
| 249 | crumbs: crumbsFor(readable), | |
| 250 | palette: repoPalette(ctx, readable), | |
| 251 | body: html`${repoHeader(ctx, readable, "settings", { compact: true })} | |
| 240 | 252 | ${lead}${proposals ? html`<div class="mt">${proposals}</div>` : null}`, |
| 253 | }); | |
| 241 | 254 | }); |
| 242 | 255 | } |
| 243 | 256 | const base = repoBase(repo); |
| 244 | 257 | const dir = repoDir(ctx.app.reposRoot, repo.ownerName, repo.name); |
| 245 | const refs = await listRefs(dir, "refs/heads/"); | |
| 258 | const refs = await timePhaseAsync(ctx.request, "refs", () => listRefs(dir, "refs/heads/")); | |
| 259 | let renderMs = 0; | |
| 260 | let since = performance.now(); | |
| 246 | 261 | const branches = refs.ok |
| 247 | 262 | ? refs.value.map((r) => r.name.replace("refs/heads/", "")).sort() |
| 248 | 263 | : []; |
| 249 | 264 | const tip = refs.ok |
| 250 | 265 | ? refs.value.find((r) => r.name === `refs/heads/${repo.defaultBranch}`)?.oid |
| 251 | 266 | : undefined; |
| 252 | const cellMemory = await checkMemoryAt(dir, tip); | |
| 267 | renderMs += performance.now() - since; | |
| 268 | const cellMemory = await timePhaseAsync(ctx.request, "check", () => checkMemoryAt(dir, tip)); | |
| 269 | since = performance.now(); | |
| 253 | 270 | if (!branches.includes(repo.defaultBranch)) |
| 254 | 271 | branches.unshift(repo.defaultBranch); |
| 255 | 272 | |
| @@ -372,7 +389,9 @@ export async function settingsPage(ctx: Ctx): Promise<Response> { | ||
| 372 | 389 | </form>`, |
| 373 | 390 | { head: html`${icon("folder")}<h2>Archive</h2>` }, |
| 374 | 391 | ); |
| 375 | const held = getMirrorTarget(ctx.app.db, repo.id); | |
| 392 | renderMs += performance.now() - since; | |
| 393 | const held = timePhase(ctx.request, "mirror", () => getMirrorTarget(ctx.app.db, repo.id)); | |
| 394 | since = performance.now(); | |
| 376 | 395 | const mirror = |
| 377 | 396 | held === undefined |
| 378 | 397 | ? null |
| @@ -388,7 +407,7 @@ export async function settingsPage(ctx: Ctx): Promise<Response> { | ||
| 388 | 407 | </form>`, |
| 389 | 408 | { head: html`${icon("branch")}<h2>Mirror</h2>` }, |
| 390 | 409 | ); |
| 391 | return settingsPageResponse( | |
| 410 | const response = settingsPageResponse( | |
| 392 | 411 | ctx, |
| 393 | 412 | repo, |
| 394 | 413 | "general", |
| @@ -401,6 +420,8 @@ export async function settingsPage(ctx: Ctx): Promise<Response> { | ||
| 401 | 420 | ? html`<div class="mt">${secrets}</div>` |
| 402 | 421 | : null}`, |
| 403 | 422 | ); |
| 423 | notePhase(ctx.request, "render", renderMs + performance.now() - since); | |
| 424 | return response; | |
| 404 | 425 | } |
| 405 | 426 | |
| 406 | 427 | export async function settingsUpdate(ctx: Ctx): Promise<Response> { |
| @@ -1538,15 +1559,17 @@ function promptsView(base: string, sources: PromptSourceView[]): Html { | ||
| 1538 | 1559 | |
| 1539 | 1560 | // GET /:owner/:name/settings/prompts |
| 1540 | 1561 | export function promptsSettingsPage(ctx: Ctx): Response { |
| 1541 | const repo = loadAdminRepo(ctx); | |
| 1562 | const repo = timedAdminRepo(ctx); | |
| 1542 | 1563 | if (!repo) return repoNotFound(ctx); |
| 1543 | const listed = listPromptSources(ctx.app, ctx.auth.user, repo); | |
| 1544 | return settingsPageResponse( | |
| 1545 | ctx, | |
| 1546 | repo, | |
| 1547 | "prompts", | |
| 1548 | "Prompts", | |
| 1549 | promptsView(repoBase(repo), listed.ok ? listed.value : []), | |
| 1564 | const listed = timePhase(ctx.request, "sources", () => listPromptSources(ctx.app, ctx.auth.user, repo)); | |
| 1565 | return timePhase(ctx.request, "render", () => | |
| 1566 | settingsPageResponse( | |
| 1567 | ctx, | |
| 1568 | repo, | |
| 1569 | "prompts", | |
| 1570 | "Prompts", | |
| 1571 | promptsView(repoBase(repo), listed.ok ? listed.value : []), | |
| 1572 | ), | |
| 1550 | 1573 | ); |
| 1551 | 1574 | } |
| 1552 | 1575 | |
| @@ -1739,10 +1762,11 @@ function labelEditor(base: string, label: Label): Html { | ||
| 1739 | 1762 | } |
| 1740 | 1763 | |
| 1741 | 1764 | export function labelsPage(ctx: Ctx): Response { |
| 1742 | const repo = loadAdminRepo(ctx); | |
| 1765 | const repo = timedAdminRepo(ctx); | |
| 1743 | 1766 | if (!repo) return repoNotFound(ctx); |
| 1767 | const labels = timePhase(ctx.request, "labels", () => listLabels(ctx.app.db, repo.id)); | |
| 1768 | const rendering = performance.now(); | |
| 1744 | 1769 | const base = repoBase(repo); |
| 1745 | const labels = listLabels(ctx.app.db, repo.id); | |
| 1746 | 1770 | |
| 1747 | 1771 | const list = |
| 1748 | 1772 | labels.length === 0 |
| @@ -1775,7 +1799,7 @@ export function labelsPage(ctx: Ctx): Response { | ||
| 1775 | 1799 | |
| 1776 | 1800 | const body = html`${list} |
| 1777 | 1801 | <div class="mt">${create}</div>`; |
| 1778 | return settingsPageResponse( | |
| 1802 | const response = settingsPageResponse( | |
| 1779 | 1803 | ctx, |
| 1780 | 1804 | repo, |
| 1781 | 1805 | "labels", |
| @@ -1783,6 +1807,8 @@ export function labelsPage(ctx: Ctx): Response { | ||
| 1783 | 1807 | body, |
| 1784 | 1808 | labelCss(labels), |
| 1785 | 1809 | ); |
| 1810 | notePhase(ctx.request, "render", performance.now() - rendering); | |
| 1811 | return response; | |
| 1786 | 1812 | } |
| 1787 | 1813 | |
| 1788 | 1814 | export async function labelCreate(ctx: Ctx): Promise<Response> { |
| @@ -1862,10 +1888,16 @@ export async function labelDelete(ctx: Ctx): Promise<Response> { | ||
| 1862 | 1888 | // ---- webhooks ---- |
| 1863 | 1889 | |
| 1864 | 1890 | export function hooksPage(ctx: Ctx): Response { |
| 1865 | const repo = loadAdminRepo(ctx); | |
| 1891 | const repo = timedAdminRepo(ctx); | |
| 1866 | 1892 | if (!repo) return repoNotFound(ctx); |
| 1893 | const hooks = timePhase(ctx.request, "hooks", () => listWebhooks(ctx.app.db, repo.id)); | |
| 1894 | const recent = timePhase( | |
| 1895 | ctx.request, | |
| 1896 | "deliveries", | |
| 1897 | () => new Map(hooks.map((h) => [h.id, recentDeliveries(ctx.app.db, h.id, 10)] as const)), | |
| 1898 | ); | |
| 1899 | const rendering = performance.now(); | |
| 1867 | 1900 | const base = repoBase(repo); |
| 1868 | const hooks = listWebhooks(ctx.app.db, repo.id); | |
| 1869 | 1901 | |
| 1870 | 1902 | const list = |
| 1871 | 1903 | hooks.length === 0 |
| @@ -1875,7 +1907,7 @@ export function hooksPage(ctx: Ctx): Response { | ||
| 1875 | 1907 | note: "Every event this repository produces can be POSTed to a URL you control, signed and retried.", |
| 1876 | 1908 | }) |
| 1877 | 1909 | : html`${hooks.map((hook) => { |
| 1878 | const deliveries = recentDeliveries(ctx.app.db, hook.id, 10); | |
| 1910 | const deliveries = recent.get(hook.id) ?? []; | |
| 1879 | 1911 | return card( |
| 1880 | 1912 | deliveries.length === 0 |
| 1881 | 1913 | ? html`<p class="muted small">No deliveries yet.</p>` |
| @@ -1972,7 +2004,9 @@ export function hooksPage(ctx: Ctx): Response { | ||
| 1972 | 2004 | |
| 1973 | 2005 | const body = html`${list} |
| 1974 | 2006 | <div class="mt">${create}</div>`; |
| 1975 | return settingsPageResponse(ctx, repo, "hooks", "Webhooks", body); | |
| 2007 | const response = settingsPageResponse(ctx, repo, "hooks", "Webhooks", body); | |
| 2008 | notePhase(ctx.request, "render", performance.now() - rendering); | |
| 2009 | return response; | |
| 1976 | 2010 | } |
| 1977 | 2011 | |
| 1978 | 2012 | export async function webhookCreate(ctx: Ctx): Promise<Response> { |
| @@ -2021,8 +2055,9 @@ export async function webhookDelete(ctx: Ctx): Promise<Response> { | ||
| 2021 | 2055 | // ---- danger zone ---- |
| 2022 | 2056 | |
| 2023 | 2057 | export function dangerPage(ctx: Ctx): Response { |
| 2024 | const repo = loadAdminRepo(ctx); | |
| 2058 | const repo = timedAdminRepo(ctx); | |
| 2025 | 2059 | if (!repo) return repoNotFound(ctx); |
| 2060 | const rendering = performance.now(); | |
| 2026 | 2061 | const base = repoBase(repo); |
| 2027 | 2062 | const full = `${repo.ownerName}/${repo.name}`; |
| 2028 | 2063 | |
| @@ -2048,7 +2083,9 @@ export function dangerPage(ctx: Ctx): Response { | ||
| 2048 | 2083 | <h2>Delete this repository</h2>`, |
| 2049 | 2084 | }, |
| 2050 | 2085 | ); |
| 2051 | return settingsPageResponse(ctx, repo, "danger", "Danger zone", body); | |
| 2086 | const response = settingsPageResponse(ctx, repo, "danger", "Danger zone", body); | |
| 2087 | notePhase(ctx.request, "render", performance.now() - rendering); | |
| 2088 | return response; | |
| 2052 | 2089 | } |
| 2053 | 2090 | |
| 2054 | 2091 | /** |
test/server-timing-sweep.test.ts −5
| @@ -24,11 +24,6 @@ const UNNAMED: Array<[path: string, issue: string]> = [ | ||
| 24 | 24 | ["/alice/forge", "#3325"], |
| 25 | 25 | ["/alice/forge/changes", "#3323"], |
| 26 | 26 | ["/alice/forge/map", "#3320"], |
| 27 | ["/alice/forge/settings", "#3390"], | |
| 28 | ["/alice/forge/settings/danger", "#3390"], | |
| 29 | ["/alice/forge/settings/hooks", "#3390"], | |
| 30 | ["/alice/forge/settings/labels", "#3390"], | |
| 31 | ["/alice/forge/settings/prompts", "#3390"], | |
| 32 | 27 | ["/alice/forge/tells", "#3322"], |
| 33 | 28 | ["/live/apps", "#3390"], |
| 34 | 29 | ["/live/on-you", "#3390"], |
test/settings-repo-timing.test.ts added +80
| @@ -0,0 +1,80 @@ | ||
| 1 | /** A repository's settings pages name session, each read and render in Server-Timing, and answer under 50 ms (#4318). */ | |
| 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 { createRepo } from "../src/actions/repos.ts"; | |
| 7 | import { createApp, type App } from "../src/app.ts"; | |
| 8 | import { createUser } from "../src/core/users.ts"; | |
| 9 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 10 | import { proveAll } from "./helpers/verified.ts"; | |
| 11 | import { quietLogs } from "./helpers/globals.ts"; | |
| 12 | ||
| 13 | quietLogs(); | |
| 14 | ||
| 15 | let root = ""; | |
| 16 | let app: App; | |
| 17 | let servers: Servers; | |
| 18 | let cookie = ""; | |
| 19 | ||
| 20 | beforeAll(async () => { | |
| 21 | root = mkdtempSync(join(tmpdir(), "forge-settings-timing-")); | |
| 22 | app = createApp({ dataDir: root, port: 0, forgeRepo: "alice/forge" }); | |
| 23 | const u = await createUser(app.db, { | |
| 24 | origin: "seed", | |
| 25 | trusted: true, | |
| 26 | username: "alice", | |
| 27 | email: "[email protected]", | |
| 28 | password: "a-good-passphrase", | |
| 29 | adminIfFirst: true, | |
| 30 | }); | |
| 31 | if (!u.ok) throw new Error(u.error); | |
| 32 | proveAll(app); | |
| 33 | const made = await createRepo(app, { id: u.value.id, username: "alice" } as never, { name: "forge" }); | |
| 34 | if (!made.ok) throw new Error(made.error); | |
| 35 | servers = await serve(app); | |
| 36 | const signin = await fetch(`${servers.url}/signin`, { | |
| 37 | method: "POST", | |
| 38 | headers: { origin: servers.url }, | |
| 39 | body: new URLSearchParams({ username: "alice", password: "a-good-passphrase" }), | |
| 40 | redirect: "manual", | |
| 41 | }); | |
| 42 | cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 43 | }); | |
| 44 | afterAll(() => { | |
| 45 | servers.stop(); | |
| 46 | app.close(); | |
| 47 | rmSync(root, { recursive: true, force: true }); | |
| 48 | }); | |
| 49 | ||
| 50 | const PAGES: [string, string[]][] = [ | |
| 51 | ["/alice/forge/settings", ["route", "session", "repo", "lead", "refs", "check", "mirror", "render", "compress"]], | |
| 52 | ["/alice/forge/settings/danger", ["route", "session", "repo", "render", "compress"]], | |
| 53 | ["/alice/forge/settings/hooks", ["route", "session", "repo", "hooks", "deliveries", "render", "compress"]], | |
| 54 | ["/alice/forge/settings/labels", ["route", "session", "repo", "labels", "render", "compress"]], | |
| 55 | ["/alice/forge/settings/prompts", ["route", "session", "repo", "sources", "render", "compress"]], | |
| 56 | ]; | |
| 57 | ||
| 58 | for (const [path, names] of PAGES) | |
| 59 | test(`GET ${path} names its phases and answers under 50 ms`, async () => { | |
| 60 | const get = async () => { | |
| 61 | const res = await fetch(`${servers.url}${path}`, { headers: { cookie } }); | |
| 62 | await res.text(); | |
| 63 | return res; | |
| 64 | }; | |
| 65 | await get(); // warm | |
| 66 | const res = await get(); | |
| 67 | expect(res.status).toBe(200); | |
| 68 | const parts = (res.headers.get("server-timing") ?? "").split(", "); | |
| 69 | const dur = (name: string) => Number(parts.find((p) => p.startsWith(`${name};`))?.match(/dur=([\d.]+)/)?.[1]); | |
| 70 | const got = parts.map((p) => p.split(";")[0]); | |
| 71 | for (const name of ["app", ...names]) { | |
| 72 | expect(got).toContain(name); | |
| 73 | expect(Number.isFinite(dur(name))).toBe(true); | |
| 74 | } | |
| 75 | expect(got.filter((n) => n === "render").length).toBe(1); | |
| 76 | expect(dur("app")).toBeLessThan(50); | |
| 77 | // The named phases cover the request (route and compress are the server's own); the issue's bound is 1 ms. | |
| 78 | const sum = names.reduce((t, n) => t + dur(n), 0); | |
| 79 | expect(Math.abs(dur("app") - sum)).toBeLessThanOrEqual(1); | |
A sum of up to 9 phases, each rounded to 0.1 ms, plus the unmeasured microtask gaps (after the handler, and between compress and |
||
| 80 | }); |
new above
offline
This puts framing on every request's hot path (
noteStartplus an extra async hop) just so these five pages can sum toappwithin 1 ms. Sibling pages (#4316, test/settings-timing.test.ts) meet the same bar with a 5 ms slack and no server change. That's fine to keep, but if a sixth page needsroute/compress, write this framing up in AGENTS.md so it doesn't become a second way of timing a page.