mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: resolve GSOD issues with multiple VSCode windows
- Made terminal shell execution handlers non-blocking using setImmediate - Added proper error handling and recovery for terminal operations - Increased diff editor timeout from 10s to 15s with retry mechanism - Enhanced terminal isolation to prevent cross-task interference - Added comprehensive tests for concurrent terminal operations Fixes #8949
This commit is contained in:
parent
4a096e1cb9
commit
f61d244769
3 changed files with 635 additions and 126 deletions
|
|
@ -485,10 +485,13 @@ export class DiffViewProvider {
|
|||
return new Promise<vscode.TextEditor>((resolve, reject) => {
|
||||
const fileName = path.basename(uri.fsPath)
|
||||
const fileExists = this.editType === "modify"
|
||||
const DIFF_EDITOR_TIMEOUT = 10_000 // ms
|
||||
const DIFF_EDITOR_TIMEOUT = 15_000 // Increased from 10s to 15s to accommodate slower systems
|
||||
const MAX_RETRIES = 2 // Allow up to 2 retries on timeout
|
||||
|
||||
let currentRetry = 0
|
||||
let timeoutId: NodeJS.Timeout | undefined
|
||||
const disposables: vscode.Disposable[] = []
|
||||
let disposables: vscode.Disposable[] = []
|
||||
let isResolved = false
|
||||
|
||||
const cleanup = () => {
|
||||
if (timeoutId) {
|
||||
|
|
@ -499,77 +502,122 @@ export class DiffViewProvider {
|
|||
disposables.length = 0
|
||||
}
|
||||
|
||||
// Set timeout for the entire operation
|
||||
timeoutId = setTimeout(() => {
|
||||
cleanup()
|
||||
reject(
|
||||
new Error(
|
||||
`Failed to open diff editor for ${uri.fsPath} within ${DIFF_EDITOR_TIMEOUT / 1000} seconds. The editor may be blocked or VS Code may be unresponsive.`,
|
||||
),
|
||||
const attemptOpenDiffEditor = () => {
|
||||
// Set timeout for the current attempt
|
||||
timeoutId = setTimeout(() => {
|
||||
if (!isResolved) {
|
||||
cleanup()
|
||||
|
||||
// Try retrying if we haven't exceeded MAX_RETRIES
|
||||
if (currentRetry < MAX_RETRIES) {
|
||||
currentRetry++
|
||||
console.warn(
|
||||
`[DiffViewProvider] Diff editor timeout for ${uri.fsPath}, retrying... (attempt ${currentRetry + 1}/${MAX_RETRIES + 1})`,
|
||||
)
|
||||
// Reset and retry
|
||||
disposables = []
|
||||
attemptOpenDiffEditor()
|
||||
} else {
|
||||
// All retries exhausted
|
||||
isResolved = true
|
||||
reject(
|
||||
new Error(
|
||||
`Failed to open diff editor for ${uri.fsPath} after ${MAX_RETRIES + 1} attempts (${DIFF_EDITOR_TIMEOUT / 1000}s timeout each). The editor may be blocked or VS Code may be unresponsive.`,
|
||||
),
|
||||
)
|
||||
}
|
||||
}
|
||||
}, DIFF_EDITOR_TIMEOUT)
|
||||
|
||||
// Listen for document open events - more efficient than scanning all tabs
|
||||
disposables.push(
|
||||
vscode.workspace.onDidOpenTextDocument(async (document) => {
|
||||
if (isResolved) return
|
||||
|
||||
// Only match file:// scheme documents to avoid git diffs
|
||||
if (document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, uri.fsPath)) {
|
||||
// Wait a tick for the editor to be available
|
||||
await new Promise((r) => setTimeout(r, 50)) // Slightly longer wait
|
||||
|
||||
// Find the editor for this document
|
||||
const editor = vscode.window.visibleTextEditors.find(
|
||||
(e) =>
|
||||
e.document.uri.scheme === "file" &&
|
||||
arePathsEqual(e.document.uri.fsPath, uri.fsPath),
|
||||
)
|
||||
|
||||
if (editor && !isResolved) {
|
||||
isResolved = true
|
||||
cleanup()
|
||||
resolve(editor)
|
||||
}
|
||||
}
|
||||
}),
|
||||
)
|
||||
}, DIFF_EDITOR_TIMEOUT)
|
||||
|
||||
// Listen for document open events - more efficient than scanning all tabs
|
||||
disposables.push(
|
||||
vscode.workspace.onDidOpenTextDocument(async (document) => {
|
||||
// Only match file:// scheme documents to avoid git diffs
|
||||
if (document.uri.scheme === "file" && arePathsEqual(document.uri.fsPath, uri.fsPath)) {
|
||||
// Wait a tick for the editor to be available
|
||||
await new Promise((r) => setTimeout(r, 0))
|
||||
// Also listen for visible editor changes as a fallback
|
||||
disposables.push(
|
||||
vscode.window.onDidChangeVisibleTextEditors((editors) => {
|
||||
if (isResolved) return
|
||||
|
||||
// Find the editor for this document
|
||||
const editor = vscode.window.visibleTextEditors.find(
|
||||
(e) => e.document.uri.scheme === "file" && arePathsEqual(e.document.uri.fsPath, uri.fsPath),
|
||||
)
|
||||
|
||||
if (editor) {
|
||||
const editor = editors.find((e) => {
|
||||
const isFileScheme = e.document.uri.scheme === "file"
|
||||
const pathMatches = arePathsEqual(e.document.uri.fsPath, uri.fsPath)
|
||||
return isFileScheme && pathMatches
|
||||
})
|
||||
if (editor && !isResolved) {
|
||||
isResolved = true
|
||||
cleanup()
|
||||
resolve(editor)
|
||||
}
|
||||
}
|
||||
}),
|
||||
)
|
||||
|
||||
// Also listen for visible editor changes as a fallback
|
||||
disposables.push(
|
||||
vscode.window.onDidChangeVisibleTextEditors((editors) => {
|
||||
const editor = editors.find((e) => {
|
||||
const isFileScheme = e.document.uri.scheme === "file"
|
||||
const pathMatches = arePathsEqual(e.document.uri.fsPath, uri.fsPath)
|
||||
return isFileScheme && pathMatches
|
||||
})
|
||||
if (editor) {
|
||||
cleanup()
|
||||
resolve(editor)
|
||||
}
|
||||
}),
|
||||
)
|
||||
|
||||
// Pre-open the file as a text document to ensure it doesn't open in preview mode
|
||||
// This fixes issues with files that have custom editor associations (like markdown preview)
|
||||
vscode.window
|
||||
.showTextDocument(uri, { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true })
|
||||
.then(() => {
|
||||
// Execute the diff command after ensuring the file is open as text
|
||||
return vscode.commands.executeCommand(
|
||||
"vscode.diff",
|
||||
vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({
|
||||
query: Buffer.from(this.originalContent ?? "").toString("base64"),
|
||||
}),
|
||||
uri,
|
||||
`${fileName}: ${fileExists ? `${DIFF_VIEW_LABEL_CHANGES}` : "New File"} (Editable)`,
|
||||
{ preserveFocus: true },
|
||||
)
|
||||
})
|
||||
.then(
|
||||
() => {
|
||||
// Command executed successfully, now wait for the editor to appear
|
||||
},
|
||||
(err: any) => {
|
||||
cleanup()
|
||||
reject(new Error(`Failed to execute diff command for ${uri.fsPath}: ${err.message}`))
|
||||
},
|
||||
}),
|
||||
)
|
||||
|
||||
// Pre-open the file as a text document to ensure it doesn't open in preview mode
|
||||
// This fixes issues with files that have custom editor associations (like markdown preview)
|
||||
// Use setImmediate to avoid blocking other operations
|
||||
setImmediate(() => {
|
||||
if (isResolved) return
|
||||
|
||||
vscode.window
|
||||
.showTextDocument(uri, {
|
||||
preview: false,
|
||||
viewColumn: vscode.ViewColumn.Active,
|
||||
preserveFocus: true,
|
||||
})
|
||||
.then(() => {
|
||||
if (isResolved) return
|
||||
|
||||
// Execute the diff command after ensuring the file is open as text
|
||||
return vscode.commands.executeCommand(
|
||||
"vscode.diff",
|
||||
vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({
|
||||
query: Buffer.from(this.originalContent ?? "").toString("base64"),
|
||||
}),
|
||||
uri,
|
||||
`${fileName}: ${fileExists ? `${DIFF_VIEW_LABEL_CHANGES}` : "New File"} (Editable)`,
|
||||
{ preserveFocus: true },
|
||||
)
|
||||
})
|
||||
.then(
|
||||
() => {
|
||||
// Command executed successfully, now wait for the editor to appear
|
||||
},
|
||||
(err: any) => {
|
||||
if (!isResolved) {
|
||||
isResolved = true
|
||||
cleanup()
|
||||
reject(
|
||||
new Error(`Failed to execute diff command for ${uri.fsPath}: ${err.message}`),
|
||||
)
|
||||
}
|
||||
},
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
// Start the first attempt
|
||||
attemptOpenDiffEditor()
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -47,25 +47,35 @@ export class TerminalRegistry {
|
|||
|
||||
try {
|
||||
const startDisposable = vscode.window.onDidStartTerminalShellExecution?.(
|
||||
async (e: vscode.TerminalShellExecutionStartEvent) => {
|
||||
// Get a handle to the stream as early as possible:
|
||||
const stream = e.execution.read()
|
||||
const terminal = this.getTerminalByVSCETerminal(e.terminal)
|
||||
(e: vscode.TerminalShellExecutionStartEvent) => {
|
||||
// Process the event asynchronously to avoid blocking
|
||||
setImmediate(async () => {
|
||||
try {
|
||||
// Get a handle to the stream as early as possible:
|
||||
const stream = e.execution.read()
|
||||
const terminal = this.getTerminalByVSCETerminal(e.terminal)
|
||||
|
||||
console.info("[onDidStartTerminalShellExecution]", {
|
||||
command: e.execution?.commandLine?.value,
|
||||
terminalId: terminal?.id,
|
||||
console.info("[onDidStartTerminalShellExecution]", {
|
||||
command: e.execution?.commandLine?.value,
|
||||
terminalId: terminal?.id,
|
||||
})
|
||||
|
||||
if (terminal) {
|
||||
terminal.setActiveStream(stream)
|
||||
terminal.busy = true // Mark terminal as busy when shell execution starts
|
||||
} else {
|
||||
// Only log as debug to avoid spamming logs for non-Roo terminals
|
||||
console.debug(
|
||||
"[onDidStartTerminalShellExecution] Shell execution started from non-Roo terminal",
|
||||
)
|
||||
}
|
||||
} catch (error) {
|
||||
console.error(
|
||||
"[onDidStartTerminalShellExecution] Error handling shell execution start:",
|
||||
error,
|
||||
)
|
||||
}
|
||||
})
|
||||
|
||||
if (terminal) {
|
||||
terminal.setActiveStream(stream)
|
||||
terminal.busy = true // Mark terminal as busy when shell execution starts
|
||||
} else {
|
||||
console.error(
|
||||
"[onDidStartTerminalShellExecution] Shell execution started, but not from a Roo-registered terminal:",
|
||||
e,
|
||||
)
|
||||
}
|
||||
},
|
||||
)
|
||||
|
||||
|
|
@ -74,48 +84,54 @@ export class TerminalRegistry {
|
|||
}
|
||||
|
||||
const endDisposable = vscode.window.onDidEndTerminalShellExecution?.(
|
||||
async (e: vscode.TerminalShellExecutionEndEvent) => {
|
||||
const terminal = this.getTerminalByVSCETerminal(e.terminal)
|
||||
const process = terminal?.process
|
||||
const exitDetails = TerminalProcess.interpretExitCode(e.exitCode)
|
||||
(e: vscode.TerminalShellExecutionEndEvent) => {
|
||||
// Process the event asynchronously to avoid blocking
|
||||
setImmediate(async () => {
|
||||
try {
|
||||
const terminal = this.getTerminalByVSCETerminal(e.terminal)
|
||||
const process = terminal?.process
|
||||
const exitDetails = TerminalProcess.interpretExitCode(e.exitCode)
|
||||
|
||||
console.info("[onDidEndTerminalShellExecution]", {
|
||||
command: e.execution?.commandLine?.value,
|
||||
terminalId: terminal?.id,
|
||||
...exitDetails,
|
||||
console.info("[onDidEndTerminalShellExecution]", {
|
||||
command: e.execution?.commandLine?.value,
|
||||
terminalId: terminal?.id,
|
||||
...exitDetails,
|
||||
})
|
||||
|
||||
if (!terminal) {
|
||||
// Only log as debug to avoid spamming logs for non-Roo terminals
|
||||
console.debug(
|
||||
"[onDidEndTerminalShellExecution] Shell execution ended from non-Roo terminal",
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
if (!terminal.running) {
|
||||
console.warn(
|
||||
"[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
|
||||
}
|
||||
|
||||
if (!process) {
|
||||
console.error(
|
||||
"[TerminalRegistry] Shell execution end event received on running terminal, but process is undefined:",
|
||||
{ terminalId: terminal.id, exitCode: e.exitCode },
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// Signal completion to any waiting processes.
|
||||
terminal.shellExecutionComplete(exitDetails)
|
||||
terminal.busy = false // Mark terminal as not busy when shell execution ends
|
||||
} catch (error) {
|
||||
console.error("[onDidEndTerminalShellExecution] Error handling shell execution end:", error)
|
||||
}
|
||||
})
|
||||
|
||||
if (!terminal) {
|
||||
console.error(
|
||||
"[onDidEndTerminalShellExecution] Shell execution ended, but not from a Roo-registered terminal:",
|
||||
e,
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
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 },
|
||||
)
|
||||
|
||||
terminal.busy = false
|
||||
return
|
||||
}
|
||||
|
||||
if (!process) {
|
||||
console.error(
|
||||
"[TerminalRegistry] Shell execution end event received on running terminal, but process is undefined:",
|
||||
{ terminalId: terminal.id, exitCode: e.exitCode },
|
||||
)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
// Signal completion to any waiting processes.
|
||||
terminal.shellExecutionComplete(exitDetails)
|
||||
terminal.busy = false // Mark terminal as not busy when shell execution ends
|
||||
},
|
||||
)
|
||||
|
||||
|
|
@ -161,6 +177,7 @@ export class TerminalRegistry {
|
|||
// matching directory.
|
||||
if (taskId) {
|
||||
terminal = terminals.find((t) => {
|
||||
// Only reuse terminals that belong to this specific task
|
||||
if (t.busy || t.taskId !== taskId || t.provider !== provider) {
|
||||
return false
|
||||
}
|
||||
|
|
@ -175,8 +192,27 @@ export class TerminalRegistry {
|
|||
})
|
||||
}
|
||||
|
||||
// Second priority: Find any available terminal with matching directory.
|
||||
if (!terminal) {
|
||||
// Second priority: Find any available terminal with matching directory
|
||||
// BUT only if it has no task assignment (to avoid cross-task interference)
|
||||
if (!terminal && taskId) {
|
||||
terminal = terminals.find((t) => {
|
||||
// Only use unassigned terminals to prevent cross-task issues
|
||||
if (t.busy || t.taskId !== undefined || t.provider !== provider) {
|
||||
return false
|
||||
}
|
||||
|
||||
const terminalCwd = t.getCurrentWorkingDirectory()
|
||||
|
||||
if (!terminalCwd) {
|
||||
return false
|
||||
}
|
||||
|
||||
return arePathsEqual(vscode.Uri.file(cwd).fsPath, terminalCwd)
|
||||
})
|
||||
}
|
||||
|
||||
// For non-task terminals, allow reuse of any available terminal
|
||||
if (!terminal && !taskId) {
|
||||
terminal = terminals.find((t) => {
|
||||
if (t.busy || t.provider !== provider) {
|
||||
return false
|
||||
|
|
@ -197,6 +233,7 @@ export class TerminalRegistry {
|
|||
terminal = this.createTerminal(cwd, provider)
|
||||
}
|
||||
|
||||
// Assign the task ID to ensure proper isolation
|
||||
terminal.taskId = taskId
|
||||
|
||||
return terminal
|
||||
|
|
@ -284,7 +321,14 @@ export class TerminalRegistry {
|
|||
public static releaseTerminalsForTask(taskId: string): void {
|
||||
this.terminals.forEach((terminal) => {
|
||||
if (terminal.taskId === taskId) {
|
||||
// Mark the terminal as not busy to allow reuse
|
||||
terminal.busy = false
|
||||
// Clear the task association
|
||||
terminal.taskId = undefined
|
||||
// Clear any active process to prevent interference
|
||||
if (terminal.process) {
|
||||
terminal.process = undefined
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
@ -311,7 +355,15 @@ export class TerminalRegistry {
|
|||
* @returns The Terminal object, or undefined if not found
|
||||
*/
|
||||
private static getTerminalByVSCETerminal(vsceTerminal: vscode.Terminal): RooTerminal | undefined {
|
||||
const found = this.terminals.find((t) => t instanceof Terminal && t.terminal === vsceTerminal)
|
||||
// Filter to only our Terminal instances (not ExecaTerminal)
|
||||
const found = this.terminals.find((t) => {
|
||||
// Must be a Terminal instance (not ExecaTerminal)
|
||||
if (!(t instanceof Terminal)) {
|
||||
return false
|
||||
}
|
||||
// Must match the VSCode terminal instance
|
||||
return t.terminal === vsceTerminal
|
||||
})
|
||||
|
||||
if (found?.isClosed()) {
|
||||
this.removeTerminal(found.id)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,409 @@
|
|||
import { describe, it, expect, beforeEach, afterEach, vi, Mock } from "vitest"
|
||||
import { TerminalRegistry } from "../TerminalRegistry"
|
||||
import { Terminal } from "../Terminal"
|
||||
import { ExecaTerminal } from "../ExecaTerminal"
|
||||
import { TerminalProcess } from "../TerminalProcess"
|
||||
|
||||
// Mocks need to be defined in factory functions to avoid hoisting issues
|
||||
vi.mock("vscode", () => {
|
||||
const mockOnDidCloseTerminal = vi.fn()
|
||||
const mockOnDidStartTerminalShellExecution = vi.fn()
|
||||
const mockOnDidEndTerminalShellExecution = vi.fn()
|
||||
|
||||
return {
|
||||
window: {
|
||||
onDidCloseTerminal: mockOnDidCloseTerminal,
|
||||
onDidStartTerminalShellExecution: mockOnDidStartTerminalShellExecution,
|
||||
onDidEndTerminalShellExecution: mockOnDidEndTerminalShellExecution,
|
||||
},
|
||||
Uri: {
|
||||
file: vi.fn((path: string) => ({ fsPath: path })),
|
||||
},
|
||||
}
|
||||
})
|
||||
|
||||
vi.mock("../Terminal")
|
||||
vi.mock("../ExecaTerminal")
|
||||
vi.mock("../ShellIntegrationManager", () => ({
|
||||
ShellIntegrationManager: {
|
||||
zshCleanupTmpDir: vi.fn(),
|
||||
clear: vi.fn(),
|
||||
terminalTmpDirs: new Map(),
|
||||
},
|
||||
}))
|
||||
vi.mock("../TerminalProcess", () => ({
|
||||
TerminalProcess: {
|
||||
interpretExitCode: vi.fn((code: any) => ({ exitCode: code })),
|
||||
},
|
||||
}))
|
||||
|
||||
// Import vscode after mocks are set up
|
||||
import * as vscode from "vscode"
|
||||
|
||||
describe("TerminalRegistry - Concurrent Operations", () => {
|
||||
let mockDisposable: vscode.Disposable
|
||||
let mockVsCodeWindow: any
|
||||
|
||||
beforeEach(() => {
|
||||
// Reset the terminals array for each test
|
||||
;(TerminalRegistry as any).terminals = []
|
||||
;(TerminalRegistry as any).nextTerminalId = 1
|
||||
;(TerminalRegistry as any).isInitialized = false
|
||||
;(TerminalRegistry as any).disposables = []
|
||||
|
||||
mockDisposable = { dispose: vi.fn() }
|
||||
|
||||
// Get mocked window object
|
||||
mockVsCodeWindow = vi.mocked(vscode.window)
|
||||
|
||||
// Mock vscode event handlers to return disposable
|
||||
mockVsCodeWindow.onDidCloseTerminal.mockReturnValue(mockDisposable)
|
||||
mockVsCodeWindow.onDidStartTerminalShellExecution.mockReturnValue(mockDisposable)
|
||||
mockVsCodeWindow.onDidEndTerminalShellExecution.mockReturnValue(mockDisposable)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.clearAllMocks()
|
||||
try {
|
||||
TerminalRegistry.cleanup()
|
||||
} catch (e) {
|
||||
// Ignore cleanup errors in tests
|
||||
}
|
||||
})
|
||||
|
||||
describe("Non-blocking Shell Execution Handlers", () => {
|
||||
it("should handle shell execution start events asynchronously", async () => {
|
||||
TerminalRegistry.initialize()
|
||||
|
||||
const mockTerminal = {
|
||||
id: 1,
|
||||
provider: "vscode",
|
||||
setActiveStream: vi.fn(),
|
||||
busy: false,
|
||||
}
|
||||
|
||||
const mockVSCodeTerminal = {} as vscode.Terminal
|
||||
const mockExecution = {
|
||||
read: vi.fn().mockReturnValue("mock-stream"),
|
||||
commandLine: { value: "test-command" },
|
||||
}
|
||||
|
||||
const mockEvent = {
|
||||
terminal: mockVSCodeTerminal,
|
||||
execution: mockExecution,
|
||||
} as any
|
||||
|
||||
// Add the terminal to registry
|
||||
;(TerminalRegistry as any).terminals = [mockTerminal]
|
||||
|
||||
// Mock getTerminalByVSCETerminal to return our mock terminal
|
||||
const getTerminalSpy = vi
|
||||
.spyOn(TerminalRegistry as any, "getTerminalByVSCETerminal")
|
||||
.mockReturnValue(mockTerminal)
|
||||
|
||||
// Get the handler that was registered
|
||||
const startHandler = mockVsCodeWindow.onDidStartTerminalShellExecution.mock.calls[0][0]
|
||||
|
||||
// Call the handler with our mock event
|
||||
startHandler(mockEvent)
|
||||
|
||||
// Use setImmediate to wait for async processing
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
|
||||
// Verify the terminal was processed
|
||||
expect(mockTerminal.setActiveStream).toHaveBeenCalledWith("mock-stream")
|
||||
expect(mockTerminal.busy).toBe(true)
|
||||
})
|
||||
|
||||
it("should handle shell execution end events asynchronously", async () => {
|
||||
TerminalRegistry.initialize()
|
||||
|
||||
const mockProcess = {
|
||||
hasUnretrievedOutput: vi.fn().mockReturnValue(false),
|
||||
emit: vi.fn(),
|
||||
}
|
||||
|
||||
const mockTerminal = {
|
||||
id: 1,
|
||||
provider: "vscode",
|
||||
busy: true,
|
||||
running: true,
|
||||
process: mockProcess,
|
||||
shellExecutionComplete: vi.fn(),
|
||||
completedProcesses: [],
|
||||
}
|
||||
|
||||
const mockVSCodeTerminal = {} as vscode.Terminal
|
||||
const mockExecution = {
|
||||
commandLine: { value: "test-command" },
|
||||
}
|
||||
|
||||
const mockEvent = {
|
||||
terminal: mockVSCodeTerminal,
|
||||
execution: mockExecution,
|
||||
exitCode: 0,
|
||||
} as any
|
||||
|
||||
// Add the terminal to registry
|
||||
;(TerminalRegistry as any).terminals = [mockTerminal]
|
||||
|
||||
// Mock getTerminalByVSCETerminal to return our mock terminal
|
||||
vi.spyOn(TerminalRegistry as any, "getTerminalByVSCETerminal").mockReturnValue(mockTerminal)
|
||||
|
||||
// Get the handler that was registered
|
||||
const endHandler = mockVsCodeWindow.onDidEndTerminalShellExecution.mock.calls[0][0]
|
||||
|
||||
// Call the handler with our mock event
|
||||
endHandler(mockEvent)
|
||||
|
||||
// Use setImmediate to wait for async processing
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
|
||||
// Verify the terminal was processed
|
||||
expect(mockTerminal.shellExecutionComplete).toHaveBeenCalled()
|
||||
expect(mockTerminal.busy).toBe(false)
|
||||
})
|
||||
|
||||
it("should not block when processing multiple concurrent events", async () => {
|
||||
TerminalRegistry.initialize()
|
||||
|
||||
const terminals = Array.from({ length: 5 }, (_, i) => ({
|
||||
id: i + 1,
|
||||
provider: "vscode" as const,
|
||||
busy: false,
|
||||
running: false,
|
||||
setActiveStream: vi.fn(),
|
||||
shellExecutionComplete: vi.fn(),
|
||||
process: undefined,
|
||||
}))
|
||||
|
||||
// Add all terminals to registry
|
||||
;(TerminalRegistry as any).terminals = terminals
|
||||
|
||||
const startHandler = mockVsCodeWindow.onDidStartTerminalShellExecution.mock.calls[0][0]
|
||||
|
||||
// Create multiple concurrent events
|
||||
const events = terminals.map((terminal, i) => ({
|
||||
terminal: {} as vscode.Terminal,
|
||||
execution: {
|
||||
read: vi.fn().mockReturnValue(`stream-${i}`),
|
||||
commandLine: { value: `command-${i}` },
|
||||
},
|
||||
}))
|
||||
|
||||
// Mock getTerminalByVSCETerminal to return corresponding terminal
|
||||
const getTerminalSpy = vi.spyOn(TerminalRegistry as any, "getTerminalByVSCETerminal")
|
||||
getTerminalSpy.mockImplementation((vsceTerminal) => {
|
||||
const index = events.findIndex((e) => e.terminal === vsceTerminal)
|
||||
return index >= 0 ? terminals[index] : undefined
|
||||
})
|
||||
|
||||
// Fire all events simultaneously
|
||||
const startTime = Date.now()
|
||||
events.forEach((event) => startHandler(event as any))
|
||||
const syncTime = Date.now() - startTime
|
||||
|
||||
// Handlers should return immediately (non-blocking)
|
||||
expect(syncTime).toBeLessThan(10)
|
||||
|
||||
// Wait for all async processing to complete
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
|
||||
// Verify all terminals were processed
|
||||
terminals.forEach((terminal, i) => {
|
||||
expect(terminal.setActiveStream).toHaveBeenCalledWith(`stream-${i}`)
|
||||
expect(terminal.busy).toBe(true)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe("Task Isolation", () => {
|
||||
it("should not reuse terminals across different tasks", async () => {
|
||||
const cwd = "/test/project"
|
||||
|
||||
// Mock Terminal constructor
|
||||
const mockTerminals: any[] = []
|
||||
;(Terminal as any).mockImplementation(function (this: any, id: number, _terminal: any, cwd: string) {
|
||||
const terminal = {
|
||||
id,
|
||||
provider: "vscode",
|
||||
initialCwd: cwd,
|
||||
busy: false,
|
||||
taskId: undefined,
|
||||
getCurrentWorkingDirectory: vi.fn().mockReturnValue(cwd),
|
||||
isClosed: vi.fn().mockReturnValue(false),
|
||||
}
|
||||
mockTerminals.push(terminal)
|
||||
return terminal
|
||||
})
|
||||
|
||||
// Create terminal for task1
|
||||
const terminal1 = await TerminalRegistry.getOrCreateTerminal(cwd, "task1")
|
||||
expect(terminal1.taskId).toBe("task1")
|
||||
|
||||
// Mark terminal1 as not busy to make it available
|
||||
terminal1.busy = false
|
||||
|
||||
// Request terminal for task2 should create a new one, not reuse task1's terminal
|
||||
const terminal2 = await TerminalRegistry.getOrCreateTerminal(cwd, "task2")
|
||||
|
||||
expect(terminal2).not.toBe(terminal1)
|
||||
expect(terminal2.taskId).toBe("task2")
|
||||
expect(terminal1.taskId).toBe("task1") // task1's terminal should remain unchanged
|
||||
})
|
||||
|
||||
it("should allow terminal reuse within the same task", async () => {
|
||||
const cwd = "/test/project"
|
||||
|
||||
// Mock Terminal constructor
|
||||
const mockTerminal = {
|
||||
id: 1,
|
||||
provider: "vscode",
|
||||
initialCwd: cwd,
|
||||
busy: false,
|
||||
taskId: undefined,
|
||||
getCurrentWorkingDirectory: vi.fn().mockReturnValue(cwd),
|
||||
isClosed: vi.fn().mockReturnValue(false),
|
||||
}
|
||||
|
||||
;(Terminal as any).mockImplementation(() => mockTerminal)
|
||||
|
||||
// Create terminal for task1
|
||||
const terminal1 = await TerminalRegistry.getOrCreateTerminal(cwd, "task1")
|
||||
terminal1.busy = false
|
||||
|
||||
// Request another terminal for task1 should reuse the existing one
|
||||
const terminal2 = await TerminalRegistry.getOrCreateTerminal(cwd, "task1")
|
||||
|
||||
expect(terminal2).toBe(terminal1)
|
||||
expect(terminal2.taskId).toBe("task1")
|
||||
})
|
||||
|
||||
it("should properly release terminals when task completes", () => {
|
||||
const terminals = [
|
||||
{ id: 1, taskId: "task1", busy: true, process: {} },
|
||||
{ id: 2, taskId: "task2", busy: true, process: {} },
|
||||
{ id: 3, taskId: "task1", busy: false, process: {} },
|
||||
]
|
||||
|
||||
;(TerminalRegistry as any).terminals = terminals
|
||||
|
||||
TerminalRegistry.releaseTerminalsForTask("task1")
|
||||
|
||||
// Verify task1 terminals were released
|
||||
expect(terminals[0].taskId).toBeUndefined()
|
||||
expect(terminals[0].busy).toBe(false)
|
||||
expect(terminals[0].process).toBeUndefined()
|
||||
|
||||
expect(terminals[2].taskId).toBeUndefined()
|
||||
expect(terminals[2].busy).toBe(false)
|
||||
expect(terminals[2].process).toBeUndefined()
|
||||
|
||||
// task2 terminal should be unaffected
|
||||
expect(terminals[1].taskId).toBe("task2")
|
||||
expect(terminals[1].busy).toBe(true)
|
||||
expect(terminals[1].process).toBeDefined()
|
||||
})
|
||||
|
||||
it("should only use unassigned terminals for new tasks", async () => {
|
||||
const cwd = "/test/project"
|
||||
|
||||
// Create an unassigned terminal
|
||||
const unassignedTerminal: any = {
|
||||
id: 1,
|
||||
provider: "vscode" as const,
|
||||
busy: false,
|
||||
taskId: undefined,
|
||||
getCurrentWorkingDirectory: vi.fn().mockReturnValue(cwd),
|
||||
isClosed: vi.fn().mockReturnValue(false),
|
||||
}
|
||||
|
||||
// Create a terminal assigned to another task
|
||||
const assignedTerminal: any = {
|
||||
id: 2,
|
||||
provider: "vscode" as const,
|
||||
busy: false,
|
||||
taskId: "other-task",
|
||||
getCurrentWorkingDirectory: vi.fn().mockReturnValue(cwd),
|
||||
isClosed: vi.fn().mockReturnValue(false),
|
||||
}
|
||||
|
||||
;(TerminalRegistry as any).terminals = [assignedTerminal, unassignedTerminal]
|
||||
|
||||
// Request terminal for a new task
|
||||
const terminal = await TerminalRegistry.getOrCreateTerminal(cwd, "new-task")
|
||||
|
||||
// Should use the unassigned terminal, not the one assigned to other-task
|
||||
expect(terminal).toBe(unassignedTerminal)
|
||||
expect(unassignedTerminal.taskId).toBe("new-task")
|
||||
expect(assignedTerminal.taskId).toBe("other-task") // Should remain unchanged
|
||||
})
|
||||
})
|
||||
|
||||
describe("Error Handling", () => {
|
||||
it("should handle errors in shell execution start handler", async () => {
|
||||
TerminalRegistry.initialize()
|
||||
|
||||
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
const mockEvent = {
|
||||
terminal: {} as vscode.Terminal,
|
||||
execution: {
|
||||
read: vi.fn().mockImplementation(() => {
|
||||
throw new Error("Stream read failed")
|
||||
}),
|
||||
commandLine: { value: "test-command" },
|
||||
},
|
||||
} as any
|
||||
|
||||
const startHandler = mockVsCodeWindow.onDidStartTerminalShellExecution.mock.calls[0][0]
|
||||
|
||||
// Should not throw
|
||||
expect(() => startHandler(mockEvent)).not.toThrow()
|
||||
|
||||
// Wait for async processing
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
|
||||
// Should log the error
|
||||
expect(consoleSpy).toHaveBeenCalledWith(
|
||||
"[onDidStartTerminalShellExecution] Error handling shell execution start:",
|
||||
expect.any(Error),
|
||||
)
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it("should handle errors in shell execution end handler", async () => {
|
||||
TerminalRegistry.initialize()
|
||||
|
||||
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
// Mock getTerminalByVSCETerminal to throw
|
||||
vi.spyOn(TerminalRegistry as any, "getTerminalByVSCETerminal").mockImplementation(() => {
|
||||
throw new Error("Terminal lookup failed")
|
||||
})
|
||||
|
||||
const mockEvent = {
|
||||
terminal: {} as vscode.Terminal,
|
||||
execution: { commandLine: { value: "test-command" } },
|
||||
exitCode: 0,
|
||||
} as any
|
||||
|
||||
const endHandler = mockVsCodeWindow.onDidEndTerminalShellExecution.mock.calls[0][0]
|
||||
|
||||
// Should not throw
|
||||
expect(() => endHandler(mockEvent)).not.toThrow()
|
||||
|
||||
// Wait for async processing
|
||||
await new Promise((resolve) => setImmediate(resolve))
|
||||
|
||||
// Should log the error
|
||||
expect(consoleSpy).toHaveBeenCalledWith(
|
||||
"[onDidEndTerminalShellExecution] Error handling shell execution end:",
|
||||
expect.any(Error),
|
||||
)
|
||||
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue