feat: add per-channel retry attempts - #6240
Conversation
|
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 (8)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughThe change adds configurable channel retry attempts, request-scoped retry state, exclusion-aware channel selection, and relay integration. It validates retry settings and adds tests for retry exhaustion and excluded-channel priority selection. ChangesChannel retry routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Relay
participant ChannelRetry
participant ChannelSelector
participant ChannelCache
Relay->>ChannelRetry: record channel failure
ChannelRetry-->>Relay: return locked channel and exclusions
Relay->>ChannelSelector: request eligible channel
ChannelSelector->>ChannelCache: select channel excluding exhausted IDs
ChannelCache-->>ChannelSelector: return channel or no candidate
ChannelSelector-->>Relay: return selected channel
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/channel_select.go (1)
142-147: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix cross-group retry condition when channels are excluded.
By forcing
priorityRetry = 0when channels are excluded (to guarantee robust priority selection), thecrossGroupRetrygroup switch condition breaks. BecausepriorityRetrysits at0, the check>= common.RetryTimeswill always evaluate to false, trapping the retry loop in the current group instead of failing over to the next group.Replace
priorityRetrywithparam.GetRetry()in the check and log message to evaluate the actual number of attempts made within the current group correctly.🐛 Proposed fix
- if crossGroupRetry && priorityRetry >= common.RetryTimes { + if crossGroupRetry && param.GetRetry() >= common.RetryTimes { // Current group has exhausted all retries, prepare to switch to next group // This request still uses current group, but next retry will use next group // 当前分组已用完所有重试次数，准备切换到下一个分组 // 本次请求仍使用当前分组，但下次重试将使用下一个分组 - logger.LogDebug(param.Ctx, "Current group %s retries exhausted (priorityRetry=%d >= RetryTimes=%d), preparing switch to next group for next retry", autoGroup, priorityRetry, common.RetryTimes) + logger.LogDebug(param.Ctx, "Current group %s retries exhausted (attempts=%d >= RetryTimes=%d), preparing switch to next group for next retry", autoGroup, param.GetRetry(), common.RetryTimes)🤖 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 `@service/channel_select.go` around lines 142 - 147, In the cross-group retry branch guarded by crossGroupRetry, replace priorityRetry with param.GetRetry() in both the RetryTimes comparison and the associated logger.LogDebug arguments, so group switching uses the actual current-group attempt count when channels are excluded.model/ability.go (1)
115-122: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLost exclusion filter when
retry != 0.When
retry != 0,channelQueryis reassigned but thechannel_id NOT IN ?filter is omitted. Additionally,getPrioritydoes not factor inexcludedchannels when determining the priority level.If a caller provides both
retry > 0andexcluded(e.g., if upstream selection stops forcingretry = 0), the query will select channels ignoring the exclusion list. The in-memory filter inGetChannelWithExclusionswould then strip them, potentially returningnileven if valid channels exist at other priorities.To fix this, update
getPriorityto accept and apply theexcludedmap, and re-apply thechannel_id NOT IN ?clause when rebuildingchannelQueryforretry != 0.🤖 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 `@model/ability.go` around lines 115 - 122, The retry path in the channel-selection logic drops exclusions. Update getPriority to accept and apply the excluded-channel map, pass excluded from its caller, and preserve the channel_id NOT IN ? condition when rebuilding channelQuery for retry != 0 in the surrounding ability query flow.
🧹 Nitpick comments (1)
model/ability.go (1)
147-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant in-memory exclusion.
Since
getChannelQueryWithExclusionsalready filters out the excluded channels in the SQL query (whenretry == 0), this in-memory filtering loop is largely redundant. Once the SQL query accurately handles exclusions for allretryvalues, this entire block can be safely removed to simplify the logic.🤖 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 `@model/ability.go` around lines 147 - 155, Update getChannelQueryWithExclusions to apply excluded-channel filtering for every retry value, then remove the redundant in-memory exclusion block that rebuilds abilities. Preserve the existing abilities result flow after the query handles exclusions.
🤖 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 `@web/default/src/i18n/locales/zh-TW.json`:
- Line 2008: Restore the protected key in the locale entry by changing
`footer.newapi.projectAttributionSuffix` back to
`footer.new\u0061pi.projectAttributionSuffix`; leave its translated value
unchanged.
---
Outside diff comments:
In `@model/ability.go`:
- Around line 115-122: The retry path in the channel-selection logic drops
exclusions. Update getPriority to accept and apply the excluded-channel map,
pass excluded from its caller, and preserve the channel_id NOT IN ? condition
when rebuilding channelQuery for retry != 0 in the surrounding ability query
flow.
In `@service/channel_select.go`:
- Around line 142-147: In the cross-group retry branch guarded by
crossGroupRetry, replace priorityRetry with param.GetRetry() in both the
RetryTimes comparison and the associated logger.LogDebug arguments, so group
switching uses the actual current-group attempt count when channels are
excluded.
---
Nitpick comments:
In `@model/ability.go`:
- Around line 147-155: Update getChannelQueryWithExclusions to apply
excluded-channel filtering for every retry value, then remove the redundant
in-memory exclusion block that rebuilds abilities. Preserve the existing
abilities result flow after the query handles exclusions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cffaca0-8a33-4b89-8665-884ebda992f3
📒 Files selected for processing (24)
constant/context_key.gocontroller/channel.gocontroller/channel_authz.gocontroller/relay.gomodel/ability.gomodel/ability_retry_test.gomodel/channel.gomodel/channel_cache.goservice/channel_retry.goservice/channel_retry_test.goservice/channel_select.goweb/classic/src/components/table/channels/modals/EditChannelModal.jsxweb/default/src/features/channels/components/drawers/channel-mutate-drawer.tsxweb/default/src/features/channels/constants.tsweb/default/src/features/channels/lib/channel-form-errors.tsweb/default/src/features/channels/lib/channel-form.tsweb/default/src/features/channels/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.jsonweb/default/src/i18n/locales/zh.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
model/ability.go (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass excluded IDs into
getPriority
getPriorityis only called fromgetChannelQueryWithExclusions, which already buildsexcludedIDs. Accepting[]inthere avoids converting the same set twice.🤖 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 `@model/ability.go` around lines 63 - 72, Update getPriority to accept the already-built excluded channel IDs as a []int parameter, and use that slice directly for the channel_id NOT IN filter instead of converting excluded internally. Update its sole caller, getChannelQueryWithExclusions, to pass excludedIDs and remove the redundant conversion path while preserving the existing empty-slice behavior.
🤖 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 `@model/ability.go`:
- Around line 63-72: Update getPriority to accept the already-built excluded
channel IDs as a []int parameter, and use that slice directly for the channel_id
NOT IN filter instead of converting excluded internally. Update its sole caller,
getChannelQueryWithExclusions, to pass excludedIDs and remove the redundant
conversion path while preserving the existing empty-slice behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 474936de-a29c-4bf1-afc3-67a273afde2b
📒 Files selected for processing (5)
model/ability.gomodel/ability_retry_test.goservice/channel_retry.goservice/channel_select.goweb/default/src/i18n/locales/zh-TW.json
🚧 Files skipped from review as they are similar to previous changes (3)
- service/channel_select.go
- model/ability_retry_test.go
- service/channel_retry.go
004a70d to
736e451
Compare
1 participant
Important
📝 变更描述 / Description
当前的
RetryTimes只控制请求级重试，重试过程中可能过早切换渠道，导致不稳定但低成本的渠道没有获得足够的重试机会，也让“成本优先还是稳定优先”的策略难以表达。本 PR 增加渠道级
retry_attempts配置：RetryTimes作为单请求的全局安全上限。例如：
这样可以在保持成本优势的同时，避免路由器在主渠道只失败一次后就随机切换。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
go test ./...、default 类型检查和生产构建、classic 生产构建，以及改动文件定向 lint。📸 运行证明 / Proof of Work
channels.retry_attempts字段存在。/api/status返回 200。Summary by CodeRabbit
New Features
Bug Fixes
Tests