fix(lsp): charge each language server its own measured footprint (SCA-4688) - #67
Merged
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bde1e1293
ℹ️ 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 a commit
that referenced
this pull request
Aug 12, 2026
Restacking #67 onto the updated #66 tip brought in test_v2_node_memory_does_not_size_the_cap_inside_a_constrained_unit (SCA-4623), which asserted default_max_clients() == 1 inside a 4 GiB systemd unit. That figure was the count bound doing the whole job while every server type was charged the typescript median; with per-type footprints the count ceiling divides by the cheapest server, so the same unit now derives 6 and the assertion failed. The defect the test guards is unchanged: a fleet must not be sized off the node's 64 GiB when the unit allows 4 GiB. So it is restated against both halves of the bound rather than retuned to the new number -- count: the unit derives 6 while the node behind it would clamp at MAX_CLIENT_CAP (24), so the cap still comes from the unit; bytes: the unit's budget is 1 GiB, which one typescript server (1700 MiB) already exceeds -- which is what actually holds the fleet inside the unit now. 126 LSP tests and 9 TUI heap-sizing tests pass; ruff and ty clean.
pai-scaffolde
added a commit
that referenced
this pull request
Aug 12, 2026
Codex review of PR #67 raised three P1s and one P2 against the enforcement path the per-type footprint change introduced. All four reproduce against the committed code; each fix below has a test that fails without it. Count the pending replacement in the one-client floor (P1). The floor read `len(_clients) - MIN_CLIENT_CAP`, so one live server plus one pending one computed nothing removable and reported zero overage. `_get_or_spawn` sweeps before `client.start()` precisely to bound the peak, and a zero overage let two typescript servers (3.4 GiB) index concurrently against a 1 GiB budget. The floor now applies to the fleet that will exist; evicting the live one still leaves the pending one, so the floor is honoured. Track draining bytes rather than equating drains with victims (P1). The sweep stopped when the number of drains in flight reached the victim count. Those units are interchangeable only while every server costs the same: a draining 150 MiB yaml server satisfied a one-victim overage, the sweep stopped, and after that cheap process exited three 1.7 GiB typescript servers sat over a 4 GiB budget with no sweep scheduled to notice. The stop condition now asks whether the fleet would still be over once every drain lands, which is exactly equivalent for a homogeneous fleet. Decouple the count-cap tests from host memory (P1). The byte budget derived from host RAM unconditionally, so the count-cap suites became host-dependent: on a 1 GiB worker the budget is 256 MiB, every fake pyright client is charged 800 MiB, and the byte bound evicted below the cap under test. The budget is now injectable behind a sentinel that keeps "derive from the host" distinguishable from "no byte budget", and the count-cap fixtures pin it off so each suite measures one bound. Production is unchanged and gated by a test that fails if the default is flipped. Report byte-budget evictions as memory evictions (P2). Every eviction logged `cap <n> exceeded` regardless of which half bound, so three typescript servers over the byte budget on a 16 GiB host reported `cap 24 exceeded` — pointing operators at a bound that was not binding. The overage now carries which bound produced it. Class sweep: the same host-dependence in `test_client_cap_e2e.py`'s fixture is fixed alongside, since it asserts exact populations against count caps. Verification: 144 passed (tests/agent/lsp + test_tui_heap_sizing); ruff and ty clean on the touched files. Each new gate confirmed to fail against the pre-fix code. The SCA-4623 constrained-unit discrimination (unit 6 vs node 24) and all four SCA-4688 acceptance criteria re-probed and intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pai-scaffolde
pushed a commit
that referenced
this pull request
Aug 14, 2026
The attribution gate compares every author email from merge-base(main, HEAD) onward against contributors/emails/, and this branch is the first on the stack authored as engineer@scaffolde.ai rather than pai@scaffolde.ai — the rest of #63..#67 carry the mapped address, which is why the gate is green there and red here. Same GitHub account (pai-scaffolde), second machine email. Created with scripts/add_contributor.py, which is the remediation the failing job itself prints; AUTHOR_MAP in scripts/release.py is untouched.
pai-scaffolde
changed the base branch from
fix/sca-4628-evict-outside-budget
to
main
August 20, 2026 01:58
…-4688) Replayed onto current main. This branch was stacked on PRs that landed as squashes, so its original history conflicted with itself; only this PR's own delta is kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pai-scaffolde
force-pushed
the
fix/sca-4688-type-aware-footprint
branch
from
August 20, 2026 03:49
6a3d95b to
dbf2905
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
The fleet cap derived from a single whole-fleet footprint constant, so every
language-server type was charged the typescript median. On the 16 GiB host
this fleet runs on:
Measured healthy peak concurrency is 7 (SCA-4584, 11.6h span, 87 production
reap events). So the fleet evicted and respawned during ordinary multi-worktree
work.
Raising the constant was not available: 7 servers at 1.3 GiB is 9.1 GiB, which a
25%-of-RAM budget cannot express at any correct scalar value. The bind was the
single value, not its size.
Measurement (acceptance criterion 1)
scripts/measure_lsp_footprint.py(added here) spawns a real server against areal project and sums physical footprint over the process subtree. RSS is
unusable: the Node-based servers run behind a shim reading 5-11 MB while the
analysis happens in a child.
Note the direction: typescript was under-charged by the old constant, while
yaml was over-charged 31x. Raw readings:
docs/lsp-footprint-measurements.json.Every charged value is its measurement rounded up — headroom always makes
the cap stricter, never looser.
Change
The budget does not move. It is the same 25% of host RAM; servers fit better
only because most of them are genuinely cheap.
Admission is now two bounds:
the most expensive one (leaving it sized off typescript would have kept the
cap of 3 this fixes);
own footprint, reporting how many LRU victims the sweep must remove — the same
unit
_enforce_cap_asyncalready loops on.Acceptance criteria
server_idvalues from a real measurement — table above, harness and raw JSON committed.test_mixed_fleet_of_seven_fits_the_incident_host, withtest_the_same_fleet_would_have_been_over_the_old_flat_chargeguarding the premise.test_typescript_heavy_fleet_still_binds,test_typescript_fleet_is_bounded_below_the_incident_footprint.test_unknown_server_charges_the_conservative_default.Notes
never evicted — otherwise every spawn would immediately undo itself.
_shutting_downis keyed by task with the client key as the value; chargingthe task would have silently mis-charged. Covered by
test_shutting_down_servers_still_hold_their_memory.pending approval gates are untouched.
Verification
Closes SCA-4688.