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

When a second running copy listed the recovery folder, it could lock an
area that this copy had just made but not yet locked, take it for a
crashed copy's empty area and remove it. Locking the area then failed,
and the recovery save reported "No such file or directory". On macOS the
lock could also fail with "Invalid argument" on the removed lock file.

Taking a new area now counts it as busy, and tries the next name, when
its folder or lock file is gone by the time it is locked, when its lock
file is there but cannot be opened (some file systems keep a removed
file that another copy still has open until it is closed), or when the
lock file that was locked is no longer the one in the folder.

Tidying an empty area, and quitting, now remove the area while it is
still locked, and remove its folder once more after letting it go if
the folder is empty.

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,72 @@ 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 have the same creation time; elsewhere `path` must exist.
+    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)]
+        {
+            use std::os::windows::fs::MetadataExt as _;
+            match (file.metadata(), std::fs::symlink_metadata(path)) {
+                (Ok(a), Ok(b)) => a.creation_time() == b.creation_time(),
+                _ => false,
+            }
+        }
+        #[cfg(not(any(unix, windows)))]
+        {
+            let _ = file;
+            std::fs::symlink_metadata(path).is_ok()
+        }
+    }
 }
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -342,29 +413,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 +592,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 +1034,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 +1046,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,204 @@ 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.
+    #[cfg(unix)]
+    {
+        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()));