From 2559541197237b89c40140392ed1272ad640495a Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya <1445792+JArmandoAnaya@users.noreply.github.com> Date: Sun, 11 Oct 2026 21:04:00 -0700 Subject: [PATCH 1/2] test: gate the frames-per-request ceiling across server and client (#849) The frames route enforces FRAMES_PER_REQUEST with a 422, and the browser frame sink slices uploads at its own copy of the number. The generated client is types only, so the contract cannot carry it to the sink. tests/scripts/frames_per_request.test.mjs now holds the route's constant, the route's published docstring and the sink's chunk size to one value, and fails naming each file and its value. The sink exports its constant and its test imports it instead of restating 32. The route file joins the frontend path group so a server-only change wakes the job that runs the gate. --- .github/path-filters.yml | 4 ++ frontend/app/src/data/frameSink.test.ts | 9 ++- frontend/app/src/data/frameSink.ts | 9 +-- tests/scripts/ci_path_filters.test.mjs | 4 ++ tests/scripts/frames_per_request.test.mjs | 76 +++++++++++++++++++++++ 5 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 tests/scripts/frames_per_request.test.mjs diff --git a/.github/path-filters.yml b/.github/path-filters.yml index 605d3be5..dfc61cfe 100644 --- a/.github/path-filters.yml +++ b/.github/path-filters.yml @@ -67,6 +67,10 @@ frontend: # to `python`, whose pytest never collects them. Without this line, editing one # of those tests would run every suite except the one being edited. - "tests/scripts/**" + # Read as text by `tests/scripts/frames_per_request.test.mjs`, which holds the + # browser's upload chunk to this route's bound; a change to the bound alone has + # to wake the job that runs it. + - "src/visionset/server/routes/video_imports.py" - ".github/workflows/ci.yml" - ".github/path-filters.yml" diff --git a/frontend/app/src/data/frameSink.test.ts b/frontend/app/src/data/frameSink.test.ts index 44bc026a..bba587e1 100644 --- a/frontend/app/src/data/frameSink.test.ts +++ b/frontend/app/src/data/frameSink.test.ts @@ -19,15 +19,12 @@ import { describe, expect, it } from "vitest"; import type { MaterializedFrame } from "@visionset/media"; -import { createLocalApiFrameSink } from "./frameSink"; +import { createLocalApiFrameSink, FRAMES_PER_REQUEST } from "./frameSink"; import { createOssDataClient } from "./ossClient"; const BASE = "http://visionset.test"; const TARGET = { projectId: "p", importId: "i" }; -/** The bound the sink slices at, restated so a change to it fails here loudly. */ -const FRAMES_PER_REQUEST = 32; - const IMPORT = { id: "i", project_id: "p", @@ -117,7 +114,9 @@ describe("what one append puts on the wire", () => { // did not slice with its files would attach every frame's metadata to the wrong // frame, and every part would still be well-formed. expect(first.descriptors).toHaveLength(FRAMES_PER_REQUEST); - expect(second.descriptors.map((one) => one.ordinal)).toEqual([32, 33, 34, 35, 36]); + expect(second.descriptors.map((one) => one.ordinal)).toEqual( + Array.from({ length: 5 }, (_, at) => FRAMES_PER_REQUEST + at), + ); }); it("sends nothing at all for an empty chunk", async () => { diff --git a/frontend/app/src/data/frameSink.ts b/frontend/app/src/data/frameSink.ts index b986e86c..61ca29a9 100644 --- a/frontend/app/src/data/frameSink.ts +++ b/frontend/app/src/data/frameSink.ts @@ -20,11 +20,12 @@ import type { AbortSignal as AbortLike, FrameSink, MaterializedFrame } from "@vi /** * How many frame parts one request may carry — the server's own bound * (`routes/video_imports.py`), restated here because exceeding it is a 422 rather - * than something a type could catch. The materializer's `FRAME_CHUNK_SIZE` is - * smaller, so the loop below normally runs once; it exists because the sink's - * contract takes an array of any length, not because the current caller needs it. + * than something a type could catch. `tests/scripts/frames_per_request.test.mjs` + * holds the two equal. The materializer's `FRAME_CHUNK_SIZE` is smaller, so the + * loop below normally runs once; it exists because the sink's contract takes an + * array of any length, not because the current caller needs it. */ -const FRAMES_PER_REQUEST = 32; +export const FRAMES_PER_REQUEST = 32; /** The parallel array the frames route reads, one entry per uploaded part. */ function descriptorsOf(frames: readonly MaterializedFrame[]): string { diff --git a/tests/scripts/ci_path_filters.test.mjs b/tests/scripts/ci_path_filters.test.mjs index 805326af..5724d708 100644 --- a/tests/scripts/ci_path_filters.test.mjs +++ b/tests/scripts/ci_path_filters.test.mjs @@ -89,6 +89,10 @@ test("these script tests wake the job that actually runs them", () => { ); }); +test("a server file a script gate reads wakes the job that runs the gate", () => { + assert.deepEqual(groupsFor("src/visionset/server/routes/video_imports.py"), ["docker", "frontend", "python"]); +}); + test("the API contract wakes both sides of it", () => { // Python regenerates it and fails on drift; the frontend regenerates its client // from it and fails on drift. A change here has to face both. diff --git a/tests/scripts/frames_per_request.test.mjs b/tests/scripts/frames_per_request.test.mjs new file mode 100644 index 00000000..7d2efc58 --- /dev/null +++ b/tests/scripts/frames_per_request.test.mjs @@ -0,0 +1,76 @@ +// Run with: pnpm test:scripts (also part of the root `pnpm test`) +// +// The frames route refuses a request carrying more than `FRAMES_PER_REQUEST` parts +// with a 422, and the browser's frame sink slices its uploads at its own copy of +// that number. The generated client is types only, so even a `maxItems` in the +// schema would reach the sink as no value at all — nothing but this gate holds the +// copies together (#849). The route's docstring states it too, and that prose +// is published in `openapi.json` as the operation's description. +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { test } from "node:test"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "../.."); + +const ROUTE = "src/visionset/server/routes/video_imports.py"; +const SINK = "frontend/app/src/data/frameSink.ts"; +const SINK_TEST = "frontend/app/src/data/frameSink.test.ts"; + +const SPELLINGS = [ + { file: ROUTE, label: "the enforced constant", pattern: /^FRAMES_PER_REQUEST: Final = (\d+)$/gm }, + { file: ROUTE, label: "the route's docstring", pattern: /at most (\d+) parts per request/g }, + { file: SINK, label: "the client's chunk size", pattern: /^export const FRAMES_PER_REQUEST = (\d+);$/gm }, +]; + +const read = (file) => readFileSync(path.join(ROOT, file), "utf8"); + +/** Every spelling's value, or a reason it could not be read as exactly one. */ +function measure(texts) { + return SPELLINGS.map(({ file, label, pattern }) => { + const found = [...texts[file].matchAll(pattern)].map((match) => Number(match[1])); + return { file, label, found }; + }); +} + +function disagreements(texts) { + const spellings = measure(texts); + const problems = spellings + .filter(({ found }) => found.length !== 1) + .map(({ file, label, found }) => `${file}: expected exactly one match for ${label}, found ${found.length}`); + if (problems.length > 0) return problems; + const values = new Set(spellings.map(({ found }) => found[0])); + if (values.size === 1) return []; + return [ + "FRAMES_PER_REQUEST disagrees across server and client — the server's value is the bound, " + + "the others must follow it:\n" + + spellings.map(({ file, label, found }) => ` ${file} (${label}): ${found[0]}`).join("\n"), + ]; +} + +const committed = () => ({ [ROUTE]: read(ROUTE), [SINK]: read(SINK) }); + +test("the server's bound and the client's chunk size are one number", () => { + assert.deepEqual(disagreements(committed()), []); +}); + +test("the sink's test takes the bound from the sink instead of restating it", () => { + const text = read(SINK_TEST); + assert.match(text, /import \{[^}]*\bFRAMES_PER_REQUEST\b[^}]*\} from "\.\/frameSink";/); + assert.doesNotMatch(text, /\bconst FRAMES_PER_REQUEST\b/, `${SINK_TEST} declares its own copy`); +}); + +test("the gate fires on a drifted copy", () => { + const texts = committed(); + const drifted = { ...texts, [SINK]: texts[SINK].replace(/(export const FRAMES_PER_REQUEST = )\d+;/, "$164;") }; + const [problem] = disagreements(drifted); + assert.ok(problem, "a client copy of 64 against the server's bound passed"); + for (const file of [ROUTE, SINK]) assert.ok(problem.includes(file), `the message does not name ${file}`); +}); + +test("the gate fires when a spelling disappears", () => { + const texts = committed(); + const renamed = { ...texts, [ROUTE]: texts[ROUTE].replace("FRAMES_PER_REQUEST: Final", "MAX_FRAMES: Final") }; + assert.deepEqual(disagreements(renamed), [`${ROUTE}: expected exactly one match for the enforced constant, found 0`]); +}); From d8626019849b1acebd2752380de3b3d08c32e7aa Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya <1445792+JArmandoAnaya@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:48:09 -0700 Subject: [PATCH 2/2] test(cycle): do not count an aborted upload as one that travelled The cancel test's fetch wrapper recorded a frames upload that the cancel aborted while its clone was still being read. fetch rejects before anything reaches the network, so Chromium never reported it and the wrapper-versus-wire comparison failed whenever the abort won that race. --- frontend/app/cycle/video-import.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frontend/app/cycle/video-import.spec.ts b/frontend/app/cycle/video-import.spec.ts index e2db3b1a..20293dc9 100644 --- a/frontend/app/cycle/video-import.spec.ts +++ b/frontend/app/cycle/video-import.spec.ts @@ -239,6 +239,10 @@ async function watch(page: Page): Promise { // silently counted as a body with nothing in it. unreadable = true; } + const signal = init?.signal ?? (input instanceof Request ? input.signal : null); + // Aborted while the clone was being read: fetch rejects without a request ever + // reaching Chromium, so it did not travel and the wire count cannot include it. + if (signal?.aborted) return native(input, init); sent.push({ method: probe.method, path: new URL(probe.url, location.href).pathname,