diff --git a/README.md b/README.md index d8a101a..399cad4 100644 --- a/README.md +++ b/README.md @@ -502,10 +502,16 @@ Gotchas specific to the image: which `--rm` throws away; with one that uid 65532 cannot write, startup fails outright, and over HTTP the default fail mode is `closed_all`, so a directory that becomes unwritable later stops the server serving. -- **Use an init process.** bugwarden installs no `SIGTERM` handler, and PID 1 - does not get the default terminate action, so without `--init` (or - `init: true` in compose) `docker stop` waits out its full timeout and ends - in `SIGKILL`. +- **Signals.** The process handles `SIGINT` and `SIGTERM`. Over HTTP both + cancel the transport token and let axum drain. Over stdio both end the + process immediately (status 0): rmcp reads stdin on a blocking thread + that cannot be cancelled while a client still holds the pipe, so + returning from `main` would hang the runtime the same way the missing + handler did. As container PID 1 this is what makes `docker stop` / + `podman stop` / a Kubernetes SIGTERM a clean exit instead of waiting + out the runtime grace period and ending in SIGKILL (137). `--init` / + `init: true` remains useful as defense in depth — it reaps any + unexpected child — but it is no longer required for a timely stop. There is no `HEALTHCHECK`: `/bin` and `/usr/bin` are empty in this base, so there is no binary to run one with. Use a TCP check on the port, or an diff --git a/compose.yaml b/compose.yaml index 9794fd2..13afb93 100644 --- a/compose.yaml +++ b/compose.yaml @@ -32,8 +32,9 @@ services: # Run `mkdir -p ./audit && chown 65532:65532 ./audit` first. # - ./audit.toml:/etc/bugwarden/audit.toml:ro # - ./audit:/var/log/bugwarden - # PID 1 does not get SIGTERM's default terminate action and bugwarden - # installs no handler, so without an init `compose down` ends in SIGKILL. + # Defense in depth: bugwarden handles SIGTERM itself, so compose down + # without an init is a graceful stop. An init process still reaps + # unexpected children if the runtime ever grows any. init: true read_only: true cap_drop: diff --git a/crates/bugwarden/src/main.rs b/crates/bugwarden/src/main.rs index 76fff61..134ce85 100644 --- a/crates/bugwarden/src/main.rs +++ b/crates/bugwarden/src/main.rs @@ -134,11 +134,39 @@ async fn main() -> anyhow::Result<()> { match cfg.transport { Transport::Stdio => { + // Two stages: an unused stdio container sits in `serve` + // (handshake). After initialize it sits in `waiting`. + // Handlers register on the first poll of `shutdown`, which + // is this `select!` immediately after the startup line. + let shutdown = shutdown_signal(); + tokio::pin!(shutdown); tracing::info!("Starting Bugzilla MCP server on stdio"); - let service = server.serve(stdio()).await.inspect_err(|e| { - tracing::error!("serving error: {:?}", e); - })?; - service.waiting().await?; + let service = tokio::select! { + result = server.serve(stdio()) => { + result.inspect_err(|e| { + tracing::error!("serving error: {:?}", e); + })? + } + () = &mut shutdown => { + tracing::info!("received shutdown signal"); + // serve() is already blocked in tokio::io::stdin()'s + // uncancellable read. Returning from main drops the + // runtime onto that blocking thread. + std::process::exit(0); + } + }; + let cancel = service.cancellation_token(); + tokio::select! { + result = service.waiting() => { + result?; + } + () = shutdown => { + tracing::info!("received shutdown signal"); + cancel.cancel(); + // Same blocking stdin read as the handshake arm. + std::process::exit(0); + } + } } Transport::Http => { let ct = tokio_util::sync::CancellationToken::new(); @@ -174,7 +202,11 @@ async fn main() -> anyhow::Result<()> { router.into_make_service_with_connect_info::(), ) .with_graceful_shutdown(async move { - let _ = tokio::signal::ctrl_c().await; + shutdown_signal().await; + tracing::info!("received shutdown signal"); + // Tear the live streamable-HTTP transport down with the + // listener: axum's graceful shutdown alone waits for + // in-flight connections, and an open MCP session is one. ct.cancel(); }) .await?; @@ -183,3 +215,39 @@ async fn main() -> anyhow::Result<()> { Ok(()) } + +/// Wait until the process should stop serving. +/// +/// `SIGINT` (`ctrl_c`) and, on Unix, `SIGTERM`. As container PID 1 the +/// kernel does not apply SIGTERM's default terminate action, so without +/// this waiter `docker stop` / `podman stop` wait out the runtime grace +/// period and SIGKILL (issue #114). Both signals take the same path: the +/// caller then cancels whatever is serving (the HTTP transport token, or +/// the stdio `select!`). A failed SIGTERM install is logged and the +/// waiter falls back to SIGINT only — refusing to start would be worse +/// than a container that still needs `--init`. +async fn shutdown_signal() { + let ctrl_c = tokio::signal::ctrl_c(); + #[cfg(unix)] + { + match tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) { + Ok(mut term) => { + tokio::select! { + _ = ctrl_c => {} + _ = term.recv() => {} + } + } + Err(err) => { + tracing::error!( + error = %err, + "failed to install SIGTERM handler; only SIGINT will stop the process" + ); + let _ = ctrl_c.await; + } + } + } + #[cfg(not(unix))] + { + let _ = ctrl_c.await; + } +} diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index d512579..fbff30e 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -1492,7 +1492,8 @@ impl BugWarden { /// decide this one too rather than leave it behind. The remaining /// fields, and why each stays inherited, are inventoried in DESIGN.md /// under "rmcp 3.1 usage notes"; the caller in `main` adds - /// `cancellation_token` so shutdown reaches the live transport. + /// `cancellation_token` so a SIGINT or SIGTERM reaches the live + /// transport (issue #114). pub fn http_server_config(&self) -> StreamableHttpServerConfig { let config = StreamableHttpServerConfig::default() .disable_allowed_hosts() diff --git a/crates/bugwarden/tests/binary_shutdown.rs b/crates/bugwarden/tests/binary_shutdown.rs new file mode 100644 index 0000000..6824462 --- /dev/null +++ b/crates/bugwarden/tests/binary_shutdown.rs @@ -0,0 +1,327 @@ +//! The shipped binary stops on SIGTERM / SIGINT (issue #114). +//! +//! As container PID 1 the kernel does not apply SIGTERM's default terminate +//! action, so a process that only listened for SIGINT (`ctrl_c`) made +//! `docker stop` wait out its grace period and SIGKILL. These tests spawn +//! the real executable — the handlers live in `main` — and assert a +//! **graceful** exit (`status.code() == Some(0)`). An unhandled SIGTERM +//! still kills a non-PID-1 child, but with `code() == None` and +//! `signal() == SIGTERM`; requiring `Some(0)` is what makes a missing +//! handler fail this file. +//! +//! Coverage contract (each of these mutations must fail at least one test): +//! - HTTP graceful-shutdown waiting only on `ctrl_c` (no SIGTERM); +//! - stdio wrapping only `waiting()` and not `serve` (the handshake wait +//! is where an unused stdio container sits); +//! - stdio wrapping only `serve` and not `waiting()`; +//! - HTTP SIGTERM returning without `ct.cancel()`, leaving an open MCP +//! session to outlive the shutdown. + +#![cfg(unix)] + +use std::net::SocketAddr; +use std::process::Stdio; +use std::time::Duration; + +use rmcp::service::RoleClient; +use rmcp::service::RunningService; +use rmcp::transport::streamable_http_client::StreamableHttpClientTransportConfig; +use rmcp::transport::StreamableHttpClientTransport; +use rmcp::ServiceExt as _; +use tokio::io::AsyncBufReadExt; +use tokio::io::AsyncWriteExt; +use tokio::io::BufReader; +use tokio::process::Child; +use tokio::process::Command; + +/// Bounded so a binary that ignores the signal fails this test rather than +/// hanging the suite until CI's own timeout kills it. Well under docker's +/// default 10s grace, so a "it will die eventually" path cannot pass. +const EXIT_TIMEOUT: Duration = Duration::from_secs(5); + +/// Every environment variable the binary reads, cleared before each spawn. +const AMBIENT_VARS: &[&str] = &[ + "BUGZILLA_SERVER", + "BUGZILLA_API_KEY", + "BUGZILLA_API_KEY_FILE", + "BUGWARDEN_POLICY", + "BUGWARDEN_AUDIT_CONFIG", + "BUGWARDEN_HTTP_TOKEN", + "BUGWARDEN_HTTP_READ_TOKEN", + "BUGZILLA_USE_AUTH_HEADER", + "MCP_TRANSPORT", + "MCP_HOST", + "MCP_PORT", + "MCP_ALLOWED_HOSTS", + "MCP_READ_ONLY", + "MCP_API_KEY_HEADER", + "RUST_LOG", +]; + +/// The scrub list is only as good as its coverage of `Cli`. +#[test] +fn the_scrub_list_covers_every_environment_fallback() { + let mut cmd = bugwarden::config::command(); + cmd.build(); + let unscrubbed: Vec = cmd + .get_arguments() + .filter_map(clap::Arg::get_env) + .map(|env| env.to_string_lossy().into_owned()) + .filter(|env| !AMBIENT_VARS.contains(&env.as_str())) + .collect(); + assert!( + unscrubbed.is_empty(), + "these environment fallbacks reach the spawned binary: {unscrubbed:?}" + ); + for var in [ + bugwarden::http_auth::WRITE_TOKEN_VAR, + bugwarden::http_auth::READ_TOKEN_VAR, + ] { + assert!(AMBIENT_VARS.contains(&var), "{var} must be scrubbed"); + } +} + +/// A port to hand the child, chosen by binding and releasing an ephemeral +/// one. Racy in principle; the child's readiness poll below turns a lost +/// race into a test failure rather than a hang. +fn free_port() -> u16 { + std::net::TcpListener::bind("127.0.0.1:0") + .expect("bind") + .local_addr() + .expect("addr") + .port() +} + +fn send_signal(pid: u32, signal: &str) { + let status = std::process::Command::new("kill") + .args(["-s", signal, &pid.to_string()]) + .status() + .expect("kill must be executable"); + assert!(status.success(), "kill -s {signal} {pid} failed: {status}"); +} + +/// Spawn the shipped binary. Stdin is piped so a stdio child does not see +/// EOF the moment we start; stderr is piped so tests can wait on the +/// startup line. `kill_on_drop` reaps a child if the assertion panics. +fn spawn_binary(args: &[&str]) -> Child { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_bugwarden")); + cmd.args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + for var in AMBIENT_VARS { + cmd.env_remove(var); + } + cmd.env("RUST_LOG", "info"); + cmd.kill_on_drop(true); + cmd.spawn().expect("the built binary must start") +} + +async fn wait_for_tcp(addr: SocketAddr) { + let ready = tokio::time::timeout(EXIT_TIMEOUT, async { + loop { + if tokio::net::TcpStream::connect(addr).await.is_ok() { + return; + } + tokio::time::sleep(Duration::from_millis(25)).await; + } + }) + .await; + assert!(ready.is_ok(), "the binary must start serving on {addr}"); +} + +/// Block until `needle` appears on the child's stderr, so a signal is not +/// delivered before `shutdown_signal` is armed. +async fn wait_for_stderr(child: &mut Child, needle: &str) { + let stderr = child.stderr.as_mut().expect("stderr is piped"); + let mut lines = BufReader::new(stderr).lines(); + let found = tokio::time::timeout(EXIT_TIMEOUT, async { + loop { + let line = lines + .next_line() + .await + .expect("stderr must be readable") + .expect("the server must log the startup line before EOF"); + if line.contains(needle) { + return; + } + } + }) + .await; + assert!( + found.is_ok(), + "timed out waiting for {needle:?} on the child's stderr" + ); +} + +async fn assert_graceful_exit(child: Child, what: &str) { + let output = tokio::time::timeout(EXIT_TIMEOUT, child.wait_with_output()) + .await + .unwrap_or_else(|_| panic!("{what}: the process must exit within {EXIT_TIMEOUT:?}")) + .expect("the child must be waitable"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + output.status.code(), + Some(0), + "{what}: SIGTERM/SIGINT must be a clean exit 0, not a signal-kill \ + (code=None) or an error: status={status:?} stderr={stderr}", + status = output.status + ); + assert!( + stderr.contains("received shutdown signal"), + "{what}: the shutdown path must log that it ran: {stderr}" + ); +} + +async fn connect_insecure(addr: SocketAddr) -> RunningService { + let transport = StreamableHttpClientTransport::with_client( + reqwest::Client::new(), + StreamableHttpClientTransportConfig::with_uri(format!("http://{addr}/mcp")), + ); + ().serve(transport) + .await + .expect("MCP handshake must succeed under --insecure-no-auth") +} + +#[tokio::test] +async fn http_sigterm_exits_zero_on_an_idle_listener() { + let port = free_port(); + let child = spawn_binary(&[ + "--bugzilla-server", + "https://bugzilla.example.invalid", + "--host", + "127.0.0.1", + "--port", + &port.to_string(), + "--insecure-no-auth", + ]); + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().expect("addr"); + wait_for_tcp(addr).await; + let pid = child.id().expect("the child has a pid"); + send_signal(pid, "TERM"); + assert_graceful_exit(child, "http idle SIGTERM").await; +} + +#[tokio::test] +async fn http_sigint_still_exits_zero() { + let port = free_port(); + let child = spawn_binary(&[ + "--bugzilla-server", + "https://bugzilla.example.invalid", + "--host", + "127.0.0.1", + "--port", + &port.to_string(), + "--insecure-no-auth", + ]); + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().expect("addr"); + wait_for_tcp(addr).await; + let pid = child.id().expect("the child has a pid"); + send_signal(pid, "INT"); + assert_graceful_exit(child, "http idle SIGINT").await; +} + +#[tokio::test] +async fn http_sigterm_cancels_a_live_session() { + // axum's graceful shutdown waits for in-flight connections. A live + // streamable-HTTP session is one: without `ct.cancel()` the process + // stays up until this test's timeout, which is the mutation. + let port = free_port(); + let child = spawn_binary(&[ + "--bugzilla-server", + "https://bugzilla.example.invalid", + "--host", + "127.0.0.1", + "--port", + &port.to_string(), + "--insecure-no-auth", + ]); + let addr: SocketAddr = format!("127.0.0.1:{port}").parse().expect("addr"); + wait_for_tcp(addr).await; + let _session = connect_insecure(addr).await; + let pid = child.id().expect("the child has a pid"); + send_signal(pid, "TERM"); + assert_graceful_exit(child, "http live-session SIGTERM").await; +} + +#[tokio::test] +async fn stdio_sigterm_during_the_handshake_wait_exits_zero() { + // `serve(stdio())` blocks on initialize. A stdio container that no + // client has spoken to yet is in this wait, not in `waiting()`. + let mut child = spawn_binary(&[ + "--transport", + "stdio", + "--bugzilla-server", + "https://bugzilla.example.invalid", + "--api-key", + "test-key", + ]); + wait_for_stderr(&mut child, "Starting Bugzilla MCP server on stdio").await; + // Keep stdin open: wait_with_output drops it and EOF-unblocks the + // child's blocking read, so a handshake arm that only `return Ok(())` + // would go green. Matching the post-initialize test, take stdin and + // wait() instead. + let _stdin = child.stdin.take(); + let pid = child.id().expect("the child has a pid"); + send_signal(pid, "TERM"); + let status = tokio::time::timeout(EXIT_TIMEOUT, child.wait()) + .await + .expect("stdio pre-handshake SIGTERM: the process must exit") + .expect("the child must be waitable"); + assert_eq!( + status.code(), + Some(0), + "stdio pre-handshake SIGTERM: clean exit 0, not a signal-kill: {status:?}" + ); +} + +#[tokio::test] +async fn stdio_sigterm_after_initialize_exits_zero() { + let mut child = spawn_binary(&[ + "--transport", + "stdio", + "--bugzilla-server", + "https://bugzilla.example.invalid", + "--api-key", + "test-key", + ]); + wait_for_stderr(&mut child, "Starting Bugzilla MCP server on stdio").await; + + let mut stdin = child.stdin.take().expect("stdin is piped"); + let stdout = child.stdout.take().expect("stdout is piped"); + let mut stdout = BufReader::new(stdout).lines(); + stdin + .write_all( + br#"{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-11-25","capabilities":{},"clientInfo":{"name":"binary-shutdown-test","version":"0"}}} +"#, + ) + .await + .expect("the child must accept the handshake"); + let reply = tokio::time::timeout(EXIT_TIMEOUT, stdout.next_line()) + .await + .expect("the handshake must not hang") + .expect("stdout must be readable") + .expect("the server must answer initialize"); + assert!( + reply.contains("bugwarden"), + "initialize must complete before the signal: {reply}" + ); + + // Keep draining stdout so a cancel-path write cannot fill the pipe + // and block the child's serve loop. + let _drain = + tokio::spawn(async move { while stdout.next_line().await.ok().flatten().is_some() {} }); + + let pid = child.id().expect("the child has a pid"); + send_signal(pid, "TERM"); + // stdin/stdout already taken; wait() not wait_with_output. + let status = tokio::time::timeout(EXIT_TIMEOUT, child.wait()) + .await + .expect("stdio post-handshake SIGTERM: the process must exit") + .expect("the child must be waitable"); + assert_eq!( + status.code(), + Some(0), + "stdio post-handshake SIGTERM: clean exit 0, not a signal-kill: {status:?}" + ); +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d00a3bf..d97a525 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1538,9 +1538,9 @@ wired, `server.rs` and `main.rs` are the reference. is someone the operator issued a token to. `--insecure-no-auth` restores the older, wider exposure to everyone — one more thing that flag hands out. If a per-caller identity (#32) changes this calculus, revisit here - and at the add_attachment row together. The `cancellation_token` is named so ctrl_c - tears the live transport down with the process instead of leaving it to - outlive the shutdown. + and at the add_attachment row together. The `cancellation_token` is named so + SIGINT and SIGTERM tear the live transport down with the process instead of + leaving it to outlive the shutdown. An operator who does know the authorities their deployment answers to names them with a repeated `--allowed-hosts`, or with `MCP_ALLOWED_HOSTS` as one @@ -1606,7 +1606,7 @@ wired, `server.rs` and `main.rs` are the reference. update_bug_fields, update_bug_dependencies, add_cc_to_bug, mark_as_duplicate, create_bug, add_attachment. - API key resolution: a match on `key_custody` (resolved once at startup, see Key custody — never re-read per request): `Server(key)` => the server's key, without touching the request at all; `PerRequest` => `ctx.extensions.get::()`, then `parts.headers.get(lowercased_header_name)`. -- HTTP serving: `let config = server.http_server_config().with_cancellation_token(ct.child_token());` — built while `server` can still be borrowed, since the body cap comes from its own guard policy — then `StreamableHttpService::new(move || Ok(server.clone()), LocalSessionManager::default().into(), config)`, never a bare `StreamableHttpServerConfig::default()`, see the field table above — then `axum::Router::new().nest_service("/mcp", service)`, `tokio::net::TcpListener::bind`, graceful shutdown on ctrl_c cancelling `ct`. +- HTTP serving: `let config = server.http_server_config().with_cancellation_token(ct.child_token());` — built while `server` can still be borrowed, since the body cap comes from its own guard policy — then `StreamableHttpService::new(move || Ok(server.clone()), LocalSessionManager::default().into(), config)`, never a bare `StreamableHttpServerConfig::default()`, see the field table above — then `axum::Router::new().nest_service("/mcp", service)`, `tokio::net::TcpListener::bind`, graceful shutdown on SIGINT or SIGTERM cancelling `ct` (`shutdown_signal` in main.rs; issue #114). Stdio uses the same waiter across `serve` (the handshake wait an unused stdio container sits in) and `waiting`; a signal at either stage `process::exit(0)`s, because rmcp's stdio transport reads stdin via `spawn_blocking` and that read does not unblock while the client holds the pipe — returning from `main` drops the runtime onto that thread. - Request `_meta` (SEP-414, e.g. `traceparent`): over every serialized transport the wire `params._meta` does NOT arrive in the params struct (`CallToolRequestParams.meta` stays `None`) — the SDK's custom @@ -2063,5 +2063,17 @@ wired, `server.rs` and `main.rs` are the reference. `WRITE_TOOLS` is additionally pinned against every tool's own `read_only_hint` annotation, so the read scope cannot drift from what clients are told. +- Process-shutdown tests (crates/bugwarden/tests/binary_shutdown.rs, the + SHIPPED BINARY, Unix only): SIGTERM and SIGINT on an idle HTTP listener + both exit `0` within 5s and log `received shutdown signal` — an unhandled + SIGTERM still kills a non-PID-1 child, but with `code() == None`, so + requiring `Some(0)` is what makes a missing handler fail; SIGTERM with a + live streamable-HTTP session still exits `0` in the same bound, which is + what `ct.cancel()` is for (axum's graceful shutdown alone waits for that + connection); stdio SIGTERM during the initialize wait (where an unused + stdio container sits) and after a completed handshake both exit `0` with + the child's stdin still held, so wrapping only `serve` or only `waiting` + fails, and a handshake arm that only `return Ok(())` cannot go green + via `wait_with_output` dropping the pipe. - CI: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace --locked`, `cargo deny check`.