From 3978f849cb8c31b7bf7cc73aa945ac44351ccfc7 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:01:49 +0200 Subject: [PATCH] fix(studio): surface a render panic instead of caching an empty frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `render_frame` panics on failure (`.expect("render frame")`, `.expect("rgba matches dimensions")`, `.expect("encode jpeg")`). Because the panic happens in the scoped child thread and is consumed by `join().unwrap_or_default()`, the `catch_unwind` wrapper in `view.rs::serve_or_render` never fires — so `fail_ledger().record_failure(key)` is unreachable on that path and the whole retry-budget mechanism is dead code for the on-demand asset handler. Instead the empty `Vec` is treated as a successful render: `frame_cache().insert(key, rendered.clone(), ...)` caches zero bytes, and the responder replies `200 image/jpeg` with an empty body. The canvas shows a permanently broken image for that (generation, frame, scale) and the cache guarantees it is never re-attempted. (The prefetch worker path calls `render_frame` directly and is correctly fenced.) Refs #220 --- crates/rustmotion-studio/src/editor/audio.rs | 4 +- crates/rustmotion-studio/src/editor/frames.rs | 11 +++- crates/rustmotion-studio/src/editor/view.rs | 24 ++++--- crates/rustmotion-studio/src/lib.rs | 2 +- crates/rustmotion-studio/src/library/data.rs | 4 +- .../src/scenario/optimistic.rs | 4 +- crates/rustmotion-studio/tests/audit_ws_g.rs | 65 ++++++++++++++++--- 7 files changed, 84 insertions(+), 30 deletions(-) diff --git a/crates/rustmotion-studio/src/editor/audio.rs b/crates/rustmotion-studio/src/editor/audio.rs index 6a232169..5b015479 100644 --- a/crates/rustmotion-studio/src/editor/audio.rs +++ b/crates/rustmotion-studio/src/editor/audio.rs @@ -165,9 +165,7 @@ pub fn prepare(scenario: Arc, total_duration: f64) { match mixed { Ok(Some(pcm_bytes)) => { let pcm: Vec = pcm_bytes - .as_chunks::<2>() - .0 - .iter() + .chunks_exact(2) .map(|b| i16::from_le_bytes([b[0], b[1]]) as f32 / 32768.0) .collect(); let _ = tx.send(Cmd::Load(Arc::new(pcm), SAMPLE_RATE)); diff --git a/crates/rustmotion-studio/src/editor/frames.rs b/crates/rustmotion-studio/src/editor/frames.rs index a734fd2e..b56ce160 100644 --- a/crates/rustmotion-studio/src/editor/frames.rs +++ b/crates/rustmotion-studio/src/editor/frames.rs @@ -26,20 +26,25 @@ pub const RENDER_STACK: usize = 32 * 1024 * 1024; /// [`render_frame`] on a thread with [`RENDER_STACK`], for callers that are not /// on the main thread. Scoped, so the scenario and tasks are borrowed rather -/// than cloned. +/// than cloned. `Err` means the render thread panicked (a Skia panic, a +/// dimension mismatch, a JPEG encode failure): the caller must not treat that +/// as a successful empty frame. +// The panic payload carries no information callers act on; they only branch +// on success vs. failure (retry-budget ledger, cache, HTTP status). +#[allow(clippy::result_unit_err)] pub fn render_frame_deep( scenario: &ResolvedScenario, tasks: &[rustmotion::encode::video::FrameTask], frame: u32, scale: f32, -) -> Vec { +) -> Result, ()> { std::thread::scope(|scope| { std::thread::Builder::new() .stack_size(RENDER_STACK) .spawn_scoped(scope, || render_frame(scenario, tasks, frame, scale)) .expect("spawn render thread") .join() - .unwrap_or_default() + .map_err(|_| ()) }) } diff --git a/crates/rustmotion-studio/src/editor/view.rs b/crates/rustmotion-studio/src/editor/view.rs index 75362516..75025a27 100644 --- a/crates/rustmotion-studio/src/editor/view.rs +++ b/crates/rustmotion-studio/src/editor/view.rs @@ -361,6 +361,10 @@ pub fn StudioApp(view: Signal) -> Element { /// into the cache for the next request. `gen_b` is `Some(model generation)` /// when serving side B (drives stale-generation eviction); side A passes the /// baseline hash inside `key.generation` and `None` here. +/// +/// `render_frame_deep` already isolates a render panic inside its own scoped +/// thread and reports it as `Err`; the `catch_unwind` here only guards the +/// (rarer) case of the OS failing to spawn that thread. fn serve_or_render( key: FrameKey, idx: u32, @@ -385,15 +389,19 @@ fn serve_or_render( { return Err(()); } - let rendered = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let outer = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { render_frame_deep(scenario, tasks, idx, scale_factor(key.scale_pct)) - })) - .map_err(|_| { - fail_ledger() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .record_failure(key); - })?; + })); + let rendered = match outer { + Ok(Ok(bytes)) => bytes, + _ => { + fail_ledger() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .record_failure(key); + return Err(()); + } + }; let (gen_b, gen_a) = match key.side { DiffSide::B => (gen_b, None), DiffSide::A => (None, Some(key.generation)), diff --git a/crates/rustmotion-studio/src/lib.rs b/crates/rustmotion-studio/src/lib.rs index 9af6d687..82f295de 100644 --- a/crates/rustmotion-studio/src/lib.rs +++ b/crates/rustmotion-studio/src/lib.rs @@ -1,6 +1,6 @@ mod app; mod components; -mod editor; +pub mod editor; mod library; pub mod scenario; diff --git a/crates/rustmotion-studio/src/library/data.rs b/crates/rustmotion-studio/src/library/data.rs index f62482cb..62f72781 100644 --- a/crates/rustmotion-studio/src/library/data.rs +++ b/crates/rustmotion-studio/src/library/data.rs @@ -132,9 +132,7 @@ pub fn render_thumbnail(path: &Path) -> Option> { if tasks.is_empty() { return None; } - Some(crate::editor::frames::render_frame_deep( - &scenario, &tasks, 0, 0.25, - )) + crate::editor::frames::render_frame_deep(&scenario, &tasks, 0, 0.25).ok() } /// Cheap check: is this JSON a Rustmotion scenario? Avoids the heavier diff --git a/crates/rustmotion-studio/src/scenario/optimistic.rs b/crates/rustmotion-studio/src/scenario/optimistic.rs index 2d0a6f32..bcc95d17 100644 --- a/crates/rustmotion-studio/src/scenario/optimistic.rs +++ b/crates/rustmotion-studio/src/scenario/optimistic.rs @@ -382,9 +382,7 @@ mod tests { .expect("render") }; let count_red = |rgba: &[u8]| { - rgba.as_chunks::<4>() - .0 - .iter() + rgba.chunks_exact(4) .filter(|p| p[0] > 180 && p[1] < 90 && p[2] < 90) .count() }; diff --git a/crates/rustmotion-studio/tests/audit_ws_g.rs b/crates/rustmotion-studio/tests/audit_ws_g.rs index 1f7d983d..89b8325a 100644 --- a/crates/rustmotion-studio/tests/audit_ws_g.rs +++ b/crates/rustmotion-studio/tests/audit_ws_g.rs @@ -1,23 +1,27 @@ -//! Regression tests for the studio's file-write pipeline: a debounced write +//! Regression tests for the studio's file-write pipeline — a debounced write //! that must rebase onto the current disk instead of replaying a stale //! in-memory snapshot, coalesced edits inside one debounce window that must //! all survive (not just the last), and undo/redo cancelling a still-pending -//! write before it can clobber the just-restored state. +//! write before it can clobber the just-restored state — plus a render-thread +//! panic that must surface as an error instead of being cached as an empty +//! JPEG. //! //! `rustmotion-studio` has no `[dev-dependencies]` and cannot gain one in //! this change, so every test below drives the crate's existing public -//! surface (`scenario::*`) against real temp files with plain synchronous -//! `#[test]`s — no Dioxus runtime, no async executor. That public surface is -//! itself the fix for the crate having no integration tests: the defects -//! lived in a debounce timer and Dioxus event handlers that cannot be driven -//! from a test, so each one was reduced to a pure decision over plain data -//! (`resolve_flush`, the pending-write queue) and the handler calls that -//! instead of deciding inline. +//! surface (`scenario::*`, `editor::frames`) against real temp files with +//! plain synchronous `#[test]`s — no Dioxus runtime, no async executor. That +//! public surface is itself the fix for the crate having no integration +//! tests: the defects lived in a debounce timer and Dioxus event handlers +//! that cannot be driven from a test, so each one was reduced to a pure +//! decision over plain data (`resolve_flush`, the pending-write queue, +//! `render_frame_deep` returning `Result`) and the handler calls that instead +//! of deciding inline. use std::fs; use std::path::PathBuf; use std::sync::{Arc, Mutex}; +use rustmotion_studio::editor::frames::render_frame_deep; use rustmotion_studio::scenario::{ apply_optimistic, empty_scenario, pending_write_slot, queue_mutation, record_edit, resolve_flush, take_pending, undo, Mutation, Shared, SharedHistory, StudioModel, @@ -219,3 +223,46 @@ fn the_write_pipeline_round_trips_an_edit_through_a_real_file() { ); let _ = fs::remove_file(&path); } + +// ── A render-thread panic surfaces as an error, never a cached empty JPEG ── + +#[test] +fn a_render_thread_panic_is_reported_as_an_error_not_an_empty_jpeg() { + let big = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 }, { "duration": 0.1 } ] }"##), + ) + .unwrap(); + let tasks = rustmotion::encode::build_frame_tasks(&big); + assert!( + tasks.len() >= 2, + "need frames spanning both scenes to reach scene_idx 1" + ); + + // Deliberately mismatched: `tasks` reference a second scene this smaller + // scenario does not have, which panics inside the render thread. + let small = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 } ] }"##), + ) + .unwrap(); + + let last_frame = (tasks.len() - 1) as u32; + let result = render_frame_deep(&small, &tasks, last_frame, 1.0); + assert!( + result.is_err(), + "a scenario/task mismatch panics inside the render thread and must surface as Err" + ); +} + +#[test] +fn a_normal_render_returns_nonempty_jpeg_bytes() { + let scenario = rustmotion::loader::load_scenario_from_source( + None, + Some(r##"{ "video": { "width": 64, "height": 64 }, "scenes": [ { "duration": 0.1 } ] }"##), + ) + .unwrap(); + let tasks = rustmotion::encode::build_frame_tasks(&scenario); + let jpeg = render_frame_deep(&scenario, &tasks, 0, 1.0).expect("a normal render succeeds"); + assert!(!jpeg.is_empty()); +}