From 7620277e6cbcda4874fe698a418b4ab3a8bd7fc7 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 30 Jul 2025 19:40:55 -0500 Subject: [PATCH 1/2] fix: Kill button now works by finding actual command PID The issue was that when using shell: true, execa returns the shell's PID, not the actual command's PID (e.g., sleep). The shell often exits immediately after spawning the command, making the stored PID invalid. Fix: 1. Store subprocess reference as instance variable to enable subprocess.kill() 2. After spawning, use psTree to find child processes and update PID to the actual command 3. In abort(), try both subprocess.kill() and direct process.kill() on the updated PID 4. Add stream wrapper to handle TypeScript types (execa can return string or Uint8Array) This seems to be broken since PR #3136 which switched from AbortController to manual process killing but didn't account for the shell vs command PID difference. --- .../terminal/ExecaTerminalProcess.ts | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/src/integrations/terminal/ExecaTerminalProcess.ts b/src/integrations/terminal/ExecaTerminalProcess.ts index 1c48d88aa67..0de0e9d6e40 100644 --- a/src/integrations/terminal/ExecaTerminalProcess.ts +++ b/src/integrations/terminal/ExecaTerminalProcess.ts @@ -9,6 +9,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { private terminalRef: WeakRef private aborted = false private pid?: number + private subprocess?: ReturnType constructor(terminal: RooTerminal) { super() @@ -36,7 +37,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { try { this.isHot = true - const subprocess = execa({ + this.subprocess = execa({ shell: true, cwd: this.terminal.getCurrentWorkingDirectory(), all: true, @@ -48,9 +49,32 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { }, })`${command}` - this.pid = subprocess.pid - const stream = subprocess.iterable({ from: "all", preserveNewlines: true }) - this.terminal.setActiveStream(stream, subprocess.pid) + this.pid = this.subprocess.pid + + // When using shell: true, the PID is for the shell, not the actual command + // Find the actual command PID after a small delay + if (this.pid) { + setTimeout(() => { + psTree(this.pid!, (err, children) => { + if (!err && children.length > 0) { + // Update PID to the first child (the actual command) + const actualPid = parseInt(children[0].PID) + this.pid = actualPid + } + }) + }, 100) + } + + const rawStream = this.subprocess.iterable({ from: "all", preserveNewlines: true }) + + // Wrap the stream to ensure all chunks are strings (execa can return Uint8Array) + const stream = (async function* () { + for await (const chunk of rawStream) { + yield typeof chunk === "string" ? chunk : new TextDecoder().decode(chunk) + } + })() + + this.terminal.setActiveStream(stream, this.subprocess.pid) for await (const line of stream) { if (this.aborted) { @@ -77,7 +101,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { timeoutId = setTimeout(() => { try { - subprocess.kill("SIGKILL") + this.subprocess?.kill("SIGKILL") } catch (e) {} resolve() @@ -85,7 +109,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { }) try { - await Promise.race([subprocess, kill]) + await Promise.race([this.subprocess, kill]) } catch (error) { console.log( `[ExecaTerminalProcess#run] subprocess termination error: ${error instanceof Error ? error.message : String(error)}`, @@ -116,6 +140,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { this.stopHotTimer() this.emit("completed", this.fullOutput) this.emit("continue") + this.subprocess = undefined } public override continue() { @@ -127,7 +152,24 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { public override abort() { this.aborted = true + // Try to kill using the subprocess object + if (this.subprocess) { + try { + this.subprocess.kill("SIGKILL") + } catch (e) { + // Ignore errors, will try direct kill below + } + } + + // Kill the stored PID (which should be the actual command after our update) if (this.pid) { + try { + process.kill(this.pid, "SIGKILL") + } catch (e) { + // Process might already be dead + } + + // Also check for any child processes psTree(this.pid, async (err, children) => { if (!err) { const pids = children.map((p) => parseInt(p.PID)) @@ -148,15 +190,6 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { ) } }) - - try { - console.error(`[ExecaTerminalProcess#abort] SIGKILL parent -> ${this.pid}`) - process.kill(this.pid, "SIGKILL") - } catch (e) { - console.warn( - `[ExecaTerminalProcess#abort] Failed to send SIGKILL to main PID ${this.pid}: ${e instanceof Error ? e.message : String(e)}`, - ) - } } } From ed8dcf4140862ac8a9bfcc569c532589e36b169b Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 30 Jul 2025 19:55:45 -0500 Subject: [PATCH 2/2] fix: address review comments for kill button PR - Use promise instead of setTimeout to avoid race condition - Add validation for parseInt to handle NaN cases - Fix setActiveStream to use updated PID instead of subprocess.pid - Clear subprocess reference in error cases - Add logging for kill operation failures --- .../terminal/ExecaTerminalProcess.ts | 70 +++++++++++++------ 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/src/integrations/terminal/ExecaTerminalProcess.ts b/src/integrations/terminal/ExecaTerminalProcess.ts index 0de0e9d6e40..2f8ebfa7a87 100644 --- a/src/integrations/terminal/ExecaTerminalProcess.ts +++ b/src/integrations/terminal/ExecaTerminalProcess.ts @@ -10,6 +10,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { private aborted = false private pid?: number private subprocess?: ReturnType + private pidUpdatePromise?: Promise constructor(terminal: RooTerminal) { super() @@ -54,15 +55,20 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { // When using shell: true, the PID is for the shell, not the actual command // Find the actual command PID after a small delay if (this.pid) { - setTimeout(() => { - psTree(this.pid!, (err, children) => { - if (!err && children.length > 0) { - // Update PID to the first child (the actual command) - const actualPid = parseInt(children[0].PID) - this.pid = actualPid - } - }) - }, 100) + this.pidUpdatePromise = new Promise((resolve) => { + setTimeout(() => { + psTree(this.pid!, (err, children) => { + if (!err && children.length > 0) { + // Update PID to the first child (the actual command) + const actualPid = parseInt(children[0].PID) + if (!isNaN(actualPid)) { + this.pid = actualPid + } + } + resolve() + }) + }, 100) + }) } const rawStream = this.subprocess.iterable({ from: "all", preserveNewlines: true }) @@ -74,7 +80,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { } })() - this.terminal.setActiveStream(stream, this.subprocess.pid) + this.terminal.setActiveStream(stream, this.pid) for await (const line of stream) { if (this.aborted) { @@ -133,6 +139,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { this.emit("shell_execution_complete", { exitCode: 1 }) } + this.subprocess = undefined } this.terminal.setActiveStream(undefined) @@ -152,23 +159,40 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { public override abort() { this.aborted = true - // Try to kill using the subprocess object - if (this.subprocess) { - try { - this.subprocess.kill("SIGKILL") - } catch (e) { - // Ignore errors, will try direct kill below + // Function to perform the kill operations + const performKill = () => { + // Try to kill using the subprocess object + if (this.subprocess) { + try { + this.subprocess.kill("SIGKILL") + } catch (e) { + console.warn( + `[ExecaTerminalProcess#abort] Failed to kill subprocess: ${e instanceof Error ? e.message : String(e)}`, + ) + } } - } - // Kill the stored PID (which should be the actual command after our update) - if (this.pid) { - try { - process.kill(this.pid, "SIGKILL") - } catch (e) { - // Process might already be dead + // Kill the stored PID (which should be the actual command after our update) + if (this.pid) { + try { + process.kill(this.pid, "SIGKILL") + } catch (e) { + console.warn( + `[ExecaTerminalProcess#abort] Failed to kill process ${this.pid}: ${e instanceof Error ? e.message : String(e)}`, + ) + } } + } + // If PID update is in progress, wait for it before killing + if (this.pidUpdatePromise) { + this.pidUpdatePromise.then(performKill).catch(() => performKill()) + } else { + performKill() + } + + // Continue with the rest of the abort logic + if (this.pid) { // Also check for any child processes psTree(this.pid, async (err, children) => { if (!err) {