diff --git a/packages/cli/src/telemetry/events.test.ts b/packages/cli/src/telemetry/events.test.ts index 9ee0b8c04..d614f6723 100644 --- a/packages/cli/src/telemetry/events.test.ts +++ b/packages/cli/src/telemetry/events.test.ts @@ -2,9 +2,20 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; const trackEvent = vi.fn(); const flush = vi.fn(() => Promise.resolve()); +const shouldTrack = vi.fn(() => true); vi.mock("./client.js", () => ({ trackEvent: (...args: unknown[]) => trackEvent(...args), flush: () => flush(), + shouldTrack: () => shouldTrack(), +})); + +// Power state shells out to `pmset`; spy so tests can assert it is NOT +// sampled for opted-out installs (the fields are built at the call site, +// before trackEvent's own shouldTrack guard). +const getPowerState = vi.fn(() => ({ on_battery: true, low_power_mode: false })); +vi.mock("./system.js", async () => ({ + ...(await vi.importActual("./system.js")), + getPowerState: () => getPowerState(), })); // identifyUser reads the install anonymousId; pin it so the $identify alias is @@ -653,3 +664,28 @@ describe("auth login telemetry events", () => { expect(trackEvent).not.toHaveBeenCalled(); }); }); + +describe("power-state sampling respects the telemetry opt-out", () => { + beforeEach(() => { + getPowerState.mockClear(); + shouldTrack.mockReturnValue(true); + }); + + it("samples power state for a tracked render", () => { + trackRenderComplete({ durationMs: 1, fps: 30, quality: "high", docker: false }); + expect(getPowerState).toHaveBeenCalled(); + const props = trackEvent.mock.calls.at(-1)?.[1] as Record; + expect(props.on_battery).toBe(true); + expect(props.low_power_mode).toBe(false); + }); + + it("does NOT spawn pmset when telemetry is disabled", () => { + // Regression: powerStateFields() is spread into the properties object at + // the call site, so it runs BEFORE trackEvent's `if (!shouldTrack())` + // guard — an opted-out install would otherwise pay two blocking + // subprocess spawns per render for an event that is then discarded. + shouldTrack.mockReturnValue(false); + trackRenderComplete({ durationMs: 1, fps: 30, quality: "high", docker: false }); + expect(getPowerState).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index bfe905617..08c2cd36f 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -1,7 +1,7 @@ import { redactTelemetryString, type OutputResolutionIssueKind } from "@hyperframes/core"; import type { SubTimelineWaitOutcome } from "@hyperframes/engine"; import { FEEDBACK_RATING_SCALE } from "../utils/feedbackRating.js"; -import { flush, trackEvent } from "./client.js"; +import { flush, shouldTrack, trackEvent } from "./client.js"; import { readConfig } from "./config.js"; import { getPowerState } from "./system.js"; @@ -10,7 +10,15 @@ import { getPowerState } from "./system.js"; // render_complete AND render_error: the DE fleet is macOS laptops whose // power management shifts render perf ~1.8x with no other telemetry signal, // and perf/soak analysis needs to segment by it (see getPowerState). +// +// shouldTrack() is checked HERE, not just inside trackEvent: this helper is +// spread into the properties object at the CALL SITE, so it runs before +// trackEvent's own `if (!shouldTrack()) return` guard. Without this an +// opted-out install would still pay two blocking `pmset` subprocess spawns +// per render for an event that is then discarded (review finding). +// shouldTrack() memoizes, so this costs nothing on the tracked path. function powerStateFields(): { on_battery?: boolean; low_power_mode?: boolean } { + if (!shouldTrack()) return {}; const power = getPowerState(); return { on_battery: power.on_battery ?? undefined, diff --git a/packages/producer/src/services/renderOrchestrator.test.ts b/packages/producer/src/services/renderOrchestrator.test.ts index e900b68f2..ff34b7e17 100644 --- a/packages/producer/src/services/renderOrchestrator.test.ts +++ b/packages/producer/src/services/renderOrchestrator.test.ts @@ -1842,6 +1842,7 @@ describe("shouldPreferParallelDrawElement (DE parallel router)", () => { probeDeGated: false, experimentalParallelDeOptIn: false, routerEnabled: true, + parallelStreamingAvailable: true, totalMemoryMb: 32768, minMemoryMb: 24576, }; @@ -1850,6 +1851,16 @@ describe("shouldPreferParallelDrawElement (DE parallel router)", () => { expect(shouldPreferParallelDrawElement(eligible)).toBe(true); }); + it("withholds the bet when parallel streaming can't run (e.g. over the duration cap)", () => { + // The router pins workerCount to 3 and skips calibration to serve the + // verified parallel DE STREAMING path. If streaming is off for this + // render — the >240s duration cap is the common case — firing would pay + // the whole cost of the pin for none of the benefit. + expect( + shouldPreferParallelDrawElement({ ...eligible, parallelStreamingAvailable: false }), + ).toBe(false); + }); + it("withholds the parallel bet below the RAM floor (16 GB black-slab report)", () => { expect(shouldPreferParallelDrawElement({ ...eligible, totalMemoryMb: 16384 })).toBe(false); }); diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 1bd7ae4c6..79f4e6494 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1331,6 +1331,16 @@ export function shouldPreferParallelDrawElement(args: { experimentalParallelDeOptIn: boolean; /** HF_DE_PARALLEL_ROUTER === "true" — the router's own kill switch, default off. */ routerEnabled: boolean; + /** + * Whether verified parallel DE STREAMING can actually run for this render + * (`shouldUseStreamingEncode` at the router's worker count with + * forceParallelStream). The router's entire value is that path; without it + * firing would pin workerCount to 3 and skip calibration while delivering + * none of the benefit — e.g. a composition longer than + * `streamingEncodeMaxDurationSeconds` (240 s default), where the duration + * cap disables streaming before the router's force flag is consulted. + */ + parallelStreamingAvailable: boolean; /** Machine RAM (os.totalmem, MB). */ totalMemoryMb: number; /** RAM floor for routing; <=0 disables the guard. */ @@ -1338,6 +1348,7 @@ export function shouldPreferParallelDrawElement(args: { }): boolean { return ( args.routerEnabled && + args.parallelStreamingAvailable && args.workerCount > 1 && typeof args.requestedWorkers !== "number" && args.useDrawElement && @@ -2366,6 +2377,15 @@ async function executeRenderPipeline(input: { process.env.PRODUCER_EXPERIMENTAL_FAST_CAPTURE === "true" || process.env.HF_DE_PARALLEL_STREAM === "true", routerEnabled: deParallelRouterEnabled, + // Router pins 3 workers for the streaming path; don't pin when the + // duration cap (or any other streaming gate) would turn that path off. + parallelStreamingAvailable: shouldUseStreamingEncode( + cfg, + outputFormat, + 3, + job.duration, + true, + ), totalMemoryMb: Math.round(totalmem() / (1024 * 1024)), minMemoryMb: deParallelMinMemoryMb, });