fix: prevent terminal worktree crash with race condition fixes (#1586) (#1658)

* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2026-02-05 22:00:49 +01:00
parent 0ddd740bd1
commit 216b58bcf1
6 changed files with 87 additions and 48 deletions
@@ -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',
    };
  }
 }
@@ -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);
@@ -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);
@@ -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) {
@@ -604,12 +604,11 @@ export const Terminal = forwardRef<TerminalHandle, TerminalProps>(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]);

@@ -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 {