From 5f18f7b421f3b621e2e3644899833c0bf4fa815e Mon Sep 17 00:00:00 2001 From: sheikhlimon Date: Thu, 22 Jan 2026 02:43:38 +0600 Subject: [PATCH 1/4] fix: cache secrets to prevent repeated keychain prompts on macOS Signed-off-by: sheikhlimon --- crates/goose/src/config/base.rs | 47 ++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index f35fbef1d4fb..7cfba57a3adf 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -12,7 +12,7 @@ use std::ffi::OsString; use std::fs::OpenOptions; use std::io::Write; use std::path::{Path, PathBuf}; -use std::sync::Mutex; +use std::sync::{Arc, Mutex}; use thiserror::Error; const KEYRING_SERVICE: &str = "goose"; @@ -105,6 +105,7 @@ pub struct Config { config_path: PathBuf, secrets: SecretStorage, guard: Mutex<()>, + secrets_cache: Arc>>>, } enum SecretStorage { @@ -133,6 +134,7 @@ impl Default for Config { config_path, secrets, guard: Mutex::new(()), + secrets_cache: Arc::new(Mutex::new(None)), } } } @@ -236,6 +238,7 @@ impl Config { service: service.to_string(), }, guard: Mutex::new(()), + secrets_cache: Arc::new(Mutex::new(None)), }) } @@ -253,6 +256,7 @@ impl Config { path: secrets_path.as_ref().to_path_buf(), }, guard: Mutex::new(()), + secrets_cache: Arc::new(Mutex::new(None)), }) } @@ -540,7 +544,16 @@ impl Config { } pub fn all_secrets(&self) -> Result, ConfigError> { - match &self.secrets { + if let Ok(cache) = self.secrets_cache.lock() { + if let Some(ref cached_secrets) = *cache { + tracing::debug!("secrets cache hit"); + return Ok(cached_secrets.clone()); + } + } + + tracing::debug!("secrets cache miss, fetching from storage"); + + let values = match &self.secrets { SecretStorage::Keyring { service } => { let result = self.handle_keyring_operation(|entry| entry.get_password(), service, None); @@ -548,20 +561,27 @@ impl Config { match result { Ok(content) => { let values: HashMap = serde_json::from_str(&content)?; - Ok(values) + values } - Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage(), + Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage()?, Err(ConfigError::KeyringError(msg)) if msg.contains("No entry found") || msg.contains("No matching entry found") => { - Ok(HashMap::new()) + HashMap::new() } - Err(e) => Err(e), + Err(e) => return Err(e), } } - SecretStorage::File { path } => self.read_secrets_from_file(path), + SecretStorage::File { path } => self.read_secrets_from_file(path)?, + }; + + if let Ok(mut cache) = self.secrets_cache.lock() { + *cache = Some(values.clone()); + tracing::debug!("secrets cached"); } + + Ok(values) } /// Parse an environment variable value into a JSON Value. @@ -786,6 +806,9 @@ impl Config { std::fs::write(path, yaml_value)?; } }; + + self.invalidate_secrets_cache(); + Ok(()) } @@ -820,6 +843,9 @@ impl Config { std::fs::write(path, yaml_value)?; } }; + + self.invalidate_secrets_cache(); + Ok(()) } @@ -858,6 +884,13 @@ impl Config { Ok(()) } + fn invalidate_secrets_cache(&self) { + if let Ok(mut cache) = self.secrets_cache.lock() { + *cache = None; + tracing::debug!("secrets cache invalidated"); + } + } + /// Check if an error string indicates a keyring availability issue that should trigger fallback fn is_keyring_availability_error(&self, error_str: &str) -> bool { error_str.contains("keyring") From a1b844f4f4ee79578e477ff0dd27f88919c6b392 Mon Sep 17 00:00:00 2001 From: sheikhlimon Date: Thu, 22 Jan 2026 19:44:34 +0600 Subject: [PATCH 2/4] hold lock during cache load to prevent race condition Signed-off-by: sheikhlimon --- crates/goose/src/config/base.rs | 59 +++++++++++++++++---------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 7cfba57a3adf..0b55a34ac22b 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -544,42 +544,43 @@ impl Config { } pub fn all_secrets(&self) -> Result, ConfigError> { - if let Ok(cache) = self.secrets_cache.lock() { - if let Some(ref cached_secrets) = *cache { - tracing::debug!("secrets cache hit"); - return Ok(cached_secrets.clone()); - } - } + let mut cache = self.secrets_cache.lock().unwrap(); - tracing::debug!("secrets cache miss, fetching from storage"); + let values = if let Some(ref cached_secrets) = *cache { + tracing::debug!("secrets cache hit"); + cached_secrets.clone() + } else { + tracing::debug!("secrets cache miss, fetching from storage"); - let values = match &self.secrets { - SecretStorage::Keyring { service } => { - let result = - self.handle_keyring_operation(|entry| entry.get_password(), service, None); + let loaded = match &self.secrets { + SecretStorage::Keyring { service } => { + let result = + self.handle_keyring_operation(|entry| entry.get_password(), service, None); - match result { - Ok(content) => { - let values: HashMap = serde_json::from_str(&content)?; - values - } - Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage()?, - Err(ConfigError::KeyringError(msg)) - if msg.contains("No entry found") - || msg.contains("No matching entry found") => - { - HashMap::new() + match result { + Ok(content) => { + let values: HashMap = serde_json::from_str(&content)?; + values + } + Err(ConfigError::FallbackToFileStorage) => { + self.fallback_to_file_storage()? + } + Err(ConfigError::KeyringError(msg)) + if msg.contains("No entry found") + || msg.contains("No matching entry found") => + { + HashMap::new() + } + Err(e) => return Err(e), } - Err(e) => return Err(e), } - } - SecretStorage::File { path } => self.read_secrets_from_file(path)?, - }; + SecretStorage::File { path } => self.read_secrets_from_file(path)?, + }; - if let Ok(mut cache) = self.secrets_cache.lock() { - *cache = Some(values.clone()); + *cache = Some(loaded.clone()); tracing::debug!("secrets cached"); - } + loaded + }; Ok(values) } From 207517e3081fa5be652e0ff24e368f69f1cac59c Mon Sep 17 00:00:00 2001 From: sheikhlimon Date: Thu, 22 Jan 2026 20:11:27 +0600 Subject: [PATCH 3/4] make it consistent to all_secrets Signed-off-by: sheikhlimon --- crates/goose/src/config/base.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 0b55a34ac22b..e18acb4b8cee 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -886,10 +886,9 @@ impl Config { } fn invalidate_secrets_cache(&self) { - if let Ok(mut cache) = self.secrets_cache.lock() { - *cache = None; - tracing::debug!("secrets cache invalidated"); - } + let mut cache = self.secrets_cache.lock().unwrap(); + *cache = None; + tracing::debug!("secrets cache invalidated"); } /// Check if an error string indicates a keyring availability issue that should trigger fallback From 68a82d7ee4acd21ccba1cef1aa2131042d08142d Mon Sep 17 00:00:00 2001 From: sheikhlimon Date: Thu, 22 Jan 2026 20:32:24 +0600 Subject: [PATCH 4/4] minimal logging Signed-off-by: sheikhlimon --- crates/goose/src/config/base.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index e18acb4b8cee..4a01609be79b 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -547,7 +547,6 @@ impl Config { let mut cache = self.secrets_cache.lock().unwrap(); let values = if let Some(ref cached_secrets) = *cache { - tracing::debug!("secrets cache hit"); cached_secrets.clone() } else { tracing::debug!("secrets cache miss, fetching from storage"); @@ -578,7 +577,6 @@ impl Config { }; *cache = Some(loaded.clone()); - tracing::debug!("secrets cached"); loaded }; @@ -888,7 +886,6 @@ impl Config { fn invalidate_secrets_cache(&self) { let mut cache = self.secrets_cache.lock().unwrap(); *cache = None; - tracing::debug!("secrets cache invalidated"); } /// Check if an error string indicates a keyring availability issue that should trigger fallback