fix(proxy): retain client-abort billing under high-concurrency mode - #1452
Conversation
High-concurrency mode previously disabled client-abort retention (shouldRetainClientAbortBilling -> false), causing the response handler to immediately cancel upstream and discard buffered bytes on client disconnect. Completed upstream streams that Codex already closed after reading response.completed were then classified as 499 CLIENT_ABORTED and cleared the sticky/affinity binding, forcing the next request off affinity_hit and churning providers per request (40% prefix-cache hit loss). Keep bounded retention (64 KiB metering + 3 MiB reservation capped by DetachedStreamBudget 64/64MiB from #1439/#1440) enabled even in high-concurrency mode so completed streams are still billed as 200 and keep the binding. Genuinely truncated streams remain failures. Update i18n (5 locales) and the high-concurrency warning to reflect that bounded retention stays on. Add regression tests covering ProxySession policy and the Codex 200-vs-499 finalization.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough高并发模式下，客户端中止后的有界计费保留逻辑保持启用。已完成的上游流继续按 200 成功处理并保留会话绑定。相关回归测试和五种语言的设置文案已同步更新。 Changes高并发客户端中止保留
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change restores bounded client-abort billing and affinity retention in high-concurrency mode, with focused regression coverage and passing checks; no actionable merge-blocking risk remains beyond normal review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (5 skipped: 5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The one-line production change (session.ts:594 shouldRetainClientAbortBilling() now unconditionally returns true) was verified end-to-end: it un-stubs the client-abort metering observer, removes both immediate-cancel paths, and re-enables the clientAbortCompleteSuccess 200-reclassification in high-concurrency mode, with memory safety genuinely bounded by DetachedStreamBudget (64 concurrent / 64 MiB defaults with a 16 MiB metering reserve). The new regression tests are real — reverting the predicate locally makes the dispatch-chain test fail with expected 499 to be 200, confirming it reproduces the exact bug rather than mirroring the classification logic.
PR Size: M
- Lines changed: 438
- Files changed: 9
Verification performed
- Ran the new regression suite: 3/3 pass with the fix; 2/3 fail against the reverted (buggy) predicate, proving test validity
- Ran
session.test.ts+system-settings-form-upstream-error-message.test.tsx: 36/36 pass tsgo --noEmittypecheck passes clean- All 5 locale files are valid JSON and updated consistently; toast is wired via
t("highConcurrencyModeWarning")(no hardcoded strings) - No stale references to the old "client-abort retention disabled in high-concurrency" behavior remain anywhere in src/, messages/, or docs
- Comment claims in session.ts verified against actual constants (64 KiB metering cap, ~3 MiB drain reservation, budget defaults)
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Note: the now-constant predicate leaves its four negated call sites in response-handler.ts as permanently-dead branches. This is acceptable for a minimal hotfix (the comment documents the rationale), but a follow-up could inline the removal if the toggle is not expected to return.
Automated review by Claude AI
1 participant
Summary
高并发模式下继续保留有界的客户端中断计费（client-abort retention）：已完成的 Codex 流在客户端挂断后仍按 200 计费并保留 sticky/affinity 绑定，修复误判 499、清除 session 绑定、前缀缓存命中率下降约 40% 的回归。
原因
高并发模式把
shouldRetainClientAbortBilling()置为false，导致response-handler.ts在客户端断开时立即cancelSource("client_detached_high_concurrency")并丢弃已收字节。Codex 读完response.completed就挂电话 —— 本该走“已完成则改判 200 正常计费”的救场分支clientAbortCompleteSuccess，却被这道!highConcurrency硬短路掉，全部被判499 CLIENT_ABORTED并clearSessionBinding()。绑定被清 → 下一条请求从affinity_hit退回initial_selection→ 每条请求切一次渠道。熔断器虽有!clientAborted豁免没被拉黑，但前缀缓存命中掉 40%。改法
src/app/v1/_lib/proxy/session.ts:594：shouldRetainClientAbortBilling()固定返回true，即高并发下也保留有界的客户端中断计费（client-abort-metering64 KiB +CLIENT_ABORT_DRAIN_RESERVATION_BYTES3 MiB，由DetachedStreamBudget的 64 并发 / 64 MiB 进程级预算兜底，来自 v0.9.4 fix(proxy): bound detached stream memory without disabling replay #1439/fix(proxy): harden detached stream cleanup #1440 修复后的上界）。这样已完成流仍能凭response.completed+billingComplete改判 200 并保留 sticky/affinity 绑定，真正截断的流继续按 499/502 走失败路径。enableHighConcurrencyModeDesc/highConcurrencyModeWarning改为“有界的客户端中断保留仍会执行”。session.test.ts同步预期；system-settings-form-upstream-error-message.test.tsx的 toast 断言同步；新增high-concurrency-client-abort-retention.test.ts:13 条回归（高并发下 200 保留绑定 / 截断不误判 200 / policy 保持）。新回归测试直接驱动真实的ProxyResponseHandler.dispatch链路（而非复制判级公式），覆盖 test: reproduce high-concurrency mode + Codex abort → 499 bug #1451 复现套件中 Greptile 指出的“镜像逻辑漂移”隐患。验证
bun run typecheck通过bunx vitest run -t "high concurrency client-abort"3 条用例通过bunx vitest run tests/unit/proxy/session.test.ts tests/unit/settings/system-settings-form-upstream-error-message.test.tsx39 条通过风险
仅解锁一条已做 OOM 上界的子路径，不涉及 Replay / hedge / gate 的高并发开关（仍关闭）。
关联 / Related
session.ts:594）与 L2 计量 observer 被桩（response-handler.ts:3602）。本 PR 是对应的生产修复：该谓词同时门控 finalization 短路（response-handler.ts:1964）、桩 observer（:3602）与两处立即 cancel/discard 路径（:3679、:4469），翻转为true后两层同时修复。shouldRetainClientAbortBilling()返回!highConcurrencyModeEnabled，旁路了客户端中断保留计费。clientAbortCompleteSuccess改判 200 路径）；同一 false-499 症状类的历史 issue：为啥大部分请求都是499 #1083、状态499问题 #985、调用gpt 5.5模型日志显示499，但是中转调用成功 #1242（均由 fix(proxy): finalize completed responses streams after client abort #1251 关闭）。DetachedStreamBudget进程级预算（默认 64 并发 / 64 MiB）兜底，每流上限CLIENT_ABORT_METER_MAX_RETAINED_BYTES64 KiB +CLIENT_ABORT_DRAIN_RESERVATION_BYTES3 MiB。Description enhanced by Claude AI