diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index f3f2c68764..c1c7f789fe 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -1276,15 +1276,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); @@ -1295,7 +1296,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. @@ -1311,12 +1312,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), @@ -1378,7 +1381,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); @@ -1418,10 +1421,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 1f3867a622..976d217833 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]