From a5ed0ff5df0f8c29d4b02c2ff19cec02d8a2477a Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Mon, 8 Jun 2026 18:27:09 +0530 Subject: [PATCH] fix(credentials): degrade auth-profile read to lock-free on disk-full (#3407) (#3409) --- src/core/observability.rs | 22 +++- src/openhuman/credentials/profiles.rs | 108 +++++++++++++++++- src/openhuman/credentials/profiles_tests.rs | 117 ++++++++++++++++++++ 3 files changed, 241 insertions(+), 6 deletions(-) diff --git a/src/core/observability.rs b/src/core/observability.rs index 102e74476..0b8db1daf 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -443,8 +443,22 @@ pub fn expected_error_kind(message: &str) -> Option { /// `"not enough space on the disk"`. /// - **Windows `ERROR_HANDLE_DISK_FULL` (39)**: same wire text but errno 39. /// The text anchor already covers it. +/// +/// A fourth shape comes from call sites that render the `io::ErrorKind` debug +/// name instead of the `io::Error` Display — notably the auth-profile +/// lock-create annotation, which emits +/// `"... (kind=Some(StorageFull), os_code=Some(28))"` (Sentry TAURI-RUST-4SZ). +/// That string carries no "no space left on device" text, so anchor +/// additionally on the cross-platform `StorageFull` ErrorKind token (std maps +/// ENOSPC / `ERROR_DISK_FULL` / `ERROR_HANDLE_DISK_FULL` all to +/// `ErrorKind::StorageFull`). This is defense-in-depth for the genuinely +/// unpreventable **write** paths (a write can't succeed on a full disk); the +/// read path no longer emits this error at all (it degrades to a lock-free +/// read — see `AuthProfilesStore::load`). fn is_disk_full_message(lower: &str) -> bool { - lower.contains("no space left on device") || lower.contains("not enough space on the disk") + lower.contains("no space left on device") + || lower.contains("not enough space on the disk") + || lower.contains("storagefull") } /// Detect the literal `"Config loading timed out"` string produced by @@ -2643,6 +2657,12 @@ mod tests { "state snapshot write failed: No space left on device (os error 28)", // Windows ERROR_DISK_FULL (112) rendering. "log rotation failed: There is not enough space on the disk. (os error 112)", + // Outer-only `{}` wire shape that production actually emits for the + // auth-profile lock-create failure (Sentry TAURI-RUST-4SZ): the + // inner io::Error Display is flattened away at the RPC boundary, so + // only the `ErrorKind` debug + os_code survive — no "no space left + // on device" text. Must still classify via the StorageFull anchor. + "Failed to create auth profile lock (kind=Some(StorageFull), os_code=Some(28))", ] { assert_eq!( expected_error_kind(raw), diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index c802fd2b2..19cba972f 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -11,7 +11,7 @@ use std::thread; use std::time::{Duration, Instant}; #[cfg(test)] -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; #[cfg(test)] use std::sync::Arc; @@ -233,6 +233,13 @@ pub struct AuthProfilesStore { /// before this split). #[cfg(test)] force_transient_failures_rename: Arc, + /// `#[cfg(test)]` failure injection — when set, the next `acquire_lock` + /// call consumes the flag and returns a synthetic `StorageFull` + /// lock-create failure, exercising the lock-free read-only fallback in + /// [`AuthProfilesStore::load`] (Sentry TAURI-RUST-4SZ). Production + /// binaries never see this field. + #[cfg(test)] + force_lock_unwritable: Arc, } impl AuthProfilesStore { @@ -278,6 +285,8 @@ impl AuthProfilesStore { force_transient_failures_write: Arc::new(AtomicUsize::new(0)), #[cfg(test)] force_transient_failures_rename: Arc::new(AtomicUsize::new(0)), + #[cfg(test)] + force_lock_unwritable: Arc::new(AtomicBool::new(false)), } } @@ -370,8 +379,28 @@ impl AuthProfilesStore { } pub fn load(&self) -> Result { - let _lock = self.acquire_lock()?; - self.load_locked() + match self.acquire_lock() { + Ok(_lock) => self.load_locked(), + Err(e) if is_lock_create_unwritable_fs(&e) => { + // RCA Sentry TAURI-RUST-4SZ: a full / read-only filesystem + // can't create the exclusive lock file, but the store already + // exists and writers publish via atomic tmp+rename, so a + // lock-free read is still consistent. The read path is the + // hot caller here (`app_state_snapshot` polls it every tick), + // so failing it strands the UI AND floods Sentry once per + // poll. Degrade to a lock-free read-only load instead — the + // user keeps their session view, and because no error is + // produced the noise stops at the source rather than being + // suppressed downstream. Opportunistic migrations are skipped + // (they couldn't persist on a full disk anyway). + log::warn!( + "[auth] auth-profile lock could not be created ({e}); \ + serving lock-free read-only load (likely disk full / read-only FS)" + ); + self.load_unlocked_readonly() + } + Err(e) => Err(e), + } } pub fn upsert_profile(&self, mut profile: AuthProfile, set_active: bool) -> Result<()> { @@ -460,6 +489,28 @@ impl AuthProfilesStore { } fn load_locked(&self) -> Result { + self.load_resolved(true) + } + + /// Lock-free read-only load used as the [`AuthProfilesStore::load`] + /// fallback when the exclusive lock can't be created because the + /// filesystem won't accept the lock file (disk full / read-only mount — + /// Sentry TAURI-RUST-4SZ). Safe without the lock because writers publish + /// the store atomically (tmp + `fs::rename`), so a bare read always sees + /// a complete file. Skips the opportunistic migration / dropped-profile + /// rewrite that `load_locked` performs — that write needs both the lock + /// and a writable disk, and this path runs precisely when neither holds. + fn load_unlocked_readonly(&self) -> Result { + self.load_resolved(false) + } + + /// Shared read + in-memory resolution worker. Reads the persisted store, + /// resolves/migrates secrets and drops unrecoverable profiles in memory, + /// and — only when `persist` is true — writes back any resulting cleanup. + /// The returned `AuthProfilesData` reflects the in-memory cleanup either + /// way, so the lock-free read path (`persist = false`) still returns a + /// correct, fully-resolved view without touching disk. + fn load_resolved(&self, persist: bool) -> Result { let mut persisted = self.read_persisted_locked()?; // `migrated` tracks enc: → enc2: XOR-cipher upgrades (original behavior). let mut migrated = false; @@ -766,6 +817,9 @@ impl AuthProfilesStore { // any `active_profiles` pointers that referenced them, so the // next read returns a clean "no active session" state. if !dropped_ids.is_empty() { + // Always apply the cleanup to the in-memory view so the returned + // data is correct even on the lock-free read path; the on-disk + // rewrite below is what's gated by `persist`. for id in &dropped_ids { persisted.profiles.remove(id); } @@ -779,8 +833,13 @@ impl AuthProfilesStore { dropped_ids.len(), self.path.display(), ); - self.write_persisted_locked(&persisted)?; - } else if migrated || keychain_migrated { + } + // Persist opportunistic cleanup / migrations only on the locked write + // path. The lock-free read-only fallback (`persist = false`, used when + // the disk can't accept the lock file) intentionally skips this — the + // write would fail on a full disk anyway, and the in-memory view above + // is already correct. + if persist && (!dropped_ids.is_empty() || migrated || keychain_migrated) { self.write_persisted_locked(&persisted)?; } @@ -1080,6 +1139,14 @@ impl AuthProfilesStore { self.force_transient_failures_rename.load(Ordering::SeqCst) } + /// Queue a single test-only forced `StorageFull` lock-create failure. The + /// next `acquire_lock` returns the synthetic disk-full error so tests can + /// drive the lock-free read-only fallback in [`AuthProfilesStore::load`]. + #[cfg(test)] + pub(super) fn force_next_lock_unwritable(&self) { + self.force_lock_unwritable.store(true, Ordering::SeqCst); + } + fn encrypt_optional(&self, value: Option<&str>) -> Result> { match value { Some(value) if !value.is_empty() => self.secret_store.encrypt(value).map(Some), @@ -1098,6 +1165,16 @@ impl AuthProfilesStore { } fn acquire_lock(&self) -> Result { + // Test-only: simulate a full / read-only filesystem that can't create + // the lock file, to drive the read-only fallback in `load`. + #[cfg(test)] + if self.force_lock_unwritable.swap(false, Ordering::SeqCst) { + let io = std::io::Error::from(std::io::ErrorKind::StorageFull); + return Err(annotate_lock_create_failure( + anyhow::Error::new(io).context("open lock file"), + )); + } + if let Some(parent) = self.lock_path.parent() { fs::create_dir_all(parent) .with_context(|| "Failed to create auth profile lock directory".to_string())?; @@ -1335,6 +1412,27 @@ impl AuthProfilesStore { /// of [`AuthProfilesStore::acquire_lock`] so unit tests can drive the /// formatting directly without depending on filesystem permissions (CI runs /// as root and bypasses `chmod 0500`). +/// True when a lock-create failure was caused by the filesystem refusing to +/// accept the lock file itself — disk full (`StorageFull`, POSIX `ENOSPC` / +/// Windows `ERROR_DISK_FULL`) or a read-only mount (`ReadOnlyFilesystem`, +/// `EROFS`). These are exactly the conditions where the **read** path can +/// safely skip the exclusive lock: the store already exists, writers publish +/// atomically, and the failing operation is the *creation of a new lock file*, +/// not the read. Lock *contention* (`AlreadyExists` / the busy-wait timeout) +/// and every other error deliberately do NOT match — those still propagate so +/// genuine problems stay visible. See [`AuthProfilesStore::load`]. +fn is_lock_create_unwritable_fs(err: &anyhow::Error) -> bool { + err.chain() + .find_map(|cause| cause.downcast_ref::()) + .map(|io| { + matches!( + io.kind(), + std::io::ErrorKind::StorageFull | std::io::ErrorKind::ReadOnlyFilesystem + ) + }) + .unwrap_or(false) +} + fn annotate_lock_create_failure(err: anyhow::Error) -> anyhow::Error { let io = err.chain().find_map(|c| c.downcast_ref::()); let kind = io.map(|ioe| ioe.kind()); diff --git a/src/openhuman/credentials/profiles_tests.rs b/src/openhuman/credentials/profiles_tests.rs index 1b7a3519b..3a4de0964 100644 --- a/src/openhuman/credentials/profiles_tests.rs +++ b/src/openhuman/credentials/profiles_tests.rs @@ -785,6 +785,123 @@ fn write_stage_exhausts_retries_on_persistent_transient() { ); } +// ───────────────────────────────────────────────────────────────────────── +// Disk-full auth-profile read resilience (Sentry TAURI-RUST-4SZ) +// ───────────────────────────────────────────────────────────────────────── + +/// When the exclusive lock can't be created because the filesystem is full, +/// the READ path must degrade to a lock-free read of the existing store +/// rather than failing — otherwise `app_state_snapshot` strands the UI and +/// floods Sentry once per poll. Writers publish atomically, so the lock-free +/// read is consistent. +#[tokio::test] +async fn load_falls_back_to_lock_free_read_when_disk_full() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), true); + + let profile = AuthProfile::new_token("openai-codex", "default", "tok-abc".into()); + store.upsert_profile(profile.clone(), true).unwrap(); + + // Next acquire_lock simulates a StorageFull (ENOSPC) lock-create failure. + store.force_next_lock_unwritable(); + + // load() must still return the persisted profile via the read-only fallback. + let data = store + .load() + .expect("load must degrade to lock-free read on disk-full"); + assert!( + data.profiles.contains_key(&profile.id), + "lock-free fallback must still surface the existing session profile" + ); + + // The flag is one-shot: the next load takes the lock normally. + let again = store + .load() + .expect("subsequent load takes the lock normally"); + assert!(again.profiles.contains_key(&profile.id)); +} + +/// The lock-free read path must return the same resolved data as the locked +/// path for a healthy store — it differs only in that it skips the +/// opportunistic on-disk rewrite, never in what it returns. +#[tokio::test] +async fn load_unlocked_readonly_matches_locked_load() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), true); + + let profile = AuthProfile::new_token("slack", "default", "tok-xyz".into()); + store.upsert_profile(profile.clone(), true).unwrap(); + + let locked = store.load().unwrap(); + let unlocked = store.load_unlocked_readonly().unwrap(); + + assert_eq!( + locked.profiles.keys().collect::>(), + unlocked.profiles.keys().collect::>(), + "lock-free read must resolve the same profile set as the locked load" + ); + assert_eq!(locked.active_profiles, unlocked.active_profiles); +} + +/// Polarity guard for the read-path fallback predicate: only genuine +/// filesystem-unwritable conditions (disk full / read-only mount) degrade the +/// read; lock contention and unrelated errors must still propagate. +#[test] +fn is_lock_create_unwritable_fs_polarity() { + let storage_full = annotate_lock_create_failure( + anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::StorageFull)) + .context("open lock file"), + ); + assert!(is_lock_create_unwritable_fs(&storage_full)); + + let read_only = annotate_lock_create_failure( + anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::ReadOnlyFilesystem)) + .context("open lock file"), + ); + assert!(is_lock_create_unwritable_fs(&read_only)); + + // Lock contention / timeout — must NOT degrade the read. + let timeout = anyhow::anyhow!("Timed out waiting for auth profile lock"); + assert!(!is_lock_create_unwritable_fs(&timeout)); + + // A different FS error (permissions) is a real problem — keep it visible. + let perm = annotate_lock_create_failure( + anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::PermissionDenied)) + .context("open lock file"), + ); + assert!(!is_lock_create_unwritable_fs(&perm)); +} + +/// Drift guard coupling the Sentry `DiskFull` classifier to the ACTUAL +/// producer output. `annotate_lock_create_failure` embeds the `io::ErrorKind` +/// debug name (`StorageFull`) instead of the io Display, and at the RPC +/// boundary the error is flattened single-line (`{}`), so the inner "no space +/// left on device" text never reaches the classifier. This asserts the +/// rendered producer string both (a) lacks that legacy anchor and (b) still +/// classifies as DiskFull — so a future format!() / std rename fails CI here +/// instead of silently re-leaking the flood. +#[test] +fn disk_full_lock_failure_string_classifies_as_disk_full() { + use crate::core::observability::{expected_error_kind, ExpectedErrorKind}; + + let err = annotate_lock_create_failure( + anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::StorageFull)) + .context("open lock file"), + ); + // The single-line Display form is what production flattens to. + let rendered = format!("{err}"); + + assert!( + !rendered.to_lowercase().contains("no space left on device"), + "outer-only render must NOT carry the legacy anchor (that's the whole bug): {rendered}" + ); + assert_eq!( + expected_error_kind(&rendered), + Some(ExpectedErrorKind::DiskFull), + "producer output must classify as DiskFull via the StorageFull anchor: {rendered}" + ); +} + #[test] fn rename_stage_retries_one_shot_transient() { let tmp = TempDir::new().unwrap();