feat(core): configure stream rate-limit retry delays - #7674
Conversation
|
Thanks for the PR — this is a clean, well-scoped one. Template looks good ✓ Problem: observed, not theoretical. Linked issue #7658 documents the hardcoded Direction: aligned. Qwen Code targets diverse OpenAI-compatible providers with very different rate-limit characteristics, and the proposed Size: touches core paths ( Approach: the scope feels right. A config field needs the full type → resolver → schema → consumer wiring, and every edit here is part of that chain — no drive-by refactors or unrelated churn. The docs delta looks large but is just the Moving on to code review. 🔍 中文说明感谢贡献——这是一个干净、范围明确的 PR。 模板完整 ✓ 问题: 已观测到,非理论性问题。关联 issue #7658 记录了 方向: 对齐。Qwen Code 面向限流特征各异的 OpenAI 兼容 provider,提议的 规模: 触及核心路径( 方案: 范围合理。一个配置字段需要完整的 类型 → 解析器 → schema → 消费方 链路,这里每处改动都属于该链路——没有顺手重构或无关改动。文档改动看起来大,只是因为 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewMy independent take before reading the diff: make the two delays optional fields on What I verified by reading the code:
Two non-blocking observations:
Test evidence (the PR's own CI)
Real-scenario testing: N/A. This is config plumbing whose observable effect (stream-side 429 retry timing) only manifests against a rate-limiting provider and can't be reliably reproduced in a local TUI session — the fake-timer unit test is the right oracle. Per the static-review rule, PR code was not executed. Not verified: the unit tests passing in CI (blocked on a maintainer approving the 中文说明代码审查读 diff 前我的独立方案:把两个延迟做成 通过阅读代码确认:
两点非阻塞观察:
测试证据(PR 自身 CI)
真实场景测试:N/A。 这是配置接线,其可观测效果(流式 429 重试时序)只在限流 provider 下显现,无法在本地 TUI 会话中可靠复现——fake-timer 单元测试是正确的验证手段。按静态审查规则,未执行 PR 代码。 未验证: 单元测试在 CI 中通过(需维护者批准 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, minimal, precedent-following change that does exactly what the welcomed issue asked; the only thing keeping this from a 5 is that the CI test suite hasn't executed yet (held for maintainer approval), so the green-tests confirmation is still pending. Stepping back: this is the kind of PR that's easy to say yes to on merit, not because I ran out of reasons to say no. The problem is real and was already welcomed by maintainers (#7658), the implementation is the minimal correct wiring — the same path The one reservation is process, not code: the 中文说明置信度:4/5 —— 干净、最小化、遵循先例的改动,完全做到了被欢迎的 issue 所要求的内容;唯一让它达不到 5 分的是 CI 测试套件尚未执行(等待维护者批准),因此"测试跑绿"的确认仍待完成。 退一步看:这个 PR 之所以容易肯定,是因为它本身过硬,而不是因为我找不到拒绝的理由。问题真实且已被维护者欢迎(#7658),实现是最小的正确接线——与 唯一的保留是流程层面,而非代码: — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
There was a problem hiding this comment.
🎉 Welcome and thanks for your first contribution!
Clean, well-structured PR. The retryInitialDelayMs / retryMaxDelayMs fields are correctly wired through the full stack (types → constants → contentGenerator → geminiChat → settings schema), defaults fall back correctly, and tests cover both the configured and resolver paths. Docs updated too. 👍
Two non-blocking suggestions posted as inline comments:
- Spread-then-override pattern in geminiChat.ts works but could be cleaner
- No validation on delay values (negative/zero/max < initial) — downstream Math.min/max prevents crashes but a debugLogger.warn would help debuggability
Both are non-blocking. Great first contribution — looking forward to more! 🎉
— qwen3.8-max-preview via Qwen Code /review
What this PR does
Adds
retryInitialDelayMsandretryMaxDelayMstogenerationConfigso stream-side rate-limit retries can use provider-appropriate delays instead of always starting at 60 seconds and backing off up to 5 minutes. The existing defaults are preserved when these options are not set.Why it's needed
Some OpenAI-compatible providers return short-lived 429 throttling errors during SSE streaming, where waiting 60 seconds before the first retry is unnecessarily slow. Making the stream retry delay configurable lets users tune the retry cadence for their provider without patching source code.
Reviewer Test Plan
How to verify
Configure an OpenAI-compatible provider with
generationConfig.retryInitialDelayMsandgenerationConfig.retryMaxDelayMs, then trigger a stream-side 429 retry. The retry event should report and wait for the configured delay values while preserving the prior default behavior when the fields are omitted.Evidence (Before & After)
N/A — configuration and retry behavior change covered by focused unit tests.
Tested on
Environment (optional)
Windows, Node v22.13.1.
npm run buildreached the web-shell build stage and timed out after 900 seconds in this local Windows environment without an error; relevant package builds and full typecheck completed successfully.Risk & Scope
Linked Issues
Fixes #7658
中文说明
What this PR does
在
generationConfig中新增retryInitialDelayMs和retryMaxDelayMs,让流式 429 限流重试可以使用适合具体 provider 的延迟,而不是始终从 60 秒开始并最多退避到 5 分钟。未配置这些字段时,现有默认行为保持不变。Why it's needed
一些 OpenAI 兼容 provider 会在 SSE 流式响应期间返回短暂的 429 限流错误,此时第一次重试等待 60 秒会显得过长。将流式重试延迟配置化后,用户可以按 provider 的限流特征调整重试节奏,而不需要修改源码。
Reviewer Test Plan
How to verify
为 OpenAI 兼容 provider 配置
generationConfig.retryInitialDelayMs和generationConfig.retryMaxDelayMs,然后触发一次流式 429 重试。重试事件应报告并等待配置的延迟值;未设置这些字段时应继续保持原有默认行为。Evidence (Before & After)
N/A —— 配置和重试行为变更已由聚焦单元测试覆盖。
Tested on
Environment (optional)
Windows,Node v22.13.1。本地 Windows 环境中
npm run build运行到 web-shell build 阶段后 900 秒超时,未出现错误输出;本次相关 package build 与完整 typecheck 已成功完成。Risk & Scope
Linked Issues
Fixes #7658