From ec528bdb0573650cf1016ee74488a6fda12c0bd9 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Thu, 17 Apr 2025 16:00:40 -0700 Subject: [PATCH 01/11] fix for stop button for execute bash --- .../core/src/amazonq/webview/ui/connector.ts | 1 + .../controllers/chat/controller.ts | 9 +- .../codewhispererChat/tools/executeBash.ts | 119 +++++++++++++++--- .../src/codewhispererChat/tools/toolUtils.ts | 20 ++- .../view/connector/connector.ts | 1 + 5 files changed, 114 insertions(+), 36 deletions(-) diff --git a/packages/core/src/amazonq/webview/ui/connector.ts b/packages/core/src/amazonq/webview/ui/connector.ts index feda513ef46..534875cd59a 100644 --- a/packages/core/src/amazonq/webview/ui/connector.ts +++ b/packages/core/src/amazonq/webview/ui/connector.ts @@ -744,6 +744,7 @@ export class Connector { tabType: 'cwc', }) } else { + console.log('messageId:', messageId, 'event Id:', eventId) this.cwChatConnector.onCustomFormAction(tabId, messageId ?? '', action, messageId ?? '') } break diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index a9711c3ab02..784f1542e23 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -709,6 +709,7 @@ export class ChatController { this.editorContextExtractor .extractContextForTrigger('ChatMessage') .then(async (context) => { + console.log('message for tool action:', message) const triggerID = message.triggerId // Check if this trigger has already been cancelled @@ -786,11 +787,7 @@ export class ChatController { return } - const output = await ToolUtils.invoke( - tool, - chatStream, - ConversationTracker.getInstance().getTokenForTrigger(triggerID) - ) + const output = await ToolUtils.invoke(tool, chatStream, triggerID) ToolUtils.validateOutput(output, tool.type) let status: ToolResultStatus = ToolResultStatus.SUCCESS @@ -1632,7 +1629,7 @@ export class ChatController { })}` ) let response: MessengerResponseType | undefined = undefined - session.createNewTokenSource() + // session.createNewTokenSource() // TODO: onProfileChanged, abort previous response? try { if (!session.context && triggerPayload.context.length) { diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 77390ebdb7d..e4a1fa95bd4 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -114,6 +114,7 @@ export interface ExecuteBashParams { command: string cwd?: string explanation?: string + triggerId?: string } export interface CommandValidation { @@ -134,10 +135,22 @@ export class ExecuteBash { private readonly workingDirectory?: string private readonly logger = getLogger('executeBash') private childProcess?: ChildProcess + // Make triggerId writable so it can be set after construction + private _triggerId?: string constructor(params: ExecuteBashParams) { this.command = params.command this.workingDirectory = params.cwd ? sanitizePath(params.cwd) : fs.getUserHomeDir() + this._triggerId = params.triggerId + } + + // Getter and setter for triggerId + get triggerId(): string | undefined { + return this._triggerId + } + + set triggerId(id: string | undefined) { + this._triggerId = id } public async validate(): Promise { @@ -232,18 +245,60 @@ export class ExecuteBash { } } - public async invoke(updates?: Writable, cancellationToken?: vscode.CancellationToken): Promise { + /** + * Check if the trigger has been cancelled using ConversationTracker + */ + private isTriggerCancelled(): boolean { + // console.log('trigger cancellation check is happening') + if (!this.triggerId) { + return false + } + + // Import here to avoid circular dependency + const { ConversationTracker } = require('../storages/conversationTracker') + const cancellationtracker = ConversationTracker.getInstance() + // console.log('trigger cancellation status:', cancellationtracker.getTokenForTrigger(this.triggerId)) + // console.log('concealltion status:', cancellationtracker.isTriggerCancelled(this.triggerId)) + return cancellationtracker.isTriggerCancelled(this.triggerId) + } + + public async invoke(updates?: Writable): Promise { this.logger.info(`Invoking bash command: "${this.command}" in cwd: "${this.workingDirectory}"`) return new Promise(async (resolve, reject) => { - // Check if cancelled before starting - if (cancellationToken?.isCancellationRequested) { + // Check if cancelled before starting using triggerId + if (this.isTriggerCancelled()) { this.logger.debug('Bash command execution cancelled before starting') reject(new Error('Command execution cancelled')) return } - this.logger.debug(`Spawning process with command: bash -c "${this.command}" (cwd=${this.workingDirectory})`) + // Modify the command to make it more cancellable by using process groups + // This ensures that when we kill the parent process, all child processes are also terminated + // The trap ensures cleanup on SIGTERM/SIGINT + const modifiedCommand = ` +exec bash -c " + # Create a new process group + set -m + + # Set up trap to kill the entire process group on exit + trap 'exit' TERM INT + + # Run the actual command in background + ${this.command} & + + # Store the PID + CMD_PID=\\$! + + # Wait for the command to finish + wait \\$CMD_PID + exit \\$? +" +` + + this.logger.debug( + `Spawning process with modified command for better cancellation support (cwd=${this.workingDirectory})` + ) const stdoutBuffer: string[] = [] const stderrBuffer: string[] = [] @@ -282,16 +337,41 @@ export class ExecuteBash { } } + // Setup a periodic check for trigger cancellation + let checkCancellationInterval: NodeJS.Timeout | undefined + if (this.triggerId) { + checkCancellationInterval = setInterval(() => { + if (this.isTriggerCancelled()) { + this.logger.debug('Trigger cancellation detected, killing child process') + this.childProcess?.stop(false, 'SIGTERM') + + // After a short delay, force kill with SIGKILL if still running + setTimeout(() => { + if (this.childProcess && !this.childProcess.stopped) { + this.logger.debug('Process still running after SIGTERM, sending SIGKILL') + this.childProcess.stop(true, 'SIGKILL') + } + }, 500) + + if (checkCancellationInterval) { + clearInterval(checkCancellationInterval) + } + } + }, 100) // Check every 100ms + } + const childProcessOptions: ChildProcessOptions = { spawnOptions: { cwd: this.workingDirectory, stdio: ['pipe', 'pipe', 'pipe'], + // Set detached to true to create a new process group + detached: true, }, collect: false, waitForStreams: true, onStdout: async (chunk: string) => { - if (cancellationToken?.isCancellationRequested) { - this.logger.debug('Bash command execution cancelled during stderr processing') + if (this.isTriggerCancelled()) { + this.logger.debug('Bash command execution cancelled during stdout processing') return } const isFirst = getAndSetFirstChunk(false) @@ -305,7 +385,7 @@ export class ExecuteBash { processQueue() }, onStderr: async (chunk: string) => { - if (cancellationToken?.isCancellationRequested) { + if (this.isTriggerCancelled()) { this.logger.debug('Bash command execution cancelled during stderr processing') return } @@ -321,21 +401,19 @@ export class ExecuteBash { }, } - this.childProcess = new ChildProcess('bash', ['-c', this.command], childProcessOptions) - - // Set up cancellation listener - if (cancellationToken) { - cancellationToken.onCancellationRequested(() => { - this.logger.debug('Cancellation requested, killing child process') - this.childProcess?.stop() - }) - } + // Use bash directly with the modified command + this.childProcess = new ChildProcess('bash', ['-c', modifiedCommand], childProcessOptions) try { const result = await this.childProcess.run() + // Clean up the interval if it exists + if (checkCancellationInterval) { + clearInterval(checkCancellationInterval) + } + // Check if cancelled after execution - if (cancellationToken?.isCancellationRequested) { + if (this.isTriggerCancelled()) { this.logger.debug('Bash command execution cancelled after completion') reject(new Error('Command execution cancelled')) return @@ -368,8 +446,13 @@ export class ExecuteBash { }, }) } catch (err: any) { + // Clean up the interval if it exists + if (checkCancellationInterval) { + clearInterval(checkCancellationInterval) + } + // Check if this was due to cancellation - if (cancellationToken?.isCancellationRequested) { + if (this.isTriggerCancelled()) { reject(new Error('Command execution cancelled')) } else { this.logger.error(`Failed to execute bash command '${this.command}': ${err.message}`) diff --git a/packages/core/src/codewhispererChat/tools/toolUtils.ts b/packages/core/src/codewhispererChat/tools/toolUtils.ts index 300d357a9fa..85522e35d44 100644 --- a/packages/core/src/codewhispererChat/tools/toolUtils.ts +++ b/packages/core/src/codewhispererChat/tools/toolUtils.ts @@ -2,7 +2,6 @@ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. * SPDX-License-Identifier: Apache-2.0 */ -import * as vscode from 'vscode' import { Writable } from 'stream' import { FsRead, FsReadParams } from './fsRead' import { FsWrite, FsWriteParams } from './fsWrite' @@ -15,6 +14,7 @@ import { fsReadToolResponseSize, } from './toolShared' import { ListDirectory, ListDirectoryParams } from './listDirectory' +import { ConversationTracker } from '../storages/conversationTracker' export enum ToolType { FsRead = 'fsRead', @@ -56,23 +56,19 @@ export class ToolUtils { } } - static async invoke( - tool: Tool, - updates?: Writable, - cancellationToken?: vscode.CancellationToken - ): Promise { - // Check if cancelled before executing - if (cancellationToken?.isCancellationRequested) { - throw new Error('Tool execution cancelled') - } - + static async invoke(tool: Tool, updates?: Writable, triggerId?: string): Promise { switch (tool.type) { case ToolType.FsRead: return tool.tool.invoke(updates) case ToolType.FsWrite: return tool.tool.invoke(updates) case ToolType.ExecuteBash: - return tool.tool.invoke(updates ?? undefined, cancellationToken) + // If triggerId is provided, update the tool's triggerId + if (triggerId) { + ;(tool.tool as ExecuteBash).triggerId = + ConversationTracker.getInstance().getTriggerIdForToolUseId(triggerId) + } + return tool.tool.invoke(updates) case ToolType.ListDirectory: return tool.tool.invoke(updates) } diff --git a/packages/core/src/codewhispererChat/view/connector/connector.ts b/packages/core/src/codewhispererChat/view/connector/connector.ts index a37c6279585..deb544ec27a 100644 --- a/packages/core/src/codewhispererChat/view/connector/connector.ts +++ b/packages/core/src/codewhispererChat/view/connector/connector.ts @@ -518,6 +518,7 @@ export class AppToWebViewMessageDispatcher { } public sendCustomFormActionMessage(message: CustomFormActionMessage) { + console.log('sendCustomFormActionMessage', message) this.appsToWebViewMessagePublisher.publish(message) } From 950b0f4263d03bf58294ab038031a4ab8011f872 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Thu, 17 Apr 2025 18:13:02 -0700 Subject: [PATCH 02/11] fix for stopping bash executions --- .../webview/ui/apps/cwChatConnector.ts | 15 +++++- .../core/src/amazonq/webview/ui/connector.ts | 1 - .../controllers/chat/controller.ts | 2 - .../codewhispererChat/tools/executeBash.ts | 48 +++++++++++++++---- .../src/codewhispererChat/tools/toolUtils.ts | 4 +- .../view/connector/connector.ts | 1 - .../core/src/shared/utilities/processUtils.ts | 29 ++++++++++- 7 files changed, 81 insertions(+), 19 deletions(-) diff --git a/packages/core/src/amazonq/webview/ui/apps/cwChatConnector.ts b/packages/core/src/amazonq/webview/ui/apps/cwChatConnector.ts index 3b19598adeb..993d7b17699 100644 --- a/packages/core/src/amazonq/webview/ui/apps/cwChatConnector.ts +++ b/packages/core/src/amazonq/webview/ui/apps/cwChatConnector.ts @@ -44,6 +44,7 @@ export class Connector extends BaseConnector { private readonly onChatAnswerUpdated private readonly onAsyncEventProgress private chatItems: Map> = new Map() // tabId -> messageId -> ChatItem + private tabIdToTriggerId: Map = new Map() // tabId -> triggerId override getTabType(): TabType { return 'cwc' @@ -135,6 +136,10 @@ export class Connector extends BaseConnector { } } + if (messageData.tabID && messageData.triggerID && messageData.triggerID !== '') { + this.storetriggerId(messageData.tabID, messageData.triggerID) + } + if (answer.messageId) { this.storeChatItem(messageData.tabID, answer.messageId, answer) } @@ -213,6 +218,14 @@ export class Connector extends BaseConnector { this.chatItems.get(tabId)?.set(messageId, { ...item }) } + private storetriggerId(tabId: string, triggerId: string): void { + this.tabIdToTriggerId.set(tabId, triggerId) + } + + private getTriggerId(tabId: string): string | undefined { + return this.tabIdToTriggerId.get(tabId) + } + private getCurrentChatItem(tabId: string, messageId: string | undefined): ChatItem | undefined { if (!messageId) { return @@ -351,7 +364,7 @@ export class Connector extends BaseConnector { formSelectedValues: action.formItemValues, tabType: this.getTabType(), tabID: tabId, - triggerId: triggerId, + triggerId: this.getTriggerId(tabId), }) if ( diff --git a/packages/core/src/amazonq/webview/ui/connector.ts b/packages/core/src/amazonq/webview/ui/connector.ts index 534875cd59a..feda513ef46 100644 --- a/packages/core/src/amazonq/webview/ui/connector.ts +++ b/packages/core/src/amazonq/webview/ui/connector.ts @@ -744,7 +744,6 @@ export class Connector { tabType: 'cwc', }) } else { - console.log('messageId:', messageId, 'event Id:', eventId) this.cwChatConnector.onCustomFormAction(tabId, messageId ?? '', action, messageId ?? '') } break diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index 784f1542e23..29b71a1f52c 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -709,9 +709,7 @@ export class ChatController { this.editorContextExtractor .extractContextForTrigger('ChatMessage') .then(async (context) => { - console.log('message for tool action:', message) const triggerID = message.triggerId - // Check if this trigger has already been cancelled if (this.isTriggerCancelled(triggerID)) { return diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index e4a1fa95bd4..92b4eba6091 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -249,7 +249,6 @@ export class ExecuteBash { * Check if the trigger has been cancelled using ConversationTracker */ private isTriggerCancelled(): boolean { - // console.log('trigger cancellation check is happening') if (!this.triggerId) { return false } @@ -257,8 +256,6 @@ export class ExecuteBash { // Import here to avoid circular dependency const { ConversationTracker } = require('../storages/conversationTracker') const cancellationtracker = ConversationTracker.getInstance() - // console.log('trigger cancellation status:', cancellationtracker.getTokenForTrigger(this.triggerId)) - // console.log('concealltion status:', cancellationtracker.isTriggerCancelled(this.triggerId)) return cancellationtracker.isTriggerCancelled(this.triggerId) } @@ -275,24 +272,26 @@ export class ExecuteBash { // Modify the command to make it more cancellable by using process groups // This ensures that when we kill the parent process, all child processes are also terminated - // The trap ensures cleanup on SIGTERM/SIGINT + // The trap ensures cleanup on SIGTERM/SIGINT and sends SIGTERM to the child process group const modifiedCommand = ` exec bash -c " # Create a new process group set -m # Set up trap to kill the entire process group on exit - trap 'exit' TERM INT + trap 'kill -TERM -\\$CMD_PID 2>/dev/null || true; exit' TERM INT # Run the actual command in background - ${this.command} & + # Use '()' to create a subshell which becomes the process group leader + (${this.command}) & # Store the PID CMD_PID=\\$! # Wait for the command to finish wait \\$CMD_PID - exit \\$? + exit_code=\\$? + exit \\$exit_code " ` @@ -343,13 +342,40 @@ exec bash -c " checkCancellationInterval = setInterval(() => { if (this.isTriggerCancelled()) { this.logger.debug('Trigger cancellation detected, killing child process') - this.childProcess?.stop(false, 'SIGTERM') + + // First try to kill the entire process group + if (this.childProcess && this.childProcess.pid) { + try { + // On Unix systems, negative PID kills the process group + this.logger.debug(`Sending SIGTERM to process group -${this.childProcess.pid}`) + process.kill(-this.childProcess.pid, 'SIGTERM') + } catch (err) { + this.logger.debug(`Failed to kill process group: ${err}`) + // Fall back to regular process termination + this.childProcess?.stop(false, 'SIGTERM') + } + } else { + this.childProcess?.stop(false, 'SIGTERM') + } // After a short delay, force kill with SIGKILL if still running setTimeout(() => { if (this.childProcess && !this.childProcess.stopped) { this.logger.debug('Process still running after SIGTERM, sending SIGKILL') - this.childProcess.stop(true, 'SIGKILL') + + // Try to kill the process group with SIGKILL + if (this.childProcess.pid) { + try { + this.logger.debug(`Sending SIGKILL to process group -${this.childProcess.pid}`) + process.kill(-this.childProcess.pid, 'SIGKILL') + } catch (err) { + this.logger.debug(`Failed to kill process group with SIGKILL: ${err}`) + // Fall back to regular process termination + this.childProcess.stop(true, 'SIGKILL') + } + } else { + this.childProcess.stop(true, 'SIGKILL') + } } }, 500) @@ -365,7 +391,11 @@ exec bash -c " cwd: this.workingDirectory, stdio: ['pipe', 'pipe', 'pipe'], // Set detached to true to create a new process group + // This allows us to kill the entire process group later detached: true, + // On Windows, we need to create a new process group + // On Unix, we need to create a new session + ...(process.platform === 'win32' ? { windowsVerbatimArguments: true } : {}), }, collect: false, waitForStreams: true, diff --git a/packages/core/src/codewhispererChat/tools/toolUtils.ts b/packages/core/src/codewhispererChat/tools/toolUtils.ts index 85522e35d44..1a286b74777 100644 --- a/packages/core/src/codewhispererChat/tools/toolUtils.ts +++ b/packages/core/src/codewhispererChat/tools/toolUtils.ts @@ -14,7 +14,6 @@ import { fsReadToolResponseSize, } from './toolShared' import { ListDirectory, ListDirectoryParams } from './listDirectory' -import { ConversationTracker } from '../storages/conversationTracker' export enum ToolType { FsRead = 'fsRead', @@ -65,8 +64,7 @@ export class ToolUtils { case ToolType.ExecuteBash: // If triggerId is provided, update the tool's triggerId if (triggerId) { - ;(tool.tool as ExecuteBash).triggerId = - ConversationTracker.getInstance().getTriggerIdForToolUseId(triggerId) + ;(tool.tool as ExecuteBash).triggerId = triggerId } return tool.tool.invoke(updates) case ToolType.ListDirectory: diff --git a/packages/core/src/codewhispererChat/view/connector/connector.ts b/packages/core/src/codewhispererChat/view/connector/connector.ts index deb544ec27a..a37c6279585 100644 --- a/packages/core/src/codewhispererChat/view/connector/connector.ts +++ b/packages/core/src/codewhispererChat/view/connector/connector.ts @@ -518,7 +518,6 @@ export class AppToWebViewMessageDispatcher { } public sendCustomFormActionMessage(message: CustomFormActionMessage) { - console.log('sendCustomFormActionMessage', message) this.appsToWebViewMessagePublisher.publish(message) } diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index 31f2ec238f3..14333718d97 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -426,13 +426,38 @@ export class ChildProcess { const command = this.#command const pid = this.pid() if (!this.stopped) { - child.kill(signal) + // Try to kill the process group if possible (Unix systems only) + if (process.platform !== 'win32' && child.pid && child.spawnargs && child.spawnfile) { + try { + // On Unix systems, negative PID kills the process group + this.#log.debug(`Attempting to kill process group -${child.pid} with ${signal || 'SIGTERM'}`) + process.kill(-child.pid, signal || 'SIGTERM') + } catch (err) { + this.#log.debug(`Failed to kill process group: ${err}, falling back to regular kill`) + child.kill(signal) + } + } else { + child.kill(signal) + } if (force === true) { waitUntil(async () => this.stopped, { timeout: ChildProcess.stopTimeout, interval: 200, truthy: true }) .then((stopped) => { if (!stopped) { - child.kill('SIGKILL') + // Try to kill the process group with SIGKILL if possible + if (process.platform !== 'win32' && child.pid) { + try { + this.#log.debug(`Attempting to kill process group -${child.pid} with SIGKILL`) + process.kill(-child.pid, 'SIGKILL') + } catch (err) { + this.#log.debug( + `Failed to kill process group with SIGKILL: ${err}, falling back to regular kill` + ) + child.kill('SIGKILL') + } + } else { + child.kill('SIGKILL') + } } }) .catch((e) => { From 7074cd8dcc54d8d5d48eb5c095b1706dc5b9d5f3 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Thu, 17 Apr 2025 18:16:49 -0700 Subject: [PATCH 03/11] removing commented out line --- .../core/src/codewhispererChat/controllers/chat/controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index 29b71a1f52c..bd40520934c 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -710,6 +710,7 @@ export class ChatController { .extractContextForTrigger('ChatMessage') .then(async (context) => { const triggerID = message.triggerId + // Check if this trigger has already been cancelled if (this.isTriggerCancelled(triggerID)) { return @@ -1627,7 +1628,6 @@ export class ChatController { })}` ) let response: MessengerResponseType | undefined = undefined - // session.createNewTokenSource() // TODO: onProfileChanged, abort previous response? try { if (!session.context && triggerPayload.context.length) { From 6640ea90c9cfcbd849796895132f320f0004e48f Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Thu, 17 Apr 2025 20:03:14 -0700 Subject: [PATCH 04/11] fix for exiting fastly from cancellation --- .../src/codewhispererChat/controllers/chat/controller.ts | 9 +++++++++ packages/core/src/codewhispererChat/tools/executeBash.ts | 5 +++++ 2 files changed, 14 insertions(+) diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index bd40520934c..1c707730a30 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -787,6 +787,7 @@ export class ChatController { } const output = await ToolUtils.invoke(tool, chatStream, triggerID) + console.log('output', output) ToolUtils.validateOutput(output, tool.type) let status: ToolResultStatus = ToolResultStatus.SUCCESS @@ -804,6 +805,10 @@ export class ChatController { status, }) } catch (e: any) { + if (this.isTriggerCancelled(triggerID)) { + getLogger().debug(`Tool execution cancelled before invoke for tabID: ${tabID}`) + return + } toolResults.push({ content: [{ text: e.message }], toolUseId: toolUse.toolUseId, @@ -811,6 +816,10 @@ export class ChatController { }) } } else { + if (this.isTriggerCancelled(triggerID)) { + getLogger().debug(`Tool execution cancelled before invoke for tabID: ${tabID}`) + return + } const toolResult: ToolResult = result toolResults.push(toolResult) } diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 92b4eba6091..7473d673711 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -382,6 +382,11 @@ exec bash -c " if (checkCancellationInterval) { clearInterval(checkCancellationInterval) } + + // Return from the function after cancellation + reject(new Error('Command execution cancelled')) + console.log('exiting the cancellation') + return } }, 100) // Check every 100ms } From 1eede2dec251d3b54eb228afb503ced0e8fb7911 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Thu, 17 Apr 2025 20:51:12 -0700 Subject: [PATCH 05/11] fix for lint errors --- .../src/codewhispererChat/controllers/chat/controller.ts | 1 - packages/core/src/codewhispererChat/tools/executeBash.ts | 7 ++++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/codewhispererChat/controllers/chat/controller.ts b/packages/core/src/codewhispererChat/controllers/chat/controller.ts index 1c707730a30..1666ddc801d 100644 --- a/packages/core/src/codewhispererChat/controllers/chat/controller.ts +++ b/packages/core/src/codewhispererChat/controllers/chat/controller.ts @@ -787,7 +787,6 @@ export class ChatController { } const output = await ToolUtils.invoke(tool, chatStream, triggerID) - console.log('output', output) ToolUtils.validateOutput(output, tool.type) let status: ToolResultStatus = ToolResultStatus.SUCCESS diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 7473d673711..1aab00edf58 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -348,7 +348,8 @@ exec bash -c " try { // On Unix systems, negative PID kills the process group this.logger.debug(`Sending SIGTERM to process group -${this.childProcess.pid}`) - process.kill(-this.childProcess.pid, 'SIGTERM') + const pid = -this.childProcess.pid + process.kill(pid, 'SIGTERM') } catch (err) { this.logger.debug(`Failed to kill process group: ${err}`) // Fall back to regular process termination @@ -367,7 +368,8 @@ exec bash -c " if (this.childProcess.pid) { try { this.logger.debug(`Sending SIGKILL to process group -${this.childProcess.pid}`) - process.kill(-this.childProcess.pid, 'SIGKILL') + const pid = -this.childProcess.pid + process.kill(pid, 'SIGKILL') } catch (err) { this.logger.debug(`Failed to kill process group with SIGKILL: ${err}`) // Fall back to regular process termination @@ -385,7 +387,6 @@ exec bash -c " // Return from the function after cancellation reject(new Error('Command execution cancelled')) - console.log('exiting the cancellation') return } }, 100) // Check every 100ms From 255ba0d0ec453c1bc73d429a58aeafb0d91fa688 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 09:00:06 -0700 Subject: [PATCH 06/11] fixes for lint --- packages/core/src/codewhispererChat/tools/executeBash.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 1aab00edf58..ebcfab02106 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -347,8 +347,8 @@ exec bash -c " if (this.childProcess && this.childProcess.pid) { try { // On Unix systems, negative PID kills the process group - this.logger.debug(`Sending SIGTERM to process group -${this.childProcess.pid}`) const pid = -this.childProcess.pid + this.logger.debug(`Sending SIGTERM to process group ${pid}`) process.kill(pid, 'SIGTERM') } catch (err) { this.logger.debug(`Failed to kill process group: ${err}`) @@ -367,8 +367,8 @@ exec bash -c " // Try to kill the process group with SIGKILL if (this.childProcess.pid) { try { - this.logger.debug(`Sending SIGKILL to process group -${this.childProcess.pid}`) const pid = -this.childProcess.pid + this.logger.debug(`Sending SIGKILL to process group ${pid}`) process.kill(pid, 'SIGKILL') } catch (err) { this.logger.debug(`Failed to kill process group with SIGKILL: ${err}`) From c96e64f7e29d34a41b8b82ba9ca1852a0423aa01 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 09:34:56 -0700 Subject: [PATCH 07/11] fix for lint erros --- packages/core/src/codewhispererChat/tools/executeBash.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index ebcfab02106..22885d6f7bf 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -347,7 +347,8 @@ exec bash -c " if (this.childProcess && this.childProcess.pid) { try { // On Unix systems, negative PID kills the process group - const pid = -this.childProcess.pid + const childProcessPid = this.childProcess.pid + const pid = -childProcessPid this.logger.debug(`Sending SIGTERM to process group ${pid}`) process.kill(pid, 'SIGTERM') } catch (err) { @@ -367,7 +368,8 @@ exec bash -c " // Try to kill the process group with SIGKILL if (this.childProcess.pid) { try { - const pid = -this.childProcess.pid + const childProcessPid = this.childProcess.pid + const pid = -childProcessPid this.logger.debug(`Sending SIGKILL to process group ${pid}`) process.kill(pid, 'SIGKILL') } catch (err) { From 64ee304036554600bca550f68ffc4789c277dff8 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 09:56:15 -0700 Subject: [PATCH 08/11] Removed unnecessary pid checks --- .../codewhispererChat/tools/executeBash.ts | 33 ++----------------- 1 file changed, 3 insertions(+), 30 deletions(-) diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 22885d6f7bf..0ba0fb6b511 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -343,22 +343,8 @@ exec bash -c " if (this.isTriggerCancelled()) { this.logger.debug('Trigger cancellation detected, killing child process') - // First try to kill the entire process group - if (this.childProcess && this.childProcess.pid) { - try { - // On Unix systems, negative PID kills the process group - const childProcessPid = this.childProcess.pid - const pid = -childProcessPid - this.logger.debug(`Sending SIGTERM to process group ${pid}`) - process.kill(pid, 'SIGTERM') - } catch (err) { - this.logger.debug(`Failed to kill process group: ${err}`) - // Fall back to regular process termination - this.childProcess?.stop(false, 'SIGTERM') - } - } else { - this.childProcess?.stop(false, 'SIGTERM') - } + // Kill the process + this.childProcess?.stop(false, 'SIGTERM') // After a short delay, force kill with SIGKILL if still running setTimeout(() => { @@ -366,20 +352,7 @@ exec bash -c " this.logger.debug('Process still running after SIGTERM, sending SIGKILL') // Try to kill the process group with SIGKILL - if (this.childProcess.pid) { - try { - const childProcessPid = this.childProcess.pid - const pid = -childProcessPid - this.logger.debug(`Sending SIGKILL to process group ${pid}`) - process.kill(pid, 'SIGKILL') - } catch (err) { - this.logger.debug(`Failed to kill process group with SIGKILL: ${err}`) - // Fall back to regular process termination - this.childProcess.stop(true, 'SIGKILL') - } - } else { - this.childProcess.stop(true, 'SIGKILL') - } + this.childProcess.stop(true, 'SIGKILL') } }, 500) From f6d4804c7aac6bc5c4822d0524482571c1a13c79 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 19:23:15 -0700 Subject: [PATCH 09/11] Import correction --- packages/core/src/codewhispererChat/tools/executeBash.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 0ba0fb6b511..4a038e4c415 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -12,6 +12,7 @@ import { split } from 'shlex' import path from 'path' import * as vscode from 'vscode' import { isInDirectory } from '../../shared/filesystemUtilities' +import { ConversationTracker } from '../storages/conversationTracker' export enum CommandCategory { ReadOnly, @@ -252,9 +253,6 @@ export class ExecuteBash { if (!this.triggerId) { return false } - - // Import here to avoid circular dependency - const { ConversationTracker } = require('../storages/conversationTracker') const cancellationtracker = ConversationTracker.getInstance() return cancellationtracker.isTriggerCancelled(this.triggerId) } From 3932b0ec57022a4a714551aa5dc5b856a0b84dce Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 19:41:48 -0700 Subject: [PATCH 10/11] Increase check interval to 500 ms --- packages/core/src/codewhispererChat/tools/executeBash.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/codewhispererChat/tools/executeBash.ts b/packages/core/src/codewhispererChat/tools/executeBash.ts index 4a038e4c415..829e190037e 100644 --- a/packages/core/src/codewhispererChat/tools/executeBash.ts +++ b/packages/core/src/codewhispererChat/tools/executeBash.ts @@ -362,7 +362,7 @@ exec bash -c " reject(new Error('Command execution cancelled')) return } - }, 100) // Check every 100ms + }, 500) // Check every 500ms } const childProcessOptions: ChildProcessOptions = { From 7ba7cc6900981908fa12b17b4c420fd9df6aa3d9 Mon Sep 17 00:00:00 2001 From: Ashish Reddy Podduturi Date: Fri, 18 Apr 2025 20:07:15 -0700 Subject: [PATCH 11/11] fix to dedupe process stop --- .../core/src/shared/utilities/processUtils.ts | 64 +++++++++++-------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts index 14333718d97..96245437684 100644 --- a/packages/core/src/shared/utilities/processUtils.ts +++ b/packages/core/src/shared/utilities/processUtils.ts @@ -409,6 +409,41 @@ export class ChildProcess { return this.#childProcess?.exitCode ?? -1 } + /** + * Attempts to kill a process group on Unix systems, falling back to regular kill if it fails + * @param child The child process to kill + * @param signal The signal to send (defaults to SIGTERM) + * @param checkSpawnProps Whether to check for child.spawnargs and child.spawnfile (used in first call only) + * @returns true if process group kill was attempted, false if regular kill was used + */ + public killProcessGroup( + child: proc.ChildProcess, + signal: NodeJS.Signals = 'SIGTERM', + checkSpawnProps: boolean = false + ): boolean { + // First call in stop() uses the stricter condition with spawnargs/spawnfile checks + // Second call in the force kill uses the less restrictive condition + const condition = checkSpawnProps + ? process.platform !== 'win32' && child.spawnargs && child.spawnfile + : process.platform !== 'win32' + + if (condition && child.pid) { + try { + // On Unix systems, negative PID kills the process group + this.#log.debug(`Attempting to kill process group -${child.pid} with ${signal}`) + process.kill(-child.pid, signal) + return true + } catch (err) { + this.#log.debug(`Failed to kill process group with ${signal}: ${err}, falling back to regular kill`) + child.kill(signal) + return false + } + } else { + child.kill(signal) + return false + } + } + /** * Stops the process. * @@ -427,37 +462,16 @@ export class ChildProcess { const pid = this.pid() if (!this.stopped) { // Try to kill the process group if possible (Unix systems only) - if (process.platform !== 'win32' && child.pid && child.spawnargs && child.spawnfile) { - try { - // On Unix systems, negative PID kills the process group - this.#log.debug(`Attempting to kill process group -${child.pid} with ${signal || 'SIGTERM'}`) - process.kill(-child.pid, signal || 'SIGTERM') - } catch (err) { - this.#log.debug(`Failed to kill process group: ${err}, falling back to regular kill`) - child.kill(signal) - } - } else { - child.kill(signal) - } + // First call uses the stricter condition with spawnargs/spawnfile checks + this.killProcessGroup(child, signal || 'SIGTERM', true) if (force === true) { waitUntil(async () => this.stopped, { timeout: ChildProcess.stopTimeout, interval: 200, truthy: true }) .then((stopped) => { if (!stopped) { // Try to kill the process group with SIGKILL if possible - if (process.platform !== 'win32' && child.pid) { - try { - this.#log.debug(`Attempting to kill process group -${child.pid} with SIGKILL`) - process.kill(-child.pid, 'SIGKILL') - } catch (err) { - this.#log.debug( - `Failed to kill process group with SIGKILL: ${err}, falling back to regular kill` - ) - child.kill('SIGKILL') - } - } else { - child.kill('SIGKILL') - } + // Second call uses the less restrictive condition + this.killProcessGroup(child, 'SIGKILL', false) } }) .catch((e) => {