Don't drop user dir deltas when server leaves room (#10982)

Fix a long-standing bug where a batch of user directory changes would be
silently dropped if the server left a room early in the batch.

* Pull out `wait_for_background_update` in tests

Co-authored-by: Patrick Cloke <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>

Author: 3 people

changelog.d/10982.bugfix

@@ -0,0 +1 @@
+Fix a long-standing bug where the remainder of a batch of user directory changes would be silently dropped if the server left a room early in the batch.

synapse/handlers/user_directory.py

@@ -220,7 +220,7 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None:
 
                         for user_id in user_ids:
                             await self._handle_remove_user(room_id, user_id)
-                        return
+                        continue
                     else:
                         logger.debug("Server is still in room: %r", room_id)
 

tests/handlers/test_stats.py

@@ -103,12 +103,7 @@ def _perform_background_initial_update(self):
         # Do the initial population of the stats via the background update
         self._add_background_updates()
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
     def test_initial_room(self):
         """
@@ -140,12 +135,7 @@ def test_initial_room(self):
         # Do the initial population of the user directory via the background update
         self._add_background_updates()
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         r = self.get_success(self.get_all_room_state())
 
@@ -568,12 +558,7 @@ def test_incomplete_stats(self):
             )
         )
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         r1stats_complete = self._get_current_stats("room", r1)
         u1stats_complete = self._get_current_stats("user", u1)

tests/handlers/test_user_directory.py

@@ -363,6 +363,45 @@ def test_reactivation_makes_regular_user_searchable(self) -> None:
         self.assertEqual(len(s["results"]), 1)
         self.assertEqual(s["results"][0]["user_id"], user)
 
+    def test_process_join_after_server_leaves_room(self) -> None:
+        alice = self.register_user("alice", "pass")
+        alice_token = self.login(alice, "pass")
+        bob = self.register_user("bob", "pass")
+        bob_token = self.login(bob, "pass")
+
+        # Alice makes two rooms. Bob joins one of them.
+        room1 = self.helper.create_room_as(alice, tok=alice_token)
+        room2 = self.helper.create_room_as(alice, tok=alice_token)
+        print("room1=", room1)
+        print("room2=", room2)
+        self.helper.join(room1, bob, tok=bob_token)
+
+        # The user sharing tables should have been updated.
+        public1 = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
+        self.assertEqual(set(public1), {(alice, room1), (alice, room2), (bob, room1)})
+
+        # Alice leaves room1. The user sharing tables should be updated.
+        self.helper.leave(room1, alice, tok=alice_token)
+        public2 = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
+        self.assertEqual(set(public2), {(alice, room2), (bob, room1)})
+
+        # Pause the processing of new events.
+        dir_handler = self.hs.get_user_directory_handler()
+        dir_handler.update_user_directory = False
+
+        # Bob leaves one room and joins the other.
+        self.helper.leave(room1, bob, tok=bob_token)
+        self.helper.join(room2, bob, tok=bob_token)
+
+        # Process the leave and join in one go.
+        dir_handler.update_user_directory = True
+        dir_handler.notify_new_event()
+        self.wait_for_background_updates()
+
+        # The user sharing tables should have been updated.
+        public3 = self.get_success(self.user_dir_helper.get_users_in_public_rooms())
+        self.assertEqual(set(public3), {(alice, room2), (bob, room2)})
+
     def test_private_room(self) -> None:
         """
         A user can be searched for only by people that are either in a public

tests/storage/databases/main/test_room.py

@@ -79,12 +79,7 @@ def test_background_populate_rooms_creator_column(self):
         self.store.db_pool.updates._all_done = False
 
         # Now let's actually drive the updates to completion
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         # Make sure the background update filled in the room creator
         room_creator_after = self.get_success(

tests/storage/test_cleanup_extrems.py

@@ -66,12 +66,7 @@ def run_delta_file(txn):
         # Ugh, have to reset this flag
         self.store.db_pool.updates._all_done = False
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
     def test_soft_failed_extremities_handled_correctly(self):
         """Test that extremities are correctly calculated in the presence of

tests/storage/test_client_ips.py

@@ -242,12 +242,7 @@ def test_updating_monthly_active_user_when_space(self):
 
     def test_devices_last_seen_bg_update(self):
         # First make sure we have completed all updates.
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         user_id = "@user:id"
         device_id = "MY_DEVICE"
@@ -311,12 +306,7 @@ def test_devices_last_seen_bg_update(self):
         self.store.db_pool.updates._all_done = False
 
         # Now let's actually drive the updates to completion
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         # We should now get the correct result again
         result = self.get_success(
@@ -337,12 +327,7 @@ def test_devices_last_seen_bg_update(self):
 
     def test_old_user_ips_pruned(self):
         # First make sure we have completed all updates.
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         user_id = "@user:id"
         device_id = "MY_DEVICE"

tests/storage/test_event_chain.py

@@ -578,12 +578,7 @@ def test_background_update_single_room(self):
         # Ugh, have to reset this flag
         self.store.db_pool.updates._all_done = False
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         # Test that the `has_auth_chain_index` has been set
         self.assertTrue(self.get_success(self.store.has_auth_chain_index(room_id)))
@@ -619,12 +614,7 @@ def test_background_update_multiple_rooms(self):
         # Ugh, have to reset this flag
         self.store.db_pool.updates._all_done = False
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         # Test that the `has_auth_chain_index` has been set
         self.assertTrue(self.get_success(self.store.has_auth_chain_index(room_id1)))

tests/storage/test_roommember.py

@@ -169,12 +169,7 @@ def prepare(self, reactor, clock, homeserver):
 
     def test_can_rerun_update(self):
         # First make sure we have completed all updates.
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
         # Now let's create a room, which will insert a membership
         user = UserID("alice", "test")
@@ -197,9 +192,4 @@ def test_can_rerun_update(self):
         self.store.db_pool.updates._all_done = False
 
         # Now let's actually drive the updates to completion
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()

tests/storage/test_user_directory.py

@@ -212,12 +212,7 @@ def _purge_and_rebuild_user_dir(self) -> None:
             )
         )
 
-        while not self.get_success(
-            self.store.db_pool.updates.has_completed_background_updates()
-        ):
-            self.get_success(
-                self.store.db_pool.updates.do_next_background_update(100), by=0.1
-            )
+        self.wait_for_background_updates()
 
     def test_initial(self) -> None:
         """

tests/unittest.py

@@ -317,6 +317,15 @@ def wait_on_thread(self, deferred, timeout=10):
             self.reactor.advance(0.01)
             time.sleep(0.01)
 
+    def wait_for_background_updates(self) -> None:
+        """Block until all background database updates have completed."""
+        while not self.get_success(
+            self.store.db_pool.updates.has_completed_background_updates()
+        ):
+            self.get_success(
+                self.store.db_pool.updates.do_next_background_update(100), by=0.1
+            )
+
     def make_homeserver(self, reactor, clock):
         """
         Make and return a homeserver.