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

Clarify order of initial vs incremental /sync #1917

Open
MadLittleMods opened this issue Aug 5, 2024 · 0 comments
Open

Clarify order of initial vs incremental /sync #1917

MadLittleMods opened this issue Aug 5, 2024 · 0 comments
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@MadLittleMods
Copy link
Contributor

Link to problem area:

Spec: https://spec.matrix.org/v1.10/client-server-api/#syncing

Events are ordered in this API according to the arrival time of the event on the homeserver. This can conflict with other APIs which order events based on their partial ordering in the event graph. This can result in duplicate events being received (once per distinct API called). Clients SHOULD de-duplicate events based on the event ID when this happens.

Issue

Spawning from a discussion with @erikjohnston,

The spec clearly states that /sync should return events according to the "arrival time of the event on the homeserver" (stream_ordering). This is not how Synapse has been behaving since the beginning. element-hq/synapse@e2accd7 even claims the following in the commit message:

The sync API often returns events in a topological rather than stream
ordering, e.g. when the user joined the room or on initial sync. When
this happens we can reuse existing pagination storage functions.

Normally, this would just be a spec-compliance problem in Synapse but I think this may be the right way to think about it.

Proposal

For initial /sync, we want to view a historical section of the timeline; to fetch events by topological_ordering (partial ordering in the event graph) (best representation of the room DAG as others were seeing it at the time). This also aligns with the order that /messages returns events in. We should also extend this to any time we're initially returning a room (therefore historical events) to the user (like a newly joined room). This behavior basically results in the same outcome as if no history was sent down /sync and /messages was used instead.

For incremental /sync, we want to get all updates for rooms since the last /sync (regardless if those updates arrived late or happened a while ago in the past); to fetch events by stream_ordering (in the order they were received by the server).

Related issues

@MadLittleMods MadLittleMods added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label Aug 5, 2024
MadLittleMods added a commit to element-hq/synapse that referenced this issue Aug 7, 2024
…remental sync (#17510)

Use `stream_ordering` based `timeline` pagination for incremental
`/sync` in Sliding Sync. Previously, we were always using a
`topological_ordering` but we should only be using that for historical
scenarios (initial `/sync`, newly joined, or haven't sent the room down
the connection before).

This is slightly different than what the [spec
suggests](https://spec.matrix.org/v1.10/client-server-api/#syncing)

> Events are ordered in this API according to the arrival time of the
event on the homeserver. This can conflict with other APIs which order
events based on their partial ordering in the event graph. This can
result in duplicate events being received (once per distinct API
called). Clients SHOULD de-duplicate events based on the event ID when
this happens.

But we've had a [discussion below in this
PR](#17510 (comment))
and this matches what Sync v2 already does and seems like it makes
sense. Created a spec issue
matrix-org/matrix-spec#1917 to clarify this.

Related issues:

 - matrix-org/matrix-spec#1917
 - matrix-org/matrix-spec#852
 - matrix-org/matrix-spec-proposals#4033
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

1 participant