fix(engine): name the fix in the ffmpeg encode-timeout error message#1858
Conversation
Two independent post-release feedback reports of hitting
ffmpegEncodeTimeout (600000ms default) on long or high-frame-count
renders, both resolved by setting FFMPEG_ENCODE_TIMEOUT_MS to a higher
value and/or PRODUCER_ENABLE_CHUNKED_ENCODE=true — env vars that already
exist and already solve this, but that neither user found from the error
message itself.
appendEncodeTimeoutMessage only stated what happened ("FFmpeg killed after
exceeding ffmpegEncodeTimeout"), not what to do about it. Name both
existing knobs in the message so the fix is immediately visible at the
point of failure instead of requiring a source dive.
One function, six call sites, all fixed at once. Existing tests assert
with toContain, so the appended text doesn't break them; added two
assertions confirming both env var names appear in the message.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 83909a45 (batch review, Group B CLI/renderer; layering on Magi's own vetting).
Note: bot-authored (Magi via miguel-heygen); COMMENT-quality — stamp routing per protocol.
Summary — Extends the ffmpeg-encode-timeout error message to explicitly name FFMPEG_ENCODE_TIMEOUT_MS and PRODUCER_ENABLE_CHUNKED_ENCODE — the two knobs that already existed and already solve the problem, but that neither reporter found without a source dive.
Message-string change only. No behavior change. One function, applies to all six call sites (chunked + streaming encode paths) by construction. Existing toContain-style assertions in chunkEncoder.test.ts continue to pass; the two new assertions pin the env-var names so a future rewording can't silently drop them.
Nits
🟡 Log-scraper compatibility — any downstream monitoring that greps for FFmpeg killed after exceeding ffmpegEncodeTimeout continues to match (the new text is appended, not replaced). If there's anything grepping for the OLD message's terminating punctuation or line-count, this would shift it, but that's a stretch. No action needed, just a heads-up.
Looks good to me otherwise. Tiny, self-contained, obviously-right actionability improvement.
— Rames D Jusso
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — verified CI green (0 fail / 0 pending) + no open CR at this head. Non-author stamp clearing the review gate on the Magi self-initiated draft-pass batch, which James greenlit and RDJ batch-cleared: both security holds re-verified at R2 (#1866 chrome-shell reclaim-race closed via the reclaim-gate + mtime recheck; #1845 Windows npx shell-injection closed — no cmd.exe, node <npx-cli.js> with pure argv), zero drift on the other nine, all green.
Merge via Magi's normal path (no admin-merge). Ordering note: the skills-manifest triple-conflict on #1877 / #1862 / #1845 (all bump hyperframes-media.hash) needs sequential rebase + regen at merge time; the rest land in any order.
— Rames Jusso
Root-caused from two independent post-release feedback reports of hitting
ffmpegEncodeTimeout(600000ms default) on long or high-frame-count renders — both resolved the same way, but had to find the fix themselves.Root cause
appendEncodeTimeoutMessageonly stated what happened ("FFmpeg killed after exceeding ffmpegEncodeTimeout"). It didn't nameFFMPEG_ENCODE_TIMEOUT_MSorPRODUCER_ENABLE_CHUNKED_ENCODE— both of which already exist and already solve this — so both users had to independently discover them (source dive or docs search) rather than reading the fix off the failure itself.Fix
Name both knobs directly in the message. One function, six call sites (chunked + streaming encode paths), fixed once.
Tests
Existing 81 tests in
chunkEncoder.test.tsassert withtoContain, so the appended text doesn't break them. Added two assertions confirming both env var names appear in the message.