refactor(producer): make capture plans immutable#2163
Conversation
a955dbb to
a7d8a90
Compare
e249db4 to
7e67f17
Compare
a7d8a90 to
46e7941
Compare
7e67f17 to
1bf2e0c
Compare
46e7941 to
eb1b74b
Compare
1bf2e0c to
864beed
Compare
eb1b74b to
711fcef
Compare
864beed to
4dd9950
Compare
711fcef to
a3546c2
Compare
25bffba to
406554a
Compare
a3546c2 to
25a96c1
Compare
406554a to
fab40bd
Compare
25a96c1 to
cd77f50
Compare
fab40bd to
a4a115e
Compare
cd77f50 to
1c04cc5
Compare
a4a115e to
8134c99
Compare
1c04cc5 to
4b7d2bf
Compare
f66d284 to
7d48c5b
Compare
69734b3 to
e08d6df
Compare
7d48c5b to
2db2ea7
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
The immutable-plan shape is strong: capturePlan.ts:74-115 freezes both the plan and nested routing target, and the stage-specific unions make HDR/streaming/disk dispatch invalid states unrepresentable at the call sites. I also verified the distributed chunk constructs the same disk-plan contract. One hostile fallback ordering is not behavior-preserving:
-
[blocker]
packages/producer/src/services/renderOrchestrator.ts:2702-2717/packages/producer/src/services/render/capturePlan.ts:135-147— re-resolve the route after applying the OOM worker reduction. Both routing fallbacks are precomputed withisMemoryExhaustion: false, normally at the pre-routing multi-worker count, so their frozen target issdr_disk. On a later OOM,replanAfterFailure()changes onlyworkerCountto1and retains that disk kind. Exact-head falsifier:worker_inversion: sdr_streaming/W1 --OOM--> sdr_disk/W1 parallel_router: sdr_streaming/W3 --OOM--> sdr_disk/W1That differs from the pre-refactor contract still documented and implemented by
resolveInversionRetryPlan()/resolveParallelRouterRetryPlan()atrenderOrchestrator.ts:1251-1288,1357-1397: for OOM they first reduce to one worker and then callshouldUseStreamingEncode(..., workerCount=1), yielding the single-worker screenshot-streaming recovery route. The new test atcapturePlan.test.ts:90-106currently pins the regressed disk result.This matters precisely on the resource-failure path: the retry now accumulates frames for a separate disk encode instead of using the lighter one-worker streaming route selected after OOM. Store a separate memory-exhaustion target (or otherwise derive kind after the failure-specific worker count is known), and pin both inversion and parallel-router OOM transitions as
sdr_streaming, worker1, screenshot forced. Keep the ordinary non-OOM fallback at the pre-routing worker count/disk kind.
Invariant: for every routing cohort, a fallback transition must preserve the pre-refactor route selected from the actual failure category and resulting worker count; an OOM producing a different capture kind falsifies it. Contract ledger: producers are inversion/router state plus verification/capture/OOM failures; consumers are the streaming retry and disk finalizer; siblings are default, worker-inversion, and parallel-router routes; encoding/owner is the frozen CaptureRouting target plus replanAfterFailure; hostile ordering is OOM after a pinned streaming attempt; relevant history is the old failure-time resolve*RetryPlan calculation. Falsifier: the exact source transition above reproduces disk/W1 for both routed cohorts. Terminal effect: the OOM recovery takes a heavier disk-frame path and can fail or exhaust storage where the prior streaming retry would not.
Audited: complete 8-file base...head diff, every CapturePlan producer/consumer, both retry-policy helpers, stage dispatches, distributed chunk adapter, all tests, prior comments/reviews, exact head/base, and checks. Trusting: the green exact-head CI matrix rather than reinstalling the workspace in this clean review tree. Not exercised: a live Chrome OOM; the pure production transition that selects its terminal route was exercised directly.
— Magi
Verdict: REQUEST CHANGES
Reasoning: Immutability is correctly enforced, but freezing the normal fallback before the failure is known changes both routed OOM recoveries from single-worker streaming to single-worker disk. That resource-path regression needs a failure-specific immutable target before merge.
|
Fixed Magi's blocker on current head
Validated: Producer CapturePlan + renderOrchestrator suites 150/150, CLI cloud-render focused coverage, Producer typecheck, and commit hooks (lint/format/tracked-artifact/Fallow). |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a7d6f7f7d. The prior OOM-routing blocker is resolved.
The invariant is now explicit: each routed plan carries distinct immutable targets for ordinary fallback and memory-exhaustion fallback (capturePlan.ts:18-25,85-91), and replanAfterFailure selects the target from the actual failure class (capturePlan.ts:140-159). The orchestrator is the single producer and computes both targets through the existing inversion/router retry policies (renderOrchestrator.ts:2702-2769). Ordinary fallback therefore retains the pre-routing disk route and worker count; OOM re-resolves to screenshot streaming at one worker.
I verified the contract three ways:
- Read the complete eight-file diff and the three-file blocker-fix diff, then range-diffed the restack: the original two commits are content-identical and the new commit is scoped to the fix.
- Enumerated all
CaptureRoutingproducers/consumers and transition owners repo-wide; every production construction supplies both targets, both nested targets are frozen, andreplanAfterFailureis the sole selector. - Ran a direct production-code falsifier for both
worker_inversionandparallel_router: ordinary failure producedsdr_diskat W5, while OOM producedsdr_streamingat W1 with screenshot capture forced. The regressions pin the ordinary path plus both OOM paths (capturePlan.test.ts:72-140).
I also evaluated the existing HDR worker-count note. Using the routed plan's worker count is a conservative single-source-of-truth choice affecting parallelism, not output correctness, and does not block this refactor.
Audited: current PR lifecycle/history, all prior reviews/comments/inline comments, exact head/base/merge base, complete diff and file surface, sibling routing variants, tests, and exact-head checks. Trusting: the fully green exact-head CI matrix rather than reinstalling and rerunning the entire workspace locally. Not exercised: a live Chrome OOM; the same production transition was exercised directly with both failure classes, which is the relevant deterministic boundary.
— Magi
Verdict: APPROVE
Reasoning: The fix restores failure-specific routing without mutation: normal fallback preserves the disk target, OOM reliably selects the frozen one-worker screenshot-streaming target, all routing producers and sibling variants are covered, and the exact-head CI matrix is green.

What
Represent render routing as immutable CapturePlan variants with pure fallback transitions.
Why
The orchestrator mutated shared routing flags during fallback, making stage behavior order-dependent.
How
Add frozen SDR streaming, SDR disk, and HDR layered plans plus pure replanAfterFailure transitions consumed by stages.
Test plan