fix(manta-cli): MAN-85 — drain in-flight spots on SIGTERM (and SIGHUP) the same way SIGINT already does

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"

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
 # <container>`, or `--stop-timeout 30` on `docker run`, or the

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 }

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,

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<PathBuf> = 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<std::process::ExitStatus> {
+    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:?}"
+    );
+}

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.