Skip to content

Commit f8bc60c

Browse files
bloveclaude
andcommitted
fix(bench): make a run prove which build it measured
Closes #355. `bench:matrix` printed `Error: Port 4173 is already in use` and then ran the entire matrix against whatever was already there — another worktree's `preview:bench` — writing ordinary-looking artifacts that described a different branch's code. On #327 that inverted a conclusion for hours: post-fix runs kept reporting `partial`, which read as "your fix did not work" rather than "you are measuring someone else's build". The hole is in `waitForServer`: it asks whether SOMETHING answers, and a foreign server answers instantly. Nothing downstream ever asked *what* answered. `vite build` now mints a fresh id, writes it to `dist/bench-build-id.txt`, and injects the same value into the bundle, which publishes it on `<html>`. Two independent checks read it: - `bench-matrix.mjs` fetches the id from the server before running anything, so a collision fails in the first second with the port to look at rather than after a full matrix. - `bench.spec.ts` compares the served id to `dist/` before it measures, so a run driven any other way — directly, from an editor, with a hand-set BASE_URL — gets the same protection. A git SHA cannot do this job: the tree is dirty for most of a development loop, so the SHA is identical across builds that differ. A per-build id answers the question actually being asked, and catches a stale `dist/` and a forgotten rebuild as well. `PRETABLE_BENCH_SKIP_BUILD_CHECK=1` opts out when the target is deliberately not a local build. Verified against the live collision that motivated it — the other worktree's server still holding 4173: Error: http://127.0.0.1:4173 is not serving the build this checkout just produced expected (dist/): 03f00ae1-… served: (no id — an older build, or not a bench server) Check `lsof -ti:4173`. and no benchmark ran. The spec-level check was mutation-tested the same way, by pointing it at a server serving a different id. A pre-check build reports no id rather than its index.html, because the preview server falls back to that page for anything it cannot find — only an id-shaped token counts as an answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 9684385 commit f8bc60c

4 files changed

Lines changed: 163 additions & 1 deletion

File tree

‎apps/bench/src/app.tsx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,18 @@ import { BenchApp } from "./bench-app";
55
import { detectBrowserVersion } from "./bench-runtime";
66

77
const APP_VERSION = import.meta.env.VITE_APP_VERSION as string;
8+
/**
9+
* Which build this page IS, so a run can prove it measured the build it meant
10+
* to. Written on `<html>` rather than inside the app so it survives whatever
11+
* the adapter is doing, and so a check can read it before any grid mounts.
12+
* Undefined under `vite dev` and vitest, where nothing is being published.
13+
*/
14+
const BENCH_BUILD_ID = import.meta.env.VITE_BENCH_BUILD_ID as
15+
string | undefined;
16+
17+
if (typeof document !== "undefined" && BENCH_BUILD_ID) {
18+
document.documentElement.dataset.benchBuildId = BENCH_BUILD_ID;
19+
}
820

921
export function App() {
1022
return (

‎apps/bench/tests/bench.spec.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,66 @@ const scriptName = process.env.PRETABLE_BENCH_SCRIPT ?? "initial";
1818
const updateRatePerSec = process.env.PRETABLE_BENCH_UPDATE_RATE_PER_SEC;
1919
const diagnostics = process.env.PRETABLE_BENCH_DIAGNOSTICS;
2020
const seed = process.env.PRETABLE_BENCH_SEED;
21+
/**
22+
* Prove the page under test is the build in this checkout.
23+
*
24+
* A benchmark number is only worth something if it names a commit. These stopped
25+
* doing that silently: with another worktree holding port 4173, `bench:matrix`
26+
* printed `Error: Port 4173 is already in use` and ran the whole suite against
27+
* that server anyway, writing artifacts that looked ordinary while describing a
28+
* different branch's code. Post-fix runs read as failures for hours because
29+
* they were measuring pre-fix code.
30+
*
31+
* `vite build` writes a fresh id into `dist/` and injects the same value into
32+
* the bundle, so the two agree only when the server is serving THIS build. That
33+
* also catches a stale `dist/` and a forgotten rebuild.
34+
*
35+
* Set `PRETABLE_BENCH_SKIP_BUILD_CHECK=1` when the target is deliberately not
36+
* a local build (a deployed bench, someone else's preview).
37+
*/
38+
const skipBuildCheck = process.env.PRETABLE_BENCH_SKIP_BUILD_CHECK === "1";
39+
const BUILD_ID_PATH = path.join(
40+
import.meta.dirname,
41+
"..",
42+
"dist",
43+
"bench-build-id.txt",
44+
);
45+
46+
async function expectServedBuildMatchesCheckout(page: {
47+
getAttribute: (name: string) => Promise<string | null>;
48+
}): Promise<void> {
49+
if (skipBuildCheck) return;
50+
51+
let expected: string;
52+
try {
53+
expected = (await readFile(BUILD_ID_PATH, "utf8")).trim();
54+
} catch {
55+
throw new Error(
56+
`No ${BUILD_ID_PATH}. Build the bench before measuring it ` +
57+
"(`pnpm --filter @pretable/app-bench build`), or set " +
58+
"PRETABLE_BENCH_SKIP_BUILD_CHECK=1 if the target is deliberately not " +
59+
"a local build.",
60+
);
61+
}
62+
63+
const served = await page.getAttribute("data-bench-build-id");
64+
expect(
65+
served,
66+
[
67+
"The page under test is not the build in this checkout, so every number",
68+
"this run produces describes code you did not build.",
69+
"",
70+
` expected (dist/): ${expected}`,
71+
` served (page): ${served ?? "(absent)"}`,
72+
"",
73+
"Usual cause: something else already holds the port, and the run",
74+
"attached to it instead of failing — check `lsof -ti:4173` for another",
75+
"worktree's `preview:bench`. An absent id means the server is serving a",
76+
"build from before this check existed, or a `vite dev` server.",
77+
].join("\n"),
78+
).toBe(expected);
79+
}
80+
2181
const adapterLabel =
2282
adapterId === "ag-grid"
2383
? "AG Grid Community adapter"
@@ -47,6 +107,9 @@ test("writes benchmark artifacts for the selected Pretable run", async ({
47107
`/?adapter=${adapterId}&scenario=${scenarioId}&scale=${scale}&script=${scriptName}${rateParam}${diagnosticsParam}${seedParam}&autorun=1${triggerParam}`,
48108
);
49109

110+
// Before anything is measured: is this even our build?
111+
await expectServedBuildMatchesCheckout(page.locator("html"));
112+
50113
await expect(page.getByLabel(adapterLabel).first()).toBeVisible();
51114

52115
let cdpSession: Awaited<

‎apps/bench/vite.config.ts‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,51 @@
1+
import { randomUUID } from "node:crypto";
2+
import fs from "node:fs";
3+
import path from "node:path";
4+
15
import react from "@vitejs/plugin-react";
26
import tailwindcss from "@tailwindcss/vite";
37
import { defineConfig } from "vitest/config";
48

59
import pkg from "./package.json" with { type: "json" };
610

11+
/**
12+
* A per-build identity, so a run can prove WHICH build it measured.
13+
*
14+
* The bench's numbers are only worth anything if they describe a known commit.
15+
* They silently stopped doing that: with a second worktree holding port 4173,
16+
* `bench:matrix` printed `Error: Port 4173 is already in use` and then ran the
17+
* whole suite against that other branch's `vite preview`, writing artifacts as
18+
* if nothing were wrong. Post-fix runs read as failures because they were
19+
* measuring pre-fix code.
20+
*
21+
* A git SHA cannot do this job: the working tree is dirty for most of a
22+
* development loop, so the SHA is identical across builds that differ. A fresh
23+
* id per build is exactly the question being asked — "is the server answering
24+
* me the one I just built?" — and it also catches a stale `dist/` and a
25+
* forgotten rebuild, which have both bitten this repo.
26+
*
27+
* Written to `dist/` so the test can read the id it expects off disk, and
28+
* injected into the bundle so the page can report the id it actually is.
29+
*/
30+
const BUILD_ID = randomUUID();
31+
const BUILD_ID_FILE = "bench-build-id.txt";
32+
733
export default defineConfig({
8-
plugins: [react(), tailwindcss()],
34+
plugins: [
35+
react(),
36+
tailwindcss(),
37+
{
38+
name: "bench-build-id",
39+
apply: "build",
40+
writeBundle(options) {
41+
const dir = options.dir ?? path.join(import.meta.dirname, "dist");
42+
fs.writeFileSync(path.join(dir, BUILD_ID_FILE), BUILD_ID, "utf8");
43+
},
44+
},
45+
],
946
define: {
1047
"import.meta.env.VITE_APP_VERSION": JSON.stringify(pkg.version),
48+
"import.meta.env.VITE_BENCH_BUILD_ID": JSON.stringify(BUILD_ID),
1149
},
1250
build: {
1351
sourcemap: true,

‎scripts/bench-matrix.mjs‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ async function run() {
234234

235235
try {
236236
await waitForServer(BENCH_BASE_URL, server);
237+
await assertServerIsOurBuild(BENCH_BASE_URL, previewLaunch.preview.cwd);
237238

238239
for (const entry of createBenchMatrixEntries(parsedArgs)) {
239240
runEntries.push(await runBenchEntry(entry, parsedArgs.passthroughArgs));
@@ -374,6 +375,54 @@ function parsePositiveInteger(value, fallback) {
374375
return Number.isInteger(parsed) && parsed > 0 ? parsed : fallback;
375376
}
376377

378+
/**
379+
* `waitForServer` asks only whether SOMETHING answers, which is why a second
380+
* worktree already holding 4173 satisfied it instantly: our own `vite preview`
381+
* failed to bind, the run continued, and the whole matrix measured another
382+
* branch's build while writing ordinary-looking artifacts.
383+
*
384+
* `vite build` writes a fresh id into `dist/` and serves it as a static file,
385+
* so asking the server for it and comparing to the one on disk answers the
386+
* question that matters — is this the build we just made?
387+
*/
388+
async function assertServerIsOurBuild(url, benchCwd) {
389+
const expected = (
390+
await readFile(path.join(benchCwd, "dist", "bench-build-id.txt"), "utf8")
391+
).trim();
392+
393+
let served = null;
394+
try {
395+
const response = await fetch(new URL("/bench-build-id.txt", url));
396+
if (response.ok) {
397+
// A build from before this file existed still answers 200 here: the
398+
// preview server falls back to index.html for anything it cannot find.
399+
// Treat only a lone id-shaped token as an answer, so the report says
400+
// "no id" rather than quoting a page of HTML at the reader.
401+
const body = (await response.text()).trim();
402+
if (/^[A-Za-z0-9-]{8,64}$/.test(body)) served = body;
403+
}
404+
} catch {
405+
// Falls through to the mismatch report below, which says more than the
406+
// fetch error would.
407+
}
408+
409+
if (served === expected) return;
410+
411+
throw new Error(
412+
[
413+
`${url} is not serving the build this checkout just produced, so every`,
414+
"number the matrix is about to write would describe code from somewhere",
415+
"else.",
416+
"",
417+
` expected (dist/): ${expected}`,
418+
` served: ${served ?? "(no id — an older build, or not a bench server)"}`,
419+
"",
420+
"Usual cause: another worktree's `preview:bench` already holds the port.",
421+
"Check `lsof -ti:4173`.",
422+
].join("\n"),
423+
);
424+
}
425+
377426
async function waitForServer(url, server, timeoutMs = 30_000) {
378427
const deadline = Date.now() + timeoutMs;
379428

0 commit comments

Comments
 (0)