fix(models): propagate session to trash recovery operations (#38179) #38241
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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/models/src/models/BaseRaw.ts (2)
327-341: Consider: Inconsistent error handling betweendeleteOneandfindOneAndDelete.Session propagation looks correct here. However, unlike
findOneAndDelete(lines 369-374), this method lacks rollback logic ifcol.deleteOnefails after the trash record is inserted.If the main collection's
deleteOnethrows (line 338-340), the trash record remains orphaned. While session/transaction usage can mitigate this (the transaction will abort on error), callers not using transactions will still have inconsistency.For consistency with
findOneAndDelete, consider wrapping the finaldeleteOnein a try/catch with rollback:🔧 Potential rollback pattern (optional)
+ try { if (options) { - return this.col.deleteOne(filter, options); + return await this.col.deleteOne(filter, options); } - return this.col.deleteOne(filter); + return await this.col.deleteOne(filter); + } catch (e) { + await this.trash?.deleteOne({ _id } as Filter<TDeleted>, { session: options?.session }); + throw e; + }
387-418: Session propagation is correct; same rollback consideration asdeleteOne.The session is properly propagated to:
find()cursor (line 387)- Each
trash.updateOne()call (lines 402-409)- Final
deleteMany()(lines 414-417)However, similar to
deleteOne, if the finaldeleteManyfails after trash records are inserted, there's no rollback. For bulk operations, this could leave multiple orphaned trash records.When using transactions, the session will handle rollback. For non-transactional usage, consider adding try/catch with bulk rollback:
🔧 Potential rollback pattern
+ try { if (options) { return this.col.deleteMany({ _id: { $in: ids } } as unknown as Filter<T>, options); } return this.col.deleteMany({ _id: { $in: ids } } as unknown as Filter<T>); + } catch (e) { + await this.trash?.deleteMany({ _id: { $in: ids } } as unknown as Filter<TDeleted>, { session: options?.session }); + throw e; + }
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/omnichannel/service.ts`:
- Around line 34-43: handlePresenceUpdate currently assumes user.roles is an
array and calls user.roles.some(...), which can throw if roles is undefined;
update the function (handlePresenceUpdate) to defensively handle
missing/non-array roles by checking Array.isArray(user.roles) (or defaulting to
an empty array) before calling .some, then only call
notifyAgentStatusChanged(user._id, user.status as UserStatus) when the
role-check passes.
🧹 Nitpick comments (4)
packages/models/src/models/BaseRaw.ts (1)
32-34: Minor: Unconventional ordering of const and import statements.The
const getCollectionNamedeclaration on line 33 is placed between two import statements. While syntactically valid, it's more conventional to keep all imports together at the top, followed by local declarations.That said, this resolves the circular dependency issue mentioned in the PR objectives, and the logic is correct.
♻️ Suggested reordering
-import { UpdaterImpl } from '../updater'; -const getCollectionName = (name: string): string => `rocketchat_${name}`; -import type { Updater } from '../updater'; +import { UpdaterImpl } from '../updater'; +import type { Updater } from '../updater'; + +const getCollectionName = (name: string): string => `rocketchat_${name}`;apps/meteor/server/modules/listeners/listeners.module.ts (1)
184-209: Consider extracting common presence notification logic.The batch handler duplicates the logic from the single
presence.statushandler (lines 159-182). Extracting this into a shared helper function would reduce duplication and ensure consistency.♻️ Suggested refactor
// Extract helper function const handleUserPresence = (user: { _id: string; username?: string; name?: string; status?: UserStatus; statusText?: string; roles?: string[] }) => { const { _id, username, name, status, statusText, roles } = user; if (!status || !username) { return; } notifications.notifyUserInThisInstance(_id, 'userData', { type: 'updated', id: _id, diff: { status, ...(statusText && { statusText }), }, unset: { ...(!statusText && { statusText: 1 }), }, }); notifications.notifyLoggedInThisInstance('user-status', [_id, username, STATUS_MAP[status], statusText, name, roles]); if (_id) { notifications.sendPresence(_id, username, STATUS_MAP[status], statusText); } }; // Then use in both handlers: service.onEvent('presence.status', ({ user }) => handleUserPresence(user)); service.onEvent('presence.status.batch', (batch) => batch.forEach(({ user }) => handleUserPresence(user)));ee/packages/presence/src/Presence.spec.ts (1)
83-100: Verify previousStatus preservation in debounce scenario.The test verifies that the final status (
'offline') is captured when the same user has multiple updates, but it doesn't verify thepreviousStatusvalue in the emitted batch. Based on the test setup, the second call passes'online'as previousStatus, which would overwrite the first call's'busy'.Consider adding an assertion to confirm the expected
previousStatusbehavior:💡 Suggested enhancement
expect(batch[0].user.status).toBe('offline'); + // Verify previousStatus from the last broadcast call is used + expect(batch[0].previousStatus).toBe('online');ee/packages/presence/src/Presence.ts (1)
131-137: Clear presence batch on stop to avoid retaining user data.
stopped()clears timers but leaves any queued user payloads in memory. Clearing the batch on stop keeps teardown tidy, especially if the service is restarted in-process.♻️ Proposed tweak
if (this.batchTimeout) { clearTimeout(this.batchTimeout); this.batchTimeout = undefined; } + this.presenceBatch.clear(); }
| private async handlePresenceUpdate(user: Pick<IUser, '_id' | 'username' | 'status' | 'statusText' | 'name' | 'roles'>): Promise<void> { | ||
| if (!user?._id) { | ||
| return; | ||
| } | ||
| const hasRole = user.roles.some((role) => ['livechat-manager', 'livechat-monitor', 'livechat-agent'].includes(role)); | ||
| if (hasRole) { | ||
| // TODO change `Livechat.notifyAgentStatusChanged` to a service call | ||
| await notifyAgentStatusChanged(user._id, user.status as UserStatus); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add defensive check for user.roles being undefined.
If user.roles is undefined or not an array, calling .some() on line 38 will throw a TypeError. While the event payload type includes roles, runtime data might not always guarantee its presence.
🛡️ Suggested defensive fix
private async handlePresenceUpdate(user: Pick<IUser, '_id' | 'username' | 'status' | 'statusText' | 'name' | 'roles'>): Promise<void> {
if (!user?._id) {
return;
}
- const hasRole = user.roles.some((role) => ['livechat-manager', 'livechat-monitor', 'livechat-agent'].includes(role));
+ const hasRole = user.roles?.some((role) => ['livechat-manager', 'livechat-monitor', 'livechat-agent'].includes(role));
if (hasRole) {
// TODO change `Livechat.notifyAgentStatusChanged` to a service call
await notifyAgentStatusChanged(user._id, user.status as UserStatus);
}
}🤖 Prompt for AI Agents
In `@apps/meteor/server/services/omnichannel/service.ts` around lines 34 - 43,
handlePresenceUpdate currently assumes user.roles is an array and calls
user.roles.some(...), which can throw if roles is undefined; update the function
(handlePresenceUpdate) to defensively handle missing/non-array roles by checking
Array.isArray(user.roles) (or defaulting to an empty array) before calling
.some, then only call notifyAgentStatusChanged(user._id, user.status as
UserStatus) when the role-check passes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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="ee/packages/presence/src/Presence.ts">
<violation number="1" location="ee/packages/presence/src/Presence.ts:341">
P1: Presence broadcasting now emits only `presence.status.batch`, breaking existing listeners still subscribed to `presence.status`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ee/packages/presence/src/Presence.ts
Outdated
| return; | ||
| } | ||
|
|
||
| this.api?.broadcast('presence.status.batch', batch); |
There was a problem hiding this comment.
P1: Presence broadcasting now emits only presence.status.batch, breaking existing listeners still subscribed to presence.status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ee/packages/presence/src/Presence.ts, line 341:
<comment>Presence broadcasting now emits only `presence.status.batch`, breaking existing listeners still subscribed to `presence.status`.</comment>
<file context>
@@ -287,21 +304,42 @@ export class Presence extends ServiceClass implements IPresence {
+ return;
+ }
+
+ this.api?.broadcast('presence.status.batch', batch);
+ }, 500);
}
</file context>
shash-hq
force-pushed
the
fix/38179-trash-race-condition
branch
from
95713c6 to
e969238
Compare
shash-hq
changed the title
2 participants
Description
This PR fixes a race condition where trash recovery operations were executed outside of the main transaction session. This could lead to data inconsistency (orphaned records) if the transaction was aborted after the trash operation succeeded but before the main operation finished (or vice-versa).
Changes
BaseRaw.tsin@rocket.chat/modelsto propagate thesessionfromdeleteOne,deleteMany, andfindOneAndDeleteoptions to the correspondingtrashCollectionoperations.BaseRaw.tsimports.Verification
upsert: trueandsessionare correctly passed totrash.updateOne.Closes #38179
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.