Skip to content

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Aug 12, 2025

Description

This PR fixes issue #7014 where the Cancel button does not stop the response streaming process when a model gets stuck in a streaming loop.

Problem

The issue was caused by a race condition where:

  1. When the user clicks Cancel, the UI sends a cancelTask message
  2. The cancelTask handler calls abortTask() which sets this.abort = true
  3. However, if a stream error occurs before abortTask() is called, the error handler checks this.abort to determine if it was user-cancelled or a streaming failure
  4. This race condition could cause the cancel to be incorrectly treated as a streaming failure

Solution

The fix ensures proper cancellation by:

  1. Setting the abort flag immediately when cancelTask is called, before calling abortTask()
  2. Checking the abort flag in the stream error handler before calling abortTask() to preserve the original cancel source
  3. Preventing redundant abort flag setting in the abortTask() method to avoid overwriting the original abort source

Changes Made

  • src/core/webview/ClineProvider.ts: Set abort flag immediately when cancel is clicked
  • src/core/task/Task.ts:
    • Check abort flag before calling abortTask() in stream error handler
    • Only set abort flag in abortTask() if not already set

Testing

  • All existing tests pass
  • Type checking passes
  • Linting passes

Related Issues

Fixes #7014


Important

Fixes race condition in Task.ts to ensure cancel button properly stops streaming responses by setting and checking abort flag correctly.

  • Behavior:
    • Fixes race condition where cancel button did not stop streaming responses in Task.ts.
    • Sets abort flag immediately when cancelTask is called in ClineProvider.ts.
    • Checks abort flag in stream error handler before calling abortTask() in Task.ts.
  • Code Changes:
    • Task.ts:
      • Moves abort check before processing stream chunks.
      • Prevents redundant abort flag setting in abortTask().
  • Testing:
    • All existing tests pass.
    • Type checking and linting pass.

This description was created by Ellipsis for 84f7196. You can customize this summary. It will automatically update as commits are pushed.

@roomote roomote bot requested review from cte, jr and mrubens as code owners August 12, 2025 18:55
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Aug 12, 2025
Copy link
Contributor Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

I reviewed my own code and found it lacking in several dimensions, yet somehow it still works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this intentional? You're setting cline.abort = true directly, then calling abortTask() which checks and potentially sets it again. While this works due to your guard clause, could we consider whether abortTask() could handle this internally with a parameter like abortTask(preserveOriginalSource = true) to make the intent clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment could be more specific about what we're preserving. Consider:

Suggested change
// Only set abort if not already set (to preserve the original abort source)
// Only set abort if not already set (to preserve whether cancellation was user-initiated vs streaming failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good defensive check here! This prevents the race condition where we'd incorrectly identify a user cancellation as a streaming failure. The logic flow is now: check existing abort state → determine cancel reason → conditionally call abortTask. This ensures we preserve the original cancellation source.

- Move abort check before awaiting iterator.next() to prevent blocking
- Add proper stream cleanup with iterator.return() when aborting
- Apply same fix to background usage collection loop

The issue was that the abort check happened after waiting for the next
chunk, which blocked cancellation. Now the check happens before each
await, allowing immediate response to cancel requests.

Fixes #7014
@roomote roomote bot force-pushed the fix/cancel-button-streaming-issue-7014 branch from 3534269 to 84f7196 Compare August 12, 2025 19:07
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 12, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 12, 2025
@daniel-lxs daniel-lxs closed this Aug 13, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 13, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Cancel does not cancel streaming

4 participants