Skip to content

Commit 62f95ec

Browse files
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
1 parent af30ae9 commit 62f95ec

3 files changed

Lines changed: 27 additions & 16 deletions

File tree

.changeset/multiple-ctrl-c-terminate.md

Lines changed: 0 additions & 5 deletions
This file was deleted.

src/integrations/terminal/TerminalProcess.ts

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ import { Terminal } from "./Terminal"
88
export class TerminalProcess extends BaseTerminalProcess {
99
// #266: Some processes (interactive tools, programs that trap SIGINT and
1010
// prompt for confirmation) need more than one Ctrl+C to actually exit. We
11-
// re-send Ctrl+C up to this many times, checking between attempts whether
12-
// the process has exited, before giving up and letting dispose() proceed.
13-
private static readonly ABORT_MAX_ATTEMPTS = 3
11+
// send Ctrl+C up to this many times in TOTAL — the immediate send in abort()
12+
// plus retries — checking between sends whether the process has exited, before
13+
// giving up and letting dispose() proceed.
14+
private static readonly CTRL_C_SEND_LIMIT = 3
1415
// Delay between Ctrl+C re-sends. Kept short so cancel stays responsive; the
15-
// whole retry window is bounded by ABORT_MAX_ATTEMPTS * ABORT_RETRY_DELAY_MS.
16+
// retry window is bounded by (CTRL_C_SEND_LIMIT - 1) * ABORT_RETRY_DELAY_MS.
1617
private static readonly ABORT_RETRY_DELAY_MS = 500
1718

1819
private terminalRef: WeakRef<Terminal>
@@ -290,15 +291,22 @@ export class TerminalProcess extends BaseTerminalProcess {
290291
}
291292

292293
/**
293-
* Re-sends Ctrl+C up to ABORT_MAX_ATTEMPTS times, waiting ABORT_RETRY_DELAY_MS
294-
* between attempts and stopping early once the process exits (or once we stop
295-
* listening). Bounded so it can never loop indefinitely.
294+
* Re-sends Ctrl+C after the immediate send in abort(), up to CTRL_C_SEND_LIMIT
295+
* total sends, waiting ABORT_RETRY_DELAY_MS between sends and stopping early once
296+
* the process exits (or once we stop listening). Bounded so it can never loop
297+
* indefinitely.
296298
*/
297299
private async retryAbort(): Promise<void> {
298-
for (let attempt = 1; attempt < TerminalProcess.ABORT_MAX_ATTEMPTS; attempt++) {
300+
// abort() already sent Ctrl+C once, so `sent` starts at 1; re-send until we
301+
// reach CTRL_C_SEND_LIMIT total.
302+
for (let sent = 1; sent < TerminalProcess.CTRL_C_SEND_LIMIT; sent++) {
299303
await new Promise((resolve) => setTimeout(resolve, TerminalProcess.ABORT_RETRY_DELAY_MS))
300304

301-
// Stop if the process already exited or we're no longer listening.
305+
// Stop as soon as there's nothing left to interrupt. `isListening` (cleared
306+
// by continue()) and `terminal.busy` (cleared by shellExecutionComplete() /
307+
// the "completed" event) are set on different code paths and can diverge, so
308+
// either one being false is a sufficient stop signal — we deliberately check
309+
// both rather than collapsing them into one.
302310
if (!this.isListening) {
303311
return
304312
}

src/integrations/terminal/__tests__/TerminalProcess.spec.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,12 @@ describe("TerminalProcess", () => {
187187
})
188188

189189
describe("abort", () => {
190-
const RETRY_DELAY_MS = 500
191-
const MAX_ATTEMPTS = 3
190+
// These MIRROR the private production constants in TerminalProcess.ts
191+
// (ABORT_RETRY_DELAY_MS and CTRL_C_SEND_LIMIT) — they can't be imported, so if
192+
// those values are ever tuned, update them here too or the timing assertions
193+
// below will keep passing while asserting the wrong cadence.
194+
const RETRY_DELAY_MS = 500 // mirrors ABORT_RETRY_DELAY_MS
195+
const MAX_ATTEMPTS = 3 // mirrors CTRL_C_SEND_LIMIT (total Ctrl+C sends)
192196

193197
beforeEach(() => {
194198
vi.useFakeTimers()
@@ -261,6 +265,10 @@ describe("TerminalProcess", () => {
261265
terminalProcess.abort()
262266

263267
// Two immediate Ctrl+C from the two abort() calls, but only one retry loop.
268+
// This count of 2 relies on the `aborting` guard being checked AFTER the
269+
// immediate sendText in abort(): the second call still fires its own Ctrl+C
270+
// before the guard short-circuits the duplicate retry loop. If the guard ever
271+
// moves above the send, this would drop to 1 immediate send (total 3, not 4).
264272
expect(mockTerminal.sendText).toHaveBeenCalledTimes(2)
265273

266274
await vi.advanceTimersByTimeAsync(RETRY_DELAY_MS * (MAX_ATTEMPTS + 2))

0 commit comments

Comments
 (0)