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/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, 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`]); +});