Merge pull request 'fix(lsp): release lock before blocking I/O in LspLifecycleManager.restart_server() to prevent deadlock' (#3165) from fix/lsp-lifecycle-restart-lock-deadlock into master

features/lsp_lifecycle_coverage.feature

@@ -182,3 +182,30 @@ Feature: LSP Lifecycle Manager coverage
   Scenario: List running returns empty list when no servers
     When llcov I list running servers
     Then llcov the running list should contain 0 entries
+
+  # ---------- restart_server: 3-phase lock — health_check not blocked ----------
+
+  Scenario: health_check is not blocked while restart_server is in progress
+    Given llcov a server config named "local/pyright" with command "pyright-langserver"
+    And llcov the server is already started for workspace "/tmp/ws"
+    When llcov I restart the server concurrently and health_check runs during blocking IO
+    Then llcov health_check should have completed without blocking
+    And llcov the restarted client should not be None
+
+  # ---------- restart_server: 3-phase lock — lock not held during initialize ----------
+
+  Scenario: restart_server does not hold the lock during client.initialize
+    Given llcov a server config named "local/pyright" with command "pyright-langserver"
+    And llcov the server is already started for workspace "/tmp/ws"
+    When llcov I restart the server and verify lock is free during initialize
+    Then llcov the lock should have been acquirable during initialize
+    And llcov the restarted client should not be None
+
+  # ---------- restart_server: ref_count preserved after restart ----------
+
+  Scenario: restart_server preserves the ref_count of the managed server
+    Given llcov a server config named "local/pyright" with command "pyright-langserver"
+    And llcov the server is already started for workspace "/tmp/ws"
+    And llcov the server is started again to bump ref count
+    When llcov I restart the server "local/pyright"
+    Then llcov the ref count for "local/pyright" should be 2

features/steps/lsp_lifecycle_coverage_steps.py

@@ -4,12 +4,16 @@ Exercises every uncovered code path in cleveragents.lsp.lifecycle including
 _ManagedServer construction, start/stop/restart lifecycle, race conditions,
 health checks, error handling, stop_all, and list_running.
 
+Also covers the 3-phase lock pattern in restart_server() to verify that
+health_check() and other operations are not blocked during blocking I/O.
+
 All external I/O (StdioTransport, LspClient) is mocked so no real
 subprocesses are spawned.
 """
 
 from __future__ import annotations
 
+import threading
 from unittest.mock import MagicMock, patch
 
 from behave import given, then, when
@@ -633,3 +637,136 @@ def step_running_entry_ref_count(context: Context, name: str, count: int) -> Non
     assert entry["ref_count"] == count, (
         f"Expected ref_count={count}, got {entry['ref_count']}"
     )
+
+
+# ---------------------------------------------------------------------------
+# Concurrent-access tests for 3-phase lock pattern in restart_server()
+# ---------------------------------------------------------------------------
+
+
+@when(
+    "llcov I restart the server concurrently and health_check runs during blocking IO"
+)
+def step_restart_concurrent_health_check(context: Context) -> None:
+    """Verify health_check() is not blocked while restart_server() is in progress.
+
+    We inject a barrier into the mock ``initialize`` call so that the
+    restart_server() thread pauses in Phase 2 (blocking I/O, no lock held).
+    A second thread then calls health_check() and records whether it
+    completed without blocking.
+    """
+    # Barrier: both threads must reach it before either proceeds.
+    # Thread 1 (restart): reaches barrier inside initialize()
+    # Thread 2 (health_check): reaches barrier after acquiring the lock
+    barrier = threading.Barrier(2, timeout=5)
+    health_check_completed = threading.Event()
+    health_check_result: list[bool] = []
+
+    new_transport = _make_mock_transport()
+    new_client = _make_mock_client()
+
+    def _blocking_initialize(workspace_path: str) -> None:
+        """Simulate a slow LSP handshake; signal the barrier mid-way."""
+        barrier.wait()  # Signal: "I'm in Phase 2, lock is released"
+
+    new_client.initialize.side_effect = _blocking_initialize
+
+    def _restart_thread() -> None:
+        p_transport = patch(
+            "cleveragents.lsp.lifecycle.StdioTransport",
+            return_value=new_transport,
+        )
+        p_client = patch(
+            "cleveragents.lsp.lifecycle.LspClient",
+            return_value=new_client,
+        )
+        with p_transport, p_client:
+            context.llcov_restarted_client = context.llcov_manager.restart_server(
+                context.llcov_config.name
+            )
+
+    def _health_check_thread() -> None:
+        barrier.wait()  # Wait until restart is in Phase 2 (lock released)
+        # health_check() must acquire _lock; if restart held it, this would block
+        result = context.llcov_manager.health_check(context.llcov_config.name)
+        health_check_result.append(result)
+        health_check_completed.set()
+
+    t_restart = threading.Thread(target=_restart_thread, daemon=True)
+    t_health = threading.Thread(target=_health_check_thread, daemon=True)
+
+    t_restart.start()
+    t_health.start()
+
+    t_restart.join(timeout=10)
+    t_health.join(timeout=10)
+
+    context.llcov_health_check_completed = health_check_completed.is_set()
+    context.llcov_health_check_result_concurrent = health_check_result
+    context.llcov_new_transport = new_transport
+    context.llcov_new_client = new_client
+
+
+@then("llcov health_check should have completed without blocking")
+def step_health_check_completed(context: Context) -> None:
+    assert context.llcov_health_check_completed, (
+        "health_check() did not complete — it was blocked while restart_server() "
+        "held the lock during blocking I/O"
+    )
+
+
+@then("llcov the restarted client should not be None")
+def step_restarted_client_not_none(context: Context) -> None:
+    assert context.llcov_restarted_client is not None, (
+        "Expected a restarted client but got None"
+    )
+
+
+@when("llcov I restart the server and verify lock is free during initialize")
+def step_restart_verify_lock_free(context: Context) -> None:
+    """Assert that _lock is NOT held when client.initialize() is called.
+
+    We inspect the lock state from within the mock initialize() call.
+    If the lock is held, ``_lock.acquire(blocking=False)`` returns False.
+    """
+    lock_was_free_during_init: list[bool] = []
+    manager = context.llcov_manager
+
+    new_transport = _make_mock_transport()
+    new_client = _make_mock_client()
+
+    def _check_lock_initialize(workspace_path: str) -> None:
+        # Try to acquire the lock non-blocking; succeeds only if not held
+        acquired = manager._lock.acquire(blocking=False)
+        lock_was_free_during_init.append(acquired)
+        if acquired:
+            manager._lock.release()
+
+    new_client.initialize.side_effect = _check_lock_initialize
+
+    p_transport = patch(
+        "cleveragents.lsp.lifecycle.StdioTransport",
+        return_value=new_transport,
+    )
+    p_client = patch(
+        "cleveragents.lsp.lifecycle.LspClient",
+        return_value=new_client,
+    )
+    with p_transport, p_client:
+        context.llcov_restarted_client = context.llcov_manager.restart_server(
+            context.llcov_config.name
+        )
+
+    context.llcov_lock_was_free_during_init = lock_was_free_during_init
+    context.llcov_new_transport = new_transport
+    context.llcov_new_client = new_client
+
+
+@then("llcov the lock should have been acquirable during initialize")
+def step_lock_free_during_init(context: Context) -> None:
+    results = context.llcov_lock_was_free_during_init
+    assert results, "initialize() was never called — cannot verify lock state"
+    assert all(results), (
+        "The lock was held during client.initialize() — restart_server() "
+        "must not hold _lock during blocking I/O"
+    )

src/cleveragents/lsp/lifecycle.py

@@ -222,6 +222,16 @@ class LspLifecycleManager:
         Stops the current process (if alive), starts a fresh one, and
         re-initializes the LSP handshake.
 
+        Uses the same 3-phase lock pattern as :meth:`start_server` to avoid
+        holding ``self._lock`` during blocking I/O:
+
+        - **Phase 1** (short lock): Read current state and remove the old
+          entry so other threads see the server as gone during restart.
+        - **Phase 2** (no lock): Stop the old transport, spawn the new
+          process, and perform the LSP handshake.  These operations can
+          block for up to 60 seconds and must not hold the lock.
+        - **Phase 3** (short lock): Commit the new client into shared state.
+
         Args:
             name: Namespaced server name.
 
@@ -232,6 +242,7 @@ class LspLifecycleManager:
             LspServerNotFoundError: If the server was never started.
             LspError: If the restart fails.
         """
+        # Phase 1: Read current state under lock (short critical section)
         with self._lock:
             managed = self._servers.get(name)
             if managed is None:
@@ -239,30 +250,48 @@ class LspLifecycleManager:
 
             logger.warning("lsp.lifecycle.restarting", server=name)
 
-            # Stop old transport
-            managed.transport.stop()
+            # Snapshot the fields we need for Phase 2 so we can release the lock
+            old_transport = managed.transport
+            config = managed.config
+            workspace_path = managed.workspace_path
+            ref_count = managed.ref_count
 
-            # Start fresh
-            transport = StdioTransport(
-                command=managed.config.command,
-                args=managed.config.args,
-                env=managed.config.env,
-                cwd=managed.workspace_path,
-            )
-            transport.start()
+            # Remove the entry so concurrent callers see the server as absent
+            # during the blocking restart.  Phase 3 will re-insert it.
+            del self._servers[name]
 
-            client = LspClient(transport, server_name=name)
-            try:
-                client.initialize(managed.workspace_path)
-            except Exception:
-                transport.stop()
-                raise
+        # Phase 2: Blocking I/O — no lock held
+        old_transport.stop()
 
-            managed.transport = transport
-            managed.client = client
+        transport = StdioTransport(
+            command=config.command,
+            args=config.args,
+            env=config.env,
+            cwd=workspace_path,
+        )
+        transport.start()
 
-            logger.info("lsp.lifecycle.restarted", server=name)
-            return client
+        client = LspClient(transport, server_name=name)
+        try:
+            client.initialize(workspace_path)
+        except Exception:
+            transport.stop()
+            raise
+
+        # Phase 3: Commit new state under lock (short critical section)
+        new_managed = _ManagedServer(
+            config=config,
+            transport=transport,
+            client=client,
+            workspace_path=workspace_path,
+        )
+        new_managed.ref_count = ref_count
+
+        with self._lock:
+            self._servers[name] = new_managed
+
+        logger.info("lsp.lifecycle.restarted", server=name)
+        return client
 
     def stop_all(self) -> None:
         """Shut down all running servers.  Used during cleanup."""