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

Ensure (room_id, next_batch_id) is unique to avoid cross-talk/conflicts between batches (MSC2716) #10877

Merged
merged 7 commits into from
Sep 29, 2021
Merged

Ensure (room_id, next_batch_id) is unique to avoid cross-talk/conflicts between batches (MSC2716) #10877

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff32a57
Ensure (room_id, next_batch_id) is unique to avoid cross-talk/conflic…
MadLittleMods Sep 22, 2021
1f3946e
Add changelog
MadLittleMods Sep 22, 2021
fd32072
Fix lint
MadLittleMods Sep 22, 2021
8daa576
Add punctuation to changelog
MadLittleMods Sep 22, 2021
21200f8
Stop people from sending duplicate insertion events
MadLittleMods Sep 24, 2021
b22c9fa
Also check insertion events from room creators in existing room versions
MadLittleMods Sep 24, 2021
ffa31e0
Fix lint
MadLittleMods Sep 24, 2021
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/10877.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure `(room_id, next_batch_id)` is unique across [MSC2716](https://github.com/matrix-org/matrix-doc/pull/2716) insertion events in rooms to avoid cross-talk/conflicts between batches.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to add a unique constraint to the DB tables?

@erikjohnston If possible, but you shied away from it in #10245 (comment)

I think we just want to make event_id unique? (Note that event IDs are globally unique).

I'm not sure whether we can assert that (room_id, next_chunk_id) is unique here, I could imagine that one day a remote server gets confused and reuses a chunk ID or something, at which point we probably don't want to explode on insertion of the event.

-- @erikjohnston, #10245 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Oh lols. I'm not sure I agree with myself there, but haven't really thought about it. Anyway, doesn't need to block this PR.

18 changes: 18 additions & 0 deletions synapse/storage/databases/main/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1795,6 +1795,24 @@ def _handle_insertion_event(self, txn: LoggingTransaction, event: EventBase):
# Invalid insertion event without next batch ID
return

conflicting_insertion_event_id = self.db_pool.simple_select_one_onecol_txn(
txn,
table="insertion_events",
keyvalues={
"room_id": event.room_id,
"next_batch_id": next_batch_id,
},
retcol="event_id",
allow_none=True,
)
if conflicting_insertion_event_id is not None:
# The new insertion event is invalid because there already exists
# and insertion event in the room with the same next_batch_id. We
# can't allow multiple because the batch pointing will get weird,
# e.g. we can't determine which insertion event the batch event is
# pointing to.
return
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

logger.debug(
"_handle_insertion_event (next_batch_id=%s) %s", next_batch_id, event
)
Expand Down