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

tweaks to shell abort logic based on feedback (#618)

This commit is contained in:
Olcan 2025-05-30 01:35:03 -07:00 committed by GitHub
parent 094b9dc474
commit a3b557222a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 38 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
packages/cli/src/ui/hooks/shellCommandProcessor.test.ts
View File

@ -105,9 +105,10 @@ describe('useShellCommandProcessor', () => {
it('should return false for non-string input', () => { it('should return false for non-string input', () => {
const { result } = setupHook(); const { result } = setupHook();
const handled = result.current.handleShellCommand([ const handled = result.current.handleShellCommand(
{ text: 'not a string' }, [{ text: 'not a string' }] as PartListUnion,
] as PartListUnion); new AbortController().signal,
);
expect(handled).toBe(false); expect(handled).toBe(false);
expect(mockAddItemToHistory).not.toHaveBeenCalled(); expect(mockAddItemToHistory).not.toHaveBeenCalled();
}); });
@ -115,7 +116,10 @@ describe('useShellCommandProcessor', () => {
it('should handle empty shell command', () => { it('should handle empty shell command', () => {
const { result } = setupHook(); const { result } = setupHook();
act(() => { act(() => {
const handled = result.current.handleShellCommand(''); const handled = result.current.handleShellCommand(
'',
new AbortController().signal,
);
expect(handled).toBe(true); expect(handled).toBe(true);
}); });
expect(mockAddItemToHistory).toHaveBeenNthCalledWith( expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
@ -144,7 +148,7 @@ describe('useShellCommandProcessor', () => {
existsSyncSpy.mockReturnValue(false); existsSyncSpy.mockReturnValue(false);
await act(async () => { await act(async () => {
result.current.handleShellCommand(command); result.current.handleShellCommand(command, new AbortController().signal);
await new Promise(process.nextTick); await new Promise(process.nextTick);
}); });
@ -182,7 +186,7 @@ describe('useShellCommandProcessor', () => {
existsSyncSpy.mockReturnValue(false); existsSyncSpy.mockReturnValue(false);
await act(async () => { await act(async () => {
result.current.handleShellCommand(command); result.current.handleShellCommand(command, new AbortController().signal);
await new Promise(process.nextTick); await new Promise(process.nextTick);
}); });
expect(mockAddItemToHistory).toHaveBeenNthCalledWith( expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
@ -210,7 +214,7 @@ describe('useShellCommandProcessor', () => {
existsSyncSpy.mockReturnValue(false); existsSyncSpy.mockReturnValue(false);
await act(async () => { await act(async () => {
result.current.handleShellCommand(command); result.current.handleShellCommand(command, new AbortController().signal);
await new Promise(process.nextTick); await new Promise(process.nextTick);
}); });
expect(mockAddItemToHistory).toHaveBeenNthCalledWith( expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
@ -237,7 +241,7 @@ describe('useShellCommandProcessor', () => {
existsSyncSpy.mockReturnValue(false); existsSyncSpy.mockReturnValue(false);
await act(async () => { await act(async () => {
result.current.handleShellCommand(command); result.current.handleShellCommand(command, new AbortController().signal);
await new Promise(process.nextTick); await new Promise(process.nextTick);
}); });
expect(mockAddItemToHistory).toHaveBeenNthCalledWith( expect(mockAddItemToHistory).toHaveBeenNthCalledWith(
@ -264,7 +268,7 @@ describe('useShellCommandProcessor', () => {
existsSyncSpy.mockReturnValue(false); existsSyncSpy.mockReturnValue(false);
await act(async () => { await act(async () => {
result.current.handleShellCommand(command); result.current.handleShellCommand(command, new AbortController().signal);
await new Promise(process.nextTick); await new Promise(process.nextTick);
}); });

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

@ -37,7 +37,7 @@ export const useShellCommandProcessor = (
* @returns True if the query was handled as a shell command, false otherwise. * @returns True if the query was handled as a shell command, false otherwise.
*/ */
const handleShellCommand = useCallback( const handleShellCommand = useCallback(
(rawQuery: PartListUnion, signal?: AbortSignal): boolean => { (rawQuery: PartListUnion, abortSignal: AbortSignal): boolean => {
if (typeof rawQuery !== 'string') { if (typeof rawQuery !== 'string') {
return false; return false;
} }
@ -149,27 +149,19 @@ export const useShellCommandProcessor = (
error = err; error = err;
}); });
const abortHandler = () => { const abortHandler = async () => {
if (child.pid && !exited) { if (child.pid && !exited) {
onDebugMessage( onDebugMessage(
`Aborting shell command (PID: ${child.pid}) due to signal.`, `Aborting shell command (PID: ${child.pid}) due to signal.`,
); );
try { try {
// attempt to SIGTERM process group (negative PID) // attempt to SIGTERM process group (negative PID)
// if SIGTERM fails after 200ms, attempt SIGKILL // fall back to SIGKILL (to group) after 200ms
process.kill(-child.pid, 'SIGTERM'); process.kill(-child.pid, 'SIGTERM');
setTimeout(() => { await new Promise((resolve) => setTimeout(resolve, 200));
// if SIGTERM fails, attempt SIGKILL if (child.pid && !exited) {
try { process.kill(-child.pid, 'SIGKILL');
if (child.pid && !exited) { }
process.kill(-child.pid, 'SIGKILL');
}
} catch (_e) {
console.error(
`failed to kill shell process ${child.pid}: ${_e}`,
);
}
}, 200);
} catch (_e) { } catch (_e) {
// if group kill fails, fall back to killing just the main process // if group kill fails, fall back to killing just the main process
try { try {
@ -185,20 +177,11 @@ export const useShellCommandProcessor = (
} }
}; };
if (signal) { abortSignal.addEventListener('abort', abortHandler, { once: true });
if (signal.aborted) {
abortHandler();
// No need to add listener if already aborted
} else {
signal.addEventListener('abort', abortHandler, { once: true });
}
}
child.on('exit', (code, processSignal) => { child.on('exit', (code, signal) => {
exited = true; exited = true;
if (signal) { abortSignal.removeEventListener('abort', abortHandler);
signal.removeEventListener('abort', abortHandler);
}
setPendingHistoryItem(null); setPendingHistoryItem(null);
output = output.trim() || '(Command produced no output)'; output = output.trim() || '(Command produced no output)';
if (error) { if (error) {
@ -207,7 +190,7 @@ export const useShellCommandProcessor = (
} else if (code !== null && code !== 0) { } else if (code !== null && code !== 0) {
const text = `Command exited with code ${code}\n${output}`; const text = `Command exited with code ${code}\n${output}`;
addItemToHistory({ type: 'error', text }, userMessageTimestamp); addItemToHistory({ type: 'error', text }, userMessageTimestamp);
} else if (signal?.aborted) { } else if (abortSignal.aborted) {
addItemToHistory( addItemToHistory(
{ {
type: 'info', type: 'info',
@ -215,9 +198,8 @@ export const useShellCommandProcessor = (
}, },
userMessageTimestamp, userMessageTimestamp,
); );
} else if (processSignal) { } else if (signal) {
const text = `Command terminated with signal ${processSignal} const text = `Command terminated with signal ${signal}.\n${output}`;
${output}`;
addItemToHistory({ type: 'error', text }, userMessageTimestamp); addItemToHistory({ type: 'error', text }, userMessageTimestamp);
} else { } else {
addItemToHistory( addItemToHistory(

12
packages/cli/src/ui/hooks/useGeminiStream.ts
View File

@ -161,7 +161,7 @@ export const useGeminiStream = (
async ( async (
query: PartListUnion, query: PartListUnion,
userMessageTimestamp: number, userMessageTimestamp: number,
signal: AbortSignal, abortSignal: AbortSignal,
): Promise<{ ): Promise<{
queryToSend: PartListUnion | null; queryToSend: PartListUnion | null;
shouldProceed: boolean; shouldProceed: boolean;
@ -199,7 +199,7 @@ export const useGeminiStream = (
return { queryToSend: null, shouldProceed: false }; // Handled by scheduling the tool return { queryToSend: null, shouldProceed: false }; // Handled by scheduling the tool
} }
if (shellModeActive && handleShellCommand(trimmedQuery, signal)) { if (shellModeActive && handleShellCommand(trimmedQuery, abortSignal)) {
return { queryToSend: null, shouldProceed: false }; return { queryToSend: null, shouldProceed: false };
} }
@ -211,7 +211,7 @@ export const useGeminiStream = (
addItem, addItem,
onDebugMessage, onDebugMessage,
messageId: userMessageTimestamp, messageId: userMessageTimestamp,
signal, signal: abortSignal,
}); });
if (!atCommandResult.shouldProceed) { if (!atCommandResult.shouldProceed) {
return { queryToSend: null, shouldProceed: false }; return { queryToSend: null, shouldProceed: false };
@ -493,12 +493,12 @@ export const useGeminiStream = (
setShowHelp(false); setShowHelp(false);
abortControllerRef.current = new AbortController(); abortControllerRef.current = new AbortController();
const signal = abortControllerRef.current.signal; const abortSignal = abortControllerRef.current.signal;
const { queryToSend, shouldProceed } = await prepareQueryForGemini( const { queryToSend, shouldProceed } = await prepareQueryForGemini(
query, query,
userMessageTimestamp, userMessageTimestamp,
signal, abortSignal,
); );
if (!shouldProceed || queryToSend === null) { if (!shouldProceed || queryToSend === null) {
@ -515,7 +515,7 @@ export const useGeminiStream = (
setInitError(null); setInitError(null);
try { try {
const stream = client.sendMessageStream(chat, queryToSend, signal); const stream = client.sendMessageStream(chat, queryToSend, abortSignal);
const processingStatus = await processGeminiStreamEvents( const processingStatus = await processGeminiStreamEvents(
stream, stream,
userMessageTimestamp, userMessageTimestamp,

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

@ -200,21 +200,16 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
}; };
shell.on('exit', exitHandler); shell.on('exit', exitHandler);
const abortHandler = () => { const abortHandler = async () => {
if (shell.pid && !exited) { if (shell.pid && !exited) {
try { try {
// attempt to SIGTERM process group (negative PID) // attempt to SIGTERM process group (negative PID)
// if SIGTERM fails after 200ms, attempt SIGKILL // fall back to SIGKILL (to group) after 200ms
process.kill(-shell.pid, 'SIGTERM'); process.kill(-shell.pid, 'SIGTERM');
setTimeout(() => { await new Promise((resolve) => setTimeout(resolve, 200));
try { if (shell.pid && !exited) {
if (shell.pid && !exited) { process.kill(-shell.pid, 'SIGKILL');
process.kill(-shell.pid, 'SIGKILL'); }
}
} catch (_e) {
console.error(`failed to kill shell process ${shell.pid}: ${_e}`);
}
}, 200);
} catch (_e) { } catch (_e) {
// if group kill fails, fall back to killing just the main process // if group kill fails, fall back to killing just the main process
try { try {