fix: allow Ctrl+C interrupt during agent thinking phase #169
Conversation
📝 WalkthroughWalkthroughAdds refined Ctrl+C/SIGINT handling: interactive input loop in Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Loop
participant Agent
participant Spinners
participant Subagents
participant Stderr as sys.stderr
User->>CLI: Press Ctrl+C
CLI->>CLI: KeyboardInterrupt caught
CLI->>Agent: Signal cancellation (first press)
Agent->>Spinners: _stop_spinners_immediately()
Agent->>Subagents: cancel active subagent tasks (thread-safe)
Agent->>Stderr: write cancellation message
alt Second Ctrl+C (force)
User->>CLI: Press Ctrl+C again
CLI->>Agent: Force-quit flow
Agent->>Spinners: immediate stop
Agent->>Stderr: write force-quit warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code_puppy/cli_runner.py (2)
883-886:⚠️ Potential issue | 🟡 MinorMissing
reset_windows_terminal_ansi()call in wiggum KeyboardInterrupt handler.The main-loop handler at Line 799 resets the Windows terminal ANSI state after a
KeyboardInterrupt, but this wiggum-loop handler does not. For consistency and to avoid the same terminal-bricking issue on Windows:Proposed fix
except KeyboardInterrupt: emit_warning("\n🍩 Wiggum loop interrupted by Ctrl+C") + reset_windows_terminal_ansi() stop_wiggum() break
1042-1050:⚠️ Potential issue | 🟠 MajorPre-existing bug:
execute_single_promptdoesn't unpack the tuple returned byrun_prompt_with_attachments.
run_prompt_with_attachmentsreturns(result, task), but here the whole tuple is assigned toresponse. Theresponse is Nonecheck on Line 1047 will never beTrue(it's a tuple), andresponse.outputon Line 1050 will raiseAttributeError. This wasn't introduced by this PR, but the early-handler changes also flow through this path.Proposed fix
- response = await run_prompt_with_attachments( + response, _task = await run_prompt_with_attachments( agent, prompt, spinner_console=message_renderer.console, )
🧹 Nitpick comments (2)
code_puppy/cli_runner.py (2)
990-993: Consider adding adone()check log or minor feedback in the early handler.The handler silently cancels the task with no user-visible feedback. Between Ctrl+C and the
CancelledErrorpropagation there can be a noticeable delay (model init, plugin hooks). A smallsys.stderr.writewould reassure the user that the interrupt was received. This is optional.💡 Optional: emit minimal feedback
def _early_cancel_handler(_sig, _frame): """Cancel the outer agent task before run_with_mcp's handler takes over.""" if not agent_task.done(): loop.call_soon_threadsafe(agent_task.cancel) + # Minimal feedback — safe to call from a signal handler + try: + sys.stderr.write("\n🐶 Interrupt received, cancelling…\n") + sys.stderr.flush() + except Exception: + pass
1011-1030: Duplicated finally-block logic across spinner / no-spinner paths.The handler-restoration logic is identical in both branches. This is a small DRY opportunity — extracting the restore into a helper or restructuring so
await agent_taskshares a single try/except/finally would reduce future drift risk. Fine to defer.♻️ Sketch: single try/finally wrapper
- if use_spinner and spinner_console is not None: - from code_puppy.messaging.spinner import ConsoleSpinner - - with ConsoleSpinner(console=spinner_console): - try: - result = await agent_task - return result, agent_task - except asyncio.CancelledError: - emit_info("Agent task cancelled") - return None, agent_task - finally: - current = signal.getsignal(signal.SIGINT) - if current is _early_cancel_handler: - signal.signal(signal.SIGINT, original_sigint) - else: - try: - result = await agent_task - return result, agent_task - except asyncio.CancelledError: - emit_info("Agent task cancelled") - return None, agent_task - finally: - current = signal.getsignal(signal.SIGINT) - if current is _early_cancel_handler: - signal.signal(signal.SIGINT, original_sigint) + def _restore_early_handler(): + current = signal.getsignal(signal.SIGINT) + if current is _early_cancel_handler: + signal.signal(signal.SIGINT, original_sigint) + + try: + if use_spinner and spinner_console is not None: + from code_puppy.messaging.spinner import ConsoleSpinner + + with ConsoleSpinner(console=spinner_console): + result = await agent_task + else: + result = await agent_task + return result, agent_task + except asyncio.CancelledError: + emit_info("Agent task cancelled") + return None, agent_task + finally: + _restore_early_handler()
The core problem: pressing Ctrl+C while the agent is thinking (waiting for the LLM to respond) produced zero visual feedback. The spinner kept spinning, making users think nothing happened — even though cancellation was scheduled. Changes in base_agent.py: - keyboard_interrupt_handler now provides immediate visual feedback by writing directly to stderr (bypassing Rich console which can deadlock with spinner) - First Ctrl+C: prints 'Cancelling...' message, stops spinners immediately via their threading events, then schedules graceful async cancellation - Second Ctrl+C: force-quits by raising KeyboardInterrupt (escape hatch if graceful cancellation hangs on HTTP connection teardown) - Replaced emit_warning() calls in schedule_agent_cancel with sys.stderr.write to prevent deadlocks when spinner thread holds Rich console lock - Pre-imported _RUNNING_PROCESSES and _active_spinners outside signal handler to avoid import-lock deadlocks Changes in cli_runner.py: - Added KeyboardInterrupt handler in main loop so force-quit (second Ctrl+C) returns gracefully to the prompt instead of crashing the program
mattnico
force-pushed
the
fix/early-sigint-handler
branch
from
87f1f56 to
6f34aa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code_puppy/agents/base_agent.py`:
- Around line 1898-1905: The loop in _stop_spinners_immediately iterates the
shared collection _active_spinners which can be mutated by other threads,
causing RuntimeError; fix by iterating over a shallow copy (e.g.,
list(_active_spinners) or set(_active_spinners)) so removals/additions during
iteration won't raise, leaving the body unchanged (still getting _stop_event via
getattr, calling set(), and setting spinner._is_spinning = False).
| def _stop_spinners_immediately() -> None: | ||
| """Stop all active spinners via their threading events (signal-safe).""" | ||
| for spinner in _active_spinners: | ||
| stop_event = getattr(spinner, "_stop_event", None) | ||
| if stop_event is not None: | ||
| stop_event.set() | ||
| # Also mark as not spinning so the context manager exits cleanly | ||
| spinner._is_spinning = False |
There was a problem hiding this comment.
Potential RuntimeError when iterating _active_spinners without copying.
_active_spinners can be mutated by spinner threads concurrently. Unlike _active_subagent_tasks (correctly copied with list(...) on line 1922), this iteration is unprotected and could raise RuntimeError: Set changed size during iteration (or similar) if a spinner is being added/removed at the moment the signal fires.
Proposed fix
def _stop_spinners_immediately() -> None:
"""Stop all active spinners via their threading events (signal-safe)."""
- for spinner in _active_spinners:
+ for spinner in list(_active_spinners):
stop_event = getattr(spinner, "_stop_event", None)
if stop_event is not None:
stop_event.set()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _stop_spinners_immediately() -> None: | |
| """Stop all active spinners via their threading events (signal-safe).""" | |
| for spinner in _active_spinners: | |
| stop_event = getattr(spinner, "_stop_event", None) | |
| if stop_event is not None: | |
| stop_event.set() | |
| # Also mark as not spinning so the context manager exits cleanly | |
| spinner._is_spinning = False | |
| def _stop_spinners_immediately() -> None: | |
| """Stop all active spinners via their threading events (signal-safe).""" | |
| for spinner in list(_active_spinners): | |
| stop_event = getattr(spinner, "_stop_event", None) | |
| if stop_event is not None: | |
| stop_event.set() | |
| # Also mark as not spinning so the context manager exits cleanly | |
| spinner._is_spinning = False |
🤖 Prompt for AI Agents
In `@code_puppy/agents/base_agent.py` around lines 1898 - 1905, The loop in
_stop_spinners_immediately iterates the shared collection _active_spinners which
can be mutated by other threads, causing RuntimeError; fix by iterating over a
shallow copy (e.g., list(_active_spinners) or set(_active_spinners)) so
removals/additions during iteration won't raise, leaving the body unchanged
(still getting _stop_event via getattr, calling set(), and setting
spinner._is_spinning = False).
1 participant
Problem
Users cannot interrupt Code Puppy with Ctrl+C during the "thinking" phase — the period between submitting a prompt and when the LLM starts streaming back tokens. Pressing Ctrl+C during this window either crashes the program entirely or is silently ignored.
Root Cause
The custom SIGINT handler that enables graceful cancellation was installed deep inside
run_with_mcp()inbase_agent.py, only after significant setup work:on_agent_run_start)During this entire setup window, asyncio's default SIGINT handler was active, which raises a raw
KeyboardInterruptinstead of gracefully cancelling the task. ThisKeyboardInterruptwould bubble up tomain_entry()and crash the program with a traceback.Additionally, the main
while Trueloop only hadexcept Exception:around agent execution — butKeyboardInterruptis aBaseException, not anException, so it was never caught gracefully at that level.Fix
1. Early SIGINT handler in
run_prompt_with_attachments()Install a preliminary SIGINT handler immediately after creating the outer agent task, before
run_with_mcpstarts executing. This handler cancels the outer task vialoop.call_soon_threadsafe(agent_task.cancel). Whenrun_with_mcpeventually installs its own, more granular handler, it naturally supersedes this one. Thefinallyblock cleans up ifrun_with_mcpnever got a chance to install its own.2.
KeyboardInterruptguard in main loopAdded
except KeyboardInterruptaround agent execution so any stray interrupts gracefully return to the prompt with a friendly message instead of crashing.Testing
test_cli_runner.pytests pass ✅test_base_agent_key_listeners.pytests pass ✅Summary by CodeRabbit