diff --git a/features/lsp_lifecycle_coverage.feature b/features/lsp_lifecycle_coverage.feature index 74c34020f..2d860cbbf 100644 --- a/features/lsp_lifecycle_coverage.feature +++ b/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 diff --git a/features/steps/lsp_lifecycle_coverage_steps.py b/features/steps/lsp_lifecycle_coverage_steps.py index 5e6d00dd1..65f4bdefc 100644 --- a/features/steps/lsp_lifecycle_coverage_steps.py +++ b/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" + ) diff --git a/src/cleveragents/lsp/lifecycle.py b/src/cleveragents/lsp/lifecycle.py index 46c7c73e3..cacf315e7 100644 --- a/src/cleveragents/lsp/lifecycle.py +++ b/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."""