Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 137 additions & 67 deletions packages/cli/src/commands/render.test.ts

Large diffs are not rendered by default.

350 changes: 202 additions & 148 deletions packages/cli/src/commands/render.ts

Large diffs are not rendered by default.

5 changes: 2 additions & 3 deletions packages/cli/src/commands/render/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
22 changes: 20 additions & 2 deletions packages/core/src/canary.test.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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", () => {
Expand Down
14 changes: 13 additions & 1 deletion packages/core/src/canaryRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
22 changes: 22 additions & 0 deletions packages/producer/src/services/renderOrchestrator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
shouldRetryViaPinnedFallback,
countElementTags,
envInt,
isDeParallelRouterEnabled,
mergeWorkerInitObservability,
resolveCompositionElementCount,
resolveDeShortBand,
Expand Down Expand Up @@ -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,
Expand Down
50 changes: 42 additions & 8 deletions packages/producer/src/services/renderOrchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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. */
Expand All @@ -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
Expand Down Expand Up @@ -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 +
Expand All @@ -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() === ""
Expand Down
Loading