Skip to content

fix(cli): size the TUI V8 heap from this process's cgroup, not the hierarchy root (SCA-4627) - #65

Merged
pai-scaffolde merged 1 commit into
mainfrom
fix/sca-4627-tui-heap-cgroup
Aug 20, 2026
Merged

fix(cli): size the TUI V8 heap from this process's cgroup, not the hierarchy root (SCA-4627)#65
pai-scaffolde merged 1 commit into
mainfrom
fix/sca-4627-tui-heap-cgroup

Conversation

@pai-scaffolde

Copy link
Copy Markdown
Collaborator

Stacked on #64 (which is stacked on #63). Review only the two commits above #64's tip; the base will collapse to main as the stack merges.

The defect

Codex flagged this as P1 during review of #64. hermes_cli/main.py _read_cgroup_memory_limit() read the identical two hierarchy roots that SCA-4623 just fixed in agent/lsp/manager.py:

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

Those are this process's limit only when it sits in the root cgroup. Under a systemd unit with MemoryMax=, or in a container without a private cgroup namespace, the root reads max and the function returns None.

Why it is worse than the LSP cap

_resolve_tui_heap_mb() then returns the flat default_mb = 8192. V8 is not cgroup-aware, so inside a 4 GiB unit the Node TUI grows its heap toward 8 GB until the cgroup OOM killer SIGKILLs it. Per that function's own docstring, the kill runs no JS handler, writes no [tui-parent] breadcrumb, and the user sees only a bare gateway stdin EOF.

SCA-4623's defect degraded to pre-cap sizing. This one is a silent kill with no diagnostic.

The change

Two commits, deliberately split:

  1. refactor(cgroup) — pure move of the resolver out of agent/lsp/manager.py into a new stdlib-only agent/cgroup_memory.py, matching the flat single-purpose convention of agent/battery.py. manager.py keeps host_memory_bytes() and delegates. No behaviour change.
  2. fix(cli)_read_cgroup_memory_limit() becomes a delegation to that module.

hermes_cli deliberately does not import agent.lsp.manager: LSP is unrelated to heap sizing and that module pulls asyncio plus the whole LSP client stack onto a CLI startup path. The new module imports in 9 ms with no asyncio, logging, or threading behind it (measured, not asserted).

cgroup_memory_limit_bytes (no underscore) is the single name crossing a module boundary; the resolution helpers stay private.

Positive control

Required, as in #64. Four new tests over a fabricated /proc + cgroupfs tree — hierarchy root at max, the real limit on the unit's own cgroup — each shown failing against the fixed-path implementation before the fix:

Test Failure before
test_unit_limit_is_read_not_the_hierarchy_root assert None == 4 GiB
test_heap_is_sized_below_the_unit_limit_not_the_flat_default assert 8192 == 3072
test_an_ancestor_limit_binds_the_unit assert None == 2 GiB
test_absent_proc_still_falls_back_to_the_fixed_paths assert None == 6 GiB

assert 8192 == 3072 is the defect itself: the heap V8 would have been handed inside a unit that cannot afford it.

Test trap closed

tests/hermes_cli/test_tui_heap_sizing.py mocked builtins.open and passed unrecognised paths through to the real open. Once resolution consults /proc, those fixed-path tests would have read the real /proc on Linux CI and started reporting the runner's own cgroup. _read now pins CGROUP_FS_ROOT at an empty root first — the same guard no_proc() gives the LSP suite in tests/agent/lsp/test_client_cap.py.

The cgroup tests in test_client_cap.py move their patch targets to the new module alongside the code. Same tests, same assertions, no coverage lost.

Class sweep

git grep -n "sys/fs/cgroup" -- '*.py' '*.ts' '*.js' '*.sh' — 3 readers; 2 fixed, 1 tombstoned:

Site Status
agent/lsp/manager.py fixed in #64, now delegates
hermes_cli/main.py this PR
gateway/cgroup_cleanup.py not a sibling — builds /sys/fs/cgroup{cgroup_path}/cgroup.procs from a caller-supplied path rather than assuming the root. Tombstoned.

Verification

  • tests/hermes_cli/test_tui_heap_sizing.py — 9 passed
  • tests/agent/lsp/ — 104 passed (extraction is behaviour-preserving)
  • tests/tools/test_docker_environment.py + tests/hermes_cli/test_agent_import.py — 99 passed
  • ruff check clean; ty check clean on both changed source modules

Closes SCA-4627.

pai-scaffolde pushed a commit that referenced this pull request Aug 10, 2026
…628)

GitHub reported #65 and #66 both CLEAN. Both branch off #63 and diverge
there, so each was measured against its own base and neither was ever
measured against the other. A local merge simulation of the real queue
shows they collide on agent/lsp/manager.py in either order, so no merge
ordering alone lands both.

The collision is add/delete, not semantic: #65 moved
CGROUP_MEMORY_LIMIT_PATHS and _cgroup_memory_limit_bytes out of
manager.py into the new agent/cgroup_memory.py, while #66 branched from
#63 (where they still lived) and added EVICTION_HANDOFF_BUDGET beside
them.

Resolution keeps both contributions: EVICTION_HANDOFF_BUDGET stays, the
relocated cgroup block is dropped in favour of the agent.cgroup_memory
import #65 introduced. tests/agent/lsp/test_client_cap.py auto-merged
onto #65's cgroup_mod form and carries no stale manager_mod references.
Tuple stays imported (still used at 8 sites), so no orphaned import.

Verified locally: no conflict markers, manager.py compiles, both
EVICTION_HANDOFF_BUDGET call sites and the cgroup_memory import resolve.
Test execution is left to CI — this host is at 93% disk with the
self-hosted runner already disk-suspended (SCA-4625), so standing up a
pinned uv env here would spend the exact resource that is scarce.
pai-scaffolde pushed a commit that referenced this pull request Aug 10, 2026
…(SCA-4633 class)

Second instance of the same class as #66: GitHub reported #62 and #63
both CLEAN because each was measured only against main, never against
each other. A merge simulation of the real queue shows they collide on
agent/lsp/manager.py, so the queue could not land in any order.

#62 is the cheaper side to absorb: it is a leaf with no dependents,
while #63 is the base of a four-PR stack (#64, #65, #66), so resolving
on #63 would force a re-merge and a fresh CI run on all four.

Resolution takes both sides rather than either: _last_used keeps #62's
_idle_clock() and the stack's second protected cap sweep is preserved.

The conflicted hunk was not the whole risk. #63 added new _last_used
write sites that git auto-merged with no conflict, and a merge that
resolved only the marked hunk would have silently reinstated the wall
clock on those paths and quietly undone #62. Audited the merged tree:
all three _last_used writes (747, 809, 1090) and the reaper cutoff
(1107) use _idle_clock(), and no time.time() remains in manager.py.
The handoff deadline keeps time.monotonic() directly, which is correct
for an elapsed-time budget.

Verified locally: no conflict markers, manager.py compiles. Test
execution is left to CI — this host is at 93% disk with the self-hosted
runner disk-suspended (SCA-4625).
@pai-scaffolde
pai-scaffolde changed the base branch from fix/sca-4623-cgroup-self-path to main August 20, 2026 01:58
…erarchy root (SCA-4627)

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
pai-scaffolde force-pushed the fix/sca-4627-tui-heap-cgroup branch from e7782f7 to 3f32737 Compare August 20, 2026 02:59
@pai-scaffolde
pai-scaffolde merged commit 2aa7f76 into main Aug 20, 2026
37 checks passed
@pai-scaffolde
pai-scaffolde deleted the fix/sca-4627-tui-heap-cgroup branch August 20, 2026 03:04
pai-scaffolde added a commit that referenced this pull request Aug 20, 2026
… (#81)

PR #64 was replayed onto main with the wrong merge base (its parent
branch's TIP rather than the true fork point). Files main had gained
after that branch forked therefore looked like deletions, so merging #64
silently reverted #79:

  * gateway/run.py — _run_state_db_maintenance_once() refactor undone
  * hermes_cli/config_defaults.py — housekeeping comment reverted
  * tests/gateway/test_state_db_periodic_maintenance.py — deleted

This re-applies #79 verbatim onto current main. The LSP work from #63
and #64 (including sessions.max_clients) is untouched. Audited: #63 and
#65 match their original diffstats exactly; #64 was the only bad replay.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant