From ec767e260202a87479538a99011acd9106335e4e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 1 Sep 2021 09:38:56 +0100 Subject: [PATCH 1/4] Fix iteration over row results We were originally iterating over 'txn', which can work, but as 'txn' was being used to run a simple_delete operation inside the loop, it was actually being redefined mid-loop. Thus, this caused the background job that was intended to remove old pushers that were associated with an unlinked email address to fail if there were any to remove. --- synapse/storage/databases/main/pusher.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/pusher.py b/synapse/storage/databases/main/pusher.py index e47caa212549..63ac09c61dad 100644 --- a/synapse/storage/databases/main/pusher.py +++ b/synapse/storage/databases/main/pusher.py @@ -430,10 +430,11 @@ def _delete_pushers(txn) -> int: """ txn.execute(sql, (last_pusher, batch_size)) + rows = txn.fetchall() last = None num_deleted = 0 - for row in txn: + for row in rows: last = row[0] num_deleted += 1 self.db_pool.simple_delete_txn( From e219d4b9174dc70d4939620e1108ddf868ce09ef Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 1 Sep 2021 09:39:36 +0100 Subject: [PATCH 2/4] Add a test that runs the background job with an unlinked pusher --- tests/push/test_email.py | 50 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index eea07485a017..ccc73f93ce09 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -67,7 +67,7 @@ def make_homeserver(self, reactor, clock): config["public_baseurl"] = "aaa" config["start_pushers"] = True - hs = self.setup_test_homeserver(config=config) + self.hs = self.setup_test_homeserver(config=config) # List[Tuple[Deferred, args, kwargs]] self.email_attempts = [] @@ -77,9 +77,9 @@ def sendmail(*args, **kwargs): self.email_attempts.append((d, args, kwargs)) return d - hs.get_send_email_handler()._sendmail = sendmail + self.hs.get_send_email_handler()._sendmail = sendmail - return hs + return self.hs def prepare(self, reactor, clock, hs): # Register the user who gets notified @@ -344,6 +344,50 @@ def test_no_email_sent_after_removed(self): pushers = list(pushers) self.assertEqual(len(pushers), 0) + def test_remove_unlinked_pushers_background_job(self): + """Checks that all existing pushers associated with unlinked email addresses are removed + upon running the remove_deleted_email_pushers background update. + """ + # disassociate the user's email address manually (without deleting the pusher). + # This resembles the old behaviour, which the background update below is intended + # to clean up. + self.get_success( + self.hs.get_datastore().user_delete_threepid( + self.user_id, "email", "a@example.com" + ) + ) + + # Run the "remove_deleted_email_pushers" background job + self.get_success( + self.hs.get_datastore().db_pool.simple_insert( + table="background_updates", + values={ + "update_name": "remove_deleted_email_pushers", + "progress_json": "{}", + "depends_on": None, + }, + ) + ) + + # ... and tell the DataStore that it hasn't finished all updates yet + self.hs.get_datastore().db_pool.updates._all_done = False + + # Now let's actually drive the updates to completion + while not self.get_success( + self.hs.get_datastore().db_pool.updates.has_completed_background_updates() + ): + self.get_success( + self.hs.get_datastore().db_pool.updates.do_next_background_update(100), + by=0.1, + ) + + # Check that all pushers with unlinked addresses were deleted + pushers = self.get_success( + self.hs.get_datastore().get_pushers_by({"user_name": self.user_id}) + ) + pushers = list(pushers) + self.assertEqual(len(pushers), 0) + def _check_for_mail(self): """Check that the user receives an email notification""" From ec1bccbf44179855d05551bedf0996ceb51bda9d Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 1 Sep 2021 09:47:33 +0100 Subject: [PATCH 3/4] Changelog --- changelog.d/10734.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10734.bugfix diff --git a/changelog.d/10734.bugfix b/changelog.d/10734.bugfix new file mode 100644 index 000000000000..15c7da449734 --- /dev/null +++ b/changelog.d/10734.bugfix @@ -0,0 +1 @@ +Remove pushers when deleting a 3pid from an account. Pushers for old unlinked emails will also be deleted. \ No newline at end of file From b1ee2167ce94f3b52ce0068ebc16134dc2fd0e1c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Wed, 1 Sep 2021 09:49:35 +0100 Subject: [PATCH 4/4] self.hs -> hs --- tests/push/test_email.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/push/test_email.py b/tests/push/test_email.py index ccc73f93ce09..c4ba13a6b270 100644 --- a/tests/push/test_email.py +++ b/tests/push/test_email.py @@ -67,7 +67,7 @@ def make_homeserver(self, reactor, clock): config["public_baseurl"] = "aaa" config["start_pushers"] = True - self.hs = self.setup_test_homeserver(config=config) + hs = self.setup_test_homeserver(config=config) # List[Tuple[Deferred, args, kwargs]] self.email_attempts = [] @@ -77,9 +77,9 @@ def sendmail(*args, **kwargs): self.email_attempts.append((d, args, kwargs)) return d - self.hs.get_send_email_handler()._sendmail = sendmail + hs.get_send_email_handler()._sendmail = sendmail - return self.hs + return hs def prepare(self, reactor, clock, hs): # Register the user who gets notified