fix(credentials): degrade auth-profile read to lock-free on disk-full (#3407) (#3409)

src/core/observability.rs

@@ -443,8 +443,22 @@ pub fn expected_error_kind(message: &str) -> Option<ExpectedErrorKind> {
 ///   `"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),

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<AtomicUsize>,
+    /// `#[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<AtomicBool>,
 }
 
 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<AuthProfilesData> {
-        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<AuthProfilesData> {
+        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<AuthProfilesData> {
+        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<AuthProfilesData> {
         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<Option<String>> {
         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<AuthProfileLockGuard> {
+        // 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::<std::io::Error>())
+        .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::<std::io::Error>());
     let kind = io.map(|ioe| ioe.kind());

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::<Vec<_>>(),
+        unlocked.profiles.keys().collect::<Vec<_>>(),
+        "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();