From 8c5f9b8082bff0cec69f254a4eac8ea50b2c660c Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 11 Mar 2026 18:20:13 +0900 Subject: [PATCH 1/2] fix(background-agent): skip terminal tasks during stale pruning Prevent TTL pruning from deleting terminal tasks before delayed notification cleanup runs. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../background-agent/task-poller.test.ts | 37 ++++++++++++++++++- src/features/background-agent/task-poller.ts | 9 +++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/features/background-agent/task-poller.test.ts b/src/features/background-agent/task-poller.test.ts index 521a85904..6da3ffb0e 100644 --- a/src/features/background-agent/task-poller.test.ts +++ b/src/features/background-agent/task-poller.test.ts @@ -1,4 +1,5 @@ -import { describe, it, expect, mock } from "bun:test" +declare const require: (name: string) => any +const { describe, it, expect, mock } = require("bun:test") import { checkAndInterruptStaleTasks, pruneStaleTasksAndNotifications } from "./task-poller" import type { BackgroundTask } from "./types" @@ -447,4 +448,38 @@ describe("pruneStaleTasksAndNotifications", () => { //#then expect(pruned).toContain("old-task") }) + + it("should skip terminal tasks even when they exceeded TTL", () => { + //#given + const tasks = new Map() + const oldStartedAt = new Date(Date.now() - 31 * 60 * 1000) + const terminalStatuses: BackgroundTask["status"][] = ["completed", "error", "cancelled", "interrupt"] + + for (const status of terminalStatuses) { + tasks.set(status, { + id: status, + parentSessionID: "parent", + parentMessageID: "msg", + description: status, + prompt: status, + agent: "explore", + status, + startedAt: oldStartedAt, + completedAt: new Date(), + }) + } + + const pruned: string[] = [] + + //#when + pruneStaleTasksAndNotifications({ + tasks, + notifications: new Map(), + onTaskPruned: (taskId) => pruned.push(taskId), + }) + + //#then + expect(pruned).toEqual([]) + expect(Array.from(tasks.keys())).toEqual(terminalStatuses) + }) }) diff --git a/src/features/background-agent/task-poller.ts b/src/features/background-agent/task-poller.ts index c27cd4874..bc4f049a4 100644 --- a/src/features/background-agent/task-poller.ts +++ b/src/features/background-agent/task-poller.ts @@ -13,6 +13,13 @@ import { } from "./constants" import { removeTaskToastTracking } from "./remove-task-toast-tracking" +const TERMINAL_TASK_STATUSES = new Set([ + "completed", + "error", + "cancelled", + "interrupt", +]) + export function pruneStaleTasksAndNotifications(args: { tasks: Map notifications: Map @@ -22,6 +29,8 @@ export function pruneStaleTasksAndNotifications(args: { const now = Date.now() for (const [taskId, task] of tasks.entries()) { + if (TERMINAL_TASK_STATUSES.has(task.status)) continue + const timestamp = task.status === "pending" ? task.queuedAt?.getTime() : task.startedAt?.getTime() From e3b17da4bdcfac88bf1094abffa41e64d7ccc53d Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 11 Mar 2026 18:20:20 +0900 Subject: [PATCH 2/2] fix(background-agent): preserve terminal tasks until notification cleanup Route terminal task cleanup through parent notifications so cancelled and errored tasks stay visible until delayed cleanup finishes. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- src/features/background-agent/manager.test.ts | 109 ++++++++++++++--- src/features/background-agent/manager.ts | 111 ++++++++---------- 2 files changed, 145 insertions(+), 75 deletions(-) diff --git a/src/features/background-agent/manager.test.ts b/src/features/background-agent/manager.test.ts index b3663916e..d05b22c41 100644 --- a/src/features/background-agent/manager.test.ts +++ b/src/features/background-agent/manager.test.ts @@ -224,6 +224,12 @@ function stubNotifyParentSession(manager: BackgroundManager): void { ;(manager as unknown as { notifyParentSession: () => Promise }).notifyParentSession = async () => {} } +async function flushBackgroundNotifications(): Promise { + for (let i = 0; i < 6; i++) { + await Promise.resolve() + } +} + function createToastRemoveTaskTracker(): { removeTaskCalls: string[]; resetToastManager: () => void } { _resetTaskToastManagerForTesting() const toastManager = initTaskToastManager({ @@ -1306,11 +1312,20 @@ describe("BackgroundManager.tryCompleteTask", () => { 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 - ;(manager as unknown as { notifyParentSession: () => Promise }).notifyParentSession = async () => { - throw new Error("notify failed") + const client = { + 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 = { id: "task-pending-cleanup", @@ -1518,7 +1533,7 @@ describe("BackgroundManager.tryCompleteTask", () => { // then expect(rejectedCount).toBe(0) expect(promptBodies.length).toBe(2) - expect(promptBodies.some((b) => b.noReply === false)).toBe(true) + expect(promptBodies.filter((body) => body.noReply === false)).toHaveLength(1) }) }) @@ -2026,7 +2041,6 @@ describe("BackgroundManager - Non-blocking Queue Integration", () => { test("should cancel running task and release concurrency", async () => { // given const manager = createBackgroundManager() - stubNotifyParentSession(manager) const concurrencyManager = getConcurrencyManager(manager) const concurrencyKey = "test-provider/test-model" @@ -2984,7 +2998,7 @@ describe("BackgroundManager.shutdown session abort", () => { }) 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 const manager = createBackgroundManager() const parentSessionID = "session-parent" @@ -3031,21 +3045,26 @@ describe("BackgroundManager.handleEvent - session.deleted cascade", () => { properties: { info: { id: parentSessionID } }, }) + await flushBackgroundNotifications() + // then - expect(taskMap.has(childTask.id)).toBe(false) - expect(taskMap.has(siblingTask.id)).toBe(false) - expect(taskMap.has(grandchildTask.id)).toBe(false) + expect(taskMap.has(childTask.id)).toBe(true) + expect(taskMap.has(siblingTask.id)).toBe(true) + expect(taskMap.has(grandchildTask.id)).toBe(true) expect(taskMap.has(unrelatedTask.id)).toBe(true) expect(childTask.status).toBe("cancelled") expect(siblingTask.status).toBe("cancelled") expect(grandchildTask.status).toBe("cancelled") expect(pendingByParent.get(parentSessionID)).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() }) - 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 const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker() const manager = createBackgroundManager() @@ -3074,9 +3093,13 @@ describe("BackgroundManager.handleEvent - session.deleted cascade", () => { properties: { info: { id: parentSessionID } }, }) + await flushBackgroundNotifications() + //#then expect(removeTaskCalls).toContain(childTask.id) expect(removeTaskCalls).toContain(grandchildTask.id) + expect(getCompletionTimers(manager).has(childTask.id)).toBe(true) + expect(getCompletionTimers(manager).has(grandchildTask.id)).toBe(true) manager.shutdown() resetToastManager() @@ -3139,7 +3162,7 @@ describe("BackgroundManager.handleEvent - session.error", () => { 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 const manager = createBackgroundManager() const concurrencyManager = getConcurrencyManager(manager) @@ -3172,18 +3195,21 @@ describe("BackgroundManager.handleEvent - session.error", () => { }, }) + await flushBackgroundNotifications() + //#then expect(task.status).toBe("error") expect(task.error).toBe("Model not found: kimi-for-coding/k2p5.") expect(task.completedAt).toBeInstanceOf(Date) 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(getCompletionTimers(manager).has(task.id)).toBe(true) manager.shutdown() }) - test("removes errored task from toast manager", () => { + test("should remove errored task from toast manager while preserving delayed cleanup", async () => { //#given const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker() const manager = createBackgroundManager() @@ -3205,8 +3231,11 @@ describe("BackgroundManager.handleEvent - session.error", () => { }, }) + await flushBackgroundNotifications() + //#then expect(removeTaskCalls).toContain(task.id) + expect(getCompletionTimers(manager).has(task.id)).toBe(true) manager.shutdown() resetToastManager() @@ -3489,7 +3518,7 @@ describe("BackgroundManager.pruneStaleTasksAndNotifications - removes pruned tas manager.shutdown() }) - test("removes stale task from toast manager", () => { + test("removes stale task from toast manager", async () => { //#given const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker() const manager = createBackgroundManager() @@ -3504,6 +3533,7 @@ describe("BackgroundManager.pruneStaleTasksAndNotifications - removes pruned tas //#when pruneStaleTasksAndNotificationsForTest(manager) + await flushBackgroundNotifications() //#then expect(removeTaskCalls).toContain(staleTask.id) @@ -3511,6 +3541,53 @@ describe("BackgroundManager.pruneStaleTasksAndNotifications - removes pruned tas manager.shutdown() resetToastManager() }) + + test("keeps stale task until notification cleanup after notifying parent", async () => { + //#given + const notifications: string[] = [] + const { removeTaskCalls, resetToastManager } = createToastRemoveTaskTracker() + const client = { + session: { + prompt: async () => ({}), + promptAsync: async (args: { path: { id: string }; body: Record & { noReply?: boolean; parts?: unknown[] } }) => { + const firstPart = args.body.parts?.[0] + if (firstPart && typeof firstPart === "object" && "text" in firstPart && typeof firstPart.text === "string") { + notifications.push(firstPart.text) + } + return {} + }, + abort: async () => ({}), + messages: async () => ({ data: [] }), + }, + } + const manager = new BackgroundManager({ client, directory: tmpdir() } as unknown as PluginInput) + const staleTask = createMockTask({ + id: "task-stale-notify-cleanup", + sessionID: "session-stale-notify-cleanup", + parentSessionID: "parent-stale-notify-cleanup", + status: "running", + startedAt: new Date(Date.now() - 31 * 60 * 1000), + }) + getTaskMap(manager).set(staleTask.id, staleTask) + getPendingByParent(manager).set(staleTask.parentSessionID, new Set([staleTask.id])) + + //#when + pruneStaleTasksAndNotificationsForTest(manager) + await flushBackgroundNotifications() + + //#then + const retainedTask = getTaskMap(manager).get(staleTask.id) + expect(retainedTask?.status).toBe("error") + expect(getTaskMap(manager).has(staleTask.id)).toBe(true) + expect(notifications).toHaveLength(1) + expect(notifications[0]).toContain("[ALL BACKGROUND TASKS COMPLETE]") + expect(notifications[0]).toContain(staleTask.description) + expect(getCompletionTimers(manager).has(staleTask.id)).toBe(true) + expect(removeTaskCalls).toContain(staleTask.id) + + manager.shutdown() + resetToastManager() + }) }) describe("BackgroundManager.completionTimers - Memory Leak Fix", () => { @@ -3614,7 +3691,7 @@ describe("BackgroundManager.completionTimers - Memory Leak Fix", () => { 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 const manager = createBackgroundManager() const task: BackgroundTask = { @@ -3643,7 +3720,7 @@ describe("BackgroundManager.completionTimers - Memory Leak Fix", () => { }) // then - expect(completionTimers.has(task.id)).toBe(false) + expect(completionTimers.has(task.id)).toBe(true) manager.shutdown() }) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index 6cd60273e..7b58076a6 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -398,7 +398,6 @@ export class BackgroundManager { }).catch(() => {}) this.markForNotification(existingTask) - this.cleanupPendingByParent(existingTask) this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => { log("[background-agent] Failed to notify on error:", err) }) @@ -671,7 +670,6 @@ export class BackgroundManager { } this.markForNotification(existingTask) - this.cleanupPendingByParent(existingTask) this.enqueueNotificationForParent(existingTask.parentSessionID, () => this.notifyParentSession(existingTask)).catch(err => { log("[background-agent] Failed to notify on resume error:", err) }) @@ -814,16 +812,14 @@ export class BackgroundManager { 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) + 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") { @@ -844,45 +840,29 @@ export class BackgroundManager { if (tasksToCancel.size === 0) return + const deletedSessionIDs = new Set([sessionID]) + for (const task of tasksToCancel.values()) { + if (task.sessionID) { + deletedSessionIDs.add(task.sessionID) + } + } + for (const task of tasksToCancel.values()) { if (task.status === "running" || task.status === "pending") { void this.cancelTask(task.id, { source: "session.deleted", reason: "Session deleted", - skipNotification: true, + }).then(() => { + if (deletedSessionIDs.has(task.parentSessionID)) { + this.pendingNotifications.delete(task.parentSessionID) + } }).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 }) }) } - - 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) @@ -1104,8 +1084,6 @@ export class BackgroundManager { this.idleDeferralTimers.delete(task.id) } - this.cleanupPendingByParent(task) - if (abortSession && task.sessionID) { this.client.session.abort({ path: { id: task.sessionID }, @@ -1212,9 +1190,6 @@ export class BackgroundManager { this.markForNotification(task) - // Ensure pending tracking is cleaned up even if notification fails - this.cleanupPendingByParent(task) - const idleTimer = this.idleDeferralTimers.get(task.id) if (idleTimer) { clearTimeout(idleTimer) @@ -1270,7 +1245,10 @@ export class BackgroundManager { this.pendingByParent.delete(task.parentSessionID) } } 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 @@ -1278,7 +1256,13 @@ export class BackgroundManager { .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}` : "" let notification: string @@ -1411,8 +1395,13 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea } const timer = setTimeout(() => { this.completionTimers.delete(taskId) - if (this.tasks.has(taskId)) { + const taskToRemove = this.tasks.get(taskId) + if (taskToRemove) { this.clearNotificationsForTask(taskId) + if (taskToRemove.sessionID) { + subagentSessions.delete(taskToRemove.sessionID) + SessionCategoryRegistry.remove(taskToRemove.sessionID) + } this.tasks.delete(taskId) log("[background-agent] Removed completed task from memory:", taskId) } @@ -1447,12 +1436,22 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea task.status = "error" task.error = errorMessage task.completedAt = new Date() + this.taskHistory.record(task.parentSessionID, { id: task.id, sessionID: task.sessionID, agent: task.agent, description: task.description, status: "error", category: task.category, startedAt: task.startedAt, completedAt: task.completedAt }) if (task.concurrencyKey) { this.concurrencyManager.release(task.concurrencyKey) task.concurrencyKey = undefined } removeTaskToastTracking(task.id) - this.cleanupPendingByParent(task) + const existingTimer = this.completionTimers.get(taskId) + if (existingTimer) { + clearTimeout(existingTimer) + this.completionTimers.delete(taskId) + } + const idleTimer = this.idleDeferralTimers.get(taskId) + if (idleTimer) { + clearTimeout(idleTimer) + this.idleDeferralTimers.delete(taskId) + } if (wasPending) { const key = task.model ? `${task.model.providerID}/${task.model.modelID}` @@ -1468,16 +1467,10 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea } } } - this.clearNotificationsForTask(taskId) - const toastManager = getTaskToastManager() - if (toastManager) { - toastManager.removeTask(taskId) - } - this.tasks.delete(taskId) - 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 stale-pruned task:", { taskId: task.id, error: err }) + }) }, }) }