Skip to content

Commit 7620277

Browse files
committed
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.
1 parent cb6dcca commit 7620277

File tree

1 file changed

+48
-15
lines changed

1 file changed

+48
-15
lines changed

src/integrations/terminal/ExecaTerminalProcess.ts

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
99
private terminalRef: WeakRef<RooTerminal>
1010
private aborted = false
1111
private pid?: number
12+
private subprocess?: ReturnType<typeof execa>
1213

1314
constructor(terminal: RooTerminal) {
1415
super()
@@ -36,7 +37,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
3637
try {
3738
this.isHot = true
3839

39-
const subprocess = execa({
40+
this.subprocess = execa({
4041
shell: true,
4142
cwd: this.terminal.getCurrentWorkingDirectory(),
4243
all: true,
@@ -48,9 +49,32 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
4849
},
4950
})`${command}`
5051

51-
this.pid = subprocess.pid
52-
const stream = subprocess.iterable({ from: "all", preserveNewlines: true })
53-
this.terminal.setActiveStream(stream, subprocess.pid)
52+
this.pid = this.subprocess.pid
53+
54+
// When using shell: true, the PID is for the shell, not the actual command
55+
// Find the actual command PID after a small delay
56+
if (this.pid) {
57+
setTimeout(() => {
58+
psTree(this.pid!, (err, children) => {
59+
if (!err && children.length > 0) {
60+
// Update PID to the first child (the actual command)
61+
const actualPid = parseInt(children[0].PID)
62+
this.pid = actualPid
63+
}
64+
})
65+
}, 100)
66+
}
67+
68+
const rawStream = this.subprocess.iterable({ from: "all", preserveNewlines: true })
69+
70+
// Wrap the stream to ensure all chunks are strings (execa can return Uint8Array)
71+
const stream = (async function* () {
72+
for await (const chunk of rawStream) {
73+
yield typeof chunk === "string" ? chunk : new TextDecoder().decode(chunk)
74+
}
75+
})()
76+
77+
this.terminal.setActiveStream(stream, this.subprocess.pid)
5478

5579
for await (const line of stream) {
5680
if (this.aborted) {
@@ -77,15 +101,15 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
77101

78102
timeoutId = setTimeout(() => {
79103
try {
80-
subprocess.kill("SIGKILL")
104+
this.subprocess?.kill("SIGKILL")
81105
} catch (e) {}
82106

83107
resolve()
84108
}, 5_000)
85109
})
86110

87111
try {
88-
await Promise.race([subprocess, kill])
112+
await Promise.race([this.subprocess, kill])
89113
} catch (error) {
90114
console.log(
91115
`[ExecaTerminalProcess#run] subprocess termination error: ${error instanceof Error ? error.message : String(error)}`,
@@ -116,6 +140,7 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
116140
this.stopHotTimer()
117141
this.emit("completed", this.fullOutput)
118142
this.emit("continue")
143+
this.subprocess = undefined
119144
}
120145

121146
public override continue() {
@@ -127,7 +152,24 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
127152
public override abort() {
128153
this.aborted = true
129154

155+
// Try to kill using the subprocess object
156+
if (this.subprocess) {
157+
try {
158+
this.subprocess.kill("SIGKILL")
159+
} catch (e) {
160+
// Ignore errors, will try direct kill below
161+
}
162+
}
163+
164+
// Kill the stored PID (which should be the actual command after our update)
130165
if (this.pid) {
166+
try {
167+
process.kill(this.pid, "SIGKILL")
168+
} catch (e) {
169+
// Process might already be dead
170+
}
171+
172+
// Also check for any child processes
131173
psTree(this.pid, async (err, children) => {
132174
if (!err) {
133175
const pids = children.map((p) => parseInt(p.PID))
@@ -148,15 +190,6 @@ export class ExecaTerminalProcess extends BaseTerminalProcess {
148190
)
149191
}
150192
})
151-
152-
try {
153-
console.error(`[ExecaTerminalProcess#abort] SIGKILL parent -> ${this.pid}`)
154-
process.kill(this.pid, "SIGKILL")
155-
} catch (e) {
156-
console.warn(
157-
`[ExecaTerminalProcess#abort] Failed to send SIGKILL to main PID ${this.pid}: ${e instanceof Error ? e.message : String(e)}`,
158-
)
159-
}
160193
}
161194
}
162195

0 commit comments

Comments
 (0)