fix(auxiliary): close evicted clients in cache to prevent fd leaks

[annguyenNous]

What does this PR do?

Fixes three resource leaks in the auxiliary client cache (agent/auxiliary_client.py) where evicted or discarded OpenAI/httpx clients are never closed, leaking sockets and file descriptors.

Bug 1 — _evict_cached_client_instance skips cleanup (line 2585)

When a specific cached client is poisoned (connection error, timeout), _evict_cached_client_instance removes it from the cache dict but never calls .close() on the evicted client. The by-provider variant _evict_cached_clients correctly calls both _force_close_async_httpx and .close() — the instance variant must match.

Impact: Every connection-error eviction leaks one httpx transport (TCP connection pool + sockets). In long-running gateways with retry/fallback logic, this accumulates until fd exhaustion.

Bug 2 — TOCTOU race in _get_cached_client (line 4567)

resolve_provider_client() runs outside the cache lock (line 4531 releases it). Two concurrent threads can both pass the cache_key not in _client_cache check, both create new clients, and the second thread's client is silently discarded without close at line 4568.

Fix: Close the redundant client before replacing the reference.

Bug 3 — FIFO eviction skips sync .close() (line 4562)

The cache-size cap eviction calls _force_close_async_httpx (which only marks httpx state as CLOSED) but does NOT call .close() on sync OpenAI clients. Compare with shutdown_cached_clients() which correctly calls .close() on both sync and async clients.

Testing

python3 -c "import ast; ast.parse(open('agent/auxiliary_client.py').read()); print('Syntax OK')"

Related

• Bug: auxiliary_client._client_cache accumulates AsyncOpenAI clients indefinitely, causing fd exhaustion in long-running gateway #10200 — original fd-exhaustion report that motivated the client cache
• Auxiliary compression timeout can poison cached sync client, causing later auxiliary calls to fail #23432 — connection-error eviction path
• fix(auxiliary): evict async wrappers on poisoned client (follow-up to #23482) #23606 — async wrapper eviction (closed without merge, different scope)

[teknium1]

Thanks for identifying the three cache-disposal paths; all three remain present on current main (agent/auxiliary_client.py:3258-3260, :5810-5848).

Problems

• The added unguarded close() calls do not preserve the current async-client rule. Standard async auxiliary clients are created as AsyncOpenAI at agent/auxiliary_client.py:4306-4377, while shutdown_cached_clients() deliberately calls close() only when it is not a coroutine function (:5682-5690). The new paths should use that same distinction after _force_close_async_httpx.
• This PR modifies only agent/auxiliary_client.py; it adds no regression tests. Existing eviction tests at tests/agent/test_auxiliary_client.py:4526-4607 assert removal only, and the FIFO test at tests/run_agent/test_async_httpx_del_neuter.py:256-293 inlines cache manipulation rather than exercising _get_cached_client.

Suggested changes

• Factor the existing sync-safe disposal behavior into a helper and use it for all three paths.
• Add tests for synchronous closure on instance/FIFO/race-loser eviction, plus an async-close guard case.

Automated hermes-sweeper review.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address auxiliary-client cache disposal leaks. #38787 covers instance eviction, FIFO cache-cap eviction, and the concurrent race-loser path, while #41144 addresses only instance eviction; both currently call close without correctly handling all asynchronous clients or wrappers.

Related pull requests

• #38787 related — (+25/-0) — keep open and revise: The diff covers all three identified leak paths, but its unguarded close calls can invoke coroutine-based AsyncOpenAI.close() without awaiting it, and it adds no regression tests. Consistent with the contributor keep_open review, disposal should be centralized in a sync-safe, owner-aware helper and tested for instance, FIFO, and race-loser eviction.
• #41144 duplicate — (+7/-0) — duplicate scope, incomplete implementation: The diff fixes only direct instance eviction and substantially overlaps #38787, but it does not dispose AsyncCodexAuxiliaryClient._real_client, does not cover cache-cap eviction, and adds no cleanup regression test. Consistent with the keep_open review on #41144, the underlying gap is real, but this narrower patch should be superseded by the comprehensive corrected implementation in #38787.

Duplicates

#41144 duplicates the instance-eviction portion of #38787; #38787 additionally covers FIFO cache-cap eviction and the concurrent redundant-client path.

Suggested consolidation

Merge #38787 only after addressing its contributor review: introduce owner-aware, sync-safe disposal that avoids unawaited async close calls and closes shared real clients exactly once, then add direct, wrapper, FIFO, and race-loser regression tests. Once that corrected implementation is verified, #41144 can be closed as a narrower duplicate of #38787; neither current diff should be merged over the documented contributor objections.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup38787 ["PRs duplicating each other"]
        P38787["PR #38787 (open)"]
        P41144["PR #41144 (open)"]
    end
    class P38787 open
    class P41144 open
    class P38787 target
    click P38787 "https://github.com/NousResearch/hermes-agent/pull/38787"
    click P41144 "https://github.com/NousResearch/hermes-agent/pull/41144"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 3 kB of PR diffs, 3 kB of issue/PR text, 3 kB of discussion (3 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.