From 8773797f5ba84cf9ce072258aa08789b6a7b2488 Mon Sep 17 00:00:00 2001 From: PAI Date: Mon, 3 Aug 2026 21:54:08 -0700 Subject: [PATCH] fix(lsp): bound the language-server fleet by idle timeout and LRU cap (SCA-4389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `LSPService` accepted an `idle_timeout`, stored it on the instance, and never read it again. `DEFAULT_IDLE_TIMEOUT`'s comment claimed "servers idle for >10min get reaped". Nothing reaped anything, and `_last_used` was written but only ever consulted to be cleared. The steady state of normal multi-worktree work was therefore an unbounded memory ratchet: one server per project root, cached for the life of the gateway, ~1.3 GiB each against a scaffolde-ai checkout. A gateway up 3 days held 13 live `typescript-language-server` trees and ~16.3 GiB on a 16 GiB host, which pushed the box into swap, which pushed free disk under the CI runner's admission floor and took self-hosted CI offline. Two independent bounds, both applied on the request path: - **Idle timeout** evicts a root nobody has asked about for `idle_timeout` seconds. Re-spawning costs seconds; holding costs 1.3 GiB indefinitely. - **LRU cap** evicts the least-recently-used root when the population would exceed `max_servers`, so N simultaneously-active worktrees cannot exceed host memory regardless of traffic. Both are operator-configurable via `lsp.idle_timeout` / `lsp.max_servers`. The cap's default is derived from host RAM rather than hardcoded — a 16 GiB Mac Mini derives 3, a 128 GiB workstation derives 26. Host memory that cannot be discovered yields a conservative 4 rather than a guess upward, since guessing high is what produced this defect. A server draining an in-flight request is never evicted: `_acquire`/ `_release` bracket the two request paths, and `_enforce_bounds` re-checks the in-flight count under the lock before shutting anything down. Eviction runs *before* spawning, so the fleet never transiently exceeds the cap. Reuse now refreshes the LRU stamp — previously a root served entirely from cache looked progressively more idle. Every eviction emits an INFO line naming the root and the reason, and clears the root's `log_active` announce entry so a later re-spawn announces honestly instead of logging "reused client" for a brand-new process. `hermes lsp status` gains `idle_timeout`, `max_servers`, and per-client `idle_seconds`/`inflight`. Tests (17, all green; full LSP suite 182 green): The bar is deliberately not "the policy function returns the right list" — a cache with an eviction path that never executes is exactly the false green this defect was. The two headline tests drive real spawned clients through the real request path and assert the process is gone, each with a positive control running the identical scenario with the bound disabled. Verified by neutering `_enforce_bounds` to a no-op with the API otherwise intact — reproducing the original defect shape — under which precisely the 4 behaviour-proving tests go red. The module docstring now states the eviction contract explicitly, with a note to delete it alongside any future removal of the eviction call: a comment describing a reaper that does not run is worse than none. Refs: SCA-4389 --- agent/lsp/eventlog.py | 22 ++ agent/lsp/manager.py | 254 +++++++++++++++++- tests/agent/lsp/test_eviction.py | 427 +++++++++++++++++++++++++++++++ 3 files changed, 698 insertions(+), 5 deletions(-) create mode 100644 tests/agent/lsp/test_eviction.py diff --git a/agent/lsp/eventlog.py b/agent/lsp/eventlog.py index b38627504b4a..cd966b0efbca 100644 --- a/agent/lsp/eventlog.py +++ b/agent/lsp/eventlog.py @@ -188,6 +188,27 @@ def log_spawn_failed(server_id: str, workspace_root: str, exc: BaseException) -> ) +def log_evicted(server_id: str, workspace_root: str, reason: str) -> None: + """A cached client was shut down to bound the server population. INFO. + + Deliberately **not** deduped. Every other announce-once helper + suppresses repeats because the event is a steady-state fact; an + eviction is a discrete action taken against a named root, and the + whole point of logging it is that "the cache never evicts" must be + falsifiable by grepping the log rather than reconstructed from + ``ps``. Suppressing the second eviction of a root would hide + exactly the thrash we would need to see. + + Also clears the root's ``log_active`` announce entry, so the INFO + line fires again when the root is next re-spawned. Without that, + a re-spawn after eviction would only ever log at DEBUG ("reused + client"), which would be a lie — it is a new process. + """ + _emit(server_id, logging.INFO, f"evicted {workspace_root} ({reason})") + with _announce_lock: + _announced_active.discard((server_id, workspace_root)) + + def reset_announce_caches() -> None: """Test-only: clear the dedup caches. Production code never calls this.""" with _announce_lock: @@ -202,6 +223,7 @@ def reset_announce_caches() -> None: "log_clean", "log_disabled", "log_active", + "log_evicted", "log_diagnostics", "log_no_project_root", "log_server_unavailable", diff --git a/agent/lsp/manager.py b/agent/lsp/manager.py index d3b4244790b1..1fd79e9879fe 100644 --- a/agent/lsp/manager.py +++ b/agent/lsp/manager.py @@ -14,6 +14,26 @@ the first request for a key spawns the client; subsequent requests re-use it. +- The client population is **bounded** by two independent rules, + both applied on the request path (see :meth:`_enforce_bounds`): + an **idle timeout** evicts a root nobody has asked about for + ``idle_timeout`` seconds, and an **LRU cap** evicts the + least-recently-used root once the population would exceed + ``max_servers``. The cap's default is derived from host RAM + (:func:`default_max_servers`), because one language server against + a large TypeScript project costs ~1.3 GiB and a 16 GiB host cannot + afford the same fleet as a 128 GiB one. A server draining an + in-flight request is never evicted. Every eviction emits an INFO + line naming the root and the reason. + + This is deliberately spelled out because it was previously false: + ``idle_timeout`` was accepted, stored, and never read, while the + constant that carried it claimed servers "get reaped". Thirteen + live servers holding ~16 GiB on a 16 GiB host was the result + (SCA-4389). If a future change removes the eviction call, delete + this paragraph with it — a comment describing a reaper that does + not run is worse than no comment. + - A **broken-set** records ``(server_id, workspace_root)`` pairs that failed to spawn or initialize. These are never retried for the life of the service. Mirrors OpenCode's design. @@ -58,7 +78,72 @@ logger = logging.getLogger("agent.lsp.manager") -DEFAULT_IDLE_TIMEOUT = 600 # seconds; servers idle for >10min get reaped +DEFAULT_IDLE_TIMEOUT = 600 # seconds; servers idle for >10min get evicted + +# Measured footprint of one ``typescript-language-server`` plus its +# ``tsserver`` child against a scaffolde-ai checkout: 1.2-1.6 GiB +# resident. 1.3 GiB is the middle of that range and is the unit the +# default cap is denominated in. It is a sizing constant, not a +# limit — nothing here enforces per-process memory. +LSP_SERVER_FOOTPRINT_BYTES = 1_300_000_000 + +# Share of host RAM the LSP fleet may occupy before the cap bites. +# A quarter leaves the agent runtime, the language toolchains, and +# whatever the operator is actually running the other three quarters. +LSP_MEMORY_BUDGET_FRACTION = 0.25 + +# Floor and ceiling on the derived cap. The floor keeps a small host +# usable at all (one server is the difference between "LSP works" and +# "LSP is off"); the ceiling stops a very large host from deriving a +# number so high it is not a bound in any practical sense. +MIN_DERIVED_MAX_SERVERS = 1 +MAX_DERIVED_MAX_SERVERS = 32 + + +def _host_memory_bytes() -> Optional[int]: + """Total physical RAM in bytes, or ``None`` when undiscoverable. + + ``None`` is a real answer rather than a failure — the caller then + falls back to a conservative fixed cap instead of pretending to + have sized against a host it could not measure. + """ + try: + return int(os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES")) + except (ValueError, OSError, AttributeError): + pass + # macOS exposes SC_PHYS_PAGES inconsistently across Python builds; + # ``sysctl hw.memsize`` is the portable second source. + try: + import subprocess + + out = subprocess.run( + ["sysctl", "-n", "hw.memsize"], + capture_output=True, + text=True, + timeout=2.0, + check=False, + ) + if out.returncode == 0 and out.stdout.strip().isdigit(): + return int(out.stdout.strip()) + except Exception: # noqa: BLE001 + pass + return None + + +def default_max_servers() -> int: + """Derive the concurrent-server cap from host memory. + + A 16 GiB Mac Mini and a 128 GiB workstation must not get the same + number — that is the point of deriving rather than hardcoding. + 16 GiB yields 3; 128 GiB yields 26. + """ + total = _host_memory_bytes() + if not total: + # Undiscoverable host memory: assume small rather than large. + # Guessing high is what produced this defect in the first place. + return 4 + derived = int((total * LSP_MEMORY_BUDGET_FRACTION) // LSP_SERVER_FOOTPRINT_BYTES) + return max(MIN_DERIVED_MAX_SERVERS, min(derived, MAX_DERIVED_MAX_SERVERS)) class _BackgroundLoop: @@ -155,6 +240,7 @@ def __init__( init_overrides: Optional[Dict[str, Dict[str, Any]]] = None, disabled_servers: Optional[List[str]] = None, idle_timeout: float = DEFAULT_IDLE_TIMEOUT, + max_servers: Optional[int] = None, ) -> None: self._enabled = enabled self._wait_mode = wait_mode if wait_mode in {"document", "full"} else "document" @@ -164,7 +250,14 @@ def __init__( self._env_overrides = env_overrides or {} self._init_overrides = init_overrides or {} self._disabled_servers = set(disabled_servers or []) + # ``0``/negative disables the respective bound. Both are read + # on every spawn — see ``_enforce_bounds``. Before SCA-4389 + # ``_idle_timeout`` was stored here and never read again, so + # the module docstring's "reaped" claim was false. self._idle_timeout = idle_timeout + self._max_servers = ( + default_max_servers() if max_servers is None else int(max_servers) + ) self._loop = _BackgroundLoop() if self._enabled: @@ -175,6 +268,10 @@ def __init__( self._broken: set = set() self._spawning: Dict[Tuple[str, str], asyncio.Future] = {} self._last_used: Dict[Tuple[str, str], float] = {} + # Requests currently using a client, keyed the same way. A key + # with a non-zero count is never evicted, so eviction cannot + # tear a server down mid-request. + self._inflight: Dict[Tuple[str, str], int] = {} self._state_lock = threading.Lock() # Delta baseline: file path → snapshot of diagnostics taken @@ -202,6 +299,17 @@ def create_from_config(cls) -> Optional["LSPService"]: lsp_cfg = {} enabled = bool(lsp_cfg.get("enabled", True)) + # Both bounds are operator-overridable; absent config derives + # the cap from host memory rather than assuming this host. + try: + idle_timeout = float(lsp_cfg.get("idle_timeout", DEFAULT_IDLE_TIMEOUT)) + except (TypeError, ValueError): + idle_timeout = DEFAULT_IDLE_TIMEOUT + max_servers_cfg = lsp_cfg.get("max_servers") + try: + max_servers = None if max_servers_cfg is None else int(max_servers_cfg) + except (TypeError, ValueError): + max_servers = None wait_mode = lsp_cfg.get("wait_mode", "document") wait_timeout = float(lsp_cfg.get("wait_timeout", DIAGNOSTICS_DOCUMENT_WAIT)) install_strategy = lsp_cfg.get("install_strategy", "auto") @@ -235,6 +343,8 @@ def create_from_config(cls) -> Optional["LSPService"]: env_overrides=env_overrides, init_overrides=init_overrides, disabled_servers=disabled, + idle_timeout=idle_timeout, + max_servers=max_servers, ) # ------------------------------------------------------------------ @@ -434,6 +544,8 @@ def _mark_broken_for_file(self, file_path: str, exc: BaseException) -> None: # ``_clients`` with a half-initialized state. with self._state_lock: client = self._clients.pop(key, None) + self._last_used.pop(key, None) + self._inflight.pop(key, None) if client is not None: try: # Fire-and-forget shutdown — give it a second to cleanup, @@ -464,13 +576,16 @@ async def _snapshot_async(self, file_path: str) -> List[Dict[str, Any]]: client = await self._get_or_spawn(file_path) if client is None: return [] + key = (client.server_id, client.workspace_root) + self._acquire(key) try: version = await client.open_file(file_path, language_id=language_id_for(file_path)) fresh = await client.wait_for_diagnostics(file_path, version, mode=self._wait_mode) except Exception as e: # noqa: BLE001 logger.debug("snapshot open/wait failed: %s", e) return [] - self._last_used[(client.server_id, client.workspace_root)] = time.time() + finally: + self._release(key) if not fresh: # No fresh data for the pre-edit content — an empty baseline # is safe: worst case the delta filter removes less, never @@ -490,6 +605,8 @@ async def _open_and_wait_async(self, file_path: str) -> Optional[List[Dict[str, client = await self._get_or_spawn(file_path) if client is None: return None + key = (client.server_id, client.workspace_root) + self._acquire(key) try: version = await client.open_file(file_path, language_id=language_id_for(file_path)) await client.save_file(file_path) @@ -499,7 +616,8 @@ async def _open_and_wait_async(self, file_path: str) -> Optional[List[Dict[str, except Exception as e: # noqa: BLE001 logger.debug("open/wait failed for %s: %s", file_path, e) return None - self._last_used[(client.server_id, client.workspace_root)] = time.time() + finally: + self._release(key) if not fresh: return None return list(client.diagnostics_for(file_path, fresh_only=True)) @@ -539,15 +657,28 @@ async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]: with self._state_lock: client = self._clients.get(key) if client is not None and client.is_running: - eventlog.log_active(srv.server_id, per_server_root) - return client + # Refresh the LRU stamp on reuse. Before SCA-4389 only + # spawn and post-request stamped it, so a root served + # entirely from cache looked progressively more idle. + self._last_used[key] = time.time() + reuse = client + else: + reuse = None spawning = self._spawning.get(key) + if reuse is not None: + eventlog.log_active(srv.server_id, per_server_root) + await self._enforce_bounds(protect=key) + return reuse if spawning is not None: try: return await spawning except Exception: # noqa: BLE001 return None + # Make room BEFORE spawning, so the fleet never transiently + # exceeds the cap. ``protect=key`` reserves this root's slot. + await self._enforce_bounds(protect=key) + # Begin spawn loop = asyncio.get_running_loop() spawn_future: asyncio.Future = loop.create_future() @@ -597,12 +728,117 @@ async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]: with self._state_lock: self._spawning.pop(key, None) + # ------------------------------------------------------------------ + # eviction (SCA-4389) + # ------------------------------------------------------------------ + + def _acquire(self, key: Tuple[str, str]) -> None: + """Mark *key* as serving a request, so eviction skips it.""" + with self._state_lock: + self._inflight[key] = self._inflight.get(key, 0) + 1 + + def _release(self, key: Tuple[str, str]) -> None: + """Drop one in-flight reference and refresh the LRU stamp.""" + with self._state_lock: + remaining = self._inflight.get(key, 0) - 1 + if remaining > 0: + self._inflight[key] = remaining + else: + self._inflight.pop(key, None) + self._last_used[key] = time.time() + + def _eviction_candidates( + self, *, protect: Optional[Tuple[str, str]] = None, now: Optional[float] = None + ) -> List[Tuple[Tuple[str, str], str]]: + """Decide what to evict. Pure over the service's state. + + Returns ``(key, reason)`` pairs. Split out from the shutdown + so the policy is testable without spawning a real server. + + *protect* is the key the current request is about to use — it + is never a candidate even if it is the LRU, otherwise a cap of + 1 would evict the very client the caller just asked for. + """ + now = time.time() if now is None else now + with self._state_lock: + keys = list(self._clients.keys()) + last_used = dict(self._last_used) + inflight = dict(self._inflight) + + def evictable(k: Tuple[str, str]) -> bool: + # AC6: a server draining a request is never a candidate. + return k != protect and inflight.get(k, 0) <= 0 + + victims: List[Tuple[Tuple[str, str], str]] = [] + chosen = set() + + # 1. Idle timeout — independent of population size. + if self._idle_timeout and self._idle_timeout > 0: + for k in keys: + if not evictable(k): + continue + age = now - last_used.get(k, now) + if age >= self._idle_timeout: + victims.append((k, f"idle {int(age)}s >= {int(self._idle_timeout)}s")) + chosen.add(k) + + # 2. LRU cap — independent of idleness, so N simultaneously + # active roots still cannot exceed the host's memory. + if self._max_servers and self._max_servers > 0: + survivors = [k for k in keys if k not in chosen] + # ``protect`` occupies a slot: it is either already in + # ``_clients`` or is about to be inserted by the caller. + projected = len(survivors) + ( + 1 if protect is not None and protect not in keys else 0 + ) + if projected > self._max_servers: + # Oldest first; unstamped keys sort oldest (0.0). + ranked = sorted( + (k for k in survivors if evictable(k)), + key=lambda k: last_used.get(k, 0.0), + ) + for k in ranked: + if projected <= self._max_servers: + break + victims.append((k, f"lru cap {self._max_servers}")) + projected -= 1 + + return victims + + async def _enforce_bounds(self, *, protect: Optional[Tuple[str, str]] = None) -> None: + """Evict idle and over-cap clients. Never raises. + + Called on every spawn/reuse — the request path is the only + clock this service has, and tying eviction to it means a cache + that is never consulted also spawns nothing new, so its + population cannot grow. + """ + victims = self._eviction_candidates(protect=protect) + if not victims: + return + pending = [] + for key, reason in victims: + with self._state_lock: + # Re-check under the lock: a request may have arrived + # for this key between the decision and the shutdown. + if self._inflight.get(key, 0) > 0: + continue + client = self._clients.pop(key, None) + self._last_used.pop(key, None) + if client is None: + continue + eventlog.log_evicted(key[0], key[1], reason) + pending.append(client.shutdown()) + if pending: + await asyncio.gather(*pending, return_exceptions=True) + async def _shutdown_async(self) -> None: with self._state_lock: clients = list(self._clients.values()) self._clients.clear() self._broken.clear() self._last_used.clear() + self._inflight.clear() await asyncio.gather( *(c.shutdown() for c in clients), return_exceptions=True, @@ -614,6 +850,7 @@ async def _shutdown_async(self) -> None: def get_status(self) -> Dict[str, Any]: """Return a snapshot of the service for the CLI status command.""" + now = time.time() with self._state_lock: clients = [ { @@ -621,6 +858,11 @@ def get_status(self) -> Dict[str, Any]: "workspace_root": k[1], "state": c.state, "running": c.is_running, + # Surfacing idleness makes "is the cache evicting?" + # answerable from ``hermes lsp status`` rather than + # only from a log grep. + "idle_seconds": int(now - self._last_used.get(k, now)), + "inflight": self._inflight.get(k, 0), } for k, c in self._clients.items() ] @@ -630,6 +872,8 @@ def get_status(self) -> Dict[str, Any]: "wait_mode": self._wait_mode, "wait_timeout": self._wait_timeout, "install_strategy": self._install_strategy, + "idle_timeout": self._idle_timeout, + "max_servers": self._max_servers, "clients": clients, "broken": broken, "disabled_servers": sorted(self._disabled_servers), diff --git a/tests/agent/lsp/test_eviction.py b/tests/agent/lsp/test_eviction.py new file mode 100644 index 000000000000..93c6602abc8d --- /dev/null +++ b/tests/agent/lsp/test_eviction.py @@ -0,0 +1,427 @@ +"""Tests for LSP client eviction — idle timeout and LRU cap (SCA-4389). + +Why this file exists at all: + +``LSPService`` accepted an ``idle_timeout``, stored it on the instance, +and never read it again. ``DEFAULT_IDLE_TIMEOUT``'s comment claimed +"servers idle for >10min get reaped". Nothing reaped anything. On a +16 GiB host that produced 13 live ``typescript-language-server`` trees +holding ~16 GiB, which pushed the box into swap, which pushed free +disk under the CI runner's admission floor and took CI offline. + +So the bar here is specifically *not* "the policy function returns the +right list". A cache with an eviction path that never executes is the +exact ``--dry-run`` false-green this issue was filed about. The two +headline tests below drive **real spawned clients** through the real +request path and assert the process is actually gone, and each carries +a **positive control** — the same scenario with the bound disabled, +proving the server survives when it should. A green with no +demonstrated red would prove nothing. +""" +from __future__ import annotations + +import logging +import sys +import time +from pathlib import Path + +import pytest + +from agent.lsp import eventlog, workspace +from agent.lsp.manager import ( + DEFAULT_IDLE_TIMEOUT, + LSPService, + default_max_servers, +) +from agent.lsp.servers import ( + SERVERS, + ServerContext, + ServerDef, + SpawnSpec, +) + +MOCK_SERVER = str(Path(__file__).parent / "_mock_lsp_server.py") + + +@pytest.fixture +def mock_pyright(monkeypatch, tmp_path): + """Install the mock LSP as ``pyright`` and neutralise cwd anchoring. + + ``resolve_workspace_for_file`` prefers the cwd's git worktree over + the file's own. These tests need **distinct** workspace roots per + file, so the cwd is parked in a non-git directory to force the + per-file anchor. + """ + home = tmp_path / "not-a-repo" + home.mkdir() + monkeypatch.chdir(str(home)) + + target_index = next(i for i, s in enumerate(SERVERS) if s.server_id == "pyright") + original = SERVERS[target_index] + + def _spawn(root: str, ctx: ServerContext) -> SpawnSpec: + return SpawnSpec( + command=[sys.executable, MOCK_SERVER], + workspace_root=root, + cwd=root, + env={"MOCK_LSP_SCRIPT": "errors"}, + initialization_options={}, + ) + + SERVERS[target_index] = ServerDef( + server_id="pyright", + extensions=original.extensions, + resolve_root=lambda fp, ws: ws, + build_spawn=_spawn, + seed_first_push=False, + description="mock pyright", + ) + workspace.clear_cache() + eventlog.reset_announce_caches() + yield tmp_path + SERVERS[target_index] = original + workspace.clear_cache() + + +def _make_repo(root: Path, name: str) -> Path: + """A minimal git worktree with one Python file, returning the file.""" + repo = root / name + repo.mkdir() + (repo / ".git").mkdir() + (repo / "pyproject.toml").write_text("") + f = repo / "x.py" + f.write_text("print('hi')\n") + return f + + +def _service(**kw) -> LSPService: + kw.setdefault("enabled", True) + kw.setdefault("wait_mode", "document") + kw.setdefault("wait_timeout", 3.0) + kw.setdefault("install_strategy", "manual") + return LSPService(**kw) + + +def _roots(svc: LSPService): + return {key[1] for key in svc._clients} + + +# --------------------------------------------------------------------------- +# AC1 — idle timeout actually shuts a server down +# --------------------------------------------------------------------------- + + +def test_idle_server_is_evicted_and_survives_when_bound_disabled(mock_pyright): + """An idle server past the timeout is shut down for real. + + Both halves run the *identical* scenario; only ``idle_timeout`` + differs. The second half is the positive control — with the bound + off, repo A must still be alive, so the first half's eviction is + attributable to the timeout and not to some unrelated teardown. + """ + a = _make_repo(mock_pyright, "repo_a") + b = _make_repo(mock_pyright, "repo_b") + + # --- bound ON: A goes idle, touching B evicts it ----------------- + svc = _service(idle_timeout=0.25, max_servers=0) # cap disabled + try: + svc.snapshot_baseline(str(a)) + assert str(a.parent) in _roots(svc), "A never spawned; test proves nothing" + client_a = svc._clients[("pyright", str(a.parent))] + + time.sleep(0.4) # A is now idle past the timeout + svc.snapshot_baseline(str(b)) # request path is the clock + + assert str(a.parent) not in _roots(svc), "idle server was NOT evicted" + assert str(b.parent) in _roots(svc), "the requested server must survive" + assert not client_a.is_running, "evicted client's process is still alive" + finally: + svc.shutdown() + + # --- bound OFF (positive control): A survives the same sequence --- + workspace.clear_cache() + svc2 = _service(idle_timeout=0, max_servers=0) + try: + svc2.snapshot_baseline(str(a)) + time.sleep(0.4) + svc2.snapshot_baseline(str(b)) + assert str(a.parent) in _roots(svc2), ( + "control failed: A died with eviction disabled, so the first " + "half's result cannot be attributed to the idle timeout" + ) + finally: + svc2.shutdown() + + +# --------------------------------------------------------------------------- +# AC2 — LRU cap bounds the population independently of idleness +# --------------------------------------------------------------------------- + + +def test_cap_plus_one_evicts_least_recently_used(mock_pyright): + """The (cap+1)-th root evicts the LRU, not an arbitrary victim. + + Every server here is freshly used, so the idle timeout cannot be + what fires — this isolates the cap. + """ + a = _make_repo(mock_pyright, "repo_a") + b = _make_repo(mock_pyright, "repo_b") + c = _make_repo(mock_pyright, "repo_c") + + svc = _service(idle_timeout=0, max_servers=2) # idle bound disabled + try: + svc.snapshot_baseline(str(a)) + time.sleep(0.05) + svc.snapshot_baseline(str(b)) + assert _roots(svc) == {str(a.parent), str(b.parent)} + client_a = svc._clients[("pyright", str(a.parent))] + + # Re-touch B so A is unambiguously the least-recently-used. + time.sleep(0.05) + svc.snapshot_baseline(str(b)) + + svc.snapshot_baseline(str(c)) # third root, cap is 2 + + assert len(svc._clients) <= 2, f"cap breached: {_roots(svc)}" + assert str(a.parent) not in _roots(svc), "LRU root was not the victim" + assert str(b.parent) in _roots(svc), "wrongly evicted the recently-used root" + assert str(c.parent) in _roots(svc), "the requested root must survive" + assert not client_a.is_running + finally: + svc.shutdown() + + +def test_population_never_exceeds_cap_across_many_roots(mock_pyright): + """The steady state of multi-worktree work stays bounded. + + This is the shape of the original defect: N worktrees touched over + time, each leaving a permanent ~1.3 GiB tenant. + """ + svc = _service(idle_timeout=0, max_servers=2) + try: + for i in range(6): + f = _make_repo(mock_pyright, f"repo_{i}") + svc.snapshot_baseline(str(f)) + assert len(svc._clients) <= 2, ( + f"after {i + 1} roots the fleet is {len(svc._clients)}, cap is 2" + ) + finally: + svc.shutdown() + + +# --------------------------------------------------------------------------- +# AC6 — never evict a server mid-request +# --------------------------------------------------------------------------- + + +def test_inflight_server_is_never_evicted(): + """A draining request protects its server from both bounds.""" + svc = _service(enabled=False, idle_timeout=1, max_servers=1) + try: + old = ("pyright", "/repo/old") + new = ("pyright", "/repo/new") + svc._clients[old] = object() + svc._clients[new] = object() + svc._last_used[old] = 0.0 # ancient: idle AND the LRU + svc._last_used[new] = time.time() + + # Not in flight -> it is a candidate under both rules. + assert any(k == old for k, _ in svc._eviction_candidates()) + + # In flight -> immune, even though nothing else changed. + svc._acquire(old) + assert not any(k == old for k, _ in svc._eviction_candidates()), ( + "an in-flight server was selected for eviction" + ) + + # Released -> the release stamps it as just-used, so it is + # neither idle nor the LRU any more. ``new`` becomes the cap + # victim instead. This is the interesting direction: finishing + # a request must count as activity, otherwise a busy server + # would be evicted the instant it went quiet. + svc._release(old) + reasons = {k: r for k, r in svc._eviction_candidates()} + assert old not in reasons, "a just-released server was still a victim" + assert new in reasons, "the cap must still bind after the release" + assert "lru cap" in reasons[new] + finally: + svc.shutdown() + + +def test_protected_key_is_never_its_own_victim(): + """With a cap of 1, the root being requested must not be evicted.""" + svc = _service(enabled=False, idle_timeout=0, max_servers=1) + try: + key = ("pyright", "/repo/only") + svc._clients[key] = object() + svc._last_used[key] = 0.0 + assert svc._eviction_candidates(protect=key) == [] + finally: + svc.shutdown() + + +# --------------------------------------------------------------------------- +# AC3 — bounds are configurable, defaults derived from host memory +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "total_bytes,expected", + [ + (16 * 1024**3, 3), # Mac Mini that produced this defect + (128 * 1024**3, 26), # workstation + (8 * 1024**3, 1), # small host still gets a usable floor + (None, 4), # undiscoverable -> assume small + ], +) +def test_default_max_servers_scales_with_host_memory(monkeypatch, total_bytes, expected): + monkeypatch.setattr( + "agent.lsp.manager._host_memory_bytes", lambda: total_bytes + ) + assert default_max_servers() == expected + + +def test_derived_cap_is_bounded_on_an_enormous_host(monkeypatch): + """A 2 TiB host must not derive a cap that is not a bound.""" + monkeypatch.setattr( + "agent.lsp.manager._host_memory_bytes", lambda: 2048 * 1024**3 + ) + assert default_max_servers() == 32 + + +def test_config_plumbs_both_bounds(monkeypatch): + """``lsp.idle_timeout`` / ``lsp.max_servers`` reach the service.""" + import hermes_cli.config as cfg_mod + + monkeypatch.setattr( + cfg_mod, "load_config", + lambda *a, **k: {"lsp": {"idle_timeout": 42, "max_servers": 7}}, + ) + svc = LSPService.create_from_config() + try: + assert svc is not None + assert svc._idle_timeout == 42 + assert svc._max_servers == 7 + finally: + if svc is not None: + svc.shutdown() + + +def test_config_defaults_when_absent(monkeypatch): + """Absent config derives the cap rather than hardcoding one.""" + import hermes_cli.config as cfg_mod + + monkeypatch.setattr(cfg_mod, "load_config", lambda *a, **k: {"lsp": {}}) + svc = LSPService.create_from_config() + try: + assert svc is not None + assert svc._idle_timeout == DEFAULT_IDLE_TIMEOUT + assert svc._max_servers == default_max_servers() + finally: + if svc is not None: + svc.shutdown() + + +def test_malformed_config_values_fall_back_rather_than_crash(monkeypatch): + import hermes_cli.config as cfg_mod + + monkeypatch.setattr( + cfg_mod, "load_config", + lambda *a, **k: {"lsp": {"idle_timeout": "soon", "max_servers": "lots"}}, + ) + svc = LSPService.create_from_config() + try: + assert svc is not None + assert svc._idle_timeout == DEFAULT_IDLE_TIMEOUT + assert svc._max_servers == default_max_servers() + finally: + if svc is not None: + svc.shutdown() + + +# --------------------------------------------------------------------------- +# AC4 — eviction is observable +# --------------------------------------------------------------------------- + + +def test_eviction_logs_root_and_reason(mock_pyright, caplog): + """"The cache never evicts" must be falsifiable from the log.""" + a = _make_repo(mock_pyright, "repo_a") + b = _make_repo(mock_pyright, "repo_b") + + svc = _service(idle_timeout=0.25, max_servers=0) + try: + svc.snapshot_baseline(str(a)) + time.sleep(0.4) + with caplog.at_level(logging.INFO, logger="hermes.lint.lsp"): + svc.snapshot_baseline(str(b)) + finally: + svc.shutdown() + + evictions = [r.getMessage() for r in caplog.records if "evicted" in r.getMessage()] + assert evictions, "eviction happened with no log line" + assert any(str(a.parent) in m for m in evictions), "log does not name the root" + assert any("idle" in m for m in evictions), "log does not give the reason" + + +def test_evicted_root_reannounces_as_active_on_respawn(caplog): + """A re-spawn after eviction is a new process and must say so. + + ``log_active`` announces INFO once per root and DEBUG thereafter. + Without clearing that entry on eviction, a respawned server would + only ever log "reused client" — which would be false. + """ + eventlog.reset_announce_caches() + with caplog.at_level(logging.DEBUG, logger="hermes.lint.lsp"): + eventlog.log_active("pyright", "/repo/a") + eventlog.log_active("pyright", "/repo/a") # deduped -> DEBUG + eventlog.log_evicted("pyright", "/repo/a", "idle 700s >= 600s") + eventlog.log_active("pyright", "/repo/a") # must be INFO again + + active_info = [ + r for r in caplog.records + if r.levelno == logging.INFO and "active for /repo/a" in r.getMessage() + ] + assert len(active_info) == 2, ( + f"expected 2 INFO 'active' lines (pre- and post-eviction), got {len(active_info)}" + ) + + +def test_status_exposes_bounds_and_idleness(): + """``hermes lsp status`` can answer "is the cache evicting?".""" + svc = _service(enabled=False, idle_timeout=600, max_servers=3) + try: + status = svc.get_status() + assert status["idle_timeout"] == 600 + assert status["max_servers"] == 3 + finally: + svc.shutdown() + + +# --------------------------------------------------------------------------- +# Regression guard on the defect itself +# --------------------------------------------------------------------------- + + +def test_idle_timeout_is_actually_read(): + """Pin the exact defect: the bound must not be write-only again. + + Before SCA-4389 ``_idle_timeout`` was assigned in ``__init__`` and + referenced nowhere else. This asserts it changes behaviour. + """ + old = ("pyright", "/repo/old") + + never = _service(enabled=False, idle_timeout=0, max_servers=0) + strict = _service(enabled=False, idle_timeout=1, max_servers=0) + try: + for svc in (never, strict): + svc._clients[old] = object() + svc._last_used[old] = 0.0 # ancient + + assert never._eviction_candidates() == [], "idle_timeout=0 must disable the bound" + assert [k for k, _ in strict._eviction_candidates()] == [old], ( + "idle_timeout is being ignored — the SCA-4389 defect is back" + ) + finally: + never.shutdown() + strict.shutdown()