REPL Ctrl-C cancels the running command cooperatively instead of killing the process

[plusky]

Closes #441.

The bug

reedline holds raw mode only inside read_line, so a Ctrl-C at the prompt is a key event — but during a dispatched command the terminal is cooked, the key is a real SIGINT, and the default disposition kills mtui outright. The flows' "unlock always" sections never run, and unlike #405's in-process strand, the leftover /var/lock/mtui.lock carries a dead pid: it reads as foreign to every later run, and only unlock --force or the 24h stale reap clears it. quit's teardown (pool claims, host close) is skipped the same way.

A second, related defect fixed along the way: request_review --watch listened on tokio::signal::ctrl_c() directly, which arms tokio's process-permanent SIGINT handler as a side effect — so after one watch, a Ctrl-C during any later command was silently swallowed instead of doing anything. Ctrl-C behavior was non-deterministic across a session.

The fix

First press cancels, second press force-quits — driving the cooperative seam that already exists (the same one MCP job_cancel uses):

• A forwarder task (one persistent SIGINT stream, armed when the REPL starts) feeds presses into a bounded channel. The dispatch site drains stale presses (a press from an edit/idle gap must not cancel the next command — and with the two-slot queue, a leftover press would otherwise consume the cooperative slot so a genuine press force-quits), installs a fresh CancellationToken per dispatch (the token is one-shot; without the per-line install, a cancelled token would fail every later command at the driver pre-flight — including quit's teardown, re-stranding exactly these locks), and runs the dispatch under a biased select that never drops the command future: a press cancels the token and reports it; the flows unwind at their own checkpoints and release their locks; a second press escalates.
• The select is biased with the completion branch first, so a press racing completion can never force-quit a command that already finished (adversarial review measured the unbiased version doing exactly that ~50% of the time, with a false "locks may remain" warning on top).
• Escalation propagates out of the loop; main drops the editor first — the session's shell history survives a force-quit — then exits 130 with a warning naming unlock --force/--pool from a new session.
• Ctrl-D's teardown gets the same protocol minus the cancel: a press never cancels the teardown (it is the cleanup), but still counts toward force-quit — review found that with the handler armed, a teardown wedged on a black-holed host previously left the operator with no exit at all.
• The bespoke waits fold onto the seam: sleep_or_interrupt selects the session token (in the inter-poll sleep and the 429 backoff, which could block a cancel up to the 60s retry clamp), and regenerate's should-stop predicate — previously wired to a spinner flag nothing ever set, making its Ctrl-C claim dead code — reads the live token. Bonus this buys: MCP job_cancel now interrupts a running watch/regenerate wait at the poll boundary, which the signal-only implementation never could.

Honesty about what the first press does per command

Cancellation is cooperative and the flows differ (documented in the new FAQ entry): update, prepare --installed-only, non-transactional downgrade, and multi-template fan-outs stop at their checkpoints with error: cancelled and locks released (update's last checkpoint sits before the point of no return; a press after dispatch is deliberately inert). install/uninstall/run/reboot and transactional downgrades have no mid-op checkpoint by design — they finish the current host operation, release locks normally, and report their result. The folded waits stop and report success in their own words.

Known remaining window, stated rather than papered over: the startup seeding load (-a/--sut before the first prompt — the slowest phase in the program) still has the old kill-with-strand behavior; arming the handler there without a consumer would make Ctrl-C a silent no-op for a minute, which is worse. Routing seeding through the same drain/token protocol is the follow-up.

Tests

Twenty-one mutation proofs across two rounds, each observed red with the test demonstrably running. The adversarial round (three independent reviewers over the uncommitted tree) reshaped the change: the biased select, the teardown escalate arm, the history-preserving exit path, and five surviving mutants killed — including a drain while→if mutant whose real-world outcome (a stale press consuming the cooperative slot) would have been strictly worse than the original bug. Two proofs are documented as bounded rather than absolute: the biased-select pin is statistical (32 independent races per run; the unbiased mutant failed both ways when measured), and the one-line Ctrl-D arm wiring is unreachable from tests (TTY-bound) — narrowed structurally by the type change instead, and said so. No test raises a real signal; the interrupt source is channel-injected, and the thin tokio::signal forwarder plus process::exit(130) stay untested by design, per the repo's established pattern. Stress: 40 consecutive suite runs (~1280 select races), zero flakes.

Residuals (documented)

• The startup seeding window (above — follow-up sketch in the impl notes).
• release_pool_claims has no timeout of its own (pre-existing; CLOSE_TIMEOUT wraps only the host close) — the teardown escalate arm is the operator's exit meanwhile.
• The first-press notice and the escalation warning are tracing::warn! — suppressible by set_log_level error, matching the warn_on_unlock_failures precedent; a conscious choice, noted.
• Non-active template groups can retain a stale cancelled token after a cancelled line (nothing reads it today; activate refreshes the active group every dispatch).

Process

Three scouts (REPL/signal internals incl. vendored reedline/crossterm/tokio semantics, the per-command cancel-coverage map, the signal-test feasibility study), implementation, then three independent read-only adversarial reviews (signal semantics, test vacuity, contracts) over the uncommitted tree — thirteen findings, all fixed before the first commit existed.

[codecov]

Codecov Report

❌ Patch coverage is 91.47982% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.28%. Comparing base (f7deb72) to head (7708d6a).

Files with missing lines	Patch %	Lines
crates/mtui-cli/src/repl.rs	90.65%	50 Missing ⚠️
crates/mtui-cli/src/main.rs	20.00%	4 Missing ⚠️
crates/mtui-core/src/commands/regenerate.rs	93.61%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   96.32%   96.28%   -0.04%     
==========================================
  Files         193      193              
  Lines       42260    42907     +647     
==========================================
+ Hits        40705    41315     +610     
- Misses       1555     1592      +37     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[mimi1vx]

Reviewed against a local worktree of the PR head (b3878cc1).

Gates run locally, all green: cargo clippy --workspace --all-targets -- -D warnings, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features --document-private-items, cargo test -p mtui-cli -p mtui-core (770 tests).

The core design is right: cooperative-first, biased select, fresh token per dispatch, dispatch future never dropped on the first press. I verified the parts that are easy to get wrong and they hold (details at the bottom). Three things need fixing before merge — one behavioural, two that record load-bearing constraints the code currently leaves undocumented or contradicted.

---

Critical (must fix)

1. The teardown protection only covers Ctrl-D, not a typed quit/exit — and three documents claim otherwise

step_teardown (fresh token, escalate-only, no cancel arm) is reachable only from Signal::CtrlD (crates/mtui-cli/src/repl.rs:287). A typed quit, exit, or exit reboot is Signal::Success(line) (repl.rs:208) and goes through step_interruptible, which does token.cancel() on the first press and, on the second, emits the mid-command warning (repl.rs:265):

forcing exit mid-command; operation locks may remain on the update's hosts — release them with unlock --force from a new session

In the exact scenario the escalate arm was added for — a blackholed refhost parking release_pool_claims, which has no timeout of its own — an operator who typed quit instead of pressing Ctrl-D force-quits and is told to run unlock --force, with no mention of unlock --pool. The RRID-based pool claim is genuinely stranded on the fleet and the message points nowhere near it.

The cancel itself is inert today (Quit::call never consults the seam, and neither release_pool_claims nor HostsGroup::close gate on it), so this is wrong operator guidance plus a latent contract hole rather than an immediate strand — but it is a hole: the moment anything on the close path gains a checkpoint, a typed quit cancels its own cleanup, which is precisely #441.

Three documents assert the fixed behaviour:

• CHANGELOG.md:218 — "During the Ctrl-D/quit teardown a press cannot cancel anything"
• docs/src/faq.md:208 — "Ctrl-C during the Ctrl-D/quit teardown behaves the same way but cancels nothing"
• AGENTS.md:233 says "Ctrl-D teardown", which is accurate — so the contract file and the user-facing docs now disagree with each other.

Suggested fix, matching the existing is_shell_line / is_edit_line precedent already in this loop: resolve the line's first token through the registry and route a Quit hit to step_teardown; or give step_interruptible a cancel_on_press: bool (see Warning 3, which wants the same seam). Please add a test that a typed quit never observes a cancelled token — a_press_during_the_teardown_does_not_cancel_it mirrored onto step_interruptible with SlowQuitCmd is a two-line variant, and it should be observed red first.

2. main.rs — the reason this cannot become fn main() -> ExitCode is undocumented, and it is a deadlock rather than a style preference

The obvious later "modernise the binary" cleanup is:

fn main() -> anyhow::Result<ExitCode> { …; Ok(ending.into()) }

which runs destructors and lets the drop(repl) dance be deleted. It would hang the force-quit. Returning from main drops runtime, and Runtime::drop blocks until in-flight spawn_blocking tasks complete. There is one live on exactly this path: Prompter::stdin() (crates/mtui-hosts/src/prompter.rs:73-90), installed at the composition root (crates/mtui-cli/src/main.rs:78), performs a blocking io::stdin().read_line() on the blocking pool to back the SSH command-timeout question. If the operator double-presses Ctrl-C while keep waiting? [Y/n] is outstanding — the single most likely moment to force-quit — that read never returns, so Runtime::drop never returns. std::process::exit(130) is what escapes it.

The comment at main.rs:95-100 justifies only drop(repl) (the history flush) and says nothing about why process::exit rather than ExitCode. By this repo's own rule — a load-bearing shape must not be left explained by a comment about something else — this constraint needs recording, or the next cleanup pass silently reintroduces a hang in the escape hatch. One sentence is enough:

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.

3. A second exit-code vocabulary, and the documented exit-code contract left stale

mtui_core::ExitStatus (crates/mtui-core/src/entrypoint.rs:42) already exists, is exported, and its module doc carries a ## Exit-code contract section presenting itself as the authority (Ok=0, Failure=1, Usage=2, with From<ExitStatus> for i32). It is consumed by seed_session (ControlFlow<ExitStatus>) and by mtui-mcp/src/main.rs:28.

After this PR main speaks two exit vocabularies nineteen lines apart:

std::process::exit(code.into());  // main.rs:85  — ExitStatus
std::process::exit(code);         // main.rs:104 — ReplExit / EXIT_INTERRUPTED

and entrypoint.rs's contract doc never learns that 130 exists. A reader of the file that claims to define mtui's exit codes concludes mtui only ever exits 0/1/2 — which packaging, wrapper scripts, and CI will act on.

Preferred fix: add ExitStatus::Interrupted => 130 and map ReplExit::ForceQuit onto it. 130 is a process-level convention, not a REPL-internal detail, and it belongs in the one place downstream will look (ExitStatus's own doc line — "the process exit status a single non-interactive command run yields" — needs widening if you take this route). Minimum acceptable: extend the ## Exit-code contract section to name 130 and cross-reference ReplExit.

---

Warnings (should fix)

1. [repl.rs:264, repl.rs:299] The escalation warning is warn!, so set_log_level error silences the one message that names the stranded locks. The PR notes the first-press notice is suppressible and calls that a conscious choice — agreed, that one is informational. The escalation line is a different class of event: it is the only record that the process is abandoning locks, emitted at the moment an operator most needs it. tracing::error! survives the level operators actually set. Same for the mid-teardown variant.

2. [repl.rs:433-535] step_interruptible and step_teardown are ~90% duplicated. Drain, fresh token, tokio::pin!, biased select, is_none fall-through, escalate-on-second — identical; the only differences are token.cancel() and the warning text. One function with a cancel_on_press: bool would also make Critical 1 a one-line call-site change instead of a third copy. Two independent copies of a subtle select protocol is the shape that drifts.

3. [repl.rs:253-271, repl.rs:287-307, main.rs:94-105] The two Repl::run escalate arms and the drop-then-exit ordering have no coverage. TTY-boundness is a fair reason for the signal source, but the wiring here — which warning pairs with which arm, and drop(repl) preceding process::exit — is what the changelog makes a user-visible promise about ("without discarding the session's command history"). FileBackedHistory's sync-on-drop makes that ordering genuinely load-bearing and nothing enforces it. Extracting fn on_escalate(kind) -> ReplExit would at least pin the arm→message pairing.

---

Suggestions

• [main.rs:101] drop(repl) drops far more than "the editor". Repl is the sole owner of Arc<Mutex<Session>>, so this synchronously tears down the entire session graph — every SSH Target, the reqwest client, the openQA transport, the template registry — before process::exit, on the one path whose whole job is get out now. I checked: nothing in that chain blocks today (no Drop on Session/Target; SpinnerGuard::drop / TtySpinner::drop only handle.abort() plus std-mutex writes). But the blast radius is wider than the comment claims. What you actually want is "run exactly one destructor, then exit", which neither ExitCode nor process::exit expresses — worth naming, and worth narrowing (Repl::into_history(), or dropping only line_editor) so this path cannot grow a blocking destructor by accident.

• [repl.rs:68, repl.rs:90] i32 is the wrong domain for an exit code. WEXITSTATUS is 8 bits, so process::exit truncates mod 256; EXIT_INTERRUPTED: i32 / exit_code() -> Option<i32> admits values that silently change meaning. 130 is fine and ExitStatus has the same shape, so consistency partly excuses it — but if Critical 3 lands as ExitStatus::Interrupted, make the domain u8 (or std::process::ExitCode right at the boundary, which is that type's one genuinely good use here even though it cannot be returned from main).

• MCP semantics of "cancel is success." Folding both waits onto the seam means a job_cancel on a request_review --watch or regenerate job now ends with the command returning Ok, i.e. job_status → done, not cancelled. AGENTS.md was amended to carve this out and that is defensible for the REPL, but a client that just called job_cancel and is told the job succeeded is a surprising surface. Worth a sentence beside the job_cancel bullet in docs/src/mcp.md:293.

• [request_review.rs:403] The watch banner still reads "watching for reactions (up to {}s, Ctrl-C to stop)". Over MCP there is no Ctrl-C, and job_cancel now works — cheap to say "Ctrl-C / job_cancel".

• [repl.rs:345-388] A non-TTY REPL now swallows SIGINT while idle. With the handler armed from the first prompt, a piped session (no raw mode, so Ctrl-C at the "prompt" is a real SIGINT) queues the press and then discards it at the next drain — the process becomes un-interruptible by Ctrl-C between commands. Narrow, and arguably out of scope, but it is a behaviour change the FAQ's "Ctrl-C at the prompt is unchanged" does not cover.

---

Verified sound

For the record, the parts I specifically tried to break and could not:

• The biased; ordering and its 32-round race pin — I traced the poll interleaving in a_press_racing_completion_never_force_quits; the presses really are queued in the same poll window as the completion, so the test would be a coin flip without biased.
• Unconditional fresh-token-per-dispatch against Command::run's pre-flight check_cancelled: run calls session.activate(...) on both the single-template and fan-out paths, which re-pushes the fresh token onto the active HostsGroup, so update_flow's five targets.cancel_requested() checkpoints observe the right token. The stale-token residual on non-active groups is genuinely inert — Quit, close, and release_pool_claims never read the seam, and run_parallel deliberately doesn't.
• edit and shell are intercepted before dispatch, so the "idle gap" drain rationale is accurate and neither can be cancelled mid-editor. tokio's sigaction handler is reset on exec, so child editors/shells keep default SIGINT; the shell bridge holds local raw mode, so Ctrl-C there is a byte to the remote PTY, not a signal.
• Drain loop terminates on both Empty and Disconnected; mpsc::Receiver::recv is cancel-safe in the select; the press.is_none() fall-through is unreachable-but-correct.
• drop(repl) before process::exit is genuinely required (reedline syncs FileBackedHistory on drop) and runtime correctly outlives it.
• No secrets reach the display or the log on any new path.

[plusky]

All addressed in 7708d6a2.

• C1+W2 folded together: step_teardown is gone — one step_interruptible(..., OnPress::{Cancel, EscalateOnly}); a typed quit/exit/EOF resolves its first token through the registry (the is_shell_line precedent) and gets the no-cancel protocol with the --force/--pool warning. help quit/echo quit don't match. Your SlowQuitCmd test exists and was observed red against the pre-review routing; the three docs now say "Ctrl-D or a typed quit/exit" truthfully.
• C2: your sentence, verbatim in spirit, at the exit site.
• C3+S2: ExitStatus::Interrupted (130) with the contract doc extended and ReplExit mapping through the one From<ExitStatus> for i32; i32 kept for consistency with the existing conversion.
• W1: both force-quit records are error! (a test pins that they outrank set_log_level error); the first-press notice stays warn per your split.
• W3: on_escalate(OnPress) -> ReplExit extracted; a test pins the arm→message pairing.
• S1: drop(repl.into_line_editor()) — exactly one destructor (the history sync) runs before process::exit; the session graph is deliberately leaked on the get-out-now path.
• S3/S4: mcp.md notes a cancelled watch/regenerate job ends done, not cancelled, and why; the banner and the --watch clap help both say "Ctrl-C / job_cancel" (hence one regenerated cli.md line + the matching schema-snapshot line).
• S5: documented in the FAQ (piped sessions queue-and-discard a between-commands Ctrl-C; kill -INT is the escape) and listed as a residual rather than changed.

Four new mutation proofs cover the routing, the token-position match, the pairing, and the error! level.