From 00377a9bdf01198354a5fef089b09d6b6c68744c Mon Sep 17 00:00:00 2001 From: Armando Vaquera <263793884+proyectoauraorg@users.noreply.github.com> Date: Sat, 23 May 2026 18:49:16 -0600 Subject: [PATCH 1/4] fix(terminal): retry Ctrl+C for processes needing multiple SIGINT (#266) Some processes (interactive tools, programs that trap SIGINT and prompt for confirmation) need more than one Ctrl+C to exit. The cancel path sent a single Ctrl+C then disposed immediately, leaving such processes running and the terminal stuck busy. abort() now kicks off a bounded, fire-and-forget retry that re-sends Ctrl+C up to 3 times (500ms apart), stopping early once the process exits or we stop listening. The synchronous cancel path is never blocked and the retry window is bounded so dispose() is never delayed indefinitely. The ExecaTerminal backend is unaffected (it sends SIGKILL directly). Follow-up to #245 / #261. Closes #266 --- .changeset/multiple-ctrl-c-terminate.md | 5 ++ src/integrations/terminal/TerminalProcess.ts | 57 ++++++++++++- .../__tests__/TerminalProcess.spec.ts | 84 +++++++++++++++++++ 3 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 .changeset/multiple-ctrl-c-terminate.md diff --git a/.changeset/multiple-ctrl-c-terminate.md b/.changeset/multiple-ctrl-c-terminate.md new file mode 100644 index 0000000000..45e551d83a --- /dev/null +++ b/.changeset/multiple-ctrl-c-terminate.md @@ -0,0 +1,5 @@ +--- +"zoo-code": patch +--- + +Retry Ctrl+C when cancelling a task so processes that ignore a single SIGINT actually terminate (#266). The terminal abort path now re-sends Ctrl+C a bounded number of times, verifying between attempts whether the process exited, before the terminal is torn down. Follow-up to #245/#261; the ExecaTerminal backend is unaffected. diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index d202191b95..40697ff48d 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -6,7 +6,19 @@ import { BaseTerminalProcess } from "./BaseTerminalProcess" import { Terminal } from "./Terminal" export class TerminalProcess extends BaseTerminalProcess { + // #266: Some processes (interactive tools, programs that trap SIGINT and + // prompt for confirmation) need more than one Ctrl+C to actually exit. We + // re-send Ctrl+C up to this many times, checking between attempts whether + // the process has exited, before giving up and letting dispose() proceed. + private static readonly ABORT_MAX_ATTEMPTS = 3 + // Delay between Ctrl+C re-sends. Kept short so cancel stays responsive; the + // whole retry window is bounded by ABORT_MAX_ATTEMPTS * ABORT_RETRY_DELAY_MS. + private static readonly ABORT_RETRY_DELAY_MS = 500 + private terminalRef: WeakRef + // Guards against overlapping abort retry loops if abort() is called again + // while a previous loop is still re-sending Ctrl+C. + private aborting = false constructor(terminal: Terminal) { super() @@ -256,9 +268,48 @@ export class TerminalProcess extends BaseTerminalProcess { } public override abort() { - if (this.isListening) { - // Send SIGINT using CTRL+C - this.terminal.terminal.sendText("\x03") + if (!this.isListening) { + return + } + + // Send SIGINT using CTRL+C. + this.terminal.terminal.sendText("\x03") + + // #266: A single Ctrl+C isn't always enough — some processes trap SIGINT + // and keep running. Kick off a bounded retry that re-sends Ctrl+C a few + // times, verifying between attempts whether the process actually exited + // (terminal.busy flips to false on completion). This is intentionally + // fire-and-forget so it never blocks the synchronous cancel path; the + // total retry window is bounded so dispose() is never delayed for long. + if (!this.aborting) { + this.aborting = true + void this.retryAbort().finally(() => { + this.aborting = false + }) + } + } + + /** + * Re-sends Ctrl+C up to ABORT_MAX_ATTEMPTS times, waiting ABORT_RETRY_DELAY_MS + * between attempts and stopping early once the process exits (or once we stop + * listening). Bounded so it can never loop indefinitely. + */ + private async retryAbort(): Promise { + for (let attempt = 1; attempt < TerminalProcess.ABORT_MAX_ATTEMPTS; attempt++) { + await new Promise((resolve) => setTimeout(resolve, TerminalProcess.ABORT_RETRY_DELAY_MS)) + + // Stop if the process already exited or we're no longer listening. + if (!this.isListening) { + return + } + + const terminal = this.terminalRef.deref() + + if (!terminal || !terminal.busy) { + return + } + + terminal.terminal.sendText("\x03") } } diff --git a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts index ba410b7158..761ea32a8d 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts @@ -186,6 +186,90 @@ describe("TerminalProcess", () => { }) }) + describe("abort", () => { + const RETRY_DELAY_MS = 500 + const MAX_ATTEMPTS = 3 + + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.runOnlyPendingTimers() + vi.useRealTimers() + }) + + it("sends a single Ctrl+C immediately and nothing else when the process exits (#266)", async () => { + // Process exits right away: terminal is no longer busy. + mockTerminalInfo.busy = false + + terminalProcess.abort() + + // Immediate Ctrl+C. + expect(mockTerminal.sendText).toHaveBeenCalledTimes(1) + expect(mockTerminal.sendText).toHaveBeenCalledWith("\x03") + + // Advance past the whole retry window; no further Ctrl+C since not busy. + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * MAX_ATTEMPTS) + expect(mockTerminal.sendText).toHaveBeenCalledTimes(1) + }) + + it("re-sends Ctrl+C up to the bounded maximum while the process stays busy (#266)", async () => { + // Process keeps ignoring SIGINT: terminal stays busy throughout. + mockTerminalInfo.busy = true + + terminalProcess.abort() + expect(mockTerminal.sendText).toHaveBeenCalledTimes(1) + + // Each retry tick re-sends Ctrl+C while still busy, bounded by MAX_ATTEMPTS. + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * (MAX_ATTEMPTS + 2)) + + expect(mockTerminal.sendText).toHaveBeenCalledTimes(MAX_ATTEMPTS) + expect(mockTerminal.sendText).toHaveBeenCalledWith("\x03") + }) + + it("stops re-sending Ctrl+C once the process exits mid-retry (#266)", async () => { + mockTerminalInfo.busy = true + + terminalProcess.abort() + expect(mockTerminal.sendText).toHaveBeenCalledTimes(1) + + // First retry tick: still busy, re-send. + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS) + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) + + // Process exits before the next tick. + mockTerminalInfo.busy = false + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * MAX_ATTEMPTS) + + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) + }) + + it("does nothing when the process is no longer listening (#266)", async () => { + terminalProcess["isListening"] = false + + terminalProcess.abort() + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * MAX_ATTEMPTS) + + expect(mockTerminal.sendText).not.toHaveBeenCalled() + }) + + it("does not start overlapping retry loops when abort() is called repeatedly (#266)", async () => { + mockTerminalInfo.busy = true + + terminalProcess.abort() + terminalProcess.abort() + + // Two immediate Ctrl+C from the two abort() calls, but only one retry loop. + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) + + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * (MAX_ATTEMPTS + 2)) + + // 2 immediate + (MAX_ATTEMPTS - 1) retries from the single loop. + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2 + (MAX_ATTEMPTS - 1)) + }) + }) + describe("getUnretrievedOutput", () => { it("returns and clears unretrieved output", () => { terminalProcess["fullOutput"] = `\x1b]633;C\x07previous\nnew output\x1b]633;D\x07` From 7b28cb2a70b70fab589291ec1f1f5d1fa3cdc016 Mon Sep 17 00:00:00 2001 From: Armando Vaquera <263793884+proyectoauraorg@users.noreply.github.com> Date: Sun, 24 May 2026 01:51:41 -0600 Subject: [PATCH 2/4] refactor(terminal): clarify Ctrl+C retry naming and comments per review (#266) - rename ABORT_MAX_ATTEMPTS -> CTRL_C_SEND_LIMIT (total sends) and start the retry loop at sent=1 so the bound reads naturally - document why both isListening and terminal.busy are checked - cross-reference the mirrored test constants to the production ones - note the double-abort send-count assumption in the test - drop the unused changeset --- .changeset/multiple-ctrl-c-terminate.md | 5 ---- src/integrations/terminal/TerminalProcess.ts | 26 ++++++++++++------- .../__tests__/TerminalProcess.spec.ts | 12 +++++++-- 3 files changed, 27 insertions(+), 16 deletions(-) delete mode 100644 .changeset/multiple-ctrl-c-terminate.md diff --git a/.changeset/multiple-ctrl-c-terminate.md b/.changeset/multiple-ctrl-c-terminate.md deleted file mode 100644 index 45e551d83a..0000000000 --- a/.changeset/multiple-ctrl-c-terminate.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"zoo-code": patch ---- - -Retry Ctrl+C when cancelling a task so processes that ignore a single SIGINT actually terminate (#266). The terminal abort path now re-sends Ctrl+C a bounded number of times, verifying between attempts whether the process exited, before the terminal is torn down. Follow-up to #245/#261; the ExecaTerminal backend is unaffected. diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index 40697ff48d..d42be5de27 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -8,11 +8,12 @@ import { Terminal } from "./Terminal" export class TerminalProcess extends BaseTerminalProcess { // #266: Some processes (interactive tools, programs that trap SIGINT and // prompt for confirmation) need more than one Ctrl+C to actually exit. We - // re-send Ctrl+C up to this many times, checking between attempts whether - // the process has exited, before giving up and letting dispose() proceed. - private static readonly ABORT_MAX_ATTEMPTS = 3 + // send Ctrl+C up to this many times in TOTAL — the immediate send in abort() + // plus retries — checking between sends whether the process has exited, before + // giving up and letting dispose() proceed. + private static readonly CTRL_C_SEND_LIMIT = 3 // Delay between Ctrl+C re-sends. Kept short so cancel stays responsive; the - // whole retry window is bounded by ABORT_MAX_ATTEMPTS * ABORT_RETRY_DELAY_MS. + // retry window is bounded by (CTRL_C_SEND_LIMIT - 1) * ABORT_RETRY_DELAY_MS. private static readonly ABORT_RETRY_DELAY_MS = 500 private terminalRef: WeakRef @@ -290,15 +291,22 @@ export class TerminalProcess extends BaseTerminalProcess { } /** - * Re-sends Ctrl+C up to ABORT_MAX_ATTEMPTS times, waiting ABORT_RETRY_DELAY_MS - * between attempts and stopping early once the process exits (or once we stop - * listening). Bounded so it can never loop indefinitely. + * Re-sends Ctrl+C after the immediate send in abort(), up to CTRL_C_SEND_LIMIT + * total sends, waiting ABORT_RETRY_DELAY_MS between sends and stopping early once + * the process exits (or once we stop listening). Bounded so it can never loop + * indefinitely. */ private async retryAbort(): Promise { - for (let attempt = 1; attempt < TerminalProcess.ABORT_MAX_ATTEMPTS; attempt++) { + // abort() already sent Ctrl+C once, so `sent` starts at 1; re-send until we + // reach CTRL_C_SEND_LIMIT total. + for (let sent = 1; sent < TerminalProcess.CTRL_C_SEND_LIMIT; sent++) { await new Promise((resolve) => setTimeout(resolve, TerminalProcess.ABORT_RETRY_DELAY_MS)) - // Stop if the process already exited or we're no longer listening. + // Stop as soon as there's nothing left to interrupt. `isListening` (cleared + // by continue()) and `terminal.busy` (cleared by shellExecutionComplete() / + // the "completed" event) are set on different code paths and can diverge, so + // either one being false is a sufficient stop signal — we deliberately check + // both rather than collapsing them into one. if (!this.isListening) { return } diff --git a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts index 761ea32a8d..2c7c4e0050 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts @@ -187,8 +187,12 @@ describe("TerminalProcess", () => { }) describe("abort", () => { - const RETRY_DELAY_MS = 500 - const MAX_ATTEMPTS = 3 + // These MIRROR the private production constants in TerminalProcess.ts + // (ABORT_RETRY_DELAY_MS and CTRL_C_SEND_LIMIT) — they can't be imported, so if + // those values are ever tuned, update them here too or the timing assertions + // below will keep passing while asserting the wrong cadence. + const RETRY_DELAY_MS = 500 // mirrors ABORT_RETRY_DELAY_MS + const MAX_ATTEMPTS = 3 // mirrors CTRL_C_SEND_LIMIT (total Ctrl+C sends) beforeEach(() => { vi.useFakeTimers() @@ -261,6 +265,10 @@ describe("TerminalProcess", () => { terminalProcess.abort() // Two immediate Ctrl+C from the two abort() calls, but only one retry loop. + // This count of 2 relies on the `aborting` guard being checked AFTER the + // immediate sendText in abort(): the second call still fires its own Ctrl+C + // before the guard short-circuits the duplicate retry loop. If the guard ever + // moves above the send, this would drop to 1 immediate send (total 3, not 4). expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * (MAX_ATTEMPTS + 2)) From 67a41818f33ed5fc53a2eb90f542d74b02a110ad Mon Sep 17 00:00:00 2001 From: Armando Vaquera <263793884+proyectoauraorg@users.noreply.github.com> Date: Sun, 24 May 2026 13:01:59 -0600 Subject: [PATCH 3/4] fix(terminal): skip Ctrl+C retry when terminal is reused by a different process (#266) --- src/integrations/terminal/TerminalProcess.ts | 7 ++++- .../__tests__/TerminalProcess.spec.ts | 28 +++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index d42be5de27..2d23e256fc 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -313,7 +313,12 @@ export class TerminalProcess extends BaseTerminalProcess { const terminal = this.terminalRef.deref() - if (!terminal || !terminal.busy) { + // Stop if the terminal is gone, idle, or has already moved on to a different + // command. If the original command exits and the terminal is reused before this + // tick fires, `terminal.busy` can be true for the NEW command while + // `terminal.process` points at a different TerminalProcess — re-sending Ctrl+C + // then would interrupt an unrelated command, so we bail out. + if (!terminal || !terminal.busy || terminal.process !== this) { return } diff --git a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts index 2c7c4e0050..890d76d6e6 100644 --- a/src/integrations/terminal/__tests__/TerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalProcess.spec.ts @@ -196,6 +196,9 @@ describe("TerminalProcess", () => { beforeEach(() => { vi.useFakeTimers() + // abort() runs against the terminal's *current* process; mirror that wiring so + // the reuse guard (terminal.process === this) lets the retry loop proceed. + mockTerminalInfo.process = terminalProcess }) afterEach(() => { @@ -242,8 +245,29 @@ describe("TerminalProcess", () => { await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS) expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) - // Process exits before the next tick. - mockTerminalInfo.busy = false + // Process exits before the next tick — drive the real completion lifecycle + // (shellExecutionComplete clears busy and releases terminal.process) rather than + // mutating busy directly, so the test exercises the production wiring. + mockTerminalInfo.shellExecutionComplete({ exitCode: 0 }) + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * MAX_ATTEMPTS) + + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) + }) + + it("stops re-sending Ctrl+C if the terminal is reused for a different process (#266)", async () => { + mockTerminalInfo.busy = true + + terminalProcess.abort() + expect(mockTerminal.sendText).toHaveBeenCalledTimes(1) + + // First retry tick: still busy, re-send. + await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS) + expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) + + // The original command exits and the terminal is reused for a NEW command before + // the next tick: terminal stays busy, but terminal.process now points at a + // different process. The retry must not interrupt that unrelated command. + mockTerminalInfo.process = new TestTerminalProcess(mockTerminalInfo) await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * MAX_ATTEMPTS) expect(mockTerminal.sendText).toHaveBeenCalledTimes(2) From 1cc5a85cded984200a2735a95d4e36058dd595ae Mon Sep 17 00:00:00 2001 From: edelauna <54631123+edelauna@users.noreply.github.com> Date: Tue, 26 May 2026 17:11:48 -0400 Subject: [PATCH 4/4] Update src/integrations/terminal/TerminalProcess.ts --- src/integrations/terminal/TerminalProcess.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index 2d23e256fc..465611c2f9 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -284,9 +284,11 @@ export class TerminalProcess extends BaseTerminalProcess { // total retry window is bounded so dispose() is never delayed for long. if (!this.aborting) { this.aborting = true - void this.retryAbort().finally(() => { - this.aborting = false - }) + void this.retryAbort() + .finally(() => { + this.aborting = false + }) + .catch((err) => console.error("[TerminalProcess] retryAbort error:", err)) } }