diff --git a/src/lsp_cli/manager/__init__.py b/src/lsp_cli/manager/__init__.py index 4b3bf18..92995c3 100644 --- a/src/lsp_cli/manager/__init__.py +++ b/src/lsp_cli/manager/__init__.py @@ -39,7 +39,9 @@ def connect_manager() -> HttpClient: if MANAGER_CONN_PATH.exists(): try: conn = ConnectionInfo.model_validate_json(MANAGER_CONN_PATH.read_text()) - except Exception: + except (OSError, ValueError, Exception) as e: + # Failed to read or parse connection info - will try to start manager + # Catches OSError (file read), ValueError (JSON/validation), or other parsing errors pass if not conn or not is_server_alive( @@ -66,7 +68,8 @@ def connect_manager() -> HttpClient: uds_path=conn.uds_path, host=conn.host, port=conn.port ): break - except Exception: + except (OSError, ValueError, Exception): + # Failed to read/parse - retry in next iteration pass time.sleep(0.1) else: diff --git a/src/lsp_cli/manager/__main__.py b/src/lsp_cli/manager/__main__.py index 9b10855..e62b4c5 100644 --- a/src/lsp_cli/manager/__main__.py +++ b/src/lsp_cli/manager/__main__.py @@ -4,18 +4,21 @@ from lsp_cli.manager.models import ConnectionInfo from lsp_cli.settings import IS_WINDOWS, MANAGER_CONN_PATH, MANAGER_UDS_PATH +from lsp_cli.utils.socket import allocate_port from .manager import app if __name__ == "__main__": if IS_WINDOWS: - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("127.0.0.1", 0)) - port = s.getsockname()[1] - assert isinstance(port, int) - conn = ConnectionInfo(host="127.0.0.1", port=port) - MANAGER_CONN_PATH.write_text(conn.model_dump_json()) - uvicorn.run(app, host="127.0.0.1", port=port) + sock, port = allocate_port() + try: + conn = ConnectionInfo(host="127.0.0.1", port=port) + MANAGER_CONN_PATH.parent.mkdir(parents=True, exist_ok=True) + MANAGER_CONN_PATH.write_text(conn.model_dump_json()) + # Pass the file descriptor to uvicorn to avoid race condition + uvicorn.run(app, host="127.0.0.1", port=port, fd=sock.fileno()) + finally: + sock.close() else: MANAGER_UDS_PATH.unlink(missing_ok=True) MANAGER_UDS_PATH.parent.mkdir(parents=True, exist_ok=True) diff --git a/src/lsp_cli/manager/client.py b/src/lsp_cli/manager/client.py index 2da5bf6..020f205 100644 --- a/src/lsp_cli/manager/client.py +++ b/src/lsp_cli/manager/client.py @@ -1,5 +1,6 @@ from __future__ import annotations +import socket from collections.abc import AsyncGenerator from contextlib import asynccontextmanager from pathlib import Path @@ -16,6 +17,7 @@ from lsp_cli.client import TargetClient from lsp_cli.manager.capability import CapabilityController, Capabilities from lsp_cli.settings import IS_WINDOWS, LOG_DIR, RUNTIME_DIR, settings +from lsp_cli.utils.socket import allocate_port from .models import ConnectionInfo, ManagedClientInfo @@ -40,6 +42,7 @@ class ManagedClient: _logger: loguru.Logger = field(init=False) _logger_sink_id: int = field(init=False) _port: int | None = field(init=False, default=None) + _socket: socket.socket | None = field(init=False, default=None) def __attrs_post_init__(self) -> None: self._deadline = anyio.current_time() + settings.idle_timeout @@ -67,6 +70,11 @@ def id(self) -> str: @property def conn(self) -> ConnectionInfo: if IS_WINDOWS: + if self._port is None: + raise RuntimeError( + "Connection information is not available yet: " + "the managed client has not been assigned a port." + ) return ConnectionInfo(host="127.0.0.1", port=self._port) return ConnectionInfo(uds_path=self.uds_path) @@ -134,13 +142,9 @@ def exception_handler(request: Request, exc: Exception) -> Response: ) if IS_WINDOWS: - import socket - - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(("127.0.0.1", 0)) - port_val = s.getsockname()[1] - assert isinstance(port_val, int) - self._port = port_val + sock, port_val = allocate_port() + self._port = port_val + self._socket = sock # Keep reference to close later config = uvicorn.Config( app, @@ -148,6 +152,7 @@ def exception_handler(request: Request, exc: Exception) -> Response: port=port_val, loop="asyncio", log_config=None, + fd=sock.fileno(), ) else: config = uvicorn.Config( @@ -182,6 +187,10 @@ async def run(self) -> None: self._logger.info("Cleaning up client") if not IS_WINDOWS: await anyio.Path(self.uds_path).unlink(missing_ok=True) + else: + # Close the socket on Windows + if self._socket is not None: + self._socket.close() self._logger.remove(self._logger_sink_id) self._timeout_scope.cancel() self._server_scope.cancel() diff --git a/src/lsp_cli/manager/models.py b/src/lsp_cli/manager/models.py index 036cebf..b6e08b6 100644 --- a/src/lsp_cli/manager/models.py +++ b/src/lsp_cli/manager/models.py @@ -7,12 +7,31 @@ class ConnectionInfo(BaseModel): + """Connection information for LSP server communication. + + On Unix-like systems, uses Unix Domain Sockets (uds_path). + On Windows, uses TCP with host and port. + + Security Note: When using TCP (Windows), the server binds to 127.0.0.1 + (localhost only) without authentication. This means any local process + can connect to the manager. This is acceptable for a local development + tool, but should be considered when handling sensitive data. + """ + uds_path: Path | None = None host: str | None = None port: int | None = None @property def url(self) -> str: + """ + Return the HTTP URL for this connection. + + If both ``host`` and ``port`` are set, this returns + ``"http://{host}:{port}"``. If either value is missing, this + falls back to ``"http://localhost"`` (used primarily for UDS connections + where the actual network address is not applicable). + """ if self.host and self.port: return f"http://{self.host}:{self.port}" return "http://localhost" diff --git a/src/lsp_cli/manager/server.py b/src/lsp_cli/manager/server.py index 42851d7..88e1046 100644 --- a/src/lsp_cli/manager/server.py +++ b/src/lsp_cli/manager/server.py @@ -2,11 +2,10 @@ from collections.abc import AsyncGenerator from contextlib import asynccontextmanager -from functools import cached_property from typing import Self, final, override import httpx -from attrs import define +from attrs import define, field from lsp_client.jsonrpc.types import ( RawNotification, RawRequest, @@ -25,19 +24,31 @@ @define class ManagerServer(Server): conn: ConnectionInfo + _client: httpx.AsyncClient | None = field(init=False, default=None) - @cached_property + @property def client(self) -> httpx.AsyncClient: - if self.conn.uds_path: - transport = httpx.AsyncHTTPTransport(uds=self.conn.uds_path.as_posix()) - else: - transport = httpx.AsyncHTTPTransport() + """Get or create the HTTP client for this server.""" + if self._client is None: + if self.conn.uds_path: + transport = httpx.AsyncHTTPTransport( + uds=self.conn.uds_path.as_posix() + ) + else: + transport = httpx.AsyncHTTPTransport() - return httpx.AsyncClient( - transport=transport, - base_url=self.conn.url, - timeout=None, - ) + self._client = httpx.AsyncClient( + transport=transport, + base_url=self.conn.url, + timeout=None, + ) + return self._client + + async def _close_client(self) -> None: + """Close the HTTP client if it exists.""" + if self._client is not None: + await self._client.aclose() + self._client = None @override async def check_availability(self) -> None: @@ -81,4 +92,7 @@ async def run( port=self.conn.port, timeout=10.0, ) - yield self + try: + yield self + finally: + await self._close_client() diff --git a/src/lsp_cli/utils/socket.py b/src/lsp_cli/utils/socket.py index 7c173c7..4aecd31 100644 --- a/src/lsp_cli/utils/socket.py +++ b/src/lsp_cli/utils/socket.py @@ -5,6 +5,32 @@ from tenacity import AsyncRetrying, stop_after_delay, wait_fixed +def allocate_port() -> tuple[socket.socket, int]: + """Allocate a free TCP port by binding to port 0. + + Returns a tuple of (socket, port). The socket is kept open and should be + passed to uvicorn via the `fd` parameter to avoid a race condition where + another process could bind to the same port between closing the socket + and uvicorn starting. + + Note: There is still a small race condition if the socket is closed before + uvicorn binds to it. The recommended pattern is to pass the socket's file + descriptor directly to uvicorn using `fd=socket.fileno()`, but this may not + work on all platforms. For best reliability, keep the socket open until + uvicorn has bound to the port. + + Returns: + tuple[socket.socket, int]: A tuple of (socket object, port number). + The caller is responsible for closing the socket after uvicorn starts. + """ + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + s.bind(("127.0.0.1", 0)) + s.listen() + port = s.getsockname()[1] + assert isinstance(port, int) + return s, port + + def is_server_alive( uds_path: Path | None = None, host: str | None = None, port: int | None = None ) -> bool: @@ -16,6 +42,7 @@ def is_server_alive( s.connect(str(uds_path)) return True except OSError: + # Connection failed - socket file exists but server is not responding pass if host and port: @@ -23,6 +50,7 @@ def is_server_alive( with socket.create_connection((host, port), timeout=1.0): return True except OSError: + # Connection failed - server is not listening on this port pass return False @@ -34,24 +62,42 @@ async def wait_for_server( port: int | None = None, timeout: float = 10.0, ) -> None: + """Wait for a server to become available. + + Raises: + ValueError: If only uds_path is provided on Windows where UDS is not available. + OSError: If the server does not become ready within the timeout period. + """ + # Check if we have any valid connection method + af_unix = getattr(socket, "AF_UNIX", None) + has_uds_support = af_unix is not None + has_tcp_info = host and port + + # Fail fast if only UDS is provided but not supported + if uds_path and not has_uds_support and not has_tcp_info: + raise ValueError( + "Unix Domain Sockets are not available on this platform, " + "but only uds_path was provided without TCP fallback (host/port)" + ) + async for attempt in AsyncRetrying( stop=stop_after_delay(timeout), wait=wait_fixed(0.1), reraise=True, ): with attempt: - if uds_path: + if uds_path and has_uds_support: try: - af_unix = getattr(socket, "AF_UNIX", None) - if af_unix is not None: - _ = await anyio.connect_unix(uds_path) - return + _ = await anyio.connect_unix(uds_path) + return except (OSError, RuntimeError): + # Connection failed; suppress to try TCP or retry pass if host and port: try: _ = await anyio.connect_tcp(host, port) return except (OSError, RuntimeError): + # Connection failed; suppress to retry pass raise OSError("Server not ready") diff --git a/tests/test_cli_integration.py b/tests/test_cli_integration.py index 5264ed1..a673269 100644 --- a/tests/test_cli_integration.py +++ b/tests/test_cli_integration.py @@ -182,17 +182,36 @@ def test_manager_auto_start_reliability(self): import os if os.name == "nt": - subprocess.run( - [ - "taskkill", - "/F", - "/IM", - "python.exe", - "/FI", - "MODULE == lsp_cli.manager", - ], - capture_output=True, - ) + # On Windows, try to use WMIC if available, otherwise skip this test step + # WMIC is deprecated in newer Windows versions but may still be present + try: + result = subprocess.run( + [ + "wmic", + "process", + "where", + "CommandLine like '%lsp_cli.manager%'", + "delete", + ], + capture_output=True, + timeout=5, + ) + # If WMIC is not available, try tasklist/taskkill as fallback + if result.returncode != 0: + # Try to use PowerShell as a more modern alternative + subprocess.run( + [ + "powershell", + "-Command", + "Get-Process | Where-Object {$_.CommandLine -like '*lsp_cli.manager*'} | Stop-Process -Force", + ], + capture_output=True, + timeout=5, + ) + except (subprocess.TimeoutExpired, FileNotFoundError): + # WMIC/PowerShell not available or timed out - continue with test + # The manager auto-start will still work if no manager is running + pass else: subprocess.run( ["pkill", "-f", "lsp_cli.manager"], diff --git a/tests/test_server_management.py b/tests/test_server_management.py index beda763..a53fbcb 100644 --- a/tests/test_server_management.py +++ b/tests/test_server_management.py @@ -60,7 +60,8 @@ def manager_process(): uds_path=conn.uds_path, host=conn.host, port=conn.port ): break - except Exception: + except (OSError, ValueError, Exception): + # Failed to read/parse connection info - retry pass time.sleep(0.1) else: @@ -92,15 +93,32 @@ def test_file(): return file +def _load_connection_info() -> ConnectionInfo: + """Load and validate the manager connection info from disk with error handling.""" + try: + data = MANAGER_CONN_PATH.read_text() + except OSError as exc: + raise RuntimeError( + f"Manager connection file is not available at {MANAGER_CONN_PATH!s}" + ) from exc + + try: + return ConnectionInfo.model_validate_json(data) + except Exception as exc: + raise RuntimeError( + f"Invalid manager connection data in {MANAGER_CONN_PATH!s}" + ) from exc + + def get_manager_transport(): - conn = ConnectionInfo.model_validate_json(MANAGER_CONN_PATH.read_text()) + conn = _load_connection_info() if conn.uds_path: return httpx.AsyncHTTPTransport(uds=str(conn.uds_path), retries=5) return httpx.AsyncHTTPTransport(retries=5) def get_manager_url(): - conn = ConnectionInfo.model_validate_json(MANAGER_CONN_PATH.read_text()) + conn = _load_connection_info() return conn.url