trevato/forge

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

More
Forks1g fMapg aRuns8g rEventsFlows

/search?q=a answers under 300 ms on a fixture of production's size, pinned by a test that reads its Server-Timing phases #11177

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/11168-search-q-a-answers-under-300-ms-on-a-fix → main 1 commit fork

  • Merged. scout/11168-search-q-a-answers-under-300-ms-on-a-fix was merged into main as f654866.

Review

Sign in to review this change.
3 files changed +119 −12
src/core/search.ts +22 −12
@@ -358,9 +358,10 @@ export function searchOriginRepos(
358358 /**
359359 * Issues and pulls whose title matches, inside a bounded set of repos.
360360 *
361 * `repo_id IN (...)` is not decoration: it is what lets SQLite walk the
362 * `issue(repo_id, …)` indexes instead of scanning every conversation in the
363 * forge, and `test/search.test.ts` asserts the plan stays that way.
361 * Each repository is its own arm, `repo_id = ?` walking `issue_repo_updated`
362 * newest first and stopping at `limit` matches: SQLite never scans every
363 * conversation in the forge, nor sorts every match of a one-letter query
364 * (#11168), and `test/search.test.ts` asserts the plan stays that way.
364365 */
365366 export function searchLocalIssues(
366367 db: Database,
@@ -374,8 +375,8 @@ export function searchLocalIssues(
374375 ): IssueHit[] {
375376 if (repos.length === 0) return [];
376377 const byId = new Map(repos.map((r) => [r.id, r]));
377 const ids = [...byId.keys()];
378 const holes = ids.map((_, i) => `?${i + 1}`).join(",");
378 // One arm a repository, so the compound stays far inside SQLite's 500.
379 const ids = [...byId.keys()].slice(0, MAX_SEARCH_REPOS);
379380 const open = [...sealedOk].filter((id) => byId.has(id));
380381 const extra: string[] = [...open, ...(sealedBy === undefined ? [] : [sealedBy])];
381382 const ors = [
@@ -386,6 +387,21 @@ export function searchLocalIssues(
386387 ...(sealedBy === undefined ? [] : [`author_id = ?${ids.length + 3 + open.length}`]),
387388 ];
388389 const sealedClause = `AND (${ors.join(" OR ")})`;
390 // One arm per repository, each walking `issue_repo_updated` newest first
391 // and stopping at `limit` matches: a one-letter query, which matches every
392 // title, reads at most `limit` rows a repository rather than sorting all of
393 // them (#11168). The arms are merged and cut to `limit` once more.
394 const arms = ids
395 .map(
396 (_, i) => `SELECT * FROM (SELECT repo_id, number, kind, title, state, updated_at
397 FROM issue
398 WHERE repo_id = ?${i + 1}
399 AND title LIKE ?${ids.length + 1} ESCAPE '\\' COLLATE NOCASE
400 ${sealedClause}
401 ORDER BY updated_at DESC
402 LIMIT ?${ids.length + 2})`,
403 )
404 .join("\nUNION ALL\n");
389405 const rows = db
390406 .query<
391407 {
@@ -398,13 +414,7 @@ export function searchLocalIssues(
398414 },
399415 (string | number)[]
400416 >(
401 `SELECT repo_id, number, kind, title, state, updated_at
402 FROM issue
403 WHERE repo_id IN (${holes})
404 AND title LIKE ?${ids.length + 1} ESCAPE '\\' COLLATE NOCASE
405 ${sealedClause}
406 ORDER BY updated_at DESC
407 LIMIT ?${ids.length + 2}`,
417 `SELECT * FROM (${arms}) ORDER BY updated_at DESC LIMIT ?${ids.length + 2}`,
408418 )
409419 .all(...ids, `%${likeEscape(query.trim())}%`, limit, ...extra);
410420
test/search-production-size.test.ts added +91
@@ -0,0 +1,91 @@
1 /**
2 * `/search?q=a` on a forge of production's size (#11168, the last box of #11120).
3 *
4 * The fixture is `big-forge.ts`'s PRODUCTION (9,000 issues and 9,000 pulls in
5 * one repository, with their runs, comments and events) plus REPOS more
6 * repositories, public and private, every name, description and title holding
7 * an `a`: the worst case for a one-letter query, which matches every row.
8 *
9 * The bound is read from the answer itself: each request's `Server-Timing`
10 * `app` phase is under APP_BUDGET_MS, and its statement count is fixed, so a
11 * read that grows with the rows (an N+1, an unbounded scan per repository)
12 * fails here in every run. The phases are printed so a pull can show them.
13 */
14 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
15 import { rm } from "node:fs/promises";
16 import type { App } from "../src/app.ts";
17 import { createSession } from "../src/core/sessions.ts";
18 import { serve, type Servers } from "../src/http/server.ts";
19 import { bigForge, PRODUCTION } from "./helpers/big-forge.ts";
20 import { expectWork, parsePhases, requestStatements } from "./helpers/work.ts";
21
22 /** The release proof reads `/search?q=a` as degraded above this. */
23 const APP_BUDGET_MS = 300;
24 /** Repositories beside the forge's own: more than production holds. */
25 const REPOS = 1_000;
26 /** What one `/search?q=a` costs in SQL, whatever the size of the forge. */
27 const STATEMENT_CEILING = 40;
28
29 let app: App, servers: Servers, dir = "", cookie = "";
30
31 beforeAll(async () => {
32 ({ app, dir } = bigForge(PRODUCTION));
33 const db = app.db;
34 db.transaction(() => {
35 // Every conversation's title matches the one letter.
36 db.run("UPDATE issue SET title = 'a change to area ' || number");
37 db.run("INSERT INTO user (id, username, email, password_hash) VALUES ('ana', 'ana', '[email protected]', '')");
38 for (let n = 1; n <= REPOS; n++)
39 db.run(
40 // Listed after the forge's own (repos list newest id first), which holds production's conversations.
41 "INSERT INTO repo (id, owner_id, name, visibility, description, created_at) VALUES (?1, ?2, ?3, ?4, 'a repository about apps', ?5)",
42 [`0${n}`, n % 2 === 0 ? "own" : "ana", `app-${n}`, n % 3 === 0 ? "private" : "public", new Date(Date.parse("2026-01-01T00:00:00Z") + n * 60_000).toISOString()],
43 );
44 })();
45 servers = await serve(app);
46 cookie = `forge_session=${createSession(db, "own").token}`;
47 }, 300_000);
48
49 afterAll(async () => {
50 servers?.stop();
51 app?.close();
52 if (dir !== "") await rm(dir, { recursive: true, force: true });
53 });
54
55 const viewers: Array<[string, () => RequestInit]> = [
56 ["signed out", () => ({})],
57 ["signed in", () => ({ headers: { cookie } })],
58 ];
59
60 describe("/search?q=a at production's size", () => {
61 test("the fixture is production's size, and every row matches", () => {
62 const n = (sql: string) => (app.db.query(sql).get() as { n: number }).n;
63 expect(n("SELECT COUNT(*) AS n FROM issue WHERE kind = 'issue'")).toBe(PRODUCTION.issues);
64 expect(n("SELECT COUNT(*) AS n FROM issue WHERE kind = 'pull'")).toBe(PRODUCTION.pulls);
65 expect(n("SELECT COUNT(*) AS n FROM repo")).toBe(REPOS + 1);
66 expect(n("SELECT COUNT(*) AS n FROM issue WHERE title LIKE '%a%'")).toBe(PRODUCTION.issues + PRODUCTION.pulls);
67 });
68
69 for (const [who, init] of viewers) {
70 test(`${who}: the app phase is under ${APP_BUDGET_MS} ms and the statements are fixed`, async () => {
71 const url = `${servers.url}/search?q=a`;
72 // The first request warms what any page warms (statements, the session).
73 await (await fetch(url, init())).text();
74 const res = await fetch(url, init());
75 const body = await res.text();
76 expect(res.status).toBe(200);
77 expect(body).toContain("Code is searched for 2 or more characters");
78 // Titles are drawn with the letter marked, so the link is what is matched.
79 expect(body).toMatch(/href="\/owner\/forge\/(issues|pulls?)\/\d+"/);
80 const header = res.headers.get("server-timing") ?? "";
81 console.log(`/search?q=a ${who}: ${header}`);
82 const phases = parsePhases(header);
83 const appMs = phases.find((p) => p.name === "app")?.ms;
84 expect(appMs).toBeDefined();
85 expect(appMs!).toBeLessThan(APP_BUDGET_MS);
86 for (const name of ["repos", "issues", "apps", "render"])
87 expect(phases.find((p) => p.name === name)?.ms ?? Infinity).toBeLessThan(APP_BUDGET_MS);
88 expectWork(await requestStatements(app.db, url, init()), { max: STATEMENT_CEILING });
89 }, 60_000);
90 }
91 });
test/search.test.ts +6
@@ -613,6 +613,12 @@ describe("search: one query, four sources", () => {
613613 // prevent; a SEARCH (with or without a sort) is the pass.
614614 expect(details.some((d) => /^SCAN\b.*\bissue\b/.test(d))).toBe(false);
615615 expect(details.some((d) => /^SEARCH\b.*\bissue\b/.test(d))).toBe(true);
616 // One walk a repository, newest first by `issue_repo_updated`, each cut at
617 // the limit: a query matching every title reads `limit` rows a repository,
618 // not every row it has (#11168).
619 const walks = details.filter((d) => /^SEARCH issue\b/.test(d));
620 expect(walks.length).toBe(repos.length);
621 for (const d of walks) expect(d).toBe("SEARCH issue USING INDEX issue_repo_updated (repo_id=?)");
616622 });
617623
618624 test("bob's own view is bounded the same way", () => {

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs