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.

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

Support MSC4222 state_after #4487

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from
Open

Support MSC4222 state_after #4487

wants to merge 38 commits into from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 30, 2024

Closes #4532
Fixes element-hq/element-web#28536
Fixes #4528

This implements support for state_after in sync responses, as per matrix-org/matrix-spec-proposals#4222

In this, the events that arrive in the timeline should no longer be added to the state of the room. Instead, all state events arrive explicitly, duplicated, in state_after. Applying the events in state_after gives the state of the room after that sync request. This fixes problems where state events in the timeline should not actually change the state of the room, for example if old state events arrive from disconnected homeservers.

Synapse impl: element-hq/synapse#17888

This will need aggressive testing against synapse both with and without PR 17888. It's quite an invasive change. I opted to require callers of methods that add timeline events to explicitly specify whether they want the timeline events to apply to state since defaulting to either is not really safe, and this helps us catch all the code paths where it happens.

This changes some interface in the js-sdk and so should probably be considered a breaking change (and I've removed some deprecated interfaces while I'm at it).

Changes to public functions:

  • SyncApi.injectRoomEvents() - parameters have changed
  • Room.addLiveEvents() - addLiveEventOptions parameter is now required
  • EventTimeline.addEvent() - toStartOfTimeline is now required; new required option addToState
  • EventTimelineSet.insertEventIntoTimeline() - new required parameter addToState
  • EventTimelineSet.addEventToTimeline() - deprecated variants removed; toStartOfTimeline is now required; new required option addToState

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).

Since it must have allowed state to be undefined previously: the test
had it as such.
if state can be undefined anyway
src/sync.ts Show resolved Hide resolved
src/sync-accumulator.ts Outdated Show resolved Hide resolved
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@toger5
Copy link
Contributor

toger5 commented Nov 12, 2024

This was tried in the SPA (EC with this branch directly connected to a home server)
We did run into the following issue:

  • The call member events were synced on most clients (we assume there the reliable state worked, cannot be proven of course)
  • The room members however showed incorrect bahaviours
  • different clients had different room member counts
  • On Hughs client a room member was there and then disappeared (but still is there on other clients)
  • room.currentState.events("m.call.member") also has those missing members (on some clients) - as does room.currentState.getMembers()`
  • `room.currentState.events("org.matrix.msc3401.call.member") showed the same events on all clients.

Found issue

This was caused by only storing events that come down through the timeline sync section in indexDB.
So a reload on the client makes you loose all events that you have synced as part of the initial sync and came down through state_after
A client that never reloaded will work fine since it has state_after state in memory and all changes will also update.
That is why we had clients with broken state (clients that were reloaded by the user) and clients that had all state.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@toger5
Copy link
Contributor

toger5 commented Nov 13, 2024

There are still issues with the local storage.
The previous comment mentions an issue where,

a reload on the client makes you loose all events that you have synced as part of the initial sync and came down through state_after

This was easily testable by reloading a client and seeing all room names to disappear.

With the fix from @t3chguy this is not the case anymore. But we still see issues after a reload (loading sync from cache).

This can be used to investigate the issue https://pr2767--element-call.netlify.app/

If you are in a call and others join and leave, you reload the client the number at the top left will not be correct. The list of org.matrix.msc3401.call.member also will be incorrect.

@dbkr
Copy link
Member Author

dbkr commented Nov 18, 2024

Thanks for continuing work on this. The changes look good as far as I can see, glad the sync accumulator wasn't too much of a big change to get working.

toger5 and others added 3 commits November 19, 2024 13:57
… and emitting for those updates. (#4242)"

This reverts commit 957329b.
…er field

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
…kr/stateafter

# Conflicts:
#	src/embedded.ts
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I only have minor comments. Nice that there is also the test to the sync accumulator

spec/unit/room-state.spec.ts Show resolved Hide resolved
src/embedded.ts Outdated Show resolved Hide resolved
src/sync-accumulator.ts Outdated Show resolved Hide resolved
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

crypto changes look fine

@toger5
Copy link
Contributor

toger5 commented Nov 26, 2024

Is there anything else we should wait for before merging this?

This mentions the EW pr needs to be merged first: element-hq/element-web#28398

@t3chguy
Copy link
Member

t3chguy commented Nov 26, 2024

Yes, it will need force merging when I am done with the release process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect state shown after rejoining room Creator sees moderator as a regular user after rejoining chat
6 participants