fix(dash): restore terminal on SIGTERM/SIGINT

[r0x0r]

Problem

A signal delivered to rocm dash — kill <pid> (SIGTERM), a supervisor stop,
or Ctrl-C (SIGINT) — took the default disposition and killed the process while
it still held the terminal in raw mode on the alternate screen. The best-effort
teardown after the event loop never ran, leaving the shell in a broken display
state (no cursor, raw mode on, stuck in the alt-screen) that needs reset.

Bare rocm was worse. It is a persistent hub: it outlives each session's Tokio
runtime and returns to a synchronous, raw-mode launcher menu between flows.
Tokio never unregisters the libc handler it installs, so once a session's
runtime is dropped nothing drains the signal pipe — the menu sat in raw mode,
catching and discarding SIGTERM/SIGINT (unkillable) after the user's first flow.

Fix

• app::run installs a termination-signal watcher (spawn_termination_watcher)
  before entering raw/alternate-screen mode. On SIGTERM/SIGINT (Unix) or
  Ctrl-C/Ctrl-Break (Windows) it restores the terminal and exits 128 + signo
  (SIGINT → 130, SIGTERM → 143). Registration is propagated via ? before any
  terminal state is mutated. The task is aborted on the clean-return path.
• dash::run_launcher keeps one multi_thread(1) runtime alive for the
  whole hub and spawns a single process-lifetime watcher on it, so every window
  — the launcher menu and each session — has a live reactor to restore the
  terminal and exit, across the session runtimes built and dropped in the loop.
• Teardown is factored into restore_terminal; the escape-sequence half is
  write_restore_sequences<W: Write> so it is unit-testable against an
  in-memory sink without mutating the shared terminal.

process::exit on a signal is deliberate (see spawn_termination_watcher
docs): we stop promptly rather than race an orderly teardown, so an in-flight
focused-install child is left to the OS (matching the default disposition) and
the embedded daemon socket self-heals on the next bind.

Signal handling uses tokio::signal (already available via tokio's full
feature) — no new dependency.

Tests

• write_restore_sequences_leaves_alt_screen_and_shows_cursor — asserts the
  emitted restore bytes against an in-memory sink.
• signal_exit_codes_follow_shell_convention — the 128 + signo mapping.
• Linux PTY scenarios in dash.feature: a steady-state SIGTERM (→143) and
  SIGINT (→130) to a running dashboard, and a launcher round-trip (open the
  dashboard, quit back to the menu, then SIGTERM → 143 + terminal restored) that
  exercises the across-session hub path. Verified red without the hub watcher,
  green with it.

The startup register-before-raw-mode ordering is covered by construction
(spawn_termination_watcher runs before enable_raw_mode); a 20 ms-polled PTY
scenario cannot observe that microsecond window, so none is claimed for it.

[r0x0r]

Thanks for tackling the broken-terminal-on-signal case — the bounded, best-effort restore_terminal() helper, the 128 + signo exit codes, and the signal_task.abort() on a clean return are all correct and nicely tested.

One substantive interaction worth deciding explicitly: the signal task ends in std::process::exit(code), which skips stack unwinding and therefore the kill_on_drop teardown of any in-flight focused install/serve child. On a clean quit, event_loop returning unwinds the runtime and kills those children; on the signal path they are now left running (orphaned). This isn't a regression versus today's default disposition (which also skips kill_on_drop), but this PR is the natural place to decide the intended behavior: should a SIGTERM'd rocm dash leave a mid-write focused install running, or terminate it with the child? The comment at focused_close_key_blocked shows the truncation hazard is already on your radar, so a one-line statement of intent in the code or PR body would close the question either way.

Minor related note (not blocking): once tokio's signal handlers are installed they stay registered process-wide even after signal_task.abort(), so during the brief clean-exit teardown window a SIGTERM is caught-and-ignored rather than terminating. The process is exiting anyway, so this is fine to leave — just worth being aware of.

[michaelroy-amd]

PR #326 review — request changes

Head reviewed: 4048d69d8fcfd1b70f2a1a157cccdfbc28e5afdc

Standards

Blocking — Windows build is red and Ctrl-Break is not handled

crates/rocm-dash-tui/src/app/mod.rs:1688-1699

The PR claims Windows Ctrl-C and Ctrl-Break support, but tokio::signal::ctrl_c() subscribes only to CTRL_C_EVENT; Tokio exposes tokio::signal::windows::ctrl_break() separately. Consequently EXIT_CODE_SIGTERM is unused on Windows, and the live windows-build-and-test check fails under -D warnings at line 1699. Handle both Windows event streams (and their registration errors) rather than suppressing the warning. The current let _ = tokio::signal::ctrl_c().await also treats registration failure as a received Ctrl-C and exits 130 immediately.

Blocking — required user-observable scenario is absent

tests/e2e-cucumber/features/dash.feature has no scenario covering this change. AGENTS.md §3 requires a Gherkin scenario for user-observable CLI behavior; the two unit tests only check constants and that a no-TTY helper call does not panic. The existing PTY harness already owns portable_pty::Child, whose process_id() API makes signal injection feasible. Add Linux PTY scenarios that deliver SIGTERM/SIGINT, assert 143/130, and verify the final output leaves the alternate screen and shows the cursor.

Spec

Blocking — a startup SIGTERM still reproduces the broken-terminal bug

crates/rocm-dash-tui/src/app/mod.rs:1610-1628

The code enters raw/alternate-screen mode before spawning an async task whose first poll registers the signal listeners. A signal received after EnterAlternateScreen but before that task is polled still takes the default disposition and skips restoration. In a PTY smoke test that sent SIGTERM as soon as \x1b[?1049h was observed, all 10/10 runs exited from the signal (-15) without LeaveAlternateScreen; delayed SIGTERM/SIGINT correctly exited 143/130 and emitted restoration sequences. Register the signal listeners synchronously before mutating terminal state, then move the initialized listeners into the watcher task. Propagate setup failure before entering raw mode; this also avoids permanently swallowing a successfully registered Unix signal if the second registration fails.

Should fix — implementation does not reuse the shared teardown helper

crates/rocm-dash-tui/src/app/mod.rs:1636-1646

The PR body says restore_terminal is reused by both signal and normal teardown paths, but normal return still duplicates the old teardown sequence. Either call the helper on the normal path or correct the PR description. One shared path avoids the two sequences drifting.

Verification

• cargo test -p rocm-dash-tui: 686 passed; 4 ignored.
• cargo clippy -p rocm-dash-tui --all-targets -- -D warnings: passed on Linux.
• cargo build -p rocm: passed on Linux.
• PTY delayed-signal smoke: SIGTERM → 143 and SIGINT → 130; both emitted LeaveAlternateScreen and Show.
• PTY startup-race smoke: 10/10 immediate SIGTERM runs exited -15 without LeaveAlternateScreen.
• Live CI: windows-build-and-test failed because EXIT_CODE_SIGTERM is unused; all other required completed checks passed at the reviewed head.

Decision: request changes.

[r0x0r]

Thanks for the thorough review — addressed all four points in fd15c13:

Blocking — Windows Ctrl-Break / red windows-build-and-test. Replaced the single tokio::signal::ctrl_c() with a TerminationSignals type that registers both tokio::signal::windows::ctrl_c() and ctrl_break(). Ctrl-Break maps to the SIGTERM code and Ctrl-C to the SIGINT code, so EXIT_CODE_SIGTERM is now used on Windows and the -D warnings build is green. A failed registration is propagated (via ?) instead of the old let _ = ctrl_c().await that treated registration failure as a received Ctrl-C and exited 130 immediately.

Blocking (Spec) — startup SIGTERM race. The listeners are now installed before enable_raw_mode()/EnterAlternateScreen. Registration returns a TerminationSignals value that is moved into the watcher task, so a signal delivered any time after the terminal switches modes — even before the task's first poll — is latched by the already-installed handlers rather than taking the default disposition. Setup failure is propagated before any terminal state is mutated.

Blocking — required user-observable scenario. Added three Linux PTY scenarios in dash.feature (11–13): a steady-state SIGTERM (→143) and SIGINT (→130) to a running dashboard, plus a startup-window SIGTERM fired the instant the alternate screen is entered. Each asserts the conventional exit code and that the terminal left the alternate screen and showed the cursor. The harness gained signal delivery via kill(1) on Child::process_id(), exit-code capture, and alt-screen/cursor restoration assertions read from the vt100 screen.

Should fix — shared teardown. The normal-return path now calls the same restore_terminal() helper the signal path uses, so the two teardown sequences can't drift.

Local: cargo test -p rocm-dash-tui --lib (656 passed), cargo clippy -p rocm-dash-tui --all-targets -- -D warnings, and cargo clippy -p e2e-cucumber --test e2e -- -D warnings all pass. The new PTY scenarios are @requires-os:linux (no GPU needed) and run on the Linux E2E lanes; I can't execute them on macOS locally, so I'm watching CI to confirm.

[r0x0r]

@michaelroy-amd all four points are addressed in fd15c13:

• Windows Ctrl-Break / red build — replaced the single ctrl_c() with a TerminationSignals type that registers both windows::ctrl_c() and ctrl_break() (Ctrl-Break → SIGTERM code, Ctrl-C → SIGINT code), so EXIT_CODE_SIGTERM is used and -D warnings is green; registration failure now propagates via ? instead of being treated as a received Ctrl-C.
• Startup SIGTERM race — listeners are registered before enable_raw_mode()/EnterAlternateScreen and the initialized value is moved into the watcher task, so a signal delivered before the first poll is latched rather than taking the default disposition; setup failure propagates before any terminal mutation.
• Required scenario — added three Linux PTY scenarios in dash.feature (steady-state SIGTERM→143, SIGINT→130, and a startup-window SIGTERM), each asserting the exit code plus leaving the alternate screen and showing the cursor.
• Shared teardown — the normal-return path now calls the same restore_terminal() helper as the signal path.

windows-build-and-test and all E2E lanes are green on the current head. Ready for a re-review — thanks.

[rominf]

The fix itself is right and the revision history shows it — registering before raw mode, propagating setup failure before any terminal state is mutated, the shared restore_terminal helper, 128 + signo codes, and signal_task.abort() on a clean return. @michaelroy-amd's startup-race point is properly addressed, and the Ctrl-Break split is a genuine improvement over ctrl_c() alone.

I want to push on one thing @r0x0r raised and marked non-blocking. The observation was exactly right — tokio never unregisters the libc handler, so it survives signal_task.abort() — but the conclusion ("the process is exiting anyway, so this is fine to leave") holds only for rocm dash. On bare rocm it doesn't: dash::run_launcher is a synchronous loop that calls run_focused / run_chat / run, each of which builds a runtime, runs a session, drops the runtime, and returns to the loop. The process keeps living, in the launcher's own raw-mode/alt-screen event::read() loop, with a handler installed that catches SIGTERM and does nothing.

I built a probe mirroring that structure rather than reasoning about it. Control (no registration) exits 143 on the first SIGTERM. The PR-shaped version survives three SIGTERMs and a SIGINT and exits 0. Details in the first comment.

That makes bare rocm — the default invocation — unkillable by the default signals after the user's first flow, while sitting in exactly the raw-mode state this PR set out to protect. So I don't think it's leaveable, though the fix is small.

Two smaller things: scenario 13 doesn't reach the window it names (the polling granularity is ~1000× the race it guards), and the new restore_terminal unit test drives real global console state in the cargo test Windows lane. Also one stale doc link.

On @r0x0r's kill_on_drop question — worth answering explicitly in the code, and I'd note that whichever way it goes, process::exit in a peer task also skips run_async's embedded-daemon socket cleanup, so the two have the same shape.

[rominf]

This registration is process-global and permanent, but it's scoped as if it were per-session — and on the bare-rocm path the process outlives the session.

dash::run_launcher (apps/rocm/src/dash.rs:359-377) is a synchronous loop:

loop {
    let serving = launcher_serving_instances(&paths);
    match rocm_dash_tui::ui::launcher::run_launcher(&theme, serving)? {
        None => return Ok(()),
        Some(choice) => match launcher_route(choice) {
            LauncherRoute::Focused(focus) => run_focused(focus)?,
            LauncherRoute::Chat => run_chat(chat_mock, ChatInferenceParams::default())?,
            LauncherRoute::Dashboard => run(None, false, chat_mock)?,
        },
    }
}

Each of those three builds a runtime, block_ons run_async → app::run, and drops the runtime on return. Tokio documents that the libc handler it installs is never removed (tokio/src/signal/unix.rs: "the libc signal handler is never unregistered"), and that handler doesn't terminate — it sets a flag and writes a byte to tokio's global pipe. After the runtime is gone there's no reactor draining that pipe and no listener, so the signal is caught and discarded.

Control returns to ui::launcher::run_launcher, which enters raw mode and the alt-screen itself (ui/launcher.rs:285-289) and blocks in event::read(). So the process sits in raw mode, permanently deaf to SIGTERM and SIGINT.

I built a probe with the same shape rather than trusting the reading:

########## CONTROL: no registration ##########
exit=143

########## PR SHAPE: register, drop runtime, keep looping ##########
session returned; runtime dropped
PID 47073 now in the synchronous outer loop
BUG: survived 5s of signals
exit=0

Three kill -TERM plus a kill -INT, all absorbed. The only difference between the two runs is whether a signal() stream was ever constructed.

Windows has the same shape for a different reason: SetConsoleCtrlHandler is installed once behind a OnceLock and the handler returns 1 ("handled") while the static watch::Sender lives, so Ctrl-C stops terminating the launcher too.

The practical effect: rocm → Set up → back at the menu → kill <pid> does nothing, and Ctrl-C doesn't either. That's the default invocation, and the terminal is in raw mode the whole time — a worse version of the bug this PR fixes.

This isn't a reason to revert; reverting leaves the handler installed too, since the current code also calls ctrl_c()/signal() inside the session. The registration needs to move up to where its lifetime matches the process: install the listeners once in apps/rocm's entry path with something alive to service them for the whole launcher loop, or keep a process-global watcher that the launcher also honours. A fallback in the synchronous launcher that restores the terminal and exits would work too, and might be simpler.

Worth a scenario as well — the existing PTY harness could drive rocm (not rocm dash), pick a flow, return to the menu, and then deliver SIGTERM. Scenarios 11/12 only cover the single-session path, which is why this is invisible today.

[rominf]

Scenario 13 can't observe the window it describes, so it doesn't guard the ordering fix.

The race is between enable_raw_mode() and the point the listeners become effective — microseconds. The scenario reaches it via:

1. wait_for_alternate_screen, which polls at POLL_INTERVAL = 20ms (tui_driver.rs:51) until the parser has already consumed \x1b[?1049h;
2. deliver_signal_and_wait, which fork/execs kill(1) — another process spawn.

By the time the signal lands, the event loop has been running for many milliseconds. So scenario 13 is behaviorally identical to scenario 11, and moving TerminationSignals::register() back below enable_raw_mode() would leave all three green. AGENTS.md §3 wants the regression test to fail before the fix.

Notably @michaelroy-amd's own smoke test did catch it (10/10 immediate SIGTERMs exited -15 without LeaveAlternateScreen) — but that harness armed on the raw byte stream, not on a 20ms poll of the parsed screen.

Two options that would work: have the product emit a startup marker before enable_raw_mode and arm a pre-forked helper on it, or cover the ordering with a unit test asserting registration precedes raw-mode entry. Dropping scenario 13 and saying in the PR text that the ordering is covered by construction is also honest — better than a scenario that reads as a guard and isn't one.

(Scenarios 11 and 12 are genuine: on main a signalled child surfaces as code=1 through portable_pty, not 143.)

[rominf]

This test mutates process-global terminal state, which is safe under nextest and not under cargo test.

restore_terminal() calls the real disable_raw_mode() and unconditionally writes \x1b[?1049l, the mouse-disable sequence, and \x1b[?25h to the process's shared stdout. On Windows disable_raw_mode is a SetConsoleMode on the shared console input handle; on Unix it's a tcsetattr guarded by crossterm's TERMINAL_MODE_PRIOR_RAW_MODE static.

The Linux lane runs cargo nextest run (ci.yml:328), which is process-per-test and isolates this completely. The required Windows lane runs cargo test --workspace --all-targets (ci.yml:357) — threads in one process — so this races every other test in the binary. It's the kind of thing that shows up as an unreproducible Windows-only flake much later.

It also writes escape sequences into the developer's shell when run locally under a real terminal.

Taking an impl Write for the escape-sequence half would let the test drive an in-memory sink and assert the emitted bytes — which is a stronger assertion than "didn't panic" anyway. The disable_raw_mode() call is the part that has to stay global, and it's also the part the test isn't really checking.

[rominf]

Two things follow from exiting inside the spawned task rather than unwinding through it, both minor on their own but they share a fix with @r0x0r's kill_on_drop question.

Socket leak. run_async (apps/rocm/src/dash.rs:662-672) aborts the embedded daemon and remove_files the socket only after app::run(args).await returns. process::exit never unwinds, so every SIGTERM/SIGINT leaves the socket path on disk. It self-heals — rocm-dash-daemon/src/server.rs unlinks a stale path on next bind — so the impact is small, but a previously-clean shutdown becomes a leaking one.

Concurrent-write race. The runtime is multi-threaded (dash.rs:222 new_multi_thread), so this task runs on a different worker than event_loop. It emits LeaveAlternateScreen + Show while the event loop may be mid-terminal.draw() on the same stdout. An interleaving of restore → draw → exit paints a partial dashboard frame into the user's normal-screen scrollback. Narrow, but it's the same class of corruption this PR fixes, and unlike the clean-exit path there's no ordering guarantee here.

Both go away with the same restructuring: make the signal a select! arm around event_loop so the loop unwinds first and the single existing teardown runs, then return the exit code up through run/run_async and process::exit at the top level. That also puts kill_on_drop back in play for @r0x0r's question — whichever answer you want, it becomes expressible rather than foreclosed.

[rominf]

Stale intra-doc link: expect_signal_exit_code doesn't exist on TuiSession. The accessor is observed_exit_code (:609) and the assertion lives in the step at dash_steps.rs. (expect_terminal_restored on the next line is fine — that one's real, at :617.)

rustdoc isn't a CI gate here, so nothing catches it.

[r0x0r]

Thanks @rominf, @michaelroy-amd, @r0x0r — rebased onto main and addressed every point. Force-pushed as a single squashed commit (2496202).

Blocking — bare rocm was unkillable across sessions (@rominf)

Your probe was exactly right: the per-session watcher registered inside app::run, but on the bare-rocm path the process outlives each session's runtime, and once that runtime drops nothing drains Tokio's signal pipe — so the synchronous launcher menu sat in raw mode, deaf to SIGTERM/SIGINT.

Moved the watcher up to where its lifetime matches the process. dash::run_launcher now builds one multi_thread(1) runtime and spawns a single process-lifetime watcher on it (rocm_dash_tui::app::spawn_termination_watcher) before the first menu paint, keeping that runtime alive for the whole hub loop. Its worker keeps draining the signal pipe across every session runtime built and dropped inside the loop, so both the menu and each session stay killable. The registration happens inside the runtime context; the returned JoinHandle is dropped on purpose (drop detaches, it doesn't abort), so the watcher lives for the process.

app::run (direct rocm dash) keeps its own watcher-with-abort() on the clean-return path — that path's process dies with the session, so nothing changed there.

Blocking — required scenario now exercises the across-session path (@rominf)

Replaced the ineffective startup-race scenario 13 with a launcher round-trip: open bare rocm, escalate into the dashboard (d), quit back to the menu (q) — dropping that session's runtime — then deliver SIGTERM and assert 143 + terminal restoration. This is the exact "back at the menu after a flow" state you described.

Verified fail-before/pass-after locally on this Linux host: with the hub watcher removed the scenario is red (the SIGTERM step hangs/does not restore), and green with it. Scenarios 11/12 (direct-dash SIGTERM/SIGINT) are unchanged and still pass.

scenario 13 couldn't observe its window (@rominf)

Agreed — a 20 ms parser poll is ~1000× the register-before-raw-mode race, so it read as a guard without being one. Dropped it. The ordering is now covered by construction: spawn_termination_watcher() is called before enable_raw_mode() in app::run, and registration is surfaced via ? before any terminal state is mutated. I didn't claim a PTY scenario for a microsecond window it can't see.

restore_terminal test mutated global console state (@rominf)

Split the helper: restore_terminal() still calls the global disable_raw_mode(), but the escape-sequence half is now write_restore_sequences<W: io::Write>(out). The unit test drives an in-memory Vec<u8> sink and asserts the emitted bytes (\x1b[?1049l, \x1b[?25h) — a stronger assertion than "didn't panic", and it no longer touches the shared terminal in the single-process cargo test Windows lane.

process::exit — socket leak, concurrent-write, kill_on_drop (@rominf, @r0x0r)

Stated the intent explicitly in spawn_termination_watcher's docs. On a signal we process::exit(128+signo) and deliberately do not unwind: SIGTERM/SIGINT means "stop now", so we restore the terminal and leave rather than race an orderly teardown against an imminent default-disposition kill. The two consequences are accepted as intended: (1) an in-flight focused install/serve child is left to the OS rather than reaped via kill_on_drop — matching the pre-existing default disposition and avoiding truncating a mid-write child on the way out; and (2) run_async's embedded-daemon socket isn't unlinked here, but the daemon unlinks a stale socket on its next bind, so it self-heals. As you noted, both share the same "peer task exits" shape.

stale doc link (@rominf)

Fixed deliver_signal_and_wait's rustdoc — it now points at observed_exit_code (the real accessor) and notes the assertion lives in the Then step, dropping the nonexistent expect_signal_exit_code.

---

Gates green locally: cargo clippy -p rocm-dash-tui -p rocm -p e2e-cucumber --all-targets -- -D warnings, cargo test -p rocm-dash-tui (656 passed), the three signal scenarios, and python3 scripts/smoke_local.py. The full pre-push gate (workspace clippy + test) passed on the force-push. Watching CI (incl. windows-build-and-test) to green.