Fix room state being updated with old (now overwritten) state and emitting for those updates. (#4242)

* Fix room state being updated with old (now overwritten) state and emitting for those updates.

* remove timestamp condition

Add configuration for toStartOfTimeline

* fix timeline tests

* only skip event adding if event_id and replaces_state is set.

* fix room tests

* test skipping insertion

* rename back to lastStateEvent

* store if a state is at the start of a timeline in the RoomState class

* make `isStartTimelineState` a `public readonly` and fix condition.

Author: toger5

spec/unit/room-state.spec.ts

@@ -1122,4 +1122,59 @@ describe("RoomState", function () {
             ).toBeFalsy();
         });
     });
+    describe("skipWrongOrderRoomStateInserts", () => {
+        const idNow = "now";
+        const idBefore = "before";
+        const endState = new RoomState(roomId);
+        const startState = new RoomState(roomId, undefined, true);
+
+        let onRoomStateEvent: (event: MatrixEvent, state: RoomState, lastStateEvent: MatrixEvent | null) => void;
+        const evNow = new MatrixEvent({
+            type: "m.call.member",
+            room_id: roomId,
+            state_key: "@user:example.org",
+            content: {},
+        });
+        evNow.event.unsigned = { replaces_state: idBefore };
+        evNow.event.event_id = idNow;
+
+        const evBefore = new MatrixEvent({
+            type: "m.call.member",
+            room_id: roomId,
+            state_key: "@user:example.org",
+            content: {},
+        });
+
+        const updatedStateWithBefore = jest.fn();
+        const updatedStateWithNow = jest.fn();
+
+        beforeEach(() => {
+            evBefore.event.event_id = idBefore;
+            onRoomStateEvent = (event, _state, _lastState) => {
+                if (event.event.event_id === idNow) {
+                    updatedStateWithNow();
+                } else if (event.event.event_id === idBefore) {
+                    updatedStateWithBefore();
+                }
+            };
+            endState.on(RoomStateEvent.Events, onRoomStateEvent);
+            startState.on(RoomStateEvent.Events, onRoomStateEvent);
+        });
+        afterEach(() => {
+            endState.off(RoomStateEvent.Events, onRoomStateEvent);
+            startState.off(RoomStateEvent.Events, onRoomStateEvent);
+            updatedStateWithNow.mockReset();
+            updatedStateWithBefore.mockReset();
+        });
+        it("should skip inserting state to the end of the timeline if the current endState events replaces_state id is the same as the inserted events id", () => {
+            endState.setStateEvents([evNow, evBefore]);
+            expect(updatedStateWithBefore).not.toHaveBeenCalled();
+            expect(updatedStateWithNow).toHaveBeenCalled();
+        });
+        it("should skip inserting state at the beginning of the timeline if the inserted events replaces_state is the event id of the current startState", () => {
+            startState.setStateEvents([evBefore, evNow]);
+            expect(updatedStateWithBefore).toHaveBeenCalled();
+            expect(updatedStateWithNow).not.toHaveBeenCalled();
+        });
+    });
 });

src/models/event-timeline.ts

@@ -127,7 +127,7 @@ export class EventTimeline {
     public constructor(private readonly eventTimelineSet: EventTimelineSet) {
         this.roomId = eventTimelineSet.room?.roomId ?? null;
         if (this.roomId) {
-            this.startState = new RoomState(this.roomId);
+            this.startState = new RoomState(this.roomId, undefined, true);
             this.endState = new RoomState(this.roomId);
         }
 
@@ -267,7 +267,7 @@ export class EventTimeline {
     /**
      * Get a pagination token
      *
-     * @param direction -   EventTimeline.BACKWARDS to get the pagination
+     * @param direction - EventTimeline.BACKWARDS to get the pagination
      *   token for going backwards in time; EventTimeline.FORWARDS to get the
      *   pagination token for going forwards in time.
      *

src/models/room-state.ts

@@ -194,10 +194,22 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
      * As the timeline might get reset while they are loading, this state needs to be inherited
      * and shared when the room state is cloned for the new timeline.
      * This should only be passed from clone.
+     * @param isStartTimelineState - Optional. This state is marked as a start state.
+     * This is used to skip state insertions that are
+     * in the wrong order. The order is determined by the `replaces_state` id.
+     *
+     * Example:
+     * A current state events `replaces_state` value is `1`.
+     * Trying to insert a state event with `event_id` `1` in its place would fail if isStartTimelineState = false.
+     *
+     * A current state events `event_id` is `2`.
+     * Trying to insert a state event where its `replaces_state` value is `2` would fail if isStartTimelineState = true.
      */
+
     public constructor(
         public readonly roomId: string,
         private oobMemberFlags = { status: OobStatus.NotStarted },
+        public readonly isStartTimelineState = false,
     ) {
         super();
         this.updateModifiedTime();
@@ -408,7 +420,7 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
      * Fires {@link RoomStateEvent.Events}
      * Fires {@link RoomStateEvent.Marker}
      */
-    public setStateEvents(stateEvents: MatrixEvent[], markerFoundOptions?: IMarkerFoundOptions): void {
+    public setStateEvents(stateEvents: MatrixEvent[], options?: IMarkerFoundOptions): void {
         this.updateModifiedTime();
 
         // update the core event dict
@@ -420,6 +432,22 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
             }
 
             const lastStateEvent = this.getStateEventMatching(event);
+
+            // Safety measure to not update the room (and emit the update) with older state.
+            // The sync loop really should not send old events but it does very regularly.
+            // Logging on return in those two conditions results in a large amount of logging. (on startup and when running element)
+            const lastReplaceId = lastStateEvent?.event.unsigned?.replaces_state;
+            const lastId = lastStateEvent?.event.event_id;
+            const newReplaceId = event.event.unsigned?.replaces_state;
+            const newId = event.event.event_id;
+            if (this.isStartTimelineState) {
+                // Add an event to the start of the timeline. Its replace id should not be the same as the one of the current/last start state event.
+                if (newReplaceId && lastId && newReplaceId === lastId) return;
+            } else {
+                // Add an event to the end of the timeline. It should not be the same as the one replaced by the current/last end state event.
+                if (lastReplaceId && newId && lastReplaceId === newId) return;
+            }
+
             this.setStateEvent(event);
             if (event.getType() === EventType.RoomMember) {
                 this.updateDisplayNameCache(event.getStateKey()!, event.getContent().displayname ?? "");
@@ -476,7 +504,7 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
                 // assume all our sentinels are now out-of-date
                 this.sentinels = {};
             } else if (UNSTABLE_MSC2716_MARKER.matches(event.getType())) {
-                this.emit(RoomStateEvent.Marker, event, markerFoundOptions);
+                this.emit(RoomStateEvent.Marker, event, options);
             }
         });