fix(presence): update connections on heartbeat and remove them when stale #37551
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 |
🦋 Changeset detectedLatest commit: 5633225 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces a PresenceReaper that periodically removes stale user-session connections, adds heartbeat typing and a Heartbeat class definition, integrates heartbeat-driven presence refreshes, and exposes a new Presence.updateConnection method to refresh activity for specific connections. Changes
Sequence DiagramsequenceDiagram
participant User as User Session
participant HB as Heartbeat (DDPCommon.Heartbeat)
participant Client as DDP Client
participant Pres as Presence Service
participant USS as UsersSessions Model
participant Reaper as PresenceReaper
rect rgba(200,220,255,0.6)
Note over User,Client: Active packet observed
User->>Client: send packet
Client->>Client: messageReceived() (sets _seenPacket false/true)
Client->>Pres: updateConnection(uid, connectionId)
Pres->>USS: updateOne / refresh session timestamp
USS-->>Pres: ack updated session
Pres->>Pres: refreshPresence for uid
end
rect rgba(220,255,200,0.6)
Note over Reaper,USS: Periodic cleanup (interval)
Reaper->>USS: query sessions with connections older than cutoff
USS-->>Reaper: user docs with connections
Reaper->>Reaper: processDocument() → build ReaperPlan (removeIds, shouldMarkOffline)
Reaper->>USS: bulkWrite() remove stale connections
Reaper->>Pres: onUpdate([userIds]) when users went offline
Pres->>Pres: handleReaperUpdates() → refresh presence / mark offline
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
cardoso
force-pushed
the
fix/presence-dangling
branch
from
8c8ebb6 to
3c26ca0
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/06 21:43 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 14 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sat, 06 Dec 2025 21:43:08 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37551 +/- ##
===========================================
- Coverage 67.78% 67.77% -0.01%
===========================================
Files 3449 3449
Lines 113987 113986 -1
Branches 20956 20956
===========================================
- Hits 77262 77258 -4
- Misses 34610 34611 +1
- Partials 2115 2117 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
cardoso
force-pushed
the
fix/presence-dangling
branch
from
c180d53 to
d90e850
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/models/src/models/UsersSessions.ts (1)
32-41: Optional status handling is good; consider a more explicit checkMaking
statusoptional and guarding the$setkeeps pings from overwriting the stored status while still refreshing_updatedAt, which matches the intended behavior.To make this future‑proof against any falsy-but-valid status values, you could prefer an explicit undefined check instead of a truthiness check:
- $set: { - ...(status && { 'connections.$.status': status }), - 'connections.$._updatedAt': new Date(), - }, + $set: { + ...(status !== undefined && { 'connections.$.status': status }), + 'connections.$._updatedAt': new Date(), + },apps/meteor/client/meteor/overrides/ddpOverREST.ts (1)
9-29: Fix unsound type predicate and make ping call explicitly fire-and-forgetThe
shouldBypasstype guard (line 9) claims to narrow to non-method messages when it returns true, but it also returnstruefor severalmethodmessages (login/resume, UserPresence:, stream-, and bypassMethods). This violates the type predicate's promise and is unsound.Additionally, the type narrowing isn't actually being used—the code immediately checks
if (message.msg === 'ping')after the predicate succeeds (line 34), indicating the type isn't relied upon.Change
shouldBypassto return a plain boolean:-const shouldBypass = (message: Meteor.IDDPMessage): message is Exclude<Meteor.IDDPMessage, Meteor.IDDPMethodMessage> => { +const shouldBypass = (message: Meteor.IDDPMessage): boolean => { if (message.msg !== 'method') { return true; } const { method, params } = message; // ... rest unchanged }For the ping side effect (lines 34–36), make the fire-and-forget intent explicit and avoid unhandled rejections:
if (message.msg === 'ping') { - sdk.call('UserPresence:ping'); + void sdk.call('UserPresence:ping').catch(() => {}); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
apps/meteor/client/meteor/overrides/ddpOverREST.ts(2 hunks)apps/meteor/definition/externals/meteor/meteor.d.ts(1 hunks)apps/meteor/server/methods/userPresence.ts(2 hunks)ee/apps/ddp-streamer/src/DDPStreamer.ts(1 hunks)ee/apps/ddp-streamer/src/configureServer.ts(1 hunks)ee/packages/presence/src/Presence.ts(2 hunks)ee/packages/presence/src/lib/processConnectionStatus.ts(0 hunks)ee/packages/presence/src/lib/processStatus.ts(1 hunks)ee/packages/presence/src/processPresenceAndStatus.ts(1 hunks)ee/packages/presence/tests/lib/processConnectionStatus.test.ts(1 hunks)ee/packages/presence/tests/lib/processPresenceAndStatus.test.ts(1 hunks)ee/packages/presence/tests/lib/processStatus.test.ts(1 hunks)packages/core-services/src/types/IPresence.ts(1 hunks)packages/model-typings/src/models/IUsersSessionsModel.ts(1 hunks)packages/models/src/models/UsersSessions.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/packages/presence/src/lib/processConnectionStatus.ts
🧰 Additional context used
🧬 Code graph analysis (8)
ee/packages/presence/tests/lib/processStatus.test.ts (1)
ee/packages/presence/src/lib/processStatus.ts (1)
processStatus(6-16)
ee/packages/presence/src/processPresenceAndStatus.ts (3)
packages/core-typings/src/IUserSession.ts (1)
IUserSessionConnection(3-9)ee/packages/presence/src/lib/processConnectionStatus.ts (1)
processConnectionStatus(6-14)ee/packages/presence/src/lib/processStatus.ts (1)
processStatus(6-16)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/apps/ddp-streamer/src/configureServer.ts (2)
ee/packages/presence/src/Presence.ts (1)
Presence(11-284)apps/meteor/server/methods/userPresence.ts (3)
userId(24-30)userId(31-37)userId(38-44)
ee/packages/presence/tests/lib/processPresenceAndStatus.test.ts (1)
ee/packages/presence/src/processPresenceAndStatus.ts (1)
processPresenceAndStatus(16-31)
packages/models/src/models/UsersSessions.ts (1)
packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/apps/ddp-streamer/src/DDPStreamer.ts (3)
ee/apps/ddp-streamer/src/configureServer.ts (4)
server(10-10)userId(119-125)userId(126-132)userId(133-139)apps/meteor/server/methods/userPresence.ts (3)
userId(24-30)userId(31-37)userId(38-44)ee/packages/presence/src/Presence.ts (1)
Presence(11-284)
apps/meteor/server/methods/userPresence.ts (2)
ee/packages/presence/src/Presence.ts (1)
Presence(11-284)ee/apps/ddp-streamer/src/configureServer.ts (3)
userId(119-125)userId(126-132)userId(133-139)
🔇 Additional comments (13)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
8-8: LGTM! Optional status enables ping-based connection tracking.Making the
statusparameter optional allows the system to update connection timestamps (via ping handlers) without forcing a status change. This helps maintain accurate connection state for presence tracking and addresses the dangling session issue.apps/meteor/definition/externals/meteor/meteor.d.ts (1)
50-61: LGTM! Improved type safety for DDP messages.The refactoring separates ping and method messages into distinct interfaces (
IDDPPingMessageandIDDPMethodMessage), then creates a union type. This enables type guards and better type narrowing at call sites, improving type safety for DDP message handling.ee/packages/presence/tests/lib/processStatus.test.ts (1)
1-30: LGTM! Comprehensive test coverage for processStatus.The test suite thoroughly validates all combinations of connection status and default status across the four UserStatus values (ONLINE, BUSY, AWAY, OFFLINE). The tests confirm the expected precedence logic:
- OFFLINE connection status always takes precedence
- ONLINE default allows connection status to pass through
- Non-ONLINE defaults override the connection status
packages/core-services/src/types/IPresence.ts (1)
18-18: LGTM! Parameter reordering improves API consistency.The signature change reorders parameters to
(uid, session, status?)which is more logical (identifying the connection before specifying its status) and makes status optional. This enables ping handlers to update connection tracking without forcing a status change, which is key to fixing dangling sessions.Note: This is a breaking change, but the PR appears to update all call sites consistently.
ee/apps/ddp-streamer/src/configureServer.ts (1)
119-139: LGTM! Presence methods updated consistently.All three presence methods (
UserPresence:online,UserPresence:away, and the newUserPresence:ping) have been updated to use the new signature with parameters in the order(userId, session, status?). The newpingmethod omits the status parameter, allowing it to update connection tracking without changing the user's status, which is crucial for fixing dangling sessions.ee/packages/presence/src/Presence.ts (2)
7-7: LGTM! Import path updated for refactored module.The import has been updated to reflect the reorganization of presence utilities, with
processPresenceAndStatusnow in a dedicated module at the root of the presence package.
206-212: LGTM! Implementation correctly handles optional status.The
setConnectionStatusmethod now accepts an optionalstatusparameter and passes it through toUsersSessions.updateConnectionStatusById. Whenstatusis undefined (as in ping handlers), the connection timestamp is updated without changing the status, which helps maintain accurate connection state while preventing unwanted status changes.ee/packages/presence/src/lib/processStatus.ts (1)
1-16: LGTM! Status precedence logic is well-designed.The
processStatusfunction implements a clear precedence hierarchy for determining user status:
- OFFLINE connection status always takes precedence - respects actual disconnection
- ONLINE default allows connection status to pass through - shows real-time presence
- Non-ONLINE defaults override connection status - respects explicit user status settings (BUSY, AWAY)
This logic is intuitive and well-tested in the corresponding test file.
ee/apps/ddp-streamer/src/DDPStreamer.ts (1)
219-225: LGTM, but note the unawaited promise for error handling purposes.The new
PINGevent handler correctly guards against missinguserIdand successfully maintains connection state. The concern about concurrent ping events is partially mitigated:✅ Safe: MongoDB's atomic
updateOnewith the$positional operator inupdateConnectionStatusByIdprevents data corruption from concurrent updates to the same user's session document.
⚠️ Consideration:Presence.setConnectionStatus()is not awaited in the PING handler, which means errors in eitherupdateConnectionStatusByIdor the subsequentupdateUserPresencecall will be silently swallowed. WhileupdateUserPresenceperforms a read-compute-write sequence that could theoretically result in stale status being written if multiple calls interleave for the same user, this is acceptable for presence tracking since the system is eventually consistent by design. However, for proper error handling and observability, consider awaiting the call or attaching a.catch()handler to log unexpected failures.ee/packages/presence/tests/lib/processConnectionStatus.test.ts (1)
4-4: Import path update looks correctThe new relative path to
processConnectionStatusmatches thetests/lib→src/liblayout and keeps the existing test behavior intact.ee/packages/presence/src/processPresenceAndStatus.ts (1)
7-31: Presence derivation from fresh sessions looks soundFiltering by
_updatedAt <= 5 minutes, reducing withprocessConnectionStatusfromOFFLINE, and then deriving the finalstatusviaprocessStatusgives the expected behavior for empty, stale-only, and mixed-session inputs and cleanly separatesstatusConnectionfromstatus.ee/packages/presence/tests/lib/processPresenceAndStatus.test.ts (1)
6-286: Test suite gives comprehensive coverage of the new presence logicThe scenarios here (single vs multiple connections, no connections, stale vs recent updates, and different default statuses) exercise all key branches of
processPresenceAndStatus, including the 5‑minute freshness window and the precedence rules betweenstatusConnectionandstatus.apps/meteor/server/methods/userPresence.ts (1)
12-44: Server-side ping wiring and parameter reordering look consistentThe new
'UserPresence:ping'method and the reordered arguments forPresence.setConnectionStatus(userId, connection.id, status)align with the updatedsetConnectionStatus(uid, session, status?)signature and allow pings to refresh_updatedAtwithout mutating status, while still guarding for missinguserId/connection.
cardoso
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/spicy-nails-design.md (1)
1-10: Changeset metadata looks good; consider clarifying the summary.The affected packages and patch version bump are appropriate for this fix. However, the summary could be more specific to help users understand the mechanism. Currently it reads generically; consider mentioning that the fix refreshes connection activity via ping messages and excludes idle sessions.
Example revision:
-Fixes user status inaccuracy by refreshing active connections and filtering out the stale ones. +Fixes user status inaccuracy by refreshing connection activity on ping and filtering out idle sessions (no activity for 5+ minutes).This gives future readers clearer context about the fix's scope and timing strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.changeset/spicy-nails-design.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
KevLehman
left a comment
KevLehman
left a comment
There was a problem hiding this comment.
I think diego has more context on how this should behave, so my comments are on code-side.
I don't see anything spooky on the implementation rather than the fact the connections would be always updated (since we're gonna update the updatedAt on every ping)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/ee/server/startup/presence.ts(2 hunks)ee/apps/ddp-streamer/src/DDPStreamer.ts(1 hunks)ee/packages/presence/src/Presence.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/apps/ddp-streamer/src/DDPStreamer.ts
🧰 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/packages/presence/src/Presence.tsapps/meteor/ee/server/startup/presence.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/ee/server/startup/presence.ts (1)
32-32: Good type annotation for the login parameter.The explicit typing clarifies the expected shape of the login object and improves code maintainability.
ee/packages/presence/src/Presence.ts (5)
13-21: Throttle implementation is correct.The per-connection throttling logic correctly gates status updates to once per 60 seconds, updating the timestamp only when proceeding. Combined with cleanup in
removeConnectionandremoveStaleConnections, this prevents unbounded map growth.
95-98: Verify the stale connection cutoff duration.The PR description states connections older than "5 minutes" are treated as stale, but
CONNECTION_STATUS_UPDATE_INTERVALis set to 60000ms (1 minute). Confirm whether the 1-minute cutoff is intentional or if it should be 5 minutes (300000ms) as described.Also applies to: 178-179
258-267: Good design: throttling applies only to heartbeat-driven updates.When
statusis explicitly provided (user action), the update proceeds immediately. Throttling only applies to heartbeat pings (no status), which aligns with the PR objective of updating_updatedAtwithout flooding the database.
111-117: Proper cleanup of timers in stopped().Both the timeout and interval are correctly cleared, preventing resource leaks when the service is stopped.
165-167: Memory leak addressed: cleanup on connection removal.The
lastConnectionStatusUpdatemap entry is properly deleted when a connection is removed, preventing unbounded map growth.
| const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat); | ||
| session.heartbeat.messageReceived = function messageReceived() { | ||
| void Presence.setConnectionStatus(login.user._id, login.connection.id); | ||
| return _messageReceived(); | ||
| }; |
There was a problem hiding this comment.
Missing null check for session.heartbeat.
The code assumes session.heartbeat exists, but if it's undefined (e.g., for non-WebSocket transports or during session initialization), this will throw a TypeError when accessing .messageReceived.
+ if (!session.heartbeat) {
+ return;
+ }
+
const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat);
session.heartbeat.messageReceived = function messageReceived() {
void Presence.setConnectionStatus(login.user._id, login.connection.id);
return _messageReceived();
};📝 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.
| const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat); | |
| session.heartbeat.messageReceived = function messageReceived() { | |
| void Presence.setConnectionStatus(login.user._id, login.connection.id); | |
| return _messageReceived(); | |
| }; | |
| if (!session.heartbeat) { | |
| return; | |
| } | |
| const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat); | |
| session.heartbeat.messageReceived = function messageReceived() { | |
| void Presence.setConnectionStatus(login.user._id, login.connection.id); | |
| return _messageReceived(); | |
| }; |
🤖 Prompt for AI Agents
In apps/meteor/ee/server/startup/presence.ts around lines 43 to 47, the code
assumes session.heartbeat exists and directly binds messageReceived which can
throw if heartbeat is undefined; guard access by checking that session.heartbeat
and session.heartbeat.messageReceived are defined before capturing and replacing
the handler, and only override messageReceived when those checks pass (otherwise
skip the override) so non-WebSocket or uninitialized sessions won’t cause a
TypeError.
aac4542 to
e59c169
Compare
cardoso
force-pushed
the
fix/presence-dangling
branch
from
e59c169 to
3f106f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/ee/server/startup/presence.ts (1)
32-47: Guardsession.heartbeatand preservemessageReceivedarguments
session.heartbeatis assumed to exist; if it’s undefined, accessing.messageReceivedwill throw. Also, the wrapper currently drops any arguments passed tomessageReceived.Consider:
- const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat); - session.heartbeat.messageReceived = function messageReceived() { - void Presence.updateConnection(login.user._id, login.connection.id); - return _messageReceived(); - }; + const heartbeat = session.heartbeat; + if (!heartbeat || typeof heartbeat.messageReceived !== 'function') { + return; + } + + const originalMessageReceived = heartbeat.messageReceived.bind(heartbeat); + heartbeat.messageReceived = function messageReceived(...args: unknown[]) { + void Presence.updateConnection(login.user._id, login.connection.id); + return originalMessageReceived(...args); + };This avoids crashes on sessions without heartbeat and keeps the original handler behavior intact.
🧹 Nitpick comments (4)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
6-10: New model methods are well-typed; consider aligning all public APIs
updateConnectionByIdandfindStaleConnectionssignatures match the raw implementation and intended usage.If
removeConnectionsByConnectionIdsfromUsersSessionsRawis meant to be used through the proxied model, consider adding it here as well to keep the public typings in sync.packages/models/src/models/UsersSessions.ts (1)
48-85: DB helpers are correct; consider usage and typingsThe three helpers are structurally sound:
updateConnectionByIdcorrectly targets a single connection via the positional operator.findStaleConnectionsandremoveConnectionsByConnectionIdsexpress the intended queries/updates.If
findStaleConnections/removeConnectionsByConnectionIdsare not actually used, consider either wiring them into the presence cleanup flow (instead of duplicating logic there) or removing them to avoid unused API surface. You might also narrow the cursor type or projection forfindStaleConnectionsif only_idis needed.ee/packages/presence/src/Presence.ts (2)
160-173: Reconsider hard‑coded test user skip and console logging
updateConnection/removeConnectionare production presence paths, butremoveConnectionnow special‑casesuid === 'rocketchat.internal.admin.test'and logs directly toconsole.log. This bakes a test‑only concern and noisy logging into core presence handling.If this is only for local or CI testing, consider:
- Guarding it behind an explicit test flag or environment check, or
- Moving the behavior into test helpers/fakes instead of the main service.
This keeps production behavior and logs clean.
Also applies to: 245-249
160-173:updateConnectionbehavior is sound; consider using the new model helper consistentlyWithin this service,
updateConnectioncorrectly:
- Throttles per session via
shouldUpdateConnectionStatus.- Uses
UsersSessions.updateConnectionByIdand checksmodifiedCount.- Calls
updateUserPresenceonly when the document actually changed.Given you’ve also added
findStaleConnections/removeConnectionsByConnectionIdson the model, consider using those helpers inremoveStaleConnectionsfor consistency and to centralize connection‑level query logic in the model layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/meteor/ee/server/startup/presence.ts(1 hunks)ee/apps/ddp-streamer/src/DDPStreamer.ts(1 hunks)ee/packages/presence/src/Presence.ts(5 hunks)ee/packages/presence/src/lib/constants.ts(1 hunks)ee/packages/presence/src/lib/processConnectionStatus.ts(2 hunks)ee/packages/presence/tests/lib/processConnectionStatus.test.ts(1 hunks)packages/core-services/src/types/IPresence.ts(1 hunks)packages/model-typings/src/models/IUsersSessionsModel.ts(1 hunks)packages/models/src/models/UsersSessions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/presence/src/lib/processConnectionStatus.ts
- ee/apps/ddp-streamer/src/DDPStreamer.ts
- ee/packages/presence/tests/lib/processConnectionStatus.test.ts
🧰 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/packages/presence/src/lib/constants.tspackages/models/src/models/UsersSessions.tsapps/meteor/ee/server/startup/presence.tspackages/model-typings/src/models/IUsersSessionsModel.tsee/packages/presence/src/Presence.tspackages/core-services/src/types/IPresence.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/ee/server/startup/presence.ts
🧬 Code graph analysis (3)
apps/meteor/ee/server/startup/presence.ts (1)
ee/packages/presence/src/Presence.ts (1)
Presence(11-393)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/packages/presence/src/Presence.ts (3)
ee/packages/presence/src/lib/constants.ts (2)
UPDATE_INTERVAL(9-9)STALE_THRESHOLD(14-14)packages/models/src/index.ts (1)
UsersSessions(216-216)packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/presence/src/lib/constants.ts (1)
1-14: Constants definitions look goodNames and values are clear and consistent with their intended usage in presence logic.
packages/core-services/src/types/IPresence.ts (1)
5-20: CurrentsetConnectionStatussignature is correct and consistentThe interface and implementation both use
setConnectionStatus(uid: string, status: UserStatus, session: string), and all call sites adhere to this order. The distinction between bumping_updatedAtand updating status is already handled by separate methods:updateConnection(uid, session)updates the timestamp only, whilesetConnectionStatus(uid, status, session)updates the connection status. Status is a required parameter by design—there is no indication that it should be made optional.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ee/packages/presence/src/Presence.ts (3)
28-34:staleConIntervalis declared but never assigned.The
staleConIntervalfield is cleared instopped()(line 115) but is never set anywhere in the code. This appears to be leftover from refactoring. Consider removing it if unused, or wire it up if intended.- private staleConInterval?: NodeJS.Timeout; -Also update
stopped():async stopped(): Promise<void> { clearTimeout(this.lostConTimeout); - clearInterval(this.staleConInterval); + // Consider adding: this.reaper.stop() once stop() is implemented }
159-169: Consider removing or guarding verbose debug logging.The
console.debugon line 160 will fire on every WebSocket ping, potentially creating significant log noise in production. Consider removing it or guarding with a debug flag.async updateConnection(uid: string, session: string): Promise<{ uid: string; session: string } | undefined> { - console.debug(`Updating connection for user ${uid} and session ${session}`); const result = await UsersSessions.updateConnectionById(uid, session);
171-234: Remove commented-out code.This large block of commented-out code appears to be superseded by the
PresenceReaperimplementation. Dead code adds noise and should be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ee/packages/presence/src/Presence.ts(5 hunks)ee/packages/presence/src/lib/PresenceReaper.spec.ts(1 hunks)ee/packages/presence/src/lib/PresenceReaper.ts(1 hunks)ee/packages/presence/src/lib/processConnectionStatus.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/packages/presence/src/lib/processConnectionStatus.tsee/packages/presence/src/lib/PresenceReaper.spec.tsee/packages/presence/src/lib/PresenceReaper.tsee/packages/presence/src/Presence.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
🧠 Learnings (10)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
🧬 Code graph analysis (3)
ee/packages/presence/src/lib/PresenceReaper.spec.ts (3)
ee/packages/presence/src/lib/PresenceReaper.ts (1)
PresenceReaper(13-131)packages/model-typings/src/models/IUsersSessionsModel.ts (1)
IUsersSessionsModel(6-20)packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/packages/presence/src/lib/PresenceReaper.ts (2)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
IUsersSessionsModel(6-20)packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/packages/presence/src/Presence.ts (2)
ee/packages/presence/src/lib/PresenceReaper.ts (1)
PresenceReaper(13-131)packages/models/src/index.ts (1)
UsersSessions(216-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (13)
ee/packages/presence/src/lib/processConnectionStatus.ts (1)
1-1: LGTM!Clean import consolidation combining value and type imports in a single statement.
ee/packages/presence/src/lib/PresenceReaper.spec.ts (4)
1-11: LGTM!Imports and mock type definition are appropriate for testing the PresenceReaper functionality.
20-34: LGTM!Test setup provides good isolation with
beforeEachand appropriate mocking of dependencies.
36-88: LGTM!Tests thoroughly cover the
processDocumentbusiness logic including stale connection identification and offline status determination. Test names clearly communicate expected behavior.
90-133: LGTM!Integration test validates the complete
run()flow including correctbulkWriteoperations andonUpdatecallback invocation.ee/packages/presence/src/lib/PresenceReaper.ts (4)
1-11: LGTM!Type definitions are well-structured and capture the necessary state for the reaping process.
24-32: LGTM!Constructor properly initializes the reaper with sensible defaults (5-minute stale threshold, 500 batch size).
46-73: LGTM!The
run()method correctly implements batch processing with proper cutoff calculation and memory-conscious batch flushing.
99-112: Good race condition mitigation.The
$pulloperation correctly includes bothidand_updatedAtconditions, ensuring connections refreshed after the initial scan are not incorrectly removed.ee/packages/presence/src/Presence.ts (4)
7-13: LGTM!Import and exported constant are appropriate for the presence service configuration.
89-91: Reaper is started but never stopped.The reaper is initialized and started here, but there's no corresponding
stop()call in thestopped()method. OncePresenceReaper.stop()is implemented (per earlier comment), it should be called instopped().
103-111: LGTM!Handler correctly processes reaper updates with appropriate logging and concurrent user presence updates.
237-240: Remove this comment — the check is intentional test infrastructure, not problematic hardcoding.The
rocketchat.internal.admin.testuser is explicitly created only whenprocess.env.TEST_MODE === 'true'(inapps/meteor/server/startup/initialData.js) and the skip is by design to prevent cleanup of this test fixture during connection handling. This is a standard test support pattern, not a maintenance risk or security concern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/ee/server/startup/presence.ts (1)
43-43: Missing null check forsession.heartbeatbefore accessing its properties.This issue was previously flagged but remains unaddressed. If
session.heartbeatis undefined (possible for non-WebSocket transports or during session initialization), accessing.messageReceivedwill throw a TypeError and crash the login handler.Apply this diff to guard the heartbeat access:
+ if (!session.heartbeat || !session.heartbeat.messageReceived) { + return; + } + const _messageReceived = session.heartbeat.messageReceived.bind(session.heartbeat); session.heartbeat.messageReceived = function messageReceived() { if (this._seenPacket === false) { void Presence.updateConnection(login.user._id, login.connection.id); } return _messageReceived(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/ee/server/startup/presence.ts(1 hunks)
🧰 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:
apps/meteor/ee/server/startup/presence.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/ee/server/startup/presence.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/presence.ts (1)
ee/packages/presence/src/Presence.ts (1)
Presence(15-384)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
cardoso
force-pushed
the
fix/presence-dangling
branch
from
445db39 to
ff983de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/presence/src/Presence.ts (1)
108-113: Resource leak: reaper is not stopped in service lifecycle.The reaper interval continues running after the service stops, which can cause memory leaks and unexpected behavior. This was previously flagged but remains unaddressed.
Implement
PresenceReaper.stop()to clear its interval, then call it here:async stopped(): Promise<void> { + this.reaper?.stop(); if (!this.lostConTimeout) { return; } clearTimeout(this.lostConTimeout); }Note: You'll also need to add the
stop()method to PresenceReaper (inee/packages/presence/src/lib/PresenceReaper.ts):private intervalHandle?: NodeJS.Timeout; public start() { if (this.running) return; this.running = true; this.intervalHandle = setInterval(() => { this.run().catch((err) => console.error('[PresenceReaper] Error:', err)); }, 60 * 1000); console.log('[PresenceReaper] Service started.'); } public stop() { if (this.intervalHandle) { clearInterval(this.intervalHandle); this.intervalHandle = undefined; } this.running = false; }
🧹 Nitpick comments (3)
ee/packages/presence/src/Presence.ts (3)
98-106: Add error handling for batch presence updates.
Promise.allwill reject if any singleupdateUserPresencecall fails, potentially leaving other users in the batch with stale presence state. Consider usingPromise.allSettledor wrapping each update in a try-catch to ensure partial failures don't block the entire batch.- await Promise.all(userIds.map((uid) => this.updateUserPresence(uid))); + const results = await Promise.allSettled(userIds.map((uid) => this.updateUserPresence(uid))); + const failures = results.filter((r) => r.status === 'rejected'); + if (failures.length > 0) { + console.error(`[PresenceReaper] Failed to update ${failures.length} users.`); + }
156-166: LGTM: Clean connection update flow.The method correctly updates the connection timestamp, early-returns on no-op updates, and refreshes user presence. This aligns with the PR objective of handling heartbeats to prevent dangling sessions.
Optional: Consider replacing
console.debugwith a structured logging framework for consistency with production logging practices.
169-172: Test-specific logic hardcoded in production code.The hardcoded check for
'rocketchat.internal.admin.test'should be externalized to configuration or an environment variable to keep test logic separate from production code.Consider one of these approaches:
Option 1: Environment variable
+ private readonly testUserId = process.env.PRESENCE_TEST_USER_ID; + async removeConnection(uid: string | undefined, session: string | undefined): Promise<{ uid: string; session: string } | undefined> { - if (uid === 'rocketchat.internal.admin.test') { - console.log('Admin detected, skipping removal of connection for testing purposes.'); + if (this.testUserId && uid === this.testUserId) { + console.log(`Test user ${uid} detected, skipping removal of connection.`); return; }Option 2: Settings-based flag
async removeConnection(uid: string | undefined, session: string | undefined): Promise<{ uid: string; session: string } | undefined> { + const testUser = await Settings.getValueById('Presence_Test_User_Id'); + if (testUser && uid === testUser) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ee/packages/presence/src/Presence.ts(5 hunks)
🧰 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/packages/presence/src/Presence.ts
🧬 Code graph analysis (1)
ee/packages/presence/src/Presence.ts (2)
ee/packages/presence/src/lib/PresenceReaper.ts (1)
PresenceReaper(13-131)packages/models/src/index.ts (1)
UsersSessions(216-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
🔇 Additional comments (2)
ee/packages/presence/src/Presence.ts (2)
7-7: LGTM: Clean integration of PresenceReaper.The import and field declaration properly introduce the reaper component for managing stale connection cleanup.
Also applies to: 29-29
84-85: LGTM: Proper reaper initialization.The reaper is correctly instantiated with the UsersSessions model and callback, then started. The callback binding to
handleReaperUpdatesestablishes the update flow for stale connection cleanup.
Adds resilience and configurability to presence reaping and connection updates to prevent dangling/stale connections and unhandled errors. - Ensure async connection updates on heartbeat are caught and logged to avoid unhandled rejections. - Refactor presence reaper to accept options (usersSessions, onUpdate, stale threshold, batch size), use a typed non-empty callback, and manage its interval lifecycle (start/stop). - Make reaper notify via batched callbacks and treat empty batches safely. - Update Presence service to instantiate and start/stop the reaper correctly and adapt to the new callback signature. - Simplify updateConnection to use a direct updateOne query and return connectionId for clarity. - Expand and harden tests for PresenceReaper (empty cursor handling, batching behavior). These changes reduce dangling connections, improve testability, and make reaper behavior configurable and safer in production.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ee/packages/presence/src/Presence.ts (1)
157-179: Core logic is correct; consider removing debug logging.The
updateConnectionmethod correctly:
- Uses the positional operator
$to update only the matched connection- Returns undefined when no modification occurs
- Triggers presence recalculation on successful update
Consider removing the
console.debugat line 158 as per coding guidelines (avoid code comments/debug artifacts in implementation). The codebase does have other console.log statements for operational logging, so if this debug output serves a specific troubleshooting need, it could remain, but otherwise it's a candidate for removal.ee/packages/presence/src/lib/PresenceReaper.ts (1)
1-24: LGTM!The type definitions are well-structured. The
NonEmptyArraytype guard pattern at lines 13-15 is a good practice for ensuring type safety when invoking the callback.Note: The explicit import of
setIntervalfrom'node:timers'at line 1 is valid but unusual sincesetIntervalis a Node.js global. This might be a style preference for explicitness or to aid type inference. If consistency with the rest of the codebase is preferred, this import could be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/meteor/ee/server/startup/presence.ts(1 hunks)ee/packages/presence/src/Presence.ts(5 hunks)ee/packages/presence/src/lib/PresenceReaper.spec.ts(1 hunks)ee/packages/presence/src/lib/PresenceReaper.ts(1 hunks)packages/core-services/src/types/IPresence.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/startup/presence.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/packages/presence/src/lib/PresenceReaper.spec.tsee/packages/presence/src/lib/PresenceReaper.tspackages/core-services/src/types/IPresence.tsee/packages/presence/src/Presence.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
🧠 Learnings (9)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
ee/packages/presence/src/lib/PresenceReaper.spec.ts
🧬 Code graph analysis (2)
ee/packages/presence/src/lib/PresenceReaper.ts (2)
packages/model-typings/src/models/IUsersSessionsModel.ts (1)
IUsersSessionsModel(6-18)packages/core-typings/src/IUserSession.ts (1)
IUserSession(11-14)
ee/packages/presence/src/Presence.ts (2)
ee/packages/presence/src/lib/PresenceReaper.ts (1)
PresenceReaper(26-156)packages/models/src/index.ts (1)
UsersSessions(216-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (12)
packages/core-services/src/types/IPresence.ts (1)
16-16: LGTM!The new
updateConnectionmethod signature is clear and type-safe, properly expressing the success case (returning the uid/connectionId pair) and failure case (undefined).ee/packages/presence/src/lib/PresenceReaper.spec.ts (3)
20-39: LGTM!The test setup is well-structured with appropriate mocks. Using a small
batchSize: 2effectively tests the batching logic without requiring many test documents.
41-93: Well-structured business logic tests.The tests thoroughly validate the core stale connection identification logic. Using
@ts-expect-errorto test private methods is an acceptable pattern for unit testing internal behavior.
95-170: LGTM!The integration tests provide comprehensive coverage of the reaper's behavior, including edge cases (empty collections), core operations (bulkWrite), and batching logic (multiple users with batch boundaries).
ee/packages/presence/src/Presence.ts (4)
29-39: LGTM!The PresenceReaper is properly configured with appropriate values (5-minute stale threshold, 500-item batch size) and correctly wired to the
handleReaperUpdatescallback.
86-86: LGTM!The reaper is correctly started during service initialization.
103-106: LGTM!The
handleReaperUpdatescallback correctly processes batches of user IDs reported by the reaper, triggering presence updates for each affected user.
108-114: LGTM!The reaper is properly stopped during service shutdown, addressing the cleanup concern from previous reviews.
ee/packages/presence/src/lib/PresenceReaper.ts (4)
47-69: LGTM!The start/stop lifecycle is correctly implemented with:
- Guards against duplicate intervals
- Proper error handling to prevent uncaught promise rejections
- Clean interval cleanup
This addresses the previous review concern about missing stop mechanism.
71-98: LGTM!The main
run()logic is well-structured:
- Calculates cutoff once for consistency
- Uses projection to minimize data transfer
- Implements efficient batching with final flush for remainder
100-116: LGTM!The
processDocumentlogic correctly:
- Separates stale from valid connections based on the cutoff
- Determines offline status only when all connections are stale
- Preserves the cutoffDate for race-condition protection in the bulkWrite operation
118-155: LGTM! Race condition properly mitigated.The
flushBatchimplementation correctly addresses the race condition concern from previous reviews. At lines 131-132, the$pulloperation includes both the connectionidand the_updatedAtcondition, ensuring that connections refreshed after the initial query are not removed.The direct access to
col.bulkWriteat line 148 bypasses the model abstraction, but this appears intentional sinceIUsersSessionsModeldoesn't expose a bulkWrite method, and this pattern is consistent with MongoDB driver usage for batch operations.
ee/packages/presence/src/Presence.ts
Outdated
| if (uid === 'rocketchat.internal.admin.test') { | ||
| console.log('Admin detected, skipping removal of connection for testing purposes.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test-specific logic in production code path.
The hardcoded check for 'rocketchat.internal.admin.test' is a test-specific workaround in the production code path. This violates separation of concerns and could cause confusion or unintended behavior if this user ID appears in production.
Consider one of these alternatives:
- Use dependency injection to provide a test-specific implementation of the removal logic
- Configure excluded user IDs via environment variables or configuration
- Mock the
removeConnectionmethod entirely in tests rather than relying on special-case logic in the implementation
async removeConnection(uid: string | undefined, session: string | undefined): Promise<{ uid: string; session: string } | undefined> {
- if (uid === 'rocketchat.internal.admin.test') {
- console.log('Admin detected, skipping removal of connection for testing purposes.');
- return;
- }
if (!uid || !session) {
return;
}📝 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.
| if (uid === 'rocketchat.internal.admin.test') { | |
| console.log('Admin detected, skipping removal of connection for testing purposes.'); | |
| return; | |
| } | |
| async removeConnection(uid: string | undefined, session: string | undefined): Promise<{ uid: string; session: string } | undefined> { | |
| if (!uid || !session) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In ee/packages/presence/src/Presence.ts around lines 182 to 185, remove the
hardcoded test-specific check for 'rocketchat.internal.admin.test' and replace
it with a testable, configurable approach: accept an injected predicate or
configuration (e.g., an excludedUserIds set or a shouldSkipRemoval(userId)
function) on the Presence instance, defaulting to none in production; update
tests to inject a predicate or mock the removeConnection method instead of
relying on the special-case; ensure the new configuration is optional and
documented so production behavior remains unchanged when not provided.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/apps/ddp-streamer/src/Client.ts (1)
77-77: Idle/heartbeat state machine is sound; consider clarifying_seenPacketsemanticsThe
_seenPacketflip inhandleIdle()and subsequent check inmessageReceived()gives you a clean two‑phase idle flow:
- After
TIMEOUTof inactivity,handleIdle()pings and sets_seenPacket = falseonly for authenticated users (Lines 205‑208).- The next inbound message flips
_seenPacketback totrueand triggers a presence refresh once (Lines 185‑193).Functionally this looks correct and low‑risk.
As an optional polish, you might rename
_seenPacketto something that reflects its “after idle ping” role (e.g.,_seenPacketSinceIdlePing) to make the small state machine more self‑documenting and reduce cognitive load for future readers.Also applies to: 205-211
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ee/apps/ddp-streamer/src/Client.ts(5 hunks)
🧰 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/ddp-streamer/src/Client.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/apps/ddp-streamer/src/Client.ts (1)
4-4: Presence update on post-idle traffic is safe and correctThe integration pattern is sound:
messageReceived()is called for every parsed message, but only updates Presence when transitioning from idle (when_seenPacketwasfalseand auserIdexists)- The guard
if (this._seenPacket || !this.userId)ensures the update runs only once per idle→active transitionPresence.updateConnectionis idempotent—it performs an atomic MongoDBupdateOneon the connection's_updatedAttimestamp, which safely handles multiple calls with the same(userId, connectionId)- The method gracefully handles reaped/closed connections by returning
undefinedwhen the query matches zero documents (modifiedCount === 0)- The fire-and-forget pattern with explicit
.catch()is appropriate; failures don't block DDP message processingThis achieves the goal of bumping connection freshness when the client proves it's still alive after an idle heartbeat.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/presence/src/Presence.ts (1)
157-179: LGTM! Atomic and safe connection update.The method correctly uses the positional operator to atomically update a specific connection's timestamp, validates the result, and triggers presence recalculation. The implementation is consistent with the PR objective of preventing dangling sessions by refreshing connection activity on heartbeat.
Optional: Consider reducing console.debug verbosity.
Line 158 will log on every heartbeat/ping. While
console.debugis typically disabled in production environments, you might consider removing this or wrapping it in a debug flag if verbose logging becomes an issue during development.- console.debug(`Updating connection for user ${uid} and connection ${connectionId}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ee/packages/presence/src/Presence.ts(5 hunks)
🧰 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/packages/presence/src/Presence.ts
🧠 Learnings (1)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
ee/packages/presence/src/Presence.ts
🧬 Code graph analysis (1)
ee/packages/presence/src/Presence.ts (2)
ee/packages/presence/src/lib/PresenceReaper.ts (1)
PresenceReaper(26-156)packages/models/src/index.ts (1)
UsersSessions(216-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
ee/packages/presence/src/Presence.ts (2)
29-39: LGTM! Clean PresenceReaper integration.The reaper is properly configured with reasonable defaults (5-minute stale threshold, batch size of 500) and correctly wired to the
handleReaperUpdatescallback.
86-86: LGTM! Proper lifecycle management.The reaper is correctly started and stopped with the service lifecycle, and the
handleReaperUpdatescallback efficiently processes batch updates usingPromise.all.Also applies to: 103-106, 109-109
cardoso
changed the title
Proposed changes (including videos or screenshots)
The
Presence.updateConnectionfunction is called when a heartbeat is received from a connection.Connections with an
_updatedAtfield older than 5 minutes are considered stale and removed periodically.Issue(s)
https://rocketchat.atlassian.net/browse/SUP-846
Steps to test or reproduce
Further comments
The majority of changed lines reported are just splitting unit tests and independent functions on the presence service.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.