fix(keyring): stop concurrent writers wiping the shared secrets file

src/openhuman/security/keyring/README.md

@@ -22,6 +22,7 @@ OS-keychain-backed secret storage with pluggable test/debug backends, plus a Cha
 | `src/openhuman/security/keyring/backend.rs` | `KeyringBackend` trait + `OsBackend` (native keychain via the `keyring` crate, service name `"openhuman"`), `FileBackend` (plaintext `dev-keychain.json`, test/debug only), and test-only `MockBackend`. |
 | `src/openhuman/security/keyring/encrypted_file_backend.rs` | `EncryptedFileBackend` — all secrets in one ChaCha20-Poly1305 `secrets.enc` file keyed by an app master key; `init_master_key`/`is_master_key_available`; legacy `dev-keychain.json` migration; corrupt-file quarantine. |
 | `src/openhuman/security/keyring/encrypted_store.rs` | `SecretStore` — config-field encryption (`enc2:` ChaCha20-Poly1305, legacy `enc:` XOR migration), keychain-backed master key with legacy `.secret_key` file migration, process-wide key cache, Windows ACL repair (`icacls`). |
+| `src/openhuman/security/keyring/file_store.rs` | Shared secrets-file primitives for both file backends: the cross-process advisory write lock (`lock_for_write`, on a sidecar `<path>.lock`), the `0600` unique-temp-then-rename `write_atomic`, and `quarantine_corrupt`. |
 | `src/openhuman/security/keyring/crypto.rs` | Shared ChaCha20-Poly1305 helpers (`chacha20_encrypt`/`chacha20_decrypt`), random-byte generation, hex encode/decode. Used by both `encrypted_store` and `encrypted_file_backend`. |
 | `src/openhuman/security/keyring/error.rs` | `KeyringError` (thiserror) with variants `Os`/`InvalidUtf8`/`MigrationReadFailed`/`VerifyFailed`/`MigrationDeleteFailed`/`RandomGeneration`/`Crypto`/`Backend`, plus a log-safe `diagnostic()` that preserves the `keyring::Error` variant + `OSStatus`. |
 | `src/openhuman/security/keyring/tests.rs` | Module tests (backend isolation via `force_backend_for_test`). |
@@ -62,7 +63,9 @@ Secret storage backend, selected once and frozen in a `OnceLock`:
 
 `SecretStore` additionally manages a master encryption key: keychain-backed (slot `secretstore.master_key`) in normal builds with one-time migration from the legacy `{data_dir}/openhuman/.secret_key` file; the file path is retained only for unit tests. Decoded keys are cached process-wide keyed by normalized path.
 
-Workspace dir resolves from `init_workspace`, else `OPENHUMAN_WORKSPACE`, else `~/.openhuman` (or `~/.openhuman-staging` under `OPENHUMAN_APP_ENV=staging`).
+Workspace dir resolves from `init_workspace`, else `OPENHUMAN_WORKSPACE`, else `~/.openhuman` (or `~/.openhuman-staging` under `OPENHUMAN_APP_ENV=staging`). **Under `cfg(test)` the environment is not consulted at all** — the fallback is a per-process scratch dir under the system temp dir, so test code cannot write into a developer's live store. A test that needs a specific location calls `init_workspace` before its first keyring call.
+
+Both file backends keep every secret in one file, so a `set` of one key rewrites all of them. That read-modify-write cycle is guarded by `file_store::lock_for_write` — an in-process mutex is not sufficient, because a desktop core, a `medulla` TUI embedding the same core, and a `cargo test` run that inherited `OPENHUMAN_WORKSPACE` all address the same path.
 
 ## Dependencies
 
@@ -86,6 +89,8 @@ Discovered consumers (`crate::openhuman::security::keyring::*`):
 - **`force_backend_for_test` panics if `BACKEND` is already initialized** — it must run before any keyring call in the same process (dedicated test binary or very top of a test).
 - **`migrate_from_file` never deletes the source unless the verified write succeeds**, so failures are retryable.
 - **`encrypted_file` corrupt/undecryptable files are quarantined** (renamed `secrets.enc.corrupt.<ts>`) and treated as empty rather than crashing.
+- **A corrupt file is never *overwritten*.** `file` reads degrade to an empty map (so a missing token means "sign in again"), but a `set`/`delete` over an unparseable file quarantines it and returns an error. Returning empty on the write path is what turned a corrupt file into a wipe: the write that followed persisted a map holding nothing but the key being set.
+- **Mutations hold a cross-process lock**, on `<secrets file>.lock` rather than the secrets file itself — `write_atomic` replaces the file by rename, so a lock on the old inode would guard nothing. Callers must hold it across the read *and* the write.
 - **`SecretStore` master-key file is write-once**; on Windows it survives transient AV-scanner sharing violations via retry/backoff and attempts `icacls` ACL self-repair on permission errors. Decoded keys are cached so repeated decrypts (e.g. snapshot polls) hit memory.
 - **Legacy formats:** `SecretStore` migrates `enc:` (XOR) → `enc2:` (ChaCha20-Poly1305) on decrypt; `EncryptedFileBackend` migrates plaintext `dev-keychain.json` → `secrets.enc` (renaming the legacy file `.json.migrated`).
 - **Errors never carry secret values** — only namespaced keys; `diagnostic()` is safe to log and preserves the underlying `keyring::Error` variant/`OSStatus`.

src/openhuman/security/keyring/backend.rs

@@ -15,9 +15,8 @@
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 
-use parking_lot::Mutex;
-
 use crate::openhuman::security::keyring::error::KeyringError;
+use crate::openhuman::security::keyring::file_store;
 
 // ── Trait ─────────────────────────────────────────────────────────────────────
 
@@ -105,23 +104,25 @@ impl KeyringBackend for OsBackend {
 /// keep unit tests and explicit recovery/debug overrides independent from the
 /// host OS keychain. Never use it in a production deployment.
 ///
-/// # Thread safety
+/// # Concurrency
 ///
-/// The `mutex` field serializes in-process read-modify-write operations on
-/// `set` and `delete`.  Cross-process safety relies on the atomic rename in
-/// `write_map`.
+/// Every secret lives in this one file, so a `set` or `delete` is a read →
+/// modify → write cycle over *all* of them. That cycle is guarded by the
+/// cross-process advisory lock in [`file_store::lock_for_write`] — an in-process
+/// mutex is not enough, because a desktop core, a `medulla` TUI embedding the
+/// same core, and a `cargo test` run that inherited `OPENHUMAN_WORKSPACE` all
+/// address the same path. Unguarded, the later writer's map (read before the
+/// earlier writer landed) silently discards the earlier one's secret; when that
+/// secret is the app session, the symptom is being signed out for no reason.
 pub struct FileBackend {
     path: PathBuf,
-    /// In-process lock covering the read→modify→write cycle in mutating ops.
-    mutex: Mutex<()>,
 }
 
 impl FileBackend {
     /// Create a `FileBackend` that reads/writes `{workspace_dir}/dev-keychain.json`.
     pub fn new(workspace_dir: &Path) -> Self {
         Self {
             path: workspace_dir.join("dev-keychain.json"),
-            mutex: Mutex::new(()),
         }
     }
 
@@ -130,7 +131,18 @@ impl FileBackend {
         &self.path
     }
 
-    fn read_map(&self) -> Result<HashMap<String, String>, KeyringError> {
+    /// Read the whole map.
+    ///
+    /// `for_write` decides what an unparseable file means, and the two answers
+    /// are not interchangeable:
+    ///
+    /// - Reading (`false`): degrade to empty. The caller sees "no such secret",
+    ///   which for a session token means signing in again — recoverable, and
+    ///   better than failing every unrelated lookup.
+    /// - Writing (`true`): quarantine the bytes and fail. Returning empty here
+    ///   is what turned a corrupt file into a *wipe*, because the write that
+    ///   followed persisted a map holding nothing but the key being set.
+    fn read_map(&self, for_write: bool) -> Result<HashMap<String, String>, KeyringError> {
         if !self.path.exists() {
             return Ok(HashMap::new());
         }
@@ -141,82 +153,56 @@ impl FileBackend {
         if bytes.is_empty() {
             return Ok(HashMap::new());
         }
-        serde_json::from_slice::<HashMap<String, String>>(&bytes)
-            .map_err(|e| {
-                // Treat a corrupt file as empty so we degrade gracefully.
+        match serde_json::from_slice::<HashMap<String, String>>(&bytes) {
+            Ok(map) => Ok(map),
+            Err(e) if for_write => {
+                file_store::quarantine_corrupt(&self.path, "json");
+                Err(KeyringError::Backend(format!(
+                    "dev-keychain.json at {} could not be parsed ({e}); it was moved aside \
+                     rather than overwritten",
+                    self.path.display()
+                )))
+            }
+            Err(e) => {
                 log::warn!(
                     "[keyring] dev-keychain.json at {} is corrupt ({e}); treating as empty",
                     self.path.display()
                 );
-                // Return empty map by converting to a no-source variant.
-                drop(e);
-                KeyringError::VerifyFailed {
-                    key: "<parse>".to_string(),
-                }
-            })
-            .or_else(|_| Ok(HashMap::new()))
+                Ok(HashMap::new())
+            }
+        }
     }
 
     fn write_map(&self, map: &HashMap<String, String>) -> Result<(), KeyringError> {
-        if let Some(parent) = self.path.parent() {
-            std::fs::create_dir_all(parent).map_err(|e| KeyringError::MigrationReadFailed {
-                path: parent.display().to_string(),
-                source: e,
-            })?;
-        }
-
-        // Serialize the map to pretty JSON.  Propagate serialization failure so
-        // callers are not silently fed empty data on a write error.
+        // Propagate serialization failure so callers are not silently fed empty
+        // data on a write error.
         let json = serde_json::to_vec_pretty(map).map_err(|e| {
             KeyringError::Backend(format!("failed to serialize dev keychain map: {e}"))
         })?;
-
-        // Atomic write: temp file + rename.
-        let tmp_path = self.path.with_extension("tmp");
-        std::fs::write(&tmp_path, &json).map_err(|e| KeyringError::MigrationDeleteFailed {
-            path: tmp_path.display().to_string(),
-            source: e,
-        })?;
-
-        // Set mode 0600 on Unix before moving into place.
-        #[cfg(unix)]
-        {
-            use std::os::unix::fs::PermissionsExt;
-            let perms = std::fs::Permissions::from_mode(0o600);
-            if let Err(e) = std::fs::set_permissions(&tmp_path, perms) {
-                log::warn!("[keyring] could not set 0600 on dev-keychain.json tmp file: {e}");
-            }
-        }
-
-        std::fs::rename(&tmp_path, &self.path).map_err(|e| {
-            KeyringError::MigrationDeleteFailed {
-                path: self.path.display().to_string(),
-                source: e,
-            }
-        })?;
-
-        Ok(())
+        file_store::write_atomic(&self.path, &json)
     }
 }
 
 impl KeyringBackend for FileBackend {
     fn get(&self, namespaced_key: &str) -> Result<Option<String>, KeyringError> {
-        let map = self.read_map()?;
+        // No lock: `write_atomic` publishes by rename, so a reader sees either
+        // the whole previous file or the whole next one, never a mix.
+        let map = self.read_map(false)?;
         Ok(map.get(namespaced_key).cloned())
     }
 
     fn set(&self, namespaced_key: &str, value: &str) -> Result<(), KeyringError> {
-        // Hold the in-process lock for the full read→modify→write cycle.
-        let _guard = self.mutex.lock();
-        let mut map = self.read_map()?;
+        // Held across the read as well as the write: taking it around the write
+        // alone would still let a stale map overwrite a concurrent one.
+        let _guard = file_store::lock_for_write(&self.path)?;
+        let mut map = self.read_map(true)?;
         map.insert(namespaced_key.to_string(), value.to_string());
         self.write_map(&map)
     }
 
     fn delete(&self, namespaced_key: &str) -> Result<(), KeyringError> {
-        // Hold the in-process lock for the full read→modify→write cycle.
-        let _guard = self.mutex.lock();
-        let mut map = self.read_map()?;
+        let _guard = file_store::lock_for_write(&self.path)?;
+        let mut map = self.read_map(true)?;
         if map.remove(namespaced_key).is_some() {
             self.write_map(&map)?;
         }

src/openhuman/security/keyring/encrypted_file_backend.rs

@@ -13,11 +13,10 @@ use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 use std::sync::OnceLock;
 
-use parking_lot::Mutex;
-
 use crate::openhuman::security::keyring::backend::KeyringBackend;
 use crate::openhuman::security::keyring::crypto::{self, KEY_LEN};
 use crate::openhuman::security::keyring::error::KeyringError;
+use crate::openhuman::security::keyring::file_store;
 use crate::openhuman::security::keyring::store::BackendKind;
 
 const KEYCHAIN_SERVICE: &str = "openhuman";
@@ -174,18 +173,24 @@ fn master_key() -> Option<&'static [u8; KEY_LEN]> {
 
 // ── Backend ──────────────────────────────────────────────────────────────────
 
+/// Every secret in one ChaCha20-Poly1305 file.
+///
+/// Mutations are a read → decrypt → modify → encrypt → write cycle over the
+/// whole set, guarded by the cross-process advisory lock in
+/// [`file_store::lock_for_write`]. An in-process mutex would not do: more than
+/// one process routinely addresses the same workspace (a desktop core and a
+/// `medulla` TUI embedding the same core), and the later writer's snapshot —
+/// read before the earlier writer landed — silently drops the earlier secret.
 pub struct EncryptedFileBackend {
     path: PathBuf,
     workspace_dir: PathBuf,
-    mutex: Mutex<()>,
 }
 
 impl EncryptedFileBackend {
     pub fn new(workspace_dir: &Path) -> Self {
         Self {
             path: workspace_dir.join(SECRETS_FILENAME),
             workspace_dir: workspace_dir.to_path_buf(),
-            mutex: Mutex::new(()),
         }
     }
 
@@ -230,42 +235,13 @@ impl EncryptedFileBackend {
         key: &[u8; KEY_LEN],
         map: &HashMap<String, String>,
     ) -> Result<(), KeyringError> {
-        if let Some(parent) = self.path.parent() {
-            std::fs::create_dir_all(parent).map_err(|e| KeyringError::MigrationReadFailed {
-                path: parent.display().to_string(),
-                source: e,
-            })?;
-        }
-
         let json = serde_json::to_vec(map)
             .map_err(|e| KeyringError::Backend(format!("failed to serialize secrets: {e}")))?;
 
         let blob = crypto::chacha20_encrypt(key, &json)
             .map_err(|e| KeyringError::Backend(format!("encryption failed: {e}")))?;
 
-        let tmp_path = self.path.with_extension("enc.tmp");
-        std::fs::write(&tmp_path, &blob).map_err(|e| KeyringError::MigrationDeleteFailed {
-            path: tmp_path.display().to_string(),
-            source: e,
-        })?;
-
-        #[cfg(unix)]
-        {
-            use std::os::unix::fs::PermissionsExt;
-            let perms = std::fs::Permissions::from_mode(0o600);
-            if let Err(e) = std::fs::set_permissions(&tmp_path, perms) {
-                log::warn!("[keyring:encrypted_file] could not set 0600 on temp file: {e}");
-            }
-        }
-
-        std::fs::rename(&tmp_path, &self.path).map_err(|e| {
-            KeyringError::MigrationDeleteFailed {
-                path: self.path.display().to_string(),
-                source: e,
-            }
-        })?;
-
-        Ok(())
+        file_store::write_atomic(&self.path, &blob)
     }
 
     fn migrate_legacy_dev_keychain(
@@ -320,17 +296,10 @@ impl EncryptedFileBackend {
         Ok(map)
     }
 
+    /// Move an undecryptable / unparseable secrets file aside so the next call
+    /// starts fresh without destroying the bytes.
     fn handle_corruption(&self) {
-        let ts = chrono::Utc::now().format("%Y%m%d%H%M%S");
-        let corrupt_path = self.path.with_extension(format!("enc.corrupt.{ts}"));
-        if let Err(e) = std::fs::rename(&self.path, &corrupt_path) {
-            log::error!("[keyring:encrypted_file] could not rename corrupt file: {e}");
-        } else {
-            log::warn!(
-                "[keyring:encrypted_file] corrupt file renamed to {}",
-                corrupt_path.display()
-            );
-        }
+        file_store::quarantine_corrupt(&self.path, "enc");
     }
 }
 
@@ -339,7 +308,17 @@ impl KeyringBackend for EncryptedFileBackend {
         let Some(key) = master_key() else {
             return Ok(None);
         };
-        let _guard = self.mutex.lock();
+        // Usually no lock is needed: `write_atomic` publishes by rename, so a
+        // reader sees either the whole previous file or the whole next one,
+        // never a mix. A missing encrypted file is different: `read_map`
+        // migrates the legacy store, which writes and renames files. Hold the
+        // same lock as writers across that read so its legacy snapshot cannot
+        // overwrite a concurrent `set`.
+        if !self.path.exists() {
+            let _guard = file_store::lock_for_write(&self.path)?;
+            let map = self.read_map(key)?;
+            return Ok(map.get(namespaced_key).cloned());
+        }
         let map = self.read_map(key)?;
         Ok(map.get(namespaced_key).cloned())
     }
@@ -350,7 +329,9 @@ impl KeyringBackend for EncryptedFileBackend {
                 "master key unavailable — cannot store secrets".to_string(),
             ));
         };
-        let _guard = self.mutex.lock();
+        // Held across the read as well as the write: taking it around the write
+        // alone would still let a stale map overwrite a concurrent one.
+        let _guard = file_store::lock_for_write(&self.path)?;
         let mut map = self.read_map(key)?;
         map.insert(namespaced_key.to_string(), value.to_string());
         self.write_map(key, &map)
@@ -360,7 +341,7 @@ impl KeyringBackend for EncryptedFileBackend {
         let Some(key) = master_key() else {
             return Ok(());
         };
-        let _guard = self.mutex.lock();
+        let _guard = file_store::lock_for_write(&self.path)?;
         let mut map = self.read_map(key)?;
         if map.remove(namespaced_key).is_some() {
             self.write_map(key, &map)?;