chore: Add visibility and error handling for BH cron jobs#38438
chore: Add visibility and error handling for BH cron jobs#38438
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughAdded try/catch error handling and logging across livechat business-hour flows and Agenda DB operations; introduced enhanced observability and logger integration in the cron scheduler. Public APIs and method signatures remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Scheduler
participant Agenda as Agenda Service
participant DB as MongoDB
participant Logger as Logger (module)
rect rgba(200,230,201,0.5)
Cron->>Agenda: schedule/add job
Agenda->>DB: insert job document
DB-->>Agenda: ack/insert result
Agenda-->>Logger: debug "job scheduled"
end
rect rgba(187,222,251,0.5)
Cron->>Agenda: run job
Agenda->>DB: fetch & lock job
DB-->>Agenda: job document
Agenda->>Logger: log "start"
Agenda->>DB: update job status/result
DB-->>Agenda: update ack
Agenda->>Logger: log "success" or "fail"
Agenda->>Logger: emit "error:database" on DB error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38438 +/- ##
===========================================
- Coverage 70.85% 70.42% -0.43%
===========================================
Files 3161 3161
Lines 109785 110159 +374
Branches 19688 19896 +208
===========================================
- Hits 77783 77576 -207
- Misses 29973 30549 +576
- Partials 2029 2034 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/agenda/src/Agenda.ts">
<violation number="1" location="packages/agenda/src/Agenda.ts:188">
P1: In `_lockOnTheFly`, if the database operation fails, the job (which was already popped from `_jobsToLock`) is permanently lost because it is not returned to the queue. Additionally, the function recursively calls itself immediately after the error, which will rapidly drain the remaining queue and trigger a storm of database errors if the issue is persistent.
Recommended fix: Return the job to the queue and exit the function to prevent job loss and avoid a tight error loop.</violation>
<violation number="2" location="packages/agenda/src/Agenda.ts:188">
P1: The `database()` method catches connection errors and emits an event but fails to rethrow the error. This causes `Agenda.start()` to hang indefinitely because it awaits `this._ready`, which only resolves when the `'ready'` event is emitted (inside `dbInit`). Since `dbInit` is skipped on connection failure, `'ready'` is never emitted.</violation>
</file>
<file name="apps/meteor/app/livechat/server/business-hour/BusinessHourManager.ts">
<violation number="1" location="apps/meteor/app/livechat/server/business-hour/BusinessHourManager.ts:229">
P2: Swallowing the error prevents the Cron system from recording the failure in `CronHistory` and triggers a false 'success' event in Agenda. The `AgendaCronJobs` wrapper relies on the job function throwing an error to correctly mark the job as failed and log it via the Cron logger. Rethrow the error to ensure observability.</violation>
<violation number="2" location="apps/meteor/app/livechat/server/business-hour/BusinessHourManager.ts:237">
P2: Swallowing the error prevents the Cron system from recording the failure in `CronHistory` and triggers a false 'success' event in Agenda. The `AgendaCronJobs` wrapper relies on the job function throwing an error to correctly mark the job as failed and log it via the Cron logger. Rethrow the error to ensure observability.</violation>
</file>
<file name="apps/meteor/app/livechat/server/business-hour/Single.ts">
<violation number="1" location="apps/meteor/app/livechat/server/business-hour/Single.ts:12">
P2: Redundant error handling. The `openBusinessHourDefault` function (in `Helper.ts`) already wraps its logic in a try-catch block and suppresses errors. Consequently, this `catch` block is unreachable, and the `try` wrapper is unnecessary.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-876
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.