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
45 changes: 8 additions & 37 deletions packages/cli/src/commands/render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ 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
Expand Down Expand Up @@ -176,10 +172,6 @@ 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(),
Expand Down Expand Up @@ -246,7 +238,6 @@ describe("renderLocal browser GPU config", () => {
configState.failMirrors = 0;
configState.writeConfigCalls = [];
trackingState.shouldTrack = true;
trackingState.canaryEnabled = true;
trackingState.renderObservations = [];
ffmpegEncoderState.mode = "software";
ffmpegEncoderState.error = null;
Expand Down Expand Up @@ -749,7 +740,6 @@ describe("renderLocal — DE parallel-router circuit breaker", () => {
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/
Expand Down Expand Up @@ -787,45 +777,26 @@ describe("renderLocal — DE parallel-router circuit breaker", () => {
manageDeParallelRouterBreaker: true,
};

// 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;
// The canary that used to gate this is gone (registry entry + guard removed
// together). The router is now a shipped default for every install, so the
// guarantee worth pinning is the inverse of the old one: an ordinary install
// must come out of the breaker with the var UNSET, so the producer's
// default-ON applies. Writing "false" here would silently disarm the fleet —
// that is exactly what gating at 5% did.
it("leaves the var unset for an ordinary install so the producer default applies", async () => {
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;
it("never overrides an explicit user value", async () => {
configState.disk = {
telemetryEnabled: true,
deParallelRouterTrialFired: false,
Expand Down
39 changes: 17 additions & 22 deletions packages/cli/src/commands/render.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
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";
Expand Down Expand Up @@ -1226,29 +1225,25 @@ function applyDeParallelRouterCircuitBreaker(quiet: boolean): boolean {
return 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.
// Nothing left to gate: the router is a shipped default for every install,
// so leave the var unset and let the producer's default-ON apply. The
// breaker above is the only thing that turns it off, per install, and only
// after a real fallback. `HF_DE_PARALLEL_ROUTER=false` remains the user-
// facing kill switch.
//
// 0.7.60-0.7.64 is why that matters: every unclamped render reverted for
// five consecutive releases and nobody saw it.
// The `de-parallel-router` canary that used to sit here was removed with its
// registry entry (they had to go together — at >=100 the evaluator
// short-circuits ahead of the CI/seedless exclusions, so deleting only the
// entry would have flipped whatever still resolved false at deletion time).
//
// 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;
}
// Two claims from the ramp's rationale were wrong, recorded so they are not
// reintroduced: "~17x jump in exposure onto <=4 CPUs / Docker" overstated
// the reach — Docker renders never use drawElement at all (0 of 4,281
// measured) and the router requires it, so no percentage ever exposed
// Docker. And "~11% of installs already route" was an OUTCOME (the share
// clearing eligibility and the old 25-render cap), not an exposure setting;
// read as a rollout knob it inverted the arithmetic, which is how gating at
// 5% came to CUT fleet exposure ~25x rather than ramp it.
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/telemetry/canary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*
* ```ts
* import { isCanaryEnabled } from "../telemetry/canary.js";
* if (isCanaryEnabled("de-parallel-router")) { ...ramped path... }
* if (isCanaryEnabled("your-feature")) { ...ramped path... }
* ```
*
* That is the whole API. Percentage lives in the registry, not at the call
Expand Down
31 changes: 5 additions & 26 deletions packages/core/src/canary.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
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 @@ -293,29 +291,10 @@ describe("registry", () => {
}
});

// The registry is data, so a ramp is a one-line edit with no code review
// 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.
// 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");
});
// The de-parallel-router wiring assertion that lived here was removed with
// the canary itself (registry entry + render.ts guard, same commit). The
// per-install circuit breaker it referenced is unchanged and is covered by
// the CLI's own render tests.

it("has in-range percentages and a parseable sunset date", () => {
for (const c of CANARIES) {
Expand All @@ -329,7 +308,7 @@ describe("registry", () => {

it("derives the override env var from the name", () => {
expect(canaryEnvVar("de-parallel-router")).toBe("HF_CANARY_DE_PARALLEL_ROUTER");
expect(findCanary("de-parallel-router")?.name).toBe("de-parallel-router");
expect(findCanary("calibration-10")?.name).toBe("calibration-10");
expect(findCanary("nope")).toBeUndefined();
});

Expand Down
21 changes: 0 additions & 21 deletions packages/core/src/canaryRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,27 +80,6 @@ export const CANARIES: readonly CanaryDefinition[] = [
owner: "vance",
sunsetAfter: "2026-09-15",
},
// ── Real rollouts ────────────────────────────────────────────────────────
{
name: "de-parallel-router",
// 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",
sunsetAfter: "2026-10-01",
},
] as const;

export function findCanary(name: string): CanaryDefinition | undefined {
Expand Down
Loading