From a3e4f904a669069f91075feb922d18a66c098581 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sun, 22 Feb 2026 15:30:15 +0900 Subject: [PATCH] refactor(background-agent): wire session-idle-event-handler into manager, add unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The extracted handleSessionIdleBackgroundEvent was never imported by manager.ts — dead code from incomplete refactoring (d53bcfbc). Replace the inline session.idle handler (58 LOC) with a call to the extracted function, remove unused MIN_IDLE_TIME_MS import, and add 13 unit tests covering all edge cases. --- src/features/background-agent/manager.ts | 66 +--- .../session-idle-event-handler.test.ts | 340 ++++++++++++++++++ 2 files changed, 350 insertions(+), 56 deletions(-) create mode 100644 src/features/background-agent/session-idle-event-handler.test.ts diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index c807277d2..c9ce281ba 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -25,7 +25,6 @@ import { hasMoreFallbacks, } from "../../shared/model-error-classifier" import { - MIN_IDLE_TIME_MS, POLLING_INTERVAL_MS, TASK_CLEANUP_DELAY_MS, } from "./constants" @@ -43,6 +42,7 @@ import { import { tryFallbackRetry } from "./fallback-retry-handler" import { registerManagerForCleanup, unregisterManagerForCleanup } from "./process-cleanup" import { isCompactionAgent, findNearestMessageExcludingCompaction } from "./compaction-aware-message-resolver" +import { handleSessionIdleBackgroundEvent } from "./session-idle-event-handler" import { MESSAGE_STORAGE } from "../hook-message-injector" import { join } from "node:path" import { pruneStaleTasksAndNotifications } from "./task-poller" @@ -740,61 +740,15 @@ export class BackgroundManager { } if (event.type === "session.idle") { - const sessionID = props?.sessionID as string | undefined - if (!sessionID) return - - const task = this.findBySession(sessionID) - if (!task || task.status !== "running") return - - const startedAt = task.startedAt - if (!startedAt) return - - // Edge guard: Require minimum elapsed time (5 seconds) before accepting idle - const elapsedMs = Date.now() - startedAt.getTime() - if (elapsedMs < MIN_IDLE_TIME_MS) { - const remainingMs = MIN_IDLE_TIME_MS - elapsedMs - if (!this.idleDeferralTimers.has(task.id)) { - log("[background-agent] Deferring early session.idle:", { elapsedMs, remainingMs, taskId: task.id }) - const timer = setTimeout(() => { - this.idleDeferralTimers.delete(task.id) - this.handleEvent({ type: "session.idle", properties: { sessionID } }) - }, remainingMs) - this.idleDeferralTimers.set(task.id, timer) - } else { - log("[background-agent] session.idle already deferred:", { elapsedMs, taskId: task.id }) - } - return - } - - // Edge guard: Verify session has actual assistant output before completing - this.validateSessionHasOutput(sessionID).then(async (hasValidOutput) => { - // Re-check status after async operation (could have been completed by polling) - if (task.status !== "running") { - log("[background-agent] Task status changed during validation, skipping:", { taskId: task.id, status: task.status }) - return - } - - if (!hasValidOutput) { - log("[background-agent] Session.idle but no valid output yet, waiting:", task.id) - return - } - - const hasIncompleteTodos = await this.checkSessionTodos(sessionID) - - // Re-check status after async operation again - if (task.status !== "running") { - log("[background-agent] Task status changed during todo check, skipping:", { taskId: task.id, status: task.status }) - return - } - - if (hasIncompleteTodos) { - log("[background-agent] Task has incomplete todos, waiting for todo-continuation:", task.id) - return - } - - await this.tryCompleteTask(task, "session.idle event") - }).catch(err => { - log("[background-agent] Error in session.idle handler:", err) + if (!props || typeof props !== "object") return + handleSessionIdleBackgroundEvent({ + properties: props as Record, + findBySession: (id) => this.findBySession(id), + idleDeferralTimers: this.idleDeferralTimers, + validateSessionHasOutput: (id) => this.validateSessionHasOutput(id), + checkSessionTodos: (id) => this.checkSessionTodos(id), + tryCompleteTask: (task, source) => this.tryCompleteTask(task, source), + emitIdleEvent: (sessionID) => this.handleEvent({ type: "session.idle", properties: { sessionID } }), }) } diff --git a/src/features/background-agent/session-idle-event-handler.test.ts b/src/features/background-agent/session-idle-event-handler.test.ts new file mode 100644 index 000000000..1e2efafbc --- /dev/null +++ b/src/features/background-agent/session-idle-event-handler.test.ts @@ -0,0 +1,340 @@ +import { describe, it, expect, mock } from "bun:test" + +import { handleSessionIdleBackgroundEvent } from "./session-idle-event-handler" +import type { BackgroundTask } from "./types" +import { MIN_IDLE_TIME_MS } from "./constants" + +function createRunningTask(overrides: Partial = {}): BackgroundTask { + return { + id: "task-1", + sessionID: "ses-idle-1", + parentSessionID: "parent-ses-1", + parentMessageID: "msg-1", + description: "test idle handler", + prompt: "test", + agent: "explore", + status: "running", + startedAt: new Date(Date.now() - (MIN_IDLE_TIME_MS + 100)), + ...overrides, + } +} + +describe("handleSessionIdleBackgroundEvent", () => { + describe("#given no sessionID in properties", () => { + it("#then should do nothing", () => { + //#given + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: {}, + findBySession: () => undefined, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) + + describe("#given non-string sessionID in properties", () => { + it("#then should do nothing", () => { + //#given + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: 123 }, + findBySession: () => undefined, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) + + describe("#given no task found for session", () => { + it("#then should do nothing", () => { + //#given + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: "ses-unknown" }, + findBySession: () => undefined, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) + + describe("#given task is not running", () => { + it("#then should do nothing", () => { + //#given + const task = createRunningTask({ status: "completed" }) + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) + + describe("#given task has no startedAt", () => { + it("#then should do nothing", () => { + //#given + const task = createRunningTask({ startedAt: undefined }) + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) + + describe("#given elapsed time < MIN_IDLE_TIME_MS", () => { + it("#when idle fires early #then should defer with timer", () => { + //#given + const realDateNow = Date.now + const baseNow = realDateNow() + const task = createRunningTask({ startedAt: new Date(baseNow) }) + const idleDeferralTimers = new Map>() + const emitIdleEvent = mock(() => {}) + + try { + Date.now = () => baseNow + (MIN_IDLE_TIME_MS - 100) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers, + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask: () => Promise.resolve(true), + emitIdleEvent, + }) + + //#then + expect(idleDeferralTimers.has(task.id)).toBe(true) + expect(emitIdleEvent).not.toHaveBeenCalled() + } finally { + clearTimeout(idleDeferralTimers.get(task.id)!) + Date.now = realDateNow + } + }) + + it("#when idle already deferred #then should not create duplicate timer", () => { + //#given + const realDateNow = Date.now + const baseNow = realDateNow() + const task = createRunningTask({ startedAt: new Date(baseNow) }) + const existingTimer = setTimeout(() => {}, 99999) + const idleDeferralTimers = new Map>([ + [task.id, existingTimer], + ]) + const emitIdleEvent = mock(() => {}) + + try { + Date.now = () => baseNow + (MIN_IDLE_TIME_MS - 100) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers, + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask: () => Promise.resolve(true), + emitIdleEvent, + }) + + //#then + expect(idleDeferralTimers.get(task.id)).toBe(existingTimer) + } finally { + clearTimeout(existingTimer) + Date.now = realDateNow + } + }) + + it("#when deferred timer fires #then should emit idle event", async () => { + //#given + const realDateNow = Date.now + const baseNow = realDateNow() + const task = createRunningTask({ startedAt: new Date(baseNow) }) + const idleDeferralTimers = new Map>() + const emitIdleEvent = mock(() => {}) + const remainingMs = 50 + + try { + Date.now = () => baseNow + (MIN_IDLE_TIME_MS - remainingMs) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers, + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask: () => Promise.resolve(true), + emitIdleEvent, + }) + + //#then - wait for deferred timer + await new Promise((resolve) => setTimeout(resolve, remainingMs + 50)) + expect(emitIdleEvent).toHaveBeenCalledWith(task.sessionID) + expect(idleDeferralTimers.has(task.id)).toBe(false) + } finally { + Date.now = realDateNow + } + }) + }) + + describe("#given elapsed time >= MIN_IDLE_TIME_MS", () => { + it("#when session has valid output and no incomplete todos #then should complete task", async () => { + //#given + const task = createRunningTask() + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(tryCompleteTask).toHaveBeenCalledWith(task, "session.idle event") + }) + + it("#when session has no valid output #then should not complete task", async () => { + //#given + const task = createRunningTask() + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(false), + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + + it("#when task has incomplete todos #then should not complete task", async () => { + //#given + const task = createRunningTask() + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: () => Promise.resolve(true), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + + it("#when task status changes during validation #then should not complete task", async () => { + //#given + const task = createRunningTask() + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: async () => { + task.status = "completed" + return true + }, + checkSessionTodos: () => Promise.resolve(false), + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + + it("#when task status changes during todo check #then should not complete task", async () => { + //#given + const task = createRunningTask() + const tryCompleteTask = mock(() => Promise.resolve(true)) + + //#when + handleSessionIdleBackgroundEvent({ + properties: { sessionID: task.sessionID! }, + findBySession: () => task, + idleDeferralTimers: new Map(), + validateSessionHasOutput: () => Promise.resolve(true), + checkSessionTodos: async () => { + task.status = "cancelled" + return false + }, + tryCompleteTask, + emitIdleEvent: () => {}, + }) + + //#then + await new Promise((resolve) => setTimeout(resolve, 10)) + expect(tryCompleteTask).not.toHaveBeenCalled() + }) + }) +})