fix(codex): stop retrying a doomed pool credential refresh, and say it was local (#4546) - #4639
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe change adds per-account Codex refresh backoff, classifies terminal and operational failures, excludes cooling accounts from routing, clears cooldown state during account resets, and marks locally synthesized refresh refusals in request logs. Tests and layout fixtures cover the new behavior. ChangesCodex pool refresh backoff
Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Codex request
participant forceRefreshCodexPoolToken
participant pool-refresh-backoff
participant routing
participant request-log
Codex request->>forceRefreshCodexPoolToken: Refresh pool credential
forceRefreshCodexPoolToken->>pool-refresh-backoff: Check and record cooldown state
pool-refresh-backoff-->>forceRefreshCodexPoolToken: Allow refresh or return cooldown error
routing->>pool-refresh-backoff: Check account eligibility
pool-refresh-backoff-->>routing: Return cooling status
Codex request->>request-log: Mark local refusal when refresh is incomplete
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7dc6f9554
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && getCodexQuotaHealthSnapshot(accountId, quotaScope, now) === null | ||
| && !isCodexQuotaAvoided(accountId, quotaScope, now) | ||
| && !isCodexAccountSoftAvoided(accountId, now) | ||
| && !isCodexPoolRefreshCooling(accountId, now) |
There was a problem hiding this comment.
This adds a new authentication and pool-selection invariant—failed credential refreshes now create a separate cooldown that changes eligibility and affinity handling—but none of the architecture documents mapped to src/codex/ or src/server/ were updated. Please document the cooldown, its lifecycle, and its relationship to credential generations in the applicable owned structure documents so the repository's architecture source of truth does not continue describing the previous routing behavior.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
| type RefreshFailureBackoff = { | ||
| consecutiveFailures: number; | ||
| cooldownUntil: number; | ||
| reason: string; | ||
| }; | ||
|
|
||
| const backoffByAccount = new Map<string, RefreshFailureBackoff>(); |
There was a problem hiding this comment.
Keying this state only by account ID lets a failure for credential generation G suppress a newly published generation G+1. For example, if a transient refresh failure opens a cooldown and the user reauthenticates the account during that window, saveCodexAccountCredential advances the generation but never clears this map, so automatic routing still excludes the valid replacement for up to 60 seconds and the stale failure count can affect later refreshes. Store the credential generation/fingerprint with the cooldown and ignore or clear entries when the account record advances, including alias propagation.
Useful? React with 👍 / 👎.
| export function markLocalRequestLogRefusal(logCtx: RequestLogContext, reason: string): void { | ||
| logCtx.localTerminalReason = reason; | ||
| logCtx.terminalSource = "synthetic"; |
There was a problem hiding this comment.
When a stored-pool request receives an upstream 401 and its forced refresh then fails, the request has already made a real upstream send, but this helper sets localTerminalReason. That field's existing contract says no upstream request was issued, and addFinalRequestLog treats its presence as locallyAnswered when finalizing usage, so the resulting row can contradict its attempt/spend data and apply zero-send usage semantics. Keep terminalSource: "synthetic" for the locally generated final response, but use a distinct refusal-origin field rather than localTerminalReason for this post-send case.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 70 / 80이 PR은 본문이 재현까지 적었습니다. 저장된 계정은 멀쩡해 보이는데 요청마다
#4546 비용 나선과 직접 맞닿습니다. 캐시·쿼터 도메인(#4624)과 probe lease(#4626)가 있어도, 한 계정의 실패한 refresh 루프가 풀을 붙잡으면 건강한 형제가 놀고 토큰만 돕니다. 다만 base가 wpg라서 라인 src/codex/pool-refresh-backoff.ts - 프로세스 메모리 Map입니다. 재시작 후 쿨다운이 사라지는 것은 주석 의도와 맞지만, 다중 인스턴스에서는 인스턴스마다 다시 시도할 수 있습니다. 단일 프록시 가정인지, 나중에 공유 저장이 필요한지 한 줄로 적어 두면 wpe 원장과 경계가 분명해집니다. 라인 src/server/responses/core.ts / request-log - 로컬 거절을 upstream overload와 구분하는 로그·응답 코드가 실제 클라이언트에 어떻게 보이는지(재시도 가능 헤더·본문)만 확인하면 좋습니다. 재시도 가능( 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
bd75d0d to
92f5ddc
Compare
e7dc6f9 to
4ecb95f
Compare
92f5ddc to
7382f6a
Compare
4ecb95f to
67c4115
Compare
7382f6a to
27b5170
Compare
a0918cd to
34614c7
Compare
34614c7 to
fe8917d
Compare
2478c48 to
6c96bdb
Compare
…t was local (#4546) Reproduced live: every request routed to one pool account returned 503 server_is_overloaded, five of five sequential probes, with a healthy stored record and not one line in the service log. The refusal was this proxy's own poolCredentialRefreshIncompleteResponse, and because only revoked/expired counted as terminal, a missing record or a token-endpoint 5xx became an endlessly retryable 503 on an account selection kept returning to. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
Hosted CI failed six rows of the pool 401 refresh suite with 503 where 401 or 200 were expected. Withholding on the FIRST non-terminal failure was wrong twice over: a single token-endpoint blip is the ordinary case the next attempt clears, and a withheld refresh never runs, so an account whose grant is actually revoked could no longer discover that - the terminal 401 it owes the operator became a retryable 503 that never resolves. The cooldown now withholds only after three consecutive failures, and the do-not-grow-inside-the-window rule applies only while it is actually withholding, so a client retrying once a second can still reach the threshold. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
…account state (#4546) A cooldown is per-account runtime state learned alongside the thread bindings, but it outlived clearThreadAccountMap. An account that had failed a refresh therefore stayed out of selection after the roster it belonged to was gone - which is what kept a replayed account unselectable on the NEXT request in the pool 401 suite. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
29c9aa3 to
6966470
Compare
…nd clear one root (#4546) (#4657) * fix(server): name the ceiling that refused, and let an operator see and clear one root (#4546) The workflow budget could refuse a task and leave nothing behind to explain it. Two specific gaps, both measured against the code rather than assumed. The refusal never reached the request log at all. runAdmittedHttpTurn returns before it calls work(), and every addFinalRequestLog in that file is inside work, so there was no row and no context to mark. And the ceiling name never reached the client either: classifyError rewrites every 429 to rate_limit_error / rate_limit_exceeded, so the body was shaped exactly like a provider rate limit and the workflow_budget_exhausted type argument was discarded on the way out. All four count denials also shared one sentence about a "concurrent-work limit", which was true of exactly one of them -- a task that had hit the SEND ceiling was told to wait for turns to finish, and waiting never helped because nothing was running. Each denial now has its own sentence naming its ceiling and saying this proxy decided it without contacting anyone. The wire status and type are unchanged on purpose, since altering them changes how every client retries, so the machine-readable name rides alongside on x-opencodex-local-refusal. Nothing upstream sets that header, which is what makes its presence conclusive. Both call sites now go through one src/server/workflow-refusal.ts instead of two inline blocks that had drifted apart; where a log context exists, the row is marked synthetic through the same helper #4639 introduced. A refusal that parsed no body, chose no model and contacted no provider is not a usage row, and forcing one would put a fabricated model and provider into usage.jsonl. It goes instead into a bounded ring of recent budget events inside the budget itself, recorded at every refusal return site in admitWorkflowTurn -- including the spend denials, which are decided inside the ledger branch and never surface to the caller that formats the response. GET /api/workflow-budget reads the tracked roots or one root, and POST /api/workflow-budget/clear clears exactly one. The clear is bounded in a specific way: it moves the windowed send ring and the child map and nothing else. active belongs to turns still in flight, and zeroing it would let their releases drive the count negative and hand out slots already taken. The spend ledger is money an operator did not ask to forgive, and a count ceiling is not a licence to reset it. The lifetime send total survives too, so clearing a ceiling cannot launder the record of what the root actually did. A test drives that last one directly: after a clear, an exhausted token budget still refuses. Both routes are declared deferred-verb in the route registry. They are owed CLI verbs and the ledger is process memory, so unlike the Lab routes there is no local projection the CLI could read instead. Local suite, typecheck, install and build: NOT RUN, per the standing instruction. Hosted CI at the exact head is the only proof. Pushed --no-verify. * fix(server): put the workflow refusal on the request log and expose its header (#4546) CI caught one test and an independent plan review caught two real gaps. The test failure was mine and it was the fixture-assumption mistake again: the tracked-roots ordering test released each lease, and release() stamps lastSeenMs from the wall clock because it feeds eviction ordering rather than a ceiling. Three injected timestamps collapsed into three near-identical real ones. The leases now stay open, which is what the test was actually about. The review's blocking finding was that skipping the request-log row was a choice, not a constraint. addFinalRequestLog takes whatever model and provider the context carries, and the /v1/responses caller already seeds unknown/unknown before the turn runs -- the same placeholder the native passthrough path writes. So the refusal now writes a real row through that context: terminalSource synthetic, a local reason, and an error code naming the ceiling, which wins over the generic 429 classification in the logs column. Only /v1/responses threads it, because the workflow root is the Codex x-codex-parent-thread-id header and no other inbound wire carries it. Second finding: the marker header was invisible to the dashboard. The data plane never sets Access-Control-Expose-Headers, so cross-origin JavaScript could not read it and the marker was useful to curl and to nothing else. The refusal now exposes it. The wire body is deliberately still rate_limit_error / rate_limit_exceeded. The review asked for a distinct error code through classifyError; that changes how every client classifies a 429 from this proxy, and the surface an operator actually reads is the log row, which now carries the name. Also recorded the two routes in structure/gui-and-management-api.md. Local suite, typecheck, install and build: NOT RUN. Hosted CI at the exact head. * fix(lib): look the refusal row up by requestId, which is the field it has (#4546) The row was written correctly -- the count assertion passed -- but the lookup read entry.id, and RequestLogEntry names that field requestId. find() returned undefined and the terminalSource assertion read a property of nothing. Local suite, typecheck, install and build: NOT RUN. Hosted CI at the exact head. * fix(server): reach every surface's log, and actually let a browser read the header (#4546) Second review round found that both of the first round's fixes stopped short. The claim that only /v1/responses carries the Codex parent-thread header was wrong. /v1/responses/compact carries it too -- its existing tests send it -- and runAdmittedHttpTurn reads it for every caller, so a compact refusal still left nothing on /api/logs. Every inbound surface that opens a request-log row now threads it: compact, images, context history, alpha search, messages, chat completions, audio transcriptions and live. /v1/messages/count_tokens is the one that does not, because it opens no row at all. Exposing the marker header was pointless while the admission refusal returned a raw Response. Access-Control-Expose-Headers without Access-Control-Allow-Origin is unreadable to cross-origin JavaScript, so the header remained useful to curl and to nothing else. The refusal is wrapped in withCors now. The pre-dispatch ceiling check in the responses path is a second refusal taken after admission already succeeded, so nothing inside the budget module saw it: it was the one refusal an operator could hit that left no event behind. It records one now, through a seam that only a caller which decided the refusal itself uses, so admitWorkflowTurn's own denials are not double-counted. The review also pointed out that a unit test on the helper proves the helper and not the wiring, and the wiring is exactly where this went wrong twice. A source guard now asserts that every runAdmittedHttpTurn call site but one threads a refusal row, and that the refusal is CORS-wrapped. Local suite, typecheck, install and build: NOT RUN. Hosted CI at the exact head. * test(server): stop pinning a call's terminator in the loopback CORS oracles (#4546) Two source-oracle tests asserted the literal "req,\n policy,\n ));" inside the Anthropic and chat branches. The invariant they exist for is that withCors finishes with the receiving listener's policy rather than the public config. The closing "));" was the outer runAdmittedHttpTurn call's terminator, which has nothing to do with that, and both went red the moment that call gained a fourth argument. The assertions now stop at the closing paren of withCors, so they still fail on config and no longer fail on an unrelated argument. Local suite, typecheck, install and build: NOT RUN. Hosted CI at the exact head.
1 participant
Summary
A Codex pool account whose forced credential refresh fails now backs off, loses selection preference, and says on the record that the refusal was local. Before this, one unhealthy account could pin the whole pool at 503 indefinitely while healthy siblings sat idle.
Reproduced live. Every request routed to one pool account returned
503with codeserver_is_overloaded— five of five sequential probes, two different models, ~600ms each. The stored record was healthy: present, nodeletedAt, refresh token present, grant fingerprint present, expiry a week out. Not one line reached the service log. Switching the active account made it stop on the very next request.The refusal was this proxy's own
poolCredentialRefreshIncompleteResponse, and three things made it permanent.isTerminalPoolRefreshFailureadmitted onlyTokenRefreshErrorwith reasonrevokedorexpired, so a missing record, a missing grant fingerprint, a token-endpoint 5xx and a generation CAS loss all became a retryable 503 whose body asks the client to retry — a loop that sustains the condition it waits out. Selection kept returning to the account because it still readhealthy. And nothing distinguished this locally synthesized refusal from a genuine upstream overload.src/codex/pool-refresh-backoff.tsadds a per-account cooldown. Consecutive non-terminal failures open a bounded growing window (2s through 60s); inside it no new forced refresh starts andisCodexPoolRefreshCoolingremoves the account from every selection path, while the binding is kept and the account is not quarantined — a token-endpoint 5xx must not retire a healthy account (#2887). A failure inside an open window does not grow it, so a burst of concurrent requests cannot race one account to the ceiling. The first successful refresh clears everything.CodexCredentialUnavailableErrorreplaces the bareErrorfor a missing record or fingerprint and is classified terminal, so the operator is told to sign in again instead of being asked to retry something that can never succeed.isTerminalPoolRefreshFailurenow delegates to one definition.The 503 wire contract is deliberately unchanged — its status and exact wording are what make the client apply retry-after backoff, and the word "reauthentication" would flip the classification. What changes is the record: the entry carries
terminalSource: "synthetic"and a distinctlocalTerminalReason, because the dashboard renders this sentence under a field named "Upstream reason", which is what sent an experienced maintainer to the provider's status page for a fault that never left his own process.Stacked on #4638.
Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The only proof for this unit is hosted CI at the exact final head SHA; this push used--no-verify.New coverage:
tests/codex-integration/codex-pool-refresh-backoff.test.tspins the growing bounded window, that a failure inside an open window does not grow it, the ceiling, automatic clearing on success, that one cooling account never cools a sibling, that the cooldown error is retryable and avoids the word that would flip its classification, and that a missing credential is terminal while a token-endpoint 5xx and a CAS loss stay transient. Registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.Known open: the compact path's refusal at
compact.tsdoes not yet receive alogCtx, so its refusals are not marked synthesized. The cooldown steps are constants rather than operator configuration.Checklist
Summary by CodeRabbit
Reliability
Error Handling