fix(render): scale timeout for long video encodes#2244
Conversation
|
@miguel-heygen is this for streaming encoding or no? streaming encoding in the past has a hard deadline of 600 seconds I think at least that's what we've seen in production and therefore limit streaming encoding to smaller videos and use serial based encoding in production for longer videos to avoid the timeout |
@jrusso1020 this type of PRs are being created by Magi based on the cli feedback |
|
@jrusso1020 This is for the separate post-capture disk-frame encode path, not streaming encode. Streaming encoding still has its existing safeguards unchanged: Both reports behind this PR were longer than that gate (353s and 754.8s), so they correctly fell back to disk capture and then hit the separate fixed So the production policy you described remains intact: long videos do not use streaming. This PR only gives their standard disk/chunked encode enough time after capture. I'll also make the PR description explicit that streaming selection and the 240s gate are unchanged. |
terencecho
left a comment
There was a problem hiding this comment.
LGTM.
Cross-checks
- Units consistency.
job.duration(seconds; test uses 754.8) ×4_000→ ms;ffmpegEncodeTimeoutis ms (600_000 = 600s baseline). Consistent. Test expectation3_019_200=Math.ceil(754.8 * 4000)✓. - Edge cases via
??+Math.ceil.undefined → 0 → falls back to configured;NaN → n>configured === false → falls back;negative → < configured → falls back. All safe by construction; no explicit guard needed. - Operator override preserved. The
Math.max(configured, scaled)shape means an operator override that's already larger than the scaled value wins — matches the PR's "preserve larger operator overrides" property. - GIF path unaffected.
encodeFramesToGifstill usesengineCfgunchanged; scaling only applies to the two video branches (matches PR body claim). - CI green — all required checks pass including Windows render + regression shards.
Nits (non-blocking)
- One point on the curve in the new test (long-video path); a
duration=0/undefinedcase that asserts falling back to configured default would be worth adding, though the existing "prefers engine config" test likely covers the short-duration default viamakeInput(). - No upper clamp: a 3600s composition yields a 14400s (4h) budget. Fine given the wall-clock is a still-progressing cap, but worth flagging if any host has its own hard limit.
- Magic constant
4_000is only documented in the adjacent comment.ENCODE_BUDGET_MULTIPLIER_MS_PER_SECwould age better; not worth churning.
Diff-scope: +30/-2, 2 files, matches title.
— Review by tai (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: 🟢 LGTM at 42a3df2
Concurring with @terencecho on units consistency + edge cases (undefined → 0 → falls back, NaN → falls back, negative → falls back) + operator-override preservation via Math.max(configured, scaled). Also endorsing his scope confirmation from Miguel's comment thread: streaming encoding still gates on shouldUseStreamingEncode and only accepts compositions ≤ streamingEncodeMaxDurationSeconds, so both reproducing cases (353s and 754.8s) fell to disk capture where this new timeout applies. GIF branch at encodeStage.ts:268 still uses the raw engineCfg.ffmpegEncodeTimeout, unchanged.
One additional risk beyond Terence's coverage: no upper cap on scaledEncodeTimeout. Math.ceil((job.duration ?? 0) * 4000) — a pathological job.duration (e.g. a corrupted 100000s composition) produces a ~4.6-day timeout, holding a worker for days. Realistically bounded by upstream producer contracts + duration validation, so low real-world risk, but a Math.min(scaled, MAX_ENCODE_TIMEOUT_MS) would be belt-and-suspenders against the pathology. Follow-up worth filing.
Second observation: "4× duration" is a "longer than what we saw yesterday" number, not a p99-derived one. Fine as a CLI-unblock; a follow-up to characterize the actual encode-time-vs-duration curve would let this be tighter.
R1 by Via
|
What
Scale the video encode timeout to at least four times the composition duration while preserving larger operator overrides.
Why
The existing 600-second total wall-clock timeout kills long disk-frame encodes that are still making progress. Two Windows reports reproduced this on 353-second and 755-second compositions; both completed after manually increasing
FFMPEG_ENCODE_TIMEOUT_MS.How
Derive a video-only engine config in the encode stage with
max(configured timeout, duration × 4 seconds). GIF behavior and short-render config identity remain unchanged.Test plan
bun test packages/producer/src/services/render/stages/encodeStage.test.ts --timeout 30000bun run --filter @hyperframes/producer typecheck