From 2b4f2c588680e093f6e3f293bbdae7136ff92390 Mon Sep 17 00:00:00 2001 From: darthnut <120891176+darthnut@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:50:41 -0700 Subject: [PATCH] Never crash: History is bounded on load and by every op that sets it, not only by a new edit (#2579) HISTORY_LIMIT (200 steps per photo) was applied only by PushHistory. A snapshot, an AddPhoto op (op log replay, undo of a removal) or a SetHistory op could carry any number of steps, and all of them stayed in memory. Photo::trim_history keeps the newest HISTORY_LIMIT steps and moves the History cursor with its step (to the oldest step kept when its own is dropped). repair(), which runs on every snapshot load, applies it and reports it; AddPhoto and SetHistory apply it too, and SetHistory's inverse restores the cursor as well when trimming moved it. Saved versions are unbounded in the same way, but there is no limit for them to apply; left for a maintainer to choose. Item 8 of #2579. Co-Authored-By: Claude Opus 5.5 --- crates/catalog/src/lib.rs | 21 +++++++++++++++++---- crates/catalog/src/model.rs | 11 +++++++++++ crates/catalog/src/tests_journal.rs | 25 +++++++++++++++++++++++++ crates/catalog/src/tests_repair.rs | 27 +++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 4 deletions(-) diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 1776c895c..dccf20ba8 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -669,10 +669,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)); @@ -723,7 +724,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)?; @@ -1022,8 +1030,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 { let mut changed = Vec::new(); for (id, p) in self.photos.iter_mut() { @@ -1031,6 +1040,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 { diff --git a/crates/catalog/src/model.rs b/crates/catalog/src/model.rs index b405e1db9..fbf3294db 100644 --- a/crates/catalog/src/model.rs +++ b/crates/catalog/src/model.rs @@ -354,6 +354,17 @@ pub fn preview_stamp(preview_only: &Option) -> 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 { let last = self.history.len().checked_sub(1)?; diff --git a/crates/catalog/src/tests_journal.rs b/crates/catalog/src/tests_journal.rs index 26b4e902a..d50c0e533 100644 --- a/crates/catalog/src/tests_journal.rs +++ b/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())); diff --git a/crates/catalog/src/tests_repair.rs b/crates/catalog/src/tests_repair.rs index 493c89544..2e083f1e1 100644 --- a/crates/catalog/src/tests_repair.rs +++ b/crates/catalog/src/tests_repair.rs @@ -127,6 +127,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() {