-
Notifications
You must be signed in to change notification settings - Fork 53
fix(terminal): terminate running process when task is cancelled (#245) #261
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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). | ||
| if (terminal.busy) { | ||
| try { | ||
| terminal.process?.abort() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| } | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
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), whileExecaTerminalProcess.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.