fix: address livechat race condition in agent assignment with locking #37776
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: 2a75da3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
|
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. WalkthroughAdds conditional per-agent locking for omnichannel assignment: new agent lock fields and Users lock methods, a conditionalLockAgent helper honoring Livechat_waiting_queue, integration into RoutingManager.takeInquiry with guaranteed unlock, and unit tests for lock behaviors. No public API signature removals. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as RoutingManager
participant Lock as conditionalLockAgent
participant Users as UsersModel
participant Settings as Livechat_Settings
participant Inquiry as LivechatInquiry
Client->>Router: takeInquiry(agentId)
Router->>Lock: conditionalLockAgent(agentId)
Lock->>Settings: read Livechat_waiting_queue
Settings-->>Lock: value
alt waiting queue enabled
Lock->>Users: acquireAgentLock(agentId, lockTime)
Users-->>Lock: acquired (true/false)
Lock-->>Router: {acquired, required, unlock}
alt acquired
Router->>Router: checkAgentBeforeTakeInquiry
Router->>Inquiry: takeInquiry + assignAgent
Inquiry-->>Router: success
else not acquired
Router-->>Client: error (agent locked)
end
else waiting queue disabled
Lock-->>Router: {acquired: false, required: false}
Router->>Inquiry: takeInquiry + assignAgent
end
Router->>Lock: unlock()
Lock->>Users: releaseAgentLock(agentId, lockTime)
Users-->>Lock: released
Lock-->>Router: done
Router-->>Client: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
d445d02 to
72296a8
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/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/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/29 13:48 (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, 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, 0.00]
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, 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, 0.00]
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, 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, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
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, 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, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 29 Jan 2026 13:48:14 GMT |
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
72296a8 to
09cb423
Compare
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
a0b19a1 to
ecdb94a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37776 +/- ##
===========================================
+ Coverage 70.84% 70.87% +0.02%
===========================================
Files 3160 3161 +1
Lines 109775 109785 +10
Branches 19727 19936 +209
===========================================
+ Hits 77775 77813 +38
+ Misses 29974 29948 -26
+ Partials 2026 2024 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
dc37156 to
fcaedbe
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: 1
🧹 Nitpick comments (4)
packages/core-typings/src/ILivechatAgent.ts (1)
17-18: Consider adding JSDoc for the locking fields.The new optional fields support the agent locking mechanism to prevent over-assignment. While the implementation is correct, brief JSDoc comments would clarify their purpose and lifecycle for future maintainers.
Example:
openBusinessHours?: string[]; + /** Timestamp when the agent was locked for assignment operations */ agentLockedAt?: Date; + /** Indicates whether the agent is currently locked for assignment operations */ agentLocked?: boolean;apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
253-255: Consider usingMeteor.Errorfor consistency.Line 254 uses
new Error('error-agent-is-locked')while the rest of this file usesMeteor.Errorfor thrown errors (e.g., lines 88, 124, 145). UsingMeteor.Errorwould provide consistent error handling on the client side.if (options.clientAction && !options.forwardingToDepartment) { - throw new Error('error-agent-is-locked'); + throw new Meteor.Error('error-agent-is-locked'); }packages/models/src/models/Users.ts (2)
61-68: Hardcoded lock timeout should be a shared constant.The 5000ms timeout is hardcoded here and also appears as the default in
acquireAgentLock(line 803). Consider extracting this to a shared constant to ensure consistency and make future adjustments easier.+const AGENT_LOCK_TIMEOUT_MS = 5000; + const queryAvailableAgentsForSelection = (extraFilters = {}, isLivechatEnabledWhenAgentIdle?: boolean): Filter<IUser> => ({ ...queryStatusAgentOnline(extraFilters, isLivechatEnabledWhenAgentIdle), $and: [ { - $or: [{ agentLocked: { $exists: false } }, { agentLockedAt: { $lt: new Date(Date.now() - 5000) } }], + $or: [{ agentLocked: { $exists: false } }, { agentLockedAt: { $lt: new Date(Date.now() - AGENT_LOCK_TIMEOUT_MS) } }], }, ], });Then update line 803:
- async acquireAgentLock(agentId: IUser['_id'], lockTimeoutMs = 5000): Promise<boolean> { + async acquireAgentLock(agentId: IUser['_id'], lockTimeoutMs = AGENT_LOCK_TIMEOUT_MS): Promise<boolean> {
803-818: Consider adding an index for lock fields if high concurrency is expected.The lock acquisition query filters on
agentLockedandagentLockedAt. Under high load with many concurrent assignments, consider adding a partial index on these fields to improve query performance:{ key: { agentLocked: 1, agentLockedAt: 1 }, sparse: true }This is a low-priority optimization that may not be needed depending on deployment scale.
📜 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 (11)
.changeset/cozy-melons-march.md(1 hunks)apps/meteor/app/livechat/server/lib/RoutingManager.ts(2 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts(1 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.ts(2 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.ts(1 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.ts(1 hunks)apps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts(1 hunks)packages/core-typings/src/ILivechatAgent.ts(1 hunks)packages/model-typings/src/models/IUsersModel.ts(1 hunks)packages/models/src/models/Users.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/end-to-end/api/livechat/04-dashboards.tspackages/model-typings/src/models/IUsersModel.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tspackages/core-typings/src/ILivechatAgent.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.tsapps/meteor/app/livechat/server/lib/RoutingManager.tspackages/models/src/models/Users.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts
🧠 Learnings (14)
📚 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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/end-to-end/api/livechat/04-dashboards.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/end-to-end/api/livechat/04-dashboards.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/end-to-end/api/livechat/04-dashboards.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/end-to-end/api/livechat/04-dashboards.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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:
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.tsapps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/04-dashboards.tsapps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.ts
🧬 Code graph analysis (7)
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.ts (1)
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
createConversation(103-123)
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.ts (1)
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
createConversation(103-123)
apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts (1)
apps/meteor/tests/data/livechat/rooms.ts (1)
startANewLivechatRoomAndTakeIt(444-475)
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts (1)
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
createConversation(103-123)
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.ts (1)
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
createConversation(103-123)
apps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts (1)
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
createConversation(103-123)
packages/models/src/models/Users.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)
⏰ 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). (12)
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, stream-hub-service, omnichannel-tran...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (15)
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-chats-filters.spec.ts (1)
33-35: LGTM! Sequential conversation creation prevents race conditions.The shift from parallel to sequential conversation creation aligns with the PR's agent locking mechanism, ensuring that agent assignment operations complete before the next one begins during test setup.
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.ts (2)
62-74: LGTM! Sequential setup prevents agent lock contention.The sequential conversation creation pattern ensures that agent assignment operations complete one at a time during test setup, avoiding the race conditions that this PR addresses.
308-320: LGTM! Consistent sequential pattern.This mirrors the Monitor role setup and maintains consistency across test suites while preventing race conditions during agent assignment.
apps/meteor/tests/e2e/omnichannel/omnichannel-assign-room-tags.spec.ts (1)
56-66: LGTM! Sequential creation avoids agent lock contention.The sequential conversation creation ensures that agent assignment operations don't conflict during test setup, consistent with the PR's locking mechanism.
apps/meteor/tests/e2e/omnichannel/omnichannel-contact-center-filters.spec.ts (2)
96-115: LGTM! Sequential setup prevents race conditions.The sequential conversation creation pattern aligns with the PR's agent locking mechanism and ensures test setup completes without agent assignment conflicts.
117-128: LGTM! Data access pattern correctly reflects conversation structure.The updated access pattern (
conversation.data.room._idandconversation.data.visitor._id) correctly matches the return type ofcreateConversation, which nests room and visitor data under adataproperty..changeset/cozy-melons-march.md (1)
1-8: LGTM! Changeset accurately documents the fix.The changeset correctly identifies the affected packages and provides a clear description of the behavioral change that prevents agent over-assignment through locking.
apps/meteor/tests/end-to-end/api/livechat/04-dashboards.ts (1)
104-113: LGTM! Sequential room creation with helpful documentation.The shift from parallel to sequential room creation prevents agent lock contention, and the inline comment clearly explains the reasoning. The agent distribution logic (first 2 rooms to agent1, remaining to agent2) is correct and maintains test consistency.
apps/meteor/tests/e2e/omnichannel/omnichannel-current-chats.spec.ts (1)
74-106: Sequential conversation creation aligns with the new locking mechanism.The change from parallel
Promise.allto sequentialawaitcalls for conversation creation is appropriate given the new agent locking mechanism. This prevents potential test flakiness when multiple conversations compete for agent locks during setup. The data access pattern correctly uses thecreateConversationreturn structure.apps/meteor/app/livechat/server/lib/RoutingManager.ts (3)
250-257: Lock acquisition with graceful fallback is well-implemented.The lock acquisition correctly handles both the locked state and continues gracefully when
clientActionis false (automated routing). The conditional throw for client-initiated actions ensures users get proper feedback while automated processes can retry with different agents.
259-273: Proper lock cleanup on callback failure.The try-catch correctly releases the agent lock before either re-throwing (for client actions) or allowing the flow to continue with a null agent. This prevents lock leaks when precondition checks fail.
320-324: Lock release in finally block is correctly guarded.The conditional
if (agent?.agentId)properly handles cases where:
- Lock was never acquired (agent set to null at line 256)
- Callback failed and agent was nullified (line 271)
- Normal flow completion
This ensures lock release only when a lock was actually held.
packages/models/src/models/Users.ts (3)
803-835: Atomic lock operations are correctly implemented.The lock acquisition uses MongoDB's atomic
updateOnewith a conditional filter, which is the correct pattern for distributed locking. The release operation properly checksagentLocked: trueto avoid releasing locks held by other processes after timeout expiration.
795-798: Read consistency settings prevent stale reads.Adding
readConcern: { level: 'majority' }andreadPreference: 'primary'ensures that agent chat counts reflect committed data from the primary, preventing race conditions where stale secondary reads could allow over-assignment.
559-562: Locked agents are consistently excluded from all selection methods.The
queryAvailableAgentsForSelectionhelper is correctly applied togetNextLeastBusyAgent,getLastAvailableAgentRouted, andgetNextAgent, ensuring locked agents are excluded regardless of which routing method is used.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/app/livechat/server/lib/RoutingManager.ts(2 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/app/livechat/server/lib/RoutingManager.ts
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (2)
packages/models/src/index.ts (2)
LivechatInquiry(176-176)LivechatRooms(178-178)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnLivechatInquiryChangedById(251-272)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (2)
263-277: Proper lock cleanup on precondition failure.The guard around
livechat.checkAgentBeforeTakeInquirycorrectly releases the agent lock before re-throwing or setting agent to null. This prevents lock leakage when precondition checks fail.
288-325: Lock cleanup is guaranteed; lock timeout is properly implemented.The try-finally block correctly ensures the agent lock is released on all paths (success and exception). The object-based logging at lines 291 and 295 properly addresses the past review comments.
Lock timeout verification complete:
Users.acquireAgentLockimplements a 5-second default timeout with automatic expiration via MongoDB's$ltcomparison, preventing indefinite lock holding.Regarding line 303: throwing plain
Error('error-room-not-found')is consistent with the local convention in this file. Line 153 in the same code path (assignAgent) uses the identical pattern for invariant violations. Meteor.Error is reserved for domain-specific, user-facing errors elsewhere in this file (e.g., lines 88, 124, 145).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
258-258: UseMeteor.Errorfor consistency.Line 258 throws a plain
Errorinstead ofMeteor.Error, which is inconsistent with other user-facing errors in this file (lines 88, 124, 145).
📜 Review details
Configuration used: Organization 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/app/livechat/server/lib/RoutingManager.ts(2 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/app/livechat/server/lib/RoutingManager.ts
🧠 Learnings (2)
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/livechat/server/lib/RoutingManager.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/livechat/server/lib/RoutingManager.ts
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (3)
apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)packages/models/src/index.ts (2)
LivechatInquiry(176-176)LivechatRooms(178-178)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnLivechatInquiryChangedById(251-272)
⏰ 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
apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.ts
Outdated
Show resolved
Hide resolved
ricardogarim
dismissed stale reviews from KevLehman, juliajforesti, and lucas-a-pelegrino
via
bd303c5
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
ad3b474 to
bd303c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/tests/end-to-end/api/livechat/07-queue.ts`:
- Around line 641-644: The cleanup call uses deleteVisitor with visitor._id but
deleteVisitor expects a token; update the Promise.all cleanup to call
deleteVisitor(visitor.token) for each visitor (matching the earlier usage at
line 143), keeping the other cleanup calls (closeOmnichannelRoom(room._id) and
deleteDepartment(department._id)) unchanged so visitors are deleted correctly.
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
184d637 to
ef0e434
Compare
ricardogarim
force-pushed
the
fix/omni-agent-lock
branch
from
ef0e434 to
9b21541
Compare
![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 1 file (changes from recent commits).
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/models/src/models/Users.ts">
<violation number="1">
P1: This aggregation no longer forces primary/majority reads, so in replica set deployments it can return stale agent chat counts and allow over-assignment again. Reapply the primary/majority read options here.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
ricardogarim
removed
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: ready to merge
ricardogarim
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
added
stat: ready to merge
As per SUP-945, in microservices deployments, Rocket.Chat instances could assign more chats to an agent than their configured maximum limit. For example, with 3 instances and an agent limit of 3 simultaneous chats, 4 chats could be assigned (3 on instance 1, 0 on instance 2, 1 on instance 3).
The race condition occurred because multiple instances simultaneously:
All instances saw the same initial count before any assignments completed, allowing over-assignment.
Proposed changes (including videos or screenshots)
Implemented agent locking during assignment to serialize operations per agent, combined with read consistency guarantees from MongoDB primary node.
Explicit locking on the users collection: in our case, transactions wouldn’t catch races unless we added artificial counters or dummy updates. A separate locks collection would add extra models, types, and indexes for little benefit. We chose two optional lock fields directly on the users collection because it’s simpler, aligns with our existing inquiry-locking pattern, and requires minimal changes.
Read consistency with replica sets: reads can hit secondaries and return stale data, causing an instance to see an outdated chat count even after acquiring a lock. To avoid this, we enforce
readPreference: primaryandreadConcern: majority, ensuring all reads reflect committed data from the primary node.Issue(s)
Steps to test or reproduce
Livechat_waiting_queuesettingFurther comments
Adding fields to a shared collection like users is risky. To minimize potential conflicts and make the intention clear, the fields are prefixed as
agentLockedandagentLockedAtinstead of generic names.Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.