From af923d947c28c3306dbd05e0ecc2b9bd4961df72 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 00:54:54 -0700 Subject: [PATCH] fix(producer,cli): round the fallback dB consistently, thread the verify threshold through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #2411 (Rames): the crash-survival RenderCaptureObservability mirror passed deFallbackFailedDb raw/unrounded while the render_complete perfSummary path rounded to 1 decimal — the same underlying PSNR could ship two different values to PostHog depending on which event fired. Extracted the existing inline round/clamp expression (previously duplicated for verifyMinDb and fallbackFailedDb) into a shared roundDb helper, applied once at the single point deFallbackFailedDb is derived from the thrown error so both downstream consumers agree. Also threads verifyThresholdDb (captured on the error but never propagated, per the nit) through DrawElementPerfInput/RenderCaptureObservability/render.ts/ telemetry as de_fallback_threshold_db on both events — the HF_DE_VERIFY_MIN_DB value the failing dB breached, letting ops read "28.4dB failed a 32dB threshold" directly instead of cross-referencing config. --- packages/cli/src/commands/render.ts | 1 + packages/cli/src/telemetry/events.test.ts | 8 ++++-- packages/cli/src/telemetry/events.ts | 4 +++ .../src/telemetry/renderObservability.test.ts | 7 +++-- .../cli/src/telemetry/renderObservability.ts | 1 + .../src/services/render/observability.ts | 2 ++ .../src/services/render/perfSummary.test.ts | 22 ++++++++++++++++ .../src/services/render/perfSummary.ts | 26 +++++++++++++------ .../src/services/renderOrchestrator.ts | 15 +++++++++-- 9 files changed, 72 insertions(+), 14 deletions(-) create mode 100644 packages/producer/src/services/render/perfSummary.test.ts diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index 9e5d2d702..979c55746 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -2066,6 +2066,7 @@ function trackRenderMetrics( deFallbackReason: perf?.drawElement?.fallbackReason, deFallbackFailedDb: perf?.drawElement?.fallbackFailedDb, deFallbackFrameIndex: perf?.drawElement?.fallbackFrameIndex, + deFallbackThresholdDb: perf?.drawElement?.fallbackThresholdDb, deBlankSuspects: perf?.drawElement?.blankSuspects, deBlankDeterministicAccepts: perf?.drawElement?.blankDeterministicAccepts, deBlankRecaptures: perf?.drawElement?.blankRecaptures, diff --git a/packages/cli/src/telemetry/events.test.ts b/packages/cli/src/telemetry/events.test.ts index 016a05ae2..9ee0b8c04 100644 --- a/packages/cli/src/telemetry/events.test.ts +++ b/packages/cli/src/telemetry/events.test.ts @@ -252,7 +252,7 @@ describe("render telemetry events", () => { ); }); - it("carries the failing dB and frame index on render_error for a psnr fallback that failed hard afterward", () => { + it("carries the failing dB, frame index, and threshold on render_error for a psnr fallback that failed hard afterward", () => { trackRenderError({ fps: 30, quality: "standard", @@ -263,6 +263,7 @@ describe("render telemetry events", () => { captureDeFallbackReason: "psnr", captureDeFallbackFailedDb: 28.4, captureDeFallbackFrameIndex: 649, + captureDeFallbackThresholdDb: 32, }); expect(trackEvent).toHaveBeenCalledWith( @@ -271,6 +272,7 @@ describe("render telemetry events", () => { de_fallback_reason: "psnr", de_fallback_failed_db: 28.4, de_fallback_frame_index: 649, + de_fallback_threshold_db: 32, }), undefined, ); @@ -296,7 +298,7 @@ describe("render telemetry events", () => { ); }); - it("carries the perfSummary-sourced failing dB and frame index on render_complete", () => { + it("carries the perfSummary-sourced failing dB, frame index, and threshold on render_complete", () => { trackRenderComplete({ durationMs: 1000, fps: 30, @@ -307,6 +309,7 @@ describe("render telemetry events", () => { deFallbackReason: "psnr", deFallbackFailedDb: 28.4, deFallbackFrameIndex: 649, + deFallbackThresholdDb: 32, }); expect(trackEvent).toHaveBeenCalledWith( @@ -315,6 +318,7 @@ describe("render telemetry events", () => { de_fallback_reason: "psnr", de_fallback_failed_db: 28.4, de_fallback_frame_index: 649, + de_fallback_threshold_db: 32, }), undefined, ); diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index 3a17624df..9e100ae3e 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -56,6 +56,7 @@ export interface RenderObservabilityTelemetryPayload { captureDeFallbackReason?: string; captureDeFallbackFailedDb?: number; captureDeFallbackFrameIndex?: number; + captureDeFallbackThresholdDb?: number; /** Non-DE parallel-streaming router outcome ("screenshot" | "beginframe" — * routed; "eligible_off" — would route but the kill switch is off). */ captureParallelStream?: string; @@ -112,6 +113,7 @@ function renderObservabilityEventProperties(props: RenderObservabilityTelemetryP de_fallback_reason: props.captureDeFallbackReason, de_fallback_failed_db: props.captureDeFallbackFailedDb, de_fallback_frame_index: props.captureDeFallbackFrameIndex, + de_fallback_threshold_db: props.captureDeFallbackThresholdDb, capture_parallel_stream: props.captureParallelStream, observability_extract_video_count: props.observabilityExtractVideoCount, observability_extracted_video_count: props.observabilityExtractedVideoCount, @@ -179,6 +181,7 @@ export function trackRenderComplete( deFallbackReason?: string; deFallbackFailedDb?: number; deFallbackFrameIndex?: number; + deFallbackThresholdDb?: number; deBlankSuspects?: number; deBlankDeterministicAccepts?: number; deBlankRecaptures?: number; @@ -268,6 +271,7 @@ export function trackRenderComplete( de_fallback_reason: props.deFallbackReason, de_fallback_failed_db: props.deFallbackFailedDb, de_fallback_frame_index: props.deFallbackFrameIndex, + de_fallback_threshold_db: props.deFallbackThresholdDb, de_blank_suspects: props.deBlankSuspects, de_blank_deterministic_accepts: props.deBlankDeterministicAccepts, de_blank_recaptures: props.deBlankRecaptures, diff --git a/packages/cli/src/telemetry/renderObservability.test.ts b/packages/cli/src/telemetry/renderObservability.test.ts index bca360959..97e5d58e9 100644 --- a/packages/cli/src/telemetry/renderObservability.test.ts +++ b/packages/cli/src/telemetry/renderObservability.test.ts @@ -82,25 +82,28 @@ describe("renderObservabilityTelemetryPayload — DE inversion/router cohort (fa expect(payload.captureDeFallbackReason).toBeUndefined(); }); - it("carries the failing dB and frame index for a psnr fallback, still visible on a hard failure", () => { + it("carries the failing dB, frame index, and threshold for a psnr fallback, still visible on a hard failure", () => { const payload = renderObservabilityTelemetryPayload( makeSummary({ deParallelRouter: "routed", deFallbackReason: "psnr", deFallbackFailedDb: 28.4, deFallbackFrameIndex: 649, + deFallbackThresholdDb: 32, }), ); expect(payload.captureDeFallbackFailedDb).toBe(28.4); expect(payload.captureDeFallbackFrameIndex).toBe(649); + expect(payload.captureDeFallbackThresholdDb).toBe(32); }); - it("leaves failedDb undefined for a blank/oom/capture_error fallback (no PSNR score exists)", () => { + it("leaves failedDb/thresholdDb undefined for a blank/oom/capture_error fallback (no PSNR score exists)", () => { const payload = renderObservabilityTelemetryPayload( makeSummary({ deFallbackReason: "oom", deFallbackFrameIndex: undefined }), ); expect(payload.captureDeFallbackFailedDb).toBeUndefined(); expect(payload.captureDeFallbackFrameIndex).toBeUndefined(); + expect(payload.captureDeFallbackThresholdDb).toBeUndefined(); }); }); diff --git a/packages/cli/src/telemetry/renderObservability.ts b/packages/cli/src/telemetry/renderObservability.ts index c3cff4176..3df009f4d 100644 --- a/packages/cli/src/telemetry/renderObservability.ts +++ b/packages/cli/src/telemetry/renderObservability.ts @@ -48,6 +48,7 @@ export function renderObservabilityTelemetryPayload( captureDeFallbackReason: capture.deFallbackReason, captureDeFallbackFailedDb: capture.deFallbackFailedDb, captureDeFallbackFrameIndex: capture.deFallbackFrameIndex, + captureDeFallbackThresholdDb: capture.deFallbackThresholdDb, captureParallelStream: capture.captureParallelStream, observabilityExtractVideoCount: extraction?.videoCount, observabilityExtractedVideoCount: extraction?.extractedVideoCount, diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index f5fa325d4..5c7757d8f 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -64,6 +64,8 @@ export interface RenderCaptureObservability { deFallbackFailedDb?: number; /** Frame index the verification failure was detected at; set for both "psnr" and "blank" fallback reasons. */ deFallbackFrameIndex?: number; + /** The HF_DE_VERIFY_MIN_DB threshold the failing dB breached; only set alongside deFallbackFailedDb (psnr reason). */ + deFallbackThresholdDb?: number; /** Auto-parallel inversion outcome: "inverted" (fired, held) | "reverted" (fired, self-verify retry rolled back). */ deWorkerInversion?: "inverted" | "reverted"; /** Worker count the resolver would have used absent the inversion; undefined if it never fired. */ diff --git a/packages/producer/src/services/render/perfSummary.test.ts b/packages/producer/src/services/render/perfSummary.test.ts new file mode 100644 index 000000000..c56e554b9 --- /dev/null +++ b/packages/producer/src/services/render/perfSummary.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from "vitest"; +import { roundDb } from "./perfSummary.js"; + +describe("roundDb", () => { + it("rounds to 1 decimal", () => { + expect(roundDb(28.4373)).toBe(28.4); + expect(roundDb(28.45)).toBe(28.5); + }); + + it("clamps at 999 (an Infinity PSNR must never ship literally to telemetry)", () => { + expect(roundDb(Infinity)).toBe(999); + expect(roundDb(50000)).toBe(999); + }); + + it("passes undefined through unchanged", () => { + expect(roundDb(undefined)).toBeUndefined(); + }); + + it("is idempotent — rounding an already-rounded value is a no-op", () => { + expect(roundDb(roundDb(28.4373))).toBe(28.4); + }); +}); diff --git a/packages/producer/src/services/render/perfSummary.ts b/packages/producer/src/services/render/perfSummary.ts index fbecc64e1..1b0b790dd 100644 --- a/packages/producer/src/services/render/perfSummary.ts +++ b/packages/producer/src/services/render/perfSummary.ts @@ -59,6 +59,19 @@ export function pushWorkerDedupPerfs( * render-level drawElement outcome. mode/gateReason |-join distinct values * across workers (bounded cardinality); counters SUM. */ +/** + * Round a dB value to 1 decimal and clamp at 999 (an `Infinity` PSNR — a + * bit-exact frame match — must not ship literally to telemetry). Single + * source of truth for every dB field crossing into `RenderPerfSummary` or + * `RenderCaptureObservability` — both must agree byte-for-byte so PostHog + * consumers joining `render_complete` against the crash-survival + * `render_error` mirror never see the same underlying score reported at two + * different precisions (review finding). + */ +export function roundDb(value: number | undefined): number | undefined { + return value === undefined ? undefined : Math.round(Math.min(value, 999) * 10) / 10; +} + /** Orchestrator-supplied render-level drawElement outcome (one shape, used by * both the aggregate function and buildRenderPerfSummary's input). */ export interface DrawElementPerfInput { @@ -76,6 +89,8 @@ export interface DrawElementPerfInput { fallbackFailedDb?: number; /** Frame index the verification failure was detected at; set for both "psnr" and "blank". */ fallbackFrameIndex?: number; + /** The HF_DE_VERIFY_MIN_DB threshold the failing dB breached; only set alongside fallbackFailedDb. */ + fallbackThresholdDb?: number; drainStats?: { verifyChecked: number; verifyMinDb?: number; @@ -109,18 +124,13 @@ function aggregateDrawElement( workerEncode: perfs.some((p) => p.deWorkerEncode), verifyArmed: perfs.reduce((sum, p) => sum + (p.deVerifyArmed ?? 0), 0), verifyChecked: drain?.verifyChecked ?? 0, - verifyMinDb: - drain?.verifyMinDb === undefined - ? undefined - : Math.round(Math.min(drain.verifyMinDb, 999) * 10) / 10, + verifyMinDb: roundDb(drain?.verifyMinDb), verifyInitMs: perfs.reduce((sum, p) => sum + (p.deVerifyInitMs ?? 0), 0), selfVerifyFallback: de.selfVerifyFallback, fallbackReason: de.fallbackReason, - fallbackFailedDb: - de.fallbackFailedDb === undefined - ? undefined - : Math.round(Math.min(de.fallbackFailedDb, 999) * 10) / 10, + fallbackFailedDb: roundDb(de.fallbackFailedDb), fallbackFrameIndex: de.fallbackFrameIndex, + fallbackThresholdDb: roundDb(de.fallbackThresholdDb), blankSuspects: drain?.blankSuspects ?? 0, blankDeterministicAccepts: drain?.blankDeterministicAccepts ?? 0, blankRecaptures: drain?.blankRecaptures ?? 0, diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 6e7592309..d2d804d89 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -103,6 +103,7 @@ import { resolveEffectiveHdrMode } from "./render/hdrMode.js"; import { buildRenderPerfSummary, pushWorkerDedupPerfs, + roundDb, worstSubTimelineWaitOutcome, } from "./render/perfSummary.js"; import { getCaptureStageBrowserConsole } from "./render/captureStageError.js"; @@ -454,6 +455,8 @@ export interface RenderPerfSummary { fallbackFailedDb?: number; /** Frame index the verification failure was detected at; set for both "psnr" and "blank" fallback reasons. */ fallbackFrameIndex?: number; + /** The HF_DE_VERIFY_MIN_DB threshold the failing dB breached; only set alongside fallbackFailedDb (psnr reason). */ + fallbackThresholdDb?: number; /** Blank-guard counters. */ blankSuspects: number; blankDeterministicAccepts: number; @@ -1744,9 +1747,14 @@ export async function executeRenderJob( let deFallbackReason: string | undefined; // Structured detail behind deFallbackReason's "blank"/"psnr" bucket — the // failing dB and frame index otherwise only exist as text inside the - // thrown error's message, unavailable to telemetry. + // thrown error's message, unavailable to telemetry. Rounded once here + // (roundDb) so both downstream consumers — the render_complete + // perfSummary path and the crash-survival RenderCaptureObservability + // mirror — report the identical dB, not two different precisions for + // the same underlying score (review finding). let deFallbackFailedDb: number | undefined; let deFallbackFrameIndex: number | undefined; + let deFallbackThresholdDb: number | undefined; let deDrainStats: import("./render/stages/captureStreamingStage.js").DeDrainStats | undefined; updateCaptureObservability({ forceScreenshot: captureForceScreenshot }); observability.checkpoint("compile", "composition metadata resolved", { @@ -2730,8 +2738,9 @@ export async function executeRenderJob( : "capture_error"; if (isVerifyError) { const verifyDetails = getDrawElementVerificationDetails(err); - deFallbackFailedDb = verifyDetails?.failedDb; + deFallbackFailedDb = roundDb(verifyDetails?.failedDb); deFallbackFrameIndex = verifyDetails?.frameIndex; + deFallbackThresholdDb = roundDb(verifyDetails?.verifyThresholdDb); } log.warn( isVerifyError @@ -2752,6 +2761,7 @@ export async function executeRenderJob( deFallbackReason, deFallbackFailedDb, deFallbackFrameIndex, + deFallbackThresholdDb, }); probeSession = null; // Must clear BEFORE resolveParallelRouterRetryPlan recomputes @@ -3042,6 +3052,7 @@ export async function executeRenderJob( fallbackReason: deFallbackReason, fallbackFailedDb: deFallbackFailedDb, fallbackFrameIndex: deFallbackFrameIndex, + fallbackThresholdDb: deFallbackThresholdDb, drainStats: deDrainStats, }, hdrDiagnostics,