Make MSC4102 "prefer unthreaded receipt" durable at insert time by erikjohnston · Pull Request #19838 · element-hq/synapse

erikjohnston · 2026-06-09T12:24:32Z

Fix #19171

Problem

TestThreadReceiptsInSyncMSC4102 is a persistent Complement flake (workers + federation). Example failing run.

MSC4102 requires that an unthreaded read receipt always wins over a clashing threaded one (same user, same event). Today that's enforced only at read time, in ReceiptInRoom.merge_to_content, which dedupes a clashing pair within
a single /sync response.

That breaks down when the two receipts are persisted at different stream positions and get served in separate /sync responses. Tracing the failing run:

Alice sends an unthreaded receipt for event B, then a threaded receipt for the same event B. Both are federated to hs2 as two separate m.receipt EDUs and persisted there at receipts stream positions 2 (unthreaded) and 3
(threaded).
Bob's initial sync advanced its receipt token to 2 but emitted no receipt; his next (incremental) sync window was (2,3], containing only the threaded receipt.
The unthreaded receipt was never surfaced, so the threaded one "won" → MSC4102 violation → timeout.

Fix

Make "unthreaded wins" durable at insert time. In _insert_linearized_receipt_txn, drop a threaded receipt if an unthreaded receipt for the same (room, type, user) already exists for the same event. It is then never
persisted, streamed, or federated, so no sync-window split can resurface it — and on hs1 the clashing threaded receipt is never sent over federation in the first place.

Key design points:

The check keys off event id, mirroring the read-time dedup in ReceiptInRoom.merge_to_content (which clashes purely on (user_id, event_id), never on ordering). This keeps the two layers consistent and order-independent.
Because we match on event id rather than event_stream_ordering, it also covers a remote unthreaded receipt whose event we hadn't seen when it arrived (NULL event_stream_ordering, which is never backfilled outside the one-time
background update). (Thanks to the Copilot review for catching this case.)
The same-thread "don't clobber a more recent event" check is unchanged; both checks now share a single query.

This is safe for notification counts: EventPushSummary.is_unread already treats the unthreaded receipt as a floor across all threads, so a threaded receipt at the same event is redundant for counts.

Copilot

Pull request overview

This PR makes MSC4102’s “prefer unthreaded read receipt” behavior durable by enforcing the preference at receipt insert time, preventing conflicting threaded receipts from being persisted (and later resurfacing across split /sync windows).

Changes:

Update receipt insertion logic to drop threaded receipts when an unthreaded receipt for the same user/room/type already supersedes it.
Add a regression test covering the threaded-vs-unthreaded insert-time behavior.
Add a Towncrier bugfix newsfragment describing the user-visible fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`synapse/storage/databases/main/receipts.py`	Enforce MSC4102 preference at insert time by dropping semantically-redundant threaded receipts.
`tests/storage/test_receipts.py`	Add regression coverage to ensure conflicting threaded receipts are dropped when an unthreaded receipt exists.
`changelog.d/19838.bugfix`	Add release note for the MSC4102 `/sync` receipt ordering bugfix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Previously the "unthreaded receipt always wins over a clashing threaded one" behaviour was only applied at read time, in `ReceiptInRoom.merge_to_content`, which dedupes the pair within a single /sync response. When the two receipts end up at different stream positions and are served in separate /sync responses (e.g. when they arrive over federation as separate EDUs), the threaded receipt could be served on its own and incorrectly win, which is what caused the `TestThreadReceiptsInSyncMSC4102` Complement flake. Drop a threaded receipt at insert time if an unthreaded receipt for the same user already exists at the same or a later event, so it never gets persisted, streamed or federated. This is safe for notification counts, since the unthreaded receipt already acts as a floor across all threads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

MadLittleMods · 2026-06-09T21:22:11Z

        )
        self.assertEqual(res, {self.room_id1: event1_2_id, self.room_id2: event2_1_id})

+    def test_threaded_receipt_dropped_when_unthreaded_exists(self) -> None:


Seems like the TestThreadReceiptsInSyncMSC4102 Complement test should also be updated to better separate read receipts across EDU's so it can consistently stress the failure case. Or could be an additional new test that does that (engineered homeserver)

Have updated it in matrix-org/complement#881

MadLittleMods · 2026-06-09T21:30:45Z

+                # Doing this at insert time, as well as when serving receipts,
+                # makes the "prefer unthreaded" behaviour durable: otherwise the
+                # threaded receipt could be served on its own in a later /sync
+                # response (e.g. when the unthreaded and threaded receipts
+                # arrive in separate federation EDUs and so end up at different
+                # stream positions), causing the client to incorrectly see it
+                # win.


Can we drop the /sync behavior in favor the insert solution we have here?

Perhaps instead an assert at the /sync level.

Mmm, possibly. We wouldn't want to do it immediately to handle existing receipts in the DB.

MadLittleMods · 2026-06-09T22:05:24Z

@@ -0,0 +1 @@
+Fix a bug where a threaded read receipt could incorrectly win over a clashing unthreaded one (MSC4102) when the two were served in separate `/sync` responses, e.g. when received over federation as separate EDUs.


How did you reproduce the TestThreadReceiptsInSyncMSC4102 failure locally? Or did the LLM just spot what was wrong based on the test flake output from CI?

I struggled: #19171

This was a case of throwing LLM at the logs of a flakey run. What it spit out made sense and seemed to match what was happening, and the suggested fix (this) made sense in the context of what we were doing when reading as well.

MadLittleMods · 2026-06-15T20:44:59Z

@@ -0,0 +1 @@
+Fix a bug where a threaded read receipt could incorrectly win over a clashing unthreaded one (MSC4102) when the two were served in separate `/sync` responses, e.g. when received over federation as separate EDUs.


Per my points in matrix-org/complement#881, I think this may be missing the mark on the behavior of MSC4102

How so? We want to make sure that when combining receipts for the same event ID we want the unthreaded to win? MSC4102 does only talk about EDUs, but I think the idea is still that you want unthreaded to win everywhere?

but I think the idea is still that you want unthreaded to win everywhere?

I don't think there is any spec for this.

We want to record all of the read receipts and only deduplicate if there are conflicting read receipts when creating m.receipt EDU's. We seem to already be doing this.

And to better clarify and exaggerate why this is fine, it's okay to send and receive a threaded read receipt for the same event two hours after the unthreaded read receipt. See MSC3771 which explains,

This MSC proposes allowing the same receipt type to exist multiple times in a room per user:

Once for the unthreaded timeline.

Once for the main timeline in the room.

Once per threaded timeline.

And the only restriction is that "this still does not allow a caller to move their receipts backwards in a room"

If this breakdown is to be believed, the bug appears to be in the following behavior especially "The unthreaded receipt was never surfaced" part. Why is /sync advancing past its persisted position?

Alice sends an unthreaded receipt for event B, then a threaded receipt for the same event B. Both are federated to hs2 as two separate m.receipt EDUs and persisted there at receipts stream positions 2 (unthreaded) and 3
(threaded).

Bob's initial sync advanced its receipt token to 2 but emitted no receipt; his next (incremental) sync window was (2,3], containing only the threaded receipt.

The unthreaded receipt was never surfaced, so the threaded one "won" → MSC4102 violation → timeout.

erikjohnston force-pushed the erikj/fix_receipts_fed branch 2 times, most recently from 73621b4 to a7434ba Compare June 9, 2026 13:01

erikjohnston requested a review from Copilot June 9, 2026 13:02

Copilot started reviewing on behalf of erikjohnston June 9, 2026 13:02 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread synapse/storage/databases/main/receipts.py

erikjohnston force-pushed the erikj/fix_receipts_fed branch from a7434ba to 8750a01 Compare June 9, 2026 15:59

erikjohnston marked this pull request as ready for review June 9, 2026 17:34

erikjohnston requested a review from a team as a code owner June 9, 2026 17:34

MadLittleMods added the A-Read-Receipts label Jun 9, 2026

MadLittleMods reviewed Jun 9, 2026

View reviewed changes

MadLittleMods added the A-Threads label Jun 9, 2026

MadLittleMods reviewed Jun 9, 2026

View reviewed changes

erikjohnston added 2 commits June 15, 2026 10:35

Mention why its safe to clobber null stream ordering

d5f8ea4

Mention we're fetching both threaded and unthreaded receipts

dcc2f64

erikjohnston mentioned this pull request Jun 15, 2026

Make TestThreadReceiptsInSyncMSC4102 deterministic matrix-org/complement#881

Open

erikjohnston requested a review from MadLittleMods June 15, 2026 15:43

MadLittleMods requested changes Jun 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make MSC4102 "prefer unthreaded receipt" durable at insert time#19838

Make MSC4102 "prefer unthreaded receipt" durable at insert time#19838
erikjohnston wants to merge 3 commits into
developfrom
erikj/fix_receipts_fed

erikjohnston commented Jun 9, 2026 •

edited by MadLittleMods

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

MadLittleMods Jun 9, 2026

Uh oh!

erikjohnston Jun 15, 2026

Uh oh!

MadLittleMods Jun 9, 2026

Uh oh!

erikjohnston Jun 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Jun 9, 2026

Uh oh!

erikjohnston Jun 11, 2026

Uh oh!

MadLittleMods Jun 15, 2026

Uh oh!

erikjohnston Jun 16, 2026

Uh oh!

MadLittleMods Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1 @@
		Fix a bug where a threaded read receipt could incorrectly win over a clashing unthreaded one (MSC4102) when the two were served in separate `/sync` responses, e.g. when received over federation as separate EDUs.

Uh oh!

Conversation

erikjohnston commented Jun 9, 2026 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erikjohnston commented Jun 9, 2026 •

edited by MadLittleMods

Loading