Skip to content

fix(agent): heal credential-pool last_status after a successful served request - #95351

Closed
SayHell0W0rld wants to merge 1 commit into
NousResearch:mainfrom
SayHell0W0rld:fix/pool-last-status-heal
Closed

SayHell0W0rld wants to merge 1 commit into
NousResearch:mainfrom
SayHell0W0rld:fix/pool-last-status-heal

Conversation

@SayHell0W0rld

@SayHell0W0rld SayHell0W0rld commented Aug 26, 2026

Copy link
Copy Markdown

Summary

Fixes gap 2 of #95166: a credential-pool entry marked STATUS_EXHAUSTED by a transient wall stays exhausted after the wall lifts, making every subsequent pool reader see a false wall.

Approach

  • _mark_healthy() (agent/credential_pool.py) — symmetric to _mark_exhausted(): resets last_status to STATUS_OK and clears error fields. No-op when the entry already has last_status=None.
  • _relay_final_response() (agent/chat_completion_helpers.py) — the unified success exit for chat-completions mode: after a successful served request, heals the active credential-pool entry if it still carries a stale exhausted/dead verdict.
  • Only chat-completions mode is covered by the success-side heal; other API modes (responses, streaming-only) are not yet wired — the issue's reported probe path uses chat-completions, so this closes the gap for the primary case.

Test Plan

  • 3 new tests in test_credential_pool.py: TestMarkHealthy — clears exhausted status, skips already-healthy entry, clears dead status.
  • Full file: 62/62 pass (3 new, 59 existing). ruff check clean. Windows footgun lint clean.

Risk / Exclusions

  • _mark_healthy is provider-agnostic and only touches the shared credential pool.
  • _relay_final_response only runs for chat-completions mode; other API modes (responses, streaming-only) are unaffected.
  • No prompt-cache or message-alternation impact (the heal runs after the response is fully constructed).

References #95166

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 26, 2026
…d request

A credential marked exhausted by a transient rate-limit/billing wall stays
exhausted even after the wall lifts and kernel-mediated requests succeed again
(issue NousResearch#95166, gap 2). Add _mark_healthy() (symmetric to _mark_exhausted) and
call it from _relay_final_response — the unified success exit for
chat-completions mode — so pool readers don't see a stale exhausted verdict.

3 regression tests; 62/62 file pass; ruff + Windows footgun lint clean.
@SayHell0W0rld
SayHell0W0rld force-pushed the fix/pool-last-status-heal branch from 3e163a2 to ff5c0a5 Compare August 26, 2026 07:40
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Plugs a real gap: nothing else in agent/ clears an exhausted/dead verdict after the provider actually serves traffic (the only other STATUS_OK writes are refresh/import paths inside credential_pool.py itself). Observations:

  • chat_completion_helpers.py:4028-4036 — match-by-runtime_api_key is the right identity (not entry id): it survives pool re-imports that re-phase credentials, and healing every duplicate entry sharing the key is correct. Gating on (STATUS_EXHAUSTED, STATUS_DEAD) at the call site keeps the hot path cheap.
  • Coverage question, worth a maintainer answer: _relay_final_response is the Relay streaming path's finalizer (relay_llm.stream at :3349/:3376). The non-streaming success path in the same module has no equivalent hook — a key that only ever serves non-streaming requests stays stuck at its stale verdict. If xai-oauth: usage/limits visibility parity with openai-codex (fetch_usage), and heal credential_pool last_status on served requests #95166's repro was streaming-only that's defensible, but the intent ("successful served request") implies both; either extend or add a note documenting the asymmetry.
  • credential_pool.py:918-935_mark_healthy's early-return covers only last_status is None; a direct call on a STATUS_OK entry still does _replace_entry + _persist (a no-op disk write). The call site pre-filters so this is unreachable today, but test_skips_already_healthy_entry pins that run — either early-return on STATUS_OK too, or make the test assert the intended no-op rather than the current replace-and-persist.
  • Observability nit: a stale-dead → ok recovery is a meaningful state transition (a flapping key will flap this branch); nothing is logged when it fires, unlike the pairing _mark_exhausted path. One logger.info("cleared stale %s verdict for %r", ...) would pay for itself the first time someone debugs pool hysteresis.
  • Tests cover the three transition shapes (exhausted→ok, dead→ok, already-ok) with full field-level assertions including last_status_at/last_error_code clears. The new method isn't included in the file's existing "does not hold the lock" deadlock test class above (line ~2079) — arguably worth adding for symmetry since it persists under the same locking regime.

No blocking issues found; the streaming-vs-non-streaming coverage gap (second bullet) is the one to resolve before merge.

@SayHell0W0rld

Copy link
Copy Markdown
Author

Merged into #90417 (both fixes live in _relay_final_response's success path; keeping one PR avoids a merge conflict between two overlapping edits of the same function). The reasoning-recovery and credential-heal logic are both preserved on the combined branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants