From 34222e01376d164530cd7f2e1bdaa04731b15db0 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:10:01 -0700 Subject: [PATCH] perf(orchestration): project explicit columns so the graph publish stops recompiling SQL (#18420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(orchestration): cache the prepared statements the graph publish recompiles SyncDatabase refuses to cache any `SELECT *` — node:sqlite can build the first row after a schema change from stale column names — so every wildcard read in the orchestration DB recompiles its SQL on each call. The graph publish runs that fan-out once per pane, ~0.7 times a second, forever. Add a per-connection prepared-statement cache scoped to the orchestration DB, whose schema is frozen in the constructor (createTables/migrate/trigger) and whose resets are DELETE-only, and route the buildByPaneKey -> getForHandle -> getRecent path through it. 5 publishes over 2 panes: 30 compilations -> 2. * perf(orchestration): project explicit columns so the existing cache covers the hot path Replaces the branch's second statement cache. The six graph-publish reads were uncacheable only because they were spelled `SELECT *` / `SELECT t.*`, which SyncDatabase refuses to cache (node:sqlite can build the first row after a schema change from stale column names). Spelling the projection out from type-checked column tuples makes them cacheable by the SyncDatabase LRU that is already merged, already bounded, and already clears on DDL — so the WeakMap and its documented cross-connection ALTER hazard both go away. Drift is caught at build time: `satisfies readonly (keyof Row)[]` plus an `Exclude extends never` assertion pins list vs type at tsc, and a PRAGMA table_info test against a freshly migrated OrchestrationDb pins list vs schema. Same win, verified: 6 compilations per publish -> 2 total then 0, identical to the WeakMap branch; 92/96/91 us CPU per 2-pane publish before, 11-12 us after on both. --- .../db/dispatch-context/dispatch-lookup.ts | 54 +++---- .../db/hot-path-statement-compilation.test.ts | 148 ++++++++++++++++++ .../orchestration/db/row-column-lists.test.ts | 57 +++++++ .../orchestration/db/row-column-lists.ts | 82 ++++++++++ .../orchestration/db/runs/run-lookup.ts | 19 +-- .../orchestration/db/tasks/task-store.ts | 42 ++--- 6 files changed, 346 insertions(+), 56 deletions(-) create mode 100644 src/main/runtime/orchestration/db/hot-path-statement-compilation.test.ts create mode 100644 src/main/runtime/orchestration/db/row-column-lists.test.ts create mode 100644 src/main/runtime/orchestration/db/row-column-lists.ts diff --git a/src/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.ts b/src/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.ts index ae1cd4f45d1..c96238f13ee 100644 --- a/src/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.ts +++ b/src/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.ts @@ -6,6 +6,23 @@ import { paneKeyMatchSuffix } from '../pane-key-match' import type { OrchestrationDb } from '../orchestration-db' +import { DISPATCH_CONTEXT_COLUMN_LIST } from '../row-column-lists' + +// Why: hoisted and wildcard-free so the graph-publish fan-out hits the SyncDatabase statement cache. +const ACTIVE_DISPATCH_BY_HANDLE_SQL = + // Why: newest-first like the pane lookups below — an unordered LIMIT 1 could pin a stale row if a handle ever has two active dispatches. + `SELECT ${DISPATCH_CONTEXT_COLUMN_LIST} FROM dispatch_contexts + WHERE assignee_handle = ? AND status IN ('pending', 'dispatched') + ORDER BY rowid DESC LIMIT 1` +const ACTIVE_DISPATCH_BY_PANE_KEY_SQL = `SELECT ${DISPATCH_CONTEXT_COLUMN_LIST} FROM dispatch_contexts + WHERE assignee_pane_key = ? AND status IN ('pending', 'dispatched') + ORDER BY rowid DESC LIMIT 1` +const ACTIVE_DISPATCH_BY_PANE_SUFFIX_SQL = `SELECT ${DISPATCH_CONTEXT_COLUMN_LIST} FROM dispatch_contexts + WHERE assignee_pane_key IS NOT NULL + AND status IN ('pending', 'dispatched') AND instr(assignee_pane_key, ':') > 1 + AND ${DISPATCH_PANE_KEY_MATCH_SUFFIX_SQL} = ? + ORDER BY rowid DESC LIMIT 1` +const LATEST_DISPATCH_BY_HANDLE_SQL = `SELECT ${DISPATCH_CONTEXT_COLUMN_LIST} FROM dispatch_contexts WHERE assignee_handle = ? ORDER BY rowid DESC LIMIT 1` export function getActiveDispatchForTerminal( this: OrchestrationDb, @@ -116,14 +133,9 @@ export function findActiveDispatchForAssignee( assigneeHandle: string, assigneePaneKey?: string ): DispatchContextRow | undefined { - const byHandle = this.db - .prepare( - // Why: newest-first like the pane lookups below — an unordered LIMIT 1 could pin a stale row if a handle ever has two active dispatches. - `SELECT * FROM dispatch_contexts - WHERE assignee_handle = ? AND status IN ('pending', 'dispatched') - ORDER BY rowid DESC LIMIT 1` - ) - .get(assigneeHandle) as DispatchContextRow | undefined + const byHandle = this.db.prepare(ACTIVE_DISPATCH_BY_HANDLE_SQL).get(assigneeHandle) as + | DispatchContextRow + | undefined if (byHandle) { return byHandle } @@ -132,13 +144,9 @@ export function findActiveDispatchForAssignee( return undefined } - const exactPane = this.db - .prepare( - `SELECT * FROM dispatch_contexts - WHERE assignee_pane_key = ? AND status IN ('pending', 'dispatched') - ORDER BY rowid DESC LIMIT 1` - ) - .get(assigneePaneKey) as DispatchContextRow | undefined + const exactPane = this.db.prepare(ACTIVE_DISPATCH_BY_PANE_KEY_SQL).get(assigneePaneKey) as + | DispatchContextRow + | undefined if (exactPane) { return exactPane } @@ -146,13 +154,7 @@ export function findActiveDispatchForAssignee( return undefined } return this.db - .prepare( - `SELECT * FROM dispatch_contexts - WHERE assignee_pane_key IS NOT NULL - AND status IN ('pending', 'dispatched') AND instr(assignee_pane_key, ':') > 1 - AND ${DISPATCH_PANE_KEY_MATCH_SUFFIX_SQL} = ? - ORDER BY rowid DESC LIMIT 1` - ) + .prepare(ACTIVE_DISPATCH_BY_PANE_SUFFIX_SQL) .get(paneKeyMatchSuffix(assigneePaneKey)) as DispatchContextRow | undefined } @@ -160,11 +162,9 @@ export function getLatestDispatchForTerminal( this: OrchestrationDb, handle: string ): DispatchContextRow | undefined { - return this.db - .prepare( - 'SELECT * FROM dispatch_contexts WHERE assignee_handle = ? ORDER BY rowid DESC LIMIT 1' - ) - .get(handle) as DispatchContextRow | undefined + return this.db.prepare(LATEST_DISPATCH_BY_HANDLE_SQL).get(handle) as + | DispatchContextRow + | undefined } export type DispatchLookupMethods = { diff --git a/src/main/runtime/orchestration/db/hot-path-statement-compilation.test.ts b/src/main/runtime/orchestration/db/hot-path-statement-compilation.test.ts new file mode 100644 index 00000000000..26f82410cfe --- /dev/null +++ b/src/main/runtime/orchestration/db/hot-path-statement-compilation.test.ts @@ -0,0 +1,148 @@ +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { RuntimeAgentOrchestrationProjection } from '../../runtime-agent-orchestration-projection' +import type { OrchestrationCompatibilityTerminalAuthority } from '../../runtime-terminal-contracts' +import type { RuntimeLeafRecord } from '../../runtime-terminal-state-records' +import { OrchestrationDb } from '../db' +import { createRootDispatch } from './root-dispatch-test-fixture' + +const COORDINATOR_HANDLE = 'term_coordinator' +const COORDINATOR_PANE = 'tab_c:leaf_c' +const WORKER_HANDLE = 'term_worker' +const WORKER_PANE = 'tab_w:leaf_w' +const IDLE_HANDLE = 'term_idle' +const IDLE_PANE = 'tab_i:leaf_i' + +// Why: mirrors SyncDatabase's `isStatementCacheable` — aggregate `(*)` is fine, any other `*` is not. +const WILDCARD_PROJECTION = /(? { + for (const db of openDatabases.splice(0)) { + try { + db.close() + } catch { + // already closed by the test + } + } + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }) + } +}) + +function openDatabase(path: string): OrchestrationDb { + const db = new OrchestrationDb(path) + openDatabases.push(db) + return db +} + +function temporaryDatabasePath(): string { + const directory = mkdtempSync(join(tmpdir(), 'orca-orchestration-hot-path-')) + temporaryDirectories.push(directory) + return join(directory, 'orchestration.db') +} + +/** Counts real SQL compilations by wrapping the node:sqlite handle SyncDatabase prepares against. */ +function trackCompiledSql(db: OrchestrationDb): string[] { + const inner = (db.db as unknown as { db: { prepare(sql: string): unknown } }).db + const original = inner.prepare.bind(inner) + const compiled: string[] = [] + inner.prepare = (sql: string) => { + compiled.push(sql) + return original(sql) + } + return compiled +} + +function seedDispatchedWorker(db: OrchestrationDb): void { + const run = db.createRun({ + objective: 'demo', + coordinatorHandle: COORDINATOR_HANDLE, + coordinatorPaneKey: COORDINATOR_PANE + }) + const task = db.createTask({ + spec: 'ship the thing', + runId: run.id, + createdByTerminalHandle: COORDINATOR_HANDLE, + createdByPaneKey: COORDINATOR_PANE, + createdByProcessIncarnation: 'inc_1', + createdByRunGeneration: run.consumer_generation + }) + createRootDispatch(db, task.id, WORKER_HANDLE, WORKER_PANE) +} + +function buildProjection(db: OrchestrationDb): RuntimeAgentOrchestrationProjection { + const leaves = [{ ptyId: 'pty_w' }, { ptyId: 'pty_i' }] as unknown as RuntimeLeafRecord[] + const handleByLeaf = new Map([ + [leaves[0] as RuntimeLeafRecord, WORKER_HANDLE], + [leaves[1] as RuntimeLeafRecord, IDLE_HANDLE] + ]) + const paneByLeaf = new Map([ + [leaves[0] as RuntimeLeafRecord, WORKER_PANE], + [leaves[1] as RuntimeLeafRecord, IDLE_PANE] + ]) + return new RuntimeAgentOrchestrationProjection({ + getDb: () => db, + getLeaves: () => leaves, + getPtys: () => [], + issueLeafHandle: (leaf) => handleByLeaf.get(leaf) ?? '', + issuePtyHandle: () => '', + makePaneKey: (leaf) => paneByLeaf.get(leaf) ?? '', + getWorktreeId: () => null, + getHandleForPaneKey: (paneKey) => (paneKey === COORDINATOR_PANE ? COORDINATOR_HANDLE : null), + getPaneKey: (handle) => (handle === COORDINATOR_HANDLE ? COORDINATOR_PANE : null), + getDispatchAuthority: (handle) => + handle === COORDINATOR_HANDLE + ? ({ + paneKey: COORDINATOR_PANE, + processIncarnation: 'inc_1' + } as OrchestrationCompatibilityTerminalAuthority) + : null + }) +} + +describe('orchestration hot-path statement compilation', () => { + it('compiles each hot-path SQL exactly once across repeated graph publishes', () => { + const db = openDatabase(':memory:') + seedDispatchedWorker(db) + const projection = buildProjection(db) + const compiled = trackCompiledSql(db) + + const publishes = [projection.buildByPaneKey()] + const compiledByFirstPublish = [...compiled] + for (let publish = 0; publish < 4; publish += 1) { + publishes.push(projection.buildByPaneKey()) + } + + const compilationsPerSql = new Map() + for (const sql of compiled) { + compilationsPerSql.set(sql, (compilationsPerSql.get(sql) ?? 0) + 1) + } + expect([...compilationsPerSql].filter(([, count]) => count > 1)).toEqual([]) + expect(compiled).toEqual(compiledByFirstPublish) + // Why: a cache that changed what the fan-out returns would be worse than the recompiles. + expect(publishes[0]).toBeDefined() + for (const publish of publishes) { + expect(publish).toEqual(publishes[0]) + } + }) + + // Why: `SELECT *` is what made these statements uncacheable, and a retained wildcard is the only + // way node:sqlite could build a row from stale column names after another connection's ALTER. + // Seeds on one connection and publishes on a second so every compilation here is hot-path SQL. + it('publishes without compiling a single wildcard projection', () => { + const path = temporaryDatabasePath() + seedDispatchedWorker(openDatabase(path)) + + const reader = openDatabase(path) + const compiled = trackCompiledSql(reader) + buildProjection(reader).buildByPaneKey() + + expect(compiled.length).toBeGreaterThan(0) + expect(compiled.filter((sql) => WILDCARD_PROJECTION.test(sql))).toEqual([]) + }) +}) diff --git a/src/main/runtime/orchestration/db/row-column-lists.test.ts b/src/main/runtime/orchestration/db/row-column-lists.test.ts new file mode 100644 index 00000000000..2c4041bebaa --- /dev/null +++ b/src/main/runtime/orchestration/db/row-column-lists.test.ts @@ -0,0 +1,57 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { OrchestrationDb } from './orchestration-db' +import { + DISPATCH_CONTEXT_COLUMNS, + RUN_COLUMNS, + selectColumns, + TASK_COLUMNS +} from './row-column-lists' + +let db: OrchestrationDb | undefined + +afterEach(() => { + db?.close() + db = undefined +}) + +function tableColumns(table: string): string[] { + const rows = (db as OrchestrationDb).db.pragma(`table_info(${table})`) as { name: string }[] + return rows.map((row) => row.name).sort() +} + +describe('row column lists', () => { + // Why: these lists replaced `SELECT *`, so a column added to the schema without being listed here + // would silently stop being read. tsc pins list↔type; this pins list↔schema. + it.each([ + ['runs', RUN_COLUMNS], + ['tasks', TASK_COLUMNS], + ['dispatch_contexts', DISPATCH_CONTEXT_COLUMNS] + ])('projects every %s column the migrated schema declares', (table, columns) => { + db = new OrchestrationDb(':memory:') + + expect([...columns].sort()).toEqual(tableColumns(table)) + }) + + it('qualifies each column when the statement joins under an alias', () => { + expect(selectColumns(['id', 'run_id'])).toBe('id, run_id') + expect(selectColumns(['id', 'run_id'], 't')).toBe('t.id, t.run_id') + }) + + // Why: an alias-qualified projection must key the returned row by the bare column name, exactly as + // the `t.*` it replaced did — otherwise every lineage consumer reads undefined. + it('returns bare column names for an alias-qualified projection', () => { + db = new OrchestrationDb(':memory:') + const run = db.createRun({ + objective: 'demo', + coordinatorHandle: 'term_c', + coordinatorPaneKey: 'tab_c:leaf_c' + }) + const task = db.createTask({ spec: 'work', runId: run.id }) + + const row = db.db + .prepare(`SELECT ${selectColumns(TASK_COLUMNS, 't')} FROM tasks t WHERE t.id = ?`) + .get(task.id) as Record + + expect(Object.keys(row).sort()).toEqual([...TASK_COLUMNS].sort()) + }) +}) diff --git a/src/main/runtime/orchestration/db/row-column-lists.ts b/src/main/runtime/orchestration/db/row-column-lists.ts new file mode 100644 index 00000000000..26255fe551a --- /dev/null +++ b/src/main/runtime/orchestration/db/row-column-lists.ts @@ -0,0 +1,82 @@ +import type { DispatchContextRow, RunRow, TaskRow } from '../types' + +// Why: `SyncDatabase` refuses to cache any `SELECT *` (node:sqlite can build the first row after a +// schema change from stale column names), so a wildcard read recompiles its SQL on every call. +// Spelling the projection out makes the hot-path statements cacheable by that existing LRU. +// Drift is caught twice: `satisfies` + the exhaustiveness assertions below pin list↔type at tsc, +// and `row-column-lists.test.ts` pins list↔schema against a freshly migrated database. + +export const RUN_COLUMNS = [ + 'id', + 'objective', + 'home_database', + 'coordinator_handle', + 'coordinator_pane_key', + 'consumer_generation', + 'legacy', + 'created_at', + 'updated_at' +] as const satisfies readonly (keyof RunRow)[] + +export const TASK_COLUMNS = [ + 'id', + 'run_id', + 'parent_id', + 'created_by_terminal_handle', + 'created_by_pane_key', + 'created_by_process_incarnation', + 'created_by_run_generation', + 'task_title', + 'display_name', + 'spec', + 'status', + 'deps', + 'result', + 'created_at', + 'completed_at' +] as const satisfies readonly (keyof TaskRow)[] + +export const DISPATCH_CONTEXT_COLUMNS = [ + 'id', + 'run_id', + 'task_id', + 'contract_version', + 'launch_token_hash', + 'assignee_handle', + 'assignee_pane_key', + 'capability_hash', + 'process_incarnation', + 'capability_revoked_at', + 'status', + 'failure_count', + 'last_failure', + 'termination_reason', + 'depth', + 'dispatched_at', + 'completed_at', + 'created_at', + 'last_heartbeat_at' +] as const satisfies readonly (keyof DispatchContextRow)[] + +// Compile check: a row field added without its column here would silently vanish from the +// projection that used to be `SELECT *`, so the missing key must fail the build. +type UnprojectedRunColumn = Exclude +type UnprojectedTaskColumn = Exclude +type UnprojectedDispatchContextColumn = Exclude< + keyof DispatchContextRow, + (typeof DISPATCH_CONTEXT_COLUMNS)[number] +> +const assertEveryRowColumnProjected: [ + UnprojectedRunColumn extends never ? true : never, + UnprojectedTaskColumn extends never ? true : never, + UnprojectedDispatchContextColumn extends never ? true : never +] = [true, true, true] +void assertEveryRowColumnProjected + +/** Projection list for a `SELECT`; `alias` qualifies each name for a joined table (`t.id, …`). */ +export function selectColumns(columns: readonly string[], alias?: string): string { + return columns.map((column) => (alias ? `${alias}.${column}` : column)).join(', ') +} + +export const RUN_COLUMN_LIST = selectColumns(RUN_COLUMNS) +export const DISPATCH_CONTEXT_COLUMN_LIST = selectColumns(DISPATCH_CONTEXT_COLUMNS) diff --git a/src/main/runtime/orchestration/db/runs/run-lookup.ts b/src/main/runtime/orchestration/db/runs/run-lookup.ts index 061a7b39497..84eeece7374 100644 --- a/src/main/runtime/orchestration/db/runs/run-lookup.ts +++ b/src/main/runtime/orchestration/db/runs/run-lookup.ts @@ -9,12 +9,20 @@ import { exposeRunTimestamps } from '../utc-timestamp' import { encodeRunListCursor, decodeRunListCursor } from '../run-list-cursor' import type { RunListPage } from '../run-list-page' import type { OrchestrationDb } from '../orchestration-db' +import { RUN_COLUMN_LIST } from '../row-column-lists' export type LegacyAdoptedMailboxOwner = { runId: string terminalHandle: string } +// Why: hoisted and wildcard-free so the per-publish run lookups hit the SyncDatabase statement cache. +const RUN_BY_ID_SQL = `SELECT ${RUN_COLUMN_LIST} FROM runs WHERE id = ?` +const RUNS_BOUND_TO_PANE_SQL = `SELECT ${RUN_COLUMN_LIST} FROM runs + WHERE coordinator_pane_key IS NOT NULL AND legacy = 0 + AND ${RUN_PANE_KEY_MATCH_SUFFIX_SQL} = ? + ORDER BY rowid` + export function getRun(this: OrchestrationDb, id: string): RunRow | undefined { const run = this.getRunRaw(id) return run ? exposeRunTimestamps(run) : undefined @@ -103,14 +111,7 @@ export function getCurrentRunForPane(this: OrchestrationDb, paneKey: string): Ru // reminted tab halves keep matching and unparseable keys keep requiring an exact match. export function runsBoundToPane(this: OrchestrationDb, paneKey: string): RunRow[] { return ( - this.db - .prepare( - `SELECT * FROM runs - WHERE coordinator_pane_key IS NOT NULL AND legacy = 0 - AND ${RUN_PANE_KEY_MATCH_SUFFIX_SQL} = ? - ORDER BY rowid` - ) - .all(paneKeyMatchSuffix(paneKey)) as RunRow[] + this.db.prepare(RUNS_BOUND_TO_PANE_SQL).all(paneKeyMatchSuffix(paneKey)) as RunRow[] ).filter( (run) => run.coordinator_pane_key !== null && isEquivalentPaneKey(run.coordinator_pane_key, paneKey) @@ -118,7 +119,7 @@ export function runsBoundToPane(this: OrchestrationDb, paneKey: string): RunRow[ } export function getRunRaw(this: OrchestrationDb, id: string): RunRow | undefined { - return this.db.prepare('SELECT * FROM runs WHERE id = ?').get(id) as RunRow | undefined + return this.db.prepare(RUN_BY_ID_SQL).get(id) as RunRow | undefined } export function unbindOtherRunsForPane( diff --git a/src/main/runtime/orchestration/db/tasks/task-store.ts b/src/main/runtime/orchestration/db/tasks/task-store.ts index 9a0b15259ba..69316765a74 100644 --- a/src/main/runtime/orchestration/db/tasks/task-store.ts +++ b/src/main/runtime/orchestration/db/tasks/task-store.ts @@ -5,6 +5,7 @@ import { LEGACY_RUN_ID } from '../contract-constants' import { generateId } from '../generated-id' import type { TaskRuntimeLineageRow } from '../run-list-page' import type { OrchestrationDb } from '../orchestration-db' +import { selectColumns, TASK_COLUMNS } from '../row-column-lists' // ── Tasks ── @@ -81,24 +82,8 @@ export function createTask( return this.db.prepare('SELECT * FROM tasks WHERE id = ?').get(id) as TaskRow } -// Why: return the active creator Dispatch proof with the Task read; runtime still owns pane/process currency. -export function getTask(this: OrchestrationDb, id: string): TaskRow | undefined -export function getTask( - this: OrchestrationDb, - id: string, - dispatchRunId: string -): TaskRuntimeLineageRow | undefined -export function getTask( - this: OrchestrationDb, - id: string, - dispatchRunId?: string -): TaskRow | TaskRuntimeLineageRow | undefined { - if (dispatchRunId === undefined) { - return this.db.prepare('SELECT * FROM tasks WHERE id = ?').get(id) as TaskRow | undefined - } - return this.db - .prepare( - `SELECT t.*, +// Why: hoisted and wildcard-free so the per-publish lineage lookup hits the SyncDatabase statement cache. +const TASK_RUNTIME_LINEAGE_SQL = `SELECT ${selectColumns(TASK_COLUMNS, 't')}, creator.id AS creator_dispatch_id, creator.run_id AS creator_dispatch_run_id, creator.assignee_pane_key AS creator_dispatch_pane_key, @@ -114,8 +99,25 @@ export function getTask( LIMIT 1 ) WHERE t.id = ?` - ) - .get(dispatchRunId, id) as TaskRuntimeLineageRow | undefined + +// Why: return the active creator Dispatch proof with the Task read; runtime still owns pane/process currency. +export function getTask(this: OrchestrationDb, id: string): TaskRow | undefined +export function getTask( + this: OrchestrationDb, + id: string, + dispatchRunId: string +): TaskRuntimeLineageRow | undefined +export function getTask( + this: OrchestrationDb, + id: string, + dispatchRunId?: string +): TaskRow | TaskRuntimeLineageRow | undefined { + if (dispatchRunId === undefined) { + return this.db.prepare('SELECT * FROM tasks WHERE id = ?').get(id) as TaskRow | undefined + } + return this.db.prepare(TASK_RUNTIME_LINEAGE_SQL).get(dispatchRunId, id) as + | TaskRuntimeLineageRow + | undefined } export function listTasks(