Add context cancellation support for Ctrl+C during model response #239
Conversation
Reviewer's GuideThis PR adds Ctrl+C support for cancelling model responses in the interactive CLI by wrapping chat invocations in a cancellable context with signal handling and propagating that context through new context-aware chat and HTTP request APIs. Sequence diagram for Ctrl+C cancellation during model responsesequenceDiagram
actor User
participant CLI
participant DesktopClient
participant ModelAPI
User->>CLI: Enter prompt
CLI->>DesktopClient: ChatWithContext(ctx, ...)
DesktopClient->>ModelAPI: doRequestWithAuthContext(ctx, ...)
ModelAPI-->>DesktopClient: Stream response
DesktopClient-->>CLI: Stream content
User-->>CLI: Press Ctrl+C
CLI->>CLI: Cancel context
CLI->>DesktopClient: Context cancelled
DesktopClient->>ModelAPI: Abort request
ModelAPI-->>DesktopClient: Return context.Canceled
DesktopClient-->>CLI: Return context.Canceled
CLI->>User: Print cancellation message
Class diagram for updated context-aware chat and request methodsclassDiagram
class Client {
+Chat(backend, model, prompt, apiKey, outputFunc, shouldUseMarkdown) error
+ChatWithContext(ctx, backend, model, prompt, apiKey, outputFunc, shouldUseMarkdown) error
+doRequestWithAuth(method, path, body, backend, apiKey) (*http.Response, error)
+doRequestWithAuthContext(ctx, method, path, body, backend, apiKey) (*http.Response, error)
}
Client <|-- ChatWithContext
Client <|-- doRequestWithAuthContext
ChatWithContext ..> doRequestWithAuthContext: uses context for cancellation
Flow diagram for signal handling and context cancellation in CLIflowchart TD
A["User input received"] --> B["Create cancellable context"]
B --> C["Set up signal handler for Ctrl+C"]
C --> D["Start chatWithMarkdownContext with context"]
D --> E["User presses Ctrl+C?"]
E -- Yes --> F["Cancel context"]
F --> G["Abort chat request and clean up"]
E -- No --> H["Continue streaming response"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the command-line interface's interactive mode by integrating context cancellation. Users can now gracefully interrupt long-running model responses using Ctrl+C, providing a more responsive and controlled user experience during interactive sessions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The Ctrl+C signal handling code is duplicated in both interactive loops—consider extracting it into a reusable helper to DRY up and simplify cleanup.
- Instead of setting up signal.Notify and a goroutine for every chat invocation, you could wire SIGINT cancellation into the root context at startup and pass that down to chatWithMarkdownContext for simpler, centralized handling.
- Since you’ve introduced ChatWithContext and doRequestWithAuthContext, consider marking the old Chat and doRequestWithAuth methods as deprecated or removing them to avoid confusion and ensure consistent use of context-aware APIs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Ctrl+C signal handling code is duplicated in both interactive loops—consider extracting it into a reusable helper to DRY up and simplify cleanup.
- Instead of setting up signal.Notify and a goroutine for every chat invocation, you could wire SIGINT cancellation into the root context at startup and pass that down to chatWithMarkdownContext for simpler, centralized handling.
- Since you’ve introduced ChatWithContext and doRequestWithAuthContext, consider marking the old Chat and doRequestWithAuth methods as deprecated or removing them to avoid confusion and ensure consistent use of context-aware APIs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Ctrl+C cancellation during model response generation in both interactive modes (readline and basic). When a user presses Ctrl+C while the model is responding, the request is now gracefully cancelled instead of terminating the entire CLI session.
Key changes:
- Added context-aware versions of chat methods (
ChatWithContext,chatWithMarkdownContext) to support cancellation - Implemented signal handlers in both interactive modes to detect Ctrl+C and cancel ongoing requests
- Updated HTTP request methods to accept and propagate context for proper cancellation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/cli/desktop/desktop.go | Added context-aware variants of Chat and doRequestWithAuth methods, with cancellation checks in the response streaming loop |
| cmd/cli/commands/run.go | Implemented Ctrl+C signal handling in both interactive modes and updated to use new context-aware chat methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for canceling model responses using Ctrl+C. The implementation correctly uses context.Context to propagate cancellation signals through the call stack, from the command-line interface down to the HTTP requests. My review includes suggestions to improve the robustness of the signal handling goroutine and to refactor duplicated code for better maintainability.
| select { | ||
| case <-sigChan: |
There was a problem hiding this comment.
The current implementation of the signal-handling goroutine can be made more robust and idiomatic. A select statement listening on both the signal channel and the context's Done() channel will prevent the goroutine from leaking if the operation completes without a signal, and makes the cleanup path clearer. When cancelChat() is called during cleanup, the chatCtx.Done() case will be triggered, allowing the goroutine to exit gracefully.
This same improvement should be applied to the similar logic in generateInteractiveBasic at lines 271-272.
select {
case <-sigChan:
cancelChat()
case <-chatCtx.Done():
}| // Create a cancellable context for the chat request | ||
| // This allows us to cancel the request if the user presses Ctrl+C during response generation | ||
| chatCtx, cancelChat := context.WithCancel(context.Background()) | ||
|
|
||
| // Set up signal handler to cancel the context on Ctrl+C | ||
| sigChan := make(chan os.Signal, 1) | ||
| signal.Notify(sigChan, syscall.SIGINT) | ||
| go func() { | ||
| select { | ||
| case <-sigChan: | ||
| cancelChat() | ||
| case <-chatCtx.Done(): | ||
| // Context cancelled, exit goroutine | ||
| } | ||
| }() | ||
|
|
||
| err := chatWithMarkdownContext(chatCtx, cmd, desktopClient, backend, model, userInput, apiKey) | ||
|
|
||
| // Clean up signal handler | ||
| signal.Stop(sigChan) | ||
| close(sigChan) | ||
| cancelChat() | ||
|
|
||
| if err != nil { | ||
| // Check if the error is due to context cancellation (Ctrl+C during response) | ||
| if errors.Is(err, context.Canceled) { | ||
| cmd.Println() | ||
| } else { | ||
| cmd.PrintErr(handleClientError(err, "Failed to generate a response")) |
There was a problem hiding this comment.
This block of code for setting up context cancellation and signal handling is almost identical to the one in generateInteractiveBasic (lines 263-290). This duplication can make future maintenance harder.
To improve maintainability, please consider refactoring this logic into a shared helper function.
ericcurtin
force-pushed
the
implement-exit-behavior
branch
from
303a652 to
0c13915
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ericcurtin
force-pushed
the
implement-exit-behavior
branch
from
aa642e7 to
51f035a
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| cancelChat() | ||
| case <-chatCtx.Done(): | ||
| // Context cancelled, exit goroutine | ||
| // Context cancelled, exit goroutine |
There was a problem hiding this comment.
Duplicate comment on line 280; remove the redundant entry.
| // Context cancelled, exit goroutine | |
Add Ctrl+C handling to basic interactive mode for consistency Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
force-pushed
the
implement-exit-behavior
branch
from
6511d31 to
8b98ec5
Compare
Add Ctrl+C handling to basic interactive mode for consistency
Summary by Sourcery
Add context cancellation support for Ctrl+C during model response generation by integrating cancellable contexts and signal handling into interactive commands and HTTP client calls
Enhancements: