Skip to content

Fix: Kill button for execute_command tool #6457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 31, 2025
Merged

Fix: Kill button for execute_command tool #6457

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7620277
fix: Kill button now works by finding actual command PID
daniel-lxs Jul 31, 2025
ed8dcf4
fix: address review comments for kill button PR
daniel-lxs Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 72 additions & 15 deletions src/integrations/terminal/ExecaTerminalProcess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
private terminalRef: WeakRef<RooTerminal>
private aborted = false
private pid?: number
private subprocess?: ReturnType<typeof execa>
private pidUpdatePromise?: Promise<void>

constructor(terminal: RooTerminal) {
super()
Expand Down Expand Up @@ -36,7 +38,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
try {
this.isHot = true

const subprocess = execa({
this.subprocess = execa({
shell: true,
cwd: this.terminal.getCurrentWorkingDirectory(),
all: true,
Expand All @@ -48,9 +50,37 @@ 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) {
this.pidUpdatePromise = new Promise<void>((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 })

// 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.pid)

for await (const line of stream) {
if (this.aborted) {
Expand All @@ -77,15 +107,15 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {

timeoutId = setTimeout(() => {
try {
subprocess.kill("SIGKILL")
this.subprocess?.kill("SIGKILL")
} catch (e) {}

resolve()
}, 5_000)
})

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)}`,
Expand All @@ -109,13 +139,15 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {

this.emit("shell_execution_complete", { exitCode: 1 })
}
this.subprocess = undefined
}

this.terminal.setActiveStream(undefined)
this.emitRemainingBufferIfListening()
this.stopHotTimer()
this.emit("completed", this.fullOutput)
this.emit("continue")
this.subprocess = undefined
Copy link
Contributor

@roomote roomote bot Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subprocess reference is only cleared on normal completion. In error cases (lines 125-136), this could prevent garbage collection. Should we also clear it in the catch block?

}

public override continue() {
Expand All @@ -127,7 +159,41 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
public override abort() {
this.aborted = true

// 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) {
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) {
const pids = children.map((p) => parseInt(p.PID))
Expand All @@ -148,15 +214,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)}`,
)
}
}
}

Expand Down