fix(background-agent): keep terminal tasks until parent notification cleanup
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -224,6 +224,12 @@ function stubNotifyParentSession(manager: BackgroundManager): void {
|
|||||||
;(manager as unknown as { notifyParentSession: () => Promise<void> }).notifyParentSession = async () => {}
|
;(manager as unknown as { notifyParentSession: () => Promise<void> }).notifyParentSession = async () => {}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async function flushBackgroundNotifications(): Promise<void> {
|
||||||
|
for (let i = 0; i < 6; i++) {
|
||||||
|
await Promise.resolve()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
function createToastRemoveTaskTracker(): { removeTaskCalls: string[]; resetToastManager: () => void } {
|
function createToastRemoveTaskTracker(): { removeTaskCalls: string[]; resetToastManager: () => void } {
|
||||||
_resetTaskToastManagerForTesting()
|
_resetTaskToastManagerForTesting()
|
||||||
const toastManager = initTaskToastManager({
|
const toastManager = initTaskToastManager({
|
||||||
@@ -1306,11 +1312,20 @@ describe("BackgroundManager.tryCompleteTask", () => {
|
|||||||
expect(abortedSessionIDs).toEqual(["session-1"])
|
expect(abortedSessionIDs).toEqual(["session-1"])
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should clean pendingByParent even when notifyParentSession throws", async () => {
|
test("should clean pendingByParent even when promptAsync notification fails", async () => {
|
||||||
// given
|
// given
|
||||||
;(manager as unknown as { notifyParentSession: () => Promise<void> }).notifyParentSession = async () => {
|
const client = {
|
||||||
throw new Error("notify failed")
|
session: {
|
||||||
|
prompt: async () => ({}),
|
||||||
|
promptAsync: async () => {
|
||||||
|
throw new Error("notify failed")
|
||||||
|
},
|
||||||
|
abort: async () => ({}),
|
||||||
|
messages: async () => ({ data: [] }),
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
manager.shutdown()
|
||||||
|
manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput)
|
||||||
|
|
||||||
const task: BackgroundTask = {
|
const task: BackgroundTask = {
|
||||||
id: "task-pending-cleanup",
|
id: "task-pending-cleanup",
|
||||||
@@ -1424,7 +1439,7 @@ describe("BackgroundManager.tryCompleteTask", () => {
|
|||||||
// then
|
// then
|
||||||
expect(rejectedCount).toBe(0)
|
expect(rejectedCount).toBe(0)
|
||||||
expect(promptBodies.length).toBe(2)
|
expect(promptBodies.length).toBe(2)
|
||||||
expect(promptBodies.some((b) => b.noReply === false)).toBe(true)
|
expect(promptBodies.filter((body) => body.noReply === false)).toHaveLength(1)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -1932,7 +1947,6 @@ describe("BackgroundManager - Non-blocking Queue Integration", () => {
|
|||||||
test("should cancel running task and release concurrency", async () => {
|
test("should cancel running task and release concurrency", async () => {
|
||||||
// given
|
// given
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
stubNotifyParentSession(manager)
|
|
||||||
|
|
||||||
const concurrencyManager = getConcurrencyManager(manager)
|
const concurrencyManager = getConcurrencyManager(manager)
|
||||||
const concurrencyKey = "test-provider/test-model"
|
const concurrencyKey = "test-provider/test-model"
|
||||||
@@ -2890,7 +2904,7 @@ describe("BackgroundManager.shutdown session abort", () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
describe("BackgroundManager.handleEvent - session.deleted cascade", () => {
|
describe("BackgroundManager.handleEvent - session.deleted cascade", () => {
|
||||||
test("should cancel descendant tasks when parent session is deleted", () => {
|
test("should cancel descendant tasks and keep them until delayed cleanup", async () => {
|
||||||
// given
|
// given
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
const parentSessionID = "session-parent"
|
const parentSessionID = "session-parent"
|
||||||
@@ -2937,21 +2951,26 @@ describe("BackgroundManager.handleEvent - session.deleted cascade", () => {
|
|||||||
properties: { info: { id: parentSessionID } },
|
properties: { info: { id: parentSessionID } },
|
||||||
})
|
})
|
||||||
|
|
||||||
|
await flushBackgroundNotifications()
|
||||||
|
|
||||||
// then
|
// then
|
||||||
expect(taskMap.has(childTask.id)).toBe(false)
|
expect(taskMap.has(childTask.id)).toBe(true)
|
||||||
expect(taskMap.has(siblingTask.id)).toBe(false)
|
expect(taskMap.has(siblingTask.id)).toBe(true)
|
||||||
expect(taskMap.has(grandchildTask.id)).toBe(false)
|
expect(taskMap.has(grandchildTask.id)).toBe(true)
|
||||||
expect(taskMap.has(unrelatedTask.id)).toBe(true)
|
expect(taskMap.has(unrelatedTask.id)).toBe(true)
|
||||||
expect(childTask.status).toBe("cancelled")
|
expect(childTask.status).toBe("cancelled")
|
||||||
expect(siblingTask.status).toBe("cancelled")
|
expect(siblingTask.status).toBe("cancelled")
|
||||||
expect(grandchildTask.status).toBe("cancelled")
|
expect(grandchildTask.status).toBe("cancelled")
|
||||||
expect(pendingByParent.get(parentSessionID)).toBeUndefined()
|
expect(pendingByParent.get(parentSessionID)).toBeUndefined()
|
||||||
expect(pendingByParent.get("session-child")).toBeUndefined()
|
expect(pendingByParent.get("session-child")).toBeUndefined()
|
||||||
|
expect(getCompletionTimers(manager).has(childTask.id)).toBe(true)
|
||||||
|
expect(getCompletionTimers(manager).has(siblingTask.id)).toBe(true)
|
||||||
|
expect(getCompletionTimers(manager).has(grandchildTask.id)).toBe(true)
|
||||||
|
|
||||||
manager.shutdown()
|
manager.shutdown()
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should remove tasks from toast manager when session is deleted", () => {
|
test("should remove cancelled tasks from toast manager while preserving delayed cleanup", async () => {
|
||||||
//#given
|
//#given
|
||||||
const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker()
|
const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker()
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
@@ -2980,9 +2999,13 @@ describe("BackgroundManager.handleEvent - session.deleted cascade", () => {
|
|||||||
properties: { info: { id: parentSessionID } },
|
properties: { info: { id: parentSessionID } },
|
||||||
})
|
})
|
||||||
|
|
||||||
|
await flushBackgroundNotifications()
|
||||||
|
|
||||||
//#then
|
//#then
|
||||||
expect(removeTaskCalls).toContain(childTask.id)
|
expect(removeTaskCalls).toContain(childTask.id)
|
||||||
expect(removeTaskCalls).toContain(grandchildTask.id)
|
expect(removeTaskCalls).toContain(grandchildTask.id)
|
||||||
|
expect(getCompletionTimers(manager).has(childTask.id)).toBe(true)
|
||||||
|
expect(getCompletionTimers(manager).has(grandchildTask.id)).toBe(true)
|
||||||
|
|
||||||
manager.shutdown()
|
manager.shutdown()
|
||||||
resetToastManager()
|
resetToastManager()
|
||||||
@@ -3045,7 +3068,7 @@ describe("BackgroundManager.handleEvent - session.error", () => {
|
|||||||
return task
|
return task
|
||||||
}
|
}
|
||||||
|
|
||||||
test("sets task to error, releases concurrency, and cleans up", async () => {
|
test("sets task to error, releases concurrency, and keeps it until delayed cleanup", async () => {
|
||||||
//#given
|
//#given
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
const concurrencyManager = getConcurrencyManager(manager)
|
const concurrencyManager = getConcurrencyManager(manager)
|
||||||
@@ -3078,18 +3101,21 @@ describe("BackgroundManager.handleEvent - session.error", () => {
|
|||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
|
||||||
|
await flushBackgroundNotifications()
|
||||||
|
|
||||||
//#then
|
//#then
|
||||||
expect(task.status).toBe("error")
|
expect(task.status).toBe("error")
|
||||||
expect(task.error).toBe("Model not found: kimi-for-coding/k2p5.")
|
expect(task.error).toBe("Model not found: kimi-for-coding/k2p5.")
|
||||||
expect(task.completedAt).toBeInstanceOf(Date)
|
expect(task.completedAt).toBeInstanceOf(Date)
|
||||||
expect(concurrencyManager.getCount(concurrencyKey)).toBe(0)
|
expect(concurrencyManager.getCount(concurrencyKey)).toBe(0)
|
||||||
expect(getTaskMap(manager).has(task.id)).toBe(false)
|
expect(getTaskMap(manager).has(task.id)).toBe(true)
|
||||||
expect(getPendingByParent(manager).get(task.parentSessionID)).toBeUndefined()
|
expect(getPendingByParent(manager).get(task.parentSessionID)).toBeUndefined()
|
||||||
|
expect(getCompletionTimers(manager).has(task.id)).toBe(true)
|
||||||
|
|
||||||
manager.shutdown()
|
manager.shutdown()
|
||||||
})
|
})
|
||||||
|
|
||||||
test("removes errored task from toast manager", () => {
|
test("should remove errored task from toast manager while preserving delayed cleanup", async () => {
|
||||||
//#given
|
//#given
|
||||||
const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker()
|
const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker()
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
@@ -3111,8 +3137,11 @@ describe("BackgroundManager.handleEvent - session.error", () => {
|
|||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
|
||||||
|
await flushBackgroundNotifications()
|
||||||
|
|
||||||
//#then
|
//#then
|
||||||
expect(removeTaskCalls).toContain(task.id)
|
expect(removeTaskCalls).toContain(task.id)
|
||||||
|
expect(getCompletionTimers(manager).has(task.id)).toBe(true)
|
||||||
|
|
||||||
manager.shutdown()
|
manager.shutdown()
|
||||||
resetToastManager()
|
resetToastManager()
|
||||||
@@ -3518,7 +3547,7 @@ describe("BackgroundManager.completionTimers - Memory Leak Fix", () => {
|
|||||||
expect(completionTimers.size).toBe(0)
|
expect(completionTimers.size).toBe(0)
|
||||||
})
|
})
|
||||||
|
|
||||||
test("should cancel timer when task is deleted via session.deleted", () => {
|
test("should preserve cleanup timer when terminal task session is deleted", () => {
|
||||||
// given
|
// given
|
||||||
const manager = createBackgroundManager()
|
const manager = createBackgroundManager()
|
||||||
const task: BackgroundTask = {
|
const task: BackgroundTask = {
|
||||||
@@ -3547,7 +3576,7 @@ describe("BackgroundManager.completionTimers - Memory Leak Fix", () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
// then
|
// then
|
||||||
expect(completionTimers.has(task.id)).toBe(false)
|
expect(completionTimers.has(task.id)).toBe(true)
|
||||||
|
|
||||||
manager.shutdown()
|
manager.shutdown()
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -390,7 +390,6 @@ export class BackgroundManager {
|
|||||||
}).catch(() => {})
|
}).catch(() => {})
|
||||||
|
|
||||||
this.markForNotification(existingTask)
|
this.markForNotification(existingTask)
|
||||||
this.cleanupPendingByParent(existingTask)
|
|
||||||
this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => {
|
this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => {
|
||||||
log("[background-agent] Failed to notify on error:", err)
|
log("[background-agent] Failed to notify on error:", err)
|
||||||
})
|
})
|
||||||
@@ -661,7 +660,6 @@ export class BackgroundManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
this.markForNotification(existingTask)
|
this.markForNotification(existingTask)
|
||||||
this.cleanupPendingByParent(existingTask)
|
|
||||||
this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => {
|
this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => {
|
||||||
log("[background-agent] Failed to notify on resume error:", err)
|
log("[background-agent] Failed to notify on resume error:", err)
|
||||||
})
|
})
|
||||||
@@ -804,16 +802,14 @@ export class BackgroundManager {
|
|||||||
this.idleDeferralTimers.delete(task.id)
|
this.idleDeferralTimers.delete(task.id)
|
||||||
}
|
}
|
||||||
|
|
||||||
this.cleanupPendingByParent(task)
|
|
||||||
this.tasks.delete(task.id)
|
|
||||||
this.clearNotificationsForTask(task.id)
|
|
||||||
const toastManager = getTaskToastManager()
|
|
||||||
if (toastManager) {
|
|
||||||
toastManager.removeTask(task.id)
|
|
||||||
}
|
|
||||||
if (task.sessionID) {
|
if (task.sessionID) {
|
||||||
subagentSessions.delete(task.sessionID)
|
SessionCategoryRegistry.remove(task.sessionID)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
this.markForNotification(task)
|
||||||
|
this.enqueueNotificationForParent(task.parentSessionID, () => this.notifyParentSession(task)).catch(err => {
|
||||||
|
log("[background-agent] Error in notifyParentSession for errored task:", { taskId: task.id, error: err })
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
if (event.type === "session.deleted") {
|
if (event.type === "session.deleted") {
|
||||||
@@ -834,47 +830,30 @@ export class BackgroundManager {
|
|||||||
|
|
||||||
if (tasksToCancel.size === 0) return
|
if (tasksToCancel.size === 0) return
|
||||||
|
|
||||||
|
const deletedSessionIDs = new Set<string>([sessionID])
|
||||||
|
for (const task of tasksToCancel.values()) {
|
||||||
|
if (task.sessionID) {
|
||||||
|
deletedSessionIDs.add(task.sessionID)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
for (const task of tasksToCancel.values()) {
|
for (const task of tasksToCancel.values()) {
|
||||||
if (task.status === "running" || task.status === "pending") {
|
if (task.status === "running" || task.status === "pending") {
|
||||||
void this.cancelTask(task.id, {
|
void this.cancelTask(task.id, {
|
||||||
source: "session.deleted",
|
source: "session.deleted",
|
||||||
reason: "Session deleted",
|
reason: "Session deleted",
|
||||||
skipNotification: true,
|
}).then(() => {
|
||||||
|
if (deletedSessionIDs.has(task.parentSessionID)) {
|
||||||
|
this.pendingNotifications.delete(task.parentSessionID)
|
||||||
|
}
|
||||||
}).catch(err => {
|
}).catch(err => {
|
||||||
|
if (deletedSessionIDs.has(task.parentSessionID)) {
|
||||||
|
this.pendingNotifications.delete(task.parentSessionID)
|
||||||
|
}
|
||||||
log("[background-agent] Failed to cancel task on session.deleted:", { taskId: task.id, error: err })
|
log("[background-agent] Failed to cancel task on session.deleted:", { taskId: task.id, error: err })
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
const existingTimer = this.completionTimers.get(task.id)
|
|
||||||
if (existingTimer) {
|
|
||||||
clearTimeout(existingTimer)
|
|
||||||
this.completionTimers.delete(task.id)
|
|
||||||
}
|
|
||||||
|
|
||||||
const idleTimer = this.idleDeferralTimers.get(task.id)
|
|
||||||
if (idleTimer) {
|
|
||||||
clearTimeout(idleTimer)
|
|
||||||
this.idleDeferralTimers.delete(task.id)
|
|
||||||
}
|
|
||||||
|
|
||||||
this.cleanupPendingByParent(task)
|
|
||||||
this.tasks.delete(task.id)
|
|
||||||
this.clearNotificationsForTask(task.id)
|
|
||||||
const toastManager = getTaskToastManager()
|
|
||||||
if (toastManager) {
|
|
||||||
toastManager.removeTask(task.id)
|
|
||||||
}
|
|
||||||
if (task.sessionID) {
|
|
||||||
subagentSessions.delete(task.sessionID)
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
for (const task of tasksToCancel.values()) {
|
|
||||||
if (task.parentSessionID) {
|
|
||||||
this.pendingNotifications.delete(task.parentSessionID)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
SessionCategoryRegistry.remove(sessionID)
|
SessionCategoryRegistry.remove(sessionID)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1094,8 +1073,6 @@ export class BackgroundManager {
|
|||||||
this.idleDeferralTimers.delete(task.id)
|
this.idleDeferralTimers.delete(task.id)
|
||||||
}
|
}
|
||||||
|
|
||||||
this.cleanupPendingByParent(task)
|
|
||||||
|
|
||||||
if (abortSession && task.sessionID) {
|
if (abortSession && task.sessionID) {
|
||||||
this.client.session.abort({
|
this.client.session.abort({
|
||||||
path: { id: task.sessionID },
|
path: { id: task.sessionID },
|
||||||
@@ -1202,9 +1179,6 @@ export class BackgroundManager {
|
|||||||
|
|
||||||
this.markForNotification(task)
|
this.markForNotification(task)
|
||||||
|
|
||||||
// Ensure pending tracking is cleaned up even if notification fails
|
|
||||||
this.cleanupPendingByParent(task)
|
|
||||||
|
|
||||||
const idleTimer = this.idleDeferralTimers.get(task.id)
|
const idleTimer = this.idleDeferralTimers.get(task.id)
|
||||||
if (idleTimer) {
|
if (idleTimer) {
|
||||||
clearTimeout(idleTimer)
|
clearTimeout(idleTimer)
|
||||||
@@ -1260,7 +1234,10 @@ export class BackgroundManager {
|
|||||||
this.pendingByParent.delete(task.parentSessionID)
|
this.pendingByParent.delete(task.parentSessionID)
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
allComplete = true
|
remainingCount = Array.from(this.tasks.values())
|
||||||
|
.filter(t => t.parentSessionID === task.parentSessionID && t.id !== task.id && (t.status === "running" || t.status === "pending"))
|
||||||
|
.length
|
||||||
|
allComplete = remainingCount === 0
|
||||||
}
|
}
|
||||||
|
|
||||||
const completedTasks = allComplete
|
const completedTasks = allComplete
|
||||||
@@ -1268,7 +1245,13 @@ export class BackgroundManager {
|
|||||||
.filter(t => t.parentSessionID === task.parentSessionID && t.status !== "running" && t.status !== "pending")
|
.filter(t => t.parentSessionID === task.parentSessionID && t.status !== "running" && t.status !== "pending")
|
||||||
: []
|
: []
|
||||||
|
|
||||||
const statusText = task.status === "completed" ? "COMPLETED" : task.status === "interrupt" ? "INTERRUPTED" : "CANCELLED"
|
const statusText = task.status === "completed"
|
||||||
|
? "COMPLETED"
|
||||||
|
: task.status === "interrupt"
|
||||||
|
? "INTERRUPTED"
|
||||||
|
: task.status === "error"
|
||||||
|
? "ERROR"
|
||||||
|
: "CANCELLED"
|
||||||
const errorInfo = task.error ? `\n**Error:** ${task.error}` : ""
|
const errorInfo = task.error ? `\n**Error:** ${task.error}` : ""
|
||||||
|
|
||||||
let notification: string
|
let notification: string
|
||||||
|
|||||||
Reference in New Issue
Block a user