diff --git a/crates/catalog/src/lock.rs b/crates/catalog/src/lock.rs index 879127c5b..8a2b496d6 100644 --- a/crates/catalog/src/lock.rs +++ b/crates/catalog/src/lock.rs @@ -7,9 +7,15 @@ //! takes the lock first, and a second opener gets [`LockError::InUse`] instead. //! //! - The lock is the OS's (`flock` on Unix, `LockFileEx` on Windows, via [`std::fs::File::try_lock`]): -//! it is released when the [`LibraryLock`] is dropped and, after a crash or a kill, by the OS when -//! the process ends. A `catalog.lock` file left behind is therefore never "stale": only a lock -//! that's actually held refuses the open. +//! it is released explicitly when the [`LibraryLock`] is dropped and, after a crash or a kill, by +//! the OS when the process ends. A `catalog.lock` file left behind is therefore never "stale": +//! only a lock that's actually held refuses the open. +//! - Dropping the lock calls [`std::fs::File::unlock`] rather than relying on the handle closing. +//! On Unix an `flock` lock belongs to the open file description and lasts while *any* descriptor +//! on it is open; a process that another thread is spawning holds a copy of every descriptor +//! (`O_CLOEXEC` only closes them at `execve`), so closing ours would leave the library "already +//! open" for a moment and a re-open would be refused (issue #680). `LOCK_UN` through our +//! descriptor frees the lock whatever copies a child still holds. //! - Who holds it (process id, computer, program, version) is written to `catalog.lock.owner`, for //! the error message. It's a separate file because a Windows lock also blocks reading the //! locked file. It's only a hint: it may be missing or left over from a crash. @@ -87,7 +93,7 @@ pub enum LockError { pub struct LibraryLock { dir: PathBuf, /// `None` when the file system can't lock (the library is open unlocked). - _file: Option, + file: Option, held: bool, } @@ -96,7 +102,7 @@ impl LibraryLock { pub fn acquire(dir: &Path, program: &str) -> Result { let unlocked = |why: io::Error| { log::warn!("library {}: can't lock {LOCK} ({why}); opening it without protection against a second program", dir.display()); - LibraryLock { dir: dir.to_path_buf(), _file: None, held: false } + LibraryLock { dir: dir.to_path_buf(), file: None, held: false } }; let file = match OpenOptions::new().read(true).write(true).create(true).truncate(false).open(dir.join(LOCK)) { Ok(f) => f, @@ -116,7 +122,7 @@ impl LibraryLock { { log::warn!("library {}: can't write {OWNER}: {e}", dir.display()); } - Ok(LibraryLock { dir: dir.to_path_buf(), _file: Some(file), held: true }) + Ok(LibraryLock { dir: dir.to_path_buf(), file: Some(file), held: true }) } /// The OS lock is held (`false`: the file system can't lock; opened unprotected). @@ -127,14 +133,29 @@ impl LibraryLock { pub fn dir(&self) -> &Path { &self.dir } + + /// Another descriptor on the locked file: what a process spawned by another thread holds + /// between fork and exec (a `dup` refers to the same open file description). Tests only. + #[cfg(test)] + pub(crate) fn descriptor_copy(&self) -> Option { + self.file.as_ref().and_then(|f| f.try_clone().ok()) + } } impl Drop for LibraryLock { /// Remove the owner note while still holding the lock (so it can't delete the next holder's), - /// then the file handle closes and the OS releases the lock. + /// then release the lock explicitly. Closing the handle isn't enough on Unix: `flock` lasts + /// while any copy of the descriptor is open, and a process another thread is spawning holds a + /// copy of every descriptor until it execs (issue #680). If the unlock fails the handle still + /// closes right after, which releases the lock once every copy is gone, as before. fn drop(&mut self) { if self.held { let _ = std::fs::remove_file(self.dir.join(OWNER)); + if let Some(f) = &self.file + && let Err(e) = f.unlock() + { + log::warn!("library {}: can't release {LOCK}: {e}", self.dir.display()); + } } } } diff --git a/crates/catalog/src/tests_lock.rs b/crates/catalog/src/tests_lock.rs index c885ec490..78755bdff 100644 --- a/crates/catalog/src/tests_lock.rs +++ b/crates/catalog/src/tests_lock.rs @@ -91,3 +91,69 @@ fn lock_holder_child() { std::fs::write(std::path::Path::new(&dir).join("child-locked"), b"").unwrap(); std::thread::sleep(std::time::Duration::from_secs(120)); } + +/// Issue #680: a process spawned by any thread holds a copy of every descriptor until it execs +/// (close-on-exec only closes them then), and `flock` lasts while any copy of the descriptor is +/// open. Dropping the `LibraryLock` must still free the library, so it releases explicitly rather +/// than relying on its handle closing. A `try_clone` of the descriptor stands in for the child's +/// copy: it refers to the same open file description, which is exactly what the child gets. +#[cfg(unix)] +#[test] +fn the_library_is_free_once_dropped_even_if_a_child_holds_a_copy() { + let dir = temp_dir("copy"); + let first = LibraryLock::acquire(&dir, "LightCraft").unwrap(); + let copy = first.descriptor_copy().unwrap(); + drop(first); + let again = LibraryLock::acquire(&dir, "lightcraft-cli").expect("the library is free once its LibraryLock is dropped"); + assert!(again.held()); + drop((again, copy)); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Issue #680, as it happens in practice: one thread spawns cheap children while another acquires, +/// drops and re-acquires the lock on its own directory. Every acquire must succeed (before the fix +/// a few per cent were refused with `InUse(None)`). Bounded: about two seconds of cycles, pass or fail. +#[cfg(unix)] +#[test] +fn dropping_the_lock_frees_the_library_while_another_thread_spawns_processes() { + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; + + let dir = temp_dir("spawn"); + let stop = Arc::new(AtomicBool::new(false)); + let spawner = { + let stop = Arc::clone(&stop); + std::thread::spawn(move || { + let mut spawned = 0usize; + while !stop.load(Ordering::Relaxed) { + if let Ok(mut c) = + std::process::Command::new("/bin/true").stdout(std::process::Stdio::null()).stderr(std::process::Stdio::null()).spawn() + { + let _ = c.wait(); + spawned += 1; + } + } + spawned + }) + }; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + let mut cycles = 0usize; + let mut refused = Vec::new(); + while cycles < 200_000 && std::time::Instant::now() < deadline { + match LibraryLock::acquire(&dir, "LightCraft") { + Ok(l) => assert!(l.held()), // dropped here: the next cycle must get it back + Err(e) => refused.push(format!("cycle {cycles}: {e:?}")), + } + cycles += 1; + } + stop.store(true, Ordering::Relaxed); + let spawned = spawner.join().unwrap(); + let _ = std::fs::remove_dir_all(&dir); + assert!(spawned > 0, "the spawning thread never got a child off the ground"); + assert!( + refused.is_empty(), + "{} of {cycles} re-acquires refused while {spawned} children were spawned: {:?}", + refused.len(), + &refused[..refused.len().min(3)] + ); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index fb1fd3165..53b5093d0 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -282,6 +282,11 @@ ) ### Reliability +- Closing a library releases its lock explicitly (issue #680). On Linux and macOS the lock used to go with the file + handle, and a program being started by another thread at that moment held a copy of the handle until it was fully + launched, so reopening the library straight away could be refused with "This library is already open in another + LightCraft program" although nothing had it open. Nothing in the shipped app is known to hit the window today; it + made a lock test flaky, and the fix closes the window for good. - The Windows installer asks where to install LightCraft (Program Files by default; upgrades keep the folder you chose) and ends on a page saying it was installed, with a "Launch LightCraft" box. It used to finish without a word, so a successful install looked like nothing had happened (issues #18, #399). Silent installs (`/qn`, `/passive`)