Merge PR #2761

crates/catalog/src/lib.rs

@@ -284,7 +284,8 @@ pub struct Catalog {
     /// What the library keeps about its folders (folder identity → record), see [`folders`].
     #[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
     folder_records: BTreeMap<String, FolderRecord>,
-    /// Increments on every applied op.
+    /// Increments on every applied op, and when a batch fails and its rollback cannot undo it whole (the catalog
+    /// changed, so what is keyed by the revision is out of date).
     #[serde(skip)]
     pub revision: u64,
     /// Which photos the latest ops touched ([`Catalog::touched_since`]).
@@ -348,6 +349,8 @@ struct Changes {
     /// outside (a library just loaded takes a range of its own, so that nothing kept from another library is
     /// taken for this one's): nothing is known of how the catalog got to where it is.
     seen: u64,
+    /// A batch that failed could not be rolled back whole: what it changed stays, so what it noted must too.
+    torn: bool,
 }
 
 impl PartialEq for Changes {
@@ -610,13 +613,40 @@ impl Catalog {
         self.albums.get_mut(&id).ok_or(CatalogError::NoAlbum(id))
     }
 
-    /// Apply an op; returns its inverse. On error nothing changes.
+    /// Apply an op; returns its inverse. On error the catalog is as it was, the revision included, and nothing is
+    /// remembered of the op in what the latest ops touched. The one exception is a batch whose rollback fails: the
+    /// catalog did change, so the revision advances and anything may be taken to have changed.
     pub fn apply(&mut self, op: Op) -> Result<Op> {
         if self.changes.seen != self.revision {
             // the revision was set from outside: what is remembered is of another past
             self.changes = Changes { floor: self.revision, seen: self.revision, ..Default::default() };
         }
-        let inv = self.apply_inner(op)?;
+        let broad = self.changes.broad;
+        self.changes.torn = false;
+        let inv = match self.apply_inner(op) {
+            Ok(inv) => inv,
+            Err(e) => {
+                let made = self.revision.saturating_add(1);
+                if self.changes.torn {
+                    // (a batch the rollback of which failed changed something and says not what: it makes a
+                    // revision, so what is kept for the one before is dropped, and anything may have changed, as
+                    // after an op of a wider reach)
+                    self.changes.torn = false;
+                    self.changes.broad = made;
+                    self.revision = made;
+                    self.changes.seen = made;
+                } else {
+                    // (everything the failed op, or the rollback of a batch, noted was made for the revision it
+                    // would have made: nothing earlier has that one. A note the cap already dropped stays
+                    // dropped, which only says less than is known, as the cap does.)
+                    while self.changes.touched.back().is_some_and(|(r, _, _)| *r == made) {
+                        self.changes.touched.pop_back();
+                    }
+                    self.changes.broad = broad;
+                }
+                return Err(e);
+            }
+        };
         self.revision += 1;
         self.changes.seen = self.revision;
         Ok(inv)
@@ -962,7 +992,9 @@ impl Catalog {
                         Err(e) => {
                             // roll back what was applied
                             for inv in inverses.into_iter().rev() {
-                                let _ = self.apply_inner(inv);
+                                if self.apply_inner(inv).is_err() {
+                                    self.changes.torn = true;
+                                }
                             }
                             return Err(e);
                         }

crates/catalog/src/tests.rs

@@ -952,12 +952,13 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them()
     c.apply(Op::SetRating { id: a, rating: 1 }).unwrap();
     assert_eq!(c.touched_since(loaded), Some(vec![a]));
     assert_eq!(c.touched_since(old), None);
-    // an op that fails changes nothing, and what is remembered never says less than what happened
+    // an op that fails changes nothing and is not remembered
     let before = c.revision;
     assert!(c.apply(Op::SetRating { id: PhotoId(9_999), rating: 1 }).is_err());
     assert_eq!(c.revision, before);
+    assert_eq!(c.touched_since(before), Some(vec![]));
     c.apply(Op::SetFlag { id: a, flag: Flag::Pick }).unwrap();
-    assert!(c.touched_since(before).is_some_and(|ids| ids.contains(&a)));
+    assert_eq!(c.touched_since(before), Some(vec![a]));
     assert!(c.aspects_since(before).is_some_and(|x| x.any_of(Aspects::FLAG)));
     // a batch that fails midway is undone as a whole; a batch in a batch is remembered photo by photo; a batch
     // with an op that changes more than photos is not
@@ -965,8 +966,8 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them()
     assert!(c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 5 }, Op::SetRating { id: PhotoId(9_999), rating: 1 }] }).is_err());
     assert_eq!((c.revision, c.photo(a).map(|p| p.rating)), (before, Some(1)));
     c.apply(Op::Batch { ops: vec![Op::Batch { ops: vec![Op::SetRating { id: a, rating: 2 }] }] }).unwrap();
-    // (the photo the failed batch asked for and never found may be named too: more is said, never less)
-    assert!(c.touched_since(before).is_some_and(|ids| ids.contains(&a)));
+    // (the failed batch is not remembered: only the batch that worked)
+    assert_eq!(c.touched_since(before), Some(vec![a]));
     let before = c.revision;
     c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 3 }, Op::RenameAlbum { id: album, name: "Journey".into() }] }).unwrap();
     assert_eq!(c.touched_since(before), None);
@@ -978,3 +979,70 @@ fn the_catalog_remembers_which_photos_its_latest_ops_changed_and_what_of_them()
     assert_eq!(c.touched_since(long_ago), None);
     assert_eq!(c.touched_since(c.revision - 10), Some(vec![a]));
 }
+
+/// An op that fails leaves nothing behind in what the catalog remembers of its ops: no photo it never changed (one
+/// that does not even exist), and, for a batch rolled back, none of what its first ops had touched.
+#[test]
+fn an_op_that_fails_leaves_no_touched_photos_behind() {
+    let mut c = Catalog::default();
+    let add = |c: &mut Catalog, name: &str| {
+        let id = c.alloc_photo_id();
+        c.apply(Op::AddPhoto {
+            photo: Box::new(Photo::new(id, Source::File { path: format!("/p/{name}") }, name, "JPEG", 60, 40, "2026-01-01T10:00:00")),
+        })
+        .unwrap();
+        id
+    };
+    let (a, b) = (add(&mut c, "a.jpg"), add(&mut c, "b.jpg"));
+    let missing = PhotoId(9_999);
+    let start = c.revision;
+    // every op that names a photo, on one that does not exist
+    for op in [Op::SetRating { id: missing, rating: 1 }, Op::SetFlag { id: missing, flag: Flag::Pick }, Op::RemovePhoto { id: missing }] {
+        assert!(c.apply(op).is_err());
+        assert_eq!(c.revision, start, "a failed op makes no revision");
+        assert_eq!(c.touched_since(start), Some(vec![]));
+        assert_eq!(c.aspects_since(start), Some(Aspects::NONE));
+    }
+    // an op that changes more than photos and fails does not make earlier revisions unknown
+    assert!(c.apply(Op::RenameAlbum { id: AlbumId(77), name: "x".into() }).is_err());
+    assert_eq!(c.touched_since(start), Some(vec![]));
+    // a batch that fails on its second op names nothing of its first, nor of the rollback
+    assert!(c.apply(Op::Batch { ops: vec![Op::SetRating { id: a, rating: 5 }, Op::SetRating { id: missing, rating: 1 }] }).is_err());
+    assert_eq!(c.revision, start);
+    assert_eq!(c.touched_since(start), Some(vec![]));
+    assert_eq!(c.aspects_since(start), Some(Aspects::NONE));
+    assert_eq!(c.photo(a).map(|p| p.rating), Some(0));
+    // a batch that ends in a failed removal of a photo that exists
+    assert!(c.apply(Op::Batch { ops: vec![Op::RemovePhoto { id: b }, Op::RemovePhoto { id: b }] }).is_err());
+    assert!(c.photo(b).is_some());
+    assert_eq!(c.touched_since(start), Some(vec![]));
+    // what succeeded after the failures is remembered, and only that
+    c.apply(Op::SetRating { id: a, rating: 2 }).unwrap();
+    assert_eq!(c.touched_since(start), Some(vec![a]));
+    assert_eq!(c.aspects_since(start), Some(Aspects::RATING));
+}
+
+/// A batch the rollback of which fails halfway leaves the catalog changed (here a stack is lost), so what the
+/// catalog remembers must not say that nothing changed: anything may have.
+#[test]
+fn a_batch_that_cannot_be_rolled_back_whole_makes_earlier_changes_unknown() {
+    let mut c = Catalog::default();
+    let mut ids = Vec::new();
+    for name in ["a.jpg", "b.jpg"] {
+        let id = c.alloc_photo_id();
+        let photo = Photo::new(id, Source::File { path: format!("/p/{name}") }, name, "JPEG", 60, 40, "2026-01-01T10:00:00");
+        c.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap();
+        ids.push(id);
+    }
+    let stack = c.alloc_stack_id();
+    c.apply(Op::AddStack { stack: Stack { id: stack, photos: ids.clone(), collapsed: false } }).unwrap();
+    let start = c.revision;
+    // the photo goes, then its stack, then an op fails: putting the stack back needs the photo, which comes later
+    let failing =
+        Op::Batch { ops: vec![Op::RemovePhoto { id: ids[0] }, Op::RemoveStack { id: stack }, Op::SetRating { id: PhotoId(9_999), rating: 1 }] };
+    assert!(c.apply(failing).is_err());
+    assert_eq!(c.revision, start + 1, "the catalog changed, so what is kept for the revision before is out of date");
+    assert!(c.photo(ids[0]).is_some() && c.stack(stack).is_none(), "the rollback went only part of the way");
+    assert_eq!(c.touched_since(start), None, "so nothing is said of what changed");
+    assert_eq!(c.touched_since(c.revision), Some(vec![]), "from after it, it is known again");
+}