fix(cli): exit via process::exit to avoid tokio stdin-blocking-pool hang

CHANGELOG.md

@@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **`MCPLS_LOG_JSON`/`MCPLS_TRUST_PROJECT_CONFIG` rejected common boolean env-var spellings** — both flags are declared as plain `bool` fields with clap's `env` attribute, whose derived value parser is `str::parse::<bool>()`, accepting only the exact lowercase literals `"true"`/`"false"`. Any other common convention (`1`/`0`, `yes`/`no`, `Y`/`N`, `on`/`off`, uppercase `TRUE`/`FALSE`) caused clap to print a parse error and exit nonzero before `logging::init` or anything else ran — no LSP servers spawned, no MCP server started, nothing logged. A new `parse_bool_flag` value parser, applied to both fields via `#[arg(value_parser = ...)]`, now accepts `1`/`0`, `true`/`false`, `yes`/`no`, `y`/`n`, and `on`/`off`, case-insensitively; the `--log-json`/`--trust-project-config` CLI flags themselves are unaffected (still bare, no-value flags). Any other value is still a clear parse error at startup. (#295)
+- **mcpls did not exit on `SIGTERM`/`SIGINT` while a stdio MCP client kept its stdin write end open** — `rmcp::transport::stdio()` is backed by `tokio::io::stdin()`, which internally parks an uncancellable `spawn_blocking` thread in a raw `read()` syscall; `#[tokio::main]`'s generated wrapper blocks in `Runtime::drop` waiting for that thread once `main`'s body returns, even though all real shutdown work (LSP server teardown, log flush) had already completed by then. `main` now calls `std::process::exit` as its final step instead of returning normally, terminating immediately once `run().await` resolves and bypassing the blocking-pool wait. `main`'s signature changed from `-> std::process::ExitCode` (added in #279) to `()`; not a breaking change for library embedders — a binary's `main` signature is not part of `mcpls-core`'s public API, and the resulting exit codes (0/1) are unchanged. Scoped to `crates/mcpls-cli/src/main.rs` only — `mcpls-core`'s `serve`/`serve_with`/`shutdown`/`run_stdio` are unaffected and keep normal `Result`-returning semantics for library embedders; their doc comments now carry a "Shutdown" note (and updated examples) warning embedders of the same `process::exit` requirement under the stdio transport. Also fixed: `await_lsp_init_handle`'s timeout branch called `JoinHandle::abort()` without a subsequent await, so on a `SIGTERM` arriving mid-`spawn_batch` (before any server registers), `process::exit` could now run before the runtime dropped the aborted task's locals — including not-yet-registered LSP `Child` handles relying on `kill_on_drop` — orphaning those processes (the failure mode #270 guards against). `abort()` is now followed by a bounded (1s) re-await of the same handle so that drop happens before `main` exits. (#308)
 - **`workspace.position_encodings` was parsed but never consumed** — the configured preference order was previously dead: `LspServer::spawn`'s `initialize` handshake always offered a hardcoded `["utf-8", "utf-16"]` regardless of what `mcpls.toml` set. It is now sent as `capabilities.general.positionEncodings` in the configured order (see the accompanying breaking changes under Changed above). Note this only changes which encodings mcpls *offers*; the encoding a server actually negotiates is still not consumed downstream of the handshake (tracked separately in #290). (#287)
 - **`LspServer::position_encoding()` was computed but never consumed** — the encoding actually negotiated with an LSP server during `initialize` was recorded but every MCP↔LSP position conversion in `bridge::encoding`/`bridge::translator` still assumed a fixed encoding regardless of it. This was not a rare edge case: under mcpls's own `capabilities.general.positionEncodings` offer order (see #287 above), UTF-8 is empirically the *default* negotiated encoding for both rust-analyzer and clangd, so any non-ASCII line on either server could silently produce wrong columns. Position conversions now resolve the real negotiated encoding per LSP server via a new `EncodingCtx` and derive UTF-8/UTF-16/UTF-32 column offsets from the actual line text — preferentially from `DocumentTracker`'s in-memory state for a tracked document (the exact content mcpls has sent that server via `didOpen`/`didChange`), falling back to an async disk read for a referenced-but-untracked file (e.g. a cross-file `references`/`rename` result). Line-text lookups guard against splitting a multi-byte UTF-8 character, which previously could panic; a malformed or out-of-range character offset now falls back to the original, unconverted value with a `tracing::warn!` instead of panicking or erroring. (#290)
 - **`--log-json`/`MCPLS_LOG_JSON` was parsed but never consumed** — the flag was defined in `Args` and the `json` feature of `tracing-subscriber` was already enabled, but `logging::init` always built the compact human-readable `fmt` layer regardless of its value. `logging::init` now takes the flag and selects the JSON `fmt` layer when set. Also fixed: on a fatal startup/runtime error, `main` previously returned `Result<()>` and let Rust's default `Termination` impl print the error via `Debug` directly to stderr, bypassing the tracing subscriber (and `--log-json`) entirely; `main` now returns `std::process::ExitCode` and logs fatal errors through `tracing::error!` before exiting, so crash output is JSON too when `--log-json` is set. (#279)

crates/mcpls-cli/src/main.rs

@@ -13,24 +13,37 @@ mod logging;
 use args::Args;
 
 #[tokio::main]
-async fn main() -> std::process::ExitCode {
+async fn main() {
     let args = Args::parse();
 
     // Initialize logging. No subscriber is installed yet, so failures here
     // must go straight to stderr.
     if let Err(err) = logging::init(&args.log_level, args.log_json) {
         eprintln!("failed to initialize logging: {err:?}");
-        return std::process::ExitCode::FAILURE;
+        std::process::exit(1);
     }
 
     // Route fatal errors through the tracing subscriber (rather than the
     // default `Result` `Termination` printer) so they honor --log-json too.
-    if let Err(err) = run(args).await {
+    let exit_code = if let Err(err) = run(args).await {
         tracing::error!(error = ?err, "mcpls exited with an error");
-        return std::process::ExitCode::FAILURE;
-    }
+        1
+    } else {
+        0
+    };
 
-    std::process::ExitCode::SUCCESS
+    // `#[tokio::main]`'s generated wrapper blocks in `Runtime::drop` ->
+    // `BlockingPool::shutdown` after this function returns, waiting for
+    // every outstanding spawn_blocking thread -- including the one
+    // `rmcp::transport::stdio()` (== `tokio::io::stdin()`) parks in a raw,
+    // uncancellable `read()` on the real stdin fd. That read only returns on
+    // more input or EOF, so if the MCP client's write end of stdin is still
+    // open, the wait never completes even though `run()` above (which
+    // includes LSP server shutdown and all shutdown logging) has already
+    // finished. `process::exit` terminates immediately, bypassing that wait
+    // -- safe here because everything that matters has already completed
+    // above. See #308.
+    std::process::exit(exit_code);
 }
 
 async fn run(args: Args) -> Result<()> {

crates/mcpls-core/README.md

@@ -54,10 +54,14 @@ flowchart LR
 use mcpls_core::{ServerConfig, Transport};
 
 #[tokio::main]
-async fn main() -> anyhow::Result<()> {
-    let config = ServerConfig::load()?;
-    mcpls_core::serve_with(config, Transport::Stdio).await?;
-    Ok(())
+async fn main() {
+    let config = ServerConfig::load().expect("failed to load config");
+    let result = mcpls_core::serve_with(config, Transport::Stdio).await;
+    // `Transport::Stdio` is backed by `tokio::io::stdin()`, which parks an
+    // uncancellable blocking-pool thread; returning normally from `main`
+    // here can hang on SIGTERM/SIGINT while a client's stdin is still open.
+    // See `serve_with`'s "Shutdown" docs.
+    std::process::exit(if result.is_ok() { 0 } else { 1 });
 }
 ```
 

crates/mcpls-core/src/lib.rs

@@ -21,13 +21,17 @@
 //! use mcpls_core::{serve, serve_with, Transport, ServerConfig};
 //!
 //! #[tokio::main]
-//! async fn main() -> Result<(), mcpls_core::Error> {
-//!     let config = ServerConfig::load()?;
+//! async fn main() {
+//!     let config = ServerConfig::load().expect("failed to load config");
 //!     // Stdio (default):
-//!     serve(config).await
+//!     let result = serve(config).await;
 //!     // HTTP (requires `transport-http` feature):
 //!     // let http = mcpls_core::HttpConfig::new("127.0.0.1:3000".parse().unwrap(), "/mcp");
-//!     // serve_with(config, Transport::Http(http)).await
+//!     // let result = serve_with(config, Transport::Http(http)).await;
+//!
+//!     // See `serve`/`serve_with`'s "Shutdown" docs: process::exit avoids a
+//!     // runtime-shutdown hang under the stdio transport.
+//!     std::process::exit(if result.is_ok() { 0 } else { 1 });
 //! }
 //! ```
 
@@ -392,6 +396,12 @@ fn canonicalize_workspace_roots(roots: &[PathBuf]) -> Vec<PathBuf> {
 /// - **All servers succeed**: Service runs normally
 /// - **Partial success**: Logs warnings for failures, continues with available servers
 /// - **All servers fail**: Returns `Error::AllServersFailedToInit` with details
+///
+/// # Shutdown
+///
+/// See [`serve_with`]'s "Shutdown" section — this function uses
+/// [`Transport::Stdio`], so the same `std::process::exit` requirement
+/// applies to callers.
 pub async fn serve(config: ServerConfig) -> Result<(), Error> {
     serve_with(config, Transport::Stdio).await
 }
@@ -424,15 +434,34 @@ pub async fn serve(config: ServerConfig) -> Result<(), Error> {
 /// (or another loopback alias) to the mcpls process. Direct non-loopback
 /// access is intentionally blocked to prevent DNS-rebinding attacks.
 ///
+/// # Shutdown
+///
+/// [`Transport::Stdio`] is backed by `tokio::io::stdin()`, which internally
+/// parks an uncancellable blocking-pool thread in a raw `read()` syscall
+/// that only returns on more input or EOF. If your `main` uses
+/// `#[tokio::main]` and simply returns after awaiting this function, the
+/// macro-generated runtime-shutdown wrapper blocks waiting for that thread
+/// -- hanging indefinitely on `SIGTERM`/`SIGINT` as long as the MCP
+/// client's stdin write end is still open, since that never triggers EOF.
+/// Call `std::process::exit` right after this function resolves instead of
+/// returning normally from `main`, as in the example below (see mcpls's own
+/// `mcpls-cli` binary; tracked as #308). This does not apply to
+/// [`Transport::Http`], which never touches `tokio::io::stdin()`.
+///
 /// # Examples
 ///
 /// ```rust,ignore
 /// use mcpls_core::{serve_with, Transport, ServerConfig};
 ///
 /// #[tokio::main]
-/// async fn main() -> Result<(), mcpls_core::Error> {
-///     let config = ServerConfig::load()?;
-///     serve_with(config, Transport::Stdio).await
+/// async fn main() {
+///     let config = ServerConfig::load().expect("failed to load config");
+///     let exit_code = match serve_with(config, Transport::Stdio).await {
+///         Ok(()) => 0,
+///         Err(_) => 1,
+///     };
+///     // See "Shutdown" above: process::exit avoids a runtime-shutdown hang.
+///     std::process::exit(exit_code);
 /// }
 /// ```
 pub async fn serve_with(config: ServerConfig, transport: Transport) -> Result<(), Error> {
@@ -619,13 +648,26 @@ const LSP_INIT_TASK_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5);
 /// detach the task — it keeps running rather than stopping, contradicting
 /// the warning logged below. Retaining ownership lets `abort()` make that
 /// message true.
+///
+/// `abort()` only *requests* cancellation; the task's locals (which may own
+/// not-yet-registered `tokio::process::Child` handles for LSP servers
+/// [`spawn_lsp_servers_background`] is still spawning via `spawn_batch`,
+/// relying entirely on `kill_on_drop` to terminate them) are only actually
+/// dropped once the runtime polls the task to completion. `mcpls-cli`'s
+/// `main` calls `std::process::exit` right after `serve_with` returns (see
+/// #308), which skips the executor's own task teardown that used to do this
+/// polling implicitly — so this function awaits the aborted handle again,
+/// bounded, to drive that drop here instead of leaving it to chance.
+/// Otherwise a `SIGTERM` arriving mid-`spawn_batch` could orphan those LSP
+/// child processes, the exact failure mode #270 was filed to prevent.
 async fn await_lsp_init_handle(mut handle: JoinHandle<()>, timeout: Duration) {
     match tokio::time::timeout(timeout, &mut handle).await {
         Ok(Ok(())) => {}
         Ok(Err(err)) => error!("Background LSP initialization task failed: {err}"),
         Err(_) => {
             warn!("Timed out waiting for background LSP initialization task to stop");
             handle.abort();
+            let _ = tokio::time::timeout(Duration::from_secs(1), handle).await;
         }
     }
 }

crates/mcpls-core/src/transport.rs

@@ -14,13 +14,20 @@
 ///
 /// # Examples
 ///
+/// See [`crate::serve_with`]'s "Shutdown" section before copying this
+/// verbatim: under [`Transport::Stdio`], `main` must call
+/// `std::process::exit` rather than returning normally, or `SIGTERM`/
+/// `SIGINT` can hang while an MCP client's stdin write end is still open
+/// (#308).
+///
 /// ```rust,ignore
 /// use mcpls_core::{Transport, serve_with, ServerConfig};
 ///
 /// #[tokio::main]
-/// async fn main() -> Result<(), mcpls_core::Error> {
-///     let config = ServerConfig::load()?;
-///     serve_with(config, Transport::Stdio).await
+/// async fn main() {
+///     let config = ServerConfig::load().expect("failed to load config");
+///     let result = serve_with(config, Transport::Stdio).await;
+///     std::process::exit(if result.is_ok() { 0 } else { 1 });
 /// }
 /// ```
 #[non_exhaustive]
@@ -185,7 +192,11 @@ async fn wait_for_shutdown_signal() {
 /// [`crate::bridge::Translator::shutdown_servers`] — before the process
 /// exits. On signal, the in-flight `RunningService` is dropped rather than
 /// awaited to completion; `rmcp` closes it asynchronously in that case,
-/// which is acceptable here since the process exits shortly after.
+/// which is acceptable here since the process exits shortly after --
+/// callers must exit via `std::process::exit` rather than returning
+/// normally from `main`, or an uncancellable `tokio::io::stdin()` blocking
+/// thread can stall runtime shutdown indefinitely (see `mcpls-cli`'s
+/// `main.rs` and #308).
 pub(crate) async fn run_stdio(
     mcp_server: crate::mcp::McplsServer,
     peer_cell: &tokio::sync::OnceCell<rmcp::Peer<rmcp::RoleServer>>,

crates/mcpls-core/tests/e2e/mcp_client.rs

@@ -349,6 +349,22 @@ impl McpClient {
         self.request_id += 1;
         self.request_id
     }
+
+    /// Return the OS process ID of the spawned mcpls process.
+    #[allow(dead_code)]
+    pub(crate) fn pid(&self) -> u32 {
+        self.process.id()
+    }
+
+    /// Non-blocking check for whether the process has exited.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if the OS query for the process status fails.
+    #[allow(dead_code)]
+    pub(crate) fn try_wait(&mut self) -> std::io::Result<Option<std::process::ExitStatus>> {
+        self.process.try_wait()
+    }
 }
 
 impl Drop for McpClient {

crates/mcpls-core/tests/e2e/protocol_tests.rs

@@ -333,3 +333,63 @@ fn test_e2e_multiple_requests() -> Result<()> {
 
     Ok(())
 }
+
+/// Test that mcpls exits promptly on `SIGTERM` while the client's stdin
+/// write end is still open (regression test for #308).
+///
+/// The MCP stdio transport is backed by `tokio::io::stdin()`, which parks an
+/// uncancellable blocking-pool thread in a raw `read()` syscall. Without the
+/// `std::process::exit` fix in `mcpls-cli`'s `main`, `#[tokio::main]`'s
+/// runtime-shutdown wait for that thread would hang indefinitely as long as
+/// the client (this test, via `McpClient`) keeps stdin's write end open.
+#[test]
+#[cfg(unix)]
+#[ignore = "Requires mcpls binary built"]
+fn test_e2e_sigterm_exits_promptly_while_client_stdin_open() -> Result<()> {
+    let mut client = McpClient::spawn()?;
+    client.initialize()?;
+
+    // `run_stdio` only registers its SIGTERM handler once `mcp_server.serve(..)`
+    // returns and its own `tokio::select!` is entered -- a short but real gap
+    // after the client's `initialize()` call already unblocks (empirically,
+    // long enough to consistently lose a signal sent with no delay at all).
+    // In real usage a client stays connected far longer than this before a
+    // `SIGTERM` arrives, so this sleep reproduces that realistic ordering
+    // instead of racing an unrelated startup window that has nothing to do
+    // with #308.
+    std::thread::sleep(std::time::Duration::from_millis(200));
+
+    let pid = client.pid();
+    let status = std::process::Command::new("kill")
+        .args(["-TERM", &pid.to_string()])
+        .status()?;
+    assert!(
+        status.success(),
+        "failed to send SIGTERM to mcpls (pid {pid})"
+    );
+
+    let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5);
+    loop {
+        if let Some(exit_status) = client.try_wait()? {
+            // Distinguishes a graceful `process::exit(0)` from the process
+            // being killed outright by the default SIGTERM disposition
+            // (e.g. if the signal handler failed to register) -- the latter
+            // would also make `try_wait` return `Some`, but with no LSP
+            // shutdown having run. On Unix, `code()` is `None` for
+            // signal-termination, so this one assertion covers both.
+            assert_eq!(
+                exit_status.code(),
+                Some(0),
+                "mcpls should exit with status 0 via its own shutdown path, not be killed \
+                 by the default SIGTERM disposition (issue #308 regression)"
+            );
+            return Ok(());
+        }
+        assert!(
+            std::time::Instant::now() < deadline,
+            "mcpls did not exit within 5s of SIGTERM while the client's stdin write end \
+             was still open (issue #308 regression)"
+        );
+        std::thread::sleep(std::time::Duration::from_millis(50));
+    }
+}