agent: a lock file is recognized by being locked, not only by its name

crates/agent/ARCHITECTURE.md

@@ -3120,14 +3120,30 @@ POSIX lock, which is why it is not the authoritative half.)
 
 Nothing else needs guarding: the only remaining hazard is _replacing_ a lock file, which an atomic
 write (`tools::write_atomic`) could do by renaming over it — it refuses a lock file as its target
-(`file_lock::is_lock_file`: the record lock file, or a legacy `lock` / `<f>.lock` sitting beside one,
-so an unrelated `Cargo.lock` is written as ever; applied after resolving a symlink; a hard link's
-rename replaces only the link). A worktree seed leaves lock files behind (a copy of a lock is no
-lock). A failed session start's directory is taken back through `file_lock::dir_holds_only_lock_files`
-and `FileLock::release_and_remove_files`, which unlocks and **closes** the lock files before unlinking
-them: an NFS client silly-renames a file it still holds open to `.nfs*`, which kept the directory from
-being removed (`serve_ws::tests::an_empty_session_directory_is_taken_back_with_its_lock` fails on a
-loopback NFS mount with the old unlink-then-close order, and passes with this one).
+(`file_lock::is_lock_file`, applied after resolving a symlink). **It never opens the target and never
+locks it — it only `stat`s.** Opening a FIFO with no writer blocks forever, opening a device can act,
+and a test lock makes another program's own non-blocking lock fail while it is held (2071 of 20000
+attempts, measured). So a target is a lock file only when one of three things says so:
+
+- **Its name**, compared without regard to ASCII case: a record lock file, or a legacy `lock` /
+  `<f>.lock` beside one.
+- **It is a lock this process holds.** `try_lock` puts the (device, inode) of each lock file it holds
+  in a small counted set and `Drop` takes them out once released; `write_atomic` looks the target's
+  `stat` up there. That covers a journal key held through `Target::Itself` under any name, a hard link
+  to one of our own lock files, and a spelling a case-insensitive filesystem folds onto one.
+- **An old binary's legacy lock with no record lock file beside it**, by where it sits: a `lock` in a
+  session directory (one holding `000001.jsonl`, which is never deleted), or a `<f>.lock` beside a
+  session file (`*.jsonl`) or the MCP manifest. Whether anyone holds it is not asked.
+
+Anything else is an ordinary file and an edit of it goes through: a SQLite database another program
+holds mid-transaction, a daemon's pid file, an unrelated `Cargo.lock`, a `lock` in a directory that
+holds no session. (Verified on a casefold ext4 directory as well as here.) A worktree seed leaves
+record lock files behind (a copy of a lock is no lock). A failed session start's directory is taken back through
+`file_lock::dir_holds_only_lock_files` and `FileLock::release_and_remove_files`, which **closes** the
+lock files' descriptors before the directory is removed: an NFS client turns the unlink of a file it
+still has open into a rename to `.nfs*`, which stays until the last close and makes `remove_dir` fail
+with ENOTEMPTY (`serve_ws::tests::an_empty_session_directory_is_taken_back_with_its_lock` fails on a
+loopback NFS mount with the old unlink, remove, close order, and passes with this one).
 
 A small file whose holder writes it under its own lock — the MCP task journal key — is locked
 **itself** (`Target::Itself`): no lock file beside it, and the key is re-read and written through the

crates/agent/src/file_lock.rs

@@ -94,34 +94,114 @@ fn with_suffix(file: &Path, suffix: &str) -> PathBuf {
     PathBuf::from(name)
 }
 
-/// Whether `path` names a lock file — for copying code that should leave locks behind (a worktree
-/// seed: a copy of a lock file is not a lock), and for an atomic write, which must never rename a
-/// fresh file over one (the lock would stay on the old, now unnamed, inode).
+/// Whether `path` is named like a record lock file — compared without regard to ASCII case, since on
+/// a case-insensitive filesystem (macOS) `.BEYOND-LOCK` is the same file. For copying code that
+/// should leave locks behind (a worktree seed: a copy of a lock file is not a lock).
 pub fn is_record_lock_file(path: &Path) -> bool {
-    path.file_name()
-        .and_then(OsStr::to_str)
-        .is_some_and(|n| n.ends_with(RECORD_SUFFIX))
+    path.file_name().and_then(OsStr::to_str).is_some_and(|n| {
+        n.len() >= RECORD_SUFFIX.len()
+            && n.as_bytes()[n.len() - RECORD_SUFFIX.len()..]
+                .eq_ignore_ascii_case(RECORD_SUFFIX.as_bytes())
+    })
 }
 
-/// Whether `path` is one of this module's lock files: a record lock file, or a legacy one — `lock` /
-/// `<f>.lock` — that sits beside a record lock file (so an unrelated `Cargo.lock` is not one). An
-/// atomic write must never rename a fresh file over either: the lock would stay on the old inode,
-/// which the path no longer names.
+/// Whether an atomic write must not rename a fresh file over `path`. Replacing a lock file leaves the
+/// lock on the old inode, which the path no longer names, so the next owner would lock the new file.
+///
+/// **This never opens `path`, and never locks it** — it only `stat`s. Opening a FIFO or a device
+/// blocks or acts, and a test lock makes another program's own non-blocking lock fail while it is
+/// held. So a file is a lock file only when one of these says so:
+///
+/// - **Its name**, without regard to ASCII case (a case-insensitive filesystem folds `.BEYOND-LOCK`
+///   onto `.beyond-lock`): a record lock file, or a legacy `lock` / `<f>.lock` beside one.
+/// - **It is a lock this process holds**: its (device, inode) is in the held set [`try_lock`] keeps.
+///   That covers a [`Target::Itself`] key (any name) and any spelling or hard link of this process's
+///   own lock files.
+/// - **An old binary's legacy lock with no record lock file beside it**, by where it sits: a `lock`
+///   in a session directory (one holding `000001.jsonl`, which is never deleted), or a `<f>.lock`
+///   beside a session file (`*.jsonl`) or the MCP manifest. Whether anyone holds it is not asked.
+///
+/// Anything else — a SQLite database another program holds open and locked, a daemon's pid file — is
+/// an ordinary file, and an edit of it goes through.
 pub fn is_lock_file(path: &Path) -> bool {
+    is_lock_file_by_name(path) || is_held_here(path)
+}
+
+fn is_lock_file_by_name(path: &Path) -> bool {
     if is_record_lock_file(path) {
         return true;
     }
     let Some(name) = path.file_name().and_then(OsStr::to_str) else {
         return false;
     };
-    let record = if name == "lock" {
-        path.with_file_name(RECORD_SUFFIX)
-    } else if let Some(stem) = name.strip_suffix(".lock") {
-        path.with_file_name(format!("{stem}{RECORD_SUFFIX}"))
-    } else {
+    let lower = name.to_ascii_lowercase();
+    if lower == "lock" {
+        return path.with_file_name(RECORD_SUFFIX).exists()
+            || path.with_file_name("000001.jsonl").is_file();
+    }
+    let Some(stem) = lower.strip_suffix(".lock") else {
         return false;
     };
-    record.exists()
+    stem.ends_with(".jsonl")
+        || stem == "mcp-manifest.json"
+        || path
+            .with_file_name(format!("{}{RECORD_SUFFIX}", &name[..stem.len()]))
+            .exists()
+}
+
+type FileId = (u64, u64);
+
+/// The (device, inode) of every lock file this process holds, counted: [`try_lock`] adds a lock's
+/// files once it holds them, and dropping the lock takes them out again once it is released.
+#[cfg(unix)]
+fn held() -> std::sync::MutexGuard<'static, std::collections::HashMap<FileId, usize>> {
+    static HELD: std::sync::OnceLock<std::sync::Mutex<std::collections::HashMap<FileId, usize>>> =
+        std::sync::OnceLock::new();
+    HELD.get_or_init(Default::default)
+        .lock()
+        .unwrap_or_else(|e| e.into_inner())
+}
+
+#[cfg(unix)]
+fn file_id(meta: &std::fs::Metadata) -> FileId {
+    use std::os::unix::fs::MetadataExt;
+    (meta.dev(), meta.ino())
+}
+
+#[cfg(unix)]
+fn mark_held(files: &[&File], add: bool) {
+    let ids: Vec<FileId> = files
+        .iter()
+        .filter_map(|f| f.metadata().ok())
+        .map(|m| file_id(&m))
+        .collect();
+    let mut held = held();
+    for id in ids {
+        if add {
+            *held.entry(id).or_default() += 1;
+        } else if let std::collections::hash_map::Entry::Occupied(mut e) = held.entry(id) {
+            *e.get_mut() -= 1;
+            if *e.get() == 0 {
+                e.remove();
+            }
+        }
+    }
+}
+
+#[cfg(not(unix))]
+fn mark_held(_: &[&File], _: bool) {}
+
+/// Whether the file at `path` is one this process holds a lock on — by `stat`, nothing opened.
+fn is_held_here(path: &Path) -> bool {
+    #[cfg(unix)]
+    {
+        std::fs::metadata(path).is_ok_and(|m| held().contains_key(&file_id(&m)))
+    }
+    #[cfg(not(unix))]
+    {
+        let _ = path;
+        false
+    }
 }
 
 /// Whether `dir` holds nothing but its own lock files — a lock directory that never became anything
@@ -154,9 +234,14 @@ impl FileLock {
         &self.record
     }
 
-    /// Release the lock and unlink its files — for a lock directory being taken back. Unlocked and
-    /// closed *first*: an NFS client cannot unlink a file it still holds open (it silly-renames it to
-    /// `.nfs*`, which then keeps the directory from being removed).
+    fn files(&self) -> Vec<&File> {
+        std::iter::once(&self.record).chain(&self.legacy).collect()
+    }
+
+    /// Release the lock and unlink its files — for a lock directory being taken back. What matters is
+    /// that the descriptors are closed before the directory is removed: an NFS client turns the unlink
+    /// of a file it still has open into a rename to `.nfs*`, which stays until the last close and keeps
+    /// `remove_dir` failing (ENOTEMPTY). Closing first means the unlinks are real ones.
     pub fn release_and_remove_files(self) {
         let (record_path, legacy_path) = (self.record_path.clone(), self.legacy_path.clone());
         drop(self);
@@ -173,6 +258,8 @@ impl Drop for FileLock {
         if let Some(legacy) = &self.legacy {
             let _ = legacy.unlock();
         }
+        // Out of the held set only once unlocked, so the set never misses a lock still held.
+        mark_held(&self.files(), false);
     }
 }
 
@@ -211,12 +298,14 @@ pub fn try_lock(target: Target<'_>) -> std::io::Result<Option<FileLock>> {
                 continue;
             }
         };
-        return Ok(Some(FileLock {
+        let lock = FileLock {
             record,
             legacy,
             record_path,
             legacy_path,
-        }));
+        };
+        mark_held(&lock.files(), true);
+        return Ok(Some(lock));
     }
     Ok(None)
 }
@@ -535,21 +624,112 @@ pub(crate) mod tests {
         assert!(try_lock(Target::Itself(&key)).unwrap().is_some());
     }
 
-    /// A legacy lock file is recognized only beside a record lock file, so an unrelated `Cargo.lock`
-    /// is just a file.
+    /// A lock file is recognized by its name in any case — a record lock file, a legacy one beside a
+    /// record lock file, or an old binary's legacy one where a session or the MCP manifest keeps it —
+    /// and nothing else is: an unrelated `Cargo.lock`, or a `lock` in a directory that holds no
+    /// session, is just a file.
     #[test]
-    fn a_legacy_lock_file_is_one_only_beside_a_record_lock_file() {
+    fn a_lock_file_is_one_by_its_name_in_any_case_or_by_where_it_sits() {
         let dir = tempfile::tempdir().unwrap();
         let session = dir.path().join("s1");
         std::fs::create_dir_all(&session).unwrap();
-        let file = dir.path().join("mcp-manifest.json");
-        let _a = try_lock(Target::Dir(&session)).unwrap().unwrap();
-        let _b = try_lock(Target::File(&file)).unwrap().unwrap();
-        assert!(is_lock_file(&session.join("lock")));
-        assert!(is_lock_file(&dir.path().join("mcp-manifest.json.lock")));
+        drop(try_lock(Target::Dir(&session)).unwrap().unwrap());
+        assert!(
+            is_lock_file(&session.join("lock")),
+            "beside a record lock file"
+        );
+        assert!(is_lock_file(&session.join(".BEYOND-LOCK")), "any case");
+        // An old binary's session directory: a segment and its legacy `lock`, no record lock file.
+        let old = dir.path().join("old");
+        std::fs::create_dir_all(&old).unwrap();
+        std::fs::write(old.join("000001.jsonl"), "").unwrap();
+        std::fs::write(old.join("lock"), "").unwrap();
+        assert!(
+            is_lock_file(&old.join("lock")),
+            "a lock in a session directory"
+        );
+        assert!(is_lock_file(&old.join("Lock")), "in any case");
+        for beside in ["1700000000_abc.jsonl.lock", "mcp-manifest.json.lock"] {
+            assert!(is_lock_file(&dir.path().join(beside)), "{beside}");
+        }
         std::fs::write(dir.path().join("Cargo.lock"), "").unwrap();
         assert!(!is_lock_file(&dir.path().join("Cargo.lock")));
-        assert!(!is_lock_file(&dir.path().join("lock")));
+        let plain = dir.path().join("plain");
+        std::fs::create_dir_all(&plain).unwrap();
+        std::fs::write(plain.join("lock"), "").unwrap();
+        assert!(!is_lock_file(&plain.join("lock")), "no session here");
+    }
+
+    /// A lock this process holds is recognized under any name — a hard link, a key locked itself —
+    /// by its inode, and stops being one once released.
+    #[cfg(unix)]
+    #[test]
+    fn a_held_lock_file_is_recognized_under_any_name() {
+        let dir = tempfile::tempdir().unwrap();
+        let file = dir.path().join("x");
+        let target = Target::File(&file);
+        let held = try_lock(target).unwrap().unwrap();
+        let alias = dir.path().join("INNOCENT.TXT");
+        std::fs::hard_link(target.record_path(), &alias).unwrap();
+        assert!(is_lock_file(&alias));
+        let key = dir.path().join("journal.key");
+        let held_key = try_lock(Target::Itself(&key)).unwrap().unwrap();
+        assert!(is_lock_file(&key), "a file locked itself is one while held");
+        drop(held);
+        drop(held_key);
+        assert!(!is_lock_file(&alias), "released");
+        assert!(!is_lock_file(&key), "released");
+    }
+
+    /// Asking whether a file is a lock file takes no lock on it, so another program's own
+    /// non-blocking `flock` on that file never fails because of us. (A test lock taken to ask made
+    /// 2071 of 20000 of its attempts fail.)
+    #[cfg(unix)]
+    #[test]
+    fn asking_never_makes_another_programs_lock_fail() {
+        let dir = tempfile::tempdir().unwrap();
+        let theirs = dir.path().join("daemon.pid");
+        std::fs::write(&theirs, "").unwrap();
+        let Ok(mut other) = Command::new("python3")
+            .args([
+                "-c",
+                "import fcntl,sys\nf=open(sys.argv[1],'a+')\nprint('ready',flush=True)\nfailed=0\nfor _ in range(20000):\n    try:\n        fcntl.flock(f,fcntl.LOCK_EX|fcntl.LOCK_NB)\n        fcntl.flock(f,fcntl.LOCK_UN)\n    except BlockingIOError:\n        failed+=1\nprint(failed,flush=True)",
+            ])
+            .arg(&theirs)
+            .stdout(Stdio::piped())
+            .spawn()
+        else {
+            eprintln!("no python3: skipping the other-program check");
+            return;
+        };
+        let mut out = BufReader::new(other.stdout.take().unwrap());
+        let mut line = String::new();
+        out.read_line(&mut line).unwrap();
+        assert_eq!(line.trim(), "ready");
+        let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false));
+        let asker = {
+            let (stop, theirs) = (stop.clone(), theirs.clone());
+            std::thread::spawn(move || {
+                let (mut asked, mut called_lock) = (0u64, 0u64);
+                while !stop.load(std::sync::atomic::Ordering::Relaxed) {
+                    called_lock += u64::from(is_lock_file(&theirs));
+                    asked += 1;
+                }
+                (asked, called_lock)
+            })
+        };
+        line.clear();
+        out.read_line(&mut line).unwrap();
+        stop.store(true, std::sync::atomic::Ordering::Relaxed);
+        let (asked, called_lock) = asker.join().unwrap();
+        other.wait().unwrap();
+        assert!(asked > 0);
+        assert_eq!(
+            line.trim(),
+            "0",
+            "its own lock attempts failed while we asked {asked} times"
+        );
+        assert_eq!(called_lock, 0, "its file is not a lock file of ours");
     }
 
     /// An old binary held only the legacy `flock`. While one runs, it must still exclude this binary.