diff --git a/CHANGELOG.md b/CHANGELOG.md index 55a500f9..41446f17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,19 @@ internal fix. Big thanks to every contributor below. status messages still take precedence; the hint clears as soon as any other update fires. Closes the loop on users who didn't know how to interrupt a long-running turn. +- **Lazy auto-reload of MCP pool on config-file change** (#1267 part 2) — + v0.8.26 surfaced the underlying spawn errors; v0.8.27 closes the + loop on the second half of the report (manual `/mcp reload` after + `~/.deepseek/mcp.json` edits). `McpPool::get_or_connect` now does a + cheap `stat` + content-hash check before each connection lookup. If + the on-disk file's mtime moved AND its content hash changed since + the pool was loaded, all live connections are dropped so the next + `get_or_connect` reattaches under the new config. Pool-construction + via `McpPool::new` (tests, ad-hoc snapshots) is unaffected — only + pools built with `from_config_path` watch the source file. No file + watcher; no long-lived task. mtime-only churn (touched but + byte-unchanged content) does not trigger a reload, so networked + filesystems with coarse mtime granularity won't churn the pool. - **HTTP 400 quota errors retried** (#1203) — some OpenAI-compatible gateways return quota/rate-limit errors as HTTP 400 instead of 429. These are now classified as retryable `RateLimited` errors. diff --git a/crates/tui/src/mcp.rs b/crates/tui/src/mcp.rs index 8cd208b2..4c828dd2 100644 --- a/crates/tui/src/mcp.rs +++ b/crates/tui/src/mcp.rs @@ -1424,15 +1424,32 @@ pub struct McpPool { connections: HashMap, config: McpConfig, network_policy: Option, + /// Source path the config was loaded from, when `from_config_path` was + /// used. `None` for pools constructed directly via `new` (tests, ad-hoc + /// snapshots). Drives the lazy-reload check (#1267 part 2): when the + /// file's mtime moves, the pool re-reads the config and compares its + /// content hash to decide whether to drop existing connections. + config_source: Option, + /// 64-bit content hash of the active config (`hash_mcp_config`). Compared + /// against the freshly-loaded config after an mtime change to skip + /// reloading when the file was merely touched. + config_hash: u64, + /// Most recently observed mtime of `config_source`. Updated whenever the + /// reload check runs (whether or not it triggered a reload). + last_mtime: Option, } impl McpPool { /// Create a new pool with the given configuration pub fn new(config: McpConfig) -> Self { + let config_hash = hash_mcp_config(&config); Self { connections: HashMap::new(), config, network_policy: None, + config_source: None, + config_hash, + last_mtime: None, } } @@ -1447,7 +1464,11 @@ impl McpPool { } else { McpConfig::default() }; - Ok(Self::new(config)) + let last_mtime = mcp_config_mtime(path); + let mut pool = Self::new(config); + pool.config_source = Some(path.to_path_buf()); + pool.last_mtime = last_mtime; + Ok(pool) } /// Attach a per-domain network policy (#135). When set, HTTP/SSE @@ -1457,8 +1478,63 @@ impl McpPool { self } + /// If the source config file's mtime has changed since the last check, + /// re-read it and (only when the content hash also changed) drop all + /// existing connections so the next `get_or_connect` reattaches under + /// the new config. No-op when the pool was constructed via [`McpPool::new`] + /// (no source path), when stat fails, or when the file content is + /// byte-identical to what we last loaded. Returns `Ok(true)` if any + /// connections were dropped, `Ok(false)` otherwise. + /// + /// This is the lazy half of the auto-reload story for #1267: instead of a + /// long-lived file watcher, the next tool invocation pays a single `stat` + /// call (and only re-reads the file when the mtime moved). On networked + /// or remote filesystems where mtime granularity is poor, the hash + /// compare keeps us from churning connections on every check. + pub async fn reload_if_config_changed(&mut self) -> Result { + let Some(path) = self.config_source.clone() else { + return Ok(false); + }; + let current_mtime = match mcp_config_mtime(&path) { + Some(m) => m, + None => return Ok(false), + }; + if Some(current_mtime) == self.last_mtime { + return Ok(false); + } + // mtime moved — we owe a re-read. + let new_config: McpConfig = if path.exists() { + let contents = fs::read_to_string(&path) + .with_context(|| format!("Failed to re-read MCP config: {}", path.display()))?; + serde_json::from_str(&contents) + .with_context(|| format!("Failed to re-parse MCP config: {}", path.display()))? + } else { + McpConfig::default() + }; + let new_hash = hash_mcp_config(&new_config); + // Always advance last_mtime so a touched-but-unchanged file doesn't + // make us re-read on every subsequent call. + self.last_mtime = Some(current_mtime); + if new_hash == self.config_hash { + return Ok(false); + } + // Real content change — drop all live connections so the next + // get_or_connect picks up the new config (sandbox flags, env, args). + self.connections.clear(); + self.config = new_config; + self.config_hash = new_hash; + Ok(true) + } + /// Get or create a connection to a server pub async fn get_or_connect(&mut self, server_name: &str) -> Result<&mut McpConnection> { + // Lazy auto-reload (#1267 part 2): cheap mtime-then-hash check before + // each connection lookup. Errors from the reload check (stat failure, + // partial config parse) are swallowed here so a transient FS hiccup + // can't take down the whole tool dispatch — the user still gets the + // existing connection to respond to. + let _ = self.reload_if_config_changed().await; + let is_ready = self .connections .get(server_name) @@ -2014,6 +2090,28 @@ pub fn load_config(path: &Path) -> Result { .with_context(|| format!("Failed to parse MCP config {}", path.display())) } +/// 64-bit content hash of an [`McpConfig`]. Used by [`McpPool`] to decide +/// whether a freshly-read config differs from the one currently driving the +/// live connections. Hashing the JSON serialization avoids forcing every +/// nested config type to derive `Hash` (the timeouts struct, network policy +/// stubs, etc.). The hash is stable across runs of the same Rust toolchain +/// for byte-identical input. +fn hash_mcp_config(config: &McpConfig) -> u64 { + use std::hash::{Hash, Hasher}; + let bytes = serde_json::to_vec(config).unwrap_or_default(); + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + bytes.hash(&mut hasher); + hasher.finish() +} + +/// Best-effort fetch of the MCP config file's last-modified time. Returns +/// `None` when the file is missing, when stat fails, or when the platform +/// doesn't expose mtime — the lazy-reload check in `McpPool::get_or_connect` +/// treats `None` as "skip the check this turn". +fn mcp_config_mtime(path: &Path) -> Option { + fs::metadata(path).ok()?.modified().ok() +} + pub fn save_config(path: &Path, cfg: &McpConfig) -> Result<()> { validate_mcp_config_path(path)?; if let Some(parent) = path.parent() { @@ -2630,6 +2728,96 @@ mod tests { assert!(pool.all_tools().is_empty()); } + /// #1267 part 2: a pool built without a source path has no file to watch, + /// so `reload_if_config_changed` must short-circuit instead of trying + /// to stat `/`. + #[tokio::test] + async fn reload_if_config_changed_is_noop_without_source_path() { + let mut pool = McpPool::new(McpConfig::default()); + let reloaded = pool.reload_if_config_changed().await.unwrap(); + assert!(!reloaded, "no source path → no reload"); + } + + /// #1267 part 2: when the on-disk config is byte-unchanged, the lazy + /// reload must not drop connections — every call to `get_or_connect` + /// would otherwise pay a full reconnect cycle on networked filesystems + /// where mtime granularity is coarse. + #[tokio::test] + async fn reload_if_config_changed_skips_when_content_unchanged() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("mcp.json"); + std::fs::write(&path, r#"{"servers":{}}"#).unwrap(); + let mut pool = McpPool::from_config_path(&path).unwrap(); + // Force the mtime to advance without changing content. + std::thread::sleep(std::time::Duration::from_millis(10)); + std::fs::write(&path, r#"{"servers":{}}"#).unwrap(); + let reloaded = pool.reload_if_config_changed().await.unwrap(); + assert!( + !reloaded, + "content-unchanged config must not trigger a reload" + ); + } + + /// #1267 part 2: when the on-disk config changes content, the next + /// `reload_if_config_changed` call must swap in the new config and + /// (would) drop all live connections. We can't stand up a real + /// `McpConnection` in a unit test, so we observe the swap via the + /// publicly-readable side: server names go from empty to non-empty. + #[tokio::test] + async fn reload_if_config_changed_swaps_config_on_content_change() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("mcp.json"); + std::fs::write(&path, r#"{"servers":{}}"#).unwrap(); + let mut pool = McpPool::from_config_path(&path).unwrap(); + assert!(pool.server_names().is_empty()); + // Mutate the file so both the mtime and the hash change. + std::thread::sleep(std::time::Duration::from_millis(10)); + std::fs::write( + &path, + r#"{"servers":{"new":{"command":"echo","args":["hi"]}}}"#, + ) + .unwrap(); + let reloaded = pool.reload_if_config_changed().await.unwrap(); + assert!(reloaded, "content-changed config must trigger reload"); + let names = pool.server_names(); + assert!( + names.iter().any(|n| *n == "new"), + "expected new server in pool after reload, got {names:?}" + ); + } + + /// #1267 part 2: hash-based comparison must be stable for byte-identical + /// configs and distinct for differing configs. + #[test] + fn hash_mcp_config_is_stable_and_change_sensitive() { + let a = McpConfig::default(); + let b = McpConfig::default(); + assert_eq!(hash_mcp_config(&a), hash_mcp_config(&b)); + let mut c = McpConfig::default(); + c.servers.insert( + "x".into(), + McpServerConfig { + command: Some("/bin/echo".into()), + args: vec!["hi".into()], + env: Default::default(), + url: None, + connect_timeout: None, + execute_timeout: None, + read_timeout: None, + disabled: false, + enabled: true, + required: false, + enabled_tools: Vec::new(), + disabled_tools: Vec::new(), + }, + ); + assert_ne!( + hash_mcp_config(&a), + hash_mcp_config(&c), + "hash must change when servers map changes" + ); + } + /// #1319: discovered tools must be sorted by name so the prompt prefix /// is stable across runs (cache-hit stability), even when the server /// returns them in arbitrary or paginated order.