diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d79eb52..8eaedc7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`ToolAnnotations` (`readOnlyHint`, `destructiveHint`, `idempotentHint`) plus top-level `Tool.title` on all 20 `#[tool]` definitions** in `mcp/server.rs` — MCP clients can now use these hints to decide when to skip confirmation dialogs. All 20 tools are marked `readOnlyHint=true`: mcpls has no write-back path today, so even `rename_symbol`, `format_document`, and `get_code_actions` only return a proposed edit rather than applying one — revisit their classification if a write-back path is added. (#136) - **Shared `PositionParams`/`RangeParams` structs** in `mcp/tools.rs`, embedded via `#[serde(flatten)]` in the eleven tool-parameter structs that previously repeated the `file_path`/`line`/`character` trio or the `start_line`/`start_character`/`end_line`/`end_character` quad verbatim. The MCP wire format (flat JSON) and generated JSON schema are unchanged. (#235) +- **`LspServerConfig::request_timeout_seconds`** — per-request LSP timeout, configurable per server and separate from the handshake-only `timeout_seconds`. Defaults to 30s (bit-identical to the previous hardcoded behavior). Bounds a single request attempt, not a whole tool call: on a `-32802` (`ServerCancelled`) response, `LspClient::request` retries up to 4 attempts total, so the worst-case latency for one tool call is `4 * request_timeout_seconds + 3.5s`. `LspClient::request_timeout()`/`completion_timeout()` accessors expose the effective value; `completion_timeout()` clamps to at most 10s regardless of the configured value — an explicit MVP ceiling, not an oversight. See `docs/user-guide/configuration.md#request_timeout_seconds`. (#267) - **`Error::CapabilityNotSupported`** — new `Error` variant returned when the LSP server routed for a request does not advertise the `ServerCapabilities` field a capability-gated tool needs. (#240) - **In-band notice when a project-local `mcpls.toml` is ignored as untrusted** — `ServerInfo.instructions` (`McplsServer::get_info`) now appends a note when a CWD-discovered `./mcpls.toml` was skipped because it wasn't trusted, so MCP clients that swallow stderr (where the existing `tracing::warn!` goes) can still see the ignore decision and act on it. New `ServerConfig::project_config_ignored` field (load-time metadata, not TOML-configurable) carries the signal from `ServerConfig::load_with_trust` through to the MCP layer. (#248) - **Automatic LSP server respawn on crash** — `Translator` now detects when a routed LSP server's child process has exited and transparently respawns and re-initializes it before resolving the next tool call for that server, instead of leaving the session degraded until mcpls itself is restarted. Concurrent callers that observe the same dead server single-flight on a per-server lock so only one respawn happens; any requests still parked on the old connection are failed immediately instead of waiting out their own timeout. A crash-looping server backs off exponentially (1s up to 30s) instead of retrying on every tool call — including the more realistic "starts, initializes, then dies again a moment later" loop, not just an outright spawn failure. New `LspServer::has_exited`, `DocumentTracker::forget_server` (resets per-server document sync state; the commit is checked against a per-server sync generation while still holding the document lock, so an in-flight sync against the old connection can never land after a concurrent respawn clears it), and `Translator::with_notification_cache` (lets the respawn path invalidate the crashed connection's cached diagnostics — triggered only when the crashed server was the diagnostics route for its language, not for a non-route server like a dedicated hover server; note that the clear itself is currently workspace-wide across *all* languages, not scoped to just the crashed one, since the diagnostics cache has no per-language clear yet — a crashed rust-analyzer also clears a healthy pyright's cached entries, though `handle_diagnostics`'s authoritative pull path is unaffected, only the cached-only path degrades until that server republishes). A respawned server's own push notifications are drained and discarded rather than reconnected to the existing notification pump — diagnostics push does not resume for it until the whole mcpls process restarts, a known scope trade-off. New `Error::ServerUnavailable` variant distinguishes "respawn could not proceed" (no config registered, or backing off) from a plain `Error::ServerTerminated`. (#249) @@ -18,7 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Sort `[workspace.dependencies]` in root `Cargo.toml` alphabetically (#232) -- **Shared `DEFAULT_LSP_TIMEOUT` constant** — the 16 handler methods in `bridge::translator` that each duplicated `Duration::from_secs(30)` now share one module-level constant; `get_completions`'s intentionally shorter timeout is now the named `COMPLETIONS_LSP_TIMEOUT` constant. No behavior change. (#231) +- **`bridge::translator`'s fixed `DEFAULT_LSP_TIMEOUT`/`COMPLETIONS_LSP_TIMEOUT` constants (added in #231 below) removed** in favor of the new per-server `request_timeout_seconds` config field (see Added) — all 17 call sites now read `client.request_timeout()`/`client.completion_timeout()`. Breaking change: `LspServerConfig` gained a field, so existing `LspServerConfig { .. }` struct-literal construction (not behind `#[non_exhaustive]`) must add `request_timeout_seconds`. Also breaking: `ServerConfig::validate()` now rejects `timeout_seconds == 0` in addition to the new `request_timeout_seconds == 0` check — no working config could previously set `timeout_seconds` to 0 (it made `initialize` fail instantly), so no functioning setup is affected. (#267) +- **Shared `DEFAULT_LSP_TIMEOUT` constant** — the 16 handler methods in `bridge::translator` that each duplicated `Duration::from_secs(30)` now share one module-level constant; `get_completions`'s intentionally shorter timeout is now the named `COMPLETIONS_LSP_TIMEOUT` constant. No behavior change. Superseded by #267 above, which replaces both constants with the configurable `request_timeout_seconds`. (#231) - Regression test pinning RFC 3986 §2.2 percent-encoding of `[`, `]`, `^`, `|`, `{`, `}`, and backtick in the `try_path_to_uri`/`encode_rfc3986_path_chars` `file://` URI conversion; confirms the `url` crate already encodes `{`, `}`, and backtick, so no code change was needed for those three. Scope: covers `file://` URI conversion only — `bridge::resources`'s separate `lsp-diagnostics://` URI construction does not call `encode_rfc3986_path_chars` and is not covered here. (#168) - Unit tests for `LspClient::should_retrigger` and its wiring into the `ServerCancelled` (-32802) retry loop: full retry exhaustion returns the original error, `retriggerRequest: false` returns immediately without retrying, and a cancelled-then-successful retry resolves normally. (#161) - **`HttpConfig` gains request body and session limits** — Breaking change: HTTP transport now caps POST request body size (`413 Payload Too Large` on overflow, wired into `rmcp`'s built-in body-size enforcement) and concurrent HTTP sessions. The session cap is a hard bound enforced atomically at session creation by a semaphore-backed `SessionManager` wrapper (not inferred from request headers, which cannot reliably distinguish session-creating requests across `rmcp`'s legacy and stateless protocol paths); requests rejected once the cap is reached receive `429 Too Many Requests` with a `Retry-After` header. `HttpConfig` gains a `new(bind, path)` constructor plus `with_max_request_body_bytes`/`with_max_concurrent_sessions` builders and is now `#[non_exhaustive]`; existing `HttpConfig { .. }` struct-literal construction must switch to `HttpConfig::new(..)`. (#243) diff --git a/README.md b/README.md index b0c232a9..9dd0e418 100644 --- a/README.md +++ b/README.md @@ -251,6 +251,7 @@ command = "rust-analyzer" args = [] file_patterns = ["**/*.rs"] timeout_seconds = 30 +request_timeout_seconds = 30 [lsp_servers.heuristics] project_markers = ["Cargo.toml", "rust-toolchain.toml"] diff --git a/crates/mcpls-core/src/bridge/translator.rs b/crates/mcpls-core/src/bridge/translator.rs index 40d39ebd..0ba09604 100644 --- a/crates/mcpls-core/src/bridge/translator.rs +++ b/crates/mcpls-core/src/bridge/translator.rs @@ -745,16 +745,6 @@ const MAX_POSITION_VALUE: u32 = 1_000_000; /// Maximum allowed range size in lines. const MAX_RANGE_LINES: u32 = 10_000; -/// Default timeout for most LSP request/response round trips. -/// -/// Independent of `LspServerConfig::timeout_seconds`, which only bounds the -/// `initialize` handshake (see `lsp::lifecycle`) and has no effect on -/// per-request timeouts. There is currently no way to configure this value. -const DEFAULT_LSP_TIMEOUT: Duration = Duration::from_secs(30); -/// Timeout for `textDocument/completion`, kept shorter than -/// [`DEFAULT_LSP_TIMEOUT`] since completions are latency-sensitive. -const COMPLETIONS_LSP_TIMEOUT: Duration = Duration::from_secs(10); - /// Validate that `path` is within one of `workspace_roots`. /// /// Free function (rather than a `Translator` method) so callers that only need @@ -1347,7 +1337,7 @@ impl Translator { }; let response: Option = client - .request("textDocument/hover", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/hover", params, client.request_timeout()) .await?; let result = match response { @@ -1402,7 +1392,7 @@ impl Translator { }; let response: Option = client - .request("textDocument/definition", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/definition", params, client.request_timeout()) .await?; let locations = match response { @@ -1472,7 +1462,7 @@ impl Translator { }; let response: Option> = client - .request("textDocument/references", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/references", params, client.request_timeout()) .await?; let locations = response.unwrap_or_default(); @@ -1528,7 +1518,7 @@ impl Translator { let params = diagnostic_request_params(TextDocumentIdentifier { uri: uri.clone() }); let pull_response: Result = client - .request("textDocument/diagnostic", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/diagnostic", params, client.request_timeout()) .await; let diag_info = { @@ -1596,7 +1586,7 @@ impl Translator { }; let response: Option = client - .request("textDocument/rename", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/rename", params, client.request_timeout()) .await?; let changes = if let Some(edit) = response { @@ -1699,7 +1689,11 @@ impl Translator { }; let response: Option = client - .request("textDocument/completion", params, COMPLETIONS_LSP_TIMEOUT) + .request( + "textDocument/completion", + params, + client.completion_timeout(), + ) .await?; let items = match response { @@ -1757,7 +1751,11 @@ impl Translator { }; let response: Option = client - .request("textDocument/documentSymbol", params, DEFAULT_LSP_TIMEOUT) + .request( + "textDocument/documentSymbol", + params, + client.request_timeout(), + ) .await?; let symbols = match response { @@ -1817,7 +1815,7 @@ impl Translator { }; let response: Option> = client - .request("textDocument/formatting", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/formatting", params, client.request_timeout()) .await?; let edits = response.unwrap_or_default(); @@ -1898,7 +1896,7 @@ impl Translator { }; let response: Option> = client - .request("workspace/symbol", params, DEFAULT_LSP_TIMEOUT) + .request("workspace/symbol", params, client.request_timeout()) .await?; let mut symbols: Vec = response @@ -1993,7 +1991,7 @@ impl Translator { }; let response: Option = client - .request("textDocument/codeAction", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/codeAction", params, client.request_timeout()) .await?; let response_vec = response.unwrap_or_default(); let mut actions = Vec::with_capacity(response_vec.len()); @@ -2070,7 +2068,7 @@ impl Translator { .request( "textDocument/prepareCallHierarchy", params, - DEFAULT_LSP_TIMEOUT, + client.request_timeout(), ) .await?; @@ -2119,7 +2117,11 @@ impl Translator { }; let response: Option> = client - .request("callHierarchy/incomingCalls", params, DEFAULT_LSP_TIMEOUT) + .request( + "callHierarchy/incomingCalls", + params, + client.request_timeout(), + ) .await?; // Pre-allocate and build result @@ -2176,7 +2178,11 @@ impl Translator { }; let response: Option> = client - .request("callHierarchy/outgoingCalls", params, DEFAULT_LSP_TIMEOUT) + .request( + "callHierarchy/outgoingCalls", + params, + client.request_timeout(), + ) .await?; // Pre-allocate and build result @@ -2416,7 +2422,11 @@ impl Translator { }; let response: Option = client - .request("textDocument/signatureHelp", params, DEFAULT_LSP_TIMEOUT) + .request( + "textDocument/signatureHelp", + params, + client.request_timeout(), + ) .await?; let result = match response { @@ -2498,7 +2508,11 @@ impl Translator { }; let response: Option = client - .request("textDocument/implementation", params, DEFAULT_LSP_TIMEOUT) + .request( + "textDocument/implementation", + params, + client.request_timeout(), + ) .await?; Ok(LocationsResult { @@ -2549,7 +2563,11 @@ impl Translator { }; let response: Option = client - .request("textDocument/typeDefinition", params, DEFAULT_LSP_TIMEOUT) + .request( + "textDocument/typeDefinition", + params, + client.request_timeout(), + ) .await?; Ok(LocationsResult { @@ -2603,7 +2621,7 @@ impl Translator { }; let response: Option> = client - .request("textDocument/inlayHint", params, DEFAULT_LSP_TIMEOUT) + .request("textDocument/inlayHint", params, client.request_timeout()) .await?; let hints = response @@ -3264,6 +3282,7 @@ sleep __SLEEP__ file_patterns: vec![], initialization_options: None, timeout_seconds: 5, + request_timeout_seconds: 5, heuristics: None, name: Some(id.to_string()), handles: None, @@ -3585,6 +3604,7 @@ fi file_patterns: vec![], initialization_options: None, timeout_seconds: 5, + request_timeout_seconds: 5, heuristics: None, name: Some("hover-only".to_string()), handles: Some(vec![ToolKind::Hover]), @@ -3597,6 +3617,7 @@ fi file_patterns: vec![], initialization_options: None, timeout_seconds: 5, + request_timeout_seconds: 5, heuristics: None, name: Some("diag-catchall".to_string()), handles: None, @@ -3829,6 +3850,7 @@ fi file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: Some("pyright".to_string()), handles: Some(vec![ToolKind::Hover]), @@ -5155,6 +5177,7 @@ fi file_patterns: vec!["**/*.tsx".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -5201,6 +5224,7 @@ fi file_patterns: vec!["**/*.js".to_string(), "**/*.jsx".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -5586,6 +5610,7 @@ fi file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: Some("pyright".to_string()), handles: Some(vec![ToolKind::Hover]), @@ -5598,6 +5623,7 @@ fi file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: Some("pylsp".to_string()), handles: Some(vec![ToolKind::Diagnostics]), diff --git a/crates/mcpls-core/src/config/mod.rs b/crates/mcpls-core/src/config/mod.rs index 5226aafe..5ed1fc4e 100644 --- a/crates/mcpls-core/src/config/mod.rs +++ b/crates/mcpls-core/src/config/mod.rs @@ -520,6 +520,18 @@ impl ServerConfig { server.language_id ))); } + if server.timeout_seconds == 0 { + return Err(Error::InvalidConfig(format!( + "timeout_seconds cannot be 0 for language '{}'", + server.language_id + ))); + } + if server.request_timeout_seconds == 0 { + return Err(Error::InvalidConfig(format!( + "request_timeout_seconds cannot be 0 for language '{}'", + server.language_id + ))); + } if let Some(name) = &server.name { if name.is_empty() { return Err(Error::InvalidConfig(format!( @@ -638,6 +650,77 @@ mod tests { assert_eq!(config.lsp_servers[0].language_id, "rust"); } + #[test] + fn test_load_from_toml_without_request_timeout_seconds_defaults_to_thirty() { + // Mirrors the shape of every auto-generated pre-#267 config file: + // `timeout_seconds` present, `request_timeout_seconds` absent. + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("config.toml"); + + let toml_content = r#" + [[lsp_servers]] + language_id = "rust" + command = "rust-analyzer" + timeout_seconds = 30 + "#; + + fs::write(&config_path, toml_content).unwrap(); + + let config = ServerConfig::load_from(&config_path).unwrap(); + assert_eq!(config.lsp_servers[0].request_timeout_seconds, 30); + } + + #[test] + fn test_validate_rejects_zero_timeout_seconds() { + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("config.toml"); + + let toml_content = r#" + [[lsp_servers]] + language_id = "rust" + command = "rust-analyzer" + timeout_seconds = 0 + "#; + + fs::write(&config_path, toml_content).unwrap(); + + let result = ServerConfig::load_from(&config_path); + if let Err(Error::InvalidConfig(msg)) = result { + // `contains("timeout_seconds cannot be 0")` would also match the + // `request_timeout_seconds` message below (it ends in the same + // suffix), so assert the exact message to actually discriminate + // which field triggered the error. + assert_eq!(msg, "timeout_seconds cannot be 0 for language 'rust'"); + } else { + panic!("Expected InvalidConfig error, got {result:?}"); + } + } + + #[test] + fn test_validate_rejects_zero_request_timeout_seconds() { + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("config.toml"); + + let toml_content = r#" + [[lsp_servers]] + language_id = "rust" + command = "rust-analyzer" + request_timeout_seconds = 0 + "#; + + fs::write(&config_path, toml_content).unwrap(); + + let result = ServerConfig::load_from(&config_path); + if let Err(Error::InvalidConfig(msg)) = result { + assert_eq!( + msg, + "request_timeout_seconds cannot be 0 for language 'rust'" + ); + } else { + panic!("Expected InvalidConfig error, got {result:?}"); + } + } + #[test] fn test_load_from_nonexistent_file() { let result = ServerConfig::load_from(Path::new("/nonexistent/config.toml")); @@ -995,6 +1078,7 @@ mod tests { file_patterns: vec!["**/*.c".to_string(), "**/*.h".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -1019,6 +1103,7 @@ mod tests { file_patterns: vec!["**/*.ts".to_string(), "**/*.tsx".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -1043,6 +1128,7 @@ mod tests { file_patterns: vec!["**/*.js".to_string(), "**/*.jsx".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -1067,6 +1153,7 @@ mod tests { file_patterns: vec!["**/*".to_string(), "**/*.{h,hpp}".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, diff --git a/crates/mcpls-core/src/config/routing.rs b/crates/mcpls-core/src/config/routing.rs index c9b459af..30b19c41 100644 --- a/crates/mcpls-core/src/config/routing.rs +++ b/crates/mcpls-core/src/config/routing.rs @@ -504,6 +504,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: name.map(str::to_string), handles, @@ -593,6 +594,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, @@ -605,6 +607,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, diff --git a/crates/mcpls-core/src/config/server.rs b/crates/mcpls-core/src/config/server.rs index e8961be6..8a025ad5 100644 --- a/crates/mcpls-core/src/config/server.rs +++ b/crates/mcpls-core/src/config/server.rs @@ -170,10 +170,26 @@ pub struct LspServerConfig { #[serde(default)] pub initialization_options: Option, - /// Request timeout in seconds. + /// Handshake timeout in seconds: bounds the `initialize` request during + /// server startup. Does not affect individual tool-call requests sent + /// after initialization; see [`Self::request_timeout_seconds`] for that. + /// The LSP server's `shutdown` request during teardown uses a separate, + /// fixed 5-second timeout that is not configurable by this field. #[serde(default = "default_timeout")] pub timeout_seconds: u64, + /// Per-request timeout in seconds, applied to each LSP request issued + /// while translating an MCP tool call (hover, definition, references, etc.). + /// + /// This bounds a single request attempt, not a whole tool call: on a + /// `-32802` (content modified) response, [`crate::lsp::LspClient::request`] + /// retries up to 4 attempts with backoff, so the worst-case latency for one + /// tool call is `4 * request_timeout_seconds + 3.5` seconds. Completion + /// requests are further capped at 10 seconds regardless of this value; see + /// [`crate::lsp::LspClient::completion_timeout`]. + #[serde(default = "default_request_timeout")] + pub request_timeout_seconds: u64, + /// Heuristics for determining if this server should be spawned. /// If not specified, the server will always attempt to spawn. #[serde(default)] @@ -202,6 +218,10 @@ const fn default_timeout() -> u64 { 30 } +const fn default_request_timeout() -> u64 { + 30 +} + impl LspServerConfig { /// Check if this server should be spawned for the given workspace. /// @@ -241,6 +261,7 @@ impl LspServerConfig { file_patterns: vec!["**/*.rs".to_string()], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers([ "Cargo.toml", "rust-toolchain.toml", @@ -261,6 +282,7 @@ impl LspServerConfig { file_patterns: vec!["**/*.py".to_string()], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers([ "pyproject.toml", "setup.py", @@ -283,6 +305,7 @@ impl LspServerConfig { file_patterns: vec!["**/*.ts".to_string(), "**/*.tsx".to_string()], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers([ "package.json", "tsconfig.json", @@ -304,6 +327,7 @@ impl LspServerConfig { file_patterns: vec!["**/*.go".to_string()], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers(["go.mod", "go.sum"])), name: None, handles: None, @@ -326,6 +350,7 @@ impl LspServerConfig { ], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers([ "CMakeLists.txt", "compile_commands.json", @@ -348,6 +373,7 @@ impl LspServerConfig { file_patterns: vec!["**/*.zig".to_string()], initialization_options: None, timeout_seconds: default_timeout(), + request_timeout_seconds: default_request_timeout(), heuristics: Some(ServerHeuristics::with_markers([ "build.zig", "build.zig.zon", @@ -422,6 +448,7 @@ mod tests { file_patterns: vec!["**/*.custom".to_string()], initialization_options: Some(serde_json::json!({"key": "value"})), timeout_seconds: 60, + request_timeout_seconds: 45, heuristics: None, name: None, handles: None, @@ -447,6 +474,15 @@ mod tests { assert_eq!(deserialized.command, original.command); assert_eq!(deserialized.args, original.args); assert_eq!(deserialized.timeout_seconds, original.timeout_seconds); + assert_eq!( + deserialized.request_timeout_seconds, + original.request_timeout_seconds + ); + } + + #[test] + fn test_default_request_timeout() { + assert_eq!(default_request_timeout(), 30); } #[test] @@ -530,6 +566,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, diff --git a/crates/mcpls-core/src/lib.rs b/crates/mcpls-core/src/lib.rs index f8eaf478..1cf4e1ee 100644 --- a/crates/mcpls-core/src/lib.rs +++ b/crates/mcpls-core/src/lib.rs @@ -1072,6 +1072,7 @@ mod tests { file_patterns: vec!["**/*.rs".to_string()], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, diff --git a/crates/mcpls-core/src/lsp/client.rs b/crates/mcpls-core/src/lsp/client.rs index 742520cf..a33412d8 100644 --- a/crates/mcpls-core/src/lsp/client.rs +++ b/crates/mcpls-core/src/lsp/client.rs @@ -31,6 +31,15 @@ const SERVER_CANCELLED_MAX_RETRIES: u32 = 3; /// Initial backoff delay for server-cancelled retries (milliseconds). const SERVER_CANCELLED_INITIAL_DELAY_MS: u64 = 500; +/// Upper bound on the effective timeout for completion requests, regardless +/// of `request_timeout_seconds`. +/// +/// Completions are latency-sensitive: a completion list that takes longer +/// than this is no longer useful to the caller. This is a deliberate MVP +/// ceiling, not an oversight — completions cannot be configured above this +/// value today. See [`LspClient::completion_timeout`]. +const COMPLETION_TIMEOUT_CAP: Duration = Duration::from_secs(10); + /// Type alias for pending request tracking map. type PendingRequests = HashMap>>; @@ -194,6 +203,70 @@ impl LspClient { *self.state.lock().await } + /// The timeout applied to a single LSP request attempt, derived from + /// [`LspServerConfig::request_timeout_seconds`]. + /// + /// This bounds one attempt, not a whole tool call: [`Self::request`] + /// retries up to `SERVER_CANCELLED_MAX_RETRIES` (3) additional times on a + /// `-32802` (`ServerCancelled`) response, so the worst-case latency for a + /// single tool call is `4 * request_timeout() + 3.5s` (the sum of the + /// retry backoff delays). + /// + /// The configured value is clamped to at least 1 second. [`ServerConfig::load_from`] + /// rejects `request_timeout_seconds == 0` at load time for TOML-sourced + /// configs, but [`crate::serve`] accepts a caller-built `ServerConfig` + /// without going through that validation, so this clamp is the last line + /// of defense against a zero-duration timeout that would fail every + /// request instantly. + /// + /// [`ServerConfig::load_from`]: crate::config::ServerConfig::load_from + /// + /// # Examples + /// + /// ``` + /// use std::time::Duration; + /// use mcpls_core::config::LspServerConfig; + /// use mcpls_core::lsp::LspClient; + /// + /// let mut config = LspServerConfig::rust_analyzer(); + /// config.request_timeout_seconds = 45; + /// let client = LspClient::new(config); + /// + /// assert_eq!(client.request_timeout(), Duration::from_secs(45)); + /// ``` + #[must_use] + pub fn request_timeout(&self) -> Duration { + Duration::from_secs(self.config.request_timeout_seconds.max(1)) + } + + /// The timeout applied to completion (`textDocument/completion`) requests. + /// + /// Equal to [`Self::request_timeout`], capped at 10 seconds. Completions + /// cannot be configured above this cap by any + /// value of `request_timeout_seconds` — if that proves insufficient in + /// practice, the fix is a dedicated `completion_timeout_seconds` field, + /// not raising this cap. + /// + /// # Examples + /// + /// ``` + /// use std::time::Duration; + /// use mcpls_core::config::LspServerConfig; + /// use mcpls_core::lsp::LspClient; + /// + /// let mut config = LspServerConfig::rust_analyzer(); + /// config.request_timeout_seconds = 300; + /// let client = LspClient::new(config); + /// + /// // Capped at 10s even though request_timeout_seconds is 300. + /// assert_eq!(client.completion_timeout(), Duration::from_secs(10)); + /// assert!(client.completion_timeout() <= client.request_timeout()); + /// ``` + #[must_use] + pub fn completion_timeout(&self) -> Duration { + self.request_timeout().min(COMPLETION_TIMEOUT_CAP) + } + /// Send request and wait for response with timeout. /// /// Automatically retries up to 3 times when the server returns error code @@ -605,6 +678,55 @@ mod tests { ); } + #[test] + fn test_request_timeout_and_completion_timeout_at_default() { + let config = LspServerConfig::rust_analyzer(); + let client = LspClient::new(config); + + assert_eq!(client.request_timeout(), Duration::from_secs(30)); + assert_eq!(client.completion_timeout(), Duration::from_secs(10)); + } + + #[test] + fn test_completion_timeout_clamps_to_ten_seconds() { + for secs in [1, 2, 3, 30, 300] { + let mut config = LspServerConfig::rust_analyzer(); + config.request_timeout_seconds = secs; + let client = LspClient::new(config); + + assert_eq!( + client.completion_timeout(), + Duration::from_secs(secs.min(10)), + "request_timeout_seconds={secs}" + ); + assert!(client.completion_timeout() <= client.request_timeout()); + } + } + + #[test] + fn test_request_timeout_clamps_zero_to_one_second() { + let mut config = LspServerConfig::rust_analyzer(); + config.request_timeout_seconds = 0; + let client = LspClient::new(config); + + assert_eq!(client.request_timeout(), Duration::from_secs(1)); + assert_eq!(client.completion_timeout(), Duration::from_secs(1)); + } + + #[test] + fn test_request_timeout_independent_per_server() { + let mut config_a = LspServerConfig::rust_analyzer(); + config_a.request_timeout_seconds = 5; + let mut config_b = LspServerConfig::pyright(); + config_b.request_timeout_seconds = 15; + + let client_a = LspClient::new(config_a); + let client_b = LspClient::new(config_b); + + assert_eq!(client_a.request_timeout(), Duration::from_secs(5)); + assert_eq!(client_b.request_timeout(), Duration::from_secs(15)); + } + #[test] fn test_register_capability_request_is_acknowledged() { let request = JsonRpcRequest { diff --git a/crates/mcpls-core/src/lsp/lifecycle.rs b/crates/mcpls-core/src/lsp/lifecycle.rs index e435ec63..79fad390 100644 --- a/crates/mcpls-core/src/lsp/lifecycle.rs +++ b/crates/mcpls-core/src/lsp/lifecycle.rs @@ -465,7 +465,11 @@ impl LspServer { .request( "initialize", params, - Duration::from_secs(config.server_config.timeout_seconds), + // `.max(1)` guards the same gap as `LspClient::request_timeout`'s + // clamp: `serve()` accepts a caller-built `ServerConfig` without + // going through `ServerConfig::validate()`, so `0` is reachable + // here and would otherwise produce a 0-duration timeout. + Duration::from_secs(config.server_config.timeout_seconds.max(1)), ) .await .map_err(|e| Error::LspInitFailed { @@ -919,6 +923,7 @@ mod tests { file_patterns: vec!["**/*.py".to_string()], initialization_options: Some(init_opts.clone()), timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1401,6 +1406,7 @@ mod tests { file_patterns: vec!["**/*.rs".to_string()], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1436,6 +1442,7 @@ mod tests { file_patterns: vec!["**/*.rs".to_string()], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1453,6 +1460,7 @@ mod tests { file_patterns: vec!["**/*.py".to_string()], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1470,6 +1478,7 @@ mod tests { file_patterns: vec!["**/*.ts".to_string()], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1510,6 +1519,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1527,6 +1537,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1560,6 +1571,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1577,6 +1589,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 10, + request_timeout_seconds: 10, heuristics: None, name: None, handles: None, @@ -1605,6 +1618,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 5, + request_timeout_seconds: 5, heuristics: None, name: None, handles: None, @@ -1741,6 +1755,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: Some("pyright-diag".to_string()), handles: Some(vec![ToolKind::Diagnostics]), @@ -1753,6 +1768,7 @@ mod tests { file_patterns: vec![], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: Some("pylsp".to_string()), handles: None, diff --git a/crates/mcpls-core/tests/integration/rust_analyzer_tests.rs b/crates/mcpls-core/tests/integration/rust_analyzer_tests.rs index 9e7e9eec..46c9a083 100644 --- a/crates/mcpls-core/tests/integration/rust_analyzer_tests.rs +++ b/crates/mcpls-core/tests/integration/rust_analyzer_tests.rs @@ -55,6 +55,7 @@ async fn setup_rust_analyzer() -> Arc> { file_patterns: vec!["**/*.rs".to_string()], initialization_options: None, timeout_seconds: 30, + request_timeout_seconds: 30, heuristics: None, name: None, handles: None, diff --git a/docs/user-guide/configuration.md b/docs/user-guide/configuration.md index ad9537f6..bec82ac9 100644 --- a/docs/user-guide/configuration.md +++ b/docs/user-guide/configuration.md @@ -64,6 +64,7 @@ command = "rust-analyzer" args = [] file_patterns = ["**/*.rs"] timeout_seconds = 30 +request_timeout_seconds = 30 # Optional: LSP server initialization options [lsp_servers.initialization_options] @@ -272,21 +273,59 @@ Glob pattern syntax: **Type**: Integer **Default**: `30` -Timeout in seconds for the initial `initialize` handshake only. Servers that -load a large project before answering `initialize` (e.g. OmniSharp on a big -Unity/C# solution) need this raised - the default 30 s can otherwise cut the -server off mid-initialization. +Timeout in seconds for the `initialize` handshake during server startup. +Servers that load a large project before answering `initialize` (e.g. +OmniSharp on a big Unity/C# solution) need this raised - the default 30 s can +otherwise cut the server off mid-initialization. This does **not** bound individual tool-call requests (hover, definition, -references, etc.) sent after initialization - those use a fixed internal -timeout (30 s for most requests, 10 s for completions) that `timeout_seconds` -does not affect. +references, etc.) sent after initialization - see `request_timeout_seconds` +below for that. The LSP server's `shutdown` request during teardown uses a +separate, fixed 5 s timeout that is not configurable. ```toml [[lsp_servers]] timeout_seconds = 60 # Increase for servers slow to complete `initialize` ``` +### `request_timeout_seconds` + +**Type**: Integer +**Default**: `30` + +Timeout in seconds applied to each individual LSP request issued while +translating an MCP tool call (hover, definition, references, diagnostics, +rename, etc.). Independent of `timeout_seconds`, which only bounds the +`initialize` handshake. + +This bounds a single request **attempt**, not a whole tool call: when the LSP +server responds with `-32802` (content modified), mcpls retries up to 4 +attempts total with exponential backoff (0.5 s + 1 s + 2 s = 3.5 s of total +sleep). So the worst-case latency for one tool call is: + +``` +4 * request_timeout_seconds + 3.5 seconds +``` + +If a tool call also triggers a server respawn (because the previous server +process had died), add `timeout_seconds` on top of that, since +`initialize` runs again before the request is retried. + +Completion requests (`textDocument/completion`) are further capped at 10 +seconds regardless of this setting - completions are latency-sensitive +enough that a slower result isn't useful, and this cap cannot currently be +raised. If completions specifically need a higher ceiling, file an issue +requesting a dedicated `completion_timeout_seconds` field rather than raising +`request_timeout_seconds`, which would not affect completions above 10 s. + +A value of `0` is rejected at config load time; the effective timeout is +always at least 1 second. + +```toml +[[lsp_servers]] +request_timeout_seconds = 60 # Increase for a slow LSP server (e.g. large monorepo indexing) +``` + ### `initialization_options` **Type**: Table (key-value pairs) @@ -723,7 +762,8 @@ language_id = "rust" command = "rust-analyzer" args = [] file_patterns = ["**/*.rs"] -timeout_seconds = 120 # 2 minutes for initial indexing +timeout_seconds = 120 # 2 minutes for initial indexing +request_timeout_seconds = 60 # slower tool-call responses (see the field's docs above for the retry-ceiling math) ``` ### Multiple Workspaces diff --git a/docs/user-guide/tools-reference.md b/docs/user-guide/tools-reference.md index 15a04b9f..3ab2126d 100644 --- a/docs/user-guide/tools-reference.md +++ b/docs/user-guide/tools-reference.md @@ -1136,7 +1136,7 @@ Common error scenarios: | LSP server not available | No server configured for file type | Add LSP server to config | | File not found | File doesn't exist | Check file path | | Position out of bounds | Invalid line/character | Verify position is valid | -| Timeout | LSP server too slow | Increase timeout in config | +| Timeout | LSP server too slow | Increase `request_timeout_seconds` in config | | No hover information | Not hoverable | Try different position | ## Performance Considerations @@ -1156,7 +1156,7 @@ Common error scenarios: ### Optimization Tips 1. Limit workspace roots to active projects -2. Increase timeouts for large codebases +2. Increase `request_timeout_seconds` for large codebases 3. Use file patterns to exclude build artifacts 4. Close unnecessary language servers diff --git a/docs/user-guide/troubleshooting.md b/docs/user-guide/troubleshooting.md index 78f28229..6ef629dc 100644 --- a/docs/user-guide/troubleshooting.md +++ b/docs/user-guide/troubleshooting.md @@ -210,14 +210,17 @@ mcpls --log-level debug - Large projects time out - Tools return timeout errors -**Note**: `timeout_seconds` only bounds the initial `initialize` handshake - it -does **not** affect the timeout on individual tool-call requests (hover, -definition, references, etc.), which is a fixed 30 s internally (10 s for -completions) and not configurable. If a server needs minutes to load a large +**Note**: `timeout_seconds` only bounds the initial `initialize` handshake - +it does **not** affect the timeout on individual tool-call requests (hover, +definition, references, etc.); use `request_timeout_seconds` for that (see +Solution 2). A tool call that triggers a server respawn (the previous process +had died) costs `timeout_seconds + (4 * request_timeout_seconds + 3.5 s)` in +the worst case, since the `initialize` handshake runs again before the +request itself is retried. If a server needs minutes to load a large solution, Solution 1 below is what helps; while it's still initializing, tool calls for that language return a "server is still initializing - wait and retry" message rather than a hard "no server configured" error. If requests -are timing out *after* initialization completes, Solution 2 or 3 are the +are timing out *after* initialization completes, Solution 2, 3, or 4 are the relevant fixes. **Solution 1**: Increase the `initialize` handshake timeout: @@ -230,14 +233,26 @@ file_patterns = ["**/*.rs"] timeout_seconds = 120 # Give a slow `initialize` handshake more time ``` -**Solution 2**: Wait for initial indexing to complete: +**Solution 2**: Increase the per-request timeout: +```toml +[[lsp_servers]] +language_id = "rust" +command = "rust-analyzer" +args = [] +file_patterns = ["**/*.rs"] +request_timeout_seconds = 60 # Give slow tool-call requests more time +``` +Note that `textDocument/completion` requests are capped at 10 s regardless of +this setting - completions cannot be raised above that ceiling today. + +**Solution 3**: Wait for initial indexing to complete: ```bash # rust-analyzer needs time to index on first run # Monitor with debug logging mcpls --log-level debug ``` -**Solution 3**: Reduce workspace size: +**Solution 4**: Reduce workspace size: ```toml [workspace] # Limit to active project only @@ -467,10 +482,10 @@ files.excludeDirs = ["target", "node_modules", ".git", "dist"] **Solutions**: -1. **Increase timeout**: +1. **Increase the per-request timeout** (this is what bounds tool calls, not `timeout_seconds`): ```toml [[lsp_servers]] -timeout_seconds = 60 +request_timeout_seconds = 60 ``` 2. **Pre-warm LSP server**: