Skip to content

Commit 90f6bb9

Browse files
committed
fix: handle race condition in terminal compound commands
- Fix race condition where shell execution end event arrives before start event - This commonly occurs with compound commands (e.g., 'cd dir && npm test') - Instead of returning early when terminal.running is false, check if a process exists - If process exists, complete it anyway to ensure LLM receives output - Add comprehensive tests for race condition handling Fixes #7430
1 parent c479678 commit 90f6bb9

File tree

2 files changed

+221
-2
lines changed

2 files changed

+221
-2
lines changed

src/integrations/terminal/TerminalRegistry.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,23 @@ export class TerminalRegistry {
9595
}
9696

9797
if (!terminal.running) {
98+
// Handle race condition: end event arrived before start event could mark terminal as running
99+
if (process) {
100+
console.warn(
101+
"[TerminalRegistry] Shell execution end event received before terminal marked as running (race condition). Completing process anyway:",
102+
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
103+
)
104+
105+
// Complete the process even though running flag wasn't set
106+
terminal.shellExecutionComplete(exitDetails)
107+
terminal.busy = false
108+
return
109+
}
110+
111+
// No process exists - this is an actual error
98112
console.error(
99-
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
100-
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
113+
"[TerminalRegistry] Shell execution end event received, but process is not running and no process exists:",
114+
{ terminalId: terminal?.id, exitCode: e.exitCode },
101115
)
102116

103117
terminal.busy = false
Lines changed: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
1+
import * as vscode from "vscode"
2+
import { TerminalRegistry } from "../TerminalRegistry"
3+
import { Terminal } from "../Terminal"
4+
import { TerminalProcess } from "../TerminalProcess"
5+
6+
// Mock vscode module
7+
vi.mock("vscode", () => {
8+
const eventHandlers: any = {
9+
startTerminalShellExecution: null,
10+
endTerminalShellExecution: null,
11+
closeTerminal: null,
12+
}
13+
14+
return {
15+
workspace: {
16+
getConfiguration: vi.fn().mockReturnValue({
17+
get: vi.fn().mockReturnValue(null),
18+
}),
19+
},
20+
window: {
21+
createTerminal: vi.fn().mockImplementation(() => ({
22+
shellIntegration: undefined,
23+
exitStatus: undefined,
24+
show: vi.fn(),
25+
sendText: vi.fn(),
26+
dispose: vi.fn(),
27+
})),
28+
onDidStartTerminalShellExecution: vi.fn().mockImplementation((handler) => {
29+
eventHandlers.startTerminalShellExecution = handler
30+
return { dispose: vi.fn() }
31+
}),
32+
onDidEndTerminalShellExecution: vi.fn().mockImplementation((handler) => {
33+
eventHandlers.endTerminalShellExecution = handler
34+
return { dispose: vi.fn() }
35+
}),
36+
onDidCloseTerminal: vi.fn().mockImplementation((handler) => {
37+
eventHandlers.closeTerminal = handler
38+
return { dispose: vi.fn() }
39+
}),
40+
},
41+
ThemeIcon: class ThemeIcon {
42+
constructor(public id: string) {}
43+
},
44+
Uri: {
45+
file: (path: string) => ({ fsPath: path }),
46+
},
47+
__eventHandlers: eventHandlers,
48+
}
49+
})
50+
51+
describe("TerminalRegistry race condition handling", () => {
52+
let mockTerminal: any
53+
let mockTerminalInfo: Terminal
54+
let terminalProcess: TerminalProcess
55+
56+
beforeAll(() => {
57+
TerminalRegistry.initialize()
58+
})
59+
60+
beforeEach(() => {
61+
// Clear terminals
62+
TerminalRegistry["terminals"] = []
63+
vi.clearAllMocks()
64+
65+
// Create mock VSCode terminal
66+
mockTerminal = {
67+
shellIntegration: {
68+
executeCommand: vi.fn(),
69+
cwd: vscode.Uri.file("/test/path"),
70+
},
71+
name: "Roo Code",
72+
processId: Promise.resolve(123),
73+
creationOptions: {},
74+
exitStatus: undefined,
75+
state: { isInteractedWith: true },
76+
dispose: vi.fn(),
77+
hide: vi.fn(),
78+
show: vi.fn(),
79+
sendText: vi.fn(),
80+
}
81+
82+
// Create Terminal instance
83+
mockTerminalInfo = new Terminal(1, mockTerminal, "/test/path")
84+
TerminalRegistry["terminals"] = [mockTerminalInfo]
85+
86+
// Create TerminalProcess
87+
terminalProcess = new TerminalProcess(mockTerminalInfo)
88+
terminalProcess.command = "cd test && echo hello"
89+
mockTerminalInfo.process = terminalProcess
90+
})
91+
92+
it("should handle end event arriving before start event (race condition)", async () => {
93+
const eventHandlers = (vscode as any).__eventHandlers
94+
const completedSpy = vi.fn()
95+
const shellExecutionCompleteSpy = vi.fn()
96+
97+
// Set up listeners
98+
terminalProcess.once("completed", completedSpy)
99+
terminalProcess.once("shell_execution_complete", shellExecutionCompleteSpy)
100+
101+
// Simulate the race condition: end event fires BEFORE start event
102+
// This simulates a fast-completing first command in a compound command
103+
104+
// 1. First, fire the END event (before terminal is marked as running)
105+
expect(mockTerminalInfo.running).toBe(false)
106+
107+
if (eventHandlers.endTerminalShellExecution) {
108+
eventHandlers.endTerminalShellExecution({
109+
terminal: mockTerminal,
110+
exitCode: 0,
111+
execution: {
112+
commandLine: { value: "cd test && echo hello" },
113+
},
114+
})
115+
}
116+
117+
// Verify that the process still received the completion signal despite the race
118+
await new Promise((resolve) => setTimeout(resolve, 10))
119+
120+
// The fix should have called shellExecutionComplete even though running was false
121+
expect(shellExecutionCompleteSpy).toHaveBeenCalledWith({ exitCode: 0 })
122+
expect(mockTerminalInfo.busy).toBe(false)
123+
124+
// 2. Now fire the START event (after end already happened)
125+
const mockStream = (async function* () {
126+
yield "\x1b]633;C\x07"
127+
yield "hello\n"
128+
yield "\x1b]633;D\x07"
129+
})()
130+
131+
if (eventHandlers.startTerminalShellExecution) {
132+
eventHandlers.startTerminalShellExecution({
133+
terminal: mockTerminal,
134+
execution: {
135+
commandLine: { value: "cd test && echo hello" },
136+
read: () => mockStream,
137+
},
138+
})
139+
}
140+
141+
// Terminal should handle this gracefully without errors
142+
expect(mockTerminalInfo.busy).toBe(true)
143+
})
144+
145+
it("should properly log warning for race condition instead of error", async () => {
146+
const eventHandlers = (vscode as any).__eventHandlers
147+
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
148+
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
149+
150+
// Fire end event before start (race condition)
151+
if (eventHandlers.endTerminalShellExecution) {
152+
eventHandlers.endTerminalShellExecution({
153+
terminal: mockTerminal,
154+
exitCode: 0,
155+
execution: {
156+
commandLine: { value: "cd test && echo hello" },
157+
},
158+
})
159+
}
160+
161+
// Should log warning, not error, for the race condition
162+
expect(warnSpy).toHaveBeenCalledWith(
163+
expect.stringContaining(
164+
"[TerminalRegistry] Shell execution end event received before terminal marked as running (race condition)",
165+
),
166+
expect.any(Object),
167+
)
168+
expect(errorSpy).not.toHaveBeenCalledWith(
169+
expect.stringContaining("Shell execution end event received, but process is not running for terminal"),
170+
expect.any(Object),
171+
)
172+
173+
warnSpy.mockRestore()
174+
errorSpy.mockRestore()
175+
})
176+
177+
it("should still error when no process exists", async () => {
178+
const eventHandlers = (vscode as any).__eventHandlers
179+
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
180+
181+
// Remove the process
182+
mockTerminalInfo.process = undefined
183+
184+
// Fire end event without a process
185+
if (eventHandlers.endTerminalShellExecution) {
186+
eventHandlers.endTerminalShellExecution({
187+
terminal: mockTerminal,
188+
exitCode: 0,
189+
execution: {
190+
commandLine: { value: "some command" },
191+
},
192+
})
193+
}
194+
195+
// Should still error when there's truly no process
196+
expect(errorSpy).toHaveBeenCalledWith(
197+
expect.stringContaining(
198+
"[TerminalRegistry] Shell execution end event received, but process is not running and no process exists",
199+
),
200+
expect.any(Object),
201+
)
202+
203+
errorSpy.mockRestore()
204+
})
205+
})

0 commit comments

Comments
 (0)