Merge pull request #6710 from dotCooCoo/fix/recovery-tidy-race

Data Recovery: saving no longer fails when another running copy tidies the new area

Cargo.lock

@@ -6499,6 +6499,7 @@ dependencies = [
  "roxmltree 0.21.2",
  "rustix 1.1.6",
  "ruzstd",
+ "same-file",
  "serde",
  "serde_json",
  "skrifa 0.47.0",

crates/engine/Cargo.toml

@@ -67,6 +67,11 @@ log = { workspace = true }
 [target.'cfg(unix)'.dependencies]
 rustix = { workspace = true, features = ["std", "process"] }
 
+# Whether a recovery area's lock file is still the one that was opened (`FolderStore::still_named`).
+# Already in Cargo.lock (walkdir), so this adds no new crate.
+[target.'cfg(windows)'.dependencies]
+same-file = "1.0.6"
+
 # The tiles of a HEIC photo decode in parallel on native; wasm stays single-threaded.
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
 heic-rs = { version = "=0.1.4", default-features = false, features = ["std", "parallel"], optional = true }

crates/engine/src/cmd/recovery.rs

@@ -111,8 +111,13 @@ pub trait RecoveryStore: Send + Sync {
     fn announced(&self, _area: &str) -> Option<bool> {
         None
     }
-    /// Tidy away area `area` once it holds no copies and nobody holds it (its lock file and folder).
+    /// Tidy away area `area` once it holds no copies (its lock file and folder). The caller holds
+    /// the area until this returns.
     fn remove_area(&self, _area: &str) {}
+    /// Remove area `area`'s folder if it is empty, after [`RecoveryStore::remove_area`] and after the
+    /// caller let the area go. On some file systems a file that is open is removed only when it is
+    /// closed, so `remove_area` leaves the folder there while the area is held.
+    fn remove_empty_area(&self, _area: &str) {}
     /// The clock heartbeats are judged by (Unix seconds; `None`: none, so no area counts as gone).
     fn now(&self) -> Option<i64> {
         vectorcraft_doc::metadata::now_unix()
@@ -295,6 +300,73 @@ impl FolderStore {
             .filter(|n| Self::plain(n))
             .collect())
     }
+
+    /// Lock area folder `dir`, which was just made. Another app tidies an empty area away
+    /// (`tidy`) by removing its lock file and folder while it holds the lock. So when the folder
+    /// or the lock file is gone by now, or the lock taken is on a lock file the folder no longer
+    /// has, that app had the area: it is [`Lock::Busy`], and the caller tries another one.
+    pub(crate) fn lock_area(dir: &std::path::Path) -> std::result::Result<Lock, String> {
+        if let Err(e) = Self::own_area(dir) {
+            let gone = std::fs::symlink_metadata(dir).is_err_and(|e| e.kind() == std::io::ErrorKind::NotFound);
+            return if gone { Ok(Lock::Busy) } else { Err(e) };
+        }
+        let path = dir.join(LOCK_FILE);
+        match std::fs::OpenOptions::new().read(true).write(true).create(true).truncate(false).open(&path) {
+            Ok(file) => Self::take(file, &path),
+            Err(e) => Self::open_failed(e, &path),
+        }
+    }
+
+    /// Opening lock file `path` failed with `e`. When the folder is gone by now, another app had the
+    /// area: it is [`Lock::Busy`]. So it is when the lock file is there but can't be opened: some
+    /// file systems keep a removed file that another app still has open until that app closes it,
+    /// and refuse to open it meanwhile. A lock file that can't be made is an error.
+    pub(crate) fn open_failed(e: std::io::Error, path: &std::path::Path) -> std::result::Result<Lock, String> {
+        match e.kind() {
+            std::io::ErrorKind::NotFound => Ok(Lock::Busy),
+            std::io::ErrorKind::PermissionDenied if std::fs::symlink_metadata(path).is_ok() => Ok(Lock::Busy),
+            _ => Err(format!("{}: {e}", path.display())),
+        }
+    }
+
+    /// Lock `file`, opened as lock file `path`. The lock holds the area only while `path` still
+    /// names that file.
+    pub(crate) fn take(file: std::fs::File, path: &std::path::Path) -> std::result::Result<Lock, String> {
+        match file.try_lock() {
+            Ok(()) if Self::still_named(&file, path) => Ok(Lock::Held(Box::new(file))),
+            Ok(()) | Err(std::fs::TryLockError::WouldBlock) => Ok(Lock::Busy),
+            // The lock file was removed meanwhile, and locking it failed instead.
+            Err(std::fs::TryLockError::Error(_)) if !Self::still_named(&file, path) => Ok(Lock::Busy),
+            Err(std::fs::TryLockError::Error(e)) => Err(format!("{}: {e}", path.display())),
+        }
+    }
+
+    /// Does `path` still name `file`? On Unix the two must be the same file (device and inode); on
+    /// Windows they must be the same file (volume serial number and file index); elsewhere `path`
+    /// must exist. When either file can't be read, it does not.
+    fn still_named(file: &std::fs::File, path: &std::path::Path) -> bool {
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::MetadataExt as _;
+            match (file.metadata(), std::fs::symlink_metadata(path)) {
+                (Ok(a), Ok(b)) => a.dev() == b.dev() && a.ino() == b.ino(),
+                _ => false,
+            }
+        }
+        #[cfg(windows)]
+        {
+            let opened = file.try_clone().and_then(same_file::Handle::from_file);
+            match (opened, same_file::Handle::from_path(path)) {
+                (Ok(a), Ok(b)) => a == b,
+                _ => false,
+            }
+        }
+        #[cfg(not(any(unix, windows)))]
+        {
+            let _ = file;
+            std::fs::symlink_metadata(path).is_ok()
+        }
+    }
 }
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -342,29 +414,23 @@ impl RecoveryStore for FolderStore {
     fn lock(&self, area: &str) -> std::result::Result<Lock, String> {
         let dir = self.area(area)?;
         Self::create_area(&dir)?;
-        Self::own_area(&dir)?;
-        let path = dir.join(LOCK_FILE);
-        let file = std::fs::OpenOptions::new()
-            .read(true)
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .open(&path)
-            .map_err(|e| format!("{}: {e}", path.display()))?;
-        match file.try_lock() {
-            Ok(()) => Ok(Lock::Held(Box::new(file))),
-            Err(std::fs::TryLockError::WouldBlock) => Ok(Lock::Busy),
-            Err(std::fs::TryLockError::Error(e)) => Err(format!("{}: {e}", path.display())),
-        }
+        Self::lock_area(&dir)
     }
     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.
+        // (copies came back meanwhile) stays. The caller still holds the lock here.
         if let Ok(dir) = self.area(area) {
             let _ = std::fs::remove_file(dir.join(LOCK_FILE));
             let _ = std::fs::remove_dir(&dir);
         }
     }
+    fn remove_empty_area(&self, area: &str) {
+        // Best effort, as above. An app that made the folder again meanwhile and has not locked it
+        // yet finds it gone and takes another area (`lock_area`); one that has keeps it.
+        if let Ok(dir) = self.area(area) {
+            let _ = std::fs::remove_dir(&dir);
+        }
+    }
 }
 
 /// The area this session keeps its copies in, while it holds it.
@@ -527,8 +593,10 @@ pub fn release(s: &mut Session) {
     for name in store.list().unwrap_or_default().iter().filter(|n| mine(n)) {
         let _ = store.remove(name);
     }
-    drop(hold);
+    // Held until it is gone, as in `tidy`.
     store.remove_area(&area);
+    drop(hold);
+    store.remove_empty_area(&area);
 }
 
 /// Let go of this session's area but leave its copies, as a crash would, so the next app started
@@ -967,7 +1035,9 @@ fn owner(s: &mut Session, store: &dyn RecoveryStore, area: &str, hold: &mut Vec<
     }
 }
 
-/// Tidy away a gone app's area `area` once no copies are left in it, then let it go (`hold`).
+/// 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.
 fn tidy(store: &dyn RecoveryStore, area: &str, hold: Vec<Hold>) {
     let names = store.list().unwrap_or_default();
     let mine: Vec<&String> = names.iter().filter(|n| n.split_once('/').is_some_and(|(a, _)| a == area)).collect();
@@ -977,8 +1047,9 @@ fn tidy(store: &dyn RecoveryStore, area: &str, hold: Vec<Hold>) {
     for n in mine {
         let _ = store.remove(n);
     }
-    drop(hold);
     store.remove_area(area);
+    drop(hold);
+    store.remove_empty_area(area);
 }
 
 /// The copies of documents open now in this session.

crates/engine/src/tests/recovery.rs

@@ -483,6 +483,205 @@ fn the_folder_store_reads_no_linked_entries() {
     let _ = std::fs::remove_dir_all(&base);
 }
 
+/// A store in memory that notes each removal of an area (its lock file and folder, then its empty
+/// folder) and whether the area was still held then.
+#[derive(Default)]
+struct Tidying {
+    inner: MemoryStore,
+    removals: std::sync::Mutex<Vec<(&'static str, bool)>>,
+}
+
+impl Tidying {
+    fn note(&self, what: &'static str, area: &str) {
+        let held = matches!(self.inner.lock(area), Ok(cmd::recovery::Lock::Busy));
+        self.removals.lock().unwrap().push((what, held));
+    }
+}
+
+impl RecoveryStore for Tidying {
+    fn list(&self) -> std::result::Result<Vec<String>, String> {
+        self.inner.list()
+    }
+    fn read(&self, name: &str) -> std::result::Result<Vec<u8>, String> {
+        self.inner.read(name)
+    }
+    fn write(&self, name: &str, bytes: &[u8]) -> std::result::Result<(), String> {
+        self.inner.write(name, bytes)
+    }
+    fn remove(&self, name: &str) -> std::result::Result<(), String> {
+        self.inner.remove(name)
+    }
+    fn location(&self) -> String {
+        self.inner.location()
+    }
+    fn lock(&self, area: &str) -> std::result::Result<cmd::recovery::Lock, String> {
+        self.inner.lock(area)
+    }
+    fn remove_area(&self, area: &str) {
+        self.note("area", area);
+    }
+    fn remove_empty_area(&self, area: &str) {
+        self.note("empty folder", area);
+    }
+    fn now(&self) -> Option<i64> {
+        self.inner.now()
+    }
+}
+
+/// An app tidying an area away (a crashed app's, or its own when it quits) removes it while it
+/// still holds it, so another app taking a new area of that name can't lock it in between. Once it
+/// let the area go, it removes the folder again if that is empty.
+#[test]
+fn areas_are_removed_while_still_held() {
+    let store = Arc::new(Tidying::default());
+    let app = || {
+        let mut s = Session::new();
+        s.recovery.set_store(store.clone());
+        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
+    };
+    let mut a = app();
+    a.execute("file.recovery.save", &json!({})).unwrap();
+    drop(a);
+    let mut b = app();
+    assert_eq!(b.execute("file.recovery.discard", &json!({})).unwrap()["discarded"].as_array().unwrap().len(), 1);
+    b.execute("file.recovery.save", &json!({})).unwrap();
+    b.execute("file.close", &json!({})).unwrap();
+    cmd::recovery::forget_all(&mut b);
+    let tidy = [("area", true), ("empty folder", false)];
+    assert_eq!(*store.removals.lock().unwrap(), [tidy, tidy].concat(), "discard's tidy, then quitting");
+}
+
+/// Another app tidied an empty area away between this app making it and locking it: the area is
+/// busy, not held and not an error.
+#[test]
+fn an_area_tidied_away_before_it_is_locked_is_busy() {
+    use cmd::recovery::Lock;
+    let base = std::env::temp_dir().join(format!("vc-recovery-tidied-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&base);
+    let store = FolderStore::new(&base);
+    drop(store.lock("5-1").unwrap());
+    let area = base.join("5-1");
+    let path = area.join(".lock");
+    let open = |create: bool| std::fs::OpenOptions::new().read(true).write(true).create(create).truncate(false).open(&path).unwrap();
+    // The lock file was opened, then removed before it was locked.
+    let opened = open(false);
+    std::fs::remove_file(&path).unwrap();
+    assert!(matches!(FolderStore::take(opened, &path), Ok(Lock::Busy)));
+    // The folder went before it was checked or its lock file opened.
+    std::fs::remove_dir(&area).unwrap();
+    assert!(matches!(FolderStore::lock_area(&area), Ok(Lock::Busy)));
+    // A lock file made again under the same name is not the one opened, also where it keeps the
+    // removed one's creation time (NTFS does within 15 seconds).
+    #[cfg(any(unix, windows))]
+    {
+        std::fs::create_dir_all(&area).unwrap();
+        let opened = open(true);
+        std::fs::remove_file(&path).unwrap();
+        std::fs::write(&path, b"").unwrap();
+        assert!(matches!(FolderStore::take(opened, &path), Ok(Lock::Busy)));
+        assert!(matches!(FolderStore::take(open(false), &path), Ok(Lock::Held(_))));
+    }
+    let _ = std::fs::remove_dir_all(&base);
+}
+
+/// Opening an area's lock file failed. When the folder went after it was checked, or the lock file
+/// is there but can't be opened (a removed lock file that another app still has open, on file
+/// systems that keep it until it is closed), the area is busy. A lock file that can't be made is an
+/// error.
+#[test]
+fn a_lock_file_that_is_gone_or_cannot_be_opened_is_busy() {
+    use cmd::recovery::Lock;
+    use std::io::ErrorKind::{NotFound, PermissionDenied};
+    let base = std::env::temp_dir().join(format!("vc-recovery-unopened-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&base);
+    std::fs::create_dir_all(&base).unwrap();
+    let path = base.join(".lock");
+    let failed = |kind: std::io::ErrorKind| FolderStore::open_failed(kind.into(), &path);
+    assert!(matches!(failed(NotFound), Ok(Lock::Busy)));
+    assert!(failed(PermissionDenied).is_err());
+    std::fs::write(&path, b"").unwrap();
+    assert!(matches!(failed(PermissionDenied), Ok(Lock::Busy)));
+    #[cfg(unix)]
+    if !rustix::process::geteuid().is_root() {
+        use std::os::unix::fs::PermissionsExt as _;
+        let mode = |p: &std::path::Path, m: u32| std::fs::set_permissions(p, std::fs::Permissions::from_mode(m)).unwrap();
+        // A lock file that is there but can't be opened.
+        let area = base.join("5-1");
+        std::fs::create_dir(&area).unwrap();
+        std::fs::write(area.join(".lock"), b"").unwrap();
+        mode(&area.join(".lock"), 0o400);
+        assert!(matches!(FolderStore::lock_area(&area), Ok(Lock::Busy)));
+        // A folder in which no lock file can be made.
+        let area = base.join("5-2");
+        std::fs::create_dir(&area).unwrap();
+        mode(&area, 0o500);
+        let r = FolderStore::lock_area(&area);
+        mode(&area, 0o700);
+        assert!(r.is_err());
+    }
+    let _ = std::fs::remove_dir_all(&base);
+}
+
+/// A folder store in which another app tidies the first area away as soon as it is made.
+struct TidiedFirst {
+    inner: FolderStore,
+    dir: std::path::PathBuf,
+    first: std::sync::atomic::AtomicBool,
+}
+
+impl RecoveryStore for TidiedFirst {
+    fn list(&self) -> std::result::Result<Vec<String>, String> {
+        self.inner.list()
+    }
+    fn read(&self, name: &str) -> std::result::Result<Vec<u8>, String> {
+        self.inner.read(name)
+    }
+    fn write(&self, name: &str, bytes: &[u8]) -> std::result::Result<(), String> {
+        self.inner.write(name, bytes)
+    }
+    fn remove(&self, name: &str) -> std::result::Result<(), String> {
+        self.inner.remove(name)
+    }
+    fn location(&self) -> String {
+        self.inner.location()
+    }
+    fn areas(&self) -> std::result::Result<Vec<String>, String> {
+        self.inner.areas()
+    }
+    fn lock(&self, area: &str) -> std::result::Result<cmd::recovery::Lock, String> {
+        if !self.first.swap(false, std::sync::atomic::Ordering::SeqCst) {
+            return self.inner.lock(area);
+        }
+        let dir = self.dir.join(area);
+        std::fs::create_dir_all(&dir).unwrap();
+        std::fs::remove_dir(&dir).unwrap();
+        FolderStore::lock_area(&dir)
+    }
+    fn remove_area(&self, area: &str) {
+        self.inner.remove_area(area);
+    }
+}
+
+#[test]
+fn a_recovery_save_takes_the_next_area_when_the_first_is_tidied_away() {
+    let dir = std::env::temp_dir().join(format!("vc-recovery-next-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    let store = Arc::new(TidiedFirst { inner: FolderStore::new(&dir), dir: dir.clone(), first: true.into() });
+    let mut s = Session::new();
+    s.recovery.set_store(store.clone());
+    s.execute("file.new", &json!({"width": 50, "height": 50})).unwrap();
+    s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 5, "height": 5})).unwrap();
+    let r = s.execute("file.recovery.save", &json!({})).unwrap();
+    let area = s.recovery.own_area().unwrap().to_string();
+    assert!(area.ends_with("-2"), "{area}");
+    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());
+    drop(s);
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 #[test]
 fn saving_reverting_or_closing_removes_the_copy() {
     let dir = std::env::temp_dir().join(format!("vc-recovery-save-{}", std::process::id()));