Pr/key enabled switch upstream#422
Conversation
WalkthroughThis PR implements a complete API key enable/disable feature. Keys can be toggled between enabled and disabled states without deletion, controlling whether they participate in request routing. The implementation spans backend pool synchronization, HTTP API, and frontend UI with multi-language support. ChangesAPI Key Enable/Disable Toggle
Sequence DiagramsequenceDiagram
participant User
participant KeyTable
participant keysApi
participant UpdateKeyEnabled
participant KeyService
participant KeyProvider
participant Redis
participant Database
User->>KeyTable: click toggle switch
KeyTable->>KeyTable: optimistic: set key.enabled
KeyTable->>keysApi: updateKeyEnabled(id, false)
keysApi->>UpdateKeyEnabled: PUT /api/keys/:id/enabled {enabled: false}
UpdateKeyEnabled->>KeyService: SetKeyEnabled(id, false)
KeyService->>KeyProvider: SetKeyEnabled(id, false)
KeyProvider->>Database: update key enabled field
KeyProvider->>Redis: update hash enabled field
KeyProvider->>Redis: remove from active_keys
Redis-->>KeyProvider: deletion confirmed
KeyProvider-->>KeyService: return updated key
KeyService-->>UpdateKeyEnabled: return updated key
UpdateKeyEnabled-->>keysApi: {id, enabled: false, status}
keysApi-->>KeyTable: response
KeyTable->>KeyTable: show success, trigger sync refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (1)
web/src/components/keys/KeyTable.vue (1)
342-344: ⚡ Quick winConsider showing an error message to the user when the toggle fails.
Currently, when the API call fails, the key's enabled state is rolled back (which provides visual feedback), but no error message is shown to the user—only a console log. Consider adding
window.$message.error(...)in the catch block to inform the user why the toggle failed, similar to how other operations in this file handle errors.💡 Suggested improvement
} catch (error) { key.enabled = previousEnabled; console.error("Update key enabled failed", error); + window.$message.error(t("keys.updateEnabledFailed")); } finally {Note: You'll need to add the corresponding i18n key
updateEnabledFailedto the locale files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/keys/KeyTable.vue` around lines 342 - 344, In the catch block that rolls back the toggle (the block containing "key.enabled = previousEnabled" and "console.error('Update key enabled failed', error)") add a user-facing error message via window.$message.error(...) (or the app's message helper) using the i18n key "updateEnabledFailed" so users see why the toggle failed; keep the existing rollback logic and console.error, and also add the "updateEnabledFailed" entry to the locale files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/handler/key_handler.go`:
- Around line 507-509: The UpdateKeyEnabledRequest struct currently uses Enabled
bool which treats a missing field as false; change it to Enabled *bool and
update the UpdateKeyEnabled handler to return a 400 Bad Request when req.Enabled
is nil (reject requests that omit enabled). Also locate the database error
handling in the same handler where it checks for "record not found" via string
matching and replace that with a proper errors.Is(err, gorm.ErrRecordNotFound)
check (or use app_errors.ParseDBError if preferred) to correctly detect wrapped
not-found errors.
- Around line 568-574: Replace the string-based not-found check in
UpdateKeyEnabled's error handling with a proper errors.Is check: after calling
s.KeyService.SetKeyEnabled, detect a missing record by using errors.Is(err,
gorm.ErrRecordNotFound) (and add the missing "errors" import and ensure "gorm"
is imported) and map that case to response.Error(c,
app_errors.ErrResourceNotFound); otherwise continue to call response.Error(c,
app_errors.ParseDBError(err)).
In `@internal/keypool/provider.go`:
- Around line 642-653: The active-list sync currently performs
p.store.HSet(keyHashKey, p.apiKeyToMap(&key)) before removing the key from the
active list, which can leave a disabled key in group:*:active_keys if LRem
fails; change the logic so that on the disable path you call
p.store.LRem(activeKeysListKey, 0, key.ID) first and return on error, then
update the hash with p.store.HSet; conversely, on the enable path update the
hash via p.store.HSet(...) first and only then call
p.store.LPush(activeKeysListKey, key.ID) (using isKeyAvailableForPool to decide
the push) while preserving the existing error messages and returns for
p.store.LRem, p.store.HSet and p.store.LPush.
---
Nitpick comments:
In `@web/src/components/keys/KeyTable.vue`:
- Around line 342-344: In the catch block that rolls back the toggle (the block
containing "key.enabled = previousEnabled" and "console.error('Update key
enabled failed', error)") add a user-facing error message via
window.$message.error(...) (or the app's message helper) using the i18n key
"updateEnabledFailed" so users see why the toggle failed; keep the existing
rollback logic and console.error, and also add the "updateEnabledFailed" entry
to the locale files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 267f700a-08a7-4366-89fa-2c56ab9ad5e2
📒 Files selected for processing (11)
internal/handler/key_handler.gointernal/keypool/provider.gointernal/models/types.gointernal/router/router.gointernal/services/key_service.goweb/src/api/keys.tsweb/src/components/keys/KeyTable.vueweb/src/locales/en-US.tsweb/src/locales/ja-JP.tsweb/src/locales/zh-CN.tsweb/src/types/models.ts
| type UpdateKeyEnabledRequest struct { | ||
| Enabled bool `json:"enabled"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
echo "== Context around UpdateKeyEnabledRequest =="
sed -n '470,650p' "$file" | nl -ba | sed -n '1,220p'
echo
echo "== Exact lines 500-590 (matches requested ranges) =="
sed -n '495,590p' "$file" | nl -ba
echo
echo "== Search for UpdateKeyEnabled and binding/error mapping =="
rg -n "UpdateKeyEnabled|ShouldBindJSON|record not found|strings\.Contains\(err\.Error|errors\.Is" "$file"Repository: tbphp/gpt-load
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
if [ ! -f "$file" ]; then
echo "Missing file: $file" >&2
exit 1
fi
python3 - <<'PY'
import itertools
path = "internal/handler/key_handler.go"
start, end = 470, 650
with open(path, "r", encoding="utf-8") as f:
for i, line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:5d}: {line.rstrip()}")
PY
echo
echo "== Search: UpdateKeyEnabled binding and error mapping =="
rg -n "UpdateKeyEnabledRequest|type UpdateKeyEnabledRequest|UpdateKeyEnabled\(|ShouldBindJSON|record not found|strings\.Contains\(|errors\.Is" "$file"Repository: tbphp/gpt-load
Length of output: 5380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SetKeyEnabled\\(" -S .
rg -n "type .*KeyService|KeyService" -S internal .
rg -n "record not found" -S .
rg -n "ParseDBError\\(" -S internal .Repository: tbphp/gpt-load
Length of output: 16179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show KeyService.SetKeyEnabled implementation
sed -n '230,320p' internal/services/key_service.go | cat
echo
echo "----"
# Show KeyProvider.SetKeyEnabled implementation
sed -n '580,720p' internal/keypool/provider.go | cat
echo
echo "----"
# Show ParseDBError implementation
sed -n '1,140p' internal/errors/errors.go | catRepository: tbphp/gpt-load
Length of output: 10432
Reject requests that omit enabled (UpdateKeyEnabled).
Enabled bool treats {} as enabled=false, causing an unintended disable; use *bool and return 400 when enabled is missing. Also replace the brittle strings.Contains(err.Error(), "record not found") check with errors.Is(err, gorm.ErrRecordNotFound) (or rely on app_errors.ParseDBError), since the provider wraps underlying GORM errors with %w.
Suggested fix
type UpdateKeyEnabledRequest struct {
- Enabled bool `json:"enabled"`
+ Enabled *bool `json:"enabled"`
}
@@
var req UpdateKeyEnabledRequest
if err := c.ShouldBindJSON(&req); err != nil {
response.Error(c, app_errors.NewAPIError(app_errors.ErrInvalidJSON, err.Error()))
return
}
+ if req.Enabled == nil {
+ response.Error(c, app_errors.NewAPIError(app_errors.ErrBadRequest, "enabled is required"))
+ return
+ }
- key, err := s.KeyService.SetKeyEnabled(uint(keyID), req.Enabled)
+ key, err := s.KeyService.SetKeyEnabled(uint(keyID), *req.Enabled)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/key_handler.go` around lines 507 - 509, The
UpdateKeyEnabledRequest struct currently uses Enabled bool which treats a
missing field as false; change it to Enabled *bool and update the
UpdateKeyEnabled handler to return a 400 Bad Request when req.Enabled is nil
(reject requests that omit enabled). Also locate the database error handling in
the same handler where it checks for "record not found" via string matching and
replace that with a proper errors.Is(err, gorm.ErrRecordNotFound) check (or use
app_errors.ParseDBError if preferred) to correctly detect wrapped not-found
errors.
| key, err := s.KeyService.SetKeyEnabled(uint(keyID), req.Enabled) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "record not found") { | ||
| response.Error(c, app_errors.ErrResourceNotFound) | ||
| } else { | ||
| response.Error(c, app_errors.ParseDBError(err)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print SetKeyEnabled implementation in internal/keypool/provider.go
file="internal/keypool/provider.go"
if [ -f "$file" ]; then
echo "== $file =="
# Print around function definition
line=$(rg -n "func \\(p \\*KeyProvider\\) SetKeyEnabled\\b" "$file" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-30))
end=$((line+120))
sed -n "${start},${end}p" "$file" | nl -ba | sed -n '1,200p'
else
echo "SetKeyEnabled not found in $file"
fi
else
echo "File not found: $file"
fi
# Also inspect internal/services/key_service.go just in case it's implemented there
file2="internal/services/key_service.go"
if [ -f "$file2" ]; then
echo "== $file2 =="
line=$(rg -n "func \\(s \\*KeyService\\) SetKeyEnabled\\b" "$file2" | head -n1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+80))
sed -n "${start},${end}p" "$file2" | nl -ba | sed -n '1,200p'
else
echo "SetKeyEnabled not found in $file2"
fi
else
echo "File not found: $file2"
fiRepository: tbphp/gpt-load
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# helper to print with line numbers (no nl dependency)
print_with_numbers() {
local f="$1"; local start="$2"; local end="$3"
local i=1
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%5d\t%s\n", NR, $0 }' "$f"
}
file="internal/keypool/provider.go"
echo "== $file =="
if [ -f "$file" ]; then
line=$(rg -n "func \\(p \\*KeyProvider\\) SetKeyEnabled\\b" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-30))
end=$((line+160))
print_with_numbers "$file" "$start" "$end"
else
echo "SetKeyEnabled not found"
fi
else
echo "File not found: $file"
fi
file2="internal/services/key_service.go"
echo "== $file2 =="
if [ -f "$file2" ]; then
line=$(rg -n "func \\(s \\*KeyService\\) SetKeyEnabled\\b" "$file2" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+120))
print_with_numbers "$file2" "$start" "$end"
else
echo "SetKeyEnabled not found"
fi
else
echo "File not found: $file2"
fiRepository: tbphp/gpt-load
Length of output: 9725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
echo "== $file (imports) =="
# Print import block (best-effort)
start=$(rg -n "^import \\(" "$file" | head -n1 | cut -d: -f1 || true)
if [ -n "${start:-}" ]; then
# find closing paren
end=$(awk -v s="$start" 'NR>=s{ if($0 ~ /^\\)/){ print NR; exit } }' "$file")
sed -n "${start},${end}p" "$file" | nl -ba | head -n 200
else
echo "No multi-line import block found"
fi
echo
echo "== $file (around SetKeyEnabled handler error mapping) =="
# Print around provided line numbers
sed -n '540,610p' "$file" | nl -ba | sed -n '1,200p'Repository: tbphp/gpt-load
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
echo "== $file (import block) =="
# find import block start
start=$(rg -n "^import \\(" "$file" | head -n1 | cut -d: -f1 || true)
if [ -z "${start:-}" ]; then
# single-line import fallback
rg -n "^import " "$file" | head -n 50
else
end=$(awk -v s="$start" 'NR>=s{ if($0 ~ /^\\)/){ print NR; exit } }' "$file")
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%5d\t%s\n", NR, $0 }' "$file"
fi
echo
echo "== $file (around SetKeyEnabled mapping) =="
awk 'NR>=540 && NR<=610 { printf "%5d\t%s\n", NR, $0 }' "$file"Repository: tbphp/gpt-load
Length of output: 1544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
awk 'NR<=140 { printf "%5d\t%s\n", NR, $0 }' "$file" | sed -n '1,140p'Repository: tbphp/gpt-load
Length of output: 5124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="internal/handler/key_handler.go"
echo "== search for errors import/usages =="
rg -n "\"errors\"" "$file" || true
rg -n "\berrors\." "$file" || trueRepository: tbphp/gpt-load
Length of output: 96
Use errors.Is for the not-found mapping.
UpdateKeyEnabled maps not-found by checking strings.Contains(err.Error(), "record not found"), but KeyProvider.SetKeyEnabled wraps the underlying GORM error with %w, so a stable check is errors.Is(err, gorm.ErrRecordNotFound) (and add the missing errors import).
Suggested fix
import (
+ "errors"
"fmt"
app_errors "gpt-load/internal/errors"
@@
key, err := s.KeyService.SetKeyEnabled(uint(keyID), req.Enabled)
if err != nil {
- if strings.Contains(err.Error(), "record not found") {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
response.Error(c, app_errors.ErrResourceNotFound)
} else {
response.Error(c, app_errors.ParseDBError(err))
}📝 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.
| key, err := s.KeyService.SetKeyEnabled(uint(keyID), req.Enabled) | |
| if err != nil { | |
| if strings.Contains(err.Error(), "record not found") { | |
| response.Error(c, app_errors.ErrResourceNotFound) | |
| } else { | |
| response.Error(c, app_errors.ParseDBError(err)) | |
| } | |
| key, err := s.KeyService.SetKeyEnabled(uint(keyID), req.Enabled) | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| response.Error(c, app_errors.ErrResourceNotFound) | |
| } else { | |
| response.Error(c, app_errors.ParseDBError(err)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/key_handler.go` around lines 568 - 574, Replace the
string-based not-found check in UpdateKeyEnabled's error handling with a proper
errors.Is check: after calling s.KeyService.SetKeyEnabled, detect a missing
record by using errors.Is(err, gorm.ErrRecordNotFound) (and add the missing
"errors" import and ensure "gorm" is imported) and map that case to
response.Error(c, app_errors.ErrResourceNotFound); otherwise continue to call
response.Error(c, app_errors.ParseDBError(err)).
| if err := p.store.HSet(keyHashKey, p.apiKeyToMap(&key)); err != nil { | ||
| return fmt.Errorf("failed to update key enabled flag in store: %w", err) | ||
| } | ||
|
|
||
| if err := p.store.LRem(activeKeysListKey, 0, key.ID); err != nil { | ||
| return fmt.Errorf("failed to LRem key from active list: %w", err) | ||
| } | ||
|
|
||
| if isKeyAvailableForPool(&key) { | ||
| if err := p.store.LPush(activeKeysListKey, key.ID); err != nil { | ||
| return fmt.Errorf("failed to LPush key to active list: %w", err) | ||
| } |
There was a problem hiding this comment.
Fail closed when syncing the active pool on disable.
This updates the hash before removing the key from group:*:active_keys. If HSet succeeds and LRem fails, this call returns an error but the list still contains the key, so SelectKey can keep routing a supposedly disabled key. Remove from the active list first on the disable path, and only LPush after the hash says enabled on the enable path.
Suggested direction
- if err := p.store.HSet(keyHashKey, p.apiKeyToMap(&key)); err != nil {
- return fmt.Errorf("failed to update key enabled flag in store: %w", err)
- }
-
- if err := p.store.LRem(activeKeysListKey, 0, key.ID); err != nil {
- return fmt.Errorf("failed to LRem key from active list: %w", err)
- }
-
- if isKeyAvailableForPool(&key) {
+ if !enabled {
+ if err := p.store.LRem(activeKeysListKey, 0, key.ID); err != nil {
+ return fmt.Errorf("failed to LRem key from active list: %w", err)
+ }
+ }
+
+ if err := p.store.HSet(keyHashKey, p.apiKeyToMap(&key)); err != nil {
+ return fmt.Errorf("failed to update key enabled flag in store: %w", err)
+ }
+
+ if enabled && isKeyAvailableForPool(&key) {
if err := p.store.LPush(activeKeysListKey, key.ID); err != nil {
return fmt.Errorf("failed to LPush key to active list: %w", err)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/keypool/provider.go` around lines 642 - 653, The active-list sync
currently performs p.store.HSet(keyHashKey, p.apiKeyToMap(&key)) before removing
the key from the active list, which can leave a disabled key in
group:*:active_keys if LRem fails; change the logic so that on the disable path
you call p.store.LRem(activeKeysListKey, 0, key.ID) first and return on error,
then update the hash with p.store.HSet; conversely, on the enable path update
the hash via p.store.HSet(...) first and only then call
p.store.LPush(activeKeysListKey, key.ID) (using isKeyAvailableForPool to decide
the push) while preserving the existing error messages and returns for
p.store.LRem, p.store.HSet and p.store.LPush.
|
Codex Review（通过 Codex 的 review 生成） 结论：这个 PR 的功能是给每个 API key 增加独立的 合并建议：功能有实际价值，适合需要临时停用单个 key、保留 key 记录、不改变 invalid/blacklist 状态的场景。但当前补丁不建议直接合并，至少需要先修复下面的问题并跑完质量门禁。 必须修复：
建议一并修复：
验证建议：
整体判断：修复 P2 并发语义和 P3 格式问题后，这个功能可以考虑合并；如果项目暂时没有“单 key 临时下线”的明确需求，也可以延后合并。 |
2 participants
添加一个针对每个密钥的启用开关，用于控制活动密钥能否进入路由池，同时不改变现有的无效/黑名单状态逻辑。
Summary by CodeRabbit