feat(enterprise): Implement cold storage archiving for read receipts#38623
feat(enterprise): Implement cold storage archiving for read receipts#38623
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: df22c76 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38623 +/- ##
===========================================
+ Coverage 69.74% 69.81% +0.07%
===========================================
Files 3291 3295 +4
Lines 119089 119168 +79
Branches 21421 21491 +70
===========================================
+ Hits 83057 83198 +141
+ Misses 32730 32675 -55
+ Partials 3302 3295 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@copilot fix the CI issues |
The PR Title Checker is failing because the title has a typo: "code storage" should be "cold storage". The PR title needs to be updated through the GitHub UI to: |
|
@copilot fix code lint and type check which failed on CI |
Fixed TypeScript type errors in the
All methods now properly match the |
|
@copilot read the CI logs of "TypeScript" and "Code Lint" and fix the reported errors |
I'm unable to access the CI logs as the workflows are still in progress for commit 42ec30f. The TypeScript and Code Lint checks haven't completed yet. I've proactively verified my recent changes:
Once the CI completes, I can access the specific error messages and fix them. Alternatively, if you can share the specific errors from the CI logs, I can address them immediately. |
There was a problem hiding this comment.
6 issues found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/ee/server/cron/readReceiptsArchive.ts">
<violation number="1" location="apps/meteor/ee/server/cron/readReceiptsArchive.ts:57">
P0: Only ignore true duplicate-key insert errors; the current check swallows any MongoBulkWriteError and can delete receipts that were never archived.</violation>
</file>
<file name="apps/meteor/app/livechat/server/lib/rooms.ts">
<violation number="1" location="apps/meteor/app/livechat/server/lib/rooms.ts:290">
P1: Adding the archive deletion shifted `Promise.allSettled` indexes, but the subsequent `result[...]` checks were not updated, so removal notifications are now keyed off the wrong operations.</violation>
</file>
<file name="apps/meteor/app/lib/server/functions/cleanRoomHistory.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146">
P1: Read receipts are no longer deleted when cleaning history without a limit, leaving orphaned hot/archive receipts after message deletion.</violation>
</file>
<file name="apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts">
<violation number="1" location="apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts:151">
P2: Deduplicate merged hot+archive receipts; otherwise reads can return duplicate entries while archiving is in progress.</violation>
</file>
<file name="apps/meteor/app/lib/server/functions/deleteMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/deleteMessage.ts:72">
P1: This unconditional archive-delete call can throw at runtime when `IReadReceiptsArchiveModel` is not registered, breaking message deletion in non-EE model setups.</violation>
</file>
<file name="apps/meteor/ee/server/startup/readReceiptsArchive.ts">
<violation number="1" location="apps/meteor/ee/server/startup/readReceiptsArchive.ts:5">
P2: Using `watch` here causes immediate duplicate startup re-scheduling and can race `has/remove/add` for the same cron job; it also skips rescheduling when cron is cleared to an empty value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
sampaiodiego
left a comment
There was a problem hiding this comment.
there are other several comments from AI agents that need to be addressed.
There was a problem hiding this comment.
I'm not a big fan of these where we need to test if the mocked models were called, it usually doesn't test the functionality itself.
b011d77 to
cb909b1
Compare
Agent-Logs-Url: https://github.com/RocketChat/Rocket.Chat/sessions/edad5fcb-4250-48d1-b2bd-e930349ee58e Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Addressed review feedback in commit 8836ff3: Changes made:
Note on test feedback: |
|
misses a changeset too |
Implementation Complete: Configurable Settings for Read Receipts Archiving ✅
Successfully implemented configurable settings for the read receipts archiving cron job and retention time, with comprehensive unit tests and translations.
New Settings
1. Message_Read_Receipt_Archive_Retention_Days
message-read-receiptmodule2. Message_Read_Receipt_Archive_Cron
message-read-receiptmodule3. Message_Read_Receipt_Archive_Batch_Size
message-read-receiptmoduleImplementation Highlights
✅ Configurable Retention: Admins can set custom retention periods (e.g., 7, 30, 90 days)
✅ Flexible Scheduling: Cron schedule can be customized (hourly, daily, weekly, etc.)
✅ Batch Processing: Processes receipts in configurable batches to prevent server overload
✅ Rate Limiting: 1-second delay between batches prevents flooding during high migrations
✅ Dynamic Updates: Cron schedule updates immediately when setting changes
✅ Secondary Read Preference: Archive reads prefer secondary replica set members
✅ Clean Implementation: Read preference passed directly to queries (no secondary collection)
✅ Comprehensive Tests: 8 unit tests covering all scenarios including batch processing
✅ Backward Compatible: Defaults to original values (30 days, 2 AM, 10k batch)
✅ Type Safe: Full TypeScript implementation with proper types
✅ Enterprise Gated: Properly requires license module
✅ Internationalized: English and Portuguese translations included
✅ Lint Clean: All ESLint rules properly handled
✅ TypeScript Clean: All type errors resolved
✅ Code Review Addressed: Uses existing utilities and follows logging conventions
Recent Changes (Code Review Feedback)
sleep()function withlib/utils/sleeplogger.info(\Starting to archive...`)`logger.info({ msg: 'Starting to archive old read receipts', batchSize, cutoffDate })Batch Processing Details
The archiving process:
Read Performance Optimization
Archive queries use MongoDB's
SECONDARY_PREFERREDread preference:find()query viaFindOptionsfindByMessageId()andfindOlderThan()Files Modified (13 total)
Modified (10):
apps/meteor/server/settings/message.ts- Settings definitions including batch sizeapps/meteor/ee/server/cron/readReceiptsArchive.ts- Use sleep utility, object-based loggingapps/meteor/ee/server/cron/readReceiptsArchive.spec.ts- Updated tests with lint fixesapps/meteor/ee/server/startup/readReceiptsArchive.ts- Watcher integrationapps/meteor/jest.config.ts- Test patternapps/meteor/ee/server/models/raw/ReadReceiptsArchive.ts- Read preference passed to queriespackages/models/src/index.ts- Export readSecondaryPreferred utilitypackages/i18n/src/locales/en.i18n.json- English translationspackages/i18n/src/locales/pt.i18n.json- Portuguese translationspackages/i18n/src/locales/pt-BR.i18n.json- Brazilian Portuguese translationsCreated (3):
Statistics