Skip to content

fix(encode): publish the output only once the render succeeds - #234

Merged
LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/encode-atomic-output
Sep 22, 2026
Merged

LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/encode-atomic-output

Conversation

@LeadcodeDev

Copy link
Copy Markdown
Owner

Severity Medium, category correctness. Location: crates/rustmotion/src/encode/video/ffmpeg.rs:620

Impact

When render_frame_task fails mid-encode (line 613 sets pipe_error and breaks), the code drops stdin and then waits for ffmpeg. ffmpeg sees a clean EOF on pipe:0, finalizes the frames it already received, writes the moov atom and exits 0. rustmotion then returns Err, but output_path now holds a structurally valid MP4 containing only the first N frames. Because ffmpeg_args emits -y (line 194), this also silently destroys a previously-good render at the same path. A user scripting rustmotion render who checks only file existence (or whose CI publishes the artifact) ships a truncated video. No code path anywhere in src/cli or src/encode removes the output on failure — only test code calls remove_file on outputs.

Fix

On the pipe_error path, call child.kill() and child.wait() before returning, and let _ = std::fs::remove_file(output_path); on both the pipe_error and !status.success() branches. Better still, have ffmpeg write to a sibling scratch path and fs::rename onto output_path only after a clean exit — the same promote-on-success discipline video_audio.rs::partial_wav_path already applies to cached WAVs.

Evidence the audit read

drop(stdin);
...
    let status = child.wait().map_err(|e| RustmotionError::FfmpegWait {
        reason: e.to_string(),
    })?;
...
    if let Some(e) = pipe_error {
        tee_stderr();

Based directly on the chantier branch.

Part of the September 2026 audit remediation chantier. Refs #220 (RM-17).

@LeadcodeDev LeadcodeDev added the bug Something isn't working label Sep 21, 2026
@LeadcodeDev LeadcodeDev self-assigned this Sep 21, 2026
@LeadcodeDev
LeadcodeDev force-pushed the fix/encode-atomic-output branch from 8ed0118 to b6af723 Compare September 21, 2026 23:39
@LeadcodeDev
LeadcodeDev force-pushed the fix/encode-atomic-output branch 2 times, most recently from e9266d1 to cec3505 Compare September 22, 2026 08:34
When render_frame_task fails mid-encode (line 613 sets pipe_error and
breaks), the code drops stdin and then *waits* for ffmpeg. ffmpeg sees a
clean EOF on pipe:0, finalizes the frames it already received, writes the
moov atom and exits 0. rustmotion then returns Err, but output_path now
holds a structurally valid MP4 containing only the first N frames. Because
ffmpeg_args emits -y (line 194), this also silently destroys a previously-
good render at the same path. A user scripting rustmotion render who checks
only file existence (or whose CI publishes the artifact) ships a truncated
video. No code path anywhere in src/cli or src/encode removes the output on
failure — only test code calls remove_file on outputs.

Fix: On the pipe_error path, call child.kill() and child.wait() before
returning, and let _ = std::fs::remove_file(output_path); on both the
pipe_error and !status.success() branches. Better still, have ffmpeg write
to a sibling scratch path and fs::rename onto output_path only after a clean
exit — the same promote-on-success discipline
video_audio.rs::partial_wav_path already applies to cached WAVs.

Refs #220
@LeadcodeDev
LeadcodeDev force-pushed the fix/encode-atomic-output branch from cec3505 to ca0729d Compare September 22, 2026 08:43
@LeadcodeDev
LeadcodeDev merged commit e34b402 into chantier/audit-2026-09 Sep 22, 2026
3 checks passed
@LeadcodeDev
LeadcodeDev deleted the fix/encode-atomic-output branch September 22, 2026 08:52
LeadcodeDev added a commit that referenced this pull request Sep 22, 2026
When render_frame_task fails mid-encode (line 613 sets pipe_error and
breaks), the code drops stdin and then *waits* for ffmpeg. ffmpeg sees a
clean EOF on pipe:0, finalizes the frames it already received, writes the
moov atom and exits 0. rustmotion then returns Err, but output_path now
holds a structurally valid MP4 containing only the first N frames. Because
ffmpeg_args emits -y (line 194), this also silently destroys a previously-
good render at the same path. A user scripting rustmotion render who checks
only file existence (or whose CI publishes the artifact) ships a truncated
video. No code path anywhere in src/cli or src/encode removes the output on
failure — only test code calls remove_file on outputs.

Fix: On the pipe_error path, call child.kill() and child.wait() before
returning, and let _ = std::fs::remove_file(output_path); on both the
pipe_error and !status.success() branches. Better still, have ffmpeg write
to a sibling scratch path and fs::rename onto output_path only after a clean
exit — the same promote-on-success discipline
video_audio.rs::partial_wav_path already applies to cached WAVs.

Refs #220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant