From 19defeabfe8fa47747d1d8bc087a84d69a09cb59 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 8 Aug 2026 14:00:19 -0700 Subject: [PATCH] =?UTF-8?q?feat(core,cli):=20ship=20the=20DE=20parallel=20?= =?UTF-8?q?router=20fleet-wide=20=E2=80=94=20remove=20the=20canary=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deletes the `de-parallel-router` canary entry and the `isCanaryEnabled` guard in render.ts together, leaving the producer's default-ON in place. Net effect for users: the parallel drawElement router is on for everyone again. ## Why, and why not a ramp Gating at 5% was itself the regression. Measured 2026-08-08, the day after v0.7.101 shipped the canary: fleet router exposure fell from 3.13-4.25% of non-CI renders to **0.13%**, roughly 25x, because out-of-cohort installs are explicitly disarmed and #2840 deleted the everyone-armed trial in the same change. 2,537 installs lost a feature they already had. Severity is speed only, never output, and nothing is persisted to disk. PR #2840's body claimed "the canary does not make exposure smaller; it makes it chosen and revertible." That was true of the end state and false of the first step. This lands the end state. Entry and guard go together deliberately: at >=100 the evaluator short-circuits ahead of the CI/seedless exclusions, so removing only the entry would have flipped whatever still resolved false at deletion time, unstaged. ## Both stated blockers are void - **≤4-CPU / Docker coverage gap.** Docker renders never use drawElement — 0 of 4,281 across every CPU tier, software GL gates it out — and the router requires it. No percentage could ever expose Docker, so no ramp closes that gap. ≤4 CPUs yields ~42 drawElement candidates in three days. - **PRINFRA-372.** Its signature has hits on 0.4.12, 0.4.37, 0.6.52, 0.6.93, 0.6.109 and 0.6.110 — versions predating drawElement (v0.7.38) and therefore this router. It is real, still live on 0.7.101, and belongs to the screenshot/beginframe path. 11 reproduction runs across four configurations on the enriched profile (darwin/arm64 25.5.0) came back clean. ## Safety unchanged The per-install circuit breaker and the per-render self-verify are untouched; `HF_DE_PARALLEL_ROUTER=false` remains the user-facing kill switch. Post-canary data at 14 days: >8 CPUs 3.02% revert (177/5,857), 5-8 CPUs 2.40% (6/250) — consistent with the 2.75-3.16% baseline. Revert path is now a code revert rather than a registry edit. That is the trade this shape accepts in exchange for one release instead of two. ## Corrects two claims that shipped wrong `~17x jump in exposure onto <=4 CPUs / Docker` overstated the reach, 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 inverts the arithmetic, which is how gating at 5% came to cut exposure rather than ramp it. Both are recorded in render.ts so they are not reintroduced. ## Tests Removed the core wiring assertion and the two CLI canary-gating tests, which pinned a gate that no longer exists. Added the inverse guarantee in its place: an ordinary install must come out of the breaker with the var UNSET so the producer default applies — writing "false" there is precisely what disarmed the fleet at 5%. core 1701 passing, cli 2491 passing, studio canary 29 passing. The 2 failures in play.test.ts reproduce on clean origin/main and are unrelated (#3114 area). oxlint and oxfmt clean. Note: telemetry for this rollout stops with the entry — `$feature/canary-de-parallel-router` and `canary_reason_de_parallel_router` are emitted from the registry, so the `Ramp —` tiles and the exposure-floor alert on PostHog dashboard 1918875 go blank once this ships. Watch drawElement engagement on 1807532 instead. Co-Authored-By: Claude Fable 5 --- packages/cli/src/commands/render.test.ts | 45 +++++------------------- packages/cli/src/commands/render.ts | 39 +++++++++----------- packages/cli/src/telemetry/canary.ts | 2 +- packages/core/src/canary.test.ts | 31 +++------------- packages/core/src/canaryRegistry.ts | 21 ----------- 5 files changed, 31 insertions(+), 107 deletions(-) diff --git a/packages/cli/src/commands/render.test.ts b/packages/cli/src/commands/render.test.ts index 177b3c75b..bddca8486 100644 --- a/packages/cli/src/commands/render.test.ts +++ b/packages/cli/src/commands/render.test.ts @@ -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 @@ -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(), @@ -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; @@ -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/ @@ -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, diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 76dbb8cde..06d03da67 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -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"; @@ -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; } diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index f42aa612b..6d2c0909b 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -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 diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 9bbb11ea8..e58441c92 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -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 { @@ -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) { @@ -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(); }); diff --git a/packages/core/src/canaryRegistry.ts b/packages/core/src/canaryRegistry.ts index 135f2da8e..bd948a0c5 100644 --- a/packages/core/src/canaryRegistry.ts +++ b/packages/core/src/canaryRegistry.ts @@ -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 {