From 712bb766ecdf8063d28f74f94303bd5efa63ce6e Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:46:25 -0400 Subject: [PATCH] Engine: a grouped action that panics is rolled back before the panic goes on Session::grouped ran its action without a panic guard, so a caller outside a command that caught the panic found the group still open: Undo and Redo disabled, the partial edits kept, and the next grouped action nested in the abandoned group. The group is now rolled back and the panic resumed. Regression test. --- crates/engine/src/lib.rs | 12 +++++++++--- crates/engine/src/tests/seams.rs | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 09e39523b..8a8d8bcfa 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -680,20 +680,26 @@ impl Session { pub fn grouped(&mut self, label: &str, f: impl FnOnce(&mut Session) -> Result) -> Result { let token = self.group_begin(label)?; let doc = token.doc(); - match f(self) { - Ok(r) => match self.group_end(token) { + // A panic in `f` rolls the group back before it goes on, so a caller that catches it + // (outside `execute`, which drops a command's groups itself) finds no group left open. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))) { + Ok(Ok(r)) => match self.group_end(token) { Ok(_) => Ok(r), // `f` closed the group's document (`file.close`, or `file.open` taking the place of // an untouched new workbook), and the group went with it: nothing is left to undo. Err(_) if !self.docs.iter().any(|d| d.uid == doc) => Ok(r), Err(e) => Err(e), }, - Err(e) => { + Ok(Err(e)) => { // The error from `f` is the one to report. A rollback fails only when the group // has already ended, and then there is nothing left to undo. let _ = self.group_rollback(token); Err(e) } + Err(panic) => { + let _ = self.group_rollback(token); + std::panic::resume_unwind(panic) + } } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 48105da25..0f1bc3072 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -579,6 +579,31 @@ fn a_command_that_panics_leaves_the_history_as_it_found_it() { assert_eq!(v(&s, "A1").display(), "1"); } +#[test] +fn a_grouped_action_that_panics_is_rolled_back_before_the_panic_goes_on() { + // Outside a command, a caller that catches the panic finds the group rolled back and no + // group left open, so Undo is enabled and the next grouped action does not nest. + let mut s = s(); + set(&mut s, "A1", "before"); + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + s.grouped::<()>("Doomed", |s| { + set(s, "A1", "half done"); + panic!("inside the group"); + }) + })); + assert!(caught.is_err(), "the panic goes on to the caller"); + let uid = s.doc().unwrap().uid; + assert!(!s.in_group(uid)); + assert_eq!(v(&s, "A1").display(), "before"); + assert_eq!(undo_labels(&s), ["Enter Cell"]); + s.grouped("Next", |s| { + set(s, "A1", "after"); + Ok(()) + }) + .unwrap(); + assert_eq!(undo_labels(&s), ["Enter Cell", "Next"]); +} + #[test] fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() { // The command takes a token the test left for it, ends or rolls back that group, and panics.