From 2c3c447dc4cf6cced2a3a56ff3cef7bc3c2f52ce Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Wed, 11 Mar 2026 21:30:04 +0900 Subject: [PATCH] fix: address review-work round 3 findings (async shutdown, signal generation, stale test name) --- src/create-managers.ts | 4 ++-- src/features/background-agent/manager.ts | 8 ++++---- src/features/skill-mcp-manager/cleanup.ts | 1 + src/features/skill-mcp-manager/connection-race.test.ts | 2 +- src/plugin-dispose.test.ts | 8 ++++---- src/plugin-dispose.ts | 4 ++-- 6 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/create-managers.ts b/src/create-managers.ts index de4f5de2f..0aefd0e20 100644 --- a/src/create-managers.ts +++ b/src/create-managers.ts @@ -53,8 +53,8 @@ export function createManagers(args: { log("[index] onSubagentSessionCreated callback completed") }, - onShutdown: () => { - tmuxSessionManager.cleanup().catch((error) => { + onShutdown: async () => { + await tmuxSessionManager.cleanup().catch((error) => { log("[index] tmux cleanup error during shutdown:", error) }) }, diff --git a/src/features/background-agent/manager.ts b/src/features/background-agent/manager.ts index d1c41d743..75c8bc95b 100644 --- a/src/features/background-agent/manager.ts +++ b/src/features/background-agent/manager.ts @@ -116,7 +116,7 @@ export class BackgroundManager { private config?: BackgroundTaskConfig private tmuxEnabled: boolean private onSubagentSessionCreated?: OnSubagentSessionCreated - private onShutdown?: () => void + private onShutdown?: () => void | Promise private queuesByKey: Map = new Map() private processingKeys: Set = new Set() @@ -134,7 +134,7 @@ export class BackgroundManager { options?: { tmuxConfig?: TmuxConfig onSubagentSessionCreated?: OnSubagentSessionCreated - onShutdown?: () => void + onShutdown?: () => void | Promise enableParentSessionNotifications?: boolean } ) { @@ -1702,7 +1702,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea * Cancels all pending concurrency waiters and clears timers. * Should be called when the plugin is unloaded. */ - shutdown(): void { + async shutdown(): Promise { if (this.shutdownTriggered) return this.shutdownTriggered = true log("[background-agent] Shutting down BackgroundManager") @@ -1720,7 +1720,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea // Notify shutdown listeners (e.g., tmux cleanup) if (this.onShutdown) { try { - this.onShutdown() + await this.onShutdown() } catch (error) { log("[background-agent] Error in onShutdown callback:", error) } diff --git a/src/features/skill-mcp-manager/cleanup.ts b/src/features/skill-mcp-manager/cleanup.ts index d2d36beaf..500294123 100644 --- a/src/features/skill-mcp-manager/cleanup.ts +++ b/src/features/skill-mcp-manager/cleanup.ts @@ -19,6 +19,7 @@ export function registerProcessCleanup(state: SkillMcpManagerState): void { state.cleanupRegistered = true const cleanup = async (): Promise => { + state.shutdownGeneration++ for (const managed of state.clients.values()) { await closeManagedClient(managed) } diff --git a/src/features/skill-mcp-manager/connection-race.test.ts b/src/features/skill-mcp-manager/connection-race.test.ts index be5b8f6bb..c571e9e30 100644 --- a/src/features/skill-mcp-manager/connection-race.test.ts +++ b/src/features/skill-mcp-manager/connection-race.test.ts @@ -142,7 +142,7 @@ describe("getOrCreateClient disconnect race", () => { expect(createdTransports[0]?.close).toHaveBeenCalledTimes(1) }) - it("#given session A in disconnectedSessions #when new connection is requested for session A #then session A is removed from disconnectedSessions and connection proceeds normally", async () => { + it("#given session A in disconnectedSessions #when new connection is requested for session A #then connection proceeds normally and disconnectedSessions entry is retained for pending race protection", async () => { const state = createState() const info = createClientInfo("session-a") const clientKey = createClientKey(info) diff --git a/src/plugin-dispose.test.ts b/src/plugin-dispose.test.ts index 17ca88fe3..57758cc17 100644 --- a/src/plugin-dispose.test.ts +++ b/src/plugin-dispose.test.ts @@ -7,7 +7,7 @@ describe("createPluginDispose", () => { test("#given plugin with active managers and hooks #when dispose() is called #then backgroundManager.shutdown() is called", async () => { // given const backgroundManager = { - shutdown: (): void => {}, + shutdown: async (): Promise => {}, } const skillMcpManager = { disconnectAll: async (): Promise => {}, @@ -29,7 +29,7 @@ describe("createPluginDispose", () => { test("#given plugin with active MCP connections #when dispose() is called #then skillMcpManager.disconnectAll() is called", async () => { // given const backgroundManager = { - shutdown: (): void => {}, + shutdown: async (): Promise => {}, } const skillMcpManager = { disconnectAll: async (): Promise => {}, @@ -64,7 +64,7 @@ describe("createPluginDispose", () => { const autoSlashCommandDisposeSpy = spyOn(autoSlashCommand, "dispose") const dispose = createPluginDispose({ backgroundManager: { - shutdown: (): void => {}, + shutdown: async (): Promise => {}, }, skillMcpManager: { disconnectAll: async (): Promise => {}, @@ -90,7 +90,7 @@ describe("createPluginDispose", () => { test("#given dispose already called #when dispose() called again #then no errors", async () => { // given const backgroundManager = { - shutdown: (): void => {}, + shutdown: async (): Promise => {}, } const skillMcpManager = { disconnectAll: async (): Promise => {}, diff --git a/src/plugin-dispose.ts b/src/plugin-dispose.ts index 7b718bbe4..6d0d65b75 100644 --- a/src/plugin-dispose.ts +++ b/src/plugin-dispose.ts @@ -2,7 +2,7 @@ export type PluginDispose = () => Promise export function createPluginDispose(args: { backgroundManager: { - shutdown: () => void + shutdown: () => void | Promise } skillMcpManager: { disconnectAll: () => Promise @@ -19,7 +19,7 @@ export function createPluginDispose(args: { } disposePromise = (async (): Promise => { - backgroundManager.shutdown() + await backgroundManager.shutdown() await skillMcpManager.disconnectAll() disposeHooks() })()