From 17a51fc5911eb2786e964d457fbca3b05defafb0 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 17:59:15 -0800 Subject: [PATCH 1/2] Data Recovery: a running app locks its area again when the lock file is removed While an app runs, its Data Recovery area's .lock file, or the whole area folder, can be removed by the user, a cleaner or another program. The app kept writing copies into an area that no lock held, and another app could lock that area and offer the copies as a crash's copies. Each recovery save and heartbeat now checks that the file at the area's .lock path is still the file the app locked. When the lock file or the folder is gone, or another file is in its place, the app makes the area again if it is gone, locks it again, and writes again the copies that went with the folder. A lock file that can't be read for another reason still counts as held. When another app holds the area by then, the app takes a new area and writes every copy there. The app does not offer or discard its own copies in the old area as a crash's copies. It removes each one when its document has a copy in the new area, or is saved, reverted or closed, and removes all of them when it quits with nothing unsaved. The app that holds the old area also removes them when it quits with nothing unsaved. A copy that was being written when the Data Recovery folder preference changed is now removed from the folder it was written to. Before, the document took that copy as its current one, and the copy stayed in the old folder after the document was saved or closed. --- crates/engine/src/cmd/recovery.rs | 181 +++++++++++++++-- crates/engine/src/tests/recovery.rs | 304 ++++++++++++++++++++++++++++ crates/ui-egui/src/recovery.rs | 4 +- 3 files changed, 469 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index b75066ec6..bb7fbc6a5 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -17,7 +17,9 @@ //! store can, an announcement that lasts as long as the app ([`RecoveryStore::announce`]: the //! browser's Web Locks). Only areas nobody holds any more (their app is gone) are offered, and //! while their copies are restored or discarded the area is held, so two apps launched together -//! never both take it. +//! never both take it. An area's lock file or folder can be removed while its app runs, so each +//! recovery save and heartbeat checks that the lock still holds the area, and locks it again or +//! takes a new area. //! //! A heartbeat alone can't tell a gone app from one whose timers stopped (a background tab), so //! when unsure an area counts as held, and its app repairs what another app did meanwhile: each @@ -100,6 +102,12 @@ pub trait RecoveryStore: Send + Sync { fn lock(&self, _area: &str) -> std::result::Result { Ok(Lock::Unsupported) } + /// Does `hold`, which [`RecoveryStore::lock`] gave for area `area`, still hold it? A lock on a + /// file or folder that was removed or replaced since holds nothing; when that can't be told now, + /// it does (default: it does). + fn still_held(&self, _area: &str, _hold: &(dyn std::any::Any + Send + Sync)) -> bool { + true + } /// Stores without locks: also hold area `area` in a way other apps can see that lasts exactly as /// long as this app runs, if the store has one (the browser's Web Locks) → it, until dropped. fn announce(&self, _area: &str) -> Option { @@ -416,6 +424,16 @@ impl RecoveryStore for FolderStore { Self::create_area(&dir)?; Self::lock_area(&dir) } + fn still_held(&self, area: &str, hold: &(dyn std::any::Any + Send + Sync)) -> bool { + let (Some(file), Ok(dir)) = (hold.downcast_ref::(), self.area(area)) else { return true }; + let path = dir.join(LOCK_FILE); + // Only a lock file or folder that is gone, or another one in its place, ends the hold. One + // that can't be read now (a network folder that doesn't answer) still counts as held. + match std::fs::symlink_metadata(&path) { + Ok(_) => Self::own_area(&dir).is_ok() && Self::still_named(file, &path), + Err(e) => e.kind() != std::io::ErrorKind::NotFound, + } + } fn remove_area(&self, area: &str) { // Best effort: whatever is left is tidied at a later launch. A folder that isn't empty // (copies came back meanwhile) stays. The caller still holds the lock here. @@ -457,6 +475,10 @@ pub struct Recovery { /// heartbeat it wrote there: that one doesn't make them look running, a newer one does (their /// app was only paused and came back). adopted: BTreeMap, + /// Copies in an area this session had to leave (`keep_hold`), by document uid, with their + /// store: each stays until its document has a copy in the new area, or needs none. They are + /// still this session's: never offered or discarded as a crash's here. + left: BTreeMap, String)>, } impl Recovery { @@ -526,14 +548,79 @@ fn beating(store: &dyn RecoveryStore, area: &str, stale: i64, mine: Option) /// Refresh this session's hold on its area (the app does this every [`HEARTBEAT_EVERY`] seconds, /// so also on its first frame after its timers were paused): its heartbeat in a store without -/// locks, and its copies ([`repair`]) → the documents whose copies went missing (write them now). -/// Best effort: a missed beat only lets another app offer the copies once three intervals have -/// passed. +/// locks, and its copies ([`repair`]) → the documents whose copies went missing (write them now), +/// and those whose only copy is in an area the session left (`keep_hold`). Best effort: a missed +/// beat only lets another app offer the copies once three intervals have passed. pub fn heartbeat(s: &mut Session) -> Vec { + let mut lost = keep_hold(s); if let Some(own) = s.recovery.own.as_ref().filter(|o| o.beats) { let _ = beat(own.store.as_ref(), &own.area); } - repair(s) + // A copy that was being written when the session left its area finished there. A document + // that gets no copy now (a complex one) keeps the copy it has. + lost.extend( + s.recovery + .left + .keys() + .filter(|&&uid| s.documents().iter().any(|d| d.uid == uid && d.recovery.is_none() && skip_reason(s, d).is_none())) + .copied(), + ); + lost.extend(repair(s)); + lost.sort_unstable(); + lost.dedup(); + lost +} + +/// Check that this session's lock still holds its area (stores with locks). The area's lock file, +/// or the whole area, can be removed while the app runs (by the user, a cleaner, another program), +/// and then another app can lock the area and offer or delete this session's copies as a crash's. +/// The area is locked again, made again first if it went (its copies are then written again). +/// When another app holds it now, the session takes a new area, and each copy left in the old one +/// stays until its document has a copy in the new one. → the documents whose copies must be +/// written again. +fn keep_hold(s: &mut Session) -> Vec { + drop_left_done(s); + let Some(own) = &mut s.recovery.own else { return vec![] }; + if own.beats || own.hold.as_deref().is_none_or(|h| own.store.still_held(&own.area, h)) { + return vec![]; + } + let (store, area) = (own.store.clone(), own.area.clone()); + // The lock held is on a file the area no longer has, so it holds nothing: it goes first (a + // removed file this app still has open can keep a new lock file from being made). + own.hold = None; + if let Ok(Lock::Held(h)) = store.lock(&area) { + own.hold = Some(h); + return repair(s); + } + let mut lost = vec![]; + for st in &mut s.docs { + if let Some(c) = st.recovery.take() { + s.recovery.left.insert(st.uid, (store.clone(), c.file)); + lost.push(st.uid); + } + } + s.recovery.own = None; + // Best effort: when no area can be taken now, the next recovery save says why. + let _ = claim(s, &store); + lost +} + +/// The copies left in an area this session left whose documents were saved, reverted or closed +/// since go. +fn drop_left_done(s: &mut Session) { + let done: Vec = s.recovery.left.keys().filter(|&&uid| !s.documents().iter().any(|d| d.uid == uid && d.is_dirty())).copied().collect(); + for uid in done { + drop_left(s, uid); + } +} + +/// Document `uid`'s copy in an area this session left goes. Best effort: a copy that can't be +/// removed is offered once nobody holds that area, which loses nothing. +fn drop_left(s: &mut Session, uid: u64) { + if let Some((store, file)) = s.recovery.left.remove(&uid) { + let _ = store.remove(&format!("{file}{DOC_EXT}")); + let _ = store.remove(&format!("{file}{META_EXT}")); + } } /// Forget this session's copies that are missing from its area, so the next recovery save writes @@ -555,10 +642,13 @@ pub fn repair(s: &mut Session) -> Vec { /// This session's area in `store`, taken now if it has none there yet: a fresh area, held for the /// session's lifetime (locked, or with a heartbeat and an announcement where the store has them). pub fn claim(s: &mut Session, store: &Arc) -> Result { - if let Some(own) = &s.recovery.own { - if own.store.location() == store.location() { + if s.recovery.own.as_ref().is_some_and(|o| o.store.location() == store.location()) { + // Documents whose copies went are left without one, so the caller writes them again. + keep_hold(s); + if let Some(own) = &s.recovery.own { return Ok(own.area.clone()); } + } else if s.recovery.own.is_some() { // The store moved (the preference names another folder): copies start again there. release(s); } @@ -588,6 +678,11 @@ pub fn release(s: &mut Session) { for st in &mut s.docs { st.recovery = None; } + // Copies left in an area this session had to leave go too. + let left: Vec = s.recovery.left.keys().copied().collect(); + for uid in left { + drop_left(s, uid); + } let Some(Own { store, area, hold, .. }) = s.recovery.own.take() else { return }; let mine = |n: &String| n.split_once('/').is_some_and(|(a, _)| a == area); for name in store.list().unwrap_or_default().iter().filter(|n| mine(n)) { @@ -750,12 +845,16 @@ pub struct RecoveryJob { pub title: String, meta: Meta, save: SaveJob, + /// The store the copy goes to (the preference can name another folder before it is finished). + store: Arc, } impl RecoveryJob { /// Encode the snapshot and store it with its details and the images the area doesn't have - /// yet → `{file, title, bytes (the document's), imageBytes (images written now)}`. - pub fn write(&self, store: &dyn RecoveryStore) -> std::result::Result { + /// yet, in the store the job was made for → `{file, title, bytes (the document's), imageBytes + /// (images written now)}`. + pub fn write(&self) -> std::result::Result { + let store = self.store.as_ref(); let doc = self.save.doc(); // A linked image keeps its preview, as a save writes it. let linked = doc.linked_only_images(); @@ -808,13 +907,35 @@ impl RecoveryJob { /// Once stored: the document (if still open) remembers the copy, so it isn't written again /// while unchanged and goes when the document is saved or closed. A document saved or closed - /// meanwhile has nothing left to recover: its copy goes at once. + /// meanwhile has nothing left to recover: its copy goes at once. So does a copy written in an + /// area the session left meanwhile (`keep_hold`), and its document gets a new one. pub fn finish(self, s: &mut Session) { let snapshot = self.save.snapshot().clone(); let copy = RecoveryCopy { file: self.file.clone(), doc: snapshot }; + let area = self.file.split_once('/').map(|(a, _)| a); + let current = s.recovery.own.as_ref().is_some_and(|o| o.store.location() == self.store.location() && Some(o.area.as_str()) == area); match s.document_mut(self.uid).filter(|st| st.is_dirty()) { - Some(st) => st.recovery = Some(copy), - None => remove_copy(s, &self.file), + Some(st) if current => { + st.recovery = Some(copy); + drop_left(s, self.uid); + } + // Written in an area left meanwhile: kept, in place of the copy left there before, + // until the document has a copy in the new area. + Some(st) if st.recovery.is_none() => { + let before = s.recovery.left.insert(self.uid, (self.store.clone(), self.file.clone())); + // The copy left there before, unless this one was written over it. Best effort, + // as in `remove_copy`. + if let Some((store, file)) = before.filter(|(old, f)| old.location() != self.store.location() || *f != self.file) { + let _ = store.remove(&format!("{file}{DOC_EXT}")); + let _ = store.remove(&format!("{file}{META_EXT}")); + } + } + // Best effort, as in `remove_copy`. + _ => { + let _ = self.store.remove(&format!("{}{DOC_EXT}", self.file)); + let _ = self.store.remove(&format!("{}{META_EXT}", self.file)); + drop_left(s, self.uid); + } } } } @@ -867,6 +988,11 @@ fn fresh_name(area: &str, stem: &str, taken: &[String]) -> String { /// when it has none and a copy is needed) → (jobs, skipped: `[{title, reason}]` for modified /// documents that get none). pub fn jobs(s: &mut Session, store: &Arc) -> Result<(Vec, Vec)> { + // A lost lock is taken again first: copies that go with it are written again, even of + // documents unchanged since. + if s.recovery.own.as_ref().is_some_and(|o| o.store.location() == store.location()) { + keep_hold(s); + } // Copies another app removed are written again. repair(s); let skipped: Vec = s @@ -923,7 +1049,7 @@ pub fn jobs(s: &mut Session, store: &Arc) -> Result<(Vec Result<()> { return Err(EngineError::Other("a document with unsaved changes has no recovery copy".into())); } for j in jobs { - j.write(store.as_ref()).map_err(EngineError::Other)?; + j.write().map_err(EngineError::Other)?; j.finish(s); } Ok(()) @@ -966,6 +1092,8 @@ pub fn forget(s: &mut Session, uid: u64) { if s.script_in_run { return; } + // So does a copy of it left in an area this session left. + drop_left(s, uid); let Some(copy) = s.document_mut(uid).and_then(|st| st.recovery.take()) else { return }; remove_copy(s, ©.file); } @@ -975,12 +1103,16 @@ pub fn forget_all(s: &mut Session) { release(s); } -/// The copies of documents with no unsaved changes left (undone back to the saved state) go. +/// The copies of documents with no unsaved changes left (undone back to the saved state) go, and +/// the copies left in an area this session left of documents closed since. pub fn forget_clean(s: &mut Session) { let uids: Vec = s.documents().iter().filter(|d| !d.is_dirty() && d.recovery.is_some()).map(|d| d.uid).collect(); for uid in uids { forget(s, uid); } + if !s.script_in_run { + drop_left_done(s); + } } fn save(s: &mut Session, _: &Value) -> Result { @@ -988,7 +1120,7 @@ fn save(s: &mut Session, _: &Value) -> Result { let (jobs, skipped) = jobs(s, &store)?; let mut saved = vec![]; for j in jobs { - saved.push(j.write(store.as_ref()).map_err(EngineError::Other)?); + saved.push(j.write().map_err(EngineError::Other)?); j.finish(s); } forget_clean(s); @@ -1057,9 +1189,15 @@ fn open_copies(s: &Session) -> Vec { s.documents().iter().filter_map(|d| d.recovery.as_ref()).map(|c| c.file.clone()).collect() } +/// The copies of documents open now in this session that are in an area it left, in `store`. +fn left_copies(s: &Session, store: &dyn RecoveryStore) -> Vec { + s.recovery.left.values().filter(|(st, _)| st.location() == store.location()).map(|(_, f)| f.clone()).collect() +} + fn list(s: &mut Session, _: &Value) -> Result { let store = need_store(s, "file.recovery.list")?; let open = open_copies(s); + let left = left_copies(s, store.as_ref()); let copies = copies_in(&store.list().map_err(EngineError::Other)?); let mut rows = vec![]; for area in store.areas().map_err(EngineError::Other)? { @@ -1071,7 +1209,7 @@ fn list(s: &mut Session, _: &Value) -> Result { let title = if meta.title.is_empty() { f.rsplit('/').next().unwrap_or(f).to_string() } else { meta.title }; rows.push(json!({ "file": f, "title": title, "path": meta.path, "format": meta.format, "saved": meta.saved, - "open": who == Owner::Mine && open.contains(f), "running": who == Owner::Running, + "open": (who == Owner::Mine && open.contains(f)) || left.contains(f), "running": who == Owner::Running, })); } // A gone app's empty area is tidied away. @@ -1095,7 +1233,14 @@ fn targets(s: &mut Session, store: &dyn RecoveryStore, cmd: &str, p: &Value) -> { return Err(bad(cmd, format!("no recovery copy `{f}` (see file.recovery.list)"))); } - let picked = |c: &str| file.is_none_or(|f| f == c); + // A copy this session left in an area it had to leave is still its own. + let left = left_copies(s, store); + if let Some(f) = file + && left.iter().any(|c| c == f) + { + return Err(bad(cmd, format!("`{f}` is the copy of a document open now"))); + } + let picked = |c: &str| file.is_none_or(|f| f == c) && !left.iter().any(|l| l == c); let areas: BTreeSet<&str> = copies.iter().filter(|(_, c)| picked(c)).map(|(a, _)| a.as_str()).collect(); let mut out = vec![]; for area in areas { diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index 4994a9072..acd982814 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -682,6 +682,310 @@ fn a_recovery_save_takes_the_next_area_when_the_first_is_tidied_away() { let _ = std::fs::remove_dir_all(&dir); } +/// A session keeping its copies in folder `dir`, with one modified document whose copy is written. +fn saved_in(dir: &std::path::Path) -> (Session, String) { + let mut s = Session::new(); + s.prefs.recovery_folder = dir.to_string_lossy().to_string(); + s.execute("file.new", &json!({"width": 50, "height": 50})).unwrap(); + s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 5, "height": 5})).unwrap(); + s.execute("file.recovery.save", &json!({})).unwrap(); + let area = s.recovery.own_area().unwrap().to_string(); + (s, area) +} + +/// A new app over folder `dir`, with nothing open. +#[cfg(unix)] +fn other_app(dir: &std::path::Path) -> Session { + let mut s = Session::new(); + s.prefs.recovery_folder = dir.to_string_lossy().to_string(); + s +} + +/// The area's lock file was removed while its app runs: the next recovery save locks the area +/// again, so another app neither offers nor discards its copy. +#[cfg(unix)] +#[test] +fn a_recovery_save_locks_the_area_again_when_its_lock_file_was_removed() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-relock-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + a.execute("file.recovery.save", &json!({})).unwrap(); + assert_eq!(a.recovery.own_area(), Some(area.as_str())); + let mut b = other_app(&dir); + assert!(offered(&mut b).is_empty()); + assert!(b.execute("file.recovery.discard", &json!({})).unwrap()["discarded"].as_array().unwrap().is_empty()); + assert!(dir.join(&area).join("Untitled-1-1.vectorcraft").is_file()); + assert!(matches!(FolderStore::new(&dir).lock(&area), Ok(Lock::Busy))); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The whole area was removed while its app runs: the next recovery save makes it again, locked, +/// with the copy written again. +#[cfg(unix)] +#[test] +fn a_recovery_save_makes_the_area_again_when_it_was_removed() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-remade-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_dir_all(dir.join(&area)).unwrap(); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + assert_eq!(r["saved"][0]["file"], format!("{area}/Untitled-1-1").as_str(), "{r}"); + assert!(dir.join(&area).join("Untitled-1-1.vectorcraft").is_file()); + assert!(matches!(FolderStore::new(&dir).lock(&area), Ok(Lock::Busy))); + assert!(offered(&mut other_app(&dir)).is_empty()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Another app locked the area after its lock file was removed: the next heartbeat takes a new +/// area, and the document is to be copied again there. Its copy in the old area stays until then. +#[cfg(unix)] +#[test] +fn an_area_another_app_locked_meanwhile_is_left_for_a_new_one() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-moved-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let store = FolderStore::new(&dir); + let Ok(Lock::Held(other)) = store.lock(&area) else { panic!("the other app locks the area") }; + let uid = a.doc().unwrap().uid; + assert_eq!(cmd::recovery::heartbeat(&mut a), [uid]); + let mine = a.recovery.own_area().unwrap().to_string(); + assert_ne!(mine, area); + assert!(a.doc().unwrap().recovery.is_none()); + assert!(dir.join(&area).join("Untitled-1-1.vectorcraft").is_file(), "kept until the new copy is written"); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + assert_eq!(r["saved"][0]["file"], format!("{mine}/Untitled-1-1").as_str(), "{r}"); + assert!(!dir.join(&area).join("Untitled-1-1.vectorcraft").exists(), "the copy left there goes"); + assert!(matches!(store.lock(&mine), Ok(Lock::Busy))); + drop(other); + assert!(offered(&mut other_app(&dir)).is_empty()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A copy written in the background into the area the session left meanwhile is not the +/// document's copy: it stays only until the document is copied again in the new area. +#[cfg(unix)] +#[test] +fn a_copy_written_into_an_area_left_meanwhile_goes_once_copied_again() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-inflight-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + a.execute("shape.ellipse", &json!({"x": 10, "y": 10, "width": 5, "height": 5})).unwrap(); + let store: Arc = Arc::new(FolderStore::new(&dir)); + let (jobs, _) = cmd::recovery::jobs(&mut a, &store).unwrap(); + assert_eq!(jobs.len(), 1); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = store.lock(&area) else { panic!("the other app locks the area") }; + cmd::recovery::heartbeat(&mut a); + for j in jobs { + j.write().unwrap(); + j.finish(&mut a); + } + assert!(a.doc().unwrap().recovery.is_none()); + assert!(dir.join(&area).join("Untitled-1-1.vectorcraft").is_file()); + let uid = a.doc().unwrap().uid; + assert_eq!(cmd::recovery::heartbeat(&mut a), [uid], "the next heartbeat has it copied again"); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + assert_eq!(r["saved"].as_array().unwrap().len(), 1, "{r}"); + assert!(!dir.join(&area).join("Untitled-1-1.vectorcraft").exists()); + assert!(cmd::recovery::heartbeat(&mut a).is_empty()); + drop(other); + assert!(offered(&mut other_app(&dir)).is_empty()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Another app locked the area meanwhile and the document is unchanged since its copy: the next +/// recovery save still writes the copy, in the new area. +#[cfg(unix)] +#[test] +fn a_recovery_save_copies_unchanged_documents_into_the_new_area() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-unchanged-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = FolderStore::new(&dir).lock(&area) else { panic!("the other app locks the area") }; + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + let mine = a.recovery.own_area().unwrap().to_string(); + assert_ne!(mine, area); + assert_eq!(r["saved"][0]["file"], format!("{mine}/Untitled-1-1").as_str(), "{r}"); + assert!(dir.join(&mine).join("Untitled-1-1.vectorcraft").is_file()); + assert!(!dir.join(&area).join("Untitled-1-1.vectorcraft").exists()); + drop(other); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A copy left in an area the session had to leave goes at once when its document is saved, +/// reverted or closed, and every one goes when the app quits with nothing unsaved. +#[cfg(unix)] +#[test] +fn copies_left_behind_go_when_their_document_is_saved_reverted_or_closed() { + use cmd::recovery::Lock; + let base = std::env::temp_dir().join(format!("vc-recovery-leftgo-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let dir = base.join("recovery"); + let path = base.join("doc.vectorcraft").to_string_lossy().to_string(); + let left = |area: &str| std::fs::read_dir(dir.join(area)).unwrap().flatten().any(|e| e.path().extension().is_some_and(|x| x == "vectorcraft")); + for how in ["save", "revert", "close", "quit"] { + let (mut a, _) = saved_in(&dir); + a.execute("document.save", &json!({"path": path})).unwrap(); + a.execute("shape.ellipse", &json!({"x": 10, "y": 10, "width": 5, "height": 5})).unwrap(); + a.execute("file.recovery.save", &json!({})).unwrap(); + let area = a.recovery.own_area().unwrap().to_string(); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = FolderStore::new(&dir).lock(&area) else { panic!("the other app locks the area") }; + cmd::recovery::heartbeat(&mut a); + assert!(left(&area), "{how}"); + match how { + "save" => a.execute("document.save", &json!({"path": path})).map(drop).unwrap(), + "revert" => a.execute("file.revert", &json!({})).map(drop).unwrap(), + "close" => a.execute("file.close", &json!({})).map(drop).unwrap(), + _ => cmd::recovery::forget_all(&mut a), + } + assert!(!left(&area), "{how}"); + drop(other); + drop(a); + } + let _ = std::fs::remove_dir_all(&base); +} + +/// The lock file was removed: the next heartbeat locks the area again, before any recovery save. +#[cfg(unix)] +#[test] +fn a_heartbeat_locks_the_area_again_when_its_lock_file_was_removed() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-beatlock-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + assert!(cmd::recovery::heartbeat(&mut a).is_empty(), "the copy is still there"); + assert_eq!(a.recovery.own_area(), Some(area.as_str())); + assert!(matches!(FolderStore::new(&dir).lock(&area), Ok(Lock::Busy))); + assert!(offered(&mut other_app(&dir)).is_empty()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The lock file was removed and can't be made again (the area folder can't be written to now): +/// the session takes a new area and writes its copy there. +#[cfg(unix)] +#[test] +fn an_area_whose_lock_file_cannot_be_made_again_is_left_for_a_new_one() { + use std::os::unix::fs::PermissionsExt as _; + + use cmd::recovery::Lock; + // Permissions don't stop the superuser. + if rustix::process::geteuid().is_root() { + return; + } + let dir = std::env::temp_dir().join(format!("vc-recovery-readonly-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + std::fs::set_permissions(dir.join(&area), std::fs::Permissions::from_mode(0o500)).unwrap(); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + let mine = a.recovery.own_area().unwrap().to_string(); + assert_ne!(mine, area); + assert_eq!(r["saved"][0]["file"], format!("{mine}/Untitled-1-1").as_str(), "{r}"); + assert!(matches!(FolderStore::new(&dir).lock(&mine), Ok(Lock::Busy))); + std::fs::set_permissions(dir.join(&area), std::fs::Permissions::from_mode(0o700)).unwrap(); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A copy left in an area the session had to leave is still the session's own after the app that +/// locked that area is gone: the session neither offers nor discards it. +#[cfg(unix)] +#[test] +fn a_copy_left_behind_is_not_offered_by_its_own_app() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-leftmine-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = FolderStore::new(&dir).lock(&area) else { panic!("the other app locks the area") }; + cmd::recovery::heartbeat(&mut a); + drop(other); + assert!(offered(&mut a).is_empty()); + let r = a.execute("file.recovery.discard", &json!({})).unwrap(); + assert!(r["discarded"].as_array().unwrap().is_empty(), "{r}"); + assert!(a.execute("file.recovery.discard", &json!({"file": format!("{area}/Untitled-1-1")})).is_err()); + assert!(dir.join(&area).join("Untitled-1-1.vectorcraft").is_file()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A complex document (Data Recovery off for those) gets no new copy after the session left its +/// area: it keeps the copy it left there, and the heartbeat asks for it only once. +#[cfg(unix)] +#[test] +fn a_complex_documents_copy_left_behind_is_asked_for_once() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-leftcomplex-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let mut a = other_app(&dir); + let mut d = vectorcraft_doc::Document::new(1000.0, 1000.0); + let layer = d.layers[0].id; + for i in 0..cmd::recovery::COMPLEX_OBJECTS { + let id = d.alloc_id(); + let r = vectorcraft_geom::Rect::new((i % 200) as f64, (i / 200) as f64, (i % 200) as f64 + 1.0, (i / 200) as f64 + 1.0); + let n = vectorcraft_doc::Node::path(id, vectorcraft_geom::shapes::rectangle(r), vectorcraft_doc::Appearance::default_art()); + d.insert(Some(layer), i, n).unwrap(); + } + a.add_document(d, None); + a.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 5, "height": 5})).unwrap(); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + let copy = r["saved"][0]["file"].as_str().unwrap().to_string(); + let area = a.recovery.own_area().unwrap().to_string(); + a.prefs.recovery_off_for_complex = true; + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = FolderStore::new(&dir).lock(&area) else { panic!("the other app locks the area") }; + let uid = a.doc().unwrap().uid; + assert_eq!(cmd::recovery::heartbeat(&mut a), [uid]); + let r = a.execute("file.recovery.save", &json!({})).unwrap(); + assert!(r["saved"].as_array().unwrap().is_empty(), "{r}"); + assert!(cmd::recovery::heartbeat(&mut a).is_empty(), "asked for once"); + assert!(dir.join(format!("{copy}.vectorcraft")).is_file(), "its copy stays"); + drop(other); + drop(a); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The preference named another folder while a copy was written in the background: the copy goes +/// from the folder it was written to, and the document keeps its copy in the new folder. +#[test] +fn a_copy_finished_after_the_folder_changed_goes_from_its_own_folder() { + let base = std::env::temp_dir().join(format!("vc-recovery-refolder-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let (old, new) = (base.join("old"), base.join("new")); + let (mut a, area) = saved_in(&old); + a.execute("shape.ellipse", &json!({"x": 10, "y": 10, "width": 5, "height": 5})).unwrap(); + let store: Arc = Arc::new(FolderStore::new(&old)); + let (jobs, _) = cmd::recovery::jobs(&mut a, &store).unwrap(); + a.prefs.recovery_folder = new.to_string_lossy().to_string(); + a.execute("file.recovery.save", &json!({})).unwrap(); + let mine = a.doc().unwrap().recovery.as_ref().unwrap().file.clone(); + for j in jobs { + j.write().unwrap(); + j.finish(&mut a); + } + assert!(!old.join(&area).join("Untitled-1-1.vectorcraft").exists()); + assert_eq!(a.doc().unwrap().recovery.as_ref().unwrap().file, mine); + assert!(new.join(format!("{mine}.vectorcraft")).is_file()); + drop(a); + let _ = std::fs::remove_dir_all(&base); +} + #[test] fn saving_reverting_or_closing_removes_the_copy() { let dir = std::env::temp_dir().join(format!("vc-recovery-save-{}", std::process::id())); diff --git a/crates/ui-egui/src/recovery.rs b/crates/ui-egui/src/recovery.rs index 443d4aad3..f6188babc 100644 --- a/crates/ui-egui/src/recovery.rs +++ b/crates/ui-egui/src/recovery.rs @@ -105,8 +105,8 @@ fn write(app: &mut VectorcraftApp, store: &Arc, pick: impl Fn let mut started = 0; for job in jobs { let (uid, label, title) = (job.uid, format!("Saving recovery data for {}", job.title), job.title.clone()); - let (done, store) = (job.clone(), store.clone()); - let work = move |_: Writer| job.write(store.as_ref()); + let done = job.clone(); + let work = move |_: Writer| job.write(); let then = move |app: &mut VectorcraftApp, r: Result| { let r = r?; done.finish(&mut app.session); From cdeb7946e0108e6bec129623636f095b0a7cc311 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 21:05:30 -0800 Subject: [PATCH 2/2] Data Recovery: a left copy that can't be removed yet stays the session's own drop_left forgot a copy in an area the session left before removing it, so a copy whose removal failed (the area read-only for a moment) could later be offered back to the same session as a crash's copy. The entry now stays until both files are removed, and the next heartbeat tries again. --- crates/engine/src/cmd/recovery.rs | 12 ++++++----- crates/engine/src/tests/recovery.rs | 33 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index bb7fbc6a5..3424da3dd 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -614,12 +614,14 @@ fn drop_left_done(s: &mut Session) { } } -/// Document `uid`'s copy in an area this session left goes. Best effort: a copy that can't be -/// removed is offered once nobody holds that area, which loses nothing. +/// Document `uid`'s copy in an area this session left goes. A copy that can't be removed now stays +/// this session's, and a later heartbeat removes it. fn drop_left(s: &mut Session, uid: u64) { - if let Some((store, file)) = s.recovery.left.remove(&uid) { - let _ = store.remove(&format!("{file}{DOC_EXT}")); - let _ = store.remove(&format!("{file}{META_EXT}")); + let Some((store, file)) = s.recovery.left.get(&uid) else { return }; + let doc = store.remove(&format!("{file}{DOC_EXT}")); + let meta = store.remove(&format!("{file}{META_EXT}")); + if doc.is_ok() && meta.is_ok() { + s.recovery.left.remove(&uid); } } diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index acd982814..f0b6a0708 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -903,6 +903,39 @@ fn an_area_whose_lock_file_cannot_be_made_again_is_left_for_a_new_one() { let _ = std::fs::remove_dir_all(&dir); } +/// A copy left behind that can't be removed when its document is saved (its area can't be written +/// to now) stays the session's own: it isn't offered here, and a later heartbeat removes it. +#[cfg(unix)] +#[test] +fn a_copy_left_behind_that_cannot_be_removed_yet_stays_the_sessions_own() { + use std::os::unix::fs::PermissionsExt as _; + + use cmd::recovery::Lock; + // Permissions don't stop the superuser. + if rustix::process::geteuid().is_root() { + return; + } + let base = std::env::temp_dir().join(format!("vc-recovery-leftstuck-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + let dir = base.join("recovery"); + let path = base.join("doc.vectorcraft").to_string_lossy().to_string(); + let (mut a, area) = saved_in(&dir); + std::fs::remove_file(dir.join(&area).join(".lock")).unwrap(); + let Ok(Lock::Held(other)) = FolderStore::new(&dir).lock(&area) else { panic!("the other app locks the area") }; + cmd::recovery::heartbeat(&mut a); + std::fs::set_permissions(dir.join(&area), std::fs::Permissions::from_mode(0o500)).unwrap(); + a.execute("document.save", &json!({"path": path})).unwrap(); + std::fs::set_permissions(dir.join(&area), std::fs::Permissions::from_mode(0o700)).unwrap(); + let copy = dir.join(&area).join("Untitled-1-1.vectorcraft"); + assert!(copy.is_file()); + drop(other); + assert!(offered(&mut a).is_empty()); + cmd::recovery::heartbeat(&mut a); + assert!(!copy.exists()); + drop(a); + let _ = std::fs::remove_dir_all(&base); +} + /// A copy left in an area the session had to leave is still the session's own after the app that /// locked that area is gone: the session neither offers nor discards it. #[cfg(unix)]