[WRONG BRANCH] 修复 routed 模型路径 429 限流重试缺失 - #984
Conversation
…sient 状态码中增加 429 修复 Codex Desktop 使用 DeepSeek 等非 OpenAI 模型时频繁触发 429 限流的问题。 原逻辑只对 passthrough 路径（ChatGPT 后端）使用带重试的 fetchWithTransientRetry， 而 routed 模型路径仅使用 fetchWithResetRetry（仅重试 TCP 连接错误）， 导致 429 直接传递给客户端，Codex 多次重试后报 exceed retry limit。 改动： - upstream-retry.ts: isTransientUpstreamStatus 增加 429 状态码 - core.ts: routed 路径和 continuation 路径改用 fetchWithTransientRetry
|
This pull request currently targets @blairevan Please retarget this PR to Its title has been prefixed with This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughThis change adds HTTP 429 to transient upstream statuses and switches the routed adapter and terminal continuation request paths to Changes429 transient retry handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
2272-2282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd transient retry handling to the MiMo Free adapter.
src/server/responses/core.ts:2264-2270callsactiveAdapter.fetchResponsedirectly.src/adapters/mimo-free.ts:232-260retries only401, so a429bypassesfetchWithTransientRetry. With fewer than two pooled keys, the recovery loop returns that429unchanged. Add equivalent transient retry handling to MiMo Free or enforce this contract for everyfetchResponseimplementation. The continuation path is Anthropic-only and does not definefetchResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 2272 - 2282, Update the MiMo Free adapter’s fetchResponse implementation to apply fetchWithTransientRetry around its upstream request, preserving the existing 401 key-recovery behavior while retrying transient responses such as 429. Ensure the recovery loop does not return an unretried 429 when the key pool has fewer than two entries, and keep the Anthropic-only continuation path unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/fix-429-transient-retry.md`:
- Line 7: Update the fenced code blocks in docs/fix-429-transient-retry.md at
the referenced sections to include an appropriate language identifier such as
text, add the required blank lines around the indented log block, and ensure the
file ends with exactly one trailing newline.
- Around line 31-36: Update docs/fix-429-transient-retry.md in the retry table
to include HTTP 429 in fetchWithTransientRetry’s status list, rename the second
changed path to fetchTerminalGuardContinuation, and clarify that the three
attempts are transient-response retries delegated through fetchWithResetRetry
rather than necessarily the total network-attempt count.
In `@src/lib/upstream-retry.ts`:
- Around line 38-40: Add regression tests covering isTransientUpstreamStatus and
the retry flow for a replayable request returning 429 then 200: verify another
attempt occurs, the transient response body is canceled, Retry-After is
respected, and an aborted signal prevents further attempts. Add integration
tests exercising both call sites changed in responses/core.ts, reusing existing
test helpers and patterns.
- Around line 38-40: The retry flow currently records every transient status as
“transient-5xx”, mislabeling HTTP 429 recovery metadata. Update
fetchWithTransientRetry and noteAttemptSend to use a 429-specific or neutral
recovery kind, then update all recovery-kind validators and consumers
accordingly; revise tests/upstream-transient-retry.test.ts to treat 429 as
transient and add an assertion that it retries.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 2272-2282: Update the MiMo Free adapter’s fetchResponse
implementation to apply fetchWithTransientRetry around its upstream request,
preserving the existing 401 key-recovery behavior while retrying transient
responses such as 429. Ensure the recovery loop does not return an unretried 429
when the key pool has fewer than two entries, and keep the Anthropic-only
continuation path unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 146f1149-a35a-47bf-9e87-5d5c1ebc278a
📒 Files selected for processing (3)
docs/fix-429-transient-retry.mdsrc/lib/upstream-retry.tssrc/server/responses/core.ts
|
|
||
| 在 Codex Desktop 中使用 `tencent/deepseek-v4-pro` 模型时，频繁报错： | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Markdown lint warnings.
Add a language identifier such as text to the fenced blocks at Lines 7, 19, and 120. Surround the indented log block at Lines 120-122 with the required blank lines. End the file with one trailing newline.
Also applies to: 19-22, 120-122
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fix-429-transient-retry.md` at line 7, Update the fenced code blocks in
docs/fix-429-transient-retry.md at the referenced sections to include an
appropriate language identifier such as text, add the required blank lines
around the indented log block, and ensure the file ends with exactly one
trailing newline.
Source: Linters/SAST tools
| | 路径 | 使用的重试函数 | 重试范围 | | ||
| |------|---------------|---------| | ||
| | **passthrough**（ChatGPT 后端） | `fetchWithTransientRetry` | TCP 连接错误 + 500/502/503/504/520/521/522 | | ||
| | **routed**（DeepSeek 等非 OpenAI 模型） | `fetchWithResetRetry` | **仅 TCP 连接错误**（ECONNRESET/EPIPE） | | ||
|
|
||
| `fetchWithResetRetry` 只重试 TCP 层面的连接断开，**不重试 HTTP 层面的错误状态码**。当上游返回 429 时，它直接透传给 Codex Desktop。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the retry documentation with the implementation.
Update the document in three places:
- Lines 33-34 omit
429from thefetchWithTransientRetrystatus list, althoughsrc/lib/upstream-retry.tsLine 39 now includes it. - Lines 84-97 call the second changed path a web-search continuation. The changed code is
fetchTerminalGuardContinuationatsrc/server/responses/core.tsLines 2492-2516. - Lines 103-107 describe three transient attempts. Each attempt delegates to
fetchWithResetRetry, so clarify that this is the transient-response retry count, not necessarily the total number of network attempts.
Incorrect path names and status lists can send operators to the wrong code path during incident analysis.
Also applies to: 82-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fix-429-transient-retry.md` around lines 31 - 36, Update
docs/fix-429-transient-retry.md in the retry table to include HTTP 429 in
fetchWithTransientRetry’s status list, rename the second changed path to
fetchTerminalGuardContinuation, and clarify that the three attempts are
transient-response retries delegated through fetchWithResetRetry rather than
necessarily the total network-attempt count.
| export function isTransientUpstreamStatus(status: number): boolean { | ||
| return status === 500 || status === 502 || status === 503 || status === 504 | ||
| return status === 429 || status === 500 || status === 502 || status === 503 || status === 504 | ||
| || status === 520 || status === 521 || status === 522; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the 429 retry contract.
Add a unit test for a replayable request that returns 429 and then 200. Assert that the retry occurs, the transient response body is canceled, Retry-After is honored, and an aborted signal stops further attempts. Add integration coverage for both changed call sites in src/server/responses/core.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/upstream-retry.ts` around lines 38 - 40, Add regression tests
covering isTransientUpstreamStatus and the retry flow for a replayable request
returning 429 then 200: verify another attempt occurs, the transient response
body is canceled, Retry-After is respected, and an aborted signal prevents
further attempts. Add integration tests exercising both call sites changed in
responses/core.ts, reusing existing test helpers and patterns.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the recovery-kind contract and all consumers.
rg -n -C 8 \
'transient-5xx|AttemptRecoveryKind|applyUpstreamRecoveryInit|noteAttemptSend' \
src --glob '*.ts'Repository: lidge-jun/opencodex
Length of output: 43741
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- upstream retry implementation ---'
sed -n '1,255p' src/lib/upstream-retry.ts
printf '%s\n' '--- recovery metadata recording ---'
sed -n '930,985p' src/server/request-log.ts
printf '%s\n' '--- recovery-kind consumers and tests ---'
rg -n -C 6 \
'recoveryKinds|transient-5xx|key-429|anthropic-oauth-429|applyUpstreamRecoveryInit|isTransientUpstreamStatus|fetchWithTransientRetry' \
src test tests 2>/dev/null || trueRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- focused transient retry tests ---'
sed -n '1,260p' tests/upstream-transient-retry.test.ts
printf '%s\n' '--- all exact recovery callback assertions ---'
rg -n -C 4 \
'recoveryKinds|recovery\)|recovery\s*=>|firstRecovery|transient-5xx|connection-reset' \
tests --glob '*.test.ts' --glob '*.spec.ts' \
| head -n 500Repository: lidge-jun/opencodex
Length of output: 12349
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
retry = Path("src/lib/upstream-retry.ts").read_text()
tests = Path("tests/upstream-transient-retry.test.ts").read_text()
request_log = Path("src/server/request-log.ts").read_text()
status_match = re.search(
r"return status === 429 \|\| status === 500 .*?status === 522;",
retry,
re.S,
)
assert status_match and "status === 429" in status_match.group(0)
assert "for (const s of [200, 400, 401, 429, 499, 507, 529])" in tests
assert "expect(isTransientUpstreamStatus(s)).toBe(false)" in tests
assert 'export type UpstreamSendRecovery = "connection-reset" | "transient-5xx";' in retry
assert 'res = await fetchWithResetRetry(doFetch, opts, "transient-5xx");' in retry
assert 'if (recovery !== "connection-reset")' in retry
assert 'attempt.recoveryKinds.push(recovery)' in request_log
print("implementation: 429 is classified as transient")
print("test: 429 is still expected to be non-transient")
print("callback: every status retry is tagged transient-5xx")
print("request init: only connection-reset changes headers/keepalive")
print("metadata: recovery kind is persisted in recoveryKinds")
PYRepository: lidge-jun/opencodex
Length of output: 421
Use a recovery kind that matches the transient status.
429 now enters fetchWithTransientRetry, but line 236 records "transient-5xx" for every status retry. noteAttemptSend persists this value in recoveryKinds, making 429 metadata inaccurate. Add a 429-specific or neutral recovery kind, update its validators and consumers, and update tests/upstream-transient-retry.test.ts, which still expects 429 to be non-transient. Add a 429 retry assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/upstream-retry.ts` around lines 38 - 40, The retry flow currently
records every transient status as “transient-5xx”, mislabeling HTTP 429 recovery
metadata. Update fetchWithTransientRetry and noteAttemptSend to use a
429-specific or neutral recovery kind, then update all recovery-kind validators
and consumers accordingly; revise tests/upstream-transient-retry.test.ts to
treat 429 as transient and add an assertion that it retries.
![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: 427470752f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| */ | ||
| export function isTransientUpstreamStatus(status: number): boolean { | ||
| return status === 500 || status === 502 || status === 503 || status === 504 | ||
| return status === 429 || status === 500 || status === 502 || status === 503 || status === 504 |
There was a problem hiding this comment.
When a Responses stream emits response.failed with error.status: 429, responsesSseToAnthropicSse consults this predicate and now emits Anthropic overloaded_error instead of the existing rate_limit_error; the focused tests/claude-outbound.test.ts case for a streamed 429 fails with exactly that type change. Please keep this shared error-mapping predicate limited to gateway transients or introduce a separate pre-stream retry predicate for HTTP 429.
AGENTS.md reference: src/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| }); | ||
| } else { | ||
| upstreamResponse = await fetchWithResetRetry( | ||
| upstreamResponse = await fetchWithTransientRetry( |
There was a problem hiding this comment.
For routed providers with an apiKeyPool, wrapping the initial fetch in fetchWithTransientRetry consumes 429 responses before the existing key-failover loop can cool the failed key and rotate. In the existing server-key-failover-e2e scenario where key A returns 429 and the retry succeeds, the second request now still uses key A, so the pool never records the cooldown or advances to key B; persistent 429s also wait through same-key backoff before failover. Please bypass transient 429 retry when key-pool failover is available, or perform rotation inside the retry path.
Useful? React with 👍 / 👎.
| */ | ||
| export function isTransientUpstreamStatus(status: number): boolean { | ||
| return status === 500 || status === 502 || status === 503 || status === 504 | ||
| return status === 429 || status === 500 || status === 502 || status === 503 || status === 504 |
There was a problem hiding this comment.
With Codex account-pool passthrough, shouldRetryCodexPoolAccountQuota only runs after this generic retry layer returns. If account A's first 429 carries Retry-After but the later same-account retries omit it or shorten it, recordCodexUpstreamOutcome sees only the final 429, so the depleted account is cooled using the wrong/default window before trying account B. Please skip generic 429 retry for usesCodexForwardPoolAuth requests, or preserve the first 429 metadata for the pool retry.
Useful? React with 👍 / 👎.
| */ | ||
| export function isTransientUpstreamStatus(status: number): boolean { | ||
| return status === 500 || status === 502 || status === 503 || status === 504 | ||
| return status === 429 || status === 500 || status === 502 || status === 503 || status === 504 |
There was a problem hiding this comment.
Adding 429 to this shared retry predicate sends rate-limit responses through retryBackoffDelayMs, which caps Retry-After at the transient retry max of 5 seconds. For an upstream 429 with Retry-After: 60, opencodex will retry the same request after only 5 seconds (twice), violating the upstream's backoff directive and making throttling worse. Please use a 429-specific backoff path that preserves the advertised delay, or return the 429 when the delay exceeds the bounded transient-retry budget.
Useful? React with 👍 / 👎.
|
Thanks for The Pr. I will be closing it as Duplicate. #865 already implements 429 Hardening and also has better documentation. |
2 participants
问题
在 Codex Desktop 中使用非 OpenAI 模型（如
deepseek-v4-pro）时，频繁触发 429 限流错误：根因
routed 模型路径（非 OpenAI 模型）使用
fetchWithResetRetry，该函数仅重试 TCP 连接错误（ECONNRESET/EPIPE），不重试 HTTP 层面的 429。而 passthrough 路径（ChatGPT 后端）已正确使用fetchWithTransientRetry。后续的 429 recovery loop 依赖
hasKeyPoolFailover——单 Key 配置下直接返回 false，429 原样透传给客户端。详见 docs/fix-429-transient-retry.md
修改
src/lib/upstream-retry.ts:isTransientUpstreamStatus增加429src/server/responses/core.ts: routed 路径和 continuation 路径的fetchWithResetRetry改为fetchWithTransientRetrySummary by CodeRabbit
Bug Fixes
Documentation