From f2140494f1ec29a567dfce265d866183ac38257b Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 02:29:00 +1100 Subject: [PATCH] perf(artifact-delivery): cut D1 round trips per run The build queue consumer runs in a US colo, and the D1 primary is in Sydney. Each round trip cost about 170 ms in production traces, and a build spent 19 to 22 of them. Every state change wrote, then read the row back. A state change now returns its row with RETURNING, a new Resolution request stores its row in one statement, and a grant that names its Resolution reads the row and its check results in one batch. --- .../artifact-delivery/server/utils/grant.ts | 46 ++++-- .../artifact-delivery/server/utils/state.ts | 58 +++---- test/unit/artifact-build-reuse.test.ts | 142 ++++++++++++++++++ test/unit/helpers/d1-sqlite.ts | 7 +- 4 files changed, 203 insertions(+), 50 deletions(-) diff --git a/layers/artifact-delivery/server/utils/grant.ts b/layers/artifact-delivery/server/utils/grant.ts index 14c26c74..5d73bf12 100644 --- a/layers/artifact-delivery/server/utils/grant.ts +++ b/layers/artifact-delivery/server/utils/grant.ts @@ -7,6 +7,15 @@ import { checksBlockArtifact } from './checks' const PUBLIC_GRANT_SECONDS = 5 * 60 +interface ArtifactCheckRow { + name: string + version: string + outcome: string + required: number + summary: string | null + findings_json: string +} + interface ArtifactGrantRow { id: string content_sha256: string @@ -48,7 +57,7 @@ export async function createPublicArtifactGrant( artifactId: string, resolutionId?: string, ): Promise { - const row = await dependencies.db.prepare( + const rowStatement = dependencies.db.prepare( `SELECT a.id, a.content_sha256, a.content_bytes, a.r2_key, a.delivery_status, aa.resolution_id, aa.attestation_json, @@ -60,7 +69,25 @@ export async function createPublicArtifactGrant( AND (?2 IS NULL OR aa.resolution_id = ?2) ORDER BY aa.created_at DESC, aa.resolution_id DESC LIMIT 1`, - ).bind(artifactId, resolutionId ?? null).first() + ).bind(artifactId, resolutionId ?? null) + const checksStatement = (id: string) => dependencies.db.prepare( + `SELECT name, version, outcome, required, summary, findings_json + FROM artifact_check_results + WHERE resolution_id = ?1 + ORDER BY name`, + ).bind(id) + // A client that names its Resolution gets both reads in one D1 round trip. + // Without the name, the check read needs the row's Resolution first. + const [row, checkRows] = resolutionId + ? await dependencies.db.batch([rowStatement, checksStatement(resolutionId)]) + .then(([rows, checks]) => [ + (rows?.results[0] as ArtifactGrantRow | undefined) ?? null, + (checks?.results ?? []) as ArtifactCheckRow[], + ] as const) + : await rowStatement.first().then(async found => [ + found, + found ? (await checksStatement(found.resolution_id).all()).results : [], + ] as const) if (!row) return { _tag: 'not-found' } if (row.delivery_status === 'revoked' || row.resolution_state === 'revoked') @@ -68,20 +95,7 @@ export async function createPublicArtifactGrant( if (row.delivery_status !== 'available' || row.resolution_state !== 'ready') return { _tag: 'denied', code: 'CHECK_BLOCKED' } - const checkRows = await dependencies.db.prepare( - `SELECT name, version, outcome, required, summary, findings_json - FROM artifact_check_results - WHERE resolution_id = ?1 - ORDER BY name`, - ).bind(row.resolution_id).all<{ - name: string - version: string - outcome: string - required: number - summary: string | null - findings_json: string - }>() - const checks = checkRows.results.map(check => checkResultSchema.parse({ + const checks = checkRows.map(check => checkResultSchema.parse({ name: check.name, version: check.version, outcome: check.outcome, diff --git a/layers/artifact-delivery/server/utils/state.ts b/layers/artifact-delivery/server/utils/state.ts index bea13ef6..a2b2664d 100644 --- a/layers/artifact-delivery/server/utils/state.ts +++ b/layers/artifact-delivery/server/utils/state.ts @@ -165,13 +165,8 @@ export async function createResolution( repositoryId: number } | { visibility: 'public' } = { visibility: 'public' }, ): Promise { - const existing = await findResolutionByRequestKey(db, identity.keyHash) - if (existing) { - return existing.request_fingerprint === identity.fingerprint - ? { _tag: 'existing', row: existing } - : { _tag: 'idempotency-conflict' } - } - + // A new request key inserts and returns its row in one round trip. A + // replayed key inserts nothing, and the lookup below finds its row. const resolutionId = crypto.randomUUID() const selectorValue = source.selector.type === 'path' ? source.selector.path : source.selector.name const insert = await db.prepare( @@ -183,7 +178,8 @@ export async function createResolution( ) VALUES ( ?1, ?2, ?3, 'requested', 0, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?14 - )`, + ) + RETURNING *`, ).bind( resolutionId, identity.keyHash, @@ -199,20 +195,15 @@ export async function createResolution( access.visibility === 'private' ? access.accountId : null, access.visibility === 'private' ? access.installationId : null, now, - ).run() - - if (Number(insert.meta.changes) === 1) { - const row = await getResolution(db, resolutionId) - if (!row) - throw new Error('Created Resolution could not be loaded') - return { _tag: 'created', row } - } + ).first>() + if (insert) + return { _tag: 'created', row: resolutionRowSchema.parse(insert) } - const raced = await findResolutionByRequestKey(db, identity.keyHash) - if (!raced) + const existing = await findResolutionByRequestKey(db, identity.keyHash) + if (!existing) throw new Error('Resolution idempotency race could not be loaded') - return raced.request_fingerprint === identity.fingerprint - ? { _tag: 'existing', row: raced } + return existing.request_fingerprint === identity.fingerprint + ? { _tag: 'existing', row: existing } : { _tag: 'idempotency-conflict' } } @@ -269,14 +260,16 @@ export async function transitionResolution( 'UPDATE artifact_resolutions', `SET state = ?1, state_version = state_version + 1, updated_at = ?2${assignments.length ? `, ${assignments.join(', ')}` : ''}`, 'WHERE id = ?3 AND state = ?4 AND state_version = ?5', + // The advanced row comes back with the write, so a transition costs one + // D1 round trip. The queue consumer runs far from the D1 primary. + 'RETURNING *', ].join(' ') - const result = await db.prepare(sql).bind(next, now, row.id, row.state, row.state_version, ...values).run() - if (Number(result.meta.changes) !== 1) - return { _tag: 'superseded' } - const advanced = await getResolution(db, row.id) + const advanced = await db.prepare(sql) + .bind(next, now, row.id, row.state, row.state_version, ...values) + .first>() if (!advanced) - throw new Error('Advanced Resolution could not be loaded') - return { _tag: 'advanced', row: advanced } + return { _tag: 'superseded' } + return { _tag: 'advanced', row: resolutionRowSchema.parse(advanced) } } /** @@ -557,16 +550,15 @@ export async function publishArtifactRecord( db.prepare( `UPDATE artifact_resolutions SET state = 'ready', state_version = state_version + 1, updated_at = ?1 - WHERE id = ?2 AND state = 'publishing' AND state_version = ?3`, + WHERE id = ?2 AND state = 'publishing' AND state_version = ?3 + RETURNING *`, ).bind(now, row.id, row.state_version), ] - const results = await db.batch(statements) - if (Number(results.at(-1)?.meta.changes) !== 1) - return { _tag: 'superseded' } - const published = await getResolution(db, row.id) + const results = await db.batch>(statements) + const published = results.at(-1)?.results?.[0] if (!published) - throw new Error('Published Resolution could not be loaded') - return { _tag: 'published', row: published } + return { _tag: 'superseded' } + return { _tag: 'published', row: resolutionRowSchema.parse(published) } } function publicArtifactPublishStatements( diff --git a/test/unit/artifact-build-reuse.test.ts b/test/unit/artifact-build-reuse.test.ts index 6725b405..d0fb36c9 100644 --- a/test/unit/artifact-build-reuse.test.ts +++ b/test/unit/artifact-build-reuse.test.ts @@ -23,6 +23,7 @@ import { } from '../../layers/artifact-delivery/server/utils/attestation' import { processArtifactBuild } from '../../layers/artifact-delivery/server/utils/build' import { bytesToBase64Url } from '../../layers/artifact-delivery/server/utils/encoding' +import { createPublicArtifactGrant } from '../../layers/artifact-delivery/server/utils/grant' import { ARTIFACT_BUILD_QUEUE_NAME, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' import { resolveSkillPageUrl } from '../../layers/artifact-delivery/server/utils/skill-page' import { @@ -602,3 +603,144 @@ function memoryBucket() { remove: (key: string) => objects.delete(key), } } + +describe('round trips to D1 for one build', () => { + // The queue consumer runs far from the D1 primary: each round trip cost + // about 170 ms in production traces on 2026-10-07, so the count is latency. + it('a fresh build reaches ready in 14 round trips', async () => { + const harness = await createReuseHarness() + const resolutionId = await harness.request(pinned) + const counted = countRoundTrips(harness.db) + + const outcome = await processArtifactBuild({ ...harness.dependencies(githubServing(source)), db: counted.db }, resolutionId) + + expect(outcome._tag).toBe('ready') + expect(counted.roundTrips()).toBe(14) + harness.close() + }) + + it('a reused build reaches ready in 11 round trips', async () => { + const harness = await createReuseHarness() + await harness.build(pinned, githubServing(source)) + const resolutionId = await harness.request(pinned) + const counted = countRoundTrips(harness.db) + + const outcome = await processArtifactBuild({ ...harness.dependencies(githubDown()), db: counted.db }, resolutionId) + + expect(outcome._tag).toBe('ready') + expect(counted.roundTrips()).toBe(11) + harness.close() + }) +}) + +describe('round trips to D1 for one Resolution request', () => { + it('a new request key stores its Resolution in one round trip', async () => { + const harness = await createReuseHarness() + const counted = countRoundTrips(harness.db) + const identity = await resolutionRequestIdentity(pinned, 'round-trip-request-key') + + const created = await createResolution(counted.db, pinned, identity, harness.now()) + + expect(created._tag).toBe('created') + expect(counted.roundTrips()).toBe(1) + harness.close() + }) + + it('a replayed request key answers its first Resolution', async () => { + const harness = await createReuseHarness() + const identity = await resolutionRequestIdentity(pinned, 'round-trip-request-key') + const first = await createResolution(harness.db, pinned, identity, harness.now()) + const counted = countRoundTrips(harness.db) + + const replay = await createResolution(counted.db, pinned, identity, harness.now()) + + expect(replay).toEqual({ _tag: 'existing', row: first._tag === 'created' ? first.row : undefined }) + expect(counted.roundTrips()).toBe(2) + harness.close() + }) +}) + +describe('round trips to D1 for one grant', () => { + it('a grant for a named Resolution reads D1 once', async () => { + const harness = await createReuseHarness() + const built = await harness.build(pinned, githubServing(source)) + const counted = countRoundTrips(harness.db) + + const grant = await createPublicArtifactGrant({ + db: counted.db, + trustedRoot: harness.trustedRoot(), + publicBaseUrl: 'https://artifacts.skilld.dev', + now: harness.now(), + }, built.row.artifact_id!, built.row.id) + + expect(grant._tag).toBe('granted') + expect(counted.roundTrips()).toBe(1) + harness.close() + }) + + it('a grant without a named Resolution still checks the newest one', async () => { + const harness = await createReuseHarness() + const built = await harness.build(pinned, githubServing(source)) + const counted = countRoundTrips(harness.db) + + const grant = await createPublicArtifactGrant({ + db: counted.db, + trustedRoot: harness.trustedRoot(), + publicBaseUrl: 'https://artifacts.skilld.dev', + now: harness.now(), + }, built.row.artifact_id!) + + expect(grant._tag).toBe('granted') + expect(counted.roundTrips()).toBe(2) + harness.close() + }) + + it('a grant for a named Resolution that blocks delivery is denied', async () => { + const harness = await createReuseHarness() + const built = await harness.build(pinned, githubServing(source)) + harness.raw.prepare(`UPDATE artifact_check_results SET outcome = 'fail', required = 1 WHERE resolution_id = ?`).run(built.row.id) + + const grant = await createPublicArtifactGrant({ + db: harness.db, + trustedRoot: harness.trustedRoot(), + publicBaseUrl: 'https://artifacts.skilld.dev', + now: harness.now(), + }, built.row.artifact_id!, built.row.id) + + expect(grant).toEqual({ _tag: 'denied', code: 'CHECK_BLOCKED' }) + harness.close() + }) +}) + +/** Counts D1 network round trips: one per statement call, one per batch. */ +function countRoundTrips(db: D1Database) { + let count = 0 + const wrap = (statement: D1PreparedStatement): D1PreparedStatement => new Proxy(statement, { + get(target, property, receiver) { + const value = Reflect.get(target, property, receiver) + if (property === 'bind') + return (...values: unknown[]) => wrap(target.bind(...values)) + if (property === 'first' || property === 'all' || property === 'run' || property === 'raw') { + return (...args: unknown[]) => { + count += 1 + return (value as (...inner: unknown[]) => unknown).apply(target, args) + } + } + return value + }, + }) + const counted = new Proxy(db, { + get(target, property, receiver) { + if (property === 'prepare') + return (sql: string) => wrap(target.prepare(sql)) + if (property === 'batch') { + return async (statements: D1PreparedStatement[]) => { + count += 1 + return await target.batch(statements) + } + } + return Reflect.get(target, property, receiver) + }, + }) + return { db: counted, roundTrips: () => count } +} diff --git a/test/unit/helpers/d1-sqlite.ts b/test/unit/helpers/d1-sqlite.ts index 7a437787..5e63d9a8 100644 --- a/test/unit/helpers/d1-sqlite.ts +++ b/test/unit/helpers/d1-sqlite.ts @@ -93,11 +93,16 @@ export function createSqliteD1( try { const out = statements.map((s) => { // `querySkills` batches three SELECTs and reads `.results` off each; - // reads carry results, writes carry the change count. + // reads carry results, writes carry the change count. A write with + // RETURNING carries both, as it does on D1. if (/^\s*(?:SELECT|WITH)/i.test(s._sql)) { const results = raw.prepare(s._sql).all(...s._values) return { results, meta: { changes: 0, last_row_id: 0 } } } + if (/\bRETURNING\b/i.test(s._sql)) { + const results = raw.prepare(s._sql).all(...s._values) + return { results, meta: { changes: results.length, last_row_id: 0 } } + } const result = raw.prepare(s._sql).run(...s._values) return { meta: { changes: Number(result.changes), last_row_id: Number(result.lastInsertRowid) } } })