jcarr
/
gemini-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Jacob Richman 2025-06-05 06:40:33 -07:00 committed by GitHub
parent 822803d9d6
commit aa386d135b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 125 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
packages/cli/src/utils/readStdin.ts
View File

@ -9,19 +9,31 @@ export async function readStdin(): Promise<string> {
let data = '';
process.stdin.setEncoding('utf8');
process.stdin.on('readable', () => {
const onReadable = () => {
let chunk;
while ((chunk = process.stdin.read()) !== null) {
data += chunk;
}
});
};
process.stdin.on('end', () => {
const onEnd = () => {
cleanup();
resolve(data);
});
};
process.stdin.on('error', (err) => {
const onError = (err: Error) => {
cleanup();
reject(err);
});
};
const cleanup = () => {
process.stdin.removeListener('readable', onReadable);
process.stdin.removeListener('end', onEnd);
process.stdin.removeListener('error', onError);
};
process.stdin.on('readable', onReadable);
process.stdin.on('end', onEnd);
process.stdin.on('error', onError);
});
}

54
packages/cli/src/utils/sandbox.ts
View File

@ -536,28 +536,28 @@ async function pullImage(sandbox: string, image: string): Promise<boolean> {
const pullProcess = spawn(sandbox, args, { stdio: 'pipe' });
let stderrData = '';
if (pullProcess.stdout) {
pullProcess.stdout.on('data', (data) => {
console.info(data.toString().trim()); // Show pull progress
});
}
if (pullProcess.stderr) {
pullProcess.stderr.on('data', (data) => {
stderrData += data.toString();
console.error(data.toString().trim()); // Show pull errors/info from the command itself
});
}
pullProcess.on('error', (err) => {
const onStdoutData = (data: Buffer) => {
console.info(data.toString().trim()); // Show pull progress
};
const onStderrData = (data: Buffer) => {
stderrData += data.toString();
console.error(data.toString().trim()); // Show pull errors/info from the command itself
};
const onError = (err: Error) => {
console.warn(
`Failed to start '${sandbox} pull ${image}' command: ${err.message}`,
);
cleanup();
resolve(false);
});
};
pullProcess.on('close', (code) => {
const onClose = (code: number | null) => {
if (code === 0) {
console.info(`Successfully pulled image ${image}.`);
cleanup();
resolve(true);
} else {
console.warn(
@ -566,9 +566,33 @@ async function pullImage(sandbox: string, image: string): Promise<boolean> {
if (stderrData.trim()) {
// Details already printed by the stderr listener above
}
cleanup();
resolve(false);
}
});
};
const cleanup = () => {
if (pullProcess.stdout) {
pullProcess.stdout.removeListener('data', onStdoutData);
}
if (pullProcess.stderr) {
pullProcess.stderr.removeListener('data', onStderrData);
}
pullProcess.removeListener('error', onError);
pullProcess.removeListener('close', onClose);
if (pullProcess.connected) {
pullProcess.disconnect();
}
};
if (pullProcess.stdout) {
pullProcess.stdout.on('data', onStdoutData);
}
if (pullProcess.stderr) {
pullProcess.stderr.on('data', onStderrData);
}
pullProcess.on('error', onError);
pullProcess.on('close', onClose);
});
}

33
packages/core/src/tools/grep.ts
View File

@ -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) {

8
packages/core/src/tools/shell.ts
View File

@ -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[] = [];

53
packages/core/src/tools/tool-registry.ts
View File

@ -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) {