Make TestThreadReceiptsInSyncMSC4102 deterministic#881
Conversation
The test only caught the MSC4102 "prefer unthreaded receipt" bug when the clashing unthreaded and threaded receipts happened to be served in the same /sync response (where read-time dedup hides the threaded one), so it flaked. Force the receipts into separate sync windows by waiting for each user to observe the unthreaded receipt before sending the clashing threaded one, then assert the threaded receipt never wins, both locally and over federation.
| // Servers should always prefer the unthreaded receipt when there is a clash of | ||
| // receipts for the same event, and that preference must be *durable*: it must | ||
| // hold even when the unthreaded and threaded receipts are served in separate | ||
| // `/sync` responses. This happens when the two land at different stream | ||
| // positions, e.g. when they arrive over federation as separate EDUs. A | ||
| // non-durable server that only dedupes at read-time will serve the threaded | ||
| // receipt on its own in a later `/sync`, letting it incorrectly win. |
There was a problem hiding this comment.
It's unclear to me that this is true for all cases.
As far as I can tell, for example, we could also tackle this problem at read-time. In Synapse, get_linearized_receipts_for_room(...)/get_linearized_receipts_for_rooms(...), could de-duplicate if a previous unthreaded receipt existed. Even in Synapse, we store things as (room_id, receipt_type, user_id, thread_id) after all which accommodates holding onto both
Which also makes me question whether our approach in element-hq/synapse#19838 is the best (not sure)
(also plays into some of the comments which make some assumptions as well)
There was a problem hiding this comment.
Is your point that actually you could correctly implement this at read time by checking for other receipts when pulling from the DB? Yes I suppose you could?
There was a problem hiding this comment.
Yes, in which case this comment assumes too much
| // We can't use a `syncHasThreadedReadReceipt` check for this: MustSyncUntil | ||
| // drops a check once it passes, whereas we need to keep scanning every | ||
| // response for the forbidden receipt right up until the marker arrives. So | ||
| // we accumulate a flag by hand and use the marker as the (positive) stop | ||
| // condition. |
There was a problem hiding this comment.
We can still use syncHasThreadedReadReceipt but composed into our own check
Lines 87 to 89 in bc2b638
| // response for the forbidden receipt right up until the marker arrives. So | ||
| // we accumulate a flag by hand and use the marker as the (positive) stop | ||
| // condition. | ||
| marker := client.SyncTimelineHasEventID(roomID, markerEventID) |
There was a problem hiding this comment.
| marker := client.SyncTimelineHasEventID(roomID, markerEventID) | |
| syncTimelineHasMarkerEventID := client.SyncTimelineHasEventID(roomID, markerEventID) |
Although, I don't think it really matters to store and re-use this.
| // which the threaded receipt must have been delivered if it was going to be. We | ||
| // assert it never is, both for the local user and for a remote user over | ||
| // federation. | ||
| func TestThreadReceiptsInSyncMSC4102(t *testing.T) { |
There was a problem hiding this comment.
MSC4102 only mentions that the de-duplication should happen when assembling EDU's and there is a unthreaded and threaded receipt in the same response.
It doesn't say anything about an unthreaded read receipt winning out over new threaded read receipts
| // now send an unthreaded RR for event B and a threaded RR for event B and ensure we see the unthreaded RR | ||
| // down /sync. Non-compliant servers will typically send the last one only. |
There was a problem hiding this comment.
I think this test needs to stay trying to send an unthreaded and threaded read receipt in the same /send federation transaction.
But we can't control how a homeserver sends EDU's so I don't think this test can be made deterministic. We need to update it to accommodate the MSC4102 behavior or them being sent across two transactions.
There was a problem hiding this comment.
I think this test needs to stay trying to send an unthreaded and threaded read receipt in the same
/sendfederation transaction.
Why?
But we can't control how a homeserver sends EDU's so I don't think this test can be made deterministic. We need to update it to accommodate the MSC4102 behavior or them being sent across two transactions.
Sorry, what are you actually suggesting we do?
There was a problem hiding this comment.
This is the whole point of the MSC4102 and we want the test to stress this behavior,
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.This change will apply to all
m.receiptEDUs, which includes both CSAPI and Federation endpoints.-- MSC4102
But we can't control how a homeserver emits EDU's so the best we can do is send them quickly in succession and see what it does. Combining into one EDU is fine (per MSC4102) or separating is fine (depends on how slow the server is to process things and when the request comes in).
We need to update the test to accommodate both expectations.
2 participants
Companion to element-hq/synapse#19838
The test only caught the MSC4102 "prefer unthreaded receipt" bug when the clashing unthreaded and threaded receipts happened to be served in the same /sync response (where read-time dedup hides the threaded one), so it flaked.
Force the receipts into separate sync windows by waiting for each user to observe the unthreaded receipt before sending the clashing threaded one, then assert the threaded receipt never wins, both locally and over federation.