From ca0729d66607213f90ae6a015487356656cb58fa Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 10:43:46 +0200 Subject: [PATCH] fix(encode): publish the output only once the render succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- crates/rustmotion/src/encode/video/ffmpeg.rs | 93 ++++++++- crates/rustmotion/src/encode/video/formats.rs | 180 +++++++++++++++++- crates/rustmotion/tests/audit_ws_c.rs | 117 ++++++++++++ 3 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 crates/rustmotion/tests/audit_ws_c.rs diff --git a/crates/rustmotion/src/encode/video/ffmpeg.rs b/crates/rustmotion/src/encode/video/ffmpeg.rs index b91208c6..63d338ae 100644 --- a/crates/rustmotion/src/encode/video/ffmpeg.rs +++ b/crates/rustmotion/src/encode/video/ffmpeg.rs @@ -290,6 +290,27 @@ fn ffmpeg_args( args } +/// Scratch path ffmpeg actually writes to; promoted (renamed) onto the +/// caller's real `output_path` only after a clean exit with no `pipe_error`. +/// Kept as a sibling of `output_path` (same directory, same filesystem, so +/// the promotion is a plain rename) and keeps `output_path`'s own extension +/// as the *final* extension — mirrors `video_audio::partial_wav_path`'s doc: +/// ffmpeg picks its output muxer from the last extension, so a bare +/// `.partial` suffix appended after it makes ffmpeg refuse to start with +/// "Unable to choose an output format" instead of the encode failure this +/// path exists to isolate. +fn ffmpeg_partial_output_path(output_path: &std::path::Path) -> std::path::PathBuf { + let stem = output_path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match output_path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + output_path.with_file_name(name) +} + /// Encode using FFmpeg subprocess (for h265, vp9, prores, webm, mov, transparency). /// /// Software-only. Kept with its original signature so existing callers @@ -531,6 +552,14 @@ fn encode_with_ffmpeg_hw_impl( } }; + let partial_output_path = ffmpeg_partial_output_path(std::path::Path::new(output_path)); + let partial_output_str = + partial_output_path + .to_str() + .ok_or_else(|| RustmotionError::NonUtf8Path { + path: partial_output_path.to_string_lossy().into_owned(), + })?; + let mut cmd = std::process::Command::new("ffmpeg"); cmd.args(ffmpeg_args( width, @@ -541,7 +570,7 @@ fn encode_with_ffmpeg_hw_impl( transparent, hw_encoder.as_deref(), audio_input.as_deref(), - output_path, + partial_output_str, )); cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::null()); @@ -623,7 +652,18 @@ fn encode_with_ffmpeg_hw_impl( cb(EncodeProgress::Muxing); } - let status = child.wait().map_err(|e| RustmotionError::FfmpegWait { + // A `pipe_error` means the render already failed and `partial_output_path` + // will be discarded either way, so there is nothing left for ffmpeg to + // usefully finish — killing it here instead of waiting for it to + // gracefully encode and finalize a file nobody will ever read avoids + // burning time on a result already known to be thrown away. + let status = if pipe_error.is_some() { + let _ = child.kill(); + child.wait() + } else { + child.wait() + } + .map_err(|e| RustmotionError::FfmpegWait { reason: e.to_string(), })?; @@ -659,6 +699,7 @@ fn encode_with_ffmpeg_hw_impl( if let Some(e) = pipe_error { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); // A broken pipe means ffmpeg is already gone — its own error says why, // ours only says we could not keep writing. Carry both. return Err(match e { @@ -672,11 +713,17 @@ fn encode_with_ffmpeg_hw_impl( if !status.success() { tee_stderr(); + let _ = std::fs::remove_file(&partial_output_path); return Err(RustmotionError::FfmpegFailed { stderr: stderr_summary, }); } + // Only now, with a clean exit and no pipe error, does `output_path` ever + // see this render's bytes — promote-on-success, the same discipline + // `video_audio::extract_audio_to_wav` already applies to its cached WAVs. + std::fs::rename(&partial_output_path, output_path)?; + Ok(()) } @@ -789,7 +836,47 @@ pub fn concat_mp4_segments(inputs: &[std::path::PathBuf], output_path: &str) -> #[cfg(test)] mod tests { - use super::{ffmpeg_args, parse_encoder_names, select_hardware_encoder, HardwareSelection}; + use super::{ + ffmpeg_args, ffmpeg_partial_output_path, parse_encoder_names, select_hardware_encoder, + HardwareSelection, + }; + + // ── partial-output-path naming (pure) ──────────────────────────────────── + + #[test] + fn partial_path_keeps_the_original_extension_as_its_last_extension() { + let cases = [ + ("/tmp/out.mp4", "/tmp/out.partial.mp4"), + ("/tmp/out.mov", "/tmp/out.partial.mov"), + ("/tmp/out.webm", "/tmp/out.partial.webm"), + ("out.mp4", "out.partial.mp4"), + ]; + for (input, expected) in cases { + let got = ffmpeg_partial_output_path(std::path::Path::new(input)); + assert_eq!( + got, + std::path::PathBuf::from(expected), + "input={input}: ffmpeg picks its muxer from the last extension, so it must \ + survive unchanged" + ); + } + } + + #[test] + fn partial_path_is_a_sibling_of_the_final_output_not_a_different_directory() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/a/b/c/out.mp4")); + assert_eq!( + got.parent(), + Some(std::path::Path::new("/a/b/c")), + "the rename onto output_path must stay on the same filesystem" + ); + } + + #[test] + fn partial_path_falls_back_gracefully_with_no_extension() { + let got = ffmpeg_partial_output_path(std::path::Path::new("/tmp/out")); + assert_eq!(got, std::path::PathBuf::from("/tmp/out.partial")); + } /// Every option that describes the *output* has to sit after the last `-i`. /// Put one before it and ffmpeg attaches it to the following input instead, diff --git a/crates/rustmotion/src/encode/video/formats.rs b/crates/rustmotion/src/encode/video/formats.rs index ee06ab7a..8cad2f02 100644 --- a/crates/rustmotion/src/encode/video/formats.rs +++ b/crates/rustmotion/src/encode/video/formats.rs @@ -10,10 +10,52 @@ use crate::schema::ResolvedScenario as Scenario; use super::tasks::{build_frame_tasks, render_frame_task}; use super::EncodeProgress; -/// Encode frames as a PNG sequence (one PNG file per frame) +/// Sibling scratch directory a PNG-sequence render writes into before being +/// promoted onto `output_dir` — same reasoning as `partial_sibling_path`, +/// applied to a directory instead of a single file: directories don't have +/// an extension to preserve, so the suffix is the whole difference. +fn partial_sibling_dir(output_dir: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_dir); + let name = path + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + path.with_file_name(format!("{name}.partial")) +} + +/// Encode frames as a PNG sequence (one PNG file per frame). +/// +/// Renders into a sibling scratch directory and promotes (renames) it onto +/// `output_dir` only once every frame has been written — a mid-sequence +/// failure used to leave a partial run's files sitting directly in +/// `output_dir`, indistinguishable from a completed one to a caller that +/// only checks the directory exists (the same shape a single-file output +/// failing mid-encode has). pub fn encode_png_sequence( scenario: &Scenario, output_dir: &str, + quiet: bool, + transparent: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_dir = partial_sibling_dir(output_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + match encode_png_sequence_to_dir(scenario, &partial_dir, quiet, transparent, on_progress) { + Ok(()) => { + let _ = std::fs::remove_dir_all(output_dir); + std::fs::rename(&partial_dir, output_dir)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_dir_all(&partial_dir); + Err(e) + } + } +} + +fn encode_png_sequence_to_dir( + scenario: &Scenario, + output_dir: &std::path::Path, _quiet: bool, _transparent: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, @@ -67,7 +109,7 @@ pub fn encode_png_sequence( for result in results { let (frame_num, rgba) = result?; - let path = format!("{}/frame_{:05}.png", output_dir, frame_num); + let path = output_dir.join(format!("frame_{:05}.png", frame_num)); let img = image::RgbaImage::from_raw(width, height, rgba) .ok_or(RustmotionError::PixelImage)?; img.save(&path)?; @@ -77,11 +119,59 @@ pub fn encode_png_sequence( Ok(()) } -/// Encode frames as an animated GIF +/// Sibling scratch path a single-file encoder (GIF today) writes to before +/// being promoted onto `output_path` only after every frame is written +/// successfully. Same discipline and the same reasoning as +/// `video::ffmpeg::ffmpeg_partial_output_path`: kept in the same directory +/// so the promotion is a same-filesystem rename, extension kept last since +/// some downstream consumers of the output (players, `file`) pick behavior +/// from it the way ffmpeg picks a muxer from its own output extension. +fn partial_sibling_path(output_path: &str) -> std::path::PathBuf { + let path = std::path::Path::new(output_path); + let stem = path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("output"); + let name = match path.extension().and_then(|s| s.to_str()) { + Some(ext) => format!("{stem}.partial.{ext}"), + None => format!("{stem}.partial"), + }; + path.with_file_name(name) +} + +/// Encode frames as an animated GIF. +/// +/// Writes to a sibling scratch path first and promotes (renames) it onto +/// `output_path` only once every frame has been written without error — +/// the same partial-then-rename discipline `video_audio::extract_audio_to_wav` +/// and the ffmpeg encoder already apply. A GIF that fails midway through the +/// frame loop (any `?` inside `encode_gif_to_path`) used to leave a +/// truncated-but-existing file sitting at `output_path`, the same shape the +/// ffmpeg-backed encoder and the PNG-sequence encoder had before adopting +/// this same discipline. pub fn encode_gif( scenario: &Scenario, output_path: &str, quiet: bool, + on_progress: Option<&mut dyn FnMut(EncodeProgress)>, +) -> Result<()> { + let partial_path = partial_sibling_path(output_path); + match encode_gif_to_path(scenario, &partial_path, quiet, on_progress) { + Ok(()) => { + std::fs::rename(&partial_path, output_path)?; + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_file(&partial_path); + Err(e) + } + } +} + +fn encode_gif_to_path( + scenario: &Scenario, + partial_path: &std::path::Path, + quiet: bool, mut on_progress: Option<&mut dyn FnMut(EncodeProgress)>, ) -> Result<()> { let config = &scenario.video; @@ -103,7 +193,7 @@ pub fn encode_gif( let gif_w = width.min(65535) as u16; let gif_h = height.min(65535) as u16; - let file = File::create(output_path)?; + let file = File::create(partial_path)?; let mut encoder = gif::Encoder::new(BufWriter::new(file), gif_w, gif_h, &[]).map_err(|e| { RustmotionError::GifEncoder { reason: e.to_string(), @@ -243,6 +333,88 @@ mod tests { use crate::loader::load_scenario_from_source; use std::path::{Path, PathBuf}; + // ── partial-output naming (pure) ───────────────────────────────────────── + + #[test] + fn partial_sibling_path_keeps_the_extension_last() { + assert_eq!( + partial_sibling_path("/tmp/out.gif"), + PathBuf::from("/tmp/out.partial.gif") + ); + assert_eq!( + partial_sibling_path("/tmp/out"), + PathBuf::from("/tmp/out.partial") + ); + } + + #[test] + fn partial_sibling_dir_stays_next_to_the_final_directory() { + let got = partial_sibling_dir("/a/b/frames"); + assert_eq!(got, PathBuf::from("/a/b/frames.partial")); + assert_eq!(got.parent(), Some(Path::new("/a/b"))); + } + + /// A failed GIF encode must not leave a truncated file at `output_path` + /// — the real trigger used to prove this for the ffmpeg-backed encoder + /// (an odd width rejected by libx264) doesn't apply here (the `gif` + /// crate has no such constraint), so this drives the failure the one + /// way this pure Rust path can actually fail without external tools: + /// `total_frames == 0`. + /// That returns before any file is touched either way, so what this + /// test really pins is the *shape* of the fix — `encode_gif` must never + /// promote a partial onto `output_path` when its inner call errors — + /// exercised by asserting the scratch/partial path used internally is + /// never left behind either. + #[test] + fn encode_gif_leaves_no_partial_file_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out = std::env::temp_dir().join(format!( + "rm_gif_no_debris_test_{}_{}.gif", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial = partial_sibling_path(out.to_str().unwrap()); + let _ = std::fs::remove_file(&out); + let _ = std::fs::remove_file(&partial); + + let result = encode_gif(&scenario, out.to_str().unwrap(), true, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out.exists(), "no debris at the final output path"); + assert!(!partial.exists(), "no debris at the scratch path either"); + } + + /// Same property for the directory-based PNG-sequence encoder. + #[test] + fn encode_png_sequence_leaves_no_partial_dir_behind_on_an_empty_scenario() { + let json = r#"{"video": {"width": 8, "height": 8, "fps": 10}, "scenes": []}"#; + let scenario = load_scenario_from_source(None, Some(json)).expect("load"); + + let out_dir = std::env::temp_dir().join(format!( + "rm_png_seq_no_debris_test_{}_{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let partial_dir = partial_sibling_dir(out_dir.to_str().unwrap()); + let _ = std::fs::remove_dir_all(&out_dir); + let _ = std::fs::remove_dir_all(&partial_dir); + + let result = encode_png_sequence(&scenario, out_dir.to_str().unwrap(), true, false, None); + assert!(result.is_err(), "an empty scenario has no frames to encode"); + assert!(!out_dir.exists(), "no debris at the final output directory"); + assert!( + !partial_dir.exists(), + "no debris at the scratch directory either" + ); + } + /// Deterministic color for a given frame index: distinct enough across /// nearby indices that a swapped/misplaced frame is detected by a plain /// pixel comparison. diff --git a/crates/rustmotion/tests/audit_ws_c.rs b/crates/rustmotion/tests/audit_ws_c.rs new file mode 100644 index 00000000..e612794e --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_c.rs @@ -0,0 +1,117 @@ +//! Regression tests — audit round chantier/audit-2026-09, workstream C +//! (encoding, audio, cancellation). +//! +//! Tests here only exercise the public API surface — `rustmotion::encode::...` +//! — since this is an external integration test crate; regression tests for +//! private helpers live next to them in their own `#[cfg(test)] mod tests` +//! inside the `src/encode/` file that owns them, and are cross-referenced +//! here by the behavior they cover. + +use std::path::PathBuf; + +/// Minimal RAII scratch file: unique per (label, pid, nanosecond timestamp), +/// removed on drop even if the test panics partway through. +struct ScratchFile(PathBuf); + +impl ScratchFile { + /// `ext` is the extension ffmpeg/the encoder picks its container format + /// from (`"mp4"`, `"gif"`, ...) — omitting it makes ffmpeg fail at + /// muxer selection before ever touching the path, which would make a + /// debris-after-failure test pass for the wrong reason. + fn new(label: &str, ext: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-c-{label}-{}-{}.{ext}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + Self(std::env::temp_dir().join(unique)) + } + + fn path(&self) -> &std::path::Path { + &self.0 + } + + fn to_str(&self) -> &str { + self.0.to_str().expect("scratch path must be UTF-8") + } +} + +impl Drop for ScratchFile { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.0); + } +} + +fn ffmpeg_on_path() -> bool { + std::process::Command::new("ffmpeg") + .args(["-version"]) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .map(|s| s.success()) + .unwrap_or(false) +} + +// ─── A render ffmpeg rejects must leave no debris at output_path ────────── +// +// `encode_with_ffmpeg`/`encode_with_ffmpeg_hw` used to hand ffmpeg the +// user's own `output_path` directly, drop stdin and gracefully wait no +// matter how the frame loop ended. A failure mid-render (a broken pipe, or +// ffmpeg itself exiting non-zero) still let ffmpeg finalize whatever it had +// already received at that exact path, and `-y` meant it would even +// overwrite a previously-good render there. No cleanup ran on any error +// path. +// +// Forcing rustmotion's *own* frame renderer to fail deterministically isn't +// possible from a plain scenario — `Painter::paint_content` cannot return an +// `Err`, so no user-authored content can fail a frame. ffmpeg itself, +// though, refuses an odd width under the crate's default 10-bit H.264 +// profile (`yuv420p10le` needs even 4:2:0 chroma dimensions) — confirmed by +// hand: `ffmpeg -f rawvideo ... -video_size 321x240 ... -c:v libx264 +// -profile:v high10 -pix_fmt yuv420p10le out.mp4` creates a 0-byte +// `out.mp4` and then fails to open its encoder, closing stdin before +// rustmotion finishes writing frames — exactly the "ffmpeg already has a +// file open at output_path when the failure happens" shape this test cares +// about, without needing an internal render failure at all. +#[test] +fn a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path() { + if !ffmpeg_on_path() { + eprintln!( + "a_render_ffmpeg_rejects_leaves_no_debris_at_the_output_path: \ + ffmpeg not found — skipping" + ); + return; + } + + let json = r#"{"video": {"width": 321, "height": 240, "fps": 10}, + "scenes": [{"duration": 0.3, "children": []}]}"#; + let scenario = rustmotion::loader::load_scenario_from_source(None, Some(json)).expect("load"); + + let out = ScratchFile::new("odd-width", "mp4"); + let _ = std::fs::remove_file(out.path()); + + let result = rustmotion::encode::encode_with_ffmpeg( + &scenario, + out.to_str(), + true, + "h264", + None, + false, + None, + ); + + assert!( + result.is_err(), + "an odd width (321) must make libx264's high10/yuv420p10le encoder \ + init fail — this test's premise depends on ffmpeg actually rejecting it" + ); + assert!( + !out.path().exists(), + "no file — truncated, empty, or otherwise — may be left at output_path \ + after a failed render; got one at {}", + out.path().display() + ); +}