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

Update the rejected state of events during resync #13459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
505be78
Update type of `EventContext.rejected`
richvdh Aug 4, 2022
b165003
Update the rejected state of events during resync
richvdh Aug 4, 2022
cf0d5d8
Merge branch 'develop' into rav/bad_events_in_faster_join
richvdh Aug 4, 2022
d4fd08e
Update synapse/storage/databases/main/events_worker.py
richvdh Aug 11, 2022
d5f8cc9
lint
richvdh Aug 11, 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/13459.misc
View file
Open in desktop
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.
60 changes: 60 additions & 0 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 @@ -2200,3 +2200,63 @@ 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_get_event_cache() rather than (just)
# _get_event_cache.invalidate().
#
# One solution might be to (somehow) get the workers to call
# _invalidate_caches_for_event() (though that will invalidate more than
# strictly necessary).
#
# https:/matrix-org/synapse/issues/12994
5 changes: 5 additions & 0 deletions synapse/storage/databases/main/state.py
View file
Open in desktop
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
View file
Open in desktop
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:/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