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

WIP: Dmr/unblock catchup #15228

Closed
wants to merge 8 commits into from
Closed

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Mar 8, 2023

Intended to fix #15220.

Follows from #14404.

Will be commitwise reviewable.

@DMRobertson DMRobertson changed the base branch from develop to release-v1.79 March 8, 2023 19:26
# ASSERT
# We should have:
# - not sent event 3: it's not ours, and the room is partial stated
# - fallen back to sending event 2: it's the most recent event in the room
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a little dodgy. In this situation, host2 is in the room prior to event_2 being created, so it's correct that we send event_2 as the catchup event.

BUT: suppose that the order of event was

  • event 2
  • host2 joins
  • event 3

then host 2 wouldn't be privvy to event 2. I think we'll either

  • block awaitng event2's full state, or
  • incorrectly send event 2 to host2

I need to probably make another test case and stare at this some more.

@@ -643,14 +643,13 @@ def check_event_is_visible(
# this check but would base the filtering on an outdated view of the membership events.

partial_state_invisible_events = set()
if filter_out_erased_senders:
Copy link
Contributor Author

@DMRobertson DMRobertson Mar 8, 2023

Choose a reason for hiding this comment

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

Am I missing something here? Why was the original logic doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have put some comments regarding that in the original PR.
We perhaps want to use a dedicated boolean like filter_partial_state for this (and keep your rename to filter_out_erased_senders for the rest of the code).
I think the logic I had in mind is that we don't care about having non local events when checking if we should use an event as an extremity for backfill: the remote server would then filter on his side anyway what need to be filtered, with a (potentially) a more current view than the joining server.

@MatMaul
Copy link
Contributor

MatMaul commented Mar 9, 2023

For people coming here after me, Avoid state lookups for events we'll ignore is the important commit.

@@ -631,44 +652,49 @@ def check_event_is_visible(
# otherwise a room could be fully joined after we retrieve those, which would then bypass
# this check but would base the filtering on an outdated view of the membership events.

partial_state_invisible_events = set()
if not check_history_visibility_only:
Copy link
Contributor

@MatMaul MatMaul Mar 9, 2023

Choose a reason for hiding this comment

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

According to some failing tests we need to keep a way to shortcut this code for the backfill use case.
In this case this function is called with backward extremities we want to use as a base for backfilling to check each extremity in turn and ignore those which users on our server wouldn't be able to see.
So I think there is no way here that we will leak events we shouldn't, so skipping this should be fine.
Why does it make the tests fail ? I haven't looked deep enough yet to understand.

@DMRobertson
Copy link
Contributor Author

I'm going to spin out the refactor parts of this in #15240.

@DMRobertson
Copy link
Contributor Author

I'm going to spin out the refactor parts of this in #15240.

And the remaining part is #15241.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Federation catchup for a destination can block indefinitely on un-partial stating
2 participants