MatrixRTC: Rename MembershipConfig parameters (#4714)

* Remove redundant sendDelayedEventAction
We do already have the state `hasMemberEvent` that allows to distinguish the two cases. No need to create two dedicated actions.

* fix missing return

* Make membership manager an event emitter to inform about status updates.
 - deprecate isJoined (replaced by isActivated)
 - move Interface types to types.ts

* add tests for status updates.

* lint

* test "reschedules delayed leave event" in case the delayed event gets canceled

* review

* fix types

* prettier

* fix legacy membership manager

* remove deprecated jitter.

* use non deprecated config fields (keep deprecated fields as fallback)

* update tests to test non deprecated names

* make local NewMembershipManager variable names consistent with config

* make LegacyMembershipManger local variables consistent with config

* comments and rename `networkErrorLocalRetryMs` -> `networkErrorRetryMs`

* review

Author: toger5

spec/unit/matrixrtc/MatrixRTCSession.spec.ts

@@ -402,10 +402,10 @@ describe("MatrixRTCSession", () => {
             jest.useRealTimers();
         });
 
-        it("uses membershipExpiryTimeout from join config", async () => {
+        it("uses membershipEventExpiryMs from join config", async () => {
             const realSetTimeout = setTimeout;
             jest.useFakeTimers();
-            sess!.joinRoomSession([mockFocus], mockFocus, { membershipExpiryTimeout: 60000 });
+            sess!.joinRoomSession([mockFocus], mockFocus, { membershipEventExpiryMs: 60000 });
             await Promise.race([sentStateEvent, new Promise((resolve) => realSetTimeout(resolve, 500))]);
             expect(client.sendStateEvent).toHaveBeenCalledWith(
                 mockRoom!.roomId,

spec/unit/matrixrtc/MembershipManager.spec.ts

@@ -214,7 +214,7 @@ describe.each([
                     });
                     const manager = new TestMembershipManager(
                         {
-                            membershipServerSideExpiryTimeout: 9000,
+                            delayedLeaveEventDelayMs: 9000,
                         },
                         room,
                         client,
@@ -293,9 +293,9 @@ describe.each([
                 await jest.advanceTimersByTimeAsync(5000);
                 expect(client._unstable_sendDelayedStateEvent).toHaveBeenCalledTimes(2);
             });
-            it("uses membershipServerSideExpiryTimeout from config", () => {
+            it("uses delayedLeaveEventDelayMs from config", () => {
                 const manager = new TestMembershipManager(
-                    { membershipServerSideExpiryTimeout: 123456 },
+                    { delayedLeaveEventDelayMs: 123456 },
                     room,
                     client,
                     () => undefined,
@@ -311,9 +311,9 @@ describe.each([
             });
         });
 
-        it("uses membershipExpiryTimeout from config", async () => {
+        it("uses membershipEventExpiryMs from config", async () => {
             const manager = new TestMembershipManager(
-                { membershipExpiryTimeout: 1234567 },
+                { membershipEventExpiryMs: 1234567 },
                 room,
                 client,
                 () => undefined,
@@ -479,9 +479,9 @@ describe.each([
 
     // TODO: Not sure about this name
     describe("background timers", () => {
-        it("sends only one keep-alive for delayed leave event per `membershipKeepAlivePeriod`", async () => {
+        it("sends only one keep-alive for delayed leave event per `delayedLeaveEventRestartMs`", async () => {
             const manager = new TestMembershipManager(
-                { membershipKeepAlivePeriod: 10_000, membershipServerSideExpiryTimeout: 30_000 },
+                { delayedLeaveEventRestartMs: 10_000, delayedLeaveEventDelayMs: 30_000 },
                 room,
                 client,
                 () => undefined,
@@ -512,7 +512,7 @@ describe.each([
         // TODO: Add git commit when we removed it.
         async function testExpires(expire: number, headroom?: number) {
             const manager = new TestMembershipManager(
-                { membershipExpiryTimeout: expire, membershipExpiryTimeoutHeadroom: headroom },
+                { membershipEventExpiryMs: expire, membershipEventExpiryHeadroomMs: headroom },
                 room,
                 client,
                 () => undefined,
@@ -733,7 +733,7 @@ describe.each([
             const unrecoverableError = jest.fn();
             (client._unstable_sendDelayedStateEvent as Mock<any>).mockRejectedValue(new HTTPError("unknown", 501));
             const manager = new TestMembershipManager(
-                { callMemberEventRetryDelayMinimum: 1000, maximumNetworkErrorRetryCount: 7 },
+                { networkErrorRetryMs: 1000, maximumNetworkErrorRetryCount: 7 },
                 room,
                 client,
                 () => undefined,

src/matrixrtc/LegacyMembershipManager.ts

@@ -64,30 +64,32 @@ export class LegacyMembershipManager implements IMembershipManager {
     private updateCallMembershipRunning = false;
     private needCallMembershipUpdate = false;
     /**
-     * If the server disallows the configured {@link membershipServerSideExpiryTimeout},
+     * If the server disallows the configured {@link delayedLeaveEventDelayMs},
      * this stores a delay that the server does allow.
      */
-    private membershipServerSideExpiryTimeoutOverride?: number;
+    private delayedLeaveEventDelayMsOverride?: number;
     private disconnectDelayId: string | undefined;
 
-    private get callMemberEventRetryDelayMinimum(): number {
-        return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
+    private get networkErrorRetryMs(): number {
+        return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
     }
-    private get membershipExpiryTimeout(): number {
-        return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION;
+    private get membershipEventExpiryMs(): number {
+        return (
+            this.joinConfig?.membershipEventExpiryMs ??
+            this.joinConfig?.membershipExpiryTimeout ??
+            DEFAULT_EXPIRE_DURATION
+        );
     }
-    private get membershipServerSideExpiryTimeout(): number {
+    private get delayedLeaveEventDelayMs(): number {
         return (
-            this.membershipServerSideExpiryTimeoutOverride ??
+            this.delayedLeaveEventDelayMsOverride ??
+            this.joinConfig?.delayedLeaveEventDelayMs ??
             this.joinConfig?.membershipServerSideExpiryTimeout ??
             8_000
         );
     }
-    private get membershipKeepAlivePeriod(): number {
-        return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
-    }
-    private get callMemberEventRetryJitter(): number {
-        return this.joinConfig?.callMemberEventRetryJitter ?? 2_000;
+    private get delayedLeaveEventRestartMs(): number {
+        return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
     }
 
     public constructor(
@@ -137,7 +139,7 @@ export class LegacyMembershipManager implements IMembershipManager {
     public join(fociPreferred: Focus[], fociActive?: Focus): void {
         this.ownFocusActive = fociActive;
         this.ownFociPreferred = fociPreferred;
-        this.relativeExpiry = this.membershipExpiryTimeout;
+        this.relativeExpiry = this.membershipEventExpiryMs;
         // We don't wait for this, mostly because it may fail and schedule a retry, so this
         // function returning doesn't really mean anything at all.
         void this.triggerCallMembershipEventUpdate();
@@ -261,7 +263,7 @@ export class LegacyMembershipManager implements IMembershipManager {
                             this.client._unstable_sendDelayedStateEvent(
                                 this.room.roomId,
                                 {
-                                    delay: this.membershipServerSideExpiryTimeout,
+                                    delay: this.delayedLeaveEventDelayMs,
                                 },
                                 EventType.GroupCallMemberPrefix,
                                 {}, // leave event
@@ -278,9 +280,9 @@ export class LegacyMembershipManager implements IMembershipManager {
                             const maxDelayAllowed = e.data["org.matrix.msc4140.max_delay"];
                             if (
                                 typeof maxDelayAllowed === "number" &&
-                                this.membershipServerSideExpiryTimeout > maxDelayAllowed
+                                this.delayedLeaveEventDelayMs > maxDelayAllowed
                             ) {
-                                this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed;
+                                this.delayedLeaveEventDelayMsOverride = maxDelayAllowed;
                                 return prepareDelayedDisconnection();
                             }
                         }
@@ -350,15 +352,15 @@ export class LegacyMembershipManager implements IMembershipManager {
             }
             logger.info("Sent updated call member event.");
         } catch (e) {
-            const resendDelay = this.callMemberEventRetryDelayMinimum + Math.random() * this.callMemberEventRetryJitter;
+            const resendDelay = this.networkErrorRetryMs;
             logger.warn(`Failed to send call member event (retrying in ${resendDelay}): ${e}`);
             await sleep(resendDelay);
             await this.triggerCallMembershipEventUpdate();
         }
     }
 
     private scheduleDelayDisconnection(): void {
-        this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.membershipKeepAlivePeriod);
+        this.memberEventTimeout = setTimeout(() => void this.delayDisconnection(), this.delayedLeaveEventRestartMs);
     }
 
     private readonly delayDisconnection = async (): Promise<void> => {

src/matrixrtc/MatrixRTCSession.ts

@@ -65,7 +65,13 @@ export type MatrixRTCSessionEventHandlerMap = {
     ) => void;
     [MatrixRTCSessionEvent.MembershipManagerError]: (error: unknown) => void;
 };
-
+// The names follow these principles:
+// - we use the technical term delay if the option is related to delayed events.
+// - we use delayedLeaveEvent if the option is related to the delayed leave event.
+// - we use membershipEvent if the option is related to the rtc member state event.
+// - we use the technical term expiry if the option is related to the expiry field of the membership state event.
+// - we use a `MS` postfix if the option is a duration to avoid using words like:
+//   `time`, `duration`, `delay`, `timeout`... that might be mistaken/confused with technical terms.
 export interface MembershipConfig {
     /**
      * Use the new Manager.
@@ -80,6 +86,8 @@ export interface MembershipConfig {
      *
      * This is what goes into the m.rtc.member event expiry field and is typically set to a number of hours.
      */
+    membershipEventExpiryMs?: number;
+    /** @deprecated renamed to `membershipEventExpiryMs`*/
     membershipExpiryTimeout?: number;
 
     /**
@@ -91,36 +99,25 @@ export interface MembershipConfig {
      *
      * This value does not have an effect on the value of `SessionMembershipData.expires`.
      */
+    membershipEventExpiryHeadroomMs?: number;
+    /** @deprecated  renamed to `membershipEventExpiryHeadroomMs`*/
     membershipExpiryTimeoutHeadroom?: number;
 
-    /**
-     * The period (in milliseconds) with which we check that our membership event still exists on the
-     * server. If it is not found we create it again.
-     */
-    memberEventCheckPeriod?: number;
-
-    /**
-     * The minimum delay (in milliseconds) after which we will retry sending the membership event if it
-     * failed to send.
-     */
-    callMemberEventRetryDelayMinimum?: number;
-
     /**
      * The timeout (in milliseconds) with which the deleayed leave event on the server is configured.
      * After this time the server will set the event to the disconnected stat if it has not received a keep-alive from the client.
      */
+    delayedLeaveEventDelayMs?: number;
+    /** @deprecated renamed to `delayedLeaveEventDelayMs`*/
     membershipServerSideExpiryTimeout?: number;
 
     /**
      * The interval (in milliseconds) in which the client will send membership keep-alives to the server.
      */
+    delayedLeaveEventRestartMs?: number;
+    /** @deprecated renamed to `delayedLeaveEventRestartMs`*/
     membershipKeepAlivePeriod?: number;
 
-    /**
-     * @deprecated It should be possible to make it stable without this.
-     */
-    callMemberEventRetryJitter?: number;
-
     /**
      * The maximum number of retries that the manager will do for delayed event sending/updating and state event sending when a server rate limit has been hit.
      */
@@ -131,6 +128,14 @@ export interface MembershipConfig {
      */
     maximumNetworkErrorRetryCount?: number;
 
+    /**
+     * The time (in milliseconds) after which we will retry a http request if it
+     * failed to send due to a network error. (send membership event, send delayed event, restart delayed event...)
+     */
+    networkErrorRetryMs?: number;
+    /** @deprecated renamed to `networkErrorRetryMs`*/
+    callMemberEventRetryDelayMinimum?: number;
+
     /**
      * If true, use the new to-device transport for sending encryption keys.
      */

src/matrixrtc/NewMembershipManager.ts

@@ -333,33 +333,38 @@ export class MembershipManager
     private focusActive?: Focus;
 
     // Config:
-    private membershipServerSideExpiryTimeoutOverride?: number;
+    private delayedLeaveEventDelayMsOverride?: number;
 
-    private get callMemberEventRetryDelayMinimum(): number {
-        return this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
+    private get networkErrorRetryMs(): number {
+        return this.joinConfig?.networkErrorRetryMs ?? this.joinConfig?.callMemberEventRetryDelayMinimum ?? 3_000;
     }
-    private get membershipEventExpiryTimeout(): number {
-        return this.joinConfig?.membershipExpiryTimeout ?? DEFAULT_EXPIRE_DURATION;
-    }
-    private get membershipEventExpiryTimeoutHeadroom(): number {
-        return this.joinConfig?.membershipExpiryTimeoutHeadroom ?? 5_000;
+    private get membershipEventExpiryMs(): number {
+        return (
+            this.joinConfig?.membershipEventExpiryMs ??
+            this.joinConfig?.membershipExpiryTimeout ??
+            DEFAULT_EXPIRE_DURATION
+        );
     }
-    private computeNextExpiryActionTs(iteration: number): number {
+    private get membershipEventExpiryHeadroomMs(): number {
         return (
-            this.state.startTime +
-            this.membershipEventExpiryTimeout * iteration -
-            this.membershipEventExpiryTimeoutHeadroom
+            this.joinConfig?.membershipEventExpiryHeadroomMs ??
+            this.joinConfig?.membershipExpiryTimeoutHeadroom ??
+            5_000
         );
     }
-    private get membershipServerSideExpiryTimeout(): number {
+    private computeNextExpiryActionTs(iteration: number): number {
+        return this.state.startTime + this.membershipEventExpiryMs * iteration - this.membershipEventExpiryHeadroomMs;
+    }
+    private get delayedLeaveEventDelayMs(): number {
         return (
-            this.membershipServerSideExpiryTimeoutOverride ??
+            this.delayedLeaveEventDelayMsOverride ??
+            this.joinConfig?.delayedLeaveEventDelayMs ??
             this.joinConfig?.membershipServerSideExpiryTimeout ??
             8_000
         );
     }
-    private get membershipKeepAlivePeriod(): number {
-        return this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
+    private get delayedLeaveEventRestartMs(): number {
+        return this.joinConfig?.delayedLeaveEventRestartMs ?? this.joinConfig?.membershipKeepAlivePeriod ?? 5_000;
     }
     private get maximumRateLimitRetryCount(): number {
         return this.joinConfig?.maximumRateLimitRetryCount ?? 10;
@@ -432,7 +437,7 @@ export class MembershipManager
             ._unstable_sendDelayedStateEvent(
                 this.room.roomId,
                 {
-                    delay: this.membershipServerSideExpiryTimeout,
+                    delay: this.delayedLeaveEventDelayMs,
                 },
                 EventType.GroupCallMemberPrefix,
                 {}, // leave event
@@ -447,7 +452,7 @@ export class MembershipManager
                     // due to lack of https://github.com/element-hq/synapse/pull/17810
                     return createInsertActionUpdate(
                         MembershipActionType.RestartDelayedEvent,
-                        this.membershipKeepAlivePeriod,
+                        this.delayedLeaveEventRestartMs,
                     );
                 } else {
                     // This action was scheduled because we are in the process of joining
@@ -525,7 +530,7 @@ export class MembershipManager
                 this.resetRateLimitCounter(MembershipActionType.RestartDelayedEvent);
                 return createInsertActionUpdate(
                     MembershipActionType.RestartDelayedEvent,
-                    this.membershipKeepAlivePeriod,
+                    this.delayedLeaveEventRestartMs,
                 );
             })
             .catch((e) => {
@@ -579,7 +584,7 @@ export class MembershipManager
             .sendStateEvent(
                 this.room.roomId,
                 EventType.GroupCallMemberPrefix,
-                this.makeMyMembership(this.membershipEventExpiryTimeout),
+                this.makeMyMembership(this.membershipEventExpiryMs),
                 this.stateKey,
             )
             .then(() => {
@@ -611,7 +616,7 @@ export class MembershipManager
             .sendStateEvent(
                 this.room.roomId,
                 EventType.GroupCallMemberPrefix,
-                this.makeMyMembership(this.membershipEventExpiryTimeout * nextExpireUpdateIteration),
+                this.makeMyMembership(this.membershipEventExpiryMs * nextExpireUpdateIteration),
                 this.stateKey,
             )
             .then(() => {
@@ -697,8 +702,8 @@ export class MembershipManager
             error.data["org.matrix.msc4140.errcode"] === "M_MAX_DELAY_EXCEEDED"
         ) {
             const maxDelayAllowed = error.data["org.matrix.msc4140.max_delay"];
-            if (typeof maxDelayAllowed === "number" && this.membershipServerSideExpiryTimeout > maxDelayAllowed) {
-                this.membershipServerSideExpiryTimeoutOverride = maxDelayAllowed;
+            if (typeof maxDelayAllowed === "number" && this.delayedLeaveEventDelayMs > maxDelayAllowed) {
+                this.delayedLeaveEventDelayMsOverride = maxDelayAllowed;
             }
             this.logger.warn("Retry sending delayed disconnection event due to server timeout limitations:", error);
             return true;
@@ -769,7 +774,7 @@ export class MembershipManager
     private actionUpdateFromNetworkErrorRetry(error: unknown, type: MembershipActionType): ActionUpdate | undefined {
         // "Is a network error"-boundary
         const retries = this.state.networkErrorRetries.get(type) ?? 0;
-        const retryDurationString = this.callMemberEventRetryDelayMinimum / 1000 + "s";
+        const retryDurationString = this.networkErrorRetryMs / 1000 + "s";
         const retryCounterString = "(" + retries + "/" + this.maximumNetworkErrorRetryCount + ")";
         if (error instanceof Error && error.name === "AbortError") {
             this.logger.warn(
@@ -819,7 +824,7 @@ export class MembershipManager
         // retry boundary
         if (retries < this.maximumNetworkErrorRetryCount) {
             this.state.networkErrorRetries.set(type, retries + 1);
-            return createInsertActionUpdate(type, this.callMemberEventRetryDelayMinimum);
+            return createInsertActionUpdate(type, this.networkErrorRetryMs);
         }
 
         // Failure