remove keyring timeout (#11798)
This commit is contained in:
@@ -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<serde_json::Error> 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<String, KeyringReadError> {
|
||||
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<Result<String, keyring::Error>>);
|
||||
static WORKER: std::sync::OnceLock<std::sync::Mutex<mpsc::SyncSender<ReadRequest>>> =
|
||||
std::sync::OnceLock::new();
|
||||
|
||||
let worker = WORKER.get_or_init(|| {
|
||||
let (tx, rx) = mpsc::sync_channel::<ReadRequest>(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<T>(
|
||||
|
||||
Reference in New Issue
Block a user