feat(lsp): add per-request LSP timeout configuration - #272
Merged
Conversation
`timeout_seconds` only ever bounded the `initialize` handshake; every per-request LSP round trip (hover, definition, references, etc.) used fixed constants that users could not tune, despite pre-#264 docs implying otherwise. Adds `request_timeout_seconds` on `LspServerConfig` as a distinct, per-request knob, reached via new `LspClient` accessors with no `Translator` signature changes. Completion requests remain capped at 10s regardless of the configured value, since they are the most latency-sensitive tool and the MCP client's own timeout leaves no margin for a longer wait. BREAKING CHANGE: `LspServerConfig` gains a new `pub` field `request_timeout_seconds` (default 30, additive under deny_unknown_fields so existing TOML configs are unaffected). `ServerConfig::validate()` now rejects `timeout_seconds == 0` and `request_timeout_seconds == 0`. Closes #267
…xture Rebasing onto main pulled in a new LspServerConfig struct literal (added by #268's workspace_symbol_search regression test) that predates request_timeout_seconds and didn't compile after the rebase.
bug-ops
force-pushed
the
per-request-lsp-timeouts
branch
from
August 4, 2026 21:03
5c7cb74 to
81262b9
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a per-request, per-LSP-server timeout configuration knob to mcpls, aligning runtime behavior with user expectations and prior documentation by making tool-call LSP round-trip timeouts configurable instead of hardcoded.
Changes:
- Introduces
LspServerConfig::request_timeout_seconds(default 30s) and threads it throughLspClient::request_timeout()/completion_timeout()sobridge::translatoruses configured timeouts instead of fixed constants. - Enforces non-zero timeouts in TOML-loaded configs (
ServerConfig::validate()rejectstimeout_seconds == 0andrequest_timeout_seconds == 0) and clamps programmatic configs to at least 1s on the runtime path. - Updates documentation + changelog to describe the new field, the completion timeout cap, and the retry-based worst-case latency math.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds request_timeout_seconds to the example LSP server config snippet. |
| docs/user-guide/troubleshooting.md | Updates troubleshooting guidance to use request_timeout_seconds and documents worst-case latency including retries. |
| docs/user-guide/tools-reference.md | Fixes timeout remediation guidance to reference request_timeout_seconds. |
| docs/user-guide/configuration.md | Documents request_timeout_seconds, including retry ceiling math and the completion timeout cap. |
| crates/mcpls-core/tests/integration/rust_analyzer_tests.rs | Updates integration test config structs to include the new field. |
| crates/mcpls-core/src/lsp/lifecycle.rs | Applies a .max(1) clamp to timeout_seconds for initialize on the non-validated serve() path. |
| crates/mcpls-core/src/lsp/client.rs | Adds request_timeout() / completion_timeout() accessors, completion cap constant, and unit tests for timeout behavior. |
| crates/mcpls-core/src/lib.rs | Updates internal test config literals to include request_timeout_seconds. |
| crates/mcpls-core/src/config/server.rs | Adds the request_timeout_seconds field with serde default + updates canned configs and tests. |
| crates/mcpls-core/src/config/routing.rs | Updates routing-related tests/config literals for the new field. |
| crates/mcpls-core/src/config/mod.rs | Validates both timeout fields are non-zero and adds TOML backward-compat/defaulting + validation tests. |
| crates/mcpls-core/src/bridge/translator.rs | Replaces hardcoded per-request timeouts with client.request_timeout() / client.completion_timeout() across tool handlers. |
| CHANGELOG.md | Documents the new config field and notes the removal of fixed translator timeout constants. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LspServerConfig::timeout_secondsonly ever bounded theinitializehandshake; every per-request LSP round trip (hover, definition, references, etc.) used fixedDEFAULT_LSP_TIMEOUT/COMPLETIONS_LSP_TIMEOUTconstants that users could not tune, despite pre-Bridge/LSP hardening: shared timeout constant, RFC 3986 encoding test, retry-loop test coverage #264 documentation implying otherwise.LspServerConfig::request_timeout_seconds(default 30, additive field) as a distinct per-request knob, reached via newLspClient::request_timeout()/completion_timeout()accessors — noTranslatorsignature changes required, all 17 call sites inbridge::translatorswapped from the fixed constants to the accessors.completion_timeout()clamps to at most 10s regardless of the configured value (an explicit MVP decision, not an oversight — completions are the most latency-sensitive tool and the MCP client's own timeout leaves no margin for a longer wait).ServerConfig::validate()now rejectstimeout_seconds == 0andrequest_timeout_seconds == 0;request_timeout()also clamps to a minimum of 1s to cover the programmaticserve()entry point, which does not go throughvalidate(). The same.max(1)guard was also applied to the pre-existing handshaketimeout_secondson theinitializecall inlifecycle.rs, closing the identical gap there.4 * request_timeout_seconds + 3.5s, sinceLspClient::requestretries up to 4 attempts on a-32802response.docs/user-guide/configuration.md,docs/user-guide/troubleshooting.md, anddocs/user-guide/tools-reference.mdto remove the stale "not configurable" claim and document the new field.Process
Went through the full team-develop pipeline: architect plan → adversarial critic (2 rounds, first returned
significanton the completion-timeout formula, retry-ceiling math, and a missing zero-value validation gap; all three resolved and the revision approved asminor) → implementation → parallel validation (tester, perf, security, and an implementation-level adversarial critic — the latter caught one blocking rustdoc inaccuracy plus thetimeout_seconds.max(1)gap noted above, both fixed) → final code review, approved. Rebased onto main after merge of #268-#271 (conflict only in CHANGELOG.md; one rebase-introduced test fixture needed the new field added).One low-severity, non-blocking item was surfaced during security review and is left for a follow-up issue (#273) rather than blocking this PR:
request_timeout_secondshas no upper bound — an extremely large configured value silently disables the timeout via tokio's ~30yr fallback deadline.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features --lib --bins(574 passed)cargo test --doc --all-features(9 passed, including 2 new doctests)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featurestimeout_secondspresent,request_timeout_secondsabsent), serde roundtripCloses #267