支持gemini风格api的系统提示词传递 - #1625
Conversation
WalkthroughRenames GeminiChatRequest.SystemInstructions → SystemInstruction and updates JSON tag; changes Gemini relay/stream handlers to stream empty responses instead of erroring; adds multi-key per-key relay routing with per-key context and retry logic; implements per-token minute and daily rate limits and corresponding UI/options; updates token/redemption preset values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller
participant Middleware
participant RelayHelper
participant GeminiRelay
participant ProcessError
Client->>Controller: incoming request
Controller->>Middleware: SetupContextForSelectedChannelWithKey(channel, key, index)
Middleware-->>Controller: context populated (ChannelKey, BaseURL, meta)
Controller->>RelayHelper: relayToChannel(ctx, relayInfo)
alt RelayFormat == Gemini
RelayHelper->>GeminiRelay: send request (uses SystemInstruction)
GeminiRelay-->>RelayHelper: stream responses (possibly empty)
else Other formats
RelayHelper->>RelayHelper: use appropriate helper (OpenAI/Claude)
end
RelayHelper-->>Controller: response or error
alt error
Controller->>ProcessError: processChannelError(err)
ProcessError-->>Controller: disable/next channel decision
Controller->>Controller: iterate next key/channel or return
else success
Controller-->>Client: return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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
🧹 Nitpick comments (3)
service/convert.go (1)
598-606: Avoid injecting empty system messages; use SetStringContent for consistencyIf SystemInstruction has no textual parts (or only whitespace), we currently prepend an empty system message. Trim and short-circuit. Also use SetStringContent to match the rest of the conversion helpers.
Apply this diff:
- if geminiRequest.SystemInstruction != nil { - // 将系统指令作为第一条消息插入 - systemMessage := dto.Message{ - Role: "system", - Content: extractTextFromGeminiParts(geminiRequest.SystemInstruction.Parts), - } - openaiRequest.Messages = append([]dto.Message{systemMessage}, openaiRequest.Messages...) - } + if geminiRequest.SystemInstruction != nil { + // 将系统指令作为第一条消息插入（忽略空白内容） + content := strings.TrimSpace(extractTextFromGeminiParts(geminiRequest.SystemInstruction.Parts)) + if content != "" { + systemMessage := dto.Message{Role: "system"} + systemMessage.SetStringContent(content) + openaiRequest.Messages = append([]dto.Message{systemMessage}, openaiRequest.Messages...) + } + }relay/gemini_handler.go (1)
93-103: Cleanup logic may drop non-text system instructions; also trim whitespaceCurrent check only looks at non-empty Text and will null out instructions containing other content types (e.g., InlineData), and won’t ignore whitespace. Tighten the check.
Apply this diff:
- if request.SystemInstruction != nil { - hasContent := false - for _, part := range request.SystemInstruction.Parts { - if part.Text != "" { - hasContent = true - break - } - } - if !hasContent { - request.SystemInstruction = nil - } - } + if request.SystemInstruction != nil { + hasContent := false + for _, part := range request.SystemInstruction.Parts { + if strings.TrimSpace(part.Text) != "" || part.InlineData != nil || part.FileData != nil || part.FunctionCall != nil || part.FunctionResponse != nil { + hasContent = true + break + } + } + if !hasContent { + request.SystemInstruction = nil + } + }relay/channel/gemini/relay-gemini.go (1)
464-472: Guard against empty/whitespace-only system prompts before setting SystemInstructionMinor nit: avoid creating SystemInstruction when concatenated system content is empty after trimming.
Apply this diff:
- if len(system_content) > 0 { - geminiRequest.SystemInstruction = &dto.GeminiChatContent{ - Parts: []dto.GeminiPart{ - { - Text: strings.Join(system_content, "\n"), - }, - }, - } - } + if len(system_content) > 0 { + joined := strings.TrimSpace(strings.Join(system_content, "\n")) + if joined != "" { + geminiRequest.SystemInstruction = &dto.GeminiChatContent{ + Parts: []dto.GeminiPart{ + { Text: joined }, + }, + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
dto/gemini.go(1 hunks)relay/channel/gemini/relay-gemini.go(1 hunks)relay/gemini_handler.go(1 hunks)service/convert.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T17:12:43.157Z
Learnt from: RedwindA
PR: QuantumNous/new-api#1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
Applied to files:
relay/channel/gemini/relay-gemini.go
🧬 Code Graph Analysis (2)
service/convert.go (1)
dto/openai_request.go (1)
Message(247-258)
relay/channel/gemini/relay-gemini.go (1)
dto/gemini.go (1)
GeminiChatContent(212-215)
| Tools json.RawMessage `json:"tools,omitempty"` | ||
| SystemInstruction *GeminiChatContent `json:"system_instruction,omitempty"` | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename aligns with Gemini style, but JSON compatibility and token accounting need attention
- Backward-compat risk: switching the JSON tag to "system_instruction" will break clients still sending "systemInstruction". We should accept both to avoid regressions.
- Token accounting: GetTokenCountMeta ignores SystemInstruction.Parts, so system prompts won’t be counted toward prompt meta, potentially skewing prompt aggregation/usage.
Proposed implementation to accept both snake_case and camelCase without changing your external JSON shape:
// In dto/gemini.go (add below the type definition)
func (r *GeminiChatRequest) UnmarshalJSON(data []byte) error {
type Alias GeminiChatRequest
aux := struct {
Alias
SystemInstructionSnake *GeminiChatContent `json:"system_instruction,omitempty"`
SystemInstructionCamel *GeminiChatContent `json:"systemInstruction,omitempty"`
}{}
if err := common.Unmarshal(data, &aux); err != nil {
return err
}
*r = GeminiChatRequest(aux.Alias)
// prefer snake_case, fallback to camelCase
if aux.SystemInstructionSnake != nil {
r.SystemInstruction = aux.SystemInstructionSnake
} else if aux.SystemInstructionCamel != nil {
r.SystemInstruction = aux.SystemInstructionCamel
}
return nil
}Also, extend GetTokenCountMeta to include system instructions:
// inside GetTokenCountMeta(), after iterating r.Contents
if r.SystemInstruction != nil {
for _, part := range r.SystemInstruction.Parts {
if part.Text != "" {
inputTexts = append(inputTexts, part.Text)
}
if part.InlineData != nil && part.InlineData.Data != "" {
if strings.HasPrefix(part.InlineData.MimeType, "image/") {
files = append(files, &types.FileMeta{FileType: types.FileTypeImage, OriginData: part.InlineData.Data})
} else if strings.HasPrefix(part.InlineData.MimeType, "audio/") {
files = append(files, &types.FileMeta{FileType: types.FileTypeAudio, OriginData: part.InlineData.Data})
} else if strings.HasPrefix(part.InlineData.MimeType, "video/") {
files = append(files, &types.FileMeta{FileType: types.FileTypeVideo, OriginData: part.InlineData.Data})
} else {
files = append(files, &types.FileMeta{FileType: types.FileTypeFile, OriginData: part.InlineData.Data})
}
}
}
}Run this to verify no stale references remain:
🏁 Script executed:
#!/bin/bash
# Search for old field/usages that may have been missed
rg -n -C2 -S '\bSystemInstructions\b|\bsystemInstruction\b|\bsystemInstructions\b'Length of output: 84
🏁 Script executed:
#!/bin/bash
# Verify occurrences of both snake_case and camelCase variants
rg -n -C2 'systemInstruction'
rg -n -C2 'SystemInstruction'
# Locate GetTokenCountMeta to inspect if it already accounts for system instructions
rg -n -C2 'GetTokenCountMeta'Length of output: 4842
🏁 Script executed:
#!/bin/bash
# Check if UnmarshalJSON is already implemented for GeminiChatRequest
rg -n -C2 'func .*GeminiChatRequest.*UnmarshalJSON' dto/gemini.go
# Inspect the GetTokenCountMeta implementation for GeminiChatRequest
rg -n -C10 'func (r \*GeminiChatRequest\) GetTokenCountMeta' -n dto/gemini.goLength of output: 251
🏁 Script executed:
#!/bin/bash
# Check for existing UnmarshalJSON implementation on GeminiChatRequest
rg -n 'func .*GeminiChatRequest.*UnmarshalJSON' dto/gemini.go || echo "No UnmarshalJSON method found"
# Display the type definition and GetTokenCountMeta implementation for GeminiChatRequest
sed -n '1,80p' dto/gemini.goLength of output: 2435
Support legacy systemInstruction and include system instructions in token accounting
To avoid breaking existing clients and ensure system prompts are counted:
• In dto/gemini.go, add an UnmarshalJSON on GeminiChatRequest that reads both
– system_instruction
– systemInstruction
and sets the single SystemInstruction field (prefer snake_case).
• In the same file’s GetTokenCountMeta, after iterating r.Contents, also append every r.SystemInstruction.Parts (text and inline data) to inputTexts and files so system prompts contribute to the returned TokenCountMeta.
Locations to update:
- dto/gemini.go: around the type definition, implement UnmarshalJSON as proposed.
- dto/gemini.go: inside GetTokenCountMeta, inject the loop over r.SystemInstruction.Parts.
--- a/dto/gemini.go
+++ b/dto/gemini.go
@@ type GeminiChatRequest struct {
SystemInstruction *GeminiChatContent `json:"system_instruction,omitempty"`
}
+// UnmarshalJSON accepts both snake_case and camelCase for SystemInstruction
+func (r *GeminiChatRequest) UnmarshalJSON(data []byte) error {
+ type Alias GeminiChatRequest
+ aux := struct {
+ Alias
+ SystemInstructionSnake *GeminiChatContent `json:"system_instruction,omitempty"`
+ SystemInstructionCamel *GeminiChatContent `json:"systemInstruction,omitempty"`
+ }{}
+
+ if err := common.Unmarshal(data, &aux); err != nil {
+ return err
+ }
+ *r = GeminiChatRequest(aux.Alias)
+ if aux.SystemInstructionSnake != nil {
+ r.SystemInstruction = aux.SystemInstructionSnake
+ } else if aux.SystemInstructionCamel != nil {
+ r.SystemInstruction = aux.SystemInstructionCamel
+ }
+ return nil
+}
@@ func (r *GeminiChatRequest) GetTokenCountMeta() *types.TokenCountMeta {
inputText := strings.Join(inputTexts, "\n")
+ // include system instruction parts in token count
+ if r.SystemInstruction != nil {
+ for _, part := range r.SystemInstruction.Parts {
+ if part.Text != "" {
+ inputTexts = append(inputTexts, part.Text)
+ }
+ if part.InlineData != nil && part.InlineData.Data != "" {
+ mt := part.InlineData.MimeType
+ var t types.FileType
+ switch {
+ case strings.HasPrefix(mt, "image/"):
+ t = types.FileTypeImage
+ case strings.HasPrefix(mt, "audio/"):
+ t = types.FileTypeAudio
+ case strings.HasPrefix(mt, "video/"):
+ t = types.FileTypeVideo
+ default:
+ t = types.FileTypeFile
+ }
+ files = append(files, &types.FileMeta{FileType: t, OriginData: part.InlineData.Data})
+ }
+ }
+ inputText = strings.Join(inputTexts, "\n")
+ }
return &types.TokenCountMeta{
CombineText: inputText,
Files: files,
MaxTokens: maxTokens,
}
}📝 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.
| Tools json.RawMessage `json:"tools,omitempty"` | |
| SystemInstruction *GeminiChatContent `json:"system_instruction,omitempty"` | |
| } | |
| // --- dto/gemini.go | |
| type GeminiChatRequest struct { | |
| Tools json.RawMessage `json:"tools,omitempty"` | |
| SystemInstruction *GeminiChatContent `json:"system_instruction,omitempty"` | |
| } | |
| // UnmarshalJSON accepts both snake_case and camelCase for SystemInstruction | |
| func (r *GeminiChatRequest) UnmarshalJSON(data []byte) error { | |
| type Alias GeminiChatRequest | |
| aux := struct { | |
| Alias | |
| SystemInstructionSnake *GeminiChatContent `json:"system_instruction,omitempty"` | |
| SystemInstructionCamel *GeminiChatContent `json:"systemInstruction,omitempty"` | |
| }{} | |
| if err := common.Unmarshal(data, &aux); err != nil { | |
| return err | |
| } | |
| *r = GeminiChatRequest(aux.Alias) | |
| if aux.SystemInstructionSnake != nil { | |
| r.SystemInstruction = aux.SystemInstructionSnake | |
| } else if aux.SystemInstructionCamel != nil { | |
| r.SystemInstruction = aux.SystemInstructionCamel | |
| } | |
| return nil | |
| } | |
| func (r *GeminiChatRequest) GetTokenCountMeta() *types.TokenCountMeta { | |
| // existing setup... | |
| inputTexts := []string{} | |
| for _, c := range r.Contents { | |
| inputTexts = append(inputTexts, c.PartsText()...) | |
| } | |
| files := []*types.FileMeta{} | |
| // ... | |
| inputText := strings.Join(inputTexts, "\n") | |
| // include system instruction parts in token count | |
| if r.SystemInstruction != nil { | |
| for _, part := range r.SystemInstruction.Parts { | |
| if part.Text != "" { | |
| inputTexts = append(inputTexts, part.Text) | |
| } | |
| if part.InlineData != nil && part.InlineData.Data != "" { | |
| mt := part.InlineData.MimeType | |
| var t types.FileType | |
| switch { | |
| case strings.HasPrefix(mt, "image/"): | |
| t = types.FileTypeImage | |
| case strings.HasPrefix(mt, "audio/"): | |
| t = types.FileTypeAudio | |
| case strings.HasPrefix(mt, "video/"): | |
| t = types.FileTypeVideo | |
| default: | |
| t = types.FileTypeFile | |
| } | |
| files = append(files, &types.FileMeta{FileType: t, OriginData: part.InlineData.Data}) | |
| } | |
| } | |
| inputText = strings.Join(inputTexts, "\n") | |
| } | |
| return &types.TokenCountMeta{ | |
| CombineText: inputText, | |
| Files: files, | |
| MaxTokens: maxTokens, | |
| } | |
| } |
🤖 Prompt for AI Agents
In dto/gemini.go around the GeminiChatRequest type definition (near lines 16-18)
implement UnmarshalJSON that accepts both "system_instruction" and legacy
"systemInstruction", unmarshals either key into the single SystemInstruction
field (prefer "system_instruction" when both present) and preserves other
fields; then in GetTokenCountMeta (same file) after iterating r.Contents add
logic to, if r.SystemInstruction != nil, iterate r.SystemInstruction.Parts and
append each part's text to inputTexts and any inline data references to files so
system prompts are included in token accounting; ensure you handle nils and
empty parts safely.
|
Gemini兼容两种字段命名方式，请不要破坏兼容性 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware/distributor.go (2)
239-263: Handle error from SetupContextForSelectedChannel; currently ignored and request proceeds with invalid context.If GetNextEnabledKey fails, the middleware should abort with an error.
- SetupContextForSelectedChannel(c, channel, modelRequest.Model) + if apiErr := SetupContextForSelectedChannel(c, channel, modelRequest.Model); apiErr != nil { + abortWithOpenAiMessage(c, http.StatusServiceUnavailable, apiErr.Error(), string(apiErr.Code())) + return + }Also applies to: 120-121
181-188: Restrict Gemini route to RPC-style paths
Require a “:” in the URL before treating any/v1beta/models/or/v1/models/request as Gemini to avoid hijacking standard model list or info endpoints:-} else if strings.HasPrefix(c.Request.URL.Path, "/v1beta/models/") || strings.HasPrefix(c.Request.URL.Path, "/v1/models/") { +} else if (strings.HasPrefix(c.Request.URL.Path, "/v1beta/models/") || strings.HasPrefix(c.Request.URL.Path, "/v1/models/")) && + strings.Contains(c.Request.URL.Path, ":") { // Gemini RPC-style endpoints like …:generateContent relayMode := relayconstant.RelayModeGemini modelName := extractModelNameFromGeminiPath(c.Request.URL.Path) if modelName != "" { modelRequest.Model = modelName } c.Set("relay_mode", relayMode)
🧹 Nitpick comments (8)
middleware/distributor.go (1)
265-299: Minor: always set ChannelIsMultiKey explicitly.Good that false is set for single-key to keep logs consistent.
service/channel.go (6)
12-38: Disable-decision heuristics are brittle; prefer typed signals over substrings.The substring checks (timeout/connect/provider/internal/no response) are easy to drift and may misclassify errors. Where possible, rely on
err.StatusCode,err.GetErrorType()(e.g., transient vs. quota), and/or a small, centralized helper that normalizes upstreams to internal error types.Apply this refactor locally or introduce a shared helper:
- errMsg := strings.ToLower(err.Error()) - if strings.Contains(errMsg, "deadline exceeded") || strings.Contains(errMsg, "timeout") || strings.Contains(errMsg, "connect") || strings.Contains(errMsg, "do request failed") || strings.Contains(errMsg, "provider returned error") || strings.Contains(errMsg, "internal server error") || strings.Contains(errMsg, "no response received") { + if types.IsTransientNetworkError(err) { // centralize mapping per provider return false }
49-70: Make ticker stoppable and reduce log churn.Consider accepting a
context.Contextto stop the goroutine cleanly on shutdown and avoid orphaned tickers. Also, daily-modelastEnableAtreset runs every minute; harmless but noisy. Optional simplification: reset only when the hour changes.Example:
-func CheckAndReEnableChannels() { - ticker := time.NewTicker(1 * time.Minute) - defer ticker.Stop() +func CheckAndReEnableChannels(ctx context.Context) { + ticker := time.NewTicker(time.Minute) + defer ticker.Stop() lastEnableAt := -1 - for range ticker.C { + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: // existing logic... + } + } }
118-121: Interval check logs every minute; gate behind debug.This line will spam logs in steady state.
- common.SysLog("checking for channels to re-enable by interval") + if common.DebugEnabled { + common.SysLog("checking for channels/keys to re-enable by interval") + }
159-168: Consistency: when re-enabling a key and channel is auto-disabled, ensure channel enable happens first and check errors.To avoid a brief mismatch (key enabled while channel remains disabled), enable the channel first and handle error; then enable the key.
- if channel.Status == common.ChannelStatusAutoDisabled { - // Also re-enable the channel itself - model.UpdateChannelStatus(channel.Id, "", common.ChannelStatusEnabled, "re-enabled by system") - } + if channel.Status == common.ChannelStatusAutoDisabled { + _ = model.UpdateChannelStatus(channel.Id, "", common.ChannelStatusEnabled, "re-enabled by system") + }(Apply same ordering in daily task.)
Also applies to: 109-113
54-65: Edge: daily scheduler hour guard can trigger twice around DST/clock jumps.If system clock adjusts or restarts mid-hour, we may double-run. If that matters, persist last-run timestamp in memory or KV to dedupe.
126-141: Type assert on status_time assumes JSON number→float64; add int fallback.If
status_timewas set programmatically, it may beint/int64.- if disabledTime, ok := otherInfo["status_time"].(float64); ok { + switch v := otherInfo["status_time"].(type) { + case float64: + disabledTime = int64(v) + case int64: + disabledTime = v + case int: + disabledTime = int64(v) + default: + continue + }controller/relay.go (1)
202-209: Minor: avoid re-fetching same key within the same j-iteration.Current flow fetches a new key then
continue, costing one empty iteration. You can process the new key immediately.- if _, ok := triedKeys[keyIdx]; ok { - // This key has been tried, get another one - key, keyIdx, keyErr = channel.GetNextEnabledKey() - if keyErr != nil { - newAPIError = keyErr - break - } - continue - } + if _, ok := triedKeys[keyIdx]; ok { + key, keyIdx, keyErr = channel.GetNextEnabledKey() + if keyErr != nil { + newAPIError = keyErr + goto next_channel + } + }Also applies to: 211-222
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
common/constants.go(1 hunks)controller/channel-test.go(1 hunks)controller/relay.go(5 hunks)main.go(1 hunks)middleware/distributor.go(1 hunks)model/channel.go(1 hunks)service/channel.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
model/channel.go (2)
model/main.go (1)
DB(63-63)common/constants.go (2)
ChannelStatusAutoDisabled(198-198)ChannelStatusManuallyDisabled(197-197)
main.go (3)
common/constants.go (2)
EnableAutodisabledChannelOrKeyAfterMinute(109-109)EnableAutodisabledChannelOrKeyAt(110-110)common/sys_log.go (1)
SysLog(10-13)service/channel.go (1)
CheckAndReEnableChannels(49-70)
middleware/distributor.go (2)
model/channel.go (1)
Channel(20-54)types/error.go (1)
NewAPIError(81-89)
service/channel.go (5)
types/error.go (1)
NewAPIError(81-89)common/constants.go (5)
AutomaticDisableChannelEnabled(102-102)ChannelStatusAutoDisabled(198-198)EnableAutodisabledChannelOrKeyAt(110-110)EnableAutodisabledChannelOrKeyAfterMinute(109-109)ChannelStatusEnabled(196-196)types/channel_error.go (1)
ChannelError(3-10)model/channel.go (4)
UpdateChannelStatus(593-654)GetAutoDisabledChannels(967-971)GetChannelsWithDisabledKeys(973-978)ChannelInfo(56-64)common/sys_log.go (2)
SysLog(10-13)SysError(15-18)
controller/relay.go (11)
relay/common/relay_info.go (1)
RelayInfo(73-118)model/channel.go (2)
Channel(20-54)ChannelInfo(56-64)types/error.go (1)
NewAPIError(81-89)common/gin.go (1)
GetRequestBody(15-27)types/relay_format.go (4)
RelayFormat(3-3)RelayFormatOpenAIRealtime(12-12)RelayFormatClaude(7-7)RelayFormatGemini(8-8)relay/websocket.go (1)
WssHelper(14-45)relay/claude_handler.go (1)
ClaudeHelper(20-126)types/channel_error.go (1)
NewChannelError(12-21)middleware/distributor.go (1)
SetupContextForSelectedChannelWithKey(265-299)common/constants.go (1)
RetryTimes(107-107)service/channel.go (1)
DisableChannel(40-47)
🔇 Additional comments (6)
controller/channel-test.go (1)
452-454: OK to remove inline re-enable; ensure scheduler covers the use case.Confirm that daily/interval re-enable (service.CheckAndReEnableChannels) now replaces this path in ops playbooks and docs.
middleware/distributor.go (1)
262-263: Good extraction: per-key setup is now reusable.service/channel.go (1)
40-47: LGTM: disable path writes status via model and logs outcome.controller/relay.go (3)
62-82: LGTM: central router resets body and reports channel errors uniformly.Good consolidation; request body reset prevents EOF on retries; error reporting flows through a single path.
285-291: LGTM: avoid futile retries on "no response received".Prevents wasting attempts on upstream silent failures.
333-342: Prefixing quota reason is correct; ensure re-enable readers checkstatus_reason.Ties to service fix; otherwise, quota-disabled channels/keys may be re-enabled.
Would you like me to add an integration test asserting a channel disabled with
err.GetErrorType()=="insufficient_quota"is not re-enabled by either daily or interval tasks?
| var EnableAutodisabledChannelOrKeyAfterMinute = 0 | ||
| var EnableAutodisabledChannelOrKeyAt = -1 // UTC hour | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and document scheduler knobs (UTC hour range; non-negative minutes).
Guard against misconfiguration: ensure 0 <= EnableAutodisabledChannelOrKeyAt <= 23 and EnableAutodisabledChannelOrKeyAfterMinute >= 0 on read; log and ignore invalid values.
Apply in main.go where these are read:
- common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0)
- common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1)
+ common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0)
+ common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1)
+ if common.EnableAutodisabledChannelOrKeyAt != -1 &&
+ (common.EnableAutodisabledChannelOrKeyAt < 0 || common.EnableAutodisabledChannelOrKeyAt > 23) {
+ common.SysLog(fmt.Sprintf("invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT=%d, disabling fixed-time schedule",
+ common.EnableAutodisabledChannelOrKeyAt))
+ common.EnableAutodisabledChannelOrKeyAt = -1
+ }
+ if common.EnableAutodisabledChannelOrKeyAfterMinute < 0 {
+ common.SysLog(fmt.Sprintf("invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE=%d, forcing to 0",
+ common.EnableAutodisabledChannelOrKeyAfterMinute))
+ common.EnableAutodisabledChannelOrKeyAfterMinute = 0
+ }📝 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.
| var EnableAutodisabledChannelOrKeyAfterMinute = 0 | |
| var EnableAutodisabledChannelOrKeyAt = -1 // UTC hour | |
| // … earlier in main.go … | |
| // Read configuration (with defaults) | |
| common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0) | |
| common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1) | |
| // Validate EnableAutodisabledChannelOrKeyAt: | |
| // -1 means “disabled”; otherwise must be within 0–23 (UTC hour) | |
| if common.EnableAutodisabledChannelOrKeyAt != -1 && | |
| (common.EnableAutodisabledChannelOrKeyAt < 0 || common.EnableAutodisabledChannelOrKeyAt > 23) { | |
| common.SysLog(fmt.Sprintf( | |
| "invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT=%d, disabling fixed-time schedule", | |
| common.EnableAutodisabledChannelOrKeyAt, | |
| )) | |
| common.EnableAutodisabledChannelOrKeyAt = -1 | |
| } | |
| // Validate EnableAutodisabledChannelOrKeyAfterMinute: | |
| // must be non-negative (minutes after startup) | |
| if common.EnableAutodisabledChannelOrKeyAfterMinute < 0 { | |
| common.SysLog(fmt.Sprintf( | |
| "invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE=%d, forcing to 0", | |
| common.EnableAutodisabledChannelOrKeyAfterMinute, | |
| )) | |
| common.EnableAutodisabledChannelOrKeyAfterMinute = 0 | |
| } | |
| // … rest of initialization … |
🤖 Prompt for AI Agents
In common/constants.go around lines 109 to 111, the scheduler knobs lack
validation and documentation: ensure when reading these values in main.go you
validate that EnableAutodisabledChannelOrKeyAt is within 0..23 and
EnableAutodisabledChannelOrKeyAfterMinute is >= 0; if either is invalid, log a
clear warning including the invalid value and ignore it (use the safe default
from constants.go), and proceed without applying the misconfigured setting. Add
a brief comment in common/constants.go documenting the expected ranges (UTC hour
0-23, non-negative minutes) so future readers know the constraints.
| common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0) | ||
| common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1) | ||
| if common.EnableAutodisabledChannelOrKeyAt != -1 { | ||
| common.SysLog(fmt.Sprintf("auto-disabled channels/keys will be re-enabled at %d:00 UTC daily", common.EnableAutodisabledChannelOrKeyAt)) | ||
| go service.CheckAndReEnableChannels() | ||
| } else if common.EnableAutodisabledChannelOrKeyAfterMinute > 0 { | ||
| common.SysLog(fmt.Sprintf("auto-disabled channels/keys will be re-enabled after %d minutes", common.EnableAutodisabledChannelOrKeyAfterMinute)) | ||
| go service.CheckAndReEnableChannels() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate env values and log precedence.
Clamp hour to [0,23], minutes to >=0, and log that “AT” takes precedence over “AFTER_MINUTE” to avoid confusion.
- common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0)
- common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1)
+ common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0)
+ common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1)
+ if common.EnableAutodisabledChannelOrKeyAt != -1 &&
+ (common.EnableAutodisabledChannelOrKeyAt < 0 || common.EnableAutodisabledChannelOrKeyAt > 23) {
+ common.SysLog(fmt.Sprintf("invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT=%d; disabling fixed-time schedule",
+ common.EnableAutodisabledChannelOrKeyAt))
+ common.EnableAutodisabledChannelOrKeyAt = -1
+ }
+ if common.EnableAutodisabledChannelOrKeyAfterMinute < 0 {
+ common.SysLog(fmt.Sprintf("invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE=%d; forcing 0",
+ common.EnableAutodisabledChannelOrKeyAfterMinute))
+ common.EnableAutodisabledChannelOrKeyAfterMinute = 0
+ }
+ if common.EnableAutodisabledChannelOrKeyAt != -1 && common.EnableAutodisabledChannelOrKeyAfterMinute > 0 {
+ common.SysLog("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT takes precedence over ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE")
+ }📝 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.
| common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0) | |
| common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1) | |
| if common.EnableAutodisabledChannelOrKeyAt != -1 { | |
| common.SysLog(fmt.Sprintf("auto-disabled channels/keys will be re-enabled at %d:00 UTC daily", common.EnableAutodisabledChannelOrKeyAt)) | |
| go service.CheckAndReEnableChannels() | |
| } else if common.EnableAutodisabledChannelOrKeyAfterMinute > 0 { | |
| common.SysLog(fmt.Sprintf("auto-disabled channels/keys will be re-enabled after %d minutes", common.EnableAutodisabledChannelOrKeyAfterMinute)) | |
| go service.CheckAndReEnableChannels() | |
| } | |
| common.EnableAutodisabledChannelOrKeyAfterMinute = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", 0) | |
| common.EnableAutodisabledChannelOrKeyAt = common.GetEnvOrDefault("ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT", -1) | |
| // Clamp hour to [0,23]; if out of range, disable fixed-time schedule | |
| if common.EnableAutodisabledChannelOrKeyAt != -1 && (common.EnableAutodisabledChannelOrKeyAt < 0 || common.EnableAutodisabledChannelOrKeyAt > 23) { | |
| common.SysLog(fmt.Sprintf( | |
| "invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT=%d; disabling fixed-time schedule", | |
| common.EnableAutodisabledChannelOrKeyAt, | |
| )) | |
| common.EnableAutodisabledChannelOrKeyAt = -1 | |
| } | |
| // Clamp minutes to >=0 | |
| if common.EnableAutodisabledChannelOrKeyAfterMinute < 0 { | |
| common.SysLog(fmt.Sprintf( | |
| "invalid ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE=%d; forcing 0", | |
| common.EnableAutodisabledChannelOrKeyAfterMinute, | |
| )) | |
| common.EnableAutodisabledChannelOrKeyAfterMinute = 0 | |
| } | |
| // If both are set, AT takes precedence over AFTER_MINUTE | |
| if common.EnableAutodisabledChannelOrKeyAt != -1 && common.EnableAutodisabledChannelOrKeyAfterMinute > 0 { | |
| common.SysLog( | |
| "ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT takes precedence over ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE", | |
| ) | |
| } | |
| // Original scheduling logic | |
| if common.EnableAutodisabledChannelOrKeyAt != -1 { | |
| common.SysLog(fmt.Sprintf( | |
| "auto-disabled channels/keys will be re-enabled at %d:00 UTC daily", | |
| common.EnableAutodisabledChannelOrKeyAt, | |
| )) | |
| go service.CheckAndReEnableChannels() | |
| } else if common.EnableAutodisabledChannelOrKeyAfterMinute > 0 { | |
| common.SysLog(fmt.Sprintf( | |
| "auto-disabled channels/keys will be re-enabled after %d minutes", | |
| common.EnableAutodisabledChannelOrKeyAfterMinute, | |
| )) | |
| go service.CheckAndReEnableChannels() | |
| } |
🤖 Prompt for AI Agents
In main.go around lines 118 to 127, validate and clamp the environment inputs
and make precedence explicit: parse ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT into
an int and clamp it to the range 0..23 (if parse fails treat as -1), parse
ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTE into an int and coerce to a
minimum of 0 (if parse fails treat as 0), then if AT != -1 log that "AT takes
precedence over AFTER_MINUTE" when both are provided and use the clamped AT
value to schedule re-enable; otherwise if AFTER_MINUTE > 0 log the minutes and
use that value; ensure you only launch the goroutine once after choosing the
validated value and include the validated/clamped values in the log messages for
clarity.
| func GetChannelsWithDisabledKeys() ([]*Channel, error) { | ||
| var channels []*Channel | ||
| // Find channels that are not manually disabled and have disabled keys | ||
| err := DB.Where("status != ? AND JSON_LENGTH(channel_info -> '$.multi_key_status_list') > 0", common.ChannelStatusManuallyDisabled).Find(&channels).Error | ||
| return channels, err | ||
| } |
There was a problem hiding this comment.
MySQL-only JSON expression breaks on PostgreSQL/SQLite; add dialect-specific query.
Current JSON_LENGTH(channel_info -> '$.multi_key_status_list') is MySQL-specific and will fail elsewhere.
-func GetChannelsWithDisabledKeys() ([]*Channel, error) {
- var channels []*Channel
- // Find channels that are not manually disabled and have disabled keys
- err := DB.Where("status != ? AND JSON_LENGTH(channel_info -> '$.multi_key_status_list') > 0", common.ChannelStatusManuallyDisabled).Find(&channels).Error
- return channels, err
-}
+func GetChannelsWithDisabledKeys() ([]*Channel, error) {
+ var channels []*Channel
+ q := DB.Omit("key").Where("status != ?", common.ChannelStatusManuallyDisabled)
+ if common.UsingMySQL {
+ // object length > 0
+ q = q.Where("JSON_LENGTH(JSON_EXTRACT(channel_info, '$.multi_key_status_list')) > 0")
+ } else if common.UsingPostgreSQL {
+ // channel_info is json/jsonb; cast to jsonb then object length
+ q = q.Where(`jsonb_object_length((channel_info->'multi_key_status_list')::jsonb) > 0`)
+ } else {
+ // SQLite JSON1 fallback: '{}' has length 2; any non-empty object > 2
+ q = q.Where(`json_extract(channel_info, '$.multi_key_status_list') IS NOT NULL AND length(json_extract(channel_info, '$.multi_key_status_list')) > 2`)
+ }
+ err := q.Find(&channels).Error
+ return channels, err
+}Additionally, consider filtering to multi-key channels only (channel_info->is_multi_key), and adding an index (PostgreSQL: expression GIN on (channel_info)) for performance.
Committable suggestion skipped: line range outside the PR's diff.
| func reEnableAllAutoDisabledChannels() { | ||
| channels, err := model.GetAutoDisabledChannels() | ||
| if err != nil { | ||
| common.SysError("failed to get auto-disabled channels for daily re-enabling: " + err.Error()) | ||
| return | ||
| } | ||
| if newAPIError != nil { | ||
| return false | ||
| for _, channel := range channels { | ||
| otherInfo := channel.GetOtherInfo() | ||
| if reason, ok := otherInfo["disable_reason"].(string); ok { | ||
| if strings.HasPrefix(reason, "insufficient_quota:") { | ||
| continue | ||
| } | ||
| } | ||
| success := model.UpdateChannelStatus(channel.Id, "", common.ChannelStatusEnabled, "re-enabled by daily task") | ||
| if success { | ||
| common.SysLog(fmt.Sprintf("channel #%d (%s) re-enabled by daily task", channel.Id, channel.Name)) | ||
| } | ||
| } | ||
| if status != common.ChannelStatusAutoDisabled { | ||
| return false | ||
|
|
||
| multiKeyChannels, err := model.GetChannelsWithDisabledKeys() | ||
| if err != nil { | ||
| common.SysError("failed to get channels with disabled keys for daily re-enabling: " + err.Error()) | ||
| return | ||
| } | ||
| for _, channel := range multiKeyChannels { | ||
| keys := channel.GetKeys() | ||
| for keyIdx, reason := range channel.ChannelInfo.MultiKeyDisabledReason { | ||
| if strings.HasPrefix(reason, "insufficient_quota:") { | ||
| continue | ||
| } | ||
| if keyIdx >= len(keys) { | ||
| continue | ||
| } | ||
| keyToReEnable := keys[keyIdx] | ||
| success := model.UpdateChannelStatus(channel.Id, keyToReEnable, common.ChannelStatusEnabled, "re-enabled by daily task") | ||
| if success { | ||
| common.SysLog(fmt.Sprintf("key #%d of channel #%d (%s) re-enabled by daily task", keyIdx, channel.Id, channel.Name)) | ||
| if channel.Status == common.ChannelStatusAutoDisabled { | ||
| // Also re-enable the channel itself | ||
| model.UpdateChannelStatus(channel.Id, "", common.ChannelStatusEnabled, "re-enabled by daily task") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verification script: confirm no other readers depend on "disable_reason".
Scan repo to ensure all re-enable/disable readers use status_reason consistently.
Also applies to: 118-172
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\b(disable_reason|status_reason)\b' --glob '!*vendor*'
rg -nP 'UpdateChannelStatus\(' -C3Length of output: 6450
Replace disable_reason with status_reason in re-enable logic
The loops at service/channel.go (lines 80 and 128) check otherInfo["disable_reason"], but all disables record their reason under status_reason. Update both checks to use otherInfo["status_reason"] so the "insufficient_quota:" skip actually takes effect.
🤖 Prompt for AI Agents
In service/channel.go around lines 72 to 116, the re-enable logic reads
otherInfo["disable_reason"] but disables record their reason under
"status_reason"; update both checks in this function to read
otherInfo["status_reason"] (cast to string as before) so the
"insufficient_quota:" skip works correctly, leaving the rest of the logic and
logging unchanged.
| otherInfo := channel.GetOtherInfo() | ||
| if reason, ok := otherInfo["disable_reason"].(string); ok { | ||
| if strings.HasPrefix(reason, "insufficient_quota:") { | ||
| continue | ||
| } | ||
| } | ||
| success := model.UpdateChannelStatus(channel.Id, "", common.ChannelStatusEnabled, "re-enabled by daily task") | ||
| if success { |
There was a problem hiding this comment.
Bug: re-enable skip uses key "disable_reason" but status writer stores "status_reason". This defeats the insufficient_quota guard.
model.UpdateChannelStatus writes otherInfo["status_reason"], but here we read disable_reason. Channels/keys disabled for quota will be re-enabled incorrectly.
Patch both places to read status_reason (fallback to disable_reason for backward compatibility):
- if reason, ok := otherInfo["disable_reason"].(string); ok {
- if strings.HasPrefix(reason, "insufficient_quota:") {
- continue
- }
- }
+ if reason, ok := otherInfo["status_reason"].(string); ok {
+ if strings.HasPrefix(reason, "insufficient_quota:") {
+ continue
+ }
+ } else if reason, ok := otherInfo["disable_reason"].(string); ok { // backward compat
+ if strings.HasPrefix(reason, "insufficient_quota:") {
+ continue
+ }
+ }And in interval mode:
- if reason, ok := otherInfo["disable_reason"].(string); ok {
+ if reason, ok := otherInfo["status_reason"].(string); ok {
if strings.HasPrefix(reason, "insufficient_quota:") {
continue
}
+ } else if reason, ok := otherInfo["disable_reason"].(string); ok { // backward compat
+ if strings.HasPrefix(reason, "insufficient_quota:") {
+ continue
+ }
}Also applies to: 128-135
🤖 Prompt for AI Agents
In service/channel.go around lines 79-86 and also apply same fix at lines
128-135, the code checks otherInfo["disable_reason"] but
model.UpdateChannelStatus writes otherInfo["status_reason"], so the
insufficient_quota guard is bypassed; change the lookup to read
otherInfo["status_reason"] first and if not present fall back to
otherInfo["disable_reason"] (preserving string assertion and the
strings.HasPrefix check), so both places use status_reason primarily with
backward-compatible fallback to disable_reason.
- What: 在充值卡显示的实付与节省金额前加入美元符号 `$`。 - Why: 满足 issue QuantumNous#1881，要求在金额前标注货币单位以减少歧义。 - Files: src/components/topup/RechargeCard.jsx - Note: 这是局部修复。建议后续实现统一的 currency formatter（Intl.NumberFormat）并从后端/配置读取货币代码以支持本地化与多币种。 Closes QuantumNous#1881
main -> alpha
fix(topup): add currency symbol to amounts in RechargeCard
Revert "fix(topup): add currency symbol to amounts in RechargeCard"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
model/channel.go (2)
970-975: Previously flagged: Avoid loading secrets unnecessarily.This issue was already raised in a previous review. The query loads the full Channel struct including the
keyfield (which contains secrets). UseOmit("key")to prevent loading sensitive data into memory.
977-982: Previously flagged: MySQL-only JSON expression breaks portability.This issue was already raised in a previous review. The
JSON_LENGTH(channel_info -> '$.multi_key_status_list')syntax is MySQL-specific and will fail on PostgreSQL and SQLite. A dialect-specific query solution was provided in the earlier review.service/channel.go (2)
80-84: Previously flagged: Bug in key name for disable reason check.This issue was already raised in a previous review. The code checks
otherInfo["disable_reason"]butmodel.UpdateChannelStatuswrites tootherInfo["status_reason"]. This causes theinsufficient_quota:skip logic to fail. The previous review recommended checkingstatus_reasonfirst with a fallback todisable_reasonfor backward compatibility.
128-132: Previously flagged: Bug in key name for disable reason check.This is the same issue flagged in the previous review and also present in
reEnableAllAutoDisabledChannels. The code checksotherInfo["disable_reason"]but should checkotherInfo["status_reason"]first (with fallback for backward compatibility).controller/relay.go (1)
184-188: Critical:breakexits outer channel loop instead of trying next channel.When
GetNextEnabledKey()fails before entering the per-key loop, thebreakat line 187 exits the outer channel retry loop (line 172) instead of moving to the next available channel. This prevents proper channel failover when all keys in a multi-key channel are disabled.Apply this diff to jump to the channel-level retry gate:
key, keyIdx, keyErr := channel.GetNextEnabledKey() if keyErr != nil { newAPIError = keyErr - break // No keys available, break to outer loop to switch channel + goto next_channel // No keys available, try next channel }
🧹 Nitpick comments (3)
service/channel.go (1)
12-12: Unused parameter:channelIdis never referenced in the function body.The signature was changed from
channelType inttochannelId int, but the parameter is not used anywhere in the function. Consider removing it if not needed, or if it's intended for future use, document why it's currently unused.controller/relay.go (2)
191-191: Document the hardcoded 20-key iteration cap.The loop condition
j < channel.ChannelInfo.MultiKeySize && j < 20caps per-key retries at 20, even ifMultiKeySizeis larger. This prevents excessive retry attempts on channels with many keys, but the rationale isn't documented.Consider adding a comment to explain the limit or defining it as a constant:
+ // Cap per-key retries to prevent excessive attempts on large multi-key channels for j := 0; j < channel.ChannelInfo.MultiKeySize && j < 20; j++ {Alternatively, define a constant:
+ const maxMultiKeyRetries = 20 + for j := 0; j < channel.ChannelInfo.MultiKeySize && j < maxMultiKeyRetries; j++ {
289-294: Consider using error types instead of string matching.The new checks prevent retries for "no response received" and "no candidates returned" errors by matching error message strings. This is fragile—if upstream error messages change, these checks will break.
If feasible, define error type constants and check
openaiErr.GetErrorType()orGetErrorCode()instead:- if strings.Contains(openaiErr.Error(), "no response received") { - return false - } - if strings.Contains(openaiErr.Error(), "no candidates returned") { - return false - } + if openaiErr.GetErrorType() == types.ErrorTypeNoResponse || + openaiErr.GetErrorType() == types.ErrorTypeNoCandidates { + return false + }If error types aren't available, at least extract the strings to constants:
+ const ( + errMsgNoResponse = "no response received" + errMsgNoCandidates = "no candidates returned" + ) + - if strings.Contains(openaiErr.Error(), "no response received") { + if strings.Contains(openaiErr.Error(), errMsgNoResponse) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controller/relay.go(5 hunks)model/channel.go(2 hunks)service/channel.go(1 hunks)web/src/components/table/redemptions/modals/EditRedemptionModal.jsx(1 hunks)web/src/components/table/tokens/modals/EditTokenModal.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
model/channel.go (2)
common/constants.go (3)
ChannelStatusAutoDisabled(198-198)ChannelStatusEnabled(196-196)ChannelStatusManuallyDisabled(197-197)model/main.go (1)
DB(63-63)
controller/relay.go (10)
relay/common/relay_info.go (1)
RelayInfo(73-118)model/channel.go (2)
Channel(20-54)ChannelInfo(56-64)types/error.go (1)
NewAPIError(81-89)common/gin.go (1)
GetRequestBody(15-27)relay/websocket.go (1)
WssHelper(14-45)relay/claude_handler.go (1)
ClaudeHelper(20-126)types/channel_error.go (1)
NewChannelError(12-21)middleware/distributor.go (1)
SetupContextForSelectedChannelWithKey(265-299)common/constants.go (1)
RetryTimes(107-107)service/channel.go (1)
DisableChannel(40-47)
service/channel.go (5)
types/error.go (1)
NewAPIError(81-89)common/constants.go (5)
AutomaticDisableChannelEnabled(102-102)ChannelStatusAutoDisabled(198-198)EnableAutodisabledChannelOrKeyAt(110-110)EnableAutodisabledChannelOrKeyAfterMinute(109-109)ChannelStatusEnabled(196-196)types/channel_error.go (1)
ChannelError(3-10)model/channel.go (4)
UpdateChannelStatus(597-658)GetAutoDisabledChannels(971-975)GetChannelsWithDisabledKeys(977-982)ChannelInfo(56-64)common/sys_log.go (2)
SysLog(10-13)SysError(15-18)
🔇 Additional comments (8)
web/src/components/table/tokens/modals/EditTokenModal.jsx (1)
477-480: Verify if quota preset changes belong in this PR.The PR objectives focus on fixing Gemini API
system_instructionparameter handling, but these changes only modify UI quota presets. Additionally, the quota range has been narrowed from $1-$1000 to $10-$40, removing both small ($1) and large ($500, $1000) options that some users may rely on.Please confirm:
- Are these quota preset changes intentional for this PR, or should they be in a separate PR?
- Is the removal of $1, $500, and $1000 presets intentional? Users who need very small or large quotas will need to manually enter values.
web/src/components/table/redemptions/modals/EditRedemptionModal.jsx (1)
282-284: Verify if quota preset changes belong in this PR.Similar to EditTokenModal.jsx, these quota preset changes appear unrelated to the PR's stated objective of fixing Gemini API
system_instructionhandling. The quota range has been narrowed from $1-$1000 to $10-$40, which may impact users who need redemption codes with very small or large quotas.Please confirm:
- Should these quota preset changes be in a separate PR focused on UI improvements?
- Is the removal of $1, $50, $100, $500, and $1000 presets intentional for redemption codes?
model/channel.go (1)
572-575: LGTM: Correctly re-enables auto-disabled channels when a key is cleared.The logic properly handles the case where a channel was auto-disabled due to all keys being disabled. When a key is re-enabled (status == Enabled), the channel status is updated from AutoDisabled to Enabled, ensuring the channel becomes available again.
service/channel.go (3)
19-38: LGTM: Improved error classification for channel disabling.The rewritten logic correctly distinguishes between transient errors (timeouts, connection issues) that shouldn't trigger disables and permanent errors (401, 403, insufficient_quota) that warrant disabling. The lowercase conversion ensures case-insensitive error message matching.
40-47: LGTM: Clean implementation with proper logging.The function correctly calls
model.UpdateChannelStatuswithChannelStatusAutoDisabledand provides informative success/failure logging.
49-70: LGTM: Well-structured periodic re-enable scheduler.The function implements two modes (daily fixed time and interval-based) with proper state tracking (
lastEnableAt) to prevent duplicate executions within the same hour. The 1-minute ticker provides reasonable granularity.controller/relay.go (2)
62-82: LGTM! Clean extraction of relay routing logic.The new
relayToChannelhelper properly:
- Resets the request body for downstream handlers (needed for retries)
- Routes to the appropriate backend based on
RelayFormat- Consolidates error handling via
processChannelErrorThis refactoring enables the multi-key retry logic without duplicating route dispatch code.
340-344: LGTM! Improved error messaging for quota-related channel disablements.The new logic prefixes the disablement reason with
"insufficient_quota: "when the error type isinsufficient_quota, making it easier to identify and distinguish quota-related failures in logs and monitoring.
|
This PR should be closed:
|
alpha -> mian
feat: matrix ci
feat: matrix ci
feat: matrix ci
feat: matrix ci
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)
setting/rate_limit.go (1)
45-51: Data race: Write operation uses read lock.The existing
UpdateModelRequestRateLimitGroupByJSONStringusesRLock()(read lock) but modifies the map, which is a data race. Your new functions at lines 98-104 and 151-157 correctly useLock()for writes. Consider fixing this existing bug for consistency and correctness.func UpdateModelRequestRateLimitGroupByJSONString(jsonStr string) error { - ModelRequestRateLimitMutex.RLock() - defer ModelRequestRateLimitMutex.RUnlock() + ModelRequestRateLimitMutex.Lock() + defer ModelRequestRateLimitMutex.Unlock() ModelRequestRateLimitGroup = make(map[string][2]int) return json.Unmarshal([]byte(jsonStr), &ModelRequestRateLimitGroup) }
♻️ Duplicate comments (1)
controller/relay.go (1)
183-227: Bug:breakstatements at lines 187, 197, 225 exit inner loop incorrectly.As flagged in previous reviews, the
breakstatements inside the multi-key loop don't correctly transition to the next channel. WhenGetNextEnabledKey()fails:
- Line 187:
breakexits the outer channel loop entirely instead of trying the next channel- Lines 197, 225:
breakexits only the inner key loop but falls through incorrectlyApply these fixes:
if keyErr != nil { newAPIError = keyErr - break // No keys available, break to outer loop to switch channel + goto next_channel // No keys available, try next channel }And for lines 194-199 and 222-226:
if keyErr != nil { newAPIError = keyErr - break + goto next_channel }
🧹 Nitpick comments (3)
relay/channel/gemini/relay-gemini-native.go (1)
166-167: Ignoring marshal error could hide issues.The marshaling error is silently discarded. While marshaling a simple struct is unlikely to fail, ignoring errors hides potential issues and violates defensive programming practices.
- responseBody, _ := common.Marshal(emptyResponse) - service.IOCopyBytesGracefully(c, resp, responseBody) + responseBody, err := common.Marshal(emptyResponse) + if err != nil { + logger.LogError(c, "failed to marshal empty response: "+err.Error()) + return usage, nil + } + service.IOCopyBytesGracefully(c, resp, responseBody)middleware/model-rate-limit.go (2)
303-303:inMemoryRateLimiter.Initcalled on every request may be inefficient.
Initis called each timecheckTokenRateLimitMemoryorcheckTokenDailyRateLimitMemoryis invoked. IfInitperforms non-trivial initialization, this could impact performance. Consider moving initialization to a sync.Once pattern or checking if already initialized.+var tokenRateLimiterOnce sync.Once + func checkTokenRateLimitMemory(c *gin.Context, rateLimitKey string, totalMaxCount, successMaxCount int, duration int64) bool { - inMemoryRateLimiter.Init(time.Duration(setting.TokenRateLimitDurationMinutes) * time.Minute) + tokenRateLimiterOnce.Do(func() { + inMemoryRateLimiter.Init(time.Duration(setting.TokenRateLimitDurationMinutes) * time.Minute) + })
216-261: Consider extracting common rate limit check logic to reduce duplication.The Redis and Memory implementations for both minute-level and daily rate limiting share significant structural similarity. Consider extracting a common interface or template function to reduce code duplication.
Also applies to: 301-324, 365-410, 450-473
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.claude/settings.local.json(1 hunks)controller/option.go(1 hunks)controller/relay.go(5 hunks)middleware/model-rate-limit.go(6 hunks)model/option.go(3 hunks)relay/channel/gemini/relay-gemini-native.go(2 hunks)relay/channel/gemini/relay-gemini.go(4 hunks)setting/rate_limit.go(2 hunks)web/src/components/settings/RateLimitSetting.jsx(2 hunks)web/src/pages/Setting/RateLimit/SettingsRequestRateLimit.jsx(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: RedwindA
Repo: QuantumNous/new-api PR: 1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
📚 Learning: 2025-08-08T17:12:43.157Z
Learnt from: RedwindA
Repo: QuantumNous/new-api PR: 1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
Applied to files:
relay/channel/gemini/relay-gemini.gorelay/channel/gemini/relay-gemini-native.go
📚 Learning: 2025-06-21T03:37:41.726Z
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
Applied to files:
relay/channel/gemini/relay-gemini.gorelay/channel/gemini/relay-gemini-native.gocontroller/relay.go
🧬 Code graph analysis (4)
controller/option.go (1)
setting/rate_limit.go (2)
CheckTokenRateLimitGroup(121-137)CheckTokenDailyRateLimitGroup(174-190)
relay/channel/gemini/relay-gemini-native.go (4)
dto/gemini.go (4)
GeminiChatResponse(260-264)GeminiChatCandidate(244-249)GeminiChatContent(212-215)GeminiPart(174-183)common/json.go (1)
Marshal(20-22)service/http.go (1)
IOCopyBytesGracefully(24-59)service/usage_helpr.go (1)
ResponseText2Usage(19-26)
web/src/pages/Setting/RateLimit/SettingsRequestRateLimit.jsx (1)
web/src/components/settings/RateLimitSetting.jsx (1)
inputs(29-44)
model/option.go (1)
setting/rate_limit.go (11)
TokenRateLimitEnabled(20-20)TokenRateLimitDurationMinutes(21-21)TokenRateLimitCount(22-22)TokenRateLimitSuccessCount(23-23)TokenRateLimitGroup2JSONString(87-96)TokenDailyRateLimitEnabled(28-28)TokenDailyRateLimitCount(29-29)TokenDailyRateLimitSuccessCount(30-30)TokenDailyRateLimitGroup2JSONString(140-149)UpdateTokenRateLimitGroupByJSONString(98-104)UpdateTokenDailyRateLimitGroupByJSONString(151-157)
🔇 Additional comments (19)
relay/channel/gemini/relay-gemini-native.go (1)
148-183: Empty response handling looks reasonable for billing purposes.The logic correctly:
- Sends a synthetic empty response with
STOPfinish reason when no responses were received- Preserves prompt tokens for billing even on empty completions
- Computes completion tokens from response text if available
This aligns with the stated goal of allowing empty completions while maintaining billing consistency.
relay/channel/gemini/relay-gemini.go (3)
974-997: Empty streaming response handling is well-structured.When no responses are received (
SendResponseCount == 0), the handler:
- Creates an empty response chunk with proper structure
- Sets
finish_reasontoSTOP- Logs errors appropriately without failing the request
This ensures clients receive a valid response even for empty completions.
1043-1043: Comment accurately describes the behavior change.The comment
// 允许空回复，正常处理(Allow empty replies, process normally) correctly documents that empty candidate responses are now handled gracefully instead of being rejected.
464-472: The review comment's code snippet does not match the actual code in the file. At line 477 in relay-gemini.go, the field isSystemInstructions(plural), notSystemInstruction(singular) as shown in the snippet. Additionally, there is no breaking change here—the struct field has always been namedSystemInstructions. The JSON tagsystemInstruction(singular) is the correct and intentional Go convention for differentiating the struct field name from the JSON serialization format. This aligns with the Gemini API's expectedsystemInstructionfield and does not constitute a breaking change.Likely an incorrect or invalid review comment.
model/option.go (2)
107-115: New token rate limit options follow established patterns.The new option keys are properly initialized using the same approach as existing rate limit options. The naming convention (
TokenRateLimitEnabled,TokenRateLimitDurationMinutes, etc.) is consistent withModelRequestRateLimit*options.
392-405: Token rate limit update handling is consistent with existing code.The new cases follow the same pattern as
ModelRequestRateLimitGrouphandling. Note thatstrconv.Atoierrors are silently ignored, which is consistent with the existing codebase pattern (e.g., lines 385-391).controller/relay.go (2)
62-82: NewrelayToChannelhelper consolidates relay dispatch logic.This refactoring extracts the common relay-to-backend dispatch pattern into a dedicated function, improving maintainability. The function properly:
- Resets the request body for each attempt
- Routes to appropriate handlers based on
RelayFormat- Processes channel errors with proper context
290-312:isInappropriateErrorhelper for forced channel switching is reasonable.The function correctly identifies "inappropriate" errors that should trigger a channel switch rather than retrying keys on the same channel. The case-insensitive check via
strings.ToLoweris appropriate.middleware/model-rate-limit.go (2)
169-175: New rate limit constants are well-organized.The constants follow the same naming pattern as existing
ModelRequestRateLimitCountMarkandModelRequestRateLimitSuccessCountMark, maintaining consistency.
476-526: Rate limit middleware ordering is correct.The middleware correctly:
- Checks per-key (token) rate limits first
- Then checks per-key daily limits
- Finally checks per-user limits
- Records success metrics after the request completes
This ordering ensures token-level limits take precedence.
controller/option.go (1)
124-141: New rate limit group validation follows established patterns.The validation for
TokenRateLimitGroupandTokenDailyRateLimitGroupmirrors the existingModelRequestRateLimitGroupvalidation. The validation functions (shown in relevant code snippets fromsetting/rate_limit.go) properly check for:
- Valid JSON format
- Non-negative rate limit values
- Values within
math.MaxInt32web/src/components/settings/RateLimitSetting.jsx (2)
35-43: New token rate limit state fields are properly initialized.The default values align with backend defaults:
- Enabled flags default to
false- Count fields default to
0- Duration defaults to
1minute- Group fields default to empty string
54-62: JSON pretty-printing with safe fallback is well-implemented.The try-catch block gracefully handles malformed JSON by preserving the original value or falling back to an empty string. This prevents UI crashes from invalid configuration data.
web/src/pages/Setting/RateLimit/SettingsRequestRateLimit.jsx (3)
42-51: LGTM - State initialization aligns with related component.The new state variables for per-key rate limiting are properly initialized and match the structure in
RateLimitSetting.jsx(as shown in the relevant code snippets). The default values are sensible.
232-346: Approve the token minute-level rate limit section with a minor observation.The section is well-structured and mirrors the existing
ModelRequestRateLimitsection. The validation rules, JSON validation, and onChange handlers follow the established pattern.Note: The
min={0}forTokenRateLimitSuccessCount(line 290) differs fromModelRequestRateLimitSuccessCountwhich hasmin={1}(line 175). This appears intentional per the extraText indicating "0代表不限制" (0 means no limit), which is a valid design choice for per-key limits.
348-444: LGTM - Daily rate limit section is consistent with the minute-level section.The daily rate limit section follows the same patterns and includes appropriate help text explaining the UTC+0 reset behavior. Form controls are properly wired to state handlers.
setting/rate_limit.go (3)
19-32: LGTM - Per-key rate limit variables are well-defined.The new variables follow the established pattern from the existing
ModelRequestRateLimit*variables. Comments are helpful for distinguishing between minute-level and daily limits. The use of separate mutexes for each rate limit group allows for more granular locking.
87-137: LGTM - Token minute rate limit functions are correctly implemented.The functions properly:
- Use
RLock()for read operations andLock()for write operations- Check for nil map before access in
GetTokenRateLimit- Validate both negative values and overflow in
CheckTokenRateLimitGroupThe validation logic correctly allows
limits[1] >= 0(unlike the model rate limit which requires>= 1), which aligns with the UI extraText stating "0 means no limit".
139-190: LGTM - Token daily rate limit functions follow the same correct patterns.The implementation mirrors the minute-level functions with proper locking, nil checks, and validation. Well-structured and consistent.
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(go build:*)", | ||
| "Bash(npm run build)" | ||
| ], | ||
| "deny": [], | ||
| "ask": [] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if .gitignore exists and what it contains regarding .claude/
cat .gitignore 2>/dev/null || echo "No .gitignore found"Repository: QuantumNous/new-api
Length of output: 232
🏁 Script executed:
# Check if .claude/ directory exists elsewhere or has other files
find . -type d -name ".claude" 2>/dev/null | head -20Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check git status to see all files in this PR/commit
git ls-files | grep -E "^\.claude" || echo "No .claude files in git tracking"Repository: QuantumNous/new-api
Length of output: 95
🏁 Script executed:
# Search for any references to .claude in the codebase
rg "\.claude" --type json --type md --type txt 2>/dev/null || echo "No references found"Repository: QuantumNous/new-api
Length of output: 82
Remove this unrelated configuration file from the PR.
The .claude/settings.local.json file is a local Claude IDE integration artifact unrelated to the PR's objective of supporting Gemini API system prompts. It's not present in .gitignore and has no precedent in the repository, suggesting it was added unintentionally. Either remove this file from the commit or add .claude/ to .gitignore if local IDE configurations should not be tracked.
🤖 Prompt for AI Agents
In .claude/settings.local.json lines 1-10: this is a local IDE/config artifact
unrelated to the PR; remove the file from the commit (git rm --cached or delete
and commit) OR add ".claude/" to .gitignore and remove the file from the repo
history/commit so it isn't tracked; update the PR commit (or create a new
commit) to reflect the removal and ensure no other local IDE files are included.
| if strings.Contains(openaiErr.Error(), "no response received") { | ||
| return false | ||
| } | ||
| if strings.Contains(openaiErr.Error(), "no candidates reMeoWturned") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Suspicious string: "reMeoWturned" appears to be a typo or obfuscation.
The string "no candidates reMeoWturned" on line 306 looks like an intentional typo or obfuscated text. If this is meant to match error messages from the Gemini API, it should be corrected to "no candidates returned".
- if strings.Contains(openaiErr.Error(), "no candidates reMeoWturned") {
+ if strings.Contains(openaiErr.Error(), "no candidates returned") {🤖 Prompt for AI Agents
In controller/relay.go around lines 303 to 308, the string literal "no
candidates reMeoWturned" is a typo/obfuscation and should be corrected to match
the actual Gemini API error text; replace it with "no candidates returned"
(preserve surrounding logic and casing) so the error check works as intended and
run relevant tests or logs to confirm the condition now matches real errors.
- Resolve conflicts in controller/relay.go, dto/gemini.go, relay-gemini.go, service/channel.go - Keep inappropriate error retry logic for channel switching - Keep auto re-enable channels feature - Update SystemInstruction field name for consistency - Fix type assertion in controller/option.go 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/gemini/relay-gemini.go (1)
1001-1042: Synthetic empty stream chunk can also mask upstream stream/parse errorsThe new
info.SendResponseCount == 0branch correctly covers the “legit empty completion” case, ensuring clients always receive at least one chunk and that completion tokens can stay at 0 while still sending a final usage frame.However, this same condition is also true when
helper.StreamScannerHandlerstops early because of a JSON unmarshal error (Line 923–928), leavinginfo.SendResponseCountat 0. In that situation we now emit a seemingly successful empty completion instead of surfacing an error (previously we at least did not emit any choice chunk, though the error was still only logged).Consider tracking a
hadDecodeErrorflag (or propagating the error fromStreamScannerHandler) and skipping the synthetic empty chunk when the stream failed to parse, returning an API error instead.
♻️ Duplicate comments (3)
.claude/settings.local.json (1)
1-11: Remove this unrelated IDE configuration file from the PR.This
.claude/settings.local.jsonis a local Claude IDE artifact unrelated to the PR's objective of supporting Gemini API system prompts. A previous review already flagged this—it should not be committed to the repository.Action: Remove this file from the commit:
git rm --cached .claude/settings.local.jsonOr, if you intend to exclude all
.claude/configurations from version control, add it to.gitignore:+.claude/Then remove the file from the repository and update the commit.
main.go (1)
118-126: Validate environment variable values and log precedence.The environment variables lack validation. Invalid values for
ENABLE_AUTODISABLED_CHANNEL_OR_KEY_AT(e.g., 25, -5) or negative values forENABLE_AUTODISABLED_CHANNEL_OR_KEY_AFTER_MINUTEcould cause undefined behavior. Consider clampingATto[0,23]andAFTER_MINUTEto>=0, and logging whenATtakes precedence overAFTER_MINUTE.controller/relay.go (1)
305-310: Suspicious string: "reMeoWturned" appears to be a typo.The string
"no candidates reMeoWturned"on line 308 looks like a typo or intentional obfuscation. If this is meant to match actual Gemini API error messages, it should be"no candidates returned". If the upstream API actually returns this obfuscated string, add a comment explaining why.- if strings.Contains(openaiErr.Error(), "no candidates reMeoWturned") { + if strings.Contains(openaiErr.Error(), "no candidates returned") {
🧹 Nitpick comments (4)
model/option.go (1)
407-420: Token rate limit numeric and group settings follow existing update patternThe new cases for
"TokenRateLimitDurationMinutes","TokenRateLimitCount","TokenRateLimitSuccessCount","TokenRateLimitGroup", and their daily counterparts integrate cleanly withupdateOptionMap, updatingsetting.*fields and delegating JSON parsing toUpdateTokenRateLimitGroupByJSONString/UpdateTokenDailyRateLimitGroupByJSONString. This matches howModelRequestRateLimit*is handled and ensures in‑memory config tracks DB values.If you expect other code paths to call
UpdateOptionwithout going through the controller‑levelCheckToken*RateLimitGroupvalidation, you might consider moving or duplicating the range checks fromCheckTokenRateLimitGroup/CheckTokenDailyRateLimitGroupinto the correspondingUpdate*GroupByJSONStringhelpers so that all writers get the same safety guarantees, regardless of entry point.controller/channel-test.go (1)
589-591: Consider removing or documenting the commented-out code.Commented-out code without explanation is typically discouraged. Since the auto-re-enable logic has been moved to a scheduled mechanism (
CheckAndReEnableChannels), either remove this dead code entirely or add a comment explaining why it's preserved (e.g., for future reference).- //if !isChannelEnabled && service.ShouldEnableChannel(newAPIError, channel.Status) { - // service.EnableChannel(channel.Id, common.GetContextKeyString(result.context, constant.ContextKeyChannelKey), channel.Name) - //}controller/relay.go (1)
192-229: Multi-key retry loop looks correct but consider edge case.The logic correctly tracks tried keys and bounds iterations. However, if
GetNextEnabledKeyuses round-robin polling and returns the same key index on consecutive calls (before the key is marked as tried), thetriedKeyscheck at line 194 combined with thecontinueat line 201 will re-fetch keys. This is bounded byj < MultiKeySizeandj < 20, so it won't infinite loop, but may waste iterations.Consider adding early exit when
len(triedKeys) >= enabledKeyCountif that information is available, to avoid unnecessaryGetNextEnabledKeycalls when all enabled keys have been tried.relay/gemini_handler.go (1)
124-135: EmptySystemInstructioncleanup is reasonableThe cleanup that sets
request.SystemInstruction = nilwhen all parts have emptyTextavoids emitting a uselesssystem_instructionobject with no real content. Given Gemini system instructions are expected to be textual, this is a sensible guard and should not affect valid requests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.claude/settings.local.json(1 hunks)common/constants.go(1 hunks)controller/channel-test.go(1 hunks)controller/option.go(1 hunks)controller/relay.go(5 hunks)dto/gemini.go(1 hunks)main.go(1 hunks)middleware/distributor.go(1 hunks)model/channel.go(2 hunks)model/option.go(3 hunks)relay/channel/gemini/relay-gemini.go(4 hunks)relay/gemini_handler.go(1 hunks)service/convert.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- model/channel.go
- dto/gemini.go
- common/constants.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
📚 Learning: 2025-08-08T17:12:43.157Z
Learnt from: RedwindA
Repo: QuantumNous/new-api PR: 1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
Applied to files:
relay/gemini_handler.goservice/convert.gorelay/channel/gemini/relay-gemini.go
📚 Learning: 2025-06-21T03:37:41.726Z
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
Applied to files:
relay/channel/gemini/relay-gemini.gocontroller/relay.go
🧬 Code graph analysis (7)
relay/gemini_handler.go (3)
dto/gemini.go (2)
GeminiChatContent(240-243)GeminiPart(202-211)common/gin.go (1)
SetContextKey(54-56)constant/context_key.go (1)
ContextKeySystemPromptOverride(49-49)
service/convert.go (1)
dto/openai_request.go (1)
Message(277-288)
relay/channel/gemini/relay-gemini.go (5)
dto/gemini.go (1)
GeminiChatContent(240-243)dto/openai_response.go (3)
ChatCompletionsStreamResponseChoice(79-84)ChatCompletionsStreamResponseChoiceDelta(86-92)ChatCompletionsStreamResponse(140-148)common/sys_log.go (1)
SysLog(11-14)service/usage_helpr.go (1)
ResponseText2Usage(19-26)types/error.go (2)
NewOpenAIError(222-245)ErrorCodePromptBlocked(72-72)
model/option.go (2)
common/constants.go (1)
OptionMap(37-37)setting/rate_limit.go (11)
TokenRateLimitEnabled(20-20)TokenRateLimitDurationMinutes(21-21)TokenRateLimitCount(22-22)TokenRateLimitSuccessCount(23-23)TokenRateLimitGroup2JSONString(87-96)TokenDailyRateLimitEnabled(28-28)TokenDailyRateLimitCount(29-29)TokenDailyRateLimitSuccessCount(30-30)TokenDailyRateLimitGroup2JSONString(140-149)UpdateTokenRateLimitGroupByJSONString(98-104)UpdateTokenDailyRateLimitGroupByJSONString(151-157)
middleware/distributor.go (2)
model/channel.go (1)
Channel(20-57)types/error.go (1)
NewAPIError(83-91)
controller/relay.go (9)
relay/common/relay_info.go (1)
RelayInfo(74-121)model/channel.go (2)
Channel(20-57)ChannelInfo(59-67)common/gin.go (1)
GetRequestBody(17-29)dto/request_common.go (1)
Request(8-12)relay/websocket.go (1)
WssHelper(14-45)types/channel_error.go (1)
NewChannelError(12-21)constant/context_key.go (1)
ContextKeyChannelKey(37-37)middleware/distributor.go (1)
SetupContextForSelectedChannelWithKey(304-338)common/constants.go (1)
RetryTimes(108-108)
controller/option.go (1)
setting/rate_limit.go (2)
CheckTokenRateLimitGroup(121-137)CheckTokenDailyRateLimitGroup(174-190)
🔇 Additional comments (10)
controller/option.go (1)
167-184: Token group validation matches existing rate‑limit patternThese new branches for
"TokenRateLimitGroup"and"TokenDailyRateLimitGroup"mirror the existing"ModelRequestRateLimitGroup"validation flow: they normalizeoption.Valueto a string, run strict JSON/range checks viasetting.CheckTokenRateLimitGroup/CheckTokenDailyRateLimitGroup, and short‑circuit with a clear error message on failure. This keeps invalid configs from reaching persistence while staying consistent with existing option handling. Looks good.model/option.go (2)
109-117: Token rate limit options correctly exposed viaInitOptionMapThe new
TokenRateLimit*andTokenDailyRateLimit*entries mirror the existingModelRequestRateLimit*wiring: they pull defaults fromsetting.*and serialize groups viaTokenRateLimitGroup2JSONString/TokenDailyRateLimitGroup2JSONString. This should make the new config available to the UI and API without changing existing behavior.
291-294: Runtime flags for token rate limits are wired consistentlyAdding
"TokenRateLimitEnabled"and"TokenDailyRateLimitEnabled"to the boolean switch keeps the “Enabled” handling uniform: values are parsed once and written intosetting.TokenRateLimitEnabled/setting.TokenDailyRateLimitEnabled, just likeModelRequestRateLimitEnabledand other flags. No issues here.middleware/distributor.go (1)
301-338: Well-structured refactoring of per-key context setup.The extraction of
SetupContextForSelectedChannelWithKeyas a dedicated helper is a good refactor. It cleanly separates per-key context initialization from channel selection logic, enabling the multi-key retry mechanism incontroller/relay.goto reuse this setup per key iteration.controller/relay.go (2)
63-83: Clean extraction of relay routing logic.The
relayToChannelhelper cleanly encapsulates the format-based handler selection and error processing. Resetting the request body before each relay attempt ensures idempotent retries.
359-363: Good enhancement for quota error identification.Prefixing the disable reason with
"insufficient_quota: "for quota-related errors improves observability and makes it easier to identify channels disabled due to quota exhaustion versus other errors.service/convert.go (1)
600-607: This review comment is based on incorrect code state and should be dismissed.The code snippet in the review shows
geminiRequest.SystemInstruction(singular), but the actual code at lines 600-607 in service/convert.go usesgeminiRequest.SystemInstructions(plural). There is no field rename fromSystemInstructionstoSystemInstructionin the current codebase. The Go struct field remains plural (SystemInstructions) with the JSON tagsystemInstruction(singular, camelCase) for proper Gemini API compatibility. No breaking change exists, and no backward compatibility measures are needed.Likely an incorrect or invalid review comment.
relay/channel/gemini/relay-gemini.go (2)
476-484: System instruction now mapped viaSystemInstruction– looks consistentUsing
geminiRequest.SystemInstructionto carry aggregatedsystemrole content matches the DTO rename and preserves previous behavior (one joined text part). No issues here.
1070-1076: Non-stream empty-candidate handling is clearer and aligns with streaming pathAllowing
len(Candidates) == 0as a valid response while explicitly treatingPromptFeedback.BlockReasonas an error (prompt_blocked) matches the desired semantics: true blocks become structured errors, and empty-but-not-blocked replies pass through. This looks good and consistent with the stream handler behavior.relay/gemini_handler.go (1)
98-121: The review comment is based on inaccurate assumptions. The JSON tag remainssystemInstruction(camelCase), notsystem_instruction(snake_case) as claimed. Additionally, the code snippet shown in the review usesrequest.SystemInstruction(singular), but the actual implementation usesrequest.SystemInstructions(plural) with the correctly matching JSON tag. There is no breaking change—backward compatibility with existing clients sendingsystemInstructionis maintained via the JSON tag mapping. The logic for creating/merging system instructions is sound.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
service/channel.go (1)
39-45: CentralizedDisableChannelhelper looks good
DisableChannelcorrectly delegates tomodel.UpdateChannelStatuswithChannelStatusAutoDisabled, passes throughUsingKeyfor multi‑key channels, and logs both success and failure viacommon.SysLog. This matches the existing status update pattern inmodel.UpdateChannelStatus(reason stored asstatus_reason) and is a clean centralization of the disable logic.If you want slightly richer diagnostics, you could optionally include
channelError.UsingKeyin the logs for multi‑key scenarios, but that’s not strictly necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model/channel.go(1 hunks)service/channel.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- model/channel.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
Learnt from: RedwindA
Repo: QuantumNous/new-api PR: 1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
🧬 Code graph analysis (1)
service/channel.go (5)
types/error.go (1)
NewAPIError(83-91)common/constants.go (2)
AutomaticDisableChannelEnabled(103-103)ChannelStatusAutoDisabled(196-196)types/channel_error.go (1)
ChannelError(3-10)model/channel.go (1)
UpdateChannelStatus(605-674)common/sys_log.go (1)
SysLog(11-14)
🔇 Additional comments (1)
service/channel.go (1)
11-37: Fix unusedchannelIdand reorderinsufficient_quotacheck inShouldDisableChannelTwo issues in the function:
channelIdparameter is unused — Go will not compile this. Add_ = channelIdor use it for logging if needed.
insufficient_quotacheck is unreachable for 429 responses — The function returnsfalseforerr.StatusCode == 429before checkingerr.GetErrorType() == "insufficient_quota". This defeats the new insufficient_quota logic for providers that return HTTP 429 with that error type.Reorder the error type check before status code checks to catch insufficient_quota regardless of HTTP status:
- if err.StatusCode == 401 { - return true - } - if err.StatusCode == 429 { - // too many requests - return false - } - if err.StatusCode == 403 { - // forbidden - return true - } - if err.GetErrorType() == "insufficient_quota" { - return true - } + if err.GetErrorType() == "insufficient_quota" { + return true + } + if err.StatusCode == 401 { + return true + } + if err.StatusCode == 429 { + return false + } + if err.StatusCode == 403 { + return true + }Also add
_ = channelIdafter the function signature to suppress the unused parameter error.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/components/table/redemptions/modals/EditRedemptionModal.jsx(1 hunks)web/src/components/table/tokens/modals/EditTokenModal.jsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: RedwindA
Repo: QuantumNous/new-api PR: 1537
File: relay/gemini_handler.go:330-342
Timestamp: 2025-08-08T17:12:43.157Z
Learning: In the new-api repository, the `GeminiEmbeddingHandler` function in `relay/gemini_handler.go` is designed specifically for native Gemini embedding requests and therefore does not require the `ConvertGeminiRequest` step that is used in the chat handler. The embedding requests are already in the native Gemini format and don't need conversion.
Learnt from: 9Ninety
Repo: QuantumNous/new-api PR: 1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
| data={[ | ||
| { value: 500000, label: '1$' }, | ||
| { value: 5000000, label: '10$' }, | ||
| { value: 10000000, label: '20$' }, | ||
| { value: 15000000, label: '30$' }, | ||
| { value: 20000000, label: '40$' }, | ||
| { value: 25000000, label: '50$' }, | ||
| { value: 30000000, label: '60$' }, | ||
| { value: 35000000, label: '70$' }, | ||
| { value: 40000000, label: '80$' }, | ||
| { value: 45000000, label: '90$' }, | ||
| { value: 50000000, label: '100$' }, | ||
| { value: 250000000, label: '500$' }, | ||
| { value: 500000000, label: '1000$' }, | ||
| ]} |
There was a problem hiding this comment.
This change is unrelated to the PR objective.
The quota preset updates are unrelated to fixing Gemini API system_instruction parameter handling, which is the stated purpose of this PR. While the calculations are mathematically correct (each value equals the dollar amount × 500,000), mixing unrelated changes reduces reviewability and makes it difficult to assess the impact of the Gemini fix independently.
Consider moving this redemption preset update to a separate PR focused on UI improvements.
Based on PR objectives and reviewer comments, this aligns with concerns about the PR containing multiple unrelated modifications.
🤖 Prompt for AI Agents
In web/src/components/table/redemptions/modals/EditRedemptionModal.jsx around
lines 309 to 320, the updated quota preset data (the array of dollar-value
pairs) is unrelated to this PR's objective and should be removed from this
change set; revert these lines back to their previous state in this PR, and
instead create a separate focused PR for the UI preset changes that includes a
clear description, rationale, and tests/QA notes; ensure the revert is committed
to the current branch so the Gemini API system_instruction fix can be reviewed
in isolation.
| data={[ | ||
| { value: 500000, label: '1$' }, | ||
| { value: 5000000, label: '10$' }, | ||
| { value: 25000000, label: '50$' }, | ||
| { value: 50000000, label: '100$' }, | ||
| { value: 250000000, label: '500$' }, | ||
| { value: 500000000, label: '1000$' }, | ||
| { value: 5000000, label: '10$' }, | ||
| { value: 10000000, label: '20$' }, | ||
| { value: 15000000, label: '30$' }, | ||
| { value: 20000000, label: '40$' }, | ||
| { value: 25000000, label: '50$' }, | ||
| { value: 30000000, label: '60$' }, | ||
| { value: 35000000, label: '70$' }, | ||
| { value: 40000000, label: '80$' }, | ||
| { value: 45000000, label: '90$' }, | ||
| { value: 50000000, label: '100$' }, |
There was a problem hiding this comment.
This change is unrelated to the PR objective.
The PR objective is to fix Gemini API's system_instruction parameter handling, but this change modifies token quota presets in the UI. This aligns with the concern raised by RedwindA that the PR contains multiple changes unrelated to the system_instruction fix.
Additionally, the new preset values remove potentially useful options:
- 1$ (5,000,000 units) – useful for testing or small quotas
- 500$ and 1000$ – useful for larger production quotas
Consider reverting this change and addressing it in a separate PR focused on improving the quota preset UX, with proper justification and user research.
🤖 Prompt for AI Agents
In web/src/components/table/tokens/modals/EditTokenModal.jsx around lines 490 to
500, the token quota preset array was modified which is unrelated to the Gemini
API system_instruction fix; revert the preset data to the original set (restore
the removed entries such as the 1$ option and larger presets like 500$ and
1000$) or remove this change entirely from this PR so the UI quota-presets
remain unchanged here, and if you want to change presets create a separate PR
with UX justification and tests.
5 participants
修复原项目使用gemini api连接时无法通过system_instruction参数使用gemini系统提示词的问题
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
UI
✏️ Tip: You can customize this high-level summary in your review settings.