diff --git a/crates/jp_cli/src/cmd/lock.rs b/crates/jp_cli/src/cmd/lock.rs index 1127305fc..87e6f72b1 100644 --- a/crates/jp_cli/src/cmd/lock.rs +++ b/crates/jp_cli/src/cmd/lock.rs @@ -124,9 +124,14 @@ pub(crate) async fn acquire_lock(mut r: LockRequest<'_>) -> Result // Wait for the next poll tick, but also listen for interrupts. tokio::select! { biased; - Some(()) = interrupt_rx.recv() => { + Some(notice) = interrupt_rx.recv() => { cancel_timer(timer).await; drop(interrupt_guard); + // The press is answered by the contention prompt below, so it + // no longer counts toward the escalation ladder: a press after + // the prompt is answered should reach a handler, not bypass + // the stack. + notice.handled(); return prompt_contention(r).await; } () = tokio::time::sleep(Duration::from_millis(500)) => {} diff --git a/crates/jp_cli/src/cmd/plugin/dispatch.rs b/crates/jp_cli/src/cmd/plugin/dispatch.rs index 8802b88a3..9b49b4db8 100644 --- a/crates/jp_cli/src/cmd/plugin/dispatch.rs +++ b/crates/jp_cli/src/cmd/plugin/dispatch.rs @@ -143,7 +143,13 @@ pub(crate) fn run_plugin( let shutdown_handle = thread::spawn(move || { let interrupted = futures::executor::block_on(async { tokio::select! { - notified = interrupt_rx.recv() => notified.is_some(), + // Asking the plugin to shut down acts on the press, so it + // leaves the escalation ladder; a further press then reaches + // the router with a fresh count. + notified = interrupt_rx.recv() => notified.is_some_and(|notice| { + notice.handled(); + true + }), () = shutdown_token.cancelled() => true, } }); diff --git a/crates/jp_cli/src/cmd/query/interrupt/handler.rs b/crates/jp_cli/src/cmd/query/interrupt/handler.rs index 6d249f3ec..19cbec1ea 100644 --- a/crates/jp_cli/src/cmd/query/interrupt/handler.rs +++ b/crates/jp_cli/src/cmd/query/interrupt/handler.rs @@ -28,6 +28,7 @@ use std::sync::Arc; +use inquire::InquireError; use jp_config::{ editor::InlineEditMode, interrupt::{ @@ -41,6 +42,7 @@ use jp_inquire::{ prompt::{PromptBackend, TerminalPromptBackend}, }; use jp_printer::Printer; +use tracing::{debug, warn}; use crate::editor::report_editor_failure; @@ -52,6 +54,36 @@ pub(crate) fn reply_edit_mode(mode: InlineEditMode) -> ReplyEditMode { } } +/// Why an interrupt prompt returned without a selection. +enum PromptExit { + /// The user pressed `Ctrl+C` on the prompt, asking to get past it. + Cancelled, + + /// The prompt could not be rendered or read at all. + Unavailable, +} + +/// Classify and record a prompt that returned without a selection. +/// +/// The distinction decides whether the press escalates. +/// A cancelled prompt is the user asking out, so it does. +/// A prompt that could not run answered nothing, so escalating on it would end +/// the run on a decision the user never made; the press stays on the router's +/// escalation ladder instead, and a second one gets them out through the ladder +/// rather than through this prompt. +fn classify_prompt_exit(prompt: &str, error: &InquireError) -> PromptExit { + match error { + InquireError::OperationCanceled | InquireError::OperationInterrupted => { + debug!(prompt, %error, "Interrupt prompt cancelled."); + PromptExit::Cancelled + } + _ => { + warn!(prompt, %error, "Interrupt prompt unavailable."); + PromptExit::Unavailable + } + } +} + /// Actions that can be taken after an interrupt. #[derive(Debug, Clone, PartialEq, Eq)] pub enum InterruptAction { @@ -111,6 +143,13 @@ pub enum InterruptAction { /// The streaming path commits partial content before completing; the tool /// path cancels the running tools. Escalate, + + /// The menu could not be shown or read, so nothing was decided. + /// + /// The caller leaves the work as it was and leaves the press on the signal + /// router's escalation ladder, so a second press gets the user out through + /// the router instead of relying on this menu. + PromptFailed, } /// Outcome of collecting a reply from the user. @@ -183,6 +222,9 @@ impl InterruptHandler

{ /// returns to the menu, while a configured (menu-less) `reply` resumes. /// Cancelling the menu itself with `Ctrl+C` escalates: the caller should /// commit partial content and begin a graceful shutdown. + /// A menu that cannot be shown at all yields + /// [`InterruptAction::PromptFailed`] rather than an escalation, leaving the + /// press on the router's escalation ladder. pub fn handle_streaming_interrupt( &self, config: &StreamingInterruptConfig, @@ -190,6 +232,12 @@ impl InterruptHandler

{ stream_alive: bool, ) -> InterruptAction { let menu = config.action == StreamingInterruptAction::Prompt; + debug!( + action = ?config.action, + menu, + stream_alive, + "Handling streaming interrupt." + ); loop { let choice = match config.action { @@ -212,7 +260,12 @@ impl InterruptHandler

{ // escalation, not a "continue". match selected { Ok(choice) => choice, - Err(_) => return InterruptAction::Escalate, + Err(error) => { + return match classify_prompt_exit("streaming_menu", &error) { + PromptExit::Cancelled => InterruptAction::Escalate, + PromptExit::Unavailable => InterruptAction::PromptFailed, + }; + } } } StreamingInterruptAction::Continue => 'c', @@ -271,6 +324,7 @@ impl InterruptHandler

{ printer: &Printer, ) -> InterruptAction { let menu = config.action == ToolInterruptAction::Prompt; + debug!(action = ?config.action, menu, "Handling tool interrupt."); loop { let choice = match config.action { @@ -293,7 +347,12 @@ impl InterruptHandler

{ // escalation, not a "continue". match selected { Ok(choice) => choice, - Err(_) => return InterruptAction::Escalate, + Err(error) => { + return match classify_prompt_exit("tool_menu", &error) { + PromptExit::Cancelled => InterruptAction::Escalate, + PromptExit::Unavailable => InterruptAction::PromptFailed, + }; + } } } ToolInterruptAction::Continue => 'c', @@ -456,7 +515,13 @@ impl InterruptHandler

{ // Whitespace counts as blank so the tool path reaches its canned // rejection rather than sending a blank-looking reply. Ok(ReplyOutcome::Submit(_)) => return ReplyResult::Empty, - Ok(ReplyOutcome::Cancelled) | Err(_) => return ReplyResult::Cancelled, + Ok(ReplyOutcome::Cancelled) => return ReplyResult::Cancelled, + Err(error) => { + // Both classifications back out to the caller's menu; only + // the log line differs. + let _unused = classify_prompt_exit("inline_reply", &error); + return ReplyResult::Cancelled; + } } } } diff --git a/crates/jp_cli/src/cmd/query/interrupt/handler_tests.rs b/crates/jp_cli/src/cmd/query/interrupt/handler_tests.rs index 12d0eb075..f44243073 100644 --- a/crates/jp_cli/src/cmd/query/interrupt/handler_tests.rs +++ b/crates/jp_cli/src/cmd/query/interrupt/handler_tests.rs @@ -1,7 +1,11 @@ use std::sync::Arc; +use inquire::InquireError; use jp_editor::MockEditorBackend; -use jp_inquire::{ReplyEditMode, ReplyOutcome, prompt::MockPromptBackend}; +use jp_inquire::{ + InlineOption, ReplyEditMode, ReplyOutcome, + prompt::{MockPromptBackend, PromptBackend}, +}; use jp_printer::{OutputFormat, Printer}; use super::*; @@ -40,6 +44,91 @@ fn handler_with_editor( InterruptHandler::with_backend(backend, Some(Arc::new(editor)), ReplyEditMode::Emacs) } +/// A backend whose prompts cannot run at all, as when there is no usable +/// terminal to read from. +/// +/// Distinct from an empty [`MockPromptBackend`], which reports +/// `OperationCanceled` — the user pressing `Ctrl+C` on the prompt. +struct UnavailableBackend; + +impl PromptBackend for UnavailableBackend { + fn inline_select( + &self, + _message: &str, + _options: Vec, + _default: Option, + _writer: &mut dyn std::io::Write, + ) -> Result { + Err(InquireError::NotTTY) + } + + fn inline_reply( + &self, + _message: &str, + _initial_text: &str, + _edit_mode: ReplyEditMode, + _editor_escape: bool, + _output: Box, + ) -> Result { + Err(InquireError::NotTTY) + } + + fn text( + &self, + _message: &str, + _default: Option<&str>, + _writer: &mut dyn std::io::Write, + ) -> Result { + Err(InquireError::NotTTY) + } + + fn select( + &self, + _message: &str, + _options: Vec, + _default: Option, + _writer: &mut dyn std::io::Write, + ) -> Result { + Err(InquireError::NotTTY) + } +} + +/// A menu the user cancels with `Ctrl+C` is a request to get past it, so it +/// escalates. +#[test] +fn streaming_menu_cancelled_by_user_escalates() { + // An empty mock queue reports `OperationCanceled`. + let handler = handler(MockPromptBackend::new()); + let action = handler.handle_streaming_interrupt( + &streaming(StreamingInterruptAction::Prompt), + &make_printer(), + true, + ); + assert_eq!(action, InterruptAction::Escalate); +} + +/// A menu that cannot run answered nothing, so it must not escalate: doing so +/// would end the run on a decision the user never made. +/// The press stays on the router's escalation ladder, which is what gets the +/// user out on the next one. +#[test] +fn streaming_menu_that_cannot_run_does_not_escalate() { + let handler = InterruptHandler::with_backend(UnavailableBackend, None, ReplyEditMode::Emacs); + let action = handler.handle_streaming_interrupt( + &streaming(StreamingInterruptAction::Prompt), + &make_printer(), + true, + ); + assert_eq!(action, InterruptAction::PromptFailed); +} + +#[test] +fn tool_menu_that_cannot_run_does_not_escalate() { + let handler = InterruptHandler::with_backend(UnavailableBackend, None, ReplyEditMode::Emacs); + let action = handler.handle_tool_interrupt(&tool(ToolInterruptAction::Prompt), &make_printer()); + assert_eq!(action, InterruptAction::PromptFailed); +} + #[test] fn streaming_interrupt_stop() { let handler = handler(MockPromptBackend::new().with_inline_responses(['s'])); diff --git a/crates/jp_cli/src/cmd/query/interrupt/signals.rs b/crates/jp_cli/src/cmd/query/interrupt/signals.rs index f5920466c..32088f612 100644 --- a/crates/jp_cli/src/cmd/query/interrupt/signals.rs +++ b/crates/jp_cli/src/cmd/query/interrupt/signals.rs @@ -17,7 +17,7 @@ use jp_inquire::{ReplyEditMode, prompt::PromptBackend}; use jp_llm::event::{Event, FinishReason, apply_patches}; use jp_printer::Printer; use tokio_util::sync::CancellationToken; -use tracing::{info, trace}; +use tracing::{debug, info, trace}; use super::handler::{InterruptAction, InterruptHandler}; use crate::cmd::query::{ @@ -58,6 +58,11 @@ pub enum StreamingInterruptResult { /// and the turn is complete. /// The caller should begin a graceful shutdown. Escalate, + + /// The menu could not be shown, so nothing was decided. + /// The caller leaves the stream as it was and leaves the press on the + /// signal router's escalation ladder. + PromptFailed, } /// Handle a Ctrl-C interrupt notification received during LLM streaming. @@ -95,9 +100,19 @@ pub fn handle_streaming_interrupt( // polling instead. let is_resume = matches!(action, InterruptAction::Resume); let is_escalate = matches!(action, InterruptAction::Escalate); + debug!( + ?action, + llm_stream_finished, "Streaming interrupt resolved." + ); + + // A menu that never ran decided nothing, so the state machine is left + // untouched: no partial commit, no phase change. + if matches!(action, InterruptAction::PromptFailed) { + return StreamingInterruptResult::PromptFailed; + } // Delegate state transition to the turn coordinator - match turn_coordinator.handle_streaming_interrupt(action, conversation_stream) { + let result = match turn_coordinator.handle_streaming_interrupt(action, conversation_stream) { // Return without persisting this cycle (previous turn cycles // are already persisted). TurnPhase::Aborted => StreamingInterruptResult::Abort, @@ -112,7 +127,10 @@ pub fn handle_streaming_interrupt( // All other phases break from loop, persist, then outer loop // decides. _ => StreamingInterruptResult::Break, - } + }; + + debug!(?result, phase = ?turn_coordinator.current_phase(), "Streaming interrupt handled."); + result } /// Handle a successful event from the LLM stream. @@ -214,6 +232,11 @@ pub enum ToolInterruptResult { /// Cancel current execution and begin a graceful shutdown: the user /// cancelled the interrupt menu itself with Ctrl-C. Escalate, + + /// The menu could not be shown, so nothing was decided. + /// The caller keeps waiting for the running tools and leaves the press on + /// the signal router's escalation ladder. + PromptFailed, } /// Handle a Ctrl-C interrupt notification received during tool execution. @@ -249,11 +272,18 @@ pub fn handle_tool_interrupt( let action = InterruptHandler::with_backend(backend, editor, edit_mode) .handle_tool_interrupt(config, printer); + debug!(?action, "Tool interrupt resolved."); + + // A menu that never ran decided nothing: the running tools are left alone + // and the state machine is not notified. + if matches!(action, InterruptAction::PromptFailed) { + return ToolInterruptResult::PromptFailed; + } // Notify the state machine (reserved for future state transitions). turn_coordinator.handle_tool_interrupt(&action); - match action { + let result = match action { InterruptAction::RestartTool => { info!("Restarting tool execution"); cancellation_token.cancel(); @@ -269,7 +299,10 @@ pub fn handle_tool_interrupt( ToolInterruptResult::Escalate } _ => ToolInterruptResult::Continue, - } + }; + + debug!(?result, "Tool interrupt handled."); + result } #[cfg(test)] diff --git a/crates/jp_cli/src/cmd/query/stream/retry.rs b/crates/jp_cli/src/cmd/query/stream/retry.rs index fd44ad384..c7faca73c 100644 --- a/crates/jp_cli/src/cmd/query/stream/retry.rs +++ b/crates/jp_cli/src/cmd/query/stream/retry.rs @@ -33,7 +33,11 @@ use jp_printer::Printer; use jp_workspace::ConversationMut; use tracing::{error, warn}; -use crate::{cmd::query::turn::TurnCoordinator, error::Error, signals::SignalRouter}; +use crate::{ + cmd::query::turn::TurnCoordinator, + error::Error, + signals::{InterruptNotice, SignalRouter}, +}; /// How many provider-requested rebuilds a turn may attempt in a row. /// @@ -319,7 +323,10 @@ pub enum StreamErrorOutcome { /// A Ctrl-C arrived during the backoff wait. /// The wait was cut short and the retry notification line cleared; the /// caller should run the streaming interrupt flow (the stream is dead). - Interrupted, + /// + /// The press is carried out unresolved: only the caller, which runs the + /// menu, knows whether it was answered. + Interrupted(InterruptNotice), } /// Single source of truth for handling stream errors during LLM streaming. @@ -382,16 +389,16 @@ pub async fn handle_stream_error( // immediately instead of after the wait. let delay = retry_state.backoff_duration(&error); let (interrupt_guard, mut interrupt_rx) = signals.push_handler(); - let interrupted = tokio::select! { + let notice = tokio::select! { biased; - Some(()) = interrupt_rx.recv() => true, - () = tokio::time::sleep(delay) => false, + notice = interrupt_rx.recv() => notice, + () = tokio::time::sleep(delay) => None, }; drop(interrupt_guard); - if interrupted { + if let Some(notice) = notice { retry_state.clear_line(printer); - return StreamErrorOutcome::Interrupted; + return StreamErrorOutcome::Interrupted(notice); } StreamErrorOutcome::Retry diff --git a/crates/jp_cli/src/cmd/query/stream/retry_tests.rs b/crates/jp_cli/src/cmd/query/stream/retry_tests.rs index 28f015a21..20f42e669 100644 --- a/crates/jp_cli/src/cmd/query/stream/retry_tests.rs +++ b/crates/jp_cli/src/cmd/query/stream/retry_tests.rs @@ -437,7 +437,7 @@ async fn interrupt_during_backoff_cuts_wait_short() { signal_handle.await.unwrap(); - assert!(matches!(result, StreamErrorOutcome::Interrupted)); + assert!(matches!(result, StreamErrorOutcome::Interrupted(_))); // The attempt was recorded before the wait began. assert_eq!(retry_state.consecutive_failures, 1); } diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index ba3112613..8eb5df799 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -120,11 +120,14 @@ use crate::{ turn::{TurnCoordinator, state::TurnState}, }, render::tool::RenderOutcome, - signals::SignalRouter, + signals::{InterruptNotice, SignalRouter}, }; #[derive(Debug)] enum ExecutionEvent { + /// A Ctrl-C press delivered to this execution phase's interrupt handler. + Interrupt(InterruptNotice), + ToolResult { index: usize, result: ExecutorResult, @@ -155,9 +158,6 @@ enum ExecutionEvent { response: ToolCallResponse, }, - /// A Ctrl-C interrupt notification from the signal router. - Interrupt, - ProgressTick { elapsed: Duration, }, @@ -961,8 +961,12 @@ impl ToolCoordinator { // channel) or when the event channel closes. let interrupt_tx = event_tx.clone(); tokio::spawn(async move { - while interrupt_rx.recv().await.is_some() { - if interrupt_tx.send(ExecutionEvent::Interrupt).await.is_err() { + while let Some(notice) = interrupt_rx.recv().await { + if interrupt_tx + .send(ExecutionEvent::Interrupt(notice)) + .await + .is_err() + { break; } } @@ -1159,18 +1163,18 @@ impl ToolCoordinator { event_tx.clone(), ); } - ExecutionEvent::Interrupt => { + ExecutionEvent::Interrupt(notice) => { if prompt_active { // An active inline prompt owns the terminal; pass the // interrupt down the handler stack instead of stacking // the menu on top of the prompt. - signals.decline(); + notice.decline(); } else { if progress_shown { tool_renderer.clear_progress(); progress_shown = false; } - match handle_tool_interrupt( + let result = handle_tool_interrupt( &cancellation_token, turn_coordinator, self.is_prompting(), @@ -1179,11 +1183,33 @@ impl ToolCoordinator { editor.clone(), edit_mode, &self.interrupt_config, - ) { - ToolInterruptResult::Continue => {} - // A tool prompt is pending; let the next handler - // down the stack take the interrupt. - ToolInterruptResult::Declined => signals.decline(), + ); + + match result { + // Answered by the menu: clear the ladder so the next + // press opens it again instead of bypassing it. + ToolInterruptResult::Continue + | ToolInterruptResult::Restart + | ToolInterruptResult::Cancelled { .. } => notice.handled(), + + // A pending tool prompt owns this press; hand it to + // the next handler down with the ladder intact. + ToolInterruptResult::Declined => notice.decline(), + + // An escalation is the user asking to get past the + // menu, and a menu that could not run answered + // nothing. Both leave the press on the ladder so it + // still gets the user out. + ToolInterruptResult::Escalate | ToolInterruptResult::PromptFailed => {} + } + + match result { + // Either the user chose to keep waiting, or the menu + // could not be shown and nothing happened. A + // declined press was already handed down the stack. + ToolInterruptResult::Continue + | ToolInterruptResult::PromptFailed + | ToolInterruptResult::Declined => {} ToolInterruptResult::Restart => { outcome.upgrade(ExecutionOutcome::Restart); } diff --git a/crates/jp_cli/src/cmd/query/turn/coordinator.rs b/crates/jp_cli/src/cmd/query/turn/coordinator.rs index 58b8a2f31..d30011650 100644 --- a/crates/jp_cli/src/cmd/query/turn/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/turn/coordinator.rs @@ -572,9 +572,13 @@ impl TurnCoordinator { // Resume and tool-related actions don't change state during // streaming. + // `PromptFailed` decided nothing at all, so it must not commit + // partial content or move the phase either; callers filter it out + // before reaching here. InterruptAction::Resume | InterruptAction::ToolCancelled { .. } - | InterruptAction::RestartTool => {} + | InterruptAction::RestartTool + | InterruptAction::PromptFailed => {} } self.state diff --git a/crates/jp_cli/src/cmd/query/turn_loop.rs b/crates/jp_cli/src/cmd/query/turn_loop.rs index 3d3183b9b..f04ff82c6 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop.rs @@ -69,14 +69,15 @@ use crate::{ editor::build_editor_backend, error::Error, render::metadata::set_rendered_arguments, - signals::SignalRouter, + signals::{InterruptNotice, SignalRouter}, timer::LineTimer, }; /// Events produced by the merged streaming loop sources. enum StreamingLoopEvent { - /// A Ctrl-C interrupt notification from the signal router. - Interrupt, + /// A Ctrl-C press delivered by the signal router, carried as the notice the + /// loop resolves once it has decided what the press did. + Interrupt(InterruptNotice), /// An event from the LLM provider stream. Llm(Box>), /// A tick from the preparing indicator timer, carrying the elapsed time @@ -155,7 +156,7 @@ fn event_keeps_waiting_indicator(event: &StreamingLoopEvent) -> bool { result.as_ref(), Ok(Event::KeepAlive | Event::Patch(_) | Event::Flush { .. }) ), - StreamingLoopEvent::Interrupt | StreamingLoopEvent::PreparingTick(_) => false, + StreamingLoopEvent::Interrupt(_) | StreamingLoopEvent::PreparingTick(_) => false, } } @@ -268,10 +269,11 @@ pub(super) async fn run_turn_loop( loop { // A Ctrl-C that landed between phases ends the turn gracefully: // commit any partial assistant content and complete. - if turn_interrupt_rx.try_recv().is_ok() { + if let Ok(notice) = turn_interrupt_rx.try_recv() { info!("Interrupt received between turn phases; completing the turn."); lock.as_mut() .update_events(|stream| turn_coordinator.complete_early(stream)); + notice.handled(); } match turn_coordinator.current_phase() { @@ -333,7 +335,7 @@ pub(super) async fn run_turn_loop( // guard deregisters the handler when the cycle ends. let (interrupt_guard, interrupt_rx) = signals.push_handler(); let interrupt_stream = StreamSource::Interrupt( - ReceiverStream::new(interrupt_rx).map(|()| StreamingLoopEvent::Interrupt), + ReceiverStream::new(interrupt_rx).map(StreamingLoopEvent::Interrupt), ); let raw_stream = provider @@ -411,7 +413,7 @@ pub(super) async fn run_turn_loop( } match event { - StreamingLoopEvent::Interrupt => { + StreamingLoopEvent::Interrupt(notice) => { // Clear the preparing display before showing the // interrupt menu to avoid visual conflicts. tool_renderer.clear_temp_line(); @@ -431,8 +433,25 @@ pub(super) async fn run_turn_loop( !llm_alive, ) }); + + // The menu answered the press, so it no longer + // counts toward the router's escalation ladder: the + // next one opens this menu again instead of + // bypassing it. An escalation is the user asking to + // get past the menu, and a menu that could not run + // answered nothing; both leave the press in place so + // the ladder still gets the user out. + match action { + StreamingInterruptResult::Escalate + | StreamingInterruptResult::PromptFailed => {} + _ => notice.handled(), + } + match action { - StreamingInterruptResult::Continue => {} + // Either the user chose to keep waiting, or the + // menu could not be shown and nothing happened. + StreamingInterruptResult::Continue + | StreamingInterruptResult::PromptFailed => {} StreamingInterruptResult::Break => break, StreamingInterruptResult::Abort => return Ok(()), // The menu itself was cancelled with Ctrl-C: @@ -480,7 +499,7 @@ pub(super) async fn run_turn_loop( // A Ctrl-C cut the backoff wait short: // run the streaming interrupt flow with // the stream known dead. - StreamErrorOutcome::Interrupted => { + StreamErrorOutcome::Interrupted(notice) => { let action = conv.update_events(|stream| { handle_streaming_interrupt( &mut turn_coordinator, @@ -493,6 +512,13 @@ pub(super) async fn run_turn_loop( true, ) }); + + match action { + StreamingInterruptResult::Escalate + | StreamingInterruptResult::PromptFailed => {} + _ => notice.handled(), + } + match action { // With a dead stream, "continue" // commits partial output as @@ -500,7 +526,11 @@ pub(super) async fn run_turn_loop( // for a fresh request; a // keep-polling Continue cannot // occur here. + // A menu that could not run also + // breaks: the stream is dead, so + // there is nothing to resume. StreamingInterruptResult::Continue + | StreamingInterruptResult::PromptFailed | StreamingInterruptResult::Break => break, StreamingInterruptResult::Abort => { return Ok(()); diff --git a/crates/jp_cli/src/signals.rs b/crates/jp_cli/src/signals.rs index e8a7590d0..c27e1ccc5 100644 --- a/crates/jp_cli/src/signals.rs +++ b/crates/jp_cli/src/signals.rs @@ -7,9 +7,13 @@ //! //! Ctrl-C escalates: the first press notifies the topmost registered handler //! (or requests a graceful shutdown when nothing handles interrupts), a second -//! press within the cooldown window bypasses all handlers and cancels the -//! shutdown token, and any press after shutdown has begun exits the process -//! immediately. +//! unanswered press within the cooldown window bypasses all handlers and +//! cancels the shutdown token, and any press after shutdown has begun exits the +//! process immediately. +//! Only presses that produced nothing escalate: resolving a delivered press +//! with [`InterruptNotice::handled`] clears the count, so answering an +//! interrupt menu and pressing again a moment later reopens the menu instead of +//! quitting. //! SIGTERM requests a graceful shutdown; SIGQUIT exits. //! Neither goes through the handler stack. //! @@ -19,6 +23,13 @@ //! The interrupt logic runs in the registering event loop's own context, never //! on the router's signal task, so handlers can block on interactive prompts //! and act on the result immediately. +//! Each delivered press arrives as an [`InterruptNotice`] the loop resolves +//! once its logic finishes: [`InterruptNotice::handled`] when it acted on the +//! press, [`InterruptNotice::decline`] when the press belongs to a handler +//! further down the stack. +//! Dropping a notice unresolved leaves the press on the ladder, so a handler +//! that was notified but produced nothing visible still escalates on the next +//! press. //! //! Code without an interrupt handler cooperates through the shutdown token //! ([`SignalRouter::shutdown_token`]) instead: awaiting its cancellation (or @@ -26,6 +37,7 @@ //! graceful shutdown request. use std::{ + fmt, sync::{ Arc, Mutex, atomic::{AtomicU64, Ordering}, @@ -96,6 +108,8 @@ impl SignalRouter { /// `escalation_cooldown` is how long the Ctrl-C escalation counter survives /// without a new press; a press arriving after the window counts as a fresh /// first press. + /// The counter also clears as soon as a handler answers a press (see + /// [`InterruptNotice::handled`]). pub fn new(runtime: &Runtime, escalation_cooldown: Duration) -> Self { #[cfg(unix)] let signals = os_signals(runtime); @@ -161,22 +175,53 @@ impl SignalRouter { /// Register an interrupt handler scope. /// - /// Returns a guard (drop to deregister) and a receiver that fires when - /// SIGINT arrives while this handler is topmost. + /// Returns a guard (drop to deregister) and a receiver that yields an + /// [`InterruptNotice`] when SIGINT arrives while this handler is topmost. /// The registering event loop polls the receiver alongside its other - /// sources and runs its interrupt logic in its own context when the - /// receiver fires. + /// sources, runs its interrupt logic in its own context when the receiver + /// fires, and resolves the notice with the outcome. #[must_use] - pub fn push_handler(&self) -> (InterruptGuard, mpsc::Receiver<()>) { + pub fn push_handler(&self) -> (InterruptGuard, mpsc::Receiver) { self.inner.push_handler() } +} + +/// A delivered Ctrl-C press, handed to the notified handler's event loop. +/// +/// The loop resolves it once its interrupt logic finishes: [`Self::handled`] +/// when it acted on the press, [`Self::decline`] to pass the press to the next +/// handler down the stack. +/// Dropping it unresolved leaves the press on the escalation ladder, which is +/// the safe default: a handler that was notified but produced nothing visible +/// should still escalate on the next press. +#[must_use = "resolving an interrupt notice decides whether the press stays on the escalation \ + ladder"] +pub struct InterruptNotice { + inner: Arc, +} + +impl fmt::Debug for InterruptNotice { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("InterruptNotice") + } +} + +impl InterruptNotice { + /// The handler acted on this press. + /// + /// Clears the escalation count, so the next press starts a fresh ladder + /// instead of bypassing the handler stack. + pub fn handled(self) { + self.inner.reset_escalation(); + } - /// Called by a handler's event loop when it declines to handle the current - /// interrupt. + /// The handler declined this press. /// - /// The router notifies the next handler on the stack, or falls back to - /// graceful shutdown when no other handler exists. - pub fn decline(&self) { + /// Notifies the next handler down the stack, or requests a graceful + /// shutdown when this was the last one. + /// The press keeps its place on the escalation ladder: nothing has acted on + /// it yet. + pub fn decline(self) { self.inner.notify_next_or_shutdown(); } } @@ -203,7 +248,7 @@ struct RegisteredHandler { /// Notifies the handler's event loop that SIGINT arrived. /// The event loop runs the interrupt logic; the router never does. - notify_tx: mpsc::Sender<()>, + notify_tx: mpsc::Sender, } /// Ctrl-C press tracking for escalation. @@ -240,6 +285,15 @@ impl EscalationState { self.last_press = Some(now); self.presses } + + /// Forget the recorded presses. + /// + /// The next press starts a fresh ladder however recently the previous one + /// arrived. + fn reset(&mut self) { + self.presses = 0; + self.last_press = None; + } } struct RouterInner { @@ -268,11 +322,11 @@ impl RouterInner { }) } - fn route(&self, signal: OsSignal) -> Routed { + fn route(self: &Arc, signal: OsSignal) -> Routed { self.route_at(signal, Instant::now()) } - fn route_at(&self, signal: OsSignal, now: Instant) -> Routed { + fn route_at(self: &Arc, signal: OsSignal, now: Instant) -> Routed { match signal { OsSignal::Interrupt => self.route_interrupt(now), @@ -295,7 +349,11 @@ impl RouterInner { /// Second press within the cooldown: bypass all handlers and request a /// graceful shutdown. /// Any press once shutdown has begun: exit the process. - fn route_interrupt(&self, now: Instant) -> Routed { + /// + /// The count tracks only presses that went unanswered: + /// [`Self::reset_escalation`] clears it when a handler reports that it + /// acted on one. + fn route_interrupt(self: &Arc, now: Instant) -> Routed { let presses = self .escalation .lock() @@ -313,15 +371,18 @@ impl RouterInner { } if let Some(notify_tx) = self.topmost() { - return match notify_tx.try_send(()) { + return match notify_tx.try_send(self.notice()) { // A full channel means the handler already has a pending - // interrupt notification; nothing to add. - Ok(()) | Err(TrySendError::Full(())) => Routed::Handler, + // interrupt notification; nothing to add. The undelivered + // notice is dropped unresolved, leaving this press on the + // ladder — which is correct, since the handler hasn't even + // picked up the previous one. + Ok(()) | Err(TrySendError::Full(_)) => Routed::Handler, // The handler's event loop is gone but its guard hasn't // dropped yet. Treat it as declined and fall back to // graceful shutdown. - Err(TrySendError::Closed(())) => { + Err(TrySendError::Closed(_)) => { self.shutdown_token.cancel(); Routed::Shutdown } @@ -332,8 +393,23 @@ impl RouterInner { Routed::Shutdown } + /// Clear the escalation count after a handler answered a press. + fn reset_escalation(&self) { + self.escalation + .lock() + .expect("escalation state lock poisoned") + .reset(); + } + + /// Build a notice for a press about to be delivered to a handler. + fn notice(self: &Arc) -> InterruptNotice { + InterruptNotice { + inner: Arc::clone(self), + } + } + /// Clone the topmost handler's notification channel. - fn topmost(&self) -> Option> { + fn topmost(&self) -> Option> { self.stack .lock() .expect("handler stack lock poisoned") @@ -343,7 +419,7 @@ impl RouterInner { /// Register a handler scope: push a fresh notification channel onto the /// stack and return the deregistration guard plus the receiver. - fn push_handler(self: &Arc) -> (InterruptGuard, mpsc::Receiver<()>) { + fn push_handler(self: &Arc) -> (InterruptGuard, mpsc::Receiver) { let (notify_tx, notify_rx) = mpsc::channel(1); let id = HandlerId(self.next_handler_id.fetch_add(1, Ordering::Relaxed)); self.stack @@ -373,7 +449,7 @@ impl RouterInner { /// Notify the handler below the topmost one, or request a graceful shutdown /// when no other handler exists. - fn notify_next_or_shutdown(&self) { + fn notify_next_or_shutdown(self: &Arc) { let next = { let stack = self.stack.lock().expect("handler stack lock poisoned"); stack @@ -388,9 +464,9 @@ impl RouterInner { return; }; - match notify_tx.try_send(()) { - Ok(()) | Err(TrySendError::Full(())) => {} - Err(TrySendError::Closed(())) => self.shutdown_token.cancel(), + match notify_tx.try_send(self.notice()) { + Ok(()) | Err(TrySendError::Full(_)) => {} + Err(TrySendError::Closed(_)) => self.shutdown_token.cancel(), } } } diff --git a/crates/jp_cli/src/signals_tests.rs b/crates/jp_cli/src/signals_tests.rs index 37ab94801..aba09670b 100644 --- a/crates/jp_cli/src/signals_tests.rs +++ b/crates/jp_cli/src/signals_tests.rs @@ -5,10 +5,23 @@ use tokio::sync::mpsc::{self, error::TryRecvError}; use super::*; /// Push a handler scope onto the router state. -fn push_handler(inner: &Arc) -> (InterruptGuard, mpsc::Receiver<()>) { +fn push_handler(inner: &Arc) -> (InterruptGuard, mpsc::Receiver) { inner.push_handler() } +/// Take a delivered press off the channel and drop it unresolved. +/// +/// Dropping models a handler that was notified but neither acted on the press +/// nor declined it, which is what keeps it on the escalation ladder. +fn took_notice(rx: &mut mpsc::Receiver) -> bool { + rx.try_recv().is_ok() +} + +/// Why no press was waiting on the channel. +fn recv_error(rx: &mut mpsc::Receiver) -> Option { + rx.try_recv().err() +} + #[test] fn interrupt_without_handlers_requests_shutdown() { let inner = RouterInner::new(Duration::from_secs(2)); @@ -26,8 +39,8 @@ fn interrupt_notifies_topmost_handler_only() { let now = Instant::now(); assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); - assert_eq!(rx_top.try_recv(), Ok(())); - assert_eq!(rx_bottom.try_recv(), Err(TryRecvError::Empty)); + assert!(took_notice(&mut rx_top)); + assert_eq!(recv_error(&mut rx_bottom), Some(TryRecvError::Empty)); assert!(!inner.shutdown_token.is_cancelled()); } @@ -38,7 +51,8 @@ fn second_interrupt_within_cooldown_bypasses_handlers() { let now = Instant::now(); assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); - assert_eq!(rx.try_recv(), Ok(())); + // Taken and dropped unresolved: the handler did nothing with the press. + assert!(took_notice(&mut rx)); let second = now + Duration::from_millis(500); assert_eq!( @@ -47,7 +61,7 @@ fn second_interrupt_within_cooldown_bypasses_handlers() { ); assert!(inner.shutdown_token.is_cancelled()); // The handler was bypassed: no second notification. - assert_eq!(rx.try_recv(), Err(TryRecvError::Empty)); + assert_eq!(recv_error(&mut rx), Some(TryRecvError::Empty)); } #[test] @@ -83,12 +97,32 @@ fn cooldown_resets_escalation_counter() { let now = Instant::now(); assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); - assert_eq!(rx.try_recv(), Ok(())); + assert!(took_notice(&mut rx)); // Past the cooldown, the next press counts as a fresh first press. let later = now + Duration::from_secs(3); assert_eq!(inner.route_at(OsSignal::Interrupt, later), Routed::Handler); - assert_eq!(rx.try_recv(), Ok(())); + assert!(took_notice(&mut rx)); + assert!(!inner.shutdown_token.is_cancelled()); +} + +#[test] +fn answered_interrupt_resets_escalation_counter() { + let inner = RouterInner::new(Duration::from_secs(2)); + let (_guard, mut rx) = push_handler(&inner); + let now = Instant::now(); + + // The handler showed its menu and the user answered it. + assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); + rx.try_recv() + .expect("the handler should be notified") + .handled(); + + // Well inside the cooldown, but the previous press was answered, so this + // one opens the menu again rather than escalating past it. + let second = now + Duration::from_millis(1400); + assert_eq!(inner.route_at(OsSignal::Interrupt, second), Routed::Handler); + assert!(took_notice(&mut rx)); assert!(!inner.shutdown_token.is_cancelled()); } @@ -104,8 +138,8 @@ fn full_notification_channel_counts_as_notified() { // press (past the cooldown) is a no-op send, not an error. let later = now + Duration::from_secs(3); assert_eq!(inner.route_at(OsSignal::Interrupt, later), Routed::Handler); - assert_eq!(rx.try_recv(), Ok(())); - assert_eq!(rx.try_recv(), Err(TryRecvError::Empty)); + assert!(took_notice(&mut rx)); + assert_eq!(recv_error(&mut rx), Some(TryRecvError::Empty)); } #[test] @@ -131,9 +165,9 @@ fn dropping_guard_deregisters_handler() { drop(guard_top); assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); - assert_eq!(rx_bottom.try_recv(), Ok(())); + assert!(took_notice(&mut rx_bottom)); // Deregistration dropped the stored sender without ever notifying. - assert_eq!(rx_top.try_recv(), Err(TryRecvError::Disconnected)); + assert_eq!(recv_error(&mut rx_top), Some(TryRecvError::Disconnected)); } #[test] @@ -148,9 +182,9 @@ fn guards_can_drop_out_of_order() { drop(guard_bottom); assert_eq!(inner.route_at(OsSignal::Interrupt, now), Routed::Handler); - assert_eq!(rx_top.try_recv(), Ok(())); + assert!(took_notice(&mut rx_top)); // Deregistration dropped the stored sender without ever notifying. - assert_eq!(rx_bottom.try_recv(), Err(TryRecvError::Disconnected)); + assert_eq!(recv_error(&mut rx_bottom), Some(TryRecvError::Disconnected)); } #[test] @@ -169,7 +203,7 @@ fn terminate_bypasses_handler_stack() { let now = Instant::now(); assert_eq!(inner.route_at(OsSignal::Terminate, now), Routed::Shutdown); - assert_eq!(rx.try_recv(), Err(TryRecvError::Empty)); + assert_eq!(recv_error(&mut rx), Some(TryRecvError::Empty)); assert!(inner.shutdown_token.is_cancelled()); } @@ -190,8 +224,8 @@ fn decline_notifies_next_handler_down() { inner.notify_next_or_shutdown(); - assert_eq!(rx_bottom.try_recv(), Ok(())); - assert_eq!(rx_top.try_recv(), Err(TryRecvError::Empty)); + assert!(took_notice(&mut rx_bottom)); + assert_eq!(recv_error(&mut rx_top), Some(TryRecvError::Empty)); assert!(!inner.shutdown_token.is_cancelled()); } @@ -202,7 +236,7 @@ fn decline_with_single_handler_requests_shutdown() { inner.notify_next_or_shutdown(); - assert_eq!(rx.try_recv(), Err(TryRecvError::Empty)); + assert_eq!(recv_error(&mut rx), Some(TryRecvError::Empty)); assert!(inner.shutdown_token.is_cancelled()); } @@ -229,10 +263,14 @@ async fn escalation_ladder_reaches_exit_through_signal_task() { // First press: notifies the topmost handler; no shutdown, no exit. signals.interrupt().await; - interrupt_rx - .recv() - .await - .expect("handler should be notified"); + // Dropped unresolved: this handler was notified but did nothing with the + // press, which is what leaves it on the ladder for the next one. + drop( + interrupt_rx + .recv() + .await + .expect("handler should be notified"), + ); assert!(!router.shutdown_token().is_cancelled()); assert!(signals.exit_codes().is_empty()); diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 9826be052..2d348b8a2 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -172,7 +172,7 @@ "summary": "Redesign `jp init` to generate schema-driven config with interactive model/mode selection and curated commented fields." }, "045-layered-interrupt-handler-stack.md": { - "hash": "a097b20423e8776532d8821aafa40828f4a0fd0aea84bd32f1420e1926751c6a", + "hash": "6fadad9b4f536e5a3e0d6d6ff346433e9dc3ea988cde6d4cb60789c6bca0204d", "summary": "Replace JP's ad-hoc interrupt handling with a layered LIFO handler stack, routing OS signals through scoped notification channels." }, "046-nested-workspace-projection.md": { diff --git a/docs/rfd/045-layered-interrupt-handler-stack.md b/docs/rfd/045-layered-interrupt-handler-stack.md index 100b31089..ee9904456 100644 --- a/docs/rfd/045-layered-interrupt-handler-stack.md +++ b/docs/rfd/045-layered-interrupt-handler-stack.md @@ -16,9 +16,11 @@ the router's), preserving the current ability to show interactive terminal menus and return actions to the surrounding event loop. When no handler is registered, SIGINT triggers a graceful shutdown via a `CancellationToken`. -Escalating Ctrl-C provides a reliable escape hatch: first press invokes the -handler, second press triggers graceful shutdown, third press terminates -immediately. +Escalating Ctrl-C provides a reliable escape hatch: the first press invokes the +handler, a second *unanswered* press triggers graceful shutdown, and a third +terminates immediately. +A handler that acts on a press reports it and clears the count, so ordinary use +of an interrupt menu never escalates by accident. ## Motivation @@ -80,7 +82,8 @@ The new design preserves this property. ### Escalating Ctrl-C -Ctrl-C follows a three-level escalation: +Ctrl-C follows a three-level escalation, counting only presses that no handler +has acted on: | Level | Behavior | | ----- | --------------------------------------- | @@ -99,10 +102,18 @@ is cancelled by Ctrl-C (when the terminal is in raw mode, see [Dual Delivery Paths](#dual-delivery-paths-and-prompt-escalation)). Both produce the same result: graceful shutdown. -The router's escalation counter resets after a configurable cooldown (e.g. 2 -seconds without a Ctrl-C). -This prevents a Ctrl-C during streaming (1st press, handled by the menu) from -counting toward escalation minutes later. +Two things reset the router's escalation counter. + +A handler that acts on a press reports it, which clears the count immediately. +This is what keeps the ladder out of ordinary menu use: opening the streaming +menu, choosing "Continue", and interrupting again a second later gives a fresh +first press rather than a shutdown. +Only presses that produced nothing accumulate, which is the situation the ladder +exists for. + +A configurable cooldown also resets it (2 seconds by default, set through +`interrupt.escalation_cooldown_secs`), so an unanswered press does not count +toward escalation minutes later. SIGTERM always triggers graceful shutdown (cancels the root `CancellationToken`). @@ -179,14 +190,20 @@ pub enum InterruptOutcome { /// The caller should trigger graceful shutdown. /// See "Dual Delivery Paths and Prompt Escalation." Escalated, + + /// The handler's interactive prompt could not run at all. + /// Nothing was decided, so the caller leaves its work untouched and leaves + /// the press on the escalation ladder. + /// See "Dual Delivery Paths and Prompt Escalation." + PromptFailed, } struct RegisteredHandler { id: HandlerId, - /// The router sends to this channel to notify the handler's event loop that - /// SIGINT arrived. - /// The event loop then calls the handler. - notify_tx: mpsc::Sender<()>, + /// The router sends an [`InterruptNotice`] to this channel to notify the + /// handler's event loop that SIGINT arrived. + /// The event loop then calls the handler and resolves the notice. + notify_tx: mpsc::Sender, } ``` @@ -204,9 +221,9 @@ Registration returns an RAII guard and a notification receiver: ```rust impl SignalRouter { /// Register a handler scope. - /// Returns a guard (drop to deregister) and a receiver that fires when - /// SIGINT arrives while this handler is topmost. - pub fn push_handler(&self) -> (InterruptGuard, mpsc::Receiver<()>) { + /// Returns a guard (drop to deregister) and a receiver that yields an + /// [`InterruptNotice`] when SIGINT arrives while this handler is topmost. + pub fn push_handler(&self) -> (InterruptGuard, mpsc::Receiver) { let (tx, rx) = mpsc::channel(1); let id = self.inner.push(tx); (InterruptGuard { inner: self.inner.clone(), id }, rx) @@ -239,15 +256,48 @@ position. If an inner guard drops before an outer one due to early return or panic unwinding, the stack remains consistent. +### Resolving a Delivered Press + +Each delivered press arrives as an `InterruptNotice`, which the event loop +resolves once its interrupt logic finishes: + +```rust +impl InterruptNotice { + /// The handler acted on this press. + /// Clears the escalation count. + pub fn handled(self) { ... } + + /// The handler declined this press. + /// Notifies the next handler down the stack, or requests a graceful shutdown + /// when this was the last one. + /// The press keeps its place on the escalation ladder. + pub fn decline(self) { ... } +} +``` + +Dropping a notice without resolving it leaves the press on the ladder. +That is the safe default: a handler that was notified but produced nothing +visible should still escalate on the next press. + +The reset lives on the delivered value rather than on the router so it cannot be +forgotten independently of receiving the press. +An event loop cannot reach the point of deciding what a press did without +holding the notice that records the decision, and the type is `#[must_use]`. +This matters because the ladder has to behave the same wherever it exists: a +reset each handler had to remember to call separately would make escalation +depend on which handler answered. + ### Notification Dispatch When SIGINT arrives and the escalation count is 1 (first press): 1. The signal task locks the stack, clones the topmost handler's `notify_tx`, and releases the lock. -2. It sends `()` to `notify_tx`. +2. It sends an `InterruptNotice` to `notify_tx`. If the channel is full (handler hasn't consumed the previous notification), - the send is a no-op — the handler already has a pending interrupt. + the send is a no-op — the handler already has a pending interrupt, and the + undelivered notice is dropped unresolved, which correctly leaves the press on + the ladder. 3. The handler's event loop wakes up and processes the interrupt. If the handler's event loop has exited but the guard hasn't been dropped yet (a @@ -386,6 +436,16 @@ returns a new `InterruptOutcome::Escalated` variant. The event loop receives this and cancels the `shutdown_token`, producing the same effect as the router's 2nd-Ctrl-C path. +A prompt that *cannot run* is a different case, and must not escalate. +An I/O failure or a missing terminal answers nothing, so treating it as an +escalation would end the run on a decision the user never made — silently, +since there is no menu on screen to explain it. +The handler returns `PromptFailed` instead: the event loop leaves its work as it +was and leaves the press on the escalation ladder. +The user's next Ctrl-C then reaches the router with the count intact and +escalates through the ladder, so getting out never depends on the prompt +working. + ```rust pub enum InterruptOutcome { /// The handler fully processed the signal. @@ -398,6 +458,10 @@ pub enum InterruptOutcome { /// The handler's interactive prompt was cancelled by Ctrl-C. /// The event loop should trigger graceful shutdown. Escalated, + + /// The handler's interactive prompt could not run at all. + /// The event loop changes nothing and leaves the press on the ladder. + PromptFailed, } ``` @@ -433,9 +497,12 @@ Note: `inquire` already distinguishes the two keys — ESC produces `InquireError::OperationCanceled` and Ctrl-C produces `InquireError::OperationInterrupted` (with the `crossterm` backend, which JP uses). -The conflation is in JP's call sites, which treat every prompt error alike. -For interrupt menus this is fine — cancelling an interrupt menu by any means -should escalate. +Cancelling an interrupt menu by either means escalates, so the two are treated +alike. +What must stay distinct is cancellation versus failure: every other +`InquireError` — `NotTTY`, an I/O error, a custom error — means the prompt +never ran, and maps to `PromptFailed` rather than an escalation. + For non-interrupt prompts (tool permissions, tool questions), distinguishing ESC ("skip this prompt") from Ctrl-C ("I want the interrupt menu") remains valuable and requires no upstream change. @@ -447,22 +514,14 @@ The tool handler already does this: when `is_prompting` is true, it suppresses the interrupt menu to let the active tool prompt handle Ctrl-C. With the new model, decline works as follows: the event loop receives the -notification, evaluates whether it should handle the interrupt, and if not, -calls `signal_router.decline()`. +notice, evaluates whether it should handle the interrupt, and if not, resolves +the notice with `decline()`. This tells the router to notify the next handler down the stack. If no handler remains, the router cancels the `shutdown_token`. -```rust -impl SignalRouter { - /// Called by a handler's event loop when it declines to handle the current - /// interrupt. - /// The router notifies the next handler on the stack, or falls back to - /// graceful shutdown. - pub fn decline(&self) { - self.inner.notify_next_or_shutdown(); - } -} -``` +Declining leaves the press on the escalation ladder. +Nothing has acted on it yet — it is only being offered to a different handler +— so it still counts if the user presses again. ### Interaction With RFD 026 and RFD 027 @@ -590,14 +649,14 @@ The router has already moved on. No deadlock or inconsistency. **Escalation state across handlers.** The escalation counter tracks Ctrl-C -presses globally. -A 1st press during streaming (handled by the streaming menu) followed by a 2nd -press during tool execution would trigger graceful shutdown, even though the -tool handler never got a chance. -This is correct — the user is escalating — but might surprise users who expect -each handler to get a "fresh" first press. -The cooldown timer mitigates this: if enough time passes between presses, the -counter resets. +presses globally rather than per handler. +A press answered by the streaming menu clears the count, so a later press during +tool execution starts fresh and reaches the tool handler. +An *unanswered* press still carries across handler boundaries: if the streaming +menu never appeared, the next press escalates even though the tool handler never +got a chance. +That is the intent — the user is escalating because nothing happened — and the +cooldown bounds how long such a press stays armed. **Escalation counter and raw-mode presses.** Because Ctrl-C during a raw-mode prompt doesn't generate SIGINT, the router's escalation counter and the @@ -612,6 +671,15 @@ unexpectedly. In practice the window is negligible, and the outcome (graceful shutdown) is correct regardless of which path triggers it. +The paths are independent, but only one of them is a hard guarantee. +A prompt library that swallowed Ctrl-C outright would leave the raw-mode path +dead, and the router cannot see the press to compensate. +`PromptFailed` covers the case the router *can* observe — a prompt that failed +rather than one that silently ate the key — by keeping the press on the ladder +so the next one escalates. +Closing the remaining gap would mean keeping signal delivery enabled during +prompts, which is a property of `reedline` and `inquire` rather than of JP. + **Turn-level handler granularity.** The `TurnInterruptHandler` covers all gaps between streaming and tool execution. Some of these gaps are very brief (a few milliseconds of persistence). diff --git a/docs/ticket/0014-ctrl-c-during-streaming-ended-the-run-without-showing-the-in.md b/docs/ticket/0014-ctrl-c-during-streaming-ended-the-run-without-showing-the-in.md new file mode 100644 index 000000000..bff05a7a3 --- /dev/null +++ b/docs/ticket/0014-ctrl-c-during-streaming-ended-the-run-without-showing-the-in.md @@ -0,0 +1,171 @@ +# T0014: Ctrl-C during streaming ended the run without showing the interrupt menu + +- **Status**: Todo +- **Kind**: Bug +- **Authors**: jp +- **Date**: 2026-08-11 + +A single `^C` during an Opus reasoning stream ended the turn immediately. +No interrupt menu appeared, no message was printed, and the process exited 0. +Not reproduced since. + +## What the trace log shows + +Thinking deltas were still arriving 1.6s before the signal, so the stream was +live: + +```json +{ + "timestamp": "2026-08-11T10:23:49.150307Z", + "level": "TRACE", + "fields": { + "message": "Received event from Anthropic API.", + "event": "...thinking_delta..." + } +} +{ + "timestamp": "2026-08-11T10:23:50.788483Z", + "level": "INFO", + "fields": { + "message": "Signal received.", + "signal": "SIGINT" + } +} +{ + "timestamp": "2026-08-11T10:23:50.788522Z", + "level": "DEBUG", + "fields": { + "message": "Routed OS signal.", + "signal": "Interrupt", + "routed": "Handler" + } +} +{ + "timestamp": "2026-08-11T10:23:50.788671Z", + "level": "INFO", + "fields": { + "message": "Interrupt received during streaming." + } +} +{ + "timestamp": "2026-08-11T10:23:50.805940Z", + "level": "INFO", + "fields": { + "message": "Flushed conversation to disk.", + "id": "jp-c17860012287" + } +} +{ + "timestamp": "2026-08-11T10:23:50.849953Z", + "level": "DEBUG", + "fields": { + "message": "RunningService dropped..." + } +} +``` + +The router picked the right handler and `handle_streaming_interrupt` +(`crates/jp_cli/src/cmd/query/interrupt/signals.rs:70`) ran. 17ms later the +conversation was flushed and teardown began, so +`InterruptHandler::handle_streaming_interrupt` returned a decision without ever +blocking for input. + +On the terminal, the buffered reasoning appeared right after the echoed `^C` +(consistent with the `flush_renderer()` + `flush_instant()` at the top of that +function) and nothing else. +No menu, no `Interrupted`. + +## The evidence does not add up + +Two ways out of that function in 17ms, and neither fits cleanly. + +`config.action != Prompt` (`handler.rs:192`) skips `inline_select` entirely and +hardcodes the choice. +`stop` and `abort` both end the turn with `Ok(())`, which matches the exit +status. +But the action was not set anywhere: not in `~/Library/Application +Support/jp/config.toml`, not in the user-global `config/` directory, not in a +user-workspace config (no such file for workspace `otvo8`), not in +`JP_CFG_INTERRUPT_*`, and not as a `config_delta` on the conversation. +So it should have resolved to the `Prompt` default. + +`inline_select` returning `Err` maps to `InterruptAction::Escalate` +(`handler.rs:215`), which cancels the shutdown token and returns +`cmd::Error::interrupted()`. +That propagates unchanged through `query.rs:942` and `run_inner`'s select, so it +should have printed `Interrupted` on stderr and exited 130. +It did neither. + +One of those two readings has to give. +The likeliest weak link is the exit status, which was recalled from a shell +prompt rather than read from `$?`. +If it was really 130, this is `inline_select` failing instantly and the question +becomes why. + +## Why it can't be timing + +The menu decision does not consult stream state. +`stream_alive` is only used inside the `'c'` branch to pick `Resume` over +`Continue`; it never suppresses the menu. +A `^C` landing between the thinking block and the first text delta, or after the +stream died, still prompts. + +## Next time + +`Err(_)` at `handler.rs:215` and `handler.rs:296` discards the `InquireError`, +which is exactly the value that would have settled this. +Same for the `Ok(ReplyOutcome::Cancelled) | Err(_)` arm in +`collect_reply_inline`. +Nothing logs the resolved `interrupt.streaming.action` either, so ruling the +config in or out took five greps across four layers instead of one grep in the +trace log. + +Adding that logging is the immediate work; the bug itself stays open until it +reproduces with the extra data. + +## Related + +RFD 060 (Config Explain) would have answered the config half directly. + +## Comments + +----- + +- **From**: jp +- **Date**: 2026-08-11T11:15:05Z + +Logging is in place. + +`handler.rs` now logs the resolved action (`Handling streaming interrupt.` with +`action` and `menu`), and `log_prompt_failure` records the `InquireError` that +the menu call sites previously threw away as `Err(_)`. + +`signals.rs` logs the chosen `InterruptAction` and the returned result. + +Still open until it reproduces. + +----- + +- **From**: jp +- **Date**: 2026-08-17T16:35:48Z + +Fixed one proven cause of this symptom, though not the evidence recorded above. + +The escalation ladder was counting presses it had already answered. +`EscalationState::bump` only reset on elapsed time, so answering a menu and +interrupting again inside the 2s cooldown counted as press two and routed +straight to `Shutdown`, bypassing the handler stack — no menu, no message. +A delivered press is now an `InterruptNotice` the handler resolves: `handled()` +clears the count, `decline()` and dropping leave it intact. +Only unanswered presses escalate. + +Also split a user-cancelled menu from one that could not run. +The latter returns `PromptFailed` and leaves the press on the ladder rather than +escalating on a decision nobody made. + +Confirmed live: eight presses, five inside the cooldown of an answered one, all +reached the handler. + +Staying open because the traces in the description showed `routed=Handler` with +a ~20ms return — a different signature from this bug — and those files are +gone.