fix: make ACP cancellation work #5583
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes cancellation functionality in the ACP implementation by ensuring the cancellation token is properly stored on the session object. Previously, cancellation requests from clients during tool calling loops were ineffective because the token wasn't accessible.
Key Changes:
- Added code to store the cancellation token in the session after creation
- Implemented proper session lookup and mutation within a lock scope
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let session = sessions | ||
| .get_mut(&session_id) | ||
| .ok_or_else(acp::Error::invalid_params)?; |
There was a problem hiding this comment.
The error acp::Error::invalid_params doesn't clearly indicate that the session was not found. Consider using a more specific error that indicates the session ID is invalid or the session doesn't exist, such as acp::Error::session_not_found or providing a custom error message.
alexhancock
force-pushed
the
alexhancock/acp-cancellation
branch
from
ab1c49c to
6663a4a
Compare
|
Replacing this with #5588 which also makes |
Fixes cancellation by properly storing the cancel token on the session. Prior to this, hitting the cancel button in clients like zed etc didn't do anything during tool calling loops. Now it works.