Skip to content

Commit 182ecc0

Browse files
committed
fix: handle race condition for compound commands in new terminals
- Add compound command detection to identify commands with &&, ||, ;, |, & operators - Implement delay mechanism for compound commands on newly spawned terminals - Ensure shell integration is fully attached before executing compound commands - Improve error handling in TerminalRegistry for race conditions - Add comprehensive tests for compound command handling Fixes #7430
1 parent 11c454f commit 182ecc0

File tree

3 files changed

+288
-5
lines changed

3 files changed

+288
-5
lines changed

src/integrations/terminal/Terminal.ts

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export class Terminal extends BaseTerminal {
1111
public terminal: vscode.Terminal
1212

1313
public cmdCounter: number = 0
14+
private shellIntegrationReady: boolean = false
1415

1516
constructor(id: number, terminal: vscode.Terminal | undefined, cwd: string) {
1617
super("vscode", id, cwd)
@@ -67,14 +68,30 @@ export class Terminal extends BaseTerminal {
6768
reject(error)
6869
})
6970

71+
// For compound commands or when shell integration is not ready,
72+
// we need to ensure shell integration is fully attached
73+
const isCompoundCommand = Terminal.isCompoundCommand(command)
74+
const needsIntegrationWait = isCompoundCommand && !this.shellIntegrationReady
75+
7076
// Wait for shell integration before executing the command
7177
pWaitFor(() => this.terminal.shellIntegration !== undefined, {
7278
timeout: Terminal.getShellIntegrationTimeout(),
7379
})
74-
.then(() => {
80+
.then(async () => {
7581
// Clean up temporary directory if shell integration is available, zsh did its job:
7682
ShellIntegrationManager.zshCleanupTmpDir(this.id)
7783

84+
// For compound commands on newly spawned terminals, add a small delay
85+
// to ensure shell integration is fully attached after the terminal is created
86+
if (needsIntegrationWait) {
87+
console.info(
88+
`[Terminal ${this.id}] Compound command detected on new terminal, ensuring shell integration is ready`,
89+
)
90+
await new Promise((resolve) => setTimeout(resolve, 100))
91+
}
92+
93+
this.shellIntegrationReady = true
94+
7895
// Run the command in the terminal
7996
process.run(command)
8097
})
@@ -190,4 +207,44 @@ export class Terminal extends BaseTerminal {
190207

191208
return env
192209
}
210+
211+
/**
212+
* Detects if a command is a compound command (contains operators like &&, ||, ;, |, &)
213+
* @param command The command to check
214+
* @returns True if the command contains compound operators
215+
*/
216+
public static isCompoundCommand(command: string): boolean {
217+
// Check for common shell operators that create compound commands
218+
// These operators can cause multiple processes to be spawned
219+
const compoundOperators = [
220+
"&&", // AND operator
221+
"||", // OR operator
222+
";", // Sequential operator
223+
"|", // Pipe operator
224+
"&", // Background operator (at end of command)
225+
]
226+
227+
// Check if command contains any compound operators
228+
// Be careful with pipe operator to avoid false positives in strings
229+
for (const operator of compoundOperators) {
230+
if (operator === "&") {
231+
// Check for background operator at the end (not &&)
232+
if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) {
233+
return true
234+
}
235+
} else if (operator === "|") {
236+
// Check for pipe operator (not ||)
237+
const pipeRegex = /(?<![|])\|(?![|])/
238+
if (pipeRegex.test(command)) {
239+
return true
240+
}
241+
} else {
242+
if (command.includes(operator)) {
243+
return true
244+
}
245+
}
246+
}
247+
248+
return false
249+
}
193250
}

src/integrations/terminal/TerminalRegistry.ts

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,26 @@ export class TerminalRegistry {
9595
}
9696

9797
if (!terminal.running) {
98-
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 },
101-
)
98+
// This can happen with compound commands where shell integration
99+
// attaches after the first command completes
100+
const isCompoundCmd = process?.command && this.isCompoundCommand(process.command)
101+
102+
if (isCompoundCmd) {
103+
console.warn(
104+
"[TerminalRegistry] Shell execution end event received for compound command before terminal marked as running (race condition):",
105+
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
106+
)
107+
108+
// If we have a process, complete it to prevent hanging
109+
if (process) {
110+
terminal.shellExecutionComplete(exitDetails)
111+
}
112+
} else {
113+
console.error(
114+
"[TerminalRegistry] Shell execution end event received, but process is not running for terminal:",
115+
{ terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode },
116+
)
117+
}
102118

103119
terminal.busy = false
104120
return
@@ -325,4 +341,28 @@ export class TerminalRegistry {
325341
ShellIntegrationManager.zshCleanupTmpDir(id)
326342
this.terminals = this.terminals.filter((t) => t.id !== id)
327343
}
344+
345+
/**
346+
* Detects if a command is a compound command (contains operators like &&, ||, ;, |, &)
347+
* @param command The command to check
348+
* @returns True if the command contains compound operators
349+
*/
350+
private static isCompoundCommand(command: string): boolean {
351+
// Check for common shell operators that create compound commands
352+
const compoundOperators = ["&&", "||", ";", "|"]
353+
354+
// Also check for background operator at the end
355+
if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) {
356+
return true
357+
}
358+
359+
return compoundOperators.some((op) => {
360+
if (op === "|") {
361+
// Check for pipe operator (not ||)
362+
const pipeRegex = /(?<![|])\|(?![|])/
363+
return pipeRegex.test(command)
364+
}
365+
return command.includes(op)
366+
})
367+
}
328368
}
Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
2+
import * as vscode from "vscode"
3+
4+
import { Terminal } from "../Terminal"
5+
import { TerminalRegistry } from "../TerminalRegistry"
6+
7+
describe("Terminal - Compound Command Handling", () => {
8+
beforeEach(() => {
9+
// Initialize the registry for tests
10+
vi.spyOn(TerminalRegistry, "initialize").mockImplementation(() => {})
11+
TerminalRegistry.initialize()
12+
})
13+
14+
afterEach(() => {
15+
vi.restoreAllMocks()
16+
})
17+
18+
describe("isCompoundCommand", () => {
19+
it("should detect && operator", () => {
20+
expect(Terminal.isCompoundCommand("cd /tmp && ls")).toBe(true)
21+
expect(Terminal.isCompoundCommand("echo hello && echo world")).toBe(true)
22+
})
23+
24+
it("should detect || operator", () => {
25+
expect(Terminal.isCompoundCommand("cd /nonexistent || echo 'failed'")).toBe(true)
26+
expect(Terminal.isCompoundCommand("test -f file.txt || touch file.txt")).toBe(true)
27+
})
28+
29+
it("should detect ; operator", () => {
30+
expect(Terminal.isCompoundCommand("cd /tmp; ls")).toBe(true)
31+
expect(Terminal.isCompoundCommand("echo first; echo second; echo third")).toBe(true)
32+
})
33+
34+
it("should detect | pipe operator", () => {
35+
expect(Terminal.isCompoundCommand("ls | grep test")).toBe(true)
36+
expect(Terminal.isCompoundCommand("cat file.txt | head -10")).toBe(true)
37+
})
38+
39+
it("should detect & background operator", () => {
40+
expect(Terminal.isCompoundCommand("npm start &")).toBe(true)
41+
expect(Terminal.isCompoundCommand("python server.py &")).toBe(true)
42+
})
43+
44+
it("should not detect && in strings or comments", () => {
45+
// These are still detected as compound commands because we check for the operator presence
46+
// This is intentional to err on the side of caution
47+
expect(Terminal.isCompoundCommand('echo "&&"')).toBe(true)
48+
})
49+
50+
it("should not detect single & in the middle of command", () => {
51+
expect(Terminal.isCompoundCommand("echo 'this & that'")).toBe(false)
52+
expect(Terminal.isCompoundCommand("url?param1=a&param2=b")).toBe(false)
53+
})
54+
55+
it("should handle complex compound commands", () => {
56+
expect(Terminal.isCompoundCommand("cd /tmp && npm install || echo 'failed'")).toBe(true)
57+
expect(Terminal.isCompoundCommand("test -d dir && (cd dir; make) || mkdir dir")).toBe(true)
58+
})
59+
60+
it("should return false for simple commands", () => {
61+
expect(Terminal.isCompoundCommand("ls")).toBe(false)
62+
expect(Terminal.isCompoundCommand("cd /tmp")).toBe(false)
63+
expect(Terminal.isCompoundCommand("echo hello")).toBe(false)
64+
expect(Terminal.isCompoundCommand("npm install")).toBe(false)
65+
})
66+
67+
it("should handle edge cases", () => {
68+
expect(Terminal.isCompoundCommand("")).toBe(false)
69+
expect(Terminal.isCompoundCommand(" ")).toBe(false)
70+
expect(Terminal.isCompoundCommand("&")).toBe(true) // Background operator
71+
expect(Terminal.isCompoundCommand("&&")).toBe(true)
72+
expect(Terminal.isCompoundCommand("||")).toBe(true)
73+
expect(Terminal.isCompoundCommand("|")).toBe(true)
74+
})
75+
})
76+
77+
describe("Compound command execution with shell integration", () => {
78+
let mockTerminal: any
79+
let terminal: Terminal
80+
81+
beforeEach(() => {
82+
// Create a mock VSCode terminal
83+
mockTerminal = {
84+
shellIntegration: undefined,
85+
sendText: vi.fn(),
86+
show: vi.fn(),
87+
hide: vi.fn(),
88+
dispose: vi.fn(),
89+
exitStatus: undefined,
90+
state: { isInteractedWith: false },
91+
creationOptions: {},
92+
name: "Test Terminal",
93+
processId: Promise.resolve(1234),
94+
}
95+
96+
// Mock vscode.window.createTerminal
97+
vi.spyOn(vscode.window, "createTerminal").mockReturnValue(mockTerminal as any)
98+
99+
// Create a Terminal instance
100+
terminal = new Terminal(1, undefined, "/tmp")
101+
})
102+
103+
it("should add delay for compound commands on new terminals", async () => {
104+
const command = "cd /tmp && npm test"
105+
const callbacks = {
106+
onLine: vi.fn(),
107+
onCompleted: vi.fn(),
108+
onShellExecutionStarted: vi.fn(),
109+
onShellExecutionComplete: vi.fn(),
110+
onNoShellIntegration: vi.fn(),
111+
}
112+
113+
// Mock shell integration becoming available
114+
setTimeout(() => {
115+
mockTerminal.shellIntegration = {
116+
executeCommand: vi.fn(),
117+
cwd: { fsPath: "/tmp" },
118+
}
119+
}, 50)
120+
121+
const processPromise = terminal.runCommand(command, callbacks)
122+
123+
// Wait a bit for the command to be processed
124+
await new Promise((resolve) => setTimeout(resolve, 200))
125+
126+
// Verify that the terminal is marked as busy initially
127+
expect(terminal.busy).toBe(true)
128+
129+
// The shellIntegrationReady flag should be set after the delay
130+
expect((terminal as any).shellIntegrationReady).toBe(true)
131+
})
132+
133+
it("should not add delay for simple commands", async () => {
134+
const command = "ls -la"
135+
const callbacks = {
136+
onLine: vi.fn(),
137+
onCompleted: vi.fn(),
138+
onShellExecutionStarted: vi.fn(),
139+
onShellExecutionComplete: vi.fn(),
140+
onNoShellIntegration: vi.fn(),
141+
}
142+
143+
// Mock shell integration being immediately available
144+
mockTerminal.shellIntegration = {
145+
executeCommand: vi.fn(),
146+
cwd: { fsPath: "/tmp" },
147+
}
148+
149+
const processPromise = terminal.runCommand(command, callbacks)
150+
151+
// Wait a bit for the command to be processed
152+
await new Promise((resolve) => setTimeout(resolve, 100))
153+
154+
// Should execute without additional delay
155+
expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command)
156+
})
157+
158+
it("should not add delay for compound commands on terminals with ready shell integration", async () => {
159+
const command = "cd /tmp && npm test"
160+
const callbacks = {
161+
onLine: vi.fn(),
162+
onCompleted: vi.fn(),
163+
onShellExecutionStarted: vi.fn(),
164+
onShellExecutionComplete: vi.fn(),
165+
onNoShellIntegration: vi.fn(),
166+
}
167+
168+
// Mock shell integration being immediately available
169+
mockTerminal.shellIntegration = {
170+
executeCommand: vi.fn(),
171+
cwd: { fsPath: "/tmp" },
172+
}
173+
174+
// Mark shell integration as ready (simulating a reused terminal)
175+
;(terminal as any).shellIntegrationReady = true
176+
177+
const processPromise = terminal.runCommand(command, callbacks)
178+
179+
// Wait a bit for the command to be processed
180+
await new Promise((resolve) => setTimeout(resolve, 100))
181+
182+
// Should execute without additional delay since shellIntegrationReady is true
183+
expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command)
184+
})
185+
})
186+
})

0 commit comments

Comments
 (0)