fix: resolve database timeout hotspots - #1385
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
📝 WalkthroughWalkthroughPR 将用户统计重置改为 Redis/Bull 异步任务,并新增状态查询、进度记录和管理界面轮询。同时更新会话查询、Replay 清理、代理状态查询、数据库索引及迁移预检。 Changes异步统计重置
会话、代理状态与 Replay
数据库与缓存基础设施
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e5f8ab075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/[locale]/dashboard/sessions/_components/active-sessions-client.tsx (1)
50-61: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win补充缺失的
refreshingi18n key。
active-sessions-client.tsx在刷新状态中使用t("refreshing"),但五种语言文件的dashboard.sessions中都缺少该 key,需要补全以免界面回退或报错。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[locale]/dashboard/sessions/_components/active-sessions-client.tsx around lines 50 - 61, 补充五种语言文件中 dashboard.sessions 下缺失的 refreshing i18n key,并为其提供对应语言的“刷新中”文案,确保 active-sessions-client.tsx 使用 t("refreshing") 时不会回退或报错。
🧹 Nitpick comments (10)
src/app/[locale]/dashboard/sessions/_components/active-sessions-query.test.ts (1)
40-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win补充测试覆盖
getAllSessions以 reject 方式响应中止的场景。当前测试仅验证
getAllSessions以resolve({ ok: false, error: "aborted" })的方式响应中止事件。如果实际实现在信号中止时改为 reject(例如底层使用fetch并触发原生AbortError),当前测试无法捕获fetchAllSessionsPage中 catch 分支的错误归一化缺口(见active-sessions-query.ts第 44-51 行的评论)。建议新增一个测试用例,让
api.getAllSessions的 mock 实现在中止时 reject 一个DOMException("AbortError"),并断言最终错误仍归一化为FETCH_SESSIONS_CANCELLED。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[locale]/dashboard/sessions/_components/active-sessions-query.test.ts around lines 40 - 62, 在现有“propagates caller cancellation to the browser request”测试旁新增一个用例,使 api.getAllSessions 在 signal 的 abort 事件中以 DOMException("AbortError") reject;调用 fetchAllSessionsPage 并触发 AbortController.abort() 后,断言最终仍以 FETCH_SESSIONS_CANCELLED 拒绝,覆盖 fetchAllSessionsPage 的取消错误归一化分支。tests/unit/drizzle/session-identity-indexes.test.ts (1)
37-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win补充断言首列的
COALESCE表达式。两个用例都只校验第 2、3 列。本 PR 的核心是让查询对齐
COALESCE(session_identity, session_id)索引,而首列表达式恰恰没有被断言。如果首列被改成单列session_identity,当前用例仍会通过。♻️ 建议的补充断言
expect(index?.config.columns).toHaveLength(3); + const identity = compileSql(index?.config.columns[0] as SQL); + expect(identity).toContain("coalesce"); + expect(identity).toContain("session_identity"); + expect(identity).toContain("session_id"); const createdAt = compileSql(index?.config.columns[1] as SQL);在
usage_ledger用例中做同样的补充。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/drizzle/session-identity-indexes.test.ts` around lines 37 - 62, Extend both tests around the messageRequest and usageLedger index lookups to assert that the first index column is the COALESCE(session_identity, session_id) expression, using the existing SQL compilation approach. Keep the current column-count and ordering assertions unchanged so the tests fail if either index is changed to a plain session_identity column.tests/unit/instrumentation-replay-cleanup.test.ts (1)
50-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win用例名称声称与 Replay 开关无关,但没有对应断言。
本 PR 移除了
ENABLE_REQUEST_REPLAY门控。请在用例中显式把该环境变量设为关闭值,再断言清理仍然执行。这样才能锁定门控移除后的行为。♻️ 建议的补充
it("starts cleanup immediately and every ten minutes regardless of Replay enablement", async () => { + vi.stubEnv("ENABLE_REQUEST_REPLAY", "false"); const setIntervalSpy = vi.spyOn(globalThis, "setInterval"); await startReplayCleanupScheduler(); await vi.runOnlyPendingTimersAsync();同时在
afterEach中调用vi.unstubAllEnvs()。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/instrumentation-replay-cleanup.test.ts` around lines 50 - 57, Update the test case around startReplayCleanupScheduler to explicitly set ENABLE_REQUEST_REPLAY to a disabled value before starting the scheduler, while retaining the assertions that cleanup runs immediately and on the interval. Add vi.unstubAllEnvs() to the test cleanup in afterEach so the environment override cannot leak between tests.src/app/api/v1/resources/users/handlers.ts (1)
242-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
let reset;缺少类型标注。第 242 行声明的
reset没有类型标注,依赖控制流推断。请显式标注为UserStatisticsResetRecord | null,提升可读性并避免后续改动时类型退化为any。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/v1/resources/users/handlers.ts` around lines 242 - 244, 为包含 findUserStatisticsReset 调用的处理流程更新 reset 声明,显式标注为 UserStatisticsResetRecord | null,并保留现有 try/catch 及后续赋值逻辑不变。src/lib/user-statistics-reset/reset-queue.ts (1)
151-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value对账逻辑与递归边界的实现正确。
allowReconciliation标志把递归深度限制为一次,避免了无限重入。终结态 job 与缺失状态记录两条修复路径都会先释放活跃锁再重试。逻辑清晰。一点可选建议:第 162 行先写入新的状态记录,第 166 行在抢锁失败后又删除它。可以把
setUserStatisticsResetStatus(queued)移到claimActiveUserStatisticsReset成功之后,减少一次 Redis 写入和一次删除。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/user-statistics-reset/reset-queue.ts` around lines 151 - 199, 在 enqueueUserStatisticsResetWithReconciliation 中调整操作顺序,先调用 claimActiveUserStatisticsReset,再仅在 claim.acquired 为 true 时写入 queued 状态;移除抢锁失败路径中对新状态记录的删除,并保持现有对账、终结态处理及递归边界逻辑不变。src/lib/user-statistics-reset/reset-service.ts (2)
102-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
drainTable的错误进度归属正确,但缺少循环上限。
while (true)循环仅依赖batchDeleted < RESET_BATCH_SIZE退出。在正常路径下该条件必然成立,因为每轮都真实删除行。但如果affectedRows因驱动返回形态不符而恒定返回RESET_BATCH_SIZE以外的错误值(参见第 24-39 行的相关问题),循环行为将不可预期。建议增加一个最大迭代次数或时间预算,作为兜底保护,并在超限时抛出USER_STATISTICS_RESET_ROWS_LOCKED。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/user-statistics-reset/reset-service.ts` around lines 102 - 136, 在 drainTable 的删除循环中增加最大迭代次数或时间预算作为兜底,避免 batchDeleted 持续达到 RESET_BATCH_SIZE 时无限循环;超出限制时抛出 USER_STATISTICS_RESET_ROWS_LOCKED,并沿用当前已累计的 deleted 进度和表对应的错误详情。
176-212: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win缓存清理失败会触发整个重置任务重试,代价较高。
第 199-201 行把
clearUserCostCache返回null(Redis 不可用)或cleanupFailed视为任务失败并抛出。Bull 会按指数退避重试最多 5 次,每次都重新执行drainTable的全表扫描和users更新。数据库删除此时已经完成,重跑只是空扫描,但在大表上仍会产生可观的查询开销。建议把缓存清理拆为独立的补偿步骤,或者只在
cleanupFailed时记录告警并返回成功,避免重复执行昂贵的删除阶段。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/user-statistics-reset/reset-service.ts` around lines 176 - 212, 调整重置流程中 clearUserCostCache 的失败处理:不要因返回 null 或 cleanupFailed 抛出 UserStatisticsResetError,避免触发 Bull 重试并重复执行昂贵的 drainTable 和 users 更新;将缓存清理失败作为独立补偿步骤处理,或记录告警后继续返回成功,同时保留数据库重置结果和现有 UserStatisticsResetError 包装逻辑。src/lib/user-statistics-reset/reset-status-store.ts (2)
58-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win活跃锁的 TTL 为 7 天,异常场景下会长期阻塞同一用户的重置。
claimActiveUserStatisticsReset复用RESET_STATUS_TTL_SECONDS(7 天)作为活跃锁的过期时间。如果 worker 进程在处理中崩溃,releaseActiveUserStatisticsReset不会执行。此时锁需要 7 天才自动过期。
reset-queue.ts中的对账逻辑(第 178-186 行)通过检查 Bull job 状态可以覆盖 job 已终结的情况,但覆盖不了 job 仍处于active而 worker 已消失的窗口。建议为活跃锁单独定义一个更短的 TTL(例如与任务最大执行时长对齐),与状态记录的保留期解耦。♻️ 建议的改动
const RESET_STATUS_TTL_SECONDS = 7 * 24 * 60 * 60; +const ACTIVE_RESET_TTL_SECONDS = 60 * 60; const ACTIVE_RESET_PREFIX = "cch:user-statistics-reset:active:"; @@ - const result = await redis.set(key, resetId, "EX", RESET_STATUS_TTL_SECONDS, "NX"); + const result = await redis.set(key, resetId, "EX", ACTIVE_RESET_TTL_SECONDS, "NX");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/user-statistics-reset/reset-status-store.ts` around lines 58 - 74, Define a dedicated, shorter TTL for the active reset lock instead of reusing RESET_STATUS_TTL_SECONDS, and use it in the redis.set call within claimActiveUserStatisticsReset. Keep the longer status-record retention TTL unchanged and align the new lock TTL with the maximum expected job execution duration.
34-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value统一删除路径为
statusStore.delete()。写入、读取的键与 JSON 格式与
RedisKVStore一致,但删除使用了手工拼接的 Redis key。该格式短期内兼容,仍建议统一走封装接口,避免后续实现变更导致删除路径失效。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/user-statistics-reset/reset-status-store.ts` around lines 34 - 56, Update deleteUserStatisticsResetStatus to remove the record through statusStore.delete(resetId) instead of calling getReadyRedis().del with a manually constructed key. Preserve the existing function signature and async behavior, and keep setUserStatisticsResetStatus and getUserStatisticsResetStatus unchanged.src/actions/users.ts (1)
2368-2384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win提取共用逻辑,避免
requiresRedisForFixed5h判断重复。
resetUserAllStatistics(Line 2369-2373)与resetUserLimitsOnly(Line 2255-2259)包含完全相同的固定 5 小时限额判断逻辑。两处独立维护存在业务规则漂移的风险:修改一处判断条件时容易忘记同步另一处。建议提取为共享辅助函数,例如
requiresRedisForFixed5hReset(user, keys),供两个函数复用。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/users.ts` around lines 2368 - 2384, 提取 `resetUserAllStatistics` 与 `resetUserLimitsOnly` 中重复的固定 5 小时限额判断为共享辅助函数,例如 `requiresRedisForFixed5hReset(user, keys)`;让两处调用该函数,并保留现有用户及 key 的默认值和判断条件不变。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/users.ts`:
- Line 2350: 调整 enqueueStarted 之后的错误处理逻辑,仅将实际 Redis 连接失败映射为
CONNECTION_FAILED;不要把 enqueueUserStatisticsReset 抛出的
USER_STATISTICS_RESET_ACTIVE_STATUS_MISSING、USER_STATISTICS_RESET_ACTIVE_JOB_TERMINAL、USER_STATISTICS_RESET_QUEUE_FAILED
或 Worker 抛出的 UserStatisticsResetError 统一转换。保留这些错误各自的
errorCode,使前端和接口响应能够按原始错误码处理。
In `@src/app/`[locale]/dashboard/_components/user/edit-user-dialog.tsx:
- Around line 543-547: 补全五个语言包中 dashboard.json 的
editDialog.resetData.queued、running、completed 和 failed 翻译,覆盖
zh-CN、zh-TW、en、ja、ru;确保与 edit-user-dialog.tsx 中 statisticsReset.status 的动态 key
完全匹配,并保留现有翻译结构。
- Around line 281-309: Update the polling effect around poll and
applyStatisticsResetStatus to enforce a finite retry limit or total polling
timeout, using backoff if retries continue, and stop polling by clearing or
marking statisticsReset failed when the limit is reached. Also update
statisticsReset in the non-retryable error branch alongside
setIsResettingAll(false), so the status badge no longer displays stale
queued/running state.
In `@src/app/`[locale]/dashboard/sessions/_components/active-sessions-query.ts:
- Around line 44-51: Normalize caller-abort errors consistently in
fetchAllSessionsPage by replacing the input.signal.aborted catch-path rethrow
with FETCH_SESSIONS_CANCELLED. In
src/app/[locale]/dashboard/sessions/_components/active-sessions-query.ts lines
44-51, update that branch; in
src/app/[locale]/dashboard/sessions/_components/active-sessions-query.test.ts
lines 40-62, add coverage where api.getAllSessions rejects with
DOMException("AbortError") on abort and assert FETCH_SESSIONS_CANCELLED.
In `@src/app/api/v1/resources/users/handlers.ts`:
- Around line 234-262: 为 getUserStatisticsReset 补充与 resetUserStatistics
一致的权限校验:读取 c.get("auth"),通过现有的管理员授权流程验证调用方后再执行
findUserStatisticsReset;同时确认该路由已挂载等价的鉴权中间件,避免仅依赖认证而允许读取其他用户的重置记录。
- Around line 253-259: Update the 404 response in the reset handling flow to
replace the hardcoded English detail in createProblemResponse with
publicActionErrorDetail(404), matching the existing 503 branch and preserving
the user.statistics_reset_not_found error code.
In `@src/app/v1/_lib/proxy/replay/replay-store.ts`:
- Around line 355-382: 调整 persistCompleted 中对 durable replay 的冲突判断:将身份字段
verifier、scopeTag、keyId、userId、format、model 与
payload、byteSize、headers、statusCode、sourceMessageRequestId
等内容或物理请求字段分离。仅在身份字段不匹配时抛出 durable replay conflict;若 replayId 相同但响应内容或物理请求 ID
不同,应保留已有 durable 行并正常返回,同时更新 isMatchingPersistedReplay 及其调用逻辑以遵循该规则。
In `@src/lib/redis/cost-cache-cleanup.ts`:
- Around line 191-194: Update the pipeline exception return in the cost cache
cleanup flow so it includes errorCount set to scanErrorCount + 1 alongside
cleanupFailed. Add or update tests covering a pipeline.exec() exception and
assert the returned error count includes the scan errors plus the pipeline
failure.
In `@src/lib/user-statistics-reset/reset-queue.ts`:
- Around line 122-142: Update the catch branch around
setUserStatisticsResetStatus so a status-write failure is handled independently:
log the failure, continue to releaseActiveUserStatisticsReset for final
attempts, and always rethrow the original caught error rather than the
status-write exception.
- Around line 57-77: Update the failed-event listener in resetQueue to handle an
absent or incomplete job before accessing job.data, job.opts, or
job.attemptsMade. Preserve available error logging, and when the job is
unavailable avoid terminal-status processing that requires job.data so the async
listener cannot throw a TypeError or produce an unhandled rejection.
---
Outside diff comments:
In `@src/app/`[locale]/dashboard/sessions/_components/active-sessions-client.tsx:
- Around line 50-61: 补充五种语言文件中 dashboard.sessions 下缺失的 refreshing i18n
key,并为其提供对应语言的“刷新中”文案,确保 active-sessions-client.tsx 使用 t("refreshing") 时不会回退或报错。
---
Nitpick comments:
In `@src/actions/users.ts`:
- Around line 2368-2384: 提取 `resetUserAllStatistics` 与 `resetUserLimitsOnly`
中重复的固定 5 小时限额判断为共享辅助函数,例如 `requiresRedisForFixed5hReset(user,
keys)`;让两处调用该函数,并保留现有用户及 key 的默认值和判断条件不变。
In
`@src/app/`[locale]/dashboard/sessions/_components/active-sessions-query.test.ts:
- Around line 40-62: 在现有“propagates caller cancellation to the browser
request”测试旁新增一个用例,使 api.getAllSessions 在 signal 的 abort 事件中以
DOMException("AbortError") reject;调用 fetchAllSessionsPage 并触发
AbortController.abort() 后,断言最终仍以 FETCH_SESSIONS_CANCELLED 拒绝,覆盖
fetchAllSessionsPage 的取消错误归一化分支。
In `@src/app/api/v1/resources/users/handlers.ts`:
- Around line 242-244: 为包含 findUserStatisticsReset 调用的处理流程更新 reset 声明,显式标注为
UserStatisticsResetRecord | null,并保留现有 try/catch 及后续赋值逻辑不变。
In `@src/lib/user-statistics-reset/reset-queue.ts`:
- Around line 151-199: 在 enqueueUserStatisticsResetWithReconciliation
中调整操作顺序,先调用 claimActiveUserStatisticsReset,再仅在 claim.acquired 为 true 时写入 queued
状态;移除抢锁失败路径中对新状态记录的删除,并保持现有对账、终结态处理及递归边界逻辑不变。
In `@src/lib/user-statistics-reset/reset-service.ts`:
- Around line 102-136: 在 drainTable 的删除循环中增加最大迭代次数或时间预算作为兜底,避免 batchDeleted 持续达到
RESET_BATCH_SIZE 时无限循环;超出限制时抛出 USER_STATISTICS_RESET_ROWS_LOCKED,并沿用当前已累计的
deleted 进度和表对应的错误详情。
- Around line 176-212: 调整重置流程中 clearUserCostCache 的失败处理:不要因返回 null 或
cleanupFailed 抛出 UserStatisticsResetError,避免触发 Bull 重试并重复执行昂贵的 drainTable 和
users 更新;将缓存清理失败作为独立补偿步骤处理,或记录告警后继续返回成功,同时保留数据库重置结果和现有 UserStatisticsResetError
包装逻辑。
In `@src/lib/user-statistics-reset/reset-status-store.ts`:
- Around line 58-74: Define a dedicated, shorter TTL for the active reset lock
instead of reusing RESET_STATUS_TTL_SECONDS, and use it in the redis.set call
within claimActiveUserStatisticsReset. Keep the longer status-record retention
TTL unchanged and align the new lock TTL with the maximum expected job execution
duration.
- Around line 34-56: Update deleteUserStatisticsResetStatus to remove the record
through statusStore.delete(resetId) instead of calling getReadyRedis().del with
a manually constructed key. Preserve the existing function signature and async
behavior, and keep setUserStatisticsResetStatus and getUserStatisticsResetStatus
unchanged.
In `@tests/unit/drizzle/session-identity-indexes.test.ts`:
- Around line 37-62: Extend both tests around the messageRequest and usageLedger
index lookups to assert that the first index column is the
COALESCE(session_identity, session_id) expression, using the existing SQL
compilation approach. Keep the current column-count and ordering assertions
unchanged so the tests fail if either index is changed to a plain
session_identity column.
In `@tests/unit/instrumentation-replay-cleanup.test.ts`:
- Around line 50-57: Update the test case around startReplayCleanupScheduler to
explicitly set ENABLE_REQUEST_REPLAY to a disabled value before starting the
scheduler, while retaining the assertions that cleanup runs immediately and on
the interval. Add vi.unstubAllEnvs() to the test cleanup in afterEach so the
environment override cannot leak between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a6f538c-7b91-45e9-bbdf-30398c8147ba
📒 Files selected for processing (54)
drizzle/0118_bright_sunspot.sqldrizzle/meta/0118_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/edit-user-dialog.tsxsrc/app/[locale]/dashboard/sessions/_components/active-sessions-client.test.tsxsrc/app/[locale]/dashboard/sessions/_components/active-sessions-client.tsxsrc/app/[locale]/dashboard/sessions/_components/active-sessions-query.test.tssrc/app/[locale]/dashboard/sessions/_components/active-sessions-query.tssrc/app/api/v1/resources/users/handlers.tssrc/app/api/v1/resources/users/router.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/drizzle/schema.tssrc/instrumentation.tssrc/lib/api-client/v1/actions/active-sessions.tssrc/lib/api-client/v1/actions/users.tssrc/lib/api-client/v1/errors.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/users.tssrc/lib/migrate.tssrc/lib/migrations/session-replay-index-preflight.tssrc/lib/proxy-status-tracker.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/replay-cleanup.tssrc/lib/user-statistics-reset/reset-queue.tssrc/lib/user-statistics-reset/reset-service.tssrc/lib/user-statistics-reset/reset-status-store.tssrc/lib/user-statistics-reset/types.tssrc/repository/message.tstests/api/v1/users/users.test.tstests/unit/actions/users-reset-5h-only.test.tstests/unit/actions/users-reset-all-statistics.test.tstests/unit/api/v1/api-client-actions.test.tstests/unit/drizzle/database-timeout-migration.test.tstests/unit/drizzle/proxy-status-indexes.test.tstests/unit/drizzle/session-identity-indexes.test.tstests/unit/frontend/api-error-i18n.test.tstests/unit/instrumentation-replay-cleanup.test.tstests/unit/lib/proxy-status-tracker.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/lib/replay-cleanup.test.tstests/unit/lib/session-replay-index-preflight.test.tstests/unit/lib/user-statistics-reset-queue.test.tstests/unit/lib/user-statistics-reset-service.test.tstests/unit/lib/user-statistics-reset-status-store.test.tstests/unit/proxy/replay-store.test.tstests/unit/repository/message-aggregate-multiple-session-stats.test.tstests/unit/repository/message-session-request-query.test.tstests/unit/usage-ledger/cleanup-immunity.test.ts
There was a problem hiding this comment.
Code Review Summary
I found two high-confidence correctness regressions in the new Proxy Status query predicates. Both come from narrowing the finalized-request signal from the repo’s existing statusCode || durationMs rule to statusCode-only logic inside src/lib/proxy-status-tracker.ts, which can misclassify completed requests and show stale per-user status.
PR Size: XL
- Lines changed: 9013
- Files changed: 54
- Split suggestion: Separate the migration/index rollout from the runtime behavior changes so database-risk review is independent of application semantics.
- Split suggestion: Separate replay cleanup and statistics-reset background work from dashboard/proxy-status UI changes so operational and UX regressions are easier to isolate.
- Split suggestion: Separate session-identity query refactors from proxy-status query changes so correctness validation stays focused per subsystem.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 2 | 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)
None.
High Priority Issues (Should Fix)
src/lib/proxy-status-tracker.ts:188now treatsstatus_code IS NULLas the sole active-request marker, which conflicts with the existing finalized-request rule used elsewhere in the dashboard (statusCode != null || durationMs != null) and can keep completed requests in the active list.src/lib/proxy-status-tracker.ts:215now requiresmr.status_code IS NOT NULLfor the latest finalized request lookup, which can skip duration-only finalized rows and fall back to an older request for a user.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#1385, applied thesize/XLlabel, and submitted the required summary review. - Left 2 high-priority inline comments in
src/lib/proxy-status-tracker.ts:188andsrc/lib/proxy-status-tracker.ts:215. - Both findings are the same semantic regression: Proxy Status now uses
statusCodeas the only finalization signal, while the rest of the dashboard still treats a request as finalized when eitherstatusCodeordurationMsis present. - Impact: completed requests can be misclassified as active, and the per-user “last request” view can fall back to stale older data.
- The summary also includes the required XL split suggestions for migrations/indexes, replay/reset background work, and dashboard/session-query changes.
When persistCompleted detected an existing matching durable row or hit a conflict during concurrent writes, the spool previously wrote an aborted terminal state to the hot layer, masking the already-persisted PG winner. persistCompleted now returns a discriminated result so the spool can detect existing winners, and conflicts raise a dedicated ReplayDurableConflictError. In both cases the spool calls the new discardOwned method to silently drop the losing hot-layer candidate without writing an aborted status. The sourceMessageRequestId check was also relaxed from the persisted-row equality test so replays of the same content from different source requests are accepted.
…ueues Statistics resets previously wiped all Redis cost-cache keys including the fixed 5h rolling windows, causing post-cutoff quota state to be lost. A new prepareUserStatisticsResetFixed5h function atomically deletes only the fixed 5h keys and records a monotonic cutoff timestamp, and clearUserCostCache now accepts preserveFixed5hCostKeys to skip those keys during the broader cleanup. The reset queue now runs this preparation phase before enqueuing and tracks a preparation version on the stored record so legacy queued jobs are reconciled idempotently on retry. Enqueue failures after preparation leave the record queued rather than discarding it, and the next enqueue attempt recovers the prepared claim. Per-batch deletion progress is persisted to the status store so retries cumulate instead of restarting, and active-claim release errors no longer mask the original business error code.
There was a problem hiding this comment.
Actionable comments posted: 1
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/replay/replay-store.ts (1)
431-448: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win按当前生产 adapter 处理
cleanupExpired返回计数。生产代码仅使用
drizzle-orm/postgres-js,需要处理执行结果的实际形状,避免已删除过期行后返回0。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/v1/_lib/proxy/replay/replay-store.ts` around lines 431 - 448, 更新 cleanupExpired 的删除结果计数逻辑,按生产环境 drizzle-orm/postgres-js 的实际 execute 返回形状读取受影响行数,避免删除成功时因非数组结果被错误返回 0。保留无删除行时返回 0,并继续返回本批实际删除数量。
🧹 Nitpick comments (2)
src/app/v1/_lib/proxy/replay/replay-spool.ts (1)
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win将相对导入改为
@/别名。这两个变更使用相对 TypeScript 导入。仓库规则要求将
src/内导入映射为@/。
src/app/v1/_lib/proxy/replay/replay-spool.ts#L6-L11: 将"./replay-store"改为"@/app/v1/_lib/proxy/replay/replay-store"。src/lib/user-statistics-reset/reset-status-store.ts#L6-L6: 将"./types"改为"@/lib/user-statistics-reset/types"。As per coding guidelines, "Use path alias
@/to map to ./src/ for imports".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/v1/_lib/proxy/replay/replay-spool.ts` around lines 6 - 11, Replace the relative replay-store import in src/app/v1/_lib/proxy/replay/replay-spool.ts lines 6-11 with the specified `@/app/v1/_lib/proxy/replay/replay-store` alias, and replace the relative types import in src/lib/user-statistics-reset/reset-status-store.ts line 6 with the specified `@/lib/user-statistics-reset/types` alias.Source: Coding guidelines
tests/unit/lib/user-statistics-reset-queue.test.ts (1)
205-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议补充
prepareFixed5h返回null的测试。
reset-queue.ts的 Line 108 和 Line 358 在准备失败时抛出USER_STATISTICS_RESET_FIXED_5H_PREPARE_FAILED。当前测试文件没有覆盖该分支。该分支决定 Redis 不可用时的入队行为,以及活跃锁是否被保留。建议增加一个用例:
boundary.prepareFixed5h.mockResolvedValue(null),断言入队抛出该错误码,并断言boundary.add未被调用。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/lib/user-statistics-reset-queue.test.ts` around lines 205 - 252, 在 enqueueUserStatisticsReset 的测试中新增 prepareFixed5h 返回 null 的用例,覆盖 USER_STATISTICS_RESET_FIXED_5H_PREPARE_FAILED 错误分支;断言入队操作抛出该错误码、boundary.add 未被调用,并验证失败时活跃锁按现有行为保留。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/redis/cost-cache-cleanup.ts`:
- Around line 77-85: Validate the return value from redis.eval in the cutoff
calculation before converting it with Number, rejecting null, undefined, and
other non-string/non-number values. Only construct and return the ISO timestamp
when the validated value converts to a finite date; otherwise return null,
preventing empty script results from becoming the Unix epoch.
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/replay/replay-store.ts`:
- Around line 431-448: 更新 cleanupExpired 的删除结果计数逻辑,按生产环境 drizzle-orm/postgres-js
的实际 execute 返回形状读取受影响行数,避免删除成功时因非数组结果被错误返回 0。保留无删除行时返回 0,并继续返回本批实际删除数量。
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Around line 6-11: Replace the relative replay-store import in
src/app/v1/_lib/proxy/replay/replay-spool.ts lines 6-11 with the specified
`@/app/v1/_lib/proxy/replay/replay-store` alias, and replace the relative types
import in src/lib/user-statistics-reset/reset-status-store.ts line 6 with the
specified `@/lib/user-statistics-reset/types` alias.
In `@tests/unit/lib/user-statistics-reset-queue.test.ts`:
- Around line 205-252: 在 enqueueUserStatisticsReset 的测试中新增 prepareFixed5h 返回
null 的用例,覆盖 USER_STATISTICS_RESET_FIXED_5H_PREPARE_FAILED
错误分支;断言入队操作抛出该错误码、boundary.add 未被调用,并验证失败时活跃锁按现有行为保留。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5df21700-6801-426e-b867-0cb7abaa30f6
📒 Files selected for processing (16)
src/actions/users.tssrc/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/user-statistics-reset/reset-queue.tssrc/lib/user-statistics-reset/reset-service.tssrc/lib/user-statistics-reset/reset-status-store.tssrc/lib/user-statistics-reset/types.tstests/unit/actions/users-reset-5h-only.test.tstests/unit/actions/users-reset-all-statistics.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/lib/user-statistics-reset-queue.test.tstests/unit/lib/user-statistics-reset-service.test.tstests/unit/lib/user-statistics-reset-status-store.test.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/replay-store.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/actions/users-reset-5h-only.test.ts
- tests/unit/lib/user-statistics-reset-status-store.test.ts
- src/actions/users.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72be4680a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…code Re-throwing the raw DOMException exposed browser-specific error text to callers. Replace it with a stable FETCH_SESSIONS_CANCELLED error so consumers can match on a known code regardless of the browser runtime.
The statistics-reset poll loop retried transient network errors indefinitely at a fixed interval. Switch to exponential backoff with a cap of five consecutive retryable failures, after which the reset is marked failed instead of leaving the UI in a perpetual loading state.
Skip the failed-job handler when job data is missing recoverable fields so undefined resetId or userId values do not corrupt status records. Wrap status persistence in a try-catch so a transient store failure still releases the active-reset claim and propagates the original business error. Use optional chaining on job.opts for null safety.
getReadyRedis threw immediately when the shared client was in a connecting state, causing spurious failures during startup or reconnect. Make it async and await the ready or end event with a configurable timeout so callers tolerate transient connection states.
When the Redis pipeline threw after some scan calls had already failed, the returned errorCount omitted those earlier scan failures. Include the accumulated scan error count so callers see the true failure total.
The 404 response for a not-found statistics reset leaked an implementation-specific detail string. Use the shared publicActionErrorDetail helper so the message matches other 404 responses across the API surface.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/user-dialogs.test.tsx`:
- Around line 428-460: Update the call-count assertion in the “marks a
statistics reset failed after five consecutive polling retries” test to expect 5
calls instead of 6, matching the polling schedule and failure behavior of
EditUserDialogInner within 32 seconds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80e19aa3-5b25-4fb7-bf20-b7f1104fe1d8
📒 Files selected for processing (12)
src/app/[locale]/dashboard/_components/user/edit-user-dialog.tsxsrc/app/[locale]/dashboard/sessions/_components/active-sessions-query.test.tssrc/app/[locale]/dashboard/sessions/_components/active-sessions-query.tssrc/app/api/v1/resources/users/handlers.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/user-statistics-reset/reset-queue.tssrc/lib/user-statistics-reset/reset-status-store.tstests/api/v1/users/users.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/lib/user-statistics-reset-queue.test.tstests/unit/lib/user-statistics-reset-status-store.test.tstests/unit/user-dialogs.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/app/api/v1/resources/users/handlers.ts
- tests/unit/lib/redis/cost-cache-cleanup.test.ts
- src/app/[locale]/dashboard/sessions/_components/active-sessions-query.ts
- src/app/[locale]/dashboard/_components/user/edit-user-dialog.tsx
- tests/api/v1/users/users.test.ts
- tests/unit/lib/user-statistics-reset-queue.test.ts
- src/lib/redis/cost-cache-cleanup.ts
- src/lib/user-statistics-reset/reset-status-store.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c91d45cfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the old behaviour of marking a reset as failed when polling retries were exhausted: the authoritative server status is now preserved and a recoverable error banner with a retry button is shown instead. Each poll request is given a 15 s AbortSignal timeout so a hung request no longer blocks subsequent retries. Guard prepareUserStatisticsResetFixed5h against invalid or missing Redis cutoff values by returning null instead of constructing an epoch date. Throw early when getResetQueue is called in development to prevent accidental Bull/Redis processor creation outside production. Add i18n strings for statusUnavailable and retryStatus across all supported locales.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7019a687c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
resetUserAllStatistics previously rejected requests with a cold/unready Redis client when fixed 5h cost windows were active, surfacing a CONNECTION_FAILED error before the job was even enqueued. The reset queue itself already handles Redis readiness, so this pre-enqueue guard was redundant and blocked otherwise valid reset requests during cold-start windows. The guard and its associated fixed-5h detection logic are removed; the corresponding test now verifies that enqueue proceeds and the queue receives the request without the action layer checking Redis status.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 406c83b10f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace assertions that expected resetUserAllStatistics to fail with CONNECTION_FAILED when Redis is unavailable. The reset now succeeds and delegates fixed 5h readiness to the background queue via enqueueUserStatisticsReset, so Redis is never contacted directly during the action call.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70d5635d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cal ledger rows Reserved canonical identities (prefix-affinity or owner-scoped session IDs) could match unrelated physical ledger rows via the session_id fallback in the OR branch of the lookup condition. For reserved identities the lookup now requires both the COALESCE expression and the explicit session_identity column to match, preserving the COALESCE expression index while avoiding aliasing. The canonical lookup used by aggregateSessionStats and aggregateMultipleSessionStats now also applies the stricter reserved-identity condition so owner-scoped queries do not silently pick up physical sessions that happen to share the same ID value.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Summary
COALESCE(session_identity, session_id)indexesSKIP LOCKEDbatches, a 5-batch/30-second tick budget, PostgreSQL advisory leader locking, and durable Replay conflict validation202REST contract0118and extend the concurrent index preflight so large indexes are built and validated before the migration records its no-opIF NOT EXISTSstatementsRoot Cause
The active Session query used
session_identity = sid OR session_id = sid, while the production index is defined onCOALESCE(session_identity, session_id). PostgreSQL selected a sequential scan for every requested Session, causing a 39-Session request to repeat scans over the 23 GBmessage_requesttable until the 90-secondstatement_timeoutcancelled it.The Replay cleanup job independently attempted to delete almost the entire 25 GB
replay_payloadsbacklog in one transaction from every Pod. That produced statement and lock timeouts, large WAL bursts, and frequent checkpoints.Behavior And API Changes
POST /api/v1/users/{id}/statistics:resetnow returns202with aLocationheader and a durable reset record instead of204.GET /api/v1/users/{id}/statistics-resets/{resetId}reports queued/running/completed/failed state, timestamps, deleted row counts, and a stable error code.Database Rollout
0118_bright_sunspot.sqlcontains only idempotentCREATE INDEX IF NOT EXISTSstatements.0118: temporary indexes are built withCREATE INDEX CONCURRENTLY, checked throughindisvalidand version markers, then switched to the canonical names.Validation
bun run build: passed, 187 static pagesbun run lint: passed, 2050 filesbun run lint:fix: passed, no changesbun run typecheck: passedbun run test: passed, 858 files / 8335 tests; 13 skippedbun run test:coverage: passed; statements 80.00%, branches 67.38%, functions 72.43%, lines 82.12%bun run validate:migrations: passed, 120 migrationsbun run test:v1: passed, 91 files / 387 tests; critical coverage check passedbun run openapi:check: passedbun run openapi:lint: passedgit diff --check: passedProduction Observation
statement_timeout0118marker before considering rollout completeResidual Risk
This environment has no production
DSN, so the PR does not include a live PostgreSQLEXPLAINor concurrency integration run. SQL shape, migration orchestration, Bull state transitions, REST contracts, and UI behavior are covered by unit/API tests; production rollout should still verify real query plans and index validity.Greptile Summary
The PR reduces database timeout pressure by aligning session queries with expression indexes and moving expensive cleanup/reset operations into bounded background workflows.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Application startup] --> B[Acquire migration advisory lock] B --> C[Build and validate concurrent indexes] C --> D[Apply migration 0118] D --> E[Start background workers] E --> F[Replay cleanup tick] F --> G[Acquire replay advisory lock] G --> H[Delete up to five 100-row batches] E --> I[Statistics reset submission] I --> J[Create durable reset status] J --> K[Enqueue deduplicated Bull job] K --> L[Delete rows in cutoff-based batches] L --> M[Clear cost caches and publish terminal status]Reviews (8): Last reviewed commit: "fix(message): prevent reserved session i..." | Re-trigger Greptile
Context used (5)