fix: stop one account failure from taking the whole proxy out of service - #5199
Conversation
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds specific pool reauthentication causes, scopes model-denial evidence to credential generations, and changes startup readiness so ChangesCodex reliability
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: High Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 16 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
|
✅ 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: 1e9b42d0f2
ℹ️ 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".
| : runtimeReauth | ||
| ? "refresh_failed" |
There was a problem hiding this comment.
After a forced quota probe returns quota_unauthorized, it also sets the runtime reauthentication flag. A subsequent ordinary account-list poll while the previous quota is still within POOL_CACHE_TTL receives a PoolQuotaResult with neither reauthReason nor needsReauth, so this fallback changes the reported cause to refresh_failed even though no refresh failed. This makes /api/codex-auth/accounts contradict the probe result on the next dashboard poll; retain the cause alongside the runtime flag or propagate it through the cached-quota path.
Useful? React with 👍 / 👎.
PoolQuotaResult now carries the failure source observed while obtaining WHAM/quota or refreshing tokens, so poolAccountDto reports the actual cause instead of inferring it from overlapping booleans. Terminal WHAM 401s surface as quota_unauthorized and terminal refresh failures as refresh_failed. Carried from #5191. The two regression cases are registered from tests/helpers/pool-reauth-cause.ts rather than written inline: the original placement grew tests/codex-integration/codex-auth-api.test.ts from 6522 to 6560 lines against a 6549-line file-size-baseline cap that only ever moves downward. Registering them through the same registerXCases seam the file already uses for two other helpers leaves it at 6525 with the assertions byte-identical. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…roxy /readyz went to a terminal failed state on any nonempty warning from the post-startup Codex sync. A warning there describes artifacts written into the LOCAL Codex home, not the proxy's ability to serve: no catalog source so Codex keeps its native catalog, combos omitted from the catalog, a conversation-history relabel left to Codex's own writer, or a caught catalog-refresh exception after which config injection still runs and still reports its own ok. The sync continues past every one of them. #5181 is what that cost. A single-replica Kubernetes deployment lost its only Service endpoint while every non-Codex route stayed healthy, because the readiness probe read a degradation of a local file as a dead process. Switching the probe to /healthz restored service, which is the reporter's own evidence that the process was viable. ok and warning are separate fields in the sync result because they answer separate questions. ok is the sync's verdict on the essential work - the write admission and the config injection. The gate now follows ok alone. The narrowing is only to warning. A throw, a null outcome, and ok !== true stay terminal, so the service-home write refusal, the external-provider injection failure, the config/integrity preflight refusal and the final injection failure all still fail readiness. A throw and a null stay terminal for a different reason: neither is a classified outcome, so the gate cannot claim one. Readiness also still stays pending until the sync SETTLES, which is the race the gate was built for and which this does not touch. This is the boundary the Claude Code roster reconciliation already has - it delays the ready transition without being allowed to fail it - applied to the catalog sync. Scope note: the brief for this work held that the Codex pool's "needs reauthentication" warning was itself the terminal warning. It is not. That line comes from warnGatedNativeSuppressedOnce (src/codex/catalog/gated-native-warn.ts) and is a bare console.warn that never enters the structured result, and per-account pool refresh failures are caught and returned as null by model-entitlements.ts rather than propagating. The account-level fault that does reach the gate is a native MAIN credential refresh failing outside that per-account catch: it rejects the entitlement gather, surfaces as the catch-all "catalog sync skipped" warning, and permanently un-readies the process. That is the same class of blast radius the report describes, and it is closed here. Closes #5181
An authenticated unsupported-model refusal is remembered for six hours under (account id, model id) with no credential generation. Re-auth of the same pool account keeps the internal id and increments the generation, and can swap the subscription underneath it — so the previous credential's refusal keeps steering routing away from an account that now has access. The account-wide forget that would have covered this (`model-entitlements.ts`) sits behind a condition requiring a previously cached roster with changed identity, so with no roster it never runs — which is exactly the state the flagship request path is usually in, since nothing on it refills the five-minute roster cache. Entries now carry the generation they were observed under, and: - the reader drops evidence whose generation is no longer live, so superseded evidence stops influencing the replacement without depending on the conditional forget; - a write from an older generation cannot overwrite newer evidence, so an in-flight generation-G refusal landing after G+1 is saved neither resurrects nor rewrites it; - a clear from an older generation cannot delete newer evidence, so a late generation-G success cannot re-admit an account the current credential has just been refused by. An equal generation still clears: that is the ordinary "this account just served this model" case. Evidence that cannot name a generation is dropped rather than attributed to whatever is current. Main-account contexts carry no pool generation, and already carry no account id, so nothing changes there. Cache-only routing, the six-hour TTL, the entry bound, positive-roster precedence and empty-filter restoration are all untouched. The liveness predicate is injected rather than imported so the store stays a leaf module; production wires `isCodexAccountGenerationLive`. Closes #4952
…ore they evict Three defects in the carried #5092, found by adversarial review of the generation fences rather than by CI, which was green on all three. 1. main-pool evidence was silently discarded. Both production call sites pass `kind === "pool" ? generation : undefined`, and the carried code dropped any refusal that could not name a generation. A `main-pool` context - the stored main login taking part in rotation - has a real accountId and no POOL credential generation, because its credential lives in auth.json. So for that account #4906 reverted: the pool would re-send the model the login had just refused, on every request. A refusal with no generation is now ACCOUNT-scoped and the generation fences do not apply to it. This is the rule core-codex-account.ts already uses for the quota writer, where an absent credential generation skips the liveness check instead of discarding the write. Request-owned `main` is unaffected: its accountId is null and the existing guard returns before any of this. 2. The stale-write fence only rejected a late refusal when the same key already held newer evidence. With no entry for its own key the stale row was inserted, and at the 512-entry bound that insert evicts the oldest valid row - which no later read fence can restore, because the evidence is gone. The write now rejects a generation that is no longer live before touching the map at all, which closes the race the issue names rather than only its common case. 3. The read fence validated liveness for every non-expired entry before checking the model or the caller's exclusion set, and DELETED any row that came back not-live. Two problems. The issue asks for validation after the exclusion read fence, and an excluded account - a draining profile switch, or a request-owned credential - was causing a credential-store read on its behalf. And the predicate cannot tell "this account reauthenticated" from "the store could not be read": loadCodexAccountRecordStore catches a read failure and returns {}, so an unreadable store deleted valid evidence permanently. The reader now matches the model and applies the exclusion set first, and SKIPS a non-live row instead of deleting it. Skipping already produces the routing outcome on every read; deletion only saved memory in a bounded map, and it was the part that was unsafe. Superseded rows still leave by TTL, by eviction, and by the account-wide forget. Each liveness check also reloaded and reparsed the whole credential store per row, on the request path. The seam is now a factory that opens once per lookup over one loadCodexAccountRecordSnapshot, which is the shape that snapshot was added for, and it is not opened at all when no matching row carries a generation. Every constraint the issue sets is unchanged: cache-only routing with no added upstream fetch, the six-hour TTL, the 512-entry bound, positive-roster precedence, and empty-filter restoration. This remains an ordering preference, not an eligibility filter, and a live-generation refusal still denies. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
1e9b42d to
3252e66
Compare
리뷰 · 우선순위 64 / 80이 PR은 계정 하나가 고장 나도 프록시 전체가 같이 멈추지 않게 막아요. 이미 켜질 때 Codex 목록 맞추기가 경고만 남기면, 예전 모델이 거절된 기록에는 로그인 세대 번호를 붙여요. 같은 계정을 다시 로그인하면 계정 이름은 그대로이고 세대만 올라가요. 죽은 세대의 거절은 다음 요청에서 건너뛰고, 늦게 온 옛 거절은 저장하지 않아요. 메인 로그인이 계정 회전에 끼는 계정 목록은 재로그인이 필요한 이유를 겹친 예/아니오로 짐작하지 않아요. 조사 함수가 본 이유를 그대로 적어요. 사용량 조회가 거절하면 src/codex/auth-api/account-list.ts:113 - 강제 새로고침 직후에는 이유가 맞아요. 그런데 그 조사가 계정에 재로그인 표시도 켜요. 사용량 기록이 5분( 메인테이너의 판단이 필요한 지점
#5191은 닫으면 안 돼요. 두 번째 커밋 너의 추천 5분 캐시로 목록을 다시 읽을 때도, 조사 때 본 이유를 유지하세요. 재로그인 표시만 켜져 있고 이번 결과에 이유가 없으면 이 댓글은 grok-bot이 작성했습니다 |
|
All three defects in the #5092 carry are real and the fixes are right. Taking them one at a time, because two of them are mistakes I'd make again without writing down why. 1. 2. The stale-write fence. I fenced on "does this key already hold newer evidence", which is only half the question. With no entry for its own key the stale row goes in, and at the 512-entry bound that insert evicts the oldest valid row — which no read fence can put back. Rejecting a non-live generation before touching the map is correct; my version protected the row and left the container unprotected. 3. The read fence deleting rows it could not prove superseded — this is the one I got most wrong, and I argued for it in the PR body. I chose delete-over-skip deliberately and described it as making the fix "self-healing". It isn't self-healing, it's destructive, and I gated it on a predicate that cannot tell "proven superseded" from "couldn't tell": // account-store.ts
} catch {
backupInvalidConfig(path);
return {}; // <- every isCodexAccountGenerationLive() now returns false
}One unreadable or corrupt The general form, which is what I'll carry forward: a destructive action must not be conditional on a predicate that fails closed to "false" when it simply couldn't determine the answer. Skip is recoverable, delete is not, and the asymmetry should decide the default. Also fair: reloading and reparsing the whole credential store per row on the request path was a straightforward performance defect I didn't consider at all — the seam I introduced invited it, and a factory opening once per lookup is the obvious shape in hindsight. Verified No disagreement with any of it. |
3 participants
Summary
Three layers of the same question: how far should one account's failure be allowed to spread?
ec96bbbde0poolAccountDtoreports the reauthentication cause the probe observed instead of inferring it from overlapping booleans8a44021ad9/readyzinto a terminal failed statefa9143bbcb1e9b42d0f2main-poolevidence, a stale write that could evict valid evidence, and a read fence that deleted evidence it could not prove was supersededCloses #5181
Closes #4952
#5181 — readiness
/readyzwent terminal-failed on any nonemptywarningfrom the post-startup Codex sync. A warning there describes artifacts written into the local Codex home: no catalog source so Codex keeps its native catalog, combos omitted from the catalog, a conversation-history relabel left to Codex's own writer, or a caught catalog-refresh exception after which config injection still runs and still reports its ownok. The sync continues past every one of them.The reporter's single-replica Kubernetes deployment lost its only Service endpoint while every non-Codex route stayed healthy. Switching the probe to
/healthzrestored service, which is their own evidence that the process was viable.okandwarningare separate fields because they answer separate questions.okis the sync's verdict on the essential work — the write admission and the config injection. The gate now followsokalone.The narrowing is only to
warning. A throw, anulloutcome andok !== truestay terminal, so the service-home write refusal, the external-provider injection failure, the config/integrity preflight refusal and the final injection failure all still fail readiness. Readiness also still stays pending until the sync settles, which is the race the gate was built for. This is the boundary the Claude Code roster reconciliation already has — it delays the ready transition without being allowed to fail it — applied to the catalog sync.The brief for this work was wrong about the mechanism, and that is recorded here rather than papered over. The "Codex accounts … need reauthentication" line in the report comes from
warnGatedNativeSuppressedOnce(src/codex/catalog/gated-native-warn.ts), a bareconsole.warnthat never enters the structured result, and per-account pool refresh failures are caught and returned asnullbymodel-entitlements.tsrather than propagating. So "a pool account needs reauth" is not by itself a readiness failure. The account-level fault that does reach the gate is a native main credential refresh failing outside that per-account catch: it rejects the entitlement gather, surfaces as the catch-allcatalog sync skippedwarning, and permanently un-readies the process. Same blast radius, different route, and closed here.Also worth recording for whoever reads this next:
scripts/ci/docker-smoke.tsruns inruntimeRole: "hub"and pre-writes a synthetic catalog, so it exercises the skip path and never the standalone-container path this issue is about.#4952 — denial evidence
fa9143bbcbis @abhisheksharma2411's PR #5092, carried unchanged.1e9b42d0f2fixes three defects that exact-head CI was green on:main-poolevidence was discarded. Both call sites passkind === "pool" ? generation : undefined, and the carried code dropped any refusal that could not name a generation. Amain-poolcontext — the stored main login in rotation — has a realaccountIdand no pool credential generation, because its credential lives inauth.json. For that account [Bug]: v2.57.0 still selects Free account for Sol/Astra after roster refresh #4906 reverted: the pool would re-send the model the login had just refused, on every request. A refusal with no generation is now account-scoped, which is the rulecore-codex-account.tsalready uses for the quota writer.loadCodexAccountRecordStorecatches a read failure and returns{}, so an unreadable store deleted valid evidence permanently. The reader now matches the model and applies the exclusion set first, and skips a non-live row instead of deleting it.Each liveness check also reloaded and reparsed the whole credential store per row on the request path; the seam is now a factory that opens once per lookup over one
loadCodexAccountRecordSnapshot, and is not opened at all when no matching row carries a generation.Cache-only routing, the six-hour TTL, the 512-entry bound, positive-roster precedence and empty-filter restoration are all unchanged. A live-generation refusal still denies.
Not in this PR
#5019 was dropped deliberately. It is already implemented by open PR #5024 (
codex/lane-t-request-owned-main) from the same maintainer, touching the sameresolveCodexAuthContextpath. A second implementation here would be a competing change to the credential fence, not a contribution. Separately, review of that path found the change is not acceptable on static evidence alone: relaxing the main-liveness predicate inside the stateful resolver mutatesactiveCodexAccountId, cooldown identity, and Direct/Pool/API independence before the selected-main branch can divert, so it needs a read-only preview seam, exact-head hosted tests and the security reviewMAINTAINERS.mdrequires for credential changes. That belongs in #5024's lane.File-size ratchet
tests/codex-integration/codex-auth-api.test.tsis capped at 6549 and sits at 6522. #5191 added 38 lines inline, which would have taken it to 6560 and broken the ratchet for this branch and every branch cut fromdevafterwards. The two cases are registered fromtests/helpers/pool-reauth-cause.tsthrough the sameregisterXCasesseam the file already uses twice, leaving it at 6525 with the assertions byte-identical.tests/server/server-live.test.tsis at its 2253 cap exactly and did not grow.Verification
No local execution. This lane is under an absolute no-local-run constraint: no test suite, no individual test, no
typecheck, no build, no install, noocx. A previous local run in this repository destroyed real~/.opencodexdata. Nothing here was executed locally, and no runtime evidence is claimed from this machine.Verification was static, plus adversarial review, plus hosted CI at the exact head:
warningassignment sites insrc/codex/sync.ts, the per-accountcatchinmodel-entitlements.tsthat swallows pool refresh failures against the native-main branch that does not, themain-poolshape insrc/codex/auth-context.ts, andreadCodexAccountRecordbeing exactlyloadCodexAccountRecordStore()[id].gpt-5.6-sol, high effort), each asked whether the change weakens a production contract and whether a real outage still fails. They refuted the brief's stated mechanism for readiness: Codex re-auth failure takes healthy multi-provider proxy out of service #5181, found all three fix(codex): scope model denial evidence to the credential generation #5092 defects fixed in1e9b42d0f2, and produced the Pool routing never selects the main login automatically when the request carries that login's own bearer #5019 deferral.tests/fixtures/file-size-baseline.jsonfor every touched file, since caps only move downward.Contract documentation was updated with the behaviour change: the
src/server/readiness.tsheader, the driving comment insrc/codex/desired-state.ts, and the Startup readiness section ofstructure/catalog.md.Tests changed or added: the warning case in
tests/server/proxy-liveness.test.tsnow expects ready across all four real warning producers, with new cases provingok=falseand a missingokstill fail;tests/server/server-live.test.tsgains anok=falseHTTP case so 503-after-settle stays covered;tests/codex-integration/codex-desired-state.test.tspins the boundary at the caller; andcodex-model-denial-evidence.test.tsgains cases for account-scopedmain-poolevidence, the capacity-eviction race, the exclusion fence, and one store open per lookup.Rebased onto
c81d43053bAn earlier head of this branch failed
file-size ratchet: repositoryon two files it never touched —src/codex/history-provider.ts(2009 lines, no cap) andtests/server/server-combo-failover-e2e.test.ts(4192 against a 4166 cap) — plus three macOS Reserve cases. All of those were inherited fromdevand are fixed ondevby #5201, which split the rollout reader intosrc/codex/history-rollout-read.ts(1849 lines now) and moved the combo cases intotests/helpers/combo-tool-routing-cases.ts(4153 lines now).This branch is rebased onto that tip. The rebase was clean with no conflicts, commit authorship is preserved for both carried commits, and none of the five files #5201 touched overlaps the seventeen this PR touches. Every file this PR does touch is within its cap at the rebased head, with
codex-auth-api.test.tsat 6525 of 6549 andserver-live.test.tsat exactly its 2253 cap.Checklist
Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com
Co-authored-by: Abhishek Sharma abhicse24@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation