commit 01c4633f2b85761816384c9ca975f5b423cdb2e4
parent fe307e8197383af1434b2e8aed38d9fc715b13ef
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Tue, 22 Sep 2026 16:31:11 -0400
ops: a bad site id is a 400, and a poll that recovers is not an outcome
Two ways the last two commits could still lose a job.
reqSiteIds took any non-empty string, and `buildAndDeployAction` reaches
`getSite`, which THROWS on an id failing SITE_ID_RE. ops() maps a plain throw
to a 500, so {"siteIds":["good","BAD"]} queued the first build, threw on the
second and answered 500 carrying no ids — a real build-deploy running that
nothing was watching, and a --wait exiting 1 about it. Site ids are now
validated up front, the same way reqSlug validates a slug and for the same
reason, and each iteration of both loops catches into `skipped` so one site can
never cost the others their job ids.
followJob's absent-from-active branch returned whatever the final log poll
said. A stale active list plus a log endpoint that had come back therefore
printed "running" as the job's result and exited 1 for a job that was fine;
only a terminal status is an outcome now, and anything else goes back to
waiting with the counters reset. The null-probe path — neither endpoint
answering — waited forever without --wait-timeout; ten consecutive silent polls
(~5 minutes at the backoff ceiling) is an editor that is gone, and it says so.
Also: --wait-timeout implies --wait rather than being silently inert, the
backoff resets alongside the failure count, and both CLI entry guards use
pathToFileURL so a repo path needing percent-encoding still runs main().
The autoRunner test now asserts that an injected state does not change the
answer (and is not mutated by the poll) instead of pinning a TypeError.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
8 files changed, 178 insertions(+), 49 deletions(-)
diff --git a/common/controller/autoRunner.test.ts b/common/controller/autoRunner.test.ts
@@ -20,11 +20,7 @@ import {
type FocusSummary,
} from "../lib/channelPriority";
import { LANES } from "../lib/autoQueueTypes";
-import {
- emptyAutoQueueKindState,
- emptyAutoQueueState,
- type AutoQueueState,
-} from "../jobs/autoQueueState";
+import { emptyAutoQueueState } from "../jobs/autoQueueState";
import type {
AutoQueueGroup,
AutoQueuePolicy,
@@ -523,26 +519,27 @@ test("computeLeafPending takes its channel listing from `shared`", async () => {
assert.equal(result.nextUp, null);
});
-test("computeLeafPending reads the injected state, not the file", async () => {
+test("an injected auto-queue state changes nothing about the answer", async () => {
const paths = pendingFixture();
- // readAutoQueueState returns a FULLY POPULATED state even when the file is
- // absent (emptyAutoQueueState), so a state missing this lane can only be the
- // injected one. Dereferencing it is the proof that no second read happened.
- const missingLane = Object.fromEntries(
- LANES.filter((lane) => lane !== LANES[0]).map((lane) => [
- lane,
- emptyAutoQueueKindState(),
- ]),
- ) as unknown as AutoQueueState;
- await assert.rejects(
- computeLeafPending(LANES[0], paths, { configs: [], state: missingLane }),
- TypeError,
- );
- // And with the lane present it goes through, off the same absent file.
- await assert.doesNotReject(
- computeLeafPending(LANES[0], paths, {
- configs: [],
- state: emptyAutoQueueState(),
- }),
- );
+ // The state carries SWRR weights, which only matter once there is work to
+ // pick between — so what `shared.state` must never do is change the result.
+ // It saves a read; it is not an input to the numbers.
+ const state = emptyAutoQueueState();
+ state[LANES[0]].runtime.currentWeights = { "prio-all": 17 };
+ const injected = await computeLeafPending(LANES[0], paths, {
+ configs: [],
+ state,
+ });
+ const read = await computeLeafPending(LANES[0], paths, { configs: [] });
+ assert.deepEqual(injected.counts, read.counts);
+ assert.deepEqual(injected.head, read.head);
+ assert.deepEqual(injected.owner, read.owner);
+ assert.equal(injected.nextUp, read.nextUp);
+
+ // AND THE CALLER'S OBJECT IS NOT TOUCHED. selectNextWork advances fairness by
+ // mutating currentWeights in place, and this runs on a 3-second poll: the
+ // deep clone that stops merely HAVING the page open from skewing a
+ // round-robin group now has to protect a state the caller still holds and
+ // will hand to the next lane.
+ assert.deepEqual(state[LANES[0]].runtime.currentWeights, { "prio-all": 17 });
});
diff --git a/editor/app/api/ops/_lib.ts b/editor/app/api/ops/_lib.ts
@@ -1,6 +1,7 @@
import { NextResponse } from "next/server";
import { authorizeWorkerRequest } from "yt-dlp-transcript-common/lib/workerToken";
import { isValidChannelSlug } from "yt-dlp-transcript-common/controller/channels";
+import { isValidSiteId } from "yt-dlp-transcript-common/lib/site";
import type { StreamActionResult } from "yt-dlp-transcript-common/jobs/streamCommand";
import type { QueueOutcome } from "../../channels/lib/queueForSlugs";
@@ -168,17 +169,35 @@ export function reqStringArray(body: OpsBody, key: string): string[] {
// has two ideas about what it wants, and picking one on its behalf is how a
// deploy of the wrong site becomes somebody's afternoon. Neither key is a 400
// naming both, because "which did you mean" is the actual question.
+//
+// A SITE ID, NOT MERELY A STRING, and checked BEFORE any job starts — the same
+// reason reqSlug exists, with a worse failure behind it. `getSite` throws a
+// plain Error on an id failing SITE_ID_RE and `ops()` maps that to a 500, so
+// ["good", "BAD"] used to queue the first build, throw on the second and answer
+// 500 with no jobs and no ids: a real build running that nothing was watching,
+// and a `--wait` exiting 1 about it.
export function reqSiteIds(body: OpsBody): string[] {
const one = body.siteId;
const many = body.siteIds;
if (one !== undefined && many !== undefined) {
throw new OpsInputError('send either "siteId" or "siteIds", not both');
}
- if (one !== undefined) return [reqString(body, "siteId")];
- if (many !== undefined) return reqStringArray(body, "siteIds");
- throw new OpsInputError(
- '"siteId" (a string) or "siteIds" (a non-empty array of strings) is required',
- );
+ let ids: string[];
+ if (one !== undefined) ids = [reqString(body, "siteId")];
+ else if (many !== undefined) ids = reqStringArray(body, "siteIds");
+ else {
+ throw new OpsInputError(
+ '"siteId" (a string) or "siteIds" (a non-empty array of strings) is required',
+ );
+ }
+ for (const id of ids) {
+ if (!isValidSiteId(id)) {
+ throw new OpsInputError(
+ `"${id}" is not a valid site id (lowercase letters, digits and "-"; must start with a letter or digit)`,
+ );
+ }
+ }
+ return ids;
}
export function oneOf<T extends string>(
diff --git a/editor/app/api/ops/build-deploy/route.ts b/editor/app/api/ops/build-deploy/route.ts
@@ -50,7 +50,18 @@ export async function POST(request: Request) {
const skipped: { siteId: string; reason: string }[] = [];
let info = false;
for (const siteId of reqSiteIds(body)) {
- const result = await buildAndDeployAction(siteId, skipArchives);
+ // ONE SITE'S THROW CANNOT COST THE OTHERS THEIR JOB IDS. An exception
+ // out of here becomes a 500 carrying no `jobs` at all, while the builds
+ // already queued run on with nobody holding their ids. reqSiteIds
+ // rejects the malformed-id case before any job starts; this catches
+ // whatever else the action can raise.
+ let result: Awaited<ReturnType<typeof buildAndDeployAction>>;
+ try {
+ result = await buildAndDeployAction(siteId, skipArchives);
+ } catch (e) {
+ skipped.push({ siteId, reason: (e as Error).message });
+ continue;
+ }
if (!result.ok) {
skipped.push({ siteId, reason: result.error });
info = info || result.info === true;
diff --git a/editor/app/api/ops/build-site/route.ts b/editor/app/api/ops/build-site/route.ts
@@ -42,12 +42,20 @@ export async function POST(request: Request) {
const jobs: { siteId: string; jobId: string }[] = [];
const skipped: { siteId: string; reason: string }[] = [];
for (const siteId of reqSiteIds(body)) {
- const result = await buildExportAction(
- siteId,
- undefined,
- skipData,
- skipArchives,
- );
+ // One site's throw cannot cost the others their job ids — see the same
+ // guard in build-deploy.
+ let result: Awaited<ReturnType<typeof buildExportAction>>;
+ try {
+ result = await buildExportAction(
+ siteId,
+ undefined,
+ skipData,
+ skipArchives,
+ );
+ } catch (e) {
+ skipped.push({ siteId, reason: (e as Error).message });
+ continue;
+ }
if (!result.ok) {
skipped.push({ siteId, reason: result.error });
continue;
diff --git a/editor/e2e/ops-api.spec.ts b/editor/e2e/ops-api.spec.ts
@@ -661,6 +661,13 @@ test("tag-videos refuses a traversing slug, a bad op and an unknown key", async
// different JSON and the only way to learn which was to get a 400. Both now
// accept both keys; sending BOTH is still refused, because a caller with two
// ideas about what to build should not have one picked for it.
+// Job sidecars on disk — the only way to say "and nothing was queued".
+async function listJobIds(): Promise<string[]> {
+ return (await readdir(resolvePath("test-transcripts/.jobs")).catch(() => []))
+ .filter((f) => f.endsWith(".meta.json"))
+ .sort();
+}
+
test("build-site and build-deploy each take siteId or siteIds, and refuse both or neither", async ({
request,
}) => {
@@ -683,6 +690,18 @@ test("build-site and build-deploy each take siteId or siteIds, and refuse both o
const withAll = await ops(request, action, { all: true, siteId: "a" });
expect(withAll.status, action).toBe(400);
expect(withAll.body.error, action).toContain("not both");
+
+ // A MALFORMED ID IS A 400 BEFORE ANY JOB STARTS, and that is the point of
+ // validating in reqSiteIds rather than letting getSite throw: an id list of
+ // ["good", "BAD"] used to queue the first build, throw on the second and
+ // answer 500 with no ids at all — a real build running that nothing was
+ // watching, and a --wait exiting 1 about it.
+ const before = await listJobIds();
+ const bad = await ops(request, action, { siteIds: ["buildsite", "BAD ID"] });
+ expect(bad.status, action).toBe(400);
+ expect(bad.body.error, action).toContain("not a valid site id");
+ expect(bad.body.jobs, action).toBeUndefined();
+ expect(await listJobIds(), action).toEqual(before);
}
});
diff --git a/scripts/archilyzer-ops.mjs b/scripts/archilyzer-ops.mjs
@@ -53,9 +53,16 @@
// stderr), so `pnpm ops … | jq` works.
import { readFile } from "node:fs/promises";
+import { pathToFileURL } from "node:url";
const DEFAULT_URL = "http://localhost:3001";
+// Consecutive polls where NEITHER the job's log NOR the active list answered,
+// after which --wait gives up. At the 30 s backoff ceiling that is ~5 minutes
+// of an editor saying nothing at all, which is not a busy server — it is a
+// server that is gone.
+const MAX_PROBE_FAILURES = 10;
+
// The read-side routes, reachable as `get <noun> <arg>`. Kept tiny and explicit:
// an ops API that let a caller assemble arbitrary GET paths would be a proxy,
// not an adapter.
@@ -116,6 +123,9 @@ export function parseArgs(argv) {
return { error: "--wait-timeout needs a positive number of seconds" };
}
waitTimeout = n;
+ // A timeout on a wait nobody asked for is not a preference, it is a typo
+ // with no effect — so it IMPLIES --wait rather than being ignored.
+ wait = true;
return null;
};
for (let i = 0; i < argv.length; i++) {
@@ -294,6 +304,7 @@ export async function followJob(jobId, quiet, opts = {}) {
let from = 0;
let failures = 0;
+ let probeFailures = 0;
let backoff = 1000;
// One log poll. Returns the status, or null when the poll itself failed —
@@ -325,28 +336,56 @@ export async function followJob(jobId, quiet, opts = {}) {
}
};
+ // "queued" and "running" are the two NON-answers. Everything else is the job
+ // having ended, which is the only thing worth returning.
+ const terminal = (s) => s !== null && s !== "queued" && s !== "running";
+
for (;;) {
const status = await pollLog();
if (status !== null) {
failures = 0;
+ probeFailures = 0;
backoff = 1000;
- if (status !== "queued" && status !== "running") return status;
+ if (terminal(status)) return status;
} else {
failures++;
if (failures >= 3) {
const listed = await stillListed();
if (listed === false) {
// Gone from the active list: it went terminal while the log endpoint
- // was unreachable. One more try at the status it ended with.
+ // was unreachable. One more try at the status it ended with — and
+ // ONLY a terminal one is an outcome. A log endpoint that came back
+ // answering "running" means the active list was stale, not that the
+ // job finished; returning that printed "running" as the result and
+ // exited 1 for a job that was fine.
const final = await pollLog();
- if (final !== null) return final;
- throw new Error(
- `lost contact with job ${jobId}: it is no longer active and its log could not be read`,
- );
+ if (terminal(final)) return final;
+ if (final === null) {
+ throw new Error(
+ `lost contact with job ${jobId}: it is no longer active and its log could not be read`,
+ );
+ }
+ // Readable again and still going: back to waiting, from scratch.
+ failures = 0;
+ probeFailures = 0;
+ backoff = 1000;
+ } else if (listed === true) {
+ // A job we can still see is a job to wait for.
+ failures = 0;
+ probeFailures = 0;
+ backoff = 1000;
+ } else {
+ // The probe failed too, so we now know nothing at all. Without a
+ // bound this waits forever on an editor that has gone away; with one
+ // it says so. Only CONSECUTIVE failures count — a single answer of
+ // either kind resets it.
+ probeFailures++;
+ if (probeFailures >= MAX_PROBE_FAILURES) {
+ throw new Error(
+ `lost contact with the editor at ${base}: ${MAX_PROBE_FAILURES} consecutive failed polls of job ${jobId} and of /api/jobs/active`,
+ );
+ }
}
- // Listed (or the probe failed too) — keep waiting. A job we can still
- // see is a job to wait for.
- if (listed === true) failures = 0;
}
backoff = Math.min(backoff * 2, 30_000);
}
@@ -449,7 +488,7 @@ async function main() {
}
// Importable for the arg-parsing tests; only the CLI entry point runs main().
-if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) {
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
main().then(
(code) => process.exit(code),
(e) => {
diff --git a/scripts/archilyzer-ops.test.mjs b/scripts/archilyzer-ops.test.mjs
@@ -218,3 +218,38 @@ test("--wait-timeout gives up on a job that never ends", async () => {
test("usage says both build routes take siteId or siteIds", () => {
assert.match(usage(), /build-site and build-deploy both take "siteId".*"siteIds"/);
});
+
+test("a recovered log endpoint answering 'running' is not an outcome", async () => {
+ // THE BUG: the absent-from-active branch returned whatever the final poll
+ // said. A stale active list plus a recovered log endpoint therefore printed
+ // "running" as the job's result and exited 1 for a job that was fine.
+ const boom = new Error("fetch failed");
+ const editor = fakeEditor({
+ log: [
+ boom,
+ boom,
+ boom,
+ { content: "", nextOffset: 0, status: "running" },
+ { content: "", nextOffset: 0, status: "done" },
+ ],
+ active: [[]],
+ });
+ assert.equal(await follow("j1", editor), "done");
+});
+
+test("an editor that answers nothing at all is given up on, not waited on", async () => {
+ // Without --wait-timeout the null-probe path used to wait forever: the log
+ // never answers AND the active list never answers, so nothing ever resets
+ // the failure count and nothing ever concludes.
+ const boom = new Error("fetch failed");
+ const editor = fakeEditor({ log: [boom], active: [boom] });
+ await assert.rejects(follow("j1", editor), /lost contact with the editor/);
+});
+
+test("--wait-timeout implies --wait", () => {
+ // A timeout on a wait nobody asked for is a typo with no effect, not a
+ // preference to honour silently.
+ const p = parseArgs(["build-index", "--wait-timeout", "30"]);
+ assert.equal(p.wait, true);
+ assert.equal(p.waitTimeout, 30);
+});
diff --git a/scripts/worktree.mjs b/scripts/worktree.mjs
@@ -9,6 +9,7 @@
import { execFileSync, spawn } from "node:child_process";
import fs from "node:fs";
import path from "node:path";
+import { pathToFileURL } from "node:url";
// Base ports (offset 0 == main worktree). Mirrors the hardcoded defaults in
// editor/export package.json scripts and the Playwright configs.
@@ -346,7 +347,7 @@ async function main() {
// Importable for the unit tests (worktreeDirFor is pure and needs no repo);
// only the CLI entry point runs main(), which shells out to git.
-if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) {
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
main()
.then((code) => process.exit(code ?? 0))
.catch((err) => {
