diff --git a/AGENTS.md b/AGENTS.md index 2b3ebaad..abfda2ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -221,9 +221,30 @@ next actionable task before working on a subsystem. into its function's normal fall-through, never early-`return` past cleanup — and a real failure collected before the cancel always outranks it. MCP `job_cancel` cancels the job's token, waits a short grace for a cooperative stop, then hard-aborts the worker — its reply distinguishes the - two and never claims to cancel a job that had already finished. A flow that + two and never claims to cancel a job that had already finished. The REPL is + the seam's second producer: a Ctrl-C *during* a command (the terminal is + cooked then, so it is a real SIGINT rather than reedline's key event) is + forwarded onto the session token instead of killing the process, and a second + press force-exits 130 with a warning about the locks that may be left behind. + It installs a **fresh token per dispatched line, unconditionally** — the token + is one-shot, so a cancelled one left in place would kill every later dispatch + at the pre-flight check, `quit`'s teardown included, stranding exactly the + locks the cancel had to release. The teardown dispatch gets the fresh token but + **no cancel arm**: a press there only escalates, because cancelling the + cleanup is what strands the locks. That is a property of the *dispatch*, not of + the key — a typed `quit`/`exit` is routed to it by resolving the line's command + position through the registry, so it is protected exactly as Ctrl-D is. + A new interrupt hook belongs on this seam, + never on its own `tokio::signal::ctrl_c` — the REPL arms SIGINT process-wide + from the first prompt onward (startup seeding is still outside that window), + and a headless tool call has no terminal to press Ctrl-C at, while a stdio + server that *does* share one would fire every listener at once, interrupting + work nobody asked to stop. A flow that stops on a cancel must surface as `CommandError::Cancelled`, not a generic - failure — and that verdict must come from the flow's own `cancelled` flag, + failure — unless stopping *is* its documented success (`request_review + --watch` returns `Ok` with "the request is still posted"; `regenerate` + abandons the wait and reports the state it last saw, since the server keeps + building) — and that verdict must come from the flow's own `cancelled` flag, never from sniffing the session token, which would mask a real host failure that merely coincided with a cancel (see `commands/perform.rs::map_flow_error`). New long-running command bodies diff --git a/CHANGELOG.md b/CHANGELOG.md index 1202dda6..51038df7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -226,6 +226,28 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht loaded template. A *cooperative* cancel is unchanged: a body that unwound through its own flow ran its own unlock discipline. Nothing else is done at the hosts; as before, the remote command may still be running there. +- Ctrl-C during a running command no longer kills the REPL outright (#441). + Killing it skipped every teardown, so each locked host was left with a + dead-pid `/var/lock/mtui.lock` that blocked the next tester. The first press + now cancels the command cooperatively — `update`, `prepare`, `downgrade`, and + fan-outs stop at their next checkpoint with their locks released, while + `install`/`uninstall`/`run` finish the host operation already under way — and + the session stays usable afterwards. A second press still force-quits (exit + 130), now naming `unlock --force` as the remedy for whatever locks it strands + — and without discarding the session's command history, as the old kill did. + During the session teardown (Ctrl-D or a typed `quit`/`exit`) a press cannot + cancel anything, because the teardown is what releases the pool claims; two + presses still force-quit, so a blackholed refhost can no longer wedge the + exit, and that message names `unlock --pool` as well. + Ctrl-C at the prompt is unchanged (it clears the line), and Ctrl-C during the + initial `-a`/`-k`/`--sut` load still exits immediately without teardown. + Two existing waits are folded onto the same seam: `request_review --watch` + stops on a cancel (and now also on an MCP `job_cancel`, which could never + interrupt it before), and `regenerate`'s wait for TeReGen finally honours the + cooperative-stop hook it always advertised. This also removes a + non-determinism: `request_review --watch` used to arm the process-wide SIGINT + handler as a side effect, after which Ctrl-C was silently swallowed for the + rest of the session. ### Security diff --git a/Cargo.lock b/Cargo.lock index 98e86202..bca8a4d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2374,6 +2374,7 @@ dependencies = [ "shlex", "tempfile", "tokio", + "tokio-util", "tracing", "tracing-subscriber", ] diff --git a/crates/mtui-cli/Cargo.toml b/crates/mtui-cli/Cargo.toml index 81c1dbfb..1bd439ec 100644 --- a/crates/mtui-cli/Cargo.toml +++ b/crates/mtui-cli/Cargo.toml @@ -42,6 +42,9 @@ mtui-types.workspace = true # startup seeding path (before the REPL loop begins). mtui-testreport.workspace = true tokio.workspace = true +# `CancellationToken`: the REPL installs a fresh one per dispatched line so a +# mid-command Ctrl-C cancels *that* command's flow (and only it). +tokio-util.workspace = true tracing.workspace = true tracing-subscriber.workspace = true # Optional desktop-notification backend (feature `notify`); a headless no-op diff --git a/crates/mtui-cli/src/lib.rs b/crates/mtui-cli/src/lib.rs index 77204d53..6dd05d86 100644 --- a/crates/mtui-cli/src/lib.rs +++ b/crates/mtui-cli/src/lib.rs @@ -19,7 +19,7 @@ pub mod startup; pub use notification::notify_user; pub use prompt::MtuiPrompt; -pub use repl::Repl; +pub use repl::{Repl, ReplExit}; pub use startup::seed_session; use std::io::Write; diff --git a/crates/mtui-cli/src/main.rs b/crates/mtui-cli/src/main.rs index 30181d3c..9f2f0849 100644 --- a/crates/mtui-cli/src/main.rs +++ b/crates/mtui-cli/src/main.rs @@ -91,5 +91,20 @@ fn main() -> anyhow::Result<()> { let session = Arc::new(Mutex::new(session)); let mut repl = Repl::new(registry, session); - runtime.block_on(repl.run()) + let ending = runtime.block_on(repl.run())?; + let Some(status) = ending.status() else { + return Ok(()); + }; + // Force-quit: run exactly one destructor, then leave. reedline persists its + // `FileBackedHistory` on drop and `std::process::exit` runs none, so the + // history would be lost; but dropping the whole `Repl` would synchronously + // tear down the entire session graph on the one path whose job is to get + // out now — `into_line_editor` keeps the blast radius at the editor. + // + // `process::exit`, not `main() -> ExitCode`: returning would drop the + // runtime, which blocks on in-flight `spawn_blocking` — including the stdin + // prompter that is very likely outstanding when someone force-quits (its + // `keep waiting? [Y/n]` read never returns, so neither would the drop). + drop(repl.into_line_editor()); + std::process::exit(status.into()); } diff --git a/crates/mtui-cli/src/repl.rs b/crates/mtui-cli/src/repl.rs index 961a8513..271743f5 100644 --- a/crates/mtui-cli/src/repl.rs +++ b/crates/mtui-cli/src/repl.rs @@ -14,6 +14,19 @@ //! * `Signal::CtrlD` → graceful session exit (Ctrl-D → `EOF` alias of //! `quit`): break the loop, process exit 0. //! +//! Ctrl-C means two different things because the terminal is in two different +//! modes. *While reading a line* reedline holds raw mode, so Ctrl-C is the key +//! event above. *While a command runs* the terminal is cooked and Ctrl-C is a +//! real SIGINT — which used to kill the process outright, skipping every +//! teardown and stranding a dead-pid operation lock on each locked host. It is +//! now forwarded onto the session's cancellation seam instead +//! (`spawn_interrupt_forwarder` → `step_interruptible`): the first press +//! cancels the running command at its next checkpoint, a second press +//! force-quits with a record of the locks that may be left behind. +//! During the session teardown — Ctrl-D *or* a typed `quit`/`exit` — the same +//! presses escalate but never *cancel*, since cancelling the cleanup is what +//! strands the locks; `OnPress` explains the split. +//! //! The read loop and dispatch are independent of the editor's input features: //! tab completion, persistent history + Ctrl-R reverse-search + inline //! hint, and the workflow-aware prompt + RRID status + input highlighter @@ -24,11 +37,13 @@ use std::ops::ControlFlow; use std::sync::{Arc, Mutex}; -use mtui_core::{EngineError, Registry, Session, dispatch_line}; +use mtui_core::{EngineError, ExitStatus, Registry, Session, dispatch_line}; use reedline::{ ColumnarMenu, DefaultHinter, Emacs, KeyCode, KeyModifiers, MenuBuilder, Reedline, ReedlineEvent, ReedlineMenu, Signal, default_emacs_keybindings, }; +use tokio::sync::mpsc; +use tokio_util::sync::CancellationToken; use crate::completer::MtuiCompleter; use crate::highlighter::MtuiHighlighter; @@ -40,6 +55,147 @@ const COMPLETION_MENU: &str = "completion_menu"; /// The banner printed once before the first prompt. const INTRO: &str = "Maintenance Test Update Installer"; +/// How many unread Ctrl-C presses the forwarder queues before coalescing. +/// +/// Two is the whole protocol: one press to cancel, one to force-quit. A third +/// while both still sit unread says nothing the second did not, so dropping it +/// is correct — the same coalescing the kernel and `tokio`'s own signal driver +/// already do. +const INTERRUPT_QUEUE: usize = 2; + +/// The command whose dispatch *is* the session teardown. +/// +/// Matched after registry resolution, so its aliases (`exit`, `EOF`) — and any +/// added later — come along for free. +const QUIT_COMMAND: &str = "quit"; + +/// How [`Repl::run`] ended. +/// +/// A force-quit is *decided* in the loop and *executed* by the caller, because +/// the process must not exit until the line editor has been dropped: reedline +/// persists its `FileBackedHistory` on drop, and `std::process::exit` runs no +/// destructors. Exiting from inside the loop would silently discard the +/// session's command history — a second cost on top of the stranded locks, and +/// one the operator never asked for. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReplExit { + /// `quit`/Ctrl-D: the teardown ran, the process exits normally. + Normal, + /// A double Ctrl-C: the caller flushes the history and exits with + /// [`ReplExit::status`]. + ForceQuit, +} + +impl ReplExit { + /// The process status to exit with, or `None` to return from `main` + /// normally. + /// + /// [`ExitStatus::Interrupted`] is 128 + `SIGINT`, the shell convention for a + /// process killed by Ctrl-C — which is exactly what used to happen here. + /// The mapping goes through [`ExitStatus`] rather than a bare integer so + /// the binary speaks one exit vocabulary, the one whose module documents + /// the contract. + #[must_use] + pub fn status(self) -> Option { + match self { + Self::Normal => None, + Self::ForceQuit => Some(ExitStatus::Interrupted), + } + } +} + +/// What a Ctrl-C press does to the dispatch it lands on. +/// +/// The one difference between an ordinary command and the session teardown, and +/// the reason they can share [`step_interruptible`]'s protocol rather than +/// keeping two copies of a subtle `select!` loop in sync. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum OnPress { + /// An ordinary command: the first press cancels it at its next checkpoint. + Cancel, + /// The `quit` teardown, however it was asked for (Ctrl-D or a typed + /// `quit`/`exit`): a press **never** cancels, because cancelling the + /// cleanup is what strands the locks. It still counts toward the + /// force-quit, because the teardown can genuinely hang — `quit`'s + /// pool-claim release has no timeout of its own. + EscalateOnly, +} + +impl OnPress { + /// The notice the *first* press prints. + /// + /// Informational, so `warn` (an operator who set `error` has said they do + /// not want it). The force-quit record in [`on_escalate`] is a different + /// class of event and is not suppressible. + fn first_press_notice(self) -> &'static str { + match self { + Self::Cancel => { + "cancelling — the command stops at its next checkpoint (a host operation already \ + under way finishes first); press Ctrl-C again to force-quit, which may strand \ + operation locks" + } + Self::EscalateOnly => { + "teardown in progress (releasing pool claims, closing hosts) — it cannot be \ + cancelled; press Ctrl-C again to force-quit, which may leave pool claims and \ + operation locks behind" + } + } + } +} + +/// Records the force-quit and reports the ending. +/// +/// The two kinds abandon different things and so must name different remedies: +/// a command leaves its operation locks, a teardown leaves the pool claims as +/// well. Extracted from the loop's arms so that pairing is pinned by a test — +/// the arms themselves need a terminal and cannot be. +/// +/// `error!`, not `warn!`: this is the only record that mtui is walking away +/// from locks it holds, and it has to survive the `set_log_level error` an +/// operator may well be running under. +fn on_escalate(on_press: OnPress) -> ReplExit { + match on_press { + OnPress::Cancel => tracing::error!( + "forcing exit mid-command; operation locks may remain on the update's hosts — \ + release them with `unlock --force` from a new session" + ), + OnPress::EscalateOnly => tracing::error!( + "forcing exit mid-teardown; pool claims and operation locks may remain on the \ + update's hosts — release them with `unlock --force` and `unlock --pool` from a new \ + session" + ), + } + ReplExit::ForceQuit +} + +/// Decides what a press must do to the dispatch of `line`. +/// +/// A typed `quit`/`exit`/`EOF` *is* the teardown — the same dispatch Ctrl-D +/// makes — so it gets the teardown's rules rather than an ordinary command's. +/// Without this, the operator who types `quit` into a session whose refhost has +/// blackholed cancels their own cleanup and is then told to run `unlock +/// --force`, with no mention of the pool claim they just stranded. +/// +/// The line's first token is resolved through the registry (the +/// [`is_shell_line`](crate::shell::is_shell_line) precedent), so only a bare +/// command-position hit counts: `help quit` is a `help` line, and `echo quit` +/// an `echo` one. An unparseable line matches nothing and takes the ordinary +/// path, where the engine renders its syntax error. +fn press_policy(registry: &Registry, line: &str) -> OnPress { + let Some(tokens) = shlex::split(line) else { + return OnPress::Cancel; + }; + let quits = tokens + .first() + .and_then(|name| registry.get(name)) + .is_some_and(|cmd| cmd.name() == QUIT_COMMAND); + if quits { + OnPress::EscalateOnly + } else { + OnPress::Cancel + } +} + /// The interactive REPL, owning the line editor and the command registry. /// /// The registry and session are held behind [`Arc`]/[`Arc`] so the @@ -108,26 +264,62 @@ impl Repl { } } + /// Consumes the REPL, returning **only** the line editor; the rest is + /// leaked, not dropped. + /// + /// For the force-quit path, where the contract is "run exactly one + /// destructor, then exit" — neither `process::exit` (runs none) nor + /// returning from `main` (runs all of them, and blocks on the runtime) + /// expresses that. The one destructor that must run is reedline's: it + /// persists the `FileBackedHistory`, and losing the session's history is + /// not part of what the operator asked for. + /// + /// Everything else is deliberately *not* dropped. `Repl` is the sole owner + /// of the `Arc>`, so dropping it would synchronously tear + /// down the whole session graph — every SSH `Target`, the HTTP clients, the + /// template registry — on the one path whose entire job is to get out now. + /// Nothing in that chain blocks today; leaking makes sure it cannot start + /// to. The process is about to exit, so the kernel reclaims the memory. + #[must_use] + pub fn into_line_editor(self) -> Reedline { + let Self { + line_editor, + registry, + session, + prompt, + } = self; + std::mem::forget((registry, session, prompt)); + line_editor + } + /// Runs the read → dispatch loop until `quit`/Ctrl-D, driving the session. /// + /// Returns how the session ended: [`ReplExit::Normal`], or + /// [`ReplExit::ForceQuit`] when a double Ctrl-C asked to stop waiting for a + /// command (or a teardown) to finish. The caller executes that decision — + /// see [`ReplExit`] for why the exit cannot happen here. + /// /// # Errors /// /// Propagates a fatal editor I/O error from [`Reedline::read_line`] (e.g. a /// broken terminal). Command failures are *not* errors here: they are /// rendered to the session display and the loop continues. /// - /// The session guard is held across `step`'s `.await` - /// (`clippy::await_holding_lock`, allowed below). It is sound: this REPL runs - /// on a current-thread `block_on`, and the editor's synchronous `read_line` - /// (the only other lock holder, via the completer) has already returned - /// before we lock. Nothing else contends — no host tasks are in flight - /// mid-line — so the guard can never block another task at the await point. A + /// The session guard is held across the dispatch's `.await` + /// (`clippy::await_holding_lock`, allowed below). It is sound because + /// nothing else can want the lock at that point: the editor's synchronous + /// `read_line` (the only other lock holder, via the completer and the + /// highlighter) has already returned before we lock, and no host tasks are + /// in flight mid-line. The runtime is multi-threaded, so a *contending* + /// task would genuinely block a worker here — there simply is none, and + /// the interrupt forwarder spawned below deliberately touches no session + /// state (it only moves a unit through a channel). A /// `tokio::sync::Mutex` is the usual remedy, but its `blocking_lock` panics /// inside `read_line`'s runtime context and its async `lock` is unreachable /// from the synchronous completer, so the std `Mutex` + a scoped allow is the - /// correct fit for this single-threaded editor↔dispatch bridge. + /// correct fit for this strictly-alternating editor↔dispatch bridge. #[allow(clippy::await_holding_lock)] - pub async fn run(&mut self) -> anyhow::Result<()> { + pub async fn run(&mut self) -> anyhow::Result { { let mut session = self .session @@ -135,6 +327,10 @@ impl Repl { .unwrap_or_else(std::sync::PoisonError::into_inner); session.display.println(INTRO); } + // Arm SIGINT before the first prompt, for the whole session (see the + // forwarder's own note on why arming early is what makes Ctrl-C + // deterministic here). + let mut interrupts = spawn_interrupt_forwarder(); loop { match self.line_editor.read_line(&self.prompt)? { @@ -172,18 +368,37 @@ impl Repl { } continue; } + // A typed `quit`/`exit` dispatches the very teardown Ctrl-D + // does, so it takes the teardown's press rules. + let on_press = press_policy(&self.registry, &line); // Lock only for the dispatch; the completer's own lock during // `read_line` was released before this returned. (Guard held // across the await — justified on `run`'s doc comment.) - let should_break = { + let outcome = { let mut session = self .session .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - step(&self.registry, &mut session, &line).await.is_break() + step_interruptible( + &self.registry, + &mut session, + &line, + &mut interrupts, + on_press, + ) + .await }; - if should_break { - break; + match outcome { + StepOutcome::Flow(flow) => { + if flow.is_break() { + break; + } + } + // A second Ctrl-C: the operator has decided not to wait + // for the cooperative stop. Honour it — loudly, because + // this is the one path that *can* leave the locks a + // clean stop would have released. + StepOutcome::Escalate => return Ok(on_escalate(on_press)), } } // Ctrl-C on a partial line: clear it and reprompt, never exit. @@ -201,11 +416,26 @@ impl Repl { // `FileBackedHistory` when the editor is dropped after `run` // returns, so no explicit history flush is needed here. Signal::CtrlD => { - let mut session = self - .session - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let _ = step(&self.registry, &mut session, "EOF").await; + let outcome = { + let mut session = self + .session + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + step_interruptible( + &self.registry, + &mut session, + "EOF", + &mut interrupts, + OnPress::EscalateOnly, + ) + .await + }; + // A double Ctrl-C during the teardown: the same escape + // hatch as mid-command, and the same honest record. What + // the teardown had not reached yet stays claimed/locked. + if outcome == StepOutcome::Escalate { + return Ok(on_escalate(OnPress::EscalateOnly)); + } break; } // `#[non_exhaustive]`: any future/host signal is ignored and we @@ -214,7 +444,179 @@ impl Repl { } } - Ok(()) + Ok(ReplExit::Normal) + } +} + +/// Spawns the SIGINT forwarder and hands back the channel the dispatch loop +/// reads presses from. +/// +/// The task is spawned once per [`Repl::run`] and outlives every dispatch: +/// `tokio::signal::ctrl_c` installs its handler *process-wide* on first use and +/// never uninstalls it, so arming here — before the first prompt — is what +/// makes Ctrl-C mean one thing for the whole session. (It previously depended +/// on history: `request_review --watch` armed the handler as a side effect, so +/// after one watch a later Ctrl-C was silently swallowed instead of killing +/// the process.) +/// +/// While `read_line` owns the terminal reedline holds raw mode, so Ctrl-C is a +/// key event and no signal is raised at all; the forwarder therefore only ever +/// sees presses from a cooked window — a running command, or a gap between +/// commands (which [`step_interruptible`] drains). +/// +/// "The whole session" means *from the first prompt onward*. Startup seeding +/// (`-a`/`-k`/`--sut`: an SVN checkout, refhost connects, pool claims) runs +/// before this, and a Ctrl-C there still kills the process the old way. Arming +/// earlier without a consumer would be worse, not better — it would make Ctrl-C +/// during a 60-second connect a silent no-op — so routing the seeding through +/// this same protocol is the fix, not moving this call. +/// +/// Only the wiring lives here. The effect it drives is [`step_interruptible`], +/// which is tested by injecting on this same channel: raising a real `SIGINT` +/// inside a shared test binary would kill or cross-contaminate every other +/// test in it, so the signal source itself stays deliberately untested. +fn spawn_interrupt_forwarder() -> mpsc::Receiver<()> { + let (tx, rx) = mpsc::channel(INTERRUPT_QUEUE); + tokio::spawn(async move { + // A full queue already holds an unread press (coalesce); a closed one + // means the loop is gone and nothing will read again. + let forward = |tx: &mpsc::Sender<()>| { + !matches!(tx.try_send(()), Err(mpsc::error::TrySendError::Closed(()))) + }; + // One long-lived stream, created *before* the first press. Calling + // `ctrl_c()` per press mints a fresh subscription each time, and a + // subscription only sees signals arriving after its first poll — so a + // second press landing between two calls would be lost. Narrow, but + // free to close, and a persistent stream is tokio's own shape for + // handling a signal repeatedly. + #[cfg(unix)] + { + use tokio::signal::unix::{SignalKind, signal}; + match signal(SignalKind::interrupt()) { + Ok(mut sigint) => { + while sigint.recv().await.is_some() { + if !forward(&tx) { + break; + } + } + } + // The handler could not be installed — SIGINT keeps its default + // disposition (it kills, as before this loop existed) and + // retrying would only spin. + Err(e) => { + tracing::debug!(error = %e, "no SIGINT handler; Ctrl-C stays fatal"); + } + } + } + // No `SignalKind` off unix; `ctrl_c` is the portable equivalent, at the + // cost of the re-subscribe window described above. + #[cfg(not(unix))] + while tokio::signal::ctrl_c().await.is_ok() { + if !forward(&tx) { + break; + } + } + }); + rx +} + +/// What one interruptible dispatch decided. +/// +/// Split from the execution so the decision is testable: [`Repl::run`] turns +/// [`Escalate`](Self::Escalate) into the process exit, which a test cannot +/// observe without taking the whole test binary with it. +#[derive(Debug, PartialEq, Eq)] +enum StepOutcome { + /// The dispatch ran to completion (cancelled or not); carries [`step`]'s + /// control flow. + Flow(ControlFlow<()>), + /// A second Ctrl-C arrived while the first cancel was still settling: stop + /// waiting for the command and force-quit. + Escalate, +} + +/// Dispatches one input `line` with Ctrl-C wired to the cancellation seam. +/// +/// The interruptible sibling of [`step`], and like it deliberately +/// TTY-free: presses arrive on `interrupts` (the +/// [`spawn_interrupt_forwarder`] channel in the live REPL, a test's own sender +/// otherwise), so the whole protocol is exercisable without raising a signal. +/// +/// Three constraints shape the body: +/// +/// * **A fresh token per dispatch, installed unconditionally.** A +/// [`CancellationToken`] is one-shot and this loop never resets it, so a +/// cancel would otherwise poison *every* later dispatch — including the +/// `quit`/`EOF` teardown, which would die at the `Command::run` pre-flight +/// check and strand exactly the pool claims and host locks a cooperative +/// cancel exists to release. Installing unconditionally (rather than only +/// after a cancel) is the MCP job layer's self-healing shape. +/// * **Stale presses are drained first.** A Ctrl-C from a cooked gap between +/// commands belongs to no dispatch and must not cancel the next one. +/// * **The dispatch future is never dropped on a press.** Cancelling the token +/// asks the flow to stop at its next checkpoint; dropping the future instead +/// would abandon it mid-step, which is the very teardown hole this replaces. +/// Escalation is the one exception, and it is the operator's explicit second +/// request. +/// +/// What the first press actually buys depends on the command: checkpointed +/// flows (`update`, `prepare`, `downgrade`, every fan-out boundary) stop +/// promptly with their locks released, while a host operation already under +/// way (`run`, `install`, `uninstall`) finishes first and unlocks normally. +/// +/// `on_press` is the *only* thing that differs between an ordinary command and +/// the session teardown ([`OnPress::EscalateOnly`] skips the `token.cancel()` +/// and prints the teardown's notice). They share this body deliberately: two +/// copies of a biased-select interrupt protocol is the shape that drifts, and +/// the reason a typed `quit` was not protected the way Ctrl-D was. +async fn step_interruptible( + registry: &Registry, + session: &mut Session, + line: &str, + interrupts: &mut mpsc::Receiver<()>, + on_press: OnPress, +) -> StepOutcome { + // Stale presses, then a token this dispatch owns (both per the contract + // above). + while interrupts.try_recv().is_ok() {} + + session.set_cancel_token(CancellationToken::new()); + // Clone before `step` borrows the session mutably; clones share state, so + // cancelling this one cancels the session's. + let token = session.cancel_token(); + + let dispatch = step(registry, session, line); + tokio::pin!(dispatch); + let mut pressed = false; + loop { + let press = tokio::select! { + // Biased, completion first: a press landing in the same poll window + // as the command's own completion must not be attributed to it. + // Unbiased, that race would force-quit a command that had just + // finished cleanly — warning about locks it had already released, + // and skipping the `quit` teardown that would have released the + // pool claims. A press that loses this race stays queued and is + // drained by the next dispatch, where it belongs. + biased; + flow = &mut dispatch => return StepOutcome::Flow(flow), + press = interrupts.recv() => press, + }; + if press.is_none() { + // The forwarder is gone (it only exits when SIGINT cannot be + // handled at all), so nothing can interrupt this dispatch: see it + // through instead of spinning on a closed channel. + return StepOutcome::Flow((&mut dispatch).await); + } + if pressed { + return StepOutcome::Escalate; + } + pressed = true; + if on_press == OnPress::Cancel { + token.cancel(); + } + // Emitted only on the signal path, so the normal dispatch's + // exactly-one-`error: `-line contract is untouched. + tracing::warn!("{}", on_press.first_press_notice()); } } @@ -283,11 +685,31 @@ mod tests { use clap::ArgMatches; use mtui_config::Config; use mtui_core::command::{Command, Scope}; - use mtui_core::error::CommandResult; + use mtui_core::error::{CommandError, CommandResult}; use mtui_core::{ColorMode, CommandPromptDisplay}; + use std::future::Future; use std::sync::Arc; use std::sync::Mutex; - use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; + use std::time::Duration; + use tokio::sync::oneshot; + + /// A generous upper bound on anything the interrupt tests wait for. The + /// work is in-process channel traffic, so overrunning it means a hang, not + /// a slow machine. + /// + /// What it buys: a dispatch that is *parked* — dropped, never woken, or + /// waiting on a cancel that never came — fails an assertion instead of + /// wedging the test binary. What it does not buy: a busy-spinning + /// regression still hangs, since nothing here can preempt one. + const BOUND: Duration = Duration::from_secs(5); + + /// How many independent races [`a_press_racing_completion_never_force_quits`] + /// runs. The `biased;` select must win every one; an unbiased select picks + /// the interrupt branch about half the time, so it survives all 32 rounds + /// with probability 2⁻³² — a statistical proof, but not a meaningfully + /// uncertain one. + const RACE_ROUNDS: usize = 32; /// A command that counts its runs; on the deny-listed name `quit` it flips /// the session's exit flag (mirroring the real `Quit`). @@ -312,6 +734,120 @@ mod tests { } } + /// A command that parks on the cancellation seam and — crucially — records + /// that it was polled all the way to its own `return` afterwards. That + /// flag is the observable proof the dispatch future was *not* dropped when + /// the interrupt arrived, which is the whole difference between a + /// cooperative cancel and the process kill this replaces. + struct ParkCmd { + started: Mutex>>, + finished: Arc, + } + + #[async_trait] + impl Command for ParkCmd { + fn name(&self) -> &'static str { + "park" + } + fn scope(&self) -> Scope { + Scope::Single + } + async fn call(&self, session: &mut Session, _args: &ArgMatches) -> CommandResult { + if let Some(tx) = self.started.lock().unwrap().take() { + let _ = tx.send(()); + } + session.cancel_token().cancelled().await; + self.finished.store(true, Ordering::SeqCst); + Err(CommandError::Cancelled(String::new())) + } + } + + /// A command that never observes the seam — the `run`/`install` shape, + /// where a cancel is inert until the host operation finishes. Only a + /// second press can get the operator out of one. + struct DeafCmd { + started: Mutex>>, + } + + #[async_trait] + impl Command for DeafCmd { + fn name(&self) -> &'static str { + "deaf" + } + fn scope(&self) -> Scope { + Scope::Single + } + async fn call(&self, _session: &mut Session, _args: &ArgMatches) -> CommandResult { + if let Some(tx) = self.started.lock().unwrap().take() { + let _ = tx.send(()); + } + std::future::pending::<()>().await; + Ok(()) + } + } + + /// A command that waits to be let go, then reports whether the token was + /// cancelled behind its back. It parks first so a stale press cannot be + /// missed by a dispatch that finished before the loop ever looked at the + /// interrupt channel. + struct GateCmd { + go: Mutex>>, + observed_cancel: Arc, + runs: Arc, + } + + #[async_trait] + impl Command for GateCmd { + fn name(&self) -> &'static str { + "gate" + } + fn scope(&self) -> Scope { + Scope::Single + } + async fn call(&self, session: &mut Session, _args: &ArgMatches) -> CommandResult { + let token = session.cancel_token(); + let go = self.go.lock().unwrap().take().expect("gate armed once"); + let _ = go.await; + self.observed_cancel + .store(token.is_cancelled(), Ordering::SeqCst); + self.runs.fetch_add(1, Ordering::SeqCst); + Ok(()) + } + } + + /// A `quit` that takes its time — the shape of the real one when a refhost + /// blackholes: `quit`'s pool-claim release has no timeout, so the teardown + /// parks. Records whether anything cancelled its token, which nothing on the + /// teardown path is allowed to do. + struct SlowQuitCmd { + go: Mutex>>, + observed_cancel: Arc, + runs: Arc, + } + + #[async_trait] + impl Command for SlowQuitCmd { + fn name(&self) -> &'static str { + "quit" + } + fn aliases(&self) -> &'static [&'static str] { + &["exit", "EOF"] + } + fn scope(&self) -> Scope { + Scope::Single + } + async fn call(&self, session: &mut Session, _args: &ArgMatches) -> CommandResult { + let token = session.cancel_token(); + let go = self.go.lock().unwrap().take().expect("gate armed once"); + let _ = go.await; + self.observed_cancel + .store(token.is_cancelled(), Ordering::SeqCst); + self.runs.fetch_add(1, Ordering::SeqCst); + session.request_exit(); + Ok(()) + } + } + /// A minimal `quit`: flips `request_exit`, like the real command. struct QuitCmd; @@ -399,6 +935,67 @@ mod tests { (flow, out) } + /// [`step_capturing_log`]'s sibling for the interrupt-aware dispatches + /// ([`step_interruptible`] and [`step_teardown`]): same scoped subscriber + /// and same current-thread runtime, plus a `driver` future standing in for + /// the SIGINT forwarder (the real one is thin wiring — a test must never + /// raise a real signal into a shared test binary). + /// + /// `dispatch` is bounded so a regression that drops or never wakes it fails + /// an assertion instead of hanging the suite. + fn capturing_log(dispatch: F, driver: D) -> (StepOutcome, String) + where + F: Future, + D: Future, + { + let buf = Arc::new(Mutex::new(Vec::new())); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .event_format(crate::logfmt::CompactLevelFormat::new(false)) + .with_writer(BufMaker(Arc::clone(&buf))) + .finish(); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_time() + .build() + .unwrap(); + let outcome = tracing::subscriber::with_default(subscriber, || { + rt.block_on(async { + let bounded = tokio::time::timeout(BOUND, dispatch); + let (outcome, ()) = tokio::join!(bounded, driver); + outcome.expect("the dispatch must settle within the bound") + }) + }); + let out = String::from_utf8(buf.lock().unwrap().clone()).unwrap(); + (outcome, out) + } + + /// Runs `f` under the REPL's real log layer on a scoped subscriber, + /// returning its value and whatever it emitted. The synchronous sibling of + /// [`capturing_log`], for the parts of the protocol that are pure decisions. + fn capture_log(f: impl FnOnce() -> T) -> (T, String) { + let buf = Arc::new(Mutex::new(Vec::new())); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .event_format(crate::logfmt::CompactLevelFormat::new(false)) + .with_writer(BufMaker(Arc::clone(&buf))) + .finish(); + let value = tracing::subscriber::with_default(subscriber, f); + let out = String::from_utf8(buf.lock().unwrap().clone()).unwrap(); + (value, out) + } + + /// One press, delivered once the probe reports it is parked mid-dispatch. + /// A probe that never reports panics here rather than quietly pressing into + /// the void — a missed handshake would otherwise turn into a green test that + /// never exercised the interrupt at all. + async fn press_once(started: oneshot::Receiver<()>, tx: mpsc::Sender<()>) { + tokio::time::timeout(BOUND, started) + .await + .expect("the probe must report started") + .expect("the probe's start signal must not be dropped"); + let _ = tx.send(()).await; + } + fn registry() -> (Registry, Arc) { let runs = Arc::new(AtomicUsize::new(0)); let mut r = Registry::new(); @@ -428,14 +1025,20 @@ mod tests { assert!(s.should_exit()); } - #[tokio::test] - async fn eof_dispatches_quit_and_breaks() { - // The Ctrl-D handler dispatches the `EOF` alias through the engine (so - // the full quit teardown runs), which must break the loop and set exit. + #[test] + fn eof_dispatches_quit_and_breaks() { + // What the Ctrl-D arm calls: `step_teardown` dispatches the `EOF` alias + // through the engine (so the full quit teardown runs), which must break + // the loop and set exit. Targeting the helper rather than bare `step` + // keeps this pinned to the code the arm actually reaches. let (r, _) = registry(); let (mut s, _buf) = session_with_buffer(); - let flow = step(&r, &mut s, "EOF").await; - assert_eq!(flow, ControlFlow::Break(())); + let (_tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + let (outcome, _out) = capturing_log( + step_interruptible(&r, &mut s, "EOF", &mut rx, OnPress::EscalateOnly), + std::future::ready(()), + ); + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); assert!(s.should_exit()); } @@ -493,6 +1096,545 @@ mod tests { assert!(rendered(&buf).is_empty()); } + /// The first Ctrl-C cancels the running command instead of killing the + /// process: the body observes the seam, unwinds through its own `return`, + /// and the loop keeps going. Rendering stays exactly one `error: cancelled` + /// line, with the notice on the signal path beside it. + #[test] + fn one_press_cancels_the_running_command_cooperatively() { + let (started_tx, started_rx) = oneshot::channel(); + let finished = Arc::new(AtomicBool::new(false)); + let mut r = Registry::new(); + r.register(Arc::new(ParkCmd { + started: Mutex::new(Some(started_tx)), + finished: Arc::clone(&finished), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "park", &mut rx, OnPress::Cancel), + press_once(started_rx, tx), + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Continue(()))); + // The body reached its own `return` *after* the cancel: the dispatch + // future was cancelled cooperatively, never dropped mid-step. + assert!( + finished.load(Ordering::SeqCst), + "the parked body must be polled to completion, not dropped" + ); + assert_eq!( + out.matches("error: cancelled").count(), + 1, + "exactly one cancelled line, got: {out:?}" + ); + // The notice fires once, and only from the signal path. + assert_eq!( + out.matches("Ctrl-C again").count(), + 1, + "exactly one notice, got: {out:?}" + ); + assert_eq!( + out.lines().count(), + 2, + "the notice and the error, nothing else: {out:?}" + ); + assert!(out.starts_with("warn: "), "notice comes first: {out:?}"); + } + + /// The token is one-shot, so a cancel must not outlive its own line: the + /// next dispatch installs a fresh one and runs normally. Without that, every + /// later command — including the `quit`/`EOF` teardown that releases the + /// pool claims and host locks — would die at the driver's pre-flight check. + #[test] + fn a_cancelled_command_does_not_poison_the_next_one() { + let (started_tx, started_rx) = oneshot::channel(); + let finished = Arc::new(AtomicBool::new(false)); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(ParkCmd { + started: Mutex::new(Some(started_tx)), + finished: Arc::clone(&finished), + })); + r.register(Arc::new(EchoCmd { + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + let (cancelled, _) = capturing_log( + step_interruptible(&r, &mut s, "park", &mut rx, OnPress::Cancel), + press_once(started_rx, tx), + ); + assert_eq!(cancelled, StepOutcome::Flow(ControlFlow::Continue(()))); + + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "echo hi", &mut rx, OnPress::Cancel), + std::future::ready(()), + ); + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Continue(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the next command ran"); + assert!(out.is_empty(), "and rendered nothing, got: {out:?}"); + } + + /// Presses that arrive while no command is running (the cooked gap between + /// commands — e.g. Ctrl-C while `edit` held the TTY) belong to no dispatch + /// and must not cancel the *next* one. + /// + /// Two presses, not one: an operator who gets no response from an editor + /// presses again, and a drain that removed only the first would leave the + /// second to cancel the next command — or, worse, to consume its "already + /// cancelling" slot so that the *next* genuine press force-quits with no + /// warning at all. + #[test] + fn a_press_from_the_idle_gap_does_not_cancel_the_next_command() { + let (go_tx, go_rx) = oneshot::channel(); + let observed = Arc::new(AtomicBool::new(false)); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(GateCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::clone(&observed), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + tx.try_send(()).expect("the queue has room"); + tx.try_send(()).expect("the queue has room for both"); + + // Hold the command at its gate for at least one poll, so a press that + // was *not* drained has every chance to be observed. + let driver = async move { + tokio::task::yield_now().await; + let _ = go_tx.send(()); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "gate", &mut rx, OnPress::Cancel), + driver, + ); + + assert_ne!( + outcome, + StepOutcome::Escalate, + "stale presses must never force-quit" + ); + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Continue(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the command ran"); + assert!( + !observed.load(Ordering::SeqCst), + "a stale press must not reach this command's token" + ); + assert!( + out.is_empty(), + "no notice and no error on the normal path, got: {out:?}" + ); + } + + /// A press landing in the same poll window as the command's own completion + /// belongs to the *next* dispatch, not this one. + /// + /// Without the `biased;` (completion first) this is a coin flip, and losing + /// it is expensive: a command that finished cleanly would force-quit the + /// session, warn about locks it had already released, and skip the `quit` + /// teardown — genuinely stranding the pool claims the warning only + /// speculated about. Both presses must instead stay queued for the next + /// dispatch to drain. + #[test] + fn a_press_racing_completion_never_force_quits() { + for round in 0..RACE_ROUNDS { + let (go_tx, go_rx) = oneshot::channel(); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(GateCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::new(AtomicBool::new(false)), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + // Release the gate *and* queue both presses before the loop is + // polled again, so the completion and the presses are ready in the + // same poll window — the race, made deterministic. + let driver = async move { + tokio::task::yield_now().await; + let _ = go_tx.send(()); + tx.try_send(()).expect("the queue has room"); + tx.try_send(()).expect("the queue has room for both"); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "gate", &mut rx, OnPress::Cancel), + driver, + ); + + assert_eq!( + outcome, + StepOutcome::Flow(ControlFlow::Continue(())), + "round {round}: the completed command must win the race" + ); + assert_eq!(runs.load(Ordering::SeqCst), 1, "round {round}"); + assert!( + out.is_empty(), + "round {round}: no notice for a command that had already finished, got: {out:?}" + ); + assert_eq!( + rx.len(), + 2, + "round {round}: both presses stay queued for the next dispatch" + ); + } + } + + /// A second press escalates: some bodies never observe the seam (`run`, + /// `install` — a host operation already under way finishes first), so the + /// operator keeps an escape hatch. The decision is returned here; + /// [`Repl::run`] is what executes the process exit. + #[test] + fn a_second_press_escalates_to_a_forced_exit() { + let (started_tx, started_rx) = oneshot::channel(); + let mut r = Registry::new(); + r.register(Arc::new(DeafCmd { + started: Mutex::new(Some(started_tx)), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + let driver = async move { + tokio::time::timeout(BOUND, started_rx) + .await + .expect("the probe must report started") + .expect("the probe's start signal must not be dropped"); + // The queue holds both, so the order of consumption is fixed + // however the tasks interleave. + let _ = tx.send(()).await; + let _ = tx.send(()).await; + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "deaf", &mut rx, OnPress::Cancel), + driver, + ); + + assert_eq!(outcome, StepOutcome::Escalate); + assert_eq!( + out.matches("Ctrl-C again").count(), + 1, + "the first press explained itself exactly once: {out:?}" + ); + } + + /// A dead forwarder must not take the dispatch down with it: the closed + /// channel yields `None` forever, and the loop has to see the command + /// through rather than abandon it (or spin). + /// + /// The probe parks, so the `None` is genuinely what the loop reacts to — a + /// command that finished on its first poll would let this pass without ever + /// reaching the branch. + #[test] + fn a_closed_interrupt_channel_still_completes_the_dispatch() { + let (go_tx, go_rx) = oneshot::channel(); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(GateCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::new(AtomicBool::new(false)), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + drop(tx); + + let driver = async move { + tokio::task::yield_now().await; + let _ = go_tx.send(()); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "gate", &mut rx, OnPress::Cancel), + driver, + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Continue(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the command still ran"); + assert!(out.is_empty(), "and rendered nothing, got: {out:?}"); + } + + /// Ctrl-D after a cancelled command must still reach the `quit` command. + /// This is the sharpest edge of the one-shot token: the cancel that stopped + /// one command would otherwise bail the *teardown* out at the driver's + /// pre-flight check, so `quit` would never be entered at all — and what + /// `quit` does once entered (release the pool claims, close the hosts) is + /// pinned by its own tests in `commands/quit.rs`. What this pins is that the + /// dispatch is not rejected before it starts. + #[test] + fn ctrl_d_tears_down_even_after_a_cancelled_command() { + let (r, _) = registry(); + let (mut s, _buf) = session_with_buffer(); + // The state a cancelled line leaves behind. + s.cancel_token().cancel(); + let (_tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + let (outcome, _out) = capturing_log( + step_interruptible(&r, &mut s, "EOF", &mut rx, OnPress::EscalateOnly), + std::future::ready(()), + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); + assert!(s.should_exit(), "the teardown ran and asked to exit"); + } + + /// A press during the teardown must **not** cancel it: cancelling the + /// cleanup is precisely what strands the locks. The teardown runs to + /// completion; the press only buys a warning. + #[test] + fn a_press_during_the_teardown_does_not_cancel_it() { + let (go_tx, go_rx) = oneshot::channel(); + let observed = Arc::new(AtomicBool::new(false)); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(SlowQuitCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::clone(&observed), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + // One press while the teardown is parked, then let it finish. + let driver = async move { + tokio::task::yield_now().await; + let _ = tx.send(()).await; + tokio::task::yield_now().await; + let _ = go_tx.send(()); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "EOF", &mut rx, OnPress::EscalateOnly), + driver, + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the teardown finished"); + assert!( + !observed.load(Ordering::SeqCst), + "a press must never cancel the teardown's token" + ); + assert_eq!( + out.matches("teardown in progress").count(), + 1, + "warned exactly once, got: {out:?}" + ); + } + + /// The teardown can genuinely hang — `quit`'s pool-claim release has no + /// timeout of its own, so a blackholed refhost parks it indefinitely. Before + /// the forwarder existed Ctrl-C killed the process there; with the handler + /// armed, a second press has to offer the same way out or Ctrl-C would do + /// nothing at all on this path. + #[test] + fn two_presses_during_the_teardown_force_quit() { + // No `go` sender is ever fired: this teardown never finishes. + let (_go_tx, go_rx) = oneshot::channel(); + let mut r = Registry::new(); + r.register(Arc::new(SlowQuitCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::new(AtomicBool::new(false)), + runs: Arc::new(AtomicUsize::new(0)), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + + let driver = async move { + tokio::task::yield_now().await; + let _ = tx.send(()).await; + let _ = tx.send(()).await; + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "EOF", &mut rx, OnPress::EscalateOnly), + driver, + ); + + assert_eq!(outcome, StepOutcome::Escalate); + assert_eq!( + out.matches("teardown in progress").count(), + 1, + "the first press explained itself exactly once, got: {out:?}" + ); + } + + /// A press that raced the *previous* dispatch's completion is queued when + /// the teardown starts; it belongs to that dispatch, not to this teardown, + /// so the teardown must drain it. Otherwise a single genuine press during a + /// hung teardown would force-quit with no warning at all. + #[test] + fn the_teardown_drains_a_press_left_over_from_the_last_dispatch() { + let (go_tx, go_rx) = oneshot::channel(); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(SlowQuitCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::new(AtomicBool::new(false)), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + tx.try_send(()).expect("the queue has room"); + tx.try_send(()).expect("the queue has room for both"); + + let driver = async move { + tokio::task::yield_now().await; + let _ = go_tx.send(()); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "EOF", &mut rx, OnPress::EscalateOnly), + driver, + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the teardown finished"); + assert!(out.is_empty(), "stale presses warn about nothing: {out:?}"); + } + + /// The force-quit's cost is paid in the exit status, not in the process + /// exiting from inside the loop: `Normal` returns from `main` (so every + /// destructor runs, reedline's history flush included), `ForceQuit` carries + /// 128 + `SIGINT` — the status a Ctrl-C death would have had. + #[test] + fn the_force_quit_exit_status_is_128_plus_sigint() { + assert_eq!(ReplExit::Normal.status(), None); + assert_eq!( + ReplExit::ForceQuit.status(), + Some(mtui_core::ExitStatus::Interrupted) + ); + assert_eq!(i32::from(mtui_core::ExitStatus::Interrupted), 130); + } + + /// Only a bare `quit`/`exit`/`EOF` in **command position** is the teardown. + /// + /// The line is resolved through the registry, so aliases route without being + /// enumerated here — and `quit` appearing as an *argument* does not, or a + /// `help quit` would silently get teardown semantics. + #[test] + fn only_a_quit_line_takes_the_teardown_path() { + let (r, _) = registry(); + for line in ["quit", "exit", "EOF", " quit ", "quit reboot"] { + assert_eq!( + press_policy(&r, line), + OnPress::EscalateOnly, + "{line:?} dispatches the teardown" + ); + } + for line in [ + "echo hi", + // `quit` is an argument here, not the command. + "echo quit", + // Neither resolves to a command at all: no command, no teardown. + "help quit", + "quitx", + "", + // Unbalanced quotes: the engine will render the syntax error. + "echo \"unbalanced", + ] { + assert_eq!( + press_policy(&r, line), + OnPress::Cancel, + "{line:?} is an ordinary line" + ); + } + } + + /// A **typed** `quit` dispatches the very teardown Ctrl-D does, so a press + /// must not cancel it there either. + /// + /// Before the routing existed, a typed `quit` took the ordinary path: the + /// first press cancelled the cleanup's token, and the second told the + /// operator to run `unlock --force` while saying nothing about the pool + /// claim they had just stranded. + #[test] + fn a_typed_quit_is_never_cancelled_by_a_press() { + let (go_tx, go_rx) = oneshot::channel(); + let observed = Arc::new(AtomicBool::new(false)); + let runs = Arc::new(AtomicUsize::new(0)); + let mut r = Registry::new(); + r.register(Arc::new(SlowQuitCmd { + go: Mutex::new(Some(go_rx)), + observed_cancel: Arc::clone(&observed), + runs: Arc::clone(&runs), + })); + let (mut s, _buf) = session_with_buffer(); + let (tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + // Exactly the decision `Repl::run` makes for this line. + let on_press = press_policy(&r, "quit"); + + let driver = async move { + tokio::task::yield_now().await; + let _ = tx.send(()).await; + tokio::task::yield_now().await; + let _ = go_tx.send(()); + }; + let (outcome, out) = capturing_log( + step_interruptible(&r, &mut s, "quit", &mut rx, on_press), + driver, + ); + + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); + assert_eq!(runs.load(Ordering::SeqCst), 1, "the teardown finished"); + assert!( + !observed.load(Ordering::SeqCst), + "a typed quit's teardown must not be cancelled by a press" + ); + assert_eq!( + out.matches("teardown in progress").count(), + 1, + "and it gets the teardown's notice, not a command's: {out:?}" + ); + } + + /// Each force-quit record must name the remedy for what *that* arm + /// abandons — a command leaves its operation locks, a teardown leaves the + /// pool claims too — and both must survive `set_log_level error`, which is + /// why they are `error!` and not `warn!`. + #[test] + fn each_escalation_names_the_remedy_for_what_it_abandons() { + let (exit, out) = capture_log(|| on_escalate(OnPress::Cancel)); + assert_eq!(exit, ReplExit::ForceQuit); + assert!( + out.starts_with("error: "), + "must outrank `set_log_level error`, got: {out:?}" + ); + assert!(out.contains("mid-command"), "{out:?}"); + assert!(out.contains("unlock --force"), "{out:?}"); + assert!( + !out.contains("unlock --pool"), + "a mid-command exit strands no pool claim: {out:?}" + ); + + let (exit, out) = capture_log(|| on_escalate(OnPress::EscalateOnly)); + assert_eq!(exit, ReplExit::ForceQuit); + assert!( + out.starts_with("error: "), + "must outrank `set_log_level error`, got: {out:?}" + ); + assert!(out.contains("mid-teardown"), "{out:?}"); + assert!( + out.contains("unlock --force") && out.contains("unlock --pool"), + "a teardown abandons both kinds of lock: {out:?}" + ); + } + + /// `quit`'s `Break` survives the interruptible wrapper — the loop must still + /// exit on it, not fall through to another prompt. + #[test] + fn quit_breaks_through_the_interruptible_path() { + let (r, _) = registry(); + let (mut s, _buf) = session_with_buffer(); + let (_tx, mut rx) = mpsc::channel(INTERRUPT_QUEUE); + let (outcome, _out) = capturing_log( + step_interruptible(&r, &mut s, "quit", &mut rx, OnPress::Cancel), + std::future::ready(()), + ); + assert_eq!(outcome, StepOutcome::Flow(ControlFlow::Break(()))); + assert!(s.should_exit()); + } + #[test] fn bad_flag_renders_error_and_continues() { let (r, runs) = registry(); diff --git a/crates/mtui-core/src/commands/regenerate.rs b/crates/mtui-core/src/commands/regenerate.rs index bfe91f17..6640e72e 100644 --- a/crates/mtui-core/src/commands/regenerate.rs +++ b/crates/mtui-core/src/commands/regenerate.rs @@ -132,15 +132,22 @@ impl Command for Regenerate { // Drive a TTY spinner for the (long-polling) wait. REPL-only (gated on // `interactive`, like the fan-out spinner); a no-op off a TTY / over - // MCP. The guard's `is_stopped` predicate feeds `regenerate_and_wait`'s - // cooperative-cancel hook so Ctrl-C during the wait bails out promptly - // instead of blocking to the next poll. + // MCP. let spin = session .is_repl .then(|| mtui_hosts::spinner(format!("Regenerating {rrid_str}"))); + // `regenerate_and_wait`'s cooperative-cancel hook, fed from the + // session's cancellation seam: a REPL Ctrl-C (forwarded onto the + // per-line token) or an MCP `job_cancel` abandons the wait at its next + // step instead of blocking to the next poll. The spinner's own stop + // flag stays in the predicate — it is set by the display layer, never + // by a cancel, so neither source can cover for the other. + let cancel = session.cancel_token(); let should_stop = || { - spin.as_ref() - .is_some_and(mtui_hosts::SpinnerGuard::is_stopped) + cancel.is_cancelled() + || spin + .as_ref() + .is_some_and(mtui_hosts::SpinnerGuard::is_stopped) }; let outcome = teregen .regenerate_and_wait(&rrid_str, force, ignore_inconsistent, should_stop) @@ -442,6 +449,81 @@ mod tests { assert_eq!(session.targets().len(), before); } + /// Signals the test the first time the mocked endpoint is hit, so a cancel + /// can be timed to land *during* the wait rather than before it. + struct SignalOnFirstHit { + hit: std::sync::Mutex>>, + body: serde_json::Value, + } + + impl wiremock::Respond for SignalOnFirstHit { + fn respond(&self, _req: &wiremock::Request) -> ResponseTemplate { + if let Some(tx) = self.hit.lock().unwrap().take() { + let _ = tx.send(()); + } + ResponseTemplate::new(200).set_body_json(self.body.clone()) + } + } + + /// A cancel arriving **mid-wait** abandons the (long-polling) wait promptly + /// instead of blocking to the next poll — the `should_stop` hook's promise, + /// which until now only the spinner's stop flag could keep, and nothing + /// signal-related ever sets that. + /// + /// Cancelling *before* the call would not prove this: a predicate that read + /// the token once, when the closure was built, would pass that way and still + /// leave a live Ctrl-C waiting out the poll interval. So the cancel is fired + /// only after TeReGen has been polled once — the wait is by then inside its + /// inter-poll sleep, which must notice within one 100ms step. + /// + /// The bound is the assertion: TeReGen never reports the job finished here, + /// so a wait that does not keep observing the seam sleeps a full poll + /// interval (5s) before looking again, many times over. + #[tokio::test] + async fn a_cancel_mid_wait_abandons_it_promptly() { + let rrid = "SUSE:Maintenance:1:1"; + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path(format!("/reports/{rrid}/regenerate"))) + .respond_with(ResponseTemplate::new(202).set_body_json(serde_json::json!({"job": 9}))) + .mount(&server) + .await; + // Still running, forever: only the cancel can end this wait. + let (hit_tx, hit_rx) = tokio::sync::oneshot::channel(); + Mock::given(method("GET")) + .and(path(format!("/reports/{rrid}/status"))) + .respond_with(SignalOnFirstHit { + hit: std::sync::Mutex::new(Some(hit_tx)), + body: serde_json::json!({"minion_state": "running"}), + }) + .mount(&server) + .await; + + let (mut session, buf) = session_with_hosts(rrid, &["h1"], "ok"); + session.config = config_for(&server); + // Fires once the wait is genuinely under way. + let cancel = session.cancel_token(); + tokio::spawn(async move { + hit_rx + .await + .expect("the wait must poll TeReGen at least once"); + cancel.cancel(); + }); + + let args = matches(&Regenerate, &[]); + tokio::time::timeout( + std::time::Duration::from_secs(2), + Regenerate.call(&mut session, &args), + ) + .await + .expect("the wait must observe the cancel, not poll on") + .unwrap(); + + // It reports what it saw rather than claiming success or failure. + let out = buf.contents(); + assert!(out.contains("did not finish (state: running)"), "{out}"); + } + #[tokio::test] async fn unreachable_teregen_reports_cleanly() { // Point at a closed port so the POST fails at the transport layer. diff --git a/crates/mtui-core/src/commands/request_review.rs b/crates/mtui-core/src/commands/request_review.rs index 99feba57..964f48d8 100644 --- a/crates/mtui-core/src/commands/request_review.rs +++ b/crates/mtui-core/src/commands/request_review.rs @@ -28,6 +28,7 @@ use async_trait::async_trait; use clap::{Arg, ArgAction, ArgMatches}; use mtui_datasources::{PostedMessage, Slack, SlackError, is_ack_reaction, is_nack_reaction}; use mtui_testreport::{SlackReviewMarker, SvnRunner, TokioSvnRunner, svn_commit_testreport}; +use tokio_util::sync::CancellationToken; use crate::command::{Command, Scope}; use crate::commands::support::{require_update, template_completion}; @@ -132,7 +133,11 @@ fn resolve_channel(session: &Session, args: &ArgMatches) -> Result, + cancel: &CancellationToken, ) -> WatchOutcome { let deadline = Instant::now() + timeout; let mut failures: u32 = 0; @@ -191,7 +197,7 @@ async fn watch( .map_or(DEFAULT_BACKOFF, Duration::from_secs) .clamp(MIN_BACKOFF, MAX_BACKOFF); tracing::debug!(?wait, "rate limited; backing off"); - if sleep_or_interrupt(jittered(wait), deadline).await { + if sleep_or_interrupt(cancel, jittered(wait), deadline).await { return WatchOutcome::Interrupted; } if Instant::now() >= deadline { @@ -212,23 +218,32 @@ async fn watch( if Instant::now() >= deadline { return WatchOutcome::TimedOut; } - if sleep_or_interrupt(jittered(poll), deadline).await { + if sleep_or_interrupt(cancel, jittered(poll), deadline).await { return WatchOutcome::Interrupted; } } } -/// Sleep for `dur` (never past `deadline`), returning `true` if Ctrl-C arrived. +/// Sleep for `dur` (never past `deadline`), returning `true` if `cancel` fired. +/// +/// The point of interrupting the sleep rather than letting the press kill the +/// process is that the user must learn their review request *was* posted — +/// only the watching stopped. /// -/// Nothing in the CLI installs a SIGINT handler today, so without this a -/// Ctrl-C during a watch kills the process outright and the user never learns -/// that their review request was in fact posted. -async fn sleep_or_interrupt(dur: Duration, deadline: Instant) -> bool { +/// This selects the session's cancellation token rather than +/// `tokio::signal::ctrl_c` directly, so the watch has one interrupt source +/// instead of two, and that source is *attributable*: the REPL forwards Ctrl-C +/// onto the token it installed for this dispatch, and an MCP `job_cancel` +/// reaches the watch too. The old signal branch could not be reached from a +/// headless tool call (which has no terminal of its own) — and where a stdio +/// server did share the operator's terminal, a Ctrl-C fired *every* listener at +/// once, interrupting a watch that nobody had asked to stop. +async fn sleep_or_interrupt(cancel: &CancellationToken, dur: Duration, deadline: Instant) -> bool { let remaining = deadline.saturating_duration_since(Instant::now()); let dur = dur.min(remaining); tokio::select! { () = tokio::time::sleep(dur) => false, - _ = tokio::signal::ctrl_c() => true, + () = cancel.cancelled() => true, } } @@ -304,7 +319,7 @@ impl Command for RequestReview { .action(ArgAction::SetTrue) .help( "After posting, watch the message for reviewer reactions until a \ - verdict or timeout (Ctrl-C stops it). Over MCP, pair this with \ + verdict or timeout (Ctrl-C / job_cancel stops it). Over MCP, pair this with \ background=true so the call does not outlive the client timeout", ), ) @@ -384,12 +399,15 @@ impl Command for RequestReview { let poll = Duration::from_secs(session.config.slack_poll_interval); let timeout = Duration::from_secs(session.config.slack_watch_timeout); + // Both interrupt sources, because both surfaces read this line: Ctrl-C + // in the REPL, `job_cancel` over MCP (where there is no Ctrl-C at all). session.display.println(&format!( - "watching for reactions (up to {}s, Ctrl-C to stop)", + "watching for reactions (up to {}s, Ctrl-C / job_cancel to stop)", timeout.as_secs() )); - let outcome = watch(&slack, &posted, poll, timeout, bot_id.as_deref()).await; + let cancel = session.cancel_token(); + let outcome = watch(&slack, &posted, poll, timeout, bot_id.as_deref(), &cancel).await; report(session, &rrid.to_string(), &outcome); // A failed watch is a failed command: over MCP the caller needs a @@ -766,6 +784,118 @@ mod tests { assert!(!out.contains("approved"), "{out}"); } + /// A cancel stops the watch at its next sleep and says so: the request + /// itself was posted and stays posted, so this is not a failure — the + /// command still succeeds. + /// + /// The cancel is the session's own token, which is what a REPL Ctrl-C (now + /// forwarded onto it) and an MCP `job_cancel` both fire. Nothing raises a + /// real signal here: the old `tokio::signal::ctrl_c` branch could only have + /// been tested that way, which is why this branch had no test at all. + #[tokio::test] + async fn a_cancel_stops_the_watch_but_keeps_the_request_posted() { + let server = MockServer::start().await; + mount_post_path(&server).await; + // A message nobody has reacted to: without the cancel this would poll + // until the (long) timeout. + mount( + &server, + "reactions.get", + json!({ "ok": true, "message": { "reactions": [] }}), + ) + .await; + let (mut session, buf) = slack_session(&server); + // The poll interval is far longer than the deadline, so the one sleep + // this watch takes is bounded by the deadline: a watch that ignored the + // cancel would time out a few seconds later instead of stopping now. + session.config.slack_watch_timeout = 5; + session.config.slack_poll_interval = 60; + // Cancelled before the call, so the first inter-poll sleep is the one + // that observes it — no timing race, and the poll before it still gets + // its one honest look at the message. + session.cancel_token().cancel(); + + let args = matches(&RequestReview, &["--watch"]); + // Not an error: the posting succeeded, only the watching stopped. + RequestReview.call(&mut session, &args).await.unwrap(); + + let out = buf.contents(); + assert!( + out.contains("stopped watching; the request is still posted"), + "{out}" + ); + assert!(!out.contains("no review reaction"), "not a timeout: {out}"); + } + + /// The rate-limit branch takes its own sleep, so it needs its own interrupt + /// arm: a 429 can park the watch for up to a minute, and a Ctrl-C that had + /// to wait that out would look exactly like a hang. + #[tokio::test] + async fn a_cancel_interrupts_the_rate_limit_backoff() { + let server = MockServer::start().await; + // Slack asking us to wait a full minute. + Mock::given(path("/reactions.get")) + .respond_with(ResponseTemplate::new(429).insert_header("retry-after", "60")) + .mount(&server) + .await; + let slack = slack_for(&server); + let posted = PostedMessage { + channel: CHANNEL.to_owned(), + ts: TS.to_owned(), + }; + let cancel = CancellationToken::new(); + cancel.cancel(); + + // The bound is the assertion: the back-off is clamped to at least + // MIN_BACKOFF and here asks for 60s, so a branch that ignored the token + // could not possibly return this quickly. + let outcome = tokio::time::timeout( + Duration::from_secs(2), + watch( + &slack, + &posted, + Duration::from_millis(10), + Duration::from_secs(600), + None, + &cancel, + ), + ) + .await + .expect("the back-off must observe the cancel"); + + assert_eq!(outcome, WatchOutcome::Interrupted); + } + + /// The seam is the *only* interrupt source: an uncancelled watch runs to + /// its deadline exactly as before. + #[tokio::test] + async fn an_uncancelled_token_does_not_interrupt_the_watch() { + let server = MockServer::start().await; + mount( + &server, + "reactions.get", + json!({ "ok": true, "message": { "reactions": [] }}), + ) + .await; + let slack = slack_for(&server); + let posted = PostedMessage { + channel: CHANNEL.to_owned(), + ts: TS.to_owned(), + }; + + let outcome = watch( + &slack, + &posted, + Duration::from_millis(10), + Duration::from_millis(50), + None, + &CancellationToken::new(), + ) + .await; + + assert_eq!(outcome, WatchOutcome::TimedOut); + } + #[tokio::test] async fn watch_gives_up_after_repeated_failures_and_fails_the_command() { let server = MockServer::start().await; @@ -814,6 +944,7 @@ mod tests { Duration::from_millis(10), Duration::from_millis(1200), None, + &CancellationToken::new(), ) .await; @@ -845,6 +976,7 @@ mod tests { Duration::from_secs(60), Duration::ZERO, None, + &CancellationToken::new(), ) .await; @@ -877,6 +1009,7 @@ mod tests { Duration::from_millis(10), Duration::from_millis(250), None, + &CancellationToken::new(), ) .await; diff --git a/crates/mtui-core/src/entrypoint.rs b/crates/mtui-core/src/entrypoint.rs index 68c896c8..6b5f1924 100644 --- a/crates/mtui-core/src/entrypoint.rs +++ b/crates/mtui-core/src/entrypoint.rs @@ -25,12 +25,18 @@ //! //! mtui distinguishes clap/argparse's usage-error convention (exit `2`) from a //! runtime failure (exit `1`), while keeping `--help`/`--version` a success -//! (exit `0`). See [`ExitStatus`]. +//! (exit `0`). One status is not an argparse outcome at all: a REPL session +//! force-quit by a double Ctrl-C exits `130` (128 + `SIGINT`), the status the +//! process would have had when the signal still killed it outright. See +//! [`ExitStatus`], and `mtui_cli::ReplExit`, which maps onto it. +//! +//! This is the whole vocabulary. Anything downstream — packaging, wrapper +//! scripts, CI — can rely on `0`/`1`/`2`/`130` and nothing else. -/// The process exit status a single non-interactive command run yields. +/// A process exit status of either entrypoint. /// -/// mtui distinguishes exit codes to preserve the argparse/clap distinction -/// between a *usage* error and a *runtime* failure: +/// Three of the four preserve the argparse/clap distinction between a *usage* +/// error and a *runtime* failure; the fourth is the signal convention: /// /// * [`Ok`](ExitStatus::Ok) → `0` — the command ran, or clap printed /// `--help`/`--version` (a success in argparse terms). @@ -38,6 +44,8 @@ /// command, unbalanced quotes, or the command body erroring. /// * [`Usage`](ExitStatus::Usage) → `2` — a genuine argument *usage* error /// (clap/argparse's exit-2 convention). +/// * [`Interrupted`](ExitStatus::Interrupted) → `130` — the REPL was +/// force-quit by a double Ctrl-C (128 + `SIGINT`); see `mtui_cli::ReplExit`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ExitStatus { /// Success (or help/version output). Process exit code `0`. @@ -46,16 +54,23 @@ pub enum ExitStatus { Failure, /// Argument usage error. Process exit code `2`. Usage, + /// Force-quit by a double Ctrl-C. Process exit code `130`. + /// + /// Not an argparse outcome: it is 128 + `SIGINT`, so a caller that already + /// reads "killed by signal N" as 128 + N sees what it expects — which is + /// what the operator got before mtui handled the signal at all. + Interrupted, } impl ExitStatus { - /// The numeric process exit code (`0`, `1`, or `2`). + /// The numeric process exit code (`0`, `1`, `2`, or `130`). #[must_use] fn code(self) -> i32 { match self { ExitStatus::Ok => 0, ExitStatus::Failure => 1, ExitStatus::Usage => 2, + ExitStatus::Interrupted => 130, } } } @@ -75,5 +90,8 @@ mod tests { assert_eq!(i32::from(ExitStatus::Ok), 0); assert_eq!(i32::from(ExitStatus::Failure), 1); assert_eq!(i32::from(ExitStatus::Usage), 2); + // 128 + SIGINT: what a Ctrl-C death reported before mtui handled the + // signal, and what wrapper scripts already read as "interrupted". + assert_eq!(i32::from(ExitStatus::Interrupted), 130); } } diff --git a/crates/mtui-mcp/tests/snapshots/it__slim_profile__slimmed_command_tool_schemas_snapshot.snap b/crates/mtui-mcp/tests/snapshots/it__slim_profile__slimmed_command_tool_schemas_snapshot.snap index ba4d7b1e..3d410087 100644 --- a/crates/mtui-mcp/tests/snapshots/it__slim_profile__slimmed_command_tool_schemas_snapshot.snap +++ b/crates/mtui-mcp/tests/snapshots/it__slim_profile__slimmed_command_tool_schemas_snapshot.snap @@ -1182,7 +1182,7 @@ expression: pretty }, "watch": { "default": false, - "description": "After posting, watch the message for reviewer reactions until a verdict or timeout (Ctrl-C stops it). Over MCP, pair this with background=true so the call does not outlive the client timeout", + "description": "After posting, watch the message for reviewer reactions until a verdict or timeout (Ctrl-C / job_cancel stops it). Over MCP, pair this with background=true so the call does not outlive the client timeout", "type": "boolean" } }, diff --git a/docs/src/cli.md b/docs/src/cli.md index d34b9667..59e9051a 100644 --- a/docs/src/cli.md +++ b/docs/src/cli.md @@ -991,7 +991,7 @@ Options: Extra context appended to the review request -w, --watch - After posting, watch the message for reviewer reactions until a verdict or timeout (Ctrl-C stops it). Over MCP, pair this with background=true so the call does not outlive the client timeout + After posting, watch the message for reviewer reactions until a verdict or timeout (Ctrl-C / job_cancel stops it). Over MCP, pair this with background=true so the call does not outlive the client timeout -h, --help Print help diff --git a/docs/src/faq.md b/docs/src/faq.md index 6f043e4b..8fc070eb 100644 --- a/docs/src/faq.md +++ b/docs/src/faq.md @@ -166,6 +166,64 @@ Under the loaded template's checkout, in `install_logs` sub-directory name is configurable under `[mtui]`; see [Configuration](configuration.md). +## What does Ctrl-C do? + +It depends on whether a command is running. + +At the prompt, Ctrl-C clears the line you were typing and reprompts. It never +exits mtui — use `quit`, `exit`, or Ctrl-D for that, so the session's hosts are +unlocked and closed on the way out. + +While a command is running, the first Ctrl-C asks it to **stop at its next +checkpoint**. Every path releases the hosts' operation locks on the way out, so +nothing is left behind — but how soon the command stops depends on where its +checkpoints are: + +- `update` checks between its steps, and for the last time just before the point + of no return: once the patch command has been dispatched the update runs to its + end (rolling back on failure) rather than leaving a half-applied update behind. +- `prepare --installed-only`, and `downgrade` on non-transactional hosts, check + between packages. A transactional (SL-Micro) `downgrade` applies in one + transaction, so it finishes first like the commands below. +- A command applying to several loaded templates stops at the next template + boundary, and reports how many it got through. +- `install`, `uninstall`, `run`, and `reboot` finish the host operation already + under way first, then stop. + +A command that *did* stop at a checkpoint reports `error: cancelled`, naming what +it completed first where it can. A command with no checkpoint left to reach +simply finishes and reports normally — the cancel arrived too late to change +anything, and saying "cancelled" would be a lie about work that was in fact done. +Two long waits are their own case: `request_review --watch` and `regenerate` +stop watching and tell you so (the review request stays posted, the +regeneration keeps running on the server), and both count as success. Either +way the session stays usable — the next command starts with a clean slate. + +A second Ctrl-C **force-quits** (exit status 130). This is the escape hatch for a +host that has stopped responding, and it comes at a cost: the running command is +abandoned where it stands, so the operation locks it holds are left behind. Your +command history is still saved. mtui warns when this happens; see the next entry +for the cleanup. + +Ctrl-C during the **teardown** behaves the same way but cancels nothing: the +teardown is what releases the pool claims and closes the hosts, so it always runs +to completion. A press there warns that it is in progress, and a second one +force-quits — leaving both the operation locks and the pool claims +(`unlock --force` and `unlock --pool`). This holds however you asked to leave: +Ctrl-D and a typed `quit`/`exit` dispatch the same teardown and get the same +protection. + +Two carve-outs. Ctrl-C during the **initial load** at startup (`-a`/`-k`/`--sut`, +before the first prompt appears) still exits immediately, without teardown. Any +locks or pool claims that partial load had already taken need +`unlock --force`/`unlock --pool`, as after any crash. + +And in a **non-interactive (piped) session** — mtui reading commands from a pipe +rather than a terminal — there is no line editor holding the keyboard, so a +Ctrl-C arriving between commands is queued and then discarded when the next +command starts: it interrupts nothing. Interrupt such a session with a signal to +the process (`kill -INT`) *while a command is running*, or `kill` it outright. + ## How do I remove a dangling lock left by a crashed session? Reconnect to the same hosts and run `unlock -f` (force) to remove locks left by diff --git a/docs/src/mcp.md b/docs/src/mcp.md index 30976302..9b374700 100644 --- a/docs/src/mcp.md +++ b/docs/src/mcp.md @@ -292,7 +292,11 @@ Four job-control tools manage them: command's failure envelope if it failed. - **`job_cancel(job_id)`** — cancel a running job. (A command already executing on a host may run to completion there even after cancel returns — the same caveat as - Ctrl-C on a foreground `run`.) + Ctrl-C on a foreground `run`.) Two commands treat a cancel as a normal ending + rather than a failure: `request_review --watch` stops watching (the request + stays posted) and `regenerate` stops waiting (the server keeps building). Both + return success, so their job ends `done`, not `cancelled`, with the reply text + saying what was and was not finished. A job blocked mid host-operation cannot stop at a checkpoint, so cancelling it force-aborts the dispatch — which skips the operation's own `unlock()`. A forced