fix(lsp): bound the language-server fleet by idle timeout and LRU cap (SCA-4389) - #52
fix(lsp): bound the language-server fleet by idle timeout and LRU cap (SCA-4389)#52pai-scaffolde wants to merge 1 commit into
Conversation
… (SCA-4389) `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
![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: 8773797f5b
ℹ️ 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".
| projected = len(survivors) + ( | ||
| 1 if protect is not None and protect not in keys else 0 | ||
| ) |
There was a problem hiding this comment.
When two callers first touch different uncached roots concurrently, each can calculate projected before either new client enters _clients; _spawning is not counted, so with max_servers=1 both requests see a projected population of one and both spawn. The cache then retains two servers, and _release() does not re-enforce the cap, so the over-cap fleet can persist indefinitely and defeat the memory safeguard under concurrent gateway traffic.
Useful? React with 👍 / 👎.
|
|
||
| # 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) |
There was a problem hiding this comment.
When two callers first request the same uncached root while this eviction awaits a victim's shutdown, both have already observed no entry in _spawning; after the await, each creates a separate client and overwrites _spawning[key]. Both language-server processes start, but only the last client remains in _clients, leaving the other process untracked and therefore unreachable by eviction or service shutdown.
Useful? React with 👍 / 👎.
| have sized against a host it could not measure. | ||
| """ | ||
| try: | ||
| return int(os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES")) |
There was a problem hiding this comment.
In a Docker, Kubernetes, or memory-limited systemd cgroup, SC_PHYS_PAGES generally reports machine RAM rather than the process's memory ceiling, so an 8 GiB container on a 128 GiB host derives 26 servers instead of one and can be OOM-killed long before this cap engages. The repository already handles this distinction in hermes_cli/main.py via the cgroup v1/v2 memory-limit files; this calculation should use the smaller effective limit, with a real-path container/cgroup test rather than only mocked physical totals.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| if client is None: | ||
| continue | ||
| eventlog.log_evicted(key[0], key[1], reason) | ||
| pending.append(client.shutdown()) |
There was a problem hiding this comment.
When an evicted server is unresponsive and graceful shutdown times out, this call removes the client from the cache but LSPClient._cleanup_process() only terminates or kills the direct process, even though it was started in a new session and servers such as typescript-language-server own large tsserver children. Those children survive as untracked processes, so repeated evictions can recreate the original memory ratchet while status reports a bounded fleet; terminate the process group on the fallback path and validate with a child-spawning server rather than only checking the parent client's is_running flag.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| lsp_cfg = {} | ||
|
|
||
| enabled = bool(lsp_cfg.get("enabled", True)) | ||
| # Both bounds are operator-overridable; absent config derives |
There was a problem hiding this comment.
Although this introduces operator-overridable lsp.idle_timeout and lsp.max_servers, neither key is added to DEFAULT_CONFIG or the canonical LSP configuration guide, whose performance section still explicitly says servers live for the process lifetime and there is no idle reaper. Operators following the documented configuration therefore cannot discover or accurately reason about the new cold-start and memory behavior; update the default schema/comments and user guide alongside this plumbing.
Useful? React with 👍 / 👎.
| "inflight": self._inflight.get(k, 0), | ||
| } |
There was a problem hiding this comment.
The normal hermes lsp status renderer in agent/lsp/cli.py ignores the newly added idle_timeout, max_servers, idle_seconds, and inflight fields; only --json exposes them. Consequently the default command still cannot answer the comment's stated question of whether the cache is evicting or approaching its cap, so these fields should also be included in the human-readable service and client rows.
Useful? React with 👍 / 👎.
| key = (client.server_id, client.workspace_root) | ||
| self._acquire(key) |
There was a problem hiding this comment.
Both request paths call _acquire() only after _get_or_spawn() returns, but reuse can await shutdowns inside _enforce_bounds(). While that await is suspended, the selected client is not marked in flight: for example, with a cap of two, a request reusing A can wait while idle B shuts down, C can fill the freed slot, and a fourth request can then select and shut down A before the original request resumes. The original request proceeds with a stopped client and silently loses diagnostics, so the lease must be acquired atomically with lookup and released in both request paths.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| eventlog.log_evicted(key[0], key[1], reason) | ||
| pending.append(client.shutdown()) | ||
| if pending: | ||
| await asyncio.gather(*pending, return_exceptions=True) |
There was a problem hiding this comment.
When a root switch triggers eviction, this await can spend roughly three seconds on a nonresponsive client's graceful shutdown before the replacement server even begins its potentially long initialization and diagnostics wait. The public wrappers still use the old eight-second baseline budget and wait_timeout + 2 post-write budget; exceeding either cancels the healthy replacement request and permanently adds its root to _broken. Slow server shutdowns or cold starts can therefore disable LSP for a project for the rest of the process, so eviction time must be budgeted separately or excluded from the failure that marks the new root broken.
Useful? React with 👍 / 👎.
Addresses the nine review findings on #75. The load-bearing one is a false green in the tool built to end false greens: the exit expression read `mutual_conflicts or diverged`, so a run in which NO PR was ever measured returned 0 and announced a landable queue. Positive control, with every probe raising: 3 PRs, 0 verdicts, exit 0. `exit_code_for` now derives the status from coverage — 2 when any PR has no verdict, 1 when everything was measured and some PR has no landing order, 0 only when every PR is in the order. `diverged` is no longer a status of its own: a diverged stack is re-probed as siblings, so it either produces real verdicts or lands in `unverified`, and failing on the divergence itself reported an un-landable queue for stacks the pair probes had already sequenced cleanly. Running that against the live queue caught a regression in the fix: a PR that fails preflight is DIRTY, which is a measured verdict, not a failure to measure. Filing it under `unverified` made the sweep exit 2 — job red, collision issue suppressed — on every run while any DIRTY PR was open. Split into `unmergeable` (blocks, exit 1) vs `unverified` (exit 2). The live queue now exits 1 with #51/#52 held for a stated reason instead of exit 2. Also from the review: - Probe commits get an explicit identity. `commit-tree` exits 128 "Author identity unknown" where actions/checkout leaves no user.name/user.email; every pair probe would raise, and combined with the old exit expression the sweep would report a landable queue having merged nothing. - The status -> outcome decision moves to scripts/ci/pr_merge_order_gate.sh so the tests execute it instead of grepping the workflow for substrings. Those assertions passed regardless of correct wiring and broke on reformatting — one of them failed on this very change while the behaviour improved. The gate accepts only 0/1 and fails the job for 2, 127, 137 and anything else. - The candidate order is replayed onto a cumulatively advancing base. Pairwise probes each start from the trunk plus ONE landed PR, so they verify a proxy; the replay measures the claim the report actually makes. A rejection recomputes the order without the culprit, bounded and terminating. - A queue truncated by `--limit` is refused rather than answered, since a collision involving an omitted PR would otherwise read as clean. - Blocked state propagates across the full ancestry DAG. The scalar `stacked_on` keeps only the last parent, so a head containing two open PRs could reach merge_order still carrying a forgotten ancestor's commits. - The empty-queue early return prunes probe refs, which it previously skipped exactly when there was most to collect. Verification: 47 tests pass (was 26), shellcheck and ruff clean, workflow YAML parses, live sweep against the 20-PR queue exits 1 with a replay-verified order. Note: I could not reproduce the pairwise-clean/cumulatively-conflicting triple the replay finding describes — 20+ line geometries plus rename and add/add families all stayed consistent. The replay is kept because it measures the report's actual claim rather than a proxy, and a replay failure is by construction a real conflict on the real landing sequence.
* feat(ci): detect PR queue collisions GitHub cannot see GitHub's `mergeStateStatus` measures each PR against its own base, never against the other open PRs. On 2026-08-10 six PRs all reported CLEAN and the queue could not land in any order, while four issues sat in review waiting on a merge path that had been broken for days. A queue that cannot land reads exactly like a queue awaiting review. `scripts/pr_merge_order.py` simulates the real three-way merge of every candidate pair with `git merge-tree --write-tree` and reports a landable order, or names the colliding pair and the files. Two decisions carry the work: Ancestry gates stacking. `baseRefName` records what a branch was opened against and keeps saying so after a rebase or force-push moves the child off its parent's head. Believing it marks a diverged pair "same stack" and skips the collision probe — the shape that renders as a tidy linear stack on the board. Every declared link is confirmed with `git merge-base --is-ancestor`; an unconfirmed one is reported and the pair is probed. The mirror case is caught too: a child targeting the trunk whose head already contains another open PR gets an ordering edge, because landing it first would merge that PR's work under a different review. Merge-commit semantics, not squash. This repository takes merge commits. A squash simulation re-applies a parent's hunks as a flat diff against a base that already has them and reports collisions that do not exist — the false positive that had to be discarded during the manual sweep. A test pins the choice by showing both semantics disagreeing on one pair. The sweep is read-only: no checkout, no merge, no branch write. Heads go to `refs/pr-merge-order/<n>`, pruned each run so the namespace cannot grow unbounded, and `merge-tree` writes only loose objects — so it stays safe to run while the queue is frozen. The repo slug is pinned to a named remote rather than left to `gh`. This checkout carries origin/pai-scaffolde alongside upstream NousResearch, and with no default set `gh pr list` resolves to upstream and returns a queue this repository never lands. Tests run against real git repositories rather than stub probes, since the failure under test only exists in actual three-way merges. Each positive control is paired with a negative one so neither can pass by a constant verdict, and the wiring tests fail if the workflow stops listening. A detector nothing listens to is not a fix: the verdict reaches the job summary every run and opens or updates a GitHub issue on collision. A sweep that cannot complete fails the job rather than reporting a clean queue. Refs: SCA-4638, SCA-4633 * fix(ci): derive the sweep's exit status from verified coverage Addresses the nine review findings on #75. The load-bearing one is a false green in the tool built to end false greens: the exit expression read `mutual_conflicts or diverged`, so a run in which NO PR was ever measured returned 0 and announced a landable queue. Positive control, with every probe raising: 3 PRs, 0 verdicts, exit 0. `exit_code_for` now derives the status from coverage — 2 when any PR has no verdict, 1 when everything was measured and some PR has no landing order, 0 only when every PR is in the order. `diverged` is no longer a status of its own: a diverged stack is re-probed as siblings, so it either produces real verdicts or lands in `unverified`, and failing on the divergence itself reported an un-landable queue for stacks the pair probes had already sequenced cleanly. Running that against the live queue caught a regression in the fix: a PR that fails preflight is DIRTY, which is a measured verdict, not a failure to measure. Filing it under `unverified` made the sweep exit 2 — job red, collision issue suppressed — on every run while any DIRTY PR was open. Split into `unmergeable` (blocks, exit 1) vs `unverified` (exit 2). The live queue now exits 1 with #51/#52 held for a stated reason instead of exit 2. Also from the review: - Probe commits get an explicit identity. `commit-tree` exits 128 "Author identity unknown" where actions/checkout leaves no user.name/user.email; every pair probe would raise, and combined with the old exit expression the sweep would report a landable queue having merged nothing. - The status -> outcome decision moves to scripts/ci/pr_merge_order_gate.sh so the tests execute it instead of grepping the workflow for substrings. Those assertions passed regardless of correct wiring and broke on reformatting — one of them failed on this very change while the behaviour improved. The gate accepts only 0/1 and fails the job for 2, 127, 137 and anything else. - The candidate order is replayed onto a cumulatively advancing base. Pairwise probes each start from the trunk plus ONE landed PR, so they verify a proxy; the replay measures the claim the report actually makes. A rejection recomputes the order without the culprit, bounded and terminating. - A queue truncated by `--limit` is refused rather than answered, since a collision involving an omitted PR would otherwise read as clean. - Blocked state propagates across the full ancestry DAG. The scalar `stacked_on` keeps only the last parent, so a head containing two open PRs could reach merge_order still carrying a forgotten ancestor's commits. - The empty-queue early return prunes probe refs, which it previously skipped exactly when there was most to collect. Verification: 47 tests pass (was 26), shellcheck and ruff clean, workflow YAML parses, live sweep against the 20-PR queue exits 1 with a replay-verified order. Note: I could not reproduce the pairwise-clean/cumulatively-conflicting triple the replay finding describes — 20+ line geometries plus rename and add/add families all stayed consistent. The replay is kept because it measures the report's actual claim rather than a proxy, and a replay failure is by construction a real conflict on the real landing sequence.
|
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
The defect
LSPServiceaccepted anidle_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._last_usedwas written on every request and only ever consulted in order to be cleared.So the steady state of normal multi-worktree work was 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.
Measured on the host (SCA-4389): a gateway up 3d07h held 13 live
typescript-language-servertrees, ~16.3 GiB, on a 16 GiB host. That pushed the box into swap, which pushed free disk under the self-hosted runner's admission floor, which took CI offline and froze the merge queue.Every worktree an agent opened added a permanent ~1.3 GiB tenant. 55 worktrees were registered.
The fix
Two independent bounds, both applied on the request path (the only clock this service has — and a cache nobody consults also spawns nothing, so its population cannot grow):
idle_timeoutseconds is shut down. Re-spawning costs seconds; holding costs 1.3 GiB indefinitely.max_servers, so N simultaneously active worktrees still cannot exceed host memory regardless of traffic.Both operator-configurable (
lsp.idle_timeout,lsp.max_servers). The cap's default is derived from host RAM rather than hardcoded:Undiscoverable memory assumes small, not large — guessing upward is what produced this defect.
Safety properties
_acquire/_releasebracket both request paths, and_enforce_boundsre-checks the in-flight count under the lock before shutting anything down.protect=keyreserves the requesting root's slot so a cap of 1 cannot evict the very client just asked for.Observability
Every eviction emits an INFO line naming the root and the reason, so "the cache never evicts" is falsifiable by grepping the log rather than reconstructed from
ps. Deliberately not deduped — suppressing a repeat eviction would hide exactly the thrash worth seeing.Eviction also clears the root's
log_activeannounce entry, so a later re-spawn announces INFO honestly instead of loggingreused clientfor a brand-new process.hermes lsp statusgainsidle_timeout,max_servers, and per-clientidle_seconds/inflight.Verification
The tests are not "the policy function returns the right list." A cache with an eviction path that never executes is precisely the false green this defect was. The two headline tests drive real spawned clients through the real request path and assert the process is actually gone (
client.is_running is False), and each carries a positive control running the identical scenario with the bound disabled — so the eviction is attributable to the bound and not to unrelated teardown.Positive control on the fix itself. With
_enforce_boundsneutered to a no-op and the API otherwise intact — reproducing the original defect shape exactly — precisely the 4 behaviour-proving tests go red:The 13 that still pass are policy/config/derivation tests that legitimately do not depend on execution. A regression test (
test_idle_timeout_is_actually_read) pins the exact defect so the bound cannot silently become write-only again.Acceptance criteria (SCA-4389)
client.shutdown()Notes
upstream/hermes/fork-contract.yaml(Executable Hermes source patches that cannot be expressed as Scaffolde overlays, plugins, or runtime config).lsp-tree-watchdog(SCA-4413) observes this fleet and explicitly does not reap. It stays as-is; this bounds it.Refs: SCA-4389