From d5572f8a6cd92c783d64c9cf9d2a8a2f56761e02 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Thu, 12 Nov 2026 01:36:09 -0800 Subject: [PATCH] Data Recovery: a keeps- entry in an area the same restore or discard holds keeps nothing Restore and discard hold each crashed app's area until they finish. When a later area's copies were kept by an entry in an area already held, the check took a second lock on that area, found it taken, and counted the area as running, so its copies were listed but neither restored nor discarded. Areas the command already holds now count as gone in that check. --- crates/engine/src/cmd/recovery.rs | 24 ++++++++++++++---------- crates/engine/src/tests/recovery.rs | 25 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index fce76bcc27..6add9cd7be 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -1253,15 +1253,16 @@ enum Owner { /// dropped) and, with `take` in a store without locks, taken over with a fresh heartbeat, so /// another app leaves it alone meanwhile. In a store without locks an area is gone only when /// nothing says its app runs: no announcement (or the store can't tell) and no heartbeat for three -/// intervals but the one this session wrote taking it over. -fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec, take: bool) -> Owner { +/// intervals but the one this session wrote taking it over. `held` names the gone apps' areas the +/// caller already holds. +fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec, take: bool, held: &[&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(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) { + if let Some(who) = keeper(s, store, area, held) { return who; } hold.push(h); @@ -1272,7 +1273,7 @@ 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) { + if let Some(who) = keeper(s, store, area, held) { return who; } // Best effort: without it another app might offer the copies too, which loses nothing. @@ -1288,12 +1289,14 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec< /// `/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 { +/// checked, keeps nothing. So does an entry in one of the gone apps' areas the caller holds +/// (`held`), whose lock this session would otherwise find taken. A store that can't be listed now +/// keeps them (a copy kept loses nothing). +fn keeper(s: &Session, store: &dyn RecoveryStore, area: &str, held: &[&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) { + for k in names.iter().filter_map(|n| n.strip_suffix(mark.as_str())).filter(|k| *k != area && !held.contains(k)) { match holder(s, store, k) { Owner::Mine => return Some(Owner::Mine), Owner::Running => who = Some(Owner::Running), @@ -1355,7 +1358,7 @@ fn list(s: &mut Session, _: &Value) -> Result { let mut rows = vec![]; for area in store.areas().map_err(EngineError::Other)? { let mut hold = vec![]; - let who = owner(s, store.as_ref(), &area, &mut hold, false); + let who = owner(s, store.as_ref(), &area, &mut hold, false, &[]); let files: Vec<&String> = copies.iter().filter(|(a, _)| *a == area).map(|(_, f)| f).collect(); for f in &files { let meta = read_meta(store.as_ref(), f); @@ -1395,10 +1398,11 @@ fn targets(s: &mut Session, store: &dyn RecoveryStore, cmd: &str, p: &Value) -> } 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![]; + let mut out: Vec = vec![]; for area in areas { let mut hold = vec![]; - match (owner(s, store, area, &mut hold, true), file) { + let held: Vec<&str> = out.iter().map(|(a, _, _)| a.as_str()).collect(); + match (owner(s, store, area, &mut hold, true, &held), file) { (Owner::Gone, _) => { let files = copies.iter().filter(|(a, c)| a == area && picked(c)).map(|(_, c)| c.clone()).collect(); out.push((area.to_string(), files, hold)); diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index 8248f12a48..95387b4054 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/crates/engine/src/tests/recovery.rs @@ -1179,6 +1179,31 @@ fn a_keeps_entry_in_a_running_apps_area_keeps_the_copies() { assert_eq!(offered(&mut b).len(), 2); } +/// An entry `/keeps-` in a crashed app's area keeps nothing while restore or +/// discard holds that area: both crashed apps' copies are restored, and discarded. +#[test] +fn a_keeps_entry_in_a_crashed_apps_area_held_by_the_same_command_keeps_nothing() { + let crashed = |store: &Arc| { + let mut s = session_in(store); + s.execute("file.recovery.save", &json!({})).unwrap(); + s.recovery.own_area().unwrap().to_string() + }; + for cmd in ["file.recovery.restore", "file.recovery.discard"] { + let store = Arc::new(MemoryStore::default()); + let a = crashed(&store); + let c = crashed(&store); + assert_ne!(a, c); + // The area that sorts first, which the command holds first, keeps the other one's copies. + let (first, second) = if a < c { (a, c) } else { (c, a) }; + store.write(&format!("{first}/keeps-{second}"), b"").unwrap(); + let mut b = relaunch(&store); + assert_eq!(offered(&mut b).len(), 2); + let r = b.execute(cmd, &json!({})).unwrap(); + let done = if cmd == "file.recovery.restore" { restored(r) } else { r["discarded"].as_array().unwrap().len() }; + assert_eq!(done, 2, "{cmd}"); + } +} + /// An entry `/keeps-` in a folder whose lock file can't be opened keeps nothing. #[cfg(unix)] #[test]