From 9d91a510645f115b3aa391e9107dc795142c9e83 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Tue, 5 May 2026 02:33:09 -0500 Subject: [PATCH] fix(scroll): RAII pause/resume guard so cancelling an interactive tool restores the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @Hmbown as the recurring "scrolling uses the parent terminal scrollbar instead of the TUI's" / "TUI rendered only at the bottom of the viewport" symptom. Same family as 5c72e5f4 and 899c703d, but neither covered the cancellation case — they fixed the per-turn scroll lock and panic-time cleanup respectively. The bug ======= `execute_tool_with_lock` in tool_execution.rs sent `Event::PauseEvents` before an interactive tool ran (which makes the TUI leave alt-screen, disable raw mode, release mouse capture so the child sees a normal terminal) and `Event::ResumeEvents` after it returned. Both sends were `let _ = tx_event.send(...).await`. If the future was dropped between the two awaits — Ctrl+C, agent cancel, parent task aborted, sub-agent terminated mid-tool — the second `await` never reached and `ResumeEvents` was never sent. The TUI sat in the paused state until the next pause/resume cycle, with the symptoms above. The fix ======= Replace the two `send` calls with a `InteractiveTerminalGuard` RAII type. `engage()` sends `PauseEvents` (only when `interactive` is true) and arms the guard. `Drop` synchronously sends `ResumeEvents` via `try_send` (Drop can't await). The guard fires on every exit path: Ok return, Err return, panic, future cancellation. `try_send` can fail on a full bounded channel; the guard logs a `tracing::warn!` rather than swallowing silently so the rare cancel-with-saturated-event-channel case is visible in traces. The TUI's own teardown path remains a final backstop. Verified locally ================ * `cargo build` clean * `cargo fmt --all -- --check` clean * `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` clean * `cargo test --workspace --all-features --locked` — 2062 passed, 0 failed Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/tui/src/core/engine/tool_execution.rs | 78 +++++++++++++++++--- 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/crates/tui/src/core/engine/tool_execution.rs b/crates/tui/src/core/engine/tool_execution.rs index 1b83cadc..9497f3cc 100644 --- a/crates/tui/src/core/engine/tool_execution.rs +++ b/crates/tui/src/core/engine/tool_execution.rs @@ -8,6 +8,67 @@ use std::{fs::OpenOptions, io::Write}; use super::*; +/// RAII guard that pauses the TUI's terminal-state ownership for the duration +/// of an interactive tool, then restores it on drop. +/// +/// Background: interactive tools (anything that needs the raw TTY — external +/// editor, `exec_shell` with stdin, etc.) need the TUI to leave alt-screen, +/// disable raw mode, and release mouse capture so the child sees a normal +/// terminal. The TUI listens for `Event::PauseEvents` / `Event::ResumeEvents` +/// and runs `pause_terminal` / `resume_terminal` in response. +/// +/// Earlier code sent `PauseEvents` before tool execution and `ResumeEvents` +/// after. That worked on the happy path, but if the tool's future was dropped +/// — Ctrl+C cancellation, sub-agent abort, parent task cancelled while the +/// tool was awaiting — the second `await` never reached and `ResumeEvents` +/// was never sent. The terminal stayed paused: parent shell scrollbar took +/// over, mouse wheel scrolled the host terminal instead of the transcript, +/// and the TUI rendered as if into a regular cooked-mode buffer. +/// +/// `Drop` runs synchronously and can't await, so we use `try_send` on a +/// **clone of the event channel** to push `ResumeEvents` non-blockingly. The +/// engine event channel is the same one we sent `PauseEvents` on, so by the +/// time we drop there is by construction at least one consumed slot, which +/// keeps `try_send` reliable in practice. +pub(super) struct InteractiveTerminalGuard { + tx: Option>, +} + +impl InteractiveTerminalGuard { + /// Send `PauseEvents` and arm the guard. If `interactive` is false the + /// guard is a no-op — `Drop` will skip the resume. + pub(super) async fn engage(tx: mpsc::Sender, interactive: bool) -> Self { + if !interactive { + return Self { tx: None }; + } + // Best-effort: if the receiver is gone the TUI has already shut down + // and there's nothing to restore. Either way we still arm the guard + // so `Drop` symmetrically tries the resume. + let _ = tx.send(Event::PauseEvents).await; + Self { tx: Some(tx) } + } +} + +impl Drop for InteractiveTerminalGuard { + fn drop(&mut self) { + if let Some(tx) = self.tx.take() { + // Synchronous, non-blocking. If the channel is full we still want + // the resume to land — log so a cancellation that loses the + // resume is visible in traces, but don't panic. The TUI also + // re-sends a resume on its own teardown path as a backstop. + if let Err(err) = tx.try_send(Event::ResumeEvents) { + tracing::warn!( + target: "engine.tool_execution", + ?err, + "InteractiveTerminalGuard: try_send(ResumeEvents) failed; \ + terminal may stay in paused state until the next \ + pause/resume cycle" + ); + } + } + } +} + pub(super) fn emit_tool_audit(event: serde_json::Value) { let Some(path) = std::env::var_os("DEEPSEEK_TOOL_AUDIT_LOG") else { return; @@ -166,11 +227,14 @@ impl Engine { ToolExecGuard::Write(lock.write().await) }; - if interactive { - let _ = tx_event.send(Event::PauseEvents).await; - } + // RAII pause/resume: ensures `Event::ResumeEvents` always fires on + // drop, even if the tool future is cancelled mid-await. See + // `InteractiveTerminalGuard` doc-comment for the regression this + // closes (parent terminal scrollback hijacking the TUI after a + // cancelled interactive tool). + let _terminal = InteractiveTerminalGuard::engage(tx_event, interactive).await; - let result = if McpPool::is_mcp_tool(&tool_name) { + if McpPool::is_mcp_tool(&tool_name) { if let Some(pool) = mcp_pool { Engine::execute_mcp_tool_with_pool(pool, &tool_name, tool_input).await } else { @@ -186,13 +250,7 @@ impl Engine { Err(ToolError::not_available(format!( "tool '{tool_name}' is not registered" ))) - }; - - if interactive { - let _ = tx_event.send(Event::ResumeEvents).await; } - - result } }