fix(v1): interception server owns and multiplexes model clients - #2279
Merged
Conversation
Since #2218 each rollout built and closed its own httpx client, so a wide run churns TCP connections at the rollout rate — the load pattern that wedges a hyper-based vllm-router. Move client ownership to the interception server: one client per distinct endpoint config, assigned to each session at register and closed with the server. Rollouts multiplexed onto a server (multiplex, default 32) now share one bounded keepalive pool, so connections are reused warm instead of reopened per rollout — shared resources without unbounded fan-in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
ApprovabilityVerdict: Needs human review This PR refactors client ownership from individual rollouts to the interception server, changing connection lifecycle semantics (creation timing, sharing across rollouts, teardown timing). While well-structured for connection pooling efficiency, such resource management changes warrant human review to verify the new lifecycle is correct. You can customize Macroscope's approvability policy. Learn more. |
samsja
approved these changes
Aug 6, 2026
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
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
InterceptionServerkeeps one liveClientper distinct endpoint config (keyed by the config's JSON — configs carry an env-var name, never the key itself), assigns it to eachRolloutSessionatregister, and closes it with the server via its exit stack.RolloutSession.clientis no longer a constructor argument; the server sets it at registration.Rolloutno longer builds (resolve_client) or closes a client.multiplex, default 32) now share one keepalive connection pool: since each rollout issues turns sequentially, a server's client never holds more than ~multiplexrequests in flight — comfortably inside the existing transport limits — so upstream connections are reused warm instead of opened and torn down per rollout.TrainClientis safe to share: all per-turn state is local, and the renderer pool has been process-wide since feat: build one client per rollout, share renderers process-wide #2218.Context
#2218 made each rollout build and close its own httpx client. At ~768 concurrent rollouts that means constant TCP churn against the inference router — the load pattern implicated in the vllm-router wedges (router stops processing mid-stream, engine drains and idles, sessions hashed to it die at the solver timeout). This restores shared, bounded connection reuse while keeping the per-rollout client configs introduced there: the sharing rate is naturally the interception server's multiplex rate, with no new knob.
Note on cancellation: previously
Rollout.abort()closed its own client, killing any in-flight upstream request with it. The shared client stays open; in-flight handlers are cancelled viasession.release()(which already existed) and their connections return to the pool.Verification
uv run ruff check/ruff format --checkclean; full non-e2e v1 suite: 84 passed.test_single_turn,test_tool,test_interaction,test_multi_agent_envlegs pass, including the tunneledharness-in-subprocess-with-tool-in-dockerleg. The modal legs fail identically on unmodifiedmain(local modal environment issue, unrelated).🤖 Generated with Claude Code
Note
Medium Risk
Changes upstream connection lifecycle and abort/cancellation behavior on the inference path at high concurrency; mitigated by per-config sharing and existing session.release() for stragglers.
Overview
Moves live model
Clientownership from eachRollouttoInterceptionServer: the server keeps one resolved client per distinctBaseClientConfig(keyed by config JSON), assigns it onRolloutSessionregister, and closes clients with the server exit stack.Rollouts no longer call
resolve_clientorclient.close()inabort/close; multiplexed sessions on the same server reuse one upstream keepalive pool instead of per-rollout TCP churn.RolloutSession.clientis optional until registration.Docs/comments are updated across client config, GEPA runner,
TrainClient, and session to describe server-owned sharing. Cancellation no longer tears down a per-rollout client onabort; in-flight work is cancelled via existingsession.release()while the shared client stays open.Reviewed by Cursor Bugbot for commit 30de027. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Move model client ownership from
RollouttoInterceptionServerwith shared cachingInterceptionServernow owns model clients, caching them by serialized endpoint config in aclientsdict and closing them via its async exit stack on teardown._clienthelper onInterceptionServerreturns a cachedClientfor a givenBaseClientConfig, creating it viaresolve_clienton first use.InterceptionServer.registerassigns the server-owned client to theRolloutSession, replacing per-rollout client construction.Rolloutno longer creates or closes aClient;RolloutSession.clientis now optional (Noneuntil assigned at registration).ElasticRendererPoolslots rather than each maintaining their own.Macroscope summarized 30de027.