fix(transport): handle SIGINT/SIGTERM in stdio transport and gracefully shut down LSP servers

CHANGELOG.md

@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **stdio transport now handles SIGINT/SIGTERM and shuts down spawned LSP servers gracefully** — `run_stdio`, the default transport used by every stdio-based MCP client, previously installed no signal handler at all, so an uncaught SIGINT/SIGTERM bypassed `kill_on_drop` and orphaned every spawned LSP child process; `run_http` already handled signals but, like the clean stdin-EOF exit path, never invoked the LSP-level graceful `shutdown`/`exit` handshake (`LspServer::shutdown()` was previously dead code outside tests). Both transports now share signal-handling logic, and `serve_with` calls a new `Translator::shutdown_servers()` after the transport future resolves — regardless of whether that was a signal, stdin EOF, or HTTP's own shutdown — which drains and gracefully shuts 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: this does not cover process termination via an uncaught panic under `panic = "abort"` (`[profile.release]`), since no `Drop` runs on that path — a real fix needs process-group isolation, which is out of scope here. (#241)
 - **HTTP transport startup warning gave inverted authentication guidance** — the non-loopback bind warning previously read "...ensure no authentication is required", which could be misread as instructing operators to confirm auth is *not* needed. mcpls performs no authentication on any transport; the message now tells operators to put such deployments behind a reverse proxy that enforces authentication. (#233)
 - **`config::mod` CWD-mutating tests could leave the process working directory changed after a mid-test panic** — added a `CwdGuard` RAII helper that restores the original directory on drop, not only on the successful path, alongside the existing mutex serialization against concurrent CWD use. (#238)
 - **`LspClient::request` leaked its `pending_requests` entry on timeout** — a timed-out request never removed its slot from the shared pending-requests map, so a server that stalled (without fully crashing) would accumulate one leaked entry per timed-out call for the life of the connection. `LspClient` also gained `fail_pending_requests`, used by the respawn path above to fail stragglers immediately rather than leaving each to time out on its own. (#239)

crates/mcpls-core/src/bridge/translator.rs

@@ -122,6 +122,11 @@ const RESPAWN_BACKOFF_BASE: Duration = Duration::from_secs(1);
 /// Upper bound on the exponential backoff delay between respawn attempts.
 const RESPAWN_BACKOFF_MAX: Duration = Duration::from_secs(30);
 
+/// Upper bound on how long [`Translator::shutdown_servers`] waits for a
+/// single LSP server's graceful `shutdown`/`exit` handshake before giving up
+/// and letting `kill_on_drop` terminate it instead.
+const SERVER_SHUTDOWN_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
+
 impl Translator {
     /// Create a new translator.
     ///
@@ -248,6 +253,18 @@ impl Translator {
         lock_std(&self.server_configs).insert(id.into(), config);
     }
 
+    /// Number of currently registered LSP servers.
+    ///
+    /// Test-only: `lsp_servers` is private, so this is the one way a test
+    /// outside this module (e.g. `crate::tests`, exercising
+    /// [`Translator::shutdown_servers`] indirectly through `serve_with`'s
+    /// shutdown sequence) can observe that a registered server was actually
+    /// drained.
+    #[cfg(test)]
+    pub(crate) fn registered_server_count(&self) -> usize {
+        lock_std(&self.lsp_servers).len()
+    }
+
     /// Snapshot of currently open document paths, used for MCP resource listing.
     #[must_use]
     pub fn open_document_paths(&self) -> Vec<PathBuf> {
@@ -260,10 +277,59 @@ impl Translator {
         self.document_tracker.is_open(path)
     }
 
-    // TODO: These methods will be implemented in Phase 3-5
-    // Initialize and shutdown are now handled by LspServer in lifecycle.rs
+    /// Gracefully shut down every registered LSP server.
+    ///
+    /// Drains the registered LSP servers and, for each one concurrently,
+    /// sends the LSP `shutdown` request and `exit` notification via
+    /// [`LspServer::shutdown`], bounded by a fixed per-server timeout. A
+    /// server that errors or fails to respond in time is simply dropped
+    /// instead: its child process handle is `kill_on_drop(true)`, so the
+    /// process is killed rather than left running. Call this once, from the
+    /// top-level shutdown path, after the MCP transport has stopped
+    /// accepting new requests.
+    ///
+    /// # Limitations
+    ///
+    /// This only runs on the normal shutdown path (stdio EOF, `SIGTERM`/
+    /// `SIGINT`, or the HTTP transport's own graceful shutdown). This crate's
+    /// workspace `[profile.release]` builds with `panic = "abort"`, so a
+    /// panic reachable from a request handler or background pump task in a
+    /// release build still terminates the process without unwinding — this
+    /// method never runs, and spawned LSP children are orphaned exactly as
+    /// before this fix. Making that path safe would need process-group
+    /// isolation (`kill_on_drop` alone doesn't help, since no `Drop` runs
+    /// either); tracked separately, out of scope here.
+    ///
+    /// `pub(crate)` rather than `pub`: this is meant for exactly one call
+    /// site (`serve_with`'s post-transport shutdown sequence), after the MCP
+    /// transport is already down. An external caller invoking it mid-session
+    /// would drain `lsp_servers` while `lsp_clients` (routing table) still
+    /// points at the now-shut-down servers, so in-flight tool calls would
+    /// resolve to a client whose server is gone.
+    pub(crate) async fn shutdown_servers(&self) {
+        let servers: Vec<(ServerId, LspServer)> = lock_std(&self.lsp_servers).drain().collect();
+        if servers.is_empty() {
+            return;
+        }
 
-    // Future implementation will use LspServer instead of LspClient directly
+        let mut tasks = tokio::task::JoinSet::new();
+        for (id, server) in servers {
+            tasks.spawn(async move {
+                match tokio::time::timeout(SERVER_SHUTDOWN_TIMEOUT, server.shutdown()).await {
+                    Ok(Ok(())) => tracing::debug!(%id, "LSP server shut down gracefully"),
+                    Ok(Err(e)) => tracing::warn!(
+                        %id, error = %e,
+                        "LSP server shutdown handshake failed, killing process instead"
+                    ),
+                    Err(_) => tracing::warn!(
+                        %id, timeout = ?SERVER_SHUTDOWN_TIMEOUT,
+                        "LSP server did not shut down in time, killing process instead"
+                    ),
+                }
+            });
+        }
+        tasks.join_all().await;
+    }
 }
 
 impl Default for Translator {
@@ -2989,6 +3055,52 @@ mod tests {
         // This test verifies the data structure is properly initialized.
     }
 
+    /// #241: `shutdown_servers` on an empty registry must return immediately
+    /// rather than blocking (e.g. on a `JoinSet` that's never populated).
+    #[tokio::test]
+    async fn test_shutdown_servers_empty_registry_returns_promptly() {
+        let translator = Translator::new();
+
+        let result =
+            tokio::time::timeout(Duration::from_secs(1), translator.shutdown_servers()).await;
+
+        assert!(
+            result.is_ok(),
+            "shutdown_servers must return promptly when no servers are registered"
+        );
+    }
+
+    /// #241: `shutdown_servers` must drain every registered `LspServer` —
+    /// this is the core behavior the issue is about (orphaned LSP children
+    /// on shutdown). Uses `fake_lsp_server()` (mock `echo`/`cat` child
+    /// processes, real `LspServer`, see `lsp::lifecycle`), which won't
+    /// answer the LSP `shutdown` handshake — proving the drain completes,
+    /// via the timeout/error fallback path, without hanging on
+    /// non-responsive servers.
+    #[tokio::test]
+    async fn test_shutdown_servers_drains_registered_servers() {
+        let translator = Translator::new();
+        translator.register_server("server-a", crate::lsp::fake_lsp_server());
+        translator.register_server("server-b", crate::lsp::fake_lsp_server());
+        assert_eq!(lock_std(&translator.lsp_servers).len(), 2);
+
+        // Bounded well above `SERVER_SHUTDOWN_TIMEOUT` (10s) so a genuine
+        // regression (a hang) still fails the test instead of the harness
+        // itself timing out ambiguously.
+        let result =
+            tokio::time::timeout(Duration::from_secs(20), translator.shutdown_servers()).await;
+
+        assert!(
+            result.is_ok(),
+            "shutdown_servers must not hang against non-responsive mock servers"
+        );
+        assert_eq!(
+            lock_std(&translator.lsp_servers).len(),
+            0,
+            "all registered servers must be drained"
+        );
+    }
+
     #[test]
     fn test_get_client_for_file_server_initializing_when_expected() {
         // A configured/applicable language whose LSP client has not registered

crates/mcpls-core/src/lib.rs

@@ -572,13 +572,28 @@ pub async fn serve_with(config: ServerConfig, transport: Transport) -> Result<()
         Transport::Http(cfg) => run_http(mcp_server, cfg).await,
     };
 
-    // Signal background pump tasks to exit.
-    let _ = cancel_tx.send(true);
+    shutdown(&cancel_tx, &translator).await;
 
     info!("MCPLS server shutting down");
     result
 }
 
+/// Post-transport shutdown sequence, run once the transport future
+/// (`run_stdio`/`run_http`) returns — whether that's because of a
+/// `SIGTERM`/`SIGINT`, stdio EOF, or (for HTTP) its own graceful shutdown.
+///
+/// Signals background pump tasks to exit, then gracefully shuts down every
+/// LSP server registered on `translator` (see
+/// [`Translator::shutdown_servers`] for what "gracefully" bounds and falls
+/// back to). Extracted from [`serve_with`] so this sequence is exercised
+/// directly in tests without needing a full stdio/HTTP transport round trip.
+async fn shutdown(cancel_tx: &tokio::sync::watch::Sender<bool>, translator: &Translator) {
+    let _ = cancel_tx.send(true);
+
+    info!("Shutting down LSP servers...");
+    translator.shutdown_servers().await;
+}
+
 /// Spawn the applicable LSP servers in a background task and register them into
 /// the shared `translator` once ready.
 ///
@@ -1114,6 +1129,43 @@ mod tests {
                 );
             }
         }
+
+        /// #241: `serve_with`'s post-transport shutdown sequence must drain
+        /// registered LSP servers rather than orphaning them. Exercises
+        /// `shutdown()` directly (the exact code `serve_with` runs after its
+        /// transport future returns) against a `Translator` with a real,
+        /// registered `LspServer` — `serve_with` itself can't be driven
+        /// through this path in a portable unit test, since it only
+        /// registers a server after a successful LSP `initialize` handshake,
+        /// which requires a real language server binary.
+        #[tokio::test]
+        async fn test_shutdown_drains_registered_lsp_server() {
+            let translator = Translator::new();
+            translator.register_server("fake-server", crate::lsp::fake_lsp_server());
+            assert_eq!(translator.registered_server_count(), 1);
+
+            let (cancel_tx, cancel_rx) = tokio::sync::watch::channel(false);
+
+            let result = tokio::time::timeout(
+                std::time::Duration::from_secs(20),
+                super::super::shutdown(&cancel_tx, &translator),
+            )
+            .await;
+
+            assert!(
+                result.is_ok(),
+                "shutdown must not hang against a non-responsive mock LSP server"
+            );
+            assert_eq!(
+                translator.registered_server_count(),
+                0,
+                "shutdown must drain every registered LSP server"
+            );
+            assert!(
+                *cancel_rx.borrow(),
+                "shutdown must signal background pump tasks to exit"
+            );
+        }
     }
 
     // ------------------------------------------------------------------