From c9a8ddaa7db5c03eae8ee4f286728e6f93b5e2be Mon Sep 17 00:00:00 2001 From: Thomas <155702229+MakerViking@users.noreply.github.com> Date: Sun, 16 Aug 2026 21:28:55 +0200 Subject: [PATCH] fix(mcp): exit when the client dies, not when stdin says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen orphaned `mimir mcp` processes had accumulated on a real machine, the oldest fourteen days old, ~2.9 GB resident between them, each holding the 651 MB SQLite database open. The reported hypothesis was that the serve loop treats a zero-length read as "no data yet". It does not. rmcp's transport returns `None` on `Ok(0)` (async_rw.rs:129), the service task ends, `waiting()` resolves. Measured: close stdin and the process exits in 0.3 s, rc=0, repeatably. A fix aimed at EOF handling would have changed nothing. The EOF never arrives. `/proc//fd/0` on a live server is `socket:[...]`, not a pipe — MCP stdio is an AF_UNIX socketpair, and a socketpair delivers EOF only once EVERY descriptor for the peer end is closed. Any unrelated process that inherited it — a background command, another server from the same client — pins it open, so when the client exits the kernel delivers nothing and the reader blocks forever. Reproduced 2/2 with a deliberately leaked descriptor, with the leaker's fd verified pointing at the same socket inode. So the trigger cannot be a descriptor. It has to be the parent dying: PR_SET_PDEATHSIG on Linux, a getppid() poll for macOS and for the arming race, SIGTERM/SIGHUP through the same path. Cancelling rmcp's token ends the service task and resolves the `waiting()` already parked on, so shutdown runs the same course as a clean EOF and the engine — and its database handle — drops on the way out. Two things the tests caught that review would not have: - The ppid snapshot must be taken in `main`, not at the watchdog. The server spends a second or two loading models; a client that exits during that window is already gone, so a ppid read there returns the reaper, compares equal to itself forever, and the watchdog never fires. - The watchdog must be armed BEFORE `serve()`, which awaits the MCP `initialize` handshake and blocks indefinitely against a client that spawns the server and dies without initializing. Armed afterwards, that entire window is unguarded — and it is the window the regression test happened to exercise, which is the only reason it was found. Both were live bugs in the first two versions of this fix. Each test was run against a disabled watchdog to confirm it fails without it; the parent-death test also had to stop redirecting the server's stdin from /dev/null (a non-interactive shell does that to background jobs), which had been handing it an instant EOF and making it pass against an unfixed binary. Residual, deliberately not papered over: a parent that dies in the microseconds between exec and `record_parent_pid` still cannot be detected. That is not the failure that produced these orphans. `--http` untouched, request handling untouched, no idle timeout. fmt, clippy -D warnings, 389 tests; manually verified that closing stdin and killing the parent both leave nothing behind. --- CHANGELOG.md | 17 +++++ crates/mimir-cli/src/main.rs | 6 ++ crates/mimir-cli/src/mcp.rs | 135 +++++++++++++++++++++++++++++++++ crates/mimir-cli/tests/e2e.rs | 136 ++++++++++++++++++++++++++++++++++ 4 files changed, 294 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43b95cc..070c43c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,23 @@ All notable changes are documented here. Versions follow semver; the CLI, the `mimir-mem` crate, and the on-disk schema move together. +## [Unreleased] +### Fixed +- **`mimir mcp` no longer outlives the client that spawned it.** Fourteen + orphaned stdio servers had accumulated on one machine, the oldest fourteen + days old, ~2.9 GB resident between them, every one holding the SQLite + database open. The EOF handling was never at fault — rmcp returns `None` on + a zero-length read and the process exits in 0.3 s when stdin closes. The + EOF simply never arrived: MCP stdio is an `AF_UNIX` **socketpair**, not a + pipe, and a socketpair delivers EOF only once *every* descriptor for the + peer end is closed, so any unrelated process that inherited it pins the + server open forever. The trigger therefore cannot be a descriptor: + `PR_SET_PDEATHSIG` (Linux) plus a `getppid()` poll (macOS, and to close the + arming race) now end the process when its parent does, and SIGTERM/SIGHUP + route through the same path — cancel the service, drain in flight, drop the + engine and its database handle. `--http` is untouched: `mimir daemon` has + its own lifetime and arms none of this. + ## [Unreleased] ### Added - **`mimir link --scan --all-projects`, and a `doctor` nudge to run it.** diff --git a/crates/mimir-cli/src/main.rs b/crates/mimir-cli/src/main.rs index 465f5fc..5a4be9b 100644 --- a/crates/mimir-cli/src/main.rs +++ b/crates/mimir-cli/src/main.rs @@ -661,6 +661,12 @@ fn main() { libc::signal(libc::SIGPIPE, libc::SIG_DFL); } + // Before anything slow. The stdio MCP server uses this to notice that the + // client that spawned it has died; read any later and a client that exits + // during startup is already gone, leaving nothing to compare against. + #[cfg(unix)] + mcp::record_parent_pid(); + // Windows hands the main thread a 1 MiB stack (Linux/macOS give 8). // clap's derived builder for our ~50 subcommands needs more than that // in debug builds, so `mimir --version` aborted with a stack overflow diff --git a/crates/mimir-cli/src/mcp.rs b/crates/mimir-cli/src/mcp.rs index 4c3a594..6a9338b 100644 --- a/crates/mimir-cli/src/mcp.rs +++ b/crates/mimir-cli/src/mcp.rs @@ -1058,9 +1058,18 @@ pub fn run( if !daemon_alive { eager_load_reranker_if_auto(&mut engine); } + // Armed BEFORE `serve`, which awaits the MCP `initialize` + // handshake and therefore blocks indefinitely against a client + // that spawns the server and dies without ever initializing — + // an orphan window that arming afterwards cannot see. Stdio + // only: the HTTP path is a daemon with its own lifetime. + #[cfg(unix)] + arm_shutdown_watchdog(); let service = MimirServer::new(engine, project_id) .serve(rmcp::transport::stdio()) .await?; + #[cfg(unix)] + register_shutdown_token(service.cancellation_token()); service.waiting().await?; Ok(()) } @@ -1068,6 +1077,132 @@ pub fn run( }) } +/// The parent pid as it was at process start. +/// +/// Captured in `main`, microseconds after exec, and deliberately not where +/// it is used. The stdio server spends a second or two loading models before +/// it starts serving, and a client that exits during that window is already +/// gone by then — a ppid read taken at that point returns the *reaper*, which +/// then compares equal to itself forever and the watchdog never fires. That +/// is not hypothetical: it is what the first version of this did, and the +/// regression test caught it. +#[cfg(unix)] +static PARENT_AT_START: std::sync::atomic::AtomicI32 = std::sync::atomic::AtomicI32::new(0); + +/// Record the parent pid. Call once, as early in `main` as possible. +#[cfg(unix)] +pub fn record_parent_pid() { + PARENT_AT_START.store( + unsafe { libc::getppid() }, + std::sync::atomic::Ordering::Relaxed, + ); +} + +/// Set by the SIGTERM/SIGHUP handler. A relaxed store is the whole handler +/// body on purpose — it is the only thing here that is async-signal-safe. +#[cfg(unix)] +static SHUTDOWN_REQUESTED: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +#[cfg(unix)] +extern "C" fn note_shutdown(_sig: libc::c_int) { + SHUTDOWN_REQUESTED.store(true, std::sync::atomic::Ordering::Relaxed); +} + +/// Exit when the client goes away, for the cases where stdin EOF never comes. +/// +/// The stdio server's EOF handling is correct and needs no help: rmcp's +/// transport returns `None` on a zero-length read and the service task ends. +/// The problem is that the EOF can simply never arrive. Claude Code wires MCP +/// stdio over an `AF_UNIX` **socketpair**, not a pipe, and a socketpair +/// delivers EOF only once *every* descriptor for the peer end is closed. Any +/// unrelated process that inherited that descriptor — a background command, +/// another server spawned by the same client — pins it open, so when the +/// client exits the kernel delivers nothing at all and the reader blocks +/// forever. Measured before this existed: fourteen orphans, the oldest +/// fourteen days old, ~2.9 GB resident between them, each holding the SQLite +/// database open. +/// +/// So the trigger cannot be a descriptor. It has to be the parent dying: +/// +/// * `PR_SET_PDEATHSIG` (Linux) has the kernel send us SIGTERM the moment the +/// parent does, regardless of who else holds the socket. +/// * A `getppid()` poll covers macOS, which has no equivalent, and closes the +/// race where the parent died in the window *before* `prctl` ran — that +/// signal is never sent, so nothing but a check would catch it. +/// +/// The poll compares against the ppid captured at startup rather than testing +/// for `1`, because a server legitimately started by pid 1 would otherwise +/// shut itself down immediately. +/// +/// Cancelling the token ends the service task, which resolves the `waiting()` +/// the caller is already parked on, so shutdown runs through exactly the same +/// path as a clean EOF: in-flight work finishes, the transport closes, the +/// engine drops and with it the database handle. +/// Where `arm_shutdown_watchdog` finds the token once there is one. Empty +/// until the handshake completes. +#[cfg(unix)] +static SHUTDOWN_TOKEN: std::sync::Mutex> = + std::sync::Mutex::new(None); + +/// Hand the watchdog a way to shut the service down gracefully. Before this, +/// it can only exit the process — which is correct, because before the +/// handshake there is no service to drain. +#[cfg(unix)] +fn register_shutdown_token(token: rmcp::service::RunningServiceCancellationToken) { + *SHUTDOWN_TOKEN.lock().unwrap() = Some(token); +} + +#[cfg(unix)] +fn arm_shutdown_watchdog() { + use std::sync::atomic::Ordering; + + #[cfg(target_os = "linux")] + // SAFETY: prctl with PR_SET_PDEATHSIG only sets this process's + // parent-death signal; it touches no memory we own. + unsafe { + libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM); + } + + let handler = note_shutdown as extern "C" fn(libc::c_int); + // SAFETY: installing a handler whose body is a single relaxed atomic + // store, which is async-signal-safe. + unsafe { + libc::signal(libc::SIGTERM, handler as libc::sighandler_t); + libc::signal(libc::SIGHUP, handler as libc::sighandler_t); + } + + // Recorded in `main`, not here — see PARENT_AT_START. A zero means the + // recorder was never called; treat that as "cannot tell" and rely on the + // signal paths rather than shutting down on a bogus comparison. + let parent_at_start = PARENT_AT_START.load(Ordering::Relaxed); + + std::thread::spawn(move || { + loop { + if SHUTDOWN_REQUESTED.load(Ordering::Relaxed) { + break; + } + if parent_at_start != 0 && unsafe { libc::getppid() } != parent_at_start { + break; + } + std::thread::sleep(std::time::Duration::from_millis(250)); + } + // A token exists only after the handshake. Before that there is no + // service, no in-flight request and nothing to drain, so exiting is + // the whole of a correct shutdown. + match SHUTDOWN_TOKEN.lock().unwrap().take() { + Some(token) => token.cancel(), + None => std::process::exit(0), + } + // Backstop. Installing a SIGTERM handler means the default "die now" + // action is gone, so a wedged shutdown would leave a process that + // ignores SIGTERM — strictly worse than the orphan this fixes. Give + // the graceful path time to finish, then leave anyway. + std::thread::sleep(std::time::Duration::from_secs(10)); + std::process::exit(0); + }); +} + /// Serve the same MCP tools over Streamable-HTTP for remote clients reached /// through a tunnel / reverse proxy. Each session gets its own engine (its own /// SQLite connection; WAL handles the concurrency). The loopback bind is the diff --git a/crates/mimir-cli/tests/e2e.rs b/crates/mimir-cli/tests/e2e.rs index 1c4b0ba..0f39871 100644 --- a/crates/mimir-cli/tests/e2e.rs +++ b/crates/mimir-cli/tests/e2e.rs @@ -1236,3 +1236,139 @@ fn session_brief_fires_suppresses_and_respects_kill_switch() { let preview = h.ok(&["brief"]); assert!(preview.contains("candidates (score desc"), "{preview}"); } + +/// Shutdown of the stdio server (`mimir mcp`). +/// +/// These exist because fourteen orphaned servers accumulated on a real +/// machine, the oldest fourteen days old, each holding the SQLite database +/// open. The EOF path was never the bug — it works, and the first test here +/// pins that — so a fix that only hardened EOF handling would have changed +/// nothing. The other two cover the ways the client can vanish *without* +/// producing an EOF. +#[cfg(unix)] +mod shutdown { + use super::*; + use std::process::Stdio; + use std::time::{Duration, Instant}; + + /// Poll for exit rather than `wait()`, so a hang fails the test instead + /// of hanging the suite. + fn exits_within(child: &mut std::process::Child, secs: u64) -> bool { + let deadline = Instant::now() + Duration::from_secs(secs); + while Instant::now() < deadline { + if child.try_wait().unwrap().is_some() { + return true; + } + std::thread::sleep(Duration::from_millis(100)); + } + let _ = child.kill(); + let _ = child.wait(); + false + } + + fn spawn_server(h: &Harness) -> std::process::Child { + Command::new(env!("CARGO_BIN_EXE_mimir")) + .arg("mcp") + .env("MIMIR_HOME", h.home.path()) + .env("HOME", h.home.path()) + .current_dir(h.cwd.path()) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawns") + } + + #[test] + fn exits_when_stdin_closes() { + let h = Harness::new(); + h.ok(&["init", "--no-model"]); + let mut child = spawn_server(&h); + drop(child.stdin.take()); // client closed the pipe + assert!( + exits_within(&mut child, 10), + "server outlived its stdin closing" + ); + } + + /// The client is gone but something else still holds the descriptor, so + /// no EOF is ever delivered. Before the parent-death watchdog this ran + /// forever. + /// + /// `sh` spawns the server and exits immediately, which reparents it + /// exactly as a departing client does — after staying alive for a few + /// seconds first, which is the real shape of the bug: a client serves a + /// session and *then* exits. (A parent that dies during the server's own + /// startup is a genuine residual gap, documented on `watch_for_shutdown`; + /// it is a millisecond-wide window and not what produced the orphans.) + /// The server's stdin stays the pipe + /// whose write end this test holds for the whole run, so EOF cannot be + /// what ends it; the pid goes through a file rather than stdout, because + /// the server inherits `sh`'s stdout and reading that to EOF would block + /// until the server had already exited — which silently made an earlier + /// version of this test pass against the unfixed binary. + #[test] + fn exits_when_the_parent_dies_without_eof() { + let h = Harness::new(); + h.ok(&["init", "--no-model"]); + let pidfile = h.cwd.path().join("server.pid"); + let mut launcher = Command::new("sh") + .arg("-c") + // `exec 3<&0` then `<&3`: a non-interactive shell redirects a + // background job's stdin from /dev/null, which would hand the + // server an instant EOF and make this test pass against an + // unfixed binary. Duplicating the descriptor defeats that. + .arg(format!( + "exec 3<&0; {} mcp <&3 >/dev/null 2>&1 & echo $! > {}; sleep 4", + env!("CARGO_BIN_EXE_mimir"), + pidfile.display() + )) + .env("MIMIR_HOME", h.home.path()) + .env("HOME", h.home.path()) + .current_dir(h.cwd.path()) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawns"); + let stdin_holder = launcher.stdin.take().expect("piped"); // held open + launcher.wait().unwrap(); // sh exits; the server is reparented + + let pid: i32 = std::fs::read_to_string(&pidfile) + .expect("pidfile") + .trim() + .parse() + .expect("pid"); + + let deadline = Instant::now() + Duration::from_secs(15); + let mut gone = false; + while Instant::now() < deadline { + // signal 0 probes liveness without delivering anything + if unsafe { libc::kill(pid, 0) } != 0 { + gone = true; + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + drop(stdin_holder); + if !gone { + unsafe { libc::kill(pid, libc::SIGKILL) }; + } + assert!(gone, "server survived its parent with stdin still open"); + } + + #[test] + fn exits_on_sigterm() { + let h = Harness::new(); + h.ok(&["init", "--no-model"]); + let mut child = spawn_server(&h); + let _stdin = child.stdin.take(); // keep it open: no EOF + std::thread::sleep(Duration::from_millis(500)); + unsafe { libc::kill(child.id() as i32, libc::SIGTERM) }; + assert!( + exits_within(&mut child, 10), + "server ignored SIGTERM — installing a handler removed the \ + default action, so this must work" + ); + } +}