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

Update the rejected state of events during resync #13459

Merged
merged 5 commits into from
Aug 11, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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/13459.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Faster joins: update the rejected state of events during de-partial-stating.
55 changes: 55 additions & 0 deletions synapse/storage/databases/main/events_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2200,3 +2200,58 @@ def _get_partial_state_events_batch_txn(
(room_id,),
)
return [row[0] for row in txn]

def mark_event_rejected_txn(
self,
txn: LoggingTransaction,
event_id: str,
rejection_reason: Optional[str],
) -> None:
"""Mark an event that was previously accepted as rejected, or vice versa

This can happen, for example, when resyncing state during a faster join.

Args:
txn:
event_id: ID of event to update
rejection_reason: reason it has been rejected, or None if it is now accepted
"""
if rejection_reason is None:
logger.info(
"Marking previously-processed event %s as accepted",
event_id,
)
self.db_pool.simple_delete_txn(
txn,
"rejections",
keyvalues={"event_id": event_id},
)
else:
logger.info(
"Marking previously-processed event %s as rejected(%s)",
event_id,
rejection_reason,
)
self.db_pool.simple_upsert_txn(
txn,
table="rejections",
keyvalues={"event_id": event_id},
values={
"reason": rejection_reason,
"last_check": self._clock.time_msec(),
},
)
self.db_pool.simple_update_txn(
txn,
table="events",
keyvalues={"event_id": event_id},
updatevalues={"rejection_reason": rejection_reason},
)

self.invalidate_get_event_cache_after_txn(txn, event_id)

# TODO(faster_joins): invalidate the cache on workers. Ideally we'd just
# call '_send_invalidation_to_replication', but we actually need the other
# end to call _invalidate_local_event_cache() rather than (just)
squahtx marked this conversation as resolved.
Show resolved Hide resolved
# _get_event_cache.invalidate().
# https://github.com/matrix-org/synapse/issues/12994
richvdh marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions synapse/storage/databases/main/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,11 @@ def _update_state_for_partial_state_event_txn(
updatevalues={"state_group": state_group},
)

# the event may now be rejected where it was not before, or vice versa,
# in which case we need to update the rejected flags.
if bool(context.rejected) != (event.rejected_reason is not None):
self.mark_event_rejected_txn(txn, event.event_id, context.rejected)
Comment on lines +435 to +436
Copy link
Contributor

Choose a reason for hiding this comment

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

Struggling to fully appreciate the scenarios in which this happens, so I'm just going to ask you a question:

If an event becomes rejected (or becomes accepted), do we not need to worry about events which refer to it becoming accepted/rejected themselves?

For example, if I send a join with my invitation as an auth event.
If the invitation is rejected, I take it that my join will also be rejected.

If this invitation later becomes accepted, should that cascade down to my join?

If so, does your code take this into account (where?)?

(...This PR looks much too trivial for what it sounds like.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, absolutely we do need to worry about that.

But when we resync the state, we work through every event in the room (in the order we received them) re-evaluating the state at that event and whether we should have accepted it or not (we call update_state_for_partial_state_event for each one). So I think this should all come out in the wash.


self.db_pool.simple_delete_one_txn(
txn,
table="partial_state_events",
Expand Down
9 changes: 0 additions & 9 deletions synapse/storage/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -539,15 +539,6 @@ def must_await_full_state(self, is_mine_id: Callable[[str], bool]) -> bool:
is_mine_id: a callable which confirms if a given state_key matches a mxid
of a local user
"""

# TODO(faster_joins): it's not entirely clear that this is safe. In particular,
# there may be circumstances in which we return a piece of state that, once we
# resync the state, we discover is invalid. For example: if it turns out that
# the sender of a piece of state wasn't actually in the room, then clearly that
# state shouldn't have been returned.
# We should at least add some tests around this to see what happens.
# https://github.com/matrix-org/synapse/issues/13006

# if we haven't requested membership events, then it depends on the value of
# 'include_others'
if EventTypes.Member not in self.types:
Expand Down