feat: per-model testing and disable in channel test - #4380
Open
wanghualoong wants to merge 4 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIterates channel models and runs per-model tests with a 120s timeout, classifies errors as channel- or model-level, disables/enables models individually, averages model latencies to update channel response time, records per-model test history, prunes 30-day history, and conditionally re-enables channels only if no channel-level errors occurred. Changes
Sequence DiagramsequenceDiagram
participant Controller as testAllChannels
participant Tester as testChannel
participant Classifier as IsChannelLevelError
participant Service as service (Enable/Disable)
participant Model as Ability/DB
participant History as ChannelTestHistory
loop per model in channel.GetModels()
Controller->>Tester: call testChannel(model) (120s timeout)
Tester-->>Controller: response / error / latency
Controller->>Classifier: classify error (if any)
Classifier-->>Controller: channel-level? yes/no
alt channel-level error
Controller->>Service: process channel-level error / ShouldDisableChannel
Service-->>Controller: decision
Controller->>Service: DisableChannel(channelId) / stop remaining models
else model-level error or timeout
Controller->>Service: DisableChannelModel(channelId, modelName, reason)
Service->>Model: UpdateAbilityModelStatus(channelId, modelName, false)
else success
Controller->>Service: EnableChannelModel(channelId, modelName)
Service->>Model: UpdateAbilityModelStatus(channelId, modelName, true)
end
Controller->>History: RecordChannelTestHistory(channelId, modelName, status, ms, err)
end
Controller->>History: PruneChannelTestHistory(30)
Controller->>Service: If no channel-level error & all models enabled -> EnableChannel(channelId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
wanghualoong
force-pushed
the
feature/per-model-channel-test
branch
from
April 22, 2026 04:48
06ad813 to
c820f31
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/channel-test.go`:
- Around line 883-918: The model-disable/invariant is inconsistent: tighten the
guard on the model-level disable (the block that calls
service.DisableChannelModel) to require common.AutomaticDisableChannelEnabled
and isChannelEnabled in addition to shouldBanModel and channel.GetAutoBan()
(e.g., if common.AutomaticDisableChannelEnabled && isChannelEnabled &&
shouldBanModel && channel.GetAutoBan() && testStatus != "unsupported") so the
automatic-disable feature is a single explicit switch; and protect the
processChannelError call by ensuring result.context is non-nil (or ensure the
timeout path carries a non-nil context) before calling processChannelError(...,
common.GetContextKeyString(result.context, ...)) to avoid a nil context panic if
IsChannelLevelError behavior changes.
- Around line 831-949: The average response time is divided by
int64(len(models)) even though totalMs only sums measured models (skips,
continues, and early break), so compute the average using the actual tested
model count: introduce a testedCount (int64) next to totalMs, increment
testedCount whenever you add milliseconds (after tok.Sub and totalMs +=
milliseconds), and at the end call channel.UpdateResponseTime(totalMs /
testedCount) only when testedCount > 0 (otherwise skip updating); update
references in this block (totalMs, models, channel.UpdateResponseTime, the loop
that breaks on channel-level error) so the average reflects only actually tested
models and avoids division-by-zero.
- Around line 848-863: The test goroutine spawns testChannel which makes HTTP
requests via adaptor.DoRequest without a cancellable context, so the child
goroutine can hang after the 120s select; fix by creating a
context.WithTimeout(testTimeout) in the caller of testChannel
(AutomaticallyTestChannels or the function that schedules the test), pass that
context into testChannel, propagate it into adaptor.DoRequest (and any
lower-level request builders) and replace http.NewRequest(...) calls with
http.NewRequestWithContext(ctx, ...) so the upstream http.Client.Do honors the
timeout and the goroutine is cancelled when the 120s deadline elapses.
In `@model/channel_test_history.go`:
- Around line 10-34: Change the ChannelTestHistory.ErrorMessage column to use a
TEXT type for cross-DB safety by adding a GORM column type tag (e.g.
gorm:"type:TEXT") on the ErrorMessage field in the ChannelTestHistory struct so
long JSON error bodies won't be truncated or cause strict-mode insert failures;
in RecordChannelTestHistory, capture and surface the result of
DB.Create(&history) (don't discard the returned error inside the gopool.Go
closure) and log any create error so failing inserts are discoverable (reference
ChannelTestHistory, ErrorMessage, RecordChannelTestHistory, gopool.Go, and
DB.Create).
In `@service/channel.go`:
- Around line 130-140: DisableChannelModel currently disables a model regardless
of the global automatic-disable toggle; update it to honor
common.AutomaticDisableChannelEnabled (or ensure callers check it) so behavior
matches DisableChannel/ShouldDisableChannel: add a guard at the start of
DisableChannelModel that returns early when
common.AutomaticDisableChannelEnabled is false, or alternatively add that same
check at the controller/channel-test.go caller (the per-model branch) before
calling DisableChannelModel; reference DisableChannelModel,
common.AutomaticDisableChannelEnabled, DisableChannel and ShouldDisableChannel
when making this change.
- Around line 129-152: The notify dedup key used in DisableChannelModel and
EnableChannelModel collides with channel-level events because both call
formatNotifyType(channelId, common.ChannelStatusAutoDisabled) /
formatNotifyType(channelId, common.ChannelStatusEnabled); update these two
functions to include the model name in the notify type (e.g. append or
interpolate modelName into the second argument) or introduce distinct
model-specific status constants and pass those to formatNotifyType so
NotifyRootUser receives a unique key per model and no longer collides with
DisableChannel/EnableChannel events.
🪄 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: ffb5b0f3-32ff-4816-a35e-7235297f7487
📒 Files selected for processing (5)
controller/channel-test.gomodel/ability.gomodel/channel_test_history.gomodel/main.goservice/channel.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
service/channel.go (2)
101-115:⚠️ Potential issue | 🟠 MajorUse a model-specific notification key.
Both model disable/enable notifications use the same dedup key as channel-level updates and the same key for every model on the channel. This can collapse per-model notifications and suppress later channel-level notifications.
🛠️ Proposed fix
func formatNotifyType(channelId int, status int) string { return fmt.Sprintf("%s_%d_%d", dto.NotifyTypeChannelUpdate, channelId, status) } + +func formatModelNotifyType(channelId int, modelName string, status int) string { + return fmt.Sprintf("%s_%d_%s_%d", dto.NotifyTypeChannelUpdate, channelId, modelName, status) +} @@ - NotifyRootUser(formatNotifyType(channelId, common.ChannelStatusAutoDisabled), subject, content) + NotifyRootUser(formatModelNotifyType(channelId, modelName, common.ChannelStatusAutoDisabled), subject, content) @@ - NotifyRootUser(formatNotifyType(channelId, common.ChannelStatusEnabled), subject, content) + NotifyRootUser(formatModelNotifyType(channelId, modelName, common.ChannelStatusEnabled), subject, content)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/channel.go` around lines 101 - 115, Notifications for per-model enable/disable are using the same deduplication key as channel-level updates and across all models; change the NotifyRootUser calls in EnableChannelModel and the corresponding disable function to use a model-specific dedup key by including modelName in the key generation (e.g. call a new or extended helper like formatModelNotifyType(channelId, modelName, common.ChannelStatusEnabled/ChannelStatusAutoDisabled) instead of formatNotifyType), update the NotifyRootUser invocation arguments accordingly so each model gets a unique notification key, and ensure any helper used (formatModelNotifyType) is implemented consistently with existing formatNotifyType semantics.
94-96:⚠️ Potential issue | 🟡 MinorHonor the global automatic-disable switch for model disables.
DisableChannelModelcan disable abilities even when global automatic channel disabling is off. Add the same global guard used by channel-disable decisions, or ensure every caller checks it before calling this public helper.🛠️ Proposed fix
func DisableChannelModel(channelId int, channelName string, modelName string, reason string) { common.SysLog(fmt.Sprintf("通道「%s」（#%d）的模型「%s」发生错误，准备禁用，原因：%s", channelName, channelId, modelName, reason)) + if !common.AutomaticDisableChannelEnabled { + common.SysLog(fmt.Sprintf("自动禁用未启用，跳过禁用通道「%s」（#%d）的模型「%s」", channelName, channelId, modelName)) + return + } err := model.UpdateAbilityModelStatus(channelId, modelName, false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/channel.go` around lines 94 - 96, DisableChannelModel can disable models regardless of the global automatic-disable setting; ensure we honor that global switch. Modify DisableChannelModel to check the global automatic-disable flag (the same flag used by channel-disable logic—e.g., the project's EnableAutoDisable/AutoDisableChannels config variable) and return immediately when automatic disables are turned off, or alternatively ensure every caller of DisableChannelModel checks that flag before invoking it; update the call site expectations and keep the existing logging and model.UpdateAbilityModelStatus(channelId, modelName, false) behavior only when the flag allows disables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/ability.go`:
- Around line 267-276: UpdateAbilityModelStatus currently updates
abilities.enabled in the DB but never refreshes the channel cache, causing stale
routing; modify UpdateAbilityModelStatus to invoke the channel cache rebuild
function immediately after a successful DB update (e.g., call
RebuildChannelCache(channelId) or the repository's existing cache refresh
function used by the ability rebuild path) and propagate or log any rebuild
error; leave IsAbilityModelEnabled as-is (it only reads), but ensure any other
code paths that flip per-model enabled state also trigger the same cache
refresh.
In `@service/channel.go`:
- Around line 73-89: Replace the custom multi-switch logic that inspects
err.StatusCode, err.ToOpenAIError(), and err.GetErrorCode() with the existing
channel-level classification helper types.IsChannelError to ensure 403 and
channel:* errors disable the channel; specifically, in the function using the
local err variable, remove the current blocks referencing err.StatusCode, oaiErr
(from err.ToOpenAIError()), and errorCode (from err.GetErrorCode()) and instead
call and return types.IsChannelError(err) (or pass the same err value to that
helper) so channel-level errors are consistently detected.
---
Duplicate comments:
In `@service/channel.go`:
- Around line 101-115: Notifications for per-model enable/disable are using the
same deduplication key as channel-level updates and across all models; change
the NotifyRootUser calls in EnableChannelModel and the corresponding disable
function to use a model-specific dedup key by including modelName in the key
generation (e.g. call a new or extended helper like
formatModelNotifyType(channelId, modelName,
common.ChannelStatusEnabled/ChannelStatusAutoDisabled) instead of
formatNotifyType), update the NotifyRootUser invocation arguments accordingly so
each model gets a unique notification key, and ensure any helper used
(formatModelNotifyType) is implemented consistently with existing
formatNotifyType semantics.
- Around line 94-96: DisableChannelModel can disable models regardless of the
global automatic-disable setting; ensure we honor that global switch. Modify
DisableChannelModel to check the global automatic-disable flag (the same flag
used by channel-disable logic—e.g., the project's
EnableAutoDisable/AutoDisableChannels config variable) and return immediately
when automatic disables are turned off, or alternatively ensure every caller of
DisableChannelModel checks that flag before invoking it; update the call site
expectations and keep the existing logging and
model.UpdateAbilityModelStatus(channelId, modelName, false) behavior only when
the flag allows disables.
🪄 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: 1d61f0cb-b42c-469d-a976-0bbb34f97c73
📒 Files selected for processing (5)
controller/channel-test.gomodel/ability.gomodel/channel_test_history.gomodel/main.goservice/channel.go
✅ Files skipped from review due to trivial changes (1)
- controller/channel-test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- model/channel_test_history.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/ability.go`:
- Around line 269-272: The cache rebuild currently uses channel.Models to
populate group2model2channels without checking per-model Ability.Enabled, so
update the cache rebuild logic in model/channel_cache.go (the code that
constructs group2model2channels) to consult Ability for each (channel.ID,
modelName) and only include the model when Ability.Enabled is true; to avoid N+1
queries preload or query all Ability rows for the set of channels/models being
processed into a map keyed by channelId+model before the loop and reference that
map while iterating channels.Models so disabled abilities are excluded from the
cache (this fixes the stale routability after InitChannelCache is called from
the Ability update).
🪄 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: e4e41604-7901-44bd-8884-d62442ae6984
📒 Files selected for processing (4)
controller/channel-test.gomodel/ability.gomodel/channel_test_history.goservice/channel.go
🚧 Files skipped from review as they are similar to previous changes (2)
- model/channel_test_history.go
- controller/channel-test.go
wanghualoong
marked this pull request as draft
May 15, 2026 05:55
Instead of testing one representative model per channel and disabling the entire channel on failure, testAllChannels() now iterates every model individually. Model-level errors (timeout, unsupported) only disable that model's ability; channel-level errors (invalid key, quota) still disable the entire channel. Each model test has 120s timeout protection and records its own test history entry. Auto-enable respects the AutomaticEnableChannelEnabled setting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix average response time dividing by actual tested count instead of total model count (avoids bias from skipped/broken models) - Gate model-level disable on AutomaticDisableChannelEnabled for consistency with threshold-based disable path - Add gorm type:text tag to ErrorMessage to prevent truncation on MySQL - Use per-model notify type key to avoid notification deduplication across multiple models in the same channel - Refresh channel cache after updating per-model ability status - Reuse types.IsChannelError() and add HTTP 403 to IsChannelLevelError for broader channel-error classification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The channel cache previously rebuilt group2model2channels by splitting channel.Models, ignoring Ability.Enabled. This meant a model disabled via UpdateAbilityModelStatus remained routable when memory cache was active. Switch to iterating abilities directly so disabled models are excluded from the routing cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wanghualoong
force-pushed
the
feature/per-model-channel-test
branch
from
May 15, 2026 06:22
c99377c to
cc0cac7
Compare
wanghualoong
marked this pull request as ready for review
May 15, 2026 06:25
6 tasks
- Use context.WithTimeout instead of time.After so the HTTP request is cancelled when the 120s deadline fires, preventing goroutine accumulation - Guard processChannelError with result.context != nil to avoid panic if IsChannelLevelError classification changes in the future - Log DB.Create errors in RecordChannelTestHistory instead of silently discarding them Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Calcium-Ion
force-pushed
the
main
branch
2 times, most recently
from
August 30, 2026 15:03
51fdfc5 to
2b6f1df
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
testAllChannels()now iterates every model in each channel individually, instead of testing one representative model per channelseedreamorimage-previewin their name are automatically skipped (marked asunsupported)AutomaticEnableChannelEnabledsettingchannel_test_historiestableChanged files
controller/channel-test.gotestAllChannels()with dual-loop per-model testing, 120s timeout, image model skipmodel/ability.goUpdateAbilityModelStatus(),IsAbilityModelEnabled()service/channel.goIsChannelLevelError(),DisableChannelModel(),EnableChannelModel()model/channel_test_history.goChannelTestHistorymodel withRecordChannelTestHistory(),PruneChannelTestHistory()model/main.goChannelTestHistoryto AutoMigrateError classification
IsChannelLevelError()):invalid_api_key,account_deactivated,billing_not_active,insufficient_quota,authentication_error,permission_error, etc. → disable entire channel + skip remaining modelsDisableChannelModel()Test plan
AutomaticEnableChannelEnabled)channel_test_historiestable records per-model results🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores