From ff33248ed525c09aa41260feb2ad56c566f739f4 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 17 Aug 2026 18:56:50 +0200 Subject: [PATCH 1/2] fix(cli, rfd): Escalate Ctrl-C only on unanswered presses `EscalationState` reset the Ctrl-C press count on elapsed time alone, so a press an interrupt menu had already answered stayed on the escalation ladder. Answering the streaming menu and interrupting again inside the two-second cooldown counted as a second press, which bypasses the handler stack and requests a graceful shutdown, so the run ended with no menu and no message. Answering the menu quickly made the next Ctrl-C fatal; taking longer than the cooldown left it working. A delivered press is now an `InterruptNotice` that the notified event loop resolves. `handled()` clears the count, `decline()` hands the press to the next handler down with the count intact, and dropping it leaves the press on the ladder, which is the safe default for a handler that was notified but produced nothing visible. Only presses that produced nothing escalate. Carrying the reset on the delivered value rather than on the router means an event loop cannot decide what a press did without holding the value that records the decision, so the ladder behaves the same at every handler site. `lock.rs` and `plugin/dispatch.rs` had the same defect and now resolve their presses too. `inline_select` returning `Err` mapped every error to an escalation, conflating the user cancelling the menu with the menu being unable to run at all. The latter now yields `PromptFailed`, which leaves the turn untouched and the press on the ladder, so a second Ctrl-C gets the user out through the router instead of through a prompt that is not working. The interrupt path also logs the resolved action, the decision the handler reached, and the `InquireError` behind a prompt that returned without a selection. That error was discarded as `Err(_)`, and its absence is why diagnosing this took several rounds of guessing. RFD 045 is updated to match: the ladder counts unanswered presses, and `SignalRouter::decline` is replaced by resolving the notice. T0014 stays open, since the traces that opened it showed a different signature and are no longer available. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/lock.rs | 7 +- crates/jp_cli/src/cmd/plugin/dispatch.rs | 8 +- .../jp_cli/src/cmd/query/interrupt/handler.rs | 71 +++++++- .../src/cmd/query/interrupt/handler_tests.rs | 91 +++++++++- .../jp_cli/src/cmd/query/interrupt/signals.rs | 43 ++++- crates/jp_cli/src/cmd/query/stream/retry.rs | 21 ++- .../src/cmd/query/stream/retry_tests.rs | 2 +- .../jp_cli/src/cmd/query/tool/coordinator.rs | 54 ++++-- .../jp_cli/src/cmd/query/turn/coordinator.rs | 6 +- crates/jp_cli/src/cmd/query/turn_loop.rs | 48 +++++- crates/jp_cli/src/signals.rs | 130 +++++++++++--- crates/jp_cli/src/signals_tests.rs | 80 ++++++--- .../045-layered-interrupt-handler-stack.md | 150 +++++++++++----- ...ng-ended-the-run-without-showing-the-in.md | 160 ++++++++++++++++++ 14 files changed, 739 insertions(+), 132 deletions(-) create mode 100644 docs/ticket/0014-ctrl-c-during-streaming-ended-the-run-without-showing-the-in.md 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/rfd/045-layered-interrupt-handler-stack.md b/docs/rfd/045-layered-interrupt-handler-stack.md index 100b31089..f09d0e34d 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..bd6ae3e50 --- /dev/null +++ b/docs/ticket/0014-ctrl-c-during-streaming-ended-the-run-without-showing-the-in.md @@ -0,0 +1,160 @@ +# 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. From 84dd7e1f48a2d3ad4e99385004b7a5dc5c76d73d Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 17 Aug 2026 19:16:21 +0200 Subject: [PATCH 2/2] fixup! fix(cli, rfd): Escalate Ctrl-C only on unanswered presses Signed-off-by: Jean Mertz --- docs/.vitepress/rfd-summaries.json | 2 +- .../045-layered-interrupt-handler-stack.md | 12 ++++----- ...ng-ended-the-run-without-showing-the-in.md | 25 +++++++++++++------ 3 files changed, 25 insertions(+), 14 deletions(-) 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 f09d0e34d..ee9904456 100644 --- a/docs/rfd/045-layered-interrupt-handler-stack.md +++ b/docs/rfd/045-layered-interrupt-handler-stack.md @@ -438,8 +438,8 @@ 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. +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 @@ -500,8 +500,8 @@ uses). 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. +`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 @@ -520,8 +520,8 @@ This tells the router to notify the next handler down the stack. If no handler remains, the router cancels the `shutdown_token`. 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. +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 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 index bd6ae3e50..bff05a7a3 100644 --- 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 @@ -151,10 +151,21 @@ Still open until it reproduces. 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. +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.