fix: video calls failing when forced E2EE meets persistent chat - #41960
fix: video calls failing when forced E2EE meets persistent chat#41960milton-rucks wants to merge 2 commits into
Conversation
Starting a video call failed when both "Force end-to-end encryption on private rooms" and video conference persistent chat were enabled. Persistent chat creates its discussion with `encrypted: false`, so the forced-encryption policy rejected the room creation with `error-encrypted-private-rooms-enforced`. Because `maybeCreateDiscussion` runs before the call URL, start message and notifications — and was not guarded — the throw aborted the whole call and left a conference record with no URL, no start message and no notifications behind. Direct calls stayed in CALLING (the 40s auto-cancel timer is registered after the discussion step, so it never ran) and group calls stayed in STARTED. Persistent chat is complementary to the call, so failing to create its discussion now logs and is skipped instead of aborting, matching what `addUserToDiscussion` already does for the subscription step. The call starts normally, just without a persistent chat discussion. Also adds a warning to the "Force end-to-end encryption on private rooms" setting stating it is not compatible with video conference persistent chat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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: a1ac3c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41960 +/- ##
===========================================
- Coverage 69.34% 69.31% -0.03%
===========================================
Files 4254 4254
Lines 168649 168650 +1
Branches 30022 30042 +20
===========================================
- Hits 116947 116904 -43
- Misses 46532 46566 +34
- Partials 5170 5180 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Only skip the persistent chat discussion when it is rejected by the forced-encryption policy, which is a known and permanent incompatibility. Any other failure (invalid room, circular prid, an app preventing the room creation, database errors) is unexpected and keeps aborting the call as it did before, instead of silently dropping persistent chat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Proposed changes (including videos or screenshots)
Fixes a regression introduced by #41095: starting a video call from a direct message or private room fails when Force end-to-end encryption on private rooms and video conference persistent chat are both enabled.
Root cause
Persistent chat creates its discussion with
encrypted: falsehardcoded (service.tscreateDiscussionForConference), andgetDiscussionTypereturns'p'for a private/DM parent. The forced-encryption policy added in #41095 therefore rejects the room creation witherror-encrypted-private-rooms-enforced.The damage comes from where that throw happens. In both
startDirectandstartGroup,maybeCreateDiscussionruns before the rest of the start sequence and was not guarded, so the throw aborted everything after it — while theVideoConferencerecord had already been inserted:VideoConferenceModel.createDirect/createGroupmaybeCreateDiscussionsetUrlByIdcreateMessage/setMessageByIdCALLINGsendPushNotification/ringbroadcastThat matches the report exactly, including direct calls stuck in
CALLINGand group calls inSTARTED. The error is then swallowed by the endpoint'scatch, which returnsdiagnoseProvider(...)— and since the provider is healthy that resolves toundefined, so the client only shows the genericerror-videoconf-unexpected.The fix
The persistent chat discussion is skipped when — and only when — it is rejected by the forced-encryption policy (
error-encrypted-private-rooms-enforced, matched via the existingisMeteorErrorhelper). That is a known, permanent incompatibility, so it is treated as a designed skip and logged atwarn.Every other failure (
invalid-room, circularpridreference,invalid-user,error-app-prevented, database errors) is rethrown and keeps the pre-existing behavior. Those are bugs rather than expected states, so they must not silently drop persistent chat — which admins may be relying on for record keeping.The call now starts normally — URL, start message, notifications and the auto-cancel timer all run — just without a persistent chat discussion.
Encryption behavior itself is unchanged: the two features remain incompatible by design, since persistent chat history is meant to be readable. That is now surfaced to admins as a warning on the setting:
Rendered through the standard
alert→Callout type='warning'path (<b>/<br/>only, matching the whitelist thatSetting.tsxpasses to<Trans>).Files
apps/meteor/server/services/video-conference/service.ts— skip the discussion on the encryption-policy rejection, rethrow anything elseapps/meteor/server/settings/e2e.ts—alertonE2E_Force_Encryption_For_Private_Roomspackages/i18n/src/locales/en.i18n.json—Force_Encryption_For_Private_Rooms_Alertapps/meteor/tests/end-to-end/apps/video-conferences.ts— regression testIssue(s)
Regression from #41095.
Steps to test or reproduce
E2E_Enable,E2E_Force_Encryption_For_Private_Rooms,Discussion_enabledandVideoConf_Enable_Persistent_Chat.Before: call creation fails, server throws
error-encrypted-private-rooms-enforced, client showserror-videoconf-unexpected, and an unusable conference record is left behind (no URL, no discussion, no start message, no notifications; direct calls stuck inCALLING, private-room calls inSTARTED).After: the call starts normally and no persistent chat discussion is created. The skip is logged server-side at
warnasSkipped the persistent chat discussion of a conference because encryption is enforced on private rooms.Also check Admin → Settings → End-to-End Encryption: the Force end-to-end encryption on private rooms setting now shows the incompatibility warning.
Further comments
The guard is placed inside
maybeCreateDiscussionrather than at the two call sites, so the "maybe" contract holds for every caller and there is a single place to reason about. Theinvalid-video-conferencethrow earlier in that method is deliberately left fatal — it means the just-created conference record is missing, which is a genuine internal inconsistency worth surfacing rather than hiding.The guard was deliberately kept narrow. A blanket
catcharound the discussion creation would also fix the orphan-record problem for any failure and would match whataddUserToDiscussiondoes one level down, but it would widen a regression fix into a behavior change and could silently stop persistent chat from being created when an unrelated bug appears. The orphan-record issue is pre-existing (it predates #41095, it is just far easier to hit now) and is better addressed on its own — the start sequence inserting the conference record before it can guarantee a usable call is the underlying problem.Worth noting separately (not changed here):
video-conference.startdiscards the real error in itscatchand returnsdiagnoseProvider(...), which is why this failure was invisible from the client. Making that endpoint surface the underlying error would help diagnose similar issues, but it is out of scope for this regression fix.Validated locally:
tsc --noEmitclean acrossapps/meteor(0 errors, with workspace deps built), ESLint clean on all changed files (remaining warnings inservice.tsare pre-existing, all above the changed region), and the i18n sort/validity check passes.🤖 Generated with Claude Code