mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
Merge pull request #2840 from heygen-com/07-27-feat_producer_enable_parallel-de_router_by_default
feat(cli,core,producer): ramp the parallel-DE router through the canary at 5%
This commit is contained in:
@@ -42,6 +42,10 @@ const configState = vi.hoisted(
|
||||
);
|
||||
|
||||
const trackingState = vi.hoisted(() => ({
|
||||
// The rollout slice. Default-ON is gated on canary enrolment, so these
|
||||
// tests control it directly rather than depending on where the test
|
||||
// machine's bucketSeed happens to land.
|
||||
canaryEnabled: true,
|
||||
// maybeEnableDeParallelRouterTrial gates on the real shouldTrack(), which
|
||||
// (via isDevMode()) always returns false when this file itself runs as
|
||||
// `.ts` source under vitest — mocked here so the CLI-trial tests can
|
||||
@@ -172,6 +176,10 @@ vi.mock("../telemetry/client.js", () => ({
|
||||
shouldTrack: vi.fn(() => trackingState.shouldTrack),
|
||||
}));
|
||||
|
||||
vi.mock("../telemetry/canary.js", () => ({
|
||||
isCanaryEnabled: vi.fn(() => trackingState.canaryEnabled),
|
||||
}));
|
||||
|
||||
vi.mock("../telemetry/events.js", () => ({
|
||||
trackRenderComplete: vi.fn(),
|
||||
trackRenderError: vi.fn(),
|
||||
@@ -238,6 +246,7 @@ describe("renderLocal browser GPU config", () => {
|
||||
configState.failMirrors = 0;
|
||||
configState.writeConfigCalls = [];
|
||||
trackingState.shouldTrack = true;
|
||||
trackingState.canaryEnabled = true;
|
||||
trackingState.renderObservations = [];
|
||||
ffmpegEncoderState.mode = "software";
|
||||
ffmpegEncoderState.error = null;
|
||||
@@ -725,7 +734,11 @@ describe("renderLocal browser GPU config", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
// Suite renamed with the breaker work: this is no longer an opt-in trial. The
|
||||
// bindings come from main's shared top-level `renderModule` import rather than
|
||||
// this suite's own beforeAll — same module instance every other suite uses, so
|
||||
// module-scope arm/consume state resets through the one `resetTrialState()`.
|
||||
describe("renderLocal — DE parallel-router circuit breaker", () => {
|
||||
const { renderLocal, __resetDeParallelRouterTrialStateForTests: resetTrialState } = renderModule;
|
||||
const savedEnv = new Map<string, string | undefined>();
|
||||
|
||||
@@ -736,6 +749,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
configState.failWrites = 0;
|
||||
configState.writeConfigCalls = [];
|
||||
trackingState.shouldTrack = true;
|
||||
trackingState.canaryEnabled = true;
|
||||
// The "managed by us" flag lives at module scope in render.ts (real CLI
|
||||
// processes only ever run one --batch sequence, so it never needs
|
||||
// resetting there) — reset explicitly here so tests don't leak arm/
|
||||
@@ -768,21 +782,73 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
browserGpuMode: "software" as const,
|
||||
hdrMode: "auto" as const,
|
||||
quiet: true,
|
||||
// The trial is OPT-IN (review): only the CLI's own sequential call sites
|
||||
// set this. These tests simulate those call sites.
|
||||
enableDeParallelRouterTrial: true,
|
||||
// Breaker management is OPT-IN (review): only the CLI's own sequential
|
||||
// call sites set it. These tests simulate those call sites.
|
||||
manageDeParallelRouterBreaker: true,
|
||||
};
|
||||
|
||||
it("enables the trial (sets the env var) on a fresh install with telemetry on", async () => {
|
||||
// The rollout slice. Default-ON means every eligible render routes the
|
||||
// moment this ships — a ~17x exposure jump. The canary is what makes that
|
||||
// fraction chosen and revertible instead of emergent.
|
||||
it("disarms for an install the canary did not enrol", async () => {
|
||||
trackingState.canaryEnabled = false;
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
// Explicit "false", not delete: with default-ON polarity, deleting the
|
||||
// var means ON — the same trap the breaker fix exists for.
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
|
||||
// Setting the registry percentage to 0 must switch the router off fleet-wide
|
||||
// without a release. That is the revert path, so it has to be pinned.
|
||||
it("registry percentage is a full kill switch", async () => {
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
|
||||
trackingState.canaryEnabled = false;
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
|
||||
delete process.env.HF_DE_PARALLEL_ROUTER;
|
||||
trackingState.canaryEnabled = true;
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
|
||||
// An explicit user choice outranks enrolment in both directions — the
|
||||
// documented escalation path for anyone who wants the router regardless.
|
||||
it("never overrides an explicit user value, enrolled or not", async () => {
|
||||
trackingState.canaryEnabled = false;
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
process.env.HF_DE_PARALLEL_ROUTER = "true";
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
|
||||
});
|
||||
|
||||
it("leaves the env var untouched on a fresh install — the router is default-ON", async () => {
|
||||
// Under the old opt-in trial this armed HF_DE_PARALLEL_ROUTER="true".
|
||||
// The router now ships on, so the breaker's job is to stay out of the
|
||||
// way until something actually fails.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not override an env var the user already set themselves", async () => {
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
@@ -794,49 +860,75 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
|
||||
it("does not enable the trial once it has already fired for this install", async () => {
|
||||
it("writes an explicit false once the breaker has tripped for this install", async () => {
|
||||
// THE regression this rework exists for: the old code disabled the
|
||||
// router by DELETING the var. With a default-ON router, absent means ON,
|
||||
// so deleting would silently re-enable it on the very host that just
|
||||
// failed. Only an explicit "false" is a real off-switch.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: true,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
|
||||
it("does not enable the trial when shouldTrack() is false (dev mode / DO_NOT_TRACK)", async () => {
|
||||
for (const emptyish of ["", " "]) {
|
||||
it(`treats a set-but-empty env var (${JSON.stringify(emptyish)}) as default, not a user choice`, async () => {
|
||||
// Both parsers read empty/whitespace as "unset → default ON", so the
|
||||
// producer routes. If ownership instead treated any defined value as a
|
||||
// user choice, the breaker would no-op and this install would keep
|
||||
// retrying a failing router forever — losing the first-fallback
|
||||
// protection that is the point of the breaker.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
process.env.HF_DE_PARALLEL_ROUTER = emptyish;
|
||||
producerState.executeImpl = async (job) => {
|
||||
job.perfSummary = {
|
||||
resolution: { width: 100, height: 100 },
|
||||
drawElement: { parallelRouter: "reverted" },
|
||||
};
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
expect(configState.writeConfigCalls).toContainEqual(
|
||||
expect.objectContaining({ deParallelRouterTrialFired: true }),
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
it("does not override an explicit user opt-in even after a fallback", async () => {
|
||||
// "Explicit user choice wins in both directions" — the opt-in half.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
trackingState.shouldTrack = false;
|
||||
process.env.HF_DE_PARALLEL_ROUTER = "true";
|
||||
producerState.executeImpl = async (job) => {
|
||||
job.perfSummary = {
|
||||
resolution: { width: 100, height: 100 },
|
||||
drawElement: { parallelRouter: "reverted" },
|
||||
};
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
|
||||
});
|
||||
|
||||
it("does not enable the trial when config.telemetryEnabled is false, even if shouldTrack() is stale-true (e.g. `hyperframes telemetry off` mid-batch)", async () => {
|
||||
it("keeps the router on for a telemetry opt-out — analytics choice must not cost performance", async () => {
|
||||
// The old trial refused to arm without recordable telemetry (no point
|
||||
// running an experiment you can't measure). Now that the router is a
|
||||
// shipped default, gating it on telemetry would punish a privacy choice
|
||||
// with a slower renderer.
|
||||
configState.disk = {
|
||||
telemetryEnabled: false,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
trackingState.shouldTrack = true;
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not enable the trial before the first-run telemetry disclosure has been shown at least once", async () => {
|
||||
// cli.ts shows this notice via a fire-and-forget, unawaited dynamic
|
||||
// import — there's no guarantee it printed before renderLocal runs on a
|
||||
// brand-new install's very first invocation. Requiring
|
||||
// telemetryNoticeShown means the trial never races an opt-in message
|
||||
// against the disclosure it depends on.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: false,
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
@@ -946,11 +1038,9 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
});
|
||||
|
||||
it("persists a later --batch row's revert even though this process already armed the trial on an earlier row", async () => {
|
||||
// Regression test for the exact scenario a --batch run hits: multiple
|
||||
// renderLocal calls in one process. Before the fix, row 2's
|
||||
// maybeEnableDeParallelRouterTrial saw process.env.HF_DE_PARALLEL_ROUTER
|
||||
// already "true" (set by row 1) and mistook that for "the user set it",
|
||||
// returning trialArmed=false — silently dropping row 2's revert.
|
||||
// The --batch scenario: multiple renderLocal calls in one process. Row 1
|
||||
// succeeds (breaker stays out of the way, env untouched); row 2 reverts
|
||||
// and must still be recorded and trip the breaker.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
@@ -964,7 +1054,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
};
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(configState.disk.deParallelRouterTrialFired).toBe(false);
|
||||
|
||||
producerState.executeImpl = async (job) => {
|
||||
@@ -978,12 +1068,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
expect(configState.writeConfigCalls).toContainEqual(
|
||||
expect.objectContaining({ deParallelRouterTrialFired: true }),
|
||||
);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
// Explicit "false", not deleted: with a default-ON router, unsetting the
|
||||
// var would re-enable it on the host that just reverted.
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
|
||||
it("does not arm the trial for programmatic callers that never opted in (opt-in polarity — also covers --batch-concurrency N>=2, which leaves it unset)", async () => {
|
||||
// The trial's process-wide env var and module-level flags are only safe
|
||||
// under sequential invocation, so enableDeParallelRouterTrial is OPT-IN
|
||||
// under sequential invocation, so manageDeParallelRouterBreaker is OPT-IN
|
||||
// (review): a programmatic renderLocal consumer that doesn't know about
|
||||
// the trial must get no trial. The CLI's concurrent-batch path relies on
|
||||
// the same default by leaving the option unset.
|
||||
@@ -992,7 +1084,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
const { enableDeParallelRouterTrial: _omitted, ...programmaticOptions } = baseOptions;
|
||||
const { manageDeParallelRouterBreaker: _omitted, ...programmaticOptions } = baseOptions;
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", programmaticOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(configState.writeConfigCalls).toHaveLength(0);
|
||||
@@ -1011,7 +1103,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
};
|
||||
};
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
|
||||
// A real interactive user can't do this mid-batch, but a wrapper script
|
||||
// invoking the CLI programmatically in the same process could — the
|
||||
@@ -1021,7 +1113,10 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
|
||||
it("caps exposure at DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS even when the router never reverts", async () => {
|
||||
it("never trips on healthy renders, however many — the old 25-render cap is gone", async () => {
|
||||
// The cap was sampling logic for an opt-in experiment. Under a shipped
|
||||
// default it would switch the feature off behind the user's back after
|
||||
// 25 good renders.
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
@@ -1034,40 +1129,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
};
|
||||
};
|
||||
|
||||
for (let i = 0; i < 25; i++) {
|
||||
for (let i = 0; i < 30; i++) {
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
}
|
||||
|
||||
expect(configState.writeConfigCalls).toContainEqual(
|
||||
expect.objectContaining({
|
||||
deParallelRouterTrialFired: true,
|
||||
deParallelRouterTrialRenderCount: 25,
|
||||
}),
|
||||
);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
|
||||
// The 26th eligible render must not re-arm it.
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
});
|
||||
|
||||
it("observes a telemetry opt-out written by another process mid-batch (arm site reads fresh, not cached)", async () => {
|
||||
configState.disk = {
|
||||
telemetryEnabled: true,
|
||||
deParallelRouterTrialFired: false,
|
||||
telemetryNoticeShown: true,
|
||||
};
|
||||
// Row 1 arms and primes the config cache.
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
|
||||
|
||||
// Another process runs `hyperframes telemetry off`, writing straight to
|
||||
// "disk" — this process's cache still says telemetryEnabled: true, so a
|
||||
// cached read at the arm site would keep arming (review finding).
|
||||
configState.disk = { ...configState.disk, telemetryEnabled: false };
|
||||
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(
|
||||
configState.writeConfigCalls.some((call) => call.deParallelRouterTrialFired === true),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
// The config write landing is NOT enough: config.json is the copy a stale
|
||||
@@ -1136,10 +1205,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
|
||||
// Nothing could persist...
|
||||
expect(configState.disk.deParallelRouterTrialFired).toBe(false);
|
||||
// ...but the in-process latch still blocks the next render from
|
||||
// re-running the experiment that just failed (review finding).
|
||||
// re-running the path that just failed (review finding) — and now does
|
||||
// it by writing an explicit "false", since absent means ON.
|
||||
producerState.executeImpl = async () => undefined;
|
||||
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
|
||||
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
+205
-151
@@ -1,4 +1,5 @@
|
||||
import { failCommand, requestCliExit } from "../utils/commandResult.js";
|
||||
import { isCanaryEnabled } from "../telemetry/canary.js";
|
||||
import { defineCommand } from "citty";
|
||||
import type { Example } from "./_examples.js";
|
||||
import { mkdtempSync, readdirSync, readFileSync, statSync, writeFileSync, rmSync } from "node:fs";
|
||||
@@ -69,7 +70,6 @@ import {
|
||||
writeConfigWithResult,
|
||||
type HyperframesConfig,
|
||||
} from "../telemetry/config.js";
|
||||
import { shouldTrack } from "../telemetry/client.js";
|
||||
import { renderJobObservabilityTelemetryPayload } from "../telemetry/renderObservability.js";
|
||||
import { bytesToMb } from "../telemetry/system.js";
|
||||
import { VERSION } from "../version.js";
|
||||
@@ -424,21 +424,26 @@ export interface RenderOptions {
|
||||
/** Skip the interactive feedback prompt after a successful render. */
|
||||
skipFeedback?: boolean;
|
||||
/**
|
||||
* OPT IN to the DE parallel-router CLI trial
|
||||
* (`maybeEnableDeParallelRouterTrial`) for this render. Default OFF —
|
||||
* OPT IN to managing the DE parallel-router circuit breaker
|
||||
* (`applyDeParallelRouterCircuitBreaker`) for this render. Default OFF —
|
||||
* only the top-level CLI render command's own call sites should ever set
|
||||
* this (review): the trial mechanism shares one process-wide env var and
|
||||
* two module-level flags across every `renderLocal` call in the process,
|
||||
* this (review): the mechanism shares one process-wide env var and two
|
||||
* module-level flags across every `renderLocal` call in the process,
|
||||
* which is safe for SEQUENTIAL calls (single render, single-concurrency
|
||||
* batch rows) but not for genuinely concurrent ones — racing invocations
|
||||
* could tear down or misattribute each other's outcome. Programmatic
|
||||
* consumers importing `renderLocal` (a future studio-server path, test
|
||||
* harnesses, distributed runners) therefore get NO trial unless they
|
||||
* explicitly opt in AND guarantee sequential invocation. The CLI sets
|
||||
* this for single renders and for `--batch` at concurrency 1; it leaves
|
||||
* it unset for `--batch-concurrency N>=2`.
|
||||
* harnesses, distributed runners) therefore do not manage the breaker
|
||||
* unless they explicitly opt in AND guarantee sequential invocation. The
|
||||
* CLI sets this for single renders and for `--batch` at concurrency 1; it
|
||||
* leaves it unset for `--batch-concurrency N>=2`.
|
||||
*
|
||||
* NOTE the asymmetry: the ROUTER itself is default-on for every consumer
|
||||
* (the producer decides that). This flag only governs whether we
|
||||
* additionally enforce the per-install breaker, because that is the part
|
||||
* with process-wide state.
|
||||
*/
|
||||
enableDeParallelRouterTrial?: boolean;
|
||||
manageDeParallelRouterBreaker?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -841,10 +846,12 @@ export async function renderLocal(
|
||||
}
|
||||
|
||||
const producer = await loadProducer();
|
||||
const deParallelRouterTrialArmed = maybeEnableDeParallelRouterTrial(
|
||||
options.quiet,
|
||||
options.enableDeParallelRouterTrial === true,
|
||||
);
|
||||
const deParallelRouterActive =
|
||||
options.manageDeParallelRouterBreaker === true
|
||||
? applyDeParallelRouterCircuitBreaker(options.quiet)
|
||||
: // Not managing the breaker: the router still runs (producer default),
|
||||
// we just don't enforce or record the per-install trip.
|
||||
false;
|
||||
|
||||
const startTime = Date.now();
|
||||
const logger = createRenderTelemetryLogger(
|
||||
@@ -894,7 +901,7 @@ export async function renderLocal(
|
||||
try {
|
||||
await producer.executeRenderJob(job, projectDir, outputPath, onProgress);
|
||||
} catch (error: unknown) {
|
||||
maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet);
|
||||
maybeConsumeDeParallelRouterTrial(deParallelRouterActive, job, options.quiet);
|
||||
handleRenderError(
|
||||
error,
|
||||
options,
|
||||
@@ -913,7 +920,7 @@ export async function renderLocal(
|
||||
// (win32/x64, CLI 0.7.58): valid MP4 on disk, exited 1 with no error print.
|
||||
markRenderSucceeded();
|
||||
|
||||
maybeConsumeDeParallelRouterTrial(deParallelRouterTrialArmed, job, options.quiet);
|
||||
maybeConsumeDeParallelRouterTrial(deParallelRouterActive, job, options.quiet);
|
||||
const elapsed = Date.now() - startTime;
|
||||
if (job.outcome === "completed_with_warnings") {
|
||||
for (const warning of job.warnings) {
|
||||
@@ -1072,37 +1079,40 @@ function createNoopProducerLogger(): ProducerLogger {
|
||||
};
|
||||
}
|
||||
|
||||
/** Backstop cap: even absent an actual router failure, stop offering the
|
||||
* trial after this many engaged (routed or reverted) renders for an
|
||||
* install. Without this, a healthy router that never reverts would stay
|
||||
* force-enabled on every eligible render forever (review finding). */
|
||||
const DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS = 25;
|
||||
|
||||
/**
|
||||
* True across every `renderLocal` call in THIS process once the trial has
|
||||
* armed `HF_DE_PARALLEL_ROUTER` here — distinct from the env var's own
|
||||
* value, which stays "true" across an entire `--batch` run. Without this,
|
||||
* a second batch row's `process.env.HF_DE_PARALLEL_ROUTER !== undefined`
|
||||
* check can't tell "we set this ourselves on row 1" from "the user set
|
||||
* this" and would wrongly treat itself as un-armed, silently dropping that
|
||||
* row's outcome from ever reaching `maybeConsumeDeParallelRouterTrial`
|
||||
* (review finding).
|
||||
* The 25-render exposure cap that bounded the old opt-in TRIAL is gone: the
|
||||
* router is default-ON as of 2026-07-27, so "stop offering it after N
|
||||
* renders" would mean switching a shipped default off behind the user's
|
||||
* back. What survives is the half that was always safety rather than
|
||||
* sampling — the per-install circuit breaker below, which latches the router
|
||||
* off for good the first time a render has to fall back.
|
||||
*/
|
||||
let deParallelRouterTrialManagedByUs = false;
|
||||
|
||||
/**
|
||||
* In-process latch mirroring `deParallelRouterTrialFired`: set the moment we
|
||||
* DECIDE the trial is over, independent of whether persisting that decision
|
||||
* to `~/.hyperframes/config.json` succeeds. `writeConfig` swallows all fs
|
||||
* The user set `HF_DE_PARALLEL_ROUTER` themselves (either polarity), latched
|
||||
* once at first observation. Their choice wins over the circuit breaker in
|
||||
* BOTH directions: we never overwrite an explicit opt-in with `"false"` on a
|
||||
* fallback, and never overwrite an explicit opt-out either. Latched rather
|
||||
* than re-read because the breaker itself writes the var — after the first
|
||||
* write a live `process.env` read could no longer tell "the user set this"
|
||||
* from "we set this" (the same distinction the old trial needed for
|
||||
* `--batch` rows sharing one process).
|
||||
*/
|
||||
let deParallelRouterUserManaged = false;
|
||||
let deParallelRouterUserManagedResolved = false;
|
||||
|
||||
/**
|
||||
* In-process latch mirroring the persisted `deParallelRouterTrialFired`: set
|
||||
* the moment the breaker trips, independent of whether persisting that to
|
||||
* `~/.hyperframes/config.json` succeeds. `writeConfig` swallows all fs
|
||||
* errors (by design — telemetry must never break the CLI), so on an
|
||||
* unwritable config (root-owned file, disk full) the fired flag can never
|
||||
* stick on disk; without this latch the trial would silently re-arm and
|
||||
* re-fail on every subsequent render in this process forever (review
|
||||
* finding). Later processes still re-arm — disk is the only cross-process
|
||||
* channel — but each process now stops after at most one failure it
|
||||
* couldn't record.
|
||||
* unwritable config (root-owned file, disk full) the flag can never stick on
|
||||
* disk; without this latch the router would re-enable and re-fail on every
|
||||
* subsequent render in this process. Later processes re-arm — disk is the
|
||||
* only cross-process channel — but each process now stops after at most one
|
||||
* failure it couldn't record.
|
||||
*/
|
||||
let deParallelRouterTrialFiredThisProcess = false;
|
||||
let deParallelRouterBreakerTrippedThisProcess = false;
|
||||
|
||||
/**
|
||||
* Test-only reset for the module-level trial state — a real CLI process
|
||||
@@ -1111,111 +1121,133 @@ let deParallelRouterTrialFiredThisProcess = false;
|
||||
* one imported module instance.
|
||||
*/
|
||||
export function __resetDeParallelRouterTrialStateForTests(): void {
|
||||
deParallelRouterTrialManagedByUs = false;
|
||||
deParallelRouterTrialFiredThisProcess = false;
|
||||
deParallelRouterBreakerTrippedThisProcess = false;
|
||||
deParallelRouterUserManaged = false;
|
||||
deParallelRouterUserManagedResolved = false;
|
||||
}
|
||||
|
||||
/**
|
||||
* True once the trial should stop offering itself: already failed (on disk
|
||||
* or via this process's in-memory latch), hit the render-count backstop, or
|
||||
* telemetry isn't actually recordable right now.
|
||||
* Has this install's router circuit breaker already tripped — on disk, or via
|
||||
* this process's in-memory latch?
|
||||
*
|
||||
* Checks BOTH `shouldTrack()` and `config.telemetryEnabled` directly, not
|
||||
* `shouldTrack()` alone: `shouldTrack()` (`../telemetry/client.js`) memoizes
|
||||
* its verdict once per process and never invalidates, so during a long
|
||||
* `--batch` run (all rows share one process) a `hyperframes telemetry off`
|
||||
* issued from another terminal mid-batch would never be observed. The
|
||||
* caller must pass a `readConfigFresh()` snapshot for the same reason —
|
||||
* `readConfig()` serves a process-lifetime cache that is exactly as stale
|
||||
* as the `shouldTrack()` memoization this check exists to bypass (review
|
||||
* finding).
|
||||
* Deliberately does NOT consider telemetry state. The old opt-in trial did:
|
||||
* there was no point running an experimental path if the resulting signal
|
||||
* couldn't be recorded. Now that the router is a shipped default, gating it
|
||||
* on telemetry would mean users who opted out of analytics silently get a
|
||||
* slower renderer — punishing a privacy choice with a performance penalty
|
||||
* (review finding). Telemetry state governs REPORTING, never behavior.
|
||||
*/
|
||||
function isDeParallelRouterTrialBlocked(config: HyperframesConfig): boolean {
|
||||
const overRenderCap =
|
||||
(config.deParallelRouterTrialRenderCount ?? 0) >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS;
|
||||
return (
|
||||
deParallelRouterTrialFiredThisProcess ||
|
||||
Boolean(config.deParallelRouterTrialFired) ||
|
||||
overRenderCap ||
|
||||
!config.telemetryEnabled ||
|
||||
!shouldTrack() ||
|
||||
// cli.ts shows the first-run telemetry disclosure via a fire-and-forget,
|
||||
// unawaited dynamic import — there's no guarantee it has printed before
|
||||
// this render command reaches this point. Requiring telemetryNoticeShown
|
||||
// means the trial simply never offers itself on a fresh install's very
|
||||
// first invocation (before the disclosure is guaranteed to have run at
|
||||
// least once), rather than racing an experimental opt-in message against
|
||||
// the disclosure it depends on (review finding).
|
||||
!config.telemetryNoticeShown
|
||||
);
|
||||
}
|
||||
|
||||
/** Shared cleanup for both `maybeEnableDeParallelRouterTrial` (this process
|
||||
* should stop offering the trial) and `maybeConsumeDeParallelRouterTrial`
|
||||
* (the trial just failed/hit its cap) — a no-op unless WE were the ones
|
||||
* managing the env var. */
|
||||
function stopManagingDeParallelRouterTrial(): void {
|
||||
if (!deParallelRouterTrialManagedByUs) return;
|
||||
delete process.env.HF_DE_PARALLEL_ROUTER;
|
||||
deParallelRouterTrialManagedByUs = false;
|
||||
function hasDeParallelRouterBreakerTripped(config: HyperframesConfig): boolean {
|
||||
return deParallelRouterBreakerTrippedThisProcess || Boolean(config.deParallelRouterTrialFired);
|
||||
}
|
||||
|
||||
/**
|
||||
* Enable the DE parallel-router experiment (`HF_DE_PARALLEL_ROUTER`, default
|
||||
* off) for this render, on every eligible render for this install (up to
|
||||
* `DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS`), so we get real-traffic router
|
||||
* telemetry (revert rate, verify-db distribution) without requiring anyone
|
||||
* to manually set the env var — see `HyperframesConfig.deParallelRouterTrialFired`.
|
||||
* See `maybeConsumeDeParallelRouterTrial` for what turns it off. Returns
|
||||
* whether this call armed it (so the caller knows to check for consumption
|
||||
* afterward) — false unless the caller explicitly opted in (`enabled` —
|
||||
* OPT-IN polarity, review: only the top-level CLI render command's own
|
||||
* sequential call sites set it; programmatic `renderLocal` consumers get no
|
||||
* trial by default because the mechanism's process-wide state is unsafe
|
||||
* under concurrent invocation — see
|
||||
* `RenderOptions.enableDeParallelRouterTrial`), if it's already failed (or
|
||||
* hit the render cap) for this install, if the user already set the env var
|
||||
* themselves (never override an explicit choice — see
|
||||
* `deParallelRouterTrialManagedByUs` for how a later `--batch` row
|
||||
* distinguishes that from our own earlier arm), or if telemetry isn't
|
||||
* actually recordable right now (see `isDeParallelRouterTrialBlocked`; no
|
||||
* point risking the experimental path if we can't even record the
|
||||
* resulting signal).
|
||||
* Latch the router OFF for the rest of this process by writing an explicit
|
||||
* `"false"`.
|
||||
*
|
||||
* Under the old default-OFF flag this deleted the var, because absent meant
|
||||
* off. With the router default-ON, deleting means ON — the same call would
|
||||
* silently RE-ENABLE the router on exactly the host that just failed
|
||||
* (review finding). Writing the explicit value is what makes the breaker a
|
||||
* breaker. No-op when the user set the var themselves: their choice wins in
|
||||
* both directions.
|
||||
*/
|
||||
function maybeEnableDeParallelRouterTrial(quiet: boolean, enabled: boolean): boolean {
|
||||
if (!enabled) return false;
|
||||
// The in-process latch alone decides once it's set — short-circuit before
|
||||
// the disk read so post-fired batch rows don't pay a config read + parse +
|
||||
// shared-cache invalidation per row for an answer module state already
|
||||
// knows (review finding).
|
||||
if (deParallelRouterTrialFiredThisProcess) {
|
||||
stopManagingDeParallelRouterTrial();
|
||||
/**
|
||||
* Mirror of the producer's `isDeParallelRouterEnabled`. Deliberately
|
||||
* duplicated rather than imported: `@hyperframes/producer` is lazily loaded
|
||||
* (`loadProducer()`) to keep CLI startup fast, and this runs on the startup
|
||||
* path. Keep the two in sync — the producer copy is the source of truth.
|
||||
*/
|
||||
function userValueEnablesDeParallelRouter(): boolean {
|
||||
const raw = process.env.HF_DE_PARALLEL_ROUTER?.trim().toLowerCase();
|
||||
if (raw === undefined || raw === "") return true;
|
||||
return !(raw === "false" || raw === "0" || raw === "off" || raw === "no");
|
||||
}
|
||||
|
||||
function applyDeParallelRouterBreaker(): void {
|
||||
if (deParallelRouterUserManaged) return;
|
||||
process.env.HF_DE_PARALLEL_ROUTER = "false";
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply this install's router circuit breaker before a render.
|
||||
*
|
||||
* The router is default-ON, so the normal path does NOTHING here — the
|
||||
* producer's own default takes over. This exists for the one case that must
|
||||
* survive a shipped default: an install that already had a render fall back
|
||||
* stays off, permanently, across processes (the verdict is persisted to
|
||||
* `~/.hyperframes/config.json`). See `maybeConsumeDeParallelRouterTrial` for
|
||||
* what trips it.
|
||||
*
|
||||
* Returns whether the router is active for this render, so the caller knows
|
||||
* to inspect the outcome afterward.
|
||||
*/
|
||||
function applyDeParallelRouterCircuitBreaker(quiet: boolean): boolean {
|
||||
// Latch the user's own choice on first observation, BEFORE the breaker can
|
||||
// write the var itself and make the two indistinguishable.
|
||||
//
|
||||
// Ownership uses the SAME normalization as the two parsers: a set-but-empty
|
||||
// (or whitespace) value means "unset / default ON", so it is NOT a user
|
||||
// choice and must stay breaker-managed. Treating any defined value as
|
||||
// user-managed would let `HF_DE_PARALLEL_ROUTER=` route the render (empty
|
||||
// parses as ON) while exempting that install from the breaker — it would
|
||||
// keep retrying a failing router forever, losing exactly the first-fallback
|
||||
// protection this PR exists to provide (review finding).
|
||||
if (!deParallelRouterUserManagedResolved) {
|
||||
deParallelRouterUserManaged = (process.env.HF_DE_PARALLEL_ROUTER ?? "").trim() !== "";
|
||||
deParallelRouterUserManagedResolved = true;
|
||||
}
|
||||
if (deParallelRouterUserManaged) {
|
||||
// Explicit choice, either polarity — report whether it enables the
|
||||
// router so outcomes are still consumed, but never override it.
|
||||
return userValueEnablesDeParallelRouter();
|
||||
}
|
||||
|
||||
// The in-process latch decides once set — short-circuit before the disk
|
||||
// read so post-trip batch rows don't pay a config read + parse per row for
|
||||
// an answer module state already knows.
|
||||
if (deParallelRouterBreakerTrippedThisProcess) {
|
||||
applyDeParallelRouterBreaker();
|
||||
return false;
|
||||
}
|
||||
const userSetIt =
|
||||
process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs;
|
||||
if (userSetIt) return false;
|
||||
|
||||
// readConfigFresh, NOT readConfig: the cached read is exactly as stale as
|
||||
// the shouldTrack() memoization the blocked-check exists to bypass — a
|
||||
// mid-batch `hyperframes telemetry off` (or another process persisting
|
||||
// fired=true) would never be observed through the cache (review finding).
|
||||
if (isDeParallelRouterTrialBlocked(readConfigFresh())) {
|
||||
stopManagingDeParallelRouterTrial();
|
||||
// readConfigFresh, NOT readConfig: the cached read is process-lifetime, so
|
||||
// another process persisting a trip mid-`--batch` would never be observed.
|
||||
if (hasDeParallelRouterBreakerTripped(readConfigFresh())) {
|
||||
deParallelRouterBreakerTrippedThisProcess = true;
|
||||
applyDeParallelRouterBreaker();
|
||||
if (!quiet) {
|
||||
console.log(
|
||||
c.dim(
|
||||
" Parallel drawElement capture stays off for this install (a previous render " +
|
||||
"had to fall back). Re-enable with HF_DE_PARALLEL_ROUTER=true.",
|
||||
),
|
||||
);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
if (deParallelRouterTrialManagedByUs) return true;
|
||||
deParallelRouterTrialManagedByUs = true;
|
||||
process.env.HF_DE_PARALLEL_ROUTER = "true";
|
||||
if (!quiet) {
|
||||
console.log(
|
||||
c.dim(
|
||||
" Trying the experimental parallel drawElement capture path for this install " +
|
||||
"(disabled automatically if it ever needs to fall back; opt out anytime: " +
|
||||
"HF_DE_PARALLEL_ROUTER=false)",
|
||||
),
|
||||
);
|
||||
// The rollout slice. Default-ON means every eligible render routes the
|
||||
// moment this ships — a ~17x jump in exposure, onto profiles today's trial
|
||||
// population never covered (<=4 CPUs, Docker: ~12% of eligible renders
|
||||
// between them). Note "trial" is not a user opt-in: it arms automatically on
|
||||
// the CLI render path, so ~11% of installs already route without anyone
|
||||
// choosing it. The opt-in is at the CALL SITE — the flag excludes
|
||||
// programmatic renderLocal consumers, not users.
|
||||
//
|
||||
// 0.7.60-0.7.64 is why that matters: every unclamped render reverted for
|
||||
// five consecutive releases and nobody saw it.
|
||||
//
|
||||
// Ramping through the registry makes the exposed fraction a number someone
|
||||
// chose. Today's ~11% is emergent — the product of eligibility rules and a
|
||||
// capped trial — so it drifts with fleet composition and cannot be reverted
|
||||
// without a release. Setting the percentage to 0 turns the router off for
|
||||
// everyone, immediately, with no code change.
|
||||
//
|
||||
// Disarm uses the same explicit "false" the breaker writes, for the same
|
||||
// reason: with default-ON polarity, deleting the var means ON.
|
||||
if (!isCanaryEnabled("de-parallel-router")) {
|
||||
applyDeParallelRouterBreaker();
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
@@ -1302,40 +1334,62 @@ function persistDeParallelRouterTrialFired(): boolean {
|
||||
* `persistDeParallelRouterTrialFired`.
|
||||
*/
|
||||
function maybeConsumeDeParallelRouterTrial(
|
||||
trialArmed: boolean,
|
||||
routerActive: boolean,
|
||||
job: RenderJob,
|
||||
quiet: boolean,
|
||||
): void {
|
||||
if (!trialArmed) return;
|
||||
if (!routerActive) return;
|
||||
const outcome = resolveDeParallelRouterOutcome(job);
|
||||
if (outcome === undefined) return;
|
||||
|
||||
const config = readConfigFresh();
|
||||
const renderCount = (config.deParallelRouterTrialRenderCount ?? 0) + 1;
|
||||
config.deParallelRouterTrialRenderCount = renderCount;
|
||||
const fired = outcome === "reverted" || renderCount >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS;
|
||||
// Trip ONLY on an actual fallback. The old trial also tripped at a
|
||||
// 25-render exposure cap, which was sampling logic: bound how long an
|
||||
// experiment force-enables itself. Under a shipped default that would
|
||||
// switch the feature off behind the user's back after 25 good renders.
|
||||
const fired = outcome === "reverted";
|
||||
if (fired) {
|
||||
config.deParallelRouterTrialFired = true;
|
||||
// Latch BEFORE attempting persistence — the decision holds for this
|
||||
// process even if the disk write never sticks (unwritable config).
|
||||
deParallelRouterTrialFiredThisProcess = true;
|
||||
stopManagingDeParallelRouterTrial();
|
||||
deParallelRouterBreakerTrippedThisProcess = true;
|
||||
applyDeParallelRouterBreaker();
|
||||
}
|
||||
writeConfig(config);
|
||||
// `!quiet`-gated like every other trial message: quiet/batch-json renders
|
||||
// must produce no unexpected terminal output — CI wrappers asserting
|
||||
// empty stderr would misread the warning as a render failure (review
|
||||
// finding). The in-process latch above already guarantees the safety
|
||||
// behavior the warning describes, whether or not it prints.
|
||||
if (fired && !persistDeParallelRouterTrialFired() && !quiet) {
|
||||
console.warn(
|
||||
c.warn(
|
||||
" Could not persist the parallel drawElement trial's off-switch to " +
|
||||
"~/.hyperframes/config.json (unwritable?). The experiment stays off for this " +
|
||||
"process; future runs may retry it. Set HF_DE_PARALLEL_ROUTER=false to opt out.",
|
||||
),
|
||||
);
|
||||
}
|
||||
// Only announce a trip the breaker could actually act on. With an explicit
|
||||
// user opt-in the breaker is a no-op, so "now off for this install" would
|
||||
// be false — and would reprint on every subsequent revert, since the user's
|
||||
// value keeps the router active (review finding).
|
||||
if (fired && !deParallelRouterUserManaged) reportDeParallelRouterBreakerTrip(quiet);
|
||||
}
|
||||
|
||||
/**
|
||||
* Tell the user the breaker tripped, and warn if the verdict couldn't be
|
||||
* persisted. All output is `!quiet`-gated: quiet/batch-json renders must
|
||||
* produce no unexpected terminal output — CI wrappers asserting empty stderr
|
||||
* would misread a warning as a render failure (review finding). The
|
||||
* in-process latch guarantees the safety behavior either way.
|
||||
*/
|
||||
function reportDeParallelRouterBreakerTrip(quiet: boolean): void {
|
||||
const persisted = persistDeParallelRouterTrialFired();
|
||||
if (quiet) return;
|
||||
console.log(
|
||||
c.dim(
|
||||
" A frame failed verification, so parallel drawElement capture fell back to the " +
|
||||
"screenshot path and is now off for this install. Re-enable: " +
|
||||
"HF_DE_PARALLEL_ROUTER=true",
|
||||
),
|
||||
);
|
||||
if (persisted) return;
|
||||
console.warn(
|
||||
c.warn(
|
||||
" Could not persist the parallel drawElement circuit breaker to " +
|
||||
"~/.hyperframes/config.json (unwritable?). It stays off for this process; " +
|
||||
"future runs may retry it. Set HF_DE_PARALLEL_ROUTER=false to opt out for good.",
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
function handleRenderError(
|
||||
|
||||
@@ -34,7 +34,6 @@ export interface RenderExecutionDependencies {
|
||||
}
|
||||
|
||||
// Exported only through render.ts so command tests can lock the user-facing guidance.
|
||||
// fallow-ignore-next-line unused-export
|
||||
export function renderLintContinuationHint(strictErrors: boolean): string {
|
||||
return strictErrors
|
||||
? " Continuing render despite lint warnings. Use --strict-all to block warnings."
|
||||
@@ -109,7 +108,7 @@ export async function executeRenderPlan(
|
||||
protocolTimeout: plan.protocolTimeout,
|
||||
playerReadyTimeout: plan.playerReadyTimeout,
|
||||
exitAfterComplete: true,
|
||||
enableDeParallelRouterTrial: true,
|
||||
manageDeParallelRouterBreaker: true,
|
||||
};
|
||||
if (plan.useDocker) {
|
||||
options.pageSideCompositing = plan.pageSideCompositing;
|
||||
@@ -259,7 +258,7 @@ async function executeBatchRender(
|
||||
exitAfterComplete: false,
|
||||
throwOnError: true,
|
||||
skipFeedback: true,
|
||||
enableDeParallelRouterTrial: plan.batchConcurrency <= 1,
|
||||
manageDeParallelRouterBreaker: plan.batchConcurrency <= 1,
|
||||
};
|
||||
const manifest = await batchModule.runBatchRender({
|
||||
prepared: preparedBatch,
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { canaryBucket, evaluateCanary, parseCanaryOverride, type CanaryInput } from "./canary.js";
|
||||
import { CANARIES, canaryEnvVar, findCanary, overdueCanaries } from "./canaryRegistry.js";
|
||||
import {
|
||||
@@ -295,8 +297,24 @@ describe("registry", () => {
|
||||
// surface. This canary's own description says "ramp only alongside the
|
||||
// per-install circuit breaker" — without an assertion, bumping it to 5
|
||||
// before that wiring lands would go green.
|
||||
it("keeps de-parallel-router at 0% until the circuit breaker is wired", () => {
|
||||
expect(findCanary("de-parallel-router")?.percentage).toBe(0);
|
||||
// The registry is data, so a ramp is a one-line edit with no code review
|
||||
// surface. The previous version enforced "ramp only alongside the circuit
|
||||
// breaker" by pinning the percentage to 0 — which blocks the ramp forever
|
||||
// and never checks the wiring it names.
|
||||
//
|
||||
// Assert the wiring instead: a non-zero percentage is allowed only while
|
||||
// the CLI render path really gates on this canary AND still consults the
|
||||
// per-install breaker. Ramping without the gate would enrol everybody at
|
||||
// once, which is the whole thing the ramp exists to prevent.
|
||||
it("only ramps de-parallel-router while the CLI render path gates on it", () => {
|
||||
const pct = findCanary("de-parallel-router")?.percentage ?? 0;
|
||||
if (pct === 0) return;
|
||||
const renderSrc = readFileSync(
|
||||
join(import.meta.dirname, "..", "..", "cli", "src", "commands", "render.ts"),
|
||||
"utf8",
|
||||
);
|
||||
expect(renderSrc).toContain('isCanaryEnabled("de-parallel-router")');
|
||||
expect(renderSrc).toContain("deParallelRouterTrialFired");
|
||||
});
|
||||
|
||||
it("has in-range percentages and a parseable sunset date", () => {
|
||||
|
||||
@@ -83,7 +83,19 @@ export const CANARIES: readonly CanaryDefinition[] = [
|
||||
// ── Real rollouts ────────────────────────────────────────────────────────
|
||||
{
|
||||
name: "de-parallel-router",
|
||||
percentage: 0,
|
||||
// Ramp 5 -> 25 -> 100. This gates the DEFAULT-ON behaviour (uncapped, no
|
||||
// telemetry precondition), not the old capped trial — so 0 means the
|
||||
// router is off for everyone and is a full revert without a release.
|
||||
//
|
||||
// Calibration validated the bucketer first: 9.62%/49.76% against 10%/50%
|
||||
// targets at n=13,547, overrides and CI both attributable, sustained
|
||||
// cohort flips at 0.10% — an order of magnitude under this feature's own
|
||||
// ~2.79% revert rate.
|
||||
//
|
||||
// At each step split revert rate by cpu_count and is_docker. Hold at 5
|
||||
// until PRINFRA-372 is resolved: `--workers auto` crashes every worker on
|
||||
// macOS arm64 while `--workers 1` is clean, and the router forces 3.
|
||||
percentage: 5,
|
||||
description:
|
||||
"Route auto multi-worker renders to verified parallel drawElement streaming (HF_DE_PARALLEL_ROUTER). Ramp only alongside the per-install circuit breaker.",
|
||||
owner: "vance",
|
||||
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
shouldRetryViaPinnedFallback,
|
||||
countElementTags,
|
||||
envInt,
|
||||
isDeParallelRouterEnabled,
|
||||
mergeWorkerInitObservability,
|
||||
resolveCompositionElementCount,
|
||||
resolveDeShortBand,
|
||||
@@ -2266,6 +2267,27 @@ describe("resolveInversionRetryPlan (self-verify retry rollback)", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("isDeParallelRouterEnabled (kill switch parsing)", () => {
|
||||
it("defaults ON when unset or set-but-empty", () => {
|
||||
expect(isDeParallelRouterEnabled({})).toBe(true);
|
||||
expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "" })).toBe(true);
|
||||
expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: " " })).toBe(true);
|
||||
});
|
||||
|
||||
it("honours every conventional spelling of off — an opt-out must never fail OPEN", () => {
|
||||
// A naive `!== "false"` would enable the router for all of these, handing
|
||||
// 3-worker parallel DE to a user who explicitly asked for none.
|
||||
for (const v of ["false", "FALSE", "False", "0", "off", "OFF", "no", "No", " false "]) {
|
||||
expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: v })).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("treats any other value as enabled", () => {
|
||||
expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "true" })).toBe(true);
|
||||
expect(isDeParallelRouterEnabled({ HF_DE_PARALLEL_ROUTER: "1" })).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("shouldPreferParallelDrawElement (DE parallel router)", () => {
|
||||
const eligible = {
|
||||
workerCount: 5,
|
||||
|
||||
@@ -1564,11 +1564,12 @@ export function resolveInversionRetryPlan(args: {
|
||||
* clear 1.25x (3,600f, 39% static/dedup-heavy) still didn't LOSE to single-
|
||||
* worker (1.16x) — dedup already skips the capture work parallelism would
|
||||
* split, so there's mechanically less headroom, not a regression. No comp
|
||||
* anywhere showed par3 < single. Default-off (HF_DE_PARALLEL_ROUTER): this
|
||||
* promotes the opt-in mechanism from #2056 into the auto-routing decision,
|
||||
* but the decision itself stays gated behind its own flag pending the
|
||||
* telemetry soak (revert rate, de_verify_min_db distribution) on real wild
|
||||
* traffic — there is currently none, since nothing routes here by default.
|
||||
* anywhere showed par3 < single. Default ON since 2026-07-27
|
||||
* (HF_DE_PARALLEL_ROUTER=false is the kill switch): the default-off soak
|
||||
* proved the safety half (zero shipped damage, 100% revert recovery), so the
|
||||
* flip trades an accepted ~2.3% revert rate for parallelizing the ≥700f
|
||||
* band. This promotes the opt-in mechanism from #2056 into the auto-routing
|
||||
* decision.
|
||||
* Takes priority over the single-worker inversion when both would fire.
|
||||
* Re-calibrated 2026-07-27: a controlled crossover sweep (three content
|
||||
* profiles including a genuinely init-expensive 24-sub-composition comp;
|
||||
@@ -1578,6 +1579,29 @@ export function resolveInversionRetryPlan(args: {
|
||||
* dropped below the inversion's threshold (700 vs 900): where both fire,
|
||||
* parallel wins over the inversion's single-worker pick (+17–21% at 700f).
|
||||
*/
|
||||
/**
|
||||
* Is the DE parallel router enabled for this process?
|
||||
*
|
||||
* Default ON since 2026-07-27; `HF_DE_PARALLEL_ROUTER` is the kill switch.
|
||||
* Every conventional spelling of "off" disables it — a naive
|
||||
* `!== "false"` would silently ignore `0`, `off`, `no`, `FALSE`, and an
|
||||
* exported-but-empty var, i.e. an opt-out that FAILS OPEN and hands the user
|
||||
* 3-worker parallel DE anyway (review finding). A set-but-empty value means
|
||||
* "unset" here, matching how the sibling HF_DE_* numeric knobs treat it.
|
||||
*
|
||||
* The CLI's circuit breaker relies on this accepting an explicit "false":
|
||||
* once an install trips the breaker it writes that value rather than
|
||||
* unsetting the var, because under a default-ON flag unsetting means ON.
|
||||
* Pure; exported for tests.
|
||||
*/
|
||||
export function isDeParallelRouterEnabled(
|
||||
env: Readonly<Record<string, string | undefined>>,
|
||||
): boolean {
|
||||
const raw = env.HF_DE_PARALLEL_ROUTER?.trim().toLowerCase();
|
||||
if (raw === undefined || raw === "") return true;
|
||||
return !(raw === "false" || raw === "0" || raw === "off" || raw === "no");
|
||||
}
|
||||
|
||||
export function shouldPreferParallelDrawElement(args: {
|
||||
workerCount: number;
|
||||
/** job.config.workers — a number means the user explicitly chose. */
|
||||
@@ -1593,7 +1617,7 @@ export function shouldPreferParallelDrawElement(args: {
|
||||
supersampling: boolean;
|
||||
probeDeGated: boolean;
|
||||
experimentalParallelDeOptIn: boolean;
|
||||
/** HF_DE_PARALLEL_ROUTER === "true" — the router's own kill switch, default off. */
|
||||
/** HF_DE_PARALLEL_ROUTER !== "false" — default ON since 2026-07-27; env var is the kill switch. */
|
||||
routerEnabled: boolean;
|
||||
/**
|
||||
* Whether verified parallel DE STREAMING can actually run for this render
|
||||
@@ -2732,7 +2756,17 @@ async function executeRenderPipeline(input: {
|
||||
? Math.min(deSingleMinFrames, deShortBandMinFrames)
|
||||
: deSingleMinFrames;
|
||||
// DE parallel-router eligibility — see shouldPreferParallelDrawElement.
|
||||
// Default-off (HF_DE_PARALLEL_ROUTER); HF_DE_PARALLEL_MIN_FRAMES default
|
||||
// Default ON since 2026-07-27 (kill switch: HF_DE_PARALLEL_ROUTER=false).
|
||||
// The soak that gated this flip answered the safety question: zero
|
||||
// damaged frames shipped across the entire default-off window — every
|
||||
// revert was the self-verify net catching a bad frame and recovering via
|
||||
// screenshot. The residual metric (revert rate ~2.3% vs the 2% goal) is
|
||||
// an efficiency cost (a revert forfeits the speedup, never correctness),
|
||||
// accepted in exchange for parallelizing the ≥700-frame band (~80% of
|
||||
// all DE capture wall-clock). Post-flip tripwire on dashboard 1807532:
|
||||
// sustained revert >10% or any verify-missed damage rolls this back —
|
||||
// one env default, decoupled from the floor change one release earlier.
|
||||
// HF_DE_PARALLEL_MIN_FRAMES default
|
||||
// 700, re-calibrated 2026-07-27 from the original safe-high 2000. A
|
||||
// controlled frame-count sweep (fixed content-per-frame, three synthetic
|
||||
// profiles × {350..3000f} × {single,par2,par3} × 3 reps, worker counts +
|
||||
@@ -2744,7 +2778,7 @@ async function executeRenderPipeline(input: {
|
||||
// duplicated init costs CPU, not wall-clock. Below ~700f the win thins
|
||||
// toward ~+10% while still paying 3 hardware-GPU browsers, so the floor
|
||||
// stays. Harness: plans/drawelement-fast-capture/de-crossover-bench.sh.
|
||||
const deParallelRouterEnabled = process.env.HF_DE_PARALLEL_ROUTER === "true";
|
||||
const deParallelRouterEnabled = isDeParallelRouterEnabled(process.env);
|
||||
const deParallelMinFramesRaw = process.env.HF_DE_PARALLEL_MIN_FRAMES;
|
||||
const deParallelMinFramesNum =
|
||||
deParallelMinFramesRaw === undefined || deParallelMinFramesRaw.trim() === ""
|
||||
|
||||
Reference in New Issue
Block a user