mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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)
This commit is contained in:
parent
903ff8c3f3
commit
4b2b91fcdc
4 changed files with 78 additions and 7 deletions
|
|
@ -35,17 +35,31 @@ export interface BackgroundTaskInfo {
|
|||
timeoutHandle: ReturnType<typeof setTimeout>
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<string, BackgroundTaskInfo> = 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<void> {
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1338,6 +1338,12 @@ export class Task extends EventEmitter<TaskEvents> 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 })
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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<typeof setTimeout> | null = null
|
||||
private static readonly GLOBAL_STATE_WRITE_THROUGH_DEBOUNCE_MS = 5000 // 5 seconds
|
||||
private pendingOperations: Map<string, PendingEditOperation> = 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()
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue