Merge pull request #983 from Silvertree2010/fix/908-audit-panic-rollback-is-partial

Restore all engine side-effect state when a command panics

crates/engine/src/cmd/mod.rs

@@ -38,6 +38,27 @@ mod view_more;
 mod vst3;
 mod zoom_toggle;
 
+#[cfg(test)]
+mod panic_probe {
+    use super::*;
+    use crate::cmd;
+
+    pub fn specs() -> Vec<CommandSpec> {
+        vec![cmd!(noundo "test.panic_probe", "Panic Probe", [], None, "{}", always, probe)]
+    }
+
+    fn probe(e: &mut Engine, p: &Value) -> Result<Value> {
+        if p.get("panic").is_none() {
+            return Ok(Value::Null);
+        }
+        e.clipboard.length = 123;
+        e.message("probe");
+        e.transport_requests.push(crate::TransportRequest::Play);
+        e.last_selection = Some((soundcraft_time::Range::new(0, 10), Vec::new()));
+        panic!("probe")
+    }
+}
+
 pub use file::bounce_job;
 
 pub(crate) use edit::remember_selection;
@@ -136,6 +157,8 @@ pub fn command_specs() -> &'static [CommandSpec] {
         v.extend(sampler::specs());
         v.extend(video::specs());
         v.extend(zoom_toggle::specs());
+        #[cfg(test)]
+        v.extend(panic_probe::specs());
         v
     })
 }

crates/engine/src/lib.rs

@@ -316,6 +316,7 @@ impl Engine {
         let before = Arc::clone(&self.doc);
         let rev = self.revision;
         let dirty = self.dirty;
+        let side_effects = (self.clipboard.clone(), self.messages.clone(), self.transport_requests.clone(), self.last_selection.clone());
         let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| (spec.run)(self, params)));
         let result = match result {
             Ok(r) => r,
@@ -323,6 +324,7 @@ impl Engine {
                 self.doc = before;
                 self.revision = rev.wrapping_add(1);
                 self.dirty = dirty;
+                (self.clipboard, self.messages, self.transport_requests, self.last_selection) = side_effects;
                 return Err(EngineError::Internal(id.to_string(), panic_message(&*p)));
             }
         };
@@ -526,3 +528,26 @@ mod dirty_tests {
         assert!(!e.is_dirty());
     }
 }
+
+#[cfg(test)]
+mod panic_tests {
+    use super::*;
+
+    #[test]
+    fn a_panicking_command_restores_side_effect_state() {
+        let mut e = Engine::default();
+        // Non-default values, so clearing them instead of restoring them would fail.
+        e.clipboard.length = 42;
+        e.messages.push("prior".into());
+        e.transport_requests.push(TransportRequest::Stop);
+        e.last_selection = Some((soundcraft_time::Range::new(20, 30), Vec::new()));
+        let (clipboard, messages, requests, selection) =
+            (e.clipboard.clone(), e.messages.clone(), e.transport_requests.clone(), e.last_selection.clone());
+        let r = e.execute("test.panic_probe", &json!({"panic": true}));
+        assert!(matches!(r, Err(EngineError::Internal(..))));
+        assert_eq!(e.clipboard, clipboard);
+        assert_eq!(e.messages, messages);
+        assert_eq!(e.transport_requests, requests);
+        assert_eq!(e.last_selection, selection);
+    }
+}