From b821b6f8f4a24aa8f028970c004a552ee5002a63 Mon Sep 17 00:00:00 2001 From: vxMacDev <77440838+vxMacDev@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:41:17 -0500 Subject: [PATCH] fix(recovery): keep the version history and branches in recovery copies (#185) Recovery copies were written with plain `save_bytes`, so a file recovered after a crash came back without its version history and branches, even though `file.recover` reads both. Copies now go through `save_bytes_with_history` like a normal save, with each open branch tab's latest edits (`branches::current`), since `sync` only puts those in before the next command. A copy was also only redone when the main file's revision changed, and a file with edits only in a branch tab didn't count as changed. Both checks now include the file's branch tabs. --- apps/figcraft/src/recovery.rs | 47 ++++++++++++++------- apps/figcraft/src/recovery_tests.rs | 65 +++++++++++++++++++++++++++++ crates/engine/src/branches.rs | 16 +++++++ 3 files changed, 112 insertions(+), 16 deletions(-) diff --git a/apps/figcraft/src/recovery.rs b/apps/figcraft/src/recovery.rs index 93335394..afaa1d5a 100644 --- a/apps/figcraft/src/recovery.rs +++ b/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. @@ -41,8 +42,8 @@ pub struct Recovery { dir: PathBuf, /// Held while the app runs: a free lock means the run that wrote the folder is gone. _lock: File, - /// The revision each document was last copied at, and the write in flight. - written: HashMap>)>, + /// The [`stamp`] each document was last copied at, and the write in flight. + written: HashMap, Option>)>, /// When (UI clock) the next copies are due. next: f64, /// Copies of online files from earlier runs, kept until their outcome is known. @@ -88,7 +89,7 @@ impl Recovery { self.settle(s); // A join on its way settles within its timeouts: look again soon. let settling = (!self.waiting.is_empty()).then_some(0.5); - let pending = s.docs.iter().any(|d| needs_copy(d) && self.written.get(&d.uid).is_none_or(|w| w.0 != d.revision)); + let pending = s.docs.iter().any(|d| needs_copy(s, d) && self.written.get(&d.uid).is_none_or(|w| w.0 != stamp(s, d))); if now < self.next { return pending.then_some(self.next - now).or(settling); } @@ -124,26 +125,28 @@ 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 self.written.remove(&d.uid).is_some() { let _ = std::fs::remove_file(&path); let _ = std::fs::remove_file(¬e); } continue; } - let entry = self.written.entry(d.uid).or_insert((0, None)); + let stamp = stamp(s, d); + let entry = self.written.entry(d.uid).or_insert((Vec::new(), None)); let busy = entry.1.as_ref().is_some_and(|h| !h.is_finished()); - if entry.0 == d.revision || busy { + if entry.0 == stamp || busy { 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; } let _ = match online { @@ -157,7 +160,7 @@ impl Recovery { } else { entry.1 = std::thread::Builder::new().name("recovery".into()).spawn(job).ok(); } - entry.0 = d.revision; + entry.0 = stamp; } // Closed documents. let open: Vec = s.docs.iter().map(|d| d.uid).collect(); @@ -201,15 +204,27 @@ fn rejoin_online(file: &Path, s: &mut Session) -> Option { s.execute("collab.recoverFile", ¶ms).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 { + 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 { + 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() } diff --git a/apps/figcraft/src/recovery_tests.rs b/apps/figcraft/src/recovery_tests.rs index 95560319..b7037871 100644 --- a/apps/figcraft/src/recovery_tests.rs +++ b/apps/figcraft/src/recovery_tests.rs @@ -62,3 +62,68 @@ fn copies_follow_saves_and_a_normal_quit_leaves_nothing() { assert!(!dir.exists(), "a normal quit removes the copies"); 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::>(), ["v1"]); + assert_eq!(ds.branches.iter().map(|b| (b.name.as_str(), rectangles(&b.head))).collect::>(), [("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::>(), [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::>(), [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::>(), ["v1 (renamed)"]); + r.finish(); + let _ = std::fs::remove_dir_all(&root); +} diff --git a/crates/engine/src/branches.rs b/crates/engine/src/branches.rs index 75065b82..8bff0568 100644 --- a/crates/engine/src/branches.rs +++ b/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 { + 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)