Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jul 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
57a8f32
Fix room state being updated with old (now overwritten) state and emi…
toger5 Jun 13, 2024
5a35e3b
remove timestamp condition
toger5 Jun 14, 2024
4ab2cc1
fix timeline tests
toger5 Jun 17, 2024
2e1b08e
only skip event adding if event_id and replaces_state is set.
toger5 Jun 17, 2024
3bb9291
fix room tests
toger5 Jun 18, 2024
d469eb0
test skipping insertion
toger5 Jun 18, 2024
b5d5dd3
rename back to lastStateEvent
toger5 Jun 21, 2024
8ebf04a
store if a state is at the start of a timeline in the RoomState class
toger5 Jun 21, 2024
161d63e
make `isStartTimelineState` a `public readonly` and fix condition.
toger5 Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions spec/unit/event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe("EventTimeline", function () {
const timelineStartState = timeline.startState!;
expect(mocked(timelineStartState).setStateEvents).toHaveBeenCalledWith(events, {
timelineWasEmpty: undefined,
toStartOfTimeline: true,
});
// @ts-ignore private prop
const timelineEndState = timeline.endState!;
Expand Down Expand Up @@ -313,9 +314,11 @@ describe("EventTimeline", function () {

expect(timeline.getState(EventTimeline.FORWARDS)!.setStateEvents).toHaveBeenCalledWith([events[0]], {
timelineWasEmpty: undefined,
toStartOfTimeline: false,
});
expect(timeline.getState(EventTimeline.FORWARDS)!.setStateEvents).toHaveBeenCalledWith([events[1]], {
timelineWasEmpty: undefined,
toStartOfTimeline: false,
});

expect(events[0].forwardLooking).toBe(true);
Expand Down Expand Up @@ -352,9 +355,11 @@ describe("EventTimeline", function () {

expect(timeline.getState(EventTimeline.BACKWARDS)!.setStateEvents).toHaveBeenCalledWith([events[0]], {
timelineWasEmpty: undefined,
toStartOfTimeline: true,
});
expect(timeline.getState(EventTimeline.BACKWARDS)!.setStateEvents).toHaveBeenCalledWith([events[1]], {
timelineWasEmpty: undefined,
toStartOfTimeline: true,
});

expect(events[0].forwardLooking).toBe(false);
Expand Down
50 changes: 50 additions & 0 deletions spec/unit/room-state.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,4 +1122,54 @@ describe("RoomState", function () {
).toBeFalsy();
});
});
describe("skipWrongOrderRoomStateInserts", () => {
const idNow = "now";
const idBefore = "before";
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();
}
};
state.on(RoomStateEvent.Events, onRoomStateEvent);
});
afterEach(() => {
state.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", () => {
state.setStateEvents([evNow, evBefore], { toStartOfTimeline: false });
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", () => {
state.setStateEvents([evBefore, evNow], { toStartOfTimeline: true });
expect(updatedStateWithBefore).toHaveBeenCalled();
expect(updatedStateWithNow).not.toHaveBeenCalled();
});
});
});
20 changes: 16 additions & 4 deletions spec/unit/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,14 @@ describe("Room", function () {
}),
];
await room.addLiveEvents(events);
expect(room.currentState.setStateEvents).toHaveBeenCalledWith([events[0]], { timelineWasEmpty: false });
expect(room.currentState.setStateEvents).toHaveBeenCalledWith([events[1]], { timelineWasEmpty: false });
expect(room.currentState.setStateEvents).toHaveBeenCalledWith([events[0]], {
timelineWasEmpty: false,
toStartOfTimeline: false,
});
expect(room.currentState.setStateEvents).toHaveBeenCalledWith([events[1]], {
timelineWasEmpty: false,
toStartOfTimeline: false,
});
expect(events[0].forwardLooking).toBe(true);
expect(events[1].forwardLooking).toBe(true);
expect(room.oldState.setStateEvents).not.toHaveBeenCalled();
Expand Down Expand Up @@ -726,8 +732,14 @@ describe("Room", function () {
];

room.addEventsToTimeline(events, true, room.getLiveTimeline());
expect(room.oldState.setStateEvents).toHaveBeenCalledWith([events[0]], { timelineWasEmpty: undefined });
expect(room.oldState.setStateEvents).toHaveBeenCalledWith([events[1]], { timelineWasEmpty: undefined });
expect(room.oldState.setStateEvents).toHaveBeenCalledWith([events[0]], {
timelineWasEmpty: undefined,
toStartOfTimeline: true,
});
expect(room.oldState.setStateEvents).toHaveBeenCalledWith([events[1]], {
timelineWasEmpty: undefined,
toStartOfTimeline: true,
});
expect(events[0].forwardLooking).toBe(false);
expect(events[1].forwardLooking).toBe(false);
expect(room.currentState.setStateEvents).not.toHaveBeenCalled();
Expand Down
6 changes: 3 additions & 3 deletions src/models/event-timeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export class EventTimeline {
throw new Error("Cannot initialise state after events are added");
}

this.startState?.setStateEvents(stateEvents, { timelineWasEmpty });
this.startState?.setStateEvents(stateEvents, { timelineWasEmpty, toStartOfTimeline: true });
this.endState?.setStateEvents(stateEvents, { timelineWasEmpty });
}

Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -375,7 +375,7 @@ export class EventTimeline {

// modify state but only on unfiltered timelineSets
if (event.isState() && timelineSet.room.getUnfilteredTimelineSet() === timelineSet) {
roomState?.setStateEvents([event], { timelineWasEmpty });
roomState?.setStateEvents([event], { timelineWasEmpty, toStartOfTimeline });
// it is possible that the act of setting the state event means we
// can set more metadata (specifically sender/target props), so try
// it again if the prop wasn't previously set. It may also mean that
Expand Down
32 changes: 27 additions & 5 deletions src/models/room-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,16 @@ export interface IMarkerFoundOptions {
*/
timelineWasEmpty?: boolean;
}

export interface ISetStateEventsOptions {
/** Whether the event is inserted at the start of the timeline
* or at the end. This is relevant for doing a sanity check:
* "Is the event id of the added event the same as the replaces_state id of the current event"
* We need to do this because sync sometimes feeds previous state events.
* If set to true the sanity check is inverted
* "Is the event id of the current event the same as the replaces_state id of the added event"
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this makes sense, as a RoomState is only ever in one direction. This sounds like if toStartOfTimeline=false then you should always drop the state event as they're never live. It should be a property on a RoomState if required

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good observation. The state itself might be the right location to store if it's a start state or an end state.
It should never change and also not be relevant outside the state object it seems logical to belong to the state.

I have trouble understanding the exact definition of start state anyway however:

If we have a state that is only set at the beginning of the room, events from a time-line in a specific interval cannot be enough to compute the start state because the start state needs all state events BEFORE the start of the time-line but the time-line only includes events after the "start".

This is slightly off topic but i still wanted to being it up. Feel free to DM me about this or point me to where i can lookup how start state is defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I understand it, it is the state at the start of the timeline, and it walks backwards as you backpaginate and load older events which undo the more recent state events.

Copy link
Contributor Author

@toger5 toger5 Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also be how i understand start state. But this is not what we are doing.
We use the content to set the state keys. What we would need to do is set the state with the previous_content field.

| = start of timeline
|| = end of timeline
--- = time steps
XV> = state event changing the state (the content of the state event is the current state from that time on (X is an arbitrary label for the state event (f.ex. a member event) X is the type and V is the value

X0>-----|X1>----X2>----||
Here we have an example where the state is changed with the first event in the timeline. But the start imo is before that event. So the state at the start is X0.

This becomes more clear when adding another type:
X0>--A0>--|A1>-A2>-X1>--||
At the start of the timeline we have X0 A0 but definitly not X1 which would be the first X type state event.

If we use the previous_content we have <XV as the previous content.

<X0>--<A0>--|<A1>-<A2>-<X1>--||

Then <A1 & <X1 is the same as X0> & A0> and the correct start state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest raising this to the wider team as the code & intent predates me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. Sorry for escalating this but I always found the startState to be arguably the correct name for that variable the way its implemented and this seemed to be relevant for this discussion. Somewhat relevant, you comment is just about where to store start/end.
What I do like about making it part of the setStateEvents property is that is makes it testable in a more isolated env.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get back to the topic what do you think about the more encapsulated testability with the current approach?

toStartOfTimeline?: boolean;
}
// possible statuses for out-of-band member loading
enum OobStatus {
NotStarted,
Expand Down Expand Up @@ -408,7 +417,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 & ISetStateEventsOptions): void {
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
this.updateModifiedTime();

// update the core event dict
Expand All @@ -419,13 +428,26 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
this.setBeacon(event);
}

const lastStateEvent = this.getStateEventMatching(event);
const lastStateEv = this.getStateEventMatching(event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this get renamed to something less clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we had a lot more linewraps that made I judged as harder to read overall.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only because on L435 and L436 you're using the array syntax to set variables which is really redundant. It'd be clearer as 4 separate instantiations.

// 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, lastId] = [lastStateEv?.event.unsigned?.replaces_state, lastStateEv?.event.event_id];
const [newReplaceId, newId] = [event.event.unsigned?.replaces_state, event.event.event_id];
if (options?.toStartOfTimeline) {
// Add an event to the start of the timeline. Its replace id not be the same as the one of the current/last start state event.
if (newReplaceId && newId && newReplaceId === lastId) return;
} else {
// Add an event to the end of the timeline. It should not be the same as the one replaced but the current/last end state event.
if (lastReplaceId && newId && lastReplaceId === newId) return;
toger5 marked this conversation as resolved.
Show resolved Hide resolved
}

this.setStateEvent(event);
if (event.getType() === EventType.RoomMember) {
this.updateDisplayNameCache(event.getStateKey()!, event.getContent().displayname ?? "");
this.updateThirdPartyTokenCache(event);
}
this.emit(RoomStateEvent.Events, event, this, lastStateEvent);
this.emit(RoomStateEvent.Events, event, this, lastStateEv);
});

this.onBeaconLivenessChange();
Expand Down Expand Up @@ -476,7 +498,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);
}
});

Expand Down
Loading