From d9f2a93ea87f1e134a612ae86b5a0d973afcee10 Mon Sep 17 00:00:00 2001 From: Aryan Date: Fri, 29 May 2026 11:59:06 -0700 Subject: [PATCH] fix(db): tolerate missing workers column in benchmark read queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #405's read queries select the `workers` column added by migration 006. Migrations are applied manually (pnpm admin:db:migrate), separately from the Vercel app deploy, so the deploy shipped a query referencing a column the production DB didn't have yet. Postgres fails to plan a bare reference to a non-existent column ("column \"workers\" does not exist"), so every cache-miss request to /api/v1/benchmarks 500'd — surfacing as the ~63% error rate after #405 (cache hits returned stale blobs; only misses hit the DB). Read `workers` via `to_jsonb(row) -> 'workers'` in all three read paths (getLatestBenchmarks base-table + view branches, getAllBenchmarksForHistory). JSON key lookup is a runtime op that returns NULL for an absent key instead of failing at parse time, so the endpoint keeps serving every other field whether or not migration 006 has run. Identical behavior once the column exists. Adds a regression test asserting the read queries never reference the workers column directly. --- packages/db/src/queries/benchmarks.test.ts | 57 ++++++++++++++++++++++ packages/db/src/queries/benchmarks.ts | 12 +++-- 2 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 packages/db/src/queries/benchmarks.test.ts diff --git a/packages/db/src/queries/benchmarks.test.ts b/packages/db/src/queries/benchmarks.test.ts new file mode 100644 index 00000000..3e32a780 --- /dev/null +++ b/packages/db/src/queries/benchmarks.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest'; + +import type { DbClient } from '../connection.js'; +import { getAllBenchmarksForHistory, getLatestBenchmarks } from './benchmarks.js'; + +/** + * A {@link DbClient} stand-in that records every SQL template it is handed and + * resolves to an empty result set. Lets us assert on the *generated SQL* without + * a live database — in particular that the read path never names the optional + * `workers` column directly. + * + * Joining the template's static segments with a ` ? ` placeholder reconstructs + * the literal SQL (interpolated values like model keys / dates become `?`), + * which is all we need to substring-match the column selection. + */ +function makeRecordingSql(): { sql: DbClient; sqlText: () => string } { + const queries: string[] = []; + const sql = ((strings: TemplateStringsArray, ..._values: unknown[]) => { + queries.push(strings.join(' ? ')); + return Promise.resolve([]); + }) as DbClient; + return { sql, sqlText: () => queries.join('\n') }; +} + +/** + * Regression guard for the #405 fallout: the read queries must surface `workers` + * via `to_jsonb(row) -> 'workers'`, NOT a bare `br.workers` / `lb.workers`. A bare + * column reference fails to plan ("column \"workers\" does not exist") when migration + * 006 hasn't been applied, which 500s every cache-miss request to /api/v1/benchmarks. + * The to_jsonb form returns null for the absent column and lets the endpoint keep + * serving everything else. + */ +describe('benchmark read queries — workers column tolerance', () => { + it('getLatestBenchmarks (no-date / materialized-view branch) does not reference lb.workers directly', async () => { + const { sql, sqlText } = makeRecordingSql(); + await getLatestBenchmarks(sql, 'dsr1'); + const text = sqlText(); + expect(text).toContain("to_jsonb(lb) -> 'workers'"); + expect(text).not.toMatch(/\blb\.workers\b/u); + }); + + it('getLatestBenchmarks (date-filtered / base-table branch) does not reference br.workers directly', async () => { + const { sql, sqlText } = makeRecordingSql(); + await getLatestBenchmarks(sql, 'dsr1', '2026-01-01'); + const text = sqlText(); + expect(text).toContain("to_jsonb(br) -> 'workers'"); + expect(text).not.toMatch(/\bbr\.workers\b/u); + }); + + it('getAllBenchmarksForHistory does not reference br.workers directly', async () => { + const { sql, sqlText } = makeRecordingSql(); + await getAllBenchmarksForHistory(sql, 'dsr1', 1024, 1024); + const text = sqlText(); + expect(text).toContain("to_jsonb(br) -> 'workers'"); + expect(text).not.toMatch(/\bbr\.workers\b/u); + }); +}); diff --git a/packages/db/src/queries/benchmarks.ts b/packages/db/src/queries/benchmarks.ts index a2167e95..b3a6e96f 100644 --- a/packages/db/src/queries/benchmarks.ts +++ b/packages/db/src/queries/benchmarks.ts @@ -90,7 +90,11 @@ export async function getLatestBenchmarks( br.conc, br.image, br.metrics, - br.workers, + -- Read workers via to_jsonb(row)->'workers' rather than a bare column ref so the + -- query still plans (and returns null) if migration 006 has not been applied yet. + -- A bare column reference throws "column does not exist" at parse time and 500s the + -- whole endpoint. See migration 006_benchmark_results_workers.sql. + to_jsonb(br) -> 'workers' AS workers, br.date::text, CASE WHEN wr.html_url IS NOT NULL THEN wr.html_url || '/attempts/' || wr.run_attempt ELSE NULL END AS run_url FROM benchmark_results br @@ -129,7 +133,8 @@ export async function getLatestBenchmarks( lb.conc, lb.image, lb.metrics, - lb.workers, + -- to_jsonb(row)->'workers' guard: tolerate a pre-migration-006 view (see date branch above). + to_jsonb(lb) -> 'workers' AS workers, lb.date::text, CASE WHEN wr.html_url IS NOT NULL THEN wr.html_url || '/attempts/' || wr.run_attempt ELSE NULL END AS run_url FROM latest_benchmarks lb @@ -176,7 +181,8 @@ export async function getAllBenchmarksForHistory( br.osl, br.conc, br.metrics - '{std_ttft,std_tpot,std_e2el,std_intvty,std_itl,mean_ttft,mean_tpot,mean_e2el,mean_intvty,mean_itl}'::text[] as metrics, - br.workers, + -- to_jsonb(row)->'workers' guard: tolerate a pre-migration-006 base table (see getLatestBenchmarks). + to_jsonb(br) -> 'workers' AS workers, br.date::text, CASE WHEN wr.html_url IS NOT NULL THEN wr.html_url || '/attempts/' || wr.run_attempt ELSE NULL END AS run_url FROM configs c