chore: use @rocket.chat/cron for presence status expiration in microservices - #41153
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
WalkthroughPresence status expiration scheduling is migrated from per-instance ChangesCron-based presence expiration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PresenceService
participant cronJobs
participant Presence
participant Database
PresenceService->>Database: register service models
PresenceService->>cronJobs: start(db)
Presence->>Presence: setupNextExpiration()
Presence->>cronJobs: remove(STATUS_EXPIRATION_JOB)
Presence->>cronJobs: addAtTimestamp(STATUS_EXPIRATION_JOB, statusExpiresAt)
cronJobs-->>Presence: trigger expiration job
Presence->>Presence: processExpiredStatuses()
Presence->>cronJobs: remove(STATUS_EXPIRATION_JOB) on stopped()
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@ee/packages/presence/src/Presence.ts`:
- Around line 154-156: The expiration callback in Presence.addAtTimestamp is
swallowing failures because the .catch() logs and then resolves, causing the
scheduler to treat a failed status expiration as successful. Update the callback
passed to cronJobs.addAtTimestamp so that handleExpirationJob still logs via
logger.error but rethrows the caught error or otherwise returns a rejected
promise, preserving the failure signal.
🪄 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
Run ID: 4f8b71fa-773c-42a2-bc67-192893e99562
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
ee/apps/presence-service/package.jsonee/apps/presence-service/src/service.tsee/packages/presence/package.jsonee/packages/presence/src/Presence.tspackages/models/src/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/apps/presence-service/src/service.tspackages/models/src/index.tsee/packages/presence/src/Presence.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
ee/apps/presence-service/src/service.tspackages/models/src/index.tsee/packages/presence/src/Presence.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
ee/apps/presence-service/src/service.tspackages/models/src/index.tsee/packages/presence/src/Presence.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
ee/apps/presence-service/src/service.tspackages/models/src/index.tsee/packages/presence/src/Presence.ts
🔇 Additional comments (7)
packages/models/src/index.ts (1)
111-111: LGTM!Also applies to: 249-249
ee/packages/presence/src/Presence.ts (3)
7-18: LGTM!
182-186: 🩺 Stability & AvailabilityDon’t remove the shared
STATUS_EXPIRATION_JOBhere unless it is instance-scoped. If that cron name is shared, one shutdown will stop expiration processing for the other running presence instances until something reschedules it.
53-53: 🩺 Stability & AvailabilityGuard the remove/add reschedule path. If
setupNextExpiration()can overlap with a later call, serialize the cron mutation and re-check the token before removing or re-adding the job.ee/packages/presence/package.json (1)
21-21: LGTM!ee/apps/presence-service/package.json (1)
24-24: LGTM!ee/apps/presence-service/src/service.ts (1)
2-2: LGTM!Also applies to: 17-18
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41153 +/- ##
===========================================
+ Coverage 68.21% 68.38% +0.16%
===========================================
Files 3968 3970 +2
Lines 154740 154931 +191
Branches 27825 27892 +67
===========================================
+ Hits 105557 105945 +388
+ Misses 44392 44244 -148
+ Partials 4791 4742 -49
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
d7cecad to
66e61a1
Compare
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
tassoevan
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Move presence status expiration off a hand-rolled
setTimeoutand onto@rocket.chat/cron, plus the wiring cron needs to run in a microservice:ICronHistoryModelinregisterServiceModels(services never did, so cron'sCronHistorywrite threw).cronJobs.start(db)in the presence-service entrypoint.setTimeout→cronJobs.addAtTimestamp— so one instance runs each expiration (agenda locks the job in Mongo), instead of every presence instance running its own timer and reverting independently.Issue(s)
Steps to test or reproduce
yarn ms).POST /api/v1/users.setStatuswithstatus: "busy"and a shortexpiresAt.rocketchat_crongets apresence-status-expirationjob at that time; after it fires the status reverts androcketchat_cron_historygets a run row.Further comments
Summary by CodeRabbit
New Features
Bug Fixes