From c196db2a0e1902a030fbcbab2b61a62858e3448b Mon Sep 17 00:00:00 2001 From: Gladdonilli Date: Tue, 13 Jan 2026 14:32:24 +0800 Subject: [PATCH 1/4] fix(background-agent): address 3 edge cases in task lifecycle - Reset startedAt on resume to prevent immediate false completion - Release concurrency immediately on completion with double-release guard - Clean up pendingByParent on session.deleted to prevent stale entries --- src/features/background-agent/manager.ts | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index f0322e48..b91b0d7c 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -286,6 +286,9 @@ export class BackgroundManager { existingTask.parentMessageID = input.parentMessageID existingTask.parentModel = input.parentModel existingTask.parentAgent = input.parentAgent + // Reset startedAt on resume to prevent immediate completion + // The MIN_IDLE_TIME_MS check uses startedAt, so resumed tasks need fresh timing + existingTask.startedAt = new Date() existingTask.progress = { toolCalls: existingTask.progress?.toolCalls ?? 0, @@ -418,6 +421,11 @@ export class BackgroundManager { task.status = "completed" task.completedAt = new Date() + // Release concurrency immediately on completion + if (task.concurrencyKey) { + this.concurrencyManager.release(task.concurrencyKey) + task.concurrencyKey = undefined // Prevent double-release + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via session.idle event:", task.id) @@ -442,6 +450,15 @@ export class BackgroundManager { if (task.concurrencyKey) { this.concurrencyManager.release(task.concurrencyKey) + task.concurrencyKey = undefined // Prevent double-release + } + // Clean up pendingByParent to prevent stale entries + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } } this.tasks.delete(task.id) this.clearNotificationsForTask(task.id) @@ -753,6 +770,11 @@ try { task.status = "completed" task.completedAt = new Date() + // Release concurrency immediately on completion + if (task.concurrencyKey) { + this.concurrencyManager.release(task.concurrencyKey) + task.concurrencyKey = undefined // Prevent double-release + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via polling:", task.id) @@ -819,6 +841,11 @@ if (lastMessage) { if (!hasIncompleteTodos) { task.status = "completed" task.completedAt = new Date() + // Release concurrency immediately on completion + if (task.concurrencyKey) { + this.concurrencyManager.release(task.concurrencyKey) + task.concurrencyKey = undefined // Prevent double-release + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via stability detection:", task.id) From 129388387bddb66a655d75f41b5eab2bb04fb971 Mon Sep 17 00:00:00 2001 From: Gladdonilli Date: Tue, 13 Jan 2026 14:40:50 +0800 Subject: [PATCH 2/4] fix: address PR review feedback - Add pendingByParent cleanup to ALL completion paths (session.idle, polling, stability) - Add null guard for task.parentSessionID before Map access - Add consistency guard in prune function (set concurrencyKey = undefined) - Remove redundant setTimeout release (already released at completion) --- src/features/background-agent/manager.ts | 44 +++++++++++++++++++++--- 1 file changed, 39 insertions(+), 5 deletions(-) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index b91b0d7c..c2ad078f 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -426,6 +426,16 @@ export class BackgroundManager { this.concurrencyManager.release(task.concurrencyKey) task.concurrencyKey = undefined // Prevent double-release } + // Clean up pendingByParent to prevent stale entries + if (task.parentSessionID) { + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } + } + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via session.idle event:", task.id) @@ -453,11 +463,13 @@ export class BackgroundManager { task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) + if (task.parentSessionID) { + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } } } this.tasks.delete(task.id) @@ -678,6 +690,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea const taskId = task.id setTimeout(() => { + // Concurrency already released at completion - just cleanup notifications and task this.clearNotificationsForTask(taskId) this.tasks.delete(taskId) log("[background-agent] Removed completed task from memory:", taskId) @@ -717,6 +730,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea task.completedAt = new Date() if (task.concurrencyKey) { this.concurrencyManager.release(task.concurrencyKey) + task.concurrencyKey = undefined // Prevent double-release } this.clearNotificationsForTask(taskId) this.tasks.delete(taskId) @@ -775,6 +789,16 @@ try { this.concurrencyManager.release(task.concurrencyKey) task.concurrencyKey = undefined // Prevent double-release } + // Clean up pendingByParent to prevent stale entries + if (task.parentSessionID) { + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } + } + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via polling:", task.id) @@ -846,6 +870,16 @@ if (lastMessage) { this.concurrencyManager.release(task.concurrencyKey) task.concurrencyKey = undefined // Prevent double-release } + // Clean up pendingByParent to prevent stale entries + if (task.parentSessionID) { + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } + } + } this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via stability detection:", task.id) From 5d99e9ab64dc66ec522a758a52d37aa30c0bba9b Mon Sep 17 00:00:00 2001 From: Gladdonilli Date: Tue, 13 Jan 2026 14:48:43 +0800 Subject: [PATCH 3/4] fix: address remaining PR review feedback - Add pendingByParent cleanup in pruneStaleTasksAndNotifications - Add double-release guard in launch error handler (L170) - Add concurrency release in resume error handler (L326) --- src/features/background-agent/manager.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index c2ad078f..369ee15d 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -183,6 +183,7 @@ export class BackgroundManager { existingTask.completedAt = new Date() if (existingTask.concurrencyKey) { this.concurrencyManager.release(existingTask.concurrencyKey) + existingTask.concurrencyKey = undefined // Prevent double-release } this.markForNotification(existingTask) this.notifyParentSession(existingTask).catch(err => { @@ -340,6 +341,11 @@ export class BackgroundManager { const errorMessage = error instanceof Error ? error.message : String(error) existingTask.error = errorMessage existingTask.completedAt = new Date() + // Release concurrency on resume error (matches launch error handler) + if (existingTask.concurrencyKey) { + this.concurrencyManager.release(existingTask.concurrencyKey) + existingTask.concurrencyKey = undefined // Prevent double-release + } this.markForNotification(existingTask) this.notifyParentSession(existingTask).catch(err => { log("[background-agent] Failed to notify on resume error:", err) @@ -732,6 +738,16 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea this.concurrencyManager.release(task.concurrencyKey) task.concurrencyKey = undefined // Prevent double-release } + // Clean up pendingByParent to prevent stale entries + if (task.parentSessionID) { + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } + } + } this.clearNotificationsForTask(taskId) this.tasks.delete(taskId) subagentSessions.delete(task.sessionID) From 4d966ec99b8c5628071bb3f6ffa651c40c29d4e9 Mon Sep 17 00:00:00 2001 From: Gladdonilli Date: Wed, 14 Jan 2026 14:08:53 +0800 Subject: [PATCH 4/4] refactor(background-agent): extract cleanupPendingByParent helper Extract duplicated 8-line pendingByParent cleanup pattern into a reusable helper method. Reduces code duplication across 5 call sites. Addresses cubic-dev-ai feedback on PR #736. --- src/features/background-agent/manager.ts | 65 ++++++++---------------- 1 file changed, 20 insertions(+), 45 deletions(-) diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index 369ee15d..2d83419a 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -433,15 +433,7 @@ export class BackgroundManager { task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - if (task.parentSessionID) { - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) - } - } - } + this.cleanupPendingByParent(task) this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via session.idle event:", task.id) @@ -469,15 +461,7 @@ export class BackgroundManager { task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - if (task.parentSessionID) { - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) - } - } - } + this.cleanupPendingByParent(task) this.tasks.delete(task.id) this.clearNotificationsForTask(task.id) subagentSessions.delete(sessionID) @@ -569,6 +553,21 @@ export class BackgroundManager { } } + /** + * Remove task from pending tracking for its parent session. + * Cleans up the parent entry if no pending tasks remain. + */ + private cleanupPendingByParent(task: BackgroundTask): void { + if (!task.parentSessionID) return + const pending = this.pendingByParent.get(task.parentSessionID) + if (pending) { + pending.delete(task.id) + if (pending.size === 0) { + this.pendingByParent.delete(task.parentSessionID) + } + } + } + private startPolling(): void { if (this.pollingInterval) return @@ -739,15 +738,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - if (task.parentSessionID) { - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) - } - } - } + this.cleanupPendingByParent(task) this.clearNotificationsForTask(taskId) this.tasks.delete(taskId) subagentSessions.delete(task.sessionID) @@ -806,15 +797,7 @@ try { task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - if (task.parentSessionID) { - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) - } - } - } + this.cleanupPendingByParent(task) this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via polling:", task.id) @@ -887,15 +870,7 @@ if (lastMessage) { task.concurrencyKey = undefined // Prevent double-release } // Clean up pendingByParent to prevent stale entries - if (task.parentSessionID) { - const pending = this.pendingByParent.get(task.parentSessionID) - if (pending) { - pending.delete(task.id) - if (pending.size === 0) { - this.pendingByParent.delete(task.parentSessionID) - } - } - } + this.cleanupPendingByParent(task) this.markForNotification(task) await this.notifyParentSession(task) log("[background-agent] Task completed via stability detection:", task.id)