From 4b2b91fcdc4a2157bb0600d7e942174bc596694e Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 10:05:30 +0000 Subject: [PATCH] fix: add user notifications for background task completion, errors, and timeouts - Add BackgroundTaskRunnerCallbacks interface with onTaskTimeout and onTaskError - Wire VS Code notifications in ClineProvider: info on completion, warning on timeout/error - Document auto-approval design decision for read-only background tasks - Add 2 new tests for callback invocation (19 total, all passing) --- src/core/task/BackgroundTaskRunner.ts | 36 ++++++++++++++++--- src/core/task/Task.ts | 6 ++++ .../__tests__/BackgroundTaskRunner.spec.ts | 25 +++++++++++++ src/core/webview/ClineProvider.ts | 18 ++++++++-- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/core/task/BackgroundTaskRunner.ts b/src/core/task/BackgroundTaskRunner.ts index f29b5da128..9f383d17c5 100644 --- a/src/core/task/BackgroundTaskRunner.ts +++ b/src/core/task/BackgroundTaskRunner.ts @@ -35,17 +35,31 @@ export interface BackgroundTaskInfo { timeoutHandle: ReturnType } +/** + * Optional callbacks that allow the owner (e.g. ClineProvider) to react to + * background task lifecycle events such as completion, timeout, or errors. + */ +export interface BackgroundTaskRunnerCallbacks { + /** Called when a background task times out. */ + onTaskTimeout?: (taskId: string, parentTaskId: string) => void + /** Called when aborting a background task throws an error. */ + onTaskError?: (taskId: string, parentTaskId: string, error: Error) => void +} + export class BackgroundTaskRunner { private backgroundTasks: Map = new Map() private maxConcurrentTasks: number private taskTimeoutMs: number + private callbacks: BackgroundTaskRunnerCallbacks constructor( maxConcurrentTasks: number = DEFAULT_MAX_BACKGROUND_TASKS, taskTimeoutMs: number = DEFAULT_BACKGROUND_TASK_TIMEOUT_MS, + callbacks: BackgroundTaskRunnerCallbacks = {}, ) { this.maxConcurrentTasks = maxConcurrentTasks this.taskTimeoutMs = taskTimeoutMs + this.callbacks = callbacks } /** @@ -163,11 +177,13 @@ export class BackgroundTaskRunner { try { await info.task.abortTask(true) } catch (error) { - console.error( - `[BackgroundTaskRunner] Error aborting background task ${taskId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ) + const err = error instanceof Error ? error : new Error(String(error)) + console.error(`[BackgroundTaskRunner] Error aborting background task ${taskId}: ${err.message}`) + try { + this.callbacks.onTaskError?.(taskId, info.parentTaskId, err) + } catch { + // Callback errors must not break cleanup. + } } this.backgroundTasks.delete(taskId) @@ -193,7 +209,17 @@ export class BackgroundTaskRunner { * Handle timeout of a background task. */ private async timeoutTask(taskId: string): Promise { + const info = this.backgroundTasks.get(taskId) + const parentTaskId = info?.parentTaskId ?? "unknown" + console.warn(`[BackgroundTaskRunner] Background task ${taskId} timed out after ${this.taskTimeoutMs}ms`) + + try { + this.callbacks.onTaskTimeout?.(taskId, parentTaskId) + } catch { + // Callback errors must not break cleanup. + } + await this.cancelTask(taskId) } } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 36d4b044e0..b82588df37 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1338,6 +1338,12 @@ export class Task extends EventEmitter implements TaskLike { let timeouts: NodeJS.Timeout[] = [] // Background tasks auto-approve all asks immediately (no user interaction). + // Design decision: Full auto-approval is safe here because background tasks + // are restricted to read-only tools only (read_file, list_files, search_files, + // codebase_search). They cannot modify files, execute commands, or perform any + // destructive operations. If a future phase introduces write-capable background + // tasks, this auto-approval should be revisited to allow selective user input + // for dangerous operations. if (this.isBackgroundTask) { this.approveAsk() await pWaitFor(() => this.askResponse !== undefined || this.lastMessageTs !== askTs, { interval: 100 }) diff --git a/src/core/task/__tests__/BackgroundTaskRunner.spec.ts b/src/core/task/__tests__/BackgroundTaskRunner.spec.ts index 1ec2819eb8..29d9e28cd5 100644 --- a/src/core/task/__tests__/BackgroundTaskRunner.spec.ts +++ b/src/core/task/__tests__/BackgroundTaskRunner.spec.ts @@ -129,6 +129,19 @@ describe("BackgroundTaskRunner", () => { it("should handle canceling unknown task gracefully", async () => { await runner.cancelTask("unknown") // should not throw }) + + it("should invoke onTaskError callback when abort throws", async () => { + const onTaskError = vi.fn() + const customRunner = new BackgroundTaskRunner(3, undefined, { onTaskError }) + const task = createMockTask("task-1") + task.abortTask.mockRejectedValue(new Error("abort failed")) + customRunner.registerTask(task, "parent-1") + + await customRunner.cancelTask("task-1") + + expect(onTaskError).toHaveBeenCalledWith("task-1", "parent-1", expect.any(Error)) + expect(customRunner.activeCount).toBe(0) + }) }) describe("cancelTasksByParent", () => { @@ -164,6 +177,18 @@ describe("BackgroundTaskRunner", () => { expect(task.abortTask).toHaveBeenCalledWith(true) expect(customRunner.activeCount).toBe(0) }) + + it("should invoke onTaskTimeout callback when task times out", async () => { + const onTaskTimeout = vi.fn() + const customRunner = new BackgroundTaskRunner(3, 5000, { onTaskTimeout }) + const task = createMockTask("task-1") + customRunner.registerTask(task, "parent-1") + + vi.advanceTimersByTime(5000) + await vi.runAllTimersAsync() + + expect(onTaskTimeout).toHaveBeenCalledWith("task-1", "parent-1") + }) }) describe("dispose", () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 7c51e4428a..b0f5f58ef0 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -80,7 +80,11 @@ import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { Task } from "../task/Task" -import { BackgroundTaskRunner, BACKGROUND_TASK_ALLOWED_TOOLS } from "../task/BackgroundTaskRunner" +import { + BackgroundTaskRunner, + BACKGROUND_TASK_ALLOWED_TOOLS, + BackgroundTaskRunnerCallbacks, +} from "../task/BackgroundTaskRunner" import { webviewMessageHandler } from "./webviewMessageHandler" import type { ClineMessage, TodoItem } from "@roo-code/types" @@ -137,7 +141,14 @@ export class ClineProvider private recentTasksCache?: string[] public readonly taskHistoryStore: TaskHistoryStore private taskHistoryStoreInitialized = false - public readonly backgroundTaskRunner: BackgroundTaskRunner = new BackgroundTaskRunner() + public readonly backgroundTaskRunner: BackgroundTaskRunner = new BackgroundTaskRunner(undefined, undefined, { + onTaskTimeout: (taskId, _parentTaskId) => { + vscode.window.showWarningMessage(`Background task ${taskId} timed out and was cancelled.`) + }, + onTaskError: (taskId, _parentTaskId, error) => { + vscode.window.showWarningMessage(`Background task ${taskId} encountered an error: ${error.message}`) + }, + }) private globalStateWriteThroughTimer: ReturnType | null = null private static readonly GLOBAL_STATE_WRITE_THROUGH_DEBOUNCE_MS = 5000 // 5 seconds private pendingOperations: Map = new Map() @@ -3013,6 +3024,9 @@ export class ClineProvider return } + // Notify the user that the background task finished. + vscode.window.showInformationMessage(`Background task ${taskId} completed.`) + const parentTaskId = info.parentTaskId const currentTask = this.getCurrentTask()