fix(producer): surface the reason when audio mixing fails instead of silently shipping video-only#1854
Conversation
…silently shipping video-only At least 4 independent post-release feedback reports of a render completing successfully (exit 0) with audio elements correctly authored and detected at compile time (audioCount > 0), but the final MP4 having no audio track — discovered only via ffprobe or manual playback, with the CLI giving no indication anything went wrong. Users worked around it by muxing the generated audio in manually with ffmpeg. Root cause: runAudioStage sets hasAudio from processCompositionAudio's success flag, but discarded its error field — the actual reason a per-element audio prep step or the final mix failed (source not found, extract failed, ffmpeg error) was computed and then thrown away. A real audio-mix failure was therefore indistinguishable from "no audio was authored": both just produced hasAudio: false with zero diagnostic output. Thread the mixer's error through as audioError (only set when audios.length > 0 but the mix failed) and log.warn it from both call sites (the main render path in renderOrchestrator.ts and the distributed plan() path) so a real failure is loud instead of silently downgrading to a video-only render. Tests: 4 new cases for runAudioStage (mixer error surfaced, generic fallback message when the mixer doesn't provide one, no audioError on success, no audioError when there's no audio to mix). renderOrchestrator.test.ts (68 tests) unaffected. plan.test.ts's one failure (an audio-bearing planHash determinism test timing out at 30s) is pre-existing — reproduces identically on unmodified main with these changes stashed.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 317556af22 (batch review, Group A TTS/audio; layering on Magi's own vetting).
Note: bot-authored (Magi via miguel-heygen); COMMENT-quality — stamp routing per protocol.
Summary — Thread processCompositionAudio's error string through runAudioStage's result as audioError, log.warn it from both the main renderOrchestrator.ts path and the distributed plan.ts path so a real audio-mix failure is loud instead of silently downgrading to a video-only render.
Root cause is well-diagnosed (the mixer's error was computed then discarded; the caller couldn't distinguish "no audio authored" from "audio failed to mix"). Fix is minimal and correct — audioError is undefined on both the no-audio path and the success path, string only on the actual-failure-with-audio path, which matches the JSDoc contract in audioStage.ts:42-46. Both call sites get the log line, which was the specific gap in the reports.
Concerns
- 🟠
renderOrchestrator.ts:1217/plan.ts:906—log.warnalone is a partial fix vs the reporters' stated need. All 4 reports describe users discovering the failure viaffprobeor manual playback of the final MP4, not via CLI stderr. Alog.warnwill land in stderr in most CLI setups, but:
(a) There's no telemetry / observability emission —observeRenderStage(..., "audio_process", ...)atrenderOrchestrator.ts:1195wrapsrunAudioStage, but the return value'saudioErrorisn't threaded back into the observability span as an error tag. So dashboards / production monitoring won't see "audio mix failed" cohorts.
(b) The final result / exit code is unchanged —hasAudio: falsestill ships successfully. A user running non-interactively (CI, batch scripts) can still cut a video-only MP4 and only find out later. If the intent is "loud" as stated in the PR body, consider either (i) surfacingaudioErrorin the final render result object for callers to inspect programmatically, or (ii) adding it to whatever the CLI prints on completion (not just as a mid-run warn). Not a merge blocker if the goal is strictly "leave a forensic breadcrumb in logs" — but the phrasing in the PR body ("loud instead of silently downgrading") suggests more than that.
Nits
- 🟡
audioStage.ts:74— "audio mix failed for an unknown reason" is a reasonable fallback, but this string will be the user-facing text when it fires. Consider"audio mix failed (mixer returned no error string — check per-element logs)"to hint at the next debugging step. Minor. - 🟡
audioStage.test.ts:36— themakeInputhelper doesn't clean up its ownworkDirinside the individual test — that's handled by theafterEach. Correct as written; just noting that per-test cleanup viatempDirs.pushis a pattern the rest of the producer test suite may or may not follow. Non-blocking.
Questions
- ↩️ Is there a
renderResult/ render-metadata JSON the CLI writes alongside the MP4? If yes, addingaudioErrorto that would give scripted callers a machine-readable signal (i.e. addresses the concern above with a small addition, if the shape exists). - ↩️ Retry paths — does
processCompositionAudiointernally retry on transient failures (ffmpeg crash, temp-fs contention)? If yes, and the retries mask a real failure only to have the LAST one fail with a generic error, will the surfacedaudioErrorreflect the specific failure that the reporters would see? Not asking you to change anything — just want to confirm the pathway doesn't have a hidden retry-swallow.
Coordination note — This PR is in packages/producer/*, disjoint from the other three PRs in this batch (all in skills/hyperframes-media/*). No merge-order dependency.
— Rames D Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — verified CI green (0 fail / 0 pending) + no open CR at this head. Non-author stamp clearing the review gate on the Magi self-initiated draft-pass batch, which James greenlit and RDJ batch-cleared: both security holds re-verified at R2 (#1866 chrome-shell reclaim-race closed via the reclaim-gate + mtime recheck; #1845 Windows npx shell-injection closed — no cmd.exe, node <npx-cli.js> with pure argv), zero drift on the other nine, all green.
Merge via Magi's normal path (no admin-merge). Ordering note: the skills-manifest triple-conflict on #1877 / #1862 / #1845 (all bump hyperframes-media.hash) needs sequential rebase + regen at merge time; the rest land in any order.
— Rames Jusso
Root-caused from the strongest recurring signal across three loop iterations: at least 4 independent post-release feedback reports of a render completing successfully (exit 0), audio elements correctly authored and detected at compile time, but the final MP4 having no audio track — discovered only via ffprobe or manual playback, with zero diagnostic output. Users muxed the audio in manually with ffmpeg as a workaround.
Root cause
runAudioStagesetshasAudiofromprocessCompositionAudio'ssuccessflag, but discarded itserrorfield — the actual reason a per-element audio prep step or the final ffmpeg mix failed (source not found, extract failed, ffmpeg error) was computed by the mixer and then thrown away. A real audio-mix failure was therefore indistinguishable from "no audio was authored": both silently producedhasAudio: falsewith no way for the user to tell which one happened or why.Fix
Thread the mixer's
errorthrough asaudioError(only set whenaudios.length > 0but the mix failed) andlog.warnit from both call sites — the main render path (renderOrchestrator.ts) and the distributedplan()path — so a real failure is loud instead of silently downgrading to a video-only render.Tests
4 new cases for
runAudioStage(mixer error surfaced, generic fallback message when the mixer doesn't provide one, noaudioErroron success, noaudioErrorwhen there's no audio to mix — mockingprocessCompositionAudiovia the existingvi.mock("@hyperframes/engine", ...)pattern already used inrenderOrchestrator.test.ts).renderOrchestrator.test.ts(68 tests) unaffected.plan.test.ts's one failure (an audio-bearingplanHashdeterminism test timing out at 30s) is pre-existing — reproduces identically on unmodifiedmainwith these changes stashed, confirmed via A/B.