diff --git a/Cargo.lock b/Cargo.lock index 4afc51b3..2475fff2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -883,6 +883,7 @@ dependencies = [ "clap", "coppa-audio", "ctrlc", + "libc", "manta-decode", "manta-engine", "manta-input", diff --git a/Cargo.toml b/Cargo.toml index fbd0bc51..f1a31e77 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,7 @@ coppa-audio = { git = "https://github.com/HagaleTechnologies/coppa.git", rev = " coppa-channel = { git = "https://github.com/HagaleTechnologies/coppa.git", rev = "f8a4d16df7e5776a0756943c05712038774e6c70" } cpal = "0.18" -ctrlc = "3" +ctrlc = { version = "3", features = ["termination"] } libc = "0.2" anyhow = "1.0" diff --git a/Dockerfile b/Dockerfile index e6b0ab9d..65197a18 100644 --- a/Dockerfile +++ b/Dockerfile @@ -41,23 +41,22 @@ WORKDIR /home/manta # metrics (:7302) -- ARCHITECTURE.md §7/§8. Defaults; override via config. EXPOSE 7300 7301 7302 -# `docker stop` sends SIGTERM by default, but manta-cli's shutdown -# handling (ctrlc, without its optional `termination` feature) only -# registers for SIGINT -- an unmodified SIGTERM would kill the process -# directly, bypassing manta_engine::listen's cleanup, track finalization, -# and the server-drain sequence, and potentially dropping final spots on -# every routine container shutdown (PR #78 review round 1). Retargeting -# the stop signal to SIGINT here is the in-scope fix for this -# release-pipeline PR; switching manta-cli's own ctrlc feature flags is a -# separate, broader behavior change to the application itself. -STOPSIGNAL SIGINT +# No STOPSIGNAL override. This image carried `STOPSIGNAL SIGINT` from PR +# #78 until MAN-85, because manta-cli's `ctrlc` handler was registered for +# SIGINT only and `docker stop`'s default SIGTERM killed the process +# outright -- bypassing manta_engine::listen's cleanup, track finalization +# and the server-drain sequence. MAN-85 enabled `ctrlc`'s `termination` +# feature, so SIGINT, SIGTERM and SIGHUP now all drive the same drain +# path; Docker's default stop signal is the correct one again, and +# retargeting it would only hide whether that path still works. +# `crates/manta-cli/tests/signal_shutdown.rs` keeps both halves honest. # `docker stop`'s own default grace period (10s on Linux) before SIGKILL # is SHORTER than manta-cli's own supported graceful-shutdown drain # window (SHUTDOWN_DRAIN_DEADLINE, 25s -- crates/manta-cli/src/main.rs) # for a legitimately-slow client's final write (PR #78 review round 5). -# STOPSIGNAL alone sends the right signal, but the container can still be -# SIGKILLed mid-drain, dropping the final spots this fix exists to +# Handling the signal is only half of it: the container can still be +# SIGKILLed mid-drain, dropping the final spots the drain exists to # preserve. A Dockerfile has no way to change the CALLER's stop grace # period -- operators must pass it explicitly: `docker stop -t 30 # `, or `--stop-timeout 30` on `docker run`, or the diff --git a/crates/manta-cli/Cargo.toml b/crates/manta-cli/Cargo.toml index 5fed79d5..10670c10 100644 --- a/crates/manta-cli/Cargo.toml +++ b/crates/manta-cli/Cargo.toml @@ -34,6 +34,7 @@ tracing-subscriber = { workspace = true } [dev-dependencies] coppa-audio = { workspace = true } +libc = { workspace = true } serde_json = { workspace = true } manta-testkit = { workspace = true } tempfile = { workspace = true } diff --git a/crates/manta-cli/src/main.rs b/crates/manta-cli/src/main.rs index 0b1ab0e8..6e864d15 100644 --- a/crates/manta-cli/src/main.rs +++ b/crates/manta-cli/src/main.rs @@ -1170,6 +1170,20 @@ fn main() -> Result<()> { ctrlc::set_handler(move || { stop_handler.store(true, std::sync::atomic::Ordering::Relaxed); })?; + // Printed AFTER the handler is installed, and via `eprintln!` + // rather than `tracing::info!` because the subscriber is only + // initialized inside `start_spot_server` -- a plain `listen` + // (no --server-config) has no subscriber at all. Two jobs: + // `listen` otherwise prints nothing at startup (2026-09-05 + // review, lens 1 #4/#7), and it is the readiness handshake + // `tests/signal_shutdown.rs` waits for -- signalling any + // earlier races `set_handler` and kills the child under the OS + // default disposition regardless of MAN-85's fix. If the + // fuller startup banner (lens 1 #7) ever replaces this line, + // it must still be emitted here, after `set_handler`, and + // `READY_MARKER` updated to match. stdout stays pure JSON + // under `--json` (MAN-59 round 6); this goes to stderr. + eprintln!("manta: listening; send SIGINT or SIGTERM to stop"); let listen_result = manta_engine::listen( src, &cfg, diff --git a/crates/manta-cli/tests/signal_shutdown.rs b/crates/manta-cli/tests/signal_shutdown.rs new file mode 100644 index 00000000..0e1ae8c9 --- /dev/null +++ b/crates/manta-cli/tests/signal_shutdown.rs @@ -0,0 +1,185 @@ +//! MAN-85: SIGTERM (and SIGHUP) must enter the same graceful-shutdown drain +//! path SIGINT already uses, and the process must exit 0 rather than with a +//! signal-derived code. +//! +//! Nothing else in this repo exercises real OS signal delivery, and no +//! `#[cfg]` in this crate can observe a *dependency's* feature flag, so this +//! file is the only thing standing between `ctrlc`'s `termination` feature +//! and a silent regression: drop the feature from the workspace `Cargo.toml` +//! and `sigterm_exits_zero_through_the_drain_path` goes red. +#![cfg(unix)] + +use std::io::{BufRead, BufReader}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; +use std::sync::OnceLock; +use std::time::{Duration, Instant}; + +/// Printed by `manta listen` immediately after `ctrlc::set_handler` returns. +/// Signalling before that line appears would race the handler registration +/// and kill the child under the OS default disposition no matter what this +/// ticket changed. +const READY_MARKER: &str = "manta: listening;"; + +/// Replay seconds in the fixture. Only has to outlast the test window -- +/// `control_still_running` below turns "too short for this machine" into an +/// explicit failure rather than a false pass. +const FIXTURE_SECONDS: f64 = 300.0; + +/// Generous upper bound on signal -> exit. The real number here is ~20 ms +/// (no clients connected, so `tasks::await_all` returns immediately); this +/// only has to be far below the fixture's natural EOF. +const EXIT_BUDGET: Duration = Duration::from_secs(15); + +/// One 300 s fixture for the whole file: ~115 MB and a full render, not +/// worth paying three times. Written under `CARGO_TARGET_TMPDIR` (cleaned +/// by `cargo clean`) rather than a `tempfile::TempDir`: Rust never runs +/// destructors for statics, so a shared `TempDir` here would leak 115 MB +/// into the system temp directory on every run. Rendered to a +/// process-unique name and atomically renamed into place, so two +/// concurrent `cargo test` invocations cannot observe a half-written WAV. +fn fixture_wav() -> &'static Path { + static WAV: OnceLock = OnceLock::new(); + WAV.get_or_init(|| { + let root = Path::new(env!("CARGO_TARGET_TMPDIR")).join("man85-signal-fixture"); + let wav = root.join("v1.wav"); + if wav.exists() { + return wav; + } + let staging = root.with_extension(format!("staging.{}", std::process::id())); + std::fs::create_dir_all(&staging).unwrap(); + let spec = manta_testkit::vectors::VectorSpec { + fs: 48_000.0, + duration_s: FIXTURE_SECONDS, + ..manta_testkit::vectors::v1() + }; + manta_testkit::vectors::write_fixture_set(&spec, &staging).unwrap(); + // Loser of a rename race: another process already published one. + if std::fs::rename(&staging, &root).is_err() { + let _ = std::fs::remove_dir_all(&staging); + } + assert!( + wav.exists(), + "fixture missing after publish: {}", + wav.display() + ); + wav + }) + .as_path() +} + +fn spawn_listen() -> Child { + Command::new(env!("CARGO_BIN_EXE_manta")) + .arg("listen") + .arg("--source") + .arg(fixture_wav()) + .arg("--json") + .stdout(Stdio::null()) + .stderr(Stdio::piped()) + .spawn() + .unwrap() +} + +/// Blocks until the child has registered its signal handler. +fn await_ready(child: &mut Child) { + let stderr = child.stderr.take().expect("stderr piped"); + for line in BufReader::new(stderr).lines() { + if line.unwrap().contains(READY_MARKER) { + return; + } + } + panic!("manta exited before printing {READY_MARKER:?}"); +} + +fn wait_bounded(child: &mut Child, budget: Duration) -> Option { + let start = Instant::now(); + loop { + if let Some(status) = child.try_wait().unwrap() { + return Some(status); + } + if start.elapsed() > budget { + return None; + } + std::thread::sleep(Duration::from_millis(20)); + } +} + +fn assert_signal_drains_and_exits_zero(sig: libc::c_int, name: &str) { + // `control` is the same command, never signalled. It is what makes the + // assertion below mean something: without it, a fixture short enough to + // hit EOF inside the test window would exit 0 on its own and this test + // would pass even with SIGTERM completely unhandled. + let mut control = spawn_listen(); + let mut child = spawn_listen(); + await_ready(&mut control); + await_ready(&mut child); + + assert!( + child.try_wait().unwrap().is_none(), + "manta exited on its own before {name} was sent" + ); + let sent = Instant::now(); + assert_eq!( + unsafe { libc::kill(child.id() as i32, sig) }, + 0, + "kill({name}) failed: {}", + std::io::Error::last_os_error() + ); + let status = wait_bounded(&mut child, EXIT_BUDGET) + .unwrap_or_else(|| panic!("{name} did not stop manta within {EXIT_BUDGET:?}")); + let elapsed = sent.elapsed(); + + let control_still_running = control.try_wait().unwrap().is_none(); + let _ = control.kill(); + let _ = control.wait(); + assert!( + control_still_running, + "an unsignalled run of the same fixture also finished within {elapsed:?}, so this \ + test cannot attribute the exit to {name} -- raise FIXTURE_SECONDS" + ); + + assert_eq!( + status.code(), + Some(0), + "{name} must run the shutdown drain and exit 0, got {status:?} after {elapsed:?}" + ); +} + +#[test] +fn sigterm_exits_zero_through_the_drain_path() { + assert_signal_drains_and_exits_zero(libc::SIGTERM, "SIGTERM"); +} + +#[test] +fn sigint_exits_zero_through_the_drain_path() { + assert_signal_drains_and_exits_zero(libc::SIGINT, "SIGINT"); +} + +#[test] +fn sighup_exits_zero_through_the_drain_path() { + // Not in MAN-85's Gherkin, but `ctrlc`'s `termination` feature is + // all-or-nothing: it registers SIGINT, SIGTERM *and* SIGHUP behind one + // signal-agnostic `FnMut()` closure. Pinning SIGHUP here makes that a + // deliberate, documented behaviour rather than an accident, and forces + // any future SIGHUP-triggered config reload (broad-review R-08 / + // MAN-30) to confront the fact that SIGHUP already means "shut down". + assert_signal_drains_and_exits_zero(libc::SIGHUP, "SIGHUP"); +} + +/// The `STOPSIGNAL SIGINT` workaround this ticket removes must not come +/// back: with SIGTERM handled, retargeting the container stop signal only +/// hides whether the real path works. +#[test] +fn dockerfile_does_not_retarget_the_container_stop_signal() { + let path = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../Dockerfile"); + let text = std::fs::read_to_string(&path).unwrap(); + let directive = text + .lines() + .map(str::trim_start) + .find(|l| l.starts_with("STOPSIGNAL")); + assert!( + directive.is_none(), + "manta handles SIGTERM natively since MAN-85; the image must use Docker's default \ + stop signal, found: {directive:?}" + ); +} diff --git a/docs/DECISIONS/2026-09-07-man85-signal-handling.md b/docs/DECISIONS/2026-09-07-man85-signal-handling.md new file mode 100644 index 00000000..b2b8e6aa --- /dev/null +++ b/docs/DECISIONS/2026-09-07-man85-signal-handling.md @@ -0,0 +1,90 @@ +# MAN-85: one signal path for SIGINT, SIGTERM and SIGHUP + +`manta listen` registers exactly one signal handler, in +`crates/manta-cli/src/main.rs`, via `ctrlc::set_handler`. Until MAN-85 the +workspace declared `ctrlc = "3"` with no features, and `ctrlc`'s SIGTERM/ +SIGHUP `sigaction` calls are compiled in only under its `termination` +feature (`ctrlc-3.5.2/src/platform/unix/mod.rs`). SIGTERM therefore kept +the OS default disposition: the kernel killed the process before +`manta_engine::listen`'s `stop` flag could be observed, so the entire +shutdown sequence -- `shutdown_tx.send(true)`, each client task's +drain-and-exit branch, `tasks::await_all` under `SHUTDOWN_DRAIN_DEADLINE` +-- never ran. Measured on `e398d46`, plain `listen`: SIGINT exit 0 in +18 ms; SIGTERM exit 143 in 11 ms. Every real service manager (systemd, +Docker, Kubernetes) sends SIGTERM on stop. + +## Options considered + +1. **Enable `ctrlc`'s `termination` feature.** One line in the workspace + `Cargo.toml`; no `main.rs` change, because the existing handler closure + already treats "the registered signal fired" as one undifferentiated + event. +2. **Move to `tokio::signal::unix::signal`** with SIGTERM and SIGINT both + wired to the stop flag, as the ticket's technical notes suggest. + +## Decision: option 1 + +A `tokio::signal` future only fires if a tokio reactor polls it, and this +codebase constructs a `tokio::runtime::Runtime` in exactly one place -- +`start_spot_server`, reached only when `--server-config` is given. A plain +`manta listen --source foo.wav` runs with no runtime at all, so option 2 +would mean either standing up a runtime purely to host a signal task, or +leaving plain `listen` on `ctrlc` while server mode alone moved to +`tokio::signal` -- two signal paths, which is the opposite of what the +ticket asks for. Option 1 sits below that split entirely: it changes only +which signals reach the already-shared `stop` flag. + +`termination = []` in `ctrlc`'s manifest carries no dependencies, so +enabling it leaves `Cargo.lock` byte-identical apart from the new `libc` +dev-dependency this ticket's test needs. + +## Accepted consequence: SIGHUP now means "shut down" + +`ctrlc`'s `termination` feature is all-or-nothing -- it registers SIGINT, +SIGTERM **and** SIGHUP behind the same handler, and `set_handler`'s +closure is a bare `FnMut()` carrying no signal identity, so a subset +cannot be selected through the public API. SIGHUP previously shared +SIGTERM's bug (measured: exit 129 in 13 ms, no drain), so this is a strict +improvement, and `crates/manta-cli/tests/signal_shutdown.rs` pins it +deliberately rather than leaving it incidental. + +It does constrain one future design. The 2026-09-05 broad review filed +"live config reload via SIGHUP or `manta reload`" as R-08 (cross- +referenced to MAN-30). SIGHUP is now claimed as a graceful-shutdown +trigger indistinguishable from SIGINT/SIGTERM, so R-08 cannot add reload +behaviour through `ctrlc`'s API. If it is ever built it needs a +signal-distinguishing mechanism of its own (`signal-hook`'s iterator API, +or `tokio::signal::unix::signal(SignalKind::hangup())` on an independent +stream) -- or it should pick the `manta reload` subcommand form instead +and leave SIGHUP alone. + +## Related, deliberately not changed here + +- **`SHUTDOWN_DRAIN_DEADLINE`'s shape.** MAN-45 records that one flat 25 s + budget covering `await_all`'s entire task registry cannot bound an + unbounded number of individually-compliant per-client backlogs. That is a + pre-existing property of the SIGINT path; MAN-85 only makes SIGTERM reach + it. +- **`Command::Soak`.** It registers no signal handler at all and stops on + its own `duration` watchdog (`crates/manta-engine/src/soak.rs`). A soak + run still cannot be interrupted by any signal. +- **`README.md`'s `docker stop -t 30` guidance.** It is about the drain's + *duration* exceeding Docker's 10 s default grace period, not about which + signal triggers it, and stays correct. + +## Migration note: MAN-75 packaging artifacts become redundant, not wrong + +MAN-75 (in flight at the time this ticket was implemented) ships +`packaging/systemd/manta.service` with `KillSignal=SIGINT`, a launchd +`kill -INT` workaround in `packaging/README.md`, and a `docker-compose.yml` +comment noting the image's (now-removed) `STOPSIGNAL SIGINT`. None of those +break after this change -- SIGINT still drains identically -- but they +become unnecessary. Follow-up cleanup, once MAN-75 is on `main`: + +- `packaging/systemd/manta.service` -- drop `KillSignal=SIGINT` and its + comment, and the matching `assert_eq!(get("Service", "KillSignal"), + "SIGINT")` test. +- `packaging/README.md` -- drop the launchd `kill -INT` workaround. +- `docker-compose.yml` -- update the stale "the image already sets + `STOPSIGNAL SIGINT`" comment; the `stop_signal`-absent assertion itself + stays correct.