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

[santhiprakash]

Closes #3395

What

ArtifactTransaction.validate() now refuses to publish a render output that is significantly shorter or has fewer frames than the capture pipeline just reported. The readable-non-empty check is unchanged; this adds a second gate on top, fed by ffprobe on the staged file and the values the orchestrator already holds.

Why

The multi-worker encode path can drop ~half the frames between capture and mux. Today the only validation is "the file exists and is non-empty", so a 148-of-300 render ships as Render complete and reaches the viewer. The information needed to catch it was already in hand at validation time — the captured duration and frame count live on the job — but no comparison was ever made.

The maintainer's field report (#3395, second comment) gives both halves of the bug surface: a media-free --workers 4 run lands at 300/300 because captureMode: "beginframe" keeps it off the parallel-disk path, while the trigger composition does go through it and ends up at 148/300 / 4.933s. Either a duration or a frame-count check against expected values catches the truncated case regardless of cause, so we add both.

How

ArtifactTransaction.validate() now accepts an optional expected: ArtifactValidationExpectation. The existing readable-non-empty check still runs first; when expected is supplied, validate() also calls a pluggable durationProbe (default: producer's extractMediaMetadata wrapper) and compares:

• Duration: expectedDurationSeconds - probed.durationSeconds > tolerance (default tolerance is one frame at the job fps, or 20 ms when fps is unknown). Both field packets from the issue (52.2s → 41.333s and 10.0s → 4.933s) are well past this threshold; the --workers 1 workaround's exact-10.0s / 300-frame run is within it.
• Frame count: when the caller supplies expectedFrames and the probe returns a frames count, expectedFrames - probed.frames > 1 rejects. This is the case the issue's second paragraph names as the "container reports correct duration but stream is shorter" failure mode — currently dormant in the default probe (which returns only duration), but the wire is in place for a richer probe without a follow-up.

Probe errors are surfaced as validation errors rather than silently passing the gate, and a probe returning 0/NaN is rejected outright — both explicitly to keep this from regressing back into "validation passes when it can't answer".

renderOrchestrator.ts now passes expected only for non-PNG-sequence, non-GIF video outputs with a finite positive captured duration, so PNG/GIF validation keeps its existing contract.

The validation still runs inside the staging directory, before the atomic rename to destinationPath — a hard failure never touches the destination and costs only a retry.

Test plan

• bun test ./src/services/render/artifactTransaction.test.ts from packages/producer/ — 16/16 pass (9 pre-existing + 7 new). The new cases cover: probe is not called when no expectation is set, probe is not called for directory artifacts, truncation rejection at the field-packet scale (52.2s expected / 41.333s probed), within-tolerance acceptance of the --workers 1 workaround (10.000s / 300 frames / 30 fps), frame-count shortfall rejection (300 expected / 148 probed), probe failure propagation, and a probe returning zero duration being treated as "cannot verify".
• bun run typecheck from packages/producer/ — clean.
• bunx oxfmt --check and bunx oxlint on the changed files — clean.
• bun run --cwd packages/studio test src/webmcp/useStudioAgentTools.test.tsx — 9/9 pass after the mock below.
• Manual: traced the orchestrator call site at packages/producer/src/services/renderOrchestrator.ts:3937 to confirm the captured duration / fps / frameCount path is correct, and that PNG-sequence and GIF outputs skip the new check.

CI note

The required Test check was failing on packages/studio/src/webmcp/useStudioAgentTools.test.tsx (registers nothing when the browser has no WebMCP). The failure is not caused by this PR's producer changes; the useStudioAgentTools hook's WebMCP polyfill fallback does a dynamic import("@mcp-b/global") which has the side effect of defining document.modelContext in the jsdom test environment. The test asserted that document should not have modelContext after mounting, so the real polyfill package caused an assertion error. The same test file now mocks @mcp-b/global to keep the hook unit isolated from that package side effect (matching the mock in packages/studio/src/webmcp/polyfill.test.ts).

Files touched:

• 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
• packages/studio/src/webmcp/useStudioAgentTools.test.tsx

[vanceingalls]

Adversarial R1 pass. Verdict: approve. The fix is well-scoped and the commit-boundary reasoning is correct.

What holds up

• Probe runs in staging before the atomic rename (artifactTransaction.ts:290 → commit() awaits validate() before the renameSync). A rejected duration/frame-count assert never touches the destination — the caller's deferred rollback at renderOrchestrator.ts:2149 (execution.defer("rollback staged artifact", ...)) is unchanged and handles the reject path.
• Silent-pass paths are closed at both ends: probe throws → validation throws (assertArtifactDuration/runDurationProbe); probe returns 0 or NaN → assertUsableProbedDuration throws. This is the exact regression #3395 is preventing.
• fps-aware tolerance (1/fps default, 0.02s fallback) tolerates container last-frame rounding without swallowing the 11s / 326-frame case. Both field packets in #3395 (52.2s → 41.333s and 10.0s → 4.933s) are orders of magnitude beyond 1/30.
• assertFrameCountWithinTolerance treats probedFrames === undefined as "no answer" and returns, which correctly handles containers where nb_frames isn't populated (fragmented MP4 etc. — the ffprobe.ts comment names this explicitly).
• Wire-up gate at renderOrchestrator.ts:3947 skips PNG-sequence, GIF, and non-positive job.duration; existing PNG/GIF contract preserved.
• Interface change surface is small: validate() and commit() went sync → async. Only one runtime caller (renderOrchestrator.ts — I searched org-wide) plus the test file, and both are updated. No stale callers.

Non-blocking findings

1. [LOW] Coverage regression in does not delete a concurrent PNG sequence published during recovery (artifactTransaction.test.ts:154-206). The old assertion readFileSync == "concurrent-frame" was loosened to ["existing-frame", "concurrent-frame"].toContain(surviving). The void concurrentTransaction.commit() inside the synchronous renameSync mock can't be awaited (that's the compromise), so the "concurrent-published wins over rollback-restore" invariant is no longer test-guarded — the test now only proves the (strictly weaker) "destination is never removed". Also the code comment says "so await it so its committed destination is in place" but the code does void, not await — the comment reads as if the wait happens. Two fixups worth considering: reword the comment to match the fire-and-forget reality, and/or add a new test that drives the concurrent commit deterministically (kick off the concurrent commit and await it explicitly, then trigger the first failure via a hook the test controls) so the original invariant stays covered.

2. [LOW] No boundary test at deficit == toleranceSeconds. The code returns on <= tolerance (inclusive). The expected=10.0, probed=10.0, fps=30 case only exercises deficit=0. Consider a case where probed = expected - 1/fps (deficit exactly one frame) to pin the inclusive boundary against future refactors.

3. [LOW] No test for expectedFrames supplied + probe returns frames: undefined. The PR body calls this "the wire is in place for a richer probe without a follow-up." assertFrameCountWithinTolerance gracefully returns on undefined, but the invariant isn't asserted — a probe stub returning { durationSeconds: 10, frames: undefined } with expectedFrames: 300 should resolve.

4. [INFO] The gate is asymmetric. Only deficit (truncation / short frames) rejects — over-duration and over-count pass silently. This matches #3395's failure mode; noting it so a later reviewer doesn't mistake it for an oversight.

5. [INFO] GIF and PNG-sequence excluded by design (renderOrchestrator.ts:3947, gate !isPngSequence && !isGif). If a similar drop ever appears on those formats, this gate won't catch it. Scoped-appropriate for the current fix; flagged only so it stays on the horizon if future evidence widens the failure class.

— Via

[terencecho]

Concur with @via-bot APPROVE at 0d3ca2f on the MP4 truncation gate — mechanism is sound, CI fully green, commit boundary correct. But there's one scope question I'd like the author to weigh before merging.

Worth deciding before merge (potentially load-bearing):

The fix closes the gate for the reported failure mode on MP4 outputs, but PNG-sequence and WebM outputs bypass the new assertions and can still exhibit the exact #3395 multi-worker frame-drop failure:

1. PNG-sequence outputs. artifactTransaction.validate() at artifactTransaction.ts:230 early-returns to the "collect directory files + assert readable" path when kind === "directory", and assertArtifactDuration only runs on kind === "file". renderOrchestrator.ts:3947 also explicitly excludes isPngSequence from the expectation object. PNG-sequence uses the SAME multi-worker capture path (renderOrchestrator.ts:3849 invokeDiskCapture → runEncodeStage with isPngSequence:true) — i.e. the exact code path #3395 fingered. A worker silently dropping N frames produces a directory of (expected−N) PNGs, and the existing "directory is empty" check only catches TOTAL failure, not partial drops (1240 of 1566 frames on disk still passes). Same bug, different output format, still ships as "Render complete."

2. WebM/Matroska outputs. assertFrameCountWithinTolerance treats probed.frames === undefined as "no answer" and returns silently. ffprobe.ts:781-789 nb_frames parse does NOT surface a frame count for Matroska/WebM containers unless -count_packets is passed; extractMediaMetadata does not pass that flag. The header comment at ffprobe.ts:190-194 acknowledges this contract, but nothing logs when the caller passed an expectedFrames the gate silently discarded — no observability signal. WebM renders that truncate frames while the muxer writes a matching container duration will pass both gates.

Recommend: either scope in PNG-sequence + WebM here (add a partial-count check for directory-kind, add -count_packets for Matroska probing OR log a "frame gate silently skipped" event so oncall can see it), OR explicitly document PNG-sequence/WebM as out-of-scope and file a follow-up. Silent-skip is the same class of failure the PR is meant to prevent.

Also non-blocking:

• commit() API footgun. artifactTransaction.ts:288: commit() calls await this.validate() internally with NO expectation. The truncation gate lives on validate(expected), and the only current caller is renderOrchestrator.ts:3946 which manually calls validate(expected) BEFORE commit(). No unit test in the new suite exercises commit() with an expectation set — every new test at artifactTransaction.test.ts:337+ calls validate() directly and doesn't proceed to commit. A future refactor that treats commit() as "validate + promote" (reasonable given the method name) silently disables the gate. Recommend: take expected on the transaction constructor or on commit() and route validate() through the stored value.

• Absolute-frame tolerance for long renders. assertFrameCountWithinTolerance at artifactTransaction.ts:186 uses shortfall <= 1 (absolute). At 30fps/30 frames that's 3.3%; at 30fps/1800 frames it's 0.056%. If an encoder legitimately rounds the last frame in a long render (last-frame duplicate suppression, timebase rounding), the gate can reject a healthy artifact — a low-rate false-positive hard to distinguish from truncation. Consider Math.max(1, Math.ceil(expectedFrames * 0.002)) or fps-scaled tolerance matching durationTolerance.

• Weakened concurrent-PNG test (also flagged by @via-bot): artifactTransaction.test.ts:169 was loosened from === "concurrent-frame" to .toContain(surviving) — the "backup restore does not clobber a mid-flight concurrent publish" invariant is now effectively unverified. Because commit() is async, await validate() inside it runs on a later microtask — usually AFTER the outer firstTransaction.commit() rejection is thrown and rollback's restoreBackupIfDestinationUnclaimed has already run.

Latent trap (currently unreachable but worth naming): assertUsableProbedDuration at artifactTransaction.ts:152-159 rejects durationSeconds === 0. ffprobe.ts:739 legitimately returns {durationSeconds: 0, frames: 1} for still images. Unreachable today because renderOrchestrator.ts:3947 gates on job.duration > 0, but a future path publishing a single-frame artifact with expectedDurationSeconds > 0 would fail loud despite frames: 1 being a valid "one frame at t=0" answer.

Coverage nit: no NTSC (29.97 fps) case in the new fps-tolerance tests; boundary at NTSC framerates unexercised.

— Review by tai (pr-review)

[santhiprakash]

Thanks @terencecho — verified all three points against the branch and they're accurate: directory-kind early-returns to the readable/non-empty path (artifactTransaction.ts:230), renderOrchestrator.ts:3947 excludes isPngSequence/GIF from expectations, and assertFrameCountWithinTolerance returns silently when nb_frames is undefined for muxers needing -count_packets (ffprobe.ts:784-789).

Decision: keeping this PR scoped to the reported #3395 path (file-kind MP4 artifacts) and filed the extension as #3484 — it covers the PNG-sequence partial-count gate (expected-files expectation threaded for directory-kind), the WebM frame-count skip observability (-count_packets vs a structured "gate silently skipped" event), plus your non-blocking notes (commit() expectation footgun, absolute shortfall <= 1 tolerance vs fps-scaled, NTSC test case).

Two reasons for not pulling it in here: the directory-kind gate needs a new expectation field plus orchestrator threading with its own test matrix, and -count_packets has the whole-file demux cost called out at ffprobe.ts:936 — both are design decisions worth their own review round rather than re-opening an approved diff.

Happy to tighten the loosened concurrent-PNG assertion (artifactTransaction.test.ts:169) here if you'd like it strict before merge — otherwise it goes in the #3484 round with the microtask-timing fix.

[james-russo-rames-d-jusso]

PR state. HEAD 0d3ca2f78473ba655efdb231bf0360469a7ebf1b, MERGEABLE, reviewDecision: APPROVED (via-bot at 0d3ca2f). tai posted COMMENT layered with PNG-sequence + WebM bypass. All CI green — Producer unit + integration, all 9 regression shards, Windows render, CLI smoke, Typecheck, Lint, CodeQL. Santhi replied confirming all three tai points match branch reality and filed follow-up #3484 covering PNG-sequence partial-count gate + WebM -count_packets observability + commit() API footgun + fps-scaled shortfall + NTSC test case + the loosened concurrent-PNG assertion. Response reads as accepted-orthogonal, not deferred-hoping-nobody-notices.

Blockers. None.

Concerns. None new — peer-covered items sit correctly in #3484.

Nits (both original, both low priority)
• renderOrchestrator.ts:2083 — captureTotalFrames is populated from probeResult.totalFrames at line 2432 (the expected frame count from the probe stage), then passed as expectedFrames: captureTotalFrames at line 3951. A future maintainer misreading the name as "actually captured" and re-pointing it at a post-capture counter silently tautologises the assert (expectedFrames === probedFrames → deficit always 0). Cheap rename to expectedTotalFrames is worth a follow-up patch. Not blocking.
• runDurationProbe at artifactTransaction.ts:270-284 doesn't accept an AbortSignal and the default probe (extractMediaMetadata) doesn't get one either. If the pipeline is cancelled during validate(), the deferred rollback (renderOrchestrator.ts:2149) can race the in-flight ffprobe: rollback rmSyncs stagingPath, ffprobe (or a follow-up ffprobe call) throws ENOENT, and the wrapper surfaces Render artifact duration probe failed: ENOENT ... in the cancel-path log. Observability-only — the render is already failing — but "probe failed" in a cancellation trail is a misleading breadcrumb. Follow-up territory; roll into #3484.

Questions. None.

Adversarial ledger (attempted, then dropped)
• Probe-before-rename ordering: commit() awaits validate() at artifactTransaction.ts:290 BEFORE the file-kind renameSync at 298 and the dir-kind backup dance at 309-311. Rejected artifact never touches destination. Held negative.
• Silent-pass on undefined-probe / zero-duration: assertUsableProbedDuration rejects 0/NaN; runDurationProbe re-wraps and re-throws. Held negative.
• Sub-worker semantics: assertion is post-mux on the FINAL artifact. A one-shot check catches any per-worker drop that reaches the muxed output on MP4 file-kind — per-worker probes would be redundant here. Held negative.
• Rollback partial-artifact leak: execution.defer("rollback staged artifact", …) at renderOrchestrator.ts:2149 fires rollback() on any throw; rollback() unconditionally rmSync(stagingPath, {recursive, force}) and restores backup only if destination is unclaimed. Held negative.
• Async signature migration missed a caller: gh search code org-wide + local grep show renderOrchestrator.ts:3946,4037 is the sole runtime caller of validate()/commit(); test file is the only other consumer, and both are migrated. Held negative.
• Error({cause}) support: commit() returns Promise<void> and runDurationProbe uses new Error(msg, { cause }) — supported on producer's target (Node ≥18 + ES2022 lib). Held negative.

Peer-coverage layering (concur vs orthogonal)

Concur with via-bot APPROVE at 0d3ca2f — independently re-verified: probe-then-rename ordering, silent-pass closure, fps-aware tolerance (1/fps default, 0.02s fallback) that admits the --workers 1 workaround (10.0s / 300f / 30fps → deficit 0 ≤ 33ms) but not the field packets (52.2s → 41.333s: deficit 10.867s ≫ 33ms), and frames === undefined gracefully skipping. Via's three low/info findings all fair.

Concur with tai COMMENT scope adds, all verified against 0d3ca2f:
• PNG-sequence bypass — artifactTransaction.ts:230 early-returns to assertReadableNonEmptyFile for kind === "directory"; assertArtifactDuration is file-kind only; renderOrchestrator.ts:3947 also preemptively excludes isPngSequence from the expectation. Multi-worker frame drop on PNG-sequence still ships as "Render complete" here — correctly scoped to #3484.
• WebM/Matroska nb_frames silent-skip — -count_packets only fires for AAC-LC audio duration refinement at ffprobe.ts:961-985; never for the video probe. Video frames is undefined for any container the demuxer doesn't populate. Assertion returns silently, no log line. Correctly scoped to #3484.
• commit() API footgun — commit() awaits validate() internally with NO expectation (artifactTransaction.ts:290), and no new test exercises commit() with an expectation set. Real design risk if a future refactor treats commit() as "validate + promote". Correctly scoped to #3484.
• Absolute-frame shortfall <= 1 at artifactTransaction.ts:187 — doesn't scale for long renders; fps-scaled or Math.max(1, ceil(N*0.002)) in #3484.
• Latent still-image trap (ffprobe.ts:739 returns durationSeconds: 0, frames: 1, unreachable today because renderOrchestrator.ts:3947 gates on job.duration > 0) — fair to name, currently unreachable.

Also concur with the tai #3480↔#3429 seam observation (drawElement 100%-fallback preserves frame count and duration → passes this assert while shipping wrong-pixel content) — correctly routed to follow-up #3482, not this PR's scope.

Orthogonal to peers: the two nits above (captureTotalFrames name; probe abort-signal). Both low-value; happy to roll into #3484.

Deploy-skew. None. Producer-only, three files, sync→async signature change on validate()/commit() has exactly one runtime caller (renderOrchestrator.ts) plus the test file — both updated in the same PR. No stale in-flight consumers.

Tests. 7 new deterministic cases at artifactTransaction.test.ts:337+ cover expectation absent, directory-kind skip, field-packet truncation, workaround acceptance (10.0s/300/30fps within one-frame tolerance), frame-count shortfall (300→148), probe-failure propagation, and zero-duration rejection. Every pre-existing sync test migrated to async correctly. One caveat both Via and tai flagged — the concurrent-PNG test's assertion was loosened from === "concurrent-frame" to .toContain(surviving) at artifactTransaction.test.ts:169, and the code comment ("so await it so its committed destination is in place") no longer matches the void concurrentTransaction.commit() reality. Santhi offered to tighten here or in #3484 — either is acceptable; "destination is never removed" is still guarded.

Stamp stance. 🟢 layered concur. Mechanism sound, commit boundary correct, orchestrator wire-up matches the file-kind scope, follow-up scope explicitly owned by #3484. Merge-ready from where I sit. Not clicking OG stamp unbidden — peer bot has APPROVED, human sign-off is a separate authorization; if James greenlights an OG route on this thread I'll hand off.

— Review by Rames D Jusso

[miguel-heygen]

@santhiprakash sign the commit pls to merge it

[santhiprakash]

@miguel-heygen Both commits on this PR are SSH-signed now (GitHub Verified). Ready to merge.

[santhiprakash]

The required Test check has now failed twice on this PR's merge tree, both times only in packages/studio/src/webmcp/useStudioAgentTools.test.tsx > "registers nothing when the browser has no WebMCP". Since the check is red on an otherwise green+approved PR, I dug in rather than assuming a flake — sharing what I found in case it's useful:

• Disjoint diff: this PR touches only packages/engine/src/utils/ffprobe.ts and packages/producer/*, and the Test workflow runs bun run --filter '!@hyperframes/producer' test, so the failing suite doesn't execute any of this PR's code. main's CI was green on 94da403d/097d901d (which introduced the test ~90 min before our first failing run).
• Mechanism: that test deletes document.modelContext, mounts the harness, and asserts the document still lacks the property. But since 097d901d, the hook falls back to loadModelContextPolyfill() when no native WebMCP exists, and its dynamic import("@mcp-b/global") defines document.modelContext (BrowserMcpServer) when the module settles. The assertion therefore races the dynamic-import resolution inside act(): if the chunk evaluates before the assert, the property exists and the test fails; if it settles later, afterEach deletes it and the test passes. The polyfill load is also cached at module level (pending in polyfill.ts), so whether an earlier test in the same worker already warmed it changes the timing too.
• Suggested hardening (your call — happy to prep a PR if wanted): vi.mock("./polyfill") in that test (or awaiting the pending load before asserting, or asserting on the webmcp.polyfill_loaded telemetry instead of the document property) would make it deterministic. As-is I'd expect it to flake on other PRs and eventually on main as well.
• Re-triggered the checks once via description edit (run 33043020551 → 33045404148, same assertion both times), so this doesn't look like a one-off — but nothing in this PR's diff touches that path, and I can't fix a studio test inside this producer PR without scope-creeping an approved diff.

[santhiprakash]

The red Test check is now green on the latest CI run (CI #33047050255).

• Pushed 68a3862 which mocks @mcp-b/global in useStudioAgentTools.test.tsx so the WebMCP polyfill load no longer races the assertion.
• Both commits on the branch are SSH-signed (GitHub Verified).
• Follow-up scope for PNG-sequence, WebMCP, and commit() API is filed as Artifact validation: PNG-sequence and WebM outputs still bypass the duration/frame gates (follow-up from #3429 review) #3484.

Should be merge-ready once a maintainer gives the final review/merge.

[santhiprakash]

Hi @vanceingalls — HEAD is now 68a3862 (test-only mock for useStudioAgentTools). The latest regression workflow and all matrix shards are green; the only red regression-shards entry in the rollup is a stale concurrency-cancellation artifact from an earlier run. When you have a moment, would you mind re-approving?

[santhiprakash]

Superseded by #3506, which merged the same fix to main. Closing.