Jacob314/memory fixes (#754)

Address multiple possible memory leaks found bystatic analysis of the codebase. The primary source of the leaks was event listeners on child processes and global objects that were not being properly removed, potentially causing their closures to be retained in memory indefinitely particularly for processes that did not exit.

There are two commits. A larger one made by gemini CLI and a smaller one by me to make sure we always disconnect child processes as part of the cleanup methods. These changes may not actually fix any leaks but do look like reasonable defensive coding to avoid leaking event listeners or child processes.

The following files were fixed:
This is Gemini's somewhat overconfident description of what it did.

packages/core/src/tools/shell.ts: Fixed a leak where an abortSignal listener was not being reliably removed.
packages/cli/src/utils/readStdin.ts: Fixed a significant leak where listeners on process.stdin were never removed.
packages/cli/src/utils/sandbox.ts: Fixed leaks in the imageExists and pullImage helper functions where listeners on spawned child processes were not being removed.
packages/core/src/tools/grep.ts: Fixed three separate leaks in the isCommandAvailable check and the git grep and system grep strategies due to un-removed listeners on child processes.
packages/core/src/tools/tool-registry.ts: Corrected a leak in the execute method of the DiscoveredTool class where listeners on the spawned tool process were not being removed.

3 files changed, 68 insertions, 26 deletions

diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts
index acdf0bc8..38a1d4f5 100644
--- a/packages/core/src/tools/grep.ts
+++ b/packages/core/src/tools/grep.ts
@@ -461,8 +461,8 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
             const stdoutChunks: Buffer[] = [];
             const stderrChunks: Buffer[] = [];
 
-            child.stdout.on('data', (chunk) => stdoutChunks.push(chunk));
-            child.stderr.on('data', (chunk) => {
+            const onData = (chunk: Buffer) => stdoutChunks.push(chunk);
+            const onStderr = (chunk: Buffer) => {
               const stderrStr = chunk.toString();
               // Suppress common harmless stderr messages
               if (
@@ -471,15 +471,17 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
               ) {
                 stderrChunks.push(chunk);
               }
-            });
-            child.on('error', (err) =>
-              reject(new Error(`Failed to start system grep: ${err.message}`)),
-            );
-            child.on('close', (code) => {
+            };
+            const onError = (err: Error) => {
+              cleanup();
+              reject(new Error(`Failed to start system grep: ${err.message}`));
+            };
+            const onClose = (code: number | null) => {
               const stdoutData = Buffer.concat(stdoutChunks).toString('utf8');
               const stderrData = Buffer.concat(stderrChunks)
                 .toString('utf8')
                 .trim();
+              cleanup();
               if (code === 0) resolve(stdoutData);
               else if (code === 1)
                 resolve(''); // No matches
@@ -492,7 +494,22 @@ export class GrepTool extends BaseTool<GrepToolParams, ToolResult> {
                   );
                 else resolve(''); // Exit code > 1 but no stderr, likely just suppressed errors
               }
-            });
+            };
+
+            const cleanup = () => {
+              child.stdout.removeListener('data', onData);
+              child.stderr.removeListener('data', onStderr);
+              child.removeListener('error', onError);
+              child.removeListener('close', onClose);
+              if (child.connected) {
+                child.disconnect();
+              }
+            };
+
+            child.stdout.on('data', onData);
+            child.stderr.on('data', onStderr);
+            child.on('error', onError);
+            child.on('close', onClose);
           });
           return this.parseGrepOutput(output, absolutePath);
         } catch (grepError: unknown) {
diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts
index 2117366a..8fa32490 100644
--- a/packages/core/src/tools/shell.ts
+++ b/packages/core/src/tools/shell.ts
@@ -257,9 +257,11 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
     abortSignal.addEventListener('abort', abortHandler);
 
     // wait for the shell to exit
-    await new Promise((resolve) => shell.on('exit', resolve));
-
-    abortSignal.removeEventListener('abort', abortHandler);
+    try {
+      await new Promise((resolve) => shell.on('exit', resolve));
+    } finally {
+      abortSignal.removeEventListener('abort', abortHandler);
+    }
 
     // parse pids (pgrep output) from temporary file and remove it
     const backgroundPIDs: number[] = [];
diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts
index 2b27a703..f3162ac0 100644
--- a/packages/core/src/tools/tool-registry.ts
+++ b/packages/core/src/tools/tool-registry.ts
@@ -53,28 +53,51 @@ Signal: Signal number or \`(none)\` if no signal was received.
     const child = spawn(callCommand, [this.name]);
     child.stdin.write(JSON.stringify(params));
     child.stdin.end();
+
     let stdout = '';
     let stderr = '';
-    child.stdout.on('data', (data) => {
-      stdout += data?.toString();
-    });
-    child.stderr.on('data', (data) => {
-      stderr += data?.toString();
-    });
     let error: Error | null = null;
-    child.on('error', (err: Error) => {
-      error = err;
-    });
     let code: number | null = null;
     let signal: NodeJS.Signals | null = null;
-    child.on(
-      'close',
-      (_code: number | null, _signal: NodeJS.Signals | null) => {
+
+    await new Promise<void>((resolve) => {
+      const onStdout = (data: Buffer) => {
+        stdout += data?.toString();
+      };
+
+      const onStderr = (data: Buffer) => {
+        stderr += data?.toString();
+      };
+
+      const onError = (err: Error) => {
+        error = err;
+      };
+
+      const onClose = (
+        _code: number | null,
+        _signal: NodeJS.Signals | null,
+      ) => {
         code = _code;
         signal = _signal;
-      },
-    );
-    await new Promise((resolve) => child.on('close', resolve));
+        cleanup();
+        resolve();
+      };
+
+      const cleanup = () => {
+        child.stdout.removeListener('data', onStdout);
+        child.stderr.removeListener('data', onStderr);
+        child.removeListener('error', onError);
+        child.removeListener('close', onClose);
+        if (child.connected) {
+          child.disconnect();
+        }
+      };
+
+      child.stdout.on('data', onStdout);
+      child.stderr.on('data', onStderr);
+      child.on('error', onError);
+      child.on('close', onClose);
+    });
 
     // if there is any error, non-zero exit code, signal, or stderr, return error details instead of stdout
     if (error || code !== 0 || signal || stderr) {