From 3608330ce6c0ba3ec8cdd4080253feb24c5328db Mon Sep 17 00:00:00 2001 From: Roomote Date: Mon, 24 Aug 2026 03:56:52 +0000 Subject: [PATCH] fix(terminal): finalize commands when terminal closes --- src/integrations/terminal/Terminal.ts | 67 +++++++++- src/integrations/terminal/TerminalProcess.ts | 17 +++ src/integrations/terminal/TerminalRegistry.ts | 13 +- .../__tests__/TerminalRegistry.spec.ts | 122 ++++++++++++++++++ 4 files changed, 208 insertions(+), 11 deletions(-) diff --git a/src/integrations/terminal/Terminal.ts b/src/integrations/terminal/Terminal.ts index 21f98b86c6..fc80dd311f 100644 --- a/src/integrations/terminal/Terminal.ts +++ b/src/integrations/terminal/Terminal.ts @@ -11,6 +11,8 @@ import { mergePromise } from "./mergePromise" export class Terminal extends BaseTerminal { public terminal: vscode.Terminal + private closed = false + private cancelShellIntegrationWait?: () => void public cmdCounter: number = 0 @@ -74,7 +76,23 @@ export class Terminal extends BaseTerminal { * active. (This value is set when onDidCloseTerminal is fired.) */ public override isClosed(): boolean { - return this.terminal.exitStatus !== undefined + return this.closed || this.terminal.exitStatus !== undefined + } + + public handleClose(): void { + if (this.closed) { + return + } + + this.closed = true + this.cancelShellIntegrationWait?.() + this.cancelShellIntegrationWait = undefined + + if (this.process instanceof TerminalProcess) { + this.process.handleTerminalClosed() + } else { + this.shellExecutionComplete({ exitCode: undefined }) + } } public override runCommand(command: string, callbacks: RooTerminalCallbacks): RooTerminalProcessResultPromise { @@ -123,6 +141,14 @@ export class Terminal extends BaseTerminal { // customised startup that suppresses the OSC 633;A marker). this.waitForShellIntegration(Terminal.getShellIntegrationTimeout()) .then(() => { + if (this.isClosed()) { + if (this.process === process) { + process.handleTerminalClosed() + } + + return + } + // Clean up temporary directory if shell integration is available, zsh did its job: ShellIntegrationManager.zshCleanupTmpDir(this.id) @@ -130,6 +156,14 @@ export class Terminal extends BaseTerminal { void process.run(command).catch((error) => process.emit("error", error)) }) .catch(() => { + if (this.isClosed()) { + if (this.process === process) { + process.handleTerminalClosed() + } + + return + } + console.log(`[Terminal ${this.id}] Shell integration not available. Command execution aborted.`) // Clean up temporary directory if shell integration is not available @@ -153,22 +187,43 @@ export class Terminal extends BaseTerminal { * than polling — important for slow-starting shells (heavy .zshrc, nvm, etc.). */ private waitForShellIntegration(timeoutMs: number): Promise { + if (this.isClosed()) { + return Promise.reject(new Error("Terminal closed before shell integration became available")) + } + if (this.terminal.shellIntegration) { return Promise.resolve() } return new Promise((resolve, reject) => { const ref = { disposable: null as vscode.Disposable | null } - const timer = setTimeout(() => { + let settled = false + let cancel = () => {} + const finish = (callback: () => void) => { + if (settled) { + return + } + + settled = true + clearTimeout(timer) ref.disposable?.dispose() - reject(new Error(`Shell integration did not activate within ${timeoutMs / 1000}s`)) + + if (this.cancelShellIntegrationWait === cancel) { + this.cancelShellIntegrationWait = undefined + } + + callback() + } + const timer = setTimeout(() => { + finish(() => reject(new Error(`Shell integration did not activate within ${timeoutMs / 1000}s`))) }, timeoutMs) + cancel = () => finish(() => reject(new Error("Terminal closed before shell integration became available"))) + this.cancelShellIntegrationWait = cancel + ref.disposable = vscode.window.onDidChangeTerminalShellIntegration((e) => { if (e.terminal === this.terminal) { - clearTimeout(timer) - ref.disposable?.dispose() - resolve() + finish(resolve) } }) }) diff --git a/src/integrations/terminal/TerminalProcess.ts b/src/integrations/terminal/TerminalProcess.ts index d1643dec3a..991318697c 100644 --- a/src/integrations/terminal/TerminalProcess.ts +++ b/src/integrations/terminal/TerminalProcess.ts @@ -58,6 +58,23 @@ export class TerminalProcess extends BaseTerminalProcess { return terminal } + public handleTerminalClosed(): void { + const executionStarted = this.ownExecution !== undefined + this.terminal.shellExecutionComplete({ exitCode: undefined }) + + if (executionStarted) { + return + } + + // run() has not installed its completion listener yet, so finish the + // startup-wait path directly instead of leaving runCommand() pending. + this.terminal.activeShellExecution = undefined + this.cleanupScriptFile() + this.stopHotTimer() + this.emit("completed", "") + this.emit("continue") + } + public override async run(command: string) { this.command = command diff --git a/src/integrations/terminal/TerminalRegistry.ts b/src/integrations/terminal/TerminalRegistry.ts index da4b3dd16d..d7385af1b5 100644 --- a/src/integrations/terminal/TerminalRegistry.ts +++ b/src/integrations/terminal/TerminalRegistry.ts @@ -33,13 +33,16 @@ export class TerminalRegistry { // TODO: This initialization code is VSCode specific, and therefore // should probably live elsewhere. - // Register handler for terminal close events to clean up temporary - // directories. + // Treat terminal closure as a completion path because VS Code may not emit + // onDidEndTerminalShellExecution after the terminal is disposed. const closeDisposable = vscode.window.onDidCloseTerminal((vsceTerminal) => { - const terminal = this.getTerminalByVSCETerminal(vsceTerminal) + // Do not use getTerminalByVSCETerminal here: exitStatus is already set when + // this event fires, so that helper removes closed terminals before returning. + const terminal = this.terminals.find((t) => t instanceof Terminal && t.terminal === vsceTerminal) - if (terminal) { - ShellIntegrationManager.zshCleanupTmpDir(terminal.id) + if (terminal instanceof Terminal) { + terminal.handleClose() + this.removeTerminal(terminal.id) } }) diff --git a/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts b/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts index f60c0d0722..47ea940bec 100644 --- a/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts +++ b/src/integrations/terminal/__tests__/TerminalRegistry.spec.ts @@ -209,6 +209,8 @@ describe("TerminalRegistry", () => { }) describe("onDidEndTerminalShellExecution race condition (#489, #622)", () => { + let closeHandler: (terminal: vscode.Terminal) => void + let shellIntegrationHandler: (event: vscode.TerminalShellIntegrationChangeEvent) => void let startHandler: (e: any) => Promise let endHandler: (e: any) => Promise @@ -221,6 +223,15 @@ describe("TerminalRegistry", () => { ;(vscode.window as any).onDidStartTerminalShellExecution ??= () => ({ dispose: () => {} }) ;(vscode.window as any).onDidEndTerminalShellExecution ??= () => ({ dispose: () => {} }) + vi.spyOn(vscode.window, "onDidCloseTerminal").mockImplementation((handler) => { + closeHandler = handler + return { dispose: vi.fn() } + }) + vi.spyOn(vscode.window, "onDidChangeTerminalShellIntegration").mockImplementation((handler) => { + shellIntegrationHandler = handler + return { dispose: vi.fn() } + }) + vi.spyOn(vscode.window, "onDidStartTerminalShellExecution" as any).mockImplementation((handler: any) => { startHandler = handler return { dispose: vi.fn() } @@ -291,6 +302,117 @@ describe("TerminalRegistry", () => { expect(completeSpy).not.toHaveBeenCalled() }) + it("finalizes an active process when its terminal closes (#1362)", () => { + const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal + const process = new TerminalProcess(terminal) + process.ownExecution = { commandLine: { value: "git status" } } as vscode.TerminalShellExecution + terminal.process = process + terminal.busy = true + terminal.running = true + const completionSpy = vi.fn() + process.on("shell_execution_complete", completionSpy) + Object.defineProperty(terminal.terminal, "exitStatus", { + value: { code: undefined, reason: 3 }, + configurable: true, + }) + + closeHandler(terminal.terminal) + + expect(completionSpy).toHaveBeenCalledOnce() + expect(completionSpy).toHaveBeenCalledWith({ exitCode: undefined }) + expect(terminal.process).toBeUndefined() + expect(terminal.busy).toBe(false) + expect(terminal.running).toBe(false) + }) + + it("unblocks a process when its terminal closes while shell integration is initializing (#1362)", async () => { + const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal + const completedSpy = vi.fn() + const completionSpy = vi.fn() + const noShellIntegrationSpy = vi.fn() + Object.defineProperty(terminal.terminal, "shellIntegration", { value: undefined, configurable: true }) + const result = terminal.runCommand("git status", { + onLine: vi.fn(), + onCompleted: completedSpy, + onShellExecutionStarted: vi.fn(), + onShellExecutionComplete: completionSpy, + onNoShellIntegration: noShellIntegrationSpy, + }) + Object.defineProperty(terminal.terminal, "exitStatus", { + value: { code: undefined, reason: 3 }, + configurable: true, + }) + + closeHandler(terminal.terminal) + await result + + expect(completionSpy).toHaveBeenCalledOnce() + expect(completedSpy).toHaveBeenCalledOnce() + expect(noShellIntegrationSpy).not.toHaveBeenCalled() + expect(terminal.process).toBeUndefined() + expect(terminal.busy).toBe(false) + }) + + it("does not submit a command when shell integration resolves immediately before terminal closure", async () => { + const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal + const executeCommand = vi.fn(() => { + throw new Error("command should not execute after terminal closure") + }) + const completedSpy = vi.fn() + const completionSpy = vi.fn() + const noShellIntegrationSpy = vi.fn() + Object.defineProperty(terminal.terminal, "shellIntegration", { value: undefined, configurable: true }) + const result = terminal.runCommand("git status", { + onLine: vi.fn(), + onCompleted: completedSpy, + onShellExecutionStarted: vi.fn(), + onShellExecutionComplete: completionSpy, + onNoShellIntegration: noShellIntegrationSpy, + }) + Object.defineProperty(terminal.terminal, "shellIntegration", { + value: { executeCommand }, + configurable: true, + }) + + shellIntegrationHandler({ + terminal: terminal.terminal, + shellIntegration: terminal.terminal.shellIntegration!, + }) + Object.defineProperty(terminal.terminal, "exitStatus", { + value: { code: undefined, reason: 3 }, + configurable: true, + }) + closeHandler(terminal.terminal) + await result + + expect(executeCommand).not.toHaveBeenCalled() + expect(completionSpy).toHaveBeenCalledOnce() + expect(completedSpy).toHaveBeenCalledOnce() + expect(noShellIntegrationSpy).not.toHaveBeenCalled() + }) + + it("does not finalize a process twice when its terminal closes after the end event", async () => { + const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal + const execution = { commandLine: { value: "git status" } } as vscode.TerminalShellExecution + const process = new TerminalProcess(terminal) + process.ownExecution = execution + terminal.process = process + terminal.busy = true + terminal.running = true + const completionSpy = vi.fn() + process.on("shell_execution_complete", completionSpy) + + await endHandler({ terminal: terminal.terminal, execution, exitCode: 0 }) + Object.defineProperty(terminal.terminal, "exitStatus", { + value: { code: 0, reason: 2 }, + configurable: true, + }) + closeHandler(terminal.terminal) + + expect(completionSpy).toHaveBeenCalledOnce() + expect(completionSpy).toHaveBeenCalledWith(expect.objectContaining({ exitCode: 0 })) + }) + it( "ignores a late end event for a superseded execution instead of completing " + "the next command on the same reused terminal",