Skip to content

fix(cli): carry worker sizing and sampled peak memory on render_error - #3874

Open
vanceingalls wants to merge 1 commit into
mainfrom
fix/render-error-sizing-telemetry
Open

fix(cli): carry worker sizing and sampled peak memory on render_error#3874
vanceingalls wants to merge 1 commit into
mainfrom
fix/render-error-sizing-telemetry

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Why

PRINFRA-341 asks whether renders that exceed the heap advisory go on to OOM. That question is unanswerable against the current fleet, for two independent instrumentation reasons found while trying to answer it:

  1. Sizing is success-only. workers_bound_by, workers_heap_based, workers_heap_limit_mb and workers_exceed_heap_advisory are emitted on render_complete but 0 of 317,253 render_error events over the last seven weeks carry any of them. Advisory-true renders can never be correlated with failures.

    The cause is lifecycle, not intent: those props are read off job.perfSummary, which is assembled after a render succeeds. A render that dies mid-capture never reaches that assignment.

  2. peak_memory_mb is sampled after the buffers drain. It is process.memoryUsage.rss() read at trackRenderComplete time. Worker-count-driven parent buffering is a mid-render phenomenon, so the field is blind to exactly the effect the heap model is about. A fleet read of it shows parent RSS flat from 1 to 32 workers, which looks like strong evidence against the per-worker constant and is in fact an artifact of when the sample is taken.

What this changes

  • Record sizing and sampled peak memory onto the job from the memory-sampler disposer, which the execution context runs on every exit including a throw, and read them on the failure path.
  • Prefer the existing sampler's true running peak over the teardown snapshot on both success and failure paths. createMemorySampler already tracked peak RSS and heap-used every 250 ms and fed perfSummary; the telemetry field simply was not reading it.
  • Add peak_heap_used_mb alongside peak_memory_mb — RSS includes native ffmpeg/Chrome allocations, heapUsed isolates the JS-object growth the heap model is actually about.

Limitation

A fatal V8 FATAL ERROR: Reached heap limit aborts the process before any event is sent, so heap OOMs remain invisible to telemetry. This narrows the gap to failures that reach an error handler; it does not close it. Making fatal OOMs observable needs a different mechanism (e.g. persisting sizing pre-capture and reading it back on next launch, next to the existing recentRenders in ~/.hyperframes/config.json) — out of scope here.

Validation

  • recordJobFailureMetrics and failureSizingTelemetry extracted so the copy is unit-testable rather than buried in a closure, and so handleRenderError's branch count does not grow.
  • Three new tests: the props survive the summary→event hop on the error path; peaks are recorded when sizing was never computed (failure before capture); an already-recorded sizing is not blanked.
  • All three verified by mutation — dropping the props, zeroing the peak, and making the sizing write unconditional each fail a test.
  • Full events.test.ts suite (51) and shared.test.ts (6) pass; engine/producer typechecks, scoped oxlint/oxfmt, tracked-artifact, large-file and fallow new-issue gates all pass.

Independent of #3803 and worth landing regardless of what happens to it — #3803's own merge decision is currently blocked on exactly the data this produces.

🤖 Generated with Claude Code

PRINFRA-341 asks whether renders that exceed the heap advisory go on to OOM.
That question is unanswerable against the current fleet: `workers_bound_by`,
`workers_heap_based`, `workers_heap_limit_mb` and `workers_exceed_heap_advisory`
are emitted on `render_complete` only, so 0 of 317,253 `render_error` events
over the last seven weeks carry any of them.

The cause is lifecycle, not intent: those props are read off `job.perfSummary`,
which is assembled after a render succeeds. A render that dies mid-capture
never reaches that assignment.

Record sizing and sampled peak memory onto the job from the memory-sampler
disposer instead, which the execution context runs on every exit including a
throw, and read them on the failure path. Also prefer the sampler's running
peak over the teardown RSS snapshot on both paths: `peak_memory_mb` previously
reported whatever RSS happened to be at teardown, missing the mid-render spike
the field exists to catch. Adds `peak_heap_used_mb` alongside it.

Limitation, stated because it bounds what this buys: a fatal V8
`FATAL ERROR: Reached heap limit` aborts the process before any event is sent,
so heap OOMs remain invisible to telemetry. This narrows the gap to failures
that reach an error handler; it does not close it.

Extracted `recordJobFailureMetrics` and `failureSizingTelemetry` so the copy is
unit-testable rather than buried in a closure, and so handleRenderError's
branch count does not grow. Tests cover the summary-to-event hop on the error
path, peaks recorded when sizing was never computed, and an existing sizing not
being blanked; all verified by mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:large_yellow_circle: db5f149c5 COMMENTED

Primary mechanism reads clean. recordJobFailureMetrics runs from the execution.defer("stop memory sampler", …) disposer in executeRenderPipeline, so it fires on every exit including a throw; it captures memSampler and workerSizing from the enclosing scope, both guarded (sampler nullable, sizing !== undefined); the sampler's peakRssBytes() / peakHeapUsedBytes() resample synchronously on read, so peaks are honest at teardown even when nothing sampled since the last 250ms tick. On the read side, failureSizingTelemetry(job, options.workers) maps sizing.boundBy → workersBoundBy → workers_bound_by (and the three siblings) with the parameter names lining up cleanly through trackRenderError into the emitted snake_case shape — events.test.ts pins the full shape. The workers: requestedWorkers ?? sizing.workers fallback is a nice tightening: pre-PR the failure path emitted workers = options.workers (usually undefined since most users don't pass --workers), now it falls back to the resolved count, bringing failure semantics closer to render_complete's.

One coverage concern to raise, and two nits.

Concern — the PRINFRA-341 gap closes on CLI but remains open on studio. trackRenderError has one other production emitter: emitStudioRenderError in packages/cli/src/server/studioRenderTelemetry.ts (studio → studio-server → executeRenderJob → same executeRenderPipeline, so the disposer this PR adds runs and populates renderJob.workerSizing / peakRssMb / peakHeapUsedMb on the studio path too). But emitStudioRenderError still calls memSnapshot() (RSS at emit-time in the studio-server process — the exact teardown-snapshot pattern the PR is moving away from) and passes zero worker-sizing fields into trackRenderError. The in-file comment justifying the omission is specifically about workers (studio has no user-supplied count, which is a real gap), but that rationale doesn't cover workersBoundBy / workersHeapBased / workersHeapLimitMb / workersExceedHeapAdvisory / peakMemoryMb / peakHeapUsedMb, which are read off job — populated by the disposer this PR adds. Every field on trackRenderError is optional in events.ts, so the compiler doesn't flag it. Net: the PR-body claim "0 of 317,253 render_error events carry sizing" closes on CLI renders but the studio slice remains at zero on the same event. A failureSizingTelemetry(renderJob, undefined) at emitStudioRenderError — deliberately passing undefined for requestedWorkers to preserve the studio-workers omission — would close it in a few lines. Non-blocker; happy to accept "explicit follow-up" as the answer since studio is a smaller volume slice, but worth stating one way or the other so the PRINFRA-341 rollout expectations are honest.

Nit — double getMemorySnapshot() on the success emit. In trackRenderMetrics (render.ts), the success emit spreads ...getMemorySnapshot() and then in the same object writes peakMemoryMb: perf?.peakRssMb ?? getMemorySnapshot().peakMemoryMb — when perf?.peakRssMb is undefined (e.g. Docker), process.memoryUsage.rss() fires twice microseconds apart. failureSizingTelemetry already hoists const snapshot = getMemorySnapshot() and reuses it; same shape here would be free.

Nit — two it()s asserting the same emit shape. events.test.ts adds one it("carries worker-sizing provenance and sampled peaks on render_error too", …) mid-file inside the existing render telemetry events describe, and another it("carries sizing and sampled peaks on the failure path", …) inside a new end-of-file render_error worker-sizing provenance describe. They assert the same snake_case shape via slightly different matchers (expect.objectContaining vs mock.calls.at(-1)?.[1]). The end-of-file block also carries the "omits them when the render failed before sizing was computed" negative-case sibling that the mid-file it doesn't, so it's the stronger of the two — suggest keeping only that one.

What I didn't verify. Whether the pre-PR fleet-consumer dashboards / PostHog queries that ask "where workers is null on render_error" (a plausible proxy for "did the user pass --workers?") would be broken by the new requestedWorkers ?? sizing.workers fallback — those queries would now see the resolved count on failures instead of null. Likely the intended outcome given the PRINFRA-341 framing, but a heads-up to Data if any such queries exist.

State at HEAD db5f149c5: isDraft=false, mergeStateStatus=BLOCKED, mergeable=MERGEABLE, reviewDecision=REVIEW_REQUIRED. HF-OSS require_last_push_approval=true, so any push voids stamps.

Review by Rames D Jusso

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants