diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index 33dcc7a72..0bf58ade3 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -467,6 +467,7 @@ impl AuthProfilesStore { } let mut waited = 0_u64; + let mut cleared_stale = false; loop { match OpenOptions::new() .create_new(true) @@ -474,12 +475,40 @@ impl AuthProfilesStore { .open(&self.lock_path) { Ok(mut file) => { - let _ = writeln!(file, "pid={}", std::process::id()); + // Issue #1612 — writing the pid line is what later lets + // a future acquirer recognise a crashed owner; if the + // write fails we must NOT report the lock as held with + // a malformed/empty file behind us, or stale recovery + // would silently degrade to the full 10s timeout for + // every subsequent acquire. + if let Err(e) = writeln!(file, "pid={}", std::process::id()) { + let _ = fs::remove_file(&self.lock_path); + return Err(e).with_context(|| { + "Failed to write auth profile lock owner".to_string() + }); + } return Ok(AuthProfileLockGuard { lock_path: self.lock_path.clone(), }); } Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // Issue #1612 — a previous openhuman crash can leave a + // stale auth-profiles.lock behind, after which every RPC + // path that touches the auth profile store fails for the + // 10s LOCK_TIMEOUT_MS window and the user gets stuck in a + // retry storm. Before falling back to the busy-wait, try + // once to peek at the writer's recorded PID and remove + // the lock if that process is no longer alive. Flag is + // flipped on the first probe (not only on success) so a + // live-pid / malformed / unreadable lock doesn't trigger + // a fresh sysinfo probe + log line on every busy-wait + // iteration. + if !cleared_stale { + cleared_stale = true; + if self.clear_lock_if_stale() { + continue; + } + } if waited >= LOCK_TIMEOUT_MS { anyhow::bail!("Timed out waiting for auth profile lock"); } @@ -493,6 +522,81 @@ impl AuthProfilesStore { } } } + + /// Returns `true` if an existing lock file was detected as stale (its + /// recorded PID is no longer running) and successfully removed. + /// Malformed locks (no `pid=` line) and locks whose PID is still alive + /// are left in place so the caller falls back to the normal busy-wait + /// and timeout path. + fn clear_lock_if_stale(&self) -> bool { + let content = match fs::read_to_string(&self.lock_path) { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return false, + Err(e) => { + tracing::warn!( + target: "auth-profiles", + "[credentials] failed to read lock file at {} for stale check: {e}", + self.lock_path.display() + ); + return false; + } + }; + + let pid = content + .lines() + .find_map(|line| line.trim().strip_prefix("pid=")?.trim().parse::().ok()); + + let Some(pid) = pid else { + tracing::warn!( + target: "auth-profiles", + "[credentials] lock at {} has no parseable pid line; leaving in place", + self.lock_path.display() + ); + return false; + }; + + if is_pid_alive(pid) { + return false; + } + + match fs::remove_file(&self.lock_path) { + Ok(()) => { + tracing::info!( + target: "auth-profiles", + "[credentials] removed stale auth profile lock at {} (pid {pid} not alive)", + self.lock_path.display() + ); + true + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => true, + Err(e) => { + tracing::warn!( + target: "auth-profiles", + "[credentials] failed to remove stale lock at {}: {e}", + self.lock_path.display() + ); + false + } + } + } +} + +/// Cross-platform best-effort check that a given OS process id is currently +/// running. Used by [`AuthProfilesStore::clear_lock_if_stale`] to decide +/// whether a recorded lock owner is still alive; a false negative just +/// means we keep waiting on a lock that was actually already gone, which +/// is the safe direction. Backed by sysinfo so we don't grow a new libc / +/// windows-sys dependency for one syscall. +fn is_pid_alive(pid: u32) -> bool { + use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, System}; + let target = Pid::from_u32(pid); + let mut sys = System::new(); + sys.refresh_processes_specifics( + ProcessesToUpdate::Some(&[target]), + true, + ProcessRefreshKind::nothing(), + ); + sys.process(target).is_some() } struct AuthProfileLockGuard { diff --git a/src/openhuman/credentials/profiles_tests.rs b/src/openhuman/credentials/profiles_tests.rs index dd72691d0..b522d6085 100644 --- a/src/openhuman/credentials/profiles_tests.rs +++ b/src/openhuman/credentials/profiles_tests.rs @@ -273,6 +273,113 @@ fn upsert_preserves_created_at_on_update() { assert_eq!(loaded.created_at, created); } +// --- Issue #1612: stale auth-profiles.lock recovery ----------------------- + +/// A pid we expect to be safely above any real process id on macOS / Linux / +/// Windows test runners. Used to simulate a lock file written by a process +/// that has since exited. +const SYNTHETIC_DEAD_PID: u32 = i32::MAX as u32; + +#[test] +fn is_pid_alive_detects_current_process() { + assert!(is_pid_alive(std::process::id())); +} + +#[test] +fn is_pid_alive_returns_false_for_synthetic_dead_pid() { + assert!(!is_pid_alive(SYNTHETIC_DEAD_PID)); +} + +#[test] +fn acquire_lock_clears_stale_lock_with_dead_pid() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, format!("pid={SYNTHETIC_DEAD_PID}\n")).unwrap(); + assert!(lock_path.exists()); + + // A no-op call that goes through acquire_lock should succeed quickly + // by recognising the previous lock as stale and removing it. + let data = store.load().unwrap(); + assert!(data.profiles.is_empty()); + assert!( + !lock_path.exists(), + "guard should have removed the lock on drop" + ); +} + +#[test] +fn acquire_lock_recovers_after_upsert_when_dead_pid_lock_left_behind() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // Pre-existing lock from a crashed previous run. + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, format!("pid={SYNTHETIC_DEAD_PID}\n")).unwrap(); + + let profile = AuthProfile::new_token("openai", "default", "tok".into()); + let id = profile.id.clone(); + store.upsert_profile(profile, true).unwrap(); + + let data = store.load().unwrap(); + assert!(data.profiles.contains_key(&id)); + assert!(!lock_path.exists()); +} + +#[test] +fn clear_lock_if_stale_leaves_live_pid_alone() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, format!("pid={}\n", std::process::id())).unwrap(); + + assert!(!store.clear_lock_if_stale()); + assert!(lock_path.exists(), "lock for live pid must not be removed"); +} + +#[test] +fn clear_lock_if_stale_leaves_malformed_lock_alone() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, "garbage without a pid line\n").unwrap(); + + assert!(!store.clear_lock_if_stale()); + assert!( + lock_path.exists(), + "malformed lock should not be auto-removed; fall back to busy-wait + timeout" + ); +} + +#[test] +fn clear_lock_if_stale_is_noop_when_lock_missing() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + assert!(!store.clear_lock_if_stale()); +} + +#[test] +fn acquire_lock_writes_pid_so_future_callers_can_recover() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // Drive a real acquire/release cycle and snapshot the on-disk lock + // while the guard is held. + let lock_path = tmp.path().join(LOCK_FILENAME); + let observed = { + let _guard = store.acquire_lock().unwrap(); + std::fs::read_to_string(&lock_path).unwrap() + }; + assert!( + observed.contains(&format!("pid={}", std::process::id())), + "lock file should embed the owning pid, got {observed:?}" + ); + assert!(!lock_path.exists(), "guard must remove lock on drop"); +} + #[test] fn auth_profile_kind_serde_roundtrip() { let json = serde_json::to_string(&AuthProfileKind::OAuth).unwrap();