fix(lsp): read the memory limit from this process's cgroup, not the hierarchy root (SCA-4623)

[pai-scaffolde]

Fixes SCA-4623. Stacked on #63 — host_memory_bytes() only exists on that branch, so this targets fix/sca-4389-lsp-client-cap, not main.

The defect

_cgroup_memory_limit_bytes consulted two fixed paths:

CGROUP_MEMORY_LIMIT_PATHS = (
    "/sys/fs/cgroup/memory.max",                    # v2
    "/sys/fs/cgroup/memory/memory.limit_in_bytes",  # v1
)

Both are hierarchy roots. Under a systemd unit with MemoryMax=, or in a container without a private cgroup namespace, the limit that binds the process lives under the path named in /proc/self/cgroup; the root reads max (v2) or the LONG_MAX sentinel (v1). Both are skipped, the function returns None, and host_memory_bytes() falls through to SC_PHYS_PAGES — the node's RAM.

On a 64 GiB node inside a 4 GiB unit that derives a cap of 12 against a budget that affords 1, so the cap permits the OOM kill inside the child cgroup that the cgroup branch was added to prevent.

Loose bound, not a regression: it degrades to the pre-cap sizing rather than breaking anything that worked. That is why it was P2 and did not block #63.

The fix

• Resolve this process's cgroup from /proc/self/cgroup and the mount from /proc/self/mountinfo, then read the limit file under that path.
• Walk to the mount point taking the minimum finite limit — an ancestor's limit binds a descendant, so a slice allowing 2 GiB is the ceiling even when the leaf declares 8 GiB.
• Strip the mountinfo root prefix: a mount can expose a subtree, so /system.slice/hermes.service under a mount of /system.slice is on disk at <mount_point>/hermes.service.
• Consult both hierarchies rather than the first that matches — a hybrid host mounts cgroup2 without the memory controller, which lives on v1.
• Sentinel handling (max, <= 0, >= 1 << 50) moved verbatim into _parse_memory_limit; the fixed paths remain the fallback for hosts with no /proc (macOS, Windows).
• _UNRESOLVED keeps "/proc could not answer" distinct from "/proc says unlimited", so an unconstrained host does not fall back to re-reading the roots and a constrained one is not second-guessed.

Anti-criterion

LSP_MEMORY_BUDGET_FRACTION, LSP_CLIENT_FOOTPRINT_BYTES and the [MIN_CLIENT_CAP, MAX_CLIENT_CAP] clamp are untouched — verified by grepping the diff for every one of them. This is limit discovery only.

Why the existing test could not catch it

test_cgroup_limit_wins_over_node_memory_when_smaller monkeypatches CGROUP_MEMORY_LIMIT_PATHS to a temp file. It proves the parsing and the min(), and cannot observe path resolution — the deployment behaviour is exactly what it stubs out.

The new tests build a fabricated /proc + cgroupfs under a new CGROUP_FS_ROOT knob, so resolution itself is the thing under test. The two existing tests gained a no_proc() call; without it they would read the real /proc on Linux CI and stop being deterministic.

Positive control

The new tests were confirmed failing against the current fixed-path behaviour reproduced under the fabricated root (a scratch patch adding only CGROUP_FS_ROOT, resolution unchanged):

test_v2_limit_read_from_this_process_cgroup_not_the_hierarchy_root
  assert None == (4 * 1073741824)
test_v2_node_memory_does_not_size_the_cap_inside_a_constrained_unit
  assert 68719476736 == (4 * 1073741824)     # 64 GiB node RAM, the defect verbatim
test_an_ancestor_limit_binds_a_descendant
  assert None == (2 * 1073741824)
test_v1_memory_limit_read_from_the_controller_mount
  assert None == (3 * 1073741824)
test_mount_root_prefix_is_stripped_from_the_cgroup_path
  assert None == (5 * 1073741824)

Verification

• pytest tests/agent/lsp/test_client_cap.py — 37 passed (8 new).
• Real-host smoke on macOS (no /proc): resolution reports _UNRESOLVED, falls back to the fixed paths, host_memory_bytes() = 16 GiB, default_max_clients() = 3 — the documented incident-host answer, unchanged.
• ruff 0.15.10 clean on both files.
• ty 0.0.21 reports the same 4 pre-existing FakeClient diagnostics before and after the change; zero new.
• The 5 failures elsewhere in tests/agent/lsp reproduce identically on the untouched base commit (conftest.py:1091 RuntimeError, missing server binaries) — pre-existing, unrelated.

Provenance

Codex review thread on #63 (P2, agent/lsp/manager.py:89), re-triaged and confirmed against head 58b1a4d70d, which is this branch's base.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5415984c7f

ℹ️ 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]

Merge-order warning for this stack (verified locally, not speculation)

This is now a 3-deep stack — #63 → #64 → #65 — and all three show MERGEABLE. That green is per-base, not per-main, so it does not tell you what happens after the first one lands. I tested the two paths with git merge-tree --write-tree (read-only, dangling objects only):

Scenario A — merge commit for each, in order:

main + #63  -> exit 0   (clean)
#63  + #64  -> exit 0   (clean)
#64  + #65  -> exit 0   (clean)

Scenario B — squash #63 first, then merge #64:

squashed63 + #64  -> exit 1
CONFLICT (content): Merge conflict in agent/lsp/manager.py
CONFLICT (add/add): Merge conflict in tests/agent/lsp/test_client_cap.py

The conflict is not a content disagreement. The simulated squash produces a tree byte-identical to #63's:

#63 tree      : 580dbaa06f0c134b5d6c46e6189f32374b4d082a
squashed tree : 580dbaa06f0c134b5d6c46e6189f32374b4d082a

It conflicts purely because squashing rewrites #63's four commits into one new SHA, so they are no longer ancestors of main, and #64 tries to re-apply them. The same cascade then hits #65.

This is worth flagging because the repo allows all three merge methods and recent history uses a mix — #59, #57, #55, #53 landed as merge commits; #60, #56, #49, #48 landed with a single parent (squash or rebase). Either is normal here, so the choice is not obvious at merge time.

Recommended: merge-commit each, in order #63 → #64 → #65. Verified clean above.

If you prefer squash/rebase: it still works, but each child must be rebased onto main after its parent lands — two extra rebases, and #64/#65 will show conflicts until you do. Rebase-and-merge carries the same hazard as squash for the same reason (it rewrites SHAs even when the branch is already fast-forwardable).

No action needed from me either way. Flagging it now rather than letting it surface as a surprise mid-merge.

[pai-scaffolde]

Update: main moved, and the merge path now has a mandatory extra step

My earlier merge-order comment was computed against main = 54d19ec. main has since advanced to dd73d9c3 (3 gateway state.db commits) and #63 is now BEHIND. Re-verified everything below against the new head rather than assuming it carried over.

Good news: the stack still composes

The new commits and this stack touch completely disjoint files:

main 54d19ec..dd73d9c3 : gateway/run.py, hermes_cli/config_defaults.py,
                         tests/gateway/test_state_db_periodic_maintenance.py
stack #63..#65         : agent/cgroup_memory.py, agent/lsp/manager.py,
                         hermes_cli/main.py, tests/agent/lsp/test_client_cap.py,
                         tests/hermes_cli/test_tui_heap_sizing.py
overlap                : (none)

newmain + #63 -> exit 0
newmain + #64 -> exit 0
newmain + #65 -> exit 0

main's history is still fast-forward (no force-push), and #64 CI is still 25 pass / 0 fail / CLEAN.

The new constraint: main requires branches be up to date

Branch protection on main has required_status_checks.strict = true. So #63 cannot be merged while it is BEHIND — it has to be updated first. That step did not exist when I wrote the earlier comment.

Which update method you pick matters as much as the merge method:

Path A - "Update branch" (merge main into #63)
  simulated update commit -> then merge #64 : exit 0, 0 conflicts

Path B - "Update branch" (rebase #63 onto main)
  simulated update commit -> then merge #64 : exit 1, 2 conflicts
    CONFLICT (content): agent/lsp/manager.py
    CONFLICT (add/add): tests/agent/lsp/test_client_cap.py

Both produce a byte-identical tree (7b9c40042d8b306ea9a85befbf8e21756b52fddc). Path B conflicts purely because rebasing rewrites #63's commits into new SHAs, so they stop being ancestors and #64 re-applies them — the same failure mode as squashing, which I demonstrated earlier.

Recommended sequence

1. fix(lsp): bound concurrent language servers with an LRU cap (SCA-4389) #63: Update branch (merge) — not rebase — then merge with a merge commit.
2. fix(lsp): read the memory limit from this process's cgroup, not the hierarchy root (SCA-4623) #64 will then be BEHIND its base; same treatment, merge commit.
3. fix(cli): size the TUI V8 heap from this process's cgroup, not the hierarchy root (SCA-4627) #65 likewise.

Avoid squash and avoid rebase anywhere in this stack until all three have landed.

Also worth knowing

required_approving_review_count is null on main — protection does not require an approving review, only up-to-date branches plus the one required check. So this is not blocked on finding a reviewer; it is blocked on the update-and-merge sequence above.