Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Give the correct next event when the message timestamps are the same - MSC3030 #13658

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Give the correct next event when the message timestamps are the same - MSC3030 #13658

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6329017
Give the correct next event when the message timestamps are the same
MadLittleMods Aug 29, 2022
47e2a40
Add changelog
MadLittleMods Aug 29, 2022
eadfde0
Better wording in changelog
MadLittleMods Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13658.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix MSC3030 `/timestamp_to_event` endpoint returning the correct next event when the events have the same timestamp.
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 10 additions & 2 deletions synapse/storage/databases/main/events_worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2111,7 +2111,14 @@ async def get_event_id_for_timestamp(
AND room_id = ?
/* Make sure event is not rejected */
AND rejections.event_id IS NULL
ORDER BY origin_server_ts %s
/**
* First sort by the message timestamp. If the message timestamps are the
* same, we want the message that logically comes "next" (before/after
* the given timestamp) based on the DAG and its topological order (`depth`).
* Finally, we can tie-break based on when it was received on the server
* (`stream_ordering`).
*/
ORDER BY origin_server_ts %s, depth %s, stream_ordering %s
Copy link
Contributor

Choose a reason for hiding this comment

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

when it was received on the server (stream_ordering).

There is a received_ts column for this (though there may very well be tie-breaks just like the origin timestamp).

origin_server_ts and depth are untrusted fields controlled by the sender's HS. I'm not sure if that is a concern here though.

Copy link
Contributor

@squahtx squahtx Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can't think of a better way to do this without ordering by depth. There is a topological_ordering column, but apparently that's just identical to depth.

We're already trusting origin_server_ts to be reasonable for this feature, so I suppose it's not much of a stretch to trust depth as well.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

origin_server_ts and depth are untrusted fields controlled by the sender's HS. I'm not sure if that is a concern here though.

We're already trusting origin_server_ts to be reasonable for this feature, so I suppose it's not much of a stretch to trust depth as well.

👍 exactly both of these

LIMIT 1;
"""

Expand All @@ -2130,7 +2137,8 @@ def get_event_id_for_timestamp_txn(txn: LoggingTransaction) -> Optional[str]:
order = "ASC"

txn.execute(
sql_template % (comparison_operator, order), (timestamp, room_id)
sql_template % (comparison_operator, order, order, order),
(timestamp, room_id),
)
row = txn.fetchone()
if row:
Expand Down