Skip to content

fix(auxiliary): close the underlying client of cached async wrappers - #67262

Open
Frowtek wants to merge 2 commits into
NousResearch:mainfrom
Frowtek:fix/aux-async-wrapper-close
Open

fix(auxiliary): close the underlying client of cached async wrappers#67262
Frowtek wants to merge 2 commits into
NousResearch:mainfrom
Frowtek:fix/aux-async-wrapper-close

Conversation

@Frowtek

@Frowtek Frowtek commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

AsyncCodexAuxiliaryClient, AsyncAnthropicAuxiliaryClient and AsyncBedrockAuxiliaryClient have no close(), while all three sync twins do.

That matters because the async shim is the object that gets cached: _to_async_client builds it from a sync wrapper that is a local transient, so no sync cache entry exists to release the transport later.

_close_cached_client reaches a client two ways, and a shim answers neither:

  • getattr(client, "close", None) — absent on the shims.
  • _force_close_async_httpx — looks for client._client, the httpx client inside an AsyncOpenAI. A shim only carries .chat, .api_key, .base_url and the mirrored ._real_client.

Both lookups miss, so closing a cached async wrapper is a complete no-op. Measured before the fix, on the same underlying client:

_close_cached_client(async wrapper) -> underlying closed: False
_close_cached_client(sync  wrapper) -> underlying closed: True

This silently defeats two documented behaviours: shutdown_cached_clients ("Close all cached clients (sync and async)") and _evict_cached_clients, which fires on every credential refresh — so each refresh leaks the transport it was meant to release, in exactly the long-running-gateway fd class _get_cached_client cites (#10200).

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/auxiliary_client.py — give each async wrapper the same close() as its sync twin, delegating to the already-mirrored _real_client. Bedrock stays a no-op (it builds per-call) but now answers the protocol. The methods are deliberately synchronous so the eviction guard that skips coroutine close() still runs them.
  • tests/agent/test_aux_async_wrapper_close.py — new: closing a cached async wrapper releases the underlying client, close() is not a coroutine, Bedrock answers the protocol without raising, and every sync close() has an async counterpart so the surface can't diverge again.

How to Test

  1. Run:

    pytest tests/agent/test_aux_async_wrapper_close.py -q
    

    All 6 tests fail without the fix and pass with it.

  2. No regressions: pytest tests/agent/ -k "auxiliary or aux_" gives 554 passed, 0 failed on clean main and 560 passed, 0 failed with this change (554 + the 6 added tests).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(auxiliary):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run the affected suites and verified no regressions against a clean main baseline
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (inline comments) — each new close() documents why the cached shim must release its own client
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — N/A (client lifecycle only)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

`AsyncCodexAuxiliaryClient`, `AsyncAnthropicAuxiliaryClient` and
`AsyncBedrockAuxiliaryClient` had no `close()`, while all three sync twins
do. That matters because the async shim is the object that gets CACHED:
`_to_async_client` builds it from a sync wrapper that is a local transient,
so no sync cache entry exists to release the transport later.

`_close_cached_client` reaches a client two ways, and a shim answers
neither:

  * `getattr(client, "close", None)` - absent on the shims.
  * `_force_close_async_httpx` - looks for `client._client`, the httpx
    client inside an `AsyncOpenAI`. A shim only carries `.chat`,
    `.api_key`, `.base_url` and the mirrored `._real_client`.

Both lookups miss, so closing a cached async wrapper is a complete no-op.
Measured before the fix, on the same underlying client:

    _close_cached_client(async wrapper) -> underlying closed: False
    _close_cached_client(sync wrapper)  -> underlying closed: True

That silently defeats two documented behaviours: `shutdown_cached_clients`
("Close all cached clients (sync and async)") and `_evict_cached_clients`,
which fires on every credential refresh - so each refresh leaks the
transport it was meant to release, in exactly the long-running-gateway fd
class `_get_cached_client` cites (NousResearch#10200).

Give each async wrapper the same `close()` as its sync twin, delegating to
the already-mirrored `_real_client` (Bedrock stays a no-op - it builds
per-call - but now answers the protocol). The methods are deliberately
synchronous so the eviction guard that skips coroutine `close()` still runs
them.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the cached-wrapper lifecycle; the Codex and Anthropic cases are real on the inspected checkout.

Problems

  • agent/auxiliary_client.py:4597-4600 also creates AsyncGeminiNativeClient for native Gemini. Its close() is a coroutine (agent/gemini_native_adapter.py:1034), while the canonical cache closer intentionally skips coroutine close methods (agent/auxiliary_client.py:6022-6024). Native Gemini cached async transports therefore retain the same shutdown/credential-eviction leak.
  • The new “every sync wrapper” parity test omits the Gemini pair at tests/agent/test_aux_async_wrapper_close.py:85-92, so it cannot catch that sibling path.

Suggested changes

  • Add a safe synchronous cache-close path for native Gemini and exercise it through the actual cached shutdown or eviction flow.

Automated hermes-sweeper review.


def test_async_wrappers_mirror_the_sync_close_surface():
"""Every sync wrapper's close() must have an async counterpart."""
pairs = [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parity list omits GeminiNativeClient / AsyncGeminiNativeClient. _to_async_client creates that wrapper (agent/auxiliary_client.py:4597-4600), but its close() is coroutine-based (agent/gemini_native_adapter.py:1034) and _close_cached_client skips coroutine closers (agent/auxiliary_client.py:6022-6024). Include a synchronous cache-close solution and a Gemini pair here so the same leak is covered.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this targets cached async wrapper teardown, while #38787 and #41243 address broader auxiliary-client/fd-reclaim paths. The overlap is complementary rather than a duplicate.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 19, 2026
_to_async_client also wraps GeminiNativeClient in AsyncGeminiNativeClient,
and that shim leaks its transport for the same reason as the other three —
by a different path.

Its close() exists, but it is a coroutine. _close_cached_client runs from
synchronous callers (CLI shutdown, credential-refresh eviction) with no loop
to await on, so the iscoroutinefunction guard deliberately skips it. The shim
also carries no _client, so _force_close_async_httpx finds nothing either.
Both lookups miss and the native-Gemini transport survives every shutdown and
every credential rotation.

Measured through the real cache flows before this change:

    shutdown_cached_clients -> leaf closed: False
    _evict_cached_clients   -> leaf closed: False

Fall back to the leaf the shim already mirrors as _real_client for
eviction-by-leaf (NousResearch#23482). GeminiNativeClient.close() IS synchronous, so the
sync closer can release it directly. The public async close() is untouched
and still awaits.

Tests: drive the actual cached shutdown and credential-eviction flows (both
fail without the fallback), assert the async close() still awaits, add the
Gemini pair to the sync/async parity list, and add a reachability invariant —
a cacheable wrapper must expose either a synchronous close() or a mirrored
_real_client leaf, otherwise the canonical closer silently does nothing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants