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

Room batch: fix up handling of unknown prev_event_ids #12316

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Room batch: fix up handling of unknown prev_event_ids #12316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/12316.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Avoid trying to calculate the state at outlier events.
21 changes: 13 additions & 8 deletions synapse/rest/client/room_batch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,19 @@ async def on_POST(
errcode=Codes.INVALID_PARAM,
Copy link
Contributor

@MadLittleMods MadLittleMods Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

more of an msc2716 question: What is prev_event_id supposed to be when a batch_id is provided? If it's the insertion event associated with batch_id, then surely that's redundant?

-- @squahtx, /~https://github.com/squahtx, #12316 (review)

It's used as the place we're inserting next to. Which means, inheriting the depth and state at that point in time.

Batches of history aren't connected together in the DAG by prev_event_id. There is only a loose connection from the batch event of the next batch pointing back at the insertion event in the previous batch. But it's also possible to derive the same information from the previous batch but it just hasn't been that way. It can change.

)

# Make sure that the prev_event_ids exist and aren't outliers - ie, they are
# regular parts of the room DAG where we know the state.
non_outlier_prev_events = await self.store.have_events_in_timeline(
prev_event_ids_from_query
)
for prev_event_id in prev_event_ids_from_query:
if prev_event_id not in non_outlier_prev_events:
raise SynapseError(
HTTPStatus.BAD_REQUEST,
"prev_event %s does not exist, or is an outlier" % (prev_event_id,),
errcode=Codes.INVALID_PARAM,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a test for this case, tests/rest/client/test_room_batch.py

Copy link
Contributor

@MadLittleMods MadLittleMods Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is surprising that it's necessary. Is get_most_recent_full_state_ids_from_event_id_list -> state_store.get_state_ids_for_event(...) returning state for an outlier? It seems to look at state_groups which we don't expect to exist for an outlier but we know it can happen somehow: #12201 (comment)

I'd expect get_most_recent_full_state_ids_from_event_id_list to find no state for an outlier and the if not state_event_ids: check would have fired before

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd expect get_most_recent_full_state_ids_from_event_id_list to find no state for an outlier and the if not state_event_ids: check would have fired before

currently, that's the case. I'm seeking to change it in #12191.


# For the event we are inserting next to (`prev_event_ids_from_query`),
# find the most recent state events that allowed that message to be
# sent. We will use that as a base to auth our historical messages
Expand All @@ -131,14 +144,6 @@ async def on_POST(
prev_event_ids_from_query
)

if not state_event_ids:
raise SynapseError(
HTTPStatus.BAD_REQUEST,
"No auth events found for given prev_event query parameter. The prev_event=%s probably does not exist."
% prev_event_ids_from_query,
errcode=Codes.INVALID_PARAM,
)

state_event_ids_at_start = []
# Create and persist all of the state events that float off on their own
# before the batch. These will most likely be all of the invite/member
Expand Down