commit 1b9d3716ed34696c7345bdbc1b23b5077f45279d
parent 9dc0cec1a1330088c0ad6f306761df3aa7745123
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Wed, 26 Aug 2026 00:01:13 -0400
arbiter: resolve every operation's lane through its own laneFor
laneForOperation special-cased digest — an `if (operation === DIGEST_KIND_ID)`
calling digestLaneFor, then `.lane` for everything else. So the rule lived in
two places and only digest's copy was live: DIARIZATION ALREADY DECLARED A
laneFor and the arbiter never called it. A sortformer/vulkan diarization
reserved as `contendsFor: "cpu"` when it was in fact holding ~4.4 GB of the same
8 GB card transcription wants.
Now: `getBackfillKind(op)?.laneFor?.(getSettings()) ?? .lane`. One resolution,
off the kind's own declaration, and digest gains the laneFor it should have had
— its lane is a configuration choice (local GPU queue vs metered network
queue), which is exactly what `lane` alone cannot carry.
Correctness-preserving on both branches: diarizationLaneFor returns
BACKFILL_QUEUE either way (only contendsFor varies), and the arbiter reserves on
unit.lane.queueKey, so no job moves queues. What changes is that the yield
decision now sees the live answer.
getBackfillKind, NOT operationCatalog(), and the new test says why: today
getBackfillKind("download") is undefined, so download and transcription get no
lane and planArbiterUnits skips them. A catalog lookup would hand them a lane
and the arbiter would start a backfill channel job for work no backfill kind can
do. arbiter.test.ts only pinned the UNKNOWN-id case, which is a different
failure.
THREE BOUNDARIES, written where they bite:
- laneFor STAYS OPTIONAL. backfillBatch.ts reads its PRESENCE as the marker for
"this kind's resource depends on settings", so "give every kind a laneFor
defaulting to lane" would not be a no-op — it would enrol every kind in the
idle-only rule and revive the exact regression that comment records. Said on
the field declaration and at the guard.
- Digest declaring one does NOT put it in that guard, and not because of the
guard: resolveBackfillKinds is filtered through laneBackfillKinds
(BACKFILL_QUEUE only), so digest can never reach `kinds`. The stale
"which today is diarization alone" is replaced with this.
- Declared-lane readers keep reading `.lane` — laneBackfillKinds,
laneKindEntriesOf, editor lanes.ts. They partition a snapshot's key set by the
declared key; a live answer there would drift the set from whatever wrote it.
backfillKinds.test.ts's `.lane` assertions pass untouched, which is the check.
Also tightened arbiter.ts's `if (!lane) continue` comment: it claimed a
switched-off kind has no lane. It does have one — getBackfillKind has never
consulted `enabled`. What keeps a disabled feature out is the `enabled` set in
runArbiterPass, which projects an empty id list. New test pins that boundary so
the redundant gate does not get "fixed" in.
common 826/826 (new controller/laneForOperation.test.ts, 4 cases, against the
real resolver — arbiter.test.ts passes laneOf in and would answer these
vacuously). tsc clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Diffstat:
4 files changed, 180 insertions(+), 26 deletions(-)
diff --git a/common/controller/arbiter.ts b/common/controller/arbiter.ts
@@ -51,7 +51,6 @@ import {
operationLabel,
type BackfillLane,
} from "../lib/backfillKinds";
-import { digestLaneFor } from "../lib/backfillKinds";
import { runDigestChannelJob } from "./digestSweep";
import { runBackfillChannelJob } from "./backfillSweep";
import {
@@ -108,20 +107,26 @@ export type ArbiterUnit = {
leafId: string;
};
-// The lane an operation runs on, or null when the catalog does not know it.
+// The lane an operation runs on, or null when the registry does not know it.
//
-// Digest is asked through digestLaneFor because its lane depends on which
-// ENGINE is configured — the local lane contends for the GPU and yields, the
-// metered one contends for nothing local and must not. Every other operation's
-// lane is a fixed declaration.
+// ONE RESOLUTION, off the kind's own declaration. A kind whose lane depends on
+// configuration says so with laneFor() — digest picks between the GPU-bound
+// local queue and the metered network one, diarization between contending for
+// the GPU and contending for cores — and a kind without one has a fixed lane.
+// This used to special-case digest here, which meant the rule lived in two
+// places and only digest's copy was live: diarization's laneFor was invisible
+// to the arbiter.
+//
+// getBackfillKind, NOT operationCatalog(), and that is deliberate. Today
+// getBackfillKind("download") is undefined, so download and transcription get
+// no lane and planArbiterUnits skips them (see the `if (!lane) continue` below)
+// — which is correct, because they are dispatched by their own runners. A
+// catalog lookup would hand them a lane and the arbiter would start a backfill
+// channel job for work no backfill kind can do.
export function laneForOperation(operation: string): BackfillLane | null {
- if (operation === DIGEST_KIND_ID) {
- const settings = getSettings();
- return digestLaneFor(
- settings.digest.remoteEnabled ? "remote-api" : "local-gpu",
- );
- }
- return getBackfillKind(operation)?.lane ?? null;
+ const kind = getBackfillKind(operation);
+ if (!kind) return null;
+ return kind.laneFor?.(getSettings()) ?? kind.lane;
}
// Turn the policy trees into dispatchable units, in priority order.
@@ -157,10 +162,16 @@ export function planArbiterUnits(
const ids = pending[leaf.id] ?? [];
if (ids.length === 0) continue;
const lane = laneOf(operation);
- // An operation the catalog does not know, or one whose feature is off,
- // has no lane — so it is not dispatched. Silently skipping is right:
- // a snapshot may name a kind that has since been renamed, and a tree may
- // name one an operator later switched off.
+ // An operation the REGISTRY does not know has no lane, so it is not
+ // dispatched. Silently skipping is right: a snapshot may name a kind that
+ // has since been renamed, and download/transcription are catalogued
+ // (digest.dependsOn needs them to be) but dispatched by their own runners.
+ //
+ // A switched-off kind does not reach here at all, and not through this
+ // branch: `enabled` below projects ids only for kinds allBackfillKinds
+ // returns, so a disabled feature arrives with an empty id list and is
+ // skipped one line up. laneForOperation itself does not consult
+ // `enabled` — see controller/laneForOperation.test.ts.
if (!lane) continue;
// Group by channel, PRESERVING the order the ids arrived in, so the first
// channel out is the one holding the highest-priority video.
diff --git a/common/controller/backfillBatch.ts b/common/controller/backfillBatch.ts
@@ -747,14 +747,26 @@ export async function runBackfillBatch(
// direction to be wrong in, since the alternative is an OOM mid-sweep.
//
// ONLY kinds that declare laneFor are considered, and that is the fix for a
- // real regression rather than a nicety. `digest` declares `contendsFor:
- // "gpu"` statically, so testing every kind's lane made this lane idle-only
- // whenever a digest was in the run — which broke the guarantee that a digest
- // runs CONCURRENTLY with a backfill rather than behind it. A kind with a
- // static GPU lane already arbitrates itself (digestBatch has its own yield);
- // laneFor marks the kinds whose resource changes under them from settings,
- // which today is diarization alone, and those are the ones nothing else is
- // deciding for.
+ // real regression rather than a nicety. `digest`'s declared lane is
+ // `contendsFor: "gpu"`, so testing every kind's lane made this lane
+ // idle-only whenever a digest was in the run — which broke the guarantee
+ // that a digest runs CONCURRENTLY with a backfill rather than behind it. A
+ // kind with a static GPU lane already arbitrates itself (digestBatch has
+ // its own yield); laneFor marks the kinds whose resource changes under
+ // them from settings, and those are the ones nothing else is deciding for.
+ //
+ // DIGEST NOW DECLARES A laneFor TOO, and is still not in this decision —
+ // but not because of this guard. `kinds` here came through
+ // resolveBackfillKinds, which filters via laneBackfillKinds and so admits
+ // BACKFILL_QUEUE kinds only; digest is on its own queue and can never
+ // reach this array, even when asked for by name. That is the invariant
+ // backfillKinds.test.ts pins.
+ //
+ // Which is also why laneFor MUST STAY OPTIONAL. This line keys off its
+ // PRESENCE, so "give every kind a laneFor defaulting to lane" would
+ // silently enrol every kind — reviving exactly the regression above for
+ // any statically GPU-bound kind that IS on this queue. Add one only where
+ // the lane genuinely varies with settings.
const gpuBound = kinds.some(
(k) => k.laneFor && laneYieldsToTranscription(k.laneFor(liveSettings)),
);
diff --git a/common/controller/laneForOperation.test.ts b/common/controller/laneForOperation.test.ts
@@ -0,0 +1,108 @@
+// The REAL lane resolver, not the stub arbiter.test.ts plans units with.
+//
+// Run with: node_modules/.bin/tsx --test common/controller/laneForOperation.test.ts
+//
+// Its own file because it needs a settings seam: laneForOperation calls
+// getSettings(), and getPaths() memoizes its first answer at module scope, so
+// the env must be set before anything imports the module under test.
+// arbiter.test.ts is deliberately settings-free — it passes `laneOf` in — and
+// these cases are exactly the ones a stub would answer vacuously.
+
+import { mkdtempSync, writeFileSync } from "node:fs";
+import { rm } from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { test, after } from "node:test";
+import assert from "node:assert/strict";
+
+// Set BEFORE anything can call getPaths(). node:test runs each file in its own
+// process, so this is scoped to this file alone.
+const ROOT = mkdtempSync(path.join(os.tmpdir(), "lane-for-operation-"));
+process.env.TRANSCRIPTS_DIR = ROOT;
+// Its own env key, defaulting to the MONOREPO root rather than the corpus —
+// without it these cases would read the live install's settings.json and the
+// remote-digest case would flip with whatever the operator has configured.
+const SETTINGS_FILE = path.join(ROOT, "settings.json");
+process.env.SETTINGS_FILE = SETTINGS_FILE;
+
+const { laneForOperation } = await import("./arbiter");
+const { DIGEST_LOCAL_QUEUE, DIGEST_REMOTE_QUEUE, BACKFILL_QUEUE } =
+ await import("../lib/queueKeys");
+
+after(() => rm(ROOT, { recursive: true, force: true }));
+
+// getSettings() re-reads the file on every call (no memoization of its own), so
+// a test can flip a setting between assertions.
+function writeSettings(settings: Record<string, unknown>): void {
+ writeFileSync(SETTINGS_FILE, JSON.stringify(settings), "utf8");
+}
+
+test("digest's lane follows remoteEnabled, live", () => {
+ // THE POINT OF laneFor. `digest.lane` is a DECLARATION and can only carry the
+ // default (the local, GPU-bound queue). Which queue a run actually takes is a
+ // configuration choice, and the arbiter must reserve by the live answer or a
+ // remote digest lands on the local key — where registry.ts's concurrency-1
+ // would serialize it behind GPU work it competes with for nothing.
+ writeSettings({ digest: { remoteEnabled: false } });
+ assert.equal(laneForOperation("digest")?.queueKey, DIGEST_LOCAL_QUEUE);
+ assert.equal(laneForOperation("digest")?.contendsFor, "gpu");
+
+ writeSettings({ digest: { remoteEnabled: true } });
+ assert.equal(laneForOperation("digest")?.queueKey, DIGEST_REMOTE_QUEUE);
+ // And it must NOT yield: metered network work competes for nothing local, so
+ // standing aside for transcription would park a lane that costs nothing to
+ // keep running.
+ assert.equal(laneForOperation("digest")?.contendsFor, "network");
+});
+
+test("diarization resolves through its laneFor too", () => {
+ // Regression guard on the shape of the resolver rather than on diarization:
+ // before this, laneForOperation special-cased digest and returned `.lane` for
+ // everything else, so diarization's laneFor was invisible to the arbiter. The
+ // queue key is the same either way by design (two diarizations must never run
+ // at once), so `contendsFor` is what shows the live answer got through.
+ writeSettings({
+ diarization: { enabled: true, engine: "sortformer", backend: "vulkan" },
+ });
+ const gpu = laneForOperation("diarization");
+ assert.equal(gpu?.queueKey, BACKFILL_QUEUE);
+ assert.equal(gpu?.contendsFor, "gpu");
+
+ writeSettings({
+ diarization: { enabled: true, engine: "sortformer", backend: "cpu" },
+ });
+ assert.equal(laneForOperation("diarization")?.contendsFor, "cpu");
+});
+
+test("a KNOWN external operation still has no lane", () => {
+ // download and transcription are in operationCatalog() — they have to be, or
+ // digest.dependsOn = ["transcription"] names nothing — but they are dispatched
+ // by their OWN runners. laneForOperation asks getBackfillKind, not the
+ // catalog, precisely so they come back null and planArbiterUnits skips them.
+ //
+ // A catalog lookup would hand them a lane and the arbiter would start a
+ // backfill channel job for work no backfill kind can do. That is a different
+ // failure from an unknown id, and only this case would catch it.
+ writeSettings({});
+ assert.equal(laneForOperation("download"), null);
+ assert.equal(laneForOperation("transcription"), null);
+ // The unknown-id case, for contrast: same answer, different reason.
+ assert.equal(laneForOperation("sortformer"), null);
+});
+
+test("a switched-off kind still resolves a lane — the off-switch is elsewhere", () => {
+ // Pinning the BOUNDARY, because it is the one an obvious-looking "fix" would
+ // move. This resolver answers "which lane would this run on", not "should it
+ // run": getBackfillKind is a registry lookup and has never consulted
+ // `enabled`. What keeps a disabled feature out of the arbiter is the `enabled`
+ // set in runArbiterPass, which projects ids only for kinds allBackfillKinds
+ // returns — so a switched-off kind arrives with an EMPTY id list and produces
+ // no unit, well before a lane is asked for.
+ //
+ // Teaching this function to return null for a disabled kind would look like a
+ // tightening and would in fact be a second, redundant gate that the sweeps and
+ // the stage cards — which call laneForOperation to label a queue, not to
+ // decide anything — would then read as "this operation has no lane at all".
+ writeSettings({ diarization: { enabled: false } });
+ assert.equal(laneForOperation("diarization")?.queueKey, BACKFILL_QUEUE);
+});
diff --git a/common/lib/backfillKinds.ts b/common/lib/backfillKinds.ts
@@ -377,7 +377,16 @@ export type BackfillKind = {
//
// Optional because most kinds have no choice, and `lane` is the answer for
// them. Mirrors digestLaneFor, which solved the same problem for the digest
- // operation's two lanes.
+ // operation's two lanes — and which digest itself now declares.
+ //
+ // OPTIONAL IS LOAD-BEARING, not tidiness. controller/backfillBatch.ts reads
+ // this field's PRESENCE as the marker for "this kind's resource depends on
+ // settings, so nothing else is deciding it for us" and makes the whole run
+ // idle-only when such a kind could take the GPU. Giving every kind a laneFor
+ // that defaults to `lane` would therefore not be a no-op: it would enrol
+ // every kind in that rule and make the lane idle-only whenever a statically
+ // GPU-bound kind was in the run — the exact regression the guard's comment
+ // records. Add one only where the lane genuinely varies.
laneFor?(settings: SiteSettings): BackfillLane;
// Ids of other kinds in this table whose output this one consumes.
//
@@ -990,6 +999,20 @@ const digest: BackfillKind = {
// `contendsFor: "gpu"` is what makes this lane — and only this lane — stand
// aside for transcription.
lane: digestLaneFor("local-gpu"),
+ // The live answer, for the same reason diarization has one: which lane this
+ // runs on is a CONFIGURATION CHOICE, not a fact about the operation, and
+ // `lane` above can only carry the default. laneForOperation asks this, so the
+ // arbiter dispatches a remote digest onto DIGEST_REMOTE_QUEUE instead of the
+ // local key it declares.
+ //
+ // Declaring it does NOT put digest into backfillBatch's idle-only rule, and
+ // the reason is worth stating because that rule keys off laneFor's PRESENCE:
+ // resolveBackfillKinds is filtered through laneBackfillKinds (BACKFILL_QUEUE
+ // only), so digest can never be among the `kinds` that guard inspects. See
+ // controller/backfillBatch.ts, where the same fact is written from the other
+ // side.
+ laneFor: (settings) =>
+ digestLaneFor(settings.digest.remoteEnabled ? "remote-api" : "local-gpu"),
// Digests are gated by their own sweep/pause switches rather than a master
// "enabled" flag, so the feature is on whenever an app is configured. The
// pause is honoured at DISPATCH (digestBatch's limit()), not here: a paused
