From 961b0d0a57914f2a4656d9ae37c804d7e0a993c3 Mon Sep 17 00:00:00 2001 From: Ne0teric Date: Fri, 17 Jul 2026 23:53:33 -0700 Subject: [PATCH] fix(tui): spawn slash workers on demand instead of one per session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every slash_worker child runs its own MCP discovery (#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. --- tests/test_tui_gateway_server.py | 81 +++++++++++++++++++++++--------- tui_gateway/server.py | 50 +++++++++----------- 2 files changed, 82 insertions(+), 49 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 71d709f5dc6a..fb57ab531774 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -7129,7 +7129,9 @@ def _fake_sync(_sid, _session): # --------------------------------------------------------------------------- # session.create / session.close race: fast /new churn must not orphan the -# slash_worker subprocess or the global approval-notify registration. +# global approval-notify registration. (Slash workers are no longer pre-warmed +# by the build thread — slash.exec spawns them on demand — so the build thread +# must ALSO never construct one here.) # --------------------------------------------------------------------------- @@ -7137,12 +7139,13 @@ def _fake_sync(_sid, _session): def test_session_create_close_race_does_not_orphan_worker(monkeypatch): """Regression guard: if session.close runs while session.create's _build thread is still constructing the agent, the build thread - must detect the orphan and clean up the slash_worker + notify - registration it's about to install. Without the cleanup those - resources leak — the subprocess stays alive until atexit and the - notify callback lingers in the global registry.""" + must detect the orphan and unregister the notify registration it's + about to install. It must also never pre-warm a slash worker (each + worker forks the full stdio MCP fleet; spawn is on-demand in + slash.exec) — a worker constructed here would be a regression.""" import threading + created_workers: list[str] = [] closed_workers: list[str] = [] unregistered_keys: list[str] = [] @@ -7150,6 +7153,7 @@ class _FakeWorker: def __init__(self, key, model, profile_home=None): self.key = key self._closed = False + created_workers.append(key) def close(self): self._closed = True @@ -7233,23 +7237,24 @@ def _slow_make_agent(sid, key, session_id=None, session_db=None, **_kwargs): ) assert close_resp.get("result", {}).get("closed") is True - # At this point session.close saw slash_worker=None (not yet - # installed) so it didn't close anything. Release the build thread - # and let it finish — it should detect the orphan and clean up the - # worker it just allocated + unregister the notify. + # At this point session.close saw slash_worker=None (never eagerly + # installed) so it had nothing to close. Release the build thread + # and let it finish — it should detect the orphan and unregister + # the notify, without ever having constructed a worker. release_build.set() # Give the build thread a moment to run through its finally. for _ in range(100): - if closed_workers: + if unregistered_keys: break import time time.sleep(0.02) - assert ( - len(closed_workers) == 1 - ), f"orphan worker was not cleaned up — closed_workers={closed_workers}" + assert created_workers == [], ( + f"build thread pre-warmed a slash worker (spawn must stay on-demand " + f"in slash.exec) — created_workers={created_workers}" + ) # Notify may be unregistered by both session.close (unconditional) # and the orphan-cleanup path; the key guarantee is that the build # thread does at least one unregister call (any prior close @@ -7263,8 +7268,9 @@ def _slow_make_agent(sid, key, session_id=None, session_db=None, **_kwargs): @pytest.mark.real_agent_prewarm def test_session_create_no_race_keeps_worker_alive(monkeypatch): """Regression guard: when session.close does NOT race, the build - thread must install the worker + notify normally and leave them - alone (no over-eager cleanup).""" + thread must install the notify normally and leave it alone (no + over-eager cleanup) — and must not pre-warm a slash worker (spawn + is on-demand in slash.exec).""" closed_workers: list[str] = [] unregistered_keys: list[str] = [] @@ -7347,8 +7353,9 @@ def __init__(self): own_unregistered == [] ), f"build thread unregistered its own notify despite no race: {own_unregistered}" - # Session should have the live worker installed. - assert session.get("slash_worker") is not None + # No pre-warmed worker: slash.exec spawns on demand, so a fresh + # session that hasn't run a worker-routed command carries None. + assert session.get("slash_worker") is None finally: # Cleanup + restore sibling sessions we snapshotted. server._sessions.clear() @@ -10053,7 +10060,8 @@ def close(self): def test_restart_slash_worker_closes_orphan_when_session_reaped(monkeypatch): """Post-turn restart of a session reaped mid-flight (e.g. close_on_disconnect - fired while `running` flipped false) must close the fresh worker, not orphan it.""" + fired while `running` flipped false) must close both the stale worker and + the fresh replacement, not orphan either.""" closed = [] class _FakeWorker: @@ -10065,11 +10073,14 @@ def close(self): monkeypatch.setattr(server, "_SlashWorker", _FakeWorker) server._sessions.pop("reaped", None) - reaped = {"session_key": "k"} # not in _sessions -> torn down concurrently + # not in _sessions -> torn down concurrently; carries a live worker so the + # restart path actually runs (a workerless session is a restart no-op now) + reaped = {"session_key": "k", "slash_worker": _FakeWorker()} server._restart_slash_worker("reaped", reaped) - assert closed == [True] - assert reaped.get("slash_worker") is None + # stale worker closed by the restart, fresh worker closed by _attach_worker + # (sid no longer maps to this session) + assert closed == [True, True] assert "reaped" not in server._sessions @@ -10082,15 +10093,41 @@ def close(self): pass monkeypatch.setattr(server, "_SlashWorker", _FakeWorker) - live = {"session_key": "k", "slash_worker": None} + old_worker = _FakeWorker() + live = {"session_key": "k", "slash_worker": old_worker} server._sessions["live-restart"] = live try: server._restart_slash_worker("live-restart", live) assert isinstance(live["slash_worker"], _FakeWorker) + assert live["slash_worker"] is not old_worker finally: server._sessions.pop("live-restart", None) +def test_restart_slash_worker_noop_without_worker(monkeypatch): + """A session that never spawned a worker (slash.exec not used yet) must + stay workerless across a restart — spawning here would fork the per-worker + stdio MCP fleet for sessions that never run worker-routed commands.""" + spawned = [] + + class _FakeWorker: + def __init__(self, *a, **k): + spawned.append(True) + + def close(self): + pass + + monkeypatch.setattr(server, "_SlashWorker", _FakeWorker) + live = {"session_key": "k", "slash_worker": None} + server._sessions["lazy-noop"] = live + try: + server._restart_slash_worker("lazy-noop", live) + assert spawned == [] + assert live["slash_worker"] is None + finally: + server._sessions.pop("lazy-noop", None) + + def test_session_close_rpc_claims_then_tears_down(monkeypatch): seen = [] claimed = {"session_key": "k"} diff --git a/tui_gateway/server.py b/tui_gateway/server.py index fc766f9f7470..7c50e1689fd8 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -1618,15 +1618,15 @@ def _build() -> None: # override is still active here. current["config_model_seen"] = _config_model_target() - try: - worker = _SlashWorker( - key, - getattr(agent, "model", _resolve_model()), - profile_home=current.get("profile_home"), - ) - _attach_worker(sid, current, worker) - except Exception: - pass + # No eager slash-worker pre-warm: slash.exec spawns one on demand + # (its error path already relies on that respawn to recover from a + # dead worker). Each worker child runs its own MCP discovery + # (#61891), so pre-warming one per session forks the full stdio + # MCP fleet — ~20 OS processes per retained session on a config + # with a few stdio servers — even for sessions that never run a + # worker-routed command. Sessions held by a live transport are + # never reaped, so with the desktop app open for days those + # fleets accumulate until the OS refuses new process spawns. try: from tools.approval import ( @@ -3092,11 +3092,16 @@ def _tool_progress_enabled(sid: str) -> bool: def _restart_slash_worker(sid: str, session: dict): worker = session.get("slash_worker") - if worker: - try: - worker.close() - except Exception: - pass + # A session that never spawned a worker has nothing stale to replace — + # the next slash.exec builds one with the current session key/model. + # Spawning here would fork the per-worker stdio MCP fleet for sessions + # that never use worker-routed commands. + if worker is None: + return + try: + worker.close() + except Exception: + pass try: new_worker = _SlashWorker( session["session_key"], @@ -5162,19 +5167,10 @@ def _init_session( except Exception: logger.debug("failed to persist resumed session cwd", exc_info=True) _register_session_cwd(_sessions[sid]) - try: - _attach_worker( - sid, - _sessions[sid], - _SlashWorker( - key, - getattr(agent, "model", _resolve_model()), - profile_home=_sessions[sid].get("profile_home"), - ), - ) - except Exception: - # Defer hard-failure to slash.exec; chat still works without slash worker. - _sessions[sid]["slash_worker"] = None + # No eager slash-worker pre-warm — the session dict already carries + # slash_worker=None and slash.exec builds one on demand. See the + # deferred-build path in _start_agent_build for the full rationale + # (per-worker MCP fleets accumulating across retained sessions). try: from tools.approval import register_gateway_notify, load_permanent_allowlist