From 3ede3ec2a8c044302c67cc92be8857ed0ef1797e Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sat, 14 Feb 2026 19:55:41 +0100 Subject: [PATCH 1/7] feat: symlink Python venvs in worktrees for instant setup Switch venv strategy from RECREATE to SYMLINK so worktree creation is near-instant (matching node_modules behavior). A health check after symlinking verifies the venv is usable; if it fails, falls back to recreate with improved robustness: - Marker-based completion tracking (.setup_complete) detects incomplete venvs - Popen-based subprocess management with proper terminate/kill on timeout - Increased pip install timeout from 120s to 300s Co-Authored-By: Claude Opus 4.6 --- .../core/workspace/dependency_strategy.py | 13 +- apps/backend/core/workspace/setup.py | 120 +++++++++++--- .../terminal/worktree-handlers.ts | 52 ++++-- tests/test_worktree_dependencies.py | 148 ++++++++++++++++-- 4 files changed, 281 insertions(+), 52 deletions(-) diff --git a/apps/backend/core/workspace/dependency_strategy.py b/apps/backend/core/workspace/dependency_strategy.py index 4b9f601453..0510ec153c 100644 --- a/apps/backend/core/workspace/dependency_strategy.py +++ b/apps/backend/core/workspace/dependency_strategy.py @@ -9,9 +9,10 @@ - **node_modules**: Safe to symlink. Node's resolution algorithm follows symlinks correctly, and the directory is self-contained. -- **venv / .venv**: Must be recreated. Python's ``pyvenv.cfg`` discovery walks the - real directory hierarchy without resolving symlinks (CPython bug #106045), so a - symlinked venv resolves paths relative to the *target*, not the worktree. +- **venv / .venv**: Symlinked for fast worktree creation. CPython bug #106045 + (pyvenv.cfg symlink resolution) does not affect typical usage (running scripts, + imports, pip). A health check after symlinking verifies usability; if it fails, + the caller falls back to recreating the venv. - **vendor (PHP)**: Safe to symlink. Composer's autoloader uses ``__DIR__``-relative paths that resolve correctly through symlinks. @@ -42,9 +43,9 @@ DEFAULT_STRATEGY_MAP: dict[str, DependencyStrategy] = { # JavaScript / Node.js — symlink is safe and fast "node_modules": DependencyStrategy.SYMLINK, - # Python — venvs MUST be recreated (pyvenv.cfg symlink bug) - "venv": DependencyStrategy.RECREATE, - ".venv": DependencyStrategy.RECREATE, + # Python — symlink for fast worktree creation (health check + fallback to recreate) + "venv": DependencyStrategy.SYMLINK, + ".venv": DependencyStrategy.SYMLINK, # PHP — Composer vendor dir is safe to symlink "vendor_php": DependencyStrategy.SYMLINK, # Ruby — Bundler vendor/bundle is safe to symlink diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index b3ed57da36..25935d645a 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -50,6 +50,10 @@ def debug_warning(*args, **kwargs): MODULE = "workspace.setup" +# Marker file written inside a recreated venv to indicate setup completed successfully. +# If the marker is absent, the venv is treated as incomplete and will be rebuilt. +VENV_SETUP_COMPLETE_MARKER = ".setup_complete" + def choose_workspace( project_dir: Path, @@ -627,6 +631,39 @@ def setup_worktree_dependencies( performed = True if config.strategy == DependencyStrategy.SYMLINK: performed = _apply_symlink_strategy(project_dir, worktree_path, config) + # For venvs, verify the symlink is usable — fall back to recreate + if performed and config.dep_type in ("venv", ".venv"): + venv_path = worktree_path / config.source_rel_path + if is_windows(): + python_bin = str(venv_path / "Scripts" / "python.exe") + else: + python_bin = str(venv_path / "bin" / "python") + try: + subprocess.run( + [python_bin, "-c", "import sys; print(sys.prefix)"], + capture_output=True, + text=True, + timeout=10, + check=True, + ) + debug( + MODULE, + f"Symlinked venv health check passed: {config.source_rel_path}", + ) + except (subprocess.SubprocessError, OSError): + debug_warning( + MODULE, + f"Symlinked venv health check failed, falling back to recreate: {config.source_rel_path}", + ) + # Remove the broken symlink and recreate + symlink_path = worktree_path / config.source_rel_path + if symlink_path.is_symlink(): + symlink_path.unlink() + elif symlink_path.exists(): + shutil.rmtree(symlink_path, ignore_errors=True) + performed = _apply_recreate_strategy( + project_dir, worktree_path, config + ) elif config.strategy == DependencyStrategy.RECREATE: performed = _apply_recreate_strategy(project_dir, worktree_path, config) elif config.strategy == DependencyStrategy.COPY: @@ -707,6 +744,40 @@ def _apply_symlink_strategy( return False +def _popen_with_cleanup( + cmd: list[str], + timeout: int, + label: str, +) -> tuple[int, str, str]: + """Run a command via Popen with proper process cleanup on timeout. + + On timeout: terminate → wait(10) → kill → wait(5) to ensure file locks + are released before any cleanup (e.g. shutil.rmtree). + + Returns (returncode, stdout, stderr). + Raises subprocess.TimeoutExpired if the process could not be stopped. + """ + proc = subprocess.Popen( + cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + try: + stdout, stderr = proc.communicate(timeout=timeout) + return proc.returncode, stdout, stderr + except subprocess.TimeoutExpired: + debug_warning(MODULE, f"{label} timed out, terminating process") + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + debug_warning(MODULE, f"{label} did not terminate, killing process") + proc.kill() + proc.wait(timeout=5) + raise + + def _apply_recreate_strategy( project_dir: Path, worktree_path: Path, @@ -717,10 +788,18 @@ def _apply_recreate_strategy( Returns True if the venv was successfully created, False if skipped or failed. """ venv_path = worktree_path / config.source_rel_path + marker_path = venv_path / VENV_SETUP_COMPLETE_MARKER if venv_path.exists(): - debug(MODULE, f"Skipping recreate {config.source_rel_path} - already exists") - return False + if marker_path.exists(): + debug( + MODULE, + f"Skipping recreate {config.source_rel_path} - already complete (marker present)", + ) + return False + # Venv exists but marker is missing — incomplete, remove and rebuild + debug(MODULE, f"Removing incomplete venv {config.source_rel_path} (no marker)") + shutil.rmtree(venv_path, ignore_errors=True) # Detect Python executable from the source venv or fall back to sys.executable source_venv = project_dir / config.source_rel_path @@ -737,29 +816,25 @@ def _apply_recreate_strategy( # Create the venv try: debug(MODULE, f"Creating venv at {venv_path}") - result = subprocess.run( + returncode, _, stderr = _popen_with_cleanup( [python_exec, "-m", "venv", str(venv_path)], - capture_output=True, - text=True, timeout=120, + label=f"venv creation ({config.source_rel_path})", ) - if result.returncode != 0: - debug_warning(MODULE, f"venv creation failed: {result.stderr}") + if returncode != 0: + debug_warning(MODULE, f"venv creation failed: {stderr}") print_status( f"Warning: Could not create venv at {config.source_rel_path}", "warning", ) - # Clean up partial venv so retries aren't blocked if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False except subprocess.TimeoutExpired: - debug_warning(MODULE, f"venv creation timed out for {config.source_rel_path}") print_status( f"Warning: venv creation timed out for {config.source_rel_path}", "warning", ) - # Clean up partial venv so retries aren't blocked if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False @@ -800,46 +875,43 @@ def _apply_recreate_strategy( if install_cmd: try: debug(MODULE, f"Installing deps from {req_file}") - pip_result = subprocess.run( + returncode, _, stderr = _popen_with_cleanup( install_cmd, - capture_output=True, - text=True, - timeout=120, + timeout=300, + label=f"pip install ({req_file})", ) - if pip_result.returncode != 0: + if returncode != 0: debug_warning( MODULE, - f"pip install failed (exit {pip_result.returncode}): " - f"{pip_result.stderr}", + f"pip install failed (exit {returncode}): {stderr}", ) print_status( f"Warning: Dependency install failed for {req_file}", "warning", ) - # Clean up broken venv so retries aren't blocked if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False except subprocess.TimeoutExpired: - debug_warning( - MODULE, - f"pip install timed out for {req_file}", - ) print_status( f"Warning: Dependency install timed out for {req_file}", "warning", ) - # Clean up broken venv so retries aren't blocked if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False except OSError as e: debug_warning(MODULE, f"pip install failed: {e}") - # Clean up broken venv so retries aren't blocked if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False + # Write completion marker so future runs know this venv is complete + try: + marker_path.touch() + except OSError: + pass # Non-fatal — venv is still usable without the marker + debug(MODULE, f"Recreated venv at {config.source_rel_path}") return True 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 b241fba7f6..95ae7db481 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -250,11 +250,12 @@ interface DependencyConfig { const DEFAULT_STRATEGY_MAP: Record = { // JavaScript / Node.js — symlink is safe and fast node_modules: 'symlink', - // Python — venvs MUST be recreated, not symlinked. - // CPython bug #106045: pyvenv.cfg discovery does not resolve symlinks, - // so a symlinked venv resolves paths relative to the target, not the worktree. - venv: 'recreate', - '.venv': 'recreate', + // Python — symlink for fast worktree creation. + // CPython bug #106045 (pyvenv.cfg symlink resolution) does not affect + // typical usage (running scripts, imports, pip). If the health check + // after symlinking fails, we fall back to recreate automatically. + venv: 'symlink', + '.venv': 'symlink', // PHP — Composer vendor dir is safe to symlink vendor_php: 'symlink', // Ruby — Bundler vendor/bundle is safe to symlink @@ -364,6 +365,24 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri switch (config.strategy) { case 'symlink': performed = applySymlinkStrategy(projectPath, worktreePath, config); + // For venvs, verify the symlink is usable — fall back to recreate if not + if (performed && (config.depType === 'venv' || config.depType === '.venv')) { + const venvPath = path.join(worktreePath, config.sourceRelPath); + const pythonBin = isWindows() + ? path.join(venvPath, 'Scripts', 'python.exe') + : path.join(venvPath, 'bin', 'python'); + try { + await execFileAsync(pythonBin, ['-c', 'import sys; print(sys.prefix)'], { + timeout: 10000, + }); + debugLog('[TerminalWorktree] Symlinked venv health check passed:', config.sourceRelPath); + } catch { + debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); + // Remove the broken symlink and recreate + try { rmSync(path.join(worktreePath, config.sourceRelPath), { force: true }); } catch { /* best-effort */ } + performed = await applyRecreateStrategy(projectPath, worktreePath, config); + } + } break; case 'recreate': performed = await applyRecreateStrategy(projectPath, worktreePath, config); @@ -434,19 +453,27 @@ function applySymlinkStrategy(projectPath: string, worktreePath: string, config: } } +/** Marker file written inside a recreated venv to indicate setup completed successfully. */ +const VENV_SETUP_COMPLETE_MARKER = '.setup_complete'; + /** * Apply recreate strategy: create a fresh virtual environment in the worktree. * - * Python venvs cannot be symlinked due to CPython bug #106045 — pyvenv.cfg - * discovery does not resolve symlinks, so paths resolve relative to the - * symlink target instead of the worktree. + * Used as a fallback when venv symlinking fails (CPython bug #106045). + * Writes a completion marker so incomplete venvs can be detected and rebuilt. */ async function applyRecreateStrategy(projectPath: string, worktreePath: string, config: DependencyConfig): Promise { const venvPath = path.join(worktreePath, config.sourceRelPath); + const markerPath = path.join(venvPath, VENV_SETUP_COMPLETE_MARKER); if (existsSync(venvPath)) { - debugLog('[TerminalWorktree] Skipping recreate', config.sourceRelPath, '- already exists'); - return false; + if (existsSync(markerPath)) { + debugLog('[TerminalWorktree] Skipping recreate', config.sourceRelPath, '- already complete (marker present)'); + return false; + } + // Venv exists but marker is missing — incomplete, remove and rebuild + debugLog('[TerminalWorktree] Removing incomplete venv', config.sourceRelPath, '(no marker)'); + try { rmSync(venvPath, { recursive: true, force: true }); } catch { /* best-effort */ } } // Detect Python executable from the source venv or fall back to system Python @@ -514,7 +541,7 @@ async function applyRecreateStrategy(projectPath: string, worktreePath: string, debugLog('[TerminalWorktree] Installing deps from', config.requirementsFile); await execFileAsync(pipExec, installArgs, { encoding: 'utf-8', - timeout: 120000, + timeout: 300000, }); } catch (error) { if (isTimeoutError(error)) { @@ -533,6 +560,9 @@ async function applyRecreateStrategy(projectPath: string, worktreePath: string, } } + // Write completion marker so future runs know this venv is complete + try { writeFileSync(markerPath, ''); } catch { /* non-fatal */ } + debugLog('[TerminalWorktree] Recreated venv at', config.sourceRelPath); return true; } diff --git a/tests/test_worktree_dependencies.py b/tests/test_worktree_dependencies.py index 4d5b6b89e2..a8aa13743c 100644 --- a/tests/test_worktree_dependencies.py +++ b/tests/test_worktree_dependencies.py @@ -69,13 +69,13 @@ def test_create_with_all_fields(self): """Config creates with all fields populated.""" config = DependencyShareConfig( dep_type="venv", - strategy=DependencyStrategy.RECREATE, + strategy=DependencyStrategy.SYMLINK, source_rel_path=".venv", requirements_file="requirements.txt", package_manager="uv", ) assert config.dep_type == "venv" - assert config.strategy == DependencyStrategy.RECREATE + assert config.strategy == DependencyStrategy.SYMLINK assert config.source_rel_path == ".venv" assert config.requirements_file == "requirements.txt" assert config.package_manager == "uv" @@ -88,13 +88,13 @@ def test_node_modules_is_symlink(self): """node_modules maps to SYMLINK.""" assert DEFAULT_STRATEGY_MAP["node_modules"] == DependencyStrategy.SYMLINK - def test_venv_is_recreate(self): - """venv maps to RECREATE.""" - assert DEFAULT_STRATEGY_MAP["venv"] == DependencyStrategy.RECREATE + def test_venv_is_symlink(self): + """venv maps to SYMLINK (fast worktree creation with health check fallback).""" + assert DEFAULT_STRATEGY_MAP["venv"] == DependencyStrategy.SYMLINK - def test_dot_venv_is_recreate(self): - """.venv maps to RECREATE.""" - assert DEFAULT_STRATEGY_MAP[".venv"] == DependencyStrategy.RECREATE + def test_dot_venv_is_symlink(self): + """.venv maps to SYMLINK (fast worktree creation with health check fallback).""" + assert DEFAULT_STRATEGY_MAP[".venv"] == DependencyStrategy.SYMLINK def test_vendor_php_is_symlink(self): """vendor_php maps to SYMLINK.""" @@ -138,7 +138,7 @@ def test_with_mock_project_index(self): by_type = {c.dep_type: c for c in configs} assert by_type["node_modules"].strategy == DependencyStrategy.SYMLINK assert by_type["node_modules"].source_rel_path == "node_modules" - assert by_type["venv"].strategy == DependencyStrategy.RECREATE + assert by_type["venv"].strategy == DependencyStrategy.SYMLINK assert by_type["venv"].source_rel_path == "apps/backend/.venv" assert by_type["venv"].requirements_file == "requirements.txt" assert by_type["venv"].package_manager == "uv" @@ -238,12 +238,12 @@ def test_multiple_python_services_own_venv_configs(self): assert "services/worker/.venv" in paths api_config = paths["services/api/.venv"] - assert api_config.strategy == DependencyStrategy.RECREATE + assert api_config.strategy == DependencyStrategy.SYMLINK assert api_config.package_manager == "pip" assert api_config.requirements_file == "requirements.txt" worker_config = paths["services/worker/.venv"] - assert worker_config.strategy == DependencyStrategy.RECREATE + assert worker_config.strategy == DependencyStrategy.SYMLINK assert worker_config.package_manager == "uv" assert worker_config.requirements_file == "pyproject.toml" @@ -582,6 +582,132 @@ def test_target_already_exists_skipped_gracefully(self, tmp_path: Path): assert not (worktree_path / "node_modules").is_symlink() +class TestVenvSymlinkWithHealthCheck: + """Tests for venv symlink strategy with health check and fallback to recreate.""" + + def test_venv_symlinked_when_source_exists(self, tmp_path: Path): + """Venv is symlinked (not recreated) when source venv exists.""" + from core.workspace.setup import setup_worktree_dependencies + + project_dir = tmp_path / "project" + project_dir.mkdir() + venv_dir = project_dir / ".venv" + venv_dir.mkdir() + # Create a minimal venv structure so the symlink target looks real + (venv_dir / "bin").mkdir() + (venv_dir / "lib").mkdir() + + worktree_path = tmp_path / "worktree" + worktree_path.mkdir() + + project_index = { + "dependency_locations": [ + {"type": ".venv", "path": ".venv", "service": "backend"}, + ] + } + + results = setup_worktree_dependencies(project_dir, worktree_path, project_index) + + target = worktree_path / ".venv" + # The symlink should have been created (regardless of health check outcome) + assert target.exists() or target.is_symlink() + + def test_venv_health_check_fallback_to_recreate(self, tmp_path: Path): + """When symlinked venv health check fails, falls back to recreate.""" + from core.workspace.setup import setup_worktree_dependencies + + project_dir = tmp_path / "project" + project_dir.mkdir() + # Create a source venv that has no python binary (health check will fail) + venv_dir = project_dir / ".venv" + venv_dir.mkdir() + + worktree_path = tmp_path / "worktree" + worktree_path.mkdir() + + project_index = { + "dependency_locations": [ + {"type": ".venv", "path": ".venv", "service": "backend"}, + ] + } + + # This should symlink, then health check fails (no python binary), + # then fall back to recreate (which will also fail since no real python + # in source). The important thing is it doesn't raise. + results = setup_worktree_dependencies(project_dir, worktree_path, project_index) + # Should not crash + assert isinstance(results, dict) + + +class TestRecreateStrategyMarker: + """Tests for the .setup_complete marker in the recreate strategy.""" + + def test_marker_constant_defined(self): + """VENV_SETUP_COMPLETE_MARKER is defined.""" + from core.workspace.setup import VENV_SETUP_COMPLETE_MARKER + assert VENV_SETUP_COMPLETE_MARKER == ".setup_complete" + + def test_incomplete_venv_detected_and_removed(self, tmp_path: Path): + """Venv without marker is detected as incomplete.""" + from core.workspace.setup import _apply_recreate_strategy, VENV_SETUP_COMPLETE_MARKER + from core.workspace.models import DependencyShareConfig, DependencyStrategy + + project_dir = tmp_path / "project" + project_dir.mkdir() + worktree_path = tmp_path / "worktree" + worktree_path.mkdir() + + # Create an incomplete venv (no marker) + incomplete_venv = worktree_path / ".venv" + incomplete_venv.mkdir() + (incomplete_venv / "bin").mkdir() + + config = DependencyShareConfig( + dep_type=".venv", + strategy=DependencyStrategy.RECREATE, + source_rel_path=".venv", + ) + + # Will try to recreate (remove incomplete + rebuild). May fail due to + # no real python, but the incomplete venv should be removed. + _apply_recreate_strategy(project_dir, worktree_path, config) + + # The incomplete venv without marker should have been removed + # (recreation may or may not succeed depending on Python availability) + if incomplete_venv.exists(): + # If it was recreated successfully, marker should exist + assert (incomplete_venv / VENV_SETUP_COMPLETE_MARKER).exists() + + def test_complete_venv_skipped(self, tmp_path: Path): + """Venv with marker is skipped (not rebuilt).""" + from core.workspace.setup import _apply_recreate_strategy, VENV_SETUP_COMPLETE_MARKER + from core.workspace.models import DependencyShareConfig, DependencyStrategy + + project_dir = tmp_path / "project" + project_dir.mkdir() + worktree_path = tmp_path / "worktree" + worktree_path.mkdir() + + # Create a complete venv (with marker) + complete_venv = worktree_path / ".venv" + complete_venv.mkdir() + (complete_venv / VENV_SETUP_COMPLETE_MARKER).touch() + # Add a canary file to verify the venv wasn't rebuilt + (complete_venv / "canary.txt").write_text("original") + + config = DependencyShareConfig( + dep_type=".venv", + strategy=DependencyStrategy.RECREATE, + source_rel_path=".venv", + ) + + result = _apply_recreate_strategy(project_dir, worktree_path, config) + + assert result is False # Skipped + # Canary file should still be present (not rebuilt) + assert (complete_venv / "canary.txt").read_text() == "original" + + class TestSymlinkNodeModulesToWorktreeBackwardCompat: """Tests for symlink_node_modules_to_worktree() backward compatibility.""" From 814cf763f8a548fdce59d3acf6e47978f02950cb Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 17:37:58 +0100 Subject: [PATCH 2/7] fix: address follow-up review findings for PR #1832 - Wrap symlink removal in try/except in Python health check fallback to prevent PermissionError from skipping recreate strategy - Add recursive: true flag to TypeScript rmSync call for consistent Windows junction handling Co-Authored-By: Claude Opus 4.6 --- apps/backend/core/workspace/setup.py | 11 +++++++---- .../main/ipc-handlers/terminal/worktree-handlers.ts | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index 25935d645a..df55060d70 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -657,10 +657,13 @@ def setup_worktree_dependencies( ) # Remove the broken symlink and recreate symlink_path = worktree_path / config.source_rel_path - if symlink_path.is_symlink(): - symlink_path.unlink() - elif symlink_path.exists(): - shutil.rmtree(symlink_path, ignore_errors=True) + try: + if symlink_path.is_symlink(): + symlink_path.unlink() + elif symlink_path.exists(): + shutil.rmtree(symlink_path, ignore_errors=True) + except OSError: + pass # Best-effort removal; recreate strategy handles existing paths performed = _apply_recreate_strategy( project_dir, worktree_path, config ) 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 95ae7db481..8a6c849d08 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -379,7 +379,7 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri } catch { debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); // Remove the broken symlink and recreate - try { rmSync(path.join(worktreePath, config.sourceRelPath), { force: true }); } catch { /* best-effort */ } + try { rmSync(path.join(worktreePath, config.sourceRelPath), { recursive: true, force: true }); } catch { /* best-effort */ } performed = await applyRecreateStrategy(projectPath, worktreePath, config); } } From 836cc385b3525657eb39ca895b4a4db5fe3e2143 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 20:28:14 +0100 Subject: [PATCH 3/7] fix: address medium-severity PR review findings - Fix potential deadlock in _popen_with_cleanup by using proc.communicate() instead of proc.wait() to drain pipes after terminate/kill - Fix health check skip on re-runs by decoupling health check from 'performed' flag - now runs whenever venv symlink exists, detecting broken source venvs between runs Co-Authored-By: Claude Opus 4.6 --- apps/backend/core/workspace/setup.py | 73 +++++++++++++++------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index df55060d70..1d0a59283b 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -632,41 +632,44 @@ def setup_worktree_dependencies( if config.strategy == DependencyStrategy.SYMLINK: performed = _apply_symlink_strategy(project_dir, worktree_path, config) # For venvs, verify the symlink is usable — fall back to recreate - if performed and config.dep_type in ("venv", ".venv"): + # Run health check whenever a venv symlink exists (not just on creation) + if config.dep_type in ("venv", ".venv"): venv_path = worktree_path / config.source_rel_path - if is_windows(): - python_bin = str(venv_path / "Scripts" / "python.exe") - else: - python_bin = str(venv_path / "bin" / "python") - try: - subprocess.run( - [python_bin, "-c", "import sys; print(sys.prefix)"], - capture_output=True, - text=True, - timeout=10, - check=True, - ) - debug( - MODULE, - f"Symlinked venv health check passed: {config.source_rel_path}", - ) - except (subprocess.SubprocessError, OSError): - debug_warning( - MODULE, - f"Symlinked venv health check failed, falling back to recreate: {config.source_rel_path}", - ) - # Remove the broken symlink and recreate - symlink_path = worktree_path / config.source_rel_path + # Check if venv exists (symlinked or otherwise) + if venv_path.exists() or venv_path.is_symlink(): + if is_windows(): + python_bin = str(venv_path / "Scripts" / "python.exe") + else: + python_bin = str(venv_path / "bin" / "python") try: - if symlink_path.is_symlink(): - symlink_path.unlink() - elif symlink_path.exists(): - shutil.rmtree(symlink_path, ignore_errors=True) - except OSError: - pass # Best-effort removal; recreate strategy handles existing paths - performed = _apply_recreate_strategy( - project_dir, worktree_path, config - ) + subprocess.run( + [python_bin, "-c", "import sys; print(sys.prefix)"], + capture_output=True, + text=True, + timeout=10, + check=True, + ) + debug( + MODULE, + f"Symlinked venv health check passed: {config.source_rel_path}", + ) + except (subprocess.SubprocessError, OSError): + debug_warning( + MODULE, + f"Symlinked venv health check failed, falling back to recreate: {config.source_rel_path}", + ) + # Remove the broken symlink and recreate + symlink_path = worktree_path / config.source_rel_path + try: + if symlink_path.is_symlink(): + symlink_path.unlink() + elif symlink_path.exists(): + shutil.rmtree(symlink_path, ignore_errors=True) + except OSError: + pass # Best-effort removal; recreate strategy handles existing paths + performed = _apply_recreate_strategy( + project_dir, worktree_path, config + ) elif config.strategy == DependencyStrategy.RECREATE: performed = _apply_recreate_strategy(project_dir, worktree_path, config) elif config.strategy == DependencyStrategy.COPY: @@ -773,11 +776,11 @@ def _popen_with_cleanup( debug_warning(MODULE, f"{label} timed out, terminating process") proc.terminate() try: - proc.wait(timeout=10) + proc.communicate(timeout=10) except subprocess.TimeoutExpired: debug_warning(MODULE, f"{label} did not terminate, killing process") proc.kill() - proc.wait(timeout=5) + proc.communicate(timeout=5) raise From 1d4b749d55a0b2b03f76212c3a87c0992dab84f3 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 22:15:40 +0100 Subject: [PATCH 4/7] fix: address all PR review findings for venv health check and fallback logic Backend fixes: - Detect broken symlinks in _apply_recreate_strategy using is_symlink() - Add OSError catch in venv creation block for missing python_exec - Update strategy_name to 'recreate' when health check triggers fallback - Log marker write failures via debug_warning instead of silent pass - Update DependencyStrategy docstring to reflect symlink-first approach Frontend fixes: - Decouple health check from 'performed' flag - run on any existing venv - Add isSymlinkOrJunction() helper for broken symlink detection - Detect broken symlinks in applyRecreateStrategy before existsSync check - Log marker write failures instead of silent catch All blocking and medium-severity review findings addressed: - [NCR-001/CMT-001] Frontend health check now runs on re-runs - [NCR-003] Backend recreate detects broken symlinks - [NCR-005] Frontend recreate detects broken symlinks - [NEW-002] Strategy name correctly updated on fallback - [NCR-004] OSError handling added to venv creation - [NEW-003] DependencyStrategy docstring updated - Marker write failures now logged for debugging Co-Authored-By: Claude Opus 4.6 --- apps/backend/core/workspace/models.py | 11 ++-- apps/backend/core/workspace/setup.py | 27 ++++++++- .../terminal/worktree-handlers.ts | 57 +++++++++++++------ 3 files changed, 70 insertions(+), 25 deletions(-) diff --git a/apps/backend/core/workspace/models.py b/apps/backend/core/workspace/models.py index b9092012ca..568cbd3cf4 100644 --- a/apps/backend/core/workspace/models.py +++ b/apps/backend/core/workspace/models.py @@ -278,12 +278,11 @@ def _scan_specs_dir(self, specs_dir: Path) -> int: class DependencyStrategy(Enum): """Strategy for sharing dependency directories across worktrees. - SYMLINK is fast but unsafe for certain ecosystems. Notably, Python venv - breaks when symlinked because CPython's pyvenv.cfg discovery walks the - real directory hierarchy without resolving symlinks first - (CPython bug #106045). This means a symlinked venv resolves its home - path relative to the symlink target's parent, not the worktree, causing - import failures and broken interpreters. + SYMLINK is fast and now safe for Python venvs with runtime health checks. + A post-symlink health check validates the venv is usable, automatically + falling back to RECREATE if the symlink is broken. This works around + CPython's pyvenv.cfg discovery issue (CPython bug #106045) while maintaining + fast worktree creation in the common case where symlinking succeeds. """ SYMLINK = "symlink" # Create a symlink to the source (fast, works for node_modules) diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index 1d0a59283b..e544382c1a 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -670,6 +670,9 @@ def setup_worktree_dependencies( performed = _apply_recreate_strategy( project_dir, worktree_path, config ) + # Update strategy name to reflect fallback + if performed: + strategy_name = "recreate" elif config.strategy == DependencyStrategy.RECREATE: performed = _apply_recreate_strategy(project_dir, worktree_path, config) elif config.strategy == DependencyStrategy.COPY: @@ -796,7 +799,14 @@ def _apply_recreate_strategy( venv_path = worktree_path / config.source_rel_path marker_path = venv_path / VENV_SETUP_COMPLETE_MARKER - if venv_path.exists(): + # Check for broken symlinks that exists() would miss + if venv_path.is_symlink() and not venv_path.exists(): + debug(MODULE, f"Removing broken symlink at {config.source_rel_path}") + try: + venv_path.unlink() + except OSError: + pass # Best-effort removal + elif venv_path.exists(): if marker_path.exists(): debug( MODULE, @@ -844,6 +854,15 @@ def _apply_recreate_strategy( if venv_path.exists(): shutil.rmtree(venv_path, ignore_errors=True) return False + except OSError as e: + debug_warning(MODULE, f"venv creation failed: {e}") + print_status( + f"Warning: Could not create venv at {config.source_rel_path}", + "warning", + ) + if venv_path.exists(): + shutil.rmtree(venv_path, ignore_errors=True) + return False # Install from requirements file if specified req_file = config.requirements_file @@ -915,8 +934,10 @@ def _apply_recreate_strategy( # Write completion marker so future runs know this venv is complete try: marker_path.touch() - except OSError: - pass # Non-fatal — venv is still usable without the marker + except OSError as e: + debug_warning( + MODULE, f"Failed to write completion marker at {marker_path}: {e}" + ) debug(MODULE, f"Recreated venv at {config.source_rel_path}") return True 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 8a6c849d08..2ef8861301 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -57,6 +57,19 @@ function isTimeoutError(error: unknown): boolean { ); } +/** + * Check if a path is a symlink or Windows junction (including broken ones). + * Uses lstatSync to detect the link itself, not what it points to. + */ +function isSymlinkOrJunction(targetPath: string): boolean { + try { + const stats = lstatSync(targetPath); + return stats.isSymbolicLink(); + } catch { + return false; // Path doesn't exist at all + } +} + /** * Fix repositories that are incorrectly marked with core.bare=true. * This can happen when git worktree operations incorrectly set bare=true @@ -366,21 +379,25 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri case 'symlink': performed = applySymlinkStrategy(projectPath, worktreePath, config); // For venvs, verify the symlink is usable — fall back to recreate if not - if (performed && (config.depType === 'venv' || config.depType === '.venv')) { + // Run health check whenever a venv exists (not just on fresh creation) + if (config.depType === 'venv' || config.depType === '.venv') { const venvPath = path.join(worktreePath, config.sourceRelPath); - const pythonBin = isWindows() - ? path.join(venvPath, 'Scripts', 'python.exe') - : path.join(venvPath, 'bin', 'python'); - try { - await execFileAsync(pythonBin, ['-c', 'import sys; print(sys.prefix)'], { - timeout: 10000, - }); - debugLog('[TerminalWorktree] Symlinked venv health check passed:', config.sourceRelPath); - } catch { - debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); - // Remove the broken symlink and recreate - try { rmSync(path.join(worktreePath, config.sourceRelPath), { recursive: true, force: true }); } catch { /* best-effort */ } - performed = await applyRecreateStrategy(projectPath, worktreePath, config); + // Check if venv path exists (as symlink or otherwise) + if (existsSync(venvPath) || isSymlinkOrJunction(venvPath)) { + const pythonBin = isWindows() + ? path.join(venvPath, 'Scripts', 'python.exe') + : path.join(venvPath, 'bin', 'python'); + try { + await execFileAsync(pythonBin, ['-c', 'import sys; print(sys.prefix)'], { + timeout: 10000, + }); + debugLog('[TerminalWorktree] Symlinked venv health check passed:', config.sourceRelPath); + } catch { + debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); + // Remove the broken symlink and recreate + try { rmSync(path.join(worktreePath, config.sourceRelPath), { recursive: true, force: true }); } catch { /* best-effort */ } + performed = await applyRecreateStrategy(projectPath, worktreePath, config); + } } } break; @@ -466,7 +483,11 @@ async function applyRecreateStrategy(projectPath: string, worktreePath: string, const venvPath = path.join(worktreePath, config.sourceRelPath); const markerPath = path.join(venvPath, VENV_SETUP_COMPLETE_MARKER); - if (existsSync(venvPath)) { + // Check for broken symlinks that existsSync would miss + if (isSymlinkOrJunction(venvPath) && !existsSync(venvPath)) { + debugLog('[TerminalWorktree] Removing broken symlink at', config.sourceRelPath); + try { rmSync(venvPath, { recursive: true, force: true }); } catch { /* best-effort */ } + } else if (existsSync(venvPath)) { if (existsSync(markerPath)) { debugLog('[TerminalWorktree] Skipping recreate', config.sourceRelPath, '- already complete (marker present)'); return false; @@ -561,7 +582,11 @@ async function applyRecreateStrategy(projectPath: string, worktreePath: string, } // Write completion marker so future runs know this venv is complete - try { writeFileSync(markerPath, ''); } catch { /* non-fatal */ } + try { + writeFileSync(markerPath, ''); + } catch (error) { + debugLog('[TerminalWorktree] Failed to write completion marker at', markerPath, ':', error); + } debugLog('[TerminalWorktree] Recreated venv at', config.sourceRelPath); return true; From 84184fafb56f185fb872373fea165d55e86228f2 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Mon, 16 Feb 2026 08:47:49 +0100 Subject: [PATCH 5/7] fix: address all 4 findings from auto-claude PR review Fixes all issues from 2026-02-16 auto-claude review: - [HIGH] NEW-001: Fix KeyError when venv symlink falls back to recreate strategy by ensuring results dict has the 'recreate' key before appending - [MEDIUM] NEW-002: Fix isSymlinkOrJunction to detect Windows junctions by using readlinkSync instead of lstatSync().isSymbolicLink() - [LOW] NEW-003: Add finally block to _popen_with_cleanup to prevent resource leaks by ensuring pipes are closed and process is reaped - [LOW] CMT-002: Remove redundant symlink_path variable and reuse venv_path Co-Authored-By: Claude Opus 4.6 --- apps/backend/core/workspace/setup.py | 27 ++++++++++++++----- .../terminal/worktree-handlers.ts | 12 +++++---- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index e544382c1a..511451ab3d 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -659,12 +659,11 @@ def setup_worktree_dependencies( f"Symlinked venv health check failed, falling back to recreate: {config.source_rel_path}", ) # Remove the broken symlink and recreate - symlink_path = worktree_path / config.source_rel_path try: - if symlink_path.is_symlink(): - symlink_path.unlink() - elif symlink_path.exists(): - shutil.rmtree(symlink_path, ignore_errors=True) + if venv_path.is_symlink(): + venv_path.unlink() + elif venv_path.exists(): + shutil.rmtree(venv_path, ignore_errors=True) except OSError: pass # Best-effort removal; recreate strategy handles existing paths performed = _apply_recreate_strategy( @@ -673,6 +672,8 @@ def setup_worktree_dependencies( # Update strategy name to reflect fallback if performed: strategy_name = "recreate" + # Ensure the key exists for the fallback strategy + results.setdefault(strategy_name, []) elif config.strategy == DependencyStrategy.RECREATE: performed = _apply_recreate_strategy(project_dir, worktree_path, config) elif config.strategy == DependencyStrategy.COPY: @@ -783,8 +784,22 @@ def _popen_with_cleanup( except subprocess.TimeoutExpired: debug_warning(MODULE, f"{label} did not terminate, killing process") proc.kill() - proc.communicate(timeout=5) + try: + proc.communicate(timeout=5) + except subprocess.TimeoutExpired: + # Final cleanup attempt if kill() also hangs + debug_warning(MODULE, f"{label} could not be stopped even after kill()") raise + finally: + # Ensure pipes are closed and process is reaped to avoid zombie processes + if proc.stdout: + proc.stdout.close() + if proc.stderr: + proc.stderr.close() + try: + proc.wait(timeout=0.1) + except subprocess.TimeoutExpired: + pass # Process still running, already logged warning above def _apply_recreate_strategy( 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 2ef8861301..f19d48df3b 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -8,7 +8,7 @@ import type { OtherWorktreeInfo, } from '../../../shared/types'; import path from 'path'; -import { existsSync, mkdirSync, writeFileSync, readFileSync, readdirSync, rmSync, symlinkSync, lstatSync, copyFileSync, cpSync, statSync } from 'fs'; +import { existsSync, mkdirSync, writeFileSync, readFileSync, readdirSync, rmSync, symlinkSync, lstatSync, copyFileSync, cpSync, statSync, readlinkSync } from 'fs'; import { execFileSync, execFile } from 'child_process'; import { promisify } from 'util'; import { minimatch } from 'minimatch'; @@ -59,14 +59,16 @@ function isTimeoutError(error: unknown): boolean { /** * Check if a path is a symlink or Windows junction (including broken ones). - * Uses lstatSync to detect the link itself, not what it points to. + * Uses readlinkSync which works for both symlinks and junctions on all platforms. */ function isSymlinkOrJunction(targetPath: string): boolean { try { - const stats = lstatSync(targetPath); - return stats.isSymbolicLink(); + // readlinkSync throws if the path is not a symlink/junction + // It works for both symlinks and junctions on Windows and Unix + readlinkSync(targetPath); + return true; } catch { - return false; // Path doesn't exist at all + return false; // Path doesn't exist or is not a symlink/junction } } From 95f32f914e56daf5e2b48911fe7e53a10b07ebe8 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Tue, 17 Feb 2026 13:35:53 +0100 Subject: [PATCH 6/7] fix: address PR #1832 review findings for worktree venv improvements - Initialize performed=False as safer default matching TypeScript impl - Fix _popen_with_cleanup docstring to accurately describe timeout behavior - Replace lstatSync with isSymlinkOrJunction for consistency - Add debug log when venv fallback to recreate occurs - Use existing venvPath variable instead of reconstructing path - Remove broken symlinks before returning false to allow fresh creation Co-Authored-By: Claude Opus 4.6 --- apps/backend/core/workspace/setup.py | 4 ++-- .../terminal/worktree-handlers.ts | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/apps/backend/core/workspace/setup.py b/apps/backend/core/workspace/setup.py index 511451ab3d..cb05322db9 100644 --- a/apps/backend/core/workspace/setup.py +++ b/apps/backend/core/workspace/setup.py @@ -628,7 +628,7 @@ def setup_worktree_dependencies( results[strategy_name] = [] try: - performed = True + performed = False if config.strategy == DependencyStrategy.SYMLINK: performed = _apply_symlink_strategy(project_dir, worktree_path, config) # For venvs, verify the symlink is usable — fall back to recreate @@ -765,7 +765,7 @@ def _popen_with_cleanup( are released before any cleanup (e.g. shutil.rmtree). Returns (returncode, stdout, stderr). - Raises subprocess.TimeoutExpired if the process could not be stopped. + Raises subprocess.TimeoutExpired if the command exceeds the given timeout (after cleanup is attempted). """ proc = subprocess.Popen( cmd, 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 f19d48df3b..12af781734 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -396,8 +396,9 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri debugLog('[TerminalWorktree] Symlinked venv health check passed:', config.sourceRelPath); } catch { debugLog('[TerminalWorktree] Symlinked venv health check failed, falling back to recreate:', config.sourceRelPath); + debugLog('[TerminalWorktree] Venv fallback: removing broken symlink and recreating for', config.sourceRelPath); // Remove the broken symlink and recreate - try { rmSync(path.join(worktreePath, config.sourceRelPath), { recursive: true, force: true }); } catch { /* best-effort */ } + try { rmSync(venvPath, { recursive: true, force: true }); } catch { /* best-effort */ } performed = await applyRecreateStrategy(projectPath, worktreePath, config); } } @@ -441,13 +442,15 @@ function applySymlinkStrategy(projectPath: string, worktreePath: string, config: return false; } - // Check for broken symlinks - try { - lstatSync(targetPath); - debugLog('[TerminalWorktree] Skipping symlink', config.sourceRelPath, '- target exists (possibly broken symlink)'); - return false; - } catch { - // Target doesn't exist at all — good, we can create symlink + // Check for broken symlinks and remove them so a fresh symlink can be created + if (isSymlinkOrJunction(targetPath)) { + if (!existsSync(targetPath)) { + debugLog('[TerminalWorktree] Removing broken symlink for', config.sourceRelPath); + try { rmSync(targetPath, { force: true }); } catch { /* best-effort */ } + } else { + debugLog('[TerminalWorktree] Skipping symlink', config.sourceRelPath, '- target exists (symlink)'); + return false; + } } const targetDir = path.dirname(targetPath); From 4cb0a14aa7aa01e0fbf018f3a9e7b441f1d300d4 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Tue, 17 Feb 2026 15:33:51 +0100 Subject: [PATCH 7/7] fix: add debug log for venv fallback success in worktree setup Add missing debug log when venv symlink health check fails and the recreate fallback succeeds, matching the Python backend's behavior for consistent debug output. Co-Authored-By: Claude Opus 4.6 --- .../src/main/ipc-handlers/terminal/worktree-handlers.ts | 3 +++ 1 file changed, 3 insertions(+) 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 12af781734..225a48f264 100644 --- a/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts @@ -400,6 +400,9 @@ async function setupWorktreeDependencies(projectPath: string, worktreePath: stri // Remove the broken symlink and recreate try { rmSync(venvPath, { recursive: true, force: true }); } catch { /* best-effort */ } performed = await applyRecreateStrategy(projectPath, worktreePath, config); + if (performed) { + debugLog('[TerminalWorktree] Venv fallback to recreate succeeded:', config.sourceRelPath); + } } } }