From b596a1eb80be4b24142d131238dd5402db6bb855 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 11 Nov 2021 16:01:13 +0100 Subject: [PATCH 1/6] Move sql file for `remove_deleted_devices_from_device_inbox` into v65 (#11303) --- changelog.d/11303.misc | 1 + .../05remove_deleted_devices_from_device_inbox.sql} | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11303.misc rename synapse/storage/schema/main/delta/{64/02remove_deleted_devices_from_device_inbox.sql => 65/05remove_deleted_devices_from_device_inbox.sql} (93%) diff --git a/changelog.d/11303.misc b/changelog.d/11303.misc new file mode 100644 index 0000000000..50af92bfa5 --- /dev/null +++ b/changelog.d/11303.misc @@ -0,0 +1 @@ +Fix an issue which prevented the 'remove deleted devices from device_inbox column' background process from running when updating from a recent Synapse version. diff --git a/synapse/storage/schema/main/delta/64/02remove_deleted_devices_from_device_inbox.sql b/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql similarity index 93% rename from synapse/storage/schema/main/delta/64/02remove_deleted_devices_from_device_inbox.sql rename to synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql index fca7290741..076179123d 100644 --- a/synapse/storage/schema/main/delta/64/02remove_deleted_devices_from_device_inbox.sql +++ b/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql @@ -19,4 +19,4 @@ -- This runs as background task, but may take a bit to finish. INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (6402, 'remove_deleted_devices_from_device_inbox', '{}'); + (6505, 'remove_deleted_devices_from_device_inbox', '{}'); From 9c59e117db6b448a1e930365014b043fa7ef26b6 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 15 Nov 2021 17:34:15 +0000 Subject: [PATCH 2/6] Run _upgrade_existing_database on workers if at current schema_version (#11346) Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/11346.bugfix | 1 + synapse/storage/prepare_database.py | 40 +++++++++++---------- tests/storage/test_rollback_worker.py | 52 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 19 deletions(-) create mode 100644 changelog.d/11346.bugfix diff --git a/changelog.d/11346.bugfix b/changelog.d/11346.bugfix new file mode 100644 index 0000000000..1fe8020eab --- /dev/null +++ b/changelog.d/11346.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.47.0rc1 which caused worker processes to not halt startup in the presence of outstanding database migrations. \ No newline at end of file diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py index 8b9c6adae2..e45adfcb55 100644 --- a/synapse/storage/prepare_database.py +++ b/synapse/storage/prepare_database.py @@ -131,24 +131,16 @@ def prepare_database( "config==None in prepare_database, but database is not empty" ) - # if it's a worker app, refuse to upgrade the database, to avoid multiple - # workers doing it at once. - if config.worker.worker_app is None: - _upgrade_existing_database( - cur, - version_info, - database_engine, - config, - databases=databases, - ) - elif version_info.current_version < SCHEMA_VERSION: - # If the DB is on an older version than we expect then we refuse - # to start the worker (as the main process needs to run first to - # update the schema). - raise UpgradeDatabaseException( - OUTDATED_SCHEMA_ON_WORKER_ERROR - % (SCHEMA_VERSION, version_info.current_version) - ) + # This should be run on all processes, master or worker. The master will + # apply the deltas, while workers will check if any outstanding deltas + # exist and raise an PrepareDatabaseException if they do. + _upgrade_existing_database( + cur, + version_info, + database_engine, + config, + databases=databases, + ) else: logger.info("%r: Initialising new database", databases) @@ -358,6 +350,18 @@ def _upgrade_existing_database( is_worker = config and config.worker.worker_app is not None + # If the schema version needs to be updated, and we are on a worker, we immediately + # know to bail out as workers cannot update the database schema. Only one process + # must update the database at the time, therefore we delegate this task to the master. + if is_worker and current_schema_state.current_version < SCHEMA_VERSION: + # If the DB is on an older version than we expect then we refuse + # to start the worker (as the main process needs to run first to + # update the schema). + raise UpgradeDatabaseException( + OUTDATED_SCHEMA_ON_WORKER_ERROR + % (SCHEMA_VERSION, current_schema_state.current_version) + ) + if ( current_schema_state.compat_version is not None and current_schema_state.compat_version > SCHEMA_VERSION diff --git a/tests/storage/test_rollback_worker.py b/tests/storage/test_rollback_worker.py index a6be9a1bb1..0ce0892165 100644 --- a/tests/storage/test_rollback_worker.py +++ b/tests/storage/test_rollback_worker.py @@ -11,6 +11,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from typing import List +from unittest import mock + from synapse.app.generic_worker import GenericWorkerServer from synapse.storage.database import LoggingDatabaseConnection from synapse.storage.prepare_database import PrepareDatabaseException, prepare_database @@ -19,6 +22,22 @@ from synapse.storage.schema import SCHEMA_VERSION from tests.unittest import HomeserverTestCase +def fake_listdir(filepath: str) -> List[str]: + """ + A fake implementation of os.listdir which we can use to mock out the filesystem. + + Args: + filepath: The directory to list files for. + + Returns: + A list of files and folders in the directory. + """ + if filepath.endswith("full_schemas"): + return [SCHEMA_VERSION] + + return ["99_add_unicorn_to_database.sql"] + + class WorkerSchemaTests(HomeserverTestCase): def make_homeserver(self, reactor, clock): hs = self.setup_test_homeserver( @@ -51,7 +70,7 @@ class WorkerSchemaTests(HomeserverTestCase): prepare_database(db_conn, db_pool.engine, self.hs.config) - def test_not_upgraded(self): + def test_not_upgraded_old_schema_version(self): """Test that workers don't start if the DB has an older schema version""" db_pool = self.hs.get_datastore().db_pool db_conn = LoggingDatabaseConnection( @@ -67,3 +86,34 @@ class WorkerSchemaTests(HomeserverTestCase): with self.assertRaises(PrepareDatabaseException): prepare_database(db_conn, db_pool.engine, self.hs.config) + + def test_not_upgraded_current_schema_version_with_outstanding_deltas(self): + """ + Test that workers don't start if the DB is on the current schema version, + but there are still outstanding delta migrations to run. + """ + db_pool = self.hs.get_datastore().db_pool + db_conn = LoggingDatabaseConnection( + db_pool._db_pool.connect(), + db_pool.engine, + "tests", + ) + + # Set the schema version of the database to the current version + cur = db_conn.cursor() + cur.execute("UPDATE schema_version SET version = ?", (SCHEMA_VERSION,)) + + db_conn.commit() + + # Path `os.listdir` here to make synapse think that there is a migration + # file ready to be run. + # Note that we can't patch this function for the whole method, else Synapse + # will try to find the file when building the database initially. + with mock.patch("os.listdir", mock.Mock(side_effect=fake_listdir)): + with self.assertRaises(PrepareDatabaseException): + # Synapse should think that there is an outstanding migration file due to + # patching 'os.listdir' in the function decorator. + # + # We expect Synapse to raise an exception to indicate the master process + # needs to apply this migration file. + prepare_database(db_conn, db_pool.engine, self.hs.config) From 6e084b62b88cf3d18646a036b7079c8a899349ab Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Tue, 16 Nov 2021 13:16:43 +0000 Subject: [PATCH 3/6] Rename `remove_deleted_devices_from_device_inbox` to ensure it is always run (#11353) Co-authored-by: reivilibre --- changelog.d/11353.misc | 1 + ...06remove_deleted_devices_from_device_inbox.sql} | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11353.misc rename synapse/storage/schema/main/delta/65/{05remove_deleted_devices_from_device_inbox.sql => 06remove_deleted_devices_from_device_inbox.sql} (53%) diff --git a/changelog.d/11353.misc b/changelog.d/11353.misc new file mode 100644 index 0000000000..fa96dae919 --- /dev/null +++ b/changelog.d/11353.misc @@ -0,0 +1 @@ +Fix an issue which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. \ No newline at end of file diff --git a/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql similarity index 53% rename from synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql rename to synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql index 076179123d..82f6408b36 100644 --- a/synapse/storage/schema/main/delta/65/05remove_deleted_devices_from_device_inbox.sql +++ b/synapse/storage/schema/main/delta/65/06remove_deleted_devices_from_device_inbox.sql @@ -18,5 +18,17 @@ -- when a device was deleted using Synapse earlier than 1.47.0. -- This runs as background task, but may take a bit to finish. +-- Remove any existing instances of this job running. It's OK to stop and restart this job, +-- as it's just deleting entries from a table - no progress will be lost. +-- +-- This is necessary due a similar migration running the job accidentally +-- being included in schema version 64 during v1.47.0rc1,rc2. If a +-- homeserver had updated from Synapse <=v1.45.0 (schema version <=64), +-- then they would have started running this background update already. +-- If that update was still running, then simply inserting it again would +-- cause an SQL failure. So we effectively do an "upsert" here instead. + +DELETE FROM background_updates WHERE update_name = 'remove_deleted_devices_from_device_inbox'; + INSERT INTO background_updates (ordering, update_name, progress_json) VALUES - (6505, 'remove_deleted_devices_from_device_inbox', '{}'); + (6506, 'remove_deleted_devices_from_device_inbox', '{}'); From edcdc5fd82ccdf3862d811f95b3c93abad8e8578 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 14:34:46 +0000 Subject: [PATCH 4/6] 1.47.0rc3 --- CHANGES.md | 15 +++++++++++++++ changelog.d/11303.misc | 1 - changelog.d/11346.bugfix | 1 - changelog.d/11353.misc | 1 - debian/changelog | 6 ++++++ synapse/__init__.py | 2 +- 6 files changed, 22 insertions(+), 4 deletions(-) delete mode 100644 changelog.d/11303.misc delete mode 100644 changelog.d/11346.bugfix delete mode 100644 changelog.d/11353.misc diff --git a/CHANGES.md b/CHANGES.md index a188bd3f4d..8b11cccc1a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,18 @@ +Synapse 1.47.0rc3 (2021-11-16) +============================== + +Bugfixes +-------- + +- Fix a bug introduced in 1.47.0rc1 which caused worker processes to not halt startup in the presence of outstanding database migrations. ([\#11346](https://github.com/matrix-org/synapse/issues/11346)) + + +Internal Changes +---------------- + +- Fix an issue which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. ([\#11303](https://github.com/matrix-org/synapse/issues/11303), [\#11353](https://github.com/matrix-org/synapse/issues/11353)) + + Synapse 1.47.0rc2 (2021-11-10) ============================== diff --git a/changelog.d/11303.misc b/changelog.d/11303.misc deleted file mode 100644 index 50af92bfa5..0000000000 --- a/changelog.d/11303.misc +++ /dev/null @@ -1 +0,0 @@ -Fix an issue which prevented the 'remove deleted devices from device_inbox column' background process from running when updating from a recent Synapse version. diff --git a/changelog.d/11346.bugfix b/changelog.d/11346.bugfix deleted file mode 100644 index 1fe8020eab..0000000000 --- a/changelog.d/11346.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix a bug introduced in v1.47.0rc1 which caused worker processes to not halt startup in the presence of outstanding database migrations. \ No newline at end of file diff --git a/changelog.d/11353.misc b/changelog.d/11353.misc deleted file mode 100644 index fa96dae919..0000000000 --- a/changelog.d/11353.misc +++ /dev/null @@ -1 +0,0 @@ -Fix an issue which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. \ No newline at end of file diff --git a/debian/changelog b/debian/changelog index b3ebfb84c7..2acd0de3f5 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +matrix-synapse-py3 (1.47.0~rc3) stable; urgency=medium + + * New synapse release 1.47.0~rc3. + + -- Synapse Packaging team Tue, 16 Nov 2021 14:32:47 +0000 + matrix-synapse-py3 (1.47.0~rc2) stable; urgency=medium [ Dan Callahan ] diff --git a/synapse/__init__.py b/synapse/__init__.py index 06b179a7e8..3b5878b912 100644 --- a/synapse/__init__.py +++ b/synapse/__init__.py @@ -47,7 +47,7 @@ try: except ImportError: pass -__version__ = "1.47.0rc2" +__version__ = "1.47.0rc3" if bool(os.environ.get("SYNAPSE_TEST_PATCH_LOG_CONTEXTS", False)): # We import here so that we don't have to install a bunch of deps when From 729acd82c86d2b705fee34d0e74ca8215b5b7658 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 14:40:54 +0000 Subject: [PATCH 5/6] mark the migration file migration as a bug --- CHANGES.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 8b11cccc1a..f528f561c7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,12 +5,7 @@ Bugfixes -------- - Fix a bug introduced in 1.47.0rc1 which caused worker processes to not halt startup in the presence of outstanding database migrations. ([\#11346](https://github.com/matrix-org/synapse/issues/11346)) - - -Internal Changes ----------------- - -- Fix an issue which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. ([\#11303](https://github.com/matrix-org/synapse/issues/11303), [\#11353](https://github.com/matrix-org/synapse/issues/11353)) +- Fix a bug in v1.47.0rc1 and rc2 which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. ([\#11303](https://github.com/matrix-org/synapse/issues/11303), [\#11353](https://github.com/matrix-org/synapse/issues/11353)) Synapse 1.47.0rc2 (2021-11-10) From 7baa671dc821327cb28eb3eb01ecbe65e5ae4926 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 16 Nov 2021 14:42:21 +0000 Subject: [PATCH 6/6] fix up changelog language --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index f528f561c7..d71ab4a9d5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,7 +5,7 @@ Bugfixes -------- - Fix a bug introduced in 1.47.0rc1 which caused worker processes to not halt startup in the presence of outstanding database migrations. ([\#11346](https://github.com/matrix-org/synapse/issues/11346)) -- Fix a bug in v1.47.0rc1 and rc2 which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. ([\#11303](https://github.com/matrix-org/synapse/issues/11303), [\#11353](https://github.com/matrix-org/synapse/issues/11353)) +- Fix a bug introduced in 1.47.0rc1 which prevented the 'remove deleted devices from `device_inbox` column' background process from running when updating from a recent Synapse version. ([\#11303](https://github.com/matrix-org/synapse/issues/11303), [\#11353](https://github.com/matrix-org/synapse/issues/11353)) Synapse 1.47.0rc2 (2021-11-10)