From 39cea3ead398b6268ee225dc80f550a8de433e3a Mon Sep 17 00:00:00 2001 From: Tyler Date: Wed, 19 Aug 2026 13:53:54 +0000 Subject: [PATCH 1/4] Stop writing settings unserialized when flock is unavailable SBS-947: serialize with exclusive-create on flock-less mounts, repair a stale unopenable leftover, and fail closed on unknown lock errors. --- CHANGELOG.md | 1 + rust/src/secure_file.rs | 517 ++++++++++++++++++++++++++++++++++------ 2 files changed, 441 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d233a23d..8c753836 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Fixed - **A corrupted Claude de-duplication key no longer silently inflates Charts and Estimated API value.** The usage index treated invalid UTF-8 in a present `dedup_key` as "this record has no key", so the same transcript event could be counted twice if its counterpart still had one. Invalid UTF-8 now rejects the whole index the way a required string already does, and the next scan rebuilds it from the transcripts. A key that was never written is still encoded as the length sentinel, not as a failed decode. +- **A filesystem that cannot flock no longer writes settings unserialized.** The state-write lock treated every `flock` errno other than contention as "no lock needed" and continued, so an NFS/FUSE/SMB home without lockd, or a leftover `state-write.lock` the current user cannot open, let the tray and the CLI replace `api_keys.json` over each other. Those mounts now serialize with an exclusive-create sibling and a staleness timeout. A stale unopenable leftover is removed and the lock is taken again. A directory in the lock path, a leftover that cannot be repaired, or an unknown flock errno fails the write instead of skipping the lock. - **Windows stops handing an npm shim or a directory to the PTY as the Codex or Claude binary.** `where.exe` prints every match on PATH and the resolver took the first line, so an extensionless POSIX shim named `codex` sitting ahead of `codex.exe` went straight to `CreateProcessW` and failed with error 193. Every candidate is now examined, a native `.exe` or `.com` wins over a `.cmd` or `.bat` shim wherever the shim sits in the list, and directories and extensionless files are refused — for PATH results, for the `CODEX_BINARY` and `CLAUDE_BINARY` overrides, and for an explicit path. A `.cmd` or `.bat` shim now runs through `%COMSPEC% /d /c` instead of being launched directly, and its arguments are escaped for the interpreter rather than for `CreateProcessW`, because the two do not agree on `&`, `|`, or `>`. The few tails `cmd.exe` cannot express at all fail with a message rather than launch something other than what was asked for. Closes #270. Thanks @ITSMERNB! ## [Ceiling] 1.5.34 - 2026-08-18 diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index 79ea3e6f..c2bf2ad1 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -22,10 +22,11 @@ const STATE_LOCK_RETRY: std::time::Duration = std::time::Duration::from_millis(2 /// Every settings and credential store shares one lock so operations spanning /// multiple files cannot interleave with a writer for any one of those files. /// -/// Where the lock cannot be enforced at all - a filesystem without `flock`, a -/// lock file this user can never open - the operation still runs, unserialized, -/// and a warning names the lock path. Blocking every write would be worse than -/// the interleaving risk, and the old lock protocol worked on those mounts. +/// `flock` is preferred because the kernel drops it if the process dies. When +/// the mount cannot flock (NFS without lockd, some FUSE/SMB), writers serialize +/// with an exclusive-create sibling and a staleness timeout. A lock file this +/// user cannot open is repaired when it is stale. Unknown lock errors fail the +/// write; they are not treated as "no lock needed" (SBS-947). pub fn with_state_write_lock(operation: impl FnOnce() -> io::Result) -> io::Result { let lock_path = dirs::config_dir() .unwrap_or_else(|| PathBuf::from(".")) @@ -66,14 +67,30 @@ pub(crate) fn with_file_write_lock( with_state_write_lock_at(&parent.join(lock_name), operation) } +/// Sibling used when `flock` is unsupported. `create_new` is atomic on NFSv3+, +/// SMB, and FUSE even when `flock` returns `ENOLCK` / `ENOTSUP`. +fn exclusive_lock_path(lock_path: &Path) -> PathBuf { + let mut name = lock_path + .file_name() + .unwrap_or_else(|| std::ffi::OsStr::new("state-write.lock")) + .to_os_string(); + name.push(".excl"); + match lock_path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.join(name), + _ => PathBuf::from(name), + } +} + struct StateWriteLock { - /// `None` once the lock was found to be unenforceable; see [`LockAttempt`]. + /// `None` when this holder used exclusive-create or the test helper. #[cfg(windows)] handle: Option, /// Open lock file whose exclusive flock is released when this is dropped. - /// `None` once the lock was found to be unenforceable; see [`LockAttempt`]. + /// `None` when this holder used exclusive-create or the test helper. #[cfg(not(windows))] _file: Option, + /// Set when this holder created the exclusive-create sibling. Unlinked on drop. + exclusive_path: Option, } /// Outcome of one attempt to take the state-write lock. @@ -81,12 +98,26 @@ enum LockAttempt { Acquired(StateWriteLock), /// Another live holder has the lock, so the attempt is worth repeating. Contended, - /// The lock can never be taken through this path: the filesystem does not - /// implement `flock`, or the lock file itself is unopenable (left by a - /// privileged run, replaced by a directory, read-only mount). Retrying - /// would only stall every write until the timeout and then fail it. - Unenforceable(io::Error), - /// Something unrelated to locking went wrong; the caller sees the error. + /// The mount does not implement `flock`. Exclusive-create is the fallback. + FlockUnsupported(io::Error), + /// The lock file exists but this process cannot open it. + Unopenable(io::Error), + /// Something unrelated to locking went wrong, or a lock error we do not + /// know how to interpret. The caller sees the error; the write does not + /// proceed unserialized. + Failed(io::Error), +} + +enum LockFileAge { + Missing, + Fresh, + Stale, + Unknown(io::Error), +} + +enum Repair { + Done, + Wait, Failed(io::Error), } @@ -103,43 +134,141 @@ impl StateWriteLock { loop { match attempt(path) { LockAttempt::Acquired(lock) => return Ok(lock), - LockAttempt::Unenforceable(error) => { - // Degrade instead of blocking a legitimate write, but say - // so: this write is not serialized against other processes. - tracing::warn!( - lock_path = %path.display(), - %error, - "state write lock cannot be enforced here; writing without cross-process serialization" - ); - return Ok(Self::unenforced()); - } LockAttempt::Failed(error) => return Err(error), - LockAttempt::Contended => { - if std::time::Instant::now() >= deadline { - return Err(io::Error::new( - io::ErrorKind::WouldBlock, - "state store is locked", - )); + LockAttempt::FlockUnsupported(error) => { + // `attempt` is usually `try_acquire`, which already falls + // back. A custom attempt that reports this still serializes + // rather than writing unserialized. + match Self::try_exclusive_create(path) { + LockAttempt::Acquired(lock) => { + tracing::info!( + lock_path = %path.display(), + %error, + "flock is unsupported here; serializing the write with exclusive-create" + ); + return Ok(lock); + } + LockAttempt::Contended => {} + LockAttempt::Failed(fallback) => { + return Err(io::Error::new( + fallback.kind(), + format!( + "flock is unsupported ({error}) and exclusive-create failed: {fallback}" + ), + )); + } + other => { + return Err(lock_attempt_unexpected( + other, + "exclusive-create fallback", + )); + } } - std::thread::sleep(STATE_LOCK_RETRY); } + LockAttempt::Unopenable(error) => match try_repair_unopenable(path, &error) { + Repair::Done => continue, + Repair::Wait => {} + Repair::Failed(repair) => return Err(repair), + }, + LockAttempt::Contended => {} } + if std::time::Instant::now() >= deadline { + return Err(io::Error::new( + io::ErrorKind::WouldBlock, + "state store is locked", + )); + } + std::thread::sleep(STATE_LOCK_RETRY); } } - /// A lock object that owns nothing, for filesystems and lock paths where no - /// lock can be taken. - fn unenforced() -> Self { + /// A lock object that owns nothing, for tests that need an `Acquired` value. + #[cfg(test)] + fn empty() -> Self { Self { #[cfg(windows)] handle: None, #[cfg(not(windows))] _file: None, + exclusive_path: None, } } - #[cfg(windows)] fn try_acquire(path: &Path) -> LockAttempt { + match Self::try_primary(path) { + LockAttempt::FlockUnsupported(_) => Self::try_exclusive_create(path), + LockAttempt::Unopenable(error) => Self::after_unopenable(path, error), + other => other, + } + } + + fn after_unopenable(path: &Path, error: io::Error) -> LockAttempt { + match try_repair_unopenable(path, &error) { + Repair::Done => match Self::try_primary(path) { + LockAttempt::FlockUnsupported(_) => Self::try_exclusive_create(path), + LockAttempt::Unopenable(still) => LockAttempt::Failed(still), + other => other, + }, + Repair::Wait => LockAttempt::Contended, + Repair::Failed(error) => LockAttempt::Failed(error), + } + } + + fn try_exclusive_create(path: &Path) -> LockAttempt { + let exclusive = exclusive_lock_path(path); + let mut options = std::fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + match options.open(&exclusive) { + Ok(file) => { + #[cfg(windows)] + let _ = file; + LockAttempt::Acquired(Self { + #[cfg(windows)] + handle: None, + #[cfg(not(windows))] + _file: Some(file), + exclusive_path: Some(exclusive), + }) + } + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => { + match lock_file_age(&exclusive) { + LockFileAge::Stale | LockFileAge::Missing => { + match std::fs::remove_file(&exclusive) { + Ok(()) => LockAttempt::Contended, + Err(error) if error.kind() == io::ErrorKind::NotFound => { + LockAttempt::Contended + } + Err(error) => LockAttempt::Failed(io::Error::new( + error.kind(), + format!( + "could not remove a stale exclusive-create lock file: {error}" + ), + )), + } + } + LockFileAge::Fresh => LockAttempt::Contended, + LockFileAge::Unknown(error) => LockAttempt::Failed(io::Error::new( + error.kind(), + format!( + "could not tell whether the exclusive-create lock file is stale: {error}" + ), + )), + } + } + Err(error) => LockAttempt::Failed(io::Error::new( + error.kind(), + format!("could not create the exclusive-create lock file: {error}"), + )), + } + } + + #[cfg(windows)] + fn try_primary(path: &Path) -> LockAttempt { use std::os::windows::ffi::OsStrExt; use windows::Win32::Foundation::{GENERIC_READ, GENERIC_WRITE}; use windows::Win32::Storage::FileSystem::{ @@ -161,13 +290,14 @@ impl StateWriteLock { } { Ok(handle) => LockAttempt::Acquired(Self { handle: Some(handle), + exclusive_path: None, }), Err(error) => classify_open_failure(&error), } } #[cfg(not(windows))] - fn try_acquire(path: &Path) -> LockAttempt { + fn try_primary(path: &Path) -> LockAttempt { let mut options = std::fs::OpenOptions::new(); options.read(true).write(true).create(true); #[cfg(unix)] @@ -180,24 +310,98 @@ impl StateWriteLock { Err(error) => return classify_open_failure(&error), }; match file.try_lock() { - Ok(()) => LockAttempt::Acquired(Self { _file: Some(file) }), + Ok(()) => LockAttempt::Acquired(Self { + _file: Some(file), + exclusive_path: None, + }), Err(error) => classify_lock_failure(error), } } } +fn lock_attempt_unexpected(attempt: LockAttempt, context: &str) -> io::Error { + let detail = match attempt { + LockAttempt::Acquired(_) => "acquired".to_string(), + LockAttempt::Contended => "contended".to_string(), + LockAttempt::FlockUnsupported(error) => format!("flock-unsupported: {error}"), + LockAttempt::Unopenable(error) => format!("unopenable: {error}"), + LockAttempt::Failed(error) => format!("failed: {error}"), + }; + io::Error::other(format!( + "internal lock state {detail} is not valid during {context}" + )) +} + +fn lock_file_age(path: &Path) -> LockFileAge { + match std::fs::metadata(path) { + Err(error) if error.kind() == io::ErrorKind::NotFound => LockFileAge::Missing, + Err(error) => LockFileAge::Unknown(error), + Ok(metadata) if metadata.is_dir() => LockFileAge::Unknown(io::Error::new( + io::ErrorKind::IsADirectory, + format!("state lock path is a directory: {}", path.display()), + )), + Ok(metadata) => match metadata.modified() { + Err(error) => LockFileAge::Unknown(error), + Ok(modified) => match std::time::SystemTime::now().duration_since(modified) { + Ok(age) if age >= STATE_LOCK_TIMEOUT => LockFileAge::Stale, + Ok(_) | Err(_) => LockFileAge::Fresh, + }, + }, + } +} + +fn try_repair_unopenable(path: &Path, open_error: &io::Error) -> Repair { + if path.is_dir() { + return Repair::Failed(io::Error::new( + io::ErrorKind::IsADirectory, + format!("state lock path is a directory: {}", path.display()), + )); + } + if matches!(open_error.kind(), io::ErrorKind::ReadOnlyFilesystem) { + return Repair::Failed(io::Error::new( + open_error.kind(), + format!("could not open the state lock file: {open_error}"), + )); + } + match lock_file_age(path) { + LockFileAge::Missing => Repair::Done, + LockFileAge::Stale => match std::fs::remove_file(path) { + Ok(()) => { + tracing::warn!( + lock_path = %path.display(), + %open_error, + "removed a stale unopenable leftover state lock file" + ); + Repair::Done + } + Err(error) if error.kind() == io::ErrorKind::NotFound => Repair::Done, + Err(error) => Repair::Failed(io::Error::new( + error.kind(), + format!( + "could not repair an unopenable state lock file ({}): {error}", + path.display() + ), + )), + }, + LockFileAge::Fresh => Repair::Wait, + LockFileAge::Unknown(error) => Repair::Failed(io::Error::new( + error.kind(), + format!("could not tell whether the unopenable state lock file is stale: {error}"), + )), + } +} + /// Classify a Win32 failure to open the lock file. /// /// A sharing or lock violation is a live holder. `ERROR_ACCESS_DENIED` is not: -/// the lock file exists but this user can never open it, so treating it as -/// contention would stall every settings write for the full timeout and then -/// fail it. +/// the lock file exists but this user can never open it. That is unopenable, +/// not "no lock needed". #[cfg(windows)] fn classify_open_failure(error: &windows::core::Error) -> LockAttempt { let io_error = || io::Error::other(format!("could not acquire state lock: {error}")); match win32_error_code(error) { WIN32_ERROR_SHARING_VIOLATION | WIN32_ERROR_LOCK_VIOLATION => LockAttempt::Contended, - WIN32_ERROR_ACCESS_DENIED => LockAttempt::Unenforceable(io_error()), + WIN32_ERROR_ACCESS_DENIED => LockAttempt::Unopenable(io_error()), _ => LockAttempt::Failed(io_error()), } } @@ -205,9 +409,8 @@ fn classify_open_failure(error: &windows::core::Error) -> LockAttempt { /// Classify a failure to open the lock file. /// /// Nothing here means "someone else holds the lock" - `open` does not block on -/// `flock`. A lock file this process can never open (left behind by a -/// privileged run, shadowed by a directory, on a read-only mount) would -/// otherwise stall every settings write for the full timeout and then fail it. +/// `flock`. A lock file this process can never open is unopenable, not a reason +/// to write unserialized. #[cfg(not(windows))] fn classify_open_failure(error: &io::Error) -> LockAttempt { let unopenable = matches!( @@ -221,7 +424,7 @@ fn classify_open_failure(error: &io::Error) -> LockAttempt { format!("could not open the state lock file: {error}"), ); if unopenable { - LockAttempt::Unenforceable(error) + LockAttempt::Unopenable(error) } else { LockAttempt::Failed(error) } @@ -230,10 +433,9 @@ fn classify_open_failure(error: &io::Error) -> LockAttempt { /// Classify a `flock` failure on the opened lock file. /// /// `WouldBlock` is the only answer that means another live holder has the lock. -/// A signal is worth another attempt. Every other errno says this filesystem -/// cannot enforce `flock` at all (NFS without lockd, some FUSE and SMB mounts), -/// and the old `create_new` protocol used to work there, so the write must not -/// be blocked by it. +/// A signal is worth another attempt. `ENOTSUP` / `ENOLCK` mean this filesystem +/// cannot flock; exclusive-create is the fallback. Every other errno is unknown +/// and fails the write (SBS-947). #[cfg(not(windows))] fn classify_lock_failure(error: std::fs::TryLockError) -> LockAttempt { match error { @@ -241,26 +443,49 @@ fn classify_lock_failure(error: std::fs::TryLockError) -> LockAttempt { std::fs::TryLockError::Error(error) if error.kind() == io::ErrorKind::Interrupted => { LockAttempt::Contended } - std::fs::TryLockError::Error(error) => LockAttempt::Unenforceable(io::Error::new( + std::fs::TryLockError::Error(error) if is_flock_unsupported(&error) => { + LockAttempt::FlockUnsupported(io::Error::new( + error.kind(), + format!("this filesystem cannot lock the state lock file: {error}"), + )) + } + std::fs::TryLockError::Error(error) => LockAttempt::Failed(io::Error::new( error.kind(), - format!("this filesystem cannot lock the state lock file: {error}"), + format!("could not lock the state lock file: {error}"), )), } } +#[cfg(not(windows))] +fn is_flock_unsupported(error: &io::Error) -> bool { + if error.kind() == io::ErrorKind::Unsupported { + return true; + } + #[cfg(unix)] + { + error.raw_os_error() == Some(libc::ENOLCK) + } + #[cfg(not(unix))] + { + false + } +} + impl Drop for StateWriteLock { fn drop(&mut self) { + if let Some(path) = self.exclusive_path.take() { + let _ = std::fs::remove_file(&path); + } #[cfg(windows)] if let Some(handle) = self.handle.take() { unsafe { let _ = windows::Win32::Foundation::CloseHandle(handle); } } - // Non-Windows: closing `_file` releases the flock. Leave the lock file - // so a leftover after crash is not treated as a live holder, and so - // unlinking cannot create a second lock inode while another holder - // still has the original file open. A leftover this process cannot open - // no longer blocks writes; see `classify_open_failure`. + // Non-Windows flock: closing `_file` releases the lock. Leave the flock + // lock file so a leftover after crash is not treated as a live holder, + // and so unlinking cannot create a second lock inode while another + // holder still has the original file open. } } @@ -1542,11 +1767,24 @@ mod tests { ); } + fn make_stale(path: &Path) { + let file = std::fs::OpenOptions::new().write(true).open(path).unwrap(); + file.set_modified( + std::time::SystemTime::now() - STATE_LOCK_TIMEOUT - std::time::Duration::from_secs(1), + ) + .unwrap(); + } + #[cfg(unix)] fn take_lock(path: &Path) -> StateWriteLock { match StateWriteLock::try_acquire(path) { LockAttempt::Acquired(lock) => lock, - _ => panic!("the lock must be free"), + LockAttempt::Contended => panic!("the lock must be free (contended)"), + LockAttempt::FlockUnsupported(error) => { + panic!("the lock must be free (flock unsupported: {error})") + } + LockAttempt::Unopenable(error) => panic!("the lock must be free (unopenable: {error})"), + LockAttempt::Failed(error) => panic!("the lock must be free (failed: {error})"), } } @@ -1623,19 +1861,107 @@ mod tests { assert_eq!(result.unwrap(), 42); } + /// Pins SBS-947: an unenforceable report is no longer a success path that + /// writes with no cross-process exclusion. #[test] - fn an_unenforceable_lock_lets_the_write_through_once() { + fn an_unenforceable_lock_fails_closed_instead_of_writing_unserialized() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + // Make exclusive-create fail too: put a directory on the sibling path. + std::fs::create_dir(exclusive_lock_path(&path)).unwrap(); + let mut attempts = 0; let started = std::time::Instant::now(); - let lock = StateWriteLock::acquire_with(Path::new("state-write.lock"), |_| { + let result = StateWriteLock::acquire_with(&path, |_| { attempts += 1; - LockAttempt::Unenforceable(io::Error::from(io::ErrorKind::Unsupported)) + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) + }); + + match result { + Ok(_) => panic!("flock-unsupported plus a broken exclusive-create must fail closed"), + Err(error) => { + let message = error.to_string(); + assert!( + message.contains("exclusive-create failed") + || message.contains("could not create"), + "the error must name the failed fallback, got {message}" + ); + } + } + assert_eq!( + attempts, 1, + "a failed fallback must not be retried as success" + ); + assert!(started.elapsed() < std::time::Duration::from_secs(1)); + } + + #[test] + fn flock_unsupported_serializes_through_exclusive_create() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + + let lock = StateWriteLock::acquire_with(&path, |_| { + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) }) - .expect("a lock that cannot be enforced must not block a legitimate write"); + .expect("flock-unsupported must fall back to exclusive-create, not skip the lock"); + + assert!( + exclusive_lock_path(&path).exists(), + "the exclusive-create sibling must exist while held" + ); + + let (ready_tx, ready_rx) = std::sync::mpsc::channel(); + let (done_tx, done_rx) = std::sync::mpsc::channel(); + let waiter_path = path.clone(); + std::thread::spawn(move || { + ready_tx.send(()).unwrap(); + let result = StateWriteLock::acquire_with(&waiter_path, |_| { + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) + }); + let _ = done_tx.send(result.map(|_| ())); + }); + ready_rx.recv().unwrap(); + assert!( + done_rx + .recv_timeout(std::time::Duration::from_millis(80)) + .is_err(), + "exclusive-create must serialize a second flock-unsupported writer" + ); drop(lock); - assert_eq!(attempts, 1, "an unenforceable lock must not be retried"); - assert!(started.elapsed() < std::time::Duration::from_secs(1)); + let result = done_rx + .recv_timeout(std::time::Duration::from_secs(2)) + .expect("waiter should acquire after the exclusive-create holder drops"); + result.unwrap(); + assert!( + !exclusive_lock_path(&path).exists(), + "dropping the exclusive-create holder must unlink the sibling" + ); + } + + #[test] + fn exclusive_create_recovers_a_stale_sibling_after_crash() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + let exclusive = exclusive_lock_path(&path); + std::fs::write(&exclusive, b"leftover after SIGKILL").unwrap(); + make_stale(&exclusive); + + let started = std::time::Instant::now(); + let lock = StateWriteLock::acquire_with(&path, |_| { + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) + }) + .expect("a stale exclusive-create leftover must be taken, not waited out"); + drop(lock); + + assert!( + started.elapsed() < std::time::Duration::from_secs(1), + "a stale exclusive-create leftover must not wait out the acquire timeout" + ); + assert!( + !exclusive.exists(), + "the recovered holder must unlink the sibling on drop" + ); } #[test] @@ -1646,7 +1972,7 @@ mod tests { if attempts < 3 { LockAttempt::Contended } else { - LockAttempt::Acquired(StateWriteLock::unenforced()) + LockAttempt::Acquired(StateWriteLock::empty()) } }) .expect("contention must be retried until the holder releases"); @@ -1663,7 +1989,7 @@ mod tests { #[cfg(unix)] #[test] - fn a_filesystem_without_flock_support_degrades_instead_of_failing() { + fn a_filesystem_without_flock_support_is_classified_for_exclusive_create() { assert!( matches!( classify_lock_failure(std::fs::TryLockError::WouldBlock), @@ -1681,49 +2007,78 @@ mod tests { "a signal must be retried, not treated as a broken filesystem" ); - // ENOTSUP / ENOLCK from NFS, SMB or FUSE mounts. match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from( io::ErrorKind::Unsupported, ))) { - LockAttempt::Unenforceable(error) => assert!( + LockAttempt::FlockUnsupported(error) => assert!( error.to_string().contains("cannot lock"), - "the degraded path must name the reason, got {error}" + "the flock-unsupported path must name the reason, got {error}" + ), + _ => panic!("ENOTSUP must be flock-unsupported, not a silent degrade or a hard fail"), + } + + match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from_raw_os_error( + libc::ENOLCK, + ))) { + LockAttempt::FlockUnsupported(_) => {} + _ => panic!("ENOLCK must be flock-unsupported"), + } + } + + /// Unknown flock errnos are not "the filesystem cannot lock". + #[cfg(unix)] + #[test] + fn an_unknown_flock_error_fails_the_write() { + match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from( + io::ErrorKind::InvalidInput, + ))) { + LockAttempt::Failed(error) => assert!( + error.to_string().contains("could not lock"), + "unknown must stay unknown, got {error}" ), - _ => panic!("an flock-less filesystem must degrade, not fail the write"), + LockAttempt::FlockUnsupported(_) => { + panic!("an unknown errno must not be collapsed into flock-unsupported") + } + LockAttempt::Contended => panic!("an unknown errno is not contention"), + LockAttempt::Unopenable(_) => panic!("an unknown flock errno is not an open failure"), + LockAttempt::Acquired(_) => panic!("an unknown errno must not acquire"), } } #[test] - fn a_lock_path_that_cannot_be_opened_does_not_block_the_write() { + fn a_lock_path_that_is_a_directory_fails_closed() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("state-write.lock"); - // A directory in the lock file's place can never be opened as a lock - // file, the same dead end as a leftover owned by another user. std::fs::create_dir(&path).unwrap(); let started = std::time::Instant::now(); let mut ran = false; - with_state_write_lock_at(&path, || { + let error = with_state_write_lock_at(&path, || { ran = true; Ok(()) }) - .expect("an unopenable lock path must not fail a legitimate write"); + .expect_err("a directory lock path must fail the write, not skip the lock"); - assert!(ran); + assert!(!ran, "the write must not run without a lock"); + assert!( + error.to_string().contains("directory"), + "the error must name the directory, got {error}" + ); assert!( started.elapsed() < std::time::Duration::from_secs(1), - "an unopenable lock path must not wait out the acquire timeout" + "a directory lock path must not wait out the acquire timeout" ); } #[cfg(unix)] #[test] - fn an_unopenable_leftover_lock_file_does_not_block_the_write() { + fn a_stale_unopenable_leftover_lock_file_is_repaired() { use std::os::unix::fs::PermissionsExt; let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("state-write.lock"); std::fs::write(&path, b"leftover from a privileged run").unwrap(); + make_stale(&path); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); if std::fs::OpenOptions::new() .read(true) @@ -1741,12 +2096,20 @@ mod tests { ran = true; Ok(()) }) - .expect("a leftover lock file this user cannot open must not fail the write"); + .expect("a stale leftover this user cannot open must be repaired, not skipped"); assert!(ran); assert!( started.elapsed() < std::time::Duration::from_secs(1), - "an unopenable leftover must not wait out the acquire timeout" + "a stale leftover must be repaired without waiting out the acquire timeout" + ); + assert!( + std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .is_ok(), + "repair must leave an openable lock file for the next writer" ); } } From 555f615918a9022f99b4a8dbf362413af7167357 Mon Sep 17 00:00:00 2001 From: Tyler South Date: Sat, 22 Aug 2026 06:48:27 -0400 Subject: [PATCH 2/4] Close the exclusive-create lock holes in the flock fallback The fallback that serializes writes when a mount cannot flock could still let two writers through, and the Windows build did not compile. - Only ENOTSUP falls back. ENOLCK also means the lock table is full or lockd failed for one call, while another process holds a real flock; serializing on the sibling instead let both writers run. - Split the crash-recovery staleness threshold (2m) from the acquire timeout (10s), so a waiter can never outlast a live holder and take its sibling. A future mtime reads as held rather than as expired. - A holder unlinks the sibling only while it is still the file it created, so a takeover cannot cascade into deleting a third lock. - Stop unlinking an unopenable state-write.lock. A live holder still has that inode open, and the replacement put two writers on two inodes. Fail the write and name the path instead. - Silence dead_code for LockAttempt::FlockUnsupported on Windows, which matches the variant but never constructs it. This was the CI failure. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- rust/src/secure_file.rs | 283 +++++++++++++++++++++++++++++++--------- 2 files changed, 220 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c753836..6938728e 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ ### Fixed - **A corrupted Claude de-duplication key no longer silently inflates Charts and Estimated API value.** The usage index treated invalid UTF-8 in a present `dedup_key` as "this record has no key", so the same transcript event could be counted twice if its counterpart still had one. Invalid UTF-8 now rejects the whole index the way a required string already does, and the next scan rebuilds it from the transcripts. A key that was never written is still encoded as the length sentinel, not as a failed decode. -- **A filesystem that cannot flock no longer writes settings unserialized.** The state-write lock treated every `flock` errno other than contention as "no lock needed" and continued, so an NFS/FUSE/SMB home without lockd, or a leftover `state-write.lock` the current user cannot open, let the tray and the CLI replace `api_keys.json` over each other. Those mounts now serialize with an exclusive-create sibling and a staleness timeout. A stale unopenable leftover is removed and the lock is taken again. A directory in the lock path, a leftover that cannot be repaired, or an unknown flock errno fails the write instead of skipping the lock. +- **A filesystem that cannot flock no longer writes settings unserialized.** The state-write lock treated every `flock` errno other than contention as "no lock needed" and continued, so an NFS/FUSE/SMB home without `lockd` let the tray and the CLI replace `api_keys.json` over each other. Only `ENOTSUP` — the filesystem saying it does not implement locking at all — now falls back, and the fallback is an exclusive-create sibling that a second writer cannot take. `ENOLCK` is no longer treated as a broken filesystem: it also means the kernel lock table is full or `lockd` failed for that one call, while another process still holds a real flock, so falling back to the sibling would have let both writers through. It fails the write instead. An unknown errno already did. Because the sibling has no kernel-backed release, a leftover from a killed process still has to age out, but the age it has to reach is now two minutes rather than the ten seconds an acquirer waits — a holder doing slow work can no longer be outlasted and have its lock taken, and a lock file stamped in the future by a skewed server clock reads as held rather than as expired. A holder unlinks the sibling on release only while it is still the same file it created, so a takeover cannot cascade into deleting a third writer's lock. A `state-write.lock` the current user cannot open — left by a `sudo` run, or by another account — now fails the write and names the path to remove. It used to be unlinked and recreated, which put the running holder and the new one on two different inodes and let both writes proceed. A directory in the lock path still fails the write. - **Windows stops handing an npm shim or a directory to the PTY as the Codex or Claude binary.** `where.exe` prints every match on PATH and the resolver took the first line, so an extensionless POSIX shim named `codex` sitting ahead of `codex.exe` went straight to `CreateProcessW` and failed with error 193. Every candidate is now examined, a native `.exe` or `.com` wins over a `.cmd` or `.bat` shim wherever the shim sits in the list, and directories and extensionless files are refused — for PATH results, for the `CODEX_BINARY` and `CLAUDE_BINARY` overrides, and for an explicit path. A `.cmd` or `.bat` shim now runs through `%COMSPEC% /d /c` instead of being launched directly, and its arguments are escaped for the interpreter rather than for `CreateProcessW`, because the two do not agree on `&`, `|`, or `>`. The few tails `cmd.exe` cannot express at all fail with a message rather than launch something other than what was asked for. Closes #270. Thanks @ITSMERNB! ## [Ceiling] 1.5.34 - 2026-08-18 diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index c2bf2ad1..26ac4252 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -14,8 +14,18 @@ const WINDOWS_DPAPI_USER: &str = "windows-dpapi-user"; const WINDOWS_DPAPI_MACHINE: &str = "windows-dpapi-machine"; static TEMP_FILE_COUNTER: AtomicU64 = AtomicU64::new(0); +/// How long an acquirer waits for a live holder before giving up. const STATE_LOCK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const STATE_LOCK_RETRY: std::time::Duration = std::time::Duration::from_millis(20); +/// How old an exclusive-create sibling must be before it is treated as a crash +/// leftover rather than a live holder. +/// +/// Deliberately much longer than `STATE_LOCK_TIMEOUT`: the sibling has no +/// kernel-backed release, so staleness is the only crash recovery there is, but +/// a holder that is merely slow must never be mistaken for a dead one. Because +/// this exceeds the acquire timeout, a waiter that arrives while a holder is +/// working can never outlast it and steal the lock (SBS-947). +const STATE_LOCK_STALE: std::time::Duration = std::time::Duration::from_secs(120); /// Serialize read-modify-write transactions across Ceiling processes. /// @@ -91,6 +101,10 @@ struct StateWriteLock { _file: Option, /// Set when this holder created the exclusive-create sibling. Unlinked on drop. exclusive_path: Option, + /// Identity of the sibling this holder created, so drop can tell its own + /// file from a replacement made after a stale takeover. + #[cfg(unix)] + exclusive_id: Option<(u64, u64)>, } /// Outcome of one attempt to take the state-write lock. @@ -99,6 +113,10 @@ enum LockAttempt { /// Another live holder has the lock, so the attempt is worth repeating. Contended, /// The mount does not implement `flock`. Exclusive-create is the fallback. + /// + /// Only `classify_lock_failure` builds this, and Windows locks through + /// `CreateFileW` instead, so there it is matched but never constructed. + #[cfg_attr(windows, allow(dead_code))] FlockUnsupported(io::Error), /// The lock file exists but this process cannot open it. Unopenable(io::Error), @@ -116,8 +134,8 @@ enum LockFileAge { } enum Repair { + /// The lock file is gone, so the open failure was transient. Try again. Done, - Wait, Failed(io::Error), } @@ -167,7 +185,6 @@ impl StateWriteLock { } LockAttempt::Unopenable(error) => match try_repair_unopenable(path, &error) { Repair::Done => continue, - Repair::Wait => {} Repair::Failed(repair) => return Err(repair), }, LockAttempt::Contended => {} @@ -191,6 +208,8 @@ impl StateWriteLock { #[cfg(not(windows))] _file: None, exclusive_path: None, + #[cfg(unix)] + exclusive_id: None, } } @@ -209,7 +228,6 @@ impl StateWriteLock { LockAttempt::Unopenable(still) => LockAttempt::Failed(still), other => other, }, - Repair::Wait => LockAttempt::Contended, Repair::Failed(error) => LockAttempt::Failed(error), } } @@ -225,6 +243,8 @@ impl StateWriteLock { } match options.open(&exclusive) { Ok(file) => { + #[cfg(unix)] + let exclusive_id = file_identity(&file); #[cfg(windows)] let _ = file; LockAttempt::Acquired(Self { @@ -233,6 +253,8 @@ impl StateWriteLock { #[cfg(not(windows))] _file: Some(file), exclusive_path: Some(exclusive), + #[cfg(unix)] + exclusive_id, }) } Err(error) if error.kind() == io::ErrorKind::AlreadyExists => { @@ -291,6 +313,8 @@ impl StateWriteLock { Ok(handle) => LockAttempt::Acquired(Self { handle: Some(handle), exclusive_path: None, + #[cfg(unix)] + exclusive_id: None, }), Err(error) => classify_open_failure(&error), } @@ -313,6 +337,8 @@ impl StateWriteLock { Ok(()) => LockAttempt::Acquired(Self { _file: Some(file), exclusive_path: None, + #[cfg(unix)] + exclusive_id: None, }), Err(error) => classify_lock_failure(error), } @@ -332,6 +358,22 @@ fn lock_attempt_unexpected(attempt: LockAttempt, context: &str) -> io::Error { )) } +/// `(device, inode)` of an open file, so a later `stat` of the same path can +/// tell whether it is still the same file. +#[cfg(unix)] +fn file_identity(file: &std::fs::File) -> Option<(u64, u64)> { + use std::os::unix::fs::MetadataExt; + let metadata = file.metadata().ok()?; + Some((metadata.dev(), metadata.ino())) +} + +#[cfg(unix)] +fn path_identity(path: &Path) -> Option<(u64, u64)> { + use std::os::unix::fs::MetadataExt; + let metadata = std::fs::metadata(path).ok()?; + Some((metadata.dev(), metadata.ino())) +} + fn lock_file_age(path: &Path) -> LockFileAge { match std::fs::metadata(path) { Err(error) if error.kind() == io::ErrorKind::NotFound => LockFileAge::Missing, @@ -343,13 +385,34 @@ fn lock_file_age(path: &Path) -> LockFileAge { Ok(metadata) => match metadata.modified() { Err(error) => LockFileAge::Unknown(error), Ok(modified) => match std::time::SystemTime::now().duration_since(modified) { - Ok(age) if age >= STATE_LOCK_TIMEOUT => LockFileAge::Stale, - Ok(_) | Err(_) => LockFileAge::Fresh, + Ok(age) if age >= STATE_LOCK_STALE => LockFileAge::Stale, + Ok(_) => LockFileAge::Fresh, + // The mtime is in the future, so the age is not measurable: + // NFS server skew, a restored VM, or a clock set forward. Stay + // on the safe side and treat it as a live holder, but say so, + // because until the clock catches up this never ages out. + Err(error) => { + tracing::warn!( + lock_path = %path.display(), + %error, + "state lock file is stamped in the future; treating it as held until the clock catches up" + ); + LockFileAge::Fresh + } }, }, } } +/// Decide what to do about a lock file this process cannot open. +/// +/// Unlinking is never one of the options. `Drop` deliberately leaves the flock +/// lock file in place, so a live holder still has this inode open; removing it +/// and creating a replacement would put two writers on two different inodes and +/// let both writes run. The flock file's mtime is stamped at first create and +/// never refreshed, so age cannot distinguish a dead leftover from a running +/// holder either. A file this user cannot open is a leftover from a privileged +/// or different-user run, so fail the write and name the path (SBS-947). fn try_repair_unopenable(path: &Path, open_error: &io::Error) -> Repair { if path.is_dir() { return Repair::Failed(io::Error::new( @@ -357,36 +420,16 @@ fn try_repair_unopenable(path: &Path, open_error: &io::Error) -> Repair { format!("state lock path is a directory: {}", path.display()), )); } - if matches!(open_error.kind(), io::ErrorKind::ReadOnlyFilesystem) { - return Repair::Failed(io::Error::new( - open_error.kind(), - format!("could not open the state lock file: {open_error}"), - )); - } match lock_file_age(path) { + // Gone between the failed open and now, so nothing is holding it. LockFileAge::Missing => Repair::Done, - LockFileAge::Stale => match std::fs::remove_file(path) { - Ok(()) => { - tracing::warn!( - lock_path = %path.display(), - %open_error, - "removed a stale unopenable leftover state lock file" - ); - Repair::Done - } - Err(error) if error.kind() == io::ErrorKind::NotFound => Repair::Done, - Err(error) => Repair::Failed(io::Error::new( - error.kind(), - format!( - "could not repair an unopenable state lock file ({}): {error}", - path.display() - ), - )), - }, - LockFileAge::Fresh => Repair::Wait, - LockFileAge::Unknown(error) => Repair::Failed(io::Error::new( - error.kind(), - format!("could not tell whether the unopenable state lock file is stale: {error}"), + _ => Repair::Failed(io::Error::new( + open_error.kind(), + format!( + "could not open the state lock file ({}): {open_error}. \ + Remove it if no other Ceiling process is running.", + path.display() + ), )), } } @@ -433,9 +476,15 @@ fn classify_open_failure(error: &io::Error) -> LockAttempt { /// Classify a `flock` failure on the opened lock file. /// /// `WouldBlock` is the only answer that means another live holder has the lock. -/// A signal is worth another attempt. `ENOTSUP` / `ENOLCK` mean this filesystem -/// cannot flock; exclusive-create is the fallback. Every other errno is unknown +/// A signal is worth another attempt. `ENOTSUP` means this filesystem cannot +/// flock at all; exclusive-create is the fallback. Every other errno is unknown /// and fails the write (SBS-947). +/// +/// `ENOLCK` is deliberately not a fallback trigger. It also means the kernel +/// lock table is full or `lockd` failed for this one call, and in that case +/// another process can still hold a real flock on the lock file. Falling back +/// would serialize on the sibling instead, which flock holders never take, so +/// both writers would proceed and last-write-wins the credential files. #[cfg(not(windows))] fn classify_lock_failure(error: std::fs::TryLockError) -> LockAttempt { match error { @@ -458,23 +507,27 @@ fn classify_lock_failure(error: std::fs::TryLockError) -> LockAttempt { #[cfg(not(windows))] fn is_flock_unsupported(error: &io::Error) -> bool { - if error.kind() == io::ErrorKind::Unsupported { - return true; - } - #[cfg(unix)] - { - error.raw_os_error() == Some(libc::ENOLCK) - } - #[cfg(not(unix))] - { - false - } + error.kind() == io::ErrorKind::Unsupported } impl Drop for StateWriteLock { fn drop(&mut self) { if let Some(path) = self.exclusive_path.take() { - let _ = std::fs::remove_file(&path); + // Unlink the sibling only while it is still the file this holder + // created. If a stale takeover replaced it, the path now belongs to + // another live writer and removing it would hand the lock to a + // third (SBS-947). + #[cfg(unix)] + let ours = match (self.exclusive_id.take(), path_identity(&path)) { + (Some(created), Some(current)) => created == current, + // No identity to compare: a plain unlink is the old behaviour. + _ => true, + }; + #[cfg(not(unix))] + let ours = true; + if ours { + let _ = std::fs::remove_file(&path); + } } #[cfg(windows)] if let Some(handle) = self.handle.take() { @@ -1768,11 +1821,12 @@ mod tests { } fn make_stale(path: &Path) { + set_age(path, STATE_LOCK_STALE + std::time::Duration::from_secs(1)); + } + + fn set_age(path: &Path, age: std::time::Duration) { let file = std::fs::OpenOptions::new().write(true).open(path).unwrap(); - file.set_modified( - std::time::SystemTime::now() - STATE_LOCK_TIMEOUT - std::time::Duration::from_secs(1), - ) - .unwrap(); + file.set_modified(std::time::SystemTime::now() - age).unwrap(); } #[cfg(unix)] @@ -2020,8 +2074,16 @@ mod tests { match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from_raw_os_error( libc::ENOLCK, ))) { - LockAttempt::FlockUnsupported(_) => {} - _ => panic!("ENOLCK must be flock-unsupported"), + LockAttempt::Failed(error) => assert!( + error.to_string().contains("could not lock"), + "ENOLCK must fail the write, got {error}" + ), + LockAttempt::FlockUnsupported(_) => panic!( + "ENOLCK also means the lock table is full or lockd failed for this call, while \ + another process still holds a real flock. Falling back to the sibling would let \ + both writers run." + ), + _ => panic!("ENOLCK must fail closed"), } } @@ -2070,15 +2132,19 @@ mod tests { ); } + /// Pins SBS-947: unlinking an unopenable lock file used to "repair" it, but + /// a live holder still has that inode open, so the replacement put two + /// writers on two different inodes. #[cfg(unix)] #[test] - fn a_stale_unopenable_leftover_lock_file_is_repaired() { + fn an_unopenable_lock_file_fails_closed_instead_of_being_unlinked() { use std::os::unix::fs::PermissionsExt; let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("state-write.lock"); std::fs::write(&path, b"leftover from a privileged run").unwrap(); make_stale(&path); + let before = path_identity(&path).expect("the leftover must exist"); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); if std::fs::OpenOptions::new() .read(true) @@ -2092,24 +2158,113 @@ mod tests { let started = std::time::Instant::now(); let mut ran = false; - with_state_write_lock_at(&path, || { + let error = with_state_write_lock_at(&path, || { ran = true; Ok(()) }) - .expect("a stale leftover this user cannot open must be repaired, not skipped"); + .expect_err("an unopenable lock file must fail the write, not be unlinked"); - assert!(ran); + assert!(!ran, "the write must not run without a lock"); + let message = error.to_string(); + assert!( + message.contains(&path.display().to_string()), + "the error must name the lock file so the user can remove it, got {message}" + ); + assert_eq!( + path_identity(&path), + Some(before), + "the lock file must not be unlinked and recreated under a live holder" + ); assert!( started.elapsed() < std::time::Duration::from_secs(1), - "a stale leftover must be repaired without waiting out the acquire timeout" + "an unopenable lock file must fail fast, not be masked by the acquire timeout" + ); + } + + /// Pins SBS-947: a holder that is merely slow is not a crash leftover. The + /// stale threshold is longer than the acquire timeout precisely so a waiter + /// can never outlast a live holder and steal the sibling. + #[test] + fn an_exclusive_create_sibling_is_not_stolen_before_the_stale_threshold() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + let exclusive = exclusive_lock_path(&path); + + let held = StateWriteLock::acquire_with(&path, |_| { + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) + }) + .expect("the first holder must take the sibling"); + + // Older than any waiter's acquire timeout, but still short of the + // crash-recovery threshold. + set_age(&exclusive, STATE_LOCK_TIMEOUT + std::time::Duration::from_secs(5)); + + assert!( + matches!( + StateWriteLock::try_exclusive_create(&path), + LockAttempt::Contended + ), + "a holder older than the acquire timeout must still read as live" + ); + assert!( + exclusive.exists(), + "a live holder's sibling must not be removed" + ); + + drop(held); + assert!( + matches!( + StateWriteLock::try_exclusive_create(&path), + LockAttempt::Acquired(_) + ), + "the sibling must be free once the holder drops" + ); + } + + /// Pins SBS-947: after a stale takeover replaced the sibling, the original + /// holder's drop must not delete the new holder's file. + #[cfg(unix)] + #[test] + fn dropping_a_holder_does_not_unlink_a_sibling_it_did_not_create() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + let exclusive = exclusive_lock_path(&path); + + let first = StateWriteLock::acquire_with(&path, |_| { + LockAttempt::FlockUnsupported(io::Error::from(io::ErrorKind::Unsupported)) + }) + .expect("the first holder must take the sibling"); + + // Stand in for a takeover: the sibling at this path is now a different + // file belonging to somebody else. + std::fs::remove_file(&exclusive).unwrap(); + std::fs::write(&exclusive, b"a later holder's sibling").unwrap(); + let replacement = path_identity(&exclusive).unwrap(); + + drop(first); + + assert_eq!( + path_identity(&exclusive), + Some(replacement), + "the first holder must not unlink a sibling it did not create" ); + } + + /// Pins SBS-947: a lock file stamped in the future has no measurable age. + /// Treating that as stale would hand a live holder's lock away on nothing + /// worse than NFS clock skew. + #[test] + fn a_lock_file_stamped_in_the_future_is_not_stale() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state-write.lock"); + std::fs::write(&path, b"held").unwrap(); + let file = std::fs::OpenOptions::new().write(true).open(&path).unwrap(); + file.set_modified(std::time::SystemTime::now() + std::time::Duration::from_secs(3600)) + .unwrap(); + assert!( - std::fs::OpenOptions::new() - .read(true) - .write(true) - .open(&path) - .is_ok(), - "repair must leave an openable lock file for the next writer" + matches!(lock_file_age(&path), LockFileAge::Fresh), + "a future mtime must read as a live holder, not as a stale leftover" ); } } From cfc2061527d919e082adb797c9235603d1704a7f Mon Sep 17 00:00:00 2001 From: Tyler South Date: Sat, 22 Aug 2026 07:42:16 -0400 Subject: [PATCH 3/4] Format the new lock tests Co-Authored-By: Claude Opus 5 (1M context) --- rust/src/secure_file.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index 26ac4252..fd686442 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -1826,7 +1826,8 @@ mod tests { fn set_age(path: &Path, age: std::time::Duration) { let file = std::fs::OpenOptions::new().write(true).open(path).unwrap(); - file.set_modified(std::time::SystemTime::now() - age).unwrap(); + file.set_modified(std::time::SystemTime::now() - age) + .unwrap(); } #[cfg(unix)] @@ -2197,7 +2198,10 @@ mod tests { // Older than any waiter's acquire timeout, but still short of the // crash-recovery threshold. - set_age(&exclusive, STATE_LOCK_TIMEOUT + std::time::Duration::from_secs(5)); + set_age( + &exclusive, + STATE_LOCK_TIMEOUT + std::time::Duration::from_secs(5), + ); assert!( matches!( From 07d2e54a32eb13c539b883b27272a88f881fd8b5 Mon Sep 17 00:00:00 2001 From: Tyler South Date: Sat, 22 Aug 2026 08:31:18 -0400 Subject: [PATCH 4/4] Address the follow-up review on the lock fallback - Read ENOTSUP/EOPNOTSUPP from the errno instead of trusting kind(). They are one value on Linux but distinct on macOS and the BSDs, where ENOTSUP decoded as Uncategorized until a recent std change, so which toolchain built this decided whether a mount got the fallback. - Never unlink the sibling when its identity cannot be confirmed. A miss meant a takeover had already replaced the file, and the old fallback deleted the replacement. - Route the post-repair retry through the loop tail so a lock file that keeps appearing and vanishing still times out. - Correct the module doc: an unopenable lock file now fails the write rather than being repaired when stale. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- rust/src/secure_file.rs | 50 ++++++++++++++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6938728e..abea1080 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ ### Fixed - **A corrupted Claude de-duplication key no longer silently inflates Charts and Estimated API value.** The usage index treated invalid UTF-8 in a present `dedup_key` as "this record has no key", so the same transcript event could be counted twice if its counterpart still had one. Invalid UTF-8 now rejects the whole index the way a required string already does, and the next scan rebuilds it from the transcripts. A key that was never written is still encoded as the length sentinel, not as a failed decode. -- **A filesystem that cannot flock no longer writes settings unserialized.** The state-write lock treated every `flock` errno other than contention as "no lock needed" and continued, so an NFS/FUSE/SMB home without `lockd` let the tray and the CLI replace `api_keys.json` over each other. Only `ENOTSUP` — the filesystem saying it does not implement locking at all — now falls back, and the fallback is an exclusive-create sibling that a second writer cannot take. `ENOLCK` is no longer treated as a broken filesystem: it also means the kernel lock table is full or `lockd` failed for that one call, while another process still holds a real flock, so falling back to the sibling would have let both writers through. It fails the write instead. An unknown errno already did. Because the sibling has no kernel-backed release, a leftover from a killed process still has to age out, but the age it has to reach is now two minutes rather than the ten seconds an acquirer waits — a holder doing slow work can no longer be outlasted and have its lock taken, and a lock file stamped in the future by a skewed server clock reads as held rather than as expired. A holder unlinks the sibling on release only while it is still the same file it created, so a takeover cannot cascade into deleting a third writer's lock. A `state-write.lock` the current user cannot open — left by a `sudo` run, or by another account — now fails the write and names the path to remove. It used to be unlinked and recreated, which put the running holder and the new one on two different inodes and let both writes proceed. A directory in the lock path still fails the write. +- **A filesystem that cannot flock no longer writes settings unserialized.** The state-write lock treated every `flock` errno other than contention as "no lock needed" and continued, so an NFS/FUSE/SMB home without `lockd` let the tray and the CLI replace `api_keys.json` over each other. Only `ENOTSUP` — the filesystem saying it does not implement locking at all — now falls back, read from the errno rather than from how a given Rust version happens to decode it, and the fallback is an exclusive-create sibling that a second writer cannot take. `ENOLCK` is no longer treated as a broken filesystem: it also means the kernel lock table is full or `lockd` failed for that one call, while another process still holds a real flock, so falling back to the sibling would have let both writers through. It fails the write instead. An unknown errno already did. Because the sibling has no kernel-backed release, a leftover from a killed process still has to age out, but the age it has to reach is now two minutes rather than the ten seconds an acquirer waits — a holder doing slow work can no longer be outlasted and have its lock taken, and a lock file stamped in the future by a skewed server clock reads as held rather than as expired. A holder unlinks the sibling on release only while it is still the same file it created, so a takeover cannot cascade into deleting a third writer's lock. A `state-write.lock` the current user cannot open — left by a `sudo` run, or by another account — now fails the write and names the path to remove. It used to be unlinked and recreated, which put the running holder and the new one on two different inodes and let both writes proceed. A directory in the lock path still fails the write. - **Windows stops handing an npm shim or a directory to the PTY as the Codex or Claude binary.** `where.exe` prints every match on PATH and the resolver took the first line, so an extensionless POSIX shim named `codex` sitting ahead of `codex.exe` went straight to `CreateProcessW` and failed with error 193. Every candidate is now examined, a native `.exe` or `.com` wins over a `.cmd` or `.bat` shim wherever the shim sits in the list, and directories and extensionless files are refused — for PATH results, for the `CODEX_BINARY` and `CLAUDE_BINARY` overrides, and for an explicit path. A `.cmd` or `.bat` shim now runs through `%COMSPEC% /d /c` instead of being launched directly, and its arguments are escaped for the interpreter rather than for `CreateProcessW`, because the two do not agree on `&`, `|`, or `>`. The few tails `cmd.exe` cannot express at all fail with a message rather than launch something other than what was asked for. Closes #270. Thanks @ITSMERNB! ## [Ceiling] 1.5.34 - 2026-08-18 diff --git a/rust/src/secure_file.rs b/rust/src/secure_file.rs index fd686442..d6f99285 100644 --- a/rust/src/secure_file.rs +++ b/rust/src/secure_file.rs @@ -35,8 +35,10 @@ const STATE_LOCK_STALE: std::time::Duration = std::time::Duration::from_secs(120 /// `flock` is preferred because the kernel drops it if the process dies. When /// the mount cannot flock (NFS without lockd, some FUSE/SMB), writers serialize /// with an exclusive-create sibling and a staleness timeout. A lock file this -/// user cannot open is repaired when it is stale. Unknown lock errors fail the -/// write; they are not treated as "no lock needed" (SBS-947). +/// user cannot open fails the write and names the path: a live holder still has +/// that inode open, so unlinking it would put two writers on two inodes. +/// Unknown lock errors fail the write too; they are not treated as "no lock +/// needed" (SBS-947). pub fn with_state_write_lock(operation: impl FnOnce() -> io::Result) -> io::Result { let lock_path = dirs::config_dir() .unwrap_or_else(|| PathBuf::from(".")) @@ -183,8 +185,11 @@ impl StateWriteLock { } } } + // Falls through to the deadline check and the sleep rather + // than retrying straight away: a lock file that keeps + // appearing and vanishing must still time out. LockAttempt::Unopenable(error) => match try_repair_unopenable(path, &error) { - Repair::Done => continue, + Repair::Done => {} Repair::Failed(repair) => return Err(repair), }, LockAttempt::Contended => {} @@ -507,7 +512,22 @@ fn classify_lock_failure(error: std::fs::TryLockError) -> LockAttempt { #[cfg(not(windows))] fn is_flock_unsupported(error: &io::Error) -> bool { - error.kind() == io::ErrorKind::Unsupported + if error.kind() == io::ErrorKind::Unsupported { + return true; + } + // `ENOTSUP` and `EOPNOTSUPP` are one value on Linux but distinct on macOS + // and the BSDs, where `ENOTSUP` decoded as `Uncategorized` until a recent + // std change. Read the errno directly so which toolchain built this does + // not decide whether a mount gets the fallback. + #[cfg(unix)] + { + let raw = error.raw_os_error(); + raw == Some(libc::ENOTSUP) || raw == Some(libc::EOPNOTSUPP) + } + #[cfg(not(unix))] + { + false + } } impl Drop for StateWriteLock { @@ -518,10 +538,14 @@ impl Drop for StateWriteLock { // another live writer and removing it would hand the lock to a // third (SBS-947). #[cfg(unix)] - let ours = match (self.exclusive_id.take(), path_identity(&path)) { - (Some(created), Some(current)) => created == current, - // No identity to compare: a plain unlink is the old behaviour. - _ => true, + let ours = match self.exclusive_id.take() { + // Anything other than the exact file this holder created is + // somebody else's lock, including a path that no longer + // resolves: a takeover may have removed the original and be + // about to create its own. + Some(created) => path_identity(&path) == Some(created), + // Nothing was recorded, so there is nothing to compare against. + None => true, }; #[cfg(not(unix))] let ours = true; @@ -2072,6 +2096,16 @@ mod tests { _ => panic!("ENOTSUP must be flock-unsupported, not a silent degrade or a hard fail"), } + // On macOS and the BSDs this errno is distinct from EOPNOTSUPP and + // older toolchains decoded it as Uncategorized, so the classification + // must not rest on `kind()` alone. + match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from_raw_os_error( + libc::ENOTSUP, + ))) { + LockAttempt::FlockUnsupported(_) => {} + _ => panic!("ENOTSUP must be flock-unsupported whatever std decodes it as"), + } + match classify_lock_failure(std::fs::TryLockError::Error(io::Error::from_raw_os_error( libc::ENOLCK, ))) {