From 6144ca08042665d8afc1c408bde68bea6da320b7 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 13:27:33 -0400 Subject: [PATCH] Never crash: last-resort guard around command execution, and a logging panic hook Session::execute runs every command (UI, CLI, control channel, MCP, scripts; file import, export and save are commands too) through guard::guarded, which catch_unwinds a panic that escapes the command and returns EngineError::Other("command `x` failed with an internal error (...); it was stopped and your project is still open"). Edits work on a copy of the project, so a command that panics mid-edit leaves the project untouched. The desktop app also installs a panic hook that logs every panic through the app logger (then runs the default hook). Co-Authored-By: Claude Opus 5.5 --- apps/effectcraft/src/main.rs | 1 + crates/engine/src/guard.rs | 65 ++++++++++++++++++++++++++++++++++++ crates/engine/src/lib.rs | 4 ++- crates/engine/src/logging.rs | 10 ++++++ 4 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 crates/engine/src/guard.rs diff --git a/apps/effectcraft/src/main.rs b/apps/effectcraft/src/main.rs index 2262ad80..43c95c6a 100644 --- a/apps/effectcraft/src/main.rs +++ b/apps/effectcraft/src/main.rs @@ -26,6 +26,7 @@ fn app_icon() -> egui::IconData { fn main() -> eframe::Result { // Help ▸ Enable Logging writes through this logger; warnings feed the compatibility report. effectcraft_engine::logging::install(); + effectcraft_engine::logging::install_panic_hook(); let mut control_port: Option = std::env::var("EFFECTCRAFT_CONTROL_PORT").ok().and_then(|p| p.parse().ok()); let mut files = Vec::new(); let mut demo = true; diff --git a/crates/engine/src/guard.rs b/crates/engine/src/guard.rs new file mode 100644 index 00000000..699e6474 --- /dev/null +++ b/crates/engine/src/guard.rs @@ -0,0 +1,65 @@ +//! The last-resort guard: a panic that escapes a command (a bug, never an expected error) is +//! turned into an [`EngineError`] instead of taking the app, and the user's open project, down +//! with it. Edits work on a copy of the project (see [`crate::Session::edit`]), so an edit that +//! panics leaves the project as it was. This is a safety net, not a substitute for returning +//! errors (AGENTS.md, "Never crash"). + +use crate::{EngineError, Result}; + +/// Run `f`; a panic inside it becomes an error naming `what`. The panic itself is logged by +/// the panic hook ([`crate::logging::install_panic_hook`]). +pub fn guarded(what: &str, f: impl FnOnce() -> Result) -> Result { + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { + Ok(r) => r, + Err(payload) => { + let msg = payload + .downcast_ref::<&str>() + .map(|s| s.to_string()) + .or_else(|| payload.downcast_ref::().cloned()) + .unwrap_or_else(|| "unknown error".into()); + log::error!("{what} panicked: {msg}"); + Err(EngineError::Other(format!("{what} failed with an internal error ({msg}); it was stopped and your project is still open"))) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_panic_becomes_an_error() { + let r: Result<()> = guarded("command `test.panic`", || panic!("boom")); + let e = r.unwrap_err().to_string(); + assert!(e.contains("test.panic") && e.contains("boom"), "{e}"); + let r: Result = guarded("x", || { + let v: Vec = vec![]; + Ok(v[3]) + }); + assert!(r.is_err()); + assert_eq!(guarded("ok", || Ok(7)).unwrap(), 7); + assert!(matches!(guarded::<()>("err", || Err(EngineError::NoComp)), Err(EngineError::NoComp))); + } + + #[test] + fn the_session_survives_a_panicking_edit() { + let mut s = crate::Session::new(); + let before = s.project.clone(); + let r: Result<()> = guarded("command `test.edit`", || { + s.edit("Broken", None, |p, _| { + p.next_id += 1; + panic!("bug in the middle of an edit") + }) + }); + assert!(r.is_err()); + assert!(std::sync::Arc::ptr_eq(&before, &s.project), "the project is untouched"); + // Later edits still work. + let next = s.project.next_id; + s.edit("Fine", None, |p, _| { + p.next_id += 1; + Ok(()) + }) + .unwrap(); + assert_eq!(s.project.next_id, next + 1); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 26dc819a..6266f1d6 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -14,6 +14,7 @@ pub mod commands; pub mod config; pub mod demo; pub mod footage_check; +pub mod guard; pub mod history; pub mod jobs; pub mod learn; @@ -490,7 +491,8 @@ impl Session { return Err(EngineError::Disabled(id.to_string(), why)); } } - let r = (spec.run)(self, ¶ms)?; + // Last-resort guard: a panicking command reports an error instead of crashing the app. + let r = guard::guarded(&format!("command `{id}`"), || (spec.run)(self, ¶ms))?; self.learn_observe(id, ¶ms); if spec.journal { self.journal.push((id.to_string(), params)); diff --git a/crates/engine/src/logging.rs b/crates/engine/src/logging.rs index 23ef28f0..04c0ab41 100644 --- a/crates/engine/src/logging.rs +++ b/crates/engine/src/logging.rs @@ -62,6 +62,16 @@ pub fn install() { } } +/// Log every panic (message and location) through the logger, then run the previous hook +/// (which prints it). Panics in commands are then caught by [`crate::guard::guarded`]. +pub fn install_panic_hook() { + let previous = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + log::error!("panic: {info}"); + previous(info); + })); +} + /// The log file inside `config_dir` (the settings folder), or the temporary folder. pub fn log_path(config_dir: Option<&Path>) -> PathBuf { config_dir.map(Path::to_path_buf).unwrap_or_else(std::env::temp_dir).join("Logs").join("EffectCraft Log.txt")