Skip to content

Commit e80039b

Browse files
authored
fix(TerminalRegistry): updating guard condition to address race condition for fast commands (#645)
1 parent 685f17e commit e80039b

2 files changed

Lines changed: 96 additions & 6 deletions

File tree

src/integrations/terminal/TerminalRegistry.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,21 @@ export class TerminalRegistry {
103103
}
104104

105105
if (!terminal.running) {
106-
console.error(
107-
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
108-
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
109-
)
106+
// The end event can arrive before setActiveStream() has set
107+
// running=true (race between the global VS Code event and the
108+
// synchronous call in TerminalProcess.run). If a process is
109+
// waiting for completion, deliver the signal so it doesn't
110+
// hang forever. See #489 / #622.
111+
if (process) {
112+
console.info(
113+
"[TerminalRegistry] End event arrived before running=true (race); delivering completion signal",
114+
{ terminalId: terminal.id, exitCode: e.exitCode },
115+
)
116+
terminal.shellExecutionComplete(exitDetails)
117+
} else {
118+
terminal.busy = false
119+
}
110120

111-
terminal.busy = false
112121
return
113122
}
114123

@@ -123,7 +132,6 @@ export class TerminalRegistry {
123132

124133
// Signal completion to any waiting processes.
125134
terminal.shellExecutionComplete(exitDetails)
126-
terminal.busy = false // Mark terminal as not busy when shell execution ends
127135
},
128136
)
129137

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,88 @@ describe("TerminalRegistry", () => {
205205
})
206206
})
207207

208+
describe("onDidEndTerminalShellExecution race condition (#489, #622)", () => {
209+
let endHandler: (e: any) => Promise<void>
210+
211+
beforeEach(() => {
212+
// Reset the initialized flag so we can call initialize() in this block.
213+
TerminalRegistry["isInitialized"] = false
214+
215+
// The global vscode mock doesn't define shell execution event
216+
// methods, so add them before spying.
217+
;(vscode.window as any).onDidStartTerminalShellExecution ??= () => ({ dispose: () => {} })
218+
;(vscode.window as any).onDidEndTerminalShellExecution ??= () => ({ dispose: () => {} })
219+
220+
vi.spyOn(vscode.window, "onDidStartTerminalShellExecution" as any).mockImplementation((_handler: any) => ({
221+
dispose: vi.fn(),
222+
}))
223+
224+
vi.spyOn(vscode.window, "onDidEndTerminalShellExecution" as any).mockImplementation((handler: any) => {
225+
endHandler = handler
226+
return { dispose: vi.fn() }
227+
})
228+
229+
TerminalRegistry.initialize()
230+
})
231+
232+
afterEach(() => {
233+
// Reset so other test blocks aren't affected.
234+
TerminalRegistry["isInitialized"] = false
235+
})
236+
237+
it("calls shellExecutionComplete when end event fires before running is set (race)", async () => {
238+
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
239+
const mockProcess = {
240+
command: "echo hello",
241+
emit: vi.fn(),
242+
hasUnretrievedOutput: vi.fn().mockReturnValue(false),
243+
} as any
244+
terminal.process = mockProcess
245+
246+
// Simulate the race: running is still false (setActiveStream hasn't
247+
// been called yet), but the end event fires.
248+
expect(terminal.running).toBe(false)
249+
250+
const mockExecution = { commandLine: { value: "echo hello" } }
251+
await endHandler({
252+
terminal: terminal.terminal,
253+
execution: mockExecution,
254+
exitCode: 0,
255+
})
256+
257+
// shellExecutionComplete should have been called exactly once, emitting
258+
// shell_execution_complete so TerminalProcess.run() unblocks.
259+
expect(mockProcess.emit).toHaveBeenCalledWith(
260+
"shell_execution_complete",
261+
expect.objectContaining({ exitCode: 0 }),
262+
)
263+
expect(mockProcess.emit).toHaveBeenCalledTimes(1)
264+
265+
// Terminal should be back to idle state.
266+
expect(terminal.busy).toBe(false)
267+
expect(terminal.running).toBe(false)
268+
})
269+
270+
it("sets busy=false without calling shellExecutionComplete when no process exists", async () => {
271+
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode") as Terminal
272+
terminal.busy = true
273+
terminal.process = undefined
274+
const completeSpy = vi.spyOn(terminal, "shellExecutionComplete")
275+
276+
expect(terminal.running).toBe(false)
277+
278+
const mockExecution = { commandLine: { value: "echo hello" } }
279+
await endHandler({
280+
terminal: terminal.terminal,
281+
execution: mockExecution,
282+
exitCode: 0,
283+
})
284+
285+
expect(terminal.busy).toBe(false)
286+
expect(completeSpy).not.toHaveBeenCalled()
287+
})
288+
})
289+
208290
describe("releaseTerminalsForTask", () => {
209291
it("aborts a busy terminal's running process and disassociates it from the task (#245)", () => {
210292
const terminal = TerminalRegistry.createTerminal("/test/path", "vscode")

0 commit comments

Comments
 (0)