Skip to content

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

Open
catalyst-cloud-connector[bot] wants to merge 1 commit into
mainfrom
MAN-85
Open

fix(manta-cli): MAN-85 — drain in-flight spots on SIGTERM (and SIGHUP) the same way SIGINT already does#127
catalyst-cloud-connector[bot] wants to merge 1 commit into
mainfrom
MAN-85

Conversation

@catalyst-cloud-connector

@catalyst-cloud-connector catalyst-cloud-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Problem

manta listen only registered a handler for SIGINT (ctrlc::set_handler, crates/manta-cli/src/main.rs:1170, gated by the ctrlc crate's default feature set). Every real service manager — systemd, Docker, Kubernetes — sends SIGTERM by default on stop, not SIGINT. With no handler registered, SIGTERM kept the OS default disposition and the kernel killed the process directly: measured on this branch's base commit, plain listen exited in 11 ms with exit code 143 (raw signal), and a server-mode run with a connected, logged-in telnet client and an unread backlog behaved identically — zero drain-related log output, process gone. Only Docker's STOPSIGNAL SIGINT workaround masked this in the one deployment path that had it.

Fix

Enable ctrlc's termination feature, which registers SIGINT, SIGTERM and SIGHUP behind the same handler closure. No change to main.rs's shutdown logic was needed: the existing handler already treats "the registered signal fired" as one undifferentiated event (it just flips an AtomicBool), so once ctrlc forwards SIGTERM to that closure it flows into the exact same stop-flag → shutdown_tx.send(true) → per-client drain → tasks::await_all(SHUTDOWN_DRAIN_DEADLINE)Ok(())/exit 0 sequence SIGINT already used. Cargo.lock is unaffected — termination = [] has no dependencies of its own.

A tokio::signal::unix::signal-based alternative (also suggested by the ticket) was considered and rejected: this codebase constructs a tokio::runtime::Runtime in exactly one place (start_spot_server, reached only when --server-config is given), so a plain manta listen --source foo.wav has no reactor to poll a tokio::signal future. Adopting it would have meant either standing up a runtime solely to host a signal task, or splitting signal handling into two divergent paths for server vs. non-server mode — the opposite of the ticket's goal that both signals share one code path. The termination-feature fix has no such asymmetry.

Accepted consequence: SIGHUP now means "shut down"

ctrlc's termination feature is all-or-nothing — its public API has no way to select a subset of {SIGINT, SIGTERM, SIGHUP}. SIGHUP previously shared SIGTERM's bug (OS-default kill, no drain); it's now a strict improvement, and is pinned by a dedicated test rather than left as an accidental side effect. This does foreclose one specific future design: the 2026-09-05 broad review's R-08 ("live config reload via SIGHUP", cross-referenced to MAN-30) can no longer claim SIGHUP through ctrlc's simple API — it would need its own signal-distinguishing mechanism, or the manta reload subcommand form instead. Recorded in the decision doc below so this isn't rediscovered later.

What changed

  • Cargo.tomlctrlc = "3"ctrlc = { version = "3", features = ["termination"] } (the actual fix, one line).
  • crates/manta-cli/src/main.rs — one eprintln! immediately after ctrlc::set_handler(...)?, printing a stderr-only readiness marker ("manta: listening; send SIGINT or SIGTERM to stop"). listen previously printed nothing at startup, so there was no way for a test (or an operator) to know the signal handler was actually installed before sending a signal; this closes that gap and doubles as the new test's readiness handshake. Confirmed stdout stays pure JSON under --json (MAN-59's stdout-purity contract) — the marker is stderr-only and the byte-for-byte stdout output is unchanged versus the pre-change binary.
  • crates/manta-cli/tests/signal_shutdown.rs (new) — four subprocess-spawning integration tests that send real OS signals to a real manta listen child and assert on its actual exit status, since nothing in this repo previously exercised real signal delivery and no #[cfg] in this crate can observe a dependency's feature flag:
    • sigterm_exits_zero_through_the_drain_path / sigint_exits_zero_through_the_drain_path / sighup_exits_zero_through_the_drain_path — spawn manta listen against a 300 s 48 kHz fixture (long enough that natural EOF can't be confused with the signal exiting the process — a concurrently-run, unsignalled "control" child guards this), wait for the readiness marker, signal, and assert status.code() == Some(0) within a bounded window. A signal-killed process can never satisfy that assertion (code() is None when killed), so the lock can't pass by accident.
    • dockerfile_does_not_retarget_the_container_stop_signal — asserts the Dockerfile carries no STOPSIGNAL directive, so the workaround this PR removes can't silently come back.
    • crates/manta-cli/Cargo.toml gains libc as a dev-only dependency (already a workspace dependency) to call libc::kill directly, rather than shelling out to /bin/kill.
  • Dockerfile — removed STOPSIGNAL SIGINT and its now-inaccurate justification comment; replaced with a comment explaining why Docker's default stop signal is correct again post-fix. The separate grace-period comment (about docker stop -t 30 — Docker's 10 s default grace period being shorter than SHUTDOWN_DRAIN_DEADLINE) is retained with one sentence reworded, since that guidance is about drain duration, not which signal triggers it, and is unaffected by this change. README.md's matching docker stop -t 30 guidance was left untouched for the same reason.
  • docs/DECISIONS/2026-09-07-man85-signal-handling.md (new) — records the root cause, the two options considered, the decision, and the SIGHUP consequence for R-08/MAN-30, per this repo's docs/DECISIONS/ convention.

Verification

Reproduced and fixed live in the implementing container (not narrated), and independently re-verified end-to-end during validation:

Scenario Before After
plain listen, SIGINT exit 0 exit 0
plain listen, SIGTERM exit 143, no drain exit 0
plain listen, SIGHUP exit 129, no drain exit 0
server mode, logged-in telnet client, unread backlog, SIGTERM exit 143, no drain exit 0, drains, ~18–32 ms
same, with the termination feature reverted, SIGTERM exit −15 (killed), no drain — confirms the new tests are a real regression lock, not a tautology

Full workspace gate, run against the completed change: cargo fmt --all --check clean; cargo clippy --workspace --all-targets --locked -- -D warnings clean, 0 warnings; cargo test --workspace --locked — 470 passed, 0 failed, 9 pre-existing ignored (hardware/soak gates, unrelated). The new signal_shutdown target: 4/4 passing.

A /code-review pass at high effort over the full diff found zero correctness or security findings, and zero findings outside the new test file. Five test-robustness findings (a fixture cache that doesn't invalidate on parameter change, an exit-time budget looser than the fixture's measured natural EOF, no kill-on-drop for spawned children on panic paths, await_ready closing stderr and discarding later diagnostics, a case-sensitive STOPSIGNAL grep guard) were identified in crates/manta-cli/tests/signal_shutdown.rs. None weakens the regression lock's actual guarantee — the primary assertion (status.code() == Some(0), unsatisfiable by a signal-killed process) catches the real regression deterministically regardless of timing, as proven by the revert-and-rerun above. Per this repo's round-2+ review-convergence policy, these are captured for a follow-up ticket rather than fixed inline here:

Follow-up needed: "The MAN-85 signal-shutdown test's fixture cache and exit budget should not be able to silently weaken the regression lock" — (1) fixture_wav() should invalidate on FIXTURE_SECONDS/sample-rate change rather than only on path existence; (2) EXIT_BUDGET (15 s) should drop to ~5 s, since the fixture's measured natural EOF (~11 s debug) is closer to the budget than the comment claims; (3) spawned Children should be killed on panic/timeout paths to avoid leaking processes across sibling test failures; (4) await_ready should keep the child's stderr open (or capture it) past the readiness marker so failures report real diagnostics instead of a bare exit code; (5) the Dockerfile STOPSIGNAL guard should match case-insensitively.

Not in scope (deliberately)

  • SHUTDOWN_DRAIN_DEADLINE's shape (MAN-45) — a pre-existing property of the SIGINT drain path (one flat 25 s budget across all tracked tasks). This PR only makes SIGTERM reach that path; it doesn't change its behavior.
  • Command::Soak — registers no signal handler and stops on its own duration watchdog; it's a bounded self-test, not a long-running daemon.
  • packaging/ (MAN-75, in flight, not yet merged as of this branch's base) — ships manta.service with KillSignal=SIGINT, a launchd kill -INT workaround, and a docker-compose.yml comment referencing the now-removed STOPSIGNAL SIGINT. All three stay correct (not wrong) after this PR regardless of merge order, since SIGINT still works identically — they just become redundant. Follow-up once MAN-75 lands: drop KillSignal=SIGINT and its matching test assertion from the systemd unit, drop the launchd kill -INT workaround, and update the stale docker-compose.yml comment. This list is also captured in the committed decision doc so it isn't lost.
  • Docker/systemd end-to-end verification — neither docker nor a live systemd are available in the container this was built and validated in. The four new signal_shutdown tests exercise the same underlying code path natively; a manual docker build && docker stop -t 30 <container> run is still recommended before the next release tag. The code review did confirm the Dockerfile's ENTRYPOINT is exec-form (not shell-form), which is the property that makes removing STOPSIGNAL safe — manta runs as PID 1 and genuinely receives the signal Docker sends.

References

MAN-85, found via the 2026-09-05 broad review (lens 1 #4, lens 2 #5). Related, not duplicated: MAN-45 (undersized per-client drain deadline within the existing SIGINT path), MAN-30/R-08 (future SIGHUP-triggered config reload, now constrained by this PR's SIGHUP consequence), MAN-75 (in-flight packaging ticket whose KillSignal=SIGINT/kill -INT stopgaps this PR supersedes).

@catalyst-cloud-connector catalyst-cloud-connector Bot changed the title feat: MAN-85 — The daemon should drain in-flight spots on SIGTERM the same way it already does on SIGINT fix(manta-cli): MAN-85 — drain in-flight spots on SIGTERM (and SIGHUP) the same way SIGINT already does Sep 7, 2026
@catalyst-cloud-connector
catalyst-cloud-connector Bot marked this pull request as ready for review September 7, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants