fix(render): keep verification fallback storage-aware - #3703
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Storage-aware verification fallback is correct — inspectDiskCaptureHeadroom is a clean refactor of the assert path, and the streaming fallback when disk lacks headroom is well tested. — Miga
miga-heygen
left a comment
There was a problem hiding this comment.
Second-pass quality review at head c1675652 (isolated worktree; 16/16 touched tests and 196/196 renderOrchestrator.test.ts reproduced, tsc --noEmit clean). One finding I consider blocking.
The storage-aware escape is only honoured on one of the two routing kinds it is computed for. packages/producer/src/services/renderOrchestrator.ts:3639-3644 computes verificationDiskFallbackAvailable for every non-default routing whose fallback is sdr_disk — that is both worker_inversion and parallel_router — and passes it into replanAfterFailure. But packages/producer/src/services/render/capturePlan.ts:153-157 only turns it into diskFallbackUnavailable when plan.routing.kind === "worker_inversion". On the default-on parallel_router path the statfsSync inspection runs and its result is discarded, so the exact abort this PR fixes is still reachable there: verification fails → resolveParallelRouterRetryPlan yields workerCount > 1 → shouldUseStreamingEncode is false → the plan reverts to sdr_disk → estimateDiskCaptureBytes (worker-count-independent) exceeds the 90% gate → assertDiskCaptureHeadroom throws. Reproduced with a scratch test: parallel_router + diskFallbackAvailable: false still yields sdr_disk at 5 workers. Structurally, the same predicate now lives in two files with two different routing-kind preconditions that already disagree; whichever is intended, one site should own it — either drop the worker_inversion restriction in capturePlan.ts, or narrow the orchestrator so the flag is only computed where it is read, and add a parallel_router case to the new tests.
Non-blocking: captureStage.ts:186 || headroom.freeBytes === null is pure type narrowing (a discriminated union return would remove it); diskFallbackAvailable bolted onto CapturePlanFailure as an optional tri-state; the escape does not verify memoryExhaustionFallback is itself off-disk (reachable by type, not by current config); two of the new headroom tests overlap and assert implementation shape; and the orchestrator glue expression where the drift lives has no test.
Positives verified: planner and executor share identical inputs (framesDir / totalFrames / buildCaptureOptions); no new silent failures; all 16 deleted lines are non-behavioural; every numeric claim in the description (41.08 / 45.64 GiB, 16 tests, typecheck) holds. Mutations: revert the storage-aware branch → 1 red; drop the 90% gate → 3 red.
Verdict: COMMENT — needs fixes before merge.
Reasoning: the fix is correct where it is wired, but the default routing path computes the flag and ignores it, leaving the reported abort live there.
Review by Miga
|
Addressed in f715280. |
…ting kind The disk-headroom flag was computed by the orchestrator for both worker_inversion and parallel_router routings but only honoured by replanAfterFailure under worker_inversion, so the default-on parallel_router path still reverted to an sdr_disk plan that the 90% gate would reject and assertDiskCaptureHeadroom aborted the render. Give the predicate a single owner: drawElementVerificationFailure decides when headroom can change the fallback (non-default routing, sdr_disk preferred fallback, off-disk memory-exhaustion fallback) and consults the shared inspector only then. replanAfterFailure now honours the resulting flag for any routing kind, and the orchestrator merges the two revert log branches keyed on that flag instead of on plan kind, which also stops OOM retries from being logged as headroom diversions. inspectDiskCaptureHeadroom returns a discriminated union so the assert no longer needs a redundant null guard to narrow freeBytes. Tests: parallel_router headroom diversion and restoration, the failure constructor across default / streaming-only / disk-only routings, and an exact-once statfs count. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
…nspector Extract the streaming drain's failure construction into `streamingCaptureFailure` so the retry classification is unit-testable, and assert that a drawElement capture failure or OOM on a routed plan never calls the disk headroom inspector — only a self-verification failure may, via `drawElementVerificationFailure`. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
f715280 to
ac71fbc
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict — APPROVE
Head verified: ac71fbcb16dc29dfcdd198026a340b0eea524348. Small, focused change (5 files, +346/-32) that surgically fixes the abort-on-headroom-shortage case without touching filesystem on paths that don't need it.
The three axes I was asked to verify
1. Disk-space check runs on all non-default retry routes uniformly. drawElementVerificationFailure gates disk inspection on a three-condition AND: routing.kind !== "default" AND routing.fallback.kind === "sdr_disk" AND routing.memoryExhaustionFallback.kind === "sdr_streaming". Both non-default routings (worker_inversion and parallel_router) that satisfy the fallback shape consult the same inspector — same lambda, same shared 90% decision as assertDiskCaptureHeadroom (both go through inspectDiskCaptureHeadroom). Test "consults disk headroom for every routing whose disk fallback has an off-disk escape" iterates ["worker_inversion", "parallel_router"] explicitly and asserts one call per routing×availability crossing. Default routing doesn't need it — its fallback is already streaming.
2. "Don't abort on an estimate it was never going to use" — the estimate is scoped correctly. The inspection is narrowly gated:
streamingCaptureFailureonly routes verification failures through the headroom-aware builder;capture_failure(both OOM and non-OOM) bypasses entirely. Test"never inspects disk headroom for a non-verification failure"uses a throw-on-call inspector to prove this.- Within verification failures, the three-AND gate above means default-routing plans, non-disk-preferred plans, and disk-with-disk-escape plans all leave
diskFallbackAvailableundefinedwithout invokinghasDiskFallbackHeadroom(). Test"skips the disk inspection when headroom cannot change the fallback"uses a throw-on-call inspector across three plan shapes (default, non-disk-preferred, disk+disk-escape) to prove no filesystem touch. - When
diskFallbackAvailable=false,replanAfterFailuresteers tomemoryExhaustionFallback(the routing's precomputed low-resource target) instead of the preferred disk fallback. So the estimate is only computed where the recovery path would have used that space, and it only redirects when the alternate is genuinely off-disk.
3. Rebase didn't drift logic. replanAfterFailure retains its full old behavior for callers passing {kind: "draw_element_verification"} without the new field (diskFallbackAvailable === undefined doesn't trigger the diskFallbackUnavailable branch → falls through to preferred fallback, unchanged). The new steering activates only when the caller explicitly passes false. Grep-verified across the whole hyperframes repo: replanAfterFailure and draw_element_verification appear ONLY in the three touched files. Single caller (renderOrchestrator.ts) is now updated to go through streamingCaptureFailure, so there's no legacy path silently getting the old blind-fallback behavior. The assertDiskCaptureHeadroom refactor is a pure extraction — same 90% threshold, same null-free-bytes-counts-as-available semantics, same error message shape (estimatedBytes and freeBytes now via extracted variable but byte-identical output).
Extra correctness notes
inspectDiskCaptureHeadroomreturn type discriminates the null-free-bytes case correctly:available: trueallowsfreeBytes: number | null,available: falserequiresfreeBytes: number— you can't refuse without a measurement.- The
renderOrchestrator.tsrevert log consolidation: unifiedfailedRouting !== "default"gate, branch ondiskFallbackLacksHeadroom = failure.kind === "draw_element_verification" && failure.diskFallbackAvailable === false. Keyed on the failure flag, not the resulting plan kind — so an OOM that lands in streaming isn't misreported as a headroom diversion. PR body's claim holds. buildCaptureOptions()inside the lambda is only invoked whenneedsHeadroomis true (deferred bydrawElementVerificationFailure), and the test pinsinspections === 1— no accidental extra invocations.
CI
24 checks visible: 5 pass, 3 pending, 1 skipping — zero failures. mergeStateStatus=blocked waiting on this approval (last-push-approval requirement). Prior miga-heygen bot APPROVE at c1675652 is stale by design — fresh approval per Miga's own stale-rebase-diff discipline.
Stamp.
— tai
miga-heygen
left a comment
There was a problem hiding this comment.
Approving at ac71fbcb, superseding my approval at c1675652. The routing gap raised in review 5189476710 is closed: drawElementVerificationFailure in capturePlan.ts:88-101 is the single owner of the disk-fallback decision and replanAfterFailure honours it for every non-default routing (:218); a scratch test at the reported production shape (parallel_router, verification failure, no headroom) lands on sdr_streaming, and three mutations restoring either form of the bug go red. 243/243 producer tests, tsc --noEmit clean. Non-blocking: the === false guard at capturePlan.ts:206 is unpinned (mutating to !== true stays green though undefined is reachable from renderOrchestrator.ts:3489-3516), and renderOrchestrator.ts:3825-3827 repeats that predicate for log text only. — Miga
What
A drawElement verification failure on a routed streaming capture (worker inversion or parallel router) now keeps a viable low-worker screenshot stream when the preferred parallel-disk fallback lacks storage headroom. Disk fallback remains preferred whenever it is feasible.
Why
The verification retry restored the pre-routing disk plan before considering storage. For the reported 5,318-frame 1080p render, that plan estimates 41.08 GiB of temporary frames and needs about 45.64 GiB free under the existing 90% gate, so recovery aborted despite an already-supported low-resource streaming route. The abort was reachable on both routing kinds, including the default-on parallel router.
How
inspectDiskCaptureHeadroomexposes the disk stage's existing 90% headroom decision as a discriminated union shared by fallback planning and disk execution.drawElementVerificationFailureis the single owner of when that headroom matters: a non-default routing whose preferred fallback issdr_diskand whose precomputed memory-exhaustion fallback is off-disk. Only then is free space inspected; every other plan leaves the flag undefined without touching the filesystem.replanAfterFailurehonours the resulting flag for any routing kind, taking the routing's precomputed low-resource target when the disk route is infeasible.Feasible and unknown headroom, default routing, OOM handling, other pinned-path capture failures, and disk-path verification keep their existing transitions.
This does not classify the reported 13 KB final frame. Determining whether that frame is visually wrong still requires the authored endpoint and a same-time screenshot/buffer comparison.
Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (not applicable)
capturePlan.test.ts: headroom diversion and restoration for bothworker_inversionandparallel_router;drawElementVerificationFailureconsults disk exactly once for routings with an off-disk escape and never for default, streaming-only, or disk-only routingscaptureStage.test.ts: shared 90% decision, unknown free space treated as available, reported 5,318-frame landscape case rejected at 45 GiB freerenderOrchestrator.test.ts: 196 tests passingProducer typecheck, changed-file oxlint and oxfmt passing