diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index 7f5f33e34..fce76bcc2 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; @@ -535,6 +540,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 { @@ -609,6 +616,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); } @@ -678,6 +694,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); + } } } @@ -726,6 +772,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())) @@ -742,6 +791,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); @@ -987,6 +1038,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`. _ => { @@ -1208,6 +1260,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 } @@ -1216,6 +1272,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); @@ -1225,6 +1284,42 @@ 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. 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}"); + 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). 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(_)) | 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 } + } + } +} + /// 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 0ba51f20a..8248f12a4 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -1057,6 +1057,142 @@ 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 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)]