Skip to content

Move start_doing_background_updates() to HomeServer.start_background_tasks() #19057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from
Draft

Move start_doing_background_updates() to HomeServer.start_background_tasks() #19057

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4cf90e1
Move `start_doing_background_updates()` to `HomeServer.start_backgrou…
MadLittleMods Oct 13, 2025
78fd66c
Add changelog
MadLittleMods Oct 13, 2025
d1ed34a
Fix logcontext problems
MadLittleMods Oct 13, 2025
01343d6
Remove duplicate database background update running
MadLittleMods Oct 13, 2025
2fcd70d
Add future TODO
MadLittleMods Oct 13, 2025
74c5e64
Fix lints
MadLittleMods Oct 13, 2025
c1d89d8
Remove assumption
MadLittleMods Oct 13, 2025
064b30b
Wait for homeserver to drive database background updates
MadLittleMods Oct 13, 2025
5e87f89
Better context for error
MadLittleMods Oct 13, 2025
edf9ef0
Comment about slight naivety
MadLittleMods Oct 13, 2025
e14e5d3
Fix lints
MadLittleMods Oct 13, 2025
19b181b
Fix logcontext problems in `tests/util/test_task_scheduler.py`
MadLittleMods Oct 13, 2025
b4d1789
Better conversion
MadLittleMods Oct 13, 2025
6a11d64
Even better adaption `incomplete_d`
MadLittleMods Oct 13, 2025
4f4bf80
Remove `deferLater from other tests
MadLittleMods Oct 13, 2025
53fd1b2
Remove mentions of `deferLater`
MadLittleMods Oct 13, 2025
9e99547
Fix lints
MadLittleMods Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.d/19036.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Move `start_doing_background_updates()` to `SynapseHomeServer.start_background_tasks()`.
Move `start_doing_background_updates()` to `HomeServer.start_background_tasks()`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the changelog from #19036 to merge with this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PresenceHandlerInitTestCase failures ❌

PresenceHandlerInitTestCase purposely tries to disable background tasks to keep PresenceHandler uninitialized until the test is prepared. It tries to test that "presence state restored from the database [when PresenceHandler is initialized] should not persist forever".

This conflicts with the changes currently being made where we expect the background tasks to run because we want the database background tasks to complete before we start the tests.

synapse/tests/handlers/test_presence.py

Lines 811 to 817 in ec7554b

class PresenceHandlerInitTestCase(unittest.HomeserverTestCase):
def default_config(self) -> JsonDict:
config = super().default_config()
# Disable background tasks on this worker so that the PresenceHandler isn't
# loaded until we request it.
config["run_background_tasks_on"] = "other"
return config

Example failures:

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_presence.PresenceHandlerInitTestCase
The "poetry.dev-dependencies" section is deprecated and will be removed in a future version. Use "poetry.group.dev.dependencies" instead.
tests.handlers.test_presence
  PresenceHandlerInitTestCase
    test_restored_presence_idles ...                                    [ERROR]
    test_restored_presence_online_after_sync_0_org_matrix_msc3026_busy ... [ERROR]
    test_restored_presence_online_after_sync_1_online ...               [ERROR]
    test_restored_presence_online_after_sync_2_unavailable ...          [ERROR]
    test_restored_presence_online_after_sync_3_offline ...              [ERROR]

===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/home/eric/Documents/github/element/synapse/tests/unittest.py", line 138, in new
    return code(orig, *args, **kwargs)
  File "/home/eric/Documents/github/element/synapse/tests/unittest.py", line 232, in setUp
    return orig()
  File "/home/eric/Documents/github/element/synapse/tests/unittest.py", line 404, in setUp
    self.hs = self.make_homeserver(self.reactor, self.clock)
  File "/home/eric/Documents/github/element/synapse/tests/unittest.py", line 525, in make_homeserver
    hs = self.setup_test_homeserver(reactor=reactor, clock=clock)
  File "/home/eric/Documents/github/element/synapse/tests/unittest.py", line 733, in setup_test_homeserver
    raise AssertionError(
builtins.AssertionError: Timed out waiting for background updates to complete (5s). Did you forget to `start_doing_background_updates()`?

tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_idles
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_0_org_matrix_msc3026_busy
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_1_online
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_2_unavailable
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_3_offline
-------------------------------------------------------------------------------
Ran 5 tests in 25.208s

FAILED (errors=5)

Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Database isn't setup for tests that use GenericWorkerServer ❌

We have a few tests that use homeserver_to_use=GenericWorkerServer and are failing with builtins.AssertionError: Timed out waiting for background updates to complete (5s). Did you forget to 'start_doing_background_updates()'? in CI (from our new assertion).

Error: 
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 138, in new
    return code(orig, *args, **kwargs)
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 232, in setUp
    return orig()
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 404, in setUp
    self.hs = self.make_homeserver(self.reactor, self.clock)
  File "/home/runner/work/synapse/synapse/tests/app/test_openid_listener.py", line 40, in make_homeserver
    hs = self.setup_test_homeserver(homeserver_to_use=GenericWorkerServer)
  File "/home/runner/work/synapse/synapse/tests/unittest.py", line 733, in setup_test_homeserver
    raise AssertionError(
builtins.AssertionError: Timed out waiting for background updates to complete (5s). Did you forget to `start_doing_background_updates()`?

tests.app.test_openid_listener.FederationReaderOpenIDListenerTests.test_openid_listener_0
tests.app.test_openid_listener.FederationReaderOpenIDListenerTests.test_openid_listener_1
tests.app.test_openid_listener.FederationReaderOpenIDListenerTests.test_openid_listener_2
tests.app.test_openid_listener.FederationReaderOpenIDListenerTests.test_openid_listener_3

...

tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_idles
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_0_org_matrix_msc3026_busy
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_1_online
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_2_unavailable
tests.handlers.test_presence.PresenceHandlerInitTestCase.test_restored_presence_online_after_sync_3_offline

...

tests.replication.test_federation_ack.FederationAckTestCase.test_federation_ack_sent

...

tests.storage.test_rollback_worker.WorkerSchemaTests.test_not_upgraded_current_schema_version_with_outstanding_deltas
tests.storage.test_rollback_worker.WorkerSchemaTests.test_not_upgraded_old_schema_version
tests.storage.test_rollback_worker.WorkerSchemaTests.test_rolling_back

Previously, we didn't wait for background updates in these cases and only ran the database background updates for the TestHomeServer (the default in tests). Basically turning a blind eye to this kind of problem.

synapse/tests/unittest.py

Lines 680 to 696 in ec7554b

async def run_bg_updates() -> None:
with LoggingContext(name="run_bg_updates", server_name=server_name):
self.get_success(stor.db_pool.updates.run_background_updates(False))
hs = setup_test_homeserver(
cleanup_func=self.addCleanup,
server_name=server_name,
config=config_obj,
reactor=reactor,
clock=clock,
**extra_homeserver_attributes,
)
stor = hs.get_datastores().main
# Run the database background updates, when running against "master".
if hs.__class__.__name__ == "TestHomeServer":
self.get_success(run_bg_updates())

Unable to run test by itself

Compounding things is that we have to deal with #15671 (previously matrix-org/synapse#15671) which prevents us from running these tests on their own so it's even harder to debug what's going on.

synapse.storage.prepare_database.UpgradeDatabaseException: Uninitialised database: run the main synapse process to prepare the database schema before starting worker processes.

1 change: 1 addition & 0 deletions changelog.d/19057.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Move `start_doing_background_updates()` to `HomeServer.start_background_tasks()`.
5 changes: 0 additions & 5 deletions synapse/app/homeserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,6 @@ def start_listening(self) -> None:
# during parsing
logger.warning("Unrecognized listener type: %s", listener.type)

def start_background_tasks(self) -> None:
super().start_background_tasks()

self.get_datastores().main.db_pool.updates.start_doing_background_updates()


def load_or_generate_config(argv_options: List[str]) -> HomeServerConfig:
"""
Expand Down
4 changes: 4 additions & 0 deletions synapse/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,12 +640,16 @@ def start_background_tasks(self) -> None:
Some handlers have side effects on instantiation (like registering
background updates). This function causes them to be fetched, and
therefore instantiated, to run those side effects.

We assume these background tasks are only run on a single Synapse instance at a
time.
"""
for i in self.REQUIRED_ON_BACKGROUND_TASK_STARTUP:
getattr(self, "get_" + i + "_handler")()
self.get_task_scheduler()
self.get_common_usage_metrics_manager().setup()
start_phone_stats_home(self)
self.get_datastores().main.db_pool.updates.start_doing_background_updates()
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To call this out again; before #19036 (comment), start_doing_background_updates() only ran on the main Synapse instance.

With this change, it runs on whatever instance is configured to run_background_tasks_on.

Based on the previous discussion, we thought this was fine. I just didn't have this in the correct spot to match that description.


def get_reactor(self) -> ISynapseReactor:
"""
Expand Down
Loading