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" + ); + } +}