Remove support for "legacy" MSC3898 group calling in MatrixRTCSession and CallMembership #4583

toger5 · 2024-12-12T14:58:24Z

MSC3898 has been superseded by MSC4143.

This PR removes support for MSC3898 and allows us to cleanup this area of the codebase significantly. We want to do this ahead of some further refactoring to make it more maintainable.

Breaking changes:

Call membership that are not in MSC4143 format will be ignored
The unused ExperimentalGroupCallRoomMemberState type is removed
The deprecated CallMembership.getLocalExpiry() function is removed

Background:

It is time to get rid of the legacy call format. (Legacy = one call event for all memberships per user. Now we have one event for each. So: Legacy: state_key = userId, non-legacy/session events: state_key=_userid_deviceId The transition has been made and deployed everywhere. No deployed client is using this system without being compatible with the new system.

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
Sign-off given on the changes (see CONTRIBUTING.md).

We actually had a bit of tests just for legacy and not for session events. All those tests got ported over so we do not remove any tests.

dbkr

This is quite a sizeable PR and not a very obvious change either. I might need some help understanding what's going on here.

CHANGELOG.md

spec/unit/matrixrtc/CallMembership.spec.ts

src/matrixrtc/MatrixRTCSession.ts

hughns

I'm finding it hard to reason about changes to the tests because the legacy behaviour is removed vs cleaning up the tests.

@toger5 would it be a lot of work to split this into two PRs? one with just the legacy removals and the other refactoring the test cases?

toger5 · 2024-12-16T19:18:16Z

@toger5 would it be a lot of work to split this into two PRs? one with just the legacy removals and the other refactoring the test cases?

it is fine to move it into a seperate PR: #4587

It will be very tedious to maintain the two branches. So as soon as we notice this needs more changes I propose just closing this PR and using it to make reviewing easier and merge the other (4587) branch.

toger5 · 2024-12-17T10:29:45Z

@dbkr We made this an "almost" exclusively remove code PR.
This mostly adds docstrings.
So this is ready for another round of reviews.

hughns

From a VoIP team point of view this looks great. 👍

I defer to web team folk for final review.

src/matrixrtc/MatrixRTCSession.ts

Co-authored-by: Hugh Nimmo-Smith <[email protected]>

dbkr

Looks generally sensible now, thanks for slimming this down.

spec/unit/matrixrtc/MatrixRTCSession.spec.ts

src/matrixrtc/CallMembership.ts

dbkr · 2025-01-06T11:34:54Z

src/matrixrtc/CallMembership.ts

+        // Assume that local clock is sufficiently in sync with other clocks in the distributed system.
+        // We used to try and adjust for the local clock being skewed, but there are cases where this is not accurate.
+        // The current implementation allows for the local clock to be -infinity to +MatrixRTCSession.MEMBERSHIP_EXPIRY_TIME/2
+        return this.getAbsoluteExpiry() - Date.now();


It looks like the impl of this is taking the legacy bit from above? I would expect it to be the other way around, although that bit just returns 'undefined'...

Two things have got conflated in this PR:

Removing the legacy logic

Implementing an expiry logic for the non-legacy code

I've removed the latter from this PR to make the review easier. It should go in another PR. What I have done is left some TODOs and commented out code in to help with this. If you would prefer that they are removed outright then we can do so.

Gotcha, thanks. I think it was simple enough this would have been okay, it just wasn't obvious what was going on from reading through. Thanks though.

…gacy

toger5 requested a review from a team as a code owner December 12, 2024 14:58

toger5 requested a review from hughns December 12, 2024 14:58

toger5 added the T-Deprecation A pull request that makes something deprecated label Dec 12, 2024

toger5 changed the title ~~remove all legacy call related code and adjust tests.~~ Remove all legacy call related code and adjust tests. Dec 12, 2024

toger5 marked this pull request as draft December 12, 2024 16:53

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch 2 times, most recently from 3c04d2b to bcef1d3 Compare December 12, 2024 16:55

github-actions bot deployed to PR Documentation Preview December 12, 2024 16:56 View deployment

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch 2 times, most recently from 042862b to 4418747 Compare December 12, 2024 18:38

github-actions bot deployed to PR Documentation Preview December 12, 2024 18:39 View deployment

github-actions bot deployed to PR Documentation Preview December 12, 2024 18:45 View deployment

github-actions bot deployed to PR Documentation Preview December 12, 2024 18:49 View deployment

remove all legacy call related code and adjust tests.

05915fb

We actually had a bit of tests just for legacy and not for session events. All those tests got ported over so we do not remove any tests.

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch from b900f7a to c0e5909 Compare December 12, 2024 18:53

github-actions bot deployed to PR Documentation Preview December 12, 2024 18:54 View deployment

toger5 marked this pull request as ready for review December 12, 2024 18:57

toger5 requested a review from a team as a code owner December 12, 2024 18:57

toger5 requested review from dbkr and MidhunSureshR December 12, 2024 18:57

dbkr reviewed Dec 13, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

spec/unit/matrixrtc/CallMembership.spec.ts Outdated Show resolved Hide resolved

src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved

hughns requested changes Dec 16, 2024

View reviewed changes

hughns changed the title ~~Remove all legacy call related code and adjust tests.~~ Remove legacy MatrixRTC related code and adjust tests Dec 16, 2024

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch from c0e5909 to 05915fb Compare December 16, 2024 16:09

github-actions bot deployed to PR Documentation Preview December 16, 2024 16:11 View deployment

toger5 changed the title ~~Remove legacy MatrixRTC related code and adjust tests~~ Remove legacy MatrixRTC related code and remove realted tests Dec 16, 2024

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch from dd8726e to 795a3cf Compare December 16, 2024 19:04

dont adjust tests but remove legacy tests

e8588e8

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch from 795a3cf to e8588e8 Compare December 16, 2024 19:21

hughns added 4 commits December 17, 2024 10:39

Remove deprecated CallMembership.getLocalExpiry()

7e88898

Remove references to legacy in test case names

b249fd7

Clean up SessionMembershipData tsdoc

4fbc3af

Remove CallMembership.expires

bb8560b

github-actions bot deployed to PR Documentation Preview December 17, 2024 10:14 View deployment

toger5 added 3 commits December 17, 2024 11:22

Use correct expire duration.

a7f1efd

make expiration methods not return optional values and update docstring

cff50e3

add docs to SessionMembershipData

2cd6f96

toger5 force-pushed the toger5/refactor-matrixrtcsession-part1-legacy branch from d4e1f51 to 2cd6f96 Compare December 17, 2024 10:22

github-actions bot deployed to PR Documentation Preview December 17, 2024 10:23 View deployment

toger5 requested review from dbkr and hughns December 17, 2024 10:27

hughns approved these changes Dec 17, 2024

View reviewed changes

src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved

Use MSC4143 (instaed of non-legacy) wording in comment

9da691c

Co-authored-by: Hugh Nimmo-Smith <[email protected]>

github-actions bot deployed to PR Documentation Preview December 17, 2024 11:31 View deployment

hughns mentioned this pull request Jan 6, 2025

MatrixRTC: Implement expiry logic for CallMembership and additional test coverage #4587

Merged

4 tasks

dbkr reviewed Jan 6, 2025

View reviewed changes

Incorporate feedback from review

456c9a6

github-actions bot deployed to PR Documentation Preview January 6, 2025 16:32 View deployment

Fix test name

8656da0

github-actions bot deployed to PR Documentation Preview January 6, 2025 16:36 View deployment

Merge branch 'develop' into toger5/refactor-matrixrtcsession-part1-le…

d3770e3

…gacy

github-actions bot deployed to PR Documentation Preview January 6, 2025 16:42 View deployment

hughns requested a review from dbkr January 6, 2025 16:42

dbkr approved these changes Jan 6, 2025

View reviewed changes

hughns added this pull request to the merge queue Jan 6, 2025

Merged via the queue into develop with commit ffd3c95 Jan 6, 2025
30 checks passed

hughns deleted the toger5/refactor-matrixrtcsession-part1-legacy branch January 6, 2025 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove support for "legacy" MSC3898 group calling in MatrixRTCSession and CallMembership #4583

Remove support for "legacy" MSC3898 group calling in MatrixRTCSession and CallMembership #4583

toger5 commented Dec 12, 2024 •

edited by hughns

Loading

dbkr left a comment

hughns left a comment

toger5 commented Dec 16, 2024 •

edited

Loading

toger5 commented Dec 17, 2024

hughns left a comment

dbkr left a comment

dbkr Jan 6, 2025

hughns Jan 6, 2025 •

edited

Loading

dbkr Jan 6, 2025

Remove support for "legacy" MSC3898 group calling in MatrixRTCSession and CallMembership #4583

Remove support for "legacy" MSC3898 group calling in MatrixRTCSession and CallMembership #4583

Conversation

toger5 commented Dec 12, 2024 • edited by hughns Loading

Checklist

dbkr left a comment

Choose a reason for hiding this comment

hughns left a comment

Choose a reason for hiding this comment

toger5 commented Dec 16, 2024 • edited Loading

toger5 commented Dec 17, 2024

hughns left a comment

Choose a reason for hiding this comment

dbkr left a comment

Choose a reason for hiding this comment

dbkr Jan 6, 2025

Choose a reason for hiding this comment

hughns Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

dbkr Jan 6, 2025

Choose a reason for hiding this comment

toger5 commented Dec 12, 2024 •

edited by hughns

Loading

toger5 commented Dec 16, 2024 •

edited

Loading

hughns Jan 6, 2025 •

edited

Loading