perf(agent): reuse the per-request OpenAI wire client across sequential LLM calls

[Soju06]

Problem

Every LLM call builds a fresh openai.OpenAI wire client in _create_request_openai_client and closes it when the request finishes. Constructing the client allocates a new httpx connection pool, so every call pays a fresh TCP + TLS handshake to the provider.

Measured in a production deployment (instrumented client construction):

• client build: 19.2ms p50 / 35.5ms p95 per LLM call
• ~5.1 LLM calls per agent turn (tool loops make several sequential calls)
• warm reuse of an already-built client: 0.008–0.039ms
• net effect: ~75–80ms saved per typical tool-loop turn

The per-request client exists for good reasons (SDK max_retries=0, per-request vision headers, the #29507 cross-thread close discipline), so the fix keeps the per-request lifecycle semantics and only skips the redundant rebuild when nothing about the request's client configuration changed.

Change

Cache ONE reusable wire client on the agent, keyed by the effective client kwargs:

• _create_request_openai_client hands back the cached client when the effective kwargs are identical to the cached ones; any kwargs change (credential rotation, provider failover, vision headers) evicts the stale client and rebuilds.
• _close_request_openai_client keeps the client for the next call only when the close reason is one of _REQUEST_CLIENT_REUSE_REASONS (request_complete / stream_request_complete) — the reasons the request workers' own finally reports for a request that actually produced a response. Every other reason (error cleanups, stale/interrupt kills, retry cleanups) does a real close.
• The cache is a single checked-out slot (in_use): a second concurrent call gets a fresh untracked client with the old build-per-request lifecycle, so two calls never share one pool's close/abort lifecycle.
• release_clients() / close() really close the cached client via a new _close_cached_request_openai_client teardown hook.
• The MoA facade and Mock passthrough branches never enter the cache.

The callers in agent/chat_completion_helpers.py now report a reuse-eligible close reason only when the request produced a response; error unwinds report request_error_cleanup / stream_error_cleanup so a retry after a request error always builds a fresh pool.

Correctness notes

• max_retries=0 preserved. The cached client is built through the same request_kwargs path, and a kwargs mismatch (including max_retries) evicts. The agent's outer retry loop remains the only retry loop.
• Cross-thread abort discipline (Interrupted OpenAI/httpx request thread survives across turns and writes TLS record bytes to unrelated file descriptors on delayed close #29507). client.close() must only run on the thread that owns the pool's FDs. A stranger-thread abort (_abort_request_openai_client, used by the interrupt-check loop and the stale-call detector) only shutdown(SHUT_RDWR)s the sockets — and now also poisons the cache slot, so a pool whose sockets were shut down from another thread is never reused: the owner thread's close discards it and the next create rebuilds. Poisoning wins over a reuse-reason close.
• Atomic stranger abort. The holder read and the abort now happen under the request-client holder lock. Previously the abort fired after releasing the lock; in that window the worker's finally could pop + cache the client and the next call check it out, so the late abort would poison the slot and shut down an innocent in-flight request's sockets. The abort itself never blocks (socket shutdown + slot poison), so holding the lock across it only delays the racing pop, never the data path. The same discipline is applied to cron's inline direct_api_call abort.
• Interrupt-break stream leak. Breaking out of a half-read SSE stream on interrupt used to abandon the response with its connection still checked out of the httpx pool, while the partial response made the worker's finally report a reuse-reason close — caching the client together with the leaked connection (one more per interrupt until the pool exhausted and every request hit PoolTimeout). The stream is now closed on the break (owner thread); if that close fails, the slot is poisoned so the finally really closes the pool. The codex streaming runtime gets the same poison-on-close-failure treatment.
• Vision headers keyed separately. The copilot vision default_headers variant produces different effective kwargs, so a vision call after a text call (or vice versa) rebuilds rather than reusing the wrong headers. Nested kwargs dicts are snapshotted at cache time so an aliased inner dict mutated in place can't spuriously compare equal.
• Credential/base_url rotation busts the key. Rotation sites mutate _client_kwargs; the kwargs comparison fails and the stale client is evicted (closed from the creating thread, which is safe because in_use was false — no worker owns the pool's FDs).
• Error paths always rebuild. Only a clean response reports a reuse reason; API errors, cancel-swallows, stale kills, and interrupts all really close, so a retry never inherits a possibly-wedged pool (pinned by test_retry_after_api_connection_error_recreates_request_client).
• Teardown while checked out. If teardown runs while a worker has the client checked out (workers can outlive turns), the hook aborts the sockets instead of closing: the slot is already detached, so the worker's own finally sees an untracked client and does the real close on its thread.

Tests

New:

• tests/agent/test_request_client_reuse.py — reuse on identical kwargs, eviction on kwargs change, vision-header variant keying, poison-on-abort, non-reuse close reasons, teardown (idle and checked-out), single-slot concurrency, MoA/Mock passthrough.
• tests/run_agent/test_request_client_reuse_abort_races.py — atomic holder-abort races (streaming, non-streaming, and cron inline paths), interrupt-break stream close and poison-on-close-failure, codex stream close-failure poisoning.

Updated:

• tests/run_agent/test_openai_client_lifecycle.py — successful requests now keep the wire client cached (closed at teardown instead of request end); the retry-after-connection-error test still pins that error paths rebuild.

Ran tests/run_agent, tests/agent, tests/cron/test_cron_direct_api_call_62151.py, tests/hermes_cli/test_codex_runtime_switch.py, and tests/acp/test_session.py locally (Linux).

Measured impact

From a production deployment of this change: client construction cost dropped from 19.2ms p50 / 35.5ms p95 per LLM call to 0.008–0.039ms on reuse. At ~5.1 calls per turn this saves ~75–80ms of pure connection-setup latency per typical tool-loop turn, plus the provider-side benefit of a persistent connection across sequential calls.

🤖 Generated with Claude Code

🤖 Generated with Claude Code

[Soju06]

The failing Python tests slices here are test_model_validation.py::TestProbeApiModelsUserAgent — broken on main itself since b8eb89f (the tests mock urllib.request.urlopen, but the probe goes through open_credentialed_url's OpenerDirector). Test-only fix: #64200. Unrelated to this PR's diff.

[teknium1]

Thanks for the focused lifecycle work. Current main still allocates a fresh request-scoped OpenAI client on every call at run_agent.py:4215; its production non-streaming worker creates that client at agent/chat_completion_helpers.py:450-457 and closes it in the worker finally at agent/chat_completion_helpers.py:473. The PR therefore addresses a live path rather than a stale snapshot.

The cache remains limited to the existing request-client seam, retains the per-request max_retries=0 setup, and explicitly preserves the existing stranger-thread FD-ownership rule documented in run_agent.py:4220-4251 (introduced by 30c22f1158c001cf35ce4a2cb5d2dc188fe43066). The checked-out-slot, poison, eviction, and teardown cases are covered by the added targeted tests, and the PR's required CI checks passed.

Automated hermes-sweeper review.

[kshitijk4poor]

Merged via #73375 (rebase — your commit landed on main as 82e2c9c with your authorship preserved).

Review notes: the live before/after E2E reproduced your measurements exactly — 4 sequential calls went from 4 wire-client builds (28-42ms each) to 1 build + 3 reuse hits, and every negative path (kwargs eviction, error-path rebuild, streaming reuse, teardown close, cross-thread poison) checked out both in your test suite and in live verification. Exceptionally well-reasoned PR — the correctness notes matched the code exactly. Thank you!