Skip to content

Commit 1f91a35

Browse files
vanceingallsclaude
andcommitted
feat(producer): enable the parallel-DE router by default, behind a per-install circuit breaker
The DE parallel router (HF_DE_PARALLEL_ROUTER) becomes default-ON. The soak answered the safety question it was gated on: zero damaged frames shipped — every fallback was the self-verification net catching a bad frame and recovering on the screenshot path. Verify PSNR p10 sits flat near 40 dB against a 32 dB floor. The residual 2.31% revert rate is an efficiency cost (a revert forfeits the speedup, never the output), accepted in exchange for parallelizing the >=700-frame band — roughly 80% of all DE capture wall-clock, frame-weighted. Default-ON is safe because the per-install circuit breaker stays underneath it. That distinction matters: 9.8% of installs hit a revert, and they are latched off permanently after the first one. Without the breaker those installs would go from "one slow render, then protected" to "every eligible render is slow". The breaker, adapted for a default-ON flag: - Writes an explicit HF_DE_PARALLEL_ROUTER=false and persists it to ~/.hyperframes/config.json, so the install stays off across processes. Absent no longer means off, so the switch has to be written, not unset. - Trips only on a real fallback, never on render count — a healthy install keeps the speedup indefinitely. - Independent of telemetry state: opting out of analytics must not cost a user the faster renderer. Telemetry governs reporting, not behavior. - An explicit user value wins in both directions, latched before the breaker can write the var and make the two indistinguishable. - The user is told when it trips and how to re-enable. isDeParallelRouterEnabled() parses the kill switch properly: false/0/off/no (case- and space-insensitive) disable; unset or empty is the default. A bare `!== "false"` would silently ignore every spelling but one and hand parallel DE to a user who asked for none. Refs PRINFRA-384 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fa56454 commit 1f91a35

5 files changed

Lines changed: 278 additions & 239 deletions

File tree

packages/cli/src/commands/render.test.ts

Lines changed: 45 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,11 @@ describe("renderLocal browser GPU config", () => {
705705
});
706706
});
707707

708-
describe("renderLocal — DE parallel-router CLI trial", () => {
708+
// Suite renamed with the breaker work: this is no longer an opt-in trial. The
709+
// bindings come from main's shared top-level `renderModule` import rather than
710+
// this suite's own beforeAll — same module instance every other suite uses, so
711+
// module-scope arm/consume state resets through the one `resetTrialState()`.
712+
describe("renderLocal — DE parallel-router circuit breaker", () => {
709713
const { renderLocal, __resetDeParallelRouterTrialStateForTests: resetTrialState } = renderModule;
710714
const savedEnv = new Map<string, string | undefined>();
711715

@@ -748,19 +752,22 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
748752
browserGpuMode: "software" as const,
749753
hdrMode: "auto" as const,
750754
quiet: true,
751-
// The trial is OPT-IN (review): only the CLI's own sequential call sites
752-
// set this. These tests simulate those call sites.
753-
enableDeParallelRouterTrial: true,
755+
// Breaker management is OPT-IN (review): only the CLI's own sequential
756+
// call sites set it. These tests simulate those call sites.
757+
manageDeParallelRouterBreaker: true,
754758
};
755759

756-
it("enables the trial (sets the env var) on a fresh install with telemetry on", async () => {
760+
it("leaves the env var untouched on a fresh install — the router is default-ON", async () => {
761+
// Under the old opt-in trial this armed HF_DE_PARALLEL_ROUTER="true".
762+
// The router now ships on, so the breaker's job is to stay out of the
763+
// way until something actually fails.
757764
configState.disk = {
758765
telemetryEnabled: true,
759766
deParallelRouterTrialFired: false,
760767
telemetryNoticeShown: true,
761768
};
762769
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
763-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
770+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
764771
});
765772

766773
it("does not override an env var the user already set themselves", async () => {
@@ -774,49 +781,30 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
774781
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
775782
});
776783

777-
it("does not enable the trial once it has already fired for this install", async () => {
784+
it("writes an explicit false once the breaker has tripped for this install", async () => {
785+
// THE regression this rework exists for: the old code disabled the
786+
// router by DELETING the var. With a default-ON router, absent means ON,
787+
// so deleting would silently re-enable it on the very host that just
788+
// failed. Only an explicit "false" is a real off-switch.
778789
configState.disk = {
779790
telemetryEnabled: true,
780791
deParallelRouterTrialFired: true,
781792
telemetryNoticeShown: true,
782793
};
783794
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
784-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
785-
});
786-
787-
it("does not enable the trial when shouldTrack() is false (dev mode / DO_NOT_TRACK)", async () => {
788-
configState.disk = {
789-
telemetryEnabled: true,
790-
deParallelRouterTrialFired: false,
791-
telemetryNoticeShown: true,
792-
};
793-
trackingState.shouldTrack = false;
794-
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
795-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
795+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
796796
});
797797

798-
it("does not enable the trial when config.telemetryEnabled is false, even if shouldTrack() is stale-true (e.g. `hyperframes telemetry off` mid-batch)", async () => {
798+
it("keeps the router on for a telemetry opt-out — analytics choice must not cost performance", async () => {
799+
// The old trial refused to arm without recordable telemetry (no point
800+
// running an experiment you can't measure). Now that the router is a
801+
// shipped default, gating it on telemetry would punish a privacy choice
802+
// with a slower renderer.
799803
configState.disk = {
800804
telemetryEnabled: false,
801805
deParallelRouterTrialFired: false,
802806
telemetryNoticeShown: true,
803807
};
804-
trackingState.shouldTrack = true;
805-
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
806-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
807-
});
808-
809-
it("does not enable the trial before the first-run telemetry disclosure has been shown at least once", async () => {
810-
// cli.ts shows this notice via a fire-and-forget, unawaited dynamic
811-
// import — there's no guarantee it printed before renderLocal runs on a
812-
// brand-new install's very first invocation. Requiring
813-
// telemetryNoticeShown means the trial never races an opt-in message
814-
// against the disclosure it depends on.
815-
configState.disk = {
816-
telemetryEnabled: true,
817-
deParallelRouterTrialFired: false,
818-
telemetryNoticeShown: false,
819-
};
820808
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
821809
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
822810
});
@@ -926,11 +914,9 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
926914
});
927915

928916
it("persists a later --batch row's revert even though this process already armed the trial on an earlier row", async () => {
929-
// Regression test for the exact scenario a --batch run hits: multiple
930-
// renderLocal calls in one process. Before the fix, row 2's
931-
// maybeEnableDeParallelRouterTrial saw process.env.HF_DE_PARALLEL_ROUTER
932-
// already "true" (set by row 1) and mistook that for "the user set it",
933-
// returning trialArmed=false — silently dropping row 2's revert.
917+
// The --batch scenario: multiple renderLocal calls in one process. Row 1
918+
// succeeds (breaker stays out of the way, env untouched); row 2 reverts
919+
// and must still be recorded and trip the breaker.
934920
configState.disk = {
935921
telemetryEnabled: true,
936922
deParallelRouterTrialFired: false,
@@ -944,7 +930,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
944930
};
945931
};
946932
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
947-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
933+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
948934
expect(configState.disk.deParallelRouterTrialFired).toBe(false);
949935

950936
producerState.executeImpl = async (job) => {
@@ -958,12 +944,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
958944
expect(configState.writeConfigCalls).toContainEqual(
959945
expect.objectContaining({ deParallelRouterTrialFired: true }),
960946
);
961-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
947+
// Explicit "false", not deleted: with a default-ON router, unsetting the
948+
// var would re-enable it on the host that just reverted.
949+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
962950
});
963951

964952
it("does not arm the trial for programmatic callers that never opted in (opt-in polarity — also covers --batch-concurrency N>=2, which leaves it unset)", async () => {
965953
// The trial's process-wide env var and module-level flags are only safe
966-
// under sequential invocation, so enableDeParallelRouterTrial is OPT-IN
954+
// under sequential invocation, so manageDeParallelRouterBreaker is OPT-IN
967955
// (review): a programmatic renderLocal consumer that doesn't know about
968956
// the trial must get no trial. The CLI's concurrent-batch path relies on
969957
// the same default by leaving the option unset.
@@ -972,7 +960,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
972960
deParallelRouterTrialFired: false,
973961
telemetryNoticeShown: true,
974962
};
975-
const { enableDeParallelRouterTrial: _omitted, ...programmaticOptions } = baseOptions;
963+
const { manageDeParallelRouterBreaker: _omitted, ...programmaticOptions } = baseOptions;
976964
await renderLocal("/tmp/project", "/tmp/out.mp4", programmaticOptions);
977965
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
978966
expect(configState.writeConfigCalls).toHaveLength(0);
@@ -991,7 +979,7 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
991979
};
992980
};
993981
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
994-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
982+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
995983

996984
// A real interactive user can't do this mid-batch, but a wrapper script
997985
// invoking the CLI programmatically in the same process could — the
@@ -1001,7 +989,10 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
1001989
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
1002990
});
1003991

1004-
it("caps exposure at DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS even when the router never reverts", async () => {
992+
it("never trips on healthy renders, however many — the old 25-render cap is gone", async () => {
993+
// The cap was sampling logic for an opt-in experiment. Under a shipped
994+
// default it would switch the feature off behind the user's back after
995+
// 25 good renders.
1005996
configState.disk = {
1006997
telemetryEnabled: true,
1007998
deParallelRouterTrialFired: false,
@@ -1014,40 +1005,14 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
10141005
};
10151006
};
10161007

1017-
for (let i = 0; i < 25; i++) {
1008+
for (let i = 0; i < 30; i++) {
10181009
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
10191010
}
10201011

1021-
expect(configState.writeConfigCalls).toContainEqual(
1022-
expect.objectContaining({
1023-
deParallelRouterTrialFired: true,
1024-
deParallelRouterTrialRenderCount: 25,
1025-
}),
1026-
);
1027-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
1028-
1029-
// The 26th eligible render must not re-arm it.
1030-
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
1031-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
1032-
});
1033-
1034-
it("observes a telemetry opt-out written by another process mid-batch (arm site reads fresh, not cached)", async () => {
1035-
configState.disk = {
1036-
telemetryEnabled: true,
1037-
deParallelRouterTrialFired: false,
1038-
telemetryNoticeShown: true,
1039-
};
1040-
// Row 1 arms and primes the config cache.
1041-
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
1042-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
1043-
1044-
// Another process runs `hyperframes telemetry off`, writing straight to
1045-
// "disk" — this process's cache still says telemetryEnabled: true, so a
1046-
// cached read at the arm site would keep arming (review finding).
1047-
configState.disk = { ...configState.disk, telemetryEnabled: false };
1048-
1049-
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
10501012
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
1013+
expect(
1014+
configState.writeConfigCalls.some((call) => call.deParallelRouterTrialFired === true),
1015+
).toBe(false);
10511016
});
10521017

10531018
it("re-asserts the fired flag when the write is lost (concurrent clobber / transient failure), without re-counting the render", async () => {
@@ -1089,10 +1054,11 @@ describe("renderLocal — DE parallel-router CLI trial", () => {
10891054
// Nothing could persist...
10901055
expect(configState.disk.deParallelRouterTrialFired).toBe(false);
10911056
// ...but the in-process latch still blocks the next render from
1092-
// re-running the experiment that just failed (review finding).
1057+
// re-running the path that just failed (review finding) — and now does
1058+
// it by writing an explicit "false", since absent means ON.
10931059
producerState.executeImpl = async () => undefined;
10941060
await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
1095-
expect(process.env.HF_DE_PARALLEL_ROUTER).toBeUndefined();
1061+
expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
10961062
});
10971063
});
10981064

0 commit comments

Comments
 (0)