Merge #331: fix(recovery): keep the version history and branches in recovery copies (#185) (@vxMacDev)

apps/figcraft/src/recovery.rs

@@ -22,7 +22,8 @@ use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex, MutexGuard};
 use std::thread::JoinHandle;
 
-use figcraft_engine::Session;
+use figcraft_engine::{DocState, Session};
+use figcraft_format::{Branch, Version};
 use serde_json::json;
 
 /// Seconds between copies of a document that keeps changing.
@@ -52,10 +53,10 @@ pub struct Recovery {
 /// The state of the copy of one document: only a write that succeeded counts as a copy.
 #[derive(Default)]
 struct Copy {
-    /// The revision whose copy was written.
-    done: Option<u64>,
-    /// The write in flight: the revision it copies, and whether it succeeded once it ends.
-    flight: Option<(u64, JoinHandle<bool>)>,
+    /// The [`stamp`] whose copy was written.
+    done: Option<Vec<u64>>,
+    /// The write in flight: the stamp it copies, and whether it succeeded once it ends.
+    flight: Option<(Vec<u64>, JoinHandle<bool>)>,
 }
 
 impl Copy {
@@ -68,16 +69,16 @@ impl Copy {
 
     /// Wait for the write in flight and take in its result.
     fn join(&mut self) {
-        if let Some((revision, h)) = self.flight.take()
+        if let Some((stamp, h)) = self.flight.take()
             && h.join().unwrap_or(false)
         {
-            self.done = Some(revision);
+            self.done = Some(stamp);
         }
     }
 
-    /// Whether `revision` has neither been copied nor is being copied.
-    fn pending(&self, revision: u64) -> bool {
-        self.done != Some(revision) && self.flight.as_ref().is_none_or(|f| f.0 != revision)
+    /// Whether `stamp` has neither been copied nor is being copied.
+    fn pending(&self, stamp: &[u64]) -> bool {
+        self.done.as_deref() != Some(stamp) && self.flight.as_ref().is_none_or(|f| f.0 != stamp)
     }
 }
 
@@ -132,7 +133,7 @@ impl Recovery {
 
     /// Whether a document with unsaved changes has a revision that is neither copied nor being copied.
     fn pending(&self, s: &Session) -> bool {
-        s.docs.iter().any(|d| needs_copy(d) && self.written.get(&d.uid).is_none_or(|w| w.pending(d.revision)))
+        s.docs.iter().any(|d| needs_copy(s, d) && self.written.get(&d.uid).is_none_or(|w| w.pending(&stamp(s, d))))
     }
 
     /// Remove earlier runs' copies whose outcome is known, once what they became is copied here.
@@ -162,7 +163,7 @@ impl Recovery {
         for d in &s.docs {
             let path = self.dir.join(format!("{}.figcraft", d.uid));
             let note = path.with_extension(ONLINE);
-            if !needs_copy(d) {
+            if !needs_copy(s, d) {
                 if let Some(mut w) = self.written.remove(&d.uid) {
                     // A write still running would bring the copy back after it was removed.
                     w.join();
@@ -179,17 +180,19 @@ impl Recovery {
                 entry.reap();
             }
             // A write still in flight (for any revision) is waited for; a failed one is tried again.
-            if entry.done == Some(d.revision) || entry.flight.is_some() {
+            let stamp = stamp(s, d);
+            if entry.done.as_ref() == Some(&stamp) || entry.flight.is_some() {
                 continue;
             }
             let doc = d.doc.clone();
+            let (versions, branches) = (d.versions.clone(), figcraft_engine::branches::current(s, d.uid));
             // Which online file it is and what of the server's this very copy has (a line each:
             // the server, the file's id, then the session, client id and last change), while it
             // is one. Written after the copy, never before: a note must not promise more of the
             // server's changes than its copy holds.
             let online = s.online_of(d.uid).map(|(server, file, at)| format!("{server}\n{file}\n{} {} {}", at.session, at.client_id, at.seq));
             let job = move || {
-                if !write_atomic(&path, &doc) {
+                if !write_atomic(&path, &doc, &versions, &branches) {
                     return false;
                 }
                 let _ = match online {
@@ -198,13 +201,13 @@ impl Recovery {
                 };
                 true
             };
-            // The revision counts as copied only once its write succeeded.
+            // The stamp counts as copied only once its write succeeded.
             if wait {
                 if job() {
-                    entry.done = Some(d.revision);
+                    entry.done = Some(stamp);
                 }
             } else {
-                entry.flight = std::thread::Builder::new().name("recovery".into()).spawn(job).ok().map(|h| (d.revision, h));
+                entry.flight = std::thread::Builder::new().name("recovery".into()).spawn(job).ok().map(|h| (stamp, h));
             }
         }
         // Closed documents.
@@ -224,7 +227,7 @@ impl Recovery {
     /// Test hook: pretend a write of `uid` at `revision` is in flight in `writer`.
     #[cfg(test)]
     pub fn hold(&mut self, uid: u64, revision: u64, writer: JoinHandle<bool>) {
-        self.written.insert(uid, Copy { done: None, flight: Some((revision, writer)) });
+        self.written.insert(uid, Copy { done: None, flight: Some((vec![revision], writer)) });
     }
 
     /// A normal quit: the documents were saved or their changes discarded on purpose. (Earlier
@@ -256,15 +259,27 @@ fn rejoin_online(file: &Path, s: &mut Session) -> Option<u64> {
     s.execute("collab.recoverFile", &params).ok()?.get("session")?.as_u64()
 }
 
-/// A document whose changes would be lost (a branch tab's edits are in its main file).
-fn needs_copy(d: &figcraft_engine::DocState) -> bool {
-    d.is_dirty() && d.branch_of.is_none()
+/// A file whose changes would be lost: in its document, its history or one of its branch tabs
+/// (a branch tab's edits are saved with its main file, so the tab has no copy of its own).
+fn needs_copy(s: &Session, d: &DocState) -> bool {
+    d.branch_of.is_none() && (d.is_dirty() || branch_tabs(s, d.uid).any(DocState::is_dirty))
 }
 
-/// Write `doc` to `path` through a temporary file; false when it couldn't be written (the copy
-/// before it is still there).
-fn write_atomic(path: &Path, doc: &figcraft_engine::doc::Document) -> bool {
-    let Ok(bytes) = figcraft_format::save_bytes(doc, None) else { return false };
+/// The tabs showing branches of the file in tab `uid`.
+fn branch_tabs(s: &Session, uid: u64) -> impl Iterator<Item = &DocState> {
+    s.docs.iter().filter(move |t| t.branch_of.as_ref().is_some_and(|(main, _)| *main == uid))
+}
+
+/// The revisions a copy of `d` is of: its own and its branch tabs' (a branch tab's edits change
+/// only its own revision). A new copy is due when they change.
+fn stamp(s: &Session, d: &DocState) -> Vec<u64> {
+    std::iter::once(d.revision).chain(branch_tabs(s, d.uid).map(|t| t.revision)).collect()
+}
+
+/// Write `doc` with its version history and branches to `path` through a temporary file; false
+/// when it couldn't be written (the copy before it is still there).
+fn write_atomic(path: &Path, doc: &figcraft_engine::doc::Document, versions: &[Version], branches: &[Branch]) -> bool {
+    let Ok(bytes) = figcraft_format::save_bytes_with_history(doc, None, versions, branches) else { return false };
     let tmp = path.with_extension("tmp");
     std::fs::write(&tmp, bytes).is_ok() && std::fs::rename(&tmp, path).is_ok()
 }

apps/figcraft/src/recovery_tests.rs

@@ -63,6 +63,71 @@ fn copies_follow_saves_and_a_normal_quit_leaves_nothing() {
     let _ = std::fs::remove_dir_all(&root);
 }
 
+/// The rectangles in `doc`.
+fn rectangles(doc: &figcraft_engine::doc::Document) -> usize {
+    doc.nodes().filter(|n| matches!(n.kind, figcraft_engine::doc::NodeKind::Rectangle)).count()
+}
+
+/// The documents a crashed run's copies under `root` open as.
+fn crash_and_recover(r: Recovery, root: &std::path::Path) -> Session {
+    drop(r);
+    let mut next = Session::empty();
+    assert_eq!(recover(root, &mut next), 1);
+    next
+}
+
+#[test]
+fn copies_keep_the_version_history_and_branches() {
+    // #185: a copy has what a save would write, the latest edits on a branch tab included.
+    let root = root("history");
+    let mut s = edited();
+    s.execute("file.saveVersion", &json!({"name": "v1"})).unwrap();
+    s.execute("branch.create", &json!({"name": "Explore"})).unwrap();
+    s.execute("shape.rectangle", &json!({"x": 20, "y": 0, "width": 10, "height": 10})).unwrap();
+    let mut r = Recovery::start(&root).unwrap();
+    r.write(&s, true);
+    let next = crash_and_recover(r, &root);
+    let ds = next.docs.first().unwrap();
+    assert_eq!(ds.versions.iter().map(|v| v.name.as_str()).collect::<Vec<_>>(), ["v1"]);
+    assert_eq!(ds.branches.iter().map(|b| (b.name.as_str(), rectangles(&b.head))).collect::<Vec<_>>(), [("Explore", 2)]);
+    let _ = std::fs::remove_dir_all(&root);
+}
+
+/// The one copy under `root` (the run's folder), read back.
+fn copy(root: &std::path::Path) -> figcraft_format::Loaded {
+    let dir = std::fs::read_dir(root).unwrap().flatten().next().unwrap().path();
+    let file = std::fs::read_dir(dir).unwrap().flatten().map(|f| f.path()).find(|f| f.extension().is_some_and(|e| e == "figcraft")).unwrap();
+    figcraft_format::load_bytes_with_versions(&std::fs::read(file).unwrap()).unwrap()
+}
+
+#[test]
+fn a_change_only_on_a_branch_tab_or_to_the_history_is_copied() {
+    let root = root("history-only");
+    let mut s = edited();
+    s.execute("file.saveVersion", &json!({"name": "v1"})).unwrap();
+    s.execute("branch.create", &json!({"name": "Explore"})).unwrap();
+    for d in &mut s.docs {
+        d.mark_saved();
+    }
+    let mut r = Recovery::start(&root).unwrap();
+    // An edit on the branch tab only: the main file has no changes of its own yet.
+    s.execute("shape.rectangle", &json!({"x": 20, "y": 0, "width": 10, "height": 10})).unwrap();
+    r.write(&s, true);
+    assert_eq!(copy(&root).branches.iter().map(|b| rectangles(&b.head)).collect::<Vec<_>>(), [2]);
+    s.execute("shape.rectangle", &json!({"x": 40, "y": 0, "width": 10, "height": 10})).unwrap();
+    r.write(&s, true);
+    assert_eq!(copy(&root).branches.iter().map(|b| rectangles(&b.head)).collect::<Vec<_>>(), [3]);
+    // A version renamed on the main file leaves its document as it was.
+    s.execute("doc.switch", &json!({"index": 0})).unwrap();
+    r.write(&s, true);
+    let id = s.docs[0].versions[0].id.clone();
+    s.execute("version.rename", &json!({"id": id, "name": "v1 (renamed)"})).unwrap();
+    r.write(&s, true);
+    assert_eq!(copy(&root).versions.iter().map(|v| v.name.as_str()).collect::<Vec<_>>(), ["v1 (renamed)"]);
+    r.finish();
+    let _ = std::fs::remove_dir_all(&root);
+}
+
 fn slow_write(path: std::path::PathBuf) -> std::thread::JoinHandle<bool> {
     std::thread::spawn(move || {
         std::thread::sleep(std::time::Duration::from_millis(150));

crates/engine/src/branches.rs

@@ -156,6 +156,22 @@ pub(crate) fn sync(s: &mut Session) {
     }
 }
 
+/// The branches of the file in tab `uid` as saving it now would write them: [`sync`] only puts a
+/// branch tab's latest edits in before the next command.
+pub fn current(s: &Session, uid: u64) -> Vec<Branch> {
+    let Some(ds) = s.docs.iter().find(|d| d.uid == uid) else { return Vec::new() };
+    let mut branches = ds.branches.clone();
+    for d in &s.docs {
+        let Some((_, id)) = d.branch_of.as_ref().filter(|(main, _)| *main == uid) else { continue };
+        let Some(b) = branches.iter_mut().find(|b| b.id == *id) else { continue };
+        b.head = d.document_itself().clone();
+        if d.versions_changed {
+            b.versions = d.versions.clone();
+        }
+    }
+    branches
+}
+
 /// The tab a save writes: the main file's for a branch tab, else the active tab.
 pub fn file_tab(s: &Session) -> usize {
     s.ds().ok().and_then(|d| d.branch_of.as_ref()).and_then(|(m, _)| s.docs.iter().position(|d| d.uid == *m)).unwrap_or(s.active)