Skip to content

feat(core): configure stream rate-limit retry delays - #7666

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
hogeheer499-commits:agent/configurable-stream-retry-7658
Jul 25, 2026
Merged

feat(core): configure stream rate-limit retry delays#7666
wenshao merged 2 commits into
QwenLM:mainfrom
hogeheer499-commits:agent/configurable-stream-retry-7658

Conversation

@hogeheer499-commits

Copy link
Copy Markdown
Contributor

What this PR does

Adds optional retryInitialDelayMs and retryMaxDelayMs generation settings for SSE stream rate-limit retries. Provider-specific generation config is routed through the existing model configuration resolver, and omitted values preserve the current 60-second initial delay and 5-minute maximum.

Why it's needed

Providers use different quota windows. The current fixed 60s → 120s → 240s backoff can make a short-lived per-minute rate limit feel unnecessarily slow, while the existing defaults remain appropriate for users who do not configure the new settings.

Reviewer Test Plan

How to verify

Configure a model provider with retryInitialDelayMs: 3000 and retryMaxDelayMs: 5000, then simulate two consecutive classified rate-limit errors during an SSE stream. The retry events should report delays of 3000ms and 5000ms, with the second delay capped by the configured maximum. Confirm that provider-specific values override global generation settings and that omitting both fields retains the existing defaults.

Commands run locally:

  • npx vitest run src/core/geminiChat.test.ts src/models/modelConfigResolver.test.ts --coverage.enabled=false — 294 tests passed
  • npm run build — passed
  • npm run lint — passed
  • npm run typecheck — passed
  • npm run generate:settings-schema — passed
  • npm run preflight — install, formatting, CI lint, build, typecheck, core tests, and web-shell tests passed; the CLI suite reported three unrelated failures that reproduce unchanged on main (agent-prompt.test.ts path matching in a workspace containing spaces and one AuthDialog.test.tsx selection-timing assertion)

Evidence (Before & After)

N/A — this is non-visual configuration behavior covered by regression tests.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment (optional)

Ubuntu Linux, Node.js/npm workspace.

Risk & Scope

  • Main risk or tradeoff: A value that is too small can cause faster retry attempts, so the schema requires positive values and the existing defaults remain unchanged.
  • Not validated / out of scope: HTTP-layer retries and non-rate-limit stream failures are unchanged; macOS and Windows were not tested locally and remain covered by CI.
  • Breaking changes / migration notes: None. Both settings are optional.

Linked Issues

Closes #7658

中文说明

本 PR 的内容

为 SSE 流式限流重试新增可选的 retryInitialDelayMsretryMaxDelayMs 生成设置。Provider 专属的生成配置通过现有模型配置解析器传递;如果省略这些值,则继续使用当前 60 秒初始延迟和 5 分钟最大延迟。

为什么需要此改动

不同 Provider 使用不同的配额窗口。当前固定的 60 秒 → 120 秒 → 240 秒退避会让短暂的分钟级限流产生不必要的等待;未配置新设置的用户仍会保留现有默认行为。

Reviewer 测试计划

验证方法

为某个模型 Provider 配置 retryInitialDelayMs: 3000retryMaxDelayMs: 5000,然后在 SSE 流中模拟两次连续且已分类的限流错误。重试事件应分别报告 3000ms 和 5000ms 延迟,其中第二次延迟受配置的最大值限制。确认 Provider 专属值覆盖全局生成设置,并确认省略两个字段时保留现有默认值。

本地运行的命令:

  • npx vitest run src/core/geminiChat.test.ts src/models/modelConfigResolver.test.ts --coverage.enabled=false — 294 个测试通过
  • npm run build — 通过
  • npm run lint — 通过
  • npm run typecheck — 通过
  • npm run generate:settings-schema — 通过
  • npm run preflight — 安装、格式检查、CI lint、构建、类型检查、core 测试和 web-shell 测试均通过;CLI 测试报告了三个与本改动无关的失败,并且这些失败在未修改的 main 上同样可复现(agent-prompt.test.ts 在包含空格的工作区中的路径匹配,以及 AuthDialog.test.tsx 的一个选择时序断言)

证据(修改前与修改后)

不适用——这是由回归测试覆盖的非视觉配置行为。

测试平台

操作系统 状态
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ✅ 已测试

环境(可选)

Ubuntu Linux,Node.js/npm workspace。

风险与范围

  • 主要风险或权衡:过小的值可能导致更快的重试,因此 schema 要求使用正数,并且现有默认值保持不变。
  • 未验证 / 不在范围内:HTTP 层重试和非限流流式失败保持不变;本地未在 macOS 和 Windows 上测试,这些平台由 CI 覆盖。
  • 破坏性变更 / 迁移说明:无。两个设置均为可选项。

关联 Issue

关闭 #7658

@hogeheer499-commits
hogeheer499-commits marked this pull request as ready for review July 24, 2026 14:43
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 7db0012 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 7db0012 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Context shift since last review: the core feature (configurable retryInitialDelayMs / retryMaxDelayMs) was merged to main via #7674 while this PR was open. After the merge-conflict resolution, the diff is now a small polish layer on top of the already-landed feature: minimum: 1 schema validation, test coverage for provider-specific override, and a docs example. The title still says "configure stream rate-limit retry delays" but the diff no longer adds those fields — worth updating to something like feat(core): add minimum validation and tests for retry delay config so the changelog stays accurate. Non-blocking.

Problem: observed. Issue #7658 documented the hardcoded 60s → 120s → 240s backoff with a concrete DashScope per-minute quota scenario. The issue was triaged, accepted, and labeled welcome-pr. The feature itself is now in main; this PR closes the remaining gaps (schema validation, provider-override test, docs).

Direction: aligned. The minimum: 1 guard addresses the previous review's follow-up about non-positive values bypassing the schema. Provider-override test coverage and docs discoverability are natural completions of the feature.

Size: 4 production logic lines (2x minimum: 1 in settingsSchema.ts + settings.schema.json), ~22 test lines, 2 docs lines. Well within bounds.

Approach: tight and focused. Every edit serves the stated goal. One observation: the test rename from "should increase delay" to "should use configured delay" creates a near-duplicate of the existing "uses configured stream rate-limit retry delays" test that #7674 already added — see Stage 2 for details. Non-blocking.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

自上次审查以来的上下文变化: 核心功能(可配置的 retryInitialDelayMs / retryMaxDelayMs)已通过 #7674 合并到 main。合并冲突解决后,当前 diff 是在已落地功能之上的小幅完善:minimum: 1 schema 校验、provider 专属覆盖的测试覆盖、以及文档示例。标题仍为 "configure stream rate-limit retry delays",但 diff 已不再添加这些字段——建议更新为类似 feat(core): add minimum validation and tests for retry delay config 的名称,以保持 changelog 准确。非阻塞。

问题:已观测到的问题。Issue #7658 记录了硬编码的 60s → 120s → 240s 退避,并给出了 DashScope 分钟级配额的具体场景。该 issue 已被分类、接受,并标记为 welcome-pr。功能本身已在 main 中;本 PR 填补剩余空缺(schema 校验、provider 覆盖测试、文档)。

方向:对齐。minimum: 1 约束回应了上次审查关于非正值绕过 schema 的后续建议。Provider 覆盖测试和文档可发现性是功能的自然完善。

规模:4 行生产逻辑(settingsSchema.ts 和 settings.schema.json 各 2 行 minimum: 1),约 22 行测试,2 行文档。远在限制之内。

方案:紧凑且聚焦。每处改动都服务于既定目标。一个观察:测试从 "should increase delay" 重命名为 "should use configured delay" 后,与 #7674 已添加的 "uses configured stream rate-limit retry delays" 测试近乎重复——详见 Stage 2。非阻塞。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 7db0012a275f93ff7ce14eff4622b084dc8b200b · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: with the core feature already in main (via #7674), the remaining work is: (1) add minimum: 1 to the schema entries for both retry-delay fields so editors flag non-positive values, (2) add provider-override test coverage so the per-field precedence chain is pinned, (3) show the fields in the docs example for discoverability. That is exactly what this PR does — no simpler path was missed.

Findings: no critical blockers, no convention violations.

  • settingsSchema.ts and settings.schema.json both gain minimum: 1 on retryInitialDelayMs and retryMaxDelayMs. The two files stay in sync — important because one drives the runtime settings dialog / validation and the other drives IDE autocompletion. The constraint matches the maxRetries precedent (positive integers only).
  • modelConfigResolver.test.ts extends the existing "modelProvider config overrides settings" test with retry-delay fields in both the settings layer and the provider layer, then asserts the provider values win and source tracking reports modelProviders. This is genuinely new coverage — main only tests settings-level resolution for these fields, not provider override.
  • model-providers.md adds the two fields to the OpenAI-compatible provider example, alongside the existing maxRetries. Helpful for discoverability.
  • geminiChat.test.ts renames "should increase delay across repeated streamed rate-limit errors" to "should use configured delay" and injects a mock config with retryInitialDelayMs: 3_000 / retryMaxDelayMs: 5_000, changing the assertions from [60_000, 120_000] to [3_000, 5_000]. Non-blocking nit: main already has a separate "uses configured stream rate-limit retry delays" test (added by feat(core): configure stream rate-limit retry delays #7674) that tests the same configured values with the same mock setup. After this PR merges, both tests will exercise the same path. The default-delay growth curve (60_000 → 120_000) loses its dedicated test in this block, though the default first delay is still pinned by the TPM and GLM retry tests. Consider either reverting this test to its original default-delay form or removing the duplicate — but neither blocks the merge.

CI Test Evidence

Fetched via API for commit 7db0012a275f93ff7ce14eff4622b084dc8b200b:

Final CI results for 7db0012 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Ubuntu test suite is still running. Windows/macOS and integration tests were skipped (standard for fork PRs). Precheck passed. Will defer approval until CI lands green.

Real-Scenario Testing

N/A — the change adds schema validation constraints and test coverage for non-visual configuration behavior. No user-visible TUI surface is affected. The previous review's maintainer verification (real E2E with a mock throttling provider) already confirmed the underlying feature works end-to-end.

中文说明

代码审查

独立方案: 核心功能已通过 #7674 合并到 main,剩余工作为:(1) 为两个重试延迟字段的 schema 条目添加 minimum: 1,使编辑器能标记非正值;(2) 添加 provider 覆盖测试覆盖,锁定逐字段优先级链;(3) 在文档示例中展示这些字段以提高可发现性。PR 的实现与此完全一致——没有遗漏更简路径。

发现: 无关键阻塞项,无规范违反。

  • settingsSchema.tssettings.schema.json 均为两个字段添加了 minimum: 1。两个文件保持同步——这很重要,因为一个驱动运行时设置对话框/校验,另一个驱动 IDE 自动补全。约束与 maxRetries 先例一致。
  • modelConfigResolver.test.ts 扩展了现有的 "modelProvider config overrides settings" 测试,在 settings 层和 provider 层都添加了重试延迟字段,断言 provider 值胜出且来源跟踪报告 modelProviders。这是真正的新覆盖——main 只测试了 settings 级解析,未测试 provider 覆盖。
  • model-providers.md 在 OpenAI 兼容 provider 示例中添加了两个字段,与现有 maxRetries 并列。有助于可发现性。
  • geminiChat.test.ts 将 "should increase delay" 重命名为 "should use configured delay" 并注入配置 mock。非阻塞注意点: main 已有 feat(core): configure stream rate-limit retry delays #7674 添加的独立 "uses configured stream rate-limit retry delays" 测试,使用相同的配置值和 mock。合并后两个测试将验证同一路径。默认延迟增长曲线在此测试块中失去专属测试,但默认首次延迟仍由 TPM 和 GLM 重试测试锁定。建议将此测试恢复为原始默认延迟形式或删除重复项——但均不阻塞合并。

CI 测试证据

通过 API 获取,commit 7db0012a275f93ff7ce14eff4622b084dc8b200b

Ubuntu 测试套件仍在运行中。Windows/macOS 和集成测试被跳过(fork PR 的标准行为)。Precheck 已通过。将推迟批准直到 CI 全绿。

真实场景测试

不适用——该改动添加 schema 校验约束和非视觉配置行为的测试覆盖。不影响用户可见的 TUI 界面。上次审查中维护者的真实 E2E 验证(使用模拟限流 provider)已确认底层功能端到端可用。

Qwen Code · qwen3.8-max-preview

Reviewed at 7db0012a275f93ff7ce14eff4622b084dc8b200b · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal polish on an already-merged feature; the only nit is a near-duplicate test that doesn't affect correctness.

The feature this PR set out to build — configurable stream rate-limit retry delays — is already in main via #7674. What remains in the diff is the finishing work: minimum: 1 schema validation (closing the gap where a 0 value produces immediate retries against a throttling provider), provider-override test coverage that wasn't in the original merge, and a docs example. All three earn their place.

My independent proposal before reading the diff was identical: schema constraint, provider-override test, docs. No simpler path exists.

The one thing I'd fix before merge if I were the author: the renamed "should use configured delay" test in geminiChat.test.ts now duplicates the "uses configured stream rate-limit retry delays" test that #7674 added. Reverting it to the original default-delay form would restore the growth-curve coverage and eliminate the duplicate. But this is a nit, not a blocker — the default first delay is still pinned elsewhere, and two tests exercising the same path is harmless redundancy.

Title mismatch noted in Stage 1: the diff no longer "configures" anything that isn't already configured. A title update would keep the changelog accurate, but this is hygiene, not a merge gate.

Approval deferred until CI lands green on 7db0012a275f93ff7ce14eff4622b084dc8b200b.

中文说明

置信度:4/5 —— 在已合并功能之上干净、最小化的完善;唯一的注意点是近乎重复的测试,不影响正确性。

本 PR 最初要构建的功能——可配置的流式限流重试延迟——已通过 #7674 合并到 main。diff 中剩余的是收尾工作:minimum: 1 schema 校验(堵上 0 值导致对限流 provider 立即重试的缺口)、原始合并中缺失的 provider 覆盖测试、以及文档示例。三者都有其存在价值。

我在阅读 diff 之前的独立方案与此完全一致:schema 约束、provider 覆盖测试、文档。不存在更简路径。

如果我是作者,合并前会修的一件事:geminiChat.test.ts 中重命名后的 "should use configured delay" 测试现在与 #7674 添加的 "uses configured stream rate-limit retry delays" 测试重复。将其恢复为原始默认延迟形式可以恢复增长曲线覆盖并消除重复。但这是注意点,不是阻塞项——默认首次延迟仍在其他地方被锁定,两个测试验证同一路径是无害的冗余。

Stage 1 中提到的标题不匹配:diff 不再"配置"任何尚未配置的东西。更新标题可以保持 changelog 准确,但这属于命名规范,不是合并门禁。

批准推迟到 CI 在 7db0012a275f93ff7ce14eff4622b084dc8b200b 上全绿后执行。

Qwen Code · qwen3.8-max-preview

Reviewed at 7db0012a275f93ff7ce14eff4622b084dc8b200b · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Maintainer local verification — real end-to-end, not just the suite

I built this PR locally and drove the real CLI against a mock OpenAI-compatible provider that throttles the way DashScope does (HTTP 200 + finish_reason:"error_finish" carrying a 429 payload), so the retry delay is measured from when requests actually arrive at the provider rather than from anything the CLI reports about itself.

Verdict: works as advertised, no blockers. Recommend merge. Two small follow-ups noted at the end — neither needs to hold this PR.

  • PR head 0f02ba29a · merge base 4c2b48115 · two isolated worktrees, separate clean npm ci in each
  • macOS 15.6 (Darwin 24.6.0), Node 22.23.1 — the platform the PR lists as untested
  • Provenance checked before trusting the A/B: each tree resolves @qwen-code/qwen-code-core to its own packages/core, and retryInitialDelayMs is present in the PR tree's core/dist and absent from base's

1. Real end-to-end run

e2e

Run Build generationConfig retryDelayMs in CLI log Gap measured at the provider Result
pr-configured PR 3000 / 5000 3000, 5000 3.02 s, 5.01 s reply after 2 retries, exit 0, 9.4 s total
base-configured base 3000 / 5000 60000, 120000 60.03 s settings had no effect
pr-defaults PR both omitted 60000, 120000 60.02 s existing default preserved

The second delay in the PR run is the interesting one: 3000 × 2 = 6000 clamped down to the configured 5000, so the cap is genuinely applied and not just carried through. pr-defaults is the no-op proof — a user who does not set these fields sees byte-identical behaviour to today.

Settings were supplied through the documented path (modelProviders.openai[].generationConfig), so this exercises the whole chain — settings.jsonresolveModelConfigContentGeneratorConfiggetRateLimitRetryDelayMs — not just the last hop.

2. Independent scenarios + mutation matrix

matrix

I wrote 10 scenarios of my own (not part of the PR) and ran each twice: once on the PR head, once on the same tree with the four changed source files reverted to the merge base while keeping the tests. 8 of 10 discriminate; the two that pass both ways are deliberate regression guards (VERIFY-1 default backoff, RESOLVE-4 no injected default). The PR's own two tests were run the same way and both fail on base source — they are load-bearing, not vacuous.

Worth calling out from that set:

  • VERIFY-3 — setting only retryMaxDelayMs: 10000 clamps the untouched 60 s default down to 10 s. The ?? fallbacks compose correctly in either direction.
  • RESOLVE-2 — a provider entry that sets only retryInitialDelayMs still inherits retryMaxDelayMs from the global model.generationConfig. Precedence is per-field, which is the behaviour you want.
  • VERIFY-6 — a configured retryMaxDelayMs: 5000 also caps a provider-supplied Retry-After: 600, so the honoured wait becomes 5 s instead of 300 s. That follows from retryAfterMaxDelayMs: options.maxDelayMs inside getRateLimitRetryDelayMs; it is defensible, just currently undocumented.

The mutation matrix confirms the PR's test bites: dropping either override, or swapping them, is caught. One mutation survives — see below.

3. Gates re-run locally on the PR head

tsc --noEmit clean for core and cli · eslint --max-warnings 0 clean on all 7 changed files · prettier --check clean on the docs and schema JSON · npm run generate:settings-schema produces no diff, so the committed settings.schema.json is in sync · npm run build -- --cli-only && npm run bundle exit 0 in both trees · vitest run geminiChat modelConfigResolver rateLimit retryPolicy4 files, 349 tests passed · git merge-tree against current origin/main is clean (branch is 18 commits behind; main's only touch to geminiChat.ts since the base is an unrelated clearDetailedSpanState removal).


Follow-ups (non-blocking)

(a) minimum: 1 is an editor-side guard, not a runtime one. The PR's Risk section says "the schema requires positive values". That holds for IDE/JSON-schema validation and for validateSettingValue, but settings.json is not validated against minimum when it is loaded, and these fields are showInDialog: false, so the dialog path never sees them. A real run with retryInitialDelayMs: 0 is accepted and produces genuine 0 ms retries — observed gaps of 13 ms and 7 ms between provider requests, i.e. up to maxRetries: 10 immediate re-attempts against a provider that is already throttling. This is exactly the mutation that survives the test suite (?? → || changes nothing observable, because no committed test pins the 0 case).

This is consistent with how sibling numeric generationConfig fields already behave, so it is not a regression this PR introduces. If you want it closed off, the cheapest fix is at the single call site — treat a non-positive value as unset, e.g. cgConfig?.retryInitialDelayMs && cgConfig.retryInitialDelayMs > 0 ? … : RATE_LIMIT_RETRY_OPTIONS.initialDelayMs — plus one test for it.

(b) Small coverage delta from repurposing the existing test. should increase delay across repeated streamed rate-limit errors was the only test pinning the default growth 60 000 → 120 000 through geminiChat; it now asserts 3 000 → 5 000. The default first delay is still pinned by the TPM and GLM tests (both assert 60000) and the exponential policy itself is covered in rateLimit.test.ts, so the loss is narrow — but adding back a defaults case next to the new one (my VERIFY-1) would restore it for free.

(c) One documentation sentence. Worth noting in model-providers.md that retryMaxDelayMs also bounds how long a provider's Retry-After will be honoured, since a user lowering it for a fast per-minute quota is implicitly weakening obedience to explicit server-directed waits.

Verification harness (mock provider, E2E driver, scenarios, mutation script) is throwaway and was not committed; the PR worktree was restored to a pristine 0f02ba29a afterwards.

中文说明

维护者本地验证 —— 真实端到端,而非仅跑测试套件

我在本地构建了该 PR,并用真实 CLI 对接一个模拟 OpenAI 兼容 Provider,该 Provider 以 DashScope 的方式返回限流(HTTP 200 + finish_reason:"error_finish",负载中带 429)。因此重试延迟是通过请求实际到达 Provider 的时刻测量的,而不是依赖 CLI 自己的汇报。

结论:行为与描述一致,无阻塞问题,建议合并。 文末有两点后续建议,均无需卡住本 PR。

  • PR head 0f02ba29a · merge base 4c2b48115 · 两个隔离 worktree,各自独立执行干净的 npm ci
  • macOS 15.6(Darwin 24.6.0),Node 22.23.1 —— 正是 PR 中标注为「未测试」的平台
  • 在采信 A/B 前先核验了产物来源:两棵树各自将 @qwen-code/qwen-code-core 解析到自己的 packages/coreretryInitialDelayMs 存在于 PR 树的 core/dist,在 base 树中不存在

1. 真实端到端运行

运行 构建 generationConfig CLI 日志中的 retryDelayMs Provider 侧实测间隔 结果
pr-configured PR 3000 / 5000 30005000 3.02 秒、5.01 秒 两次重试后成功返回,exit 0,共 9.4 秒
base-configured base 3000 / 5000 60000120000 60.03 秒 配置完全无效
pr-defaults PR 两项都省略 60000120000 60.02 秒 保留现有默认值

PR 运行中的第二次延迟最能说明问题:3000 × 2 = 6000 被压到配置的 5000,说明上限确实生效,而非简单透传。pr-defaults 则是「无副作用」的证明 —— 未配置这两个字段的用户,行为与当前完全一致。

配置通过文档中的路径(modelProviders.openai[].generationConfig)提供,因此验证覆盖了完整链路:settings.jsonresolveModelConfigContentGeneratorConfiggetRateLimitRetryDelayMs,而不只是最后一环。

2. 独立场景 + 变异矩阵

我自己编写了 10 个场景(不属于本 PR),每个都跑两遍:一遍在 PR head 上,一遍在把四个被改动的源文件回退到 merge base、但保留测试的同一棵树上。10 个中有 8 个具备判别力;两个双向通过的是刻意设计的回归保护(VERIFY-1 默认退避、RESOLVE-4 不注入默认值)。PR 自带的两个测试用同样方式运行,在 base 源码上均失败 —— 说明它们是有效测试,而非空测试。

其中值得一提的:

  • VERIFY-3 —— 只设置 retryMaxDelayMs: 10000 时,未改动的 60 秒默认初始值会被压到 10 秒。?? 回退在两个方向上都能正确组合。
  • RESOLVE-2 —— Provider 条目只设置 retryInitialDelayMs 时,retryMaxDelayMs 仍会从全局 model.generationConfig 继承。优先级是逐字段的,这正是期望的行为。
  • VERIFY-6 —— 配置 retryMaxDelayMs: 5000 后,Provider 返回的 Retry-After: 600 也会被压到 5 秒而非 300 秒。这源自 getRateLimitRetryDelayMs 内部的 retryAfterMaxDelayMs: options.maxDelayMs,设计上说得通,只是目前没有文档说明。

变异矩阵确认 PR 自带测试确实「咬得住」:删除任一 override、或把两者对调,都会被捕获。有一个变异存活,见下文。

3. 在 PR head 上本地重跑各项门禁

core 与 cli 的 tsc --noEmit 均无错误 · 7 个改动文件 eslint --max-warnings 0 通过 · 文档与 schema JSON 的 prettier --check 通过 · npm run generate:settings-schema 无 diff,说明提交的 settings.schema.json 是同步的 · 两棵树的 npm run build -- --cli-only && npm run bundle 均 exit 0 · vitest run geminiChat modelConfigResolver rateLimit retryPolicy4 个文件、349 个测试通过 · 对当前 origin/maingit merge-tree 无冲突(分支落后 18 个提交;main 自 base 以来对 geminiChat.ts 的唯一改动是无关的 clearDetailedSpanState 删除)。

后续建议(非阻塞)

(a) minimum: 1 是编辑器侧的约束,不是运行时约束。 PR 的风险章节写道「schema 要求使用正数」。这对 IDE / JSON schema 校验以及 validateSettingValue 成立,但 settings.json 在加载时并不会按 minimum 校验,而且这两个字段是 showInDialog: false,对话框路径根本不会经过它们。真实运行中 retryInitialDelayMs: 0 会被接受并产生真正的 0 毫秒重试 —— 实测 Provider 请求间隔为 13 毫秒和 7 毫秒,也就是对一个已经在限流的 Provider 连打最多 maxRetries: 10 次。这恰好就是那个在测试套件中存活的变异(?? → || 没有可观察差异,因为没有已提交的测试锁定 0 这一取值)。

这与现有同类数值型 generationConfig 字段的行为一致,因此并非本 PR 引入的回归。若希望堵上,最省事的做法是在唯一调用点把非正值视为未设置,例如 cgConfig?.retryInitialDelayMs && cgConfig.retryInitialDelayMs > 0 ? … : RATE_LIMIT_RETRY_OPTIONS.initialDelayMs,再补一个测试。

(b) 改写既有测试带来的小幅覆盖损失。 should increase delay across repeated streamed rate-limit errors 原本是 geminiChat 中唯一锁定默认增长曲线 60 000 → 120 000 的测试,现在改为断言 3 000 → 5 000。默认的首次延迟仍由 TPM 和 GLM 两个测试锁定(均断言 60000),指数退避策略本身也在 rateLimit.test.ts 中有覆盖,所以损失范围很窄 —— 但在新测试旁边补回一个默认值场景(即我的 VERIFY-1)几乎零成本。

(c) 补一句文档。 建议在 model-providers.md 中说明:retryMaxDelayMs 同时限定了 Provider 的 Retry-After 最多被遵守多久。为了应对分钟级配额而调小该值的用户,实际上也削弱了对服务端明确指定等待时间的遵守。

验证用的脚手架(模拟 Provider、E2E 驱动脚本、场景用例、变异脚本)均为一次性产物,未提交;PR worktree 事后已恢复到干净的 0f02ba29a

wenshao
wenshao previously approved these changes Jul 25, 2026
@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

- settingsSchema.ts: keep minimum:1 validation from PR, adopt main's
  'Retry Max Delay' label
- geminiChat.ts: use main's pre-computed retryInitialDelayMs /
  retryMaxDelayMs variables instead of inline expressions
- settings.schema.json: keep minimum:1 on both retry delay fields
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge resolution for PR #7666

Root cause

The feature from this PR — configurable stream rate-limit retry delays — was already merged to main via a separate PR (#7674, commit 0b5116a1b). Main subsequently evolved: the clearDetailedSpanState calls were removed from geminiChat.ts (telemetry refactor), the settings label was changed to 'Retry Max Delay', and the inline cgConfig?.retryInitialDelayMs ?? ... expressions were extracted into pre-computed local variables. This PR's branch still had the original inline form and added minimum: 1 validation that main never received.

Textual or semantic

All three conflicts were textual — both sides agreed on the semantics; only the surface form differed.

packages/cli/src/config/settingsSchema.ts — kept the PR's minimum: 1 on both retry-delay fields (new validation main lacked) and adopted main's label 'Retry Max Delay' over the PR's 'Retry Maximum Delay' to match the already-landed canonical naming.

packages/core/src/core/geminiChat.ts — adopted main's pre-computed locals:

const retryInitialDelayMs =
  cgConfig?.retryInitialDelayMs ?? RATE_LIMIT_RETRY_OPTIONS.initialDelayMs;
const retryMaxDelayMs =
  cgConfig?.retryMaxDelayMs ?? RATE_LIMIT_RETRY_OPTIONS.maxDelayMs;

used at the call site as initialDelayMs: retryInitialDelayMs, maxDelayMs: retryMaxDelayMs. The PR's inline form was semantically identical but duplicated the fallback logic. The clearDetailedSpanState removals auto-merged cleanly (no conflict, PR removes them in line with main's telemetry refactor).

packages/vscode-ide-companion/schemas/settings.schema.json — kept minimum: 1 on both retryInitialDelayMs and retryMaxDelayMs (PR's contribution; main's copy of the schema lacked the constraint).

What is load-bearing

  • The retryInitialDelayMs / retryMaxDelayMs locals in geminiChat.ts must be declared before the for (;;) retry loop — they are read inside the catch block on each retry iteration. Moving them inside the loop would re-evaluate per iteration (no behavior change today since cgConfig is loop-invariant, but fragile).
  • minimum: 1 in the schema and settingsSchema must stay consistent — one without the other would let the JSON schema accept 0 while the runtime validator rejects it, or vice versa.

What I could not verify

No build, typecheck, or tests were run. The minimum: 1 constraint may cause existing tests that pass 0 or negative delay values to fail — the PR's own test file (geminiChat.test.ts) auto-merged without conflict, so I could not inspect or modify it under the scope rules. If any test asserts on retryInitialDelayMs: 0, it will now fail schema validation.

中文说明

PR #7666 合并冲突解决

根本原因

此 PR 的功能(可配置的流速率限制重试延迟)已通过另一个 PR #7674(提交 0b5116a1b)先行合并到 main。此后 main 发生了多处变更:geminiChat.ts 中移除了 clearDetailedSpanState 调用(遥测重构),设置标签改为 'Retry Max Delay',内联的 cgConfig?.retryInitialDelayMs ?? ... 表达式被提取为预计算的局部变量。此 PR 分支仍保留原始内联形式,并新增了 main 没有的 minimum: 1 校验。

解决策略

三处冲突均为文本冲突,语义一致:

  • settingsSchema.ts:保留 PR 的 minimum: 1 校验,采用 main 的标签 'Retry Max Delay'
  • geminiChat.ts:采用 main 的预计算局部变量 retryInitialDelayMs / retryMaxDelayMs,替代 PR 的内联表达式(语义相同,避免重复)。clearDetailedSpanState 的移除由自动合并处理。
  • settings.schema.json:保留两个延迟字段的 minimum: 1 约束。

关键约束

  • geminiChat.ts 中的延迟变量必须在 for (;;) 循环之前声明。
  • JSON schema 与 settingsSchema 的 minimum: 1 必须保持一致。

未能验证

未运行构建或测试。若有测试传入 0 作为延迟值,可能因新增的 minimum: 1 校验而失败。

@wenshao

wenshao commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment on lines +7889 to +7898
it('should use configured delay across repeated streamed rate-limit errors', async () => {
vi.useFakeTimers();

try {
vi.mocked(mockConfig.getContentGeneratorConfig).mockReturnValue({
authType: AuthType.USE_OPENAI,
model: 'test-model',
retryInitialDelayMs: 3_000,
retryMaxDelayMs: 5_000,
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test was renamed and rewritten from testing default exponential backoff (60 000 → 120 000 ms) to testing configured delays (3 000 → 5 000 ms), but the existing test at line 7984 ("uses configured stream rate-limit retry delays") already covers the same configured-delay path with the same values and the same two-429-then-recovery structure. Meanwhile the default-path fallback (cgConfig?.retryInitialDelayMs ?? RATE_LIMIT_RETRY_OPTIONS.initialDelayMs) no longer has a dedicated stream-level test for the OpenAI-compatible path — the GLM test at line 7808 covers the fallback mechanically but through a different provider branch.

Consider restoring the original test's default-delay assertions (60 000, 120 000) so both the default and configured paths are covered, or adding a separate test for the default path.

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 25, 2026
Merged via the queue into QwenLM:main with commit 8878264 Jul 25, 2026
70 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.1.

@yiliang114

Copy link
Copy Markdown
Collaborator

⚠️ Failed to process this request. Please re-mention the bot to retry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream rate-limit retry delay is hardcoded to 60s/120s/240s — not configurable

5 participants