fix(codex): promote a healthy detour instead of releasing it, and honour Retry-After (#4546) - #4616
Conversation
…our Retry-After (#4546) Refs #4546. PRD R07. When a transient hold outlived its window, routing deleted the whole affinity entry -- including the detour account that had actually been serving the thread -- and re-picked cold. The timer expiring restores the right to re-decide; it is not itself a recovery, and treating it as one threw away the single piece of evidence the request had. A still-healthy detour is now promoted to the binding instead, with the move recorded as rebound/transient_hold_expired so the reason is visible. A detour that has itself gone unhealthy still falls through to the cold path. Retry-After is a lower bound on the transient path. The local maximum delay bounds our own exponential backoff and has no business shortening a wait the provider stated: sending early is a request we already know will be refused, which is the storm the header exists to prevent. It is opt-in per caller so the change lands on the transient path first rather than silently lengthening every adapter's backoff, and an honoured wait is ceilinged at one minute so an hour-long Retry-After cannot park a request.
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. |
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
리뷰 · 우선순위 70 / 80설명 이 PR(#4616)은 #4546 cost-guard 열차의 R07 조각이다. 지금 문제의 핵심은 아주 단순한 오해였다. 스레드가 잠깐 다른 계정(detour)으로 우회하는 동안 affinity 엔트리에는 고친 뒤의 동작은 두 번째 축은 현재 테스트는 라인 2946-2973 / src/codex/routing.ts - 홀드 만료 시 healthy detour 승격 분기가 들어갔고 의도는 맞다. 다만 같은 동작을 고정하는 routing 단위 테스트가 없어, 이후 affinity 리팩터에서 조용히 깨질 위험이 있다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: f5e878a534
ℹ️ 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".
| if (!isIndependentCodexQuotaScope(quotaScope)) promoteActiveCodexAccount(config, expiredDetour); | ||
| bindThreadAffinity(threadId, expiredDetour, now, quotaScope); | ||
| return { | ||
| status: "selected", | ||
| accountId: expiredDetour, | ||
| affinity: { move: "rebound", reason: "transient_hold_expired" }, |
There was a problem hiding this comment.
When an ordinary binding's hold has expired and its recorded detour is healthy, this branch now resolves to the detour, but previewReusableAffinityAccount still returns null for the expired hold and lets previewCodexAccountForRequest make a fresh strategy pick. In the subagent fallback path, preview can therefore score a different account—and potentially choose a different model—than final authentication uses. Apply the same side-effect-free expired-detour selection in preview and add an expiry case asserting preview/resolve equality.
AGENTS.md reference: src/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| const expiredDetour = entry.transientDetourAccountId; | ||
| if ( | ||
| isTransientHoldExpired(entry, now) | ||
| && generationLive |
There was a problem hiding this comment.
This promotion is reachable only for the ordinary affinity entry. If a model-scoped affinity account fails and detours to a healthy model-eligible sibling, the earlier detourEntry branch still deletes that model affinity when the hold expires before execution can reach this block. With another eligible sibling, the subsequent cold strategy selection may move the conversation away from the account that served throughout the hold, recreating the cache-loss behavior this change fixes. Apply equivalent promotion to model-detour affinities and cover that expiry path.
AGENTS.md reference: src/AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| // A provider that names a wait is stating when it will serve again; sending earlier is a | ||
| // request we already know will be refused, and refusing it twice is the retry storm the | ||
| // header exists to prevent. The local maximum bounds our OWN exponential backoff and has no | ||
| // business shortening someone else's instruction. The ceiling is separate: it stops an | ||
| // hour-long Retry-After from parking a request forever. | ||
| const ceiling = opts.retryAfterCeilingMs ?? RETRY_AFTER_CEILING_MS; | ||
| return Math.min(Math.max(retryAfter, jittered), ceiling); |
There was a problem hiding this comment.
The commit changes shared account routing under src/codex/ and retry transport behavior under src/lib/, but it changes no structure/ file. The source instructions require the same change to update every structure document mapped to each affected source area; at minimum, the existing affinity contract in structure/providers/openai-tiers.md and retry contract in structure/transports/responses.md now omit the new promotion and Retry-After semantics. Synchronize all owners listed by structure/INDEX.md.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
1 participant
Summary
Refs #4546. PRD R07.
When a transient hold outlived its window, routing deleted the whole affinity entry — including the detour account that had actually been serving the thread — and re-picked cold. The timer expiring restores the right to re-decide; it is not itself a recovery, and treating it as one threw away the single piece of evidence the request had. A still-healthy detour is now promoted to the binding, recorded as
rebound/transient_hold_expiredso the reason stays visible. A detour that has itself gone unhealthy still falls through to the cold path, and generation or quota refusals still release as before.Retry-Afterbecomes a lower bound on the transient path. The local maximum delay bounds our own exponential backoff and has no business shortening a wait the provider stated — sending early is a request we already know will be refused, which is the storm the header exists to prevent. It is opt-in per caller so the change lands on the transient path first rather than silently lengthening every adapter's backoff, and an honoured wait is ceilinged at one minute so an hour-longRetry-Aftercannot park a request.Still out of scope
The transient half-open probe lease is not in this PR: a soft-avoided account still receives no traffic, so the two-consecutive-success clearing rule is met through the held fallback rather than a single leased probe.
Verification
Three new cases in
tests/lib/upstream-retry.test.tspin the opt-in lower bound, the ceiling, and that opting in never shortens below local backoff. The existing cap test does not opt in and keeps its behaviour. Local suite NOT RUN by policy; hosted CI at the exact head is the evidence.Checklist
dev