fix(scroll): RAII pause/resume guard so cancelling an interactive tool restores the terminal
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 as5c72e5f4and899c703d, 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<mpsc::Sender<Event>>,
|
||||
}
|
||||
|
||||
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<Event>, 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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user