Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce #17291. #17338

Merged
merged 5 commits into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 14 additions & 6 deletions synapse/storage/databases/main/event_federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@ def __init__(
500000, "_event_auth_cache", size_callback=len
)

# Flag used by unit tests to disable fallback when there is no chain cover
# index.
self.tests_allow_no_chain_cover_index = True

self._clock.looping_call(self._get_stats_for_federation_staging, 30 * 1000)

if isinstance(self.database_engine, PostgresEngine):
Expand Down Expand Up @@ -220,8 +224,10 @@ async def get_auth_chain_ids(
)
except _NoChainCoverIndex:
# For whatever reason we don't actually have a chain cover index
# for the events in question, so we fall back to the old method.
pass
# for the events in question, so we fall back to the old method
# (except in tests)
if not self.tests_allow_no_chain_cover_index:
raise

return await self.db_pool.runInteraction(
"get_auth_chain_ids",
Expand Down Expand Up @@ -271,7 +277,7 @@ def _get_auth_chain_ids_using_cover_index_txn(
if events_missing_chain_info:
# This can happen due to e.g. downgrade/upgrade of the server. We
# raise an exception and fall back to the previous algorithm.
logger.info(
logger.error(
Copy link
Member Author

Choose a reason for hiding this comment

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

We really shouldn't be seeing these anymore I don't think, except while old servers are upgrading (with the background update running)

"Unexpectedly found that events don't have chain IDs in room %s: %s",
room_id,
events_missing_chain_info,
Expand Down Expand Up @@ -482,8 +488,10 @@ async def get_auth_chain_difference(
)
except _NoChainCoverIndex:
# For whatever reason we don't actually have a chain cover index
# for the events in question, so we fall back to the old method.
pass
# for the events in question, so we fall back to the old method
# (except in tests)
if not self.tests_allow_no_chain_cover_index:
raise

return await self.db_pool.runInteraction(
"get_auth_chain_difference",
Expand Down Expand Up @@ -710,7 +718,7 @@ def _fixup_auth_chain_difference_sets(
if events_missing_chain_info - event_to_auth_ids.keys():
# Uh oh, we somehow haven't correctly done the chain cover index,
# bail and fall back to the old method.
logger.info(
logger.error(
"Unexpectedly found that events don't have chain IDs in room %s: %s",
room_id,
events_missing_chain_info - event_to_auth_ids.keys(),
Expand Down
14 changes: 7 additions & 7 deletions synapse/storage/databases/main/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,13 @@ def calculate_chain_cover_index_for_events_txn(
retcols=("room_id", "has_auth_chain_index"),
allow_none=True,
)
if row is None:
if row is None or row[1] is False:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also a bug, though I don't think caused a problem in practice.

return {}

# Filter out already persisted events.
# Filter out events that we've already calculated.
rows = self.db_pool.simple_select_many_txn(
txn,
table="events",
table="event_auth_chains",
Copy link
Member Author

Choose a reason for hiding this comment

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

It makes more sense to check if we have calculated the chain links rather than if we've persisted the event.

column="event_id",
iterable=[e.event_id for e in state_events],
keyvalues={},
Expand All @@ -319,7 +319,7 @@ def calculate_chain_cover_index_for_events_txn(
state_events = [
event
for event in state_events
if event.event_id in already_persisted_events
if event.event_id not in already_persisted_events
Copy link
Member Author

@erikjohnston erikjohnston Jun 20, 2024

Choose a reason for hiding this comment

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

The missing not is the actual bug.

]

if not state_events:
Expand Down Expand Up @@ -600,6 +600,9 @@ def _persist_event_auth_chain_txn(
events: List[EventBase],
new_event_links: Dict[str, NewEventChainLinks],
) -> None:
if new_event_links:
self._persist_chain_cover_index(txn, self.db_pool, new_event_links)

# We only care about state events, so this if there are no state events.
if not any(e.is_state() for e in events):
return
Expand All @@ -622,9 +625,6 @@ def _persist_event_auth_chain_txn(
],
)

if new_event_links:
self._persist_chain_cover_index(txn, self.db_pool, new_event_links)

@classmethod
def _add_chain_cover_index(
cls,
Expand Down
2 changes: 2 additions & 0 deletions tests/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,8 @@ def setUp(self) -> None:
self._hs_args = {"clock": self.clock, "reactor": self.reactor}
self.hs = self.make_homeserver(self.reactor, self.clock)

self.hs.get_datastores().main.tests_allow_no_chain_cover_index = False

# Honour the `use_frozen_dicts` config option. We have to do this
# manually because this is taken care of in the app `start` code, which
# we don't run. Plus we want to reset it on tearDown.
Expand Down
Loading