Public: anyone can read this; the crew can change it.
/search answers under 80 ms signed out, Server-Timing names repos, issues and code, finished from pull #7064's branch #7399
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/7112-search-answers-under-80-ms-signed-out-se → main 2 commits fork 4 threads · 4 unresolved
7 files
changed +127
−15
src/actions/search.ts +25 −1
| @@ -70,8 +70,13 @@ export interface SearchInput { | ||
| 70 | 70 | repo?: Repo; |
| 71 | 71 | /** Start no new grep after this many ms and report `moreCode`. @default unset: grep every repository */ |
| 72 | 72 | codeBudgetMs?: number; |
| 73 | /** Told how long each part took, once per part asked for, for Server-Timing. */ | |
| 74 | phase?: (name: SearchPhase, ms: number) => void; | |
| 73 | 75 | } |
| 74 | 76 | |
| 77 | /** The parts a search is timed in. */ | |
| 78 | export type SearchPhase = "repos" | "issues" | "code"; | |
| 79 | ||
| 75 | 80 | /** |
| 76 | 81 | * Search everything the actor can reach. Anonymous callers get the public |
| 77 | 82 | * half — the same rows a signed-out repo listing shows — and no origins, |
| @@ -100,6 +105,9 @@ export async function search( | ||
| 100 | 105 | |
| 101 | 106 | const wants = (which: SearchKind) => kind === "all" || kind === which; |
| 102 | 107 | |
| 108 | const phase = input.phase ?? (() => {}); | |
| 109 | // The visibility pass is the first of the repos' work, so its clock starts here. | |
| 110 | const reposAt = performance.now(); | |
| 103 | 111 | // One visibility pass. Issues and code both search "the repos this actor |
| 104 | 112 | // may read", so it is resolved once and shared rather than re-derived. |
| 105 | 113 | const visible = |
| @@ -118,6 +126,10 @@ export async function search( | ||
| 118 | 126 | ...(actor ? searchOriginRepos(app.db, actor.id, q, limit) : []), |
| 119 | 127 | ]; |
| 120 | 128 | |
| 129 | // Named whenever it did work: asked for, or the pass issues and code share. | |
| 130 | if (wants("repos") || (input.repo === undefined && visible.length > 0)) phase("repos", performance.now() - reposAt); | |
| 131 | ||
| 132 | const issuesAt = performance.now(); | |
| 121 | 133 | const issues: IssueHit[] = wants("issues") |
| 122 | 134 | ? searchLocalIssues( |
| 123 | 135 | app.db, |
| @@ -137,7 +149,9 @@ export async function search( | ||
| 137 | 149 | if (wants("issues") && input.live === true && actor !== undefined) { |
| 138 | 150 | issues.push(...(await liveIssues(app, actor, q, limit))); |
| 139 | 151 | } |
| 152 | if (wants("issues")) phase("issues", performance.now() - issuesAt); | |
| 140 | 153 | |
| 154 | const codeAt = performance.now(); | |
| 141 | 155 | const code = wants("code") && input.withCode !== false |
| 142 | 156 | ? await grepRepos( |
| 143 | 157 | app.reposRoot, |
| @@ -149,6 +163,7 @@ export async function search( | ||
| 149 | 163 | { memberId: actor?.id }, |
| 150 | 164 | ) |
| 151 | 165 | : { hits: [], truncated: false, more: false, skipped: false }; |
| 166 | if (wants("code")) phase("code", performance.now() - codeAt); | |
| 152 | 167 | |
| 153 | 168 | return { |
| 154 | 169 | repos, |
| @@ -237,9 +252,14 @@ export async function searchWithin( | ||
| 237 | 252 | const truncated = { repos: false, issues: false, code: false }; |
| 238 | 253 | let moreCode = false; |
| 239 | 254 | let codeSkipped = false; |
| 255 | // One figure per part, summed over the repositories, not one per repository. | |
| 256 | const spent: Partial<Record<SearchPhase, number>> = {}; | |
| 257 | const sum = (name: SearchPhase, ms: number) => { | |
| 258 | spent[name] = (spent[name] ?? 0) + ms; | |
| 259 | }; | |
| 240 | 260 | for (const repo of repos) { |
outdated
Nothing tests this summing. Add a run-scoped search test over two or more repositories that asserts |
||
| 241 | 261 | if (!canRead(app.db, repo, actor)) continue; |
| 242 | const one = await search(app, actor, { ...input, live: false, repo }); | |
| 262 | const one = await search(app, actor, { ...input, live: false, repo, phase: sum }); | |
| 243 | 263 | hits.repos.push(...one.repos); |
| 244 | 264 | hits.issues.push(...one.issues); |
| 245 | 265 | hits.code.push(...one.code); |
| @@ -249,5 +269,9 @@ export async function searchWithin( | ||
| 249 | 269 | moreCode ||= one.moreCode; |
| 250 | 270 | codeSkipped ||= one.codeSkipped; |
| 251 | 271 | } |
| 272 | for (const name of ["repos", "issues", "code"] as const) { | |
| 273 | const ms = spent[name]; | |
| 274 | if (ms !== undefined) input.phase?.(name, ms); | |
| 275 | } | |
| 252 | 276 | return { ...hits, truncated, moreCode, codeSkipped }; |
| 253 | 277 | } |
src/http/api.ts +5 −6
| @@ -138,7 +138,7 @@ import { holdPull, releasePull } from "../actions/holds.ts"; | ||
| 138 | 138 | import { getHold, type Hold } from "../core/holds.ts"; |
| 139 | 139 | import { watchMilestone } from "../actions/watches.ts"; |
| 140 | 140 | import { THOUGHT_MAX, keepThought } from "../actions/thought.ts"; |
| 141 | import { search, searchWithin } from "../actions/search.ts"; | |
| 141 | import { search, searchWithin, type SearchPhase } from "../actions/search.ts"; | |
| 142 | 142 | import { runRepos } from "./run_gate.ts"; |
| 143 | 143 | import { |
| 144 | 144 | commitToBranch, |
| @@ -2075,11 +2075,10 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2075 | 2075 | : { codeBudgetMs: ctx.app.config.codeBudgetMs ?? CODE_BUDGET_MS }), |
| 2076 | 2076 | }; |
| 2077 | 2077 | const mine = runRepos(ctx.app, ctx.auth); |
| 2078 | const found = await timePhaseAsync(ctx.request, "search", () => | |
| 2079 | mine === undefined | |
| 2080 | ? search(ctx.app, ctx.auth.user, input) | |
| 2081 | : searchWithin(ctx.app, ctx.auth.user, mine, input), | |
| 2082 | ); | |
| 2078 | const timed = { ...input, phase: (name: SearchPhase, ms: number) => notePhase(ctx.request, name, ms) }; | |
| 2079 | const found = await (mine === undefined | |
| 2080 | ? search(ctx.app, ctx.auth.user, timed) | |
| 2081 | : searchWithin(ctx.app, ctx.auth.user, mine, timed)); | |
| 2083 | 2082 | return timePhase(ctx.request, "render", () => json(found)); |
| 2084 | 2083 | } finally { |
| 2085 | 2084 | held?.release(); |
src/web/pages/search.ts +6 −3
| @@ -27,7 +27,7 @@ import { | ||
| 27 | 27 | type SearchResults, |
| 28 | 28 | } from "../../core/search.ts"; |
| 29 | 29 | import type { Ctx } from "../../http/context.ts"; |
| 30 | import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts"; | |
| 30 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; | |
| 31 | 31 | import { notFoundPage, pageHead } from "../chrome.ts"; |
| 32 | 32 | import { html, when, type Html } from "../html.ts"; |
| 33 | 33 | import { DELETE_REPO, STOP_APP } from "../danger_words.ts"; |
| @@ -321,7 +321,9 @@ async function find( | ||
| 321 | 321 | ctx.auth.user === undefined && |
| 322 | 322 | (a.kind === "code" || a.kind === "all") && |
| 323 | 323 | cleanQuery(a.query) !== ""; |
| 324 | const held = greps ? strangerCodeSearch(ctx.app, ctx.request) : undefined; | |
| 324 | const held = greps | |
| 325 | ? timePhase(ctx.request, "gate", () => strangerCodeSearch(ctx.app, ctx.request)) | |
| 326 | : undefined; | |
| 325 | 327 | const withCode = held === undefined || held.allowed; |
| 326 | 328 | try { |
| 327 | 329 | const found = await search(ctx.app, ctx.auth.user, { |
| @@ -329,6 +331,7 @@ async function find( | ||
| 329 | 331 | kind: a.kind, |
| 330 | 332 | live: a.live, |
| 331 | 333 | withCode, |
| 334 | phase: (name, ms) => notePhase(ctx.request, name, ms), | |
| 332 | 335 | ...(a.codeAll |
| 333 | 336 | ? {} |
| 334 | 337 | : { codeBudgetMs: ctx.app.config.codeBudgetMs ?? CODE_BUDGET_MS }), |
| @@ -403,7 +406,7 @@ export async function searchPage(ctx: Ctx): Promise<Response> { | ||
| 403 | 406 | return blank; |
| 404 | 407 | } |
| 405 | 408 | |
| 406 | const { found, busy } = await timePhaseAsync(ctx.request, "search", () => find(ctx, a)); | |
| 409 | const { found, busy } = await find(ctx, a); | |
| 407 | 410 | const apps: Apps = timePhase(ctx.request, "apps", () => { |
| 408 | 411 | const seen = appsOf(ctx, ctx.auth.user); |
| 409 | 412 | return { |
test/api-search-timing.test.ts +3 −2
| @@ -36,9 +36,10 @@ async function phases(path: string): Promise<{ status: number; names: string[]; | ||
| 36 | 36 | return { status: res.status, names: names.slice(names.indexOf("app") + 1), body }; |
| 37 | 37 | } |
| 38 | 38 | |
| 39 | test("the API search names session, search and render in order", async () => { | |
| 39 | test("the API search names session, then the parts asked for, then render", async () => { | |
| 40 | 40 | const got = await phases("/api/v1/search?q=forge&kind=repos"); |
| 41 | 41 | expect(got.status).toBe(200); |
| 42 | expect(got.names).toEqual(["session", "search", "render"]); | |
| 42 | expect(got.names).toEqual(["session", "repos", "render"]); | |
| 43 | expect((await phases("/api/v1/search?q=forge")).names).toEqual(["session", "repos", "issues", "code", "render"]); | |
| 43 | 44 | expect(typeof JSON.parse(got.body)).toBe("object"); |
| 44 | 45 | }); |
test/search-budget.test.ts +35 −1
| @@ -3,8 +3,9 @@ import { afterAll, beforeAll, expect, test } from "bun:test"; | ||
| 3 | 3 | import { cp, mkdir, mkdtemp, rm } from "node:fs/promises"; |
| 4 | 4 | import { tmpdir } from "node:os"; |
| 5 | 5 | import { join } from "node:path"; |
| 6 | import { CODE_BUDGET_MS, grepCount, grepRepos } from "../src/core/search.ts"; | |
| 6 | import { CODE_BUDGET_MS, grepCount, grepRepos, type GrepWho } from "../src/core/search.ts"; | |
| 7 | 7 | import type { Repo } from "../src/core/repos.ts"; |
| 8 | import { createGrepSlots } from "../src/search/grep_slots.ts"; | |
| 8 | 9 | |
| 9 | 10 | let root = ""; |
| 10 | 11 | const repos: Repo[] = []; |
| @@ -67,3 +68,36 @@ test("a repo that cannot be grepped, such as one with no commits, leaves more fa | ||
| 67 | 68 | expect(r.more).toBe(false); |
| 68 | 69 | expect(r.hits).toEqual([]); |
| 69 | 70 | }); |
| 71 | ||
| 72 | test("a grep that never settles is cut at the budget: the first batch's repositories are kept and the abandoned slots free only when it settles", async () => { | |
| 73 | const slots = createGrepSlots(); | |
| 74 | const eight = Array.from({ length: 8 }, (_, i) => ({ id: `n${i}`, ownerName: "u", name: `n${i}`, defaultBranch: "main" }) as Repo); | |
| 75 | const open: (() => void)[] = []; | |
| 76 | const started: Promise<unknown>[] = []; | |
| 77 | let allFour!: () => void; | |
| 78 | const fourStarted = new Promise<void>((resolve) => (allFour = resolve)); | |
| 79 | const grep: NonNullable<GrepWho["grep"]> = async (dir) => { | |
| 80 | if (/n[0-3](\.git)?$/.test(dir)) return [{ path: "a.txt", line: 1, text: "sqlite" }]; | |
| 81 | const never = new Promise<never[]>((resolve) => open.push(() => resolve([]))); | |
| 82 | started.push(never); | |
| 83 | if (started.length === 4) allFour(); | |
| 84 | return never; | |
| 85 | }; | |
| 86 | const cut = await grepRepos(root, eight, "sqlite", 200, 30, { | |
| 87 | grep, | |
outdated
Nit: |
||
| 88 | slots, | |
| 89 | now: () => 0, | |
| 90 | timer: () => ({ done: Promise.resolve(), clear() {} }), | |
| 91 | }); | |
| 92 | expect(cut.more).toBe(true); | |
| 93 | expect(cut.hits.length).toBeGreaterThanOrEqual(1); | |
| 94 | expect(cut.hits.map((h) => h.repo.name)).toEqual(["n0", "n1", "n2", "n3"]); | |
| 95 | // The abandoned batch starts its greps after the answer; wait for them, not a clock. | |
| 96 | await fourStarted; | |
| 97 | // The four abandoned greps still hold the strangers' four slots. | |
| 98 | expect(slots.held()).toEqual({ total: 4, strangers: 4 }); | |
| 99 | for (const settle of open) settle(); | |
| 100 | await Promise.allSettled(started); | |
| 101 | await Promise.resolve(); | |
| 102 | expect(slots.held()).toEqual({ total: 0, strangers: 0 }); | |
| 103 | }); |
test/search-timing.test.ts +2 −2
| @@ -36,12 +36,12 @@ const get = (path: string) => fetch(servers.url + path, { headers: { authorizati | ||
| 36 | 36 | const phases = (res: Response) => |
| 37 | 37 | (res.headers.get("server-timing") ?? "").split(", ").map((p) => p.split(";")[0]!); |
| 38 | 38 | |
| 39 | test("a search names session, search, apps and render once each, with durations", async () => { | |
| 39 | test("a search names session, repos, issues, code, apps and render once each, with durations", async () => { | |
| 40 | 40 | const res = await get("/search?q=forge"); |
| 41 | 41 | expect(res.status).toBe(200); |
| 42 | 42 | expect(await res.text()).toContain("Results for"); |
| 43 | 43 | const header = res.headers.get("server-timing") ?? ""; |
| 44 | for (const n of ["session", "search", "apps", "render"]) { | |
| 44 | for (const n of ["session", "repos", "issues", "code", "apps", "render"]) { | |
| 45 | 45 | expect(phases(res).filter((x) => x === n).length).toBe(1); |
| 46 | 46 | const part = header.split(", ").find((p) => p.startsWith(`${n};`))!; |
| 47 | 47 | expect(Number(/dur=([\d.]+)/.exec(part)?.[1])).not.toBeNaN(); |
test/search-within-timing.test.ts added +51
| @@ -0,0 +1,51 @@ | ||
| 1 | /** A run-scoped search names each part once, however many repositories it spans (#7112). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { searchWithin, type SearchPhase } from "../src/actions/search.ts"; | |
| 7 | import { createApp } from "../src/app.ts"; | |
| 8 | import { getRepo } from "../src/core/repos.ts"; | |
| 9 | import { createToken } from "../src/core/tokens.ts"; | |
| 10 | import { createUser } from "../src/core/users.ts"; | |
| 11 | import { serve } from "../src/http/server.ts"; | |
| 12 | import { quietLogs } from "./helpers/globals.ts"; | |
| 13 | ||
| 14 | quietLogs(); | |
| 15 | ||
| 16 | let dir = ""; | |
| 17 | let app: ReturnType<typeof createApp>; | |
| 18 | let servers: Awaited<ReturnType<typeof serve>>; | |
| 19 | ||
| 20 | beforeAll(async () => { | |
| 21 | dir = await mkdtemp(join(tmpdir(), "forge-search-within-")); | |
| 22 | app = createApp({ dataDir: dir, port: 0 }); | |
| 23 | servers = await serve(app); | |
| 24 | const made = await createUser(app.db, { | |
| 25 | origin: "seed", trusted: true, username: "alice", email: "[email protected]", | |
| 26 | password: "a-good-passphrase", adminIfFirst: true, | |
| 27 | }); | |
| 28 | if (!made.ok) throw new Error(made.error); | |
| 29 | const bearer = createToken(app.db, made.value.id, "t", "admin").token; | |
| 30 | for (const name of ["one", "two"]) { | |
| 31 | await fetch(`${servers.url}/api/v1/repos`, { | |
| 32 | method: "POST", | |
| 33 | headers: { authorization: `Bearer ${bearer}`, "content-type": "application/json" }, | |
| 34 | body: JSON.stringify({ name, visibility: "public" }), | |
| 35 | }); | |
| 36 | } | |
| 37 | }, 60_000); | |
| 38 | ||
| 39 | afterAll(async () => { | |
| 40 | servers.stop(); | |
| 41 | app.close(); | |
| 42 | await rm(dir, { recursive: true, force: true }); | |
| 43 | }); | |
| 44 | ||
| 45 | test("searchWithin over two readable repositories reports repos, issues and code once each", async () => { | |
| 46 | const repos = ["one", "two"].map((n) => getRepo(app.db, "alice", n)!); | |
| 47 | expect(repos.every((r) => r !== undefined)).toBe(true); | |
| 48 | const named: SearchPhase[] = []; | |
| 49 | await searchWithin(app, undefined, repos, { q: "one", phase: (name) => named.push(name) }); | |
| 50 | expect(named.sort()).toEqual(["code", "issues", "repos"]); | |
| 51 | }); |
new above
offline
Non-blocking: the condition depends on what the pass returned, not on whether it ran. When
kind=codeorkind=issuesand no repository is visible,listReposVisibleTostill runs, but nothing names it. The preview has no public repositories and shows exactly that:session, gate, code, apps, render.wants("repos") || (input.repo === undefined && (wants("issues") || wants("code")))says what the comment means. The gap is sub-millisecond, so this can wait.