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

Several places in the code use timestamp to determine event order #3325

Open
andybalaam opened this issue Apr 27, 2023 · 9 comments
Open

Several places in the code use timestamp to determine event order #3325

andybalaam opened this issue Apr 27, 2023 · 9 comments
Labels
T-Task Tasks for the team like planning

Comments

@andybalaam
Copy link
Contributor

When are processing events, we sometimes need to know what order they are in. (For example, to decide whether an event is before the last read receipt so we know whether a room is unread.)

The correct order to use is the order in which they are provided during the sync request from the homeserver. It is the homeserver's job to determine a linear order from the DAG, and we need to agree with the homeserver to make sure we are giving the correct results for e.g. the notifications for a room.

In some places in the code we use the timestamp (ts property) of events to determine their order, because we don't have a record of the order of events from sync.

We should keep track of the server-provided event order and replace all uses of ts with the correct order. In the meantime, if we add code that depends on ts, we should reference this issue in a comment in the code.

Impact of this issue: sometimes there may be stuck unreads or notifications due to inconsistencies between how the homeserver and the client understand the order of events. This may also cause other problems - we're not certain. We think the problems we see will be relatively rare, which is why this has not yet been fixed.

@t3chguy
Copy link
Member

t3chguy commented Apr 27, 2023

The correct order to use is the order in which they are provided during the sync request from the homeserver.

What about when you have events from /context/ which follows a different ordering? Similar question for /relations/.

@andybalaam
Copy link
Contributor Author

The correct order to use is the order in which they are provided during the sync request from the homeserver.

What about when you have events from /context/ which follows a different ordering? Similar question for /relations/.

I'm not sure, but in principle I think the answer is that we should keep hold of our current best guess of the order the server has, even if sometimes it's not 100% correct.

In future we might need to figure out an MSC that makes this easier to deal with.

@t3chguy
Copy link
Member

t3chguy commented Apr 27, 2023

Related matrix-org/matrix-spec#852

I'm not sure, but in principle I think the answer is that we should keep hold of our current best guess of the order the server has, even if sometimes it's not 100% correct.

The server has 2 main orderings though, topological ordering and stream ordering, which do you propose we try and guess? Some APIs return one, some the other. They do not fit within the same total order.

@andybalaam
Copy link
Contributor Author

Related matrix-org/matrix-spec#852

I'm not sure, but in principle I think the answer is that we should keep hold of our current best guess of the order the server has, even if sometimes it's not 100% correct.

The server has 2 main orderings though, topological ordering and stream ordering, which do you propose we try and guess? Some APIs return one, some the other. They do not fit within the same total order.

I can't find the answer in the spec but my current belief is that we need stream order to handle read receipts correctly. Do you have an opinion?

@t3chguy
Copy link
Member

t3chguy commented Apr 27, 2023

Do you have an opinion?

Everything is awful :)

@andybalaam
Copy link
Contributor Author

lol. I asked in the matrix spec room: link to message

@andybalaam
Copy link
Contributor Author

I was encouraged to log an issue against the spec, so I did that here: matrix-org/matrix-spec#1503

@justjanne
Copy link
Contributor

@andybalaam
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

No branches or pull requests

3 participants