diff --git a/CHANGELOG.md b/CHANGELOG.md index b8d8162ae..93dd9bc8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,17 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **LSP — `LspLifecycleManager.restart_server()` deadlock fix**: Refactored + `restart_server()` to use the same 3-phase lock pattern as `start_server()`, + releasing `self._lock` before all blocking I/O (Phase 1: lock+snapshot+remove + old entry; Phase 2: `stop()`/`start()`/`initialize()` without lock; Phase 3: + lock+insert new entry). Previously the lock was held across all blocking I/O + (up to 120 s), causing `health_check()`, `list_running()`, and `stop_server()` + callers to block for the full restart duration. The `ref_count` from the + original managed server is preserved across the restart. Three BDD scenarios + verify the fix using `threading.Barrier` synchronisation and + `acquire(blocking=False)` lock-state inspection. (#3165) + - Fixed session leak in all `AutomationProfileRepository` public methods (`get_by_name()`, `list_all()`, `upsert()`, and `delete()`): added `finally: if self._auto_commit: session.close()` blocks matching the diff --git a/docs/reference/lsp_lifecycle_restart.md b/docs/reference/lsp_lifecycle_restart.md new file mode 100644 index 000000000..ed61f02e9 --- /dev/null +++ b/docs/reference/lsp_lifecycle_restart.md @@ -0,0 +1,73 @@ +# LSP Lifecycle Manager — `restart_server()` Lock Pattern + +## Overview + +`LspLifecycleManager.restart_server()` uses a **3-phase lock pattern** to +prevent deadlocks under concurrent access. This matches the pattern already +used by `start_server()`. + +## The Problem (pre-fix) + +Prior to PR #3165, `restart_server()` held `self._lock` across **all** blocking +I/O operations: + +``` +acquire lock + transport.stop() ← blocking I/O (up to 60s) + transport.start() ← blocking I/O + client.initialize() ← blocking I/O (up to 60s) +release lock +``` + +Any concurrent caller of `health_check()`, `list_running()`, or `stop_server()` +would block for up to 60 seconds waiting for the lock. + +## The Fix — 3-Phase Lock Pattern + +``` +Phase 1 (lock held — short): + Read current state, snapshot fields, remove old entry from _servers + +Phase 2 (lock RELEASED — blocking I/O): + old_transport.stop() + StdioTransport(...).start() + client.initialize() + +Phase 3 (lock held — short): + Insert new _ManagedServer into _servers, preserving ref_count +``` + +Removing the old entry in Phase 1 ensures concurrent callers see the server +as absent during the restart window (consistent with the `start_server` +contract). The `ref_count` from the original managed server is carried +forward to the new one. + +## Concurrency Guarantees + +| Concurrent operation | Behaviour during restart | +|---------------------|--------------------------| +| `health_check()` | Returns immediately (server absent in Phase 2) | +| `list_running()` | Returns immediately (server absent in Phase 2) | +| `stop_server()` | Returns immediately (server absent in Phase 2) | +| `start_server()` | Proceeds normally (no lock contention in Phase 2) | + +## BDD Coverage + +Three scenarios in `features/lsp_lifecycle_coverage.feature` verify the fix: + +1. **`health_check is not blocked while restart_server is in progress`** — + uses a `threading.Barrier` to synchronise two threads at the exact moment + the lock is released in Phase 2. + +2. **`restart_server does not hold the lock during client.initialize`** — + inspects `_lock` state from within the mock `initialize()` side-effect + using `acquire(blocking=False)`. + +3. **`restart_server preserves the ref_count of the managed server`** — + starts the server twice (ref_count=2), restarts it, asserts ref_count=2. + +## References + +- PR #3165 — fix implementation +- Issue #3026 — original bug report +- `src/cleveragents/lsp/lifecycle.py` — implementation