Fix run_in_background not be awaited properly causing LoggingContext problems

[MadLittleMods]

Fix run_in_background not be awaited properly causing LoggingContext problems

Basically, searching for any instance of run_in_background(...) and making sure we wrap the deferred in make_deferred_yieldable(...) if we try to await the result to make it follow the Synapse logcontext rules.

Turns out, we only have this problem in some tests (phew)

Part of #18905

Dev notes

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_federation.FederationTestCase.test_rejected_message_event_state

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.storage.databases.main.test_events_worker.HaveSeenEventsTestCase.test_simple

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.crypto.test_keyring.KeyringTestCase.test_verify_json_objects_for_server_awaits_previous_requests

from tests.unittest import HomeserverTestCase


class AsdfTestCase(HomeserverTestCase):
    def test_asdf1(self) -> None:
        self.assertTrue(True)

    def test_asdf2(self) -> None:
        res = self.get_success(
            self.hs.get_device_handler().check_device_registered(
                user_id="@boris:foo",
                device_id="fco",
                initial_device_display_name="display name",
            )
        )
        self.assertEqual(res, "fco")

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)

[MadLittleMods]

I originally tried adapting things to keep these LoggingContext around but then realized there is no point in having these here in the tests.

The only reason I can see why this might have been added was to avoid these kind of warnings in _trial_temp/test.log:

Starting metrics collection 'xxx' from sentinel context: metrics will be lost

But it's expected that test things run outside of Synapse and by their nature should happen in the sentinel logcontext. And if we really don't want to see these kind of errors, we would need to refactor get_success(...) to look more like get_success(func, OTHER_SERVER, ev) where we call the function with some homeserver_test_case logcontext instead of passing in an awaitable.

[reivilibre]

is it possible the name was added here for logging clarity, to be fair?

[MadLittleMods]

It's possible that was the original reason but with a more nailed down definition of sentinel logcontext and when it should be used, I don't think it makes sense.

And after working with logcontext a bunch, I've kinda come to the conclusion that the name doesn't matter much outside of a request. Or at-least doesn't matter much to what I'm trying to accomplish.

[reivilibre]

is it possible the name was added here for logging clarity, to be fair?

[MadLittleMods]

Thanks for the review @reivilibre 🦜