Merge pull request #14 from storytold/pr/never-crash-guard

Never crash: last-resort guard around every command

crates/engine/src/guard.rs

@@ -0,0 +1,89 @@
+//! Last-resort guard (craftrules `standards/never-crash.md`): a panic that escapes a command —
+//! any edit, import or export, from the UI, the CLI, MCP or the control channel — becomes an
+//! error, and the document is kept as it was before the command (and saved for crash recovery
+//! when a recovery directory is set). It is a safety net: commands still must not panic.
+
+use std::panic::{AssertUnwindSafe, catch_unwind};
+
+use serde_json::Value;
+
+use crate::{EngineError, Result, Session};
+
+/// Text of a panic payload (`panic!` with a literal or a formatted message).
+pub fn panic_message(payload: &(dyn std::any::Any + Send)) -> String {
+    payload
+        .downcast_ref::<&str>()
+        .map(|s| s.to_string())
+        .or_else(|| payload.downcast_ref::<String>().cloned())
+        .unwrap_or_else(|| "unknown panic".into())
+}
+
+/// Log every panic (message and location) before it unwinds, then run the previous hook.
+/// Installed once per process, when the first [`Session`] is made.
+pub fn install_panic_hook() {
+    static ONCE: std::sync::Once = std::sync::Once::new();
+    ONCE.call_once(|| {
+        let previous = std::panic::take_hook();
+        std::panic::set_hook(Box::new(move |info| {
+            let at = info.location().map(|l| format!(" at {}:{}", l.file(), l.line())).unwrap_or_default();
+            log::error!("panic{at}: {}", panic_message(info.payload()));
+            previous(info);
+        }));
+    });
+}
+
+impl Session {
+    /// Run command `id` (`f`) inside the guard.
+    pub(crate) fn guarded(&mut self, id: &str, f: impl FnOnce(&mut Session) -> Result<Value>) -> Result<Value> {
+        let before = self.active().map(|d| (d.uid, d.doc.clone(), d.selection.clone(), d.revision));
+        match catch_unwind(AssertUnwindSafe(|| f(self))) {
+            Ok(r) => r,
+            Err(payload) => {
+                let msg = panic_message(payload.as_ref());
+                // Keep the document as it was before the command.
+                if let (Some((uid, doc, selection, revision)), Some(st)) = (before, self.active_mut())
+                    && st.uid == uid
+                {
+                    st.doc = doc;
+                    st.selection = selection;
+                    st.interaction = None;
+                    st.revision = revision.max(st.revision).saturating_add(1);
+                }
+                if let Some(dir) = self.recovery_dir.clone()
+                    && let Err(e) = crate::recovery::save(self, &dir)
+                {
+                    log::error!("crash recovery save after `{id}` failed: {e}");
+                }
+                log::error!("command `{id}` panicked: {msg}");
+                Err(EngineError::Internal(id.to_string(), msg))
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use serde_json::json;
+
+    use crate::Session;
+
+    #[test]
+    fn a_panicking_command_is_an_error_and_keeps_the_document() {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({})).unwrap();
+        s.execute("frame.create", &json!({"rect": [36, 36, 200, 200], "content": "text", "text": "keep me"})).unwrap();
+        let before = s.active().unwrap().doc.clone();
+        // A command that edits the document and then panics half way.
+        let err = s
+            .guarded("test.panic", |s| {
+                s.execute("frame.create", &json!({"rect": [36, 236, 200, 400]}))?;
+                panic!("boom");
+            })
+            .unwrap_err();
+        assert!(err.to_string().contains("test.panic") && err.to_string().contains("boom"), "{err}");
+        let st = s.active().unwrap();
+        assert!(std::sync::Arc::ptr_eq(&st.doc, &before), "the document is the one from before the command");
+        // The session keeps working.
+        s.execute("frame.create", &json!({"rect": [36, 236, 200, 400]})).unwrap();
+    }
+}

crates/engine/src/lib.rs

@@ -9,6 +9,7 @@
 
 pub mod cmd;
 pub mod dtd;
+pub mod guard;
 pub mod links;
 pub mod math;
 pub mod recovery;
@@ -40,6 +41,9 @@ pub enum EngineError {
     NoDocument,
     #[error("{0}")]
     Other(String),
+    /// A command panicked; the guard kept the document (a bug: please report it).
+    #[error("internal error in `{0}` (the document was kept as it was): {1}")]
+    Internal(String, String),
 }
 
 impl From<designcraft_doc::DocError> for EngineError {
@@ -254,6 +258,7 @@ impl Default for Session {
 
 impl Session {
     pub fn new() -> Self {
+        guard::install_panic_hook();
         Session {
             docs: vec![],
             active: None,
@@ -341,7 +346,13 @@ impl Session {
     }
 
     /// Run a command by id. Edits push one undo step (unless inside an interaction).
+    /// Run command `id`. A panic inside it becomes [`EngineError::Internal`] and leaves the
+    /// document as it was (see [`guard`]).
     pub fn execute(&mut self, id: &str, params: &Value) -> Result<Value> {
+        self.guarded(id, |s| s.execute_unguarded(id, params))
+    }
+
+    fn execute_unguarded(&mut self, id: &str, params: &Value) -> Result<Value> {
         let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.into()))?;
         (spec.enabled)(self).map_err(|e| EngineError::Disabled(id.into(), e))?;
         let before = self.active().map(|d| (d.uid, d.doc.clone()));