From ac8a0a112c856f2ea88a3699d29f5c515c9ea23e Mon Sep 17 00:00:00 2001 From: Barzabel Date: Wed, 14 Oct 2026 22:10:49 +0300 Subject: [PATCH] M1: keep a recovery copy until this run has its own (#187) --- apps/figcraft/src/recovery.rs | 122 +++++++++++++------- apps/figcraft/src/recovery_online_tests.rs | 26 ++++- apps/figcraft/src/recovery_tests.rs | 127 ++++++++++++++++++++- apps/figcraft/src/services.rs | 2 + 4 files changed, 234 insertions(+), 43 deletions(-) diff --git a/apps/figcraft/src/recovery.rs b/apps/figcraft/src/recovery.rs index 93335394..bf6f8df6 100644 --- a/apps/figcraft/src/recovery.rs +++ b/apps/figcraft/src/recovery.rs @@ -4,7 +4,10 @@ //! in the config folder, in the background, at most every [`INTERVAL`] seconds, and the copy goes //! once the document is saved or closed. The app holds a lock on its folder. At startup, the folders //! whose lock is free belong to runs that ended without closing: their copies open again as -//! unsaved documents (`file.recover`) and the folder goes. Closing the app normally removes its own. +//! unsaved documents (`file.recover`). Each such copy **stays until this run has its own copy of +//! the document** (or it is saved or closed): until then it is the only copy on disk, and a run +//! that can't keep copies of its own leaves it for the next start. Then it goes, and its folder +//! with its last copy. Closing the app normally removes its own. //! //! A document that is one of your online files has a note beside its copy (`.online`, written //! after the copy it describes): the server, the file's id, and who the app was in the file with @@ -49,12 +52,21 @@ pub struct Recovery { waiting: Vec, } -/// A copy of one of your online files whose file is being joined again as session `session`. +/// A copy from an earlier run, kept until what it became is safe. pub struct Waiting { - session: u64, + until: Until, copy: PathBuf, - /// The lock of the folder it is in: no other start takes the copy meanwhile. - _lock: Arc, + /// The lock of the folder it is in: no other start takes the copy meanwhile (none for a copy + /// left directly in the root by older versions). + _lock: Option>, +} + +/// What a kept copy waits for. +enum Until { + /// One of your online files, joined again as this session: its outcome. + Session(u64), + /// Opened as document `uid`: this run's own copy of it (or its save or close). + Copied(u64), } /// What [`recover_all`] brought back. @@ -62,21 +74,26 @@ pub struct Waiting { pub struct Recovered { /// Documents opened from their copies. pub copies: usize, - /// Copies of online files waiting for their file ([`Recovery::adopt`]). + /// The copies kept until what they became is safe ([`Recovery::adopt`]). pub waiting: Vec, } impl Recovery { /// Take this run's folder under `root` (`None` when it can't be created or locked: no copies). + /// A crashed run with the same process id may still hold its folder (its copies kept): then + /// another name is taken. pub fn start(root: &Path) -> Option { - let dir = root.join(std::process::id().to_string()); - std::fs::create_dir_all(&dir).ok()?; - let lock = File::create(dir.join(LOCK)).ok()?; - lock.try_lock().ok()?; - Some(Self { dir, _lock: lock, written: HashMap::new(), next: 0.0, waiting: Vec::new() }) + let pid = std::process::id(); + (0..10).find_map(|n| { + let dir = root.join(if n == 0 { pid.to_string() } else { format!("{pid}-{n}") }); + std::fs::create_dir_all(&dir).ok()?; + let lock = File::options().write(true).create(true).truncate(false).open(dir.join(LOCK)).ok()?; + lock.try_lock().ok()?; + Some(Self { dir, _lock: lock, written: HashMap::new(), next: 0.0, waiting: Vec::new() }) + }) } - /// Keep earlier runs' copies of online files until their outcome is known. + /// Keep earlier runs' copies until what they became is safe. pub fn adopt(&mut self, waiting: Vec) { self.waiting.extend(waiting); } @@ -86,8 +103,10 @@ impl Recovery { /// an idle window can sleep). pub fn tick(&mut self, s: &Session, now: f64) -> Option { 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); + // A join on its way settles within its timeouts: look again soon. A copy waiting for this + // run's own is looked at again with the next copies. + let joining = self.waiting.iter().any(|w| matches!(w.until, Until::Session(_))); + let settling = if joining { Some(0.5) } else { (!self.waiting.is_empty()).then_some(INTERVAL) }; let pending = s.docs.iter().any(|d| needs_copy(d) && self.written.get(&d.uid).is_none_or(|w| w.0 != d.revision)); if now < self.next { return pending.then_some(self.next - now).or(settling); @@ -97,26 +116,28 @@ impl Recovery { settling } - /// Remove earlier runs' copies whose outcome is known, once what they became is copied here. - fn settle(&mut self, s: &Session) { - if self.waiting.iter().all(|w| s.live.recovering(w.session)) { - return; + /// Remove earlier runs' copies once what they became is safe: copied here (the write + /// succeeded), saved or closed. An online file's copy goes once its outcome is known and every + /// document is safe, since which of them it became isn't known. + pub fn settle(&mut self, s: &Session) { + let joined = |w: &Waiting| matches!(w.until, Until::Session(id) if !s.live.recovering(id)); + if self.waiting.iter().any(joined) { + self.write(s, true); } - self.write(s, true); - let (done, waiting): (Vec, Vec) = - std::mem::take(&mut self.waiting).into_iter().partition(|w| !s.live.recovering(w.session)); + let all_safe = s.docs.iter().all(|d| self.safe(d)); + let (done, waiting): (Vec, Vec) = std::mem::take(&mut self.waiting).into_iter().partition(|w| match w.until { + Until::Session(_) => joined(w) && all_safe, + Until::Copied(uid) => s.docs.iter().find(|d| d.uid == uid).is_none_or(|d| self.safe(d)), + }); self.waiting = waiting; - for w in done { - let _ = std::fs::remove_file(&w.copy); - let _ = std::fs::remove_file(w.copy.with_extension(ONLINE)); - // The folder goes with its last copy (while another waits there its lock is held). - let dir = w.copy.parent().map(Path::to_path_buf).filter(|_| Arc::strong_count(&w._lock) == 1); - drop(w); - if let Some(dir) = dir { - let _ = std::fs::remove_file(dir.join(LOCK)); - let _ = std::fs::remove_dir(dir); - } - } + done.into_iter().for_each(remove_kept); + } + + /// Whether document `d`'s work is on disk: saved (or nothing to keep), or copied here with no + /// write in flight. + fn safe(&self, d: &figcraft_engine::DocState) -> bool { + let written = self.written.get(&d.uid).is_some_and(|w| w.1.as_ref().is_none_or(JoinHandle::is_finished)); + !needs_copy(d) || (written && self.dir.join(format!("{}.figcraft", d.uid)).is_file()) } /// Copy every document with unsaved changes; `wait`: before returning (after a panic). @@ -175,7 +196,9 @@ impl Recovery { /// A normal quit: the documents were saved or their changes discarded on purpose. (Earlier /// runs' copies still waiting for their outcome stay for the next start.) pub fn finish(self) { - let Self { dir, _lock, written, .. } = self; + let Self { dir, _lock, written, waiting, .. } = self; + // The documents earlier runs' copies became were saved or discarded too. + waiting.into_iter().filter(|w| matches!(w.until, Until::Copied(_))).for_each(remove_kept); // Windows removes a folder only once nothing in it is open: copies being written first. for h in written.into_values().filter_map(|w| w.1) { let _ = h.join(); @@ -185,6 +208,20 @@ impl Recovery { } } +/// Remove a kept copy and its note, and its folder with its last copy (while another is kept +/// there, its lock is held). +fn remove_kept(w: Waiting) { + let _ = std::fs::remove_file(&w.copy); + let _ = std::fs::remove_file(w.copy.with_extension(ONLINE)); + let last = w._lock.as_ref().is_some_and(|l| Arc::strong_count(l) == 1); + let dir = w.copy.parent().map(Path::to_path_buf).filter(|_| last); + drop(w); + if let Some(dir) = dir { + let _ = std::fs::remove_file(dir.join(LOCK)); + let _ = std::fs::remove_dir(dir); + } +} + /// The extension of the note beside the copy of a document that is one of your online files. const ONLINE: &str = "online"; @@ -222,9 +259,9 @@ pub fn recover(root: &Path, s: &mut Session) -> usize { } /// Open the copies left by runs that ended without closing (folders under `root` whose lock is -/// free, and copies directly in `root` from older versions) as unsaved documents, then remove them. -/// A copy that can't be read stays where it is. A copy of one of your online files is kept -/// instead while its file is joined again ([`Recovered::waiting`]). +/// free, and copies directly in `root` from older versions) as unsaved documents. Each is kept +/// until this run has its own copy of the document ([`Recovered::waiting`]); a copy of one of your +/// online files, while its file is joined again. A copy that can't be read stays where it is. pub fn recover_all(root: &Path, s: &mut Session) -> Recovered { let mut out = Recovered::default(); let Ok(entries) = std::fs::read_dir(root) else { return out }; @@ -234,10 +271,15 @@ pub fn recover_all(root: &Path, s: &mut Session) -> Recovered { } // One of your online files comes back from its server, its copy kept until then. if let Some((session, lock)) = lock.and_then(|l| Some((rejoin_online(file, s)?, l))) { - out.waiting.push(Waiting { session, copy: file.to_path_buf(), _lock: lock.clone() }); - } else if s.execute("file.recover", &json!({"path": file.to_string_lossy()})).is_ok() { + out.waiting.push(Waiting { until: Until::Session(session), copy: file.to_path_buf(), _lock: Some(lock.clone()) }); + return; + } + let before: Vec = s.docs.iter().map(|d| d.uid).collect(); + if s.execute("file.recover", &json!({"path": file.to_string_lossy()})).is_ok() { out.copies += 1; - let _ = std::fs::remove_file(file); + if let Some(uid) = s.docs.iter().map(|d| d.uid).find(|u| !before.contains(u)) { + out.waiting.push(Waiting { until: Until::Copied(uid), copy: file.to_path_buf(), _lock: lock.cloned() }); + } } }; for e in entries.flatten() { @@ -265,7 +307,7 @@ pub fn recover_all(root: &Path, s: &mut Session) -> Recovered { for f in files.iter().filter(|f| f.extension().is_some_and(|e| e == ONLINE) && !f.with_extension("figcraft").exists()) { let _ = std::fs::remove_file(f); } - // Empty now unless a copy couldn't be read, or waits for its file (kept, its lock held). + // Empty now unless a copy couldn't be read, or is kept (its lock held). if Arc::strong_count(&lock) == 1 { drop(lock); let _ = std::fs::remove_file(path.join(LOCK)); diff --git a/apps/figcraft/src/recovery_online_tests.rs b/apps/figcraft/src/recovery_online_tests.rs index ead3749a..c9036773 100644 --- a/apps/figcraft/src/recovery_online_tests.rs +++ b/apps/figcraft/src/recovery_online_tests.rs @@ -223,6 +223,30 @@ fn a_copy_ahead_of_the_server_has_its_edits_applied_and_sent() { let _ = std::fs::remove_dir_all(&root); } +#[test] +fn an_online_copy_stays_while_what_it_became_cant_be_copied() { + let root = root("ahead-blocked"); + let (mut s, script, mut server, _) = shared(); + s.execute("shape.ellipse", &json!({"x": 30, "y": 0, "width": 10, "height": 10})).unwrap(); + script.lock().unwrap().sent.clear(); + let left = crash(&s, &root); + let (mut next, script, mut r, _) = restart(&s, &root); + arrive(&mut next, &script, &mut server); + assert!(next.docs[1].is_dirty(), "the copy's edit is on top, not saved anywhere yet"); + // Something is in the way of this run's copy: writing it fails. + let mine = root.join(std::process::id().to_string()).join(format!("{}.figcraft", next.docs[1].uid)); + std::fs::create_dir_all(&mine).unwrap(); + r.tick(&next, 1.0); + assert_eq!((copies(&left, "figcraft"), copies(&left, "online")), (1, 1), "kept: the work is nowhere else on disk"); + // Once it can be written, the old copy goes. + std::fs::remove_dir(&mine).unwrap(); + next.execute("shape.rectangle", &json!({"x": 50, "y": 0, "width": 4, "height": 4})).unwrap(); + r.tick(&next, 2.0); + assert!(!left.exists()); + drop(r); + let _ = std::fs::remove_dir_all(&root); +} + #[test] fn a_copy_the_server_already_has_is_discarded() { let root = root("equal"); @@ -332,7 +356,7 @@ fn without_its_record_the_copy_opens_as_any_other() { assert_eq!(recover(&root, &mut next), 1); assert!(script.lock().unwrap().joined.is_empty()); assert!(next.docs.first().is_some_and(|d| d.is_dirty() && d.path.is_none())); - assert!(std::fs::read_dir(&root).unwrap().next().is_none(), "the folder is gone"); + assert!(std::fs::read_dir(&root).unwrap().next().is_some(), "the copy stays until a run has its own"); let _ = std::fs::remove_dir_all(&root); } diff --git a/apps/figcraft/src/recovery_tests.rs b/apps/figcraft/src/recovery_tests.rs index 95560319..2efed538 100644 --- a/apps/figcraft/src/recovery_tests.rs +++ b/apps/figcraft/src/recovery_tests.rs @@ -4,7 +4,7 @@ use figcraft_engine::Session; use serde_json::json; -use crate::recovery::{Recovery, recover}; +use crate::recovery::{Recovery, recover, recover_all}; fn root(name: &str) -> std::path::PathBuf { let r = std::env::temp_dir().join(format!("figcraft-recovery-{name}-{}", std::process::id())); @@ -34,7 +34,7 @@ fn a_crashed_runs_unsaved_documents_come_back() { let ds = next.docs.first().unwrap(); assert!(ds.is_dirty() && ds.path.is_none()); assert_eq!(next.doc().unwrap().node(next.doc().unwrap().pages[0]).map(|p| p.children.len()), Some(1)); - assert!(std::fs::read_dir(&root).unwrap().next().is_none(), "the folder is gone"); + assert!(std::fs::read_dir(&root).unwrap().next().is_some(), "the copy stays until a run has its own"); let _ = std::fs::remove_dir_all(&root); } @@ -62,3 +62,126 @@ 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); } + +/// A run that ended without closing, leaving a copy of `s`'s unsaved document: its folder (as +/// another process's). +fn crashed(s: &Session, root: &std::path::Path) -> std::path::PathBuf { + let mut r = Recovery::start(root).unwrap(); + r.write(s, true); + drop(r); + let left = root.join("crashed"); + std::fs::rename(root.join(std::process::id().to_string()), &left).unwrap(); + left +} + +fn copies_in(d: &std::path::Path) -> usize { + std::fs::read_dir(d).map_or(0, |d| d.flatten().filter(|f| f.path().extension().is_some_and(|e| e == "figcraft")).count()) +} + +/// The next start: the copies come back, and this run's recovery keeps them until it covers them. +fn restart(root: &std::path::Path) -> (Session, Recovery) { + let mut next = Session::empty(); + let recovered = recover_all(root, &mut next); + assert_eq!(recovered.copies, 1); + let mut r = Recovery::start(root).unwrap(); + r.adopt(recovered.waiting); + (next, r) +} + +#[test] +fn a_recovered_copy_stays_until_this_run_has_its_own_copy() { + let root = root("handoff"); + let left = crashed(&edited(), &root); + let (next, mut r) = restart(&root); + assert_eq!(copies_in(&left), 1, "the only copy on disk until this run writes one"); + r.write(&next, true); + r.tick(&next, 100.0); + assert!(!left.exists(), "covered by this run's copy, the old one goes"); + assert_eq!(copies_in(&root.join(std::process::id().to_string())), 1); + r.finish(); + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn a_recovered_copy_stays_while_its_replacement_cant_be_written() { + let root = root("blocked"); + let left = crashed(&edited(), &root); + let (next, mut r) = restart(&root); + // Something is in the way of this run's copy: writing it fails. + let mine = root.join(std::process::id().to_string()).join(format!("{}.figcraft", next.docs[0].uid)); + std::fs::create_dir_all(&mine).unwrap(); + r.write(&next, true); + r.tick(&next, 100.0); + assert_eq!(copies_in(&left), 1, "kept: the work is nowhere else on disk"); + // Once it can be written, the old copy goes. + std::fs::remove_dir(&mine).unwrap(); + let mut next = next; + next.execute("shape.rectangle", &json!({"x": 1, "y": 1, "width": 2, "height": 2})).unwrap(); + r.write(&next, true); + r.tick(&next, 200.0); + assert!(!left.exists()); + r.finish(); + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn without_a_recovery_of_its_own_the_copy_stays_for_the_next_start() { + let root = root("nocopies"); + crashed(&edited(), &root); + let mut next = Session::empty(); + let recovered = recover_all(&root, &mut next); + assert_eq!(recovered.copies, 1); + // This run couldn't take a folder of its own: it ends (or crashes) with nothing copied. + drop(recovered); + let mut after = Session::empty(); + assert_eq!(recover(&root, &mut after), 1, "the work comes back again"); + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn a_recovered_copy_goes_once_its_document_is_saved_or_closed() { + for close in [false, true] { + let root = root(if close { "closed" } else { "saved" }); + let left = crashed(&edited(), &root); + let (mut next, mut r) = restart(&root); + if close { + next.docs.clear(); + } else { + next.ds_mut().unwrap().mark_saved(); + } + r.tick(&next, 0.0); + assert!(!left.exists(), "close {close}"); + r.finish(); + let _ = std::fs::remove_dir_all(&root); + } +} + +#[test] +fn a_normal_quit_removes_the_recovered_copies_it_took_over() { + let root = root("quitafter"); + let left = crashed(&edited(), &root); + let (_, r) = restart(&root); + // Quitting asked to save or discard every document. + r.finish(); + assert!(!left.exists()); + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn a_run_whose_folder_name_is_still_held_takes_another() { + let root = root("pidreuse"); + // A crashed run with this process's id whose folder the start still holds (its copy waits). + let held = root.join(std::process::id().to_string()); + std::fs::create_dir_all(&held).unwrap(); + let lock = std::fs::File::create(held.join("lock")).unwrap(); + lock.try_lock().unwrap(); + let mut r = Recovery::start(&root).expect("a folder of its own"); + let s = edited(); + r.write(&s, true); + assert_eq!(copies_in(&held), 0); + let dirs = std::fs::read_dir(&root).unwrap().flatten().filter(|e| e.path() != held).count(); + assert_eq!(dirs, 1); + r.finish(); + drop(lock); + let _ = std::fs::remove_dir_all(&root); +} diff --git a/apps/figcraft/src/services.rs b/apps/figcraft/src/services.rs index f4aeb63d..6fd7463d 100644 --- a/apps/figcraft/src/services.rs +++ b/apps/figcraft/src/services.rs @@ -242,6 +242,8 @@ impl Services for Desktop { fn emergency_save(&mut self, session: &Session) { if let Some(r) = crate::recovery::lock(&self.recovery).as_mut() { r.write(session, true); + // Earlier runs' copies now covered go, so the next start doesn't open them twice. + r.settle(session); } } fn reduce_motion(&self) -> bool {