fix(notification): 修复总开关关闭后排行榜/成本预警仍推送 & 成本预警间隔≥60分钟塌缩为每小时 (#1236)#1240
Merged
Conversation
…itch off and fix cost alert interval collapse
When the master switch or sub-switch is turned off, previously enqueued
repeatable jobs continued to fire because the processor did not re-check
settings at execution time. Additionally, the scheduleNotifications
function was guarded by NODE_ENV="production", so changes in development
were silently ignored. The cost alert interval >=60 minutes collapsed to
an hourly cron because Bull's cron only supports minutes 0-59.
- Remove NODE_ENV guard so scheduleNotifications always runs on settings save.
- Add enabled/sub-switch checks in daily-leaderboard and cost-alert job
processors to skip sending when disabled.
- Extract removeAllRepeatableJobs and make individual key removal resilient
to transient Redis errors.
- For cost alert intervals >=60 minutes, use {every} instead of */N cron to
schedule every N minutes correctly.
- Add unit tests covering processor skip behavior, removal resilience, and
cost alert interval mapping.
Fixes #1236
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough无条件触发 scheduleNotifications；队列处理器在运行期复核总开关与子开关并在关闭时跳过；新增集中 repeatable 清理与 interval->repeat 工具；cost-alert 与 cache-hit-rate-alert 在 legacy/targets 模式下统一调度生成规则；补充单元测试覆盖。 变更详情通知调度优化
估计代码审查工作量🎯 4 (Complex) | ⏱️ ~45 minutes 可能相关的 PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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 refactors the notification scheduling and queue processing logic. It removes the production-only restriction for scheduling notifications, introduces execution-time checks to skip daily leaderboard and cost alert tasks if their switches are disabled, and adds a robust helper to remove repeatable jobs while gracefully handling individual failures. It also addresses Bull's cron limitation for intervals of 60 minutes or more by falling back to fixed millisecond intervals. The reviewer feedback suggests further refining this interval logic by only using cron expressions when the interval divides 60 evenly, preventing uneven execution times at hour boundaries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a25f4f5745
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR correctly addresses three related bugs in the notification scheduling system: missing runtime switch validation, environment-gated rescheduling, and Bull cron interval collapse for >=60 minute intervals. The changes are well-scoped, the test coverage is thorough, and the code follows existing project patterns.
PR Size: S
- Lines changed: 389 (357 additions, 32 deletions)
- Files changed: 3
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (9 new tests covering skip behavior, switch validation, and interval scheduling)
- Code clarity - Good
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/notification/notification-queue.test.ts (2)
204-233: ⚡ Quick win建议补充 cost-alert 处理器的正向用例（主开关与子开关均开启）。
daily-leaderboard同时覆盖了跳过与发送两条路径（lines 158-201），但cost-alert仅覆盖了跳过场景。本次 PR 的核心修复正是处理器执行期的开关校验，缺少“两者均开启时调用generateCostAlerts与sendWebhookMessage各 1 次并返回成功”的对照用例，会让回归保护出现盲区。💚 建议新增的正向用例
it("sends when both master and sub-switch are enabled", async () => { mockGetNotificationSettings.mockResolvedValue( makeSettings({ enabled: true, costAlertEnabled: true }) ); const handler = await loadProcessor(); const result = await handler( makeJob({ type: "cost-alert", webhookUrl: "https://example.com/hook" }) ); expect(result).toEqual({ success: true }); expect(mockGenerateCostAlerts).toHaveBeenCalledTimes(1); expect(mockSendWebhookMessage).toHaveBeenCalledTimes(1); });🤖 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/notification/notification-queue.test.ts` around lines 204 - 233, Add a positive test for the cost-alert processor that sets makeSettings({ enabled: true, costAlertEnabled: true }), calls loadProcessor() and then handler(makeJob({ type: "cost-alert", webhookUrl: "https://example.com/hook" })), and assert the result equals { success: true } and that mockGenerateCostAlerts and mockSendWebhookMessage were each called exactly once; place this alongside the existing cost-alert tests to cover the "both master and sub-switch enabled" path.
259-297: ⚡ Quick win建议补充 Targets 模式（
useLegacyMode: false）下的 cost-alert 调度用例。当前 interval→cron/every 映射仅在 legacy 模式下验证。但本次改动同样调整了 targets 模式的调度策略（优先 binding 的
scheduleCron，否则按 interval 选择 cron/every，并为每个 binding 固定jobId）。该路径目前完全未被覆盖，建议至少补充一条用例：通过mockGetEnabledBindingsByType返回一个启用的 binding，断言queueAdd的repeat与jobId符合预期。🤖 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/notification/notification-queue.test.ts` around lines 259 - 297, Add a unit test for targets mode (useLegacyMode: false) that mocks getNotificationSettings with useLegacyMode: false and costAlertEnabled true, and mocks mockGetEnabledBindingsByType to return one enabled binding (include an id/identifier and no scheduleCron to force interval-based scheduling); then import and call scheduleNotifications() and assert that queueAdd was called for type "cost-alert" with the expected repeat (either { every: 60*60*1000 } for interval >=60 or { cron: "*/30 * * * *" } for <60 depending on the interval you choose) and that the queued job includes the binding-specific jobId (the fixed job id logic in scheduleNotifications / binding handling). Ensure the test references scheduleNotifications, mockGetEnabledBindingsByType, and queueAdd so it fails if those behaviors change.
🤖 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.
Nitpick comments:
In `@tests/unit/notification/notification-queue.test.ts`:
- Around line 204-233: Add a positive test for the cost-alert processor that
sets makeSettings({ enabled: true, costAlertEnabled: true }), calls
loadProcessor() and then handler(makeJob({ type: "cost-alert", webhookUrl:
"https://example.com/hook" })), and assert the result equals { success: true }
and that mockGenerateCostAlerts and mockSendWebhookMessage were each called
exactly once; place this alongside the existing cost-alert tests to cover the
"both master and sub-switch enabled" path.
- Around line 259-297: Add a unit test for targets mode (useLegacyMode: false)
that mocks getNotificationSettings with useLegacyMode: false and
costAlertEnabled true, and mocks mockGetEnabledBindingsByType to return one
enabled binding (include an id/identifier and no scheduleCron to force
interval-based scheduling); then import and call scheduleNotifications() and
assert that queueAdd was called for type "cost-alert" with the expected repeat
(either { every: 60*60*1000 } for interval >=60 or { cron: "*/30 * * * *" } for
<60 depending on the interval you choose) and that the queued job includes the
binding-specific jobId (the fixed job id logic in scheduleNotifications /
binding handling). Ensure the test references scheduleNotifications,
mockGetEnabledBindingsByType, and queueAdd so it fails if those behaviors
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d8b37ba-bf96-4c55-a6d3-c83f898236a1
📒 Files selected for processing (3)
src/actions/notifications.tssrc/lib/notification/notification-queue.tstests/unit/notification/notification-queue.test.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
The scheduler previously added new repeatable jobs even when old ones could not be removed, causing duplicate notifications. Circuit-breaker jobs also lacked a run-time switch check, so alerts could fire after the feature was disabled. - Extract clampIntervalMinutes and intervalToRepeat helpers; use cron only when the interval divides 60 evenly, falling back to the every option otherwise. This avoids uneven cron patterns and simplifies scheduling across legacy and targets modes. - Return a success boolean from removeAllRepeatableJobs and abort the reschedule if any removal fails, preventing old and new jobs from running simultaneously. - Add an execution-time guard in the circuit-breaker queue processor that re-verifies the master and circuit-breaker switches before sending, skipping with success: true, skipped: true when off. - Add unit tests for the circuit-breaker switch logic, the cost-alert positive path, targets-mode scheduling with binding jobId and timezone, and the abort-on-removal-failure scenario. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
已处理全部 review 反馈(commit 545b98f)
本地全绿: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/notification/notification-queue.test.ts (1)
251-285: ⚡ Quick win补一条
circuitBreakerEnabled: false的跳过用例。Line 251-285 目前只覆盖了总开关关闭和总/子开关都开启的路径，没有显式锁住“总开关开启但
circuitBreakerEnabled关闭”时应返回skipped的分支。这里正好是这次运行期开关复核的关键条件之一，少这条用例会让子开关失效的回归漏过。可直接补上的用例
describe("notification queue processor - circuit-breaker", () => { const data = { providerName: "OpenAI", providerId: 1, failureCount: 5, retryAt: "2026-06-02T12:30:00Z", }; it("skips sending when the master switch is off", async () => { mockGetNotificationSettings.mockResolvedValue( makeSettings({ enabled: false, circuitBreakerEnabled: true }) ); const handler = await loadProcessor(); const result = await handler( makeJob({ type: "circuit-breaker", webhookUrl: "https://example.com/hook", data }) ); expect(result).toEqual({ success: true, skipped: true }); expect(mockSendWebhookMessage).not.toHaveBeenCalled(); }); + + it("skips sending when the circuit-breaker sub-switch is off", async () => { + mockGetNotificationSettings.mockResolvedValue( + makeSettings({ enabled: true, circuitBreakerEnabled: false }) + ); + + const handler = await loadProcessor(); + const result = await handler( + makeJob({ type: "circuit-breaker", webhookUrl: "https://example.com/hook", data }) + ); + + expect(result).toEqual({ success: true, skipped: true }); + expect(mockSendWebhookMessage).not.toHaveBeenCalled(); + }); it("sends when both master and sub-switch are enabled", async () => { mockGetNotificationSettings.mockResolvedValue( makeSettings({ enabled: true, circuitBreakerEnabled: true }) );🤖 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/notification/notification-queue.test.ts` around lines 251 - 285, Add a new unit test in the "notification queue processor - circuit-breaker" suite that mocks mockGetNotificationSettings to return makeSettings({ enabled: true, circuitBreakerEnabled: false }), then load the processor via loadProcessor(), call the handler with makeJob({ type: "circuit-breaker", webhookUrl: "...", data }), and assert the result equals { success: true, skipped: true } and that mockSendWebhookMessage was not called; this covers the branch where the master switch is on but circuitBreakerEnabled is false.
🤖 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.
Nitpick comments:
In `@tests/unit/notification/notification-queue.test.ts`:
- Around line 251-285: Add a new unit test in the "notification queue processor
- circuit-breaker" suite that mocks mockGetNotificationSettings to return
makeSettings({ enabled: true, circuitBreakerEnabled: false }), then load the
processor via loadProcessor(), call the handler with makeJob({ type:
"circuit-breaker", webhookUrl: "...", data }), and assert the result equals {
success: true, skipped: true } and that mockSendWebhookMessage was not called;
this covers the branch where the master switch is on but circuitBreakerEnabled
is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4c79513-9ed1-4035-9286-fa38643c60ab
📒 Files selected for processing (2)
src/lib/notification/notification-queue.tstests/unit/notification/notification-queue.test.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Add a test case verifying that the notification queue processor skips sending when circuitBreakerEnabled is false. Covers a scenario flagged as missing during code review. Refs #1236 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
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
1 participant
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.
背景
修复 #1236：用户反馈「每日用户消费排行榜」推送异常 ——
根因分析
经多代理交叉排查（4 个并行调查 + 3 个对抗式验证 agent），定位到以下问题：
1. 执行期未复核开关（症状 3 的直接原因）
notification-queue.ts队列处理器中，daily-leaderboard与cost-alert两个 case 不会在任务执行时复核settings.enabled/ 子开关，只判断是否有数据。这与cache-hit-rate-alert(:267) 及独立函数notifier.ts:139的行为不一致。因此当总开关关闭后，关闭前已入队的 repeatable 作业仍会继续触发并发送。2. 仅生产环境重新调度（症状 2/3 的放大器）
updateNotificationSettingsAction仅在NODE_ENV === "production"时调用scheduleNotifications()，而默认值为"development"。未显式设置该变量的自部署环境在改设置（含关闭总开关）时不会刷新/移除 Bull repeatable 作业，导致旧调度残留、关开关无效。scheduleNotifications内部已 fail-open，启动入口与notification-bindings早已无条件调用它，移除该 gate 安全且一致。3. 成本预警间隔 ≥60 分钟塌缩为每小时（真正的「每隔 1 小时」来源）
cost-alert使用*/${interval} * * * *，而 Bull 的 cron 分钟字段仅 0-59，*/60会塌缩成「每小时第 0 分」，*/120同样退化为每小时。默认costAlertCheckInterval=60即命中此 bug。cache-hit-rate-alert早已用<=59 ? cron : every规避，此次让成本预警对齐该逻辑（legacy + targets 两条路径）。改动
daily-leaderboard、cost-alert处理器在生成/发送前校验enabled与对应子开关，关闭即skipped。{ every }，legacy 与 targets 模式一致；间隔做[1, 1440]归一化。removeAllRepeatableJobs，单个 key 移除失败（Redis 瞬时错误）不再中断整轮重调度。测试
新增
tests/unit/notification/notification-queue.test.ts（9 个用例）：skipped，且不调用生成与发送；scheduleNotifications移除全部 repeatable 作业；{ every: 3600000 }，interval=30 →*/30 * * * *。本地验证（全绿）
build✅ /lint✅ /typecheck✅ /test✅（6180 passed, 13 skipped，含本次新增 9 项）范围说明
scheduleCron加校验：0 * * * *是合法 cron，属用户意图，非 bug。circuit-breaker为事件驱动而非定时任务，无需此修复。🤖 Generated with Claude Code
Greptile Summary
This PR fixes three root causes behind #1236: (1) the queue processor for
daily-leaderboardandcost-alertnow re-checkssettings.enabled/ sub-switches at execution time so stale repeatable jobs can no longer fire after the master switch is turned off; (2) theNODE_ENV === \"production\"gate that silently prevented non-production deployments from rescheduling Bull jobs on settings changes is removed; (3)*/60 * * * *cron collapse is fixed by routing intervals ≥60 min through{ every: ms }.circuit-breaker,daily-leaderboard, andcost-alertprocessor branches all early-return{ success: true, skipped: true }when the relevant switches are off.intervalToRepeat(n, tz?)helper centralises thecron-vs-everydecision;clampIntervalMinutesnormalises raw DB values to[1, 1440].removeAllRepeatableJobstolerates single-key Redis failures; if any removal fails during rescheduling,scheduleNotificationsaborts rather than creating duplicate jobs.Confidence Score: 5/5
Safe to merge — all three documented bug scenarios are closed, the helpers are correct for all edge cases, and the added test suite exercises both the skip paths and the interval-routing decisions end-to-end.
The changes are narrowly scoped to the notification queue: adding execution-time guards, removing the environment gate that made settings changes a no-op in most self-hosted deployments, and fixing the cron collapse. No auth, data-model, or external API surfaces are touched. The intervalToRepeat helper handles boundary values correctly, and removeAllRepeatableJobs gracefully tolerates partial Redis failures without risking duplicate job scheduling.
No files require special attention.
Important Files Changed
Reviews (3): Last reviewed commit: "test(notification): add circuit-breaker ..." | Re-trigger Greptile