From 8166185a0f1715eb3cfa77b8f5cb06ab1e5faf26 Mon Sep 17 00:00:00 2001 From: Jason Baldwin <33934381+bitshiftza@users.noreply.github.com> Date: Sat, 10 Oct 2026 12:34:07 +0200 Subject: [PATCH] LR-LIB-LOCK: release the library lock explicitly on drop (#680) LibraryLock freed the library by letting its File close. On Unix the flock lock belongs to the open file description and lasts while any descriptor on it is open. When another thread spawns a process, posix_spawn copies the whole descriptor table into the child and O_CLOEXEC only closes the copies at execve, so a LibraryLock dropped in that window stayed locked until the child exec'd. Re-acquiring the library then failed with LockError::InUse(None) ("already open in another LightCraft program") although nobody had it open. That is the flake in tests_lock::second_opener_is_refused_until_the_first_lets_go, whose neighbour lock_held_by_another_process_is_released_when_it_dies spawns the test binary on a parallel libtest thread. Drop now removes the owner note while still holding the lock (so it can't delete the next holder's), then calls File::unlock(): LOCK_UN through our descriptor frees the lock whatever copies a child still holds. An unlock error is logged, never a panic; the handle still closes right after, which releases the lock once every copy is gone, as before. Tests: the_library_is_free_once_dropped_even_if_a_child_holds_a_copy holds a try_clone of the lock's descriptor across the drop (the same open file description a child gets between fork and exec) and re-acquires; dropping_the_lock_frees_the_library_while_another_thread_ spawns_processes runs a thread spawning /bin/true against two seconds of acquire/drop cycles. Both fail every run without the fix (the first deterministically) and pass with it; the flaky pair went from 3/100 to 0/100 failed runs here. Both are cfg(unix): Windows handles aren't inherited by CreateProcess. --- crates/catalog/src/lock.rs | 35 +++++++++++++---- crates/catalog/src/tests_lock.rs | 66 ++++++++++++++++++++++++++++++++ docs/whats-new.md | 5 +++ 3 files changed, 99 insertions(+), 7 deletions(-) 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`)