Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/terminate-process-on-cancel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"zoo-code": patch
---

Terminate the running command when a task is cancelled or torn down (#245). Pressing cancel (✕) now aborts the underlying terminal process instead of leaving it running while the terminal stays stuck "busy" until a manual kill.
16 changes: 16 additions & 0 deletions src/integrations/terminal/TerminalRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,22 @@ export class TerminalRegistry {
public static releaseTerminalsForTask(taskId: string): void {
this.terminals.forEach((terminal) => {
if (terminal.taskId === taskId) {
// #245: If the terminal is still executing a command when its task is torn
// down (user pressed cancel ✕, or the task was switched/removed), abort the
// process. Otherwise the command keeps running orphaned and the terminal stays
// stuck "busy" — the cancel-doesn't-terminate bug. abort() is safe when idle
// (Ctrl+C is gated on an active stream; Execa abort is idempotent).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR description says ExecaTerminalProcess.abort() does "SIGINT → SIGKILL", but looking at the implementation it sends SIGKILL directly (no SIGINT first). The comment here ("Execa abort is idempotent") is fine, but might be worth noting the asymmetry: TerminalProcess.abort() sends Ctrl+C (a single, soft signal), while ExecaTerminalProcess.abort() sends SIGKILL immediately. For long-running processes that need graceful shutdown, a single Ctrl+C or SIGINT before SIGKILL would be friendlier — but that's a pre-existing concern, not something this PR needs to fix.

if (terminal.busy) {
try {
terminal.process?.abort()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if terminal.busy is true but process is still undefined? ExecaTerminal.runCommand sets this.busy = true before assigning this.process — tiny window but possible. The ?. handles it (no-op), but also worth noting: TerminalProcess.abort() only sends a single Ctrl+C (\x03). If the running process requires multiple Ctrl+C presses to terminate (e.g. some interactive tools), this single abort won't be enough and the terminal will stay busy. Is there a retry/escalation path, or is single-Ctrl+C considered best-effort here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I created a followup issue for this: #266

} catch (error) {
console.error(
`[TerminalRegistry] Error aborting process for terminal ${terminal.id} on release:`,
error,
)
}
}

terminal.taskId = undefined
}
})
Expand Down
62 changes: 62 additions & 0 deletions src/integrations/terminal/__tests__/TerminalRegistry.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,66 @@ describe("TerminalRegistry", () => {
}
})
})

describe("releaseTerminalsForTask", () => {
it("aborts a busy terminal's running process and disassociates it from the task (#245)", () => {
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode")
const abort = vi.fn()
terminal.taskId = "task-245"
terminal.busy = true
terminal.process = { abort } as any

TerminalRegistry.releaseTerminalsForTask("task-245")

expect(abort).toHaveBeenCalledTimes(1)
expect(terminal.taskId).toBeUndefined()
})

it("does not abort an idle (not busy) terminal but still disassociates it", () => {
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode")
const abort = vi.fn()
terminal.taskId = "task-idle"
terminal.busy = false
terminal.process = { abort } as any

TerminalRegistry.releaseTerminalsForTask("task-idle")

expect(abort).not.toHaveBeenCalled()
expect(terminal.taskId).toBeUndefined()
})

it("only releases terminals belonging to the given task", () => {
const a = TerminalRegistry.createTerminal("/a", "vscode")
const b = TerminalRegistry.createTerminal("/b", "vscode")
const abortA = vi.fn()
const abortB = vi.fn()
a.taskId = "task-A"
a.busy = true
a.process = { abort: abortA } as any
b.taskId = "task-B"
b.busy = true
b.process = { abort: abortB } as any

TerminalRegistry.releaseTerminalsForTask("task-A")

expect(abortA).toHaveBeenCalledTimes(1)
expect(a.taskId).toBeUndefined()
expect(abortB).not.toHaveBeenCalled()
expect(b.taskId).toBe("task-B")
})

it("swallows errors thrown by process.abort() and still disassociates the terminal", () => {
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode")
terminal.taskId = "task-throw"
terminal.busy = true
terminal.process = {
abort: vi.fn(() => {
throw new Error("boom")
}),
} as any

expect(() => TerminalRegistry.releaseTerminalsForTask("task-throw")).not.toThrow()
expect(terminal.taskId).toBeUndefined()
})
})
})
Loading