refactor(background-agent): wire session-idle-event-handler into manager, add unit tests
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.
This commit is contained in:
YeonGyu-Kim
@@ -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<string, unknown>,
|
||||
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 } }),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
340
src/features/background-agent/session-idle-event-handler.test.ts
Normal file
340
src/features/background-agent/session-idle-event-handler.test.ts
Normal file
@@ -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> = {}): 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<string, ReturnType<typeof setTimeout>>()
|
||||
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<string, ReturnType<typeof setTimeout>>([
|
||||
[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<string, ReturnType<typeof setTimeout>>()
|
||||
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()
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user