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

MSC4102: Clarifying precedence in threaded and unthreaded read receipts in EDUs #4102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Feb 15, 2024

Rendered

No client implementation required.
Server implementation: matrix-org/sliding-sync#401 & element-hq/synapse#16927

Fixes the issue element-hq/element-x-ios#1479

Comment on lines +59 to +60
When a server is combining receipts into an EDU, if there are multiple receipts for the same (user, event, receipt type), always
choose the receipt which is unthreaded (has no `thread_id`) when aggregating into an EDU.
Copy link
Member

Choose a reason for hiding this comment

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

Combining leads to data loss, which is possibly why we still have stuck notifications/badge counts. If a threaded and unthreaded receipt are queued for sending, those should be independent EDUs because maps can't support both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Losing the threaded receipt isn't really data loss though, which is what the "Potential Issues" section talks about.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, fair point. I'm not sure this needs to be an MSC then - it feels like a clarification that can be made directly to the spec, given it's effectively talking about a data optimization and not functional specified behaviour.

@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Feb 15, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Implementation requirements:

  • Demonstration of a bug fixed (likely a client implementation)

Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'd prefer to change the format but that would be a lot of pain and this proposal will fix the issue.

Related: matrix-org/matrix-spec#1684

Comment on lines +7 to +9
This proposal clarifies previously undefined behaviour around how read receipts are expressed over
the CSAPI and SSAPI. This clarification reliably provides clients with sufficient information to
determine read receipts, without placing additional burdens on client implementations.
Copy link
Member

Choose a reason for hiding this comment

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

element-hq/synapse#16927 doesn't seem to make any changes to the server-server API and Synapse currently will send multiple EDUs over federation if there's both an unthreaded and threaded receipt (or just multiple threads).

This is since the EDU format is a bit different there. It maps room ID -> receipt type -> user ID -> {event IDs, data: {ts, thread ID (if applicable)}. (The client-server API maps event ID -> receipt type -> user ID -> {ts, thread ID (if applicable)}.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, which is fine. Federation can send multiple EDUs if it wants to, so long as at the end of the day the receiving server has the right receipts to combine them in /sync. The Complement test does check that this MSC works over federation.

Copy link
Member

Choose a reason for hiding this comment

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

I guess my point is that there isn't undefined behavior of the SSAPI.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 9, 2024
# Synapse 1.102.0 (2024-03-05)

### Bugfixes

- Revert element-hq/synapse#16756, which caused incorrect notification counts on mobile clients since v1.100.0. ([\#16979](element-hq/synapse#16979))


# Synapse 1.102.0rc1 (2024-02-20)

### Features

- A metric was added for emails sent by Synapse, broken down by type: `synapse_emails_sent_total`. Contributed by Remi Rampin. ([\#16881](element-hq/synapse#16881))

### Bugfixes

- Do not send multiple concurrent requests for keys for the same server. ([\#16894](element-hq/synapse#16894))
- Fix performance issue when joining very large rooms that can cause the server to lock up. Introduced in v1.100.0. ([\#16903](element-hq/synapse#16903))
- Always prefer unthreaded receipt when >1 exist ([MSC4102](matrix-org/matrix-spec-proposals#4102)). ([\#16927](element-hq/synapse#16927))

### Improved Documentation

- Fix a small typo in the Rooms section of the Admin API documentation. Contributed by @RainerZufall187. ([\#16857](element-hq/synapse#16857))

### Internal Changes

- Don't invalidate the entire event cache when we purge history. ([\#16905](element-hq/synapse#16905))
- Add experimental config option to not send device list updates for specific users. ([\#16909](element-hq/synapse#16909))
- Fix incorrect docker hub link in release script. ([\#16910](element-hq/synapse#16910))



### Updates to locked dependencies

* Bump attrs from 23.1.0 to 23.2.0. ([\#16899](element-hq/synapse#16899))
* Bump bcrypt from 4.0.1 to 4.1.2. ([\#16900](element-hq/synapse#16900))
* Bump pygithub from 2.1.1 to 2.2.0. ([\#16902](element-hq/synapse#16902))
* Bump sentry-sdk from 1.40.0 to 1.40.3. ([\#16898](element-hq/synapse#16898))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants