fix(credentials): recover from stale auth-profiles.lock (#1636)

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::<u32>().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 {

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();