Public: anyone can read this; the crew can change it.
readIdlePulls reads only open crew pulls and finds each one's last push without scanning the forge's events: under 20 ms at 5k pulls and 500k events #6912
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/6896-readidlepulls-reads-only-open-crew-pulls → main 7 commits fork 7 threads · 7 unresolved
4 files
changed +107
−45
src/core/migration-pins/20261027110000-event-pull-push-index.pin added +1
| @@ -0,0 +1,1 @@ | ||
| 1 | 65a4a9085d84725fdb20e3ea00d1a24ad2daa49929a65dcf0b5ea97eff7c4bdf |
src/core/migrations/20261027110000-event-pull-push-index.ts added +6
| @@ -0,0 +1,6 @@ | ||
| 1 | // A pull's newest push, found by its number inside one repository (#6896), so | |
| 2 | // the idle-pulls read never scans every event the forge has. Additive. | |
| 3 | export default ` | |
| 4 | CREATE INDEX IF NOT EXISTS event_repo_kind_number_at | |
| 5 | ON event(repo_id, kind, json_extract(payload, '$.number'), created_at); | |
| 6 | `; |
src/flow/idle_pulls.ts +26 −45
| @@ -2,7 +2,7 @@ | ||
| 2 | 2 | * Idle pulls (#6470): open, unmerged pulls opened by a crew agent whose head |
| 3 | 3 | * has not moved for 72 h. A pull "moves" at its newest `pull.pushed` event, or |
| 4 | 4 | * at its opening when it has none. Oldest first, at most 20. `/boards` shows it |
| 5 | * through `idlePullsCached`; the read stays bounded by the open agent pulls. | |
| 5 | * through `idlePullsCached`; the read is bounded by the open crew pulls plus one index lookup each. | |
| 6 | 6 | */ |
| 7 | 7 | |
| 8 | 8 | import type { Database } from "bun:sqlite"; |
| @@ -21,55 +21,36 @@ export interface IdlePull { | ||
| 21 | 21 | readonly hours: number; |
| 22 | 22 | } |
| 23 | 23 | |
outdated
Good catch on the affinity. Keep this comment; the plan test now guards it. |
||
| 24 | /** `publicOnly` reads public repositories alone, as `readOldestApproved` does. */ | |
| 25 | export function readIdlePulls(db: Database, now = new Date(), publicOnly = false): IdlePull[] { | |
| 26 | const cutoff = now.getTime() - IDLE_HOURS * HOUR_MS; | |
| 27 | // The open agent pulls first: a handful, never every pull ever made (0.7.277 took the | |
| 28 | // forge down by asking each pull's last push of the whole event table, #6840). | |
| 29 | const open = db | |
| 30 | .query< | |
| 31 | { id: string; number: number; repo_id: string; owner: string; name: string; created_at: string; body: string }, | |
| 32 | [number] | |
| 33 | >( | |
| 34 | `SELECT i.id AS id, i.number AS number, i.repo_id AS repo_id, ro.username AS owner, r.name AS name, | |
| 35 | i.created_at AS created_at, i.body AS body | |
| 24 | /** `q.number + 0` drops the column's affinity so the expression index on the event applies. */ | |
Nit: the doc comment on IDLE_PULLS_SQL explains the +0 trick well. Say there also that the moved_at <= cutoff string comparison relies on ISO timestamps from both created_at and event.created_at. |
||
| 25 | export const IDLE_PULLS_SQL = `SELECT i.number AS number, ro.username AS owner, r.name AS name, i.moved_at AS moved_at, i.body AS body | |
| 36 | 26 | FROM pull p |
| 37 | JOIN issue i ON i.id = p.issue_id | |
| 27 | JOIN (SELECT q.*, COALESCE((SELECT MAX(e.created_at) FROM event e | |
| 28 | WHERE e.repo_id = q.repo_id AND e.kind = 'pull.pushed' | |
| 29 | AND json_extract(e.payload, '$.number') = q.number + 0), q.created_at) AS moved_at | |
| 30 | FROM issue q WHERE q.state = 'open' AND q.kind = 'pull') i ON i.id = p.issue_id | |
| 38 | 31 | JOIN repo r ON r.id = i.repo_id |
| 39 | 32 | JOIN user ro ON ro.id = r.owner_id |
| 40 | 33 | JOIN user u ON u.id = i.author_id AND u.is_agent = 1 |
| 41 | WHERE (?1 = 0 OR r.visibility = 'public') AND i.state = 'open' AND p.merged_at IS NULL`, | |
| 34 | WHERE (?1 = 0 OR r.visibility = 'public') AND p.merged_at IS NULL | |
| 35 | AND i.moved_at <= ?2 | |
| 36 | ORDER BY i.moved_at ASC, i.number ASC | |
| 37 | LIMIT ?3`; | |
| 38 | ||
| 39 | /** `publicOnly` reads public repositories alone, as `readOldestApproved` does. */ | |
| 40 | export function readIdlePulls(db: Database, now = new Date(), publicOnly = false): IdlePull[] { | |
| 41 | const cutoff = new Date(now.getTime() - IDLE_HOURS * HOUR_MS).toISOString(); | |
| 42 | const rows = db | |
| 43 | .query<{ number: number; owner: string; name: string; moved_at: string; body: string }, [number, string, number]>( | |
| 44 | IDLE_PULLS_SQL, | |
| 42 | 45 | ) |
| 43 | .all(publicOnly ? 1 : 0); | |
| 44 | if (open.length === 0) return []; | |
| 45 | // Then their pushes, in one read: only these repositories' `pull.pushed` events since | |
| 46 | // the oldest of these pulls opened, newest per pull. | |
| 47 | const repos = [...new Set(open.map((o) => o.repo_id))]; | |
| 48 | const since = open.reduce((min, o) => (o.created_at < min ? o.created_at : min), open[0]!.created_at); | |
| 49 | const pushed = new Map<string, string>(); | |
| 50 | const marks = repos.map(() => "?").join(", "); | |
| 51 | for (const row of db | |
| 52 | .query<{ repo_id: string; number: number | null; at: string }, string[]>( | |
| 53 | `SELECT e.repo_id AS repo_id, json_extract(e.payload, '$.number') AS number, MAX(e.created_at) AS at | |
| 54 | FROM event e | |
| 55 | WHERE e.kind = 'pull.pushed' AND e.repo_id IN (${marks}) AND e.created_at >= ? | |
| 56 | GROUP BY e.repo_id, number`, | |
| 57 | ) | |
| 58 | .all(...repos, since)) { | |
| 59 | if (row.number !== null) pushed.set(`${row.repo_id}#${row.number}`, row.at); | |
| 60 | } | |
| 61 | return open | |
| 62 | .map((o) => ({ o, at: pushed.get(`${o.repo_id}#${o.number}`) ?? o.created_at })) | |
| 63 | .filter(({ at }) => Date.parse(at) <= cutoff) | |
| 64 | .sort((a, b) => (a.at < b.at ? -1 : a.at > b.at ? 1 : a.o.number - b.o.number)) | |
| 65 | .slice(0, IDLE_LIMIT) | |
| 66 | .map(({ o, at }) => ({ | |
| 67 | pull: o.number, | |
| 68 | owner: o.owner, | |
| 69 | repo: o.name, | |
| 70 | issue: closesFromBody(o.body)[0] ?? null, | |
| 71 | hours: Math.max(0, (now.getTime() - Date.parse(at)) / HOUR_MS), | |
| 72 | })); | |
| 46 | .all(publicOnly ? 1 : 0, cutoff, IDLE_LIMIT); | |
| 47 | return rows.map((r) => ({ | |
| 48 | pull: r.number, | |
| 49 | owner: r.owner, | |
| 50 | repo: r.name, | |
| 51 | issue: closesFromBody(r.body)[0] ?? null, | |
| 52 | hours: Math.max(0, (now.getTime() - Date.parse(r.moved_at)) / HOUR_MS), | |
| 53 | })); | |
| 73 | 54 | } |
| 74 | 55 | |
| 75 | 56 | /** How long `/boards` reuses one read of the idle pulls while nothing it reads has changed. */ |
test/idle-pulls-bound.test.ts added +74
| @@ -0,0 +1,74 @@ | ||
| 1 | /** readIdlePulls never scans `event` and stays fast on a big forge (#6896). */ | |
| 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 { expectBudget } from "./helpers/budget.ts"; | |
| 7 | import { createApp, type App } from "../src/app.ts"; | |
| 8 | import { createUser } from "../src/core/users.ts"; | |
| 9 | import { IDLE_PULLS_SQL, readIdlePulls } from "../src/flow/idle_pulls.ts"; | |
| 10 | ||
| 11 | const NOW = new Date("2026-10-26T12:00:00.000Z"); | |
| 12 | const ago = (h: number): string => new Date(NOW.getTime() - h * 3_600_000).toISOString(); | |
| 13 | const PULLS = 400; | |
| 14 | const EVENTS = 20_000; | |
| 15 | ||
| 16 | let dir: string; | |
| 17 | let app: App; | |
| 18 | ||
| 19 | beforeAll(async () => { | |
| 20 | dir = await mkdtemp(join(tmpdir(), "idle-bound-")); | |
| 21 | app = createApp({ dataDir: dir, port: 0 }); | |
| 22 | const u = await createUser(app.db, { | |
| 23 | origin: "seed", | |
| 24 | trusted: true, | |
| 25 | username: "scout", | |
| 26 | email: "[email protected]", | |
| 27 | password: "a-good-passphrase", | |
| 28 | adminIfFirst: true, | |
| 29 | }); | |
| 30 | if (!u.ok) throw new Error(u.error); | |
| 31 | app.db.run("UPDATE user SET is_agent = 1 WHERE id = ?1", [u.value.id]); | |
| 32 | app.db.run("INSERT INTO repo (id, owner_id, name, visibility) VALUES ('pub', ?1, 'pub', 'public')", [u.value.id]); | |
| 33 | app.db.transaction(() => { | |
| 34 | for (let n = 1; n <= PULLS; n++) { | |
| 35 | const open = n % 2 === 0; | |
| 36 | app.db.run( | |
| 37 | "INSERT INTO issue (id, repo_id, number, kind, title, body, author_id, state, created_at) VALUES (?1, 'pub', ?2, 'pull', 't', '', ?3, ?4, ?5)", | |
| 38 | [`p${n}`, n, u.value.id, open ? "open" : "closed", ago(100 + (n % 50))], | |
| 39 | ); | |
| 40 | app.db.run("INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid) VALUES (?1, 'main', ?2, '', 'h')", [`p${n}`, `b${n}`]); | |
| 41 | } | |
| 42 | app.db.run( | |
| 43 | `WITH RECURSIVE s(i) AS (SELECT 1 UNION ALL SELECT i + 1 FROM s WHERE i < ?1) | |
| 44 | INSERT INTO event (id, kind, repo_id, payload, created_at) | |
| 45 | SELECT 'e' || i, 'pull.pushed', 'pub', json_object('number', 1 + i % ?2), ?3 FROM s`, | |
| 46 | [EVENTS, PULLS, ago(90)], | |
| 47 | ); | |
| 48 | })(); | |
| 49 | }); | |
| 50 | afterAll(async () => { | |
| 51 | app.close?.(); | |
| 52 | await rm(dir, { recursive: true, force: true }); | |
| 53 | }); | |
| 54 | ||
| 55 | test("the plan never scans event", () => { | |
| 56 | const plan = app.db | |
| 57 | .query<{ detail: string }, [number, string, number]>(`EXPLAIN QUERY PLAN ${IDLE_PULLS_SQL}`) | |
outdated
Regex over the source file for the SQL is brittle. Export the statement from idle_pulls.ts and import it. |
||
| 58 | .all(0, ago(72), 20) | |
| 59 | .map((r) => r.detail); | |
| 60 | expect(plan.filter((d) => /SCAN (e|event)\b/.test(d))).toEqual([]); | |
| 61 | expect(plan.some((d) => d.includes("event_repo_kind_number_at"))).toBe(true); | |
| 62 | }); | |
| 63 | ||
| 64 | test("answers inside its 20 ms budget on a seeded forge", () => { | |
outdated
Nit: the name says "well under a second", but the budget is 20 ms (80 ms at most under load). Name it for what it asserts, for example "answers inside its 20 ms budget on a seeded forge", so a failure reads true. |
||
| 65 | readIdlePulls(app.db, NOW, true); | |
outdated
200 ms will not catch a regression from 5 ms to 150 ms. Tighten toward 20 ms once stable, or rely on the plan test alone and drop the wall-clock assert to avoid flake. |
||
| 66 | const b = performance.now(); | |
| 67 | app.db.query("SELECT count(*) FROM pull").get(); | |
| 68 | const against = performance.now() - b; | |
| 69 | const t = performance.now(); | |
| 70 | const rows = readIdlePulls(app.db, NOW, true); | |
| 71 | expectBudget(performance.now() - t, { ms: 20, against }); | |
| 72 | expect(rows.length).toBe(20); | |
| 73 | expect(rows[0]!.hours).toBeGreaterThanOrEqual(rows[19]!.hours); | |
outdated
Wall-clock bound is flaky on shared CI. Rely on the plan test, or use a loose bound (about 10x). |
||
| 74 | }); |
new above
offline
False: /boards reads this through idlePullsCached. Restore the original sentence and describe the new bound (open pulls, one index lookup each).