From 10abddd9b686f85e8b43bad1d3fa89238ba06cf2 Mon Sep 17 00:00:00 2001 From: Jared Lunde Date: Wed, 7 Oct 2026 04:51:07 -0700 Subject: [PATCH 1/2] agent: a lock file is recognized by being locked, not only by its name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rename-target check in write_atomic was name-based, which missed: a spelling a case-insensitive filesystem folds onto a lock file (`.BEYOND-LOCK`, `LOCK`); an old binary's legacy `lock` held with no record lock file beside it; and a journal key held through Target::Itself. file_lock::is_lock_file is now: named like a record lock file (ASCII case-insensitive), or locked right now — asked of the file itself (file_lock::is_locked: F_OFD_GETLK, which also reports this process's other descriptions, plus a non-blocking test flock). A lock nobody holds needs no protecting; Cargo.lock is written as ever. Verified on a casefold ext4 directory too. The release doc now says what matters: the descriptors are closed before remove_dir (an unlink of an open file on NFS becomes a .nfs* rename until the last close). Tests (each fails with its fix reverted): held legacy lock with no record beside it, a key held Itself, and a case-folded spelling are refused by write_atomic; a hard link under any name is recognized; the folded-spelling test exercises a real case-insensitive dir when the temp dir is one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk --- crates/agent/ARCHITECTURE.md | 22 ++++--- crates/agent/src/file_lock.rs | 112 +++++++++++++++++++++++----------- crates/agent/src/tools/mod.rs | 86 ++++++++++++++++++++++++-- crates/agent/src/worktree.rs | 2 +- 4 files changed, 175 insertions(+), 47 deletions(-) diff --git a/crates/agent/ARCHITECTURE.md b/crates/agent/ARCHITECTURE.md index 8262c414..6485df8a 100644 --- a/crates/agent/ARCHITECTURE.md +++ b/crates/agent/ARCHITECTURE.md @@ -3120,14 +3120,20 @@ 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` / `.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): anything named like a record lock +file, compared without regard to ASCII case, or **any file locked right now** — asked of the file +itself (`file_lock::is_locked`: `F_OFD_GETLK`, which also reports this process's own other +descriptions, and a non-blocking test `flock`). Asking the file covers what no name can: an old +binary's legacy `lock` with no record lock file beside it, a journal key held through +`Target::Itself`, a spelling a case-insensitive filesystem folds onto a lock file, a hard link. A lock +file nobody holds needs no protecting, and an unrelated `Cargo.lock` is written as ever. (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 diff --git a/crates/agent/src/file_lock.rs b/crates/agent/src/file_lock.rs index 1cdb478a..dd17b161 100644 --- a/crates/agent/src/file_lock.rs +++ b/crates/agent/src/file_lock.rs @@ -94,34 +94,55 @@ 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` / -/// `.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`: it is named like a record lock +/// file, or **the file there is locked right now** — by anyone, any kind of lock this module takes. +/// Replacing a locked file leaves the lock on the old inode, which the path no longer names, so the +/// next owner would lock the new file. +/// +/// Asking the file itself, not its name, is what covers every case a name cannot: a legacy `lock` +/// an old binary holds with no record lock file beside it, a journal key held through +/// [`Target::Itself`], a spelling a case-insensitive filesystem folds onto a lock file, a hard link. +/// A lock nobody holds is not protected, and needs no protecting. pub fn is_lock_file(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 { + is_record_lock_file(path) || is_locked(path) +} + +/// Whether anyone holds a lock on the file at `path` right now: an OFD or POSIX lock (`F_OFD_GETLK`, +/// which reports another description's lock — this process's own included), or an `flock` (a +/// non-blocking test lock, let go at once). The probe opens its own descriptor, which with OFD locks +/// releases nothing. +pub fn is_locked(path: &Path) -> bool { + let Ok(file) = OpenOptions::new().read(true).open(path) else { return false; }; - record.exists() + #[cfg(any(target_os = "linux", target_os = "android"))] + { + let mut probe = whole_file(libc::F_WRLCK); + if nix::fcntl::fcntl(&file, nix::fcntl::FcntlArg::F_OFD_GETLK(&mut probe)).is_ok() + && probe.l_type != libc::F_UNLCK as libc::c_short + { + return true; + } + } + match file.try_lock() { + Ok(()) => { + let _ = file.unlock(); + false + } + Err(std::fs::TryLockError::WouldBlock) => true, + Err(std::fs::TryLockError::Error(_)) => false, + } } /// Whether `dir` holds nothing but its own lock files — a lock directory that never became anything @@ -154,9 +175,10 @@ 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). + /// 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); @@ -535,21 +557,43 @@ 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 record name in any case, or by being locked right now: an + /// unrelated `Cargo.lock` nobody holds 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_name_in_any_case_or_by_being_held() { 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"))); + let held = try_lock(Target::Dir(&session)).unwrap().unwrap(); + assert!( + is_lock_file(&session.join("lock")), + "the legacy flock is held" + ); + assert!(is_lock_file(&session.join(".BEYOND-LOCK")), "any case"); 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"))); + drop(held); + assert!( + !is_lock_file(&session.join("lock")), + "a legacy lock file nobody holds needs no protecting" + ); + } + + /// The probe asks the file, so a hard link under any name to a held lock file is one too — the + /// same answer a case-insensitive filesystem's folded spelling gets. + #[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 _key = try_lock(Target::Itself(&key)).unwrap().unwrap(); + assert!(is_lock_file(&key), "a file locked itself is one while held"); } /// An old binary held only the legacy `flock`. While one runs, it must still exclude this binary. diff --git a/crates/agent/src/tools/mod.rs b/crates/agent/src/tools/mod.rs index bfe1d9b9..e6e19dee 100644 --- a/crates/agent/src/tools/mod.rs +++ b/crates/agent/src/tools/mod.rs @@ -177,13 +177,17 @@ pub(crate) fn write_atomic(path: &str, content: &[u8]) -> std::io::Result<()> { let p = std::path::Path::new(path); let resolved = resolve_symlink_target(p); let p = resolved.as_deref().unwrap_or(p); - // Never a fresh file renamed over a lock file (reached by name or through a symlink — resolved - // just above): the lock would stay on the old inode, which the path no longer names, so the next - // owner would lock the new file. Opening one is harmless (see `file_lock`); replacing it is not. + // Never a fresh file renamed over a lock file — one named so, or any file locked right now (asked + // of the file, so a folded spelling, a legacy lock or a key held `Itself` are all seen): the lock + // would stay on the old inode, which the path no longer names. Opening one is harmless (see + // `file_lock`); replacing it is not. if crate::file_lock::is_lock_file(p) { return Err(std::io::Error::new( std::io::ErrorKind::PermissionDenied, - format!("{} is a lock file; it is not written", p.display()), + format!( + "{} is a lock file, or locked by a running process; it is not replaced", + p.display() + ), )); } let name = match p.file_name() { @@ -926,6 +930,80 @@ mod tests { assert_eq!(std::fs::read(&cargo_lock).unwrap(), b"[[package]]\n"); } + /// The cases a name cannot see: an old binary's session directory, whose legacy `lock` it holds + /// with no record lock file beside it; a journal key held through `Target::Itself`; and a + /// record lock file spelled in another case (one file on a case-insensitive filesystem). Each is + /// refused while held — asked of the file, not its name. + #[test] + #[cfg(unix)] + fn write_atomic_never_replaces_a_file_locked_right_now() { + use std::io::{BufRead as _, BufReader}; + let dir = tempfile::tempdir().unwrap(); + // An old binary: an `flock` on `/lock`, nothing else. + let old_session = dir.path().join("old"); + std::fs::create_dir_all(&old_session).unwrap(); + let legacy = old_session.join("lock"); + if let Ok(mut old) = std::process::Command::new("python3") + .args([ + "-c", + "import fcntl,sys\nf=open(sys.argv[1],'a+')\nfcntl.flock(f,fcntl.LOCK_EX)\nprint('locked',flush=True)\nsys.stdin.read()", + ]) + .arg(&legacy) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .spawn() + { + let mut line = String::new(); + BufReader::new(old.stdout.take().unwrap()) + .read_line(&mut line) + .unwrap(); + assert!( + write_atomic(legacy.to_str().unwrap(), b"x").is_err(), + "an old binary's held lock file is not replaced" + ); + drop(old.stdin.take()); + old.wait().unwrap(); + write_atomic(legacy.to_str().unwrap(), b"x").unwrap(); + } + // A journal key locked itself. + let key = dir.path().join("journal.key"); + let held = crate::file_lock::try_lock(crate::file_lock::Target::Itself(&key)) + .unwrap() + .unwrap(); + assert!( + write_atomic(key.to_str().unwrap(), b"x").is_err(), + "a key held through Itself is not replaced" + ); + drop(held); + // A record lock file in another case. + let shouty = dir.path().join("S2.BEYOND-LOCK"); + assert!(write_atomic(shouty.to_str().unwrap(), b"x").is_err()); + } + + /// On a case-insensitive filesystem (macOS; a casefold ext4 directory) `LOCK` and `.BEYOND-LOCK` + /// are the held lock files themselves. Skipped where the temp dir is case-sensitive. + #[test] + #[cfg(unix)] + fn write_atomic_never_replaces_a_lock_file_through_a_folded_spelling() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("Probe"), "").unwrap(); + if !dir.path().join("PROBE").exists() { + return; + } + let session = dir.path().join("s1"); + std::fs::create_dir_all(&session).unwrap(); + let _held = crate::file_lock::try_lock(crate::file_lock::Target::Dir(&session)) + .unwrap() + .unwrap(); + for folded in ["LOCK", ".BEYOND-LOCK"] { + let p = session.join(folded); + assert!( + write_atomic(p.to_str().unwrap(), b"x").is_err(), + "{folded} reaches a held lock file" + ); + } + } + #[test] #[cfg(unix)] fn write_atomic_does_not_follow_a_symlink_planted_at_the_old_deterministic_temp_name() { diff --git a/crates/agent/src/worktree.rs b/crates/agent/src/worktree.rs index 8c459126..eeaccbe9 100644 --- a/crates/agent/src/worktree.rs +++ b/crates/agent/src/worktree.rs @@ -427,7 +427,7 @@ impl Git { let from = src.join(rel); // A lock file is left behind: a copy of one is not a lock (see // `file_lock`). - if !from.is_file() || crate::file_lock::is_lock_file(&from) { + if !from.is_file() || crate::file_lock::is_record_lock_file(&from) { continue; } let to = dst.join(rel); From 471de969ea1fcdd61d74007b0fce6e1cfde50876 Mon Sep 17 00:00:00 2001 From: Jared Lunde Date: Wed, 7 Oct 2026 05:34:42 -0700 Subject: [PATCH 2/2] =?UTF-8?q?agent:=20decide=20a=20lock=20file=20by=20st?= =?UTF-8?q?at=20alone=20=E2=80=94=20never=20open=20or=20test-lock=20the=20?= =?UTF-8?q?target?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_atomic's lock-file check opened its target read-only and took a test flock on it. That hung on a FIFO (and could act on a device or block on a hung mount), made other programs' own non-blocking flocks fail (2071/20000), and refused legitimate edits of files others hold locked (a SQLite WAL database). is_lock_file now only stats. A target is a lock file when: - its name says so, ASCII case-insensitively (record lock file, or a legacy lock beside one); - its (dev, inode) is one this process holds: try_lock records each held lock file in a counted set, Drop removes them after unlocking (covers Itself keys, hard links, folded spellings of our own locks); - it is an old binary's legacy lock by where it sits: `lock` in a session dir (000001.jsonl present) or `.lock` beside a *.jsonl or the MCP manifest. is_locked's probe is deleted. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JimHGjsfk2Ktm5GxyZJKKk --- crates/agent/ARCHITECTURE.md | 28 +++-- crates/agent/src/file_lock.rs | 226 +++++++++++++++++++++++++++------- crates/agent/src/tools/mod.rs | 141 ++++++++++++++------- 3 files changed, 297 insertions(+), 98 deletions(-) diff --git a/crates/agent/ARCHITECTURE.md b/crates/agent/ARCHITECTURE.md index 6485df8a..92de6282 100644 --- a/crates/agent/ARCHITECTURE.md +++ b/crates/agent/ARCHITECTURE.md @@ -3120,15 +3120,25 @@ 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`, applied after resolving a symlink): anything named like a record lock -file, compared without regard to ASCII case, or **any file locked right now** — asked of the file -itself (`file_lock::is_locked`: `F_OFD_GETLK`, which also reports this process's own other -descriptions, and a non-blocking test `flock`). Asking the file covers what no name can: an old -binary's legacy `lock` with no record lock file beside it, a journal key held through -`Target::Itself`, a spelling a case-insensitive filesystem folds onto a lock file, a hard link. A lock -file nobody holds needs no protecting, and an unrelated `Cargo.lock` is written as ever. (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::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` / + `.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 `.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 diff --git a/crates/agent/src/file_lock.rs b/crates/agent/src/file_lock.rs index dd17b161..e1064126 100644 --- a/crates/agent/src/file_lock.rs +++ b/crates/agent/src/file_lock.rs @@ -105,43 +105,102 @@ pub fn is_record_lock_file(path: &Path) -> bool { }) } -/// Whether an atomic write must not rename a fresh file over `path`: it is named like a record lock -/// file, or **the file there is locked right now** — by anyone, any kind of lock this module takes. -/// Replacing a locked file leaves the lock on the old inode, which the path no longer names, so the -/// next owner would lock the new file. +/// 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. /// -/// Asking the file itself, not its name, is what covers every case a name cannot: a legacy `lock` -/// an old binary holds with no record lock file beside it, a journal key held through -/// [`Target::Itself`], a spelling a case-insensitive filesystem folds onto a lock file, a hard link. -/// A lock nobody holds is not protected, and needs no protecting. +/// **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` / `.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 `.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_record_lock_file(path) || is_locked(path) + is_lock_file_by_name(path) || is_held_here(path) } -/// Whether anyone holds a lock on the file at `path` right now: an OFD or POSIX lock (`F_OFD_GETLK`, -/// which reports another description's lock — this process's own included), or an `flock` (a -/// non-blocking test lock, let go at once). The probe opens its own descriptor, which with OFD locks -/// releases nothing. -pub fn is_locked(path: &Path) -> bool { - let Ok(file) = OpenOptions::new().read(true).open(path) else { +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; }; - #[cfg(any(target_os = "linux", target_os = "android"))] - { - let mut probe = whole_file(libc::F_WRLCK); - if nix::fcntl::fcntl(&file, nix::fcntl::FcntlArg::F_OFD_GETLK(&mut probe)).is_ok() - && probe.l_type != libc::F_UNLCK as libc::c_short - { - return true; - } + 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(); } - match file.try_lock() { - Ok(()) => { - let _ = file.unlock(); - false + let Some(stem) = lower.strip_suffix(".lock") else { + return false; + }; + 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> { + static HELD: std::sync::OnceLock>> = + 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 = 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(); + } } - Err(std::fs::TryLockError::WouldBlock) => true, - Err(std::fs::TryLockError::Error(_)) => false, + } +} + +#[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 } } @@ -175,6 +234,10 @@ impl FileLock { &self.record } + 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 @@ -195,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); } } @@ -233,12 +298,14 @@ pub fn try_lock(target: Target<'_>) -> std::io::Result> { 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) } @@ -557,43 +624,112 @@ pub(crate) mod tests { assert!(try_lock(Target::Itself(&key)).unwrap().is_some()); } - /// A lock file is recognized by its record name in any case, or by being locked right now: an - /// unrelated `Cargo.lock` nobody holds 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_lock_file_is_one_by_name_in_any_case_or_by_being_held() { + 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 held = try_lock(Target::Dir(&session)).unwrap().unwrap(); + drop(try_lock(Target::Dir(&session)).unwrap().unwrap()); assert!( is_lock_file(&session.join("lock")), - "the legacy flock is held" + "beside a record lock file" ); assert!(is_lock_file(&session.join(".BEYOND-LOCK")), "any case"); - std::fs::write(dir.path().join("Cargo.lock"), "").unwrap(); - assert!(!is_lock_file(&dir.path().join("Cargo.lock"))); - drop(held); + // 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(&session.join("lock")), - "a legacy lock file nobody holds needs no protecting" + 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"))); + 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"); } - /// The probe asks the file, so a hard link under any name to a held lock file is one too — the - /// same answer a case-insensitive filesystem's folded spelling gets. + /// 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 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 _key = try_lock(Target::Itself(&key)).unwrap().unwrap(); + 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. diff --git a/crates/agent/src/tools/mod.rs b/crates/agent/src/tools/mod.rs index e6e19dee..27327de5 100644 --- a/crates/agent/src/tools/mod.rs +++ b/crates/agent/src/tools/mod.rs @@ -177,17 +177,14 @@ pub(crate) fn write_atomic(path: &str, content: &[u8]) -> std::io::Result<()> { let p = std::path::Path::new(path); let resolved = resolve_symlink_target(p); let p = resolved.as_deref().unwrap_or(p); - // Never a fresh file renamed over a lock file — one named so, or any file locked right now (asked - // of the file, so a folded spelling, a legacy lock or a key held `Itself` are all seen): the lock - // would stay on the old inode, which the path no longer names. Opening one is harmless (see - // `file_lock`); replacing it is not. + // Never a fresh file renamed over a lock file — by its name in any case, by where it sits, or by + // being one this process holds (see `file_lock::is_lock_file`, which only `stat`s: a FIFO or a + // device is never opened, nobody's lock is ever tested): the lock would stay on the old inode, + // which the path no longer names. if crate::file_lock::is_lock_file(p) { return Err(std::io::Error::new( std::io::ErrorKind::PermissionDenied, - format!( - "{} is a lock file, or locked by a running process; it is not replaced", - p.display() - ), + format!("{} is a lock file; it is not replaced", p.display()), )); } let name = match p.file_name() { @@ -930,42 +927,26 @@ mod tests { assert_eq!(std::fs::read(&cargo_lock).unwrap(), b"[[package]]\n"); } - /// The cases a name cannot see: an old binary's session directory, whose legacy `lock` it holds - /// with no record lock file beside it; a journal key held through `Target::Itself`; and a - /// record lock file spelled in another case (one file on a case-insensitive filesystem). Each is - /// refused while held — asked of the file, not its name. + /// The lock files a held record lock file beside them cannot show: an old binary's session + /// directory (a segment and its legacy `lock`, no record lock file), its session file's + /// `.lock`, a journal key held through `Target::Itself`, and a record lock file spelled in + /// another case. A `lock` in a directory that holds no session is an ordinary file. #[test] #[cfg(unix)] - fn write_atomic_never_replaces_a_file_locked_right_now() { - use std::io::{BufRead as _, BufReader}; + fn write_atomic_never_replaces_a_lock_file_a_record_beside_it_cannot_show() { let dir = tempfile::tempdir().unwrap(); - // An old binary: an `flock` on `/lock`, nothing else. let old_session = dir.path().join("old"); std::fs::create_dir_all(&old_session).unwrap(); + std::fs::write(old_session.join("000001.jsonl"), "").unwrap(); let legacy = old_session.join("lock"); - if let Ok(mut old) = std::process::Command::new("python3") - .args([ - "-c", - "import fcntl,sys\nf=open(sys.argv[1],'a+')\nfcntl.flock(f,fcntl.LOCK_EX)\nprint('locked',flush=True)\nsys.stdin.read()", - ]) - .arg(&legacy) - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .spawn() - { - let mut line = String::new(); - BufReader::new(old.stdout.take().unwrap()) - .read_line(&mut line) - .unwrap(); - assert!( - write_atomic(legacy.to_str().unwrap(), b"x").is_err(), - "an old binary's held lock file is not replaced" - ); - drop(old.stdin.take()); - old.wait().unwrap(); - write_atomic(legacy.to_str().unwrap(), b"x").unwrap(); - } - // A journal key locked itself. + std::fs::write(&legacy, "").unwrap(); + assert!( + write_atomic(legacy.to_str().unwrap(), b"x").is_err(), + "an old binary's session lock is not replaced" + ); + let old_file_lock = dir.path().join("1700000000_abc.jsonl.lock"); + std::fs::write(&old_file_lock, "").unwrap(); + assert!(write_atomic(old_file_lock.to_str().unwrap(), b"x").is_err()); let key = dir.path().join("journal.key"); let held = crate::file_lock::try_lock(crate::file_lock::Target::Itself(&key)) .unwrap() @@ -975,13 +956,77 @@ mod tests { "a key held through Itself is not replaced" ); drop(held); - // A record lock file in another case. + write_atomic(key.to_str().unwrap(), b"x").unwrap(); let shouty = dir.path().join("S2.BEYOND-LOCK"); assert!(write_atomic(shouty.to_str().unwrap(), b"x").is_err()); + let plain = dir.path().join("plain"); + std::fs::create_dir_all(&plain).unwrap(); + write_atomic(plain.join("lock").to_str().unwrap(), b"x").unwrap(); + } + + /// Deciding whether a target is a lock file never opens it: a FIFO with no writer (whose + /// read-only open blocks forever) is written at once, and a device is only `stat`ed. + #[test] + #[cfg(unix)] + fn write_atomic_never_opens_its_target_to_decide() { + let dir = tempfile::tempdir().unwrap(); + let fifo = dir.path().join("pipe"); + let made = std::process::Command::new("mkfifo") + .arg(&fifo) + .status() + .is_ok_and(|s| s.success()); + assert!(made, "mkfifo"); + let (tx, rx) = std::sync::mpsc::channel(); + let path = fifo.to_str().unwrap().to_owned(); + std::thread::spawn(move || { + let _ = tx.send(write_atomic(&path, b"x")); + }); + rx.recv_timeout(std::time::Duration::from_secs(10)) + .expect("write_atomic hung on a FIFO") + .unwrap(); + assert_eq!(std::fs::read(&fifo).unwrap(), b"x"); + for device in ["/dev/null", "/dev/zero", "/dev/tty"] { + assert!(!crate::file_lock::is_lock_file(std::path::Path::new( + device + ))); + } + } + + /// A SQLite database in WAL mode that another program holds open mid-transaction (its POSIX + /// locks on the file) is an ordinary file, not a lock file: an edit of it goes through. + #[test] + #[cfg(unix)] + fn write_atomic_edits_a_sqlite_database_another_program_holds() { + use std::io::{BufRead as _, BufReader}; + let dir = tempfile::tempdir().unwrap(); + let db = dir.path().join("app.db"); + let Ok(mut other) = std::process::Command::new("python3") + .args([ + "-c", + "import sqlite3,sys\nc=sqlite3.connect(sys.argv[1],isolation_level=None)\nc.execute('pragma journal_mode=wal')\nc.execute('create table t(x)')\nc.execute('begin immediate')\nc.execute('insert into t values(1)')\nprint('held',flush=True)\nsys.stdin.read()", + ]) + .arg(&db) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .spawn() + else { + eprintln!("no python3: skipping the SQLite check"); + return; + }; + let mut line = String::new(); + BufReader::new(other.stdout.take().unwrap()) + .read_line(&mut line) + .unwrap(); + assert_eq!(line.trim(), "held"); + let written = write_atomic(db.to_str().unwrap(), b"replaced"); + drop(other.stdin.take()); + other.wait().unwrap(); + written.unwrap(); } /// On a case-insensitive filesystem (macOS; a casefold ext4 directory) `LOCK` and `.BEYOND-LOCK` - /// are the held lock files themselves. Skipped where the temp dir is case-sensitive. + /// are the held lock files themselves, and `LOCK` in an old binary's session directory is its + /// legacy lock. Skipped where the temp dir is case-sensitive. #[test] #[cfg(unix)] fn write_atomic_never_replaces_a_lock_file_through_a_folded_spelling() { @@ -995,11 +1040,19 @@ mod tests { let _held = crate::file_lock::try_lock(crate::file_lock::Target::Dir(&session)) .unwrap() .unwrap(); - for folded in ["LOCK", ".BEYOND-LOCK"] { - let p = session.join(folded); + let old_session = dir.path().join("old"); + std::fs::create_dir_all(&old_session).unwrap(); + std::fs::write(old_session.join("000001.jsonl"), "").unwrap(); + std::fs::write(old_session.join("lock"), "").unwrap(); + for folded in [ + session.join("LOCK"), + session.join(".BEYOND-LOCK"), + old_session.join("LOCK"), + ] { assert!( - write_atomic(p.to_str().unwrap(), b"x").is_err(), - "{folded} reaches a held lock file" + write_atomic(folded.to_str().unwrap(), b"x").is_err(), + "{} reaches a lock file", + folded.display() ); } }