fix(tui): spawn slash workers on demand instead of one per session - #66783
fix(tui): spawn slash workers on demand instead of one per session#66783Ne0teric wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
{
"event": "COMMENT",
Code Review Summary\n\nVerdict: Approved\n\nLooks good. No obvious issues found.\n\n---\nReviewed by Hermes Agent",
"comments": []
}
|
Thanks for the focused TUI resource-lifecycle fix. Current I found no substantive correctness or design-fit issue in the proposed scope. GitHub reports this branch as conflicting with current Automated hermes-sweeper review. |
Every slash_worker child runs its own MCP discovery (NousResearch#61891), which forks the full configured stdio MCP fleet — on a config with a handful of stdio servers that is ~20 OS processes per worker once npx/cmd wrappers are counted. The gateway pre-warmed a worker for every session at create/build time, and sessions held by a live transport are (by design) never reaped, so a desktop app left open for days accumulates one fleet per retained session. On a real setup this reached ~120 processes across 6 sessions and pushed Windows commit charge to the point where CreateProcess started failing system-wide ("Not enough memory resources are available to process this command"). slash.exec already spawns a worker on demand when the session has none and already recovers from a dead worker the same way, so the eager pre-warm is pure pre-warming: - drop the pre-warm in the deferred session-build path - drop the pre-warm in _init_session - make _restart_slash_worker a no-op for sessions that never spawned a worker (the next slash.exec builds one with the current session key/model, so no stale-key worker can exist) Only sessions that actually run a worker-routed slash command now pay for a fleet. Cost: the first such command in a session takes the CLI build + MCP discovery hit that session.create used to absorb. Tests: the two create/close-race guards now assert the build thread never constructs a worker (the notify-unregister guarantees are kept); the restart-orphan guard seeds a live worker so the close path is still exercised; new test pins the restart no-op for workerless sessions.
1f0de39 to
961b0d0
Compare
|
Rebased onto current |
|
Independent macOS incident evidence strongly confirms the failure mode and impact described here. On a 48 GB Apple Silicon Mac, Hermes Desktop was left open overnight and macOS reported the application above 50 GB before the machine froze. The system jetsam snapshot attributed 49.69 GiB current / 57.23 GiB lifetime-max to the Hermes coalition:
The desktop backend's final disconnect diagnostic reported I independently reproduced and tested this PR's fix shape on current Validation on current main:
One MCP integration cleanup test failed because the live-system test guard refused to terminate its own reparented subprocess; it fails identically on an untouched detached This is not just idle-memory optimization. It prevents a confirmed system-wide out-of-memory failure on macOS, and it directly addresses the process multiplication seen in the diagnostic report. |
With the eager pre-warm removed (PR #66783), slash.exec is the only spawn path — and it runs on the RPC thread pool, so two concurrent worker-routed commands on a fresh session could both see slash_worker=None and each fork a full stdio-MCP-fleet worker (the _attach_worker race loser leaking unclosed). Add a per-session spawn lock with a double-check, plus a regression test racing two slash.exec calls through handle_request. Also maps Ne0teric's contributor email.
With the eager pre-warm removed (PR #66783), slash.exec is the only spawn path — and it runs on the RPC thread pool, so two concurrent worker-routed commands on a fresh session could both see slash_worker=None and each fork a full stdio-MCP-fleet worker (the _attach_worker race loser leaking unclosed). Add a per-session spawn lock with a double-check, plus a regression test racing two slash.exec calls through handle_request. Also maps Ne0teric's contributor email.
|
Merged via #71006 with your authorship preserved — on-demand spawn shipped plus a per-session spawn lock on top (two concurrent slash commands could double-fork MCP fleets). The OOM reports in your thread made the impact case. Thanks. |
With the eager pre-warm removed (PR NousResearch#66783), slash.exec is the only spawn path — and it runs on the RPC thread pool, so two concurrent worker-routed commands on a fresh session could both see slash_worker=None and each fork a full stdio-MCP-fleet worker (the _attach_worker race loser leaking unclosed). Add a per-session spawn lock with a double-check, plus a regression test racing two slash.exec calls through handle_request. Also maps Ne0teric's contributor email.
Problem
Every
slash_workerchild runs its own MCP discovery (#61891), which forks the full configured stdio MCP fleet — with a handful of stdio servers that's ~20 OS processes per worker once the npx/cmd wrappers are counted. The gateway pre-warms a worker for every session at create/build time, and sessions held by a live transport are (correctly) never reaped — so a desktop app left open for days accumulates one full fleet per retained session.On a real setup (Windows, 5 stdio MCP servers, desktop app open ~2 days) this reached ~120 processes across 6 retained sessions, pushed system commit charge to ~90%, and Windows began failing unrelated process spawns system-wide with "Not enough memory resources are available to process this command".
Fix
slash.execalready spawns a worker on demand when the session has none, and its error path already recovers from a dead worker the same way — the eager pre-warm is pure pre-warming. This PR:_start_agent_build)_init_session_restart_slash_workera no-op for sessions that never spawned a worker — the nextslash.execbuilds one with the current session key/model, so no stale-key worker can exist for themOnly sessions that actually run a worker-routed slash command now pay for a fleet. Trade-off: the first worker-routed command in a session takes the CLI-build + MCP-discovery hit that
session.createused to absorb (a few seconds on a config with several stdio servers).Tests
test_restart_slash_worker_closes_orphan_when_session_reapedseeds a live worker so the reap-race close path is still exercised (stale + fresh both closed).test_restart_slash_worker_noop_without_workerpins the no-op.tests/test_tui_gateway_server.py,tests/test_lazy_session_regressions.py,tests/tui_gateway: 745 passed; the 4 failures present are identical on cleanmainin the same environment (pre-existing, unrelated).🤖 Generated with Claude Code