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

@@ -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>,
+    file: Option<File>,
     held: bool,
 }
 
@@ -96,7 +102,7 @@ impl LibraryLock {
     pub fn acquire(dir: &Path, program: &str) -> Result<LibraryLock, LockError> {
         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<File> {
+        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());
+            }
         }
     }
 }

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)]
+    );
+}

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`)