-
Notifications
You must be signed in to change notification settings - Fork 266
[Fix] Commands stay Running when user closes their terminal #1363
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -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<void> | ||
| let endHandler: (e: any) => Promise<void> | ||
|
|
||
|
|
@@ -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() | ||
|
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Assert the completion callback payload. These assertions only count As per path instructions: “Reject weak assertions on values that could take multiple forms.” Also applies to: 390-390 🤖 Prompt for AI AgentsSource: Path instructions |
||
| 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", | ||
|
|
||
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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Exercise closure after stream output.
This test creates
TerminalProcessdirectly. It does not executeTerminalProcess.run(). It cannot verify iterator release or buffered-output delivery when the terminal closes after output arrives but before the end event.Add a regression test that drives a stream chunk, closes the terminal before
onDidEndTerminalShellExecution, awaits the command result, and asserts the output and iterator cleanup.As per coding guidelines: “For regressions, add the test at the lowest layer that would have failed.” As per path instructions: “Require regression coverage at the lowest valid harness with behavior-focused assertions.”
🤖 Prompt for AI Agents
Sources: Coding guidelines, Path instructions