fix(tui): spawn slash workers on demand instead of one per session - #71006
Merged
Conversation
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.
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.
alt-glitch
added
type/bug
Contributor
૮ >ﻌ< ა ci reviewran on 74dc090 all good! |
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
3 participants
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.
Summary
The TUI no longer forks one _SlashWorker (plus MCP fleet) per session eagerly — workers spawn on first slash use, fixing the ~120-process / ~50GB OOM reports.
Changes
Validation
899 targeted tests green; ruff clean.
Infographic