-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: ensure cancel button properly aborts streaming responses (#7014) #7016
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Set abort flag immediately in ClineProvider.cancelTask() before calling abortTask() - Add early abort check at the beginning of streaming loop before processing chunks - Add proper iterator cleanup when aborting stream - Keep existing abort check after chunk processing for redundancy This ensures the cancel button works even when chunks are arriving constantly, addressing the issue where the abort flag appeared to never be set. Fixes #7014
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the cancel button, but forgot to test if it actually wants to be clicked.
@@ -1663,6 +1663,26 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||
const iterator = stream[Symbol.asyncIterator]() | |||
let item = await iterator.next() | |||
while (!item.done) { | |||
// Check abort flag BEFORE processing the chunk | |||
if (this.abort) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding tests for this critical cancel functionality to prevent regression. The fix looks good, but tests would verify:
- The abort flag is set immediately when cancel is clicked
- The early abort check works before processing chunks
- Iterator cleanup happens properly when aborting
@@ -1663,6 +1663,26 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||
const iterator = stream[Symbol.asyncIterator]() | |||
let item = await iterator.next() | |||
while (!item.done) { | |||
// Check abort flag BEFORE processing the chunk | |||
if (this.abort) { | |||
console.log(`aborting stream (early check), this.abandoned = ${this.abandoned}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abort check and cleanup logic is duplicated here and at lines 1730-1747. Consider extracting this into a helper method like handleStreamAbort()
to reduce duplication and ensure consistency:
console.log(`aborting stream (early check), this.abandoned = ${this.abandoned}`) | |
private async handleStreamAbort(iterator: AsyncIterator<any>, cancelReason: string) { | |
console.log(`aborting stream (${cancelReason}), this.abandoned = ${this.abandoned}`) | |
if (!this.abandoned) { | |
await abortStream(cancelReason) | |
} | |
if (iterator.return) { | |
await iterator.return(undefined).catch(() => {}) | |
} | |
} |
@@ -1243,7 +1247,7 @@ export class ClineProvider | |||
timeout: 3_000, | |||
}, | |||
).catch(() => { | |||
console.error("Failed to abort task") | |||
console.error("Failed to abort task gracefully") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Could be more specific about the timeout duration:
console.error("Failed to abort task gracefully") | |
console.error("Failed to abort task gracefully within 3 second timeout") |
@@ -1228,6 +1228,10 @@ export class ClineProvider | |||
const rootTask = cline.rootTask | |||
const parentTask = cline.parentTask | |||
|
|||
// Set the abort flag immediately to signal cancellation | |||
cline.abort = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix! Setting the abort flag immediately before calling abortTask()
eliminates the race condition. This ensures the flag is set even if the stream check happens before abortTask()
propagates the flag internally.
- Add test for cancelling endless streaming responses - Add test for cancellation during chunk processing - Add test for immediate abort flag setting - Add test for proper resource cleanup during cancellation These tests verify that the cancellation mechanism properly handles all edge cases including endless streams, ensuring the fix in PR #7016 works correctly. Addresses feedback in issue #7014
Summary
This PR fixes the issue where the Cancel button doesn't stop the response streaming process, even when chunks are arriving constantly.
Problem
As reported by @pwilkin in #7014, the Cancel button had no effect on streaming responses. Even with chunks arriving constantly (which should trigger the abort check between chunks), the cancellation didn't work. The investigation revealed that the abort flag wasn't being set properly before waiting for the stream to finish.
Root Cause
The issue was in
ClineProvider.cancelTask()
:cline.abortTask()
which sets the abort flagpWaitFor
Solution
ClineProvider.cancelTask()
, we now setcline.abort = true
directly before callingabortTask()
Changes
src/core/webview/ClineProvider.ts
: Set abort flag immediately when cancel is clickedsrc/core/task/Task.ts
: Check abort flag before and after processing each chunk, with proper iterator cleanupTesting
Related Issues
Fixes #7014
Important
Fixes issue where Cancel button did not stop streaming tasks by setting abort flag immediately and adding early abort checks in
ClineProvider.ts
andTask.ts
.ClineProvider.ts
andTask.ts
.abort
flag immediately inClineProvider.cancelTask()
.Task.ts
.Task.ts
.This description was created by
for 6aa129e. You can customize this summary. It will automatically update as commits are pushed.