🐛 fix(channel): evict auto-disabled multi-key channels from cache#4983
🐛 fix(channel): evict auto-disabled multi-key channels from cache#4983t0ng7u wants to merge 1 commit into
Conversation
Ensure multi-key channels are removed from the in-memory routing cache when all keys become auto-disabled, preventing subsequent requests from repeatedly selecting channels with no available keys. Also make multi-key status updates more robust by handling missing key matches, checking actual enabled key availability, and restoring the channel status when a key is re-enabled. Add regression coverage for disabled cached channels and multi-key cache eviction.
WalkthroughThe PR enhances multi-key channel status management by adding auto-disable behavior when no multi-keys are enabled and optimizing cache updates to skip unnecessary writes when status does not change. This improves channel lifecycle handling and reduces redundant cache operations in the in-memory caching path. ChangesMulti-key Channel Status Auto-disable and Cache Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 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: 2
🧹 Nitpick comments (1)
model/channel_cache_test.go (1)
55-68: ⚡ Quick winExercise
UpdateChannelStatusdirectly in this regression.This test currently calls
handlerMultiKeyUpdate()andCacheUpdateChannelStatus()manually, so it won't catch a regression in the newUpdateChannelStatus()cache-eviction branch. Since the production change is the conditionalCacheUpdateChannelStatus()call insideUpdateChannelStatus, the regression should go through that public flow instead of its internals.🤖 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/channel_cache_test.go` around lines 55 - 68, The test exercises channel-status changes by calling handlerMultiKeyUpdate and CacheUpdateChannelStatus directly, so it won't catch regressions in the new UpdateChannelStatus cache-eviction branch; change the test to invoke the public UpdateChannelStatus(path that takes channel id/status/reason or the exported UpdateChannelStatus function) after performing the multi-key update (or replace the manual CacheUpdateChannelStatus call with a call to UpdateChannelStatus for the channel id and new status), ensuring the call goes through UpdateChannelStatus (not CacheUpdateChannelStatus) so the conditional cache-eviction branch is exercised; keep references to handlerMultiKeyUpdate and CacheUpdateChannelStatus only for setup/verification but use UpdateChannelStatus to perform the status update.
🤖 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/channel.go`:
- Around line 717-725: The DB-path early return that checks channel.Status ==
status must be changed so the no-op check runs after handlerMultiKeyUpdate();
instead of returning based solely on channel.Status, reload or compare the
persisted ChannelInfo/MultiKeyStatusList against the updated
channelCache.ChannelInfo (or compare MultiKeyStatusList entries) and only
short-circuit if the per-key status lists are identical; this ensures
handlerMultiKeyUpdate(channelCache, usingKey, status, reason) updates are
persisted via CacheUpdateChannelStatus/DB write even when the aggregate
channel.Status remains enabled.
- Around line 653-663: When handling the whole-channel enable path (when
usingKey == ""), do not just flip channel.Status and return; you must clear or
recompute the per-key disabled state so GetNextEnabledKey() won't still see all
keys as disabled. In the branch that sets channel.Status, update
channel.MultiKeyStatusList (or the structure holding per-key disabled maps) to
reflect the new enabled state (e.g., reset/clear the list or set each key's
status to enabled) and persist any changes (same place
GetOtherInfo()/SetOtherInfo() are used for channel metadata) before returning so
route cache and per-key lookup remain consistent.
---
Nitpick comments:
In `@model/channel_cache_test.go`:
- Around line 55-68: The test exercises channel-status changes by calling
handlerMultiKeyUpdate and CacheUpdateChannelStatus directly, so it won't catch
regressions in the new UpdateChannelStatus cache-eviction branch; change the
test to invoke the public UpdateChannelStatus(path that takes channel
id/status/reason or the exported UpdateChannelStatus function) after performing
the multi-key update (or replace the manual CacheUpdateChannelStatus call with a
call to UpdateChannelStatus for the channel id and new status), ensuring the
call goes through UpdateChannelStatus (not CacheUpdateChannelStatus) so the
conditional cache-eviction branch is exercised; keep references to
handlerMultiKeyUpdate and CacheUpdateChannelStatus only for setup/verification
but use UpdateChannelStatus to perform the status 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: b2631c94-4e48-41ce-acd1-0ee944fd40b2
📒 Files selected for processing (2)
model/channel.gomodel/channel_cache_test.go
| if keyIndex < 0 { | ||
| if usingKey != "" { | ||
| common.SysLog(fmt.Sprintf("failed to update multi-key status: channel_id=%d, using key not found", channel.Id)) | ||
| return | ||
| } | ||
| channel.Status = status | ||
| info := channel.GetOtherInfo() | ||
| info["status_reason"] = reason | ||
| info["status_time"] = common.GetTimestamp() | ||
| channel.SetOtherInfo(info) | ||
| return |
There was a problem hiding this comment.
Don't re-enable the aggregate channel without reconciling per-key state.
When usingKey == "", this branch flips channel.Status and returns without touching MultiKeyStatusList. If a fully auto-disabled multi-key channel is re-enabled through this path, GetNextEnabledKey() will still see every key as disabled while the route cache now treats the channel as enabled. Please clear or recompute the per-key disabled maps before returning from the whole-channel enable path.
🤖 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/channel.go` around lines 653 - 663, When handling the whole-channel
enable path (when usingKey == ""), do not just flip channel.Status and return;
you must clear or recompute the per-key disabled state so GetNextEnabledKey()
won't still see all keys as disabled. In the branch that sets channel.Status,
update channel.MultiKeyStatusList (or the structure holding per-key disabled
maps) to reflect the new enabled state (e.g., reset/clear the list or set each
key's status to enabled) and persist any changes (same place
GetOtherInfo()/SetOtherInfo() are used for channel metadata) before returning so
route cache and per-key lookup remain consistent.
| beforeStatus := channelCache.Status | ||
| pollingLock := GetChannelPollingLock(channelId) | ||
| pollingLock.Lock() | ||
| // 如果是多Key模式，更新缓存中的状态 | ||
| handlerMultiKeyUpdate(channelCache, usingKey, status, reason) | ||
| pollingLock.Unlock() | ||
| if beforeStatus != channelCache.Status { | ||
| CacheUpdateChannelStatus(channelId, channelCache.Status) | ||
| } |
There was a problem hiding this comment.
Persist per-key recoveries even when the aggregate status stays enabled.
This cache-path change updates channelCache.ChannelInfo before comparing beforeStatus, but the DB path still short-circuits at Line 750 when channel.Status == status. Re-enabling one key on an already-enabled multi-key channel will clear the in-memory MultiKeyStatusList entry and then return before the DB copy is updated, so the key comes back as disabled after a reload. The no-op check for multi-key channels needs to run after handlerMultiKeyUpdate() and consider ChannelInfo changes, not just channel.Status.
🤖 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/channel.go` around lines 717 - 725, The DB-path early return that
checks channel.Status == status must be changed so the no-op check runs after
handlerMultiKeyUpdate(); instead of returning based solely on channel.Status,
reload or compare the persisted ChannelInfo/MultiKeyStatusList against the
updated channelCache.ChannelInfo (or compare MultiKeyStatusList entries) and
only short-circuit if the per-key status lists are identical; this ensures
handlerMultiKeyUpdate(channelCache, usingKey, status, reason) updates are
persisted via CacheUpdateChannelStatus/DB write even when the aggregate
channel.Status remains enabled.
1 participant
Ensure multi-key channels are removed from the in-memory routing cache when all keys become auto-disabled, preventing subsequent requests from repeatedly selecting channels with no available keys.
Also make multi-key status updates more robust by handling missing key matches, checking actual enabled key availability, and restoring the channel status when a key is re-enabled. Add regression coverage for disabled cached channels and multi-key cache eviction.
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
Bug Fixes
Tests