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

much improved support for background processes, avoiding termination (via SIGPIPE) or eventual blocking (e.g. due to filled OS buffers) (#586)

This commit is contained in:
Olcan 2025-05-28 14:45:46 -07:00 committed by GitHub
parent 00805cb2cd
commit 0d99398689
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 35 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
packages/cli/src/ui/hooks/shellCommandProcessor.ts
View File

@ -120,13 +120,19 @@ export const useShellCommandProcessor = (
stdio: ['ignore', 'pipe', 'pipe'], stdio: ['ignore', 'pipe', 'pipe'],
}); });
let exited = false;
let output = ''; let output = '';
const handleOutput = (data: string) => { const handleOutput = (data: string) => {
output += data; // continue to consume post-exit for background processes
setPendingHistoryItem({ // removing listeners can overflow OS buffer and block subprocesses
type: 'info', // destroying (e.g. child.stdout.destroy()) can terminate subprocesses via SIGPIPE
text: output, if (!exited) {
}); output += data;
setPendingHistoryItem({
type: 'info',
text: output,
});
}
}; };
child.stdout.on('data', handleOutput); child.stdout.on('data', handleOutput);
child.stderr.on('data', handleOutput); child.stderr.on('data', handleOutput);
@ -136,7 +142,8 @@ export const useShellCommandProcessor = (
error = err; error = err;
}); });
child.on('close', (code, signal) => { child.on('exit', (code, signal) => {
exited = true;
setPendingHistoryItem(null); setPendingHistoryItem(null);
output = output.trim() || '(Command produced no output)'; output = output.trim() || '(Command produced no output)';
if (error) { if (error) {

46
packages/server/src/tools/shell.ts
View File

@ -143,8 +143,7 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
let command = params.command.trim(); let command = params.command.trim();
if (!command.endsWith('&')) command += ';'; if (!command.endsWith('&')) command += ';';
// note the final echo is only to trigger the stderr handler below command = `{ ${command} }; __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; exit $__code;`;
command = `{ ${command} }; __code=$?; pgrep -g 0 >${tempFilePath} 2>&1; ( trap '' PIPE ; echo >&2 ); exit $__code;`;
// spawn command in specified directory (or project root if not specified) // spawn command in specified directory (or project root if not specified)
const shell = spawn('bash', ['-c', command], { const shell = spawn('bash', ['-c', command], {
@ -153,35 +152,33 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
cwd: path.resolve(this.config.getTargetDir(), params.directory || ''), cwd: path.resolve(this.config.getTargetDir(), params.directory || ''),
}); });
let exited = false;
let stdout = ''; let stdout = '';
let output = ''; let output = '';
shell.stdout.on('data', (data: Buffer) => { shell.stdout.on('data', (data: Buffer) => {
const str = data.toString(); // continue to consume post-exit for background processes
stdout += str; // removing listeners can overflow OS buffer and block subprocesses
output += str; // destroying (e.g. shell.stdout.destroy()) can terminate subprocesses via SIGPIPE
if (onOutputChunk) { if (!exited) {
onOutputChunk(str); const str = data.toString();
stdout += str;
output += str;
if (onOutputChunk) {
onOutputChunk(str);
}
} }
}); });
let stderr = ''; let stderr = '';
shell.stderr.on('data', (data: Buffer) => { shell.stderr.on('data', (data: Buffer) => {
let str = data.toString(); if (!exited) {
// if the temporary file exists, close the streams and finalize stdout/stderr const str = data.toString();
// otherwise these streams can delay termination ('close' event) until all background processes exit stderr += str;
if (fs.existsSync(tempFilePath)) { output += str;
shell.stdout.destroy(); if (onOutputChunk) {
shell.stderr.destroy(); onOutputChunk(str);
// exclude final \n, which should be from echo >&2 unless there are background processes writing to stderr
if (str.endsWith('\n')) {
str = str.slice(0, -1);
} }
} }
stderr += str;
output += str;
if (onOutputChunk) {
onOutputChunk(str);
}
}); });
let error: Error | null = null; let error: Error | null = null;
@ -193,14 +190,15 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
let code: number | null = null; let code: number | null = null;
let processSignal: NodeJS.Signals | null = null; let processSignal: NodeJS.Signals | null = null;
const closeHandler = ( const exitHandler = (
_code: number | null, _code: number | null,
_signal: NodeJS.Signals | null, _signal: NodeJS.Signals | null,
) => { ) => {
exited = true;
code = _code; code = _code;
processSignal = _signal; processSignal = _signal;
}; };
shell.on('close', closeHandler); shell.on('exit', exitHandler);
const abortHandler = () => { const abortHandler = () => {
if (shell.pid) { if (shell.pid) {
@ -220,7 +218,7 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
abortSignal.addEventListener('abort', abortHandler); abortSignal.addEventListener('abort', abortHandler);
// wait for the shell to exit // wait for the shell to exit
await new Promise((resolve) => shell.on('close', resolve)); await new Promise((resolve) => shell.on('exit', resolve));
abortSignal.removeEventListener('abort', abortHandler); abortSignal.removeEventListener('abort', abortHandler);