diff --git a/packages/cli/src/commands/render.ts b/packages/cli/src/commands/render.ts index b14f1a01d8..504eab24c9 100644 --- a/packages/cli/src/commands/render.ts +++ b/packages/cli/src/commands/render.ts @@ -1017,6 +1017,38 @@ function getMemorySnapshot() { }; } +/** + * Sizing + memory props for the failure path. + * + * `job.perfSummary` — where the success path reads these from — is assembled + * only after a render completes, so these come off the job fields the + * orchestrator's unconditional memory-sampler disposer writes + * (`recordJobFailureMetrics`). Extracted from `handleRenderError` to keep its + * branch count down. + * + * Prefers the sampled running peak to the teardown RSS snapshot: the snapshot + * reads whatever RSS happens to be at teardown and misses the mid-render spike + * the field exists to catch. + */ +function failureSizingTelemetry(job: RenderJob | undefined, requestedWorkers: number | undefined) { + const snapshot = getMemorySnapshot(); + const base = { + ...snapshot, + peakMemoryMb: job?.peakRssMb ?? snapshot.peakMemoryMb, + peakHeapUsedMb: job?.peakHeapUsedMb, + }; + const sizing = job?.workerSizing; + if (!sizing) return { ...base, workers: requestedWorkers }; + return { + ...base, + workers: requestedWorkers ?? sizing.workers, + workersBoundBy: sizing.boundBy, + workersHeapBased: sizing.heapBasedWorkers, + workersHeapLimitMb: sizing.heapLimitMb, + workersExceedHeapAdvisory: sizing.exceedsHeapAdvisory, + }; +} + function metaString(meta: Record | undefined, key: string): string | undefined { const value = meta?.[key]; return typeof value === "string" ? value : undefined; @@ -1439,14 +1471,13 @@ function handleRenderError( fps: fpsToNumber(options.fps), quality: options.quality, docker, - workers: options.workers, gpu: options.gpu, authoringSkill: options.authoringSkill, elapsedMs: Date.now() - startTime, errorMessage: message, failedStage, ...renderJobObservabilityTelemetryPayload(job), - ...getMemorySnapshot(), + ...failureSizingTelemetry(job, options.workers), }); // Failed renders join the recent-renders ring too — a bug report filed via // `hyperframes feedback` is MOST likely to be about a failed render. @@ -1599,6 +1630,10 @@ function trackRenderMetrics( extractCacheMisses: extract?.cacheMisses, ...renderJobObservabilityTelemetryPayload(job), ...getMemorySnapshot(), + // Same reason as the failure path: prefer the sampled running peak to the + // teardown snapshot. Falls back when perfSummary is absent (e.g. Docker). + peakMemoryMb: perf?.peakRssMb ?? getMemorySnapshot().peakMemoryMb, + peakHeapUsedMb: perf?.peakHeapUsedMb, }); } diff --git a/packages/cli/src/telemetry/events.test.ts b/packages/cli/src/telemetry/events.test.ts index f7aa33ae34..68a3871bf0 100644 --- a/packages/cli/src/telemetry/events.test.ts +++ b/packages/cli/src/telemetry/events.test.ts @@ -355,6 +355,38 @@ describe("render telemetry events", () => { ); }); + // PRINFRA-341 could not be decided because these props were render_complete + // only: 0 of 317k render_error events carried them, so advisory-true renders + // could never be correlated with failures. Dropping them again re-opens that. + it("carries worker-sizing provenance and sampled peaks on render_error too", () => { + trackRenderError({ + fps: 30, + quality: "high", + docker: false, + workers: 6, + workersBoundBy: "heap", + workersHeapBased: 4, + workersHeapLimitMb: 4144, + workersExceedHeapAdvisory: true, + peakMemoryMb: 1633, + peakHeapUsedMb: 900, + }); + + expect(trackEvent).toHaveBeenCalledWith( + "render_error", + expect.objectContaining({ + workers: 6, + workers_bound_by: "heap", + workers_heap_based: 4, + workers_heap_limit_mb: 4144, + workers_exceed_heap_advisory: true, + peak_memory_mb: 1633, + peak_heap_used_mb: 900, + }), + undefined, + ); + }); + it("ties feedback to its report and recent renders via feedback_id + recent_render_ids", () => { trackRenderFeedback({ rating: 3, @@ -864,3 +896,40 @@ describe("power-state sampling respects the telemetry opt-out", () => { expect(getPowerState).not.toHaveBeenCalled(); }); }); + +// Appended at end of file deliberately: inserting mid-file shifts the line +// numbers of pre-existing clone groups, which makes fallow re-report them as +// new findings on an unrelated change. +describe("render_error worker-sizing provenance", () => { + // PRINFRA-341 could not be decided because these props were render_complete + // only: 0 of 317k render_error events carried them, so advisory-true renders + // could never be correlated with failures. Dropping them reopens that. + it("carries sizing and sampled peaks on the failure path", () => { + trackRenderError({ + fps: 30, + quality: "high", + docker: false, + workers: 6, + workersBoundBy: "heap", + workersHeapBased: 4, + workersHeapLimitMb: 4144, + workersExceedHeapAdvisory: true, + peakMemoryMb: 1633, + peakHeapUsedMb: 900, + }); + const props = trackEvent.mock.calls.at(-1)?.[1] as Record; + expect(props.workers_bound_by).toBe("heap"); + expect(props.workers_heap_based).toBe(4); + expect(props.workers_heap_limit_mb).toBe(4144); + expect(props.workers_exceed_heap_advisory).toBe(true); + expect(props.peak_memory_mb).toBe(1633); + expect(props.peak_heap_used_mb).toBe(900); + }); + + it("omits them when the render failed before sizing was computed", () => { + trackRenderError({ fps: 30, quality: "draft", docker: false }); + const props = trackEvent.mock.calls.at(-1)?.[1] as Record; + expect(props.workers_bound_by).toBeUndefined(); + expect(props.workers_heap_limit_mb).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/telemetry/events.ts b/packages/cli/src/telemetry/events.ts index 838b1cfc30..53337cf29e 100644 --- a/packages/cli/src/telemetry/events.ts +++ b/packages/cli/src/telemetry/events.ts @@ -316,6 +316,8 @@ export function trackRenderComplete( capturePeakMs?: number; // Resource usage peakMemoryMb?: number; + /** Sampled running peak of V8 heapUsed (complement to RSS). */ + peakHeapUsedMb?: number; memoryFreeMb?: number; tmpPeakBytes?: number; // Per-stage timings (subset of RenderPerfSummary.stages) @@ -413,6 +415,7 @@ export function trackRenderComplete( video_count: props.videoCount, capture_peak_ms: props.capturePeakMs, peak_memory_mb: props.peakMemoryMb, + peak_heap_used_mb: props.peakHeapUsedMb, memory_free_mb: props.memoryFreeMb, tmp_peak_bytes: props.tmpPeakBytes, stage_compile_ms: props.stageCompileMs, @@ -460,7 +463,18 @@ export function trackRenderError( errorMessage?: string; elapsedMs?: number; peakMemoryMb?: number; + peakHeapUsedMb?: number; memoryFreeMb?: number; + /** + * Worker sizing on the failure path. Previously `render_complete`-only, + * which left PRINFRA-341's central question unanswerable: 0 of 317k + * `render_error` events carried these, so advisory-true renders could + * never be correlated with failures. + */ + workersBoundBy?: string; + workersHeapBased?: number; + workersHeapLimitMb?: number; + workersExceedHeapAdvisory?: boolean; // Attribute this event to a specific user (e.g. the browser user who // triggered a studio render); defaults to the install anonymousId. distinctId?: string; @@ -480,7 +494,12 @@ export function trackRenderError( error_message: props.errorMessage ? redactTelemetryMessage(props.errorMessage) : undefined, elapsed_ms: props.elapsedMs, peak_memory_mb: props.peakMemoryMb, + peak_heap_used_mb: props.peakHeapUsedMb, memory_free_mb: props.memoryFreeMb, + workers_bound_by: props.workersBoundBy, + workers_heap_based: props.workersHeapBased, + workers_heap_limit_mb: props.workersHeapLimitMb, + workers_exceed_heap_advisory: props.workersExceedHeapAdvisory, ...powerStateFields(), // gpu_renderer arrives via renderObservabilityEventProperties below: // on the failure path perfSummary is never built, so live capture diff --git a/packages/producer/src/services/render/shared.test.ts b/packages/producer/src/services/render/shared.test.ts index 04e47bc90d..fd99404506 100644 --- a/packages/producer/src/services/render/shared.test.ts +++ b/packages/producer/src/services/render/shared.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { resolveBrowserMediaEnd } from "./shared.js"; +import { recordJobFailureMetrics, resolveBrowserMediaEnd } from "./shared.js"; describe("resolveBrowserMediaEnd", () => { it("prefers a runtime duration over a stale compiler-clamped end", () => { @@ -15,3 +15,39 @@ describe("resolveBrowserMediaEnd", () => { expect(resolveBrowserMediaEnd(0, 5.04, 0)).toBe(5.04); }); }); + +describe("recordJobFailureMetrics", () => { + const sampler = { + peakRssBytes: () => 300 * 1024 * 1024, + peakHeapUsedBytes: () => 120 * 1024 * 1024, + }; + + // The whole point: perfSummary is success-only, so a thrown render must + // still leave sizing + peaks on the job for render_error to read. + it("copies sampled peaks and sizing onto the job", () => { + const job: { peakRssMb?: number; peakHeapUsedMb?: number; workerSizing?: { workers: number } } = + {}; + recordJobFailureMetrics(job, sampler, { workers: 6 }); + expect(job.peakRssMb).toBe(300); + expect(job.peakHeapUsedMb).toBe(120); + expect(job.workerSizing).toEqual({ workers: 6 }); + }); + + it("records peaks even when sizing was never computed (failure before capture)", () => { + const job: { peakRssMb?: number; peakHeapUsedMb?: number; workerSizing?: { workers: number } } = + {}; + recordJobFailureMetrics(job, sampler, undefined); + expect(job.peakRssMb).toBe(300); + expect(job.workerSizing).toBeUndefined(); + }); + + it("keeps an already-recorded sizing rather than blanking it", () => { + const job: { peakRssMb?: number; peakHeapUsedMb?: number; workerSizing?: { workers: number } } = + { + workerSizing: { workers: 4 }, + }; + recordJobFailureMetrics(job, null, undefined); + expect(job.workerSizing).toEqual({ workers: 4 }); + expect(job.peakRssMb).toBeUndefined(); + }); +}); diff --git a/packages/producer/src/services/render/shared.ts b/packages/producer/src/services/render/shared.ts index 15eaf8226f..abeac251b2 100644 --- a/packages/producer/src/services/render/shared.ts +++ b/packages/producer/src/services/render/shared.ts @@ -364,6 +364,38 @@ export interface MemorySampler { stop: () => void; } +/** + * Copy sizing + sampled peak memory onto the job so they survive a failed + * render. + * + * `job.perfSummary` is assembled only after a render succeeds, so a throw + * mid-capture previously reached `render_error` with no sizing or memory + * context — 0 of 317k fleet failures carried `workers_heap_*`, which is why + * PRINFRA-341's "do advisory-true renders OOM?" question was unanswerable. + * Called from the unconditional memory-sampler disposer, so it runs on both + * the success and failure paths. + * + * ponytail: structurally typed rather than importing RenderJob/WorkerSizing — + * renderOrchestrator already imports this module, so a nominal import here + * would be a cycle. + * + * Does NOT make fatal V8 OOMs observable: `FATAL ERROR: Reached heap limit` + * aborts the process before any event is sent. This covers failures that + * reach an error handler at all. + */ +export function recordJobFailureMetrics( + job: { peakRssMb?: number; peakHeapUsedMb?: number; workerSizing?: TSizing }, + sampler: Pick | null, + sizing: TSizing | undefined, +): void { + if (sampler) { + job.peakRssMb = Math.round(sampler.peakRssBytes() / (1024 * 1024)); + job.peakHeapUsedMb = Math.round(sampler.peakHeapUsedBytes() / (1024 * 1024)); + } + // Leave a previously-recorded sizing in place rather than blanking it. + if (sizing !== undefined) job.workerSizing = sizing; +} + export function createMemorySampler(intervalMs: number = 250): MemorySampler { let peakRss = 0; let peakHeap = 0; diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index ad4abc2d35..47c652e206 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -110,7 +110,12 @@ import { outputSupportsPageSideShaderCompositing, type RenderOutputFormat, } from "./render/renderFormat.js"; -import { createMemorySampler, type MemorySampler, updateJobStatus } from "./render/shared.js"; +import { + createMemorySampler, + type MemorySampler, + recordJobFailureMetrics, + updateJobStatus, +} from "./render/shared.js"; import { buildRenderErrorDetails } from "./render/cleanup.js"; import { publishRenderFailure } from "./render/renderEventPublisher.js"; import { EncoderInterruptedError } from "./render/encoderInterruption.js"; @@ -625,6 +630,22 @@ export interface RenderJob { totalFrames?: number; framesRendered?: number; perfSummary?: RenderPerfSummary; + /** + * Worker sizing + peak memory, recorded as soon as they are known rather + * than assembled at the end like {@link perfSummary}. + * + * `perfSummary` is only assigned on the success path, so a render that dies + * mid-capture carried no sizing or memory context into `render_error` — the + * gap that made PRINFRA-341's "do advisory-true renders OOM?" question + * unanswerable across 317k fleet failures. These are written by the + * unconditional memory-sampler disposer, so they survive a throw. + * + * NOT a fix for fatal V8 OOMs: `FATAL ERROR: Reached heap limit` aborts the + * process, so no telemetry is sent at all. This covers recoverable failures. + */ + workerSizing?: WorkerSizing; + peakRssMb?: number; + peakHeapUsedMb?: number; failedStage?: string; errorDetails?: { message: string; @@ -2250,6 +2271,9 @@ async function executeRenderPipeline(input: { await closeCaptureSession(session); }); execution.defer("stop memory sampler", () => { + // Runs on every exit including a throw, unlike the perfSummary assembly + // at the end of the happy path — see recordJobFailureMetrics. + recordJobFailureMetrics(job, memSampler, workerSizing); memSampler?.stop(); memSampler = null; });