feat(monitor): add scheduled retest for auto-disabled channels - #2988
Closed
cyilin36 wants to merge 22 commits into
Closed
feat(monitor): add scheduled retest for auto-disabled channels#2988cyilin36 wants to merge 22 commits into
cyilin36 wants to merge 22 commits into
Conversation
Contributor
WalkthroughAdds an automated auto-disabled channel testing system: serialized task runner, scheduler, runner implementation, logging utilities, controller integration, settings UI, and main startup hook. New APIs provide submission guard, periodic scheduling, and centralized failure/log handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Scheduler as Scheduler\n(StartAutoDisabledChannelTestScheduler)
participant RunGuard as RunGuard (SubmitWithPending)
participant Runner as AutoDisabledRunner\n(RunAutoDisabledChannelTest)
participant Controller as Controller/TestExecutor
participant ModelDB as Model/DB
participant Logger as Logger/ErrorLog
Scheduler->>RunGuard: SubmitWithPending(TaskKindAutoDisabledChannel, job)
RunGuard-->>Scheduler: ack / queued or started
RunGuard->>Runner: start job
Runner->>ModelDB: ListChannels(filter: AutoDisabled)
ModelDB-->>Runner: channels[]
loop per channel
Runner->>Controller: Execute(channel) -> test result (success|failure, ms)
Controller-->>Runner: ChannelTestExecution
alt success && ms < threshold
Runner->>ModelDB: EnableChannel / UpdateResponseTime
ModelDB-->>Runner: OK
else failure or slow
Runner->>Logger: RecordChannelTestErrorLog(...)
Logger-->>ModelDB: model.RecordErrorLog(...)
ModelDB-->>Runner: logged
Runner->>Runner: HandleFailure callback (centralized)
end
end
Runner->>Scheduler: NotifyDone (optional)
RunGuard-->>Runner: finish, run next pending (if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
service/channeltest/scheduler.go (1)
31-31: Silently discarding the error fromrun(false).If the run callback returns an error (e.g., nil Execute function, or
SubmitWithPendingreturnsErrTaskRunning), it's swallowed here. Consider logging it so operators can diagnose scheduling issues.📝 Proposed fix
- _ = run(false) + if err := run(false); err != nil { + common.SysLog(fmt.Sprintf("auto-disabled channel test run error: %v", err)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/channeltest/scheduler.go` at line 31, The call to run(false) in scheduler.go currently discards any returned error; update the code that invokes run(false) to capture its error (e.g., err := run(false)) and surface it either by logging it with the component logger (including the error value and context such as "scheduler run failed" or the task id) or by returning it up the call chain; ensure errors from cases like a nil Execute function or SubmitWithPending returning ErrTaskRunning are included in the log/return so operators can diagnose scheduling failures (refer to the run function, SubmitWithPending and ErrTaskRunning symbols when locating the call).controller/channel-test.go (2)
369-376: Pre-existing: directjson.Marshalusage violates coding guidelines.Line 369 calls
json.Marshal(convertedRequest)directly instead of usingcommon.Marshal(). While this is not introduced in this PR, it violates the project's coding guidelines requiring all JSON marshal/unmarshal operations to use wrapper functions fromcommon/json.go. Consider addressing this in a follow-up. As per coding guidelines: "Do NOT directly import or callencoding/jsonfor marshal/unmarshal in business code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel-test.go` around lines 369 - 376, Replace the direct call to json.Marshal(convertedRequest) with the project wrapper common.Marshal to comply with the JSON-encoding guideline; update the block in the function that produces testResult to call common.Marshal(convertedRequest), capture its error into the same error variable, and construct the same testResult (context: c, localErr: err, newAPIError: types.NewError(err, types.ErrorCodeJsonMarshalFailed)) when err != nil — locate the call to json.Marshal in controller/channel-test.go (around the code that returns testResult) and swap it to use common.Marshal().
927-929: RemoveAutomaticallyTestAutoDisabledChannels— it is an unused trivial wrapper.This function is unreferenced in the codebase and only delegates to
StartAutoDisabledChannelTestScheduler()with no additional logic. Removing it eliminates unnecessary API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel-test.go` around lines 927 - 929, Remove the unused trivial wrapper function AutomaticallyTestAutoDisabledChannels by deleting its declaration; leave StartAutoDisabledChannelTestScheduler untouched (it contains the real logic). Also search for and remove any stray references, tests or documentation that mention AutomaticallyTestAutoDisabledChannels and run a build to confirm there are no remaining references to that symbol.service/channeltest/auto_disabled_runner.go (1)
81-84:NewAPIErroris silently overwritten whenLocalErris also set.If
Executereturns both aLocalErrand aNewAPIError, line 83 replaces the originalNewAPIErrorwith a genericErrorCodeInvalidRequestderived fromLocalErr. This may lose specific upstream error details. If this is intentional (LocalErr always takes precedence), a brief comment would clarify the design intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/channeltest/auto_disabled_runner.go` around lines 81 - 84, The code overwrites result.NewAPIError with a generic types.NewOpenAIError based on result.LocalErr, losing any upstream error detail; update the logic in auto_disabled_runner.go (around newAPIError/result.NewAPIError/result.LocalErr) to only create/assign types.NewOpenAIError when result.NewAPIError is nil (i.e., preserve result.NewAPIError if present), or if the overwrite was intentional, add a clear comment next to the assignment explaining that LocalErr must take precedence and why.web/src/pages/Setting/Operation/SettingsMonitoring.jsx (1)
339-343: Nit: inconsistentguttervalue on the save button row.All data rows use
gutter={16}; this row usesgutter={12}. Since there are no<Col>children here, the value has no visual effect, but it may be a leftover typo.🔧 Suggested fix
- <Row gutter={12}> + <Row gutter={16}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/Setting/Operation/SettingsMonitoring.jsx` around lines 339 - 343, The Row wrapping the save Button uses gutter={12} which is inconsistent with other data rows that use gutter={16}; update the Row containing the Button (the one calling onSubmit and rendering t('保存监控设置')) to use gutter={16} (or remove the gutter prop entirely) so it matches the rest of the layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/channeltest/auto_disabled_runner.go`:
- Around line 90-93: The code calls common.GetContextKeyString(result.Context,
constant.ContextKeyChannelKey) without checking result.Context and can panic if
result.Context is nil; update the block in auto_disabled_runner.go (around the
passed++/enabled++ logic) to guard result.Context for nil before calling
GetContextKeyString and pass a safe fallback (e.g., empty string or other safe
identifier) into options.EnableChannel(channel.Id, <contextKey>, channel.Name),
or skip/handle enabling when context is absent so EnableChannel is never called
with a nil-derived context; reference symbols: result.Context,
common.GetContextKeyString, constant.ContextKeyChannelKey,
options.EnableChannel.
In `@service/channeltest/scheduler.go`:
- Around line 25-36: The loop reads
monitorSetting.AutoTestAutoDisabledChannelMinutes and calls
time.Sleep(math.Round(frequency) minutes) which can become time.Sleep(0) and
spin; clamp the computed sleep interval to a sensible minimum (e.g., ensure
minutes = int(math.Round(frequency)); if minutes < 1 { minutes = 1 }), then call
time.Sleep(time.Duration(minutes) * time.Minute) and optionally log when you
clamp the value; apply the same guard to the AutomaticallyTestChannels routine
in controller/channel-test.go and reference GetMonitorSetting,
AutoTestAutoDisabledChannelMinutes, AutoTestAutoDisabledChannelEnabled, and run
when making the change.
---
Nitpick comments:
In `@controller/channel-test.go`:
- Around line 369-376: Replace the direct call to json.Marshal(convertedRequest)
with the project wrapper common.Marshal to comply with the JSON-encoding
guideline; update the block in the function that produces testResult to call
common.Marshal(convertedRequest), capture its error into the same error
variable, and construct the same testResult (context: c, localErr: err,
newAPIError: types.NewError(err, types.ErrorCodeJsonMarshalFailed)) when err !=
nil — locate the call to json.Marshal in controller/channel-test.go (around the
code that returns testResult) and swap it to use common.Marshal().
- Around line 927-929: Remove the unused trivial wrapper function
AutomaticallyTestAutoDisabledChannels by deleting its declaration; leave
StartAutoDisabledChannelTestScheduler untouched (it contains the real logic).
Also search for and remove any stray references, tests or documentation that
mention AutomaticallyTestAutoDisabledChannels and run a build to confirm there
are no remaining references to that symbol.
In `@service/channeltest/auto_disabled_runner.go`:
- Around line 81-84: The code overwrites result.NewAPIError with a generic
types.NewOpenAIError based on result.LocalErr, losing any upstream error detail;
update the logic in auto_disabled_runner.go (around
newAPIError/result.NewAPIError/result.LocalErr) to only create/assign
types.NewOpenAIError when result.NewAPIError is nil (i.e., preserve
result.NewAPIError if present), or if the overwrite was intentional, add a clear
comment next to the assignment explaining that LocalErr must take precedence and
why.
In `@service/channeltest/scheduler.go`:
- Line 31: The call to run(false) in scheduler.go currently discards any
returned error; update the code that invokes run(false) to capture its error
(e.g., err := run(false)) and surface it either by logging it with the component
logger (including the error value and context such as "scheduler run failed" or
the task id) or by returning it up the call chain; ensure errors from cases like
a nil Execute function or SubmitWithPending returning ErrTaskRunning are
included in the log/return so operators can diagnose scheduling failures (refer
to the run function, SubmitWithPending and ErrTaskRunning symbols when locating
the call).
In `@web/src/pages/Setting/Operation/SettingsMonitoring.jsx`:
- Around line 339-343: The Row wrapping the save Button uses gutter={12} which
is inconsistent with other data rows that use gutter={16}; update the Row
containing the Button (the one calling onSubmit and rendering t('保存监控设置')) to
use gutter={16} (or remove the gutter prop entirely) so it matches the rest of
the layout.
- Added caching for the original Content-Type header in the parseMultipartFormData function. - This change ensures that the Content-Type is retrieved from the context if previously set, enhancing performance and consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/channel-test.go (1)
801-851:⚠️ Potential issue | 🔴 CriticalThe "automatically channel test finished" log fires prematurely because
SubmitWithPendingis asynchronous.
SubmitWithPending(lines 801-851) spawns the callback in a goroutine viagopool.Go()inservice/channeltest/runner_guard.go:57and returns immediately without waiting. The caller at line 915 logs"automatically channel test finished"right aftertestAllChannels(false)returns, so this message appears before the channel testing actually begins. Either wait for the async operation to complete or move the log into the callback before it returns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel-test.go` around lines 801 - 851, The caller logs "automatically channel test finished" immediately after testAllChannels(...) returns, but SubmitWithPending spawns the provided callback asynchronously (via gopool.Go in service/channeltest/runner_guard.go) so the log fires before work completes; fix by either waiting for the async task to finish or moving the completion log/notify inside the callback: update the testAllChannels flow to signal completion (e.g., use a sync.WaitGroup or a done channel returned from SubmitWithPending, or modify SubmitWithPending to offer a blocking variant) or simply move the Notify/automatic-finished log into the anonymous callback passed to SubmitWithPending so it runs just before the callback returns.
🧹 Nitpick comments (3)
common/gin.go (1)
306-312: Extract a shared helper and promote the magic string to a named constant.Lines 306–312 are a verbatim copy of the caching block already present in
ParseMultipartFormReusable(lines 248–254). The raw string"_original_multipart_ct"appears four times across the file. A helper function eliminates the duplication, and the type assertion can be made panic-safe with a comma-ok check at the same time.♻️ Proposed refactor: helper + constant
Add a constant and a helper near the top of the file (or alongside the other
constdeclarations):+const keyOriginalMultipartCT = "_original_multipart_ct" + +// getOrCacheMultipartContentType returns the Content-Type that was active when +// the multipart body was first read, caching it on the first call so that +// subsequent calls are unaffected by header mutations. +func getOrCacheMultipartContentType(c *gin.Context) string { + if saved, ok := c.Get(keyOriginalMultipartCT); ok { + if ct, ok := saved.(string); ok { + return ct + } + } + ct := c.Request.Header.Get("Content-Type") + c.Set(keyOriginalMultipartCT, ct) + return ct +}Replace the duplicated block in
ParseMultipartFormReusable(lines 248–254):- var contentType string - if saved, ok := c.Get("_original_multipart_ct"); ok { - contentType = saved.(string) - } else { - contentType = c.Request.Header.Get("Content-Type") - c.Set("_original_multipart_ct", contentType) - } + contentType := getOrCacheMultipartContentType(c)Replace the newly added block in
parseMultipartFormData(lines 306–312):- var contentType string - if saved, ok := c.Get("_original_multipart_ct"); ok { - contentType = saved.(string) - } else { - contentType = c.Request.Header.Get("Content-Type") - c.Set("_original_multipart_ct", contentType) - } + contentType := getOrCacheMultipartContentType(c)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/gin.go` around lines 306 - 312, Extract the duplicated caching logic into a small helper and replace the magic string with a named constant: add a const (e.g. originalMultipartCTKey) and a helper function (e.g. getOriginalMultipartContentType(c *gin.Context) string) that checks c.Get(originalMultipartCTKey) with a comma-ok type assertion and returns the cached value or reads c.Request.Header.Get("Content-Type"), sets the cache with c.Set(originalMultipartCTKey, value), and returns it; then call this helper from ParseMultipartFormReusable and parseMultipartFormData to remove duplication and make the type assertion safe.model/log.go (1)
302-308: Duplicated anonymous struct literal — consider a named typeThe anonymous struct type is written out identically in both the
var channelsdeclaration (line 294) and theappendliteral (lines 302–308). Defining a small named type eliminates duplication and makes future changes to the struct (e.g., adding a field) a single-point edit.♻️ Proposed refactor
+type channelIdName struct { + Id int `gorm:"column:id"` + Name string `gorm:"column:name"` +} if channelIds.Len() > 0 { - var channels []struct { - Id int `gorm:"column:id"` - Name string `gorm:"column:name"` - } + var channels []channelIdName if common.MemoryCacheEnabled { for _, channelId := range channelIds.Items() { if cacheChannel, err := CacheGetChannel(channelId); err == nil { - channels = append(channels, struct { - Id int `gorm:"column:id"` - Name string `gorm:"column:name"` - }{ + channels = append(channels, channelIdName{ Id: channelId, Name: cacheChannel.Name, }) } } } else { - if err = DB.Table("channels").Select("id, name").Where("id IN ?", channelIds.Items()).Find(&channels).Error; err != nil { + if err = DB.Table("channels").Select("id, name").Where("id IN ?", channelIds.Items()).Find(&channels).Error; err != nil { return logs, total, err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/log.go` around lines 302 - 308, The anonymous struct used for channels is duplicated; define a small named type (e.g., channelEntry) with the same fields and tags, replace the existing var channels declaration to use []channelEntry, and change the append call that currently constructs an anonymous struct to construct a channelEntry instead (references: the channels variable and the append(...) literal in model/log.go).controller/channel-test.go (1)
870-872: Consider logging or forwarding the discarded failurereasonstring.The fourth parameter passed to
HandleFailure(likely a human-readable reason such as "response time exceeded") is silently dropped. If it carries diagnostic information beyond whatnewAPIErroralready encodes, surfacing it in the log or passing it tohandleChannelTestFailurewould improve observability for the auto-disabled retest flow.♻️ Suggested change
- HandleFailure: func(channel *model.Channel, result channeltest.ChannelTestExecution, newAPIError *types.NewAPIError, _ string) { - handleChannelTestFailure(channel, testResult{context: result.Context}, newAPIError) + HandleFailure: func(channel *model.Channel, result channeltest.ChannelTestExecution, newAPIError *types.NewAPIError, reason string) { + if reason != "" { + common.SysLog(fmt.Sprintf("auto-disabled channel #%d failure reason: %s", channel.Id, reason)) + } + handleChannelTestFailure(channel, testResult{context: result.Context}, newAPIError)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/channel-test.go` around lines 870 - 872, The HandleFailure callback currently discards the fourth parameter (the human-readable reason string); update the HandleFailure implementation to either include that reason when calling handleChannelTestFailure or log it before returning. Specifically, modify the anonymous HandleFailure function to capture the fourth arg (e.g., reason string) and pass it into handleChannelTestFailure (e.g., extend testResult with a reason field or add an extra param) or call processLogger/whatever logging helper with a message including channel.ID, result.Context and the reason so the discarded diagnostic text is preserved; ensure changes reference HandleFailure and handleChannelTestFailure so callers remain consistent.
🤖 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 931-933: Remove the unused exported wrapper
AutomaticallyTestAutoDisabledChannels: delete the function declaration that only
calls StartAutoDisabledChannelTestScheduler(), since main.go already invokes
StartAutoDisabledChannelTestScheduler() directly; ensure there are no external
references to AutomaticallyTestAutoDisabledChannels before removing it and run
tests/build to confirm nothing else depends on it.
In `@model/log.go`:
- Around line 298-310: The cache lookup loop silently swallows CacheGetChannel
errors causing empty channel names when MemoryCacheEnabled is true; update the
logic in the block that iterates channelIds (where CacheGetChannel is called) to
collect IDs that missed the cache and perform a single DB query fallback to load
their names (or at minimum log the cache-miss via debug with channelId and
error). Ensure you still append the channel structs with the DB-derived Name for
missed IDs so the API returns correct channel_name (use the existing channels
slice and the channelIds.Items() iteration context).
---
Outside diff comments:
In `@controller/channel-test.go`:
- Around line 801-851: The caller logs "automatically channel test finished"
immediately after testAllChannels(...) returns, but SubmitWithPending spawns the
provided callback asynchronously (via gopool.Go in
service/channeltest/runner_guard.go) so the log fires before work completes; fix
by either waiting for the async task to finish or moving the completion
log/notify inside the callback: update the testAllChannels flow to signal
completion (e.g., use a sync.WaitGroup or a done channel returned from
SubmitWithPending, or modify SubmitWithPending to offer a blocking variant) or
simply move the Notify/automatic-finished log into the anonymous callback passed
to SubmitWithPending so it runs just before the callback returns.
---
Nitpick comments:
In `@common/gin.go`:
- Around line 306-312: Extract the duplicated caching logic into a small helper
and replace the magic string with a named constant: add a const (e.g.
originalMultipartCTKey) and a helper function (e.g.
getOriginalMultipartContentType(c *gin.Context) string) that checks
c.Get(originalMultipartCTKey) with a comma-ok type assertion and returns the
cached value or reads c.Request.Header.Get("Content-Type"), sets the cache with
c.Set(originalMultipartCTKey, value), and returns it; then call this helper from
ParseMultipartFormReusable and parseMultipartFormData to remove duplication and
make the type assertion safe.
In `@controller/channel-test.go`:
- Around line 870-872: The HandleFailure callback currently discards the fourth
parameter (the human-readable reason string); update the HandleFailure
implementation to either include that reason when calling
handleChannelTestFailure or log it before returning. Specifically, modify the
anonymous HandleFailure function to capture the fourth arg (e.g., reason string)
and pass it into handleChannelTestFailure (e.g., extend testResult with a reason
field or add an extra param) or call processLogger/whatever logging helper with
a message including channel.ID, result.Context and the reason so the discarded
diagnostic text is preserved; ensure changes reference HandleFailure and
handleChannelTestFailure so callers remain consistent.
In `@model/log.go`:
- Around line 302-308: The anonymous struct used for channels is duplicated;
define a small named type (e.g., channelEntry) with the same fields and tags,
replace the existing var channels declaration to use []channelEntry, and change
the append call that currently constructs an anonymous struct to construct a
channelEntry instead (references: the channels variable and the append(...)
literal in model/log.go).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
common/gin.gocontroller/channel-test.gomodel/log.gorelay/helper/stream_scanner.goservice/channeltest/auto_disabled_runner.goservice/channeltest/scheduler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- service/channeltest/scheduler.go
- service/channeltest/auto_disabled_runner.go
Comment on lines
+931
to
+933
| func AutomaticallyTestAutoDisabledChannels() { | ||
| StartAutoDisabledChannelTestScheduler() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all usages of AutomaticallyTestAutoDisabledChannels
rg -n 'AutomaticallyTestAutoDisabledChannels' --type go
# Also check for StartAutoDisabledChannelTestScheduler to see where it's called
rg -n 'StartAutoDisabledChannelTestScheduler' --type goRepository: QuantumNous/new-api
Length of output: 357
Remove the unused AutomaticallyTestAutoDisabledChannels wrapper function.
The function is only defined at line 931 and never called anywhere in the codebase. Since main.go directly calls StartAutoDisabledChannelTestScheduler() (line 108), this exported wrapper is dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/channel-test.go` around lines 931 - 933, Remove the unused
exported wrapper AutomaticallyTestAutoDisabledChannels: delete the function
declaration that only calls StartAutoDisabledChannelTestScheduler(), since
main.go already invokes StartAutoDisabledChannelTestScheduler() directly; ensure
there are no external references to AutomaticallyTestAutoDisabledChannels before
removing it and run tests/build to confirm nothing else depends on it.
Comment on lines
+298
to
+310
| if common.MemoryCacheEnabled { | ||
| // Cache get channel | ||
| for _, channelId := range channelIds.Items() { | ||
| if cacheChannel, err := CacheGetChannel(channelId); err == nil { | ||
| channels = append(channels, struct { | ||
| Id int `gorm:"column:id"` | ||
| Name string `gorm:"column:name"` | ||
| }{ | ||
| Id: channelId, | ||
| Name: cacheChannel.Name, | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent cache miss silently produces empty ChannelName for valid channels
CacheGetChannel (when MemoryCacheEnabled=true) only looks up channelsIDM and returns an error — with no DB fallback — if the channel isn't found (e.g., cache not yet warmed after a new channel is created, or a transient eviction). Errors are swallowed here, so any such channel gets an empty channel_name in the API response with no indication to the caller. The original DB path would have returned the correct name.
Consider falling back to a single DB query for the IDs that were missed in the cache, or at least logging at debug level when cache misses occur so they are observable:
🛡️ Proposed fix: fall back to DB for cache misses
if common.MemoryCacheEnabled {
// Cache get channel
+ var missedIds []int
for _, channelId := range channelIds.Items() {
if cacheChannel, err := CacheGetChannel(channelId); err == nil {
channels = append(channels, struct {
Id int `gorm:"column:id"`
Name string `gorm:"column:name"`
}{
Id: channelId,
Name: cacheChannel.Name,
})
+ } else {
+ missedIds = append(missedIds, channelId)
}
}
+ if len(missedIds) > 0 {
+ var missedChannels []struct {
+ Id int `gorm:"column:id"`
+ Name string `gorm:"column:name"`
+ }
+ if err = DB.Table("channels").Select("id, name").Where("id IN ?", missedIds).Find(&missedChannels).Error; err == nil {
+ channels = append(channels, missedChannels...)
+ }
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if common.MemoryCacheEnabled { | |
| // Cache get channel | |
| for _, channelId := range channelIds.Items() { | |
| if cacheChannel, err := CacheGetChannel(channelId); err == nil { | |
| channels = append(channels, struct { | |
| Id int `gorm:"column:id"` | |
| Name string `gorm:"column:name"` | |
| }{ | |
| Id: channelId, | |
| Name: cacheChannel.Name, | |
| }) | |
| } | |
| } | |
| if common.MemoryCacheEnabled { | |
| // Cache get channel | |
| var missedIds []int | |
| for _, channelId := range channelIds.Items() { | |
| if cacheChannel, err := CacheGetChannel(channelId); err == nil { | |
| channels = append(channels, struct { | |
| Id int `gorm:"column:id"` | |
| Name string `gorm:"column:name"` | |
| }{ | |
| Id: channelId, | |
| Name: cacheChannel.Name, | |
| }) | |
| } else { | |
| missedIds = append(missedIds, channelId) | |
| } | |
| } | |
| if len(missedIds) > 0 { | |
| var missedChannels []struct { | |
| Id int `gorm:"column:id"` | |
| Name string `gorm:"column:name"` | |
| } | |
| if err = DB.Table("channels").Select("id, name").Where("id IN ?", missedIds).Find(&missedChannels).Error; err == nil { | |
| channels = append(channels, missedChannels...) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/log.go` around lines 298 - 310, The cache lookup loop silently swallows
CacheGetChannel errors causing empty channel names when MemoryCacheEnabled is
true; update the logic in the block that iterates channelIds (where
CacheGetChannel is called) to collect IDs that missed the cache and perform a
single DB query fallback to load their names (or at minimum log the cache-miss
via debug with channelId and error). Ensure you still append the channel structs
with the DB-derived Name for missed IDs so the API returns correct channel_name
(use the existing channels slice and the channelIds.Items() iteration context).
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
2 participants
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.
What changed
This PR adds scheduled retesting for auto-disabled channels:
New settings
monitor_setting.auto_test_auto_disabled_channel_enabledmonitor_setting.auto_test_auto_disabled_channel_minutesmonitor_setting.auto_test_auto_disabled_channel_response_thresholdSummary by CodeRabbit
New Features
Improvements