Skip to content

fix(session-notification): add grace period to prevent late events from cancelling idle notifications #2012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
crazyrabbit0 wants to merge 1 commit into
base: dev
Choose a base branch
from
+107 −11
Open

fix(session-notification): add grace period to prevent late events from cancelling idle notifications #2012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions src/hooks/session-notification-scheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ type SessionNotificationConfig = {
idleConfirmationDelay: number
skipIfIncompleteTodos: boolean
maxTrackedSessions: number
/** Grace period in ms to ignore late-arriving activity events after scheduling (default: 100) */
activityGracePeriodMs?: number
}

export function createIdleNotificationScheduler(options: {
Expand All @@ -24,8 +26,11 @@ export function createIdleNotificationScheduler(options: {
const sessionActivitySinceIdle = new Set<string>()
const notificationVersions = new Map<string, number>()
const executingNotifications = new Set<string>()
const scheduledAt = new Map<string, number>()

function cleanupOldSessions(): void {
const activityGracePeriodMs = options.config.activityGracePeriodMs ?? 100

function cleanupOldSessions(): void {
const maxSessions = options.config.maxTrackedSessions
if (notifiedSessions.size > maxSessions) {
const sessionsToRemove = Array.from(notifiedSessions).slice(0, notifiedSessions.size - maxSessions)
Expand All @@ -43,44 +48,58 @@ export function createIdleNotificationScheduler(options: {
const sessionsToRemove = Array.from(executingNotifications).slice(0, executingNotifications.size - maxSessions)
sessionsToRemove.forEach((id) => executingNotifications.delete(id))
}
if (scheduledAt.size > maxSessions) {
const sessionsToRemove = Array.from(scheduledAt.keys()).slice(0, scheduledAt.size - maxSessions)
sessionsToRemove.forEach((id) => scheduledAt.delete(id))
}
}

function cancelPendingNotification(sessionID: string): void {
function cancelPendingNotification(sessionID: string): void {
const timer = pendingTimers.get(sessionID)
if (timer) {
clearTimeout(timer)
pendingTimers.delete(sessionID)
}
scheduledAt.delete(sessionID)
sessionActivitySinceIdle.add(sessionID)
notificationVersions.set(sessionID, (notificationVersions.get(sessionID) ?? 0) + 1)
}

function markSessionActivity(sessionID: string): void {
function markSessionActivity(sessionID: string): void {
const scheduledTime = scheduledAt.get(sessionID)
if (scheduledTime && Date.now() - scheduledTime < activityGracePeriodMs) {
return
}

cancelPendingNotification(sessionID)
if (!executingNotifications.has(sessionID)) {
notifiedSessions.delete(sessionID)
}
}

async function executeNotification(sessionID: string, version: number): Promise<void> {
async function executeNotification(sessionID: string, version: number): Promise<void> {
if (executingNotifications.has(sessionID)) {
pendingTimers.delete(sessionID)
scheduledAt.delete(sessionID)
return
}

if (notificationVersions.get(sessionID) !== version) {
pendingTimers.delete(sessionID)
scheduledAt.delete(sessionID)
return
}

if (sessionActivitySinceIdle.has(sessionID)) {
sessionActivitySinceIdle.delete(sessionID)
pendingTimers.delete(sessionID)
scheduledAt.delete(sessionID)
return
}

if (notifiedSessions.has(sessionID)) {
pendingTimers.delete(sessionID)
scheduledAt.delete(sessionID)
return
}

Expand Down Expand Up @@ -113,19 +132,21 @@ export function createIdleNotificationScheduler(options: {
} finally {
executingNotifications.delete(sessionID)
pendingTimers.delete(sessionID)
scheduledAt.delete(sessionID)
if (sessionActivitySinceIdle.has(sessionID)) {
notifiedSessions.delete(sessionID)
sessionActivitySinceIdle.delete(sessionID)
}
}
}

function scheduleIdleNotification(sessionID: string): void {
function scheduleIdleNotification(sessionID: string): void {
if (notifiedSessions.has(sessionID)) return
if (pendingTimers.has(sessionID)) return
if (executingNotifications.has(sessionID)) return

sessionActivitySinceIdle.delete(sessionID)
scheduledAt.set(sessionID, Date.now())

const currentVersion = (notificationVersions.get(sessionID) ?? 0) + 1
notificationVersions.set(sessionID, currentVersion)
Expand All @@ -138,12 +159,13 @@ export function createIdleNotificationScheduler(options: {
cleanupOldSessions()
}

function deleteSession(sessionID: string): void {
function deleteSession(sessionID: string): void {
cancelPendingNotification(sessionID)
notifiedSessions.delete(sessionID)
sessionActivitySinceIdle.delete(sessionID)
notificationVersions.delete(sessionID)
executingNotifications.delete(sessionID)
scheduledAt.delete(sessionID)
}

return {
Expand Down
84 changes: 79 additions & 5 deletions src/hooks/session-notification.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,15 @@ describe("session-notification", () => {
expect(notificationCalls).toHaveLength(0)
})

test("should cancel pending notification on session activity", async () => {
test("should cancel pending notification on session activity", async () => {
// given - main session is set
const mainSessionID = "main-cancel"
setMainSession(mainSessionID)

const hook = createSessionNotification(createMockPluginInput(), {
idleConfirmationDelay: 100, // Long delay
idleConfirmationDelay: 100,
skipIfIncompleteTodos: false,
activityGracePeriodMs: 0,
})

// when - session goes idle
Expand Down Expand Up @@ -258,14 +259,15 @@ describe("session-notification", () => {
expect(notificationCalls).toHaveLength(0)
})

test("should mark session activity on message.updated event", async () => {
test("should mark session activity on message.updated event", async () => {
// given - main session is set
const mainSessionID = "main-message"
setMainSession(mainSessionID)

const hook = createSessionNotification(createMockPluginInput(), {
idleConfirmationDelay: 50,
skipIfIncompleteTodos: false,
activityGracePeriodMs: 0,
})

// when - session goes idle, then message.updated fires
Expand All @@ -292,14 +294,15 @@ describe("session-notification", () => {
expect(notificationCalls).toHaveLength(0)
})

test("should mark session activity on tool.execute.before event", async () => {
test("should mark session activity on tool.execute.before event", async () => {
// given - main session is set
const mainSessionID = "main-tool"
setMainSession(mainSessionID)

const hook = createSessionNotification(createMockPluginInput(), {
idleConfirmationDelay: 50,
skipIfIncompleteTodos: false,
activityGracePeriodMs: 0,
})

// when - session goes idle, then tool.execute.before fires
Expand All @@ -324,7 +327,7 @@ describe("session-notification", () => {
expect(notificationCalls).toHaveLength(0)
})

test("should not send duplicate notification for same session", async () => {
test("should not send duplicate notification for same session", async () => {
// given - main session is set
const mainSessionID = "main-dup"
setMainSession(mainSessionID)
Expand Down Expand Up @@ -358,4 +361,75 @@ describe("session-notification", () => {
// then - only one notification should be sent
expect(notificationCalls).toHaveLength(1)
})

test("should ignore activity events within grace period", async () => {
// given - main session is set
const mainSessionID = "main-grace"
setMainSession(mainSessionID)

const hook = createSessionNotification(createMockPluginInput(), {
idleConfirmationDelay: 50,
skipIfIncompleteTodos: false,
activityGracePeriodMs: 100,
})

// when - session goes idle
await hook({
event: {
type: "session.idle",
properties: { sessionID: mainSessionID },
},
})

// when - activity happens immediately (within grace period)
await hook({
event: {
type: "tool.execute.before",
properties: { sessionID: mainSessionID },
},
})

// Wait for idle delay to pass
await new Promise((resolve) => setTimeout(resolve, 100))

// then - notification SHOULD be sent (activity was within grace period, ignored)
expect(notificationCalls.length).toBeGreaterThanOrEqual(1)
})

test("should cancel notification for activity after grace period", async () => {
// given - main session is set
const mainSessionID = "main-grace-cancel"
setMainSession(mainSessionID)

const hook = createSessionNotification(createMockPluginInput(), {
idleConfirmationDelay: 200,
skipIfIncompleteTodos: false,
activityGracePeriodMs: 50,
})

// when - session goes idle
await hook({
event: {
type: "session.idle",
properties: { sessionID: mainSessionID },
},
})

// when - wait for grace period to pass
await new Promise((resolve) => setTimeout(resolve, 60))

// when - activity happens after grace period
await hook({
event: {
type: "tool.execute.before",
properties: { sessionID: mainSessionID },
},
})

// Wait for original delay to pass
await new Promise((resolve) => setTimeout(resolve, 200))

// then - notification should NOT be sent (activity cancelled it after grace period)
expect(notificationCalls).toHaveLength(0)
})
})