From 72c5ac44b9dfe8c8af9b63dcda902c2ad3188bed Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:09 +0200 Subject: [PATCH] fix(batch): index the name template by character MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolve_name_template` walks the template as raw bytes and casts each one to `char`, which in Rust is a Latin-1 reinterpretation of the byte value. Substituted `{field}` values are fine (they go through `push_str` at line 71); the literal text around them is not. I confirmed the behaviour by compiling the same loop standalone: `résumé-{id}.mp4` yields `résumé-VAL.mp4` and `动画-{id}.mp4` yields `å¨ç»-VAL.mp4`. So `rustmotion batch --name-template "résumé-{id}.mp4"` writes files literally named `résumé-abc.mp4` to disk, and the same corruption lands in the `[ok] ` progress lines and in every failure message (`item N: ...`). A localisation batch — the exact use case `{lang}/{id}.mp4` in the module doc is built for — is where a non-ASCII template is most likely. None of the five tests in `name_template_tests` (lines 448-495) uses a non-ASCII template. Refs #220 --- crates/rustmotion/src/cli/commands/batch.rs | 35 +++- crates/rustmotion/src/cli/commands/render.rs | 75 +++++++- crates/rustmotion/tests/audit_ws_d.rs | 170 ++++++++++++++++++- 3 files changed, 265 insertions(+), 15 deletions(-) diff --git a/crates/rustmotion/src/cli/commands/batch.rs b/crates/rustmotion/src/cli/commands/batch.rs index b8ec3643..e68d2f20 100644 --- a/crates/rustmotion/src/cli/commands/batch.rs +++ b/crates/rustmotion/src/cli/commands/batch.rs @@ -71,11 +71,14 @@ pub(crate) fn resolve_name_template( out.push_str(&replacement); cursor = start + end + 1; // skip past '}' } else { - out.push(bytes[cursor] as char); - cursor += 1; + let ch = template[cursor..].chars().next().expect( + "cursor sits on a UTF-8 char boundary: every branch above advances it either \ + past an ASCII '{'/'}' byte or by a full char's own byte length", + ); + out.push(ch); + cursor += ch.len_utf8(); } } - let _ = bytes; Ok(out) } @@ -492,6 +495,32 @@ mod name_template_tests { "static.mp4" ); } + + /// The literal (non-`{field}`) text of the template used to be walked + /// byte-by-byte and each byte cast straight to `char` — a Latin-1 + /// reinterpretation of whatever UTF-8 continuation bytes an accented + /// character produced. `é` is `0xC3 0xA9` in UTF-8; cast individually + /// that becomes `é`, exactly the corruption this asserts is gone. + #[test] + fn accented_literal_text_round_trips() { + let data = row(&[("id", json!("abc"))]); + assert_eq!( + resolve_name_template("résumé-{id}.mp4", &data, 0).unwrap(), + "résumé-abc.mp4" + ); + } + + /// A non-Latin script exercises characters that are more than two UTF-8 + /// bytes wide, where a byte-at-a-time cast produces even more mangled + /// output than the two-byte Latin-1 case above. + #[test] + fn cjk_literal_text_round_trips() { + let data = row(&[("id", json!("1"))]); + assert_eq!( + resolve_name_template("动画-{id}.mp4", &data, 0).unwrap(), + "动画-1.mp4" + ); + } } #[cfg(test)] diff --git a/crates/rustmotion/src/cli/commands/render.rs b/crates/rustmotion/src/cli/commands/render.rs index 2c6a11da..53fd6fd2 100644 --- a/crates/rustmotion/src/cli/commands/render.rs +++ b/crates/rustmotion/src/cli/commands/render.rs @@ -8,6 +8,20 @@ use std::path::{Path, PathBuf}; use crate::cli::commands::validation::{self, ValidationSource}; +/// Every process-global decode cache a `--watch` iteration must forget +/// before re-rendering, so an edited asset is never served from a stale +/// entry keyed only on its path. `ASSET_CACHE` already had a public +/// clear function; `GIF_CACHE`/`VIDEO_FRAME_CACHE` did not, so those two are +/// cleared here by calling `DashMap::clear()` on the map `gif_cache()`/ +/// `video_frame_cache()` already return, rather than adding new functions to +/// `rustmotion-core`'s `engine::renderer::assets` (owned by a sibling +/// workstream in this chantier). +fn clear_all_media_caches() { + engine::clear_asset_cache(); + engine::gif_cache().clear(); + engine::video_frame_cache().clear(); +} + /// Load + validate a scenario for watch mode. On validation failure prints the /// report and returns the typed error so the caller can decide how to handle it. /// @@ -373,7 +387,7 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); + clear_all_media_caches(); if let Err(e) = cmd_render( scenario, output, @@ -430,6 +444,8 @@ pub fn cmd_watch( match load_for_watch(input, no_validate, lenient, strict_anim, strict_attrs) { Ok(scenario) => { + clear_all_media_caches(); + // Reset error backoff on a successful load if consecutive_err_count > 0 && suppressed { eprintln!("Recovered from previous errors."); @@ -452,7 +468,6 @@ pub fn cmd_watch( let use_prev = if prev_config_hash == Some(config_hash) { prev_segments.as_deref() } else { - engine::clear_asset_cache(); None }; @@ -528,7 +543,6 @@ pub fn cmd_watch( Err(e) => eprintln!("Render error: {}", e), } } else { - engine::clear_asset_cache(); if let Err(e) = cmd_render( scenario, output, @@ -596,3 +610,58 @@ fn render_single_frame( img.save(output)?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::engine::renderer::{asset_cache, gif_cache, video_frame_cache}; + + fn unique_marker(label: &str) -> String { + format!( + "rustmotion-audit-ws-d-rm25-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ) + } + + /// `--watch` only ever called `engine::clear_asset_cache()`, and + /// only conditionally in the incremental branch (when the video config + /// hash changed). `GIF_CACHE`/`VIDEO_FRAME_CACHE` had no clear function + /// at all, so an edited GIF or embedded video stayed stale for the rest + /// of a `--watch` session no matter how many times the source file + /// changed. This populates all three caches with markers unique to this + /// test run — safe against the other tests in this binary that share the + /// same process-global caches — and asserts a single call clears every + /// one of them, not just the asset cache. + #[test] + fn clear_all_media_caches_clears_gif_and_video_caches_not_just_images() { + let marker = unique_marker("clear-all"); + + let mut surface = skia_safe::surfaces::raster_n32_premul((1, 1)).expect("raster surface"); + asset_cache().insert(marker.clone(), surface.image_snapshot()); + gif_cache().insert( + marker.clone(), + std::sync::Arc::new((Vec::new(), Vec::new(), 0.0)), + ); + video_frame_cache().insert(marker.clone(), std::sync::Arc::new(Vec::new())); + + assert!(asset_cache().contains_key(&marker)); + assert!(gif_cache().contains_key(&marker)); + assert!(video_frame_cache().contains_key(&marker)); + + clear_all_media_caches(); + + assert!(!asset_cache().contains_key(&marker)); + assert!( + !gif_cache().contains_key(&marker), + "gif cache must be cleared too, not just the asset cache" + ); + assert!( + !video_frame_cache().contains_key(&marker), + "video frame cache must be cleared too, not just the asset cache" + ); + } +} diff --git a/crates/rustmotion/tests/audit_ws_d.rs b/crates/rustmotion/tests/audit_ws_d.rs index 36c6a6a1..23c484e7 100644 --- a/crates/rustmotion/tests/audit_ws_d.rs +++ b/crates/rustmotion/tests/audit_ws_d.rs @@ -1,10 +1,29 @@ //! Regression tests for the media-decoding, frame-cache, and render/batch //! CLI hardening pass on this branch. //! -//! Video frame preextraction (`preload::preextract_video_frames`) is -//! exercised through the crate's public `rustmotion::engine::preload` -//! surface directly — no subprocess needed, since the budget math and the -//! cache it guards are both `pub`. +//! Two kinds of coverage land here: +//! +//! - Video frame preextraction (`preload::preextract_video_frames`) is +//! exercised through the crate's public `rustmotion::engine::preload` +//! surface directly — no subprocess needed, since the budget math and the +//! cache it guards are both `pub`. +//! - The batch `--name-template` byte-vs-char bug +//! (`cli::commands::batch::resolve_name_template`) cannot be reached this +//! way: `cli::commands` is a private module (`mod commands;` in +//! `src/cli/mod.rs`), so — mirroring `audit_ws_b.rs`'s reasoning for the +//! same constraint — the only externally-observable contract is the +//! compiled binary itself, driven as a subprocess. +//! +//! The GIF cache-stampede/decompression-bomb caps and the video component's +//! dead-field fixes (`fit`, `loop_video`, `trim_end`, straight-vs-premultiplied +//! alpha) live in `rustmotion-components`'s own test suite instead — see +//! that crate's `audit_ws_d.rs` and `gif.rs`'s in-file `mod tests`. The +//! `--watch` cache-clearing fix needs a private helper in +//! `cli::commands::render` and lives in that file's own `mod tests` for the +//! same private-module reason as the name-template test above. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; use rustmotion::engine::preload::{ video_frame_byte_size, would_exceed_cache_budget, VIDEO_FRAME_CACHE_BUDGET_BYTES, @@ -23,11 +42,10 @@ fn frame_byte_size_matches_plain_multiplication_for_ordinary_dimensions() { /// The secondary hazard this closes: `width * height * 4` in plain `u32` /// wraps for a large-enough declared size (65536×16384 wraps to 0, which -/// used to turn into a division by zero downstream). `u32::MAX` on both -/// dimensions is the most extreme case reachable from a `style.width`/ -/// `style.height` pair — the fixed computation must saturate to `u64::MAX`, -/// not wrap to some small number that would slip past the budget check -/// below. +/// used to turn into a division by zero downstream). `u32::MAX` on both dimensions +/// is the most extreme case reachable from a `style.width`/`style.height` +/// pair — the fixed computation must saturate to `u64::MAX`, not wrap to +/// some small number that would slip past the budget check below. #[test] fn frame_byte_size_saturates_instead_of_wrapping_on_extreme_dimensions() { let huge = video_frame_byte_size(u32::MAX, u32::MAX); @@ -61,3 +79,137 @@ fn would_exceed_cache_budget_rejects_only_once_the_sum_crosses_the_ceiling() { "saturating add must not wrap past the ceiling" ); } + +// ─── `batch --name-template` must not mangle non-ASCII bytes ────────────── + +/// Minimal RAII scratch directory, mirroring `audit_ws_b.rs`'s `ScratchDir`. +struct ScratchDir(PathBuf); + +impl ScratchDir { + fn new(label: &str) -> Self { + let unique = format!( + "rustmotion-audit-ws-d-{label}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock before UNIX epoch") + .as_nanos() + ); + let path = std::env::temp_dir().join(unique); + std::fs::create_dir_all(&path).expect("create scratch dir"); + Self(path) + } +} + +impl Drop for ScratchDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn write_file(dir: &Path, name: &str, contents: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, contents).expect("write scratch file"); + path +} + +fn run_batch(file: &Path, data: &Path, output_dir: &Path, name_template: &str) -> Output { + Command::new(env!("CARGO_BIN_EXE_rustmotion")) + .arg("--quiet") + .arg("batch") + .arg("--file") + .arg(file) + .arg("--data") + .arg(data) + .arg("--output-dir") + .arg(output_dir) + .arg("--name-template") + .arg(name_template) + .arg("--format") + .arg("png-seq") + .output() + .expect("failed to spawn `rustmotion batch`") +} + +/// `resolve_name_template` used to walk the template as raw bytes and +/// cast each one to `char` — a Latin-1 reinterpretation that mangles every +/// non-ASCII byte: `résumé-{id}.mp4` became `résumé-VAL.mp4` on disk. +/// `batch`'s own module doc gives `{lang}/{id} +/// .mp4` as the flagship use case for `--name-template`, which is exactly +/// the localisation scenario most likely to carry accents. +/// +/// Driven as a subprocess rather than calling `resolve_name_template` +/// directly: it is `pub(crate)` inside the private `cli::commands::batch` +/// module, unreachable from an external integration test. +#[test] +fn batch_name_template_round_trips_accented_characters_on_disk() { + let scratch = ScratchDir::new("rm30"); + let template = write_file( + &scratch.0, + "template.json", + &serde_json::json!({ + "config": { "id": { "type": "string", "default": "x" } }, + "video": { "width": 32, "height": 32, "fps": 1 }, + "scenes": [{ "duration": 1.0, "children": [] }] + }) + .to_string(), + ); + let data = write_file( + &scratch.0, + "data.jsonl", + &serde_json::json!({"id": "abc"}).to_string(), + ); + let output_dir = scratch.0.join("out"); + std::fs::create_dir_all(&output_dir).expect("create output dir"); + + let result = run_batch(&template, &data, &output_dir, "résumé-{id}.png"); + + assert!( + result.status.success(), + "batch must succeed: stdout={}\nstderr={}", + String::from_utf8_lossy(&result.stdout), + String::from_utf8_lossy(&result.stderr) + ); + + let expected = output_dir.join("résumé-abc.png"); + assert!( + expected.is_dir(), + "expected an accent-preserving output directory at {}, found instead: {:?}", + expected.display(), + std::fs::read_dir(&output_dir) + .map(|entries| entries + .filter_map(|e| e.ok().map(|e| e.file_name())) + .collect::>()) + .unwrap_or_default() + ); + assert!(expected.join("frame_00000.png").exists()); +} + +/// A template with no non-ASCII content must still resolve exactly as +/// before — the fix changes how a byte becomes a `char`, not the loop's +/// control flow (the `{`/`}` brace scan is untouched). +#[test] +fn batch_name_template_plain_ascii_is_unaffected() { + let scratch = ScratchDir::new("rm30-ascii"); + let template = write_file( + &scratch.0, + "template.json", + &serde_json::json!({ + "config": { "id": { "type": "string", "default": "x" } }, + "video": { "width": 32, "height": 32, "fps": 1 }, + "scenes": [{ "duration": 1.0, "children": [] }] + }) + .to_string(), + ); + let data = write_file( + &scratch.0, + "data.jsonl", + &serde_json::json!({"id": "abc"}).to_string(), + ); + let output_dir = scratch.0.join("out"); + std::fs::create_dir_all(&output_dir).expect("create output dir"); + + let result = run_batch(&template, &data, &output_dir, "clip-{id}.png"); + assert!(result.status.success()); + assert!(output_dir.join("clip-abc.png").is_dir()); +}