From 7b288826b76cae63b553fced10d3779355b592ca Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Fri, 22 Feb 2019 11:33:51 +0000 Subject: Fix backfill storing incorrect state for events --- synapse/handlers/federation.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 083f2e0ac3..40655bf92c 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -772,8 +772,11 @@ class FederationHandler(BaseHandler): ev_infos = [] for a in auth_events.values(): - if a.event_id in seen_events: + # We only want to persist auth events as outliers that we haven't + # seen and aren't about to persist as part of the backfilled chunk. + if a.event_id in seen_events or a.event_id in event_map: continue + a.internal_metadata.outlier = True ev_infos.append({ "event": a, -- cgit 1.4.1 From 9342cc6ab167836ca3965923989c33f5230c27e9 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 10:02:12 +0000 Subject: Add comments and paranoia --- synapse/handlers/federation.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 40655bf92c..fbf044b407 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -770,6 +770,18 @@ class FederationHandler(BaseHandler): set(auth_events.keys()) | set(state_events.keys()) ) + # We now have a chunk of events plus associated state and auth chain to + # persist. We do the persistence in two steps: + # 1. Auth events and state get persisted as outliers, plus the + # backward extremities get persisted (as non-outliers). + # 2. The rest of the events in the chunk get persisted one by one, as + # each one depends on the previous event for its state. + # + # The important thing is that events in the chunk get persisted as + # non-outliers, including when those events are also in the state or + # auth chain. Caution must therefore be taken to ensure that they are + # not accidentally marked as outliers. + ev_infos = [] for a in auth_events.values(): # We only want to persist auth events as outliers that we haven't @@ -789,13 +801,18 @@ class FederationHandler(BaseHandler): }) for e_id in events_to_state: + # For paranoia we ensure that these events are marked as + # non-outliers + ev = event_map[e_id] + ev.internal_metadata.outlier = False + ev_infos.append({ - "event": event_map[e_id], + "event": ev, "state": events_to_state[e_id], "auth_events": { (auth_events[a_id].type, auth_events[a_id].state_key): auth_events[a_id] - for a_id in event_map[e_id].auth_event_ids() + for a_id in ev.auth_event_ids() if a_id in auth_events } }) @@ -811,6 +828,10 @@ class FederationHandler(BaseHandler): if event in events_to_state: continue + # For paranoia we ensure that these events are marked as + # non-outliers + event.internal_metadata.outlier = False + # We store these one at a time since each event depends on the # previous to work out the state. # TODO: We can probably do something more clever here. -- cgit 1.4.1 From f5050e148c2e6660e7be3d582e7925c1acfb4c02 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 10:02:58 +0000 Subject: Newsfile --- changelog.d/4718.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4718.bugfix diff --git a/changelog.d/4718.bugfix b/changelog.d/4718.bugfix new file mode 100644 index 0000000000..e21300def5 --- /dev/null +++ b/changelog.d/4718.bugfix @@ -0,0 +1 @@ +Fix paginating over federation persisting incorrect state -- cgit 1.4.1 From 890cb048fdbb05bcd8b80d3038f4811a1fdfe9f0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 14:42:11 +0000 Subject: Assert rather than clobber the values --- synapse/handlers/federation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index fbf044b407..8e6d0a3bbc 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -804,7 +804,7 @@ class FederationHandler(BaseHandler): # For paranoia we ensure that these events are marked as # non-outliers ev = event_map[e_id] - ev.internal_metadata.outlier = False + assert(not event.internal_metadata.is_outlier()) ev_infos.append({ "event": ev, @@ -830,7 +830,7 @@ class FederationHandler(BaseHandler): # For paranoia we ensure that these events are marked as # non-outliers - event.internal_metadata.outlier = False + assert(not event.internal_metadata.is_outlier()) # We store these one at a time since each event depends on the # previous to work out the state. -- cgit 1.4.1 From d730c2c22b53bef52afb6b26a878acc186e4ecae Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 14:45:02 +0000 Subject: More comments --- synapse/handlers/federation.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 8e6d0a3bbc..ca9b281be1 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -782,6 +782,7 @@ class FederationHandler(BaseHandler): # auth chain. Caution must therefore be taken to ensure that they are # not accidentally marked as outliers. + # Step 1a: persist auth events that *don't* appear in the chunk ev_infos = [] for a in auth_events.values(): # We only want to persist auth events as outliers that we haven't @@ -800,6 +801,8 @@ class FederationHandler(BaseHandler): } }) + # Step 1b: persist the events in the chunk we fetched state for (i.e. + # the backwards extremities) as non-outliers. for e_id in events_to_state: # For paranoia we ensure that these events are marked as # non-outliers @@ -822,6 +825,7 @@ class FederationHandler(BaseHandler): backfilled=True, ) + # Step 2: Persist the rest of the events in the chunk one by one events.sort(key=lambda e: e.depth) for event in events: -- cgit 1.4.1 From 9c598dddcbd75f60d422164215f3b078def604b2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 16:32:02 +0000 Subject: Fix typo --- synapse/handlers/federation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ca9b281be1..f80486102a 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -807,7 +807,7 @@ class FederationHandler(BaseHandler): # For paranoia we ensure that these events are marked as # non-outliers ev = event_map[e_id] - assert(not event.internal_metadata.is_outlier()) + assert(not ev.internal_metadata.is_outlier()) ev_infos.append({ "event": ev, -- cgit 1.4.1 From 108d5fb20dc90ae292554a33c3a1370050bd3179 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 25 Feb 2019 16:32:19 +0000 Subject: Fixup changelog --- changelog.d/4718.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/4718.bugfix b/changelog.d/4718.bugfix index e21300def5..a7d1963ee1 100644 --- a/changelog.d/4718.bugfix +++ b/changelog.d/4718.bugfix @@ -1 +1 @@ -Fix paginating over federation persisting incorrect state +Fix paginating over federation persisting incorrect state. -- cgit 1.4.1