fix(engine,mac): release the single-instance flock explicitly (#324)

crates/roost-engine/Cargo.toml

@@ -21,7 +21,6 @@ portable-pty = { workspace = true }
 libc = "0.2"
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
-fs2 = "0.4"
 
 tokio = { workspace = true }
 futures = { workspace = true }

crates/roost-engine/src/single_instance.rs

@@ -17,17 +17,24 @@
 //!    diagnostics + an "activate the running window" hint).
 //!
 //! The returned [`InstanceLock`] holds the open file descriptor.
-//! Dropping it releases the flock.
+//! Dropping it flock(LOCK_UN)s explicitly and then closes — closing
+//! alone is not enough, because the lock belongs to the open file
+//! description and any fork()ed child that inherited the fd would keep
+//! it alive until exec (issue #324).
+//!
+//! One residual window survives that and cannot be closed from here:
+//! if the process is SIGKILLed (so `Drop` never runs) while a just-
+//! forked child has not yet reached `exec`, that child's inherited
+//! description keeps the lock until it does. The window is the length
+//! of a fork→exec, it self-heals, and closing it would mean changing
+//! how every subprocess in the tree is spawned.
 //!
 //! M6 hardens this with the explicit stale-socket recovery loop.
 
-use std::fs::{File, OpenOptions};
+use std::fs::{File, OpenOptions, TryLockError};
 use std::io::{Read, Seek, SeekFrom, Write};
-use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 
-use fs2::FileExt;
-
 /// Live single-instance lock. Drop releases the flock.
 #[derive(Debug)]
 pub struct InstanceLock {
@@ -45,35 +52,19 @@ impl InstanceLock {
 
 impl Drop for InstanceLock {
     fn drop(&mut self) {
-        // Drop releases the flock via `_file`'s File drop — that's
-        // the only signal that matters for "this instance is gone."
-        //
-        // We deliberately do NOT unlink the lock file here, because
-        // the file handle is dropped AFTER this body returns (drop
-        // order: fields drop in declaration order *after* the drop
-        // impl runs, with `_file` listed first → released first,
-        // but `remove_file(&path)` still risks racing with another
-        // process that has already opened the file by name).
-        //
-        // Stale lock files left behind after a clean exit are
-        // harmless: the next `acquire()` overwrites the PID
-        // contents after successfully taking the flock. Callers
-        // that want explicit cleanup can call `release()`.
-    }
-}
+        // Closing the fd is NOT enough (issue #324). flock(2) locks
+        // live on the open file description, so a fork()ed child that
+        // inherited the fd keeps the lock alive until it execs — and
+        // in that window our drop silently fails to release. LOCK_UN
+        // clears the lock on the description itself, which every
+        // inheriting fd shares, so release is unconditional.
+        let _ = self._file.unlock();
 
-impl InstanceLock {
-    /// Explicit consuming cleanup: drop the file handle (releases
-    /// the flock), then unlink the lock file. Safe because the
-    /// flock is gone before we touch the path.
-    pub fn release(self) -> std::io::Result<()> {
-        let path = self.path.clone();
-        drop(self);
-        match std::fs::remove_file(&path) {
-            Ok(_) => Ok(()),
-            Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()),
-            Err(err) => Err(err),
-        }
+        // We deliberately do NOT unlink the lock file here: another
+        // process may already have opened it by name and be waiting on
+        // the flock. Stale lock files left behind after a clean exit
+        // are harmless — the next `acquire()` overwrites the PID
+        // contents after successfully taking the flock.
     }
 }
 
@@ -97,7 +88,7 @@ pub fn acquire(lock_path: impl AsRef<Path>) -> Result<InstanceLock, AcquireError
     if let Some(parent) = lock_path.parent() {
         std::fs::create_dir_all(parent)?;
     }
-    let mut file = OpenOptions::new()
+    let file = OpenOptions::new()
         .create(true)
         // Don't truncate at open — we may still need to read the
         // prior holder's PID below if the flock attempt fails.
@@ -108,31 +99,34 @@ pub fn acquire(lock_path: impl AsRef<Path>) -> Result<InstanceLock, AcquireError
         .write(true)
         .open(&lock_path)?;
 
-    // `fs2::FileExt::try_lock_exclusive` is `flock(LOCK_EX | LOCK_NB)`.
-    if let Err(err) = file.try_lock_exclusive() {
-        // Read whatever PID the previous holder wrote (best-effort).
-        let pid = read_pid(&file).unwrap_or(0);
-        // Suppress the unused fd warning on platforms where we
-        // don't reference `_raw_fd` directly.
-        let _raw_fd = file.as_raw_fd();
-        return Err(match err.kind() {
-            std::io::ErrorKind::WouldBlock => AcquireError::AlreadyHeld(pid),
-            _ => AcquireError::Io(err),
+    // `File::try_lock` is `flock(LOCK_EX | LOCK_NB)` on unix, and unlike
+    // the io::Error it replaces it distinguishes contention from a real
+    // failure in the type rather than by errno.
+    if let Err(err) = file.try_lock() {
+        return Err(match err {
+            // Read whatever PID the previous holder wrote (best-effort).
+            TryLockError::WouldBlock => AcquireError::AlreadyHeld(read_pid(&file).unwrap_or(0)),
+            TryLockError::Error(err) => AcquireError::Io(err),
         });
     }
 
+    // Wrap the locked file BEFORE anything fallible, so every `?`
+    // below releases through `Drop`'s LOCK_UN rather than dropping a
+    // bare `File` that a forked child could still be holding open.
+    let mut lock = InstanceLock {
+        _file: file,
+        path: lock_path,
+    };
+
     // We own the lock — write our PID into the file. Truncate
     // first to clear stale PID bytes from a prior holder.
-    file.set_len(0)?;
-    file.seek(SeekFrom::Start(0))?;
+    lock._file.set_len(0)?;
+    lock._file.seek(SeekFrom::Start(0))?;
     let pid = std::process::id();
-    writeln!(file, "{pid}")?;
-    file.flush()?;
+    writeln!(lock._file, "{pid}")?;
+    lock._file.flush()?;
 
-    Ok(InstanceLock {
-        _file: file,
-        path: lock_path,
-    })
+    Ok(lock)
 }
 
 fn read_pid(file: &File) -> std::io::Result<i32> {
@@ -148,8 +142,21 @@ fn read_pid(file: &File) -> std::io::Result<i32> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::io::BufRead;
+    use std::os::unix::io::AsRawFd;
     use tempfile::tempdir;
 
+    /// Kills and reaps on every exit path, so a panic mid-test can't
+    /// leave a child holding an inherited lock description.
+    struct ReapOnDrop(std::process::Child);
+
+    impl Drop for ReapOnDrop {
+        fn drop(&mut self) {
+            let _ = self.0.kill();
+            let _ = self.0.wait();
+        }
+    }
+
     #[test]
     fn first_acquire_succeeds() {
         let dir = tempdir().unwrap();
@@ -182,14 +189,97 @@ mod tests {
         assert!(second.lock_path().exists());
     }
 
+    // Regression guard for #324. flock(2) locks live on the *open file
+    // description*, not on the fd or the process, so a fork()ed child
+    // that inherited the fd keeps the lock alive until it execs. Before
+    // the explicit LOCK_UN in `Drop`, a sibling test in this binary
+    // spawning a subprocess during our lock window made `drop` a no-op
+    // and the next acquire() see WouldBlock -> AlreadyHeld(our own pid).
+    // Clearing FD_CLOEXEC makes that window deterministic instead of
+    // ~2%-of-runs.
     #[test]
-    fn release_unlinks_the_lock_file() {
+    fn drop_releases_even_when_a_forked_child_inherited_the_fd() {
         let dir = tempdir().unwrap();
         let path = dir.path().join("roost.lock");
         let lock = acquire(&path).unwrap();
-        assert!(path.exists());
-        lock.release().unwrap();
-        assert!(!path.exists(), "release() must unlink the lock file");
+
+        // SAFETY: plain fcntl on an fd we own; clearing FD_CLOEXEC is
+        // what makes the child inherit the lock's open file description.
+        let cleared = unsafe { libc::fcntl(lock._file.as_raw_fd(), libc::F_SETFD, 0) };
+        assert_eq!(cleared, 0, "failed to clear FD_CLOEXEC on the lock fd");
+
+        let _child = ReapOnDrop(
+            std::process::Command::new("/bin/sleep")
+                .arg("30")
+                .spawn()
+                .expect("spawn /bin/sleep"),
+        );
+
+        drop(lock);
+
+        match acquire(&path) {
+            Ok(second) => assert!(second.lock_path().exists()),
+            Err(err) => panic!("drop must release the flock even with an inherited fd: {err:?}"),
+        }
+    }
+
+    // The same-process test above exercises RAII drop; this one
+    // exercises process exit, which is the property the UI actually
+    // relies on after a crash. Neither subsumes the other.
+    #[test]
+    fn a_dead_process_releases_the_lock() {
+        let dir = tempdir().unwrap();
+        let path = dir.path().join("roost.lock");
+
+        let holder = std::process::Command::new(std::env::current_exe().unwrap())
+            .arg("--exact")
+            .arg("single_instance::tests::hold_the_lock_until_killed")
+            .arg("--ignored")
+            .arg("--nocapture")
+            .env("ROOST_TEST_LOCK_PATH", &path)
+            .stdout(std::process::Stdio::piped())
+            .spawn()
+            .expect("spawn the lock holder");
+        let mut holder = ReapOnDrop(holder);
+
+        // The child prints a line once it owns the flock; waiting on
+        // that instead of sleeping keeps the test deterministic. Its
+        // libtest banner comes out first, so scan rather than take the
+        // first line.
+        let stdout = std::io::BufReader::new(holder.0.stdout.take().unwrap());
+        let ready = stdout
+            .lines()
+            .map_while(Result::ok)
+            .any(|line| line.trim() == "locked");
+        assert!(ready, "the holder never reported that it took the lock");
+
+        match acquire(&path) {
+            Err(AcquireError::AlreadyHeld(pid)) => {
+                assert_eq!(pid as u32, holder.0.id(), "should report the holder's pid");
+            }
+            other => panic!("expected AlreadyHeld while the child lives, got {other:?}"),
+        }
+
+        holder.0.kill().unwrap();
+        holder.0.wait().unwrap();
+        acquire(&path).expect("the lock must be free once the holder is gone");
+    }
+
+    /// Helper process for [`a_dead_process_releases_the_lock`]: takes
+    /// the lock, announces it, then waits to be killed. `#[ignore]`
+    /// keeps it out of the normal run; without the env var (a bare
+    /// `--include-ignored` sweep) it is a no-op rather than a hang.
+    #[test]
+    #[ignore = "helper process for a_dead_process_releases_the_lock"]
+    fn hold_the_lock_until_killed() {
+        let Ok(path) = std::env::var("ROOST_TEST_LOCK_PATH") else {
+            return;
+        };
+        let _lock = acquire(&path).expect("holder must take the lock");
+        println!("locked");
+        std::io::Write::flush(&mut std::io::stdout()).unwrap();
+        // Bounded so a stranded helper cannot outlive the suite.
+        std::thread::sleep(std::time::Duration::from_secs(30));
     }
 
     #[test]

mac/Sources/Roost/SingleInstance.swift

@@ -61,7 +61,10 @@ final class SingleInstance: @unchecked Sendable {
         }
     }
 
-    private let lockFD: Int32
+    /// Module-internal rather than private so `SingleInstanceTests`
+    /// can hand the fd to a child process and prove the LOCK_UN in
+    /// `deinit` (issue #324).
+    let lockFD: Int32
     let lockPath: String
 
     private init(lockFD: Int32, lockPath: String) {
@@ -70,8 +73,15 @@ final class SingleInstance: @unchecked Sendable {
     }
 
     deinit {
-        // Closing the fd releases the flock automatically (BSD
-        // flock semantics). We do NOT unlink the lockfile —
+        // LOCK_UN first, then close. Closing alone is not enough:
+        // flock(2) locks belong to the open file description, so a
+        // fork()ed child that inherited the fd — every PTY spawn, in
+        // the window between fork and exec — keeps the lock alive past
+        // our close. LOCK_UN clears it on the description all those
+        // fds share. Same defect and same fix as the Rust side
+        // (issue #324, `crates/roost-engine/src/single_instance.rs`).
+        _ = roost_flock(lockFD, LOCK_UN)
+        // We do NOT unlink the lockfile —
         // unlinking on shutdown would race with a concurrent second
         // launch that already opened the same path; the GTK side
         // uses the same "leave it on disk" convention. The PID in
@@ -129,6 +139,10 @@ final class SingleInstance: @unchecked Sendable {
         }
         if written < 0 {
             let writeErrno = errno
+            // No `SingleInstance` exists yet, so `deinit`'s LOCK_UN can't
+            // run — release here or a forked child could hold the lock on
+            // past this close (#324).
+            _ = roost_flock(fd, LOCK_UN)
             Darwin.close(fd)
             throw SingleInstanceError.writeFailed(errno: writeErrno)
         }

mac/Tests/RoostTests/SingleInstanceTests.swift

@@ -92,6 +92,53 @@ struct SingleInstanceTests {
         }
     }
 
+    // Regression guard for #324, mirroring the Rust
+    // `drop_releases_even_when_a_forked_child_inherited_the_fd`.
+    // flock(2) locks belong to the open file description, not the fd
+    // or the process, so a fork()ed child that inherited the fd keeps
+    // the lock alive past our close(2). The app forks on every PTY
+    // spawn. Clearing FD_CLOEXEC makes the fork→exec window
+    // deterministic instead of intermittent.
+    //
+    // Foundation's `Process` is deliberately NOT used here: on Darwin it
+    // spawns with POSIX_SPAWN_CLOEXEC_DEFAULT, which closes every fd in
+    // the child regardless of FD_CLOEXEC, so the test would pass
+    // vacuously. Plain `posix_spawn` inherits normally.
+    @Test func releaseOnDeinitSurvivesAForkedChildHoldingTheFD() throws {
+        let path = uniqueLockPath()
+        defer { unlink(path) }
+
+        var child: pid_t = 0
+        defer {
+            if child > 0 {
+                kill(child, SIGKILL)
+                var status: Int32 = 0
+                waitpid(child, &status, 0)
+            }
+        }
+
+        do {
+            let first = try SingleInstance.acquire(lockPath: path)
+            guard case .acquired(let inst) = first else {
+                Issue.record("first acquire failed: \(first)")
+                return
+            }
+            #expect(fcntl(inst.lockFD, F_SETFD, 0) == 0)
+
+            var argv: [UnsafeMutablePointer<CChar>?] = [strdup("/bin/sleep"), strdup("30"), nil]
+            defer { argv.forEach { free($0) } }
+            #expect(posix_spawn(&child, "/bin/sleep", nil, nil, &argv, environ) == 0)
+        }
+
+        switch try SingleInstance.acquire(lockPath: path) {
+        case .acquired: break
+        case .alreadyHeld(let pid):
+            Issue.record("an inherited fd blocked re-acquire: alreadyHeld(\(pid))")
+        case .bypassed:
+            Issue.record("expected re-acquire, got bypassed")
+        }
+    }
+
     private func uniqueLockPath() -> String {
         let id = UUID().uuidString
         return "/tmp/roost-tests-\(id).lock"

tools/README.md

@@ -14,6 +14,7 @@ tools/
     linux/          uinput key/pointer + clipboard + single-monitor (COSMIC/Wayland).
     (mac/)          CGEvent equivalent — planned.
   perf/           Render-path cost — a sibling axis, not a layer (see intro above).
+  repro/          (non-tier — on-demand drivers for intermittent bugs; see repro/README.md)
   roosttest_unit/ (non-tier — fast unit tests for the harness wiring itself)
   shed/           (non-tier — Apple VZ Linux microVM driver for Linux testing from a Mac)
   wayland/        (non-tier — Wayland-specific test support)