feat: add costResetAt for soft user limit reset without deleting data#890
Merged
Conversation
Add costResetAt timestamp column to users table that clips all cost calculations to start from reset time instead of all-time. This enables admins to reset a user's rate limits without destroying historical usage data (messageRequest/usageLedger rows are preserved). Key changes: - Schema: new cost_reset_at column on users table - Repository: costResetAt propagated through all select queries, key validation, and statistics aggregation (with per-user batch support) - Rate limiting: all 12 proxy guard checks pass costResetAt; service and lease layers clip time windows accordingly - Auth cache: hydrate costResetAt from Redis cache as Date; invalidate auth cache on reset to avoid stale costResetAt - Actions: resetUserLimitsOnly sets costResetAt + clears cost cache; getUserLimitUsage/getUserAllLimitUsage/getKeyLimitUsage/getMyQuota clip time ranges by costResetAt - UI: edit-user-dialog with separate Reset Limits Only (amber) vs Reset All Statistics (red) with confirmation dialogs - i18n: all 5 languages (en, zh-CN, zh-TW, ja, ru) - Tests: 10 unit tests for resetUserLimitsOnly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…te validation - Extract resetUserCostResetAt repo function with updatedAt + auth cache invalidation - Extract clearUserCostCache helper to deduplicate Redis cleanup between reset functions - Use instanceof Date checks in lease-service and my-usage for costResetAt validation - Remove dead hasActiveSessions variable in cost-cache-cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e in user edit dialog R3: Replace truthiness checks with `instanceof Date` in 3 places (users.ts clipStart, quotas page). R4: Show last reset timestamp in edit-user-dialog Reset Limits section (5 langs). Add 47 unit tests covering costResetAt across key-quota, redis cleanup, statistics, and auth cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clip all period time ranges by user's costResetAt and replace getTotalUsageForKey with sumKeyTotalCost supporting resetAt parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nable mock Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pping, error handling - keys.ts: eliminate redundant findUserById call, use joined costResetAt + instanceof Date guard - users.ts: handle resetUserCostResetAt return value (false = soft-deleted user) - service.ts: add instanceof Date guard to costResetAt comparison - statistics.ts: fix sumKeyTotalCost/sumUserTotalCost to use max(resetAt, maxAgeCutoff) instead of replacing maxAgeDays; refactor nested ternaries to if-blocks in quota functions - cost-cache-cleanup.ts: wrap pipeline.exec() in try/catch to honor never-throws contract - Update test for pipeline.exec throw now caught inside clearUserCostCache Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…With Verify each window (5h/daily/weekly/monthly) is clipped individually instead of checking unordered calledWith matches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- ja/dashboard.json: replace fullwidth parens with halfwidth - api-key-auth-cache-reset-at.test: override CI env so shouldUseRedisClient() works - key-quota-concurrent-inherit.test: add logger.info mock, sumKeyTotalCost mock, userCostResetAt field - my-usage-concurrent-inherit.test: add logger.info/debug mocks - total-usage-semantics.test: update call assertions for new costResetAt parameter - users-reset-all-statistics.test: mock resetUserCostResetAt, update pipeline.exec error expectations - rate-limit-guard.test: add cost_reset_at: null to expected checkCostLimitsWithLease args Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough向 users 表添加 cost_reset_at 列并将该重置时间点在配额/统计/速率限制、租约、认证缓存、Redis 清理、UI 和重置操作中贯穿：按 reset 时间裁剪时间窗口、传递 reset 参数、新增 Redis 成本缓存清理工具及对应测试。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
Contributor
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! This pull request introduces a significant feature that enhances user quota management by providing a non-destructive way to reset user limits. Instead of wiping all historical usage data, administrators can now perform a 'soft reset' that simply marks a point in time from which all future cost calculations will begin. This change offers greater flexibility and control over user billing cycles and quota enforcement, allowing for more nuanced management of user accounts without data loss. 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
|
github-actions
Bot
added
size/XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature: a "soft reset" for user limits using a costResetAt timestamp. The changes are thoughtfully propagated throughout the application stack, from the database schema and repositories to server actions, the rate-limiting pipeline, and the dashboard UI. The addition of a dedicated Redis cache cleanup utility is a good design choice, and the new functionality is well-covered by unit tests. I have one suggestion to improve the robustness of the API key authentication cache. Overall, this is a high-quality contribution.
Comment on lines
173
to
174
| if (user.expiresAt != null && !expiresAt) return null; | ||
| if (user.deletedAt != null && !deletedAt) return null; |
Contributor
There was a problem hiding this comment.
There's a small inconsistency in date validation here. For expiresAt and deletedAt, if the date string from the cache is invalid, parseOptionalDate returns null, and the subsequent checks will invalidate the entire cache entry by returning null.
However, there's no such check for costResetAt. If user.costResetAt is an invalid date string, it will be hydrated as null and the function will return a user object with a null costResetAt, which might not be the intended behavior.
For consistency and to improve robustness against potentially malformed cache data, consider adding a similar check for costResetAt.
Suggested change
| if (user.expiresAt != null && !expiresAt) return null; | |
| if (user.deletedAt != null && !deletedAt) return null; | |
| if (user.expiresAt != null && !expiresAt) return null; | |
| if (user.deletedAt != null && !deletedAt) return null; | |
| if (user.costResetAt != null && !costResetAt) return null; |
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1903
to
+1910
| // 1. Delete all messageRequest logs for this user | ||
| await db.delete(messageRequest).where(eq(messageRequest.userId, userId)); | ||
|
|
||
| // Check for errors | ||
| const errors = results?.filter(([err]) => err); | ||
| if (errors && errors.length > 0) { | ||
| logger.warn("Some Redis deletes failed during user statistics reset", { | ||
| errorCount: errors.length, | ||
| userId, | ||
| }); | ||
| } | ||
| // Also clear ledger rows -- the ONLY legitimate DELETE path for usage_ledger | ||
| await db.delete(usageLedger).where(eq(usageLedger.userId, userId)); | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| // Clear costResetAt since all data is wiped (also invalidates auth cache) | ||
| await resetUserCostResetAt(userId, null); |
There was a problem hiding this comment.
Missing database transaction around three sequential writes
The three DB operations here run as separate, independent statements with no transaction wrapper. If the process crashes or the DB throws between any two calls — for example after usageLedger rows are deleted but before resetUserCostResetAt completes — the user will be left in an inconsistent state:
messageRequestdata deleted,usageLedgerdata retained (or vice versa), with a stale non-nullcostResetAtstill filtering queries.- Auth-cache invalidation inside
resetUserCostResetAtwill never run, so in-flight requests continue reading the old cached user.
These three writes should be wrapped in a single Drizzle transaction so they succeed or fail atomically:
await db.transaction(async (tx) => {
await tx.delete(messageRequest).where(eq(messageRequest.userId, userId));
await tx.delete(usageLedger).where(eq(usageLedger.userId, userId));
await tx
.update(users)
.set({ costResetAt: null, updatedAt: new Date() })
.where(and(eq(users.id, userId), isNull(users.deletedAt)));
});
// Invalidate auth cache after the transaction succeeds
await invalidateCachedUser(userId).catch(() => {});Note that invalidateCachedUser must remain outside the transaction (it talks to Redis, not the DB) and should be called only on success.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/users.ts
Line: 1903-1910
Comment:
**Missing database transaction around three sequential writes**
The three DB operations here run as separate, independent statements with no transaction wrapper. If the process crashes or the DB throws between any two calls — for example after `usageLedger` rows are deleted but before `resetUserCostResetAt` completes — the user will be left in an inconsistent state:
- `messageRequest` data deleted, `usageLedger` data retained (or vice versa), with a stale non-null `costResetAt` still filtering queries.
- Auth-cache invalidation inside `resetUserCostResetAt` will never run, so in-flight requests continue reading the old cached user.
These three writes should be wrapped in a single Drizzle transaction so they succeed or fail atomically:
```ts
await db.transaction(async (tx) => {
await tx.delete(messageRequest).where(eq(messageRequest.userId, userId));
await tx.delete(usageLedger).where(eq(usageLedger.userId, userId));
await tx
.update(users)
.set({ costResetAt: null, updatedAt: new Date() })
.where(and(eq(users.id, userId), isNull(users.deletedAt)));
});
// Invalidate auth cache after the transaction succeeds
await invalidateCachedUser(userId).catch(() => {});
```
Note that `invalidateCachedUser` must remain outside the transaction (it talks to Redis, not the DB) and should be called only on success.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
172
to
183
| @@ -178,6 +179,7 @@ function hydrateUserFromCache(payload: CachedUserPayloadV1): User | null { | |||
| updatedAt, | |||
| expiresAt: expiresAt === undefined ? undefined : expiresAt, | |||
| deletedAt: deletedAt === undefined ? undefined : deletedAt, | |||
| costResetAt: costResetAt === undefined ? undefined : costResetAt, | |||
| } as User; | |||
There was a problem hiding this comment.
costResetAt silently degrades on invalid dates, inconsistent with expiresAt/deletedAt
For expiresAt and deletedAt, the code correctly returns null (cache miss) when the cached value is non-null but fails date parsing:
if (user.expiresAt != null && !expiresAt) return null;
if (user.deletedAt != null && !deletedAt) return null;costResetAt has no equivalent guard. If a corrupted or undeserializable string reaches this path, parseOptionalDate returns null and the cache entry is silently accepted as-is — with costResetAt treated as "no reset", causing cost calculations to count all historical usage since the beginning of time rather than from the intended reset point.
Consider adding a consistency guard (or at minimum an explanatory comment for the deliberate omission):
const costResetAt = parseOptionalDate(user.costResetAt);
if (user.costResetAt != null && costResetAt == null) {
// Invalid costResetAt: fail-open (treat as no reset) rather than cache-miss,
// because an incorrect cutoff only affects quota counting, not access control.
}This documents the intentional fail-open design choice and makes it easier for future contributors to understand the asymmetry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/security/api-key-auth-cache.ts
Line: 172-183
Comment:
**`costResetAt` silently degrades on invalid dates, inconsistent with `expiresAt`/`deletedAt`**
For `expiresAt` and `deletedAt`, the code correctly returns `null` (cache miss) when the cached value is non-null but fails date parsing:
```ts
if (user.expiresAt != null && !expiresAt) return null;
if (user.deletedAt != null && !deletedAt) return null;
```
`costResetAt` has no equivalent guard. If a corrupted or undeserializable string reaches this path, `parseOptionalDate` returns `null` and the cache entry is silently accepted as-is — with `costResetAt` treated as "no reset", causing cost calculations to count all historical usage since the beginning of time rather than from the intended reset point.
Consider adding a consistency guard (or at minimum an explanatory comment for the deliberate omission):
```ts
const costResetAt = parseOptionalDate(user.costResetAt);
if (user.costResetAt != null && costResetAt == null) {
// Invalid costResetAt: fail-open (treat as no reset) rather than cache-miss,
// because an incorrect cutoff only affects quota counting, not access control.
}
```
This documents the intentional fail-open design choice and makes it easier for future contributors to understand the asymmetry.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/lib/rate-limit/service.ts (1)
158-170:⚠️ Potential issue | 🟠 Major
cost_reset_at现在只修正了 DB fallback，没有修正 Redis fast path。Line 168 引入了
cost_reset_at，但 Line 186-Line 330 仍直接读取旧的cost_5h_rolling、cost_daily_*、cost_weekly、cost_monthly键。只要cost_reset_at落在当前窗口内、而 reset 时 Redis 不可用或 cleanup 漏删了旧键，这里就会继续按重置前用量拒绝请求；DB fallback 虽然做了裁剪，但根本不会被触发。这里要么像total_cost:*一样把 reset epoch 编进窗口键，要么在存在有效cost_reset_at时绕过这些旧窗口缓存。 Based on learnings,clearUserCostCache被设计成 best-effort，调用方依赖先写入costResetAt也能保证正确性；这里的 fast path 还没有满足这个前提。Also applies to: 186-330
🤖 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 158 - 170, checkCostLimits currently reads Redis window keys (cost_5h_rolling, cost_daily_*, cost_weekly, cost_monthly) without respecting cost_reset_at, so if cost_reset_at is within the current window Redis cached keys can cause incorrect rejections; update the fast path in checkCostLimits (the code that reads those cost_* keys between the current window logic and the DB fallback) to check cost_reset_at and, if present and falls inside the relevant window, bypass those old window keys (either by: 1) including the reset epoch into the Redis window key naming like total_cost:* so cached windows are namespaced by reset epoch, or 2) short-circuiting the Redis fast path to force the same DB-derived logic used by the fallback), and ensure this same check applies to all places reading cost_5h_rolling, cost_daily_*, cost_weekly, cost_monthly; also consider reusing the total_cost:* pattern and/or calling clearUserCostCache semantics so the fast path behaves consistently with the DB fallback.src/lib/security/api-key-auth-cache.ts (1)
172-182:⚠️ Potential issue | 🟠 Major无效的
costResetAt会被静默吞掉。这里对
expiresAt/deletedAt都做了“非空但解析失败就判缓存无效”的校验，但costResetAt没有同样处理。这样一来，损坏或陈旧的缓存载荷会被当成“未重置”，soft reset 后的限额计算会继续按旧累计值运行，直到缓存过期。建议补上与其他日期字段一致的校验
const expiresAt = parseOptionalDate(user.expiresAt); const deletedAt = parseOptionalDate(user.deletedAt); const costResetAt = parseOptionalDate(user.costResetAt); if (user.expiresAt != null && !expiresAt) return null; if (user.deletedAt != null && !deletedAt) return null; + if (user.costResetAt != null && !costResetAt) return null; return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/security/api-key-auth-cache.ts` around lines 172 - 182, The cache reconstruction silently ignores a failed parse of costResetAt; mirror the existing checks for expiresAt/deletedAt by adding the same validation for costResetAt: after const costResetAt = parseOptionalDate(user.costResetAt); if (user.costResetAt != null && !costResetAt) return null; this ensures a non-null but unparsable user.costResetAt invalidates the cache (use the same variables costResetAt and user.costResetAt as in the snippet).
🧹 Nitpick comments (5)
tests/unit/proxy/rate-limit-guard.test.ts (1)
159-167: 建议补一条cost_reset_at非空透传用例。这几处断言只验证了默认
null，即使 guard 忘了把真实的重置时间传给checkCostLimitsWithLease，测试仍然会通过。建议至少构造一次带具体Date的 session，并断言收到相同的cost_reset_at，这样才能真正覆盖这次新增字段。Also applies to: 220-228, 564-572
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/rate-limit-guard.test.ts` around lines 159 - 167, Add a test that constructs a session with a concrete Date for cost_reset_at and then invokes the guard so that rateLimitServiceMock.checkCostLimitsWithLease is called; assert that the call includes the exact Date value (not null) in the cost_reset_at field. Specifically, in the existing unit test around rate-limit-guard.test.ts where checkCostLimitsWithLease is asserted, create a session object with cost_reset_at set to a new Date(...), run the same guard invocation (the code path that calls checkCostLimitsWithLease) and add an expectation that checkCostLimitsWithLease received that same Date for cost_reset_at to ensure the guard forwards non-null reset times.src/types/user.ts (1)
21-21:costResetAt不建议再引入undefined语义。这里已经把“未重置”表达成
null，再声明成可选字段会额外引入undefined这个未定义状态，和其他层里被归一化成Date | null的用法不一致。若undefined没有单独业务含义，建议改成必填的Date | null，或明确注释它与null的区别。Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined.
Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/user.ts` at line 21, The field costResetAt currently uses an optional property (costResetAt?: Date | null) which introduces an undefined state inconsistent with other layers that use Date | null; make it a required property with explicit nullable type by changing the declaration to costResetAt: Date | null (or, if undefined has a distinct business meaning, add a clarifying comment explaining that), and update any related types/usages (e.g., the other occurrence referenced) to expect Date | null to ensure consistency.src/actions/users.ts (1)
1828-1843: 变量命名keyHashes具有误导性，但缓存标识符一致性本身没有问题。检查结果表明缓存标识符使用是一致的：
usageLedger.key字段存储原始 API 密钥字符串（非哈希值）rate-limit-guard.ts:67向checkTotalCostLimit传递{ keyHash: key.key, ... }，即原始密钥RateLimitService用该原始值构建缓存键：total_cost:key:${options?.keyHash}clearUserCostCache接收相同的密钥列表，并使用相同标识符扫描删除：total_cost:key:${keyHash}因此，软重置和完全重置路径都能正确删除对应的总额度缓存。
建议： 将变量名从
keyHashes改为keyStrings或keyValues，避免误导（这些是原始密钥字符串，不是哈希值）。相同建议也适用于RateLimitService的options.keyHash参数名。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/users.ts` around lines 1828 - 1843, Rename misleading identifier keyHashes to keyStrings (or keyValues) in this user cost-reset flow: update the local variable declaration that maps keys.map((k) => k.key) to use keyStrings, update any downstream usage passed into clearUserCostCache({ userId, keyIds, keyStrings }) and update callers/consumers such as RateLimitService's options.keyHash to a clearer name (e.g., options.keyString) so the intent (these are raw API key strings, not hashes) is explicit; keep findKeyList, resetUserCostResetAt and clearUserCostCache behavior unchanged.src/repository/statistics.ts (1)
473-484: 截止日期计算逻辑正确但重复出现多次，建议抽取辅助函数。相同的 cutoff 计算模式（取
maxAgeDays和resetAt中较晚者）在文件中重复了 6 次以上（Lines 473-484, 506-517, 567-570, 644-647, 778-786, 854-862）。♻️ 建议抽取辅助函数减少重复
+/** + * 计算有效的截止时间：取 maxAgeDays 和 resetAt 中较晚的日期 + */ +function computeEffectiveCutoff( + maxAgeDays: number, + resetAt?: Date | null +): Date | null { + const maxAgeCutoff = + Number.isFinite(maxAgeDays) && maxAgeDays > 0 + ? new Date(Date.now() - Math.floor(maxAgeDays) * 24 * 60 * 60 * 1000) + : null; + + if (!(resetAt instanceof Date) || Number.isNaN(resetAt.getTime())) { + return maxAgeCutoff; + } + + return maxAgeCutoff && maxAgeCutoff > resetAt ? maxAgeCutoff : resetAt; +} export async function sumKeyTotalCost( keyHash: string, maxAgeDays: number = 365, resetAt?: Date | null ): Promise<number> { const conditions = [eq(usageLedger.key, keyHash), LEDGER_BILLING_CONDITION]; - // Use the more recent of resetAt and maxAgeDays cutoff - const maxAgeCutoff = - Number.isFinite(maxAgeDays) && maxAgeDays > 0 - ? new Date(Date.now() - Math.floor(maxAgeDays) * 24 * 60 * 60 * 1000) - : null; - let cutoff = maxAgeCutoff; - if (resetAt instanceof Date && !Number.isNaN(resetAt.getTime())) { - cutoff = maxAgeCutoff && maxAgeCutoff > resetAt ? maxAgeCutoff : resetAt; - } + const cutoff = computeEffectiveCutoff(maxAgeDays, resetAt); if (cutoff) { conditions.push(gte(usageLedger.createdAt, cutoff)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/statistics.ts` around lines 473 - 484, Extract the repeated cutoff calculation into a reusable helper (e.g., computeCutoff(maxAgeDays, resetAt)) that returns either a Date or null by performing the same logic currently inlined (compute maxAgeCutoff from maxAgeDays, pick the later of maxAgeCutoff and resetAt if resetAt is a valid Date), then replace every occurrence that sets cutoff and pushes gte(usageLedger.createdAt, cutoff) with a call to this helper and a single conditional that does conditions.push(gte(usageLedger.createdAt, cutoff)) when non-null; update all sites that use the inlined variables (maxAgeCutoff, cutoff, maxAgeDays, resetAt) to call the new function instead.tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts (1)
27-37: 缺少环境变量清理逻辑，可能导致测试污染。
beforeEach中修改了process.env，但没有对应的afterEach来恢复原始环境变量。这可能影响其他测试文件的执行。♻️ 建议添加 afterEach 清理
const originalEnv = process.env; beforeEach(() => { process.env = { ...originalEnv, ENABLE_API_KEY_REDIS_CACHE: "true", REDIS_URL: "redis://localhost:6379", ENABLE_RATE_LIMIT: "true", CI: "", NEXT_PHASE: "", }; }); + +afterEach(() => { + process.env = originalEnv; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts` around lines 27 - 37, The test modifies process.env in the beforeEach using originalEnv and then reassigns process.env for the test, but lacks cleanup; add an afterEach that restores process.env = originalEnv to avoid cross-test pollution—locate the beforeEach block and add a matching afterEach cleanup that reassigns the originalEnv captured by the originalEnv constant so tests don't leak environment changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/en/dashboard.json`:
- Around line 1564-1566: 将 "resetSection" 分组标题从 "Reset Data"
修改为更中性的文案以避免混淆“仅重置限额 (Reset Limits)”与“重置全部统计 (Reset Statistics)”的语义；在
messages/en/dashboard.json 中找到 "resetSection" 对象并将其 "title" 字段改为如 "Reset
Options" 或 "Reset & Cleanup"（任选其一），以明确该分组包含不同级别的重置操作。
In `@src/actions/key-quota.ts`:
- Around line 120-130: The limitTotal is still capped to the last 365 days
because the call uses a hardcoded 365 in sumKeyTotalCost; change this so
sumKeyTotalCost respects costResetAt (i.e., remove the hardcoded 365 and pass a
null/undefined days or update sumKeyTotalCost to ignore the days window when
costResetAt is provided) so limitTotal accumulates from costResetAt onward;
update the call site where sumKeyTotalCost(keyRow.key, 365, costResetAt) is
invoked and/or adjust sumKeyTotalCost implementation to treat a provided
costResetAt as the start of the accumulation period.
In `@src/actions/keys.ts`:
- Around line 755-766: The total-cost call is still hard-limited to 365 days;
update the sumKeyTotalCost invocation in the Promise.all tuple so it computes
the full accumulated cost since the key's last reset (use costResetAt) instead
of the fixed 365-day window — e.g. calculate the number of days (or pass an
unbounded flag) from costResetAt to now and pass that value (or the unbounded
parameter) into sumKeyTotalCost(key.key, ..., costResetAt) so limitTotal
reflects “since last reset”; locate the array destructuring around
sumKeyTotalCost and change the second argument currently set to 365 accordingly.
In `@src/actions/users.ts`:
- Around line 1903-1910: Current deletes and the call to
resetUserCostResetAt(userId, null) are separate writes and must be atomic; wrap
the deletion of messageRequest and usageLedger and the resetUserCostResetAt call
inside a single DB transaction so either all DB changes commit or none do. Keep
any Redis/ cache cleanup (e.g., clearUserCostCache) outside the transaction as
best-effort. Locate the operations on messageRequest, usageLedger and the call
resetUserCostResetAt and perform them within one db.transaction/transactional
context provided by your DB client.
In `@src/lib/rate-limit/lease-service.ts`:
- Line 46: The cache-backed lease currently omits costResetAt from the lease
freshness check, so cached leases can remain valid after a usage-reset; include
costResetAt (or its timestamp) as a field on the Lease object written into Redis
(the optional costResetAt?: Date | null) and update the cache-write path to
persist it, then update the cache-hit / validation logic that returns a lease to
compare the stored lease.costResetAt to the authoritative DB/reset timestamp
and, if they differ, treat the cache entry as stale and force a refresh
(skip/evict cached lease and reload from DB). Ensure the same comparison is
applied wherever leases are consumed so cached entries cannot be reused across
resets.
In `@src/lib/redis/cost-cache-cleanup.ts`:
- Around line 51-60: The various scanPattern(...) calls inside
clearUserCostCache silently swallow errors with catch(() => []); change each to
log a warning and return [] instead: in every occurrence of scanPattern(redis,
...) (including the patterns using userId, keyHash, keyId and the `total_cost` /
`lease` patterns) replace the empty catch with a catch(err => {
processLogger.warn(`Redis scan failed for pattern <pattern>`, { err, userId,
keyHash, keyId }); return []; }) so the warn includes the pattern and available
identifiers (userId, keyHash, keyId) and still returns [] for best-effort
behavior. Ensure you reference the same variables (redis, userId,
keyHashes/keyHash, keyIds/keyId) and the processLogger used elsewhere in the
file.
In `@tests/unit/actions/users-reset-limits-only.test.ts`:
- Around line 108-116: The test currently expects ERROR_CODES.PERMISSION_DENIED
when getSessionMock resolves to null; change the assertion to expect
ERROR_CODES.UNAUTHORIZED so it matches the repository pattern (missing session
-> UNAUTHORIZED). Specifically update the assertion referencing
resetUserLimitsOnly, getSessionMock, and ERROR_CODES to assert UNAUTHORIZED
instead of PERMISSION_DENIED.
In `@tests/unit/lib/redis/cost-cache-cleanup.test.ts`:
- Around line 40-45: The beforeEach currently calls vi.clearAllMocks(), which
only clears call history and allows changed mock implementations like
getRedisClientMock.mockReturnValue(null) to leak between tests; replace
vi.clearAllMocks() with vi.resetAllMocks() (or vi.restoreAllMocks() if using
spies) to reset mock implementations, and then re-establish the default mock
behaviors for getRedisClientMock, redisMock.status, redisPipelineMock.exec, and
scanPatternMock (e.g., set getRedisClientMock to return the default client and
redisPipelineMock.exec.mockResolvedValue([]),
scanPatternMock.mockResolvedValue([])) inside beforeEach so each test starts
with a clean, consistent mock state.
In `@tests/unit/repository/statistics-reset-at.test.ts`:
- Around line 45-265: The tests currently only assert returned mock values and
don’t verify that resetAt actually changed the query cutoff; update tests for
functions sumUserTotalCost, sumKeyTotalCost, sumUserTotalCostBatch,
sumKeyTotalCostBatchByIds, sumUserQuotaCosts, and sumKeyQuotaCostsById to assert
the DB/query layer was invoked with the expected cutoff (e.g., a where clause or
parameter containing the resetAt timestamp vs the maxAgeDays-derived date) by
checking the dbResultMock call args or by spying on the query-builder call that
constructs the cutoff, and add separate expectations for calls when resetAt is
valid, null/undefined, and invalid (NaN) to ensure correct branch selection.
---
Outside diff comments:
In `@src/lib/rate-limit/service.ts`:
- Around line 158-170: checkCostLimits currently reads Redis window keys
(cost_5h_rolling, cost_daily_*, cost_weekly, cost_monthly) without respecting
cost_reset_at, so if cost_reset_at is within the current window Redis cached
keys can cause incorrect rejections; update the fast path in checkCostLimits
(the code that reads those cost_* keys between the current window logic and the
DB fallback) to check cost_reset_at and, if present and falls inside the
relevant window, bypass those old window keys (either by: 1) including the reset
epoch into the Redis window key naming like total_cost:* so cached windows are
namespaced by reset epoch, or 2) short-circuiting the Redis fast path to force
the same DB-derived logic used by the fallback), and ensure this same check
applies to all places reading cost_5h_rolling, cost_daily_*, cost_weekly,
cost_monthly; also consider reusing the total_cost:* pattern and/or calling
clearUserCostCache semantics so the fast path behaves consistently with the DB
fallback.
In `@src/lib/security/api-key-auth-cache.ts`:
- Around line 172-182: The cache reconstruction silently ignores a failed parse
of costResetAt; mirror the existing checks for expiresAt/deletedAt by adding the
same validation for costResetAt: after const costResetAt =
parseOptionalDate(user.costResetAt); if (user.costResetAt != null &&
!costResetAt) return null; this ensures a non-null but unparsable
user.costResetAt invalidates the cache (use the same variables costResetAt and
user.costResetAt as in the snippet).
---
Nitpick comments:
In `@src/actions/users.ts`:
- Around line 1828-1843: Rename misleading identifier keyHashes to keyStrings
(or keyValues) in this user cost-reset flow: update the local variable
declaration that maps keys.map((k) => k.key) to use keyStrings, update any
downstream usage passed into clearUserCostCache({ userId, keyIds, keyStrings })
and update callers/consumers such as RateLimitService's options.keyHash to a
clearer name (e.g., options.keyString) so the intent (these are raw API key
strings, not hashes) is explicit; keep findKeyList, resetUserCostResetAt and
clearUserCostCache behavior unchanged.
In `@src/repository/statistics.ts`:
- Around line 473-484: Extract the repeated cutoff calculation into a reusable
helper (e.g., computeCutoff(maxAgeDays, resetAt)) that returns either a Date or
null by performing the same logic currently inlined (compute maxAgeCutoff from
maxAgeDays, pick the later of maxAgeCutoff and resetAt if resetAt is a valid
Date), then replace every occurrence that sets cutoff and pushes
gte(usageLedger.createdAt, cutoff) with a call to this helper and a single
conditional that does conditions.push(gte(usageLedger.createdAt, cutoff)) when
non-null; update all sites that use the inlined variables (maxAgeCutoff, cutoff,
maxAgeDays, resetAt) to call the new function instead.
In `@src/types/user.ts`:
- Line 21: The field costResetAt currently uses an optional property
(costResetAt?: Date | null) which introduces an undefined state inconsistent
with other layers that use Date | null; make it a required property with
explicit nullable type by changing the declaration to costResetAt: Date | null
(or, if undefined has a distinct business meaning, add a clarifying comment
explaining that), and update any related types/usages (e.g., the other
occurrence referenced) to expect Date | null to ensure consistency.
In `@tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts`:
- Around line 27-37: The test modifies process.env in the beforeEach using
originalEnv and then reassigns process.env for the test, but lacks cleanup; add
an afterEach that restores process.env = originalEnv to avoid cross-test
pollution—locate the beforeEach block and add a matching afterEach cleanup that
reassigns the originalEnv captured by the originalEnv constant so tests don't
leak environment changes.
In `@tests/unit/proxy/rate-limit-guard.test.ts`:
- Around line 159-167: Add a test that constructs a session with a concrete Date
for cost_reset_at and then invokes the guard so that
rateLimitServiceMock.checkCostLimitsWithLease is called; assert that the call
includes the exact Date value (not null) in the cost_reset_at field.
Specifically, in the existing unit test around rate-limit-guard.test.ts where
checkCostLimitsWithLease is asserted, create a session object with cost_reset_at
set to a new Date(...), run the same guard invocation (the code path that calls
checkCostLimitsWithLease) and add an expectation that checkCostLimitsWithLease
received that same Date for cost_reset_at to ensure the guard forwards non-null
reset times.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 902071c5-fb16-4fcd-b107-aa37b2857248
📒 Files selected for processing (35)
drizzle/0080_fresh_clint_barton.sqldrizzle/meta/0080_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/actions/key-quota.tssrc/actions/keys.tssrc/actions/my-usage.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/edit-user-dialog.tsxsrc/app/[locale]/dashboard/quotas/users/page.tsxsrc/app/v1/_lib/proxy/rate-limit-guard.tssrc/drizzle/schema.tssrc/lib/rate-limit/lease-service.tssrc/lib/rate-limit/service.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/security/api-key-auth-cache.tssrc/repository/_shared/transformers.tssrc/repository/key.tssrc/repository/statistics.tssrc/repository/user.tssrc/types/user.tstests/unit/actions/key-quota-concurrent-inherit.test.tstests/unit/actions/key-quota-cost-reset.test.tstests/unit/actions/my-usage-concurrent-inherit.test.tstests/unit/actions/total-usage-semantics.test.tstests/unit/actions/users-reset-all-statistics.test.tstests/unit/actions/users-reset-limits-only.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/lib/security/api-key-auth-cache-reset-at.test.tstests/unit/proxy/rate-limit-guard.test.tstests/unit/repository/statistics-reset-at.test.ts
Comment on lines
+108
to
+116
| test("should return PERMISSION_DENIED when no session", async () => { | ||
| getSessionMock.mockResolvedValue(null); | ||
|
|
||
| const { resetUserLimitsOnly } = await import("@/actions/users"); | ||
| const result = await resetUserLimitsOnly(123); | ||
|
|
||
| expect(result.ok).toBe(false); | ||
| expect(result.errorCode).toBe(ERROR_CODES.PERMISSION_DENIED); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== resetUserLimitsOnly implementation ==="
rg -n -C3 'export async function resetUserLimitsOnly|UNAUTHORIZED|PERMISSION_DENIED' src/actions/users.ts
echo
echo "=== null-session handling in nearby Server Actions ==="
rg -n -C2 'if \(!session\)|UNAUTHORIZED|PERMISSION_DENIED' src/actions -g '*.ts'Repository: ding113/claude-code-hub
Length of output: 39138
将 PERMISSION_DENIED 改为 UNAUTHORIZED 以符合仓库规范。
当前 resetUserLimitsOnly 的实现（src/actions/users.ts 第 1815-1821 行）使用 if (!session || session.user.role !== "admin") 的合并检查，直接返回 PERMISSION_DENIED。这与同文件中大多数其他函数的模式不一致：其他函数分别检查缺失 session（返回 UNAUTHORIZED）和权限不足（返回 PERMISSION_DENIED）。测试应改为期望 UNAUTHORIZED 以保持一致性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/users-reset-limits-only.test.ts` around lines 108 - 116,
The test currently expects ERROR_CODES.PERMISSION_DENIED when getSessionMock
resolves to null; change the assertion to expect ERROR_CODES.UNAUTHORIZED so it
matches the repository pattern (missing session -> UNAUTHORIZED). Specifically
update the assertion referencing resetUserLimitsOnly, getSessionMock, and
ERROR_CODES to assert UNAUTHORIZED instead of PERMISSION_DENIED.
Comment on lines
+45
to
+265
| describe("sumUserTotalCost", () => { | ||
| test("with valid resetAt -- queries DB and returns cost", async () => { | ||
| const resetAt = new Date("2026-02-15T00:00:00Z"); | ||
| dbResultMock.mockReturnValue([{ total: 42.5 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, resetAt); | ||
|
|
||
| expect(result).toBe(42.5); | ||
| }); | ||
|
|
||
| test("without resetAt -- uses maxAgeDays cutoff instead", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 100.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365); | ||
|
|
||
| expect(result).toBe(100.0); | ||
| }); | ||
|
|
||
| test("with null resetAt -- treated same as undefined", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 50.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, null); | ||
|
|
||
| expect(result).toBe(50.0); | ||
| }); | ||
|
|
||
| test("with invalid Date (NaN) -- skips resetAt, falls through to maxAgeDays", async () => { | ||
| const invalidDate = new Date("invalid"); | ||
| dbResultMock.mockReturnValue([{ total: 75.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, invalidDate); | ||
|
|
||
| expect(result).toBe(75.0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumKeyTotalCost", () => { | ||
| test("with valid resetAt -- uses resetAt instead of maxAgeDays cutoff", async () => { | ||
| const resetAt = new Date("2026-02-20T00:00:00Z"); | ||
| dbResultMock.mockReturnValue([{ total: 15.0 }]); | ||
|
|
||
| const { sumKeyTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCost("sk-hash", 365, resetAt); | ||
|
|
||
| expect(result).toBe(15.0); | ||
| }); | ||
|
|
||
| test("without resetAt -- falls back to maxAgeDays", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 30.0 }]); | ||
|
|
||
| const { sumKeyTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCost("sk-hash", 365); | ||
|
|
||
| expect(result).toBe(30.0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumUserTotalCostBatch", () => { | ||
| test("with resetAtMap -- splits users: individual queries for reset users", async () => { | ||
| const resetAtMap = new Map([[10, new Date("2026-02-15T00:00:00Z")]]); | ||
| // Calls: 1) individual sumUserTotalCost(10) => where => [{ total: 25 }] | ||
| // 2) batch for user 20 => groupBy => [{ userId: 20, total: 50 }] | ||
| dbResultMock | ||
| .mockReturnValueOnce([{ total: 25.0 }]) | ||
| .mockReturnValueOnce([{ userId: 20, total: 50.0 }]); | ||
|
|
||
| const { sumUserTotalCostBatch } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCostBatch([10, 20], 365, resetAtMap); | ||
|
|
||
| expect(result.get(10)).toBe(25.0); | ||
| expect(result.get(20)).toBe(50.0); | ||
| }); | ||
|
|
||
| test("with empty resetAtMap -- single batch query for all users", async () => { | ||
| dbResultMock.mockReturnValue([ | ||
| { userId: 10, total: 25.0 }, | ||
| { userId: 20, total: 50.0 }, | ||
| ]); | ||
|
|
||
| const { sumUserTotalCostBatch } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCostBatch([10, 20], 365, new Map()); | ||
|
|
||
| expect(result.get(10)).toBe(25.0); | ||
| expect(result.get(20)).toBe(50.0); | ||
| }); | ||
|
|
||
| test("empty userIds -- returns empty map immediately", async () => { | ||
| const { sumUserTotalCostBatch } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCostBatch([], 365); | ||
|
|
||
| expect(result.size).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumKeyTotalCostBatchByIds", () => { | ||
| test("with resetAtMap -- splits keys into individual vs batch", async () => { | ||
| const resetAtMap = new Map([[1, new Date("2026-02-15T00:00:00Z")]]); | ||
| dbResultMock | ||
| // 1) PK lookup: key strings | ||
| .mockReturnValueOnce([ | ||
| { id: 1, key: "sk-a" }, | ||
| { id: 2, key: "sk-b" }, | ||
| ]) | ||
| // 2) individual sumKeyTotalCost for key 1 | ||
| .mockReturnValueOnce([{ total: 10.0 }]) | ||
| // 3) batch for key 2 | ||
| .mockReturnValueOnce([{ key: "sk-b", total: 20.0 }]); | ||
|
|
||
| const { sumKeyTotalCostBatchByIds } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCostBatchByIds([1, 2], 365, resetAtMap); | ||
|
|
||
| expect(result.get(1)).toBe(10.0); | ||
| expect(result.get(2)).toBe(20.0); | ||
| }); | ||
|
|
||
| test("empty keyIds -- returns empty map immediately", async () => { | ||
| const { sumKeyTotalCostBatchByIds } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCostBatchByIds([], 365); | ||
|
|
||
| expect(result.size).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumUserQuotaCosts", () => { | ||
| const ranges = { | ||
| range5h: { | ||
| startTime: new Date("2026-03-01T07:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeDaily: { | ||
| startTime: new Date("2026-03-01T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeWeekly: { | ||
| startTime: new Date("2026-02-23T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeMonthly: { | ||
| startTime: new Date("2026-02-01T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| }; | ||
|
|
||
| test("with resetAt -- returns correct cost summary", async () => { | ||
| const resetAt = new Date("2026-02-25T00:00:00Z"); | ||
| dbResultMock.mockReturnValue([ | ||
| { | ||
| cost5h: "1.0", | ||
| costDaily: "2.0", | ||
| costWeekly: "3.0", | ||
| costMonthly: "4.0", | ||
| costTotal: "5.0", | ||
| }, | ||
| ]); | ||
|
|
||
| const { sumUserQuotaCosts } = await import("@/repository/statistics"); | ||
| const result = await sumUserQuotaCosts(10, ranges, 365, resetAt); | ||
|
|
||
| expect(result.cost5h).toBe(1.0); | ||
| expect(result.costDaily).toBe(2.0); | ||
| expect(result.costWeekly).toBe(3.0); | ||
| expect(result.costMonthly).toBe(4.0); | ||
| expect(result.costTotal).toBe(5.0); | ||
| }); | ||
|
|
||
| test("without resetAt -- uses only maxAgeDays cutoff", async () => { | ||
| dbResultMock.mockReturnValue([ | ||
| { cost5h: "0", costDaily: "0", costWeekly: "0", costMonthly: "0", costTotal: "0" }, | ||
| ]); | ||
|
|
||
| const { sumUserQuotaCosts } = await import("@/repository/statistics"); | ||
| const result = await sumUserQuotaCosts(10, ranges, 365); | ||
|
|
||
| expect(result.cost5h).toBe(0); | ||
| expect(result.costTotal).toBe(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumKeyQuotaCostsById", () => { | ||
| test("with resetAt -- same cutoff logic as sumUserQuotaCosts", async () => { | ||
| const resetAt = new Date("2026-02-25T00:00:00Z"); | ||
| const ranges = { | ||
| range5h: { | ||
| startTime: new Date("2026-03-01T07:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeDaily: { | ||
| startTime: new Date("2026-03-01T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeWeekly: { | ||
| startTime: new Date("2026-02-23T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| rangeMonthly: { | ||
| startTime: new Date("2026-02-01T00:00:00Z"), | ||
| endTime: new Date("2026-03-01T12:00:00Z"), | ||
| }, | ||
| }; | ||
| // First: getKeyStringByIdCached lookup, then main query | ||
| dbResultMock.mockReturnValueOnce([{ key: "sk-test-hash" }]).mockReturnValueOnce([ | ||
| { | ||
| cost5h: "2.0", | ||
| costDaily: "4.0", | ||
| costWeekly: "6.0", | ||
| costMonthly: "8.0", | ||
| costTotal: "10.0", | ||
| }, | ||
| ]); | ||
|
|
||
| const { sumKeyQuotaCostsById } = await import("@/repository/statistics"); | ||
| const result = await sumKeyQuotaCostsById(42, ranges, 365, resetAt); | ||
|
|
||
| expect(result.cost5h).toBe(2.0); | ||
| expect(result.costTotal).toBe(10.0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
这些测试没有真正校验 resetAt 是否影响查询。
当前断言基本都只看 dbResultMock 返回了什么；无论传不传 resetAt，mock 链都会返回同一类结果，所以即使仓库层完全忽略了 resetAt，这组用例也大概率还是绿的。这里至少要断言生成的 cutoff/查询分支发生了变化，否则这套测试只是在验证 mock，而不是验证新语义。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/repository/statistics-reset-at.test.ts` around lines 45 - 265, The
tests currently only assert returned mock values and don’t verify that resetAt
actually changed the query cutoff; update tests for functions sumUserTotalCost,
sumKeyTotalCost, sumUserTotalCostBatch, sumKeyTotalCostBatchByIds,
sumUserQuotaCosts, and sumKeyQuotaCostsById to assert the DB/query layer was
invoked with the expected cutoff (e.g., a where clause or parameter containing
the resetAt timestamp vs the maxAgeDays-derived date) by checking the
dbResultMock call args or by spying on the query-builder call that constructs
the cutoff, and add separate expectations for calls when resetAt is valid,
null/undefined, and invalid (NaN) to ensure correct branch selection.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return []; | ||
| }), | ||
| // Total cost cache keys (with optional resetAt suffix) | ||
| scanPattern(redis, `total_cost:user:${userId}`).catch(() => []), |
Contributor
There was a problem hiding this comment.
[CRITICAL] [ERROR-SILENT] Redis scan failures are swallowed (no log), risking stale cost/lease cache after resets
Evidence (src/lib/redis/cost-cache-cleanup.ts:25,52-60):
- Doc says:
Never throws -- logs errors internally. - But several scans silently ignore failures:
scanPattern(redis, \total_cost:user:${userId}`).catch(() => [])`
Why this is a problem: If any of these scans fail, resetUserLimitsOnly / resetUserAllStatistics can leave stale total_cost:* or lease:* keys behind. That can make a reset appear to succeed while rate limiting still uses old cached values.
Suggested fix:
const scanOrWarn = (pattern: string) =>
scanPattern(redis, pattern).catch((error) => {
logger.warn("Failed to scan Redis pattern during cost cache cleanup", {
userId,
pattern,
error: error instanceof Error ? error.message : String(error),
});
return [] as string[];
});
const scanResults = await Promise.all([
// ...existing key/user cost patterns...
scanOrWarn(`total_cost:user:${userId}`),
scanOrWarn(`total_cost:user:${userId}:*`),
...keyHashes.map((keyHash) => scanOrWarn(`total_cost:key:${keyHash}`)),
...keyHashes.map((keyHash) => scanOrWarn(`total_cost:key:${keyHash}:*`)),
...keyIds.map((keyId) => scanOrWarn(`lease:key:${keyId}:*`)),
scanOrWarn(`lease:user:${userId}:*`),
]);
|
|
||
| // Verify quota aggregation uses the constant for all-time usage | ||
| expect(content).toMatch(/sumUserQuotaCosts\([^)]*ALL_TIME_MAX_AGE_DAYS\s*\)/); | ||
| expect(content).toMatch(/sumUserQuotaCosts\([\s\S]*?ALL_TIME_MAX_AGE_DAYS/); |
Contributor
There was a problem hiding this comment.
[MEDIUM] [TEST-BRITTLE] Source-code string assertions will fail on harmless refactors
Evidence (tests/unit/actions/total-usage-semantics.test.ts:251-277):
const content = await fs.readFile(myUsagePath, "utf-8");expect(content).toMatch(/sumUserQuotaCosts\([\s\S]*?ALL_TIME_MAX_AGE_DAYS/);
Why this is a problem: This couples the test to source formatting/identifiers and breaks on refactors that keep behavior identical. The mock-based assertions earlier in this file already cover the all-time semantics.
Suggested fix (remove the source-code matching block and rely on behavior assertions):
- describe("source code verification", () => {
- it("should verify sumUserCost passes ALL_TIME_MAX_AGE_DAYS when period is total", async () => {
- const fs = await import("node:fs/promises");
- const path = await import("node:path");
- const myUsagePath = path.join(process.cwd(), "src/actions/my-usage.ts");
- const content = await fs.readFile(myUsagePath, "utf-8");
- expect(content).toContain("const ALL_TIME_MAX_AGE_DAYS = Infinity");
- expect(content).toMatch(/sumUserQuotaCosts\([\s\S]*?ALL_TIME_MAX_AGE_DAYS/);
- expect(content).toMatch(/sumKeyQuotaCostsById\([\s\S]*?ALL_TIME_MAX_AGE_DAYS/);
- });
-
- it("should verify getUserAllLimitUsage passes ALL_TIME_MAX_AGE_DAYS", async () => {
- const fs = await import("node:fs/promises");
- const path = await import("node:path");
- const usersPath = path.join(process.cwd(), "src/actions/users.ts");
- const content = await fs.readFile(usersPath, "utf-8");
- expect(content).toContain("const ALL_TIME_MAX_AGE_DAYS = Infinity");
- expect(content).toMatch(/sumUserTotalCost\(userId,\s*ALL_TIME_MAX_AGE_DAYS/);
- });
- });
+
+ // Source-level string matching removed: behavior is covered by the mock-based tests above.
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR is XL and spans DB schema, repository, rate-limit pipeline, Redis cache cleanup, dashboard UI, and tests; that increases regression risk and makes it harder to validate reset semantics end-to-end. The most urgent issue is silent Redis scan failures during cache cleanup, which can leave stale cost/lease keys after a “reset” while still reporting success.
PR Size: XL
- Lines changed: 6219
- Files changed: 35
- Split suggestions:
- DB schema + repository + migrations (
src/drizzle/schema.ts,src/repository/*,drizzle/*) - Rate-limit pipeline + Redis cleanup (
src/lib/rate-limit/*,src/lib/redis/*,src/app/v1/_lib/proxy/*) - Dashboard UI + i18n (
src/app/[locale]/dashboard/**,messages/**) - Tests-only PR (
tests/unit/**)
- DB schema + repository + migrations (
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/lib/redis/cost-cache-cleanup.ts:52[ERROR-SILENT] SilentscanPattern(...).catch(() => [])leaves stale reset-related Redis keys (Confidence: 100)
High Priority Issues (Should Fix)
tests/unit/actions/total-usage-semantics.test.ts:261[TEST-BRITTLE] Source-code string assertions (fs.readFile+ regex) break on refactors (Confidence: 80)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR introduces costResetAt, a soft user limit reset mechanism that preserves historical usage data while resetting cost calculations. The implementation is well-structured with comprehensive test coverage (10 new test files). However, one critical data consistency issue must be addressed before merging.
PR Size: XL
- Lines changed: 6,219 (5,988 additions + 231 deletions)
- Files changed: 35
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 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 |
Critical Issues (Must Fix)
1. Missing Transaction in resetUserAllStatistics (src/actions/users.ts)
The resetUserAllStatistics function executes three separate database write operations without wrapping them in a transaction:
// Line 1904
await db.delete(messageRequest).where(eq(messageRequest.userId, userId));
// Line 1907
await db.delete(usageLedger).where(eq(usageLedger.userId, userId));
// Line 1910
await resetUserCostResetAt(userId, null);Impact: If any operation fails mid-execution, the database is left in an inconsistent state:
- If DELETE
usageLedgerfails after DELETEmessageRequestsucceeds: request logs are gone but ledger rows remain - If
resetUserCostResetAtfails: user'scostResetAttimestamp remains stale while their data is already deleted
Fix: Wrap all DB writes in a transaction (the codebase already uses db.transaction() pattern elsewhere, e.g., line 905):
await db.transaction(async (tx) => {
await tx.delete(messageRequest).where(eq(messageRequest.userId, userId));
await tx.delete(usageLedger).where(eq(usageLedger.userId, userId));
await tx
.update(usersTable)
.set({ costResetAt: null, updatedAt: new Date() })
.where(and(eq(usersTable.id, userId), isNull(usersTable.deletedAt)));
});Review Coverage
- Logic and correctness - Issue found
- Security (OWASP Top 10) - Clean
- Error handling - Clean (Redis errors properly caught and logged)
- Type safety - Clean (
costResetAt?: Date | nullproperly typed) - Documentation accuracy - Clean (comments match implementation)
- Test coverage - Good (10 new test files covering main scenarios)
- Code clarity - Clean (follows existing patterns)
Positive Observations
- i18n coverage is complete for all 5 languages (en, zh-CN, zh-TW, ja, ru)
- No emoji in code (complies with CLAUDE.md)
- The
clearUserCostCacheutility is well-designed with proper error handling costResetAtclipping logic is consistently applied across all rate-limit checks
Automated review by Claude AI
Contributor
There was a problem hiding this comment.
- Detected PR
#890(feat: add costResetAt for soft user limit reset without deleting data) - Applied label
size/XL - Posted 2 inline review comments (diff-only):
src/lib/redis/cost-cache-cleanup.ts:52[CRITICAL] [ERROR-SILENT] silentscanPattern(...).catch(() => [])can leave stale reset-related Redis keystests/unit/actions/total-usage-semantics.test.ts:261[MEDIUM] [TEST-BRITTLE]fs.readFile+ regex source assertions break on refactors
- Submitted the required PR review summary via
gh pr review
miraserver
pushed a commit
to miraserver/claude-code-hub
that referenced
this pull request
Mar 9, 2026
… cache validation, scan logging - Wrap resetUserAllStatistics DB operations in atomic transaction (tx.delete + tx.update) - Add missing costResetAt validation in hydrateUserFromCache (matching expiresAt/deletedAt pattern) - Add logger.warn to 6 silent .catch() handlers in cost-cache-cleanup scanPattern calls - Update structural tests for tx.delete pattern inside transaction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Suggested fixes for 3 confirmed review findingsAfter deep validation of all automated review comments (CodeRabbit, Gemini, Greptile, Claude AI, Codex AI), I identified 3 real issues and excluded 1 false positive (Redis fast path in Fix 1: Transaction atomicity in
|
Critical: - Wrap resetUserAllStatistics DB writes in transaction for atomicity - Change sumKeyTotalCost maxAgeDays from 365 to Infinity for unbounded accumulation from costResetAt - Add costResetAtMs to BudgetLease cache with stale detection Medium: - Add logger.warn to silent Redis scan failure handlers - Add fail-open documentation for costResetAt validation - Fix test mock leak (vi.resetAllMocks + re-establish defaults) Minor: - Rename i18n "Reset Data" to "Reset Options" across 5 languages - Remove brittle source code string assertion tests - Update test assertions for transaction and Infinity changes
Comment on lines
+52
to
+85
| scanPattern(redis, `total_cost:user:${userId}`).catch((err) => { | ||
| logger.warn("Failed to scan total cost pattern", { | ||
| userId, | ||
| pattern: `total_cost:user:${userId}`, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; | ||
| }), | ||
| scanPattern(redis, `total_cost:user:${userId}:*`).catch((err) => { | ||
| logger.warn("Failed to scan total cost pattern", { | ||
| userId, | ||
| pattern: `total_cost:user:${userId}:*`, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; | ||
| }), | ||
| ...keyHashes.map((keyHash) => | ||
| scanPattern(redis, `total_cost:key:${keyHash}`).catch((err) => { | ||
| logger.warn("Failed to scan total cost key pattern", { | ||
| keyHash, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; | ||
| }) | ||
| ), | ||
| ...keyHashes.map((keyHash) => | ||
| scanPattern(redis, `total_cost:key:${keyHash}:*`).catch((err) => { | ||
| logger.warn("Failed to scan total cost key pattern", { | ||
| keyHash, | ||
| error: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| return []; | ||
| }) | ||
| ), |
There was a problem hiding this comment.
scanPattern used for known exact keys — unnecessary overhead
Lines 52–59 and 69–75 use scanPattern for keys that have no glob wildcards and are fully determined at call time (e.g., exact user and key cost cache keys). Running a full-keyspace Redis SCAN to locate a single known key can require many round-trips on large Redis instances.
Since these exact-key patterns are already fully known, they can be added directly to the pipeline via direct DEL commands instead of using SCAN. The wildcard suffix patterns (the :* variants at lines 60–67 and 77–85) do need scanPattern to enumerate matching keys, but the exact-match keys do not.
Separating exact-key deletes from wildcard scans would be more efficient and make the intent clearer — avoid unnecessary SCAN overhead for predictable keys.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/redis/cost-cache-cleanup.ts
Line: 52-85
Comment:
**`scanPattern` used for known exact keys — unnecessary overhead**
Lines 52–59 and 69–75 use `scanPattern` for keys that have no glob wildcards and are fully determined at call time (e.g., exact user and key cost cache keys). Running a full-keyspace Redis SCAN to locate a single known key can require many round-trips on large Redis instances.
Since these exact-key patterns are already fully known, they can be added directly to the pipeline via direct DEL commands instead of using SCAN. The wildcard suffix patterns (the `:*` variants at lines 60–67 and 77–85) do need `scanPattern` to enumerate matching keys, but the exact-match keys do not.
Separating exact-key deletes from wildcard scans would be more efficient and make the intent clearer — avoid unnecessary SCAN overhead for predictable keys.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/actions/users-reset-all-statistics.test.ts (1)
147-148:⚠️ Potential issue | 🟡 Minor给成功路径的 key fixture 补上
key字段。实现会把
keyHashes = keys.map((k) => k.key)传给clearUserCostCache({ includeActiveSessions: true })。这些 fixture 只有id，所以传进去的其实是undefined[]，active-session / key-hash 相关清理分支并没有被真实覆盖。建议把 mock 改成带真实key字符串的对象，并补一个对清理入参的断言。As per coding guidelines,
**/*.test.{ts,tsx,js,jsx}: All new features must have unit test coverage of at least 80%.Also applies to: 172-173, 188-189, 225-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/users-reset-all-statistics.test.ts` around lines 147 - 148, The test fixtures for findKeyListMock must include a real key string because the implementation builds keyHashes = keys.map(k => k.key) and then passes those to clearUserCostCache({ includeActiveSessions: true }), so update the mocked responses (e.g., the objects returned by findKeyListMock.mockResolvedValue at the spots using findKeyListMock) to include a key property like { id: 1, key: "key:1" } and similarly for other occurrences (the blocks analogous at the other mock sites), and add an assertion that clearUserCostCache was called with the expected keyHashes and includeActiveSessions: true to ensure the active-session/key-hash cleanup branch is actually covered.
♻️ Duplicate comments (1)
tests/unit/repository/statistics-reset-at.test.ts (1)
45-271:⚠️ Potential issue | 🟠 Major这些
resetAt用例还没有锁住真正的 cutoff 语义。当前断言基本都只看 mock 返回值和是否触达 DB；如果仓库层继续走
maxAgeDays，甚至完全忽略resetAt，这组测试大多也会继续通过。建议让chain()记录where()/ cutoff 参数，明确断言 valid / null / invalidresetAt的分支，以及 batch 场景里“带 reset 的实体走单查、其余实体走批量查”。 As per coding guidelines,**/*.test.{ts,tsx,js,jsx}: All new features must have unit test coverage of at least 80%.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/repository/statistics-reset-at.test.ts` around lines 45 - 271, Tests currently only assert mock return values, not that resetAt actually changes the DB cutoff; update the tests to spy/capture the query builder chain() (or the db call) and assert the produced where()/cutoff arguments for each branch: for sumUserTotalCost and sumKeyTotalCost assert that a valid Date resetAt results in a where cutoff equal to that resetAt, while undefined/null/invalid Date falls back to the maxAgeDays cutoff; for sumUserTotalCostBatch and sumKeyTotalCostBatchByIds assert that entries present in resetAtMap invoke individual lookups (single-key queries whose where uses that resetAt) and the remaining IDs/keys go to the batch query (groupBy) with the maxAgeDays cutoff; do the same for sumUserQuotaCosts and sumKeyQuotaCostsById (including getKeyStringByIdCached lookup) by recording the where arguments and adding explicit expect(...) checks for the cutoff values rather than only checking dbResultMock was called.
🧹 Nitpick comments (1)
tests/unit/usage-ledger/cleanup-immunity.test.ts (1)
32-40: LGTM! 模式匹配更新正确。使用更宽泛的
.delete(usageLedger)模式能同时捕获事务内（tx.delete）和直接调用（db.delete）两种情况，提高了测试的健壮性。可选改进：保持第 21 行的一致性
第 21 行仍使用
db.delete(usageLedger)检查，而第 34、37 行已改用更宽泛的.delete(usageLedger)模式。虽然第 32-40 行的综合检查已确保只有一条删除路径，但为保持一致性可考虑更新：it("removeUser does not delete from usageLedger", () => { const removeUserMatch = usersTs.match(/export async function removeUser[\s\S]*?^}/m); expect(removeUserMatch).not.toBeNull(); const removeUserBody = removeUserMatch![0]; - expect(removeUserBody).not.toContain("db.delete(usageLedger)"); + expect(removeUserBody).not.toContain(".delete(usageLedger)"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/usage-ledger/cleanup-immunity.test.ts` around lines 32 - 40, The test mixes a narrow check ("db.delete(usageLedger)") with broader checks using the pattern /.delete(usageLedger)/; update the remaining narrow assertion to use the same broader pattern so all checks are consistent: replace the literal "db.delete(usageLedger)" assertion with a check that searches usersTs for ".delete(usageLedger)" (same regex used by usersTs.matchAll) and keep the existing assertions around usersTs, deleteIndex and resetUserAllStatistics unchanged.
🤖 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/security/api-key-auth-cache.ts`:
- Around line 170-183: The code silently treats an invalid costResetAt as
“unset”, which can cause stale quota windows to persist; update the validation
to mirror expiresAt/deletedAt by checking parseOptionalDate for costResetAt and
returning null on invalid data (i.e., add a guard like: if (user.costResetAt !=
null && !costResetAt) return null), and keep the returned costResetAt assignment
consistent with the other date fields (use costResetAt === undefined ? undefined
: costResetAt); locate parseOptionalDate and the return object in
api-key-auth-cache.ts to apply this change.
In `@tests/unit/actions/users-reset-all-statistics.test.ts`:
- Around line 25-31: The test is mocking resetUserCostResetAt
(resetUserCostResetAtMock) which is no longer used by resetUserAllStatistics and
misses asserting the new behavior invalidateCachedUser; remove the unused
resetUserCostResetAtMock from the vi.mock replacement so the mock doesn't claim
coverage for a non-existent call, ensure the module mock includes
invalidateCachedUser replaced by invalidateCachedUserMock (or add that mock if
missing), and update at least one successful-case test for
resetUserAllStatistics to assert invalidateCachedUserMock was called with the
expected userId (use findUserByIdMock to locate the userId or the same fixture)
so the new cache-invalidation behavior is actually verified; apply the same
change pattern to the other affected test blocks referencing
resetUserCostResetAtMock/invalidateCachedUserMock.
---
Outside diff comments:
In `@tests/unit/actions/users-reset-all-statistics.test.ts`:
- Around line 147-148: The test fixtures for findKeyListMock must include a real
key string because the implementation builds keyHashes = keys.map(k => k.key)
and then passes those to clearUserCostCache({ includeActiveSessions: true }), so
update the mocked responses (e.g., the objects returned by
findKeyListMock.mockResolvedValue at the spots using findKeyListMock) to include
a key property like { id: 1, key: "key:1" } and similarly for other occurrences
(the blocks analogous at the other mock sites), and add an assertion that
clearUserCostCache was called with the expected keyHashes and
includeActiveSessions: true to ensure the active-session/key-hash cleanup branch
is actually covered.
---
Duplicate comments:
In `@tests/unit/repository/statistics-reset-at.test.ts`:
- Around line 45-271: Tests currently only assert mock return values, not that
resetAt actually changes the DB cutoff; update the tests to spy/capture the
query builder chain() (or the db call) and assert the produced where()/cutoff
arguments for each branch: for sumUserTotalCost and sumKeyTotalCost assert that
a valid Date resetAt results in a where cutoff equal to that resetAt, while
undefined/null/invalid Date falls back to the maxAgeDays cutoff; for
sumUserTotalCostBatch and sumKeyTotalCostBatchByIds assert that entries present
in resetAtMap invoke individual lookups (single-key queries whose where uses
that resetAt) and the remaining IDs/keys go to the batch query (groupBy) with
the maxAgeDays cutoff; do the same for sumUserQuotaCosts and
sumKeyQuotaCostsById (including getKeyStringByIdCached lookup) by recording the
where arguments and adding explicit expect(...) checks for the cutoff values
rather than only checking dbResultMock was called.
---
Nitpick comments:
In `@tests/unit/usage-ledger/cleanup-immunity.test.ts`:
- Around line 32-40: The test mixes a narrow check ("db.delete(usageLedger)")
with broader checks using the pattern /.delete(usageLedger)/; update the
remaining narrow assertion to use the same broader pattern so all checks are
consistent: replace the literal "db.delete(usageLedger)" assertion with a check
that searches usersTs for ".delete(usageLedger)" (same regex used by
usersTs.matchAll) and keep the existing assertions around usersTs, deleteIndex
and resetUserAllStatistics unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a124230-9572-4a3d-a317-02df5d50a673
📒 Files selected for processing (19)
messages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/actions/key-quota.tssrc/actions/keys.tssrc/actions/users.tssrc/lib/rate-limit/lease-service.tssrc/lib/rate-limit/lease.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/security/api-key-auth-cache.tstests/unit/actions/key-quota-cost-reset.test.tstests/unit/actions/total-usage-semantics.test.tstests/unit/actions/users-reset-all-statistics.test.tstests/unit/actions/users-reset-limits-only.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/repository/statistics-reset-at.test.tstests/unit/usage-ledger/cleanup-immunity.test.ts
✅ Files skipped from review due to trivial changes (1)
- messages/zh-TW/dashboard.json
🚧 Files skipped from review as they are similar to previous changes (6)
- messages/ja/dashboard.json
- tests/unit/actions/total-usage-semantics.test.ts
- tests/unit/actions/users-reset-limits-only.test.ts
- tests/unit/actions/key-quota-cost-reset.test.ts
- messages/en/dashboard.json
- src/actions/keys.ts
Comment on lines
170
to
+183
| const expiresAt = parseOptionalDate(user.expiresAt); | ||
| const deletedAt = parseOptionalDate(user.deletedAt); | ||
| const costResetAt = parseOptionalDate(user.costResetAt); | ||
| if (user.expiresAt != null && !expiresAt) return null; | ||
| if (user.deletedAt != null && !deletedAt) return null; | ||
| // costResetAt: intentional fail-open on invalid date -- affects quota counting window, not access control | ||
|
|
||
| return { | ||
| ...(payload.user as User), | ||
| createdAt, | ||
| updatedAt, | ||
| expiresAt: expiresAt === undefined ? undefined : expiresAt, | ||
| deletedAt: deletedAt === undefined ? undefined : deletedAt, | ||
| costResetAt: costResetAt === undefined ? undefined : costResetAt, |
There was a problem hiding this comment.
无效的 costResetAt 不应被静默降级为 null。
expiresAt / deletedAt 遇到坏缓存都会直接 miss，但这里会把非法 costResetAt 当成“未设置 reset”。一旦 Redis 里出现坏值，后续限额窗口会错误地回溯到历史消费，管理员刚执行的“仅重置限额”在缓存 TTL 内仍可能继续按旧数据限流。
建议修改
const expiresAt = parseOptionalDate(user.expiresAt);
const deletedAt = parseOptionalDate(user.deletedAt);
const costResetAt = parseOptionalDate(user.costResetAt);
if (user.expiresAt != null && !expiresAt) return null;
if (user.deletedAt != null && !deletedAt) return null;
- // costResetAt: intentional fail-open on invalid date -- affects quota counting window, not access control
+ if (user.costResetAt != null && !costResetAt) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const expiresAt = parseOptionalDate(user.expiresAt); | |
| const deletedAt = parseOptionalDate(user.deletedAt); | |
| const costResetAt = parseOptionalDate(user.costResetAt); | |
| if (user.expiresAt != null && !expiresAt) return null; | |
| if (user.deletedAt != null && !deletedAt) return null; | |
| // costResetAt: intentional fail-open on invalid date -- affects quota counting window, not access control | |
| return { | |
| ...(payload.user as User), | |
| createdAt, | |
| updatedAt, | |
| expiresAt: expiresAt === undefined ? undefined : expiresAt, | |
| deletedAt: deletedAt === undefined ? undefined : deletedAt, | |
| costResetAt: costResetAt === undefined ? undefined : costResetAt, | |
| const expiresAt = parseOptionalDate(user.expiresAt); | |
| const deletedAt = parseOptionalDate(user.deletedAt); | |
| const costResetAt = parseOptionalDate(user.costResetAt); | |
| if (user.expiresAt != null && !expiresAt) return null; | |
| if (user.deletedAt != null && !deletedAt) return null; | |
| if (user.costResetAt != null && !costResetAt) return null; | |
| return { | |
| ...(payload.user as User), | |
| createdAt, | |
| updatedAt, | |
| expiresAt: expiresAt === undefined ? undefined : expiresAt, | |
| deletedAt: deletedAt === undefined ? undefined : deletedAt, | |
| costResetAt: costResetAt === undefined ? undefined : costResetAt, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/security/api-key-auth-cache.ts` around lines 170 - 183, The code
silently treats an invalid costResetAt as “unset”, which can cause stale quota
windows to persist; update the validation to mirror expiresAt/deletedAt by
checking parseOptionalDate for costResetAt and returning null on invalid data
(i.e., add a guard like: if (user.costResetAt != null && !costResetAt) return
null), and keep the returned costResetAt assignment consistent with the other
date fields (use costResetAt === undefined ? undefined : costResetAt); locate
parseOptionalDate and the return object in api-key-auth-cache.ts to apply this
change.
Comment on lines
+25
to
+31
| const resetUserCostResetAtMock = vi.fn(); | ||
| vi.mock("@/repository/user", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("@/repository/user")>(); | ||
| return { | ||
| ...actual, | ||
| findUserById: findUserByIdMock, | ||
| resetUserCostResetAt: resetUserCostResetAtMock, |
There was a problem hiding this comment.
校准测试依赖，避免覆盖目标和实现脱节。
resetUserAllStatistics() 现在是在事务里直接清空 costResetAt，提交后再调用 invalidateCachedUser(userId)；这里却 mock 了不会走到的 resetUserCostResetAt，同时没有对真正的新行为 invalidateCachedUser 做任何断言。这样即使把 auth cache 失效逻辑删掉，成功路径测试也仍然会通过。建议删掉 resetUserCostResetAtMock，并在至少一个成功用例里断言 invalidateCachedUserMock 被调用。
As per coding guidelines, **/*.test.{ts,tsx,js,jsx}: All new features must have unit test coverage of at least 80%.
Also applies to: 73-77, 100-109, 144-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/users-reset-all-statistics.test.ts` around lines 25 - 31,
The test is mocking resetUserCostResetAt (resetUserCostResetAtMock) which is no
longer used by resetUserAllStatistics and misses asserting the new behavior
invalidateCachedUser; remove the unused resetUserCostResetAtMock from the
vi.mock replacement so the mock doesn't claim coverage for a non-existent call,
ensure the module mock includes invalidateCachedUser replaced by
invalidateCachedUserMock (or add that mock if missing), and update at least one
successful-case test for resetUserAllStatistics to assert
invalidateCachedUserMock was called with the expected userId (use
findUserByIdMock to locate the userId or the same fixture) so the new
cache-invalidation behavior is actually verified; apply the same change pattern
to the other affected test blocks referencing
resetUserCostResetAtMock/invalidateCachedUserMock.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR merges PR #853 into the
fix/user-reset-statsbranch and introducescostResetAt— a soft user limit reset mechanism that allows administrators to reset rate limits without deleting historical usage data. Unlike the destructive "Reset All Statistics" option, this preserves allmessageRequestandusageLedgerrows while starting cost calculations from the reset timestamp.Problem
Previously, when an admin needed to reset a user's cost limits (e.g., after billing adjustments or grace periods), the only option was full data deletion via "Reset All Statistics" — wiping all usage history, statistics, and logs. This was destructive and lost valuable audit data.
Solution
Introduce
costResetAttimestamp field on theuserstable that clips all cost calculation windows to start from the reset time instead of all-time. This enables a soft reset where:Related
Changes
Core Implementation
cost_reset_atnullable timestamp column onuserstable (migration0080_fresh_clint_barton)costResetAtpropagated through all user select queries, key validation, and statistics aggregation with per-user batch supportcostResetAt; service and lease layers clip time windows accordinglycostResetAtfrom Redis cache asDate; invalidates auth cache on reset to avoid stale timestampsActions & API
resetUserLimitsOnly— setscostResetAt+ clears cost cache (preserves historical data)resetUserStatistics(existing) — deletes all records + clearscostResetAt(destructive)getUserLimitUsage/getUserAllLimitUsage/getKeyLimitUsage/getMyQuota— all clip time ranges bycostResetAtUI & i18n
costResetAtTesting
Added 47+ unit tests covering:
tests/unit/actions/users-reset-limits-only.test.ts— reset flowtests/unit/actions/users-reset-all-statistics.test.ts— destructive reset behaviortests/unit/repository/statistics-reset-at.test.ts— statistics clippingtests/unit/lib/security/api-key-auth-cache-reset-at.test.ts— auth cache hydrationtests/unit/lib/redis/cost-cache-cleanup.test.ts— Redis cleanuptests/unit/proxy/rate-limit-guard.test.ts— rate limit handlingtests/unit/actions/key-quota-cost-reset.test.ts— key quota with costResetAtBreaking Changes
None — the
cost_reset_atcolumn is nullable and backward compatible. Existing users without the field behave as before (all-time cost calculations).Validation
Notes
0080_fresh_clint_bartonbun run lintstill reports existing repo-level Biome config/schema mismatch and unrelated pre-existing lint issues (not introduced by this PR)Description enhanced by Claude AI
Greptile Summary
This PR introduces
costResetAt— a soft per-user rate-limit reset that clips all cost-calculation windows to a timestamp without deleting any historical records. The feature spans the full stack: DB schema, repository layer, rate-limit/lease service, Redis cache invalidation, and admin UI.Key changes:
cost_reset_atnullable timestamp column onuserstable (backward-compatible migration)resetUserLimitsOnlyaction setscostResetAt = NOW()and clears Redis cost caches;resetUserAllStatisticsnow uses a Drizzle transaction wrapping three sequential writescostResetAtas a floor for the query windowcostResetAttimestamp suffix to prevent stale hits across resetsresetUserAllStatisticshas been addressed with a transaction;costResetAtcache degradation is now documented with an explanatory commentPerformance consideration:
sumUserTotalCostBatchandsumKeyTotalCostBatchByIdsfall back to N individual queries for each entity with a non-nullcostResetAt. This can degrade batch efficiency in deployments with many soft-reset users; embedding the per-user/key cutoff directly in the SQL query would restore O(1) round-trip behavior.Confidence Score: 4/5
costResetAtconsistently, and previously flagged issues have been addressed.resetUserAllStatisticsand the inconsistentcostResetAthandling in auth cache—have been fixed in this revision. The only remaining findings are performance optimizations: the batch query functions can degrade to N+1 for reset entities, and Redis exact-key deletes use unnecessary SCAN operations. Neither affects correctness or security.src/repository/statistics.ts— the N+1 query degradation insumUserTotalCostBatchandsumKeyTotalCostBatchByIdsis worth addressing before this pattern scales to production with many reset users.Comments Outside Diff (1)
src/repository/statistics.ts, line 554-607 (link)N+1 query degradation in batch total-cost functions
sumUserTotalCostBatchandsumKeyTotalCostBatchByIdsfan out into individualsumUserTotalCost/sumKeyTotalCostcalls for every entity with a non-nullcostResetAt. In deployments where many users have been soft-reset, the admin quotas page at/dashboard/quotas/userswill fire one DB round-trip per reset user instead of a single batch query.A more scalable approach is to embed the per-user/key cutoff directly in the SQL using a conditional expression:
This collapses all users back into a single query regardless of how many have been reset, restoring the original O(1) DB round-trip guarantee of the batch function.
Prompt To Fix With AI
Last reviewed commit: b0b89bc