Add a migration step to cleanup potential leftovers of bug 11833

[MatMaul]

I haven't include a changelog since it's a follow up to a fixed bug, but can do if we prefer to do so.

[clokep]

I haven't include a changelog since it's a follow up to a fixed bug, but can do if we prefer to do so.

If you use the same changelog text (but under a file with the new PR number) they'll get merged by towncrier at release time!

[MatMaul]

Oh good to know thanks ! updated.

This is currently untested manually. Do we have a test running the migration or we rely on manual testing for that ?

[squahtx]

I think we test that migrations run without error somewhere(?). Apart from that, we test their correctness with unit tests or manually.

There's an example unit test for a destructive background update here:

synapse/tests/handlers/test_deactivate_account.py

Lines 220 to 234 in 33ebee4

	def _rerun_retroactive_account_data_deletion_update(self) -> None:
	# Reset the 'all done' flag
	self._store.db_pool.updates._all_done = False
	self.get_success(
	self._store.db_pool.simple_insert(
	"background_updates",
	{
	"update_name": "delete_account_data_for_deactivated_users",
	"progress_json": "{}",
	},
	)
	)
	self.wait_for_background_updates()

synapse/tests/handlers/test_deactivate_account.py

Lines 303 to 324 in 33ebee4

	# Check that the account data is there.
	self.assertIsNotNone(
	self.get_success(
	self._store.get_global_account_data_by_type_for_user(
	self.user,
	AccountDataTypes.DIRECT,
	)
	),
	)
	# Re-run the retroactive deletion update
	self._rerun_retroactive_account_data_deletion_update()
	# Check that the account data was NOT cleared.
	self.assertIsNotNone(
	self.get_success(
	self._store.get_global_account_data_by_type_for_user(
	self.user,
	AccountDataTypes.DIRECT,
	)
	),
	)

We have tests for both deletions and non-deletions for that one.

I think it's best to do at least some testing of new background updates, either manual or automated.

[squahtx]

lgtm!

I think the worst that could happen if the background update goes wrong is that Synapse has to backfill events from other homeservers?

[MatMaul]

I am planning to test this manually by:

• launch a synapse 1.58
• join a room
• add a fake row to this table pointing to an event in the joined room
• purge the room
• launch a synapse dev
• check that the table is empty

I am happy to do that in an automated way if the test framework is able to handle something like that but I guess it can't :)

[richvdh]

for links: related to issue #11833. follow-up to #12770.

@MatMaul: please add links between issues whereever relevant to help future archaeologists!

[MatMaul]

Indeed, will do proper # referencing next time, the bug number in the commit msg is not enough for GH to pick it up :)

[MatMaul]

I am planning to test this manually by:
...

This testing plan finally happened, and went as expected, I think we are ready to merge.