fix(base): Fix state events handling in sliding sync #4095
Conversation
With sliding sync, we must handle state events from `required_state` only, not from `timeline`, this is a mistake as they might be incomplete or _staled_.
Sliding sync expects all state events to be in `required_state`. State events in `timeline` **must be ignored**. However, in sync v2, state events in `timeline` **must be handled**. In the sync response flow, both sliding sync and sync v2 uses the same `handle_timeline` method. This patch adds an argument to ignore state events. This is not ideal, but it's a temporary solution as a first step. The next step is to refactor this code, but let's start easy. The rest of the patch updates the tests accordingly.
Hywan
requested review from
andybalaam and
poljar
and removed request for
a team and
andybalaam
|
For what it's worth: this patch breaks support for the sliding sync proxy, which behaves differently and expected that we did handle those timeline events, as far as I recall (hence the change in the test expectation you've tweaked). It might not be the first patch that did break the sliding sync proxy, but it's at least one. Should we merge it only when we officially deprecate support for the sliding sync proxy? |
Hmm, AFAIR we shouldn't handle state events for the proxy case either. I think this was raised multiple times, but we failed to do something about it. @kegsay could you please confirm or deny this? |
To fix the `test_left_room`, we need to ask for the `m.room.member` state event from `required_state`. The rest of the patch rewrites the test a little bit to make it more Rust idiomatic.
To fix the `test_room_avatar_group_conversation`, we need to ask for the `m.room.avatar` state event from `required_state`. The rest of the patch rewrites the test a little bit to make it more Rust idiomatic. The `response.rooms.*.avatar` field from sliding sync should contain the new avatar, but for the moment, it doesn't. It seems to be a bug.
…` state. This patch updates the `required_state` of `all_rooms` inside the `RoomListService` to add `m.room.name`. Apparently, Synapse doesn't always update the `response.rooms.*.avatar` field when the avatar is updated. It's being investigated, but it doesn't hurt to ensure we get it from the state events.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4095 +/- ##
=======================================
Coverage 84.67% 84.68%
=======================================
Files 269 269
Lines 28753 28741 -12
=======================================
- Hits 24348 24339 -9
+ Misses 4405 4402 -3 ☔ View full report in Codecov by Sentry. |
|
Kegan has confirmed the proxy also requires to look inside the |
| @@ -384,6 +384,7 @@ impl BaseClient { | |||
| room: &Room, | |||
| limited: bool, | |||
| events: Vec<Raw<AnySyncTimelineEvent>>, | |||
| ignore_state_events: bool, | |||
There was a problem hiding this comment.
Not a fan of this boolean, but ok it's not as complex as I imagined it would be.
There was a problem hiding this comment.
Me neither! I'm gonna refactor this in a next PR (see the PR description where I already mention this).
Sliding sync expects all state events to be in
required_state. Stateevents in
timelinemust be ignored. However, in sync v2, stateevents in
timelinemust be handled.Two patches:
From sliding sync response handler, we don't gather state events
from
timelineanymore,In the same handler, both sliding sync and sync v2 uses the same
handle_timelinemethod. I add an argument to ignore stateevents. This is not ideal, but it's a temporary solution as a first
step. The next step is to refactor this code, but let's start easy.
Other patches updates the tests.
Next step: clean this flow/handler just like #4061 did.