Skip to content

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Oct 9, 2025

Move start_doing_background_updates() to SynapseHomeServer.start_background_tasks() (more sane standard location for this sort of thing)

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

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

self.get_datastores().main.db_pool.updates.start_doing_background_updates()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one difference here is that previously, start_doing_background_updates() only ran on the main Synapse instance.

But start_background_tasks() runs on the worker that supposed to run_background_tasks.

if hs.config.worker.run_background_tasks:
hs.start_background_tasks()

Is that okay? Nothing is immediately coming to mind on why this would be bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems OK to me too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think there is a flaw with this setup.

SynapseHomeServer is for the main Synapse instance. We have SynapseHomeServer.start_background_tasks() setup to call start_doing_background_updates() but if the run_background_tasks_on is set to anything else besides main, then we will never run SynapseHomeServer.start_background_tasks() specifically.

What we actually need to do is just move this to HomeServer.start_background_tasks() instead. We want start_doing_background_updates() to run on whatever homeserver is configured to run_background_tasks_on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is being addressed in #19057

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deciding to revert this PR in the meantime -> #19059

While I figure out all of the intricacies in the test infrastructure from moving it to the correct spot. Will re-introduce things in #19057

@MadLittleMods MadLittleMods marked this pull request as ready for review October 9, 2025 22:06
@MadLittleMods MadLittleMods requested a review from a team as a code owner October 9, 2025 22:06
@MadLittleMods MadLittleMods merged commit d399d76 into develop Oct 10, 2025
107 of 112 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/move-start-background-updates branch October 10, 2025 19:30
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @reivilibre 🦧

MadLittleMods added a commit that referenced this pull request Oct 13, 2025
….start_background_tasks()` (#19036)"

This reverts commit d399d76.
MadLittleMods added a commit that referenced this pull request Oct 20, 2025
….start_background_tasks()` (#19036)" (#19059)

### Why

See
#19036 (comment)

Revert while I figure out the tests in
#19057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants