Skip to content

fix(audio): preserve causes and use portable padding#2769

Open
jrusso1020 wants to merge 2 commits into
mainfrom
fix/audio-failure-causes-portable-pad
Open

fix(audio): preserve causes and use portable padding#2769
jrusso1020 wants to merge 2 commits into
mainfrom
fix/audio-failure-causes-portable-pad

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

What

  • replace apad=whole_dur=... with the portable, finite apad,atrim=0:<compositionDuration> filter chain
  • preserve bounded typed audio failure causes through engine, in-process producer, and distributed planning
  • emit structured reason/stage/owner/retryability telemetry without logging raw stderr
  • keep warning snapshots isolated by deep-cloning mutable warning fields

Why

Production audio_processing_failed events currently discard the underlying FFmpeg/ffprobe cause. A reproduced Windows failure on the bundled FFmpeg rejects apad=whole_dur with Option 'whole_dur' not found, even though the same composition can render video and be muxed locally.

The missing causes also make deterministic input failures indistinguishable from transient infrastructure failures. This change fixes the known portability bug and makes future failures actionable without weakening the existing fail-closed audio correctness policy.

How

  • pad each delayed audio input indefinitely and trim it to the composition duration before amix, preserving authored clip/delay timing while avoiding the unsupported whole_dur option
  • classify source, download, probe, extract, prepare, mix, timeout, binary, cancellation, and internal failures
  • treat invalid authored media as user-owned only in extract/prepare; producer-generated mix/silence failures stay system-owned
  • redact URLs and absolute paths (including paths with spaces), cap every detail and aggregate error at 2,000 characters, and preserve unknown legacy retryability as unknown
  • aggregate retryability is true only when every typed cause is retryable

This is additive/backward-compatible telemetry and does not change retry or fallback policy. It is safe to canary independently; any later policy change can key off the new bounded taxonomy.

Test plan

  • Unit tests added/updated
  • Focused engine/producer/distributed suites: 76 tests passed
  • Engine and producer typechecks
  • Lint, formatting, tracked-artifact, and fallow gates
  • Manual production canary after merge
  • Documentation updated (not applicable)

Independent review: approved after timing, privacy, propagation, retry, and snapshot-isolation findings were addressed.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: APPROVE

Portability fix + typed failure taxonomy — well-executed, well-tested, additive. The apad,atrim=0:X shape is semantically equivalent to apad=whole_dur=X for the pipeline's needs (delay + trim to composition duration), and the taxonomy propagation preserves diagnostic content without introducing new metric tags or unbounded telemetry. Windows CI (Render on windows-latest 8m18s, Tests on windows-latest 11m5s — both pass) is direct end-to-end validation of the very platform this fix targets.

Empirical premises verified against main @ base 3b3d4f5:

  • RunFfmpegResult.terminationReasonpackages/engine/src/utils/runFfmpeg.ts:28, exercised across streaming/chunk encoder / videoFrameExtractor already. ✓
  • formatFfmpegErrorpackages/engine/src/utils/runFfmpeg.ts:71, re-exported from engine index. ✓
  • formatFilterNumberpackages/engine/src/services/audioMixer.ts:27, existing helper. ✓
  • applyDistributedAudioWarningPolicy runtime callers: only packages/producer/src/services/distributed/plan.ts:946, which the PR updates. No positional-log breakage risk. ✓
  • Legacy inline warning-clone sites that the PR replaces (frameCapture.ts:3776, renderEventPublisher.ts:8, captureHdrStage.ts:131, renderOrchestrator.ts:618) all only cloned sources; the new shared cloneCaptureWarning also deep-clones the new failureReasons/failureStages fields, closing the aliasing hole that would otherwise ship with the taxonomy. ✓
  • No other user of apad=whole_dur in the live code path (see P2 below for the dead-code sibling). ✓

End-to-end validation

  • User-facing exercise path: plan() (distributed) and executeRenderPipeline (in-process) → runAudioStageprocessCompositionAudio → filter emission at packages/engine/src/services/audioMixer.ts (post-PR L522). Downstream: audioResult.audioFailuresapplyDistributedAudioWarningPolicy / orchestrator's inline aggregation → applyRenderWarningPolicy → structured log.warn with warningReasons/warningStages/warningOwners/warningRetryable.
  • Filter-string test-lock: packages/engine/src/services/audioMixer.test.ts L79-83 asserts apad,atrim=0:2 present and whole_dur absent — the exact shape swap. Multi-track filter test at L603-609 pins atrim= count to trackCount * 2 and apad, count to trackCount, catching any silent revert to the single-atrim whole_dur shape. Golden churn on the multi-track test is bounded and semantic — checking that the new shape emits both trims and one apad per track, not merely a text-match.
  • Taxonomy propagation coverage: plan.test.ts — typed-details, mixed-cause retryability rule (every retryable only), legacy-untyped preservation (does not invent ownership). renderEventPublisher.test.ts — deep-clone-across-sink-boundary (mutating job.warnings[0].details.failureReasons after publish() does not affect the delivered snapshot; sink mutation does not bleed back), plus explicit taxonomy log-shape assertions.
  • Windows-native run: Render on windows-latest (8m18s pass) and Tests on windows-latest (11m5s pass) both green on head 4b024ed. That is the direct field of failure this PR targets.
  • Producer-lane check: the one non-green matrix job (Producer: ${{ matrix.lane }} tests, job 89645115967) is cancelled, not failed. Not blocking; not a real signal.

P2 (address before merge or acknowledge)

packages/producer/src/services/audioExtractor.ts:210 — same portability bug, dead code.
The filter builder there emits the identical broken shape:

[${i}:a]atrim=0:${trimDuration},volume=${track.volume},adelay=${delayMs}|${delayMs},apad=whole_dur=${totalDuration}[a${i}]

Zero runtime callers today (rg processAudio\|from.*audioExtractor outside the file itself returns nothing) — the module is orphaned. So it does not block this PR's correctness on Windows. But because the whole thesis is "the portable filter shape is apad,atrim, apad=whole_dur is not supported by the bundled Windows FFmpeg", leaving the identical broken shape in a sibling is a re-materialization landmine the moment anyone revives or copies from that module. Suggest either:

  • align the shape here (mirror the change, no other refactor), or
  • delete audioExtractor.ts in a follow-up (there is a scoped ticket-worthy cleanup here regardless of this PR).

Not blocking — flag with a one-line commit or a TODO(#…).

P3 nits

  • packages/producer/src/services/render/audioPadTrim.ts:249 — comment references apad=whole_dur as the shape used in the codebase (paraphrased: "vs a working manual apad=whole_dur command"). After merge, the module the comment contrasts against no longer emits whole_dur. Consider updating the comment or noting the migration.
  • packages/engine/src/services/audioMixer.ts probeFailure, L292 — the timedOut regex is /(?:timed?\s*out|timeout|deadline|inactivity|aborted)/i. aborted in retryable: !invalidMedia && (unavailable || timedOut) yields retryable: true, but user-initiated aborts should be retryable: false. Low-frequency because extractAudioMetadata is called guarded by a signal check, but the semantics are wrong. Consider splitting aborted off — either into its own branch (with owner: "user", retryable: false, reason: "cancelled") or dropping it from the timedOut set.
  • Duplicate ownership/retryability aggregation — the same 8-line failureOwner/retryable derivation appears in both packages/producer/src/services/distributed/plan.ts L273-279 and packages/producer/src/services/renderOrchestrator.ts L2196-2203. Not a bug (they must stay identical for cross-mode parity), but it's ripe for extraction to a helper (e.g. deriveAudioFailureAggregate(audioFailures)). Skip if you'd rather keep the PR tight.
  • boundedDetail regex fallback — the POSIX path regex at audioMixer.ts L307-311 lazy-matches \/.+?(?=:\s[A-Z]|\s(?:ENOENT|EACCES|EPERM)\b|\r?$). On an error string like /var/log/render.log then something else happened (path with no terminator marker), the lazy match will extend to end-of-line and swallow the trailing prose. That is over-redaction, not a leak, so it's safe; just noting so you're not surprised by aggressive redaction in unusual FFmpeg outputs.

Nice-to-have (non-nit)

  • The cloneCaptureWarning helper is now the canonical clone. Consider adding a tiny unit test at packages/engine/src/services/captureWarning.test.ts that mutates each of sources, failureReasons, failureStages on the clone and asserts the source array is untouched. Currently the invariant is only exercised transitively via the renderEventPublisher sink-boundary test.

— Via

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Independent exact-head review on 4b024ed0e1a50e6871ccd5e40d255123c200e978: no additional blocking findings beyond Via’s documented non-blocking follow-ups.

Fresh validation:

  • 76/76 focused tests passed in their native lanes (33 Vitest + 43 Bun).
  • Full audio-mux-parity render passed end to end: compile, 300-frame Chrome capture, FFmpeg audio processing/mux, 100/100 visual checkpoints, stream drift 0.005s, audio correlation 0.99997 with zero lag.
  • All required GitHub checks are green on this head, including Windows render/tests.

Approved.

@jrusso1020
jrusso1020 force-pushed the fix/audio-failure-causes-portable-pad branch from 4b024ed to b550865 Compare July 25, 2026 19:15

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jrusso1020

Copy link
Copy Markdown
Collaborator Author

Addressed the actionable review follow-ups in b55086513:

  • aligned the orphaned audioExtractor.ts filter with portable apad,atrim
  • updated the stale audioPadTrim comment
  • classified probe aborts as user-owned, non-retryable cancelled failures and timeouts as retryable system ffmpeg_timeout failures, with direct tests
  • added a direct deep-clone test for every mutable CaptureWarning.details array

I intentionally left the duplicate distributed/in-process audio aggregation in place to keep this correctness PR narrow; the two copies remain parity-tested. The bounded-detail over-redaction note is safe and unchanged.

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.

3 participants