Skip to content

Conversation

@sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Nov 6, 2025

relates to https://rocketchat.atlassian.net/browse/FDR-255

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling during federation setup to prevent startup failures and ensure system stability when initialization encounters issues.

@changeset-bot
Copy link

changeset-bot bot commented Nov 6, 2025

⚠️ No Changeset found

Latest commit: b08dae9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 6, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 6, 2025

Walkthrough

The change adds error handling to the federation matrix setup process by wrapping the setupFederationMatrix() call in a try-catch block. Instead of allowing potential failures to propagate, errors are now caught and logged, ensuring the startup process can gracefully handle initialization failures.

Changes

Cohort / File(s) Summary
Error handling for federation setup
apps/meteor/ee/server/startup/federation.ts
Wraps setupFederationMatrix() invocation in try-catch block to capture and log errors during initialization, converting an unconditional await into a guarded, error-handled call.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Straightforward error handling addition with no logic complexity
  • Single file affected with a localized change pattern
  • No API or behavioral changes to the function itself

Suggested labels

stat: ready to merge

Suggested reviewers

  • ggazzo

Poem

🐰 A federation takes flight,
With errors caught, held tight,
No crash, no fright—
Just graceful logs in the night,
All systems running right! 🌙

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: wrapping setupFederationMatrix() in error handling to fix mongo initialization issues in federation-matrix startup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-federation-mongo-init

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7fd62c1 and 10e0d4b.

📒 Files selected for processing (1)
  • apps/meteor/ee/server/startup/federation.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
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.
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.

Applied to files:

  • apps/meteor/ee/server/startup/federation.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.

Applied to files:

  • apps/meteor/ee/server/startup/federation.ts
📚 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/ee/server/startup/federation.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/ee/server/startup/federation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts

Applied to files:

  • apps/meteor/ee/server/startup/federation.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.

Applied to files:

  • apps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/federation.ts (1)
ee/packages/federation-matrix/src/setup.ts (1)
  • setupFederationMatrix (103-120)
⏰ 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: 🔨 Test UI (EE) / MongoDB 5.0 (4/5)
  • GitHub Check: 🔨 Test UI (EE) / MongoDB 5.0 (3/5)
  • GitHub Check: 🔨 Test API (CE) / MongoDB 5.0 (1/1)
🔇 Additional comments (1)
apps/meteor/ee/server/startup/federation.ts (1)

74-78: Based on my verification using the retrieved learnings and script output, I can now provide a rewritten review comment.

The key finding is that the SDK's init() establishes infrastructure first, and configuration can be applied later via settings watchers before events are processed, with config only mattering when events occur. This indicates the system is intentionally designed to be resilient to setup timing and order.

The original review comment's concern about "state inconsistency" overstates the issue. The script output confirms that serviceEnabled is already checked at line 46 before any federation operations are attempted via FederationMatrixService.notifyUserTyping(). The try-catch wrapper is appropriate defensive coding that prevents startup crashes and logs failures for visibility.

The try-catch wrapper is good defensive coding and appropriately prevents startup failures.

The error handling added here follows the established pattern for the federation startup flow. Since serviceEnabled is checked before federation operations are attempted (line 46), operations won't proceed if the service hasn't been properly configured. The try-catch logs errors for operator visibility while allowing the service to continue running with federation gracefully disabled, which aligns with the system's design intent.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.14%. Comparing base (7fd62c1) to head (10e0d4b).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37427      +/-   ##
===========================================
+ Coverage    68.13%   68.14%   +0.01%     
===========================================
  Files         3364     3364              
  Lines       115811   115811              
  Branches     20903    20909       +6     
===========================================
+ Hits         78911    78924      +13     
+ Misses       34216    34198      -18     
- Partials      2684     2689       +5     
Flag Coverage Δ
e2e 57.49% <ø> (-0.02%) ⬇️
unit 72.16% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rodrigok rodrigok marked this pull request as ready for review November 6, 2025 22:14
@rodrigok rodrigok added the stat: QA assured Means it has been tested and approved by a company insider label Nov 6, 2025
@sampaiodiego sampaiodiego added this to the 7.13.0 milestone Nov 7, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 7, 2025
@sampaiodiego sampaiodiego merged commit f03bf2c into develop Nov 7, 2025
1 check passed
@sampaiodiego sampaiodiego deleted the fix-federation-mongo-init branch November 7, 2025 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants