-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle race condition for compound commands in new terminals #7435
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
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 |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ export class Terminal extends BaseTerminal { | |
| public terminal: vscode.Terminal | ||
|
|
||
| public cmdCounter: number = 0 | ||
| private shellIntegrationReady: boolean = false | ||
|
|
||
| constructor(id: number, terminal: vscode.Terminal | undefined, cwd: string) { | ||
| super("vscode", id, cwd) | ||
|
|
@@ -67,14 +68,30 @@ export class Terminal extends BaseTerminal { | |
| reject(error) | ||
| }) | ||
|
|
||
| // For compound commands or when shell integration is not ready, | ||
| // we need to ensure shell integration is fully attached | ||
| const isCompoundCommand = Terminal.isCompoundCommand(command) | ||
| const needsIntegrationWait = isCompoundCommand && !this.shellIntegrationReady | ||
|
|
||
| // Wait for shell integration before executing the command | ||
| pWaitFor(() => this.terminal.shellIntegration !== undefined, { | ||
| timeout: Terminal.getShellIntegrationTimeout(), | ||
| }) | ||
| .then(() => { | ||
| .then(async () => { | ||
| // Clean up temporary directory if shell integration is available, zsh did its job: | ||
| ShellIntegrationManager.zshCleanupTmpDir(this.id) | ||
|
|
||
| // For compound commands on newly spawned terminals, add a small delay | ||
| // to ensure shell integration is fully attached after the terminal is created | ||
| if (needsIntegrationWait) { | ||
| console.info( | ||
| `[Terminal ${this.id}] Compound command detected on new terminal, ensuring shell integration is ready`, | ||
| ) | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
| } | ||
|
|
||
| this.shellIntegrationReady = true | ||
|
|
||
| // Run the command in the terminal | ||
| process.run(command) | ||
| }) | ||
|
|
@@ -190,4 +207,44 @@ export class Terminal extends BaseTerminal { | |
|
|
||
| return env | ||
| } | ||
|
|
||
| /** | ||
|
Contributor
Author
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. This JSDoc is helpful, but could we expand it to explain why each operator causes race conditions? For example, mentioning that |
||
| * Detects if a command is a compound command (contains operators like &&, ||, ;, |, &) | ||
| * @param command The command to check | ||
| * @returns True if the command contains compound operators | ||
| */ | ||
| public static isCompoundCommand(command: string): boolean { | ||
|
Contributor
Author
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. I notice we have duplicate |
||
| // Check for common shell operators that create compound commands | ||
| // These operators can cause multiple processes to be spawned | ||
| const compoundOperators = [ | ||
| "&&", // AND operator | ||
| "||", // OR operator | ||
| ";", // Sequential operator | ||
| "|", // Pipe operator | ||
| "&", // Background operator (at end of command) | ||
| ] | ||
|
|
||
| // Check if command contains any compound operators | ||
| // Be careful with pipe operator to avoid false positives in strings | ||
| for (const operator of compoundOperators) { | ||
| if (operator === "&") { | ||
| // Check for background operator at the end (not &&) | ||
| if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) { | ||
| return true | ||
| } | ||
| } else if (operator === "|") { | ||
| // Check for pipe operator (not ||) | ||
| const pipeRegex = /(?<![|])\|(?![|])/ | ||
| if (pipeRegex.test(command)) { | ||
| return true | ||
| } | ||
| } else { | ||
| if (command.includes(operator)) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,10 +95,26 @@ export class TerminalRegistry { | |
| } | ||
|
|
||
| if (!terminal.running) { | ||
| console.error( | ||
| "[TerminalRegistry] Shell execution end event received, but process is not running for terminal:", | ||
| { terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode }, | ||
| ) | ||
| // This can happen with compound commands where shell integration | ||
| // attaches after the first command completes | ||
| const isCompoundCmd = process?.command && this.isCompoundCommand(process.command) | ||
|
|
||
| if (isCompoundCmd) { | ||
| console.warn( | ||
| "[TerminalRegistry] Shell execution end event received for compound command before terminal marked as running (race condition):", | ||
| { terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode }, | ||
| ) | ||
|
|
||
| // If we have a process, complete it to prevent hanging | ||
| if (process) { | ||
| terminal.shellExecutionComplete(exitDetails) | ||
| } | ||
| } else { | ||
| console.error( | ||
| "[TerminalRegistry] Shell execution end event received, but process is not running for terminal:", | ||
| { terminalId: terminal?.id, command: process?.command, exitCode: e.exitCode }, | ||
| ) | ||
| } | ||
|
|
||
| terminal.busy = false | ||
| return | ||
|
|
@@ -325,4 +341,28 @@ export class TerminalRegistry { | |
| ShellIntegrationManager.zshCleanupTmpDir(id) | ||
| this.terminals = this.terminals.filter((t) => t.id !== id) | ||
| } | ||
|
|
||
| /** | ||
| * Detects if a command is a compound command (contains operators like &&, ||, ;, |, &) | ||
| * @param command The command to check | ||
| * @returns True if the command contains compound operators | ||
| */ | ||
| private static isCompoundCommand(command: string): boolean { | ||
|
Contributor
Author
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. This method is |
||
| // Check for common shell operators that create compound commands | ||
| const compoundOperators = ["&&", "||", ";", "|"] | ||
|
|
||
| // Also check for background operator at the end | ||
| if (command.trimEnd().endsWith("&") && !command.trimEnd().endsWith("&&")) { | ||
| return true | ||
| } | ||
|
|
||
| return compoundOperators.some((op) => { | ||
| if (op === "|") { | ||
| // Check for pipe operator (not ||) | ||
| const pipeRegex = /(?<![|])\|(?![|])/ | ||
| return pipeRegex.test(command) | ||
| } | ||
| return command.includes(op) | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest" | ||
| import * as vscode from "vscode" | ||
|
|
||
| import { Terminal } from "../Terminal" | ||
| import { TerminalRegistry } from "../TerminalRegistry" | ||
|
|
||
| describe("Terminal - Compound Command Handling", () => { | ||
| beforeEach(() => { | ||
| // Initialize the registry for tests | ||
| vi.spyOn(TerminalRegistry, "initialize").mockImplementation(() => {}) | ||
| TerminalRegistry.initialize() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| describe("isCompoundCommand", () => { | ||
| it("should detect && operator", () => { | ||
| expect(Terminal.isCompoundCommand("cd /tmp && ls")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("echo hello && echo world")).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect || operator", () => { | ||
| expect(Terminal.isCompoundCommand("cd /nonexistent || echo 'failed'")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("test -f file.txt || touch file.txt")).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect ; operator", () => { | ||
| expect(Terminal.isCompoundCommand("cd /tmp; ls")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("echo first; echo second; echo third")).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect | pipe operator", () => { | ||
| expect(Terminal.isCompoundCommand("ls | grep test")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("cat file.txt | head -10")).toBe(true) | ||
| }) | ||
|
|
||
| it("should detect & background operator", () => { | ||
| expect(Terminal.isCompoundCommand("npm start &")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("python server.py &")).toBe(true) | ||
| }) | ||
|
|
||
| it("should not detect && in strings or comments", () => { | ||
| // These are still detected as compound commands because we check for the operator presence | ||
| // This is intentional to err on the side of caution | ||
| expect(Terminal.isCompoundCommand('echo "&&"')).toBe(true) | ||
|
Contributor
Author
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. The comment mentions this is intentional to "err on the side of caution", but detecting operators inside quoted strings could lead to false positives. Have we considered more sophisticated parsing that respects shell quoting rules? For example, |
||
| }) | ||
|
|
||
| it("should not detect single & in the middle of command", () => { | ||
| expect(Terminal.isCompoundCommand("echo 'this & that'")).toBe(false) | ||
| expect(Terminal.isCompoundCommand("url?param1=a¶m2=b")).toBe(false) | ||
| }) | ||
|
|
||
| it("should handle complex compound commands", () => { | ||
| expect(Terminal.isCompoundCommand("cd /tmp && npm install || echo 'failed'")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("test -d dir && (cd dir; make) || mkdir dir")).toBe(true) | ||
| }) | ||
|
|
||
| it("should return false for simple commands", () => { | ||
| expect(Terminal.isCompoundCommand("ls")).toBe(false) | ||
| expect(Terminal.isCompoundCommand("cd /tmp")).toBe(false) | ||
| expect(Terminal.isCompoundCommand("echo hello")).toBe(false) | ||
| expect(Terminal.isCompoundCommand("npm install")).toBe(false) | ||
| }) | ||
|
|
||
| it("should handle edge cases", () => { | ||
| expect(Terminal.isCompoundCommand("")).toBe(false) | ||
| expect(Terminal.isCompoundCommand(" ")).toBe(false) | ||
| expect(Terminal.isCompoundCommand("&")).toBe(true) // Background operator | ||
| expect(Terminal.isCompoundCommand("&&")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("||")).toBe(true) | ||
| expect(Terminal.isCompoundCommand("|")).toBe(true) | ||
| }) | ||
| }) | ||
|
|
||
| describe("Compound command execution with shell integration", () => { | ||
| let mockTerminal: any | ||
| let terminal: Terminal | ||
|
|
||
| beforeEach(() => { | ||
| // Create a mock VSCode terminal | ||
| mockTerminal = { | ||
| shellIntegration: undefined, | ||
| sendText: vi.fn(), | ||
| show: vi.fn(), | ||
| hide: vi.fn(), | ||
| dispose: vi.fn(), | ||
| exitStatus: undefined, | ||
| state: { isInteractedWith: false }, | ||
| creationOptions: {}, | ||
| name: "Test Terminal", | ||
| processId: Promise.resolve(1234), | ||
| } | ||
|
|
||
| // Mock vscode.window.createTerminal | ||
| vi.spyOn(vscode.window, "createTerminal").mockReturnValue(mockTerminal as any) | ||
|
|
||
| // Create a Terminal instance | ||
| terminal = new Terminal(1, undefined, "/tmp") | ||
| }) | ||
|
|
||
| it("should add delay for compound commands on new terminals", async () => { | ||
| const command = "cd /tmp && npm test" | ||
| const callbacks = { | ||
| onLine: vi.fn(), | ||
| onCompleted: vi.fn(), | ||
| onShellExecutionStarted: vi.fn(), | ||
| onShellExecutionComplete: vi.fn(), | ||
| onNoShellIntegration: vi.fn(), | ||
| } | ||
|
|
||
| // Mock shell integration becoming available | ||
| setTimeout(() => { | ||
| mockTerminal.shellIntegration = { | ||
| executeCommand: vi.fn(), | ||
| cwd: { fsPath: "/tmp" }, | ||
| } | ||
| }, 50) | ||
|
|
||
| const processPromise = terminal.runCommand(command, callbacks) | ||
|
|
||
| // Wait a bit for the command to be processed | ||
| await new Promise((resolve) => setTimeout(resolve, 200)) | ||
|
|
||
| // Verify that the terminal is marked as busy initially | ||
| expect(terminal.busy).toBe(true) | ||
|
|
||
| // The shellIntegrationReady flag should be set after the delay | ||
| expect((terminal as any).shellIntegrationReady).toBe(true) | ||
|
Contributor
Author
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. Using |
||
| }) | ||
|
|
||
| it("should not add delay for simple commands", async () => { | ||
| const command = "ls -la" | ||
| const callbacks = { | ||
| onLine: vi.fn(), | ||
| onCompleted: vi.fn(), | ||
| onShellExecutionStarted: vi.fn(), | ||
| onShellExecutionComplete: vi.fn(), | ||
| onNoShellIntegration: vi.fn(), | ||
| } | ||
|
|
||
| // Mock shell integration being immediately available | ||
| mockTerminal.shellIntegration = { | ||
| executeCommand: vi.fn(), | ||
| cwd: { fsPath: "/tmp" }, | ||
| } | ||
|
|
||
| const processPromise = terminal.runCommand(command, callbacks) | ||
|
|
||
| // Wait a bit for the command to be processed | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
|
|
||
| // Should execute without additional delay | ||
| expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command) | ||
| }) | ||
|
|
||
| it("should not add delay for compound commands on terminals with ready shell integration", async () => { | ||
| const command = "cd /tmp && npm test" | ||
| const callbacks = { | ||
| onLine: vi.fn(), | ||
| onCompleted: vi.fn(), | ||
| onShellExecutionStarted: vi.fn(), | ||
| onShellExecutionComplete: vi.fn(), | ||
| onNoShellIntegration: vi.fn(), | ||
| } | ||
|
|
||
| // Mock shell integration being immediately available | ||
| mockTerminal.shellIntegration = { | ||
| executeCommand: vi.fn(), | ||
| cwd: { fsPath: "/tmp" }, | ||
| } | ||
|
|
||
| // Mark shell integration as ready (simulating a reused terminal) | ||
| ;(terminal as any).shellIntegrationReady = true | ||
|
|
||
| const processPromise = terminal.runCommand(command, callbacks) | ||
|
|
||
| // Wait a bit for the command to be processed | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
|
|
||
| // Should execute without additional delay since shellIntegrationReady is true | ||
| expect(mockTerminal.shellIntegration.executeCommand).toHaveBeenCalledWith(command) | ||
| }) | ||
| }) | ||
| }) | ||
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.
The 100ms delay is a magic number. Should we extract this to a named constant like
SHELL_INTEGRATION_STABILIZATION_DELAYfor better maintainability? This would also make it easier to adjust if we find different timing requirements in the future.