fix(producer): validate distributed video metadata

[jrusso1020]

What

• enforce a finite, validated meta/videos.json contract shared by Plan v1 and Plan v2
• preserve authored finite ends and source-derived trim-aware ends; bound any still-open end at the validated composition end
• fail distributed planning when any declared video source did not extract instead of publishing a blank-capable plan
• make the v1 chunk reader reject malformed/null video timing before frame injection
• route deterministic video-source/metadata failures as non-retryable in AWS and GCP while retaining retries for transient extraction failures

Why

An open-ended video whose remote source could not be resolved retained Infinity through planning. Plan v2 correctly rejected that value, while Plan v1 serialized it as null; the v1 frame lookup could then suppress injected frames and silently produce incorrect output.

The invariant belongs at the shared metadata boundary. Both protocols must receive identical finite timing, and unavailable sources must fail closed before plan publication.

Test plan

• producer distributed planning, metadata, v1 chunk boundary, Plan v2 conversion/materialization, and public exports
• core runtime media semantics (authored slots, natural duration, looping, non-looping hold)
• engine video extraction and frame lookup
• AWS Lambda/CDK/SAM and GCP Cloud Run error normalization/retry classification
• producer, core, engine, AWS, and GCP typechecks/builds
• formatting, oxlint, tracked-artifact, fallow, and commit hooks
• exact incident composition replayed through the AWS Lambda handler's Lambda-local path in a Lambda-like container; Plan v1 and Plan v2 both fail closed as VIDEO_SOURCE_UNRENDERABLE during planning, before plan publication
• full PR CI, including all nine regression shards and Windows render/tests

No production flags or deployment/release workflows are changed.

[jrusso1020]

• fix(producer): validate distributed video metadata #2839 👈 (View in Graphite)
• main

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

[miguel-heygen]

Verdict: REQUEST CHANGES

Reasoning: The shared v1/v2 metadata parser and extraction fail-closed work are sound, and I verified the AWS/SAM/CDK and GCP retry tables agree. One blocking seam remains: the plan-side writer throws PlanVideosMetadataError without the INVALID_VIDEO_METADATA code, so the new non-retryable workflow classification is unreachable for the actual planner failure. The adapter tests fabricate an object that already has the code and therefore do not cover this producer→adapter contract. Add the stable code to the real error (and pin that real error through each adapter); then deterministic plan metadata failures fail fast as intended rather than consuming the catch-all retry budget.

[james-russo-rames-d-jusso]

Reviewed at e2cbe32.

Ran a staff-eng lens over the whole diff (shared parser SSOT, buildPlanVideosJson clamp, extraction fail-closed, chunk reader validation, adapter retry-classification updates) with three parallel cross-repo Explore sweeps: (a) all producers/consumers of meta/videos.json, (b) all handlers of VideoElement.end === Infinity | null, (c) cross-repo retry-code symmetry against hyperframes-internal.

On Miguel's P1 blocker (shared.ts:72 — PlanVideosMetadataError has no code, so cloud normalizers never reclassify it and the new non-retryable classification is unreachable for the actual planner-side failure): confirmed independently. Adding a cross-lens sync-list to make the blast radius concrete — every place that needs the fix or its downstream:

• packages/aws-lambda/src/handler.ts:146-158 normalizeTerminalErrorName keys exclusively on candidate.code; PlanVideosMetadataError.code is undefined, so .name stays "PlanVideosMetadataError".
• packages/aws-lambda/src/cdk/HyperframesRenderStack.ts:196-219 NON_RETRYABLE_PLAN set contains "INVALID_VIDEO_METADATA" but not "PlanVideosMetadataError". Step Functions matches on Error.Name, so a bare PlanVideosMetadataError throw retries. Same shape in the SAM template at examples/aws-lambda/template.yaml:266,310.
• packages/gcp-cloud-run/src/server.ts:180-193 normalizeTerminalErrorName — same code-only check. :898-921 NON_RETRYABLE_ERROR_NAMES — has "INVALID_VIDEO_METADATA" but not "PlanVideosMetadataError". Cloud Workflows' HTTP 400↔500 predicate at terraform/workflow.yaml:167-179 then routes it as retryable (500).
• packages/producer/src/services/distributed/renderChunk.ts:135-144 — the chunk-side wrapper correctly attaches the code (RenderChunkValidationError(INVALID_VIDEO_METADATA, ...)), so the seam is asymmetric right now: chunk-side fails closed non-retryably, plan-side fails and retries forever on the identical shape violation. Fix at the source (attach code = INVALID_VIDEO_METADATA to PlanVideosMetadataError) closes both.

Adapter tests (aws-lambda/src/handler.test.ts:246-255, gcp-cloud-run/src/server.test.ts:577-609) fabricate errors that already carry the code, so they don't exercise the producer→adapter contract Miguel called out — an integration test that lets a real PlanVideosMetadataError fly out of plan() would pin this.

Sibling gap that survives the fix (worth flagging in the same PR or a stacked follow-up):

• hyperframes-internal/packages/producer-internal/src/distributedHandlers.ts:91-102 — the NonRetryableErrorCode TypeScript union is missing "INVALID_VIDEO_METADATA", "VIDEO_SOURCE_UNRENDERABLE", and "VIDEO_EXTRACTION_FAILED". The comment at :91 says "Mirrored in the Python DISTRIBUTED_NON_RETRYABLE_ERROR_CODES tuple — keep in sync" — presumably that Python tuple is stale too. Runtime code at :334-336 passes err.code through so RenderChunkValidationError("INVALID_VIDEO_METADATA") still surfaces as errorCode 422 by accident, but the contract advertised by the union is behind. distributedHandlers.test.ts:218-222 parameterizes only over ["PLAN_HASH_MISMATCH", "MISSING_PLAN_ARTIFACT", "CHUNK_INDEX_OUT_OF_RANGE"] — none of the new codes are covered. Same class of drift as the P1 above, just on the internal sidecar surface.

Adjacent pre-existing Infinity-end holes the PR does NOT close (not blockers, but the "silent-blank open-ended video" bug class is bigger than the distributed pipeline):

• packages/producer/src/services/render/stages/probeStage.ts:459-464 — if (projectedEnd > 0 && (existing.end <= 0 || ...)). existing.end === Infinity doesn't satisfy existing.end <= 0, so the browser reconciliation can't overwrite an Infinity end. Same shape at the audio twin at :505-510. Widening to !Number.isFinite(existing.end) || existing.end <= 0 closes it.
• packages/engine/src/services/audioMixer.ts:572-577 — if (element.end - element.start <= 0) gates the fallback probe. Infinity - start === Infinity, which is > 0, so the probe is skipped and ffmpeg gets Infinity fed as -t duration downstream — reads to EOF instead of failing closed.
• packages/producer/src/services/render/videoFrameCoverage.ts:125-130 — Number.isFinite(end) guard returns 0 (coverage of 0 passes trivially), so the coverage error can't catch the exact silent-blank clip this PR is trying to eliminate on the in-process path.
• In-process parity: the whole clamp in buildPlanVideosJson at shared.ts:246-266 lives only on the distributed writer. In-process (executeRenderJob) has no equivalent gate — if a top-level video's src is unresolvable and extraction never mutates end, Infinity survives into FrameLookupTable construction (video id isn't added → runtime falls back to raw <video> decode). Distributed now fails closed here; in-process still silent-fails.

The other things I checked and did NOT flag: planV2's private parsePlanVideosJson → shared parser refactor at planV2.ts:330-339 preserves the PlanV2IntegrityError re-wrap; parseSharedPlanVideosJson tightens two invariants (videoCodec non-empty, one-to-one videos↔extracted, no duplicate ids) that planV2 previously didn't enforce — both tightenings are safe because planV2 planDirs are planner-controlled and always come from buildPlanVideosJson. The two new plan-side tests (plan.test.ts:400-475) both correctly assert existsSync(join(brokenPlanDir, "meta", "videos.json")) === false on the fail-closed path. The Number.POSITIVE_INFINITY / NaN / 0 / 2 (matches video.start=2) parameterization in videoMetadata.test.ts:97-108 covers the "no safe boundary" branch cleanly. rebuildExtractedFramesFromPlanDir's dense-v1 vs sparse-v2 split (renderChunk.ts:342-390) is orthogonal to this PR's scope and unchanged.

CI at head: Format / Preflight / Typecheck / SDK / Studio / Producer unit+integration / CLI smoke / all completed regression shards green; several long-running shards + Windows Tests still in flight at time of review — worth a re-check before merge.

Nothing in the diff itself I'd block on beyond Miguel's P1 (which is real). LGTM from my side once that seam attaches the stable code and the internal-repo union is either updated in the same drop or flagged as a follow-up.

— Review by Rames D Jusso

[vanceingalls]

Code Review: fix(producer): validate distributed video metadata

Overall verdict: Approve with important finding. The core fix is correct and well-structured. One classification gap survives — a low-frequency path where a PlanVideosMetadataError escapes plan() untagged and gets retried 4× instead of failing terminal. The other points are nits. The incident-class bug (Infinity→null→frame suppression) is fully closed.

---

1. Correctness — does the fix close the Infinity→null bug?

Yes, completely.

The bug path was: composition.videos[i].end = Infinity → JSON.stringify serializes Infinity as null → v1 chunk reader parsed naively → null end passed to frame lookup → no injection window → blank video.

The fix closes it at the write side in plan.ts: buildPlanVideosJson maps any non-finite end to compositionEnd, then immediately round-trips through parsePlanVideosJson, which applies readFiniteNumber to every end field. A null end fails the parse — so a well-formed videos.json can never contain a null or Infinity end.

The fix also closes it at the read side: validatePlanVideosForChunk wraps parsePlanVideosJson before any frame injection, so a corrupted or legacy artifact with null timing trips a non-retryable INVALID_VIDEO_METADATA rather than silently producing blank output.

One scenario to verify on your end (works correctly per the code but worth confirming operationally): a composition whose data-duration resolves to a positive finite value will bound open-ended videos at that composition end. The test at videoMetadata.test.ts:1057 confirms the frame lookup table stays active at t=7 and shuts off at t=8.01, which matches in-process hold-last semantics for non-looping clips.

---

2. Shared boundary — is shared.ts actually shared by both v1 and v2?

Yes. Verified three call sites:

• v1 write (plan.ts): calls buildPlanVideosJson which internally calls parsePlanVideosJson
• v1 read (renderChunk.ts): calls validatePlanVideosForChunk → parsePlanVideosJson
• v2 read (planV2.ts): calls parseSharedPlanVideosJson (alias of parsePlanVideosJson) wrapped in a re-throw that converts PlanVideosMetadataError to PlanV2IntegrityError

The boundary is correct. Both protocols receive identical validated finite timing from the same validator.

---

3. Fail-closed behavior before publication

Holds. Verified ordering in plan.ts:

1. runExtractVideosStage(...) runs extraction
2. assertVideoExtractionSucceeded(extractionResult) throws if any source failed — this is before buildPlanVideosJson and before writeFileSync(videos.json)
3. buildPlanVideosJson throws PlanVideosMetadataError if any end can't be resolved — also before writeFileSync
4. Only on success: writeFileSync(join(planDir, PLAN_VIDEOS_META_RELATIVE_PATH), ...)

The integration test at plan.test.ts:312 asserts existsSync(join(brokenPlanDir, "meta", "videos.json")) is false after a failed plan, confirming the file is never written on failure. Fail-closed holds.

One subtlety: assertVideoExtractionSucceeded is called unconditionally — it ignores the HF_VIDEO_EXTRACTION_FAILURE_MODE canary that runExtractVideosStage internally respects. The comment in plan.ts makes this explicit and intentional. That's the right call for distributed, but means any composition currently silently degrading on extraction failure will now hard-fail on deploy. Worth confirming the current production extraction-failure rate before shipping to avoid converting silent-degrade into mass hard-fail.

---

4. Retry classification — [important]

There is a classification gap in the plan-time path.

buildPlanVideosJson throws PlanVideosMetadataError — an Error subclass with .name = "PlanVideosMetadataError" and no .code property. This throw is uncaught in plan().

At the AWS boundary, normalizeTerminalErrorName rewrites .name only when .code matches a known set. Since PlanVideosMetadataError has no .code, the name stays "PlanVideosMetadataError". That name is absent from the SAM template's MaxAttempts: 0 non-retryable lists, the CDK's non-retryable sets, and GCP's NON_RETRYABLE_ERROR_NAMES.

Result: a plan-time PlanVideosMetadataError gets retried 4× before failing terminal, adding ~2 minutes of wasted retry time per affected render.

Realistic reachability: Not reachable in the primary incident scenario (assertVideoExtractionSucceeded fires first, correctly classified as VIDEO_SOURCE_UNRENDERABLE). But reachable if extraction reports success while producing an empty extracted array for a declared video, or if job.duration is zero/NaN for a composition with an open-ended video.

Fix: In plan.ts, wrap buildPlanVideosJson in a catch that converts PlanVideosMetadataError to a coded error (INVALID_VIDEO_METADATA), or have buildPlanVideosJson throw a coded error directly. Add a test asserting the classification.

Note: the chunk path is already correct — validatePlanVideosForChunk re-throws as RenderChunkValidationError(INVALID_VIDEO_METADATA), which is non-retryable everywhere. Only the plan-time call is unguarded.

---

5. Test coverage

Strong overall. The tests exercise the actual failure modes:

• videoMetadata.test.ts: buildPlanVideosJson bounds open-ended clips; parsePlanVideosJson rejects serialized-null timing; frame lookup stays active through the bounded window. Directly covers the incident scenario.
• renderChunkVideoMetadata.test.ts: validatePlanVideosForChunk rejects a null end → INVALID_VIDEO_METADATA. Covers the legacy-artifact read path.
• plan.test.ts: Two integration tests assert videos.json is absent and error code is VIDEO_SOURCE_UNRENDERABLE. Covers the fail-closed-before-publish path.
• planV2.test.ts: Confirms v2 accepts and materializes the same bounded timing v1 produces. Covers the parity claim.

Gap: No test covers PlanVideosMetadataError classification at the adapter boundary (the gap in point #4). A unit test asserting that a buildPlanVideosJson throw from plan() resolves to a non-retryable code at the handler would lock in the invariant.

Minor pre-existing gap: No test for a video with authored end <= start as a finite value — the parser copies finite ends unchanged without range-checking. Not introduced here, but adjacent to the timing invariant work.

---

6. Plan v1/v2 parity

Holds. buildPlanVideosJson is called once at plan write time. The written videos.json contains only finite ends. Plan v2 reads the same videos.json via parseSharedPlanVideosJson, which validates finitude. Both protocols fail on the same malformed input via the shared PlanVideosMetadataError. The planV2.test.ts test at line 438 validates that a v2 materialization of a bounded open-ended video carries the correct finite end.

---

7. Additional findings

[nit] parsePlanVideosJson does not validate end > start for authored finite ends. A video authored with start=5, end=3 would pass validation and produce degenerate frame injection behavior. Pre-existing gap — mentioning because this PR is the natural place to close timing invariants.

[nit] The compositionEnd <= 0 guard uses <= 0 rather than < 0. A composition with exactly 0s duration is degenerate and should fail regardless, so the behavior is correct — a comment explaining why 0 is excluded would help. Covered by the test at videoMetadata.test.ts:1023.

[nit] GCP's NON_RETRYABLE_ERROR_NAMES includes RenderChunkValidationError (the class name), meaning any future RenderChunkValidationError with a new code is non-retryable by default. Probably the right default, but worth noting as an implicit policy.

[verified non-issue] The RenderChunk state in the SAM template omits VIDEO_SOURCE_UNRENDERABLE from its non-retryable list — correct, because VIDEO_SOURCE_UNRENDERABLE only fires at plan time, not chunk time.

[verified non-issue] VideoElement has exactly 7 fields. parsePlanVideosJson reads exactly those 7. No field-stripping regression from the old verbatim write.

---

Summary

The core fix is sound: shared validator eliminates the protocol split, buildPlanVideosJson prevents Infinity/null from reaching disk, assertVideoExtractionSucceeded gates before publication, and validatePlanVideosForChunk rejects legacy-format artifacts at read time. Tests cover all three layers. The one finding worth addressing before merge is the PlanVideosMetadataError retry classification — low-frequency but a real wasted-retry gap in a PR whose stated goal is fail-closed reliability.

Review by Vai

[miguel-heygen]

R2 delta review at 1555ff63d — the P1 is closed at the source contract.

PlanVideosMetadataError now owns code = INVALID_VIDEO_METADATA in packages/producer/src/services/distributed/shared.ts:75, so the actual exception thrown by buildPlanVideosJson() is classifiable without adapter-specific knowledge. Both cloud tests now drive that real class through their public boundaries: AWS asserts the Step Functions error name becomes INVALID_VIDEO_METADATA (packages/aws-lambda/src/handler.test.ts:256), while GCP asserts HTTP 400 plus the same discriminator (packages/gcp-cloud-run/src/server.test.ts:611). The public re-export at packages/producer/src/distributed.ts:146 keeps those adapter tests on the package contract rather than a private import.

I also rechecked the full original retry tables and the plan/chunk asymmetry: AWS SAM/CDK and GCP all classify INVALID_VIDEO_METADATA terminal, and the chunk-side coded wrapper remains unchanged. Full exact-head CI is green, including producer unit/integration, both Windows lanes, and all regression shards.

Cross-repo deployment note, tracked on HFI#531 rather than blocking this source PR: the internal sidecar is still pinned to published producer 0.7.76, so it must bump to the first release containing this head before the production producer→sidecar chain is live.

Verdict: APPROVE
Reasoning: The real plan-side error now carries the stable code, and both adapters test that concrete object through the boundary that previously false-greened.

— Magi

[james-russo-rames-d-jusso]

Reviewed at 1555ff63.

R1→R2 delta is 4 files / ~40 net lines: the .code field lands on PlanVideosMetadataError, both cloud-adapter tests are re-wired to throw the real class (rather than a fabricated {code, name}), and PlanVideosMetadataError is re-exported from packages/producer/src/distributed.ts so downstream tests can construct it. All my R1 concerns and Miguel's P1 are cleanly resolved. Vance's APPROVAL landed against this head at pullrequestreview-4794487451.

Blockers

None.

Concerns

Non-blocking — SSE-transport defense-in-depth divergence. R2 also added INVALID_VIDEO_METADATA to the OSS producer's streaming-path allowlist at packages/producer/src/server.ts:122:

const SAFE_RENDER_ERROR_CODES = new Set<string>([
  "INVALID_VIDEO_METADATA",
  "VIDEO_SOURCE_UNRENDERABLE",
  "VIDEO_EXTRACTION_FAILED",
]);

Meaning the OSS streaming/render envelope (/render at server.ts:704, /render-stream at server.ts:542) is now permitted to surface errorCode: "INVALID_VIDEO_METADATA". That's the right forward-looking shape.

But the corresponding sanitizer allowlists downstream weren't extended:

• HFI producerErrorTransport.ts:9-12 — TERMINAL_VIDEO_EXTRACTION_ERROR_MESSAGES still has only 2 codes. sanitizeProducerErrorPayload at line 55 fails open — non-allowlisted errorCode values return the raw payload with the original error message intact. Since internal-server.ts:118, 121, 124 wraps /render and /render-stream responses through this sanitizer, a future in-process path that surfaces INVALID_VIDEO_METADATA (currently unreachable — PlanVideosMetadataError is only thrown from distributed plan()) would leak the raw producer message.
• EF hyperframes_producer_failure.py:18-41 — ProducerTerminalErrorCode Literal + _PRODUCER_TERMINAL_ERROR_CODES tuple + _PRODUCER_TERMINAL_ERROR_MESSAGES dict — same divergence. producer_terminal_error_code(event) returns None for INVALID_VIDEO_METADATA and the streaming classifier falls back to retryable RuntimeError.

Today PlanVideosMetadataError only fires from services/distributed/plan.ts:1064 (buildPlanVideosJson in the distributed plan activity), so the streaming envelope currently doesn't carry this code and the leak isn't reachable. But the OSS-side allowlist expansion in this PR moves the boundary forward without moving the downstream boundaries, which trades an in-band bug for a latent divergence. Either extend the two sibling allowlists in the same stack, or explicitly document that streaming-path emission of INVALID_VIDEO_METADATA is intentionally forbidden and revert the OSS SAFE_RENDER_ERROR_CODES addition.

Note also that packages/producer/src/services/distributed/renderChunk.ts:135-144 throws RenderChunkValidationError with .code = "INVALID_VIDEO_METADATA" — that class isn't PlanVideosMetadataError, but its .code is the same and if it ever bubbles out of an in-process render() call the streaming envelope would carry it (haven't traced whether that path exists).

Nits

• shared.ts:74: the // fallow-ignore-next-line unused-class-member directive reads a little odd since code IS read across the package boundary by both cloud normalizers — but this is an existing HF idiom (see aws-lambda/src/chromium.ts:49) for fields with cross-package consumers that the in-package linter can't see, so no change needed.

Questions

None.

What I didn't verify

• Whether any in-process (non-distributed) code path in the OSS producer can plausibly throw RenderChunkValidationError({code: "INVALID_VIDEO_METADATA"}) and surface via the streaming envelope. That would turn the "concern" above into a reachable defense-in-depth gap. Left as a future-proofing question.
• Full test run of packages/producer/src/services/distributed/videoMetadata.test.ts locally — trusting CI green.

— Review by Rames D Jusso

[jrusso1020]

Merge activity

• Jul 28, 7:42 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jul 28, 7:42 AM UTC: @jrusso1020 merged this pull request with Graphite.