Merge pull request #2029 from coleleavitt/fix/plug-resource-leaks

fix: plug resource leaks and add hook command timeout

src/tools/lsp/lsp-manager-process-cleanup.ts

@@ -1,45 +1,71 @@
 type ManagedClientForCleanup = {
   client: {
-    stop: () => Promise<void>
-  }
-}
+    stop: () => Promise<void>;
+  };
+};
 
 type ProcessCleanupOptions = {
-  getClients: () => IterableIterator<[string, ManagedClientForCleanup]>
-  clearClients: () => void
-  clearCleanupInterval: () => void
-}
+  getClients: () => IterableIterator<[string, ManagedClientForCleanup]>;
+  clearClients: () => void;
+  clearCleanupInterval: () => void;
+};
+
+type RegisteredHandler = {
+  event: string;
+  listener: (...args: unknown[]) => void;
+};
+
+export type LspProcessCleanupHandle = {
+  unregister: () => void;
+};
+
+export function registerLspManagerProcessCleanup(options: ProcessCleanupOptions): LspProcessCleanupHandle {
+  const handlers: RegisteredHandler[] = [];
 
-export function registerLspManagerProcessCleanup(options: ProcessCleanupOptions): void {
   // Synchronous cleanup for 'exit' event (cannot await)
   const syncCleanup = () => {
     for (const [, managed] of options.getClients()) {
       try {
         // Fire-and-forget during sync exit - process is terminating
-        void managed.client.stop().catch(() => {})
+        void managed.client.stop().catch(() => {});
       } catch {}
     }
-    options.clearClients()
-    options.clearCleanupInterval()
-  }
+    options.clearClients();
+    options.clearCleanupInterval();
+  };
 
   // Async cleanup for signal handlers - properly await all stops
   const asyncCleanup = async () => {
-    const stopPromises: Promise<void>[] = []
+    const stopPromises: Promise<void>[] = [];
     for (const [, managed] of options.getClients()) {
-      stopPromises.push(managed.client.stop().catch(() => {}))
+      stopPromises.push(managed.client.stop().catch(() => {}));
     }
-    await Promise.allSettled(stopPromises)
-    options.clearClients()
-    options.clearCleanupInterval()
-  }
+    await Promise.allSettled(stopPromises);
+    options.clearClients();
+    options.clearCleanupInterval();
+  };
 
-  process.on("exit", syncCleanup)
+  const registerHandler = (event: string, listener: (...args: unknown[]) => void) => {
+    handlers.push({ event, listener });
+    process.on(event, listener);
+  };
+
+  registerHandler("exit", syncCleanup);
 
   // Don't call process.exit() here; other handlers (background-agent manager) handle final exit.
-  process.on("SIGINT", () => void asyncCleanup().catch(() => {}))
-  process.on("SIGTERM", () => void asyncCleanup().catch(() => {}))
+  const signalCleanup = () => void asyncCleanup().catch(() => {});
+  registerHandler("SIGINT", signalCleanup);
+  registerHandler("SIGTERM", signalCleanup);
   if (process.platform === "win32") {
-    process.on("SIGBREAK", () => void asyncCleanup().catch(() => {}))
+    registerHandler("SIGBREAK", signalCleanup);
   }
+
+  return {
+    unregister: () => {
+      for (const { event, listener } of handlers) {
+        process.off(event, listener);
+      }
+      handlers.length = 0;
+    },
+  };
 }

src/tools/lsp/lsp-server.ts

@@ -1,73 +1,74 @@
-import type { ResolvedServer } from "./types"
-import { registerLspManagerProcessCleanup } from "./lsp-manager-process-cleanup"
-import { cleanupTempDirectoryLspClients } from "./lsp-manager-temp-directory-cleanup"
-import { LSPClient } from "./lsp-client"
+import { LSPClient } from "./lsp-client";
+import { registerLspManagerProcessCleanup, type LspProcessCleanupHandle } from "./lsp-manager-process-cleanup";
+import { cleanupTempDirectoryLspClients } from "./lsp-manager-temp-directory-cleanup";
+import type { ResolvedServer } from "./types";
 interface ManagedClient {
-  client: LSPClient
-  lastUsedAt: number
-  refCount: number
-  initPromise?: Promise<void>
-  isInitializing: boolean
-  initializingSince?: number
+  client: LSPClient;
+  lastUsedAt: number;
+  refCount: number;
+  initPromise?: Promise<void>;
+  isInitializing: boolean;
+  initializingSince?: number;
 }
 class LSPServerManager {
-  private static instance: LSPServerManager
-  private clients = new Map<string, ManagedClient>()
-  private cleanupInterval: ReturnType<typeof setInterval> | null = null
-  private readonly IDLE_TIMEOUT = 5 * 60 * 1000
-  private readonly INIT_TIMEOUT = 60 * 1000
+  private static instance: LSPServerManager;
+  private clients = new Map<string, ManagedClient>();
+  private cleanupInterval: ReturnType<typeof setInterval> | null = null;
+  private readonly IDLE_TIMEOUT = 5 * 60 * 1000;
+  private readonly INIT_TIMEOUT = 60 * 1000;
+  private cleanupHandle: LspProcessCleanupHandle | null = null;
   private constructor() {
-    this.startCleanupTimer()
-    this.registerProcessCleanup()
+    this.startCleanupTimer();
+    this.registerProcessCleanup();
   }
   private registerProcessCleanup(): void {
-    registerLspManagerProcessCleanup({
+    this.cleanupHandle = registerLspManagerProcessCleanup({
       getClients: () => this.clients.entries(),
       clearClients: () => {
-        this.clients.clear()
+        this.clients.clear();
       },
       clearCleanupInterval: () => {
         if (this.cleanupInterval) {
-          clearInterval(this.cleanupInterval)
-          this.cleanupInterval = null
+          clearInterval(this.cleanupInterval);
+          this.cleanupInterval = null;
         }
       },
-    })
+    });
   }
 
   static getInstance(): LSPServerManager {
     if (!LSPServerManager.instance) {
-      LSPServerManager.instance = new LSPServerManager()
+      LSPServerManager.instance = new LSPServerManager();
     }
-    return LSPServerManager.instance
+    return LSPServerManager.instance;
   }
 
   private getKey(root: string, serverId: string): string {
-    return `${root}::${serverId}`
+    return `${root}::${serverId}`;
   }
 
   private startCleanupTimer(): void {
-    if (this.cleanupInterval) return
+    if (this.cleanupInterval) return;
     this.cleanupInterval = setInterval(() => {
-      this.cleanupIdleClients()
-    }, 60000)
+      this.cleanupIdleClients();
+    }, 60000);
   }
 
   private cleanupIdleClients(): void {
-    const now = Date.now()
+    const now = Date.now();
     for (const [key, managed] of this.clients) {
       if (managed.refCount === 0 && now - managed.lastUsedAt > this.IDLE_TIMEOUT) {
-        managed.client.stop()
-        this.clients.delete(key)
+        managed.client.stop();
+        this.clients.delete(key);
       }
     }
   }
 
   async getClient(root: string, server: ResolvedServer): Promise<LSPClient> {
-    const key = this.getKey(root, server.id)
-    let managed = this.clients.get(key)
+    const key = this.getKey(root, server.id);
+    let managed = this.clients.get(key);
     if (managed) {
-      const now = Date.now()
+      const now = Date.now();
       if (
         managed.isInitializing &&
         managed.initializingSince !== undefined &&
@@ -75,45 +76,45 @@ class LSPServerManager {
       ) {
         // Stale init can permanently block subsequent calls (e.g., LSP process hang)
         try {
-          await managed.client.stop()
+          await managed.client.stop();
         } catch {}
-        this.clients.delete(key)
-        managed = undefined
+        this.clients.delete(key);
+        managed = undefined;
       }
     }
     if (managed) {
       if (managed.initPromise) {
         try {
-          await managed.initPromise
+          await managed.initPromise;
         } catch {
           // Failed init should not keep the key blocked forever.
           try {
-            await managed.client.stop()
+            await managed.client.stop();
           } catch {}
-          this.clients.delete(key)
-          managed = undefined
+          this.clients.delete(key);
+          managed = undefined;
         }
       }
 
       if (managed) {
         if (managed.client.isAlive()) {
-          managed.refCount++
-          managed.lastUsedAt = Date.now()
-          return managed.client
+          managed.refCount++;
+          managed.lastUsedAt = Date.now();
+          return managed.client;
         }
         try {
-          await managed.client.stop()
+          await managed.client.stop();
         } catch {}
-        this.clients.delete(key)
+        this.clients.delete(key);
       }
     }
 
-    const client = new LSPClient(root, server)
+    const client = new LSPClient(root, server);
     const initPromise = (async () => {
-      await client.start()
-      await client.initialize()
-    })()
-    const initStartedAt = Date.now()
+      await client.start();
+      await client.initialize();
+    })();
+    const initStartedAt = Date.now();
     this.clients.set(key, {
       client,
       lastUsedAt: initStartedAt,
@@ -121,37 +122,37 @@ class LSPServerManager {
       initPromise,
       isInitializing: true,
       initializingSince: initStartedAt,
-    })
+    });
 
     try {
-      await initPromise
+      await initPromise;
     } catch (error) {
-      this.clients.delete(key)
+      this.clients.delete(key);
       try {
-        await client.stop()
+        await client.stop();
       } catch {}
-      throw error
+      throw error;
     }
-    const m = this.clients.get(key)
+    const m = this.clients.get(key);
     if (m) {
-      m.initPromise = undefined
-      m.isInitializing = false
-      m.initializingSince = undefined
+      m.initPromise = undefined;
+      m.isInitializing = false;
+      m.initializingSince = undefined;
     }
 
-    return client
+    return client;
   }
 
   warmupClient(root: string, server: ResolvedServer): void {
-    const key = this.getKey(root, server.id)
-    if (this.clients.has(key)) return
-    const client = new LSPClient(root, server)
+    const key = this.getKey(root, server.id);
+    if (this.clients.has(key)) return;
+    const client = new LSPClient(root, server);
     const initPromise = (async () => {
-      await client.start()
-      await client.initialize()
-    })()
+      await client.start();
+      await client.initialize();
+    })();
 
-    const initStartedAt = Date.now()
+    const initStartedAt = Date.now();
     this.clients.set(key, {
       client,
       lastUsedAt: initStartedAt,
@@ -159,53 +160,55 @@ class LSPServerManager {
       initPromise,
       isInitializing: true,
       initializingSince: initStartedAt,
-    })
+    });
 
     initPromise
       .then(() => {
-        const m = this.clients.get(key)
+        const m = this.clients.get(key);
         if (m) {
-          m.initPromise = undefined
-          m.isInitializing = false
-          m.initializingSince = undefined
+          m.initPromise = undefined;
+          m.isInitializing = false;
+          m.initializingSince = undefined;
         }
       })
       .catch(() => {
         // Warmup failures must not permanently block future initialization.
-        this.clients.delete(key)
-        void client.stop().catch(() => {})
-      })
+        this.clients.delete(key);
+        void client.stop().catch(() => {});
+      });
   }
 
   releaseClient(root: string, serverId: string): void {
-    const key = this.getKey(root, serverId)
-    const managed = this.clients.get(key)
+    const key = this.getKey(root, serverId);
+    const managed = this.clients.get(key);
     if (managed && managed.refCount > 0) {
-      managed.refCount--
-      managed.lastUsedAt = Date.now()
+      managed.refCount--;
+      managed.lastUsedAt = Date.now();
     }
   }
 
   isServerInitializing(root: string, serverId: string): boolean {
-    const key = this.getKey(root, serverId)
-    const managed = this.clients.get(key)
-    return managed?.isInitializing ?? false
+    const key = this.getKey(root, serverId);
+    const managed = this.clients.get(key);
+    return managed?.isInitializing ?? false;
   }
 
   async stopAll(): Promise<void> {
+    this.cleanupHandle?.unregister();
+    this.cleanupHandle = null;
     for (const [, managed] of this.clients) {
-      await managed.client.stop()
+      await managed.client.stop();
     }
-    this.clients.clear()
+    this.clients.clear();
     if (this.cleanupInterval) {
-      clearInterval(this.cleanupInterval)
-      this.cleanupInterval = null
+      clearInterval(this.cleanupInterval);
+      this.cleanupInterval = null;
     }
   }
 
   async cleanupTempDirectoryClients(): Promise<void> {
-    await cleanupTempDirectoryLspClients(this.clients)
+    await cleanupTempDirectoryLspClients(this.clients);
   }
 }
 
-export const lspManager = LSPServerManager.getInstance()
+export const lspManager = LSPServerManager.getInstance();