Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough在数据库、后端、前端、Redis 与速率限制服务中新增并接入“并发 UA(User‑Agent)上限”字段与逻辑:包括迁移、类型/验证、API/表单、代理层并发检查、Redis 原子追踪、补丁契约以及单元测试覆盖(均为增量更改)。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求全面引入了一个新的多层级并发 User-Agent (UA) 限流系统,旨在更精细地控制不同实体(用户、密钥、提供商)的活跃 UA 数量。通过在数据库中添加相应字段、更新 Redis 存储和处理逻辑,以及在用户界面中提供配置选项,该功能增强了系统的资源管理能力,并确保在 UA 限制达到时能优雅地进行服务降级或回退,从而提升了系统的稳定性和可控性。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| if (!uaCheckResult.allowed) { | ||
| logger.warn("ProviderSelector: Provider concurrent UA limit exceeded, trying fallback", { | ||
| providerName: session.provider.name, | ||
| providerId: session.provider.id, | ||
| current: uaCheckResult.count, | ||
| limit: uaLimit, | ||
| attempt: attemptCount, | ||
| }); | ||
|
|
||
| const failedContext = session.getLastSelectionContext(); | ||
| session.addProviderToChain(session.provider, { | ||
| reason: "concurrent_limit_failed", | ||
| selectionMethod: failedContext?.groupFilterApplied | ||
| ? "group_filtered" | ||
| : "weighted_random", | ||
| circuitState: getCircuitState(session.provider.id), | ||
| attemptNumber: attemptCount, | ||
| errorMessage: uaCheckResult.reason || "并发 UA 限制已达到", | ||
| decisionContext: failedContext | ||
| ? { | ||
| ...failedContext, | ||
| concurrentLimit: uaLimit, | ||
| currentConcurrent: uaCheckResult.count, | ||
| } | ||
| : { | ||
| totalProviders: 0, | ||
| enabledProviders: 0, | ||
| targetType: session.provider.providerType as NonNullable< | ||
| ProviderChainItem["decisionContext"] | ||
| >["targetType"], | ||
| requestedModel: session.getOriginalModel() || "", | ||
| groupFilterApplied: false, | ||
| beforeHealthCheck: 0, | ||
| afterHealthCheck: 0, | ||
| priorityLevels: [], | ||
| selectedPriority: 0, | ||
| candidatesAtPriority: [], | ||
| concurrentLimit: uaLimit, | ||
| currentConcurrent: uaCheckResult.count, | ||
| }, | ||
| }); | ||
|
|
||
| excludedProviders.push(session.provider.id); | ||
|
|
||
| const { provider: fallbackProvider, context: retryContext } = | ||
| await ProxyProviderResolver.pickRandomProvider(session, excludedProviders); | ||
|
|
||
| if (!fallbackProvider) { | ||
| logger.error("ProviderSelector: No fallback providers available", { | ||
| excludedCount: excludedProviders.length, | ||
| totalAttempts: attemptCount, | ||
| }); | ||
| break; | ||
| } | ||
|
|
||
| session.setProvider(fallbackProvider); | ||
| session.setLastSelectionContext(retryContext); | ||
| continue; | ||
| } |
| if (!uaConcurrentCheck.allowed) { | ||
| const rejectedBy = uaConcurrentCheck.rejectedBy ?? "key"; | ||
| const fallbackCurrentUsage = | ||
| rejectedBy === "user" ? uaConcurrentCheck.userCount : uaConcurrentCheck.keyCount; | ||
| const fallbackLimitValue = | ||
| rejectedBy === "user" ? normalizedUserUaConcurrentLimit : effectiveKeyUaConcurrentLimit; | ||
| const currentUsage = Number(uaConcurrentCheck.reasonParams?.current); | ||
| const limitValue = Number(uaConcurrentCheck.reasonParams?.limit); | ||
| const resolvedCurrentUsage = Number.isFinite(currentUsage) | ||
| ? currentUsage | ||
| : fallbackCurrentUsage; | ||
| const resolvedLimitValue = Number.isFinite(limitValue) ? limitValue : fallbackLimitValue; | ||
|
|
||
| logger.warn( | ||
| `[RateLimit] ${rejectedBy === "user" ? "User" : "Key"} UA limit exceeded: key=${key.id}, user=${user.id}, current=${resolvedCurrentUsage}, limit=${resolvedLimitValue}` | ||
| ); | ||
|
|
||
| const resetTime = new Date().toISOString(); | ||
|
|
||
| const { getLocale } = await import("next-intl/server"); | ||
| const locale = await getLocale(); | ||
| const message = await getErrorMessageServer( | ||
| locale, | ||
| uaConcurrentCheck.reasonCode ?? ERROR_CODES.RATE_LIMIT_CONCURRENT_UAS_EXCEEDED, | ||
| { | ||
| current: String(resolvedCurrentUsage), | ||
| limit: String(resolvedLimitValue), | ||
| } | ||
| ); | ||
|
|
||
| throw new RateLimitError( | ||
| "rate_limit_error", | ||
| message, | ||
| "concurrent_uas", | ||
| resolvedCurrentUsage, | ||
| resolvedLimitValue, | ||
| resetTime, | ||
| null | ||
| ); | ||
| } |
| const result = (await RateLimitService.redis.eval( | ||
| CHECK_AND_TRACK_KEY_USER_SESSION, | ||
| 3, // KEYS count |
There was a problem hiding this comment.
Code Review Summary
This PR implements a multi-level concurrent User-Agent (UA) rate limiting feature across User/Key/Provider dimensions. The implementation is well-structured with proper atomic Redis operations for race condition prevention, comprehensive test coverage, and i18n support for all 5 languages.
PR Size: XL
- Lines changed: 5342 (5319 additions, 23 deletions)
- Files changed: 76
Note: This is a large PR. Consider splitting future PRs of this size into smaller, focused changes for easier review:
- Core rate-limit logic (concurrent-ua-limit.ts, service.ts changes)
- Redis integration (active-ua-keys.ts)
- UI/Dashboard changes
- i18n updates
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (Fail Open pattern with logging)
- Type safety - Clean (proper null/undefined handling)
- Documentation accuracy - Clean
- Test coverage - Adequate (new tests for all core functions)
- Code clarity - Good
Notable Implementation Details
-
Atomic Operations: Uses Lua scripts for atomic check-and-track operations to prevent race conditions in concurrent scenarios.
-
Redis Cluster Compatibility: Uses hash tags (
{active_uas}) for Key/User/Global UA keys to ensure same-slot placement in Redis Cluster. -
Fail Open Pattern: When Redis is unavailable, requests are allowed through (Fail Open) rather than blocked, ensuring availability.
-
Provider Fallback: When a provider hits its concurrent UA limit, the system automatically falls back to the next available provider without session count leakage.
-
UA Identity Resolution: Uses SHA256 hash of parsed client type to create stable UA identifiers, avoiding memory issues from long UA strings.
Automated review by Claude AI
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/provider-selector.ts (1)
282-423:⚠️ Potential issue | 🟠 MajorUA 先追踪后 Session 失败时会留下“幽灵 UA”计数。
当前流程先执行
checkAndTrackProviderUa,若随后checkAndTrackProviderSession失败并切换 fallback,前一个 provider 上新追踪到的 UA 没有回滚,会导致 UA 计数虚高并误触发限流。🛠 建议修复(在 session 检查失败分支回滚 UA,或合并为单个原子脚本)
const checkResult = await RateLimitService.checkAndTrackProviderSession( session.provider.id, session.sessionId, limit ); if (!checkResult.allowed) { + // 回滚本次新追踪的 UA,避免 fallback 后遗留 UA 计数 + if (uaCheckResult.tracked) { + void RateLimitService.untrackProviderUa(session.provider.id, uaId); + } + logger.warn( "ProviderSelector: Provider concurrent session limit exceeded, trying fallback", { providerName: session.provider.name,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/provider-selector.ts` around lines 282 - 423, When RateLimitService.checkAndTrackProviderUa succeeds but the subsequent RateLimitService.checkAndTrackProviderSession fails, the UA counter on the previous provider is never decremented causing "ghost UA" counts; update the session failure branch (the block handling !checkResult.allowed) to undo the prior UA tracking by calling the corresponding rollback/decrement on the UA tracker (e.g., a new RateLimitService.rollbackProviderUa or RateLimitService.decrementProviderUa with session.provider.id and uaId) before adding provider to chain and switching provider, or alternatively replace the two-step calls with a single atomic operation in RateLimitService (e.g., checkAndTrackProviderUaAndSession) that performs both checks/updates transactionally; locate fixes around RateLimitService.checkAndTrackProviderUa, RateLimitService.checkAndTrackProviderSession, and the failure handling code that calls session.addProviderToChain/session.setProvider to ensure UA is reverted on session-check failure.
🧹 Nitpick comments (5)
src/lib/redis/active-ua-keys.ts (1)
20-28: 可选:为keyId/userId增加整数边界保护。当前签名是
number,若上游误传NaN/Infinity/小数,会生成异常 key(如:NaN:)。建议在此层或调用边界做Number.isSafeInteger校验,降低线上排查成本。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/active-ua-keys.ts` around lines 20 - 28, The functions getKeyActiveUasKey and getUserActiveUasKey currently accept a number and can produce invalid keys if passed NaN/Infinity/floats; add input validation at the top of each function using Number.isSafeInteger(keyId) / Number.isSafeInteger(userId) and throw a clear TypeError (e.g., "getKeyActiveUasKey: keyId must be a safe integer" / "getUserActiveUasKey: userId must be a safe integer") so callers surface incorrect values early; alternatively, if you prefer coercion, explicitly coerce with Math.trunc after checking finite and log a warning—ensure the chosen behavior is consistent for both functions.src/app/[locale]/settings/providers/_components/forms/provider-form.legacy.tsx (1)
1521-1535: 新增 UA 限制输入后,折叠摘要未展示该值。Line [1521] 新增了输入项,但上方 rate-limit 摘要(Line [1320] 附近)仍未包含
limitConcurrentUas,用户折叠后无法快速确认该配置。♻️ 建议补齐摘要展示
@@ if (limitConcurrentSessions) limits.push( t("sections.rateLimit.summary.concurrent", { count: limitConcurrentSessions, }) ); + if (limitConcurrentUas) + limits.push( + t("sections.rateLimit.summary.concurrentUas", { + count: limitConcurrentUas, + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form.legacy.tsx around lines 1521 - 1535, The rate-limit collapsed summary does not include the newly added limitConcurrentUas value; update the summary renderer (the JSX/logic that builds the rate-limit summary near the existing summary code around where other rate-limit fields are displayed) to read the limitConcurrentUas state and include it in the collapsed text (use limitConcurrentUas?.toString() or a fallback like "—"/placeholder when empty), ensuring the displayed label matches t("sections.rateLimit.limitConcurrentUas.label") so users can see the configured concurrent UA limit when the section is collapsed; update any summaryFormatting helper or conditional that collects other fields (e.g., maxTokens, requestsPerMinute) to also include limitConcurrentUas.src/types/key.ts (1)
26-26: 建议补充limitConcurrentUas的语义注释(0 / undefined)新增字段本身没问题,但建议在类型旁明确
0与undefined的业务含义,避免后续在继承逻辑里被误用。Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined.
Also applies to: 59-60, 85-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/key.ts` at line 26, Add a clear JSDoc comment for the limitConcurrentUas property in the interface explaining the distinct meanings: explicitly state that 0 means “disable concurrency” (or “no concurrent UAs allowed”), undefined means “inherit default or upstream setting”, and that callers must not treat them interchangeably; update the type if desirable to number | undefined (or a branded type) to make absence explicit; apply the same JSDoc + type clarification to the other analogous fields mentioned in the review so their semantics (0 vs undefined) are unambiguous in interfaces like limitConcurrentUas.tests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts (1)
115-133: 建议:增强 mock session 对象的类型安全当前 session 对象使用
any类型。考虑从ProxySession类型中提取接口或使用Partial<ProxySession>来提高类型安全性,便于未来维护时捕获接口变更。可选改进
// 可以考虑提取 session mock 工厂函数供其他测试复用 function createMockSession(overrides: Partial<{ sessionId: string; userAgent: string; // ... other fields }> = {}) { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts` around lines 115 - 133, Replace the ad-hoc any-typed session mock with a typed mock using Partial<ProxySession> (or an interface extracted from ProxySession) so TypeScript will catch API changes; update the session declaration from "const session: any = { ... }" to "const session: Partial<ProxySession> = { ... }" (or use a createMockSession factory) and ensure methods like shouldReuseProvider, getOriginalModel, getCurrentModel, setProvider, setLastSelectionContext, getLastSelectionContext, and addProviderToChain match the ProxySession signatures and return types; adjust any uses in the test to account for Partial (e.g., cast where necessary) so the mock remains compatible with functions expecting Provider and other concrete types.src/app/v1/_lib/proxy/rate-limit-guard.ts (1)
48-50: 注释编号不一致检查顺序注释中的编号存在不一致:
- 第 48-50 行注释显示
3-5. 资源/频率保护和6-9. 短期周期限额- 第 258 行 User RPM 标注为
5.- 第 288 行 Key 5h 限额标注为
5.(应为6.)建议统一注释中的步骤编号,以便于代码维护。
Also applies to: 258-258, 288-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/rate-limit-guard.ts` around lines 48 - 50, The numbered steps in the comment blocks are inconsistent; update the inline comment labels so they follow the intended sequence (e.g., "3-5. 资源/频率保护", "6-9. 短期周期限额", "10-13. 中长期周期限额") and fix the mismatched labels where "User RPM" and "Key 5h" are currently misnumbered—locate the comment strings (e.g., the block containing "资源/频率保护", the "User RPM" label, and the "Key 5h" label) and renumber them to match the sequence (change the "User RPM" label to the correct step within 3-5 and change the "Key 5h" label from 5 to 6 so the short-period section reads 6-9).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx:
- Line 66: The fallbackLabel for the option with type "limitUas" is hardcoded as
"并发 UA"; replace it with an i18n lookup so the UI uses translated text instead
of a literal string: update the option entry in the list inside
limit-rule-picker.tsx to call the component's translation helper (e.g.
t('dashboard.limitUas') or the existing i18n helper used in this file) and add
the corresponding translation key "dashboard.limitUas" to all locale files
(zh-CN, zh-TW, en, ja, ru); ensure you use the same i18n accessor (function or
hook) that other options in this component use so fallback behavior remains
consistent.
- Around line 213-215: The step attribute only affects UI stepping but does not
prevent users from submitting decimals; update the form submit path in
handleSubmit to validate and coerce integer-only limits for the relevant types
(e.g., when type === "limitUas" and likewise for "limitSessions" / "limitRpm"):
check that the submitted value is a non-negative integer (e.g., Number.isInteger
and >= 0) and either coerce to an integer or return a field validation error so
the form cannot submit decimals like 1.5; reference the existing type variable
and handleSubmit function in limit-rule-picker.tsx to find where to insert this
validation and the error feedback.
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/index.tsx:
- Line 352: The limits tab status logic is missing the limitConcurrentUas field
so updating only UA concurrency does not flip the tab out of default; update
getTabStatus() to include state.rateLimit.limitConcurrentUas in the
comparison/dirty check alongside the other rateLimit properties and ensure the
same key name (limit_concurrent_uas / limitConcurrentUas mapping) is considered
in the object built at the top where limit_concurrent_uas is set (refer to the
existing assignment limit_concurrent_uas: state.rateLimit.limitConcurrentUas and
the getTabStatus function) so the tab status reflects changes to UA concurrency.
In `@src/app/api/actions/`[...route]/route.ts:
- Line 94: The field description for limitConcurrentUas is hardcoded with
describe("并发UA上限"); update it to use the i18n lookup used across the project
(e.g., call the shared translator function like i18n.t or t with the appropriate
key) instead of a raw string so the OpenAPI/schema docs render localized text;
replace the describe argument in the z.number().nullable().describe(...) call
for limitConcurrentUas (and the similar hardcoded describe at the other
occurrence around line 129) with the i18n key lookup (e.g.,
i18n.t('schema.limitConcurrentUas')) and ensure the i18n key exists in all 5
language resource files (zh-CN, zh-TW, en, ja, ru).
In `@src/lib/provider-patch-contract.ts`:
- Around line 228-229: The switch branch handling "limit_concurrent_uas"
currently accepts any finite number and therefore permits negatives and
decimals; change the validation so that for the "limit_concurrent_uas" case you
only accept non-negative integers (e.g. use Number.isInteger(value) && value >=
0) instead of the generic isFinite check, update the rejection path/error
message accordingly, and keep the "circuit_breaker_failure_threshold" branch
behavior unchanged unless it also needs integer semantics—locate the switch on
the patch key (the case "limit_concurrent_uas") in provider-patch-contract.ts
and replace the permissive finite-number check with an integer-and-non-negative
check.
In `@src/lib/rate-limit/service.ts`:
- Around line 75-79: The code hardcodes the provider Redis key as
`provider:${providerId}:active_uas` instead of using the project's
key-builder/hashtag convention; add a new helper
getProviderActiveUasKey(providerId) in "@/lib/redis/active-ua-keys" that returns
the key using the same `{active_uas}` hash tag pattern, then replace all
occurrences of the hardcoded `provider:${providerId}:active_uas` in service.ts
(the spots near usages alongside getGlobalActiveUasKey, getKeyActiveUasKey,
getUserActiveUasKey and the occurrences around lines 818-829) to call
getProviderActiveUasKey(providerId) so the provider UA key is consistent and
cluster-hash-safe.
In `@src/lib/redis/active-ua-keys.ts`:
- Around line 5-7: Add and export a provider-dimension key builder in
src/lib/redis/active-ua-keys.ts (e.g., getProviderActiveUAKey(providerId:
string)) that constructs the provider active-UA Redis key using the same
hash-tagging scheme as the existing global/key/user helpers, then import and use
that function inside src/lib/rate-limit/service.ts to replace the currently
hard-coded provider key at the provider-key usage (the hardcoded key referenced
around the provider logic near the existing rate-limit handling). Ensure the
function signature and export match the module's style so callers in service.ts
can replace the literal string with getProviderActiveUAKey(providerId).
In `@src/lib/validation/schemas.ts`:
- Around line 155-157: The validation messages for the chained validators (.int,
.min, .max) are hardcoded Chinese strings; replace these literal messages with
i18n keys and use the i18n lookup function used elsewhere in the project (e.g.,
t('validation.concurrentUALimit.int')) when constructing the schema so messages
render per locale; update the same pattern for the other occurrences noted (the
other .int/.min/.max chains at the reported locations) and ensure the keys exist
in all supported locales (zh-CN, zh-TW, en, ja, ru).
- Around line 153-159: The schema for limitConcurrentUas uses z.coerce.number()
which converts null to 0 and breaks the intended semantics (null = unlimited,
undefined = inherit); change the schema to accept null explicitly by using
z.union([z.null(), z.coerce.number().int().min(0).max(1000)]) (and keep
.optional() if needed) so null is preserved, apply the same pattern to the other
affected fields referenced in the comment, and remove hardcoded Chinese
validation messages from the .int/.min/.max calls—replace them with keys that
reference the i18n translation strings so all messages are loaded from the
localization files.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 282-423: When RateLimitService.checkAndTrackProviderUa succeeds
but the subsequent RateLimitService.checkAndTrackProviderSession fails, the UA
counter on the previous provider is never decremented causing "ghost UA" counts;
update the session failure branch (the block handling !checkResult.allowed) to
undo the prior UA tracking by calling the corresponding rollback/decrement on
the UA tracker (e.g., a new RateLimitService.rollbackProviderUa or
RateLimitService.decrementProviderUa with session.provider.id and uaId) before
adding provider to chain and switching provider, or alternatively replace the
two-step calls with a single atomic operation in RateLimitService (e.g.,
checkAndTrackProviderUaAndSession) that performs both checks/updates
transactionally; locate fixes around RateLimitService.checkAndTrackProviderUa,
RateLimitService.checkAndTrackProviderSession, and the failure handling code
that calls session.addProviderToChain/session.setProvider to ensure UA is
reverted on session-check failure.
---
Nitpick comments:
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form.legacy.tsx:
- Around line 1521-1535: The rate-limit collapsed summary does not include the
newly added limitConcurrentUas value; update the summary renderer (the JSX/logic
that builds the rate-limit summary near the existing summary code around where
other rate-limit fields are displayed) to read the limitConcurrentUas state and
include it in the collapsed text (use limitConcurrentUas?.toString() or a
fallback like "—"/placeholder when empty), ensuring the displayed label matches
t("sections.rateLimit.limitConcurrentUas.label") so users can see the configured
concurrent UA limit when the section is collapsed; update any summaryFormatting
helper or conditional that collects other fields (e.g., maxTokens,
requestsPerMinute) to also include limitConcurrentUas.
In `@src/app/v1/_lib/proxy/rate-limit-guard.ts`:
- Around line 48-50: The numbered steps in the comment blocks are inconsistent;
update the inline comment labels so they follow the intended sequence (e.g.,
"3-5. 资源/频率保护", "6-9. 短期周期限额", "10-13. 中长期周期限额") and fix the mismatched labels
where "User RPM" and "Key 5h" are currently misnumbered—locate the comment
strings (e.g., the block containing "资源/频率保护", the "User RPM" label, and the
"Key 5h" label) and renumber them to match the sequence (change the "User RPM"
label to the correct step within 3-5 and change the "Key 5h" label from 5 to 6
so the short-period section reads 6-9).
In `@src/lib/redis/active-ua-keys.ts`:
- Around line 20-28: The functions getKeyActiveUasKey and getUserActiveUasKey
currently accept a number and can produce invalid keys if passed
NaN/Infinity/floats; add input validation at the top of each function using
Number.isSafeInteger(keyId) / Number.isSafeInteger(userId) and throw a clear
TypeError (e.g., "getKeyActiveUasKey: keyId must be a safe integer" /
"getUserActiveUasKey: userId must be a safe integer") so callers surface
incorrect values early; alternatively, if you prefer coercion, explicitly coerce
with Math.trunc after checking finite and log a warning—ensure the chosen
behavior is consistent for both functions.
In `@src/types/key.ts`:
- Line 26: Add a clear JSDoc comment for the limitConcurrentUas property in the
interface explaining the distinct meanings: explicitly state that 0 means
“disable concurrency” (or “no concurrent UAs allowed”), undefined means “inherit
default or upstream setting”, and that callers must not treat them
interchangeably; update the type if desirable to number | undefined (or a
branded type) to make absence explicit; apply the same JSDoc + type
clarification to the other analogous fields mentioned in the review so their
semantics (0 vs undefined) are unambiguous in interfaces like
limitConcurrentUas.
In `@tests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts`:
- Around line 115-133: Replace the ad-hoc any-typed session mock with a typed
mock using Partial<ProxySession> (or an interface extracted from ProxySession)
so TypeScript will catch API changes; update the session declaration from "const
session: any = { ... }" to "const session: Partial<ProxySession> = { ... }" (or
use a createMockSession factory) and ensure methods like shouldReuseProvider,
getOriginalModel, getCurrentModel, setProvider, setLastSelectionContext,
getLastSelectionContext, and addProviderToChain match the ProxySession
signatures and return types; adjust any uses in the test to account for Partial
(e.g., cast where necessary) so the mock remains compatible with functions
expecting Provider and other concrete types.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (76)
drizzle/0078_wild_scourge.sqldrizzle/meta/0078_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/en/errors.jsonmessages/en/quota.jsonmessages/en/settings/providers/form/sections.jsonmessages/ja/dashboard.jsonmessages/ja/errors.jsonmessages/ja/quota.jsonmessages/ja/settings/providers/form/sections.jsonmessages/ru/dashboard.jsonmessages/ru/errors.jsonmessages/ru/quota.jsonmessages/ru/settings/providers/form/sections.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/errors.jsonmessages/zh-CN/quota.jsonmessages/zh-CN/settings/providers/form/sections.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/errors.jsonmessages/zh-TW/quota.jsonmessages/zh-TW/settings/providers/form/sections.jsonsrc/actions/keys.tssrc/actions/providers.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/create-user-dialog.tsxsrc/app/[locale]/dashboard/_components/user/edit-key-dialog.tsxsrc/app/[locale]/dashboard/_components/user/edit-user-dialog.tsxsrc/app/[locale]/dashboard/_components/user/forms/add-key-form.tsxsrc/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsxsrc/app/[locale]/dashboard/_components/user/forms/key-edit-section.tsxsrc/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsxsrc/app/[locale]/dashboard/_components/user/forms/limit-rules-display.tsxsrc/app/[locale]/dashboard/_components/user/forms/user-edit-section.tsxsrc/app/[locale]/dashboard/_components/user/forms/user-form.tsxsrc/app/[locale]/dashboard/_components/user/key-list-header.tsxsrc/app/[locale]/dashboard/_components/user/key-list.tsxsrc/app/[locale]/dashboard/_components/user/user-key-manager.tsxsrc/app/[locale]/dashboard/_components/user/user-key-table-row.tsxsrc/app/[locale]/dashboard/quotas/users/_components/types.tssrc/app/[locale]/dashboard/quotas/users/_components/user-quota-list-item.tsxsrc/app/[locale]/dashboard/quotas/users/_components/users-quota-client.tsxsrc/app/[locale]/dashboard/quotas/users/page.tsxsrc/app/[locale]/dashboard/users/users-page-client.tsxsrc/app/[locale]/settings/providers/_components/batch-edit/build-patch-draft.tssrc/app/[locale]/settings/providers/_components/forms/provider-form.legacy.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.tssrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/limits-section.tsxsrc/app/api/actions/[...route]/route.tssrc/app/v1/_lib/proxy/error-handler.tssrc/app/v1/_lib/proxy/errors.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/rate-limit-guard.tssrc/drizzle/schema.tssrc/lib/auth.tssrc/lib/permissions/user-field-permissions.tssrc/lib/provider-patch-contract.tssrc/lib/rate-limit/concurrent-ua-limit.tssrc/lib/rate-limit/service.tssrc/lib/redis/active-ua-keys.tssrc/lib/utils/error-messages.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/key.tssrc/repository/provider.tssrc/repository/user.tssrc/types/key.tssrc/types/provider.tssrc/types/user.tstests/unit/lib/rate-limit/concurrent-ua-limit.test.tstests/unit/lib/rate-limit/service-extra.test.tstests/unit/proxy/provider-selector-concurrent-ua-fallback.test.tstests/unit/proxy/rate-limit-guard.test.ts
src/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx
Outdated
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
Show resolved
Hide resolved
| limitMonthlyUsd: z.number().nullable().describe("月消费上限"), | ||
| limitTotalUsd: z.number().nullable().describe("总消费上限"), | ||
| limitConcurrentSessions: z.number().nullable().describe("并发Session上限"), | ||
| limitConcurrentUas: z.number().nullable().describe("并发UA上限"), |
There was a problem hiding this comment.
新增字段描述仍为硬编码文本,未走 i18n。
这里新增的 describe("并发UA上限") 会直接进入接口文档展示,属于用户可见文本,建议改为从多语言资源注入,避免仅单语生效。
As per coding guidelines, "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/actions/`[...route]/route.ts at line 94, The field description
for limitConcurrentUas is hardcoded with describe("并发UA上限"); update it to use
the i18n lookup used across the project (e.g., call the shared translator
function like i18n.t or t with the appropriate key) instead of a raw string so
the OpenAPI/schema docs render localized text; replace the describe argument in
the z.number().nullable().describe(...) call for limitConcurrentUas (and the
similar hardcoded describe at the other occurrence around line 129) with the
i18n key lookup (e.g., i18n.t('schema.limitConcurrentUas')) and ensure the i18n
key exists in all 5 language resource files (zh-CN, zh-TW, en, ja, ru).
src/lib/validation/schemas.ts
Outdated
| .int("并发UA上限必须是整数") | ||
| .min(0, "并发UA上限不能为负数") | ||
| .max(1000, "并发UA上限不能超过1000") |
There was a problem hiding this comment.
新增校验提示文案是硬编码字符串,未走 i18n
这些新增提示是用户可见文本,建议改为 i18n key 映射,避免只支持单语言。
As per coding guidelines: All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text.
Also applies to: 289-291, 420-422, 540-542, 780-782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/validation/schemas.ts` around lines 155 - 157, The validation
messages for the chained validators (.int, .min, .max) are hardcoded Chinese
strings; replace these literal messages with i18n keys and use the i18n lookup
function used elsewhere in the project (e.g.,
t('validation.concurrentUALimit.int')) when constructing the schema so messages
render per locale; update the same pattern for the other occurrences noted (the
other .int/.min/.max chains at the reported locations) and ensure the keys exist
in all supported locales (zh-CN, zh-TW, en, ja, ru).
drizzle/0078_wild_scourge.sql
Outdated
| @@ -0,0 +1,3 @@ | |||
| ALTER TABLE "keys" ADD COLUMN "limit_concurrent_uas" integer DEFAULT 0;--> statement-breakpoint | |||
| ALTER TABLE "providers" ADD COLUMN "limit_concurrent_uas" integer DEFAULT 0;--> statement-breakpoint | |||
| ALTER TABLE "users" ADD COLUMN "limit_concurrent_uas" integer; No newline at end of file | |||
There was a problem hiding this comment.
SQL file missing newline at end
| ALTER TABLE "users" ADD COLUMN "limit_concurrent_uas" integer; | |
| ALTER TABLE "users" ADD COLUMN "limit_concurrent_uas" integer; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: drizzle/0078_wild_scourge.sql
Line: 3
Comment:
SQL file missing newline at end
```suggestion
ALTER TABLE "users" ADD COLUMN "limit_concurrent_uas" integer;
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const globalKey = getGlobalActiveUasKey(); | ||
| const keyKey = getKeyActiveUasKey(keyId); | ||
| const userKey = getUserActiveUasKey(userId); | ||
| const now = Date.now(); |
There was a problem hiding this comment.
Reusing CHECK_AND_TRACK_KEY_USER_SESSION Lua script for UA tracking is efficient, but the script name may cause confusion since it suggests session-specific logic. Consider adding a comment explaining this reuse is intentional and semantically correct.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/rate-limit/service.ts
Line: 689
Comment:
Reusing `CHECK_AND_TRACK_KEY_USER_SESSION` Lua script for UA tracking is efficient, but the script name may cause confusion since it suggests session-specific logic. Consider adding a comment explaining this reuse is intentional and semantically correct.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx (1)
51-51: JSDoc 注释遗漏了limitRpm翻译键。
LIMIT_TYPE_OPTIONS包含"limitRpm",但此处文档注释中limitTypes.{...}未列出该键,可能导致调用方遗漏翻译。建议补充文档
- * - limitTypes.{limit5h|limitDaily|limitWeekly|limitMonthly|limitTotal|limitSessions|limitUas} + * - limitTypes.{limitRpm|limit5h|limitDaily|limitWeekly|limitMonthly|limitTotal|limitSessions|limitUas}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx at line 51, JSDoc for limitTypes in limit-rule-picker.tsx omits the "limitRpm" translation key while LIMIT_TYPE_OPTIONS includes it; update the JSDoc comment (the line with limitTypes.{...}) to include "limitRpm" so callers see the full set of translation keys and won't miss providing a translation for limitRpm.src/lib/rate-limit/service.ts (1)
797-852: Provider UA key 构建已修复,但 reason 字符串建议使用 i18n。
getProviderActiveUasKey(providerId)的使用解决了之前评审中关于硬编码 Redis key 的问题,现在 Provider UA key 与其他维度保持一致。Line 839 的
reason字段使用了硬编码中文字符串"供应商并发 UA 上限已达到"。虽然这与现有checkAndTrackProviderSession(Line 782)的模式一致,但根据编码规范,用户可见的字符串应使用 i18n。建议后续统一重构这些 reason 字符串。♻️ 建议修复(可选)
if (allowed === 0) { return { allowed: false, count, tracked: false, - reason: `供应商并发 UA 上限已达到(${count}/${limit})`, + reasonCode: ERROR_CODES.RATE_LIMIT_CONCURRENT_UAS_EXCEEDED, + reasonParams: { current: count, limit, target: "provider" }, }; }这样可以与
checkAndTrackKeyUserUa的返回结构保持一致,由调用方根据reasonCode进行 i18n 处理。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rate-limit/service.ts` around lines 797 - 852, The hard-coded Chinese reason string in checkAndTrackProviderUa should be replaced with an i18n-friendly code/field to match the pattern used by checkAndTrackKeyUserUa; update the return when allowed === 0 to return a reasonCode (e.g. "PROVIDER_UA_LIMIT_REACHED") instead of the literal `"供应商并发 UA 上限已达到(${count}/${limit})"`, keep or remove the human-readable reason field as needed, and ensure callers of checkAndTrackProviderUa are adjusted to use the new reasonCode for localization (reference: function checkAndTrackProviderUa and parity with checkAndTrackKeyUserUa).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx:
- Line 51: JSDoc for limitTypes in limit-rule-picker.tsx omits the "limitRpm"
translation key while LIMIT_TYPE_OPTIONS includes it; update the JSDoc comment
(the line with limitTypes.{...}) to include "limitRpm" so callers see the full
set of translation keys and won't miss providing a translation for limitRpm.
In `@src/lib/rate-limit/service.ts`:
- Around line 797-852: The hard-coded Chinese reason string in
checkAndTrackProviderUa should be replaced with an i18n-friendly code/field to
match the pattern used by checkAndTrackKeyUserUa; update the return when allowed
=== 0 to return a reasonCode (e.g. "PROVIDER_UA_LIMIT_REACHED") instead of the
literal `"供应商并发 UA 上限已达到(${count}/${limit})"`, keep or remove the human-readable
reason field as needed, and ensure callers of checkAndTrackProviderUa are
adjusted to use the new reasonCode for localization (reference: function
checkAndTrackProviderUa and parity with checkAndTrackKeyUserUa).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (10)
src/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/index.tsxsrc/app/api/actions/[...route]/route.tssrc/lib/provider-patch-contract.tssrc/lib/rate-limit/service.tssrc/lib/redis/active-ua-keys.tssrc/lib/utils/error-messages.tssrc/lib/validation/schemas.tssrc/types/key.tstests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/[locale]/settings/providers/_components/forms/provider-form/index.tsx
- tests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts
- src/app/api/actions/[...route]/route.ts
- src/lib/utils/error-messages.ts
- src/lib/redis/active-ua-keys.ts
|
已按 CodeRabbit / Gemini 审查意见修正(commit 7f5e3df):
如还有需要调整的点请继续指出。 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/rate-limit/service.ts (1)
694-696: 建议在 Key/User UA 路径统一使用通用脚本别名,避免语义漂移。当前
checkAndTrackKeyUserUa仍调用CHECK_AND_TRACK_KEY_USER_SESSION,建议改为CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER,与同文件 Session 路径保持一致,降低后续误读成本。建议修改
- const result = (await RateLimitService.redis.eval( - CHECK_AND_TRACK_KEY_USER_SESSION, + const result = (await RateLimitService.redis.eval( + CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rate-limit/service.ts` around lines 694 - 696, The call inside checkAndTrackKeyUserUa is using the wrong script alias CHECK_AND_TRACK_KEY_USER_SESSION; update the RateLimitService.redis.eval invocation in the checkAndTrackKeyUserUa implementation to use CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER instead of CHECK_AND_TRACK_KEY_USER_SESSION so the UA path uses the same ZSET-member script alias as the Session path (adjust the constant passed as the first argument to redis.eval where checkAndTrackKeyUserUa is defined).drizzle/0078_wild_scourge.sql (1)
1-3: 建议补充非负约束,增强数据完整性。新增的
limit_concurrent_uas字段目前缺少 CHECK 约束。虽然代码库中limit_concurrent_sessions等类似字段也未添加此类约束,但为了防止通过 SQL 直写绕过应用校验导致负数进入数据库破坏限流语义,建议在此迁移中补充非负验证。users表的可空字段可允许 NULL。建议的迁移补丁
ALTER TABLE "keys" ADD COLUMN "limit_concurrent_uas" integer DEFAULT 0;--> statement-breakpoint ALTER TABLE "providers" ADD COLUMN "limit_concurrent_uas" integer DEFAULT 0;--> statement-breakpoint ALTER TABLE "users" ADD COLUMN "limit_concurrent_uas" integer; +ALTER TABLE "keys" + ADD CONSTRAINT "keys_limit_concurrent_uas_non_negative" + CHECK ("limit_concurrent_uas" >= 0);--> statement-breakpoint +ALTER TABLE "providers" + ADD CONSTRAINT "providers_limit_concurrent_uas_non_negative" + CHECK ("limit_concurrent_uas" >= 0);--> statement-breakpoint +ALTER TABLE "users" + ADD CONSTRAINT "users_limit_concurrent_uas_non_negative" + CHECK ("limit_concurrent_uas" IS NULL OR "limit_concurrent_uas" >= 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0078_wild_scourge.sql` around lines 1 - 3, 为新增的 limit_concurrent_uas 字段添加非负约束以保证数据完整性:在 ALTER TABLE "keys" 和 ALTER TABLE "providers" 的列定义中增加非负 CHECK(例如 DEFAULT 0 且 CHECK (limit_concurrent_uas >= 0)),并为约束命名以便识别(例如 chk_keys_limit_concurrent_uas_nonnegative / chk_providers_limit_concurrent_uas_nonnegative);对于 ALTER TABLE "users" 保持可空但添加 CHECK 以允许 NULL 或非负值(例如 CHECK (limit_concurrent_uas IS NULL OR limit_concurrent_uas >= 0))以防止负数写入数据库。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/rate-limit/service.ts`:
- Around line 810-842: The rejection branch currently returns a hardcoded
Chinese reason string; change the returned payload where allowed === 0 to remove
the hardcoded text and instead return a machine-readable reason code and
parameters for i18n (e.g., reasonCode: 'provider.ua_concurrency_limit_reached'
and reasonParams: { count, limit }). Update the object returned in the same
place that currently references count/limit (the block reading result and
checking allowed === 0 in the function that calls RateLimitService.redis.eval
with CHECK_AND_TRACK_ZSET_MEMBER and SESSION_TTL_MS) so upper layers can perform
localization using the provided reasonCode/reasonParams. Ensure no user-facing
strings remain in this file.
---
Nitpick comments:
In `@drizzle/0078_wild_scourge.sql`:
- Around line 1-3: 为新增的 limit_concurrent_uas 字段添加非负约束以保证数据完整性:在 ALTER TABLE
"keys" 和 ALTER TABLE "providers" 的列定义中增加非负 CHECK(例如 DEFAULT 0 且 CHECK
(limit_concurrent_uas >= 0)),并为约束命名以便识别(例如
chk_keys_limit_concurrent_uas_nonnegative /
chk_providers_limit_concurrent_uas_nonnegative);对于 ALTER TABLE "users" 保持可空但添加
CHECK 以允许 NULL 或非负值(例如 CHECK (limit_concurrent_uas IS NULL OR
limit_concurrent_uas >= 0))以防止负数写入数据库。
In `@src/lib/rate-limit/service.ts`:
- Around line 694-696: The call inside checkAndTrackKeyUserUa is using the wrong
script alias CHECK_AND_TRACK_KEY_USER_SESSION; update the
RateLimitService.redis.eval invocation in the checkAndTrackKeyUserUa
implementation to use CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER instead of
CHECK_AND_TRACK_KEY_USER_SESSION so the UA path uses the same ZSET-member script
alias as the Session path (adjust the constant passed as the first argument to
redis.eval where checkAndTrackKeyUserUa is defined).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (3)
drizzle/0078_wild_scourge.sqlsrc/lib/rate-limit/service.tssrc/lib/redis/lua-scripts.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/provider-selector.ts (1)
280-357:⚠️ Potential issue | 🟠 MajorProvider UA 与 Session 分步追踪会造成 UA 计数泄漏。
当前流程是“先追踪 UA,再追踪 Session”。当 UA 检查通过并完成追踪后,如果 Session 检查失败并触发回退,已写入的 UA 成员不会撤销,直到 TTL 过期前都会占用并发 UA 名额,可能导致后续请求被误判超限。
建议修复(最小止血 + 长期方案)
- const uaCheckResult = await RateLimitService.checkAndTrackProviderUa( + const uaCheckResult = await RateLimitService.checkAndTrackProviderUa( session.provider.id, uaId, uaLimit ); + const uaTrackedThisAttempt = uaCheckResult.tracked; @@ const checkResult = await RateLimitService.checkAndTrackProviderSession( session.provider.id, session.sessionId, limit ); if (!checkResult.allowed) { + // 避免“UA 已追踪但 Session 未通过”导致的计数泄漏 + if (uaTrackedThisAttempt) { + await RateLimitService.untrackProviderUa(session.provider.id, uaId); + } // === 并发限制失败 === logger.warn( "ProviderSelector: Provider concurrent session limit exceeded, trying fallback",// src/lib/rate-limit/service.ts static async untrackProviderUa(providerId: number, uaId: string): Promise<void> { if (!RateLimitService.redis || RateLimitService.redis.status !== "ready") return; await RateLimitService.redis.zrem(getProviderActiveUasKey(providerId), uaId); }更稳妥的长期方案:把“provider UA + provider session”的检查与追踪合并到同一个 Lua 原子脚本,一次性 check+track,彻底消除双写不一致。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/provider-selector.ts` around lines 280 - 357, The code first calls RateLimitService.checkAndTrackProviderUa(…) and then RateLimitService.checkAndTrackProviderSession(…), which can leak UA entries when the session check fails and the code falls back; fix by undoing the UA tracking on failure (call a new RateLimitService.untrackProviderUa(providerId, uaId) immediately before adding provider to chain / selecting fallback) or, better, replace the two-step flow with a single atomic check+track in a Lua script inside RateLimitService to perform provider UA+session checkAndTrack atomically; update the failure path in ProviderSelector (around calls to checkAndTrackProviderUa, checkAndTrackProviderSession, session.addProviderToChain, ProxyProviderResolver.pickRandomProvider, and session.setProvider) to call untrackProviderUa on session check failure so UA slot is released.
🧹 Nitpick comments (1)
src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx (1)
54-71: 建议抽离resolveChainItemErrorMessage为共享工具。该函数与
src/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsx中实现几乎一致,后续容易出现 fallback 规则漂移,建议统一到单一 util。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx around lines 54 - 71, Extract the duplicate logic in resolveChainItemErrorMessage into a shared utility function (e.g., export function resolveChainItemErrorMessage(...) in a new util module) and replace the inline implementations in both LogicTraceTab and provider-chain-popover to import and use that single exported function; ensure the new util preserves the same behavior (string errorMessage check, errorCode validation, tErrors call with item.errorParams fallback and try/catch returning item.errorCode) and update exports/imports accordingly so both components rely on the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 280-357: The code first calls
RateLimitService.checkAndTrackProviderUa(…) and then
RateLimitService.checkAndTrackProviderSession(…), which can leak UA entries when
the session check fails and the code falls back; fix by undoing the UA tracking
on failure (call a new RateLimitService.untrackProviderUa(providerId, uaId)
immediately before adding provider to chain / selecting fallback) or, better,
replace the two-step flow with a single atomic check+track in a Lua script
inside RateLimitService to perform provider UA+session checkAndTrack atomically;
update the failure path in ProviderSelector (around calls to
checkAndTrackProviderUa, checkAndTrackProviderSession,
session.addProviderToChain, ProxyProviderResolver.pickRandomProvider, and
session.setProvider) to call untrackProviderUa on session check failure so UA
slot is released.
---
Nitpick comments:
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx:
- Around line 54-71: Extract the duplicate logic in resolveChainItemErrorMessage
into a shared utility function (e.g., export function
resolveChainItemErrorMessage(...) in a new util module) and replace the inline
implementations in both LogicTraceTab and provider-chain-popover to import and
use that single exported function; ensure the new util preserves the same
behavior (string errorMessage check, errorCode validation, tErrors call with
item.errorParams fallback and try/catch returning item.errorCode) and update
exports/imports accordingly so both components rely on the same implementation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (8)
src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsxsrc/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsxsrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/session.tssrc/lib/rate-limit/service.tssrc/types/message.tstests/unit/lib/rate-limit/service-extra.test.tstests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/lib/rate-limit/service-extra.test.ts
- Provider Session 超限回退时回滚本次新增 UA(避免幽灵计数)\n- 增加 ZSET member 回滚 Lua 脚本并补齐单测\n- 抽离 ProviderChain 错误信息解析工具,移除重复实现\n- 修正文档注释遗漏的 limitRpm 翻译键
|
已根据 AI review 的主要问题做了修正(commit 918bccb):
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/[locale]/dashboard/logs/_components/resolve-chain-item-error-message.ts (1)
11-18: 建议先标准化errorCode再查翻译。Line 11 已用
trim()判空,但 Line 16/18 仍直接使用原始item.errorCode;如果上游带前后空格,会导致翻译键命中失败并回退到带空格的错误码。建议修改
- if (typeof item.errorCode !== "string" || !item.errorCode.trim()) { + const errorCode = typeof item.errorCode === "string" ? item.errorCode.trim() : ""; + if (!errorCode) { return null; } try { - return tErrors(item.errorCode, item.errorParams ?? undefined); + return tErrors(errorCode, item.errorParams ?? undefined); } catch { - return item.errorCode; + return errorCode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/resolve-chain-item-error-message.ts around lines 11 - 18, Normalize item.errorCode to a trimmed string once and use that normalized value for translation and fallback: check typeof item.errorCode === "string", set const code = item.errorCode.trim(), return null if !code, then call tErrors(code, item.errorParams ?? undefined) inside the try and return code in the catch; update references to use code instead of item.errorCode (symbols: item.errorCode, tErrors).src/lib/rate-limit/service.ts (1)
695-697: 建议统一 Lua 脚本别名,降低维护时的语义分叉风险。这里建议与同文件的 Session 分支保持一致,统一使用
CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER,避免同一语义下出现两种常量名。♻️ 建议修改
- CHECK_AND_TRACK_KEY_USER_SESSION, + CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rate-limit/service.ts` around lines 695 - 697, Replace the Lua script alias CHECK_AND_TRACK_KEY_USER_SESSION with the unified constant CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER in the Redis eval invocation so this branch uses the same semantic name as the Session branch; update the call site that constructs the eval (the one using RateLimitService.redis.eval and CHECK_AND_TRACK_KEY_USER_SESSION) to pass CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER instead and ensure any related parameter ordering or KEYS count remains unchanged.src/app/v1/_lib/proxy/provider-selector.ts (1)
290-349: 建议抽取统一的并发失败回退流程,减少分支漂移。UA 超限与 Session 超限分支的“记录链路 → 加入排除 → 选 fallback → 切换重试”逻辑高度重复,后续维护很容易出现行为不一致。建议抽一个私有 helper 统一处理。
Also applies to: 366-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/provider-selector.ts` around lines 290 - 349, Extract the repeated “record failure → add to excluded → pick fallback → switch and retry” flow into a private helper (e.g., handleConcurrentLimitFallback) and call it from both UA-limit and Session-limit branches to avoid divergence: the helper should accept the Session instance, the failing provider info (session.provider), the decision context pieces (uaCheckResult, uaLimit or sessionLimit), attemptCount, excludedProviders array, and logger; inside it call session.getLastSelectionContext(), session.addProviderToChain(...) with the same structured payload, push the provider id into excludedProviders, call ProxyProviderResolver.pickRandomProvider(session, excludedProviders), log and return a result indicating whether a fallback was found (and the fallback provider/context) so callers can call session.setProvider(...) and session.setLastSelectionContext(...) or break if none. Ensure you reuse symbols session.addProviderToChain, session.getLastSelectionContext, ProxyProviderResolver.pickRandomProvider, session.setProvider, and session.setLastSelectionContext so both branches (UA and Session) invoke identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx:
- Around line 195-197: The SelectItem currently uses
getTranslation(translations, `limitTypes.${opt}`, opt) which falls back to the
internal enum key (opt) when a translation is missing; update the SelectItem to
call getTranslation(translations, `limitTypes.${opt}`) without the opt fallback
so user-facing text always comes from i18n, and add the missing limitTypes.*
entries across all five locales (zh-CN, zh-TW, en, ja, ru) to the translation
files so no fallback occurs; references: SelectItem, getTranslation,
translations, and the limitTypes.* keys.
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx:
- Line 29: 在 LogicTraceTab.tsx 中把相对导入改为仓库约定的别名导入:将当前的 import {
resolveChainItemErrorMessage } from "../../resolve-chain-item-error-message";
替换为使用 "@/..." 路径别名指向同一模块(保留导入的符号 resolveChainItemErrorMessage 不变),确保导入路径从 src
根目录开始并符合项目的别名配置(如 tsconfig/webpack 别名)。
---
Nitpick comments:
In
`@src/app/`[locale]/dashboard/logs/_components/resolve-chain-item-error-message.ts:
- Around line 11-18: Normalize item.errorCode to a trimmed string once and use
that normalized value for translation and fallback: check typeof item.errorCode
=== "string", set const code = item.errorCode.trim(), return null if !code, then
call tErrors(code, item.errorParams ?? undefined) inside the try and return code
in the catch; update references to use code instead of item.errorCode (symbols:
item.errorCode, tErrors).
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 290-349: Extract the repeated “record failure → add to excluded →
pick fallback → switch and retry” flow into a private helper (e.g.,
handleConcurrentLimitFallback) and call it from both UA-limit and Session-limit
branches to avoid divergence: the helper should accept the Session instance, the
failing provider info (session.provider), the decision context pieces
(uaCheckResult, uaLimit or sessionLimit), attemptCount, excludedProviders array,
and logger; inside it call session.getLastSelectionContext(),
session.addProviderToChain(...) with the same structured payload, push the
provider id into excludedProviders, call
ProxyProviderResolver.pickRandomProvider(session, excludedProviders), log and
return a result indicating whether a fallback was found (and the fallback
provider/context) so callers can call session.setProvider(...) and
session.setLastSelectionContext(...) or break if none. Ensure you reuse symbols
session.addProviderToChain, session.getLastSelectionContext,
ProxyProviderResolver.pickRandomProvider, session.setProvider, and
session.setLastSelectionContext so both branches (UA and Session) invoke
identical behavior.
In `@src/lib/rate-limit/service.ts`:
- Around line 695-697: Replace the Lua script alias
CHECK_AND_TRACK_KEY_USER_SESSION with the unified constant
CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER in the Redis eval invocation so this branch
uses the same semantic name as the Session branch; update the call site that
constructs the eval (the one using RateLimitService.redis.eval and
CHECK_AND_TRACK_KEY_USER_SESSION) to pass CHECK_AND_TRACK_KEY_USER_ZSET_MEMBER
instead and ensure any related parameter ordering or KEYS count remains
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (9)
src/app/[locale]/dashboard/_components/user/forms/limit-rule-picker.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsxsrc/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsxsrc/app/[locale]/dashboard/logs/_components/resolve-chain-item-error-message.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/lib/rate-limit/service.tssrc/lib/redis/lua-scripts.tstests/unit/lib/rate-limit/service-extra.test.tstests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/redis/lua-scripts.ts
- tests/unit/lib/rate-limit/service-extra.test.ts
- src/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsx
- tests/unit/proxy/provider-selector-concurrent-ua-fallback.test.ts
| <SelectItem key={opt} value={opt}> | ||
| {getTranslation(translations, `limitTypes.${opt}`, opt)} | ||
| </SelectItem> |
There was a problem hiding this comment.
避免把内部枚举值作为下拉文案回退值。
getTranslation(..., opt) 在缺失翻译时会把 limitUas 这类内部 key 直接展示给用户,属于用户可见硬编码文本路径。建议改为仅使用 i18n key,并确保 5 种语言都补齐 limitTypes.*。
🔧 建议修改
- {getTranslation(translations, `limitTypes.${opt}`, opt)}
+ {getTranslation(
+ translations,
+ `limitTypes.${opt}`,
+ getTranslation(translations, "fields.type.placeholder", ""),
+ )}As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/_components/user/forms/limit-rule-picker.tsx
around lines 195 - 197, The SelectItem currently uses
getTranslation(translations, `limitTypes.${opt}`, opt) which falls back to the
internal enum key (opt) when a translation is missing; update the SelectItem to
call getTranslation(translations, `limitTypes.${opt}`) without the opt fallback
so user-facing text always comes from i18n, and add the missing limitTypes.*
entries across all five locales (zh-CN, zh-TW, en, ja, ru) to the translation
files so no fallback occurs; references: SelectItem, getTranslation,
translations, and the limitTypes.* keys.
| import { cn } from "@/lib/utils"; | ||
| import { formatProbability, formatProviderTimeline } from "@/lib/utils/provider-chain-formatter"; | ||
| import type { ProviderChainItem } from "@/types/message"; | ||
| import { resolveChainItemErrorMessage } from "../../resolve-chain-item-error-message"; |
There was a problem hiding this comment.
请改为 @/ 路径别名导入。
Line 29 目前使用相对路径导入,不符合仓库的导入规范,建议统一改为 @/ 别名路径。
建议修改
-import { resolveChainItemErrorMessage } from "../../resolve-chain-item-error-message";
+import { resolveChainItemErrorMessage } from "@/app/[locale]/dashboard/logs/_components/resolve-chain-item-error-message";As per coding guidelines: Use path alias @/ to reference files in ./src/ directory.
📝 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.
| import { resolveChainItemErrorMessage } from "../../resolve-chain-item-error-message"; | |
| import { resolveChainItemErrorMessage } from "@/app/[locale]/dashboard/logs/_components/resolve-chain-item-error-message"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx
at line 29, 在 LogicTraceTab.tsx 中把相对导入改为仓库约定的别名导入:将当前的 import {
resolveChainItemErrorMessage } from "../../resolve-chain-item-error-message";
替换为使用 "@/..." 路径别名指向同一模块(保留导入的符号 resolveChainItemErrorMessage 不变),确保导入路径从 src
根目录开始并符合项目的别名配置(如 tsconfig/webpack 别名)。
- SelectItem 文案缺失时不再回退到内部枚举值\n- logs 详情页改为使用 @/ 别名导入共享 util
|
补充处理了 CodeRabbit 最新两条意见(commit 77aa2c3):
|
- Key/User UA 原子检查改用通用 Lua 别名,避免语义误读\n- UNTRACK_ZSET_MEMBER_IF_SCORE_MATCH 改为数值比较,避免 score 字符串格式差异\n- 补齐 untrackProviderUa 单测覆盖
|
补充做了两点“加固/一致性”修正(commit 8dc1a53),避免后续维护误读与边界问题:
|
|
补充修正(commit 372d37e):
本地已通过:bun run lint:fix / bun run lint / bun run typecheck / bun run test / bun run build。 CI checks 正在跑。 |
|
补充修正(commit 8df8bd5):
本地已通过:bun run lint:fix / bun run lint / bun run typecheck / bun run test / bun run build。 CI checks 正在跑。 |
- Provider legacy 表单折叠摘要补齐 limitConcurrentUas\n- Provider session 并发回退链路改用 errorCode/errorParams(便于 i18n 展示)\n- 迁移补充 limit_concurrent_uas 非负 CHECK 约束\n- Key 并发限额文案明确 0 继承 User\n- active_uas Redis key builder 增加 safe integer 校验\n- 修正 rate-limit-guard 步骤编号注释
|
已根据 CodeRabbit 等 AI review 补齐细节并推送最新提交 e136a0f:
本地与 CI 已通过(除 Greptile Review 常驻 pending 外)。 |
|
已逐条对照 CodeRabbit / Greptile / Gemini 的 review comments 复查当前实现:
如方便请再做一次人工 review/approve(历史 CodeRabbit 的 CHANGES_REQUESTED 可能不会自动消失)。 |
变更摘要
实现多层级并发 UA(User-Agent)限制,维度覆盖 User / Key / Provider,并与现有并发 Session 逻辑语义对齐:
limit_concurrent_uas为 0 表示不限制;超限时自动 fallback 到下一个可用 Provider{active_uas}hash tag;UA member 使用clientType分桶并 sha256,避免 UA 过长新增错误码:
RATE_LIMIT_CONCURRENT_UAS_EXCEEDEDKEY_LIMIT_CONCURRENT_UAS_EXCEEDS_USER_LIMIT数据库
迁移
drizzle/0078_wild_scourge.sql:users.limit_concurrent_uas:可空(null/<=0 视为不限制)keys.limit_concurrent_uas/providers.limit_concurrent_uas:默认 0主要改动点
src/lib/rate-limit/concurrent-ua-limit.ts:UA limit 归一化、Key/User 有效上限解析、UA identity(bucket + sha256)src/lib/redis/active-ua-keys.ts:UA 维度的 Redis key builders(含 cluster hash tag)src/lib/rate-limit/service.ts:Key/User UA 与 Provider UA 原子 check+track(复用 Lua 脚本别名),Provider UA untrack 回滚src/app/v1/_lib/proxy/rate-limit-guard.ts:在并发 Session 之前增加 Key/User UA 并发保护src/app/v1/_lib/proxy/provider-selector.ts:Provider UA check + fallback,且在 Session fallback 时回滚 UAlimitConcurrentUas,并补齐 5 语言文案(含 legacy provider form 折叠摘要)测试
bun run lint:fix、bun run lint、bun run typecheck、bun run test、bun run buildGreptile Summary
This PR implements multi-level concurrent User-Agent (UA) limiting at User, Key, and Provider dimensions to prevent resource exhaustion and fix issue #769 where keys bypassed user-level concurrent session limits.
Key Implementation Details:
{active_uas}hash tag for all UA tracking keys to avoid CROSSSLOT errors in clustered environmentslimitConcurrentUaswhen not explicitly set (0 value)Technical Highlights:
CHECK_AND_TRACK_KEY_USER_SESSION) for both session and UA tracking by adding generic aliasesNo Critical Issues Found - The implementation is well-architected with proper atomicity guarantees, comprehensive error handling, and thorough test coverage.
Confidence Score: 5/5
Important Files Changed
{active_uas}hash tag for cluster compatibility. Includes proper type checking.limit_concurrent_uascolumns with proper constraints. Default 0 for keys/providers, nullable for users.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Request arrives] --> B[Rate Limit Guard] B --> C{Check Total Limits} C -->|Failed| X[Reject: Total limit] C -->|Pass| D{Check UA Concurrent} D -->|Failed| Y[Reject: UA limit] D -->|Pass| E{Check Session Concurrent} E -->|Failed| Z[Reject: Session limit] E -->|Pass| F{Check RPM/Cost Limits} F -->|Failed| W[Reject: Cost/RPM limit] F -->|Pass| G[Provider Selection] G --> H{Check Provider UA} H -->|Exceeded| I[Try Next Provider] I --> H H -->|Pass| J{Check Provider Session} J -->|Exceeded| K[Rollback Provider UA] K --> I J -->|Pass| L[Track Session] L --> M[Proxy Request] I -->|No Fallback| N[Reject: No providers] subgraph "Key/User Inheritance" D1[Key limit=0?] D1 -->|Yes| D2[Inherit User limit] D1 -->|No| D3[Use Key limit] endLast reviewed commit: e136a0f