-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix duplicate rehydrate during reasoning; centralize rehydrate and preserve cancel metadata #8171
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
Changes from 2 commits
bcf2241
3bbf984
e5be214
054764c
b357f4c
b1958e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import type { ClineMessage } from "@roo-code/types" | ||
| import { readTaskMessages, saveTaskMessages } from "./taskMessages" | ||
| import { readApiMessages, saveApiMessages } from "./apiMessages" | ||
| import type { ClineApiReqCancelReason } from "../../shared/ExtensionMessage" | ||
|
|
||
| // Safely add cancelReason to the last api_req_started UI message | ||
| export async function addCancelReasonToLastApiReqStarted(args: { | ||
| taskId: string | ||
| globalStoragePath: string | ||
| reason: ClineApiReqCancelReason | ||
| }): Promise<void> { | ||
| const { taskId, globalStoragePath, reason } = args | ||
|
|
||
| try { | ||
| const uiMsgs = (await readTaskMessages({ taskId, globalStoragePath })) as ClineMessage[] | ||
|
|
||
| if (!Array.isArray(uiMsgs) || uiMsgs.length === 0) { | ||
| return | ||
| } | ||
|
|
||
| // Find last api_req_started | ||
| const revIdx = uiMsgs | ||
| .slice() | ||
| .reverse() | ||
| .findIndex((m) => m?.type === "say" && (m as any)?.say === "api_req_started") | ||
|
|
||
| if (revIdx === -1) { | ||
| return | ||
| } | ||
|
|
||
| const idx = uiMsgs.length - 1 - revIdx | ||
|
|
||
| try { | ||
| const existing = uiMsgs[idx]?.text ? JSON.parse(uiMsgs[idx].text as string) : {} | ||
| uiMsgs[idx].text = JSON.stringify({ ...existing, cancelReason: reason }) | ||
| await saveTaskMessages({ messages: uiMsgs as any, taskId, globalStoragePath }) | ||
| } catch { | ||
| // Non-fatal parse or write failure | ||
| return | ||
| } | ||
| } catch { | ||
| // Non-fatal read failure | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Append an assistant interruption marker to API conversation history | ||
| // only if the last message isn't already an assistant. | ||
| export async function appendAssistantInterruptionIfNeeded(args: { | ||
| taskId: string | ||
| globalStoragePath: string | ||
| text: string | ||
| }): Promise<void> { | ||
| const { taskId, globalStoragePath, text } = args | ||
|
|
||
| try { | ||
| const apiMsgs = await readApiMessages({ taskId, globalStoragePath }) | ||
| const last = apiMsgs.at(-1) | ||
|
|
||
| if (!last || last.role !== "assistant") { | ||
| apiMsgs.push({ | ||
| role: "assistant", | ||
| content: [{ type: "text", text }], | ||
| ts: Date.now(), | ||
| } as any) | ||
|
|
||
| await saveApiMessages({ messages: apiMsgs as any, taskId, globalStoragePath }) | ||
| } | ||
| } catch { | ||
| // Non-fatal read/write failure | ||
| return | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,9 +89,16 @@ import { Task } from "../task/Task" | |
| import { getSystemPromptFilePath } from "../prompts/sections/custom-system-prompt" | ||
|
|
||
| import { webviewMessageHandler } from "./webviewMessageHandler" | ||
| import type { ClineMessage } from "@roo-code/types" | ||
| import { readApiMessages, saveApiMessages, saveTaskMessages } from "../task-persistence" | ||
| import { getNonce } from "./getNonce" | ||
| import { getUri } from "./getUri" | ||
|
|
||
| import { | ||
| addCancelReasonToLastApiReqStarted, | ||
| appendAssistantInterruptionIfNeeded, | ||
| } from "../task-persistence/cancelBookkeeping" | ||
| import { RESPONSE_INTERRUPTED_BY_USER } from "../../shared/messages" | ||
| /** | ||
| * https://github.com/microsoft/vscode-webview-ui-toolkit-samples/blob/main/default/weather-webview/src/providers/WeatherViewProvider.ts | ||
| * https://github.com/KumarVariable/vscode-extension-sidebar-html/blob/master/src/customSidebarViewProvider.ts | ||
|
|
@@ -196,7 +203,35 @@ export class ClineProvider | |
| const onTaskStarted = () => this.emit(RooCodeEventName.TaskStarted, instance.taskId) | ||
| const onTaskCompleted = (taskId: string, tokenUsage: any, toolUsage: any) => | ||
| this.emit(RooCodeEventName.TaskCompleted, taskId, tokenUsage, toolUsage) | ||
| const onTaskAborted = () => this.emit(RooCodeEventName.TaskAborted, instance.taskId) | ||
| const onTaskAborted = async () => { | ||
| this.emit(RooCodeEventName.TaskAborted, instance.taskId) | ||
|
|
||
| try { | ||
| // Only rehydrate on genuine streaming failures. | ||
| // User-initiated cancels are handled by cancelTask(). | ||
| if (instance.abortReason === "streaming_failed") { | ||
| // Defensive safeguard: if another path already replaced this instance, skip | ||
| const current = this.getCurrentTask() | ||
| if (current && current.instanceId !== instance.instanceId) { | ||
| this.log( | ||
| `[onTaskAborted] Skipping rehydrate: current instance ${current.instanceId} != aborted ${instance.instanceId}`, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| const { historyItem } = await this.getTaskWithId(instance.taskId) | ||
| const rootTask = instance.rootTask | ||
| const parentTask = instance.parentTask | ||
| await this.createTaskWithHistoryItem({ ...historyItem, rootTask, parentTask }) | ||
| } | ||
| } catch (error) { | ||
| this.log( | ||
| `[onTaskAborted] Failed to rehydrate after streaming failure: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }`, | ||
| ) | ||
| } | ||
| } | ||
| const onTaskFocused = () => this.emit(RooCodeEventName.TaskFocused, instance.taskId) | ||
| const onTaskUnfocused = () => this.emit(RooCodeEventName.TaskUnfocused, instance.taskId) | ||
| const onTaskActive = (taskId: string) => this.emit(RooCodeEventName.TaskActive, taskId) | ||
|
|
@@ -2525,14 +2560,24 @@ export class ClineProvider | |
|
|
||
| console.log(`[cancelTask] cancelling task ${task.taskId}.${task.instanceId}`) | ||
|
|
||
| const { historyItem } = await this.getTaskWithId(task.taskId) | ||
| const { historyItem, uiMessagesFilePath } = await this.getTaskWithId(task.taskId) | ||
|
|
||
| // Preserve parent and root task information for history item. | ||
| const rootTask = task.rootTask | ||
| const parentTask = task.parentTask | ||
|
|
||
| // Mark this as a user-initiated cancellation so provider-only rehydration can occur | ||
| task.abortReason = "user_cancelled" | ||
|
||
|
|
||
| // Capture the current instance to detect if rehydrate already occurred elsewhere | ||
| const originalInstanceId = task.instanceId | ||
|
|
||
| // Begin abort (non-blocking) | ||
| task.abortTask() | ||
|
|
||
| // Immediately mark the original instance as abandoned to prevent any residual activity | ||
| task.abandoned = true | ||
|
|
||
| await pWaitFor( | ||
| () => | ||
| this.getCurrentTask()! === undefined || | ||
|
|
@@ -2549,11 +2594,43 @@ export class ClineProvider | |
| console.error("Failed to abort task") | ||
| }) | ||
|
|
||
| if (this.getCurrentTask()) { | ||
| // 'abandoned' will prevent this Cline instance from affecting | ||
| // future Cline instances. This may happen if its hanging on a | ||
| // streaming request. | ||
| this.getCurrentTask()!.abandoned = true | ||
| // Defensive safeguard: if current instance already changed, skip rehydrate | ||
| const current = this.getCurrentTask() | ||
| if (current && current.instanceId !== originalInstanceId) { | ||
| this.log( | ||
| `[cancelTask] Skipping cancel bookkeeping and rehydrate: current instance ${current.instanceId} != original ${originalInstanceId}`, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| // Provider-side cancel bookkeeping to mirror abortStream effects for user_cancelled | ||
| try { | ||
| // Persist cancelReason to last api_req_started in UI messages | ||
| await addCancelReasonToLastApiReqStarted({ | ||
| taskId: task.taskId, | ||
| globalStoragePath: this.contextProxy.globalStorageUri.fsPath, | ||
| reason: "user_cancelled", | ||
| }) | ||
|
|
||
| // Append assistant interruption marker to API conversation history if needed | ||
| await appendAssistantInterruptionIfNeeded({ | ||
| taskId: task.taskId, | ||
| globalStoragePath: this.contextProxy.globalStorageUri.fsPath, | ||
| text: `[${RESPONSE_INTERRUPTED_BY_USER}]`, | ||
| }) | ||
| } catch (e) { | ||
| this.log(`[cancelTask] Cancel bookkeeping failed: ${e instanceof Error ? e.message : String(e)}`) | ||
| } | ||
|
|
||
| // Final race check before rehydrate to avoid duplicate rehydration | ||
| { | ||
| const currentAfterBookkeeping = this.getCurrentTask() | ||
| if (currentAfterBookkeeping && currentAfterBookkeeping.instanceId !== originalInstanceId) { | ||
| this.log( | ||
| `[cancelTask] Skipping rehydrate after bookkeeping: current instance ${currentAfterBookkeeping.instanceId} != original ${originalInstanceId}`, | ||
| ) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Clears task again, so we need to abortTask manually above. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| /** | ||
| * Centralized user-facing message constants for interruption labels. | ||
| * TODO: Consider moving these to i18n JSON in src/i18n/locales/* and wiring through t() | ||
| * so they can be localized consistently across the UI. | ||
| * | ||
| * Note: These are plain phrases (no surrounding brackets). Call sites add any desired decoration. | ||
| */ | ||
| export const RESPONSE_INTERRUPTED_BY_USER = "Response interrupted by user" | ||
|
||
| export const RESPONSE_INTERRUPTED_BY_API_ERROR = "Response interrupted by API Error" | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Should we check if there's already a reason here before overwriting?