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

Store rejected remote invite events as outliers #4405

Merged
merged 7 commits into from Jan 24, 2019

Conversation

4 participants
@erikjohnston
Copy link
Member

erikjohnston commented Jan 16, 2019

Currently they're stored as non-outliers even though the server isn't in the room, which can be problematic in places where the code assumes it has the state for all non outlier events.

In particular, there is an edge case where persisting the leave event triggers a state resolution, which requires looking up the room version from state. Since the server doesn't have the state, this causes an exception to be thrown.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #4405 into develop will increase coverage by 1.13%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4405      +/-   ##
===========================================
+ Coverage    73.64%   74.78%   +1.13%     
===========================================
  Files          302      336      +34     
  Lines        29818    33998    +4180     
  Branches      4895     5528     +633     
===========================================
+ Hits         21960    25425    +3465     
- Misses        6426     7007     +581     
- Partials      1432     1566     +134

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 16, 2019

@richvdh
Copy link
Member

richvdh left a comment

this might all be valid, but I'm struggling to see what it has to do with how we store rejections. Could you clarify?

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 21, 2019

this might all be valid, but I'm struggling to see what it has to do with how we store rejections. Could you clarify?

Oh, the terminology here isn't great. These aren't rejected invite events, but leave events where the user has declined the invite to a room

erikjohnston added some commits Jan 16, 2019

Store rejected remote invite events as outliers
Currently they're stored as non-outliers even though the server isn't in
the room, which can be problematic in places where the code assumes it
has the state for all non outlier events.

In particular, there is an edge case where persisting the leave event
triggers a state resolution, which requires looking up the room version
from state. Since the server doesn't have the state, this causes an
exception to be thrown.

@erikjohnston erikjohnston force-pushed the erikj/fixup_rejecting_invites branch from 5408728 to 03ced6c Jan 23, 2019

@erikjohnston erikjohnston force-pushed the erikj/fixup_rejecting_invites branch from 03ced6c to 7c288c2 Jan 23, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 23, 2019

I've now tidied things up a bit, including moving the signing to where we actually create the event, rather than doing a weird resigning long after the fact.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 23, 2019

@erikjohnston erikjohnston added this to To Do in Homeserver Task Board via automation Jan 23, 2019

@erikjohnston erikjohnston moved this from To Do to In progress in Homeserver Task Board Jan 23, 2019

@richvdh
Copy link
Member

richvdh left a comment

There appear to be two completely orthogonal changes here (moving the signing, and setting the new_remote_event flag), but otherwise it looks plausible modulo the below

Show resolved Hide resolved synapse/events/__init__.py Outdated
@@ -571,7 +573,18 @@ def send_request(destination):
if "prev_state" not in pdu_dict:
pdu_dict["prev_state"] = []

ev = builder.EventBuilder(pdu_dict)
# Strip off the fields that we want to clobber.

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

could you add something to the docstring to say that we will do the hashing/signing magic?

Show resolved Hide resolved synapse/handlers/federation.py Outdated
def is_new_remote_event(self):
"""Whether this is a new remote event, like an invite or an invite
rejection. This is needed as those events are marked as outliers, but
they still need to be processed.

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

could you define "processed" a bit better?

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 24, 2019

@richvdh
Copy link
Member

richvdh left a comment

I feel like we could have come up with a less unwieldy term than "out of band membership", but meh, naming is hard. lgtm.

@erikjohnston erikjohnston merged commit 80bcca6 into develop Jan 24, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 24, 2019

I feel like we could have come up with a less unwieldy term than "out of band membership", but meh, naming is hard. lgtm.

Part of me feels like the entire thing with remote invites is a bodge job, which is why its hard to name sensibly.

@hawkowl hawkowl moved this from In progress to Done in Homeserver Task Board Jan 28, 2019

erikjohnston added a commit that referenced this pull request Jan 30, 2019

Fix remote invite rejections not comming down sync
This was broken in PR #4405, commit 886e5ac, where we changed remote
rejections to be outliers.

The fix is to explicitly add the leave event in when we know its an out
of band invite. We can't always add the event as if the server is/was in
the room there might be more events to send down the sync than just the
leave.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment