Skip to content

fix(engine): fail partial audio track preparation#2488

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/fail-on-dropped-audio-track
Jul 15, 2026
Merged

fix(engine): fail partial audio track preparation#2488
miguel-heygen merged 1 commit into
mainfrom
fix/fail-on-dropped-audio-track

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

Stop audio processing when any authored track fails preparation instead of mixing and reporting success with the remaining tracks.

Why

A short SFX could be omitted while the overall mix still returned success. The producer therefore had no failure to surface, and the final video could ship with a digitally silent cue despite a zero exit code.

How

After concurrent track preparation completes, the mixer now returns a failed result when any source/download/extract/prepare error was collected. It does not run a partial mix. The existing producer error path then surfaces the affected track id and reason.

Test plan

  • Unit tests added/updated

  • Manual testing performed

  • Documentation updated (not applicable)

  • Red-first regression reproduced one failed short cue alongside a valid track: current main returned success and invoked a partial mix.

  • bun run --cwd packages/engine test -- audioMixer.test.ts — 14/14 passed.

  • bun run --cwd packages/engine typecheck — passed.

  • Producer unit-vitest lane: audioStage.test.ts passed 4/4; 137 tests passed overall before seven unrelated suites hit an unbuilt @hyperframes/studio-server/manual-edits-render-script workspace artifact in the isolated worktree.

  • Pre-commit lint, format, typecheck, tracked-artifact, and fallow gates passed.

@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.

R1 (Via) — position: RIGHT (ship-ready, two nits + orthogonal CI note)

Head SHA e5d6ea1 · +74/-0 across audioMixer.ts (+19) and audioMixer.test.ts (+55) · no prior reviews at this head.

Thesis check. The fix is what the PR body claims: after the concurrent per-track Promise.all, if any element pushed into errors[], return { success: false } with the concatenated reasons instead of proceeding to mixAudioTracks and rewriting .error to "Warnings: …" while keeping success truthy. Verified end-to-end.

Extrapolation-blocker (fail-fast on ALL partial-state configs, not just the cited case). ✅ The gate is a uniform if (errors.length > 0) on the accumulator. Six writers push into it:

  1. Cancelled: <id> (line 528, signal.aborted at entry)
  2. Download failed: <id> — <msg> (543, remote fetch throw)
  3. Source not found: <id> (551, missing local file)
  4. Extract failed: <id> (577, video→audio extraction failure)
  5. Prepare failed: <id> (592, trim/prep failure — the cited case)
  6. Error: <id> — <msg> (623, catch-all)

The new invariant fires on any of the six. The single regression test only exercises path (5); see nit 1.

Funnel-completeness (started === completed + failed on every terminating path). ✅ Each element in the input either pushes to tracks[] OR pushes to errors[] and returns from its async body; the branches are mutually exclusive and every element hits exactly one. elements.length === tracks.length + errors.length after the Promise.all on both the success and new fail-fast paths.

Full dispatch chain (verified). processCompositionAudio has exactly one non-test consumer: runAudioStage at packages/producer/src/services/render/stages/audioStage.ts:618. On success: false, .error is assigned to audioError (line 630), which flows into applyRenderWarningPolicy in renderOrchestrator.ts:1972-1985 with code: "audio_processing_failed". That helper unconditionally throws RenderQualityError when the audio-processing-failure flag is set (renderOrchestrator.ts:571-574) — independent of strictness. Distributed path (plan.ts:261-275) mirrors it. So the render terminates and the affected track id + reason surface in the interpolated "Audio mix failed; output would be video-only: <err>" message. No consumer regexes on the prefix "Warnings:" or "Audio processing failed:" — the string is opaque payload.

Reachability. ✅ Always-on. No feature flag, no config gate.

Envelope. Clean. Sole commit e5d6ea1 authored + committed by miguel-heygen only. No Co-Authored-By: Claude / bot trailer.


Nit 1 (regression-coverage parameterization). The new test at audioMixer.test.ts:146 covers only Prepare failed. A describe.each / it.each across the six writer sites (Cancel / Download / Source-missing / Extract / Prepare / catch-all) is what Miguel's parameterize-across-every-partial-state directive is after. All six converge on the same gate, so this is stronger evidence rather than a new correctness concern — but the coverage matrix is the intended shape. Non-blocking.

Nit 2 (dead code after fail-fast). With the early-return at line 632-644, the ternary at line 658 — error: errors.length > 0 ? \Warnings: ${errors.join(", ")}` : mixResult.error— has become unreachable in itserrors.length > 0branch. The"Warnings:"string is now dead. Safe to simplify toerror: mixResult.error` in a follow-up. Non-blocking.

Aside (redundant, not wrong). The inline rmSync(workDir, { recursive: true, force: true }) at line 634 duplicates the producer's own finally cleanup at renderOrchestrator.ts:3105 (which rms the outer workDir, of which audio-work is a subdir). Harmless — just cleans a bit earlier while the RenderQualityError unwinds three frames up.


CI note (orthogonal — not a blocker): Typecheck lane is red at head but the failure is a bun install --frozen-lockfile integrity check on fontkit@2.0.4 — the install exits 1 before typecheck runs. Registry / lockfile flake, unrelated to this diff. Job 87407525015, run 29431522555. A retry should clear it.

— Via

@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.

Reviewed at e5d6ea16. Typecheck failure is environmental — bun install hit a fontkit@2.0.4 tarball IntegrityCheckFailed on the npm mirror, not a real type error. Not a review-holder.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Failure path is now fully wired end-to-end. The pre-existing audioStage.ts was already propagating audioResult.error into AudioStageResult.audioError, and both callers already consume it:

  • renderOrchestrator.ts:1972applyRenderWarningPolicy(job, [...])
  • distributed/plan.ts:935applyDistributedAudioWarningPolicy(job, audioResult.audioError, log)
    Before this PR, processCompositionAudio never returned success: false on a partial track failure, so that propagation was effectively dead code sitting on top of a silent partial mix. This PR turns it on. Observability contract restored end-to-end.

🟢 Defensive-bug-pin verified. The new test mocks runFfmpeg to fail exclusively on the missing-cue prep call. Against pre-PR code, the mixer would fall through to mixAudioTracks (which would succeed with the one valid track) and return success: true — the test's expect(result.success).toBe(false) and expect(result.error).toMatch(/Prepare failed: missing-cue/) would both fail. Real red-first pin.

🟢 tracksProcessed === 1 matches the assertion. tracks.push(...) sits inside the try's success branch — on prep failure the code errors.push(...); return; skips the push. So tracks at the new failure-return contains only the successful pushes (1 in the test's two-track fixture). Not a hard-coded number; scales with real inputs.

🟢 workDir cleanup wrapped in try/catch so a cleanup error can't shadow the real audio-failure result. Same shape as the success-path cleanup immediately below.

🟢 Inline comment ("Never turn a per-track preparation failure into a successful partial mix.") captures both the invariant and the reason for it, which is what future contributors will need if they're ever tempted to soften the failure into a partial mix again.

Verdict framing

Small, focused correctness fix that wires an already-half-implemented observability path all the way through. LGTM from my side.

Review by Rames D Jusso

@miguel-heygen miguel-heygen merged commit 968c903 into main Jul 15, 2026
65 of 66 checks passed
@miguel-heygen miguel-heygen deleted the fix/fail-on-dropped-audio-track branch July 15, 2026 19:26
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