fix(lsp): recover from LSP server crashes and stop leaking timed-out requests - #263
Conversation
LspClient::request never removed its slot from the shared pending_requests map when a request timed out, so a server that stalled without fully crashing accumulated one leaked map entry per timed-out call for the life of the connection. Also add LspClient::fail_pending_requests, draining and failing every still-pending request as Error::ServerTerminated; used by the upcoming respawn path to fail stragglers immediately instead of leaving each to time out on its own.
mcpls previously surfaced a clean error on every tool call once a routed LSP server's child process died, but never attempted to recover it — the session stayed degraded for that language until the entire mcpls process was restarted. Translator now detects a dead child (LspServer::has_exited) and transparently respawns and re-initializes it before resolving the next tool call for that server. Concurrent callers observing the same dead server single-flight on a per-server lock so only one respawn happens; requests still parked on the old connection are failed immediately via fail_pending_requests instead of waiting out their own timeout. A crash-looping server backs off exponentially (1s up to 30s) instead of retrying on every call, including the "starts, initializes, then dies again shortly after" loop, not just an outright spawn failure. DocumentTracker::forget_server resets per-server document sync state on respawn so the next access re-opens instead of sending a stale didChange; a per-server generation counter closes the race where an in-flight sync against the old connection could otherwise land after a concurrent respawn. The respawn path also invalidates the crashed connection's cached diagnostics via the new Translator::with_notification_cache when the crashed server was the diagnostics route for its language, so a crash doesn't leave stale pre-crash diagnostics merged into every later poll. 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 documented scope trade-off. New Error::ServerUnavailable variant distinguishes "respawn could not proceed" (no config registered, or backing off) from a plain Error::ServerTerminated.
There was a problem hiding this comment.
🟡 Not ready to approve
New regression tests spawn Unix-specific binaries (e.g. sleep, cat, echo) without a Windows-compatible path, which is likely to break the existing Windows CI matrix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves mcpls’s resilience and resource hygiene in the MCP↔LSP bridge by (1) preventing timed-out LSP requests from leaking in-memory bookkeeping and (2) adding transparent, single-flighted LSP server respawn when a routed server subprocess crashes, including reset of per-server document sync state and invalidation of stale diagnostics cache.
Changes:
- Fix
LspClient::requesttimeout cleanup by removing timed-out entries frompending_requests, and addfail_pending_requeststo immediately unblock stragglers when a connection is discarded. - Add crash detection (
LspServer::has_exited) andTranslatorrespawn supervision with per-server single-flight + exponential backoff, plus document sync reset (DocumentTracker::forget_server) and diagnostics cache invalidation on respawn (diagnostics-route only). - Wire respawn configuration registration through startup (
register_servers), addError::ServerUnavailable, and document behavior inCHANGELOG.md.
File summaries
| File | Description |
|---|---|
| crates/mcpls-core/src/lsp/lifecycle.rs | Stores the child handle and adds has_exited(); adds a regression test for crash detection. |
| crates/mcpls-core/src/lsp/client.rs | Fixes timeout leak in pending_requests and adds fail_pending_requests, plus regression tests. |
| crates/mcpls-core/src/lib.rs | Passes per-server spawn configs into the translator and wires in a shared diagnostics cache handle. |
| crates/mcpls-core/src/error.rs | Adds Error::ServerUnavailable to distinguish “dead” vs “could not respawn/backing off”. |
| crates/mcpls-core/src/bridge/translator.rs | Implements respawn detection/recovery, single-flight locking, backoff, and cache invalidation integration. |
| crates/mcpls-core/src/bridge/state.rs | Adds per-server generation tracking and forget_server to prevent post-respawn sync races. |
| CHANGELOG.md | Documents respawn-on-crash and timeout-leak fix under Unreleased entries. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
test_has_exited_reflects_child_process_state and test_request_timeout_removes_pending_entry each spawn a real sleep subprocess to exercise their respective code paths, which is unavailable on the Windows CI runner and would fail to spawn there. Gate both behind #[cfg(unix)], matching the existing convention for the sh-scripted fake-LSP-server harness in translator.rs's respawn_tests module. Also cut the lifecycle.rs test from three spawned processes down to one by reusing the sleep child's own piped stdin/stdout instead of spawning separate cat/echo processes for it.
Summary
LspClient::requestnever removed itspending_requestsentry on timeout, leaking one map entry per timed-out call for the life of the connection.LspClientgainsfail_pending_requeststo fail stragglers immediately instead of leaving each to time out on its own.Translatornow detects a dead child process and transparently respawns and re-initializes it before resolving the next tool call for that server.Details
fail_pending_requests.DocumentTracker::forget_serverresets per-server document sync state on respawn so the next access re-opens instead of sending a staledidChange; a per-server generation counter closes the race where an in-flight sync against the old connection could otherwise land after a concurrent respawn.Error::ServerUnavailablevariant distinguishes "respawn could not proceed" (no config registered, or backing off) from a plainError::ServerTerminated.Went through several rounds of adversarial review before this PR: initial implementation, three rounds of critique catching a critical stale-diagnostics bug and two significant concurrency races (a document-sync TOCTOU and a backoff-reset bug that let post-init crash loops bypass backoff), and a final code review pass. Also required a non-trivial rebase merge with #260's capability-gating work, since both PRs touched the same client-resolution call sites — respawn-awareness and capability-gating are now composed correctly at every call site (respawn-check-and-recover before capability-gating, since a dead server can't be asked about capabilities).
Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features --lib --bins(531 passed)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-featurescargo test --doc --workspace --all-featuresprepare_document