commit cea69e7bb246d609c66d92300d1d4be0109c57e4 parent 6b0036ad5458903604812f7e7e85c65d1e55a056 Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st> Date: Sun, 20 Sep 2026 22:02:57 -0400 review: the merge must remove a directory, and a channel must not defeat the rule Four fixes from the branch review. **reconcileVideoDirs removed the emptied `clips/` with rm() and no `recursive`**, which throws EISDIR on a directory. The catch swallowed it, the source survived, and the rmdir(srcDir) that follows then failed ENOTEMPTY — so every clips-on-both-sides reconcile reported a false "source dir not empty" conflict and left the video dir split in two. rmdir, and a node:test over all four cases (merge, same-name collision, source-only, dry run) that fails 2 of 4 against the old call. **A channel's `ytdlpExtraArgs` could make a window write metadata.** It is free text an operator typed into a form, appended to every invocation and validated only as strings, so a channel carrying --write-info-json would put an undownloaded video into the index through the one path that must never do it. The six sidecar refusals now come AFTER the operator's args and `-o` after those, because yt-dlp takes the last occurrence of an option — ordering is the guard, and the e2e asserts the ordering, not just the absence of the file. **The sweep button treated "already cached" as a failure.** A generous window routinely covers the next clip, so the route's 409 for that case is the common outcome, not a refusal: the run aborted on its own success. It is counted and skipped when the body carries the cached pads; only the running-job 409 stops the sweep. A third fixture (g01's padded window contains g02's) pins it. **The editor stub's port was outside the queue lock's preflight.** `PORT+1` is now `EDITOR_STUB_PORT`, in PORT_BASES, in the --ports spec and in the table. Nits taken while there: the slug goes through `isValidChannelSlug` rather than a second grammar; the sweep's success line reads a local flag instead of a stale `failed` closure, and a vanished job is a failure rather than a success; the fetched argv stays in the sidecar and off the HTTP envelope; the span cap is re-checked in the action so a hand-edited replay spec cannot bypass it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Diffstat:
15 files changed, 456 insertions(+), 31 deletions(-)
diff --git a/WORKTREES.md b/WORKTREES.md @@ -28,6 +28,7 @@ defaults — nothing changes for the primary checkout. | `HOMEPAGE_E2E_PORT` | 3040 | homepage's own Playwright suite | | `UMTOOL_PORT` | 3050 | um-clip triage tool real dev (`pnpm dev:umtool`) | | `UMTOOL_E2E_PORT` | 3051 | umtool's own Playwright suite | +| `EDITOR_STUB_PORT` | 3052 | stub editor the umtool e2e suite fetches clips from | | `PLAYWRIGHT_BASE_URL` | `http://localhost:3011` | node-side fetches in specs | So worktree #1 runs editor on **3101**, test server on **3111**, export on **3110**, etc. diff --git a/common/controller/reconcileVideoDirs.test.ts b/common/controller/reconcileVideoDirs.test.ts @@ -0,0 +1,166 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import path from "node:path"; +import os from "node:os"; +import { mkdtemp, mkdir, readdir, readFile, rm, writeFile } from "node:fs/promises"; +import { reconcileVideoDirs } from "./reconcileVideoDirs"; + +// Reconciling a video dir whose name is not its canonical id, when BOTH sides +// hold a `clips/`. +// +// This case has its own test because `clips/` is the first DIRECTORY the merge +// ever had to move, and the merge is written for files. The first version +// removed the emptied source with rm() and no `recursive`, which throws EISDIR +// on a directory; the catch swallowed it, the source `clips/` survived, and the +// rmdir(srcDir) that follows then failed ENOTEMPTY — so a merge that had in +// fact worked was reported as a "source dir not empty" conflict and the whole +// video dir was left split in two. + +const VIDEO_URL = "https://www.youtube.com/watch?v=canonical01"; + +async function makeChannel(): Promise<string> { + const root = await mkdtemp(path.join(os.tmpdir(), "reconcile-clips-")); + const dataDir = path.join(root, "data"); + await mkdir(dataDir, { recursive: true }); + return root; +} + +// A video dir whose name differs from the canonical id in its metadata, so +// reconcile wants to merge it into `canonical01`. +async function writeStrayDir( + channelDir: string, + name: string, + files: Record<string, string>, +): Promise<string> { + const dir = path.join(channelDir, "data", name); + await mkdir(dir, { recursive: true }); + await writeFile( + path.join(dir, "metadata.info.json"), + JSON.stringify({ webpage_url: VIDEO_URL }), + ); + for (const [rel, body] of Object.entries(files)) { + const full = path.join(dir, rel); + await mkdir(path.dirname(full), { recursive: true }); + await writeFile(full, body); + } + return dir; +} + +test("clips/ on both sides MERGES, and the source dir is actually removed", async (t) => { + const channelDir = await makeChannel(); + t.after(() => rm(channelDir, { recursive: true, force: true })); + + await writeStrayDir(channelDir, "canonical01", { + "clips/0.00-10.00.mp4": "destination window", + "clips/0.00-10.00.json": '{"requestedBy":"umtool"}', + }); + await writeStrayDir(channelDir, "v-canonical01", { + "clips/20.00-30.00.mp4": "source window", + "audio.mp3": "source audio", + }); + + const result = await reconcileVideoDirs({ channelDir }); + + // A MERGE, not a conflict — the whole point. + assert.deepEqual(result.conflicts, []); + assert.equal(result.merged.length, 1); + assert.equal(result.merged[0].from, "v-canonical01"); + assert.equal(result.merged[0].to, "canonical01"); + // The window is reported under its path inside clips/, not as a bare name. + assert.ok( + result.merged[0].movedFiles.includes(path.join("clips", "20.00-30.00.mp4")), + JSON.stringify(result.merged[0].movedFiles), + ); + + // The source dir is gone, which is what the EISDIR bug prevented. + const dirs = await readdir(path.join(channelDir, "data")); + assert.deepEqual(dirs.sort(), ["canonical01"]); + + // BOTH halves of the cache survive: one directory, both windows. + const clips = await readdir(path.join(channelDir, "data", "canonical01", "clips")); + assert.deepEqual(clips.sort(), [ + "0.00-10.00.json", + "0.00-10.00.mp4", + "20.00-30.00.mp4", + ]); +}); + +test("a window present on both sides keeps the source bytes and stashes the other", async (t) => { + const channelDir = await makeChannel(); + t.after(() => rm(channelDir, { recursive: true, force: true })); + + await writeStrayDir(channelDir, "canonical01", { + "clips/5.00-15.00.mp4": "destination bytes", + }); + await writeStrayDir(channelDir, "v-canonical01", { + "clips/5.00-15.00.mp4": "source bytes", + }); + + const result = await reconcileVideoDirs({ channelDir }); + assert.deepEqual(result.conflicts, []); + assert.equal(result.merged.length, 1); + + const clipsDir = path.join(channelDir, "data", "canonical01", "clips"); + const clips = await readdir(clipsDir); + // The same rule the file case uses: the source's bytes win, the canonical + // copy is stashed rather than destroyed. + assert.deepEqual(clips.sort(), [ + "5.00-15.00.mp4", + "5.00-15.00.mp4.dup-v-canonical01", + ]); + assert.equal( + await readFile(path.join(clipsDir, "5.00-15.00.mp4"), "utf8"), + "source bytes", + ); + assert.equal( + await readFile( + path.join(clipsDir, "5.00-15.00.mp4.dup-v-canonical01"), + "utf8", + ), + "destination bytes", + ); + assert.deepEqual(await readdir(path.join(channelDir, "data")), ["canonical01"]); +}); + +test("clips/ only on the source side moves whole, as any other entry would", async (t) => { + const channelDir = await makeChannel(); + t.after(() => rm(channelDir, { recursive: true, force: true })); + + await writeStrayDir(channelDir, "canonical01", { "audio.mp3": "dst" }); + await writeStrayDir(channelDir, "v-canonical01", { + "clips/1.00-2.00.mp4": "only source has windows", + }); + + const result = await reconcileVideoDirs({ channelDir }); + assert.deepEqual(result.conflicts, []); + assert.equal(result.merged.length, 1); + assert.deepEqual( + await readdir(path.join(channelDir, "data", "canonical01", "clips")), + ["1.00-2.00.mp4"], + ); + assert.deepEqual(await readdir(path.join(channelDir, "data")), ["canonical01"]); +}); + +test("a dry run reports the same merge and moves nothing", async (t) => { + const channelDir = await makeChannel(); + t.after(() => rm(channelDir, { recursive: true, force: true })); + + await writeStrayDir(channelDir, "canonical01", { + "clips/0.00-10.00.mp4": "dst", + }); + await writeStrayDir(channelDir, "v-canonical01", { + "clips/20.00-30.00.mp4": "src", + }); + + const result = await reconcileVideoDirs({ channelDir, dryRun: true }); + assert.deepEqual(result.conflicts, []); + assert.equal(result.merged.length, 1); + assert.deepEqual( + (await readdir(path.join(channelDir, "data"))).sort(), + ["canonical01", "v-canonical01"], + ); + assert.deepEqual( + await readdir(path.join(channelDir, "data", "v-canonical01", "clips")), + ["20.00-30.00.mp4"], + ); +}); diff --git a/common/controller/reconcileVideoDirs.ts b/common/controller/reconcileVideoDirs.ts @@ -1,5 +1,5 @@ import path from "node:path"; -import { readdir, readFile, rename, rm, rmdir, stat } from "node:fs/promises"; +import { readdir, readFile, rename, rmdir, stat } from "node:fs/promises"; import { extractVideoId } from "../ytdlp/runYtdlp"; import { CLIPS_DIR_NAME } from "../lib/clipWindow"; @@ -89,7 +89,14 @@ async function mergeDir( if (f === CLIPS_DIR_NAME && dstSet.has(f)) { const inner = await mergeDir(src, path.join(dstDir, f), srcName, dryRun); moved.push(...inner.map((n) => path.join(f, n))); - if (!dryRun) await rm(src, { recursive: false, force: true }).catch(() => {}); + // rmdir, NOT rm: `src` is a DIRECTORY, and rm() without `recursive` + // throws EISDIR on one. The catch swallowed it, the emptied source + // `clips/` survived, and the rmdir(srcDir) below then failed ENOTEMPTY — + // so every clips-on-both-sides reconcile reported a false "source dir not + // empty" conflict and left the whole video dir unmerged. Still tolerant: + // a stray file the recursion could not move is a reason to leave the dir + // for the conflict path, not to delete it. + if (!dryRun) await rmdir(src).catch(() => {}); continue; } if (!dstSet.has(f)) { diff --git a/common/ytdlp/fetchWindowManaged.ts b/common/ytdlp/fetchWindowManaged.ts @@ -118,6 +118,16 @@ export type FetchWindowResult = { provenance: ClipProvenance | null; }; +// The argv a window was fetched with stays in the SIDECAR and does not ride +// back out over HTTP: it is a debugging record for whoever is standing at the +// disk, and it names this instance's cookie browser and the operator's own +// extra args. A caller asking "is this cached" does not need either. +function withoutArgs(p: ClipProvenance | null): ClipProvenance | null { + if (!p?.ytdlp) return p; + const { ytdlp: _ytdlp, ...rest } = p; + return rest; +} + function cachedResult(hit: ClipWindow): FetchWindowResult { return { file: hit.path, @@ -125,7 +135,7 @@ function cachedResult(hit: ClipWindow): FetchWindowResult { to: hit.to, bytes: hit.bytes, cached: true, - provenance: hit.provenance, + provenance: withoutArgs(hit.provenance), }; } @@ -187,10 +197,31 @@ export async function fetchWindowManaged( clipFormatSelector(maxHeight), "--merge-output-format", "mp4", - "-o", - part, ...FULL_LOG_PROGRESS_ARGS, ...channelExtraArgs(opts.channelConfig, cookies), + // THE NEGATIONS COME AFTER THE CHANNEL'S OWN ARGS, AND -o AFTER THOSE. + // + // `ytdlpExtraArgs` is free text an operator typed into a form; it is + // validated as strings and nothing more. A channel carrying + // `--write-info-json` (or --write-thumbnail / --write-description / + // --write-subs / --download-archive / a second -o) would, appended after + // the argv above, make a WINDOW fetch write metadata.info.json into an + // undownloaded video's dir — and the index keys a video's presence on + // exactly that file (see lib/clipWindow.ts). yt-dlp takes the LAST + // occurrence of an option, so the only reliable place for the refusal is + // after the operator's args, and for the output path likewise. + // + // This does not take the operator's settings away: --limit-rate, + // --proxy, --extractor-args and the rest still apply. It refuses exactly + // the six that would write a sidecar this path must never write. + "--no-write-info-json", + "--no-write-description", + "--no-write-thumbnail", + "--no-write-subs", + "--no-write-auto-subs", + "--no-download-archive", + "-o", + part, "--", opts.videoUrl, ]; @@ -284,6 +315,6 @@ export async function fetchWindowManaged( to, bytes: st.size, cached: false, - provenance, + provenance: withoutArgs(provenance), }; } diff --git a/editor/app/api/media/fetch-window/route.ts b/editor/app/api/media/fetch-window/route.ts @@ -1,5 +1,6 @@ import { NextResponse } from "next/server"; import { authorizeWorkerRequest } from "yt-dlp-transcript-common/lib/workerToken"; +import { isValidChannelSlug } from "yt-dlp-transcript-common/controller/channels"; import { MAX_CLIP_WINDOW_SECONDS } from "yt-dlp-transcript-common/lib/clipWindow"; import { fetchFullSourceAction, @@ -24,11 +25,14 @@ export const dynamic = "force-dynamic"; // // The download PAUSE does not gate it; see fetchWindowAction for why. -// Both land in a filesystem path. Anchored — and `.` / `..` are refused +// A video id lands in a filesystem path. Anchored — and `.` / `..` are refused // separately, because the class allows a dot and "`..`" alone would otherwise -// pass a pattern written to stop traversal. +// pass a pattern written to stop traversal. The SLUG uses the repo's own +// isValidChannelSlug (controller/channels.ts) rather than a second grammar +// here: a slug IS a directory name, and two definitions of what one may +// contain is one more than the corpus can have. const ID_RE = /^[\w.-]+$/; -const isPathSegment = (v: string): boolean => +const isVideoId = (v: string): boolean => ID_RE.test(v) && v !== "." && v !== ".."; type Body = { @@ -103,13 +107,13 @@ export async function POST(request: Request) { const channelSlug = str(body.channelSlug); const videoId = str(body.videoId); - if (!channelSlug || !isPathSegment(channelSlug)) { + if (!isValidChannelSlug(channelSlug)) { return NextResponse.json( - { error: "channelSlug is required and must match /^[\\w.-]+$/" }, + { error: "channelSlug is required and must be a valid channel slug" }, { status: 400 }, ); } - if (!videoId || !isPathSegment(videoId)) { + if (!videoId || !isVideoId(videoId)) { return NextResponse.json( { error: "videoId is required and must match /^[\\w.-]+$/" }, { status: 400 }, diff --git a/editor/app/channels/[slug]/videos/[id]/videoActions.ts b/editor/app/channels/[slug]/videos/[id]/videoActions.ts @@ -42,7 +42,10 @@ import { savedVideoPath, type SavedVideoOrigin, } from "yt-dlp-transcript-common/lib/savedVideo"; -import type { ClipProvenance } from "yt-dlp-transcript-common/lib/clipWindow"; +import { + MAX_CLIP_WINDOW_SECONDS, + type ClipProvenance, +} from "yt-dlp-transcript-common/lib/clipWindow"; import { clipWindowPath, findContainingClipWindow, @@ -772,6 +775,25 @@ export async function fetchWindowAction(req: { stream?: boolean; }): Promise<FetchMediaOutcome> { const { slug, videoId, from, to } = req; + // THE CAP IS CHECKED HERE TOO, not only at the HTTP door. Retry re-runs from + // a stored JobSpec, and a spec is a JSON file on disk — a hand-edited one + // must not be able to ask for an hour of source through a path the route + // already refused. + if ( + !Number.isFinite(from) || + !Number.isFinite(to) || + from < 0 || + from >= to || + to - from > MAX_CLIP_WINDOW_SECONDS + ) { + return { + ok: false, + status: 400, + error: + `${from}–${to} is not a fetchable window ` + + `(at most ${MAX_CLIP_WINDOW_SECONDS}s, from < to, from >= 0).`, + }; + } const r = await loadConfigOrError(slug); if (!r.ok) return { ok: false, status: 404, error: r.error }; const paths = getPaths(); diff --git a/editor/e2e/fetch-window.spec.ts b/editor/e2e/fetch-window.spec.ts @@ -11,7 +11,13 @@ import { readFile, stat } from "node:fs/promises"; import { test, expect, type APIRequestContext } from "@playwright/test"; -import { generateReport, resetData, resolvePath, writeSettings } from "./helpers"; +import { + generateReport, + resetData, + resolvePath, + writeChannelConfig, + writeSettings, +} from "./helpers"; import { baseUrl } from "./baseUrl"; const TOKEN = "test-worker-token"; @@ -210,6 +216,60 @@ test("a fetched window lands in clips/, carries its provenance, and leaves the v expect((await invocations()).length).toBe(before); }); +test("a channel's own yt-dlp args cannot make a window write metadata", async ({ + request, +}) => { + test.setTimeout(60_000); + // THE ADVERSARIAL CASE FOR THE LOAD-BEARING RULE. `ytdlpExtraArgs` is free + // text an operator typed into a form, appended to every invocation. A + // channel carrying --write-info-json would, unguarded, make a WINDOW fetch + // write metadata.info.json into a video that has never been downloaded — and + // the index keys a video's presence on exactly that file. + await writeChannelConfig(SLUG, { + url: "https://www.youtube.com/@example/videos", + ytdlpExtraArgs: [ + "--write-info-json", + "--write-thumbnail", + "--write-description", + ], + }); + + const post = await request.post(`${baseUrl}/api/media/fetch-window`, { + headers: AUTH, + data: { + channelSlug: SLUG, + videoId: VIDEO, + from: 50, + to: 70, + requestedBy: "umtool", + manifest: "demo-report", + clipId: "c09", + reason: "the channel is configured to write sidecars", + }, + }); + expect(post.status()).toBe(202); + const { jobId } = (await post.json()) as { jobId: string }; + const finished = await pollJob(request, jobId); + expect(finished.status, JSON.stringify(finished)).toBe("done"); + + expect(await exists(clipRel("50.00-70.00.mp4"))).toBe(true); + // STILL no metadata, so still not in the index. + expect(await exists(rel(`data/${VIDEO}/metadata.info.json`))).toBe(false); + + // yt-dlp takes the LAST occurrence of an option, so the refusals have to come + // after the operator's args — and the output path after those. + const inv = await invocations(); + const line = inv.split("\n").find((l) => l.includes("download-sections:*50.00-70.00")); + expect(line, inv).toBeTruthy(); + const writeAt = line!.indexOf("--write-info-json"); + const refuseAt = line!.indexOf("--no-write-info-json"); + expect(writeAt).toBeGreaterThan(-1); + expect(refuseAt).toBeGreaterThan(writeAt); + expect(line).toContain("--no-write-thumbnail"); + expect(line).toContain("--no-write-description"); + expect(line).toContain("--no-download-archive"); +}); + test("a 429 fails the job and puts the platform in cooldown", async ({ request, }) => { diff --git a/editor/e2e/fixtures/bin/fake-ytdlp.mjs b/editor/e2e/fixtures/bin/fake-ytdlp.mjs @@ -507,7 +507,12 @@ async function main() { await appendFile( "fake-ytdlp.invocations", `download-sections:${arg("--download-sections")} dest=${dest} ` + - `fmt=${arg("-f") ?? ""} cookies=${cookieArg()}\n`, + `fmt=${arg("-f") ?? ""} cookies=${cookieArg()} ` + + // THE WHOLE ARGV, in order. A spec has to be able to assert that the + // sidecar refusals come AFTER a channel's own ytdlpExtraArgs, because + // yt-dlp takes the last occurrence of an option and that ordering is + // the entire guard. + `argv=${argv.join(" ")}\n`, ); if (cookieGateBlocked(url)) failCookieGate(url); if (url.toLowerCase().includes("ratelimit")) { diff --git a/plans/FACTS.md b/plans/FACTS.md @@ -4187,7 +4187,21 @@ umtool asks the editor for a clip window instead of running yt-dlp `<name>.dup-<srcName>`. It now special-cases `clips` and merges the two subdirectories, because stashing the directory would hide every window in it from `listClipWindows` (which reads `clips/` and nothing else). A file present - in both is the same bytes — a window's name IS its span. + in both is the same bytes — a window's name IS its span. **Remove the emptied + source with `rmdir`, never `rm` without `recursive`**: the latter throws + EISDIR on a directory, the catch swallows it, and the `rmdir(srcDir)` that + follows then fails ENOTEMPTY — so a merge that worked is reported as a + "source dir not empty" conflict and the video dir is left split in two. + `controller/reconcileVideoDirs.test.ts` pins all four cases. +- **A channel's `ytdlpExtraArgs` can defeat the no-info-json rule, so the + window fetch refuses the sidecars explicitly.** That field is free text an + operator typed into a form, validated only as strings, and appended to every + invocation — a channel carrying `--write-info-json` would make a window fetch + write one. `fetchWindowManaged` appends `--no-write-info-json + --no-write-description --no-write-thumbnail --no-write-subs + --no-write-auto-subs --no-download-archive` AFTER `channelExtraArgs(...)`, and + `-o` after those, because yt-dlp takes the LAST occurrence of an option. + Ordering is the guard; `editor/e2e/fetch-window.spec.ts` asserts it. - **`umtool/lib/paths.mjs` READ_ROOTS now includes `CHANNELS_DIR`, and WRITE_ROOTS deliberately does not.** `/api/report/raw` resolves the file it serves through `resolveInRoots`, so without the read root a corpus window @@ -4211,6 +4225,14 @@ umtool asks the editor for a clip window instead of running yt-dlp `common/lib/clipWindow.ts` and `umtool/report-to-video/build-video.mjs` both declare them. A request that rounds differently addresses a different file and the cache misses forever. +- **`clips/` HAS NO GARBAGE COLLECTION, and its bytes are counted by nothing.** + Nothing prunes a window once it is fetched (the retention sweep is + pointer-driven and `pruneSavedVideos` never sees it), and the snapshot's + `totalMediaBytes` walk is flat over a video dir, so a `clips/` subdirectory + adds to the disk without adding to the number the operator reads. Deliberate + for now — a window is seconds, not a recording — but a channel walked many + times by many reports will accumulate them silently. A later sweep owes both: + a count on the storage page and an eviction rule. - **`SavedVideoPointer.origin` is separate from `keepReason` on purpose.** `keepReason` stays `override`/`pin` so `pruneSavedVideos` never evicts a container somebody asked for; `origin` is who asked. `parseSavedVideoPointer` diff --git a/scripts/worktree.mjs b/scripts/worktree.mjs @@ -24,6 +24,7 @@ const PORT_BASES = { HOMEPAGE_E2E_PORT: 3040, // homepage's own Playwright suite UMTOOL_PORT: 3050, // um-clip triage tool real dev UMTOOL_E2E_PORT: 3051, // umtool's own Playwright suite + EDITOR_STUB_PORT: 3052, // stub editor the umtool e2e suite fetches clips from }; const OFFSET_STEP = 100; diff --git a/umtool/components/projects/FetchUnfetchedButton.tsx b/umtool/components/projects/FetchUnfetchedButton.tsx @@ -47,6 +47,10 @@ export default function FetchUnfetchedButton({ // POLLED BY ID, not by "whatever is running". A bare GET answers with // `runningJob()`, which is null the moment the job ends — so a fetch that // FAILED between two polls would read as nothing running, i.e. as success. + // + // A null job for an id we were GIVEN is not "done" either: the runner has + // forgotten a job it told us about, which is a state nobody can call a + // success. Returned as null and treated as a failure by the caller. async function poll(id: string): Promise<JobView | null> { for (;;) { await new Promise((r) => setTimeout(r, 1000)); @@ -66,6 +70,10 @@ export default function FetchUnfetchedButton({ setDone(0); setMsg(null); let i = 0; + // A LOCAL FLAG, not the `failed` state: `run` closes over the value + // `failed` had when this render was made, so reading it after setFailed(true) + // reads `false` and the success line prints over a run that stopped. + let ok = true; for (const clip of pending) { if (stop.current) { setMsg(`stopped after ${i} of ${total}`); @@ -85,36 +93,56 @@ export default function FetchUnfetchedButton({ const body = (await res.json().catch(() => ({}))) as { error?: string; job?: JobView; + cachedBefore?: number; + cachedAfter?: number; + cachedPad?: number; }; if (res.status === 409) { - // The route's own words, not a paraphrase: "already cached to −N s" - // and "a job is already running (build)" are different problems and - // only one of them is this button's to solve. + // TWO DIFFERENT 409s, and only one of them is a problem. + // + // "already cached to −N s / +N s" means the window arrived while this + // sweep was running — which is the COMMON case here, because a + // generous fetch for one clip routinely covers the next one in the + // list. Treating it as a failure aborted the run on its own success. + // The route marks that case with the cached pads; everything else + // (notably "a job is already running") stops the sweep in the route's + // own words. + const alreadyCached = + body.cachedBefore !== undefined || body.cachedPad !== undefined; + if (alreadyCached) { + i += 1; + setDone(i); + continue; + } + ok = false; setFailed(true); setMsg(body.error ?? "refused"); break; } if (!res.ok) { + ok = false; setFailed(true); setMsg(body.error ?? `HTTP ${res.status}`); break; } if (!body.job?.id) { + ok = false; setFailed(true); setMsg("the fetch route started no job"); break; } const finished = await poll(body.job.id); - if (finished?.state === "failed") { + if (!finished || finished.state === "failed") { + ok = false; setFailed(true); - setMsg(`${clip.id}: ${finished.error ?? "fetch failed"}`); + setMsg(`${clip.id}: ${finished?.error ?? "fetch failed"}`); break; } i += 1; setDone(i); } setRunning(false); - if (!failed && !stop.current && i === total) { + if (ok && !stop.current && i === total) { setMsg(`fetched ${total} clip${total === 1 ? "" : "s"} — reload to see them`); } } diff --git a/umtool/e2e/fixtures/make-fixture.mjs b/umtool/e2e/fixtures/make-fixture.mjs @@ -682,6 +682,19 @@ const CUES = { [3, 6, "It has a second clip to fetch."], [6, 9, "And nothing else cites it."], ], + // Long enough that one clip's PADDED window can contain another's. See + // editor-fetch-reuse-fixture. + vid5: [ + [0, 3, "The reuse fixture begins here."], + [3, 6, "It runs on for a while."], + [6, 9, "Long enough to hold two clips."], + [9, 12, "One inside the other's pad."], + [12, 15, "Which is the whole point."], + [15, 18, "The second one costs nothing."], + [18, 21, "Because the first already paid."], + [21, 24, "And containment is the predicate."], + [24, 27, "That is where it ends."], + ], }; for (const [vid, rows] of Object.entries(CUES)) { const dir = path.join(CHANNELS, "testchan", "data", vid); @@ -892,8 +905,9 @@ const WALK = writeProject( // -- THE EDITOR-FETCH FIXTURE ------------------------------------------------- // -// Two clips on vid1 and NOTHING in out/clips-raw, so both read "not fetched -// yet" and `ready 0 of 2`. That is the state the editor fetch exists to leave: +// One clip on vid3 — a source nothing else cites — and NOTHING in +// out/clips-raw, so it reads "not fetched yet" and `ready 0 of 1`. That is the +// state the editor fetch exists to leave: // after it, the window is in the CORPUS (channels/testchan/data/vid1/clips/) // rather than in this project's own out/ — which is the whole argument, since // the next report citing vid1 gets it for free. @@ -921,6 +935,24 @@ writeProject( ]), ); +// A SWEEP WHERE THE SECOND CLIP IS ALREADY PAID FOR. +// +// g01 is 5–25 s, so the route's default ±20 s of pad makes it 0.00–45.00. +// g02 is 10–15 s, whose padded ask is [0, 35] — inside that. So a sweep that +// fetches g01 finds g02 already covered and the route answers 409 "already +// cached", which is the COMMON case (a generous window routinely covers its +// neighbour) and must not read as a failure that aborts the run. +// +// Its own source again: the corpus is keyed by video, so this could not share +// one with the fixtures above. +writeProject( + "editor-fetch-reuse-fixture", + manifest("editor-fetch-reuse-fixture", "The Editor Fetch Reuse Fixture", { siteOrigin: "https://archive.example" }, [ + { type: "clip", id: "g01", video: "vid5", start: 5.0, end: 25.0, cite: 5, section: 0, lock: true, quote: "runs on for a while" }, + { type: "clip", id: "g02", video: "vid5", start: 10.0, end: 15.0, cite: 10, section: 0, lock: true, quote: "one inside the other" }, + ]), +); + // -- STUB BINARIES, so a build is offline and deterministic -------------------- // // The pipeline shells out to yt-dlp for the availability preflight and for every @@ -1196,11 +1228,11 @@ console.log(` SONG_CODE_DIR=${path.join(dest, "code")}`); console.log(` SONG_DIR=${path.join(dest, "data")}`); console.log(` SONG_REPORTS_DIR=${reports}`); console.log(` YTDLP_BIN=${path.join(BIN, "yt-dlp")} QRENCODE_BIN=${path.join(BIN, "qrencode")}`); -console.log(` CHANNELS_DIR=${CHANNELS} (testchan/vid1 punctuated, vid2 not; vid3/vid4 for the editor fetch)`); +console.log(` CHANNELS_DIR=${CHANNELS} (testchan/vid1 punctuated, vid2 not; vid3/vid4/vid5 for the editor fetch)`); console.log(` projects: report-fixture (4 clips, 1 mid-sentence), no-origin-fixture,`); console.log(` localhost-fixture, bike-fixture (sweep), find/ (shadowed),`); console.log(` deep/nested/solo-fixture (collapse case), bench-fixture (writable),`); console.log(` walk-fixture (read-only: w01/w04 walkable, w02 unfetched, w03 judged),`); -console.log(` editor-fetch-fixture + editor-fetch-many-fixture (nothing cached — the editor fetch's subjects),`); +console.log(` editor-fetch-{,many-,reuse-}fixture (nothing cached — the editor fetch's subjects),`); console.log(` longform-fixture (cue gap, legacy .bak, ffmeta), longform-edit-fixture, dash-fixture`); console.log(` ${taken} candidate files copied, 2 mix tracks synthesised`); diff --git a/umtool/e2e/report-fetch-via-editor.spec.ts b/umtool/e2e/report-fetch-via-editor.spec.ts @@ -24,6 +24,7 @@ const HERE = path.dirname(fileURLToPath(import.meta.url)); const FIXTURE = path.join(HERE, "..", ".e2e-song"); const PROJECT = "reports/editor-fetch-fixture"; const MANY = "reports/editor-fetch-many-fixture"; +const REUSE = "reports/editor-fetch-reuse-fixture"; const STUB_LOG = path.join(FIXTURE, "editor-stub.requests.json"); const YTDLP_LOG = path.join(FIXTURE, "bin", "yt-dlp.invocations"); const clipsDir = (video: string) => @@ -193,3 +194,46 @@ test("the project page fetches the unfetched clips one at a time, and Stop halts // Leave the runner quiet for the next spec. await waitForJob(request, baseURL!); }); + +test("a clip the previous fetch already covers does not abort the sweep", async ({ + page, + request, + baseURL, +}) => { + test.setTimeout(120_000); + const ytdlpBefore = ytdlpCalls(); + + // g01 is 5–25 s, so ±20 s of pad makes it 0.00–45.00. g02's padded ask is + // [0, 35] — INSIDE that — so the route answers 409 "already cached to −N s" + // rather than queueing a job. + // + // That is the common case, not an edge: a window is deliberately generous + // and the predicate is containment, so a sweep down a timeline routinely + // finds its next clip already paid for. Treating it as a refusal stopped the + // run on its own success and left the counter short. + await page.goto(`/browse/${REUSE}`); + await expect(page.locator("[data-ready-count]")).toContainText("ready 0 of 2"); + await page.getByRole("button", { name: /fetch 2 unfetched clips/i }).click(); + + await expect(page.locator("[data-fetch-unfetched-msg]")).toContainText( + /fetched 2 clips/, + { timeout: 90_000 }, + ); + + // ONE ask on the wire for two clips: the second was never fetched, and never + // needed to be. + const asks = stubRequests().filter( + (a) => a.manifest === "editor-fetch-reuse-fixture", + ); + expect(asks.map((a) => a.clipId)).toEqual(["g01"]); + expect(existsSync(path.join(clipsDir("vid5"), "0.00-45.00.mp4"))).toBe(true); + + await page.reload(); + await expect(page.locator("[data-ready-count]")).toContainText("ready 2 of 2"); + await expect( + page.getByRole("button", { name: /unfetched clip/i }), + ).toHaveCount(0); + + expect(ytdlpCalls()).toBe(ytdlpBefore); + await waitForJob(request, baseURL!); +}); diff --git a/umtool/package.json b/umtool/package.json @@ -8,7 +8,7 @@ "build": "next build", "start": "next start --port ${UMTOOL_PORT:-3050}", "typecheck": "tsc --noEmit", - "e2e": "node ../scripts/queue-lock.mjs --ports UMTOOL_E2E_PORT:3051 -- playwright test" + "e2e": "node ../scripts/queue-lock.mjs --ports UMTOOL_E2E_PORT:3051,EDITOR_STUB_PORT:3052 -- playwright test" }, "dependencies": { "class-variance-authority": "^0.7.1", diff --git a/umtool/playwright.config.ts b/umtool/playwright.config.ts @@ -3,10 +3,12 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; const PORT = Number(process.env.UMTOOL_E2E_PORT ?? 3051); -// The editor stub's port. Derived from the suite's own, so a worktree that gets -// a different port block gets a different stub port too and two checkouts can -// never drive each other's stub. -const STUB_PORT = PORT + 1; +// The editor stub's port. Named, because the queue lock's port PREFLIGHT only +// checks the ports it is given: a bare PORT+1 was outside it, so a second +// checkout's stub could already hold the port and this run would drive it. It +// is in scripts/worktree.mjs PORT_BASES (so a worktree gets its own) and in +// package.json's --ports spec (so the preflight sees it). +const STUB_PORT = Number(process.env.EDITOR_STUB_PORT ?? PORT + 1); // The package is "type": "module", so there is no __dirname here. const FIXTURE = path.join(path.dirname(fileURLToPath(import.meta.url)), ".e2e-song");