Skip to content

fix(render): stream long low-memory captures#2245

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/low-memory-long-streaming
Jul 13, 2026
Merged

fix(render): stream long low-memory captures#2245
miguel-heygen merged 1 commit into
mainfrom
fix/low-memory-long-streaming

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

Keep single-worker video renders on streaming encode in low-memory mode even when their duration exceeds the normal streaming cap.

Why

A 411-second render on an 8GB Windows host silently switched to disk capture because of the 240-second cap, then Chrome OOM-crashed at 56%. Raising the cap and bounding the frame data-URI cache completed the render. Low-memory mode should prefer bounded streaming memory over accumulating the full capture.

How

Teach shouldUseStreamingEncode to bypass only the duration clamp when lowMemoryMode is active. Format, validity, enablement, and worker-count gates remain unchanged.

Test plan

  • bunx vitest run packages/producer/src/services/renderOrchestrator.test.ts -t shouldUseStreamingEncode
  • bun run --filter @hyperframes/producer typecheck
  • pre-commit lint, format, tracked-artifact, fallow, and typecheck hooks

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 5b9d3cac.

Correct fix, well-scoped. The trade-off it encodes is exactly the right one: on a memory-constrained host, bounded streaming memory beats parallel disk's wall-clock win — the disk path was silently OOM-crashing at 56% before this. Traced the invariant Miguel relies on ("low-memory mode already pins capture to one worker") through captureCost.ts:136if (cfg.lowMemoryMode && requestedWorkers === undefined) clamps workers to 1, so the bypass never conjures multi-worker streaming on a low-memory host. And even if a user passes --workers 3 explicitly on a low-memory host, the workerCount === 1 check at the tail of the function still gates them into disk — the bypass only removes the duration cap, not the other filters.

Grep of all shouldUseStreamingEncode( callers (renderOrchestrator.ts:1091,1189,1902,2126 — 4 sites) confirms every one passes full EngineConfig-shaped cfg, so lowMemoryMode flows through unset-arguments-free. The Partial<Pick<..., "lowMemoryMode">> type-level optionality is defensive against future callers that construct a subset without the flag; those get the old behavior (undefined → falsy → cap applies). Backward-safe.

Not addressed and not in scope: the observability log at renderOrchestrator.ts:2135 emits maxDurationSeconds: cfg.streamingEncodeMaxDurationSeconds alongside enabled: useStreamingEncode. Post-fix, a low-memory 411s render will log enabled=true, durationSeconds=411, maxDurationSeconds=240 — the log line will look internally inconsistent to a reader trying to reason about why streaming stayed on. Nothing broken, just slightly misleading; could add an overrideReason: "lowMemoryMode" field or similar in a follow-up if this trips up debugging.

LGTM.

Review by Rames D Jusso

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on Rames-D's go (R2 delta clean, nit round addressed) + Magi's CI-green confirmation. Verified at final head 5b9d3cac: CI green, no changes-requested, mergeable.

@miguel-heygen miguel-heygen merged commit 9c0c1f9 into main Jul 13, 2026
48 checks passed
@miguel-heygen miguel-heygen deleted the fix/low-memory-long-streaming branch July 13, 2026 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants