diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e1daba8..3d9bc4d4 100644 --- a/CHANGELOG.md +++ b/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::()`, 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) diff --git a/crates/mcpls-cli/src/main.rs b/crates/mcpls-cli/src/main.rs index a9255fcc..83eef5b5 100644 --- a/crates/mcpls-cli/src/main.rs +++ b/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<()> { diff --git a/crates/mcpls-core/README.md b/crates/mcpls-core/README.md index 75ae4de6..712677c6 100644 --- a/crates/mcpls-core/README.md +++ b/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 }); } ``` diff --git a/crates/mcpls-core/src/lib.rs b/crates/mcpls-core/src/lib.rs index 534746fa..83c87116 100644 --- a/crates/mcpls-core/src/lib.rs +++ b/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 { /// - **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,6 +648,18 @@ 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(())) => {} @@ -626,6 +667,7 @@ async fn await_lsp_init_handle(mut handle: JoinHandle<()>, timeout: Duration) { Err(_) => { warn!("Timed out waiting for background LSP initialization task to stop"); handle.abort(); + let _ = tokio::time::timeout(Duration::from_secs(1), handle).await; } } } diff --git a/crates/mcpls-core/src/transport.rs b/crates/mcpls-core/src/transport.rs index dd663b2b..65a067e7 100644 --- a/crates/mcpls-core/src/transport.rs +++ b/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>, diff --git a/crates/mcpls-core/tests/e2e/mcp_client.rs b/crates/mcpls-core/tests/e2e/mcp_client.rs index 6e3394a9..37e7e56e 100644 --- a/crates/mcpls-core/tests/e2e/mcp_client.rs +++ b/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> { + self.process.try_wait() + } } impl Drop for McpClient { diff --git a/crates/mcpls-core/tests/e2e/protocol_tests.rs b/crates/mcpls-core/tests/e2e/protocol_tests.rs index 5330ce7c..125c9748 100644 --- a/crates/mcpls-core/tests/e2e/protocol_tests.rs +++ b/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)); + } +}