From 05dcab02848317b3467fef419f3423d7d1b4ae11 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 23:46:07 -0800 Subject: [PATCH 1/2] Data Recovery: another app leaves a copy alone until its app writes it again When a running app has to leave its recovery area (its lock file was removed and another app locked the area), its copies stay in the old area until it writes them again in its new area. Only that app knew this, so once the other app let the old area go, any app could offer, restore or discard those copies while their app still ran. The app now writes an entry `/keeps-` in its own area while copies of its documents are left in the old one, and removes it when none are. Another app that finds an area unlocked checks for such an entry, and treats the area as a running app's while the area that names it is held. After a crash the new area is no longer held, so the copies are offered as before. The heartbeat writes the entry again if it was removed. --- crates/engine/src/cmd/recovery.rs | 95 ++++++++++++++++++++++++++++- crates/engine/src/tests/recovery.rs | 48 +++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index 3424da3dd..db2d1ecd4 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -19,7 +19,9 @@ //! while their copies are restored or discarded the area is held, so two apps launched together //! 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. +//! takes a new area. The copies it had in the old area stay there until it has written them again +//! in the new one, and an entry `/keeps-` ([`mark_left`]) tells other apps +//! that those copies belong to a running app, so none of them offers or discards them meanwhile. //! //! 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 @@ -61,6 +63,9 @@ const BLOB_EXT: &str = ".blob"; /// The heartbeat entry of an area in a store without locks: when its app last said it runs (Unix /// seconds, as text). const BEAT: &str = "heartbeat"; +/// The start of an entry `/keeps-`: the app holding `` still keeps the copies +/// it left in `` ([`mark_left`]). +const KEEPS: &str = "keeps-"; /// The least time without a heartbeat after which an area's app counts as gone (seconds); else /// three Data Recovery intervals. const MIN_STALE: i64 = 180; @@ -479,6 +484,8 @@ pub struct Recovery { /// 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)>, + /// The `/keeps-` entries this session wrote in its area ([`mark_left`]). + marked: BTreeSet, } impl Recovery { @@ -553,6 +560,15 @@ fn beating(store: &dyn RecoveryStore, area: &str, stale: i64, mine: Option) /// 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); + // An entry that couldn't be written before, or that was removed since (by the user, a + // cleaner), is written now. + if let Some(own) = &s.recovery.own + && !s.recovery.marked.is_empty() + && let Ok(names) = own.store.list() + { + s.recovery.marked.retain(|n| names.contains(n)); + } + mark_left(s); if let Some(own) = s.recovery.own.as_ref().filter(|o| o.beats) { let _ = beat(own.store.as_ref(), &own.area); } @@ -622,6 +638,36 @@ fn drop_left(s: &mut Session, uid: u64) { let meta = store.remove(&format!("{file}{META_EXT}")); if doc.is_ok() && meta.is_ok() { s.recovery.left.remove(&uid); + mark_left(s); + } +} + +/// Record in this session's area each area it left that still holds copies of its documents: an +/// entry `/keeps-`, so another app leaves those copies alone while this one +/// runs ([`keeper`]). The entries of areas that hold none of its copies any more go. Best effort: +/// an entry not written or removed now is written or removed at the next heartbeat. +fn mark_left(s: &mut Session) { + let Some(own) = &s.recovery.own else { return }; + let (store, area) = (own.store.clone(), own.area.clone()); + let want: BTreeSet = s + .recovery + .left + .values() + .filter(|(st, _)| st.location() == store.location()) + .filter_map(|(_, f)| f.split_once('/').map(|(a, _)| a)) + .filter(|a| *a != area) + .map(|a| format!("{area}/{KEEPS}{a}")) + .collect(); + let gone: Vec = s.recovery.marked.difference(&want).cloned().collect(); + for n in gone { + if store.remove(&n).is_ok() { + s.recovery.marked.remove(&n); + } + } + for n in want { + if !s.recovery.marked.contains(&n) && store.write(&n, b"").is_ok() { + s.recovery.marked.insert(n); + } } } @@ -670,6 +716,9 @@ pub fn claim(s: &mut Session, store: &Arc) -> Result } }; s.recovery.own = Some(Own { store: store.clone(), area: area.clone(), hold, beats }); + // The entries written before are in the area this session had (released or left). + s.recovery.marked.clear(); + mark_left(s); return Ok(area); } Err(EngineError::Other("no free recovery area".into())) @@ -686,6 +735,8 @@ pub fn release(s: &mut Session) { drop_left(s, uid); } let Some(Own { store, area, hold, .. }) = s.recovery.own.take() else { return }; + // Its `keeps-` entries go with the area's other entries. + s.recovery.marked.clear(); 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)) { let _ = store.remove(name); @@ -931,6 +982,7 @@ impl RecoveryJob { let _ = store.remove(&format!("{file}{DOC_EXT}")); let _ = store.remove(&format!("{file}{META_EXT}")); } + mark_left(s); } // Best effort, as in `remove_copy`. _ => { @@ -1152,6 +1204,10 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec< } match store.lock(area) { Ok(Lock::Held(h)) => { + // Copies an app left here when it had to leave the area stay its own while it runs. + if let Some(who) = keeper(s, store, area) { + return who; + } hold.push(h); Owner::Gone } @@ -1160,6 +1216,9 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec< if store.announced(area).unwrap_or(false) || beating(store, area, stale_after(s), s.recovery.adopted.get(area).copied()) { return Owner::Running; } + if let Some(who) = keeper(s, store, area) { + return who; + } // Best effort: without it another app might offer the copies too, which loses nothing. if take && let Ok(t) = beat(store, area) { s.recovery.adopted.insert(area.to_string(), t); @@ -1169,6 +1228,40 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec< } } +/// Who keeps the copies left in `area` by an app that had to leave it, from the entries +/// `/keeps-` ([`mark_left`]): this session, or another app that is running. `None`: +/// no app that runs keeps them. A store that can't be listed now keeps them (a copy kept loses +/// nothing). +fn keeper(s: &Session, store: &dyn RecoveryStore, area: &str) -> Option { + let Ok(names) = store.list() else { return Some(Owner::Running) }; + let mark = format!("/{KEEPS}{area}"); + let mut who = None; + for k in names.iter().filter_map(|n| n.strip_suffix(mark.as_str())).filter(|k| *k != area) { + match holder(s, store, k) { + Owner::Mine => return Some(Owner::Mine), + Owner::Running => who = Some(Owner::Running), + Owner::Gone => {} + } + } + who +} + +/// Whose area `area` is, without taking it over: this session's, a running app's, or nobody's (a +/// lock taken to tell goes at once). +fn holder(s: &Session, store: &dyn RecoveryStore, area: &str) -> Owner { + if s.recovery.own.as_ref().is_some_and(|o| o.area == area && o.store.location() == store.location()) { + return Owner::Mine; + } + match store.lock(area) { + Ok(Lock::Held(_)) => Owner::Gone, + Ok(Lock::Busy) | Err(_) => Owner::Running, + Ok(Lock::Unsupported) => { + let runs = store.announced(area).unwrap_or(false) || beating(store, area, stale_after(s), s.recovery.adopted.get(area).copied()); + if runs { Owner::Running } else { Owner::Gone } + } + } +} + /// Tidy away a gone app's area `area` once no copies are left in it, then let it go (`hold`). The /// area is removed while it is still held, so an app taking a new area of that name meanwhile /// finds it held or gone and takes the next name. diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index f0b6a0708..84e212c82 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -958,6 +958,54 @@ fn a_copy_left_behind_is_not_offered_by_its_own_app() { let _ = std::fs::remove_dir_all(&dir); } +/// A copy left in an area the session had to leave is not another app's to offer, restore or +/// discard while the session runs, even after the app that locked that area let it go: an entry in +/// the session's new area says it keeps it. Once the copy is written again in the new area, the +/// entry goes; after a crash before that, the copy is offered. +#[cfg(unix)] +#[test] +fn a_copy_left_behind_is_not_offered_by_another_app_while_its_app_runs() { + use cmd::recovery::Lock; + let dir = std::env::temp_dir().join(format!("vc-recovery-leftother-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let left_behind = |area: &str| dir.join(area).join("Untitled-1-1.vectorcraft"); + let leave = |a: &mut Session, area: &str| { + 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(a); + drop(other); + a.recovery.own_area().unwrap().to_string() + }; + let (mut a, area) = saved_in(&dir); + let mine = leave(&mut a, &area); + assert_ne!(mine, area); + assert!(dir.join(&mine).join(format!("keeps-{area}")).is_file()); + // Removed meanwhile: the next heartbeat writes it again. + std::fs::remove_file(dir.join(&mine).join(format!("keeps-{area}"))).unwrap(); + cmd::recovery::heartbeat(&mut a); + assert!(dir.join(&mine).join(format!("keeps-{area}")).is_file()); + let mut b = other_app(&dir); + assert!(offered(&mut b).is_empty()); + assert_eq!(restored(b.execute("file.recovery.restore", &json!({})).unwrap()), 0); + assert!(b.execute("file.recovery.discard", &json!({})).unwrap()["discarded"].as_array().unwrap().is_empty()); + assert!(b.execute("file.recovery.discard", &json!({"file": format!("{area}/Untitled-1-1")})).is_err()); + assert!(left_behind(&area).is_file()); + // Written again in the new area: the copy left behind and the entry go. + a.execute("file.recovery.save", &json!({})).unwrap(); + assert!(!left_behind(&area).exists()); + assert!(!dir.join(&mine).join(format!("keeps-{area}")).exists()); + assert!(offered(&mut b).is_empty()); + drop(a); + let _ = std::fs::remove_dir_all(&dir); + // A crash before the copy is written again: the copy left behind is offered. + let (mut a, area) = saved_in(&dir); + leave(&mut a, &area); + drop(a); + assert_eq!(offered(&mut other_app(&dir)).len(), 1); + assert!(left_behind(&area).is_file()); + 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)] From 1d103695c4a3133248660d57524068114e8b15dc Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 12 Nov 2026 00:12:16 -0800 Subject: [PATCH 2/2] Data Recovery: a keeps- entry counts only in an area this account can lock An entry `/keeps-` kept the copies in the left area whenever the lock check on `` failed, so an entry in a folder of another account, or in one whose lock file can't be opened, kept a crashed app's copies from being offered. Such an entry now keeps nothing: only an area that this session holds, or that a running app of this account has locked, keeps the copies it names. --- crates/engine/src/cmd/recovery.rs | 12 ++-- crates/engine/src/tests/recovery.rs | 88 +++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index db2d1ecd4..a2395c744 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -1230,8 +1230,9 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec< /// Who keeps the copies left in `area` by an app that had to leave it, from the entries /// `/keeps-` ([`mark_left`]): this session, or another app that is running. `None`: -/// no app that runs keeps them. A store that can't be listed now keeps them (a copy kept loses -/// nothing). +/// no app that runs keeps them. An entry counts only in an area this account can lock or tell is +/// held ([`holder`]), so an entry in a folder of another account, or in one whose lock can't be +/// checked, keeps nothing. A store that can't be listed now keeps them (a copy kept loses nothing). fn keeper(s: &Session, store: &dyn RecoveryStore, area: &str) -> Option { let Ok(names) = store.list() else { return Some(Owner::Running) }; let mark = format!("/{KEEPS}{area}"); @@ -1247,14 +1248,15 @@ fn keeper(s: &Session, store: &dyn RecoveryStore, area: &str) -> Option { } /// Whose area `area` is, without taking it over: this session's, a running app's, or nobody's (a -/// lock taken to tell goes at once). +/// lock taken to tell goes at once). An area whose lock fails (not a recovery area of this +/// account, a lock file that can't be opened) is nobody's. fn holder(s: &Session, store: &dyn RecoveryStore, area: &str) -> Owner { if s.recovery.own.as_ref().is_some_and(|o| o.area == area && o.store.location() == store.location()) { return Owner::Mine; } match store.lock(area) { - Ok(Lock::Held(_)) => Owner::Gone, - Ok(Lock::Busy) | Err(_) => Owner::Running, + Ok(Lock::Held(_)) | Err(_) => Owner::Gone, + Ok(Lock::Busy) => Owner::Running, Ok(Lock::Unsupported) => { let runs = store.announced(area).unwrap_or(false) || beating(store, area, stale_after(s), s.recovery.adopted.get(area).copied()); if runs { Owner::Running } else { Owner::Gone } diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index 84e212c82..5a0ece5e9 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -1006,6 +1006,94 @@ fn a_copy_left_behind_is_not_offered_by_another_app_while_its_app_runs() { let _ = std::fs::remove_dir_all(&dir); } +/// A store in memory whose areas named `foreign-…` can't be locked by this account (as another +/// account's folder in a shared recovery folder). +#[derive(Default)] +struct ForeignAreas(MemoryStore); + +impl RecoveryStore for ForeignAreas { + fn list(&self) -> std::result::Result, String> { + self.0.list() + } + fn read(&self, name: &str) -> std::result::Result, String> { + self.0.read(name) + } + fn write(&self, name: &str, bytes: &[u8]) -> std::result::Result<(), String> { + self.0.write(name, bytes) + } + fn remove(&self, name: &str) -> std::result::Result<(), String> { + self.0.remove(name) + } + fn location(&self) -> String { + self.0.location() + } + fn lock(&self, area: &str) -> std::result::Result { + if area.starts_with("foreign-") { + return Err(format!("{area}: belongs to another account")); + } + self.0.lock(area) + } + fn now(&self) -> Option { + self.0.now() + } +} + +/// An entry `/keeps-` in an area this account can't lock keeps nothing: a crashed +/// app's copies in the left area are still offered. +#[test] +fn a_keeps_entry_in_an_area_that_cannot_be_locked_keeps_nothing() { + let store = Arc::new(ForeignAreas::default()); + let app = || { + let mut s = Session::new(); + s.recovery.set_store(store.clone()); + s + }; + let mut a = app(); + a.execute("file.new", &json!({"width": 50, "height": 50})).unwrap(); + a.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 5, "height": 5})).unwrap(); + a.execute("file.recovery.save", &json!({})).unwrap(); + let area = a.recovery.own_area().unwrap().to_string(); + drop(a); + store.write(&format!("foreign-1/keeps-{area}"), b"").unwrap(); + let mut b = app(); + assert_eq!(offered(&mut b).len(), 1); + assert_eq!(restored(b.execute("file.recovery.restore", &json!({})).unwrap()), 1); +} + +/// An entry `/keeps-` in a running app's area keeps a crashed app's copies in the +/// left area until that app is gone too. +#[test] +fn a_keeps_entry_in_a_running_apps_area_keeps_the_copies() { + let store = Arc::new(MemoryStore::default()); + let mut a = session_in(&store); + a.execute("file.recovery.save", &json!({})).unwrap(); + let area = a.recovery.own_area().unwrap().to_string(); + drop(a); + let mut c = session_in(&store); + c.execute("file.recovery.save", &json!({})).unwrap(); + let keeper = c.recovery.own_area().unwrap().to_string(); + store.write(&format!("{keeper}/keeps-{area}"), b"").unwrap(); + let mut b = relaunch(&store); + assert!(offered(&mut b).is_empty()); + assert_eq!(restored(b.execute("file.recovery.restore", &json!({})).unwrap()), 0); + drop(c); + assert_eq!(offered(&mut b).len(), 2); +} + +/// An entry `/keeps-` in a folder whose lock file can't be opened keeps nothing. +#[cfg(unix)] +#[test] +fn a_keeps_entry_in_a_folder_whose_lock_cannot_be_checked_keeps_nothing() { + let dir = std::env::temp_dir().join(format!("vc-recovery-keepsunlocked-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let (a, area) = saved_in(&dir); + drop(a); + std::fs::create_dir_all(dir.join("keeper").join(".lock")).unwrap(); + std::fs::write(dir.join("keeper").join(format!("keeps-{area}")), b"").unwrap(); + assert_eq!(offered(&mut other_app(&dir)).len(), 1); + 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)]