fix(credentials): isolate Codex 429 rotation and stale rechecks - #65399
fix(credentials): isolate Codex 429 rotation and stale rechecks#65399JIMVD2002 wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the thorough attribution coverage. The narrow premise is real on current main: agent/agent_runtime_helpers.py:798, :822, :846, and :946 omit the existing CredentialPool.api_key_hint mechanism (agent/credential_pool.py:1583-1602).
Problems
agent/conversation_loop.py:1645promotes a probe before examining the Responses status.agent/codex_runtime.py:918-927returnsstatus="failed"responses without raising, so a failed terminal response can transiently persistSTATUS_OKbefore later processing handles it.agent/credential_pool.py:128-133adds a one-hour Codex reprobe despite a provider-supplied reset. Current main honorslast_error_reset_at(agent/credential_pool.py:354-362), andAGENTS.md:155-163specifically cautions against cooldown reprobes of confirmed-empty buckets.
Suggested changes
- Keep the exact request-key attribution and exact-key refresh fixes, but separate/remove the forced cooldown probe behavior pending a maintainer decision.
- Promote a probe only after a completed response is accepted; add a
response.failedregression.
Automated hermes-sweeper review.
| # A structurally valid response proves that the exact runtime | ||
| # credential used for this request is healthy. Promote only a | ||
| # matching in-flight Codex recheck; normal credentials are no-op. | ||
| from agent.agent_runtime_helpers import record_credential_pool_success |
There was a problem hiding this comment.
run_codex_stream() returns a final object for response.failed events (agent/codex_runtime.py:918-927), so this runs before that failed response is rejected downstream. Promote only after a completed response has been accepted, and add a response.failed regression so a failed probe cannot become ok.
| @@ -113,6 +125,12 @@ def _load_config_safe() -> Optional[dict]: | |||
| EXHAUSTED_TTL_401_SECONDS = 5 * 60 # 5 minutes | |||
| EXHAUSTED_TTL_429_SECONDS = 60 * 60 # 1 hour | |||
| EXHAUSTED_TTL_DEFAULT_SECONDS = 60 * 60 # 1 hour | |||
| # Even when Codex reports a multi-day reset_at, permit one atomically claimed | |||
There was a problem hiding this comment.
This deliberately bypasses the provider reset timestamp after one hour. Current main honors last_error_reset_at, and AGENTS.md:155-163 warns against re-probing confirmed-empty cooldown buckets. Please keep the attribution fix separate from this policy change unless a maintainer explicitly approves it.
Remove the forced Codex cooldown reprobe and premature probe promotion per review. Preserve exact request-key exhaustion and refresh routing, and add fail-closed coverage for unknown keys and legacy pool doubles.
801ba97 to
17d3c77
Compare
|
Addressed the sweeper review in
Validation after rebasing onto current The net PR diff is now limited to three files: |
|
I think the remaining attribution boundary is slightly narrower than the current implementation suggests. A safer invariant is to capture the concrete credential at the transport dispatch boundary and carry it in attempt-local state through recovery. The attempt should close when the request completes, late worker callbacks should be ignored, and an observed empty or unmatched credential should fail closed rather than falling back to A useful regression would dispatch two overlapping requests with synthetic credentials A and B, rotate shared agent state before A fails, and assert that only A receives the failure metadata. The same test should cover a late callback after the attempt closes. I can contribute that focused regression/attempt-local binding if it fits the intended scope of this PR. |
|
Thanks for the thorough work here — closing as redundant after verification against current main. The core fixes this PR carries all landed via the recent attribution series with a kwarg-based design:
Your Appreciate the investigation, @JIMVD2002 — it independently identified the same root causes the merged series fixed. |
Summary
Fix OpenAI Codex credential-pool recovery so a 429/401 is bound to the exact credential that sent the request instead of a potentially stale
pool.current()entry.This prevents a real
usage_limit_reachedresponse from one Codex account from persisting exhaustion metadata onto healthy sibling accounts.What changed
api_key_hintthrough billing, rate-limit, and auth recovery paths.Regression coverage
The tests cover:
Verification
Rebased/cherry-picked conflict-free onto current upstream
main(2ea39daeb1). Patch ID matches the independently reviewed local deployment commit.Live profile validation was performed separately with three real OpenAI Codex OAuth credentials using minimal streaming Responses calls. After label-bound token synchronization, all three returned HTTP 200 with valid stream events. No credentials or secret values are included in this PR.