Skip to content

fix(core): align maxRetries semantics#909

Merged
dingyi222666 merged 2 commits into
ChatLunaLab:v1-devfrom
CookSleep:fix/max-retries-semantics
Jun 7, 2026
Merged

fix(core): align maxRetries semantics#909
dingyi222666 merged 2 commits into
ChatLunaLab:v1-devfrom
CookSleep:fix/max-retries-semantics

Conversation

@CookSleep

@CookSleep CookSleep commented Jun 7, 2026

Copy link
Copy Markdown
Member

摘要

  • 修复 maxRetries 实现,使其遵循已有配置语义:模型请求失败后的最大重试次数。
  • 模型流式请求、非流式生成、平台客户端初始化都改为初次尝试后最多再重试 maxRetries 次。
  • 允许 maxRetries: 0,用于显式关闭重试。

验证

  • yarn fast-build core

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 61aa3028-7d1c-47f0-b778-b149839a4a15

📥 Commits

Reviewing files that changed from the base of the PR and between 291c4cb and 36c1aac.

📒 Files selected for processing (1)
  • packages/core/src/llm-core/platform/model.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/core/src/llm-core/platform/model.ts

Walkthrough

该 PR 统一了重试计数语义:在平台客户端和聊天模型中,将 maxRetries(重试次数)改为 maxAttempts = maxRetries + 1(总尝试次数)驱动循环,调整边界判定条件以使终止时机更清晰,并放宽配置允许零重试。

Changes

重试机制边界与计数语义统一

层次 / 文件 摘要
配置架构:允许零重试
packages/core/src/services/chat.ts
maxRetries 配置约束的最小值从 1 调整为 0,用户可禁用重试。
客户端重试边界修正
packages/core/src/llm-core/platform/client.ts
BasePlatformClient.isAvailable() 的循环条件从 retryCount < maxRetries 改为 retryCount <= maxRetries,失败判定从精确匹配改为 >= maxRetries,改变最后一次重试触发与退出时机。
聊天模型重试计数重构
packages/core/src/llm-core/platform/model.ts
_streamResponseChunks_generateWithRetry 中引入 maxAttempts = Math.max(1, (this._options.maxRetries ?? 0) + 1) 计算,更新循环边界、终止判定与错误决策函数参数;同步修正重试日志的计数分母以保持一致性。

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • ChatLunaLab/chatluna#621: 同样修改 ChatLunaChatModel 的流式/非流式重试循环边界与计数处理,代码级相关。
  • ChatLunaLab/chatluna#842: 修改 BasePlatformClient.isAvailable() 的重试边界与配置 Schema,代码级相关。
  • ChatLunaLab/chatluna#809: 在 _streamResponseChunks/_generateWithRetry 路径调整重试触发与终止判定,代码级相关。

Suggested reviewers

  • dingyi222666

Poem

🐰 重试的边界细调整,
从严格小于到包含相等,
总尝试数如今更清晰,
maxRetries + 1 的新契机,
零重试也被准许呢!✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed 标题清晰准确地总结了本次更改的主要目的:修复并对齐maxRetries的语义定义。
Description check ✅ Passed 描述充分关联到本次变更集,解释了maxRetries语义的修复及其在多个模块中的一致性调整。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the retry logic across the platform client and chat model to use a clearer distinction between retries and attempts, and updates the schema to allow a minimum of 0 retries. The reviewer identified an issue in ChatLunaChatModel where the new calculation (this._options.maxRetries ?? 1) + 1 inadvertently changes the default number of attempts from 1 to 2 when maxRetries is undefined. The reviewer suggested changing this to (this._options.maxRetries ?? 0) + 1 in two places to preserve the original default behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/core/src/llm-core/platform/model.ts Outdated
Comment thread packages/core/src/llm-core/platform/model.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/core/src/llm-core/platform/model.ts (1)

235-235: 💤 Low value

注意 maxAttempts 计算逻辑的重复。

maxAttempts 的计算公式 Math.max(1, (this._options.maxRetries ?? 1) + 1) 在流式(Line 235)和非流式(Line 633)路径中重复出现。虽然按照代码规范应优先内联而非提取为函数,但这种重复可能在将来需要调整逻辑时(例如更改默认值)导致遗漏同步修改的风险。

另外,模型层的 fallback 默认值 ?? 1(表示 1 次重试 = 2 次总尝试)与配置层的默认值 5(现在表示 5 次重试 = 6 次总尝试)不一致。虽然实际运行时 maxRetries 应该总是从配置传入,这个 fallback 不太会被触发,但如果确实触发,会产生与配置默认值不同的行为。

Also applies to: 633-633

🤖 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 `@packages/core/src/llm-core/platform/model.ts` at line 235, The repeated
maxAttempts calculation using Math.max(1, (this._options.maxRetries ?? 1) + 1)
(seen where maxAttempts is declared and again later) should be consolidated into
a single helper and the fallback default aligned with the configuration default;
add a small private utility (e.g., computeMaxAttempts or normalizeMaxRetries)
that reads this._options.maxRetries, applies the proper fallback (use the config
default 5 instead of 1) and returns Math.max(1, retries + 1), then replace the
inline occurrences of maxAttempts with calls to that helper so both the stream
path and non-stream path use the same logic and default.
🤖 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.

Nitpick comments:
In `@packages/core/src/llm-core/platform/model.ts`:
- Line 235: The repeated maxAttempts calculation using Math.max(1,
(this._options.maxRetries ?? 1) + 1) (seen where maxAttempts is declared and
again later) should be consolidated into a single helper and the fallback
default aligned with the configuration default; add a small private utility
(e.g., computeMaxAttempts or normalizeMaxRetries) that reads
this._options.maxRetries, applies the proper fallback (use the config default 5
instead of 1) and returns Math.max(1, retries + 1), then replace the inline
occurrences of maxAttempts with calls to that helper so both the stream path and
non-stream path use the same logic and default.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f3261163-b79b-4e19-90fa-755d50a1ecc7

📥 Commits

Reviewing files that changed from the base of the PR and between b1e0ddb and 291c4cb.

📒 Files selected for processing (3)
  • packages/core/src/llm-core/platform/client.ts
  • packages/core/src/llm-core/platform/model.ts
  • packages/core/src/services/chat.ts

@dingyi222666 dingyi222666 merged commit da16548 into ChatLunaLab:v1-dev Jun 7, 2026
5 checks passed
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.

2 participants