From 71fc4be1ed729e26b1dc0a4466abdd03be548a53 Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Wed, 2 Sep 2026 19:01:59 +0000 Subject: [PATCH] remove keyring timeout (#11798) --- crates/goose/src/config/base.rs | 89 +-------------------------------- 1 file changed, 2 insertions(+), 87 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 6d710c777..8e48d19b4 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -49,18 +49,6 @@ pub enum ConfigError { LockError(String), #[error("Secret stored using file-based fallback")] FallbackToFileStorage, - #[error("Timed out reading the system keyring")] - KeyringTimeout, -} - -/// Outcome of a bounded keyring read. -/// -/// A timeout is kept separate from a keyring error so callers never confuse -/// "the read did not finish" with "there is no entry". -#[cfg(feature = "system-keyring")] -enum KeyringReadError { - Keyring(keyring::Error), - TimedOut, } impl From for ConfigError { @@ -924,23 +912,8 @@ impl Config { match &self.secrets { #[cfg(feature = "system-keyring")] SecretStorage::Keyring { service } => { - let result = match Self::read_keyring_password_with_timeout(service) { - Ok(content) => Ok(content), - // A timed-out read says nothing about whether secrets - // exist. Surface it instead of falling back, so the - // empty file store is never cached as authoritative and - // a later mutation cannot overwrite the real keyring. - Err(KeyringReadError::TimedOut) => { - tracing::warn!( - "keyring read timed out after 3s; not falling back to file \ - storage (set GOOSE_DISABLE_KEYRING=1 to skip the keyring)" - ); - Err(ConfigError::KeyringTimeout) - } - Err(KeyringReadError::Keyring(keyring_err)) => { - self.handle_keyring_fallback_error(&keyring_err, None) - } - }; + let result = + self.handle_keyring_operation(|entry| entry.get_password(), service, None); match result { Ok(content) => Ok(serde_json::from_str(&content)?), @@ -1155,64 +1128,6 @@ impl Config { Entry::new(service, KEYRING_USERNAME) } - /// Read the keyring password on a dedicated thread with a timeout. - /// - /// A synchronous keychain read can block indefinitely — e.g. an unsigned - /// binary triggers a macOS keychain ACL prompt that can't be answered when - /// running headless or over piped stdio (as with `goose acp`). Because this - /// read sits on the `session/new` critical path, a block there hangs the - /// whole async runtime. Bounding it keeps the runtime responsive. - /// - /// A timeout is reported distinctly from a keyring error. It must never be - /// mistaken for "this user has no secrets": the entry may hold every - /// configured credential and simply be waiting on an ACL prompt, so - /// treating it as absent would cache an empty secret map and let the next - /// mutation overwrite the real keyring contents. - #[cfg(feature = "system-keyring")] - fn read_keyring_password_with_timeout(service: &str) -> Result { - use std::sync::mpsc; - use std::time::Duration; - - // One long-lived worker performs every keyring read through a - // single-slot queue. If a read blocks indefinitely (e.g. a pending - // keychain ACL prompt on a headless host), at most one thread is ever - // stuck and at most one request is ever queued behind it — later - // lookups fail fast with a timeout instead of growing an unbounded - // queue. Replies to abandoned requests land in dropped receivers and - // are discarded. - type ReadRequest = (String, mpsc::Sender>); - static WORKER: std::sync::OnceLock>> = - std::sync::OnceLock::new(); - - let worker = WORKER.get_or_init(|| { - let (tx, rx) = mpsc::sync_channel::(1); - std::thread::Builder::new() - .name("goose-keyring-read".to_string()) - .spawn(move || { - while let Ok((service, reply)) = rx.recv() { - let result = Self::get_keyring_entry(&service) - .and_then(|entry| entry.get_password()); - let _ = reply.send(result); - } - }) - .expect("failed to spawn keyring reader thread"); - std::sync::Mutex::new(tx) - }); - - let (tx, rx) = mpsc::channel(); - worker - .lock() - .unwrap() - .try_send((service.to_string(), tx)) - .map_err(|_| KeyringReadError::TimedOut)?; - - match rx.recv_timeout(Duration::from_secs(3)) { - Ok(Ok(password)) => Ok(password), - Ok(Err(err)) => Err(KeyringReadError::Keyring(err)), - Err(_) => Err(KeyringReadError::TimedOut), - } - } - /// Handle keyring errors with automatic fallback to file storage #[cfg(feature = "system-keyring")] fn handle_keyring_fallback_error(