From 311ea08d0e08ab7f6a2cc31f42c38eec8bb42d51 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 22 Jul 2025 04:28:35 +0000 Subject: [PATCH] fix: improve stability and error handling to address issue #6045 - Add ErrorBoundary component to prevent UI crashes - Improve stream error handling in Task.ts with better recovery - Enhance abort/cancellation flow with timeouts to prevent hanging - Add long-running operation indicator for better user feedback - Fix timeout handling in ClineProvider to prevent indefinite waits - Add proper cleanup in dispose() method for streaming states These changes address the stability issues reported in #6045 by: 1. Preventing crashes from propagating through error boundaries 2. Handling stream failures gracefully with user notification 3. Ensuring operations don't hang indefinitely with proper timeouts 4. Providing visual feedback for long-running operations --- src/core/task/Task.ts | 47 +++++++- src/core/webview/ClineProvider.ts | 90 ++++++++++----- webview-ui/src/App.tsx | 21 ++-- webview-ui/src/components/ErrorBoundary.tsx | 105 ++++++++++++++++++ webview-ui/src/components/chat/ChatRow.tsx | 14 +++ .../common/LongRunningOperationIndicator.tsx | 98 ++++++++++++++++ 6 files changed, 334 insertions(+), 41 deletions(-) create mode 100644 webview-ui/src/components/ErrorBoundary.tsx create mode 100644 webview-ui/src/components/common/LongRunningOperationIndicator.tsx diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index ccd24b7d71c..b35cd896ca3 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1072,6 +1072,13 @@ export class Task extends EventEmitter { } catch (error) { console.error("Error reverting diff changes:", error) } + + // Reset all streaming-related state + this.isStreaming = false + this.isWaitingForFirstChunk = false + this.didCompleteReadingStream = true + this.presentAssistantMessageLocked = false + this.presentAssistantMessageHasPendingUpdates = false } public async abortTask(isAbandoned = false) { @@ -1091,6 +1098,10 @@ export class Task extends EventEmitter { console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) // Don't rethrow - we want abort to always succeed } + + // Ensure streaming state is reset even if dispose fails + this.isStreaming = false + this.isWaitingForFirstChunk = false // Save the countdown message in the automatic retry or other content. try { // Save the countdown message in the automatic retry or other content. @@ -1359,6 +1370,24 @@ export class Task extends EventEmitter { let reasoningMessage = "" this.isStreaming = true + // Add a timeout for the entire streaming operation + const streamTimeout = setTimeout( + async () => { + if (this.isStreaming && !this.abort) { + console.error(`Stream timeout for task ${this.taskId} - aborting stream`) + await this.say( + "error", + "The API request timed out. This might be due to network issues or server problems. The task will be cancelled.", + undefined, + false, + ) + // Trigger abort to clean up properly + this.abort = true + } + }, + 5 * 60 * 1000, + ) // 5 minute timeout + try { for await (const chunk of stream) { if (!chunk) { @@ -1451,19 +1480,25 @@ export class Task extends EventEmitter { ? undefined : (error.message ?? JSON.stringify(serializeError(error), null, 2)) - // Now call abortTask after determining the cancel reason - await this.abortTask() + try { + // Now call abortTask after determining the cancel reason + await this.abortTask() - await abortStream(cancelReason, streamingFailedMessage) + await abortStream(cancelReason, streamingFailedMessage) - const history = await provider?.getTaskWithId(this.taskId) + const history = await provider?.getTaskWithId(this.taskId) - if (history) { - await provider?.initClineWithHistoryItem(history.historyItem) + if (history) { + await provider?.initClineWithHistoryItem(history.historyItem) + } + } catch (abortError) { + console.error(`Error during stream failure recovery for task ${this.taskId}:`, abortError) + // Continue execution even if abort fails to prevent hanging } } } finally { this.isStreaming = false + clearTimeout(streamTimeout) } if (inputTokens > 0 || outputTokens > 0 || cacheWriteTokens > 0 || cacheReadTokens > 0) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6231f081670..4ff4e64517e 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -192,13 +192,20 @@ export class ClineProvider console.log(`[subtasks] removing task ${cline.taskId}.${cline.instanceId} from stack`) try { - // Abort the running task and set isAbandoned to true so - // all running promises will exit as well. - await cline.abortTask(true) + // Set a timeout for the abort operation + const abortPromise = cline.abortTask(true) + const timeoutPromise = new Promise((_, reject) => + setTimeout(() => reject(new Error("Abort timeout")), 3000), + ) + + // Race between abort completion and timeout + await Promise.race([abortPromise, timeoutPromise]) } catch (e) { this.log( `[subtasks] encountered error while aborting task ${cline.taskId}.${cline.instanceId}: ${e.message}`, ) + // Force abandon the task if abort fails + cline.abandoned = true } // Make sure no reference kept, once promises end it will be @@ -971,38 +978,69 @@ export class ClineProvider console.log(`[subtasks] cancelling task ${cline.taskId}.${cline.instanceId}`) - const { historyItem } = await this.getTaskWithId(cline.taskId) - // Preserve parent and root task information for history item. - const rootTask = cline.rootTask - const parentTask = cline.parentTask + let historyItem: HistoryItem | undefined + let rootTask: Task | undefined + let parentTask: Task | undefined + + try { + const taskData = await this.getTaskWithId(cline.taskId) + historyItem = taskData.historyItem + // Preserve parent and root task information for history item. + rootTask = cline.rootTask + parentTask = cline.parentTask + } catch (error) { + console.error(`Failed to get task history for ${cline.taskId}:`, error) + // Continue with cancellation even if we can't get history + } + // Start the abort process cline.abortTask() - await pWaitFor( - () => - this.getCurrentCline()! === undefined || - this.getCurrentCline()!.isStreaming === false || - this.getCurrentCline()!.didFinishAbortingStream || - // If only the first chunk is processed, then there's no - // need to wait for graceful abort (closes edits, browser, - // etc). - this.getCurrentCline()!.isWaitingForFirstChunk, - { - timeout: 3_000, - }, - ).catch(() => { - console.error("Failed to abort task") - }) + // Wait for abort to complete with a timeout + try { + await pWaitFor( + () => { + const currentCline = this.getCurrentCline() + return ( + !currentCline || + currentCline.taskId !== cline.taskId || + !currentCline.isStreaming || + currentCline.didFinishAbortingStream || + currentCline.isWaitingForFirstChunk + ) + }, + { + timeout: 5_000, // Increased timeout for better reliability + interval: 100, + }, + ) + } catch (error) { + console.error("Timeout waiting for task abort, forcing cleanup") + // Force cleanup if timeout occurs + const currentCline = this.getCurrentCline() + if (currentCline && currentCline.taskId === cline.taskId) { + currentCline.abandoned = true + // Force remove from stack + await this.removeClineFromStack() + } + } - if (this.getCurrentCline()) { + // Final check and cleanup + const currentCline = this.getCurrentCline() + if (currentCline && currentCline.taskId === cline.taskId) { // 'abandoned' will prevent this Cline instance from affecting // future Cline instances. This may happen if its hanging on a // streaming request. - this.getCurrentCline()!.abandoned = true + currentCline.abandoned = true } - // Clears task again, so we need to abortTask manually above. - await this.initClineWithHistoryItem({ ...historyItem, rootTask, parentTask }) + // Only reinitialize with history if we have it + if (historyItem) { + await this.initClineWithHistoryItem({ ...historyItem, rootTask, parentTask }) + } else { + // Just clear the task if no history available + await this.removeClineFromStack() + } } async updateCustomInstructions(instructions?: string) { diff --git a/webview-ui/src/App.tsx b/webview-ui/src/App.tsx index 3c4c14f5dfe..886ca5cff8b 100644 --- a/webview-ui/src/App.tsx +++ b/webview-ui/src/App.tsx @@ -5,6 +5,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query" import { ExtensionMessage } from "@roo/ExtensionMessage" import TranslationProvider from "./i18n/TranslationContext" import { MarketplaceViewStateManager } from "./components/marketplace/MarketplaceViewStateManager" +import { ErrorBoundary } from "./components/ErrorBoundary" import { vscode } from "./utils/vscode" import { telemetryClient } from "./utils/TelemetryClient" @@ -283,15 +284,17 @@ const App = () => { const queryClient = new QueryClient() const AppWithProviders = () => ( - - - - - - - - - + + + + + + + + + + + ) export default AppWithProviders diff --git a/webview-ui/src/components/ErrorBoundary.tsx b/webview-ui/src/components/ErrorBoundary.tsx new file mode 100644 index 00000000000..b6e968dcf8a --- /dev/null +++ b/webview-ui/src/components/ErrorBoundary.tsx @@ -0,0 +1,105 @@ +import React, { Component, ErrorInfo, ReactNode } from "react" +import { VSCodeButton } from "@vscode/webview-ui-toolkit/react" + +interface Props { + children: ReactNode + fallback?: ReactNode +} + +interface State { + hasError: boolean + error: Error | null + errorInfo: ErrorInfo | null +} + +export class ErrorBoundary extends Component { + constructor(props: Props) { + super(props) + this.state = { + hasError: false, + error: null, + errorInfo: null, + } + } + + static getDerivedStateFromError(error: Error): State { + // Update state so the next render will show the fallback UI + return { + hasError: true, + error, + errorInfo: null, + } + } + + componentDidCatch(error: Error, errorInfo: ErrorInfo) { + // Log error details for debugging + console.error("ErrorBoundary caught an error:", error, errorInfo) + + // Update state with error details + this.setState({ + error, + errorInfo, + }) + } + + handleReset = () => { + this.setState({ + hasError: false, + error: null, + errorInfo: null, + }) + } + + render() { + if (this.state.hasError) { + // Custom fallback UI + if (this.props.fallback) { + return this.props.fallback + } + + // Default error UI + return ( +
+

Something went wrong

+

+ An unexpected error occurred. The application may continue to work, but some features might be + affected. +

+ + {this.state.error && ( +
+ + Error details + + 
+								{this.state.error.message}
+								{this.state.error.stack && "\n\n" + this.state.error.stack}
+
+
+ )} + +
+ Try Again + window.location.reload()}> + Reload Window + +
+
+ ) + } + + return this.props.children + } +} + +// Higher-order component for easier usage +export function withErrorBoundary

( + Component: React.ComponentType

, + fallback?: ReactNode, +): React.ComponentType

{ + return (props: P) => ( + + + + ) +} diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index 926bd400f04..d543f6b2d83 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -46,6 +46,7 @@ import { CommandExecutionError } from "./CommandExecutionError" import { AutoApprovedRequestLimitWarning } from "./AutoApprovedRequestLimitWarning" import { CondenseContextErrorRow, CondensingContextRow, ContextCondenseRow } from "./ContextCondenseRow" import CodebaseSearchResultsDisplay from "./CodebaseSearchResultsDisplay" +import { LongRunningOperationIndicator } from "../common/LongRunningOperationIndicator" interface ChatRowProps { message: ClineMessage @@ -987,6 +988,9 @@ export const ChatRowContent = ({ /> ) case "api_req_started": + // Show long-running operation indicator for streaming API requests + const isApiStreaming = isLast && !cost && !apiRequestFailedMessage && !apiReqCancelReason + return ( <>

)} + {isApiStreaming && ( + { + vscode.postMessage({ type: "cancelTask" }) + }} + /> + )} + {isExpanded && (
void +} + +export const LongRunningOperationIndicator: React.FC = ({ + isRunning, + operationName = "Operation", + elapsedTime, + estimatedTime, + showCancel = true, + onCancel, +}) => { + const [seconds, setSeconds] = useState(0) + + useEffect(() => { + if (!isRunning) { + setSeconds(0) + return + } + + const interval = setInterval(() => { + setSeconds((prev) => prev + 1) + }, 1000) + + return () => clearInterval(interval) + }, [isRunning]) + + if (!isRunning) { + return null + } + + const formatTime = (totalSeconds: number): string => { + const minutes = Math.floor(totalSeconds / 60) + const secs = totalSeconds % 60 + if (minutes > 0) { + return `${minutes}m ${secs}s` + } + return `${secs}s` + } + + const displayTime = elapsedTime !== undefined ? elapsedTime : seconds + + return ( +
+ +
+
{operationName} in progress...
+
+ Elapsed: {formatTime(displayTime)} + {estimatedTime && Estimated: {formatTime(estimatedTime)}} +
+
+ {showCancel && onCancel && ( + + )} +
+ ) +} + +// Hook to track long-running operations +export const useLongRunningOperation = (threshold: number = 3000) => { + const [isLongRunning, setIsLongRunning] = useState(false) + const [startTime, setStartTime] = useState(null) + + const start = () => { + setStartTime(Date.now()) + setTimeout(() => { + setIsLongRunning(true) + }, threshold) + } + + const stop = () => { + setIsLongRunning(false) + setStartTime(null) + } + + const elapsedTime = startTime ? Math.floor((Date.now() - startTime) / 1000) : 0 + + return { + isLongRunning, + elapsedTime, + start, + stop, + } +}