Skip to content

Fixes #4903: Consistent cancellation error messages for thinking vs streaming phases #4904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixes #4903: Consistent cancellation error messages for thinking vs streaming phases #4904

wants to merge 2 commits into from
+9 −4

Conversation

roomote
Copy link
Collaborator

@roomote roomote commented Jun 19, 2025

Summary

This PR fixes the inconsistent cancellation error messages reported in issue #4903. Previously, when users cancelled during the thinking phase of reasoning models (like Claude 4.0 Sonnet), they would see a misleading red 'API Streaming Failed' error instead of the expected grey 'API Request Cancelled' message.

Changes Made

  • Modified to properly detect user-initiated cancellations
  • Added logic to check if is true when handling streaming errors
  • When a user cancels (abort = true), the cancellation is now consistently treated as 'user_cancelled' regardless of whether it occurs during thinking or streaming phases
  • When it's an actual streaming error (abort = false), it's still treated as 'streaming_failed'

Testing

  • Existing tests continue to pass
  • The fix ensures consistent UX for user-initiated cancellations across all phases of model execution

Related Issues

Fixes #4903
Related to #4039

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@roomote
Fixes #4903: Consistent cancellation error messages for thinking vs s…
90873aa 
…treaming phases

- Modified Task.ts to check if cancellation was user-initiated (this.abort = true)
- When user cancels during thinking phase, now shows 'API Request Cancelled' instead of 'API Streaming Failed'
- Ensures consistent UX for user-initiated cancellations regardless of when they occur
@roomote roomote requested review from mrubens, cte and jr as code owners June 19, 2025 21:38
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jun 19, 2025
@ellipsis-dev ellipsis.dev
Copy link

ellipsis-dev bot commented Jun 19, 2025

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

Generated with ❤️ by ellipsis.dev

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 19, 2025
hannesrudolph
hannesrudolph reviewed Jun 19, 2025
View reviewed changes
Copy link
Collaborator

@hannesrudolph hannesrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I've left a couple of comments on the PR:

  1. An unnecessary log file seems to have been included in the commit.
  2. A question about test coverage for the cancellation logic change.

Please take a look when you have a moment.

src/core/task/Task.ts
? undefined
: (error.message ?? JSON.stringify(serializeError(error), null, 2))

await abortStream(cancelReason, streamingFailedMessage)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to correctly handle the inconsistent cancellation message. I'm curious if there are any existing tests that cover this scenario, or if we could add a new one to ensure this behavior is protected against regressions?

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 19, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 20, 2025
@daniel-lxs
Copy link
Collaborator

This fix seems to solve the issue:

image

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannesrudolph hannesrudolph hannesrudolph left review comments

@mrubens mrubens Awaiting requested review from mrubens mrubens is a code owner

@cte cte Awaiting requested review from cte cte is a code owner

@jr jr Awaiting requested review from jr jr is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working PR - Needs Review size:S This PR changes 10-29 lines, ignoring generated files.
Projects
Roo Code Roadmap
Status: PR [Needs Review]
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Inconsistent cancellation error messages - "API Streaming Failed" during thinking vs "API Request Cancelled" during streaming
3 participants
@roomote @daniel-lxs @hannesrudolph