mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-01 19:42:03 +00:00
feat(cli,core): ramp the default-on router through the canary
Rebased onto main (was 308 behind) and gated the new default-on behaviour on the de-parallel-router canary, at 5%. Default-ON without a ramp is a ~17x exposure jump: from ~6% of eligible renders today to all of them, landing on profiles the opt-in trial never covered (<=4 CPUs and Docker, ~12% of eligible renders between them). 0.7.60-0.7.64 is why that matters — every unclamped render reverted for five consecutive releases and nobody noticed. The gate reuses the breaker's own disarm: non-enrolled installs get an explicit HF_DE_PARALLEL_ROUTER=false, because with default-ON polarity deleting the var means ON. Setting the registry percentage to 0 is therefore a full fleet-wide revert with no release. Today's ~11% of installs routing is emergent — the product of eligibility rules and a capped trial — so it drifts with fleet composition and cannot be turned off without shipping. The point of the canary is that the number becomes chosen and revertible, not that it is smaller. Also replaces the registry test that pinned the percentage to 0. Its intent was 'ramp only alongside the circuit breaker', but pinning 0 blocks the ramp forever and never checks the wiring it names. It now asserts the wiring directly, and fails if either the canary gate or the breaker consult is removed. Hold at 5% until PRINFRA-372 resolves: --workers auto crashes every worker on macOS arm64 while --workers 1 is clean, and the router forces 3 workers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
af535080a2
commit
4a2514232b
@@ -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;
|
||||
@@ -740,6 +749,7 @@ 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/
|
||||
@@ -777,6 +787,55 @@ 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;
|
||||
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
|
||||
|
||||
@@ -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";
|
||||
@@ -1224,6 +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 the opt-in
|
||||
// trial never covered (<=4 CPUs, Docker: ~12% of eligible renders between
|
||||
// them). 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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user