fix(background-agent): fail cancellation when abort fails
This commit is contained in:
@@ -11,11 +11,14 @@ afterEach(() => {
|
|||||||
while (managersToShutdown.length > 0) managersToShutdown.pop()?.shutdown()
|
while (managersToShutdown.length > 0) managersToShutdown.pop()?.shutdown()
|
||||||
})
|
})
|
||||||
|
|
||||||
function createBackgroundManager(config?: { defaultConcurrency?: number }): BackgroundManager {
|
function createBackgroundManager(
|
||||||
|
config?: { defaultConcurrency?: number },
|
||||||
|
abortSession: () => Promise<unknown> = async () => ({ data: true }),
|
||||||
|
): BackgroundManager {
|
||||||
const directory = tmpdir()
|
const directory = tmpdir()
|
||||||
const client = { session: {} as PluginInput["client"]["session"] } as PluginInput["client"]
|
const client = { session: {} as PluginInput["client"]["session"] } as PluginInput["client"]
|
||||||
|
|
||||||
Reflect.set(client.session, "abort", async () => ({ data: true }))
|
Reflect.set(client.session, "abort", abortSession)
|
||||||
Reflect.set(client.session, "create", async () => ({ data: { id: `session-${crypto.randomUUID().slice(0, 8)}` } }))
|
Reflect.set(client.session, "create", async () => ({ data: { id: `session-${crypto.randomUUID().slice(0, 8)}` } }))
|
||||||
Reflect.set(client.session, "get", async () => ({ data: { directory } }))
|
Reflect.set(client.session, "get", async () => ({ data: { directory } }))
|
||||||
Reflect.set(client.session, "messages", async () => ({ data: [] }))
|
Reflect.set(client.session, "messages", async () => ({ data: [] }))
|
||||||
@@ -111,6 +114,31 @@ describe("BackgroundManager.cancelTask cleanup", () => {
|
|||||||
expect(manager.getTask(task.id)?.sessionId).toBe(task.sessionId)
|
expect(manager.getTask(task.id)?.sessionId).toBe(task.sessionId)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("#given running task abort returns SDK error #when cancelTask runs #then cancellation fails and task stays running", async () => {
|
||||||
|
// given
|
||||||
|
const manager = createBackgroundManager(undefined, async () => ({ error: { message: "session still active" } }))
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "task-abort-error",
|
||||||
|
parentSessionId: "parent-session-abort-error",
|
||||||
|
sessionId: "session-abort-error",
|
||||||
|
})
|
||||||
|
|
||||||
|
getTaskMap(manager).set(task.id, task)
|
||||||
|
getPendingByParent(manager).set(task.parentSessionId, new Set([task.id]))
|
||||||
|
|
||||||
|
// when
|
||||||
|
const cancelled = await manager.cancelTask(task.id, {
|
||||||
|
skipNotification: true,
|
||||||
|
source: "test",
|
||||||
|
})
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(cancelled).toBe(false)
|
||||||
|
expect(task.status).toBe("running")
|
||||||
|
expect(getTaskMap(manager).get(task.id)).toBe(task)
|
||||||
|
expect(getPendingByParent(manager).get(task.parentSessionId)).toEqual(new Set([task.id]))
|
||||||
|
})
|
||||||
|
|
||||||
test("#given a running task #when cancelTask called with skipNotification=false #then task is also eventually removed", async () => {
|
test("#given a running task #when cancelTask called with skipNotification=false #then task is also eventually removed", async () => {
|
||||||
// given
|
// given
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
|
|||||||
@@ -301,14 +301,21 @@ export class BackgroundManager {
|
|||||||
this.registerProcessCleanup()
|
this.registerProcessCleanup()
|
||||||
}
|
}
|
||||||
|
|
||||||
private async abortSessionWithLogging(sessionID: string, reason: string): Promise<void> {
|
private async abortSessionWithLogging(sessionID: string, reason: string): Promise<boolean> {
|
||||||
try {
|
try {
|
||||||
await abortWithTimeout(this.client, sessionID)
|
const aborted = await abortWithTimeout(this.client, sessionID)
|
||||||
|
if (!aborted) {
|
||||||
|
log(`[background-agent] Session abort did not complete during ${reason}:`, {
|
||||||
|
sessionID,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
return aborted
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
log(`[background-agent] Failed to abort session during ${reason}:`, {
|
log(`[background-agent] Failed to abort session during ${reason}:`, {
|
||||||
sessionID,
|
sessionID,
|
||||||
error,
|
error,
|
||||||
})
|
})
|
||||||
|
return false
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2179,6 +2186,13 @@ The task was re-queued on a fallback model after a retryable failure.
|
|||||||
}
|
}
|
||||||
|
|
||||||
const wasRunning = task.status === "running"
|
const wasRunning = task.status === "running"
|
||||||
|
if (wasRunning && abortSession && task.sessionId) {
|
||||||
|
const aborted = await this.abortSessionWithLogging(task.sessionId, `task cancellation (${source})`)
|
||||||
|
if (!aborted) return false
|
||||||
|
|
||||||
|
clearDelegatedChildSessionBootstrap(task.sessionId)
|
||||||
|
SessionCategoryRegistry.remove(task.sessionId)
|
||||||
|
}
|
||||||
if (task.currentAttemptID) {
|
if (task.currentAttemptID) {
|
||||||
finalizeAttempt(task, task.currentAttemptID, "cancelled", reason)
|
finalizeAttempt(task, task.currentAttemptID, "cancelled", reason)
|
||||||
} else {
|
} else {
|
||||||
@@ -2210,14 +2224,6 @@ The task was re-queued on a fallback model after a retryable failure.
|
|||||||
this.idleDeferralTimers.delete(task.id)
|
this.idleDeferralTimers.delete(task.id)
|
||||||
}
|
}
|
||||||
|
|
||||||
if (abortSession && task.sessionId) {
|
|
||||||
// Awaited to prevent dangling promise during subagent teardown (Bun/WebKit SIGABRT)
|
|
||||||
await this.abortSessionWithLogging(task.sessionId, `task cancellation (${source})`)
|
|
||||||
|
|
||||||
clearDelegatedChildSessionBootstrap(task.sessionId)
|
|
||||||
SessionCategoryRegistry.remove(task.sessionId)
|
|
||||||
}
|
|
||||||
|
|
||||||
removeTaskToastTracking(task.id)
|
removeTaskToastTracking(task.id)
|
||||||
|
|
||||||
// Update continuation marker for CLI run mode
|
// Update continuation marker for CLI run mode
|
||||||
|
|||||||
@@ -408,6 +408,24 @@ describe("background_cancel", () => {
|
|||||||
expect(output).toContain("Task cancelled successfully")
|
expect(output).toContain("Task cancelled successfully")
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test("reports an error when manager cannot cancel a running task", async () => {
|
||||||
|
// #given
|
||||||
|
const task = createTask({ status: "running" })
|
||||||
|
const manager = unsafeTestValue<BackgroundManager>({
|
||||||
|
getTask: (id: string) => (id === task.id ? task : undefined),
|
||||||
|
getAllDescendantTasks: () => [task],
|
||||||
|
cancelTask: async () => false,
|
||||||
|
})
|
||||||
|
const client = { session: { abort: async () => ({}) } } as BackgroundCancelClient
|
||||||
|
const tool = createBackgroundCancel(manager, client)
|
||||||
|
|
||||||
|
// #when
|
||||||
|
const output = await tool.execute({ taskId: task.id }, mockContext)
|
||||||
|
|
||||||
|
// #then
|
||||||
|
expect(output).toContain(`[ERROR] Failed to cancel task: ${task.id}`)
|
||||||
|
})
|
||||||
|
|
||||||
test("cancels all running or pending tasks", async () => {
|
test("cancels all running or pending tasks", async () => {
|
||||||
// #given
|
// #given
|
||||||
const taskA = createTask({ id: "task-a", status: "running" })
|
const taskA = createTask({ id: "task-a", status: "running" })
|
||||||
|
|||||||
Reference in New Issue
Block a user