From f3c7155865dedfaf6b7ccf395ba68095d5e7346e Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Tue, 12 May 2026 12:09:46 -0500 Subject: [PATCH] fix(engine): surface cancellation reasons in waits --- crates/tui/src/core/engine.rs | 69 ++++++++++++++++++++++++- crates/tui/src/core/engine/approval.rs | 26 ++++++++-- crates/tui/src/core/engine/turn_loop.rs | 13 ++++- 3 files changed, 103 insertions(+), 5 deletions(-) diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index aadef244..9c596f42 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -210,6 +210,40 @@ impl Default for EngineConfig { } } +/// Reason the active turn was cancelled. The token from `tokio_util` +/// does not carry a cause, so the engine keeps a sibling latch for +/// approval and user-input waits that need to explain cancellation. +/// +/// `External`, `Preempted`, and `Internal` are reserved for the +/// remaining direct cancellation paths tracked in #1541. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[allow(dead_code)] +pub enum CancelReason { + /// User-initiated cancel (Esc, `/cancel`, click cancel on modal). + User, + /// External / runtime-API cancel (HTTP `DELETE /v1/threads/...`, + /// task manager stop, parent agent cancel). + External, + /// Cancel triggered when a new turn starts before the previous one + /// finished — e.g. plain Enter while busy after the queueing path + /// pre-empts the running turn. + Preempted, + /// Engine internals tore down the turn (drop, channel close, + /// shutdown). Rare — surfaced as an internal error. + Internal, +} + +impl CancelReason { + fn describe(self) -> &'static str { + match self { + Self::User => "user cancelled the request", + Self::External => "request cancelled by external caller", + Self::Preempted => "request was preempted by a new turn", + Self::Internal => "engine torn down before approval resolved", + } + } +} + /// Handle to communicate with the engine #[derive(Clone)] pub struct EngineHandle { @@ -219,6 +253,10 @@ pub struct EngineHandle { pub rx_event: Arc>>, /// Shared pointer to the cancellation token for the current request. cancel_token: Arc>, + /// Latched reason for the most recent cancellation. Read by the + /// approval / user-input handlers to enrich their error strings. + /// Cleared by the engine when a fresh turn starts. + cancel_reason: Arc>>, /// Send approval decisions to the engine tx_approval: mpsc::Sender, /// Send user input responses to the engine @@ -234,8 +272,20 @@ impl EngineHandle { Ok(()) } - /// Cancel the current request + /// Cancel the current request (user-initiated path — keeps the + /// public `cancel()` signature stable). Equivalent to + /// `cancel_with_reason(CancelReason::User)`. pub fn cancel(&self) { + self.cancel_with_reason(CancelReason::User); + } + + /// Cancel the current request and latch the reason so downstream + /// "request cancelled" error messages can name a cause. + pub fn cancel_with_reason(&self, reason: CancelReason) { + match self.cancel_reason.lock() { + Ok(mut slot) => *slot = Some(reason), + Err(poisoned) => *poisoned.into_inner() = Some(reason), + } match self.cancel_token.lock() { Ok(token) => token.cancel(), Err(poisoned) => poisoned.into_inner().cancel(), @@ -340,6 +390,11 @@ pub struct Engine { pub(super) rx_subagent_completion: mpsc::UnboundedReceiver, cancel_token: CancellationToken, shared_cancel_token: Arc>, + /// Latched reason for the current cancellation, mirrored to + /// `EngineHandle::cancel_reason`. Read by `approval.rs` when + /// surfacing the "Request cancelled while awaiting …" error so the + /// user-facing message names a cause. + pub(super) cancel_reason: Arc>>, tool_exec_lock: Arc>, capacity_controller: CapacityController, /// Append-only layered context manager (#159). Opt-in for v0.7.5 while @@ -379,6 +434,13 @@ impl Engine { *poisoned.into_inner() = token; } } + // Fresh turn → clear any latched cancellation reason from the + // previous turn so a downstream "request cancelled" message + // doesn't inherit a stale cause. + match self.cancel_reason.lock() { + Ok(mut slot) => *slot = None, + Err(poisoned) => *poisoned.into_inner() = None, + } } fn env_only_api_key_recovery_hint(api_config: &Config) -> Option { @@ -431,6 +493,7 @@ impl Engine { let (tx_subagent_completion, rx_subagent_completion) = mpsc::unbounded_channel(); let cancel_token = CancellationToken::new(); let shared_cancel_token = Arc::new(StdMutex::new(cancel_token.clone())); + let cancel_reason: Arc>> = Arc::new(StdMutex::new(None)); let tool_exec_lock = Arc::new(RwLock::new(())); // Create clients for both providers @@ -565,6 +628,7 @@ impl Engine { rx_subagent_completion, cancel_token: cancel_token.clone(), shared_cancel_token: shared_cancel_token.clone(), + cancel_reason: cancel_reason.clone(), tool_exec_lock, capacity_controller, seam_manager, @@ -581,6 +645,7 @@ impl Engine { tx_op, rx_event: Arc::new(RwLock::new(rx_event)), cancel_token: shared_cancel_token, + cancel_reason, tx_approval, tx_user_input, tx_steer, @@ -2022,10 +2087,12 @@ pub(crate) fn mock_engine_handle() -> MockEngineHandle { let (tx_steer, rx_steer) = mpsc::channel(64); let cancel_token = CancellationToken::new(); let shared_cancel_token = Arc::new(StdMutex::new(cancel_token.clone())); + let cancel_reason: Arc>> = Arc::new(StdMutex::new(None)); let handle = EngineHandle { tx_op, rx_event: Arc::new(RwLock::new(rx_event)), cancel_token: shared_cancel_token, + cancel_reason, tx_approval, tx_user_input, tx_steer, diff --git a/crates/tui/src/core/engine/approval.rs b/crates/tui/src/core/engine/approval.rs index b307d469..ac04900b 100644 --- a/crates/tui/src/core/engine/approval.rs +++ b/crates/tui/src/core/engine/approval.rs @@ -49,6 +49,21 @@ pub(super) enum ApprovalResult { } impl Engine { + /// Format a cancellation suffix when the engine knows the cause. + /// Some internal cancellation paths still use the raw token while + /// #1541 is open; those keep the legacy message without a guessed + /// reason. + fn cancel_reason_suffix(&self) -> String { + let reason = match self.cancel_reason.lock() { + Ok(slot) => *slot, + Err(poisoned) => *poisoned.into_inner(), + }; + match reason { + Some(reason) => format!(" (reason: {})", reason.describe()), + None => String::new(), + } + } + pub(super) async fn await_tool_approval( &mut self, tool_id: &str, @@ -56,14 +71,18 @@ impl Engine { loop { tokio::select! { _ = self.cancel_token.cancelled() => { + let suffix = self.cancel_reason_suffix(); return Err(ToolError::execution_failed( - "Request cancelled while awaiting approval".to_string(), + format!("Request cancelled while awaiting approval{suffix}"), )); } decision = self.rx_approval.recv() => { let Some(decision) = decision else { return Err(ToolError::execution_failed( - "Approval channel closed".to_string(), + "Approval channel closed — engine is shutting down. \ + The approval modal can no longer reach the engine; \ + this is typically a teardown race, not a user action." + .to_string(), )); }; match decision { @@ -99,8 +118,9 @@ impl Engine { loop { tokio::select! { _ = self.cancel_token.cancelled() => { + let suffix = self.cancel_reason_suffix(); return Err(ToolError::execution_failed( - "Request cancelled while awaiting user input".to_string(), + format!("Request cancelled while awaiting user input{suffix}"), )); } decision = self.rx_user_input.recv() => { diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 63e63b45..3823ca8a 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -845,7 +845,7 @@ impl Engine { let mgr = self.subagent_manager.read().await; mgr.running_count() }; - if running > 0 { + if should_hold_turn_for_subagents(completions.len(), running) { let _ = self .tx_event .send(Event::status(format!( @@ -1874,6 +1874,10 @@ XML unless the user explicitly asks to debug sub-agent internals.\n\n\ } } +fn should_hold_turn_for_subagents(queued_completions: usize, running_children: usize) -> bool { + queued_completions > 0 || running_children > 0 +} + /// Resolve an `"auto"` reasoning-effort tier to a concrete value. /// /// When the configured effort is `"auto"`, inspects the last user message @@ -1949,6 +1953,13 @@ mod tests { assert!(text.contains("Build passed")); } + #[test] + fn turn_holds_open_for_running_or_completed_subagents() { + assert!(should_hold_turn_for_subagents(1, 0)); + assert!(should_hold_turn_for_subagents(0, 1)); + assert!(!should_hold_turn_for_subagents(0, 0)); + } + #[test] fn resolve_auto_effort_ignores_stored_turn_metadata() { let messages = vec![Message {