fix(transport): handle SIGINT/SIGTERM in stdio transport and gracefully shut down LSP servers - #270
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses process hygiene in mcpls-core by ensuring both stdio and HTTP transports react to SIGINT/SIGTERM and trigger an orderly shutdown path that drains and shuts down spawned LSP servers instead of orphaning them.
Changes:
- Added shared SIGINT/SIGTERM handling and applied it to both
run_stdioandrun_http, including a bounded HTTP graceful shutdown wait. - Introduced a post-transport shutdown sequence in
serve_withthat signals background tasks to stop and drains registered LSP servers viaTranslator::shutdown_servers(). - Extended
LspServer::shutdown()to attempt the LSPshutdown/exithandshake and then wait a bounded grace period for the child process to exit before relying onkill_on_drop.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mcpls-core/src/transport.rs | Adds shared shutdown-signal handling; stdio returns on signal; HTTP graceful shutdown is bounded post-signal. |
| crates/mcpls-core/src/lsp/mod.rs | Re-exports a test-only fake LSP server helper for shutdown-path tests. |
| crates/mcpls-core/src/lsp/lifecycle.rs | Adds bounded child-exit grace in LspServer::shutdown() and introduces a shared test helper for mock servers. |
| crates/mcpls-core/src/lib.rs | Ensures transport completion triggers a centralized shutdown sequence that drains LSP servers. |
| crates/mcpls-core/src/bridge/translator.rs | Implements shutdown_servers() to concurrently drain and shut down registered servers with per-server timeouts. |
| CHANGELOG.md | Documents the fix and its limitations under Unreleased/Fixed. |
bug-ops
added a commit
that referenced
this pull request
Aug 4, 2026
… comment Copilot review on #270 correctly flagged the doc comment claiming fake_lsp_server() was pub(crate) when the signature is pub — clarify that pub is intentional (lifecycle is a private module, so pub already stays crate-scoped, and clippy's redundant_pub_crate lint forbids the pub(crate) form here).
bug-ops
enabled auto-merge (squash)
August 4, 2026 20:31
bug-ops
added a commit
that referenced
this pull request
Aug 4, 2026
… comment Copilot review on #270 correctly flagged the doc comment claiming fake_lsp_server() was pub(crate) when the signature is pub — clarify that pub is intentional (lifecycle is a private module, so pub already stays crate-scoped, and clippy's redundant_pub_crate lint forbids the pub(crate) form here).
bug-ops
force-pushed
the
fix/241-stdio-sigterm-handling
branch
from
August 4, 2026 20:38
9c124f6 to
af72641
Compare
…ly shut down LSP servers run_stdio, the default transport for stdio-based MCP clients, installed no signal handler, so an uncaught SIGINT/SIGTERM bypassed kill_on_drop and orphaned every spawned LSP child process. Separately, LspServer::shutdown() was dead code outside tests, so even the clean stdin-EOF exit path never gave LSP servers a graceful shutdown/exit handshake. Both transports now share signal-handling logic, and serve_with calls a new Translator::shutdown_servers() after the transport future resolves, draining and gracefully shutting down every registered LspServer concurrently with a bounded per-server grace period before falling back to kill_on_drop. run_http's graceful shutdown wait is now itself bounded so a stuck connection can't block LSP cleanup indefinitely. Known limitation: process termination via an uncaught panic under panic = "abort" still bypasses this cleanup, since no Drop runs on that path; a real fix needs process-group isolation and is out of scope here. Closes #241
… comment Copilot review on #270 correctly flagged the doc comment claiming fake_lsp_server() was pub(crate) when the signature is pub — clarify that pub is intentional (lifecycle is a private module, so pub already stays crate-scoped, and clippy's redundant_pub_crate lint forbids the pub(crate) form here).
bug-ops
force-pushed
the
fix/241-stdio-sigterm-handling
branch
from
August 4, 2026 20:38
af72641 to
e47ffe2
Compare
…io test The rebase conflict fix for McplsServer::new's new fifth parameter (added by #248 on main) was applied to the working tree but never committed, so CI still built against the stale four-argument call.
This was referenced Aug 5, 2026
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
run_stdio, the default transport used by every stdio-based MCP client (Claude Desktop, Claude Code, etc.), installed no signal handler at all — an uncaught SIGINT/SIGTERM bypassedkill_on_dropand orphaned every spawned LSP child process. It now shares the same signal-handling logicrun_httpalready had.LspServer::shutdown()(the graceful LSPshutdown/exithandshake) was previously dead code outside tests.serve_withnow calls a newTranslator::shutdown_servers()after the transport future resolves — on signal, stdin EOF, or HTTP's own shutdown — which drains and gracefully shuts down every registeredLspServerconcurrently, with a bounded per-server grace period before falling back tokill_on_drop.run_http's own graceful shutdown wait is now bounded too, so a stuck in-flight connection can't block LSP cleanup indefinitely.Known limitation
Process termination via an uncaught panic under
panic = "abort"([profile.release]) still bypasses this cleanup, since noDropruns on that path. A real fix needs process-group isolation, which is out of scope here and documented as a# Limitationsnote onTranslator::shutdown_servers. Recommend filing a narrower follow-up issue for that vector before/independent of closing this one.Test plan
cargo +nightly fmt --all -- --checkcargo clippy --all-targets --all-features --workspace -- -D warningscargo nextest run --workspace --all-features --lib --bins(489/489)RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --workspaceserve_with's shutdown path draining a registered server,run_stdio's prompt-return behavior, and a paused-clock regression test provingrun_httpno longer self-terminates on ordinary uptimeCloses #241