feat: 用户组限制功能（并发数、RPM、RPD、TPM、TPD）和分组描述优化 - #2824
Conversation
- 新增 setting/group_limit.go: 用户组限制配置管理 - 新增 common/group_limiter.go: 限流器实现（内存/Redis） - 新增 middleware/group-limit.go: 用户组限制中间件 - 新增 web/src/pages/Setting/RateLimit/SettingsGroupLimit.jsx: 前端设置页面 - 修改 controller/user.go: GetSelf 添加 group_limits 和 group_description - 修改 controller/group.go: GetUserGroups 添加限制信息 - 修改 router/relay-router.go: 修复中间件执行顺序 - 修改前端组件: 数据看板、个人设置、令牌管理显示限制信息 - 新增 Custom-Upgrade.md: 自定义修改记录文档
WalkthroughThis pull request implements a comprehensive user-group rate-limiting system with dual storage backends (in-memory and Redis), integrating rate-limit middleware across API routes, extending backend data models and controllers to manage group descriptions and limits, and updating the frontend UI to display group-specific constraints and descriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant TokenAuth as Token Auth
participant GroupLimit as GroupLimit Middleware
participant Limiter as GroupLimiter<br/>(Memory/Redis)
participant Handler as Request Handler
participant DB as Database/Cache
Client->>Router: HTTP Request
Router->>TokenAuth: Pass Request
TokenAuth->>TokenAuth: Validate User Token
TokenAuth->>GroupLimit: Pass Authenticated Request
alt GroupLimitEnabled is true
GroupLimit->>GroupLimit: Extract UserID & Group
alt Check RPM/RPD/Concurrency
GroupLimit->>Limiter: CheckConcurrency(userID, limit)
Limiter->>DB: (Redis/Memory) Get current count
DB-->>Limiter: Return count
Limiter->>Limiter: Compare vs limit
alt Limit Exceeded
Limiter-->>GroupLimit: false
GroupLimit->>Client: 429 Rate Limit Error
else Limit OK
Limiter-->>GroupLimit: true
GroupLimit->>GroupLimit: Store ConcurrencyLocked=true
GroupLimit->>Handler: Continue
end
end
else GroupLimitEnabled is false
GroupLimit->>Handler: Pass Through
end
Handler->>Handler: Process Request (estimate tokens)
Handler->>GroupLimit: (via context) RecordGroupLimitTokens
GroupLimit->>Limiter: RecordTokens, CheckTPM, CheckTPD
Limiter->>DB: (Redis/Memory) Record/Update token counts
Handler-->>Client: Response
GroupLimit->>Limiter: ReleaseConcurrency(userID)
Limiter->>DB: Decrement concurrency counter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/option.go (1)
145-198:⚠️ Potential issue | 🟡 MinorAvoid mutating group descriptions before persistence.
UpdateGroupDescriptionByJSONStringmutates in‑memory state beforemodel.UpdateOptionpersists. If the DB update fails, runtime state may drift from persisted config. Prefer a validation‑only parse here (similar toGroupLimitConfigs), then letUpdateOptionapply the update after persistence.🛠️ Suggested adjustment
case "GroupDescription": - err = ratio_setting.UpdateGroupDescriptionByJSONString(option.Value.(string)) + var tmp map[string]string + err = json.Unmarshal([]byte(option.Value.(string)), &tmp) if err != nil { c.JSON(http.StatusOK, gin.H{ "success": false, "message": "分组描述设置失败: " + err.Error(), }) return }
🤖 Fix all issues with AI agents
In `@common/group_limiter.go`:
- Around line 496-542: The expiry for daily keys uses UTC-aligned truncation
causing early resets in non-UTC zones; update RecordTPD (and similarly
RecordRPD) to compute the next local-midnight using the current time's location
(e.g., now := time.Now(); tomorrow := time.Date(now.Year(), now.Month(),
now.Day()+1, 0, 0, 0, 0, now.Location())) and pass that to RDB.ExpireAt so the
key expires at local midnight instead of UTC midnight. Ensure the change is
applied to the ExpireAt call in RecordTPD and the equivalent ExpireAt in
RecordRPD.
In `@middleware/group-limit.go`:
- Around line 99-110: The ctx.ConcurrencyLocked flag is being set
unconditionally even when limiter.CheckConcurrency returns an error (fail-open);
change the logic in the middleware handling around limiter.CheckConcurrency so
that ctx.ConcurrencyLocked is set to true only when CheckConcurrency returns
(allowed == true && err == nil). Locate the block using
limiter.CheckConcurrency, ctx.ConcurrencyLocked, and abortWithGroupLimitError
and move or wrap the assignment to ctx.ConcurrencyLocked so it happens only
after a successful lock acquisition; ensure existing error-logging behavior
remains and that ReleaseConcurrency will only be invoked when
ctx.ConcurrencyLocked is true.
- Around line 33-39: The panic recovery defer unconditionally calls c.Next(),
which can re-run handlers if c.Next() already executed; introduce a boolean flag
(e.g., nextCalled) scoped in the middleware, set it true immediately before
calling c.Next(), and change the defer's recovery branch to only call c.Next()
when nextCalled is false; update references around the existing defer func, the
c.Next() invocation, and the panic logging (toString / common.SysLog) so the
recovery no longer double-invokes c.Next().
In `@test.sh`:
- Around line 2-10: The committed script exposes a hardcoded secret in the
API_KEY variable and uses it directly in the curl call; revoke/rotate the
exposed key immediately and change the script to read the key from an
environment variable (e.g., reference API_KEY from process env) instead of the
hardcoded string, and update the curl invocation to use that env variable
(remove the literal value assigned to API_KEY and replace usages of API_KEY in
the curl -H "Authorization: Bearer ..." header). Ensure the repo secret is
removed from history and any CI/CD or deployment docs are updated to instruct
setting the environment variable securely.
In `@web/src/pages/Setting/RateLimit/SettingsGroupLimit.jsx`:
- Around line 59-66: The Promise.all(requestQueue) resolution branch handles
failures inconsistently: when requestQueue.length === 1 and
res.includes(undefined) it returns silently, but for length > 1 it shows an
error via showError(t('部分保存失败，请重试')). Update the single-request branch in the
Promise.all.then handler so that when res.includes(undefined) it also calls
showError(t('部分保存失败，请重试')) (or a more specific single-item message) instead of
just returning; locate the Promise.all handling around the requestQueue variable
and adjust the conditional for the length === 1 case to mirror the error
reporting used for multiple requests.
🧹 Nitpick comments (5)
web/src/helpers/render.jsx (1)
820-907: Unify limit formatting to avoid drift across UI.
formatLimitValueduplicates logic already present inDashboardHeader.jsx, and the “∞” display diverges from the localized “无限制” elsewhere. Consider exporting the helper and standardizing the output so all limit displays stay consistent.♻️ Possible consolidation (in this file)
-const formatLimitValue = (value) => { - if (value === 0 || value === undefined || value === null) { - return '∞'; - } +export const formatLimitValue = (value) => { + if (value === 0 || value === undefined || value === null) { + return i18next.t('无限制'); + }middleware/group-limit.go (1)
289-302: Consider using fmt.Sprintf for better panic value logging.The
toStringhelper returns an empty string for panic values that aren'tstringorerrortypes. Panics can be any type (e.g.,int, struct), and this would lose potentially useful debugging information.Optional: Use fmt.Sprintf for broader type coverage
+import "fmt" + // toString 安全地将 interface{} 转换为字符串 func toString(v interface{}) string { if v == nil { return "" } switch val := v.(type) { case string: return val case error: return val.Error() default: - return "" + return fmt.Sprintf("%v", val) } }setting/group_limit.go (2)
10-11: GuardGroupLimitEnabledif it can change at runtime.If this flag is toggled after startup, unsynchronized reads/writes can race. Consider
atomic.Boolor guarding updates with a settings mutex.
68-99: Validate non‑negative limits when ingesting JSON.Negative values currently slip through and are treated as “no limit” (
limit <= 0), which is easy to misconfigure. Consider rejecting negatives during update (and mirroring the same check inValidateGroupLimitConfigsJSON).♻️ Proposed validation in the update path
import ( "encoding/json" + "fmt" "sync" "github.com/QuantumNous/new-api/common" ) @@ newConfigs := make(map[string]GroupLimitConfig) err := json.Unmarshal([]byte(jsonStr), &newConfigs) if err != nil { return err } + for group, cfg := range newConfigs { + if cfg.Concurrency < 0 || cfg.RPM < 0 || cfg.RPD < 0 || cfg.TPM < 0 || cfg.TPD < 0 { + return fmt.Errorf("group %s has negative limits", group) + } + } groupLimitConfigs = newConfigs return nil }common/group_limiter.go (1)
466-494: Consider atomic Redis rate‑limit checks to avoid overshoot.
CheckRPMandCheckTPMperform read‑then‑write sequences; under concurrency the limit can be exceeded. If strict enforcement is required, consider a Lua script (ZADD/ZREMRANGEBYSCORE/ZCARD) or MULTI/EXEC to make the check+record+trim atomic.Also applies to: 544-601
| func (l *RedisGroupLimiter) CheckTPD(userID int, limit int64, tokens int64) (bool, error) { | ||
| if limit <= 0 { | ||
| return true, nil | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| today := time.Now().Format("2006-01-02") | ||
| key := l.tpdKey(userID, today) | ||
|
|
||
| // 获取当前令牌数 | ||
| currentStr, err := RDB.Get(ctx, key).Result() | ||
| var current int64 = 0 | ||
| if err == nil { | ||
| current, _ = strconv.ParseInt(currentStr, 10, 64) | ||
| } | ||
|
|
||
| // 检查是否超过限制 | ||
| if current+tokens > limit { | ||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| // RecordTPD 记录每日令牌使用量 | ||
| func (l *RedisGroupLimiter) RecordTPD(userID int, tokens int64) error { | ||
| if tokens <= 0 { | ||
| return nil | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| today := time.Now().Format("2006-01-02") | ||
| key := l.tpdKey(userID, today) | ||
|
|
||
| // 增加令牌计数 | ||
| _, err := RDB.IncrBy(ctx, key, tokens).Result() | ||
| if err != nil { | ||
| SysLog("Redis error in RecordTPD: " + err.Error()) | ||
| return err | ||
| } | ||
|
|
||
| // 设置过期时间为明天 | ||
| tomorrow := time.Now().Add(24 * time.Hour).Truncate(24 * time.Hour) | ||
| RDB.ExpireAt(ctx, key, tomorrow) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "group_limiter.go" -type fRepository: QuantumNous/new-api
Length of output: 88
🏁 Script executed:
# Once we find the file, let's look at the specific lines mentioned
# and also check lines 603-646 to see if similar code exists
find . -name "group_limiter.go" -path "*/common/*"Repository: QuantumNous/new-api
Length of output: 88
🏁 Script executed:
# Get the overall structure of the repo
git ls-files | grep -E "group_limiter|group.*limiter" | head -20Repository: QuantumNous/new-api
Length of output: 86
🏁 Script executed:
# Search for the file more broadly
find . -type f -name "*group*limiter*" 2>/dev/nullRepository: QuantumNous/new-api
Length of output: 88
🏁 Script executed:
# Also search for go files in common directory
find . -type f -path "*/common/*.go" 2>/dev/null | head -20Repository: QuantumNous/new-api
Length of output: 512
🏁 Script executed:
# Read the file to see the relevant sections
cat -n ./common/group_limiter.go | head -100Repository: QuantumNous/new-api
Length of output: 3120
🏁 Script executed:
# Check the total lines in the file
wc -l ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 92
🏁 Script executed:
# Now let's read around lines 496-542 to see the code
sed -n '490,550p' ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 1376
🏁 Script executed:
# And check lines 603-646 mentioned in "Also applies to"
sed -n '595,655p' ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 1396
🏁 Script executed:
# Let's verify the exact behavior of time.Truncate() with a test
cat > /tmp/time_test.go << 'EOF'
package main
import (
"fmt"
"time"
)
func main() {
// Simulate the current code logic
now := time.Now()
// Method 1: Current approach (truncating 24 hours from now)
tomorrow_current := now.Add(24 * time.Hour).Truncate(24 * time.Hour)
// Method 2: Proposed approach (local midnight)
tomorrow_proposed := time.Date(now.Year(), now.Month(), now.Day()+1, 0, 0, 0, 0, now.Location())
// Show keys used
today_key := now.Format("2006-01-02")
fmt.Printf("Current time: %v\n", now)
fmt.Printf("Local timezone: %v\n", now.Location())
fmt.Printf("Today's key (local): %s\n", today_key)
fmt.Printf("Current method expiry: %v (zone: %v)\n", tomorrow_current, tomorrow_current.Location())
fmt.Printf("Proposed method expiry: %v (zone: %v)\n", tomorrow_proposed, tomorrow_proposed.Location())
fmt.Printf("Difference: %v\n", tomorrow_proposed.Sub(tomorrow_current))
}
EOF
go run /tmp/time_test.goRepository: QuantumNous/new-api
Length of output: 113
🏁 Script executed:
# Let's also check if there are any other similar patterns in the file
grep -n "Truncate(24 \* time.Hour)" ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 208
🏁 Script executed:
# Check for any timezone-related imports or configurations
grep -n -E "timezone|location|UTC|Local" ./common/group_limiter.go | head -20Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Let me write a Python script to simulate the time behavior
python3 << 'EOF'
from datetime import datetime, timedelta
import pytz
# Test various timezones
timezones = ['UTC', 'Asia/Shanghai', 'America/New_York', 'Europe/London']
for tz_name in timezones:
tz = pytz.timezone(tz_name)
# Create a time: 2025-02-15 04:00:00 in that timezone
test_time = tz.localize(datetime(2025, 2, 15, 4, 0, 0))
# What the current code does:
# Add 24 hours, then truncate to 24-hour boundary
# (Truncate always goes to UTC epoch multiples: midnight UTC)
tomorrow_24h = test_time + timedelta(hours=24)
# Calculate what truncate(24h) does: rounds down to UTC midnight
# Get the Unix timestamp and round down to nearest 86400-second boundary
ts = tomorrow_24h.timestamp()
truncated_ts = int(ts // 86400) * 86400 # Rounds DOWN to nearest 24-hour boundary
truncated_time = datetime.utcfromtimestamp(truncated_ts)
# Convert back to local time
truncated_local = tz.normalize(tz.localize(truncated_time).astimezone(tz))
# What the proposed code does: midnight in local timezone
today_local = test_time.replace(hour=0, minute=0, second=0, microsecond=0)
proposed_tomorrow = today_local + timedelta(days=1)
# Format the key like in the code
key_date = test_time.strftime("%Y-%m-%d")
print(f"\n{tz_name}:")
print(f" Current time: {test_time}")
print(f" Date key: {key_date}")
print(f" Current code expiry: {truncated_local} (UTC: {datetime.utcfromtimestamp(truncated_ts)})")
print(f" Proposed expiry: {proposed_tomorrow}")
print(f" Match: {truncated_local.date() == proposed_tomorrow.date()}")
EOFRepository: QuantumNous/new-api
Length of output: 179
🏁 Script executed:
# Install pytz first
pip install pytz -q && python3 << 'EOF'
from datetime import datetime, timedelta
import pytz
# Test various timezones
timezones = ['UTC', 'Asia/Shanghai', 'America/New_York', 'Europe/London']
for tz_name in timezones:
tz = pytz.timezone(tz_name)
# Create a time: 2025-02-15 04:00:00 in that timezone
test_time = tz.localize(datetime(2025, 2, 15, 4, 0, 0))
# What the current code does:
# Add 24 hours, then truncate to 24-hour boundary
# Truncate(24h) in Go rounds DOWN to the nearest 24-hour boundary from Unix epoch
tomorrow_24h = test_time + timedelta(hours=24)
# Truncate to 24-hour boundary (nearest multiple of 86400 seconds from Unix epoch)
ts = tomorrow_24h.timestamp()
truncated_ts = int(ts // 86400) * 86400
# Convert back to UTC and then to local timezone
truncated_utc = datetime.utcfromtimestamp(truncated_ts)
truncated_local = truncated_utc.replace(tzinfo=pytz.UTC).astimezone(tz)
# What the proposed code does: midnight in local timezone
today_local = test_time.replace(hour=0, minute=0, second=0, microsecond=0)
proposed_tomorrow = today_local + timedelta(days=1)
# Format the key like in the code
key_date = test_time.strftime("%Y-%m-%d")
print(f"\n{tz_name}:")
print(f" Current time: {test_time}")
print(f" Date key (local): {key_date}")
print(f" Current code expiry: {truncated_local}")
print(f" Proposed expiry: {proposed_tomorrow}")
print(f" Expiry off by: {(proposed_tomorrow - truncated_local.replace(tzinfo=None)).total_seconds() / 3600:.1f} hours")
EOFRepository: QuantumNous/new-api
Length of output: 406
🏁 Script executed:
pip install pytz -q && python3 << 'EOF'
from datetime import datetime, timedelta
import pytz
# Test various timezones
timezones = ['UTC', 'Asia/Shanghai', 'America/New_York', 'Europe/London']
for tz_name in timezones:
tz = pytz.timezone(tz_name)
# Create a time: 2025-02-15 04:00:00 in that timezone
test_time = tz.localize(datetime(2025, 2, 15, 4, 0, 0))
# What the current code does:
# Add 24 hours, then truncate to 24-hour boundary
tomorrow_24h = test_time + timedelta(hours=24)
# Truncate to 24-hour boundary (nearest multiple of 86400 seconds from Unix epoch)
ts = tomorrow_24h.timestamp()
truncated_ts = int(ts // 86400) * 86400
# Convert back to UTC and then to local timezone
truncated_utc = datetime.utcfromtimestamp(truncated_ts)
truncated_local = truncated_utc.replace(tzinfo=pytz.UTC).astimezone(tz)
# What the proposed code does: midnight in local timezone
today_local = test_time.replace(hour=0, minute=0, second=0, microsecond=0)
proposed_tomorrow = today_local + timedelta(days=1)
# Format the key like in the code
key_date = test_time.strftime("%Y-%m-%d")
# Calculate difference in hours
diff_seconds = (proposed_tomorrow.replace(tzinfo=None) - truncated_local.replace(tzinfo=None)).total_seconds()
print(f"\n{tz_name}:")
print(f" Current time: {test_time}")
print(f" Date key (local): {key_date}")
print(f" Current code expiry: {truncated_local}")
print(f" Proposed expiry: {proposed_tomorrow}")
if abs(diff_seconds) > 0:
print(f" ⚠️ Expiry off by: {diff_seconds / 3600:.1f} hours")
else:
print(f" ✓ Expiry matches")
EOFRepository: QuantumNous/new-api
Length of output: 950
🏁 Script executed:
# Let's check how RDB is configured and if there's any timezone configuration
grep -n "RDB" ./common/group_limiter.go | head -10Repository: QuantumNous/new-api
Length of output: 603
🏁 Script executed:
# Check if there's any Redis-related code that might use timezone info
grep -n "var RDB\|func.*RDB\|RDB.*=" ./common/group_limiter.go | head -5Repository: QuantumNous/new-api
Length of output: 99
🏁 Script executed:
# Find the RDB initialization to understand the context better
fd -e go -x grep -l "var RDB" {} \; | head -5Repository: QuantumNous/new-api
Length of output: 80
🏁 Script executed:
# Look for any timezone initialization in the codebase
rg "time\.LoadLocation|timezone|TIMEZONE" --type go | head -10Repository: QuantumNous/new-api
Length of output: 420
🏁 Script executed:
# Let's verify the exact code at lines 496-542 to match the review comment
sed -n '496,542p' ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 1053
🏁 Script executed:
# And verify lines 603-646 mentioned in "Also applies to"
sed -n '603,646p' ./common/group_limiter.goRepository: QuantumNous/new-api
Length of output: 978
Daily limit keys reset early in non-UTC timezones due to UTC-aligned expiry.
Date keys use local time (Format("2006-01-02")), but expiry is set via time.Now().Add(24*time.Hour).Truncate(24*time.Hour), which truncates to UTC midnight boundaries. In timezones east of UTC (e.g., Shanghai +08:00), keys expire ~16 hours early; in timezones west of UTC (e.g., New York -05:00), they expire ~5 hours early. This allows daily limits to reset prematurely and be exceeded. Use local midnight instead:
Proposed fix
- tomorrow := time.Now().Add(24 * time.Hour).Truncate(24 * time.Hour)
- RDB.ExpireAt(ctx, key, tomorrow)
+ now := time.Now()
+ tomorrow := time.Date(now.Year(), now.Month(), now.Day()+1, 0, 0, 0, 0, now.Location())
+ RDB.ExpireAt(ctx, key, tomorrow)Applies to both RecordTPD (lines 538-541) and RecordRPD (lines 642-645).
🤖 Prompt for AI Agents
In `@common/group_limiter.go` around lines 496 - 542, The expiry for daily keys
uses UTC-aligned truncation causing early resets in non-UTC zones; update
RecordTPD (and similarly RecordRPD) to compute the next local-midnight using the
current time's location (e.g., now := time.Now(); tomorrow :=
time.Date(now.Year(), now.Month(), now.Day()+1, 0, 0, 0, 0, now.Location())) and
pass that to RDB.ExpireAt so the key expires at local midnight instead of UTC
midnight. Ensure the change is applied to the ExpireAt call in RecordTPD and the
equivalent ExpireAt in RecordRPD.
| // 安全地执行限流检查，任何错误都不影响原有功能 | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| common.SysLog("GroupLimit panic recovered: " + toString(r)) | ||
| c.Next() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Panic recovery after c.Next() causes double-handling.
The defer with panic recovery calls c.Next() if a panic occurs. However, if the panic happens after c.Next() has already been called (e.g., during concurrency release at line 120-122), this would call c.Next() again, potentially causing issues with response handling.
Proposed fix to track whether Next was called
func GroupLimit() gin.HandlerFunc {
return func(c *gin.Context) {
// 检查功能是否启用
if !setting.GroupLimitEnabled {
c.Next()
return
}
+ nextCalled := false
// 安全地执行限流检查，任何错误都不影响原有功能
defer func() {
if r := recover(); r != nil {
common.SysLog("GroupLimit panic recovered: " + toString(r))
- c.Next()
+ if !nextCalled {
+ c.Next()
+ }
}
}()
// ... (skip to line 115-116)
// 存储上下文信息
c.Set(groupLimitContextKey, ctx)
// 处理请求
+ nextCalled = true
c.Next()🤖 Prompt for AI Agents
In `@middleware/group-limit.go` around lines 33 - 39, The panic recovery defer
unconditionally calls c.Next(), which can re-run handlers if c.Next() already
executed; introduce a boolean flag (e.g., nextCalled) scoped in the middleware,
set it true immediately before calling c.Next(), and change the defer's recovery
branch to only call c.Next() when nextCalled is false; update references around
the existing defer func, the c.Next() invocation, and the panic logging
(toString / common.SysLog) so the recovery no longer double-invokes c.Next().
| // 检查并发数限制 | ||
| if config.Concurrency > 0 { | ||
| allowed, err := limiter.CheckConcurrency(userID, config.Concurrency) | ||
| if err != nil { | ||
| common.SysLog("GroupLimit CheckConcurrency error: " + err.Error()) | ||
| // 错误时允许通过 | ||
| } else if !allowed { | ||
| abortWithGroupLimitError(c, "concurrency", config.Concurrency) | ||
| return | ||
| } | ||
| ctx.ConcurrencyLocked = true | ||
| } |
There was a problem hiding this comment.
Concurrency lock flag set incorrectly when CheckConcurrency errors.
When CheckConcurrency returns an error, the code logs it and continues (fail-open). However, ctx.ConcurrencyLocked is still set to true at line 109, even though no lock was actually acquired. This will cause ReleaseConcurrency to be called unnecessarily (or incorrectly decrement a counter).
Proposed fix to only set flag when lock is acquired
// 检查并发数限制
if config.Concurrency > 0 {
allowed, err := limiter.CheckConcurrency(userID, config.Concurrency)
if err != nil {
common.SysLog("GroupLimit CheckConcurrency error: " + err.Error())
// 错误时允许通过
} else if !allowed {
abortWithGroupLimitError(c, "concurrency", config.Concurrency)
return
+ } else {
+ ctx.ConcurrencyLocked = true
}
- ctx.ConcurrencyLocked = true
}📝 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.
| // 检查并发数限制 | |
| if config.Concurrency > 0 { | |
| allowed, err := limiter.CheckConcurrency(userID, config.Concurrency) | |
| if err != nil { | |
| common.SysLog("GroupLimit CheckConcurrency error: " + err.Error()) | |
| // 错误时允许通过 | |
| } else if !allowed { | |
| abortWithGroupLimitError(c, "concurrency", config.Concurrency) | |
| return | |
| } | |
| ctx.ConcurrencyLocked = true | |
| } | |
| // 检查并发数限制 | |
| if config.Concurrency > 0 { | |
| allowed, err := limiter.CheckConcurrency(userID, config.Concurrency) | |
| if err != nil { | |
| common.SysLog("GroupLimit CheckConcurrency error: " + err.Error()) | |
| // 错误时允许通过 | |
| } else if !allowed { | |
| abortWithGroupLimitError(c, "concurrency", config.Concurrency) | |
| return | |
| } else { | |
| ctx.ConcurrencyLocked = true | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@middleware/group-limit.go` around lines 99 - 110, The ctx.ConcurrencyLocked
flag is being set unconditionally even when limiter.CheckConcurrency returns an
error (fail-open); change the logic in the middleware handling around
limiter.CheckConcurrency so that ctx.ConcurrencyLocked is set to true only when
CheckConcurrency returns (allowed == true && err == nil). Locate the block using
limiter.CheckConcurrency, ctx.ConcurrencyLocked, and abortWithGroupLimitError
and move or wrap the assignment to ctx.ConcurrencyLocked so it happens only
after a successful lock acquisition; ensure existing error-logging behavior
remains and that ReleaseConcurrency will only be invoked when
ctx.ConcurrencyLocked is true.
| API_URL="https://www.fcloud.net/v1/chat/completions" | ||
| API_KEY="sk-O2syV2QtPZGQHkMiXgrpi2JeXhWqzGBPmLksQTus4tsoXBGN" | ||
|
|
||
| # 同时发送3个请求（假设限制为2） | ||
| for i in {1..6}; do | ||
| curl -s "$API_URL" \ | ||
| -H "Authorization: Bearer $API_KEY" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"model":"claude-opus-4-5","messages":[{"role":"user","content":"hi"}]}' & |
There was a problem hiding this comment.
Remove the hardcoded API key and rotate it immediately.
Committing an API key to source control is a critical security leak. Please revoke/rotate the exposed key and load secrets from the environment instead.
🔒 Suggested fix
-API_URL="https://www.fcloud.net/v1/chat/completions"
-API_KEY="sk-O2syV2QtPZGQHkMiXgrpi2JeXhWqzGBPmLksQTus4tsoXBGN"
+API_URL="${API_URL:-https://www.fcloud.net/v1/chat/completions}"
+: "${API_KEY:?Set API_KEY in environment}"📝 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.
| API_URL="https://www.fcloud.net/v1/chat/completions" | |
| API_KEY="sk-O2syV2QtPZGQHkMiXgrpi2JeXhWqzGBPmLksQTus4tsoXBGN" | |
| # 同时发送3个请求（假设限制为2） | |
| for i in {1..6}; do | |
| curl -s "$API_URL" \ | |
| -H "Authorization: Bearer $API_KEY" \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"model":"claude-opus-4-5","messages":[{"role":"user","content":"hi"}]}' & | |
| API_URL="${API_URL:-https://www.fcloud.net/v1/chat/completions}" | |
| : "${API_KEY:?Set API_KEY in environment}" | |
| # 同时发送3个请求（假设限制为2） | |
| for i in {1..6}; do | |
| curl -s "$API_URL" \ | |
| -H "Authorization: Bearer $API_KEY" \ | |
| -H "Content-Type: application/json" \ | |
| -d '{"model":"claude-opus-4-5","messages":[{"role":"user","content":"hi"}]}' & |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 6-6: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
In `@test.sh` around lines 2 - 10, The committed script exposes a hardcoded secret
in the API_KEY variable and uses it directly in the curl call; revoke/rotate the
exposed key immediately and change the script to read the key from an
environment variable (e.g., reference API_KEY from process env) instead of the
hardcoded string, and update the curl invocation to use that env variable
(remove the literal value assigned to API_KEY and replace usages of API_KEY in
the curl -H "Authorization: Bearer ..." header). Ensure the repo secret is
removed from history and any CI/CD or deployment docs are updated to instruct
setting the environment variable securely.
| Promise.all(requestQueue) | ||
| .then((res) => { | ||
| if (requestQueue.length === 1) { | ||
| if (res.includes(undefined)) return; | ||
| } else if (requestQueue.length > 1) { | ||
| if (res.includes(undefined)) | ||
| return showError(t('部分保存失败，请重试')); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for single request failure.
When requestQueue.length === 1 and res.includes(undefined), the code returns silently without showing an error. In contrast, when multiple requests fail, an error is displayed. This inconsistency may confuse users.
Proposed fix to show error on single request failure
.then((res) => {
if (requestQueue.length === 1) {
- if (res.includes(undefined)) return;
+ if (res.includes(undefined)) return showError(t('保存失败'));
} else if (requestQueue.length > 1) {
if (res.includes(undefined))
return showError(t('部分保存失败，请重试'));
}📝 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.
| Promise.all(requestQueue) | |
| .then((res) => { | |
| if (requestQueue.length === 1) { | |
| if (res.includes(undefined)) return; | |
| } else if (requestQueue.length > 1) { | |
| if (res.includes(undefined)) | |
| return showError(t('部分保存失败，请重试')); | |
| } | |
| Promise.all(requestQueue) | |
| .then((res) => { | |
| if (requestQueue.length === 1) { | |
| if (res.includes(undefined)) return showError(t('保存失败')); | |
| } else if (requestQueue.length > 1) { | |
| if (res.includes(undefined)) | |
| return showError(t('部分保存失败，请重试')); | |
| } |
🤖 Prompt for AI Agents
In `@web/src/pages/Setting/RateLimit/SettingsGroupLimit.jsx` around lines 59 - 66,
The Promise.all(requestQueue) resolution branch handles failures inconsistently:
when requestQueue.length === 1 and res.includes(undefined) it returns silently,
but for length > 1 it shows an error via showError(t('部分保存失败，请重试')). Update the
single-request branch in the Promise.all.then handler so that when
res.includes(undefined) it also calls showError(t('部分保存失败，请重试')) (or a more
specific single-item message) instead of just returning; locate the Promise.all
handling around the requestQueue variable and adjust the conditional for the
length === 1 case to mirror the error reporting used for multiple requests.
2 participants
Summary by CodeRabbit
Release Notes