fix(auxiliary): evict async wrappers on poisoned client (follow-up to #23482) - #23606
Closed
wuli666 wants to merge 1 commit into
Closed
fix(auxiliary): evict async wrappers on poisoned client (follow-up to #23482)#23606wuli666 wants to merge 1 commit into
wuli666 wants to merge 1 commit into
Conversation
…ousResearch#23482) NousResearch#23482 fixed cache poisoning in the sync path: when a Codex auxiliary timeout closes the underlying OpenAI client, _evict_cached_client_instance walks CodexAuxiliaryClient wrappers via their _real_client attribute and drops the cache entry so the next aux call rebuilds. The cache key includes async_mode (see _client_cache_key), so the sync and async clients for the same provider live in two distinct entries pointing at the same underlying transport. The fix walked the sync wrapper's _real_client correctly but the async wrappers (AsyncCodexAuxiliaryClient, AsyncAnthropicAuxiliaryClient, AsyncGeminiNativeClient) never exposed _real_client at all, so the async entry survived eviction and kept handing out the poisoned client. Effect on async aux callers: one timeout now poisons every subsequent async aux call (compression, vision, session_search, title_generation) with 'Connection error' until gateway restart -- even while the sync route recovered as designed in NousResearch#23482. Mirror the sync wrapper's _real_client onto each async wrapper so the existing eviction helper finds them. Three changes, one per wrapper: - AsyncCodexAuxiliaryClient: self._real_client = sync_wrapper._real_client (the underlying OpenAI client) - AsyncAnthropicAuxiliaryClient: same shape - AsyncGeminiNativeClient: self._real_client = sync_client (Gemini's native facade is itself the leaf; no OpenAI client beneath it) Update _evict_cached_client_instance docstring to reflect that it now covers both sync and async wrappers via the same attribute walk. Test: TestAuxiliaryClientPoisonedCacheEviction.test_evict_cached_client_instance_walks_async_wrapper seeds both sync and async cache entries pointing at the same leaf and asserts both are dropped on a single eviction call. Verified the test fails without the wrapper changes ("async cache entry survived eviction -- wrapper is missing _real_client") and passes with them. Refs NousResearch#23482, NousResearch#23432
Contributor
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.
What does this PR do?
Closes the async-side gap left by #23482. That fix solved cache poisoning on the sync auxiliary path — which is what the original #23432 reporter reproduced via
context_compressor.py→call_llm. The async side has the identical bug classbut was not covered.
The cache key includes
async_mode(see_client_cache_key), so the sync and async clients for the same provider live in two distinct cache entries pointing at the same underlying transport. The #23482 fix walked the sync wrapper's_real_clientcorrectly, but the async wrappers —AsyncCodexAuxiliaryClient,AsyncAnthropicAuxiliaryClient,AsyncGeminiNativeClient— never exposed_real_client, so the async entry survived eviction and kept handing out the poisonedclient.
Async-only callers that are NOT covered by #23482:
tools/vision_tools.py(5 call sites)vision_analyzefor image inputstools/session_search_tool.pysession_searchover chat historytools/web_tools.pyagent/plugin_llm.pyplugins/teams_pipeline/pipeline.pytrajectory_compressor.pycontext_compressor)For any of these, one timeout permanently poisons every subsequent async aux call with
Connection erroruntil gateway restart — even while the sync route recovered as designed in #23482.Fix
Mirror the sync wrapper's
_real_clientonto each async wrapper so the existing eviction helper finds them. Three lines, one per wrapper:AsyncCodexAuxiliaryClientself._real_client = sync_wrapper._real_clientAsyncAnthropicAuxiliaryClientAsyncGeminiNativeClientself._real_client = sync_clientGeminiNativeClientdirectlyPlus a one-line docstring update on
_evict_cached_client_instanceto reflect that it now covers both sync and async wrappers via the same attribute walk.Why this approach (vs. modifying the eviction helper)
The async wrappers already store enough to reach the leaf (via
self._sync.chat.completions._sync._clientor similar). I considered teaching_evict_cached_client_instanceto walk those chains, but mirroring_real_clientis:_real_client(e.g. a future "is this client healthy?" probe) automatically covers async tooRelated Issue
Refs #23482 (parent fix), #23432 (original issue — closed by #23482; sync side fully resolved, this PR addresses the parallel bug class on the async side).
Type of Change
Changes Made
agent/auxiliary_client.py(+18 / -3)AsyncCodexAuxiliaryClient.__init__: mirror_real_client(with explanatory comment)AsyncAnthropicAuxiliaryClient.__init__: same_evict_cached_client_instancedocstring: now covers async wrappersagent/gemini_native_adapter.py(+6)AsyncGeminiNativeClient.__init__: mirror_real_clientpointing at sync_clienttests/agent/test_auxiliary_client.py(+36)test_evict_cached_client_instance_walks_async_wrapper: seeds both sync and async cache entries pointing at the same leaf, asserts a single eviction call drops both. Without the wrapper changes the test fails with the assertion message"async cache entry survived eviction — wrapper is missing _real_client".How to Test
Checklist
Code
fix(auxiliary): ...)+60 / -3across 3 filespytest tests/ -qand all tests pass — rantests/agent/test_auxiliary_client.py(147/148 pass; the one pre-existing failure is environmental and present onmain); did not run the full repository suite locally. Relying on CIto validate the full suite.
Documentation & Housekeeping
_evict_cached_client_instancedocstring updated to cover async wrapperscli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A