diff --git a/doc/performance/2026-11-04-prepared-github-review-wait.md b/doc/performance/2026-11-04-prepared-github-review-wait.md new file mode 100644 index 00000000000..bae6eed20db --- /dev/null +++ b/doc/performance/2026-11-04-prepared-github-review-wait.md @@ -0,0 +1,109 @@ +# Prepared GitHub review-wait read + +Date: 2026-11-04. + +`readInheritedGitHubReviewWait` builds a fixed seven-table selector on every +read. A sampled production window attributed 6.3% of SQL serialization CPU to +the matching shape (29 samples, mean 204 microseconds). This is a share of +serialization CPU, not total server CPU. + +## Change and preserved contract + +The GitHub owner prepares the existing Drizzle selector once per exact supplied +`Db` object. A `WeakMap` retains only the compiled query and decoders. Company, +issue, agent and source-comment IDs are placeholders filled on every execution. +Database transactions and nested savepoints have different object identities +and therefore own separate prepared queries and sessions. No database rows, +results, promises or authorization decisions are cached. + +Provenance still runs before the selector, and publication still runs after it. +The SQL, field order, JSON `->'reviewedCommit'` projection, latest-review ordering, +company boundaries, connection/repository/agent checks and publication lookup +are unchanged. This does not change postgres.js driver preparation settings. + +## Reproduction + +Install the locked dependencies, then run with the repository's supported Node: + +```sh +node --import ./server/node_modules/tsx/dist/loader.mjs \ + server/scripts/benchmark-prepared-github-review-wait.ts \ + --baseline-ref=e54f9fbf828e06e385e0c62dc381ccb46190aa30 +``` + +The script imports the complete baseline service from that explicit commit, +verifies the unchanged provenance dependency, seeds disposable real PostgreSQL, +and compares the complete result and every generated query, parameter and query +count. It records main-selector compilation counts separately from timed runs. +The timed path calls the actual owning service, including provenance and +publication reads, with no replay driver or substituted authorization result. + +## Accepted measurement + +Baseline: `e54f9fbf828e06e385e0c62dc381ccb46190aa30`. +Treatment source SHA256: `fe647e7fe2aed82609e8f1d66a0109e7a10ee28313ae2ef218ba3ab39b5cd105`. +Runtime: v24.22.2, darwin/arm64. +Database: PostgreSQL 18.1 on x86_64-apple-darwin23.6.0. +Fixture: one synthetic completed review and its full live binding/publication. + +The final measurement used an exclusive build/benchmark window, 500 warmup +reads per version and nine alternating pairs of 100 sequential reads each. +CPU measures the Node process; it excludes PostgreSQL process CPU. It includes +SQL construction, driver work, row decoding and the rest of the service. + +| Median per complete service read | Baseline | Treatment | Reduction | +| --- | ---: | ---: | ---: | +| Process CPU | 1.00336 ms | 0.75908 ms | 24.35% | +| Wall time | 2.87089 ms | 2.38731 ms | 16.84% | + +Main-selector compilations per ten warm service reads: **10 → 0**. All service +results, generated SQL, bound parameters and query counts matched. Connection +revocation was observed by both versions after the cache was warm. + +Each sample below totals 100 service reads; odd pairs run baseline first and even +pairs run treatment first. No samples were discarded. + +| Pair | Baseline CPU ms | Treatment CPU ms | Baseline wall ms | Treatment wall ms | +| --- | ---: | ---: | ---: | ---: | +| 1 | 109.301 | 82.314 | 301.860 | 326.252 | +| 2 | 110.681 | 98.892 | 310.177 | 248.191 | +| 3 | 102.627 | 94.743 | 314.427 | 317.478 | +| 4 | 98.198 | 78.979 | 287.089 | 323.037 | +| 5 | 100.336 | 75.908 | 288.454 | 232.344 | +| 6 | 91.043 | 63.248 | 247.297 | 238.731 | +| 7 | 72.471 | 59.875 | 223.578 | 206.735 | +| 8 | 93.542 | 54.337 | 252.195 | 219.263 | +| 9 | 119.050 | 65.356 | 265.733 | 220.582 | + +The smallest and largest CPU samples per read were +0.72471–1.19050 ms for baseline and +0.54337–0.98892 ms for treatment. +These are local synthetic service measurements. They do not predict a +production request latency percentage or measure database execution savings. + +## Correctness and setup record + +The focused real-PostgreSQL suite covers malformed JSON and strict type equality, +current connection/publication/provenance changes, company and agent boundaries, +repository/thread checks, a newer queued head after a warm completed review, +changing company/issue/agent/comment bindings on a shared handle, and independent +transaction/savepoint visibility through rollback. + +An initial sandbox run could not bind loopback. The next run exposed missing +macOS library aliases in the freshly unpacked embedded-Postgres dependency. +The fixture ran after restoring aliases to existing identical library bytes in +`node_modules`; no application source or package lock changed. The first short +warmup benchmark and the next long warmup benchmark overlapped another fixture +and are diagnostic only. The accepted numbers above come from the final +exclusive-window run. Setup failures are not counted as passing tests. + +The first shared-consumer integration import and direct server typecheck lacked +built workspace SDK/Runner outputs. Building `@paperclipai/plugin-sdk` (including +its shared-package prerequisite) resolved the SDK import. An overly broad run of +the full external-chat integration file was interrupted to run only its affected +consumer test. That interrupted run is not a passing suite. Full repository +validation is tracked separately from the focused tests and benchmark. + +Focused verification passed: `github-review-wait.test.ts` (16 tests), plus +`external-chat-wait.integration.test.ts -t 'reads small current external review waits without saved prompts'` +(four affected consumer cases; 269 unrelated cases skipped). diff --git a/server/scripts/benchmark-prepared-github-review-wait.ts b/server/scripts/benchmark-prepared-github-review-wait.ts new file mode 100644 index 00000000000..d4bd1d7e594 --- /dev/null +++ b/server/scripts/benchmark-prepared-github-review-wait.ts @@ -0,0 +1,114 @@ +import assert from "node:assert/strict"; +import { SQL } from "drizzle-orm"; +import { execFileSync } from "node:child_process"; +import { promises as fs } from "node:fs"; +import path from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; +import { applyPendingMigrations, createDb, startEmbeddedPostgresTestDatabase } from "../../packages/db/src/index.js"; +import { readInheritedGitHubReviewWait } from "../../server/src/services/chat-channels/github/review-wait.js"; +import { captureReviewWaitQueries, seedGitHubReviewWait, + revokeReviewWaitConnection, type CapturedReviewWaitQuery } from "../../server/src/__tests__/helpers/github-review-wait.js"; + +const baselineRef = process.argv.find(arg => arg.startsWith("--baseline-ref="))?.slice(15); +assert.ok(baselineRef, "Pass an explicit pre-fix --baseline-ref="); +const baselineRevision = execFileSync("git", ["rev-parse", "--verify", `${baselineRef}^{commit}`], { encoding: "utf8" }).trim(); +const treatmentRevision = execFileSync("git", ["rev-parse", "HEAD"], { encoding: "utf8" }).trim(); +const modulePath = "server/src/services/chat-channels/github/review-wait.ts"; +const helperPath = "server/src/services/chat-channels/run-origin.ts"; +const root = fileURLToPath(new URL("../../", import.meta.url)); +assert.equal(execFileSync("git", ["show", `${baselineRevision}:${helperPath}`], { encoding: "utf8" }), + await fs.readFile(path.join(root, helperPath), "utf8"), "Baseline provenance helper must be unchanged"); +const keepAlive = setInterval(() => {}, 1_000); +let temporary: Awaited> | undefined; +let db: ReturnType | undefined; +let baselineDir: string | undefined; +try { + const oldSource = execFileSync("git", ["show", `${baselineRevision}:${modulePath}`], { encoding: "utf8" }); + const moduleUrl = new URL(`../../${modulePath}`, import.meta.url); + baselineDir = await fs.mkdtemp(path.join(root, "server/scripts/.review-wait-baseline-")); + const baselineFile = path.join(baselineDir, "review-wait.ts"); + await fs.writeFile(baselineFile, oldSource.replace(/from "(\.{1,2}\/[^\"]+)"/g, + (_match, specifier: string) => `from ${JSON.stringify(fileURLToPath(new URL(specifier, moduleUrl)))}`)); + const baseline = await import(pathToFileURL(baselineFile).href) as typeof import("../../server/src/services/chat-channels/github/review-wait.js"); + const externalUrl = process.env.PAPERCLIP_TEST_DATABASE_URL; + if (externalUrl) { + const parsed = new URL(externalUrl); + assert.ok(["127.0.0.1", "localhost"].includes(parsed.hostname) && parsed.pathname.startsWith("/review_wait_benchmark"), + "External benchmark database must be a disposable local review_wait_benchmark database"); + await applyPendingMigrations(externalUrl); + db = createDb(externalUrl); + } else { + temporary = await startEmbeddedPostgresTestDatabase("github-review-wait-benchmark-"); + db = createDb(temporary.connectionString); + } + const f = await seedGitHubReviewWait(db); + async function capture(reader: typeof readInheritedGitHubReviewWait) { + const queries: CapturedReviewWaitQuery[] = []; + const result = await reader(captureReviewWaitQueries(db!, queries), f.run); + return { result, queries }; + } + const before = await capture(baseline.readInheritedGitHubReviewWait); + const after = await capture(readInheritedGitHubReviewWait); + assert.deepEqual(after, before, "Complete service output, SQL, bound parameters and query count must match"); + assert.equal(before.result?.kind, "published"); + + async function compilations(reader: typeof readInheritedGitHubReviewWait) { + const original = SQL.prototype.toQuery; + let joined = 0; + SQL.prototype.toQuery = function (...args: Parameters) { + const query = original.apply(this, args); + if (query.sql.includes('join "chat_github_reviews"')) joined++; + return query; + }; + try { + for (let i = 0; i < 10; i++) await reader(db!, f.run); + } finally { SQL.prototype.toQuery = original; } + return joined; + } + const buildCounts = { baseline: await compilations(baseline.readInheritedGitHubReviewWait), + treatment: await compilations(readInheritedGitHubReviewWait) }; + buildCounts.treatment = await compilations(readInheritedGitHubReviewWait); + assert.equal(buildCounts.baseline, 10); + assert.equal(buildCounts.treatment, 0); + + await revokeReviewWaitConnection(db, f); + assert.deepEqual(await readInheritedGitHubReviewWait(db, f.run), { kind: "revoked" }); + assert.deepEqual(await baseline.readInheritedGitHubReviewWait(db, f.run), { kind: "revoked" }); + await db.$client.unsafe("UPDATE tool_connections SET enabled = true WHERE id = $1::uuid", [f.connectionId]); + + const iterations = Number(process.env.REVIEW_WAIT_BENCH_ITERATIONS ?? 100); + assert.ok(Number.isInteger(iterations) && iterations > 0); + const readers = { baseline: baseline.readInheritedGitHubReviewWait, treatment: readInheritedGitHubReviewWait }; + const samples = { baseline: [] as { cpuMs: number; wallMs: number }[], treatment: [] as { cpuMs: number; wallMs: number }[] }; + for (const reader of Object.values(readers)) for (let i = 0; i < 500; i++) await reader(db, f.run); + for (let pair = 0; pair < 9; pair++) { + for (const version of pair % 2 ? ["treatment", "baseline"] as const : ["baseline", "treatment"] as const) { + const cpu = process.cpuUsage(), wall = performance.now(); + for (let i = 0; i < iterations; i++) await readers[version](db, f.run); + const elapsed = process.cpuUsage(cpu); + samples[version].push({ cpuMs: (elapsed.user + elapsed.system) / 1_000, wallMs: performance.now() - wall }); + } + } + const stats = (values: typeof samples.baseline) => { + const cpu = values.map(value => value.cpuMs / iterations).sort((a, b) => a - b); + const wall = values.map(value => value.wallMs / iterations).sort((a, b) => a - b); + return { samples: values, medianCpuMsPerRead: cpu[Math.floor(cpu.length / 2)], + minCpuMsPerRead: cpu[0], maxCpuMsPerRead: cpu.at(-1), medianWallMsPerRead: wall[Math.floor(wall.length / 2)] }; + }; + console.log(JSON.stringify({ baselineRevision, treatmentRevision, + treatmentDiff: execFileSync("git", ["diff", "HEAD", "--", modulePath], { cwd: root, encoding: "utf8" }), + runtime: process.version, platform: process.platform, arch: process.arch, + postgresVersion: (await db.$client.unsafe("SELECT version()"))[0]!.version, + fixture: { reviews: 1, synthetic: true }, correctness: { completeOutputEqual: true, + everyGeneratedQueryAndParameterEqual: true, currentConnectionRevocationEqual: true }, + generatedQueries: before.queries, compilationCountsPerTenWarmReads: buildCounts, + measurement: "Complete actual service, sequential real PostgreSQL reads; process CPU includes client work, excludes database process CPU", + timings: { iterations, warmups: 500, pairs: 9, order: "alternating", baseline: stats(samples.baseline), treatment: stats(samples.treatment) }, + }, null, 2)); +} finally { + try { + await db?.$client.end({ timeout: 0 }); + await temporary?.cleanup(); + if (baselineDir) await fs.rm(baselineDir, { recursive: true, force: true }); + } finally { clearInterval(keepAlive); } +} diff --git a/server/src/__tests__/github-review-wait.test.ts b/server/src/__tests__/github-review-wait.test.ts index d4b04a6d2b1..cad6571fe42 100644 --- a/server/src/__tests__/github-review-wait.test.ts +++ b/server/src/__tests__/github-review-wait.test.ts @@ -80,8 +80,41 @@ describe("inherited GitHub review wait projection", () => { }, ); + it("binds each call afresh while reusing the compiled query", async () => { + const first = await seedGitHubReviewWait(db), second = await seedGitHubReviewWait(db); + const queries: CapturedReviewWaitQuery[] = []; + const captured = captureReviewWaitQueries(db, queries); + for (const f of [first, second, first]) { + expect(await readInheritedGitHubReviewWait(captured, f.run)).toMatchObject({ reviewId: f.reviewId }); + } + const joined = queries.filter(query => query.sql.includes('join "chat_github_reviews"')); + expect(joined).toHaveLength(3); + expect(joined[0]).toEqual(joined[2]); + expect(joined[0]!.sql).toBe(joined[1]!.sql); + expect(joined[0]!.params).not.toEqual(joined[1]!.params); + }); + + it("keeps transaction and savepoint reads on their own sessions", async () => { + const f = await seedGitHubReviewWait(db); + expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published"); + await expect(db.transaction(async tx => { + const scoped = tx as unknown as Db; + await tx.update(toolConnections).set({ enabled: false }).where(eq(toolConnections.id, f.connectionId)); + expect(await readInheritedGitHubReviewWait(scoped, f.run)).toEqual({ kind: "revoked" }); + await expect(tx.transaction(async savepoint => { + await savepoint.update(toolConnections).set({ enabled: true }).where(eq(toolConnections.id, f.connectionId)); + expect((await readInheritedGitHubReviewWait(savepoint as unknown as Db, f.run))?.kind).toBe("published"); + throw new Error("rollback savepoint"); + })).rejects.toThrow("rollback savepoint"); + expect(await readInheritedGitHubReviewWait(scoped, f.run)).toEqual({ kind: "revoked" }); + throw new Error("rollback transaction"); + })).rejects.toThrow("rollback transaction"); + expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published"); + }); + it("does not reuse an older completed review when a newer head is queued", async () => { const f = await seedGitHubReviewWait(db); + expect((await readInheritedGitHubReviewWait(db, f.run))?.kind).toBe("published"); const [review] = await db.select().from(chatGitHubReviews).where(eq(chatGitHubReviews.id, f.reviewId)); await db.insert(chatGitHubReviews).values({ ...review!, id: randomUUID(), deliveryId: randomUUID(), headSha: "2".repeat(40), state: "queued", createdAt: new Date(review!.createdAt.getTime() + 1_000) }); diff --git a/server/src/__tests__/helpers/github-review-wait.ts b/server/src/__tests__/helpers/github-review-wait.ts index cab33031a96..eb3bea3fcef 100644 --- a/server/src/__tests__/helpers/github-review-wait.ts +++ b/server/src/__tests__/helpers/github-review-wait.ts @@ -1,5 +1,5 @@ import { createHash, randomUUID } from "node:crypto"; -import { and, eq } from "drizzle-orm"; +import { and, eq, fillPlaceholders } from "drizzle-orm"; import { agents, chatActions, chatConversations, chatEndpointResources, chatEndpoints, chatGitHubReviews, chatMessageLinks, companies, heartbeatRuns, issueComments, issues, runIdentityContexts, toolApplications, toolConnections, type Db } from "@paperclipai/db"; @@ -61,6 +61,16 @@ export function captureReviewWaitQueries(db: Db, queries: CapturedReviewWaitQuer const builder = target.select(...fields), from = builder.from.bind(builder); builder.from = ((...args: Parameters) => { const selection = from(...args), execute = selection.execute.bind(selection); + const prepare = selection.prepare.bind(selection); + selection.prepare = ((...args: Parameters) => { + const prepared = prepare(...args), executePrepared = prepared.execute.bind(prepared); + prepared.execute = async (values = {}) => { + const query = prepared.getQuery(); + queries.push({ sql: query.sql, params: fillPlaceholders(query.params, values) }); + return executePrepared(values); + }; + return prepared; + }) as typeof selection.prepare; selection.execute = async () => { queries.push(selection.toSQL()); return execute(); }; return selection; }) as typeof builder.from; diff --git a/server/src/services/chat-channels/github/review-wait.ts b/server/src/services/chat-channels/github/review-wait.ts index 49205369189..c3481257780 100644 --- a/server/src/services/chat-channels/github/review-wait.ts +++ b/server/src/services/chat-channels/github/review-wait.ts @@ -33,6 +33,47 @@ export async function hasPublishedGitHubReview(db: Db, binding: { type WaitRun = Pick; +const inheritedReviewQueries = new WeakMap>(); + +function prepareInheritedReviewQuery(db: Db) { + return db.select({ + id: chatGitHubReviews.id, endpointId: chatGitHubReviews.endpointId, + state: chatGitHubReviews.state, repository: chatGitHubReviews.repository, + repositoryId: chatGitHubReviews.repositoryId, headSha: chatGitHubReviews.headSha, + reviewedCommit: sql`${chatGitHubReviews.assessment}->'reviewedCommit'`, reviewRunId: chatGitHubReviews.runId, + connectionEnabled: toolConnections.enabled, connectionStatus: toolConnections.status, + reviewAgentId: heartbeatRuns.agentId, reviewCompanyId: heartbeatRuns.companyId, + resourceRepository: chatEndpointResources.providerResourceId, + resourceRepositoryId: sql`${chatEndpointResources.metadata}->>'providerRepositoryId'`, + pullNumber: chatGitHubReviews.pullNumber, thread: chatConversations.externalThreadId, + }).from(chatMessageLinks) + .innerJoin(chatConversations, and(eq(chatConversations.id, chatMessageLinks.conversationId), + eq(chatConversations.companyId, sql.placeholder("companyId")), eq(chatConversations.issueId, sql.placeholder("issueId")))) + .innerJoin(chatEndpoints, and(eq(chatEndpoints.id, chatConversations.endpointId), + eq(chatEndpoints.companyId, sql.placeholder("companyId")), eq(chatEndpoints.provider, "github"), + eq(chatEndpoints.assignedAgentId, sql.placeholder("agentId")))) + .leftJoin(toolConnections, and(eq(toolConnections.id, chatEndpoints.connectionId), + eq(toolConnections.companyId, sql.placeholder("companyId")))) + .innerJoin(chatEndpointResources, and(eq(chatEndpointResources.id, chatConversations.resourceId), + eq(chatEndpointResources.companyId, sql.placeholder("companyId")), eq(chatEndpointResources.endpointId, chatEndpoints.id))) + .innerJoin(chatGitHubReviews, and(eq(chatGitHubReviews.endpointId, chatEndpoints.id), + eq(chatGitHubReviews.companyId, sql.placeholder("companyId")), eq(chatGitHubReviews.issueId, sql.placeholder("issueId")))) + .leftJoin(heartbeatRuns, eq(heartbeatRuns.id, chatGitHubReviews.runId)) + .where(and(eq(chatMessageLinks.companyId, sql.placeholder("companyId")), eq(chatMessageLinks.commentId, sql.placeholder("commentId")), + eq(chatMessageLinks.direction, "inbound"))) + .orderBy(desc(chatGitHubReviews.createdAt), desc(chatGitHubReviews.id)).limit(1) + .prepare("inherited_github_review_wait"); +} + +function inheritedReviewQuery(db: Db) { + let query = inheritedReviewQueries.get(db); + if (!query) { + query = prepareInheritedReviewQuery(db); + inheritedReviewQueries.set(db, query); + } + return query; +} + /** Passive review waits inherit the same accepted requester lineage as GitHub * tools. This is not a grant to publish, run a monitor, or reuse attachments. */ export async function readInheritedGitHubReviewWait(db: Db, run: WaitRun) { @@ -45,32 +86,9 @@ export async function readInheritedGitHubReviewWait(db: Db, run: WaitRun) { throw error; } if (!commentId) return null; - const [review] = await db.select({ - id: chatGitHubReviews.id, endpointId: chatGitHubReviews.endpointId, - state: chatGitHubReviews.state, repository: chatGitHubReviews.repository, - repositoryId: chatGitHubReviews.repositoryId, headSha: chatGitHubReviews.headSha, - reviewedCommit: sql`${chatGitHubReviews.assessment}->'reviewedCommit'`, reviewRunId: chatGitHubReviews.runId, - connectionEnabled: toolConnections.enabled, connectionStatus: toolConnections.status, - reviewAgentId: heartbeatRuns.agentId, reviewCompanyId: heartbeatRuns.companyId, - resourceRepository: chatEndpointResources.providerResourceId, - resourceRepositoryId: sql`${chatEndpointResources.metadata}->>'providerRepositoryId'`, - pullNumber: chatGitHubReviews.pullNumber, thread: chatConversations.externalThreadId, - }).from(chatMessageLinks) - .innerJoin(chatConversations, and(eq(chatConversations.id, chatMessageLinks.conversationId), - eq(chatConversations.companyId, run.companyId), eq(chatConversations.issueId, run.nativeIssueId))) - .innerJoin(chatEndpoints, and(eq(chatEndpoints.id, chatConversations.endpointId), - eq(chatEndpoints.companyId, run.companyId), eq(chatEndpoints.provider, "github"), - eq(chatEndpoints.assignedAgentId, run.agentId))) - .leftJoin(toolConnections, and(eq(toolConnections.id, chatEndpoints.connectionId), - eq(toolConnections.companyId, run.companyId))) - .innerJoin(chatEndpointResources, and(eq(chatEndpointResources.id, chatConversations.resourceId), - eq(chatEndpointResources.companyId, run.companyId), eq(chatEndpointResources.endpointId, chatEndpoints.id))) - .innerJoin(chatGitHubReviews, and(eq(chatGitHubReviews.endpointId, chatEndpoints.id), - eq(chatGitHubReviews.companyId, run.companyId), eq(chatGitHubReviews.issueId, run.nativeIssueId))) - .leftJoin(heartbeatRuns, eq(heartbeatRuns.id, chatGitHubReviews.runId)) - .where(and(eq(chatMessageLinks.companyId, run.companyId), eq(chatMessageLinks.commentId, commentId), - eq(chatMessageLinks.direction, "inbound"))) - .orderBy(desc(chatGitHubReviews.createdAt), desc(chatGitHubReviews.id)).limit(1); + const [review] = await inheritedReviewQuery(db).execute({ + companyId: run.companyId, issueId: run.nativeIssueId, agentId: run.agentId, commentId, + }); // A newer unreviewed head cannot inherit the prior head's publication. if (!review) return null; if (!review.connectionEnabled || review.connectionStatus !== "active" || review.state !== "completed" ||