From 5bce4f6140ca3b0f69ca733ec6f99c4ff648233d Mon Sep 17 00:00:00 2001 From: Mega Mind <146339422+M3gA-Mind@users.noreply.github.com> Date: Wed, 24 Jun 2026 22:53:14 +0530 Subject: [PATCH] fix(credentials): reclaim leaked self-owned auth-profile lock immediately (#2318) (#4064) --- src/openhuman/credentials/profiles.rs | 203 ++++++++++++++++++-- src/openhuman/credentials/profiles_tests.rs | 101 ++++++++++ 2 files changed, 288 insertions(+), 16 deletions(-) diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index 19cba972f..42938606f 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -3,10 +3,11 @@ use crate::openhuman::util::retry_with_backoff; use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::fs::{self, OpenOptions}; use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::{Mutex, MutexGuard, OnceLock, TryLockError}; use std::thread; use std::time::{Duration, Instant}; @@ -1189,6 +1190,36 @@ impl AuthProfilesStore { // both `LOCK_TIMEOUT_MS` and `next_stale_recheck_ms` significantly // later than intended. let started_at = Instant::now(); + + // Serialize same-process acquirers on an in-memory lock keyed by this + // path before we ever touch the on-disk lock file. Two things depend on + // holding it: (1) concurrent `app_state_snapshot` calls in this process + // queue here instead of racing `create_new`/`Drop` on the file, and + // (2) it lets us treat an on-disk lock recording our own pid as a leaked + // `Drop` unlink and reclaim it immediately — no other thread in this + // process can hold a live guard while we own this in-memory lock (see + // `reclaim_self_owned_lock`). Held for the lifetime of the returned + // guard. + // + // Use `try_lock` against the *same* `LOCK_TIMEOUT_MS` budget as the + // on-disk wait rather than a blocking `lock()`: a wedged in-process + // holder must not be able to strand an RPC/blocking worker past the + // timeout the caller (e.g. `app_state_snapshot`) expects. Poison is + // recoverable — the `()` payload carries no invariant. + let in_process_lock = in_process_lock_for(&self.lock_path); + let in_process_guard = loop { + match in_process_lock.try_lock() { + Ok(guard) => break guard, + Err(TryLockError::Poisoned(poisoned)) => break poisoned.into_inner(), + Err(TryLockError::WouldBlock) => { + if started_at.elapsed().as_millis() as u64 >= LOCK_TIMEOUT_MS { + anyhow::bail!("Timed out waiting for auth profile lock"); + } + thread::sleep(Duration::from_millis(LOCK_WAIT_MS)); + } + } + }; + let mut cleared_stale = false; // Periodically re-probe for stale locks during the busy-wait. A // lock that started fresh (live pid, recent mtime) can age past @@ -1225,6 +1256,7 @@ impl AuthProfilesStore { } return Ok(AuthProfileLockGuard { lock_path: self.lock_path.clone(), + _in_process: in_process_guard, }); } Err(e) => { @@ -1234,9 +1266,23 @@ impl AuthProfilesStore { .map_or(false, |ioe| ioe.kind() == std::io::ErrorKind::AlreadyExists); if is_already_exists { + // A lock file recording our own pid can only be a + // leaked `Drop` unlink: we hold the in-process lock, so + // no live same-process guard exists. Reclaim it + // immediately rather than spinning the 30s age floor + // (`STALE_LOCK_AGE_MS`) that sits *behind* the ~10s RPC + // timeout — that gap is what produced the sustained + // "Timed out waiting for auth profile lock" retry storm + // (Sentry TAURI-RUST-B1 / #2318). Cheap enough (one tiny + // read) to re-probe every spin, which also self-heals a + // lock our own `Drop` leaks mid-wait. + if self.reclaim_self_owned_lock() { + continue; + } // 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 + // stale auth-profiles.lock behind (a *different*, now-dead + // pid, or an aged leak), after which every RPC path that + // touches the auth profile store fails for the // `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 @@ -1292,13 +1338,14 @@ impl AuthProfilesStore { /// 1. The recorded `pid=` line points at a process that is no longer /// running — classic crashed-owner recovery (Issue #1612). /// 2. The lock file's mtime is older than [`STALE_LOCK_AGE_MS`]. This - /// catches the Windows case where the previous owner's - /// `AuthProfileLockGuard::drop` could not unlink the file (AV / - /// indexer briefly held a handle) and orphaned the lock with its - /// still-alive pid inside — every subsequent acquirer would - /// otherwise spin the full `LOCK_TIMEOUT_MS` and bail. No - /// legitimate auth-profile op holds the lock long enough to be - /// affected, so a too-old lock is unambiguously a leak. + /// catches a *different* still-alive process that leaked its lock (its + /// `AuthProfileLockGuard::drop` could not unlink the file — e.g. Windows + /// AV / indexer briefly held a handle — and orphaned it with that live + /// pid inside). No legitimate auth-profile op holds the lock long enough + /// to be affected, so a too-old lock is unambiguously a leak. A lock + /// leaked by *this* process is handled far sooner — immediately, without + /// the age floor — by [`reclaim_self_owned_lock`](Self::reclaim_self_owned_lock), + /// which is sound because acquirers serialize on the in-process lock. /// /// 3. The lock file has no parseable `pid=` line and is older than /// [`MALFORMED_LOCK_GRACE_MS`]. A healthy holder writes its pid within @@ -1399,6 +1446,84 @@ impl AuthProfilesStore { } } } + + /// Remove the on-disk lock file **iff** it records this process's own pid. + /// Returns `true` when a self-owned lock was found and removed. + /// + /// MUST only be called while holding this path's in-process lock (acquired + /// at the top of [`acquire_lock`](Self::acquire_lock) via + /// [`in_process_lock_for`]). That invariant is what makes the removal safe: + /// same-process acquirers serialize on the in-process lock, so no other + /// thread in this process can be holding a live `AuthProfileLockGuard` while + /// we run. A lock file carrying our own pid is therefore necessarily a leak + /// — a previous guard's `Drop` could not unlink it (on Windows, AV / the + /// Search indexer briefly hold a handle on the just-written file) and + /// orphaned it with our still-alive pid inside. + /// + /// This is the fast self-heal for Sentry TAURI-RUST-B1 (#2318): without it + /// the only recovery for a leaked-but-live-pid lock is the age branch in + /// [`clear_lock_if_stale`](Self::clear_lock_if_stale), whose + /// [`STALE_LOCK_AGE_MS`] floor (30s) is far longer than the ~10s RPC + /// timeout, so every `app_state_snapshot` timed out and retry-stormed for + /// the whole 30s window before age-reclaim kicked in. + fn reclaim_self_owned_lock(&self) -> bool { + let me = std::process::id(); + tracing::trace!( + target: "auth-profiles", + "[credentials] probing for self-owned leaked lock at {} (our pid {me})", + self.lock_path.display() + ); + let content = match fs::read_to_string(&self.lock_path) { + Ok(s) => s, + // NotFound: already gone (raced with another reclaim / the guard's + // own Drop). Any other read error: fall back to the age-based and + // busy-wait paths rather than guess at ownership. + Err(e) => { + tracing::debug!( + target: "auth-profiles", + "[credentials] self-owned probe could not read lock at {} ({e}); \ + deferring to stale/busy-wait recovery", + self.lock_path.display() + ); + return false; + } + }; + + let pid = content + .lines() + .find_map(|line| line.trim().strip_prefix("pid=")?.trim().parse::().ok()); + if pid != Some(me) { + tracing::trace!( + target: "auth-profiles", + "[credentials] lock at {} is not self-owned (recorded {pid:?}, our pid {me}); \ + deferring to stale/busy-wait recovery", + self.lock_path.display() + ); + return false; + } + + match fs::remove_file(&self.lock_path) { + Ok(()) => { + tracing::info!( + target: "auth-profiles", + "[credentials] reclaimed leaked self-owned auth profile lock at {} \ + (recorded our own live pid {me}; a prior guard's Drop unlink was blocked, \ + most likely by Windows AV/indexer)", + 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 reclaim self-owned auth profile lock at {}: {e}", + self.lock_path.display() + ); + false + } + } + } } /// Cross-platform best-effort check that a given OS process id is currently @@ -1455,8 +1580,51 @@ fn is_pid_alive(pid: u32) -> bool { sys.process(target).is_some() } +/// Process-wide registry of in-memory locks, one per auth-profile lock-file +/// path. Returns a `'static` handle so the held guard can live inside +/// [`AuthProfileLockGuard`] for the lifetime of the on-disk lock. +/// +/// Entries are created on first use and never removed — the set of distinct +/// auth-profile lock paths in a process is tiny (effectively one), so leaking +/// a `Mutex<()>` per path is negligible and buys us a `'static` lifetime +/// without `unsafe` or a self-referential guard. Serializing same-process +/// acquirers here is what lets [`AuthProfilesStore::reclaim_self_owned_lock`] +/// treat an on-disk lock carrying our own pid as a leak (no live same-process +/// guard can exist while a caller holds this lock). +/// +/// The registry key is **canonicalized** (real parent directory + lock +/// filename), so two `AuthProfilesStore`s pointing at the same lock through +/// aliased spellings (`state` vs `state/.`, relative vs absolute, a symlinked +/// parent, Windows case variants) share one mutex. Without this, aliased paths +/// would take *different* mutexes and a second same-process acquirer could see +/// the first live guard's pid, mistake it for a leak, and reclaim it — entering +/// the critical section concurrently. `acquire_lock` always calls this *after* +/// `create_dir_all(parent)`, so the parent exists and `canonicalize` succeeds; +/// if it ever fails we fall back to the raw path (no worse than before). +fn in_process_lock_for(path: &Path) -> &'static Mutex<()> { + static REGISTRY: OnceLock>>> = OnceLock::new(); + let key = path + .parent() + .and_then(|parent| { + let canonical = fs::canonicalize(parent).ok()?; + path.file_name().map(|name| canonical.join(name)) + }) + .unwrap_or_else(|| path.to_path_buf()); + let registry = REGISTRY.get_or_init(|| Mutex::new(HashMap::new())); + let mut map = registry + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + // Deref the `&mut &'static Mutex<()>` to copy the `'static` reference out, + // so the returned handle is not tied to the dropped `map` guard. + *map.entry(key) + .or_insert_with(|| &*Box::leak(Box::new(Mutex::new(())))) +} + struct AuthProfileLockGuard { lock_path: PathBuf, + /// Held to serialize same-process acquirers for `lock_path`; released when + /// this guard drops. Never read — see [`in_process_lock_for`]. + _in_process: MutexGuard<'static, ()>, } impl Drop for AuthProfileLockGuard { @@ -1465,11 +1633,13 @@ impl Drop for AuthProfileLockGuard { // search indexer routinely hold a transient handle on a file just // after it is written, which makes `fs::remove_file` fail with // `PermissionDenied`. A failed unlink here leaks the lock file - // with the still-alive owner pid inside, which would cause every - // subsequent acquirer to spin the full `LOCK_TIMEOUT_MS` and bail - // with "Timed out waiting for auth profile lock". The age-based - // reclaim in `clear_lock_if_stale` is the safety net; this retry - // loop is the first line of defence so we don't rely on it. + // with the still-alive owner pid inside. Recovery is layered: this + // retry loop is the first line of defence; if it still fails, the + // next acquirer in THIS process reclaims the self-owned lock + // immediately via `reclaim_self_owned_lock` (it holds the in-process + // lock, so the recorded pid being ours unambiguously means a leak); + // and the age-based reclaim in `clear_lock_if_stale` remains the + // cross-process safety net. for attempt in 0..5u32 { match fs::remove_file(&self.lock_path) { Ok(()) => return, @@ -1479,7 +1649,8 @@ impl Drop for AuthProfileLockGuard { tracing::warn!( target: "auth-profiles", "[credentials] failed to remove auth profile lock at {} after {} attempts: {e}. \ - The age-based stale-lock reclaim will recover within {}ms.", + The next acquirer in this process will reclaim the self-owned lock immediately; \ + a leak from another process is recovered by the age-based reclaim within {}ms.", self.lock_path.display(), attempt + 1, STALE_LOCK_AGE_MS, diff --git a/src/openhuman/credentials/profiles_tests.rs b/src/openhuman/credentials/profiles_tests.rs index 3a4de0964..0aaf71cc7 100644 --- a/src/openhuman/credentials/profiles_tests.rs +++ b/src/openhuman/credentials/profiles_tests.rs @@ -489,6 +489,107 @@ fn clear_lock_if_stale_leaves_live_pid_alone() { assert!(lock_path.exists(), "lock for live pid must not be removed"); } +// --- #2318 (TAURI-RUST-B1): immediate self-owned leaked-lock recovery ------ + +#[test] +fn reclaim_self_owned_lock_removes_leaked_self_pid_lock() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // Simulate a lock our own process leaked because a prior guard's Drop + // unlink was blocked (Windows AV/indexer): the file carries our live pid. + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, format!("pid={}\n", std::process::id())).unwrap(); + + assert!( + store.reclaim_self_owned_lock(), + "a lock recording our own pid is a leaked Drop unlink and must be reclaimed" + ); + assert!(!lock_path.exists()); +} + +#[test] +fn reclaim_self_owned_lock_leaves_foreign_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={SYNTHETIC_DEAD_PID}\n")).unwrap(); + + assert!( + !store.reclaim_self_owned_lock(), + "a lock owned by another pid must not be removed by the self-owned fast path" + ); + assert!(lock_path.exists()); +} + +#[test] +fn reclaim_self_owned_lock_is_noop_when_lock_missing() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + assert!(!store.reclaim_self_owned_lock()); +} + +#[test] +fn acquire_lock_recovers_immediately_from_leaked_self_pid_lock() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // A lock leaked by this very process (live pid inside). The only previous + // recovery was the 30s age floor — far longer than the 10s RPC timeout — + // which produced the retry storm. The self-owned fast path must clear it + // well inside that window. + let lock_path = tmp.path().join(LOCK_FILENAME); + std::fs::write(&lock_path, format!("pid={}\n", std::process::id())).unwrap(); + + let started = std::time::Instant::now(); + let data = store.load().unwrap(); + let elapsed = started.elapsed(); + + assert!(data.profiles.is_empty()); + assert!( + elapsed < std::time::Duration::from_secs(5), + "self-owned leaked lock should be reclaimed immediately, not after the \ + {STALE_LOCK_AGE_MS}ms age floor (took {elapsed:?})" + ); + assert!( + !lock_path.exists(), + "guard should have removed the lock on drop" + ); +} + +#[test] +fn acquire_lock_serializes_same_process_acquirers() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + let lock_path = tmp.path().join(LOCK_FILENAME); + + // Hold the in-process lock for this path directly, forcing a concurrent + // acquirer to spin the `try_lock` WouldBlock retry loop until we release — + // it must NOT race the on-disk file or bail early. Use the same registry + // entry `acquire_lock` resolves (canonicalized parent + filename). + let held = in_process_lock_for(&lock_path) + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + let other = store.clone(); + let waiter = std::thread::spawn(move || other.load()); + + // Let the waiter enter the WouldBlock loop, then release; it should proceed. + std::thread::sleep(std::time::Duration::from_millis(150)); + drop(held); + + let result = waiter.join().expect("waiter thread panicked"); + assert!( + result.is_ok(), + "same-process acquirer should succeed once the in-process lock frees: {result:?}" + ); + assert!( + !lock_path.exists(), + "the waiter's guard should have removed the on-disk lock on drop" + ); +} + #[test] fn clear_lock_if_stale_leaves_malformed_lock_alone() { let tmp = TempDir::new().unwrap();