diff --git a/Cargo.lock b/Cargo.lock index 5c2bdbdbc..f6bebf778 100644 --- a/Cargo.lock +++ b/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", diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 836d4b744..0fccbcdec 100644 --- a/crates/engine/Cargo.toml +++ b/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 } diff --git a/crates/engine/src/cmd/recovery.rs b/crates/engine/src/cmd/recovery.rs index 4cf070c6a..b75066ec6 100644 --- a/crates/engine/src/cmd/recovery.rs +++ b/crates/engine/src/cmd/recovery.rs @@ -111,8 +111,13 @@ pub trait RecoveryStore: Send + Sync { fn announced(&self, _area: &str) -> Option { 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 { 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 { + 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 { + 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 { + 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 { 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) { 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) { 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. diff --git a/crates/engine/src/tests/recovery.rs b/crates/engine/src/tests/recovery.rs index 9fe2d846c..4994a9072 100644 --- a/crates/engine/src/tests/recovery.rs +++ b/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>, +} + +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, String> { + self.inner.list() + } + fn read(&self, name: &str) -> std::result::Result, 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 { + 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 { + 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, String> { + self.inner.list() + } + fn read(&self, name: &str) -> std::result::Result, 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, String> { + self.inner.areas() + } + fn lock(&self, area: &str) -> std::result::Result { + 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()));