Skip to content

Conversation

@MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Nov 13, 2025

Fix potential lost logcontext when PerDestinationQueue.shutdown(...)

Spawning from looking at the logs in #19165 (comment) which mention the federation_transaction_transmission_loop. I don't think it's the source of the lost logcontext that person in the issue is experiencing because this only applies when you try to shutdown the homeserver.

Problem code introduced in #18828

To explain the fix, see the Deferred callbacks section of our logcontext docs for more info (specifically using solution 2).

Heads-up, I wrote the docs too so it's my assumptions/understanding all the way down. Apply your own scrutiny.

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)

Comment on lines +190 to +191
with PreserveLoggingContext():
self.active_transmission_loop.cancel()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To explain the fix, see the Deferred callbacks section of our logcontext docs for more info (specifically using solution 2).

Heads-up, I wrote the docs too so it's my assumptions/understanding all the way down. Apply your own scrutiny.

@MadLittleMods MadLittleMods marked this pull request as ready for review November 13, 2025 17:08
@MadLittleMods MadLittleMods requested a review from a team as a code owner November 13, 2025 17:08
Copy link
Member

@devonh devonh left a comment

Choose a reason for hiding this comment

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

Based on my understanding, and your docs this change makes sense to me.

Though I agree it shouldn't fix the issue reported as normal Synapse usage never calls shutdown.

@MadLittleMods MadLittleMods merged commit 408a05e into develop Nov 13, 2025
45 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/lost-logcontext-per-destination-queue-shutdown branch November 13, 2025 21:17
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @devonh 🐄

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.

3 participants