fix(lsp): bound concurrent language servers with an LRU cap (SCA-4389)

[pai-scaffolde]

Supersedes the still-novel half of #50. Closes the remaining acceptance item on SCA-4389.

Why #50 could not just be merged

#50 has been open since Jul 30 waiting on an operator merge. It has since gone mergeable_state=dirty — 4 conflicting files, and the branch is 3060 commits behind main.

Rebasing it mechanically would have been wrong. #50 shipped two things, and only one of them is still ours:

#50 acceptance	Status on today's main
1. idle-timeout eviction	already there — the fork has since picked up the upstream reaper
2. concurrent LRU cap	still absent — genuinely fork-local work

The upstream reaper commits d7578018c and 24a56f027 are both ancestors of origin/main now (git merge-base --is-ancestor → true), and _idle_reaper_loop / _reap_idle_once / _touch are live in agent/lsp/manager.py. Re-landing #50's version would have duplicated working code and resolved 4 conflicts to do it.

Verified the other half is still missing: grepping origin/main:agent/lsp/manager.py for max_client|lru|cap|evict returns one hit, the word "captured" in an unrelated docstring. So this PR is #50 rebased down to only what main still lacks.

The gap this closes

idle_timeout bounds how long a server survives. It does not bound how many exist. That is exactly why the incident was not preventable by the reaper: 13 live typescript-language-server processes held ~16 GiB and put pai-mac-mini into swap while every one of them was inside its idle window.

lsp.max_clients bounds the population itself. Unset derives from host memory — a quarter of RAM at ~1.3 GiB/server (the median resident footprint measured across those same 13 processes, range 1.2-1.65 GiB) — clamped to [1, 24]. On the 16 GiB incident host that derives 3, verified live. A cgroup limit wins over sysconf when smaller, so a 4 GiB container does not size its cap off 64 GiB of node RAM.

In-flight protection is load-bearing, not defensive

Eviction adds refcounted in-flight tracking, and the idle reaper now honours it too.

main already documents why this matters, at the MIN_IDLE_TIMEOUT clamp: reaping a client mid-request makes the outer wait time out, and that handler marks the (server, workspace) pair broken for the whole process lifetime. idle_timeout defends against that with a 30s floor.

A cap has no equivalent defense — it fires on spawn pressure, with no time floor at all. Without the guard, a cap could permanently disable LSP for a workspace. So when every client is busy, this deliberately stays briefly over the cap rather than killing a live diagnostic, and the next idle sweep collects the slack.

The just-spawned key is also protected: evicting the client the caller is about to return would make every spawn immediately undo itself.

Verification

• 24 new tests, all passing: cap derivation (incident host → 3, floor, ceiling, unreadable-memory fallback, cgroup precedence, v1 unlimited sentinel), LRU ordering, multi-victim drain, spawn protection, in-flight protection, refcounting, reaper in-flight guard, and config coercion of 0 / -1 / "nonsense" / inf / nan.
• No regressions. tests/agent/lsp/: 55 passed on clean main → 79 passed with this change. The same 5 pre-existing failures appear on both — test_client_e2e / test_diagnostics_field / test_stale_diagnostics trip the repo's live-system os.kill guard locally because they spawn real servers; they are unrelated to this diff and identical before and after.
• tests/hermes_cli/test_config.py + test_aux_config.py: 77 passed (touched config_defaults.py).
• ruff check clean across agent/, hermes_cli/config_defaults.py, tests/agent/lsp/.

Note on #50

Recommend closing #50 in favour of this branch — its idle-eviction half is now redundant against main, and its conflicts are against code that has since been replaced upstream.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7168f6a527

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[pai-scaffolde]

Added a09ea71ac4 — closes the last open item on SCA-4620 (the fleet-cap split of SCA-4607) on this branch rather than in a fourth competing PR.

What was still open. SCA-4620 scopes the cap as "evict before spawning, not after". This branch enforced it after client.start(), so a spawn at the ceiling briefly held cap + 1 live servers.

That was not a code-reading inference — it is measured over the real spawn path. The new test records how many servers are already running each time a new one starts; at cap=2 it read [0, 1, 2, 2]:

AssertionError: a spawn began while 2 servers were already live under cap=2;
the fleet peaked at 3. Evict before spawning, not after.

On a host at its memory ceiling that transient is the outage, not a detail: ~1.3 GiB is resident before anything is reclaimed, and the idle reaper cannot help because every one of those clients was used seconds ago.

The change

• _get_or_spawn enforces the cap before client.start(). No protect there — nothing is being returned yet, and a stale dead client under the same key should be reclaimed. The post-spawn protected sweep stays, to catch spawns for other roots that raced the reservation.
• _enforce_cap_async counts a key in _spawning but not yet in _clients as occupying a slot. That reservation is what leaves room for the pending spawn, and it also stops N concurrent spawns for N different roots from each claiming the same single free slot.

Why a new test file. test_client_cap.py drives _enforce_cap_async against hand-seeded state. That proves the eviction rule (LRU victim, drain until satisfied, skip in-flight) but never exercises a spawn, so it cannot see how many servers are actually alive. test_client_cap_e2e.py goes through get_diagnostics_sync with real subprocesses, every client freshly used, so the idle reaper is structurally excluded and only the cap can hold the line.

Verification

uv run pytest tests/agent/lsp/test_client_cap_e2e.py tests/agent/lsp/test_client_cap.py -q
29 passed in 0.65s

uv run pytest tests/agent/lsp/ -q
89 passed in 5.12s

uv run ruff check agent/lsp/manager.py tests/agent/lsp/test_client_cap_e2e.py
All checks passed!

The population probe is a verified positive control: it failed before the ordering fix and passes after, so it is genuinely sensitive to the defect.

Anti-criterion held. The upstream idle reaper (d7578018c / 24a56f027) is untouched — git diff origin/main...HEAD -- agent/lsp/manager.py | grep -E '_reap_idle_once|_idle_reaper_loop|DEFAULT_IDLE_TIMEOUT|log_reaped' returns nothing, and test_the_idle_reaper_still_works_under_the_cap asserts it is still running and still reaps while a cap is in force. Duplicating that leg is what made #50/#51/#52 unmergeable.

[pai-scaffolde]

Independent verification for SCA-4620 + triage of the 6 open review threads

Posting from SCA-4620 (the split-out "fleet has no size cap" issue). I started a
parallel implementation of the same cap, found this PR during dedup, and dropped
mine — this PR is the better implementation (RAM-derived default, in-flight
refcounting, and user docs; my version had a hardcoded default and no in-flight
handling). No competing PR will be opened.

Verification I ran against a09ea71ac4

Probe	Result
pytest tests/agent/lsp/test_client_cap.py tests/agent/lsp/test_client_cap_e2e.py	29 passed
pytest tests/agent/lsp (full suite)	89 passed
ruff check . (the blocking lint gate)	passed
ty check agent/lsp/manager.py	passed
PR CI	All required checks pass, mergeState CLEAN

Against SCA-4620's four scope items: max_clients config + documented floor ✅,
LRU victim on _last_used evicted before spawn ✅, a log_evicted line
distinct from log_reaped ✅, and tests covering the N+1-all-active case the
reaper structurally cannot ✅ (test_cap_holds_the_fleet_with_every_client_active).
The anti-criterion holds too — the upstream reaper is used, not re-implemented
(test_the_idle_reaper_still_works_under_the_cap).

Thread triage — all 6 were written against 7168f6a527, head is now a09ea71ac4

1 of the 2 P1s is already fixed by the newer commit. The other is real.

#	Thread	Verdict on current head
P1	Evict an idle victim before starting the replacement	✅ FIXED by a09ea71ac4 — await self._enforce_cap_async() now runs before client.start(). Safe to resolve.
P1	Re-enforce the cap when busy clients become idle	❌ STILL OPEN — see below
P2	Protect every newly spawned client during concurrent enforcement	⚠️ PARTIALLY fixed — see below
P2	Resolve the process's actual cgroup memory limit	❌ STILL OPEN — CGROUP_MEMORY_LIMIT_PATHS (manager.py:85-88) is a fixed 2-tuple of hierarchy-root paths; nothing reads /proc/self/cgroup.
P2	Keep eviction shutdown outside the diagnostics timeout budget	❌ STILL OPEN, and slightly more exposed now: _evict() awaits client.shutdown() inline, and the new pre-spawn _enforce_cap_async() puts that await on the critical path before client.start().
P2	Preserve the idle-reaping opt-out	❌ STILL OPEN — resolve_max_clients (manager.py:175-183) maps 0/negative to the derived cap, so there is no unbounded opt-out for users who set idle_timeout: 0 to keep indexes warm.

The open P1 is genuine — and it reopens exactly the incident this PR bounds

_enforce_cap_async correctly refuses to kill a live request and breaks out
over the cap when every client is in flight:

if victim is None:
    # Everything left is in-flight or protected.  Going over
    # the cap briefly is the right trade against killing a
    # live request; the next idle sweep collects the slack.
    break

But _release() (manager.py:790-796) only decrements _inflight — it never
re-runs enforcement:

def _release(self, key: Tuple[str, str]) -> None:
    with self._state_lock:
        remaining = self._inflight.get(key, 0) - 1
        if remaining > 0:
            self._inflight[key] = remaining
        else:
            self._inflight.pop(key, None)

So "briefly" is load-bearing and unenforced. The overage persists until
idle_timeout — and forever when idle_timeout: 0, which is a documented
supported setting. A burst that opens more workspaces than max_clients with all
of them active is precisely the SCA-4389 shape (13 live × ~1.3 GiB), so the cap
can still be walked past by the one workload it exists to stop.

test_release_makes_a_client_evictable_again doesn't catch it because it
hand-cranks enforce_cap_now() after the release; the production path has no
such call.

Smallest fix: have the last release schedule enforcement on the loop, e.g. drop
to zero in _release → asyncio.run_coroutine_threadsafe(self._enforce_cap_async(), loop)
(fire-and-forget so the releasing request isn't charged for a victim's shutdown,
which also keeps P2 #5 from getting worse). Worth a test that goes over cap with
all clients busy, releases them, and asserts the fleet drains without calling
enforce_cap_now().

Why the concurrent-spawn P2 is only partially fixed

a09ea71ac4 added the pending reservation, which does stop two concurrent
spawns from each claiming the same free slot. The residual race is narrower:
once spawn A executes self._clients[key] = client, A is in _clients with
_inflight[key] == 0 (the caller only _acquire()s later, at manager.py:624/654,
after _get_or_spawn returns). A is therefore in _evictable(), and a
concurrent spawn B — whose protect is key_B, not key_A — can select and
shut it down. A's caller then gets a stopped client.

Reserving not-yet-returned spawn keys globally (rather than a per-call
protect) would close both this and the original report.

---

I'm leaving SCA-4620 in review against this PR rather than opening anything
competing. Happy to push the P1 fix here if you'd rather hand it off — I held
off because this branch had a commit 20 minutes before I looked and I didn't
want to collide with an in-flight run.

[pai-scaffolde]

SCA-4621 fixed on this branch — 4e965beb6c

Closes the review thread about _release never re-enforcing the cap. Verified against this PR's own head (a09ea71ac4) by reading the live code, not the review text — the thread was written against 7168f6a527, so it needed re-confirmation before acting.

The gap, restated from the code

_enforce_cap_async breaks out over the cap when every client is in flight. That trade is correct and stays. What was missing is anything that collects the slack afterwards:

• _release() only decremented the refcount.
• the spawn path only enforces on the next spawn — a saturated burst may never issue one.
• the idle reaper does not run at all under idle_timeout: 0, a documented setting for keeping indexes warm.

So "going over the cap briefly" was load-bearing and unenforced. Under idle_timeout: 0 the overage is permanent, which is the SCA-4389 incident shape the cap exists to prevent.

The change

The last release for a key (refcount hits zero) schedules a cap sweep on the background loop, fire-and-forget through the existing safe_schedule_threadsafe helper.

Deliberately not awaited: charging the releasing request for a victim's shutdown would push eviction cost into the diagnostics timeout budget it is trying to leave — that is the separate open P2 on this PR, and this must not make it worse.

_overage_locked() gates the schedule so only over-cap releases pay anything, and it is now the single definition of overage that _enforce_cap_async reads too, so the two cannot drift.

The anti-criterion is intact. The sweep is the same _enforce_cap_async, so busy and protected clients are still never evicted — re-asserted directly by a new test on the new path.

Why the existing test did not catch this

test_release_makes_a_client_evictable_again hand-cranks enforce_cap_now() after the release. Production has no such call, so the test passed while the lifecycle gap stayed open. The new tests run against a real background loop and never touch the hand-crank.

Verification

tests/agent/lsp/test_client_cap.py — 28 pass / 0 fail.

Full tests/agent/lsp/ — 88 pass / 5 fail, and the same 5 fail identically on the pristine base (84 pass / 5 fail, i.e. my change is +4 tests, 0 regressions). Those 5 are environmental to a local runner, not code defects: 2 in test_client_e2e.py hit tests/conftest.py's live-system guard blocking os.kill on a PID outside the test subtree, and 3 more in test_diagnostics_field.py / test_stale_diagnostics.py fail the same way with and without this commit.

ruff@0.15.10 and ty@0.0.21 both clean on the changed files.

Positive control (required by the issue, and it is the whole point given the above): commenting out the _release schedule call reds exactly the 3 new behavioral tests and nothing else — 25 pass / 3 fail — and reverting returns 28 pass / 0 fail.

FAILED test_release_drains_the_overage_with_nothing_hand_cranking_it
FAILED test_release_triggered_sweep_still_refuses_to_evict_a_busy_client
FAILED test_partial_release_of_a_shared_client_schedules_no_sweep

One observation, deliberately not changed

_touch() runs after _release() at both call sites, so at sweep time the just-finished client still carries the previous request's _last_used. With several evictable clients that can order LRU slightly stale. I left it alone: where it actually matters (one evictable candidate) the ordering is irrelevant, and bumping _last_used inside _release would also refresh the idle clock on the error path, changing reaper semantics for no gain here. Flagging rather than folding it into this fix.

[pai-scaffolde]

Re-triaged all 6 review threads against head 58b1a4d70d — one was real, now fixed

Every thread was written against 7168f6a527. Two commits have landed since, so I re-checked each against current code rather than trusting the thread state (isResolved: false is not evidence a finding is still live).

Thread	Verdict at head	Disposition
P1 Re-enforce the cap when busy clients become idle	was real	Fixed in 4e965beb6c (SCA-4621). _release now schedules a sweep when the refcount hits zero.
P1 Evict an idle victim before starting the replacement	was real	Fixed in a09ea71ac4 (SCA-4620). _enforce_cap_async() runs before client.start().
P2 Protect every newly spawned client during concurrent enforcement	REAL — reproduced	Fixed in 58b1a4d70d (this push). Detail below.
P2 Resolve the process's actual cgroup memory limit	real, narrow	Split to a tracked follow-up. Not a blocker — see below.
P2 Keep eviction shutdown outside the diagnostics timeout budget	contested	Conflicts with the accepted P1. See below.
P2 Preserve the idle-reaping opt-out	product decision	Needs an owner call, not an engineering fix. See below.

---

Fixed: the concurrent-spawn eviction race

This one was correct and I reproduced it rather than reasoning about it.

_get_or_spawn publishes into _clients (L796) and only then returns; the caller runs _acquire after that return (_snapshot_async L656, _open_and_wait_async L686). Across that window the client is a running language server with an in-flight count of zero, so _evictable offered it up.

protect cannot cover it — it is per-call and names only the sweeping caller's own key, and the sweep that does the damage belongs to a different root's spawn, which passes no protect at all (L787).

Positive controls, both confirmed failing before the change:

unit: assert [('pyright', '/fresh')] == []
      Left contains one more item: ('pyright', '/fresh')

e2e:  AssertionError: the concurrent sweep evicted a client that had
      not reached its caller yet
      WARNING hermes.lint.lsp: lsp[pyright] fresh diagnostics timed out for .../x.py

That warning is the user-visible symptom you predicted: the caller silently loses its diagnostics.

Fix: a key still registered in _spawning has not reached its caller yet, so _evictable skips it. The counting side already agreed — _overage_locked counts _spawning reservations against the cap, so a slot is reserved for exactly the clients now protected, and no state is left where the cap is satisfiable only by evicting something unreachable.

You also asked for coverage of the real concurrent spawn path rather than isolated eviction helpers. Added: test_a_concurrent_spawn_does_not_evict_a_client_mid_handover drives a real subprocess through get_diagnostics_sync and asserts client.is_running after a concurrent no-protect sweep runs inside the handover window.

tests/agent/lsp: 95 passed. ruff and ty clean.

---

Not fixed here, with reasons

cgroup limit resolution — correct finding. CGROUP_MEMORY_LIMIT_PATHS reads hierarchy roots, so under a systemd unit with MemoryMax= the applicable limit is missed and the cap sizes from node RAM. It is a loose bound, not a regression — it degrades to pre-cap sizing rather than breaking anything — so it does not block this PR. Split to a tracked issue with the fix scoped (resolve via /proc/self/cgroup + /proc/self/mountinfo, walk to root taking the minimum) and a required positive control, precisely because the existing test stubs out the path resolution it would need to observe.

Eviction shutdown inside the diagnostics budget — the finding is accurate about the spawn path, but the proposed fix contradicts the P1 you filed above it. Making room before client.start() is what stops the fleet reaching cap + 1 live servers; detaching the victim's shutdown reintroduces exactly that peak. The release path already avoids the charge (_schedule_cap_enforcement is deliberately fire-and-forget) — only the spawn path pays, and it must, by construction. A real resolution means budgeting eviction separately in the outer timeout, which is a design change rather than a defect fix. Flagging for the maintainer rather than silently picking a side.

idle_timeout: 0 opt-out — real tension, but it is a product call, not a bug. Reserving a sentinel for "unbounded" would hand back the unbounded accumulation that caused SCA-4389 (13 live servers, ~16.3 GiB, host into swap, self-hosted runner under its disk-admission floor, CI offline). Whether warm-index users get an escape hatch from the memory guard is the maintainer's decision, so I have not made it unilaterally.

---

I do not hold merge authority on this PR.

[pai-scaffolde]

Codex thread triage — the P1 and one P2 are stale, resolved by later commits

GitHub still shows 5 unresolved threads on this PR. They were all submitted at 06:33:09Z against commit 7168f6a5, and three commits have landed since. Recording the state so the open-thread count is not read as outstanding work:

Thread	Status	Resolved by
P1 Evict an idle victim before starting the replacement	Stale — fixed	a09ea71a "make room before the spawn, not after" (16:45Z)
P2 Protect every newly spawned client during concurrent enforcement	Stale — fixed	58b1a4d7 "a client mid-handover is not an eviction candidate" (18:13Z)
P2 Resolve the process's actual cgroup memory limit	Split out	SCA-4623 for manager.py; the TUI twin is PR #65 (SCA-4627)
P2 Preserve the idle-reaping opt-out	Rebutted by design	see below
P2 Keep eviction shutdown outside the diagnostics timeout budget	Still open	not addressed here

The P1 fix is visible at the spawn path — the pre-spawn sweep now runs with key already registered in _spawning, so the cap counts the incoming server as occupying its slot and the fleet never reaches cap + 1 during startup, which was the finding's whole concern.

On the idle-reaping opt-out, this is a deliberate stance rather than an oversight: resolve_max_clients coerces 0 to a derived cap because garbage input must not silently restore the unbounded accumulation the cap exists to stop. idle_timeout: 0 keeps its documented meaning — the reaper does not run — and the all-busy branch comments that case explicitly. What a user cannot do is disable the peak bound, which is the one thing SCA-4389 measured at 13 live servers / ~16.3 GiB.

The eviction-shutdown P2 is genuinely still open and I am not silently closing it. It is a latency concern (an unresponsive evicted server can eat ~3s of the diagnostics budget), not a correctness or memory-bound one, so it does not gate this PR's purpose. Flagging it here rather than folding it in, since this PR is CLEAN and green and the merge order is #63 → #64 → #65.