From 1f88ebf3ecdaee340d8673e9868f7fd1d8ed053c Mon Sep 17 00:00:00 2001 From: Synero Date: Mon, 14 Sep 2026 03:11:26 +0000 Subject: [PATCH] test(agent): pin the auth-refresh retry boundary and the pool-gate split Review feedback on the fall-through fix: the accept boundary and the retry-fails-with-a-connection-error path through the credential rung were reachable but untested, and two comments described behavior the code does not have. Tests and comments only, no behavior change. --- agent/auxiliary_client.py | 12 +++- .../test_auxiliary_auth_rung_fallthrough.py | 72 +++++++++++++++++-- 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 064c100641..ffc747d390 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -7033,10 +7033,17 @@ def _ladder_credential_rungs( step, lambda exc: _credential_rung_accepts(exc) or _is_connection_error(exc)) if first_err is None: return resp, None + # ``first_err`` is now the retry's own failure, not the original auth error: the + # pool gate below and the ladder tail's eviction check both read this narrowed + # value. An unclaimed failure (e.g. a 500) re-raised out of ``_rung`` above + # instead, since the provider-fallback rung only acts on ``_FALLBACK_REASONS``. pool_provider = _recoverable_pool_provider(resolved_provider, client, main_runtime=route.main_runtime) # Capture the exact key used so recovery finds the right pool entry even if another # process rotated the pool meanwhile (current() would be None). _client_api_key = str(getattr(client, "api_key", "") or "") + # Gate on the narrowed error: a connection failure from the retry above arrives here + # unaccepted on purpose (a fresh key cannot fix an unreachable endpoint), so rotation + # is skipped and ``first_err`` is handed to the provider-fallback chain as-is. if pool_provider and _credential_rung_accepts(first_err): recovery_err = first_err # Skip the extra retry for clear payment/quota errors — the endpoint won't accept @@ -7192,8 +7199,9 @@ def _aux_recovery_ladder( return resp # Connection/timeout errors poison the cached client (closed transport, half-read # stream); evict so the next aux call rebuilds a fresh one. - # Drop it from the cache regardless of whether we found a fallback above so the next auxiliary call - # rebuilds a fresh client instead of reusing the dead one. See issue #23432. + # Reached only when no fallback answered, so the next auxiliary call rebuilds a fresh + # client instead of reusing the dead one. ``first_err`` is the narrowed error from the + # rungs above, not necessarily the original one. See issue #23432. # Mirror the sync path: drop poisoned clients on connection/timeout so the next aux call rebuilds. See # issue #23432. if _is_connection_error(first_err): diff --git a/tests/agent/test_auxiliary_auth_rung_fallthrough.py b/tests/agent/test_auxiliary_auth_rung_fallthrough.py index eee78af652..bad27743be 100644 --- a/tests/agent/test_auxiliary_auth_rung_fallthrough.py +++ b/tests/agent/test_auxiliary_auth_rung_fallthrough.py @@ -11,8 +11,12 @@ bare ``yield`` inside a ``return`` statement: return (yield _LadderStep( "retry_same_provider", ...)), None # generic credential rung -``_rung()`` exists to convert a retry failure into ``(None, exc)`` so the caller can -fall through to the next rung. Used this way no exception is caught: when the +``_rung()`` exists to convert a retry failure into ``(None, exc)`` -- but only when the +rung's accept predicate claims the error -- so the caller can fall through to the next +rung. An unclaimed failure (a 500, a malformed response) re-raises on purpose, since +``_ladder_provider_fallback`` only acts on the reasons in ``_FALLBACK_REASONS``; that +boundary is pinned by ``test_post_refresh_retry_reraises_non_recoverable_error``. +Used this way no exception is caught: when the refreshed client also fails (e.g. an out-of-credit 404 on a stale Nous runtime token), the error escapes ``_aux_recovery_ladder`` and ``_ladder_provider_fallback`` never runs -- the configured ``auxiliary..fallback_chain`` is silently skipped. The @@ -92,9 +96,9 @@ def hermetic(monkeypatch): chain_calls = [] def _fake_provider_fallback(first_err, route): + """Stands in for the last rung: a generator that performs no steps.""" chain_calls.append(first_err) - if False: # pragma: no cover - makes this a generator function - yield None + yield from () return "chain-response" monkeypatch.setattr(aux, "_recoverable_pool_provider", lambda *a, **kw: None) @@ -278,3 +282,63 @@ def test_auth_refresh_retry_failure_reaches_the_configured_chain_over_http( "401, then the refreshed retry fails on credits, then the configured chain: %r" % (seen,) ) + + +def test_post_refresh_retry_reraises_non_recoverable_error(monkeypatch, hermetic): + """A retry failure the rung does not claim re-raises instead of voiding the ladder. + + ``_rung()`` only converts a failure into ``(None, exc)`` when the rung's accept + predicate claims it. The post-refresh retry claims credential and connection + failures; anything else (a 500, a malformed response) re-raises on purpose, because + ``_ladder_provider_fallback`` only acts on the reasons in ``_FALLBACK_REASONS``. + Pinning it here turns the boundary that lives in the lambda into a documented + contract: the retry's own error surfaces, and no fallback is pretended. + """ + monkeypatch.setattr(aux, "_refresh_nous_auxiliary_client", + lambda **kwargs: (_FakeClient(), AUX_MODEL)) + ladder = _ladder() + performed = [] + + def perform(step): + performed.append(step.kind) + raise _ApiError("Error code: 500 - Internal Server Error", status_code=500) + + with pytest.raises(_ApiError) as excinfo: + aux._drive_ladder(ladder, perform) + + assert "500" in str(excinfo.value), "the retry's own failure must surface" + assert performed == ["call"], "the post-refresh retry is the only request" + assert hermetic == [], "an unclaimed failure has no reason to consult the chain" + + +def test_post_refresh_retry_connection_error_skips_pool_rotation(monkeypatch, hermetic): + """A connection failure from the refresh retry skips rotation and reaches the chain. + + The retry predicate accepts connection errors, the pool gate below it does not: a + fresh key cannot fix an unreachable endpoint, so the narrowed ``first_err`` goes + straight to the provider fallback. The ladder tail then evicts the poisoned route + client based on that narrowed error, not on the original one. + """ + monkeypatch.setattr(aux, "_auth_refresh_provider_for_route", lambda *a, **kw: "codex") + monkeypatch.setattr(aux, "_refresh_provider_credentials", lambda *a, **kw: True) + monkeypatch.setattr(aux, "_evict_cached_clients", lambda *a, **kw: None) + rotations = [] + monkeypatch.setattr(aux, "_recoverable_pool_provider", lambda *a, **kw: "openrouter") + monkeypatch.setattr(aux, "_recover_provider_pool", + lambda *a, **kw: rotations.append(a) or True) + + ladder = _ladder( + base_info="https://openrouter.ai/api/v1", resolved_provider="openrouter") + retry_error = ConnectionError("Connection refused by the endpoint") + performed = [] + + def perform(step): + performed.append(step.kind) + raise retry_error + + assert aux._drive_ladder(ladder, perform) == "chain-response" + assert performed == ["retry_same_provider"], "only the post-refresh retry is attempted" + assert rotations == [], "a connection error must not rotate credentials" + assert hermetic and hermetic[0] is retry_error, ( + "the chain sees the narrowed connection error, not the original auth error" + )