refactor(producer): share plan execution builder - #2906
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 review @ 9f3e9ce
Verdict: APPROVE. This is a compatibility-preserving refactor that does exactly what the description promises: it removes the migration-era illusion that v2 invoked a v1 planner while keeping every public serialized shape, every event union, every SDK handle key set, and every manifest byte the same. The test suite locks each of those contracts explicitly.
Grade: A Rubric: CORRECT
Focus-by-focus:
1. Public serialized-shape compatibility. Contracts locked in code:
- V1 descriptor byte-identity:
JSON.stringify(PLAN_PROTOCOL_V1) === '{"schemaVersion":1,"artifactLayout":"plan-dir-v1","hashSchema":"hyperframes-plan-hash-v1"}'(planProtocol.test.ts:785), ANDPLAN_PROTOCOL_V1 === CURRENT_PLAN_PROTOCOL(identity, not just structural equality — same frozen object reference viaexport const CURRENT_PLAN_PROTOCOL = PLAN_PROTOCOL_V1). Downstreamx.protocol === CURRENT_PLAN_PROTOCOLcomparisons continue to succeed. - V1 result:
plan()returns the exact historical{ planDir, planProtocol, planHash, chunkCount, totalFrames, fps, width, height, format, ffmpegVersion, producerVersion }key set (plan.test.ts:596). - V1 SDK handles:
renderToLambdareturn locked to["renderId","executionArn","bucketName","stateMachineArn","outputS3Uri","projectS3Uri","startedAt"](renderToLambda.test.ts:174);renderToCloudRunlocked to["renderId","executionName","bucketName","workflowId","outputGcsUri","projectGcsUri","startedAt"](renderToCloudRun.test.ts:326). New assertions — these were not test-locked before, so the PR strengthens the compat contract. - V2 manifest keys (
planV2.test.ts:961):["artifacts","chunkCount","ffmpegVersion","format","fps","height","limitations","planHash","producerVersion","protocol","sourcePlanV1Hash","totalFrames","width"]+Object.hasOwn(manifest, "executionPlanHash") === false. - V2 result keys (
:945): 14-key set includingsourcePlanV1Hash+Object.hasOwn(first, "executionPlanHash") === false. - V2 chunk materialization keys (
:1008): 7-key set + noexecutionPlanHash. - V1 event types (
aws-lambda/src/events.ts,gcp-cloud-run/src/events.ts): only JSDoc@deprecatedtags added. Field shapes untouched —PlanV1Event.PlanProtocol?: "v1",PlanV1LambdaResult.PlanS3Uri,PlanV1ResultBody.PlanGcsUri, etc. — verified against the diff, nothing removed or renamed.
No new enumerable alias fields anywhere. The neutral getPlanV2ExecutionPlanHash(manifest) is a function, not a property — it reads manifest.sourcePlanV1Hash without exposing an executionPlanHash field on manifests, results, or chunks. This is exactly the "no new enumerable field" contract James called out, and it's the correct shape for it (an accessor gives you the neutral name in code without polluting serialized shapes).
2. Shared planner architecture. buildLocalExecutionPlan(projectDir, config, executionPlanDir, options) (plan.ts:700) is the neutral internal builder — its LocalExecutionPlan return has no planProtocol field (protocol-agnostic). Two wrappers consume it:
plan()(plan.ts:747) is now a strict remapper: awaitsbuildLocalExecutionPlan()and mapsexecutionPlanHash → planHash,executionPlanDir → planDir, tacks onplanProtocol: PLAN_PROTOCOL_V1. Same input → same result shape.- V2 (
planV2WithPublisher,planV2.ts:1360):await buildLocalExecutionPlan(projectDir, config, stagingRoot, { executionPlanSizeLimitBytes: Number.MAX_SAFE_INTEGER }); return await publishPlanV2FromExecutionPlan(stagingRoot, publisher);. V2 no longer transits throughplan()— the shared builder is called directly, and v2 explicitly opts out of the historical v1 monolithic-plan-size cap via the newexecutionPlanSizeLimitBytesoption (which takes precedence overconfig.planDirSizeLimitBytes, preserved as fallback for v1).
The layering is clean: shared builder owns compile/probe/extract/audio/freeze; the two transport paths differ only in how they wrap that output. This is the "migration-era coupling" the description names, and the removal is real.
3. Preserved v1 compatibility.
- Omitted
planProtocol→ v1:readPlanProtocol({ planHash: "legacy" }) === CURRENT_PLAN_PROTOCOL(test-locked at:790, unchanged). - Cloud SDKs default-to-v1 preserved:
renderToCloudRun.test.ts:348asserts full input equals{... PlanProtocol: "v1"}when caller passes no protocol — new assertion, defensively locked. - Legacy exports retained:
plan,renderChunk,assemble,CURRENT_PLAN_PROTOCOL,PlanV1Event,RenderChunkV1Event,AssembleV1Event,PlanV1LambdaResult,PlanV1ResultBody. NewPLAN_PROTOCOL_V1added alongsideCURRENT_PLAN_PROTOCOL(as its underlying reference); nothing renamed at the public surface, nothing removed.createPlanV2FromV1/publishPlanV2FromV1retained as one-line delegates to the neutral names, marked@deprecated. freezePlan.tsnow emitsPLAN_PROTOCOL_V1instead ofCURRENT_PLAN_PROTOCOL— but since these are the same frozen object reference,plan.json'sprotocolfield is byte-identical to before. Rename-at-emission-site, no wire change.
4. Byte-identical v2 manifests across canonical + legacy paths. Directly test-locked at planV2.test.ts:980 — the new "keeps legacy conversion names as byte-identical compatibility aliases" test:
const canonical = createPlanV2FromExecutionPlan(executionPlanDir, join(root, "canonical"));
const compatibility = createPlanV2FromV1(executionPlanDir, join(root, "compatibility"));
const published = await publishPlanV2FromExecutionPlan(executionPlanDir, publisher);
expect(readFileSync(canonical.manifestPath)).toEqual(readFileSync(compatibility.manifestPath));
expect(getPlanV2ExecutionPlanHash(published)).toBe(getPlanV2ExecutionPlanHash(canonical));
expect(published.sourcePlanV1Hash).toBe(canonical.sourcePlanV1Hash);
expect(Object.hasOwn(published, "executionPlanHash")).toBe(false);Byte-equality of the serialized plan.json is asserted with readFileSync (not just parsed structural equality), so any drift in ordering, whitespace, or hash framing surfaces. And this holds by construction, not just observation: createPlanV2FromV1 is now a one-line delegate to createPlanV2FromExecutionPlan, so the two paths run the same code. The determinism test above (:924) also asserts a second invocation yields byte-identical bytes.
The sourcePlanV1Hash manifest wire key is retained while the internal value is the neutral execution-plan hash — the comment at planV2.ts:1274 calls this out ("Retain the established manifest key byte-for-byte. It now acts as the wire alias for the neutral local execution-plan hash").
Findings.
- P1: none.
- P2: none.
- Non-blocker (integrity error string rename):
"v1 plan content fingerprint does not match plan.json.planHash"→"execution plan content fingerprint does not match plan.json.planHash"(planV2.ts:1241). If any Datadog monitors or log scrapers key on the old phrase, they'll miss the new one. Worth a heads-up to whoever owns log-based alerting on the producer; not a code defect. - Non-blocker (staging dir prefix rename):
.plan-v2-source-→.execution-plan-on the mkdtemp prefix (planV2.ts:1358). Same log-observability concern as above — very low blast radius since it's a temp-dir name, but flagging for completeness. - Non-blocker (
@deprecatedJSDoc): the deprecation tags added toPlanV1Event,PlanV1LambdaResult,RenderChunkV1Event,AssembleV1Event,PlanV1ResultBody,plan(),PlanResult,createPlanV2FromV1,publishPlanV2FromV1,PlanV2Manifest.sourcePlanV1Hash,PlanV2Result.sourcePlanV1Hash, andPlanV2MaterializationResult.sourcePlanV1Hashare correct (the intent is to nudge new integrations toward v2). Existing consumers pinning strict TypeScript may see new deprecation warnings at compile time — this is intended, but worth flagging so downstream teams aren't surprised. - Non-blocker (adversarial future-drift risk): the accessor pattern (
getPlanV2ExecutionPlanHash(manifest) → manifest.sourcePlanV1Hash) means the wire key and the accessor name have permanently drifted. That's fine today, but a future author renaming the manifest field without updating the accessor would silently returnundefined. Consider areadonlytype-lock on the accessor'sPick<PlanV2Manifest, "sourcePlanV1Hash">return (already there — good) plus one small runtime assertion (typeof plan.sourcePlanV1Hash === "string") inside the accessor. Not needed for this PR.
CI state. All required checks green at the head SHA:
- Full CI matrix (Format, Lint, Typecheck, Test, Build, Producer unit + integration, SDK unit+contract+smoke, CLI smoke, Runtime contract, Studio smoke, Fallow audit, Semantic PR title, File size): SUCCESS.
- 9/9 regression shards: SUCCESS.
- Windows render verification (post-rerun): SUCCESS.
- CodeQL (actions + JS/TS + Python): SUCCESS.
- Docs / Mintlify: SUCCESS. Preview-regression, Graphite, Player perf: SUCCESS.
- One earlier
regressionFAILURE row exists in the rollup at 91029316323 but that was CANCELLED and re-ran at 91032478954 SUCCESS (cancel-and-restart on stack update). Not a real failure.
Test coverage. 141 focused compat tests + 990 producer unit + 1,419 core + 140 Lambda + 101 Cloud Run. The 200/200 James mentions in the ask maps to the focused Plan-v1/v2 compat surface — verified by counting the assertions in planV2.test.ts, plan.test.ts, planProtocol.test.ts, publicExports.test.ts, and planSizeCap.test.ts. Coverage feels exhaustive for the surface being refactored.
Ship it.
— Via
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
PR #2906 Review: refactor(producer): share plan execution builder
Overview
+494/−171 across the producer, aws-lambda, and gcp-cloud-run packages. The refactor extracts a shared buildLocalExecutionPlan() builder and makes plan() a thin deprecated v1 wrapper while v2 publishes from the shared representation directly. Well-executed — the compatibility surface is thorough.
1. Serialized-shape compatibility — SAFE
Every wire-visible surface verified:
| Object | Assertion type | New executionPlanHash field? |
|---|---|---|
v1 PlanResult |
Object.keys(b).toEqual([…11 keys…]) |
Not present |
v2 PlanV2Result |
Object.keys(first).toEqual([…14 keys…]) |
hasOwn → false |
| v2 manifest (on-disk JSON) | Object.keys(manifest).toEqual([…13 keys…]) |
hasOwn → false |
| v2 materialization result | Object.keys(chunk).toEqual([…7 keys…]) |
hasOwn → false |
| Lambda SDK handle | Object.keys(handle).toEqual([…7 keys…]) |
N/A |
| Cloud Run SDK handle | Object.keys(handle).toEqual([…7 keys…]) |
N/A |
| Lambda SFN input | toEqual() (full shape) |
N/A |
| Cloud Run workflow arg | toEqual() (full shape) |
N/A |
The sourcePlanV1Hash wire key is retained byte-for-byte everywhere. The new getPlanV2ExecutionPlanHash() accessor reads plan.sourcePlanV1Hash under the hood — neutral API name, established wire key unchanged. The test at line 991 additionally asserts published.sourcePlanV1Hash === canonical.sourcePlanV1Hash through the accessor.
buildLocalExecutionPlan and LocalExecutionPlan are correctly NOT re-exported from distributed.ts or index.ts — they're internal to the package.
2. Shared planner architecture — SOUND
buildLocalExecutionPlan() is a true SSOT. It is the previous plan() body with neutral return field names (executionPlanDir, executionPlanHash). Both transports consume it as peers:
- v1 path:
plan()→buildLocalExecutionPlan()→ explicit field mapping toPlanResult(14 lines, no logic, no computation) - v2 path:
planV2WithPublisher()→buildLocalExecutionPlan()→publishPlanV2FromExecutionPlan()
Zero independent planning logic in either path. The v2 path's buildPlanV2Publication() is transport concern (listing files, hashing blobs, building the manifest), not planning logic.
The v2 publication path still calls readPlanProtocolV1() on the execution directory's plan.json — this is an integrity check, not a v1 concept leak. The local execution directory IS a v1-layout directory (written by freezePlan with PLAN_PROTOCOL_V1), and this validation ensures it's well-formed before publishing. The inline comment at the call site documents this: "The shared local representation intentionally retains the v1-compatible descriptor while both legacy readers and v2 materialization are supported."
3. v1 compatibility — PRESERVED
- Omitted
planProtocol→readPlanProtocolreturnsPLAN_PROTOCOL_V1(unchanged behavior, tested) PLAN_PROTOCOL_V1 === CURRENT_PLAN_PROTOCOL— same object reference (identity test)- JSON byte-identical:
'{"schemaVersion":1,"artifactLayout":"plan-dir-v1","hashSchema":"hyperframes-plan-hash-v1"}' - v1 event types (
PlanV1Event,RenderChunkV1Event,AssembleV1Event) — only JSDoc@deprecatedadded, no field changes - All deprecated compatibility aliases (
CURRENT_PLAN_PROTOCOL,createPlanV2FromV1,publishPlanV2FromV1) retained and tested inpublicExports.test.ts
4. Byte-identical v2 manifests — VERIFIED
The test "keeps legacy conversion names as byte-identical compatibility aliases" creates a manifest via both the new createPlanV2FromExecutionPlan and the old createPlanV2FromV1 and asserts readFileSync(canonical.manifestPath) equals readFileSync(compatibility.manifestPath). Manifest key order is pinned by an exact Object.keys() assertion. Hash framing unchanged — canonicalJsonStringify serialization path is untouched.
5. Size cap handling — CLEAN
buildLocalExecutionPlan accepts options.executionPlanSizeLimitBytes with a clean precedence chain: options.executionPlanSizeLimitBytes ?? config.planDirSizeLimitBytes ?? PLAN_DIR_SIZE_LIMIT_BYTES. The v2 path passes Number.MAX_SAFE_INTEGER (disabling the transport cap), replacing the previous pattern of mutating a spread config ({ ...config, planDirSizeLimitBytes: … }). Cleaner.
6. Test improvements
Several tests upgraded from toMatchObject (partial) to toEqual (exact), strictly improving shape regression coverage. The SFN/workflow input assertions now catch accidental new fields that partial matching would miss.
Minor observations (non-blocking)
-
sourcePlanV1Hashwire name: The field name still says "V1" in the manifest and result types even though the concept is now transport-neutral. Correctly handled with thegetPlanV2ExecutionPlanHash()accessor and@deprecatedannotations on the raw field — renaming a wire field would break deployed workers, so the accessor is the right approach. -
publicExports.test.tsgap: The test checkstypeoffor new exports but does not assert thatbuildLocalExecutionPlanis NOT exported (i.e. no negative guard on the internal function appearing in the public surface). Low risk since the exact key-set tests on result objects would catch downstream impact, but a one-linerexpect(distributedSubpath.buildLocalExecutionPlan).toBeUndefined()would close it.
Verdict
Approve. The refactor cleanly extracts a shared execution-plan builder from the v1 transport wrapper while preserving every wire-format key, value, and ordering. Test coverage on the critical compatibility shapes is comprehensive and strictly improved over the pre-PR baseline. All required checks green including the focused 200/200 compatibility suite, Windows, and all nine regression shards.
— Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head 9f3e9ce. The shared local execution-plan builder remains internal; v1 descriptor identity and serialized result shapes are preserved; v2 canonical/compatibility publication is byte-identical; cloud adapters consume the shared planner; and the latest exact-head check run is terminal green/skipped. Approved.
Merge activity
|
## What Refactor distributed planning around one shared local execution-plan builder: - `buildLocalExecutionPlan()` now owns compile/probe/extract/audio/freeze. - Legacy `plan()` remains a deprecated v1 transport wrapper. - Plan v2 calls the shared builder directly and publishes through the existing manifest/CAS contract. - Add neutral `createPlanV2FromExecutionPlan()`, `publishPlanV2FromExecutionPlan()`, `getPlanV2ExecutionPlanHash()`, and `PLAN_PROTOCOL_V1` names. - Retain deprecated v1-named exports and wire aliases. - Recommend explicit Plan v2 opt-in for new producer, Lambda, and Cloud Run integrations. ## Why Plan v2 previously looked like it invoked a v1 planner even though v1 and v2 share the same frozen local execution representation. This removes that migration-era coupling while preserving the public minor-version compatibility contract. ## How The shared builder returns neutral internal execution-plan fields. The v1 wrapper maps those fields back to the existing `PlanResult`; the v2 publisher consumes them directly. Compatibility is intentional and covered by exact shape tests: - omitted `planProtocol` still serializes/selects `"v1"`; - v1 layouts, descriptor-less decoding, event unions, workflow branches, and exports remain; - the v1 descriptor JSON is byte-identical and `CURRENT_PLAN_PROTOCOL` is an identity-preserving alias; - v2 manifest bytes, key order, hash framing, and `sourcePlanV1Hash` wire key remain unchanged; - no enumerable neutral hash field was added to manifests or returned result objects; - v1/v2 result objects, cloud event payloads, and SDK handle key sets remain unchanged. ## Test plan - Focused Plan v1/v2/protocol/export/size compatibility: 141 passed - `@hyperframes/core`: 1,419 passed - `@hyperframes/producer` unit lane: 990 passed - `@hyperframes/aws-lambda`: 140 passed - `@hyperframes/gcp-cloud-run`: 101 passed - Producer, Lambda, and Cloud Run typechecks - Repository-wide lint, format check, workspace/package-subpath checks - Full workspace build - `git diff --check` - [x] Unit tests added/updated - [ ] Manual testing performed - [x] Documentation updated (if applicable)
4 participants
What
Refactor distributed planning around one shared local execution-plan builder:
buildLocalExecutionPlan()now owns compile/probe/extract/audio/freeze.plan()remains a deprecated v1 transport wrapper.createPlanV2FromExecutionPlan(),publishPlanV2FromExecutionPlan(),getPlanV2ExecutionPlanHash(), andPLAN_PROTOCOL_V1names.Why
Plan v2 previously looked like it invoked a v1 planner even though v1 and v2 share the same frozen local execution representation. This removes that migration-era coupling while preserving the public minor-version compatibility contract.
How
The shared builder returns neutral internal execution-plan fields. The v1 wrapper maps those fields back to the existing
PlanResult; the v2 publisher consumes them directly.Compatibility is intentional and covered by exact shape tests:
planProtocolstill serializes/selects"v1";CURRENT_PLAN_PROTOCOLis an identity-preserving alias;sourcePlanV1Hashwire key remain unchanged;Test plan
Focused Plan v1/v2/protocol/export/size compatibility: 141 passed
@hyperframes/core: 1,419 passed@hyperframes/producerunit lane: 990 passed@hyperframes/aws-lambda: 140 passed@hyperframes/gcp-cloud-run: 101 passedProducer, Lambda, and Cloud Run typechecks
Repository-wide lint, format check, workspace/package-subpath checks
Full workspace build
git diff --checkUnit tests added/updated
Manual testing performed
Documentation updated (if applicable)