mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-08-31 02:41:44 +00:00
feat(core,cli): ship the DE parallel router fleet-wide — remove the canary gate (#3120)
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user