fix(auxiliary): evict Codex auxiliary client on timeout - #25064
Closed
wesleyhere wants to merge 1 commit into
Closed
wesleyhere wants to merge 1 commit into
wesleyhere wants to merge 1 commit into
Conversation
wesleyhere
marked this pull request as ready for review
May 13, 2026 15:33
19 tasks
Collaborator
|
This has already landed on main in the same cleanup path. Automated hermes-sweeper review found the Codex auxiliary in-loop timeout now routes through the timeout closer before raising. Evidence:
Thanks for isolating the race; the fix is now covered by current main. |
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?
Fixes a race in the Codex auxiliary Responses timeout cleanup path that can leave a poisoned client in the auxiliary cache.
Root cause
_CodexCompletionsAdapter.create()inagent/auxiliary_client.pyhas two paths that detect the same total-timeout deadline:Timer thread —
threading.Timer(total_timeout, _close_client_on_timeout)fires from a separate thread when the deadline elapses._close_client_on_timeout()setstimed_out, closes the underlyingOpenAIclient, and calls_evict_cached_client_instance()to drop the cached wrapper (added by fix(auxiliary): evict cached client on timeout/connection error #23482; extended to async wrappers by fix(auxiliary): evict async wrappers on poisoned client (follow-up to #23482) #23931).In-loop deadline check —
_check_cancelled()is invoked between stream events insidefor _event in stream:. Whentime.monotonic() >= deadline, it previously settimed_outonly, then raisedTimeoutErrordirectly — skipping the close and the cache eviction.The two paths are armed at the same threshold but run on different threads, so they race. On tight timeouts the in-loop check wins deterministically: stream iteration drives
_check_cancelled()on every event, while the timer thread may not be scheduled in time. When path 2 wins, the timeout exits without running the cleanup path that closes the Codex client and evicts cached wrappers. That leaves cache state dependent on a thread scheduling race: later auxiliary calls can reuse a stale or poisoned wrapper instead of rebuilding cleanly, surfacing asopenai.APIConnectionError: Connection error(the cascade originally tracked in #23432).Fix
Route the in-loop deadline check through
_close_client_on_timeout()so both detection paths perform the same cleanup (close + cache eviction) before raisingTimeoutError._close_client_on_timeout()is already idempotent —timed_out.set()is set-or-no-op,client.close()is safe to call twice, and_evict_cached_client_instance()no-ops on an already-evicted entry — so the timer firing afterward causes no harm.def _check_cancelled() -> None: if deadline is not None and time.monotonic() >= deadline: - timed_out.set() + _close_client_on_timeout() raise TimeoutError(_timeout_message())Related Issue
Refs #23617, #21761, #22986. No standalone issue; the
_check_cancelledrace is covered by the regression test added here and by the related work below from broader angles.Prior art / related work:
_check_cancelled()cleanup change, but bundles it with a larger threaded stream consumer, Codex compression routing changes, and task-aware auto cache keys. This PR is the minimal isolated fix for the timeout cleanup race. If fix(agent): avoid Codex stream for compression summaries #23617 lands first, this PR can be closed; if maintainers prefer the smaller fix, it can land independently and fix(agent): avoid Codex stream for compression summaries #23617 can rebase/drop this line.APIConnectionErrorrates around stream-timeout enforcement; this PR addresses one narrow cleanup race in that area.Type of Change
Changes Made
agent/auxiliary_client.py: call_close_client_on_timeout()when_check_cancelled()detects the Codex auxiliary total-timeout deadline, so timeout cleanup consistently closes and evicts the cached client wrapper.tests/agent/test_auxiliary_client.py: add deterministic regression coverage for the loop-detected timeout path by disabling the timer callback and forcing_check_cancelled()to win._close_client_on_timeout()only runs when the deadline is exceeded, and it is already used by the timer path. Calling it from_check_cancelled()makes both timeout detection paths perform the same cleanup.How to Test
Run the focused regression coverage:
scripts/run_tests.sh tests/agent/test_auxiliary_client.py -k 'CodexAuxiliaryAdapterTimeout or AuxiliaryClientPoisonedCacheEviction'Expected result:
Optional wider check before requesting review:
Checklist
Code
fix(scope):,feat(scope):, etc.)scripts/run_tests.shtarget and all selected tests passDocumentation & Housekeeping
docs/, docstrings) - N/Acli-config.yaml.exampleif I added/changed config keys - N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - N/AFor New Skills
N/A