fix(lsp): bound the language-server fleet by count, not only idleness (SCA-4389) - #58
fix(lsp): bound the language-server fleet by count, not only idleness (SCA-4389)#58pai-scaffolde wants to merge 1 commit into
Conversation
… (SCA-4389) `main` already reaps *idle* clients — `_reap_idle_once` landed in `ede8809bb` and was hardened in `24a56f027`. That bounds a fleet nobody is touching. It does not bound a fleet everybody is touching, and only the second describes the outage. Thirteen live `typescript-language-server` trees held ~16 GiB on a 16 GiB host, pushed the box into swap, pushed free disk under the CI runner's admission floor, and took self-hosted CI offline for 5.5h. Every one of the thirteen had a fresh `_last_used`: they were all in active use across thirteen worktrees. The reaper was working exactly as designed and could not have helped. Nothing capped the population. This adds the second bound, alongside the existing one rather than replacing it: - **LRU cap.** `_enforce_population_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. Operator-overridable via `lsp.max_servers`. - **Derived default.** The cap comes from host RAM, not a constant: a 16 GiB Mac Mini derives 3, a 128 GiB workstation 26. Host memory that cannot be measured yields a conservative 4 rather than a guess upward — guessing high is what produced this defect. - **Eviction runs before the spawn**, so the fleet never transiently holds cap+1. On a host already at its ceiling the transient is the outage. - **In-flight protection.** `_acquire`/`_release` bracket the two request paths and `_enforce_population_cap` skips any key with a non-zero count, so a bound cannot tear a server down mid-request. The idle reaper needs no equivalent — `MIN_IDLE_TIMEOUT` is floored above the per-op wait budget, whereas the cap fires on demand with no such time guarantee. Evictions log at INFO via a `log_evicted_over_cap` line kept distinct from `log_reaped`: "your fleet is at its ceiling" and "a server went idle" call for different operator responses. The module docstring now states the eviction contract, with a note to delete it alongside any future removal of the eviction call. That paragraph is not decoration — the original defect was precisely a `DEFAULT_IDLE_TIMEOUT` comment promising servers "get reaped" while `_idle_timeout` was stored and never read. Tests (10 new, all green; 65 pass in tests/agent/lsp/): 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 behaviour tests drive real spawned mock servers through the real request path and assert the OS process is gone, with a positive control running the identical scenario uncapped to prove the fleet does reach three. Verified by neutering `_enforce_population_cap` to a no-op with the API otherwise intact: precisely the 3 behaviour-proving tests go red, and independently removing only the in-flight guard reddens only that test. The 5 failures in `tests/agent/lsp/` under a bare venv are pre-existing and identical on clean `origin/main` (a `tests/conftest.py` live-system guard on real subprocess teardown); this change is neutral on them — 55 passed before, 65 after. Refs: SCA-4389
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c57c4255
ℹ️ 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".
| try: | ||
| return int(os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES")) |
There was a problem hiding this comment.
When Hermes runs in a memory-limited Docker, Kubernetes, or Nix container, these sysconf values generally describe the host's physical RAM rather than the process's cgroup limit. For example, a 4 GiB container on a 128 GiB host derives a cap of 26 instead of roughly 1, allowing the language servers to trigger the cgroup OOM killer despite this safeguard. The repository already handles this distinction in hermes_cli.main._read_cgroup_memory_limit; use the smaller of the physical-memory and cgroup limits here.
Useful? React with 👍 / 👎.
| # Make room *before* spawning: the fleet must never hold | ||
| # cap+1 servers, not even transiently. | ||
| await self._enforce_population_cap() |
There was a problem hiding this comment.
When parallel non-overlapping write_file or patch calls request different new roots, both spawn tasks can run this check before either has inserted its client into _clients. Because _spawning is not counted as reserved capacity, each task observes the same free slot and both start a server; at max_servers - 1 clients this leaves max_servers + 1, and a larger parallel batch can overshoot further. Serialize the cap-and-spawn transaction or reserve a slot before awaiting startup.
Useful? React with 👍 / 👎.
| evictable = [ | ||
| key for key in self._clients if self._inflight.get(key, 0) == 0 | ||
| ] | ||
| evictable.sort(key=lambda k: self._last_used.get(k, 0.0)) | ||
| victims = evictable[:surplus] |
There was a problem hiding this comment.
When all clients needed to satisfy surplus are in flight, victims is shorter than surplus, but the caller still proceeds to spawn the incoming server. Nothing runs enforcement when those requests are released, so the fleet remains above max_servers indefinitely if the existing roots are subsequently reused; the new in-flight test creates exactly this over-cap state but only checks that the pinned key survives. Defer the spawn until capacity is available or schedule the remaining eviction on release.
Useful? React with 👍 / 👎.
| # Make room *before* spawning: the fleet must never hold | ||
| # cap+1 servers, not even transiently. | ||
| await self._enforce_population_cap() |
There was a problem hiding this comment.
When the fleet is full and the requested language-server binary is unavailable, this eviction runs before build_spawn() returns None, so a healthy LRU server is shut down even though no replacement can be started. The next edit for that evicted root must pay for a complete respawn and re-index, causing avoidable latency and process churn whenever users touch a file for a missing or failed auto-install server. Resolve the SpawnSpec before making room, while still performing the actual process start only after eviction.
Useful? React with 👍 / 👎.
| # Absent or malformed config derives the cap from host memory | ||
| # rather than assuming this host. | ||
| max_servers_cfg = lsp_cfg.get("max_servers") | ||
| try: | ||
| max_servers = None if max_servers_cfg is None else int(max_servers_cfg) |
There was a problem hiding this comment.
Because lsp.max_servers is consumed here but was not added under DEFAULT_CONFIG["lsp"], hermes config set lsp.max_servers 7 treats the supported option as an unknown key, and the dashboard's schema/default endpoints cannot expose it because those are generated from DEFAULT_CONFIG. Add a None or equivalent dynamic-default sentinel to the canonical config schema so operators can discover and configure the new behavioral setting through the existing configuration UX.
AGENTS.md reference: AGENTS.md:L58-L64
Useful? React with 👍 / 👎.
| assert small == 3 | ||
| assert small < 13, "a 16 GiB host must not permit the fleet that broke it" | ||
| assert large == 26 |
There was a problem hiding this comment.
These exact 3 and 26 assertions freeze the current footprint and budget constants, so any legitimate policy retuning breaks the suite even when the required behavior still holds: small hosts permit fewer servers, the outage-sized fleet is rejected, and both results remain within the clamps. Retain those relational and safety assertions instead of turning the current derived enumeration into a change detector.
AGENTS.md reference: AGENTS.md:L80-L83
Useful? React with 👍 / 👎.
| await asyncio.gather( | ||
| *(client.shutdown() for client in clients), | ||
| return_exceptions=True, |
There was a problem hiding this comment.
When an LRU server is slow or unresponsive during shutdown, this await can consume up to roughly three seconds (shutdown request timeout plus the termination grace period) inside _get_or_spawn, but get_diagnostics_sync still gives the entire spawn, initialize, and diagnostics path only _wait_timeout + 2 seconds. With the default seven-second outer budget, a replacement that normally initializes and responds in six seconds now times out after eviction and is marked broken for the rest of the service lifetime. Either budget explicitly for teardown or move capacity acquisition outside the diagnostics deadline without transiently exceeding the process cap.
Useful? React with 👍 / 👎.
|
Closing per principal decision 2026-08-20: superseded or abandoned (see landing ledger in scaffolde-ai .scaffolde/tasks/task-hermes-pr-backlog-landing/LEDGER.md). #61 superseded by merged #78; #70 superseded by merged #71 (sys.modules fix verified on main); #50/#51/#52/#58 abandoned SCA-4389 alternatives — #63's approach won and is landed. |
1 participant
Why this exists when three PRs already claim SCA-4389
#50,#51and#52are all open, allDIRTY, and all untouched for5-7 days. They conflict for one reason: each re-delivers the idle
reaper, which has since landed on
main(ede8809bb, hardened in24a56f027). They were written against amainthat had no reaper at all.So the branch that carries them is no longer the delta. This PR is cut
from current
origin/mainand carries only what main still lacks.What main still lacks
Main bounds the fleet by idleness only. That bounds a fleet nobody is
touching. It does not bound a fleet everybody is touching — and only the
second describes the outage:
Every one of those thirteen had a fresh
_last_used— they were inactive use across thirteen worktrees.
_reap_idle_oncewas workingexactly as designed and could not have helped. Nothing capped the
population.
The change
_enforce_population_capevicts the least-recently-used root when the population would exceedmax_servers. Operator-overridable vialsp.max_servers._acquire/_releasebracket the two request paths; keys with a non-zero count are never evicted. The idle reaper needs no equivalent —MIN_IDLE_TIMEOUTis floored above the per-op wait budget; the cap fires on demand with no such guarantee.The idle reaper is left exactly as it is. The two bounds are
independent and both are wanted.
Verification
10 new tests, all green; 65 pass in
tests/agent/lsp/.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 behaviour tests drive real spawned mock
servers through the real request path and assert the OS process is
gone, with a positive control running the identical scenario uncapped
to prove the fleet does reach three.
Controls actually run, not asserted:
_enforce_population_capto a no-op, API otherwise intact →precisely the 3 behaviour-proving tests go red, 7 pass.
test_inflight_client_is_never_evictedgoes red.The 5 failures in
tests/agent/lsp/under a bare venv are pre-existingand identical on clean
origin/main(atests/conftest.pylive-systemguard on real subprocess teardown). This change is neutral on them:
55 passed before, 65 after.
ruff@0.15.10 checkclean on all three files.Note for the reviewer
I did not close
#50/#51/#52— that call is the operator's. Ifthis lands, all three are fully superseded: their idle-reaper half is
already on main and their cap half is here, rebased.
Refs: SCA-4389