feat(producer): refuse distributed-unsupported formats (webm + HDR mp4)#815
Conversation
27a3d70 to
5f0239a
Compare
07b1a71 to
60e67b7
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Review: refuse distributed-unsupported formats (webm + HDR mp4)
Good upgrade from ad-hoc throws to a typed error class. Validation runs before mkdirSync(planDir) so banned configs never leak partial directories — tests verify existsSync(planDir) is false after the throw. Exporting rejectUnsupportedDistributedFormat separately lets adapters gate at their own input layer.
Minor note
rejectUnsupportedDistributedFormat checks hdrMode === "force-hdr" but reports format: "mp4-hdr" regardless of the actual format. A mov + force-hdr would be rejected with the wrong format in the error. Low severity — mov + force-hdr is likely an edge case — but worth matching the actual config format in the error.
LGTM.
vanceingalls
left a comment
There was a problem hiding this comment.
Format banlist for distributed mode — refuses webm + HDR mp4 with a typed non-retryable error before any planDir work happens. Replaces the ("force-hdr" as unknown) cast from #808 with a proper error class. Tight, targeted, well-tested.
Strengths
plan.ts:46-83—FormatNotSupportedInDistributedErrorcarriesformatANDreasonfields. Adapter retry policies can route oncode, observability can group byformat, error messages get the user-friendlyreason. Three orthogonal axes from one error class.plan.ts:63-84— exportingrejectUnsupportedDistributedFormatas a pure helper means adapters can pre-validate at their input layer (Step Functions input validation, Temporal workflow start) before the activity even runs. The error class is the same oneplan()would have thrown — so adapter-side and activity-side rejections look identical to the workflow.plan.ts:447-450— the validator runs as the very first line ofplan(), BEFOREmkdirSync(planDir). End-to-end tests atplanFormatBanlist.test.ts:222-267assertexistsSync(planDir) === falsepost-throw — pins the no-leaked-partial-planDir contract.- Replaces the
("force-hdr" as unknown)cast from #808 with the typed class. Cleanup that also widens the error surface.
Findings
important — the banlist enforces at plan-time but the chunk worker doesn't re-validate (plan.ts:447, renderChunk.ts:622-700)
rejectUnsupportedDistributedFormat runs once at plan time. The chunk worker reads plan.dimensions.format and uses it to branch (renderChunk.ts:684, assemble.ts:483) but never re-checks against the banlist. So a planDir hand-crafted (or generated by an out-of-date producer) with plan.dimensions.format === "webm" would skip the validator and run all the way to chunk capture, where it'd produce garbage instead of erroring with the typed code.
This is Rule 2 territory: the contract ("distributed mode rejects webm + HDR") is satisfied at one site (plan()), but every consumer of plan.dimensions.format is implicitly trusting the contract holds. Cheap defense: call rejectUnsupportedDistributedFormat({ format: plan.dimensions.format }) at the top of renderChunk and assemble too. Three lines, same error class, no behavior change on valid inputs.
important — hdrMode is checked but force-hdr could still leak via producerConfig (plan.ts:77-83)
The validator checks config.hdrMode, but a caller passing producerConfig: { ... hdrMode: "force-hdr" } (if such a field exists in EngineConfig) gets it merged into cfg at plan.ts:616-620. The validator doesn't see the merged value. Trace through whether EngineConfig carries hdrMode — if it does, the same defense as above: validate against the resolved/merged config, not just the caller's direct config.hdrMode.
Trivial to check; if EngineConfig doesn't carry hdrMode this is a non-issue. Worth a quick grep to confirm.
nit — the error message could be more discoverable about which format is supported (plan.ts:33-37)
super(
`[plan] format ${JSON.stringify(format)} is not supported in distributed mode: ${reason}. ` +
`Render with the in-process renderer (\`executeRenderJob\`) — it has full format ` +
`support — or pick a distributed-supported format: mp4 SDR, mov ProRes 4444, or ` +
`png-sequence.`,
);Good as is. The supported list at the end is exactly what an adopter hitting this error wants. Flagging only because the analogous message in PlanTooLargeError (#814) names the size cap AND the v1.5 roadmap link — same shape would let this error message point at the v1.5 issue where webm/HDR support is planned, so adopters can subscribe instead of working around in their own code.
nit — test could assert the reason field (planFormatBanlist.test.ts:170-219)
The tests assert code, format, and the message contains certain substrings — but never assert reason is set on the error instance. The reason field carries the "why was this banned" diagnostic that adapters might surface to users; pin it with a test so a future refactor doesn't accidentally drop it.
Verdict: APPROVE
Reasoning: Clean, focused, replaces a smelly cast with a proper typed error class. The Rule-2 finding (chunk worker / assemble don't re-validate) is a real gap but the architecture currently funnels all entries through plan(), so it's an in-spec gap worth fixing in a follow-up — not a blocker.
— Vai
5f0239a to
819a06c
Compare
60e67b7 to
72407de
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review
PR rebased onto the new bases. rejectUnsupportedDistributedFormat is called at the top of plan() (plan.ts:100), exported via #816, and tested for both webm and force-hdr rejection paths. The (config.format as string) cast at the runtime branch handles the JS-caller-passes-webm-anyway case — belt-and-suspenders against the TS type narrowing.
Net-new findings
None gating. Single call site is acceptable here because (a) the gate is exported so adapters can re-call it at their own input layer (per the module doc), and (b) format/hdrMode types are narrowed at the public surface so TS callers can't get past compilation.
Verdict: APPROVE
Reasoning: Rebase only; content matches prior approval. Single-site enforcement is fine given the type narrowing + adapter-side gate availability. CI required checks green.
— Vai
819a06c to
dbbd329
Compare
81975cc to
5c789ef
Compare
6aca0b4 to
8590f07
Compare
5c789ef to
72c6cb5
Compare
e813fcb to
fbbd41a
Compare
72c6cb5 to
a1ab804
Compare
a1ab804 to
552027b
Compare
The base branch was changed.
552027b to
e506848
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamping after rebase. LGTM.
Merge activity
|

What
Phase 3 of the distributed rendering plan: §11 PR 3.5 format banlist. Extends
plan()to refuse two v1-unsupported formats up front with a typed non-retryableFormatNotSupportedInDistributedError(code === "FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED").Why
Both webm and HDR mp4 are documented as deferred to v1.5 (§7.2 + §12), but until this PR the only signal at the runtime layer is the in-process pipeline silently producing wrong output (chunk concat-copy doesn't round-trip VP9; HDR signaling gets stripped at the chunk boundary). Failing fast at
plan()time keeps adopters from spending fan-out compute on a render that can't succeed and gives them a typed error code their workflow adapter can route on.How
services/distributed/plan.ts:FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED— non-retryable error code matching §11's wording.FormatNotSupportedInDistributedError— typed error class withcode,format, andreasonfields. Message names the rejected format and tells adopters to fall back to the in-process renderer (executeRenderJob) which has full format support.rejectUnsupportedDistributedFormat(config)— pure helper exported separately so adapters can run the same gate at their input layer (Step Functions input validation, Temporal workflow start) before the activity even runs.plan()callsrejectUnsupportedDistributedFormat(config)as the first line of the function — BEFOREmkdirSync(planDir)so a banned input never produces a partial planDir.if (hdrMode === "force-hdr") throw new Error(...)with the typed error class.What did NOT change
executeRenderJob, the in-process orchestrator, thehyperframes renderCLI, producer HTTP routes — all unchanged. The in-process renderer continues to accept webm + HDR (its existing functionality).Test plan
packages/producer/src/services/distributed/planFormatBanlist.test.ts. 5 cases:rejectUnsupportedDistributedFormataccepts the v1-supported formats (mp4, mov, png-sequence) with bothautoandforce-sdrhdrMode.code === FORMAT_NOT_SUPPORTED_IN_DISTRIBUTED,format === "webm", message mentions in-process renderer.hdrMode === "force-hdr") — error hasformat === "mp4-hdr", message mentions HDR.plan(): webm throws with no planDir leaking to disk.plan(): HDR mp4 throws with no planDir leaking to disk.bun test packages/producer/src/services/distributed/— 30 pass.bun run --filter @hyperframes/producer typecheck— clean.bunx oxlint+bunx oxfmt --check— clean on changed files.executeRenderJobis unchanged; PSNR baselines should hold.This is PR 5 of a 6-PR Phase 3 stack:
services/distributed/plan.ts(feat(producer): add services/distributed/plan.ts #808)services/distributed/renderChunk.ts(feat(producer): add services/distributed/renderChunk.ts #809)services/distributed/assemble.ts(feat(producer): add services/distributed/assemble.ts #813)planDirsize cap (PLAN_TOO_LARGE) (feat(producer): enforce planDir size cap (PLAN_TOO_LARGE) #814)@hyperframes/producer/distributedsubpath🤖 Generated with Claude Code