From 361fff982a3e90fe94b4520b42d74726b8eca9ef Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:02:18 -0400 Subject: [PATCH] Engine: a dry run journals nothing, so the journal and Record Actions' start stay as they were Every nested command a dry run executed was journaled. Past the journal's limits that evicted the person's own oldest entries, and the restore could only cut back what was left, so one `macro.check` of a long macro could empty the journal and move a Record Actions recording's start. While `Saved` holds the session's state, commands are not journaled (`Session::journal_paused`), and the restore puts the journal and the recording start back exactly; the count of dropped entries it relied on is gone. The fallible calls of a dry run come before the pause, and a panic at the top level turns journaling back on. - Test: `a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were` (a full journal, a Record Actions start, and a check of 12,000 nested steps; replaces the test that accepted the loss). --- crates/engine/src/lib.rs | 18 ++++++++--------- crates/engine/src/macros/run.rs | 26 ++++++++++++------------- crates/engine/src/tests/macros.rs | 32 ++++++++++++++----------------- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 16ab84aab..daea7f7e8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -445,9 +445,9 @@ pub struct Session { /// Serialized size of each journal entry, measured once when it is pushed, and their sum. journal_sizes: Vec, journal_bytes: usize, - /// Entries the journal has dropped from its front to stay within its limits, ever; a dry run - /// that puts the journal back needs to know how many went while it ran. - journal_dropped: u64, + /// While a dry run runs (the footprint, `macro.check`), its commands are not journaled: the + /// dry run is put back, and journaling it would evict the person's own oldest entries. + pub(crate) journal_paused: u32, pub ui_requests: Vec, /// Format painter source style (sticky when double-clicked). pub format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -572,16 +572,10 @@ impl Session { self.journal.drain(..n); self.journal_sizes.drain(..n); self.journal_bytes = kept; - self.journal_dropped = self.journal_dropped.saturating_add(n as u64); self.recording = self.recording.map(|r| r.saturating_sub(n)); } } - /// Entries dropped from the journal's front so far, for a caller that puts the journal back. - pub(crate) fn journal_dropped(&self) -> u64 { - self.journal_dropped - } - /// Cuts the journal back to its first `keep` entries, with its size bookkeeping. pub(crate) fn truncate_journal(&mut self, keep: usize) { if self.journal_sizes.len() == self.journal.len() { @@ -675,6 +669,8 @@ impl Session { if self.depth == 0 { self.trim_history(); self.group_step = None; + // A dry run the panic cut short never put journaling back on. + self.journal_paused = 0; } return Err(EngineError::Internal(id.to_string(), msg)); } @@ -700,7 +696,9 @@ impl Session { { rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params: cmd::journal_params(¶ms) }); } - self.push_journal(id, cmd::journal_params(¶ms)); + if self.journal_paused == 0 { + self.push_journal(id, cmd::journal_params(¶ms)); + } } // Inside an undo group the group owns the step. A command that ended a group open before // it leaves the group its step and makes one of its own for what it changed after the end. diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 373d61d8b..2cfa37b34 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -1008,10 +1008,9 @@ struct Saved { clipboard: Option, prefs: crate::Prefs, mode: crate::Mode, - /// The journal's length and how many entries it had dropped from its front; a dry run that - /// pushes past the journal's limits drops more, which a length alone cannot see. + /// The journal's length: the dry run journals nothing (`Session::journal_paused`), so nothing + /// of the person's is evicted, and the length puts back exactly what was there. journal: usize, - journal_dropped: u64, recording: Option, ui_requests: usize, format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -1024,9 +1023,10 @@ struct Saved { } impl Saved { + /// Takes the state and stops journaling until [`Saved::restore`]. fn take(s: &mut Session, doc: u64) -> Saved { let d = s.docs.iter().find(|d| d.uid == doc); - Saved { + let saved = Saved { active: s.active, docs: s.docs.iter().map(|d| d.uid).collect(), saved: d.map(|d| d.saved.clone()), @@ -1039,7 +1039,6 @@ impl Saved { prefs: s.prefs.clone(), mode: s.mode, journal: s.journal.len(), - journal_dropped: s.journal_dropped(), recording: s.recording, ui_requests: s.ui_requests.len(), format_painter: s.format_painter.clone(), @@ -1049,7 +1048,9 @@ impl Saved { draw: (s.draw_tool.clone(), s.draw_color.clone(), s.draw_width), comments_pane: s.comments_pane, show_all_notes: s.show_all_notes, - } + }; + s.journal_paused = s.journal_paused.saturating_add(1); + saved } fn restore(self, s: &mut Session, doc: u64) { @@ -1070,11 +1071,9 @@ impl Saved { s.clipboard = self.clipboard; s.prefs = self.prefs; s.mode = self.mode; - // Entries the dry run pushed go; entries the journal dropped meanwhile are gone for good, - // and the recording's start moves back with them. - let dropped = usize::try_from(s.journal_dropped().saturating_sub(self.journal_dropped)).unwrap_or(usize::MAX); - s.truncate_journal(self.journal.saturating_sub(dropped)); - s.recording = self.recording.map(|r| r.saturating_sub(dropped)); + s.truncate_journal(self.journal); + s.recording = self.recording; + s.journal_paused = s.journal_paused.saturating_sub(1); s.ui_requests.truncate(self.ui_requests); s.format_painter = self.format_painter; s.last_find = self.last_find; @@ -1092,9 +1091,10 @@ impl Saved { pub fn footprint(s: &mut Session, p: &Prepared, run: &Machine) -> Result { let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?; s.set_active(i); - let saved = Saved::take(s, p.doc); let before = s.doc()?.wb.clone(); let token = s.group_begin("Check Macro")?; + // After the calls that can fail, so nothing returns between taking and putting back. + let saved = Saved::take(s, p.doc); if let Some(d) = s.docs.iter_mut().find(|d| d.uid == p.doc) { d.touched = Some(Vec::new()); } @@ -1498,8 +1498,8 @@ fn conflict_rule(p: &Json, m: &Macro) -> Result { /// For `macro.check`, as the run would: shifts the data, checks the footprint again, and puts /// everything back. What still stands in the way, or `None`. fn shifted_conflicts(s: &mut Session, fp: &Footprint, p: &Prepared, budget: &Machine, down: bool) -> Result> { - let saved = Saved::take(s, p.doc); let token = s.group_begin("Check Macro")?; + let saved = Saved::take(s, p.doc); let mut spent = budget.clone(); (spent.executed, spent.param_bytes) = (fp.executed, fp.param_bytes); let again = make_room(s, fp, p, down).and_then(|()| footprint(s, p, &spent)); diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index d94494ba5..5df8faa11 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -418,33 +418,29 @@ fn a_dry_run_leaves_a_clean_workbook_clean() { } #[test] -fn a_dry_run_puts_the_journal_back_even_after_it_trimmed_it() { - // Every nested command a dry run executes is journaled, so a long macro pushes the journal - // past its limit and the journal drops its oldest entries. Putting the journal back by its - // old length would then keep entries from the dry run and point a recording at the wrong - // place; the restore counts what was dropped. +fn a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were() { + // A dry run journals nothing: journaling its nested commands would push a full journal past + // its limit and evict the person's own oldest entries, and move Record Actions' start. let mut s = s(); data(&mut s); - s.execute("selection.set", json!({"range": "A1"})).unwrap(); - let journal = s.journal.clone(); - s.recording = Some(journal.len().saturating_sub(1)); + for i in 0..crate::JOURNAL_MAX_ENTRIES { + s.execute("selection.set", json!({"range": format!("A{}", i % 50 + 1)})).unwrap(); + } + s.recording = Some(s.journal.len().saturating_sub(100)); put( &mut s, json!({"name": "Long", "steps": [{"type": "repeat", "over": "A1:A200", "body": [ {"type": "repeat", "over": "A1:A60", "body": [{"command": "cell.set", "params": {"cell": "Row.cell(1,1)", "input": "x"}}]} ]}]}), ); - s.execute("macro.check", json!({"name": "Long"})).unwrap(); - let sets = |j: &[(String, Json)]| j.iter().filter(|(id, _)| id == "cell.set").count(); - assert!(sets(&s.journal) <= sets(&journal), "no dry-run entry stays"); - assert!(s.journal.len() <= journal.len(), "{} entries, had {}", s.journal.len(), journal.len()); - assert!(journal.ends_with(&s.journal), "what stays is the journal's own tail"); - assert!(s.recording.is_some_and(|r| r <= s.journal.len()), "{:?}", s.recording); - // A short dry run leaves the journal exactly as it was. - put(&mut s, json!({"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "H1", "input": "x"}}]})); let (journal, recording) = (s.journal.clone(), s.recording); - s.execute("macro.check", json!({"name": "Short"})).unwrap(); - assert_eq!((&s.journal, s.recording), (&journal, recording)); + let c = s.execute("macro.check", json!({"name": "Long"})).unwrap(); + assert!(c.get("footprint").is_some(), "{c}"); + assert_eq!((s.journal.len(), s.recording), (journal.len(), recording)); + assert!(s.journal == journal, "the journal is as it was"); + // The check itself is a query and isn't journaled; a command after it is. + s.execute("selection.set", json!({"range": "B1"})).unwrap(); + assert_eq!(s.journal.last().map(|(id, _)| id.as_str()), Some("selection.set")); } #[test]