Repository navigation
Conversation
fe47190 to
1f61338
Compare
…eaming when headroom is insufficient When the pre-capture disk-headroom gate rejects a render, the render failed outright even when single-worker streaming was eligible: auto-parallel renders always take the disk path, and the gate threw inside runCaptureStage instead of feeding a replan. A machine with modest free disk therefore failed renders that single-worker streaming could finish. Two pieces, split along the existing seams: - capturePlan.ts grows a 'disk_headroom' CapturePlanFailure and the matching replanAfterFailure transition: sdr_disk -> createCapturePlan's fully hardened single-worker streaming shape, keeping the original forceScreenshot and reverting any active routing so telemetry records the lost bet. disk_headroom on any other plan kind throws instead of falling through to the generic retry tail, which would silently replan a streaming plan. - renderOrchestrator.ts grows the policy predicate shouldDegradeDiskCaptureToStreaming (multi-worker sdr_disk + no headroom + single-worker streaming eligible, reusing shouldUseStreamingEncode so png-sequence/gif, the operator duration cap, and enableStreamingEncode=false keep the gate error) and hooks it right after the capture_strategy checkpoint, before capture runs. Also corrects the streaming-gate doc comment, which still described disk frames as raw RGBA after the estimate became format-keyed. Refs heygen-com#4060.
1f61338 to
169a158
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving. #4060 is still open and still reproduces in shape on main: a multi-worker mp4 render captures to disk, and if the up-front headroom estimate does not fit, the render fails even when a one-worker run would stream with no disk at all. Main's answer today (#5934) is an error message telling the user to re-run with --workers 1. This PR makes that same choice automatically, before capture starts.
What I checked:
- The predicate reuses
shouldUseStreamingEncode(cfg, format, 1, duration)for eligibility, so png-sequence, gif, the duration cap andPRODUCER_ENABLE_STREAMING_ENCODE=falsestill get the gate error. Single-worker disk plans are left alone. That is the same conditiondiskCaptureRemedyuses to decide whether to suggest--workers 1, so the two agree. - The replan goes through
replanAfterFailureandsyncCapturePlan, so motion blur and--resolutionroute checks still run on the new plan (both are supported onsdr_streaming). Active routing is reverted, the input plan is not mutated, and adisk_headroomfailure on any other plan kind throws instead of falling into the generic tail. framesDiralready exists at that point (created near the top of the pipeline), so the statfs reads a real directory.- I merged this head onto current main (no conflicts) and ran
capturePlan.test.tsplusrenderOrchestrator.test.ts: 316 passed.tsc --noEmitfor producer is clean.
Two non-blocking notes:
- The new tests cover the predicate and the replan, but nothing drives the orchestrator hook itself. A small test that stubs free disk low on a multi-worker mp4 render and asserts the
disk_headroom_degradedcheckpoint would pin the wiring. - With this in, the
--workers 1branch ofdiskCaptureRemedyis only reachable from the measured mid-capture projection, not the up-front gate. That is fine, just worth knowing when reading that message later.
Need: yes, renders that one worker could finish still fail on modest disks · Size: ok · Reuse: built on the existing replanAfterFailure, inspectDiskCaptureHeadroom and shouldUseStreamingEncode seams · Simpler: already minimal
— Rames
|
Thanks, @CasbaL. This one is approved, and it'll go into the merge queue once these are sorted:
Once it's green and signed, we'll queue it at your new head. If you push code changes beyond a rebase, we'll take another look first. — Rames |
|
This is approved at the current head, but main requires signed commits, so GitHub won't merge it yet. Could you re-sign the commits ( — Rames |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Checked at 169a158. The disk-headroom replan routes through replanAfterFailure in capturePlan.ts, the one place every capture-plan fallback already goes through, and renderOrchestrator.ts decides the degrade with shouldDegradeDiskCaptureToStreaming. PNG and alpha outputs still refuse, as before.
I ran capturePlan.test.ts and renderOrchestrator.test.ts: 317 passed. With capturePlan.ts and renderOrchestrator.ts put back to the merge base, the new cases fail (8 failures, for example "degrades a multi-worker disk plan when disk lacks headroom and streaming is eligible"), so the tests guard the change.
Verdict: APPROVE
Reasoning: fixes the remaining half of #4060 at the planner that owns fallbacks, tests fail without it, CI green.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
One change before this can merge: the JSDoc on shouldDegradeDiskCaptureToStreaming (renderOrchestrator.ts:1577-1584) is 8 lines, and the repository merges comments of at most 3 to 4 lines that say what the code cannot. The function's name and its four arguments already say most of it. Something like this keeps the one non-obvious point:
/** Single-worker disk plans stay on disk: they are there because streaming is not eligible. */The rest of the reasoning reads well in the PR description. Everything else is ready: the tests pass and fail without the change, and CI is green. Please push that and sign the commit; we will merge it once it is green.
…o comment convention Signed-off-by: CasbaL <[email protected]>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 0b5f9e7782d2ef0f1635b05e6f40b985bd060430.
The last source delta trims the JSDoc exactly as Miguel requested. The fallback still reuses the existing planner and only degrades a multi-worker disk plan when one-worker streaming is eligible.
Compared current source to reviewed planner and orchestration contracts, no render execution. Earlier full 317-test result is Miguel-attributed at 169a158. Two unsigned commits remain. Fresh independent fallback source pass also found no blocker; no new CI claim.
Need: yes · Size: ok · Reuse: existing implementation seam · Simpler: already minimal · Structure: fine
Risk: medium - capture planner fallback and disk headroom.
Verdict: APPROVE
Reasoning: The last source delta trims the JSDoc exactly as Miguel requested. The fallback still reuses the existing planner and only degrades a multi-worker disk plan when one-worker streaming is eligible.
Reasoning: The last source delta trims the JSDoc exactly as Miguel requested. The fallback still reuses the existing planner and only degrades a multi-worker disk plan when one-worker streaming is eligible.
Summary
Fixes #4060. Successor to #4061, which was closed as stale and can no longer be reopened (GitHub blocks reopening a PR whose head branch was force-pushed while closed) — same branch name, rebased onto v0.8.143.
Of the original two-part fix, upstream has since absorbed the estimate half: JPEG now bills at
raw/8and a measured projection replaces the static estimate once ten frames exist. What remains valuable — and what this PR is — is the degrade.Problem. When the pre-capture disk-headroom gate rejects a render, the render fails outright even when single-worker streaming was eligible: auto-parallel renders always take the disk path, and the gate throws inside
runCaptureStageinstead of feeding a replan. A machine with modest free disk therefore failed renders that single-worker streaming could finish.What changed (two pieces, split along main's existing seams):
capturePlan.ts— adisk_headroomCapturePlanFailureand the matchingreplanAfterFailuretransition:sdr_disk→ the fully hardened single-worker streaming plan, keeping the originalforceScreenshotand reverting any active routing so telemetry records the lost bet. On any other plan kind it throws instead of falling through to the generic retry tail, which would silently replan a streaming plan.renderOrchestrator.ts— the policy predicateshouldDegradeDiskCaptureToStreaming(multi-workersdr_disk+ no headroom +shouldUseStreamingEncode(cfg, format, 1, duration)eligible — so png-sequence/gif, the operator duration cap, andenableStreamingEncode=falsekeep the gate error), hooked right after thecapture_strategycheckpoint and before capture runs. The degrade emits its owndisk_headroom_degradedcheckpoint, so degraded renders are countable rather than indistinguishable.Intentionally unchanged: the estimate itself. Upstream's policy of billing PNG at full raw RGBA is kept — png-sequence/gif are streaming-ineligible, so for those renders the pre-capture refusal is the only safe outcome on a tight disk, and this degrade cannot help them by design. (This also retires the
frameExtblocker from #4061: the new diff never touchesframeExt.)Also corrects the streaming-gate doc comment, which still described disk frames as raw RGBA after the estimate became format-keyed (follow-up 2 from #4061).
Test plan
capturePlan.test.ts: degrade transition (multi-worker disk → hardened single-worker streaming, input plan never mutated), active-routing revert records the lost bet with frozen routing, anddisk_headroomon a streaming plan throws.renderOrchestrator.test.ts: newshouldDegradeDiskCaptureToStreamingdecision table (5 cases — multi-worker disk + no headroom + streaming-eligible degrades; sufficient headroom / streaming or layered plans / ineligible streaming / single-worker plans do not).capturePlan/captureStage/renderOrchestratorsuites pass (333 passed; the 2 failingresolveDeviceScaleFactorcases fail identically on clean v0.8.143 — pre-existing and unrelated).tsc --noEmitclean for@hyperframes/producer.