fix(codex): keep a bound thread's prompt cache and tell configured routing apart from adopted routing - #4581
Conversation
Under the quota account-pool strategy a live thread binding could be moved to any strictly-cooler eligible account once its account crossed autoSwitchThreshold. "Strictly cooler" had no floor, so once every account sat in the 80-100% band the coolest was still over the threshold and a long-running conversation was handed from account to account on consecutive turns. Codex prompt caches are account-isolated, so each hop restarted from a cold prefix. A live binding may now only move to an account that clears the same bar reset-first already applied through hasCodexQuotaHeadroom, and that is also strictly cooler than the bound account. Headroom alone is not enough, because that predicate answers true for unknown usage, which is the right default for an unbound pick and a guess when a warm prefix is at stake. Movement is now bounded by the number of accounts rather than the number of turns, and every release path is untouched: a 429/402 refusal, failover, exhaustion, pause and generation checks all still drop the binding before the preference rule is consulted. Refs #4546
structure/manifest.json lists providers/openai-tiers.md as a doc for src/codex/, so narrowing the quota-strategy live-rebind rule obliges the same change here. Records why both bars are load-bearing, what the rule deliberately does not change, and that the rule is written twice on purpose. Also folds an independent roadmap review into devlog/_plan: the Codex-client process set must not be the app-server lister, adopted stays an opportunity inference, and tests extend existing subsystem files rather than adding new ones. Refs #4546
Four cases next to the existing cache-affinity tests. The first is the #4546 death spiral and fails without the fix: with a=95, b=90, c=97 the pre-fix rule handed the thread to whichever account was one point cooler on every resolve. The scores are deliberately unequal, since equal scores would not move even before the fix and would pass for the wrong reason. The other three keep the change a narrowing rather than a pin: a thread still moves onto an account with genuine headroom, a 429 still releases a binding the preference rule would have kept, and a fully spent account still yields to a sibling with headroom. Preview is asserted beside every resolve because the rule is written twice. Refs #4546
ocx status printed routing=opencodex-local straight from config on disk, so an operator read it as proof that their running Codex CLI was pooled. A client that started before the route was injected keeps dialing chatgpt.com directly, which is what #4550 reports: repeated usage_limit_exceeded while a healthy secondary account sat in the pool, and no proxy usage record for those turns. Status now carries adoption evidence. A running Codex client whose start time precedes the recorded injection cannot have read the injected route, so it is named as pending-client-restart with its pid, and the summary says to restart it. Anything unverifiable stays unknown rather than claiming a clean bill of health: a failed process walk is distinct from an empty one, because an empty walk means "no clients running" and would read as adopted. adopted is deliberately an inference about opportunity, not an observation of live traffic, and the module comment names every false-adopted source this evidence does not cover, including same-second starts, an empty match set, a different CODEX_HOME, and Codex surfaces the CLI predicate never matches. The Codex CLI matching rules were private and reachable only through a count, so they are now one exported predicate with two callers rather than a second copy. The adoption field is optional and the routing detail string keeps its old shape as a prefix, so the GUI and server consumers of StartupHealth are untouched, and a stale client never changes status, protection, rebootSafe or recommendedCommand. Refs #4550
Twelve cases through the pure derivation and the injected lister and start-time seams, so nothing reads a real process table or the journal. A pre-injection client is named with its pid, a post-injection client is adopted, a start in the same wall-clock second is not stale, and enumeration failure, a missing injection time and an unreadable start time each resolve to unknown rather than to a clean bill of health. A stale client outranks an unreadable one. The detail string is asserted byte-identical to the old routing=..., service=..., shim=... form when nothing is pending, which is why the existing assertions in this file still hold, and a stale client is asserted not to move status, protection, rebootSafe or recommendedCommand. Refs #4550
|
Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (20)
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. |
리뷰 · 우선순위 64 / 80지금 이 PR은 두 덩어리입니다. (1) 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Merging into dev under the single-maintainer dev integration clause in MAINTAINERS.md. Exact-head evidence at 4b22b25: 30 successes, 2 skips. The two cancelled entries are label and enforce-target runs that later runs at the same SHA superseded with successes. Reviewed independently before merge, and the review is worth recording because it is narrower than the pull request title suggests. src/codex/routing.ts routes live quota rebinds through a cache-safe replacement check at both the affinity re-evaluation and the reusable-account preview, so a bound thread can move only to an account with real quota headroom that is also strictly cooler. That is what stops the all-hot ping-pong that was discarding the account-isolated prompt-cache prefix, and the new cases pin stay-put, a single move onto real headroom, release on 429, and exhaustion with a sibling. The spiral is genuinely prevented rather than made less likely. What is not done: a below-threshold sibling still takes the thread once, so the cache is lost once before it goes sticky, and unbound subagent cascading is untouched. The pooled-status half is honest reporting, not transport interception. A pre-injection matched CLI is now named as pending-client-restart with its pid, adopted stays an inference, and the routing string is byte-identical for the adopted and unknown cases. So the printed claim is accurate when a stale CLI is found, and it is still not proof about live traffic when the matcher misses a process or a resumed WebSocket stays direct. Because of those residuals I am leaving both issues open with the landed scope recorded on them rather than closing them against this merge. Local suite runs: NOT RUN. Hosted CI at the exact head is the proof of record. |
The L2 lane unit shipped its headroom floor while this unit was in flight. Both quota call sites use pickCacheSafeQuotaReplacement unchanged; this unit covers what that PR left open.
The #4581 conflict boundary cut through "a fully spent bound account still moves to a sibling with headroom", so the merged file lost its closing brace and the whole file failed to parse.
… case The destination rule is only reachable when a threshold crossing can move a bound thread, which is now what pool.cacheAffinity: false selects.
#4580) * docs(devlog): roadmap the #4546 cost-guard stabilization program Eight-phase plan: hold a live binding for cache, release it only on real evidence, bound retry amplification and fan-out spend, group credentials by observed quota and cache domain, reinforce the Codex prompt cache, and report cache state honestly. * fix(codex): hold a bound thread's account for its prompt cache (#4546) Pooled routing moved a live conversation whenever its account crossed autoSwitchThreshold, and moved it to whichever account was strictly cooler with no floor under the destination. Provider prompt caches are account-isolated, so every hop re-sent the whole prefix; once a pool sat in the 80-99% band the coolest account was still over the threshold and the thread was handed on again every turn. The same predicate short-circuits the 60s re-score interval, so in that band the thread was re-scored per request. pool.cacheAffinity shipped as the opt-in cure (#4292) but defaulted off, so the install that gets hurt was exactly the install that had never heard of it. Three changes: - pool.cacheAffinity now defaults ON. A bound thread keeps its account until that account genuinely cannot serve. An explicit false restores capacity-first routing. - A bound thread may only move to a destination with genuine quota headroom and strictly lower usage, under either setting. CODEX_UNKNOWN_USAGE_SCORE is 101, so an unmeasured account is excluded without a special case. When the whole pool is hot, nobody moves. - A transient failure streak no longer deletes a live binding. The request detours to a remembered alternate while the thread keeps its home, and returns as soon as that account serves again; a hold outliving 10 minutes releases normally. This is independent of cacheAffinity, because attributing a 5xx is not a quota preference. The model-detour lane gets the same hold. Quota refusals (429/402), pauses, plan exclusion, credential invalidation, generation bumps and TTL expiry still release a binding, unchanged. The account-wide clear on a transient streak is gone: each pinned thread reaches the same detour on its own next request, and wiping the map retired bindings for quota scopes the failure never described. Preview never starts a hold or picks a fresh detour, because pickRoundRobinAccount commits and advances the ring and preview is contractually read-only; it reports the detour the request path already chose. Six existing tests encoded the old default and now state it explicitly with pool.cacheAffinity: false. * docs: align pool routing docs with cache-first bound-thread policy (#4546) Every locale of the provider reference, the accounts CLI page and the dashboard guide documented pool.cacheAffinity as opt-in and off, and described the auto-switch threshold as the bar for moving a bound task. Both became wrong when the default flipped, and a default documented in eight languages is wrong in eight languages. Also corrects the transient-failure wording: a 5xx streak no longer releases a live binding, independently of cacheAffinity. structure/providers/openai-tiers.md and the GUI strategy hints carried the same two claims. The auth-context regression now asserts the session comes home to its held account once health clears, which is what proves a late failure did not disturb the binding; under the old rule there was a newer binding to protect instead. * test(gui): update pool copy assertions to the cache-first wording (#4546) Four dashboard tests pinned the sentences that said the usage threshold rebinds a bound task. Those strings changed with the default, so the assertions moved to the claims the copy now makes: bound tasks keep affinity by default, and the threshold rebind is what an explicit pool.cacheAffinity: false restores. * docs(devlog): record that the destination rule landed as #4581 The L2 lane unit shipped its headroom floor while this unit was in flight. Both quota call sites use pickCacheSafeQuotaReplacement unchanged; this unit covers what that PR left open. * fix(test): restore a test closer dropped by the rebase splice The #4581 conflict boundary cut through "a fully spent bound account still moves to a sibling with headroom", so the merged file lost its closing brace and the whole file failed to parse. * test(codex): state capacity-first explicitly in the #4581 destination case The destination rule is only reachable when a threshold crossing can move a bound thread, which is now what pool.cacheAffinity: false selects.
1 participant
Summary
Two account-pool defects where the proxy's behaviour and its reporting disagreed with what the operator believed.
#4546 — pool rotation destroyed the prompt cache of a bound conversation. Under the
quotastrategy a live thread binding could move to any strictly cooler eligible account once its account crossedautoSwitchThreshold. "Strictly cooler" had no floor, so once every account sat in the 80-100% band the coolest was still over the threshold and a long-running conversation was handed from account to account on consecutive turns. Codex prompt caches are account-isolated, so each hop restarted from a cold prefix: the reporter measured 7k-token turns becoming 150k-token turns, and 1.9 billion tokens across 15,607 requests in about 13 hours on five accounts.A live binding may now move only to an account that has genuine quota headroom and is strictly cooler than the bound account. Headroom alone is not enough, because
hasCodexQuotaHeadroomdeliberately answers true for unknown usage — correct for an unbound pick, a guess when a warm prefix is at stake — andCODEX_UNKNOWN_USAGE_SCOREis 101, so the second bar excludes an unobserved destination without a special case. Before: with a=95, b=90, c=97 the thread moved every turn. After: it stays, and movement is bounded by the number of accounts rather than the number of turns.This is the same bar
reset-firstalready applied throughhasCodexQuotaHeadroom;quotawas the asymmetric outlier. It narrows a preference and never a refusal — a 429/402 with no success since, a failover streak, pause, cooldown, a lost generation and an unusable account all still release the binding before the rule is consulted. Unbound selection is untouched, andpool.cacheAffinitykeeps its meaning and its default as the stronger sticky-until-exhaustion opt-in.#4550 —
ocx statuspresented configured routing as adopted routing. It printedrouting=opencodex-localread from config on disk, so an operator read it as proof their running Codex CLI was pooled. A client that started before the route was injected keeps dialingwss://chatgpt.com/backend-api/codex/responsesdirectly, which is exactly what the report shows: repeatedusage_limit_exceededwhile a healthy secondary account sat in the pool, and no proxy usage record for those turns.Status now carries adoption evidence. A running Codex client whose start time precedes the recorded injection cannot have read the injected route, so it is named as
pending-client-restartwith its pid and the summary says to restart it.adoptedis deliberately an inference about opportunity, not an observation of live traffic, and the module comment names every false-adoptedsource this evidence does not cover — a client the matcher misses, a resumed thread holding an already-open direct WebSocket,OPENAI_BASE_URLoverrides, same-second starts, an empty match set, a differentCODEX_HOME, and surfaces such ascodex-code-mode-hostand VS Code extension hosts. Anything unverifiable isunknown: a failed process walk stays distinct from an empty one, because an empty walk means "no clients running" and would read asadopted.The Codex CLI matching rules were private and reachable only through a count, which can answer "is Codex busy" but cannot name a stale pid, so they are now one exported predicate with two callers rather than a second copy. The new
routingAdoptionfield is optional and the routing detail string keeps its old shape as a prefix, so the GUI and server consumers ofStartupHealthare untouched and a stale client never changesstatus,protection,rebootSafeorrecommendedCommand.structure/providers/openai-tiers.mdrecords the new rebind invariant, asstructure/manifest.jsonownership ofsrc/codex/requires. Planning notes are indevlog/_plan/260914_l2_pool_routing_cache/.Verification
Local suite runs were NOT RUN for this unit, by explicit instruction. No
bun run test, no barebun test, nobun run typecheck, nobun install, nobun run build:gui— not locally, and not by any delegated agent. The proof for this pull request is hosted CI at the exact final head SHA and nothing else; a green run at an earlier head is not evidence for a later one.4b22b25f61c254cd29a823ed125ece5233b9289a— conclusion success, all 24 checks green across Linux, macOS and Windows. That run is the only proof for this head.src/behaviour changes:tests/codex-integration/codex-pool-rotation.test.ts— four cases beside the existing cache-affinity tests. The first is the [Bug]: Account pool routing destroys prompt cache and triggers 10x-50x token burn death-spiral above 80% usage threshold #4546 death spiral and fails without the fix: with a=95, b=90, c=97 the pre-fix rule handed the thread to whichever account was one point cooler on every resolve. The scores are deliberately unequal, since equal scores would not move even before the fix and the case would pass for the wrong reason. The other three keep the change a narrowing rather than a pin — a thread still moves onto an account with genuine headroom, a 429 still releases a binding the preference rule would have kept, and a fully spent account still yields to a sibling with headroom. Preview is asserted beside every resolve because both call sites share one helper.tests/service/autostart-health.test.ts— adoption derivation through its pure and injectable seams: a pre-injection client ispending-client-restartwith its pid, a post-injection client isadopted, a same-second start is not stale, enumeration failure and unreadable start times areunknown, a stale client outranks an unreadable one, non-opencodex-localrouting isnot-applicable, the detail string stays a prefix, and a stale client leaves the restart-safety classification unchanged.adoptedwas not sound as written, and the write scope omitted thestructure/doc that ownssrc/codex/. All three were fixed. Round 2 returned NEAR-PASS and verified the committedpickCacheSafeQuotaReplacementagainst every call site, finding no case that returns the wrong account and no weakened release path.Checklist
Docs: the
autoSwitchThreshold,accountPoolStrategy, pool-routing prose and strategy table now state the headroom bar, in the English reference and in all seven translated locales. No credential, token or request content is read or logged; the adoption check reads process ids and start times plus the journal's own timestamp, and it never mutates the journal — it parsesJOURNAL_PATHdirectly rather than callingreadJournal, which can delete a corrupt file.Closes #4546
Closes #4550