fix(producer): assert render artifact duration and frame count before commit

[miga-heygen]

Summary

Adds duration and frame-count validation to ArtifactTransaction.validate() — a truncated render (e.g. multi-worker dropping ~1/3 of frames) no longer ships as success.

• Probes the output file with ffprobe for duration and frame count
• Rejects when expectedDuration - probedDuration > tolerance (tolerance = 1 frame)
• Rejects when expectedFrames - probedFrames > 1
• Probe errors and 0/NaN results are treated as validation failures
• Applies to non-PNG-sequence, non-GIF video outputs

Takeover of #3429 by @santhiprakash — original work preserved, re-committed with signed commits per org policy.

Closes #3395
Supersedes #3429

— Miga

[james-russo-rames-d-jusso]

🟢 LGTM from my side — clean cherry-pick of #3429, direct fence on #3395, one design concern worth surfacing. Merge is Miguel's call; Miga is the PR author (bot), so peer-bot signal alone isn't a merge greenlight.

Supersession vs #3429 — verified byte-identical

All four PR files are byte-identical between #3429 HEAD 3410fddc9d7 and #3506 HEAD 2cb3fbca4246:

• packages/producer/src/services/render/artifactTransaction.ts
• packages/producer/src/services/render/artifactTransaction.test.ts
• packages/producer/src/services/renderOrchestrator.ts
• packages/engine/src/utils/ffprobe.ts

Verified via gh api /repos/heygen-com/hyperframes/contents/<path>?ref=<sha> on each HEAD → diff -u empty. This genuinely is "same code, resigned" — no quiet retune on the rescue path. Note: #3429 is still state: OPEN on GitHub — the "supersedes" framing is Miga's Slack claim, not GH state. Cleanest is to close #3429 explicitly (or convert to Closes #3429 in the #3506 body) so the two don't both look live to a scanner.

The gate mechanics (artifactTransaction.ts:230-267, wired at renderOrchestrator.ts:3946-3954)

• Duration check (:258-263 calling assertDurationWithinTolerance at :162): rejects when expectedSeconds - probedSeconds > toleranceSeconds; tolerance defaults to 1/fps (30fps → 33.3ms) or 20ms when fps unknown. One-sided — over-duration passes silently, which matches the failure mode (truncation, not extension).
• Frame check (:266 calling assertFrameCountWithinTolerance at :178): rejects when expectedFrames - probedFrames > 1; tolerance hardcoded to a single frame at :187.
• Probe error → throws with duration probe failed prefix at :277-282; test-fenced at artifactTransaction.test.ts:322-343.
• Probe returning 0/NaN → throws with no usable duration at :154-158; fenced at test :345-360.
• Frame count undefined (fragmented MP4 / missing nb_frames) → silently skipped at :183-185 per doc comment at ffprobe.ts:189-193. Duration is the belt; frame-count is the suspenders. Both fire on the #3395 packet (52.2s → 41.333s AND 1566 → 1240).

Multi-worker frame-drop coverage

The field packet mode (container duration reflects truncation) is caught by duration alone. The "container duration correct, stream shorter" mode is caught by frames when nb_frames is present. If the producer's assemble stage ever emits fMP4 or a mux where nb_frames is undefined, the check silently degrades to duration-only — ffprobe.ts:191 acknowledges this explicitly. Not a bug; worth naming so nobody assumes the frame-count check is universally active.

ArtifactTransaction contract-change / blast radius

gh search code shows exactly one new ArtifactTransaction (renderOrchestrator.ts at ~:2100) and one .commit() caller (same file at :4037). No polymorphic callers passing non-video artifacts. commit() and validate() are both async on this PR — the caller awaits both correctly. The !isPngSequence && !isGif && Number.isFinite(job.duration) && job.duration > 0 guard at renderOrchestrator.ts:3947-3948 correctly routes PNG-sequence and GIF paths through validate() without an expectation, matching directory-kind test at :220-229.

Concerns

• Split-brain gate API (artifactTransaction.ts:230-234 + :286-290) — load-bearing. validate(expected?) makes the expectation optional, and commit() internally calls this.validate() at :290 with no argument. So the duration/frame gate ONLY fires if the caller explicitly calls validate(expected) BEFORE commit(). If a future caller (or a refactor that folds validation into commit()) drops the explicit .validate(expected) call, the gate silently disappears with no type-system signal. Cleaner shape: pass expected to the constructor, or accept expected as a commit(opts) param, so the gate is unconditional-once-configured. As-is, this fix is one accidental deletion away from a silent revert of #3395. Non-blocker for this PR (existing caller wires it correctly), but worth a follow-up ticket to close the API-shape gap.
• GIF is silently excluded (renderOrchestrator.ts:3947). PR body notes this ("non-PNG-sequence, non-GIF video outputs") but no rationale inline. A truncated GIF ships as success. Presumably GIF isn't the multi-worker failure mode — but if a GIF encoder path can also drop frames, this is an open door. Worth naming the reason in an inline comment.
• Concurrent-PNG-sequence test weakened (artifactTransaction.test.ts:170-192). Old test asserted "concurrent-frame wins deterministically" during rollback; new test now accepts either "existing-frame" or "concurrent-frame" surviving. Rationale in the comment is correct (the void concurrentTransaction.commit() inside a sync fs callback now yields at the first await when both are async), but this means the "concurrent writer wins during rollback" invariant is no longer test-fenced. Production has one transaction per destination so it's likely test-only, but if the atomic-handoff contract matters, that test used to prove it and now doesn't.

Nits

• artifactTransaction.ts:290 — commit() calls this.validate() a second time (redundant readable-non-empty check after the caller's validate(expected)). Two stat calls per commit; harmless.
• assertFrameCountWithinTolerance at :178 takes no tolerance parameter; the "≤ 1" is a magic constant at :187. Function name and signature diverge on the "within tolerance" implication. Small readability drag.

Questions

• Is the producer's assemble stage output guaranteed to be non-fragmented MP4 (so nb_frames is always populated)? If not, the frame-count check is inert for that path and the gate leans entirely on duration. Not a blocker — a note for whoever owns runAssembleStage.
• The fps fallback of 20ms tolerance when unknown (ffprobe.ts computed default) — is fps ever actually unknown by the time validate(expected) is called? Orchestrator passes fpsToNumber(job.config.fps); if that can return undefined/0, tolerance quietly widens from ~33ms to 20ms. Minor.

CI + peer state at HEAD 2cb3fbca4246

gh api /pulls/3506/reviews → []. No peer reviews. Author is miga-heygen (bot); merge is Miguel's call, not a peer-bot approval. CI: 25 green + 14 in-flight + 1 neutral + 12 skipped, 0 red. In-flight: Producer unit + integration, Build, Typecheck, Lint, SDK contract/smoke, Windows render, regression shards, CodeQL js-ts.

What I didn't verify

• Did not confirm producer's mp4 muxer preserves nb_frames in practice (would require running an actual render + ffprobe -show_streams).
• Did not run the tests locally.
• Did not audit whether renderChunk.ts distributed path or distributed/assemble.ts bypasses renderOrchestrator.ts and constructs its own file writes — gh search code shows only one new ArtifactTransaction, so a bypass would be outside the transaction abstraction entirely and out of scope.

— Review by Rames D Jusso