From 216b58bcf14c574a39647e15f52a6ab30d2ba5c3 Mon Sep 17 00:00:00 2001 From: VDT-91 Date: Thu, 5 Feb 2026 22:00:49 +0100 Subject: [PATCH] fix: prevent terminal worktree crash with race condition fixes (#1586) (#1658) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: prevent terminal worktree crash with race condition fixes - Remove 100ms dispose delay in Terminal.tsx to prevent race where new terminal mounts before old one is cleaned up - Convert blocking git operations (fetch, worktree add/remove, branch delete) to async in worktree-handlers.ts to avoid freezing main process - Add safeSendToRenderer() checks in pty-manager.ts, terminal-lifecycle.ts, and session-handler.ts to prevent crashes when window is destroyed - Add explicit fitAddon disposal before xterm disposal in useXterm.ts Co-Authored-By: Claude Opus 4.5 * fix: reset isDisposedRef on xterm reinitialization for React StrictMode React StrictMode double-mounts components during development. This caused the terminal display bug where terminals showed cursors but no text output: 1. Component mounts → isDisposedRef initialized to false 2. StrictMode unmount → dispose() sets isDisposedRef.current = true 3. StrictMode remount → same ref persists with true value (never reset) 4. All xterm.write() calls were skipped because isDisposed was true The fix resets isDisposedRef.current = false when xterm reinitializes, ensuring the callback can write to the terminal after remount. Also reset dimensionsReadyCalledRef to prevent stale dimension state. Co-Authored-By: Claude Opus 4.5 * fix: add timeout-specific error messages for git operations When execFileAsync times out, provide a clear user-facing message instead of a generic error, helping users understand the operation timed out rather than failed for other reasons. Co-Authored-By: Claude Opus 4.5 * fix: resolve CI failures from outdated develop branch - Fix Ruff formatting in parallel_orchestrator_reviewer.py and pydantic_models.py - Fix test_integration_phase4.py to register module in sys.modules before exec_module for Python 3.12+ dataclass compatibility Co-Authored-By: Claude Opus 4.5 * fix: address PR review feedback for terminal worktree crash fix - Convert git rev-parse to async (execFileAsync with timeout) for consistency with other git operations in the PR, avoiding main process blocking - Extract duplicated timeout detection logic to isTimeoutError() helper function to reduce code duplication and improve maintainability - Improve Python 3.12+ dataclass comment with more precise explanation of the sys.modules registration requirement Addresses review findings: - NEW-002 [MEDIUM]: Inconsistent async/sync - rev-parse now uses execFileAsync - 2d4eb2f04acb [LOW]: Duplicated timeout detection logic extracted to helper - NEW-004 [LOW]: Comment now explains the AttributeError cause more precisely Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- .../terminal/worktree-handlers.ts | 61 ++++++++++++++----- .../frontend/src/main/terminal/pty-manager.ts | 16 +++-- .../src/main/terminal/session-handler.ts | 7 +-- .../src/main/terminal/terminal-lifecycle.ts | 18 +++--- .../src/renderer/components/Terminal.tsx | 11 ++-- .../renderer/components/terminal/useXterm.ts | 22 +++++-- 6 files changed, 87 insertions(+), 48 deletions(-) diff --git a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts index 6989e014..57d6bf58 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -44,6 +44,18 @@ const GIT_PORCELAIN = { COMMIT_SHA_LENGTH: 8, } as const; +/** + * Check if an error was caused by a timeout (execFileAsync with timeout sets killed=true). + * This helper centralizes the timeout detection logic to avoid duplication. + */ +function isTimeoutError(error: unknown): boolean { + return ( + error instanceof Error && + 'killed' in error && + (error as NodeJS.ErrnoException & { killed?: boolean }).killed === true + ); +} + /** * Fix repositories that are incorrectly marked with core.bare=true. * This can happen when git worktree operations incorrectly set bare=true @@ -399,12 +411,12 @@ async function createTerminalWorktree( const isRemoteRef = baseBranch.startsWith('origin/'); const remoteBranchName = isRemoteRef ? baseBranch.replace('origin/', '') : baseBranch; - // Fetch the branch from remote + // Fetch the branch from remote (async to avoid blocking main process) try { - execFileSync(getToolPath('git'), ['fetch', 'origin', remoteBranchName], { + await execFileAsync(getToolPath('git'), ['fetch', 'origin', remoteBranchName], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 30000, env: getIsolatedGitEnv(), }); debugLog('[TerminalWorktree] Fetched latest from origin/' + remoteBranchName); @@ -426,10 +438,10 @@ async function createTerminalWorktree( } else { // Default behavior: check if remote version exists and use it for latest code try { - execFileSync(getToolPath('git'), ['rev-parse', '--verify', `origin/${baseBranch}`], { + await execFileAsync(getToolPath('git'), ['rev-parse', '--verify', `origin/${baseBranch}`], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 10000, env: getIsolatedGitEnv(), }); baseRef = `origin/${baseBranch}`; @@ -443,18 +455,20 @@ async function createTerminalWorktree( // Use --no-track to prevent the new branch from inheriting upstream tracking // from the base ref (e.g., origin/main). This ensures users can push with -u // to correctly set up tracking to their own remote branch. - execFileSync(getToolPath('git'), ['worktree', 'add', '-b', branchName, '--no-track', worktreePath, baseRef], { + // Use async to avoid blocking the main process on large repos. + await execFileAsync(getToolPath('git'), ['worktree', 'add', '-b', branchName, '--no-track', worktreePath, baseRef], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 60000, env: getIsolatedGitEnv(), }); debugLog('[TerminalWorktree] Created worktree with branch:', branchName, 'from', baseRef); } else { - execFileSync(getToolPath('git'), ['worktree', 'add', '--detach', worktreePath, baseRef], { + // Use async to avoid blocking the main process on large repos. + await execFileAsync(getToolPath('git'), ['worktree', 'add', '--detach', worktreePath, baseRef], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 60000, env: getIsolatedGitEnv(), }); debugLog('[TerminalWorktree] Created worktree in detached HEAD mode from', baseRef); @@ -507,9 +521,16 @@ async function createTerminalWorktree( } } + // Check if error was due to timeout + const isTimeout = isTimeoutError(error); + return { success: false, - error: error instanceof Error ? error.message : 'Failed to create worktree', + error: isTimeout + ? 'Git operation timed out. The repository may be too large or the network connection is slow. Please try again.' + : error instanceof Error + ? error.message + : 'Failed to create worktree', }; } } @@ -718,10 +739,11 @@ async function removeTerminalWorktree( try { if (existsSync(worktreePath)) { - execFileSync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { + // Use async to avoid blocking the main process on large repos + await execFileAsync(getToolPath('git'), ['worktree', 'remove', '--force', worktreePath], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 60000, env: getIsolatedGitEnv(), }); debugLog('[TerminalWorktree] Removed git worktree'); @@ -733,10 +755,11 @@ async function removeTerminalWorktree( debugError('[TerminalWorktree] Invalid branch name in config:', config.branchName); } else { try { - execFileSync(getToolPath('git'), ['branch', '-D', config.branchName], { + // Use async to avoid blocking the main process + await execFileAsync(getToolPath('git'), ['branch', '-D', config.branchName], { cwd: projectPath, encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'], + timeout: 30000, env: getIsolatedGitEnv(), }); debugLog('[TerminalWorktree] Deleted branch:', config.branchName); @@ -760,9 +783,17 @@ async function removeTerminalWorktree( return { success: true }; } catch (error) { debugError('[TerminalWorktree] Error removing worktree:', error); + + // Check if error was due to timeout + const isTimeout = isTimeoutError(error); + return { success: false, - error: error instanceof Error ? error.message : 'Failed to remove worktree', + error: isTimeout + ? 'Git operation timed out. The repository may be too large. Please try again.' + : error instanceof Error + ? error.message + : 'Failed to remove worktree', }; } } diff --git a/apps/frontend/src/main/terminal/pty-manager.ts b/apps/frontend/src/main/terminal/pty-manager.ts index 18fc7ab1..b85d2ba4 100644 --- a/apps/frontend/src/main/terminal/pty-manager.ts +++ b/apps/frontend/src/main/terminal/pty-manager.ts @@ -9,6 +9,7 @@ import { existsSync } from 'fs'; import type { TerminalProcess, WindowGetter, WindowsShellType } from './types'; import { isWindows, getWindowsShellPaths } from '../platform'; import { IPC_CHANNELS } from '../../shared/constants'; +import { safeSendToRenderer } from '../ipc-handlers/utils'; import { getClaudeProfileManager } from '../claude-profile-manager'; import { readSettingsFile } from '../settings-utils'; import { debugLog, debugError } from '../../shared/utils/debug-logger'; @@ -221,11 +222,9 @@ export function setupPtyHandlers( // Call custom data handler onDataCallback(terminal, data); - // Send to renderer - const win = getWindow(); - if (win) { - win.webContents.send(IPC_CHANNELS.TERMINAL_OUTPUT, id, data); - } + // Send to renderer with isDestroyed() check to prevent crashes + // when the window is closed during terminal activity + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_OUTPUT, id, data); }); // Handle terminal exit @@ -245,10 +244,9 @@ export function setupPtyHandlers( // to avoid pty.node SIGABRT from destroyed BrowserWindow resources if (isShuttingDown) return; - const win = getWindow(); - if (win) { - win.webContents.send(IPC_CHANNELS.TERMINAL_EXIT, id, exitCode); - } + // Send to renderer with isDestroyed() check to prevent crashes + // when the window is closed during terminal exit + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_EXIT, id, exitCode); // Call custom exit handler onExitCallback(terminal); diff --git a/apps/frontend/src/main/terminal/session-handler.ts b/apps/frontend/src/main/terminal/session-handler.ts index cd22f480..f04e4a85 100644 --- a/apps/frontend/src/main/terminal/session-handler.ts +++ b/apps/frontend/src/main/terminal/session-handler.ts @@ -10,6 +10,7 @@ import type { TerminalProcess, WindowGetter } from './types'; import { getTerminalSessionStore, type TerminalSession } from '../terminal-session-store'; import { IPC_CHANNELS } from '../../shared/constants'; import { debugLog, debugError } from '../../shared/utils/debug-logger'; +import { safeSendToRenderer } from '../ipc-handlers/utils'; /** * Track session IDs that have been claimed by terminals to prevent race conditions. @@ -336,10 +337,8 @@ export function captureClaudeSessionId( updateClaudeSessionId(terminal.projectPath, terminalId, sessionId); } - const win = getWindow(); - if (win) { - win.webContents.send(IPC_CHANNELS.TERMINAL_CLAUDE_SESSION, terminalId, sessionId); - } + // Use safeSendToRenderer with isDestroyed() check to prevent crashes + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_CLAUDE_SESSION, terminalId, sessionId); } else { // Session was claimed by another terminal, keep polling for a different one debugLog('[SessionHandler] Session ID was claimed by another terminal, continuing to poll:', sessionId); diff --git a/apps/frontend/src/main/terminal/terminal-lifecycle.ts b/apps/frontend/src/main/terminal/terminal-lifecycle.ts index 77565b6e..39b7728d 100644 --- a/apps/frontend/src/main/terminal/terminal-lifecycle.ts +++ b/apps/frontend/src/main/terminal/terminal-lifecycle.ts @@ -17,6 +17,7 @@ import type { } from './types'; import { isWindows } from '../platform'; import { debugLog, debugError } from '../../shared/utils/debug-logger'; +import { safeSendToRenderer } from '../ipc-handlers/utils'; /** * Options for terminal restoration @@ -205,13 +206,11 @@ export async function restoreTerminal( } // Send title change event for all restored terminals so renderer updates - const win = getWindow(); - if (win) { - win.webContents.send(IPC_CHANNELS.TERMINAL_TITLE_CHANGE, session.id, session.title); - // Always sync worktreeConfig to renderer (even if undefined) to ensure correct state - // This handles both: showing labels after recovery AND clearing stale labels when worktrees are deleted - win.webContents.send(IPC_CHANNELS.TERMINAL_WORKTREE_CONFIG_CHANGE, session.id, terminal.worktreeConfig); - } + // Use safeSendToRenderer with isDestroyed() check to prevent crashes + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_TITLE_CHANGE, session.id, session.title); + // Always sync worktreeConfig to renderer (even if undefined) to ensure correct state + // This handles both: showing labels after recovery AND clearing stale labels when worktrees are deleted + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_WORKTREE_CONFIG_CHANGE, session.id, terminal.worktreeConfig); // Defer Claude resume until terminal becomes active (is viewed by user) // This prevents all terminals from resuming Claude simultaneously on app startup, @@ -230,9 +229,8 @@ export async function restoreTerminal( // Notify renderer that this terminal has a pending Claude resume // The renderer will trigger the resume when the terminal tab becomes active - if (win) { - win.webContents.send(IPC_CHANNELS.TERMINAL_PENDING_RESUME, terminal.id, storedClaudeSessionId); - } + // Use safeSendToRenderer with isDestroyed() check to prevent crashes + safeSendToRenderer(getWindow, IPC_CHANNELS.TERMINAL_PENDING_RESUME, terminal.id, storedClaudeSessionId); // Persist the Claude mode and pending resume state if (terminal.projectPath) { diff --git a/apps/frontend/src/renderer/components/Terminal.tsx b/apps/frontend/src/renderer/components/Terminal.tsx index 76a9c733..fdb80c2f 100644 --- a/apps/frontend/src/renderer/components/Terminal.tsx +++ b/apps/frontend/src/renderer/components/Terminal.tsx @@ -604,12 +604,11 @@ export const Terminal = forwardRef(function Termi postCreationTimeoutRef.current = null; } - setTimeout(() => { - if (!isMountedRef.current) { - dispose(); - isCreatedRef.current = false; - } - }, 100); + // Dispose synchronously on unmount to prevent race conditions + // where a new terminal mounts before the old one is cleaned up. + // The previous 100ms delay created a window where both terminals existed. + dispose(); + isCreatedRef.current = false; }; }, [id, dispose, cleanupAutoNaming]); diff --git a/apps/frontend/src/renderer/components/terminal/useXterm.ts b/apps/frontend/src/renderer/components/terminal/useXterm.ts index 137fdadb..a5d4a709 100644 --- a/apps/frontend/src/renderer/components/terminal/useXterm.ts +++ b/apps/frontend/src/renderer/components/terminal/useXterm.ts @@ -72,6 +72,13 @@ export function useXterm({ terminalId, onCommandEnter, onResize, onDimensionsRea return; } + // Reset refs when (re)initializing xterm + // This is critical for React StrictMode which unmounts/remounts components, + // causing dispose() to set isDisposedRef.current = true on the first unmount. + // Without this reset, the remounted component would still have isDisposed = true. + isDisposedRef.current = false; + dimensionsReadyCalledRef.current = false; + debugLog(`[useXterm] Initializing xterm for terminal: ${terminalId}`); const xterm = new XTerm({ @@ -490,15 +497,22 @@ export function useXterm({ terminalId, onCommandEnter, onResize, onDimensionsRea // Serialize buffer before disposing to preserve ANSI formatting serializeBuffer(); - if (xtermRef.current) { - xtermRef.current.dispose(); - xtermRef.current = null; + // Dispose addons explicitly before disposing xterm + // While xterm.dispose() handles loaded addons, explicit disposal ensures + // resources are freed in a predictable order and prevents potential leaks + if (fitAddonRef.current) { + fitAddonRef.current.dispose(); + fitAddonRef.current = null; } if (serializeAddonRef.current) { serializeAddonRef.current.dispose(); serializeAddonRef.current = null; } - fitAddonRef.current = null; + // Note: webLinksAddon is local and will be disposed when xterm.dispose() is called + if (xtermRef.current) { + xtermRef.current.dispose(); + xtermRef.current = null; + } }, [serializeBuffer, terminalId]); return {