Merge PR #2714: Never crash: History is bounded on load and by every op that sets it (#2579 item 8)

crates/catalog/src/lib.rs

@@ -699,10 +699,11 @@ impl Catalog {
             | Op::SetFolderRecord { .. } => self.changes.broad = self.revision.saturating_add(1),
         }
         Ok(match op {
-            Op::AddPhoto { photo } => {
+            Op::AddPhoto { mut photo } => {
                 if self.photos.contains_key(&photo.id) {
                     return Err(CatalogError::Invalid(format!("photo {:?} exists", photo.id)));
                 }
+                photo.trim_history();
                 let id = photo.id;
                 self.next_photo = self.next_photo.max(id.0.saturating_add(1));
                 self.photos.insert(id, Arc::new(*photo));
@@ -753,7 +754,14 @@ impl Catalog {
             }
             Op::SetHistory { id, history } => {
                 let p = self.photo_mut(id)?;
-                Op::SetHistory { id, history: std::mem::replace(&mut p.history, history) }
+                let old = std::mem::replace(&mut p.history, history);
+                let old_cursor = p.history_cursor;
+                p.trim_history();
+                if p.history_cursor == old_cursor {
+                    Op::SetHistory { id, history: old }
+                } else {
+                    Op::Batch { ops: vec![Op::SetHistory { id, history: old }, Op::SetHistoryCursor { id, cursor: old_cursor }] }
+                }
             }
             Op::PushHistory { id, step } => {
                 let p = self.photo_mut(id)?;
@@ -1054,8 +1062,9 @@ impl Catalog {
     /// finds in them, and walks folders up to the top): every photo, album and stack carries the
     /// id it is filed under; the id counters lie past every id in use; a stack keeps only photos
     /// in the catalog, once each, and goes when fewer than two are left; an album whose parent is
-    /// missing, not a folder or inside itself moves to the top level. Returns what was changed
-    /// (nothing, for a catalog this build wrote).
+    /// missing, not a folder or inside itself moves to the top level; a photo keeps its newest
+    /// [`HISTORY_LIMIT`] History steps. Returns what was changed (nothing, for a catalog this build
+    /// wrote).
     pub fn repair(&mut self) -> Vec<String> {
         let mut changed = Vec::new();
         for (id, p) in self.photos.iter_mut() {
@@ -1063,6 +1072,10 @@ impl Catalog {
                 changed.push(format!("photo {} was filed as {}", p.id.0, id.0));
                 Arc::make_mut(p).id = *id;
             }
+            if p.history.len() > HISTORY_LIMIT {
+                let n = Arc::make_mut(p).trim_history();
+                changed.push(format!("photo {}: {n} History step(s) beyond the newest {HISTORY_LIMIT} dropped", id.0));
+            }
         }
         for (id, a) in self.albums.iter_mut() {
             if a.id != *id {

crates/catalog/src/model.rs

@@ -354,6 +354,17 @@ pub fn preview_stamp(preview_only: &Option<String>) -> String {
 }
 
 impl Photo {
+    /// Keep the newest [`HISTORY_LIMIT`](crate::HISTORY_LIMIT) History steps; the cursor moves with
+    /// its step, or to the oldest step kept when its own is dropped. Returns how many were dropped.
+    pub fn trim_history(&mut self) -> usize {
+        let n = self.history.len().saturating_sub(crate::HISTORY_LIMIT);
+        if n > 0 {
+            self.history.drain(..n);
+            self.history_cursor = self.history_cursor.map(|c| c.saturating_sub(n));
+        }
+        n
+    }
+
     /// The History step the settings are at (the cursor, clamped to the list; `None` = no history).
     pub fn history_index(&self) -> Option<usize> {
         let last = self.history.len().checked_sub(1)?;

crates/catalog/src/tests_journal.rs

@@ -262,6 +262,31 @@ fn push_history_is_bounded_and_invertible() {
     assert_eq!(c.to_snapshot(), before);
 }
 
+/// A photo added with, or given, a whole History (op log replay, undo of a removal) kept every
+/// step: only `PushHistory` applied the limit (#2579).
+#[test]
+fn add_photo_and_set_history_are_bounded_and_invertible() {
+    let mut c = Catalog::new();
+    let id = c.alloc_photo_id();
+    let step = |i: usize| HistoryStep { label: format!("s{i}"), settings: Arc::new(DevelopSettings::default()) };
+    let mut p = Photo::new(id, Source::Demo { scene: 1 }, "a", "JPEG", 1, 1, "t");
+    p.history = (0..HISTORY_LIMIT + 5).map(step).collect();
+    p.history_cursor = Some(10);
+    c.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap();
+    let p = c.photo(id).unwrap();
+    assert_eq!((p.history.len(), p.history[0].label.as_str()), (HISTORY_LIMIT, "s5"));
+    assert_eq!(p.history_cursor, Some(5), "the cursor stays on step s10");
+
+    let before = c.to_snapshot();
+    let inv = c.apply(Op::SetHistory { id, history: (0..HISTORY_LIMIT * 2).map(step).collect() }).unwrap();
+    let p = c.photo(id).unwrap();
+    assert_eq!(p.history.len(), HISTORY_LIMIT);
+    assert_eq!(p.history[0].label, format!("s{HISTORY_LIMIT}"));
+    assert_eq!(p.history_cursor, Some(0), "its step was dropped: the oldest one kept");
+    c.apply(inv).unwrap();
+    assert_eq!(c.to_snapshot(), before);
+}
+
 #[test]
 fn fs_store_roundtrip() {
     let dir = std::env::temp_dir().join(format!("lc-journal-{}", std::process::id()));

crates/catalog/src/tests_repair.rs

@@ -195,6 +195,33 @@ fn id_counters_are_moved_past_the_ids_in_use() {
     assert!(c.apply(Op::AddPhoto { photo: Box::new(p) }).is_err(), "the id is taken: refused, not overwritten");
 }
 
+/// History was bounded only where an edit adds a step, so a snapshot holding more steps kept
+/// every one in memory on each load (#2579). The newest are kept and the cursor stays on its step.
+#[test]
+fn history_beyond_the_limit_is_trimmed_on_load() {
+    let (mut c, ids) = sound();
+    let extra = HISTORY_LIMIT * 5;
+    let step = HistoryStep { label: "s0".into(), settings: std::sync::Arc::new(lightcraft_develop::DevelopSettings::default()) };
+    c.apply(Op::PushHistory { id: ids[0], step }).unwrap();
+    c.apply(Op::SetHistoryCursor { id: ids[0], cursor: Some(extra - 3) }).unwrap();
+    let (loaded, report) = load_edited(&c, |v| {
+        let history = &mut v["photos"][ids[0].0.to_string()]["history"];
+        let one = history[0].clone();
+        *history = (0..extra)
+            .map(|i| {
+                let mut s = one.clone();
+                s["label"] = format!("s{i}").into();
+                s
+            })
+            .collect();
+    });
+    let p = loaded.photo(ids[0]).unwrap();
+    assert_eq!(p.history.len(), HISTORY_LIMIT);
+    assert_eq!(p.history[0].label, format!("s{}", extra - HISTORY_LIMIT));
+    assert_eq!(p.history_index().map(|i| p.history[i].label.clone()), Some(format!("s{}", extra - 3)));
+    assert_eq!(report.repaired.len(), 1, "{:?}", report.repaired);
+}
+
 /// A sound catalog loads unchanged.
 #[test]
 fn a_sound_snapshot_needs_no_repair() {