fix(secrets): cache decoded key + retry transient reads (OPENHUMAN-TAURI-58) (#1509)

src/openhuman/security/secrets.rs

@@ -23,8 +23,11 @@
 use anyhow::{Context, Result};
 use chacha20poly1305::aead::{Aead, KeyInit, OsRng};
 use chacha20poly1305::{AeadCore, ChaCha20Poly1305, Key, Nonce};
+use std::collections::HashMap;
 use std::fs;
 use std::path::{Path, PathBuf};
+use std::sync::{Mutex, OnceLock};
+use std::time::Duration;
 
 /// Length of the random encryption key in bytes (256-bit, matches `ChaCha20`).
 const KEY_LEN: usize = 32;
@@ -168,11 +171,33 @@ impl SecretStore {
     }
 
     /// Load the encryption key from disk, or create one if it doesn't exist.
+    ///
+    /// The decoded key is cached process-wide keyed by `key_path`, so repeated
+    /// callers (e.g. every `app_state_snapshot` poll) hit memory instead of
+    /// disk. This also rides over transient Windows sharing violations that
+    /// can occur when an AV scanner briefly locks the file — once we've read
+    /// it successfully, we never need to read it again for this process.
     fn load_or_create_key(&self) -> Result<Vec<u8>> {
+        // Normalize the path once so all callers share the same cache slot
+        // regardless of how `key_path` was spelled (relative vs absolute,
+        // symlinks, case-variants on Windows).
+        let cache_key_path = normalize_cache_path(&self.key_path);
+
+        if let Some(cached) = cached_key(&cache_key_path) {
+            return Ok(cached);
+        }
+
         if self.key_path.exists() {
-            let hex_key =
-                fs::read_to_string(&self.key_path).context("Failed to read secret key file")?;
-            hex_decode(hex_key.trim()).context("Secret key file is corrupt")
+            let hex_key = read_key_file_with_retry(&self.key_path)
+                .context("Failed to read secret key file")?;
+            let key = hex_decode(hex_key.trim()).context("Secret key file is corrupt")?;
+            anyhow::ensure!(
+                key.len() == KEY_LEN,
+                "Secret key file has wrong length: expected {KEY_LEN} bytes, got {}",
+                key.len()
+            );
+            cache_key(&cache_key_path, &key);
+            Ok(key)
         } else {
             let key = generate_random_key();
             if let Some(parent) = self.key_path.parent() {
@@ -197,6 +222,7 @@ impl SecretStore {
                         "USERNAME environment variable is empty; \
                          cannot restrict key file permissions via icacls"
                     );
+                    cache_key(&cache_key_path, &key);
                     return Ok(key);
                 };
 
@@ -221,11 +247,87 @@ impl SecretStore {
                 }
             }
 
+            cache_key(&cache_key_path, &key);
             Ok(key)
         }
     }
 }
 
+/// Normalize a path into a stable cache key. Tries `canonicalize` first (so
+/// symlinks, relative paths, and Windows case-variants all collapse to the
+/// same key), falls back to `std::path::absolute` when the file does not yet
+/// exist (e.g. the create branch in `load_or_create_key`), and finally to the
+/// raw path so a normalization failure never breaks the cache.
+fn normalize_cache_path(path: &Path) -> PathBuf {
+    fs::canonicalize(path)
+        .or_else(|_| std::path::absolute(path))
+        .unwrap_or_else(|_| path.to_path_buf())
+}
+
+/// Process-wide cache of decoded key bytes keyed by absolute path.
+///
+/// Loading the key once per process is both faster and more reliable than
+/// re-reading `.secret_key` on every decrypt. On Windows the file can be
+/// transiently inaccessible (AV scanners holding a handle), and re-reading
+/// turned that transient failure into a perma-failure for every subsequent
+/// RPC call.
+fn key_cache() -> &'static Mutex<HashMap<PathBuf, Vec<u8>>> {
+    static CACHE: OnceLock<Mutex<HashMap<PathBuf, Vec<u8>>>> = OnceLock::new();
+    CACHE.get_or_init(|| Mutex::new(HashMap::new()))
+}
+
+fn cached_key(path: &Path) -> Option<Vec<u8>> {
+    key_cache().lock().ok()?.get(path).cloned()
+}
+
+fn cache_key(path: &Path, key: &[u8]) {
+    if let Ok(mut cache) = key_cache().lock() {
+        cache.insert(path.to_path_buf(), key.to_vec());
+    }
+}
+
+/// Clear the cached key for `path`. Test-only — production code should never
+/// need to invalidate the cache, since the key file is write-once.
+#[cfg(test)]
+pub(super) fn clear_cached_key(path: &Path) {
+    let normalized = normalize_cache_path(path);
+    if let Ok(mut cache) = key_cache().lock() {
+        cache.remove(&normalized);
+    }
+}
+
+/// Read the key file, retrying transient errors a handful of times.
+///
+/// Windows AV scanners (Defender, etc.) routinely hold short-lived read
+/// handles right after a file is created, which surfaces as
+/// `ERROR_SHARING_VIOLATION` (raw OS error 32) or `PermissionDenied`. A few
+/// short backoffs are enough to ride over the lock; the typical successful
+/// path returns on the first attempt with zero added latency.
+fn read_key_file_with_retry(path: &Path) -> std::io::Result<String> {
+    use std::io::ErrorKind;
+
+    const MAX_ATTEMPTS: u32 = 5;
+    let mut last_err: Option<std::io::Error> = None;
+    for attempt in 0..MAX_ATTEMPTS {
+        match fs::read_to_string(path) {
+            Ok(contents) => return Ok(contents),
+            Err(err) => {
+                let transient = matches!(
+                    err.kind(),
+                    ErrorKind::PermissionDenied | ErrorKind::Interrupted | ErrorKind::WouldBlock
+                ) || err.raw_os_error() == Some(32); // ERROR_SHARING_VIOLATION (Windows)
+                last_err = Some(err);
+                if !transient || attempt + 1 == MAX_ATTEMPTS {
+                    break;
+                }
+                let backoff_ms = 10u64 << attempt; // 10, 20, 40, 80 ms
+                std::thread::sleep(Duration::from_millis(backoff_ms));
+            }
+        }
+    }
+    Err(last_err.unwrap_or_else(|| std::io::Error::other("read_to_string failed")))
+}
+
 /// XOR cipher with repeating key. Same function for encrypt and decrypt.
 fn xor_cipher(data: &[u8], key: &[u8]) -> Vec<u8> {
     if key.is_empty() {

src/openhuman/security/secrets_tests.rs

@@ -549,6 +549,58 @@ fn generate_random_key_has_no_uuid_fixed_bits() {
     );
 }
 
+#[test]
+fn key_loaded_once_then_cached() {
+    // After the first read, subsequent decrypts must not depend on the key
+    // file being readable. This is the property that protects us from
+    // transient Windows sharing violations on `.secret_key` (Sentry
+    // OPENHUMAN-TAURI-58: "Failed to read secret key file" hammering
+    // app_state_snapshot).
+    let tmp = TempDir::new().unwrap();
+    let store = SecretStore::new(tmp.path(), true);
+
+    let encrypted = store.encrypt("cached-secret").unwrap();
+    assert!(store.key_path.exists());
+
+    // Make the file unreadable by deleting it — the in-memory cache should
+    // still satisfy the decrypt.
+    fs::remove_file(&store.key_path).unwrap();
+    let decrypted = store.decrypt(&encrypted).unwrap();
+    assert_eq!(decrypted, "cached-secret");
+
+    // After clearing the cache, the disappearance is visible again: the
+    // store falls back to the "create new key" branch and decryption with
+    // the original ciphertext fails.
+    super::clear_cached_key(&store.key_path);
+    let result = store.decrypt(&encrypted);
+    assert!(
+        result.is_err(),
+        "Without cache and without file, decrypt must fail"
+    );
+}
+
+#[test]
+fn malformed_key_file_rejected_not_panic() {
+    // hex_decode only checks the string is even-length, so a truncated /
+    // padded key file would previously sail through and panic later inside
+    // `Key::from_slice` (ChaCha20-Poly1305 requires exactly 32 bytes).
+    // Verify we now reject with a clean error.
+    let tmp = TempDir::new().unwrap();
+    let store = SecretStore::new(tmp.path(), true);
+
+    // Write a 30-byte hex key (60 chars, even, decodes cleanly, wrong length).
+    fs::create_dir_all(&tmp.path()).unwrap();
+    fs::write(&store.key_path, "aa".repeat(30)).unwrap();
+    super::clear_cached_key(&store.key_path);
+
+    let err = store.encrypt("anything").unwrap_err();
+    let msg = format!("{err:?}");
+    assert!(
+        msg.contains("wrong length"),
+        "expected wrong-length error, got: {msg}"
+    );
+}
+
 #[cfg(unix)]
 #[test]
 fn key_file_has_restricted_permissions() {