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

/sync and /members do not return "current state" #16940

Open
Tracked by #245
richvdh opened this issue Feb 19, 2024 · 8 comments
Open
Tracked by #245

/sync and /members do not return "current state" #16940

richvdh opened this issue Feb 19, 2024 · 8 comments

Comments

@richvdh
Copy link
Member

richvdh commented Feb 19, 2024

Consider a DAG like this:

             E1
           ↗    ↖
          |      S2
          |
        --|---
          |
          E3

The client then initialsyncs, with limit=1 (or maybe there are lots of events at E3), represented by the horizontal dashed line.

The "current state" (as defined by "what will be the state of the room if I send an event right now", which is what is important for encryption, etc) includes S2. However, the result from /sync does not include S2 (and will not, at least until there is an event in the DAG which joins the forks).

@richvdh
Copy link
Member Author

richvdh commented Feb 21, 2024

In general, this seems like a major problem. As long as there is no event in the DAG which references S2 as a prev_event, S2 will not be served to the client. If S2 is, say, a membership event, then this could lead to UTDs.

@kegsay
Copy link
Contributor

kegsay commented Feb 21, 2024

Specficially this would in the common case cause a single UTD, as the act of sending the encrypted message would merge the forks and cause S2 to arrive on the client. The UTD cause would be not sharing room keys with the new user, for potentially a long time after they joined.

kegsay added a commit to matrix-org/complement that referenced this issue Feb 22, 2024
@richvdh
Copy link
Member Author

richvdh commented Feb 23, 2024

Specficially this would in the common case cause a single UTD,

Unfortunately I think it's considerably worse than that. Per #16941, Synapse still won't send down S2 to the client even after the DAG is merged. I've proposed an improvement to that, but it still doesn't help clients which have lazy-loading enabled (which is basically all the ones we care about).

@richvdh richvdh changed the title /sync does not return "current state" /sync and /members do not return "current state" Feb 23, 2024
@richvdh
Copy link
Member Author

richvdh commented Feb 23, 2024

This also extends to /members (at least when called with an ?at= param, which is what you have to do for lazy-loading).

@richvdh
Copy link
Member Author

richvdh commented Apr 10, 2024

As long as there is no event in the DAG which references S2 as a prev_event, S2 will not be served to the client. If S2 is, say, a membership event, then this could lead to UTDs.

It's not even clear that events that reference S2 are sufficient to resolve this situation. Consider:

             E1
           ↗    ↖
          |      S2
          |       |
        --|-------|--
          |       |
          E3      |
          |       |
        --|-------|--
          |       |
          E4      |
          |       E5
          E6

What happens now? I'm pretty sure that S2 is not returned here.

Fundamentally, the expected behaviour is very poorly defined.

@erikjohnston
Copy link
Member

The correct fix for all of this (probably) is to switch to using current_state_delta_stream table to track the state changes, rather than looking at the state at individual events. This is not a small piece of work.

@kegsay
Copy link
Contributor

kegsay commented Sep 24, 2024

@erikjohnston would it help at all that at least for SSS we only care about returning the current state? We may not be able to fix it for sync v2 easily, but can we for SSS?

@erikjohnston
Copy link
Member

@erikjohnston would it help at all that at least for SSS we only care about returning the current state? We may not be able to fix it for sync v2 easily, but can we for SSS?

Yup! The SSS implementation actually tracks current state properly

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

No branches or pull requests

4 participants