diff --git a/assets/settings/default.json b/assets/settings/default.json index 071c84c0005df18045799e5b666971728cad2f8e..e0316f2090ac1374e2a2d200c92bc39a93de77f8 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -2262,6 +2262,17 @@ "ssh_connections": [], // Whether to read ~/.ssh/config for ssh connection sources. "read_ssh_config": true, + // Default timeout in seconds for all context server tool calls. + // Individual servers can override this in their configuration. + // Examples: + // "context_servers": { + // "my-stdio-server": { + // "command": "/path/to/server", + // "timeout": 120 // Override: 2 minutes for this server + // }, + // } + // Default: 60 + "context_server_timeout": 60, // Configures context servers for use by the agent. "context_servers": {}, // Configures agent servers available in the agent panel. diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 0613002b5197b5ea6d4be7df852008ff7adaa035..06d6ef3963bfeec7fd69513328eeda1c76d43edc 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -306,6 +306,7 @@ impl AgentConnection for AcpConnection { project::context_server_store::ContextServerConfiguration::Http { url, headers, + timeout: _, } => Some(acp::McpServer::Http( acp::McpServerHttp::new(id.0.to_string(), url.to_string()).headers( headers diff --git a/crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs b/crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs index b30f1494f0d4dcbf3ef63cc7f549d16374f4899b..f37da34b8a735efaa9c07eab822e35a819691edf 100644 --- a/crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs +++ b/crates/agent_ui/src/agent_configuration/configure_context_server_modal.rs @@ -172,6 +172,7 @@ impl ConfigurationSource { enabled: true, url, headers: auth, + timeout: None, }, ) }) @@ -411,6 +412,7 @@ impl ConfigureContextServerModal { enabled: _, url, headers, + timeout: _, } => Some(ConfigurationTarget::ExistingHttp { id: server_id, url, diff --git a/crates/context_server/src/client.rs b/crates/context_server/src/client.rs index 605f24178916faa5173c32c28be6c80ee625cb6c..1981138328d264e40a50caaf36f23976b6b419cb 100644 --- a/crates/context_server/src/client.rs +++ b/crates/context_server/src/client.rs @@ -176,7 +176,7 @@ impl Client { .map(|name| name.to_string_lossy().into_owned()) .unwrap_or_else(String::new); - let timeout = binary.timeout.map(Duration::from_millis); + let timeout = binary.timeout.map(Duration::from_secs); let transport = Arc::new(StdioTransport::new(binary, working_directory, &cx)?); Self::new(server_id, server_name.into(), transport, timeout, cx) } diff --git a/crates/context_server/src/context_server.rs b/crates/context_server/src/context_server.rs index 92804549c69b01dd3729efb3a0b47905cd73d813..40d372db6876ef938f9ae32bdf7dd404b11af0d4 100644 --- a/crates/context_server/src/context_server.rs +++ b/crates/context_server/src/context_server.rs @@ -10,6 +10,7 @@ use collections::HashMap; use http_client::HttpClient; use std::path::Path; use std::sync::Arc; +use std::time::Duration; use std::{fmt::Display, path::PathBuf}; use anyhow::Result; @@ -39,6 +40,7 @@ pub struct ContextServer { id: ContextServerId, client: RwLock>>, configuration: ContextServerTransport, + request_timeout: Option, } impl ContextServer { @@ -54,6 +56,7 @@ impl ContextServer { command, working_directory.map(|directory| directory.to_path_buf()), ), + request_timeout: None, } } @@ -63,6 +66,7 @@ impl ContextServer { headers: HashMap, http_client: Arc, executor: gpui::BackgroundExecutor, + request_timeout: Option, ) -> Result { let transport = match endpoint.scheme() { "http" | "https" => { @@ -73,14 +77,23 @@ impl ContextServer { } _ => anyhow::bail!("unsupported MCP url scheme {}", endpoint.scheme()), }; - Ok(Self::new(id, transport)) + Ok(Self::new_with_timeout(id, transport, request_timeout)) } pub fn new(id: ContextServerId, transport: Arc) -> Self { + Self::new_with_timeout(id, transport, None) + } + + pub fn new_with_timeout( + id: ContextServerId, + transport: Arc, + request_timeout: Option, + ) -> Self { Self { id, client: RwLock::new(None), configuration: ContextServerTransport::Custom(transport), + request_timeout, } } @@ -113,7 +126,7 @@ impl ContextServer { client::ContextServerId(self.id.0.clone()), self.id().0, transport.clone(), - None, + self.request_timeout, cx.clone(), )?, }) diff --git a/crates/project/src/context_server_store.rs b/crates/project/src/context_server_store.rs index 7ba46a46872ba57c758baccf9f67b0039818ee75..03612a593a8fb4e08579b3b1effba5728ba2627c 100644 --- a/crates/project/src/context_server_store.rs +++ b/crates/project/src/context_server_store.rs @@ -2,6 +2,7 @@ pub mod extension; pub mod registry; use std::sync::Arc; +use std::time::Duration; use anyhow::{Context as _, Result}; use collections::{HashMap, HashSet}; @@ -18,6 +19,10 @@ use crate::{ worktree_store::WorktreeStore, }; +/// Maximum timeout for context server requests +/// Prevents extremely large timeout values from tying up resources indefinitely. +const MAX_TIMEOUT_SECS: u64 = 600; // 10 minutes + pub fn init(cx: &mut App) { extension::init(cx); } @@ -102,6 +107,7 @@ pub enum ContextServerConfiguration { Http { url: url::Url, headers: HashMap, + timeout: Option, }, } @@ -151,9 +157,14 @@ impl ContextServerConfiguration { enabled: _, url, headers: auth, + timeout, } => { let url = url::Url::parse(&url).log_err()?; - Some(ContextServerConfiguration::Http { url, headers: auth }) + Some(ContextServerConfiguration::Http { + url, + headers: auth, + timeout, + }) } } } @@ -250,7 +261,8 @@ impl ContextServerStore { this.available_context_servers_changed(cx); }), cx.observe_global::(|this, cx| { - let settings = Self::resolve_context_server_settings(&this.worktree_store, cx); + let settings = + &Self::resolve_project_settings(&this.worktree_store, cx).context_servers; if &this.context_server_settings == settings { return; } @@ -264,7 +276,8 @@ impl ContextServerStore { let mut this = Self { _subscriptions: subscriptions, - context_server_settings: Self::resolve_context_server_settings(&worktree_store, cx) + context_server_settings: Self::resolve_project_settings(&worktree_store, cx) + .context_servers .clone(), worktree_store, project: weak_project, @@ -482,17 +495,27 @@ impl ContextServerStore { configuration: Arc, cx: &mut Context, ) -> Result> { + let global_timeout = + Self::resolve_project_settings(&self.worktree_store, cx).context_server_timeout; + if let Some(factory) = self.context_server_factory.as_ref() { return Ok(factory(id, configuration)); } match configuration.as_ref() { - ContextServerConfiguration::Http { url, headers } => Ok(Arc::new(ContextServer::http( + ContextServerConfiguration::Http { + url, + headers, + timeout, + } => Ok(Arc::new(ContextServer::http( id, url, headers.clone(), cx.http_client(), cx.background_executor().clone(), + Some(Duration::from_secs( + timeout.unwrap_or(global_timeout).min(MAX_TIMEOUT_SECS), + )), )?)), _ => { let root_path = self @@ -511,19 +534,27 @@ impl ContextServerStore { }) }) }); - Ok(Arc::new(ContextServer::stdio( - id, - configuration.command().unwrap().clone(), - root_path, - ))) + + let mut command = configuration + .command() + .context("Missing command configuration for stdio context server")? + .clone(); + command.timeout = Some( + command + .timeout + .unwrap_or(global_timeout) + .min(MAX_TIMEOUT_SECS), + ); + + Ok(Arc::new(ContextServer::stdio(id, command, root_path))) } } } - fn resolve_context_server_settings<'a>( + fn resolve_project_settings<'a>( worktree_store: &'a Entity, cx: &'a App, - ) -> &'a HashMap, ContextServerSettings> { + ) -> &'a ProjectSettings { let location = worktree_store .read(cx) .visible_worktrees(cx) @@ -532,7 +563,7 @@ impl ContextServerStore { worktree_id: worktree.read(cx).id(), path: RelPath::empty(), }); - &ProjectSettings::get(location, cx).context_servers + ProjectSettings::get(location, cx) } fn update_server_state( @@ -1257,6 +1288,7 @@ mod tests { enabled: true, url: server_url.to_string(), headers: Default::default(), + timeout: None, }, )], ) @@ -1327,6 +1359,160 @@ mod tests { } } + #[gpui::test] + async fn test_context_server_global_timeout(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + SettingsStore::update_global(cx, |store, cx| { + store + .set_user_settings(r#"{"context_server_timeout": 90}"#, cx) + .expect("Failed to set test user settings"); + }); + }); + + let (_fs, project) = setup_context_server_test(cx, json!({"code.rs": ""}), vec![]).await; + + let registry = cx.new(|_| ContextServerDescriptorRegistry::new()); + let store = cx.new(|cx| { + ContextServerStore::test( + registry.clone(), + project.read(cx).worktree_store(), + project.downgrade(), + cx, + ) + }); + + let result = store.update(cx, |store, cx| { + store.create_context_server( + ContextServerId("test-server".into()), + Arc::new(ContextServerConfiguration::Http { + url: url::Url::parse("http://localhost:8080") + .expect("Failed to parse test URL"), + headers: Default::default(), + timeout: None, + }), + cx, + ) + }); + + assert!( + result.is_ok(), + "Server should be created successfully with global timeout" + ); + } + + #[gpui::test] + async fn test_context_server_per_server_timeout_override(cx: &mut TestAppContext) { + const SERVER_ID: &str = "test-server"; + + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + SettingsStore::update_global(cx, |store, cx| { + store + .set_user_settings(r#"{"context_server_timeout": 60}"#, cx) + .expect("Failed to set test user settings"); + }); + }); + + let (_fs, project) = setup_context_server_test( + cx, + json!({"code.rs": ""}), + vec![( + SERVER_ID.into(), + ContextServerSettings::Http { + enabled: true, + url: "http://localhost:8080".to_string(), + headers: Default::default(), + timeout: Some(120), + }, + )], + ) + .await; + + let registry = cx.new(|_| ContextServerDescriptorRegistry::new()); + let store = cx.new(|cx| { + ContextServerStore::test( + registry.clone(), + project.read(cx).worktree_store(), + project.downgrade(), + cx, + ) + }); + + let result = store.update(cx, |store, cx| { + store.create_context_server( + ContextServerId("test-server".into()), + Arc::new(ContextServerConfiguration::Http { + url: url::Url::parse("http://localhost:8080") + .expect("Failed to parse test URL"), + headers: Default::default(), + timeout: Some(120), + }), + cx, + ) + }); + + assert!( + result.is_ok(), + "Server should be created successfully with per-server timeout override" + ); + } + + #[gpui::test] + async fn test_context_server_stdio_timeout(cx: &mut TestAppContext) { + const SERVER_ID: &str = "stdio-server"; + + let (_fs, project) = setup_context_server_test( + cx, + json!({"code.rs": ""}), + vec![( + SERVER_ID.into(), + ContextServerSettings::Stdio { + enabled: true, + command: ContextServerCommand { + path: "/usr/bin/node".into(), + args: vec!["server.js".into()], + env: None, + timeout: Some(180000), + }, + }, + )], + ) + .await; + + let registry = cx.new(|_| ContextServerDescriptorRegistry::new()); + let store = cx.new(|cx| { + ContextServerStore::test( + registry.clone(), + project.read(cx).worktree_store(), + project.downgrade(), + cx, + ) + }); + + let result = store.update(cx, |store, cx| { + store.create_context_server( + ContextServerId("stdio-server".into()), + Arc::new(ContextServerConfiguration::Custom { + command: ContextServerCommand { + path: "/usr/bin/node".into(), + args: vec!["server.js".into()], + env: None, + timeout: Some(180000), + }, + }), + cx, + ) + }); + + assert!( + result.is_ok(), + "Stdio server should be created successfully with timeout" + ); + } + fn dummy_server_settings() -> ContextServerSettings { ContextServerSettings::Stdio { enabled: true, diff --git a/crates/project/src/project_settings.rs b/crates/project/src/project_settings.rs index 903e5f32b6eba985d846eb44b592d215d61517c0..50a2b675d282ee5a5549082fceadaa2bf49e8d51 100644 --- a/crates/project/src/project_settings.rs +++ b/crates/project/src/project_settings.rs @@ -59,6 +59,9 @@ pub struct ProjectSettings { /// Settings for context servers used for AI-related features. pub context_servers: HashMap, ContextServerSettings>, + /// Default timeout for context server requests in seconds. + pub context_server_timeout: u64, + /// Configuration for Diagnostics-related features. pub diagnostics: DiagnosticsSettings, @@ -141,6 +144,8 @@ pub enum ContextServerSettings { /// Optional authentication configuration for the remote server. #[serde(skip_serializing_if = "HashMap::is_empty", default)] headers: HashMap, + /// Timeout for tool calls in milliseconds. + timeout: Option, }, Extension { /// Whether the context server is enabled. @@ -167,10 +172,12 @@ impl From for ContextServerSettings { enabled, url, headers, + timeout, } => ContextServerSettings::Http { enabled, url, headers, + timeout, }, } } @@ -188,10 +195,12 @@ impl Into for ContextServerSettings { enabled, url, headers, + timeout, } => settings::ContextServerSettingsContent::Http { enabled, url, headers, + timeout, }, } } @@ -560,6 +569,7 @@ impl Settings for ProjectSettings { .into_iter() .map(|(key, value)| (key, value.into())) .collect(), + context_server_timeout: project.context_server_timeout.unwrap_or(60), lsp: project .lsp .clone() diff --git a/crates/settings/src/settings_content/project.rs b/crates/settings/src/settings_content/project.rs index 8ce7d64940004a1be76e383a71439fc7f725a077..d4f986a69417dca1fe5e6ce8a98028535e1709ab 100644 --- a/crates/settings/src/settings_content/project.rs +++ b/crates/settings/src/settings_content/project.rs @@ -54,6 +54,12 @@ pub struct ProjectSettingsContent { #[serde(default)] pub context_servers: HashMap, ContextServerSettingsContent>, + /// Default timeout in seconds for context server tool calls. + /// Can be overridden per-server in context_servers configuration. + /// + /// Default: 60 + pub context_server_timeout: Option, + /// Configuration for how direnv configuration should be loaded pub load_direnv: Option, @@ -234,6 +240,8 @@ pub enum ContextServerSettingsContent { /// Optional headers to send. #[serde(skip_serializing_if = "HashMap::is_empty", default)] headers: HashMap, + /// Timeout for tool calls in seconds. Defaults to global context_server_timeout if not specified. + timeout: Option, }, Extension { /// Whether the context server is enabled. @@ -275,7 +283,7 @@ pub struct ContextServerCommand { pub path: PathBuf, pub args: Vec, pub env: Option>, - /// Timeout for tool calls in milliseconds. Defaults to 60000 (60 seconds) if not specified. + /// Timeout for tool calls in seconds. Defaults to 60 if not specified. pub timeout: Option, } diff --git a/crates/settings/src/vscode_import.rs b/crates/settings/src/vscode_import.rs index 81096256a5159bb629677f5714dc56f03c98a90d..f251faca4d5a0b06faaba9f7bec4ba620b43b72e 100644 --- a/crates/settings/src/vscode_import.rs +++ b/crates/settings/src/vscode_import.rs @@ -403,6 +403,7 @@ impl VsCodeSettings { terminal: None, dap: Default::default(), context_servers: self.context_servers(), + context_server_timeout: None, load_direnv: None, slash_commands: None, git_hosting_providers: None, diff --git a/crates/settings_ui/src/page_data.rs b/crates/settings_ui/src/page_data.rs index e8884e2e02c6d55d44dd30415caa5ceeb11ad3b4..5fd19427c60b91afd06aacbcce4455780d4a120b 100644 --- a/crates/settings_ui/src/page_data.rs +++ b/crates/settings_ui/src/page_data.rs @@ -6193,6 +6193,22 @@ pub(crate) fn settings_data(cx: &App) -> Vec { metadata: None, files: USER, }), + SettingsPageItem::SectionHeader("Context Servers"), + SettingsPageItem::SettingItem(SettingItem { + title: "Context Server Timeout", + description: "Default timeout in seconds for context server tool calls. Can be overridden per-server in context_servers configuration.", + field: Box::new(SettingField { + json_path: Some("context_server_timeout"), + pick: |settings_content| { + settings_content.project.context_server_timeout.as_ref() + }, + write: |settings_content, value| { + settings_content.project.context_server_timeout = value; + }, + }), + metadata: None, + files: USER | PROJECT, + }), ]; items.extend(edit_prediction_language_settings_section()); items.extend(