Implement synapse issue #16751: Treat local_media_directory as optional storage provider #19204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… as optional storage provider This allows off-site media storage without local cache by making the local media directory behave like any other MediaStorageProvider. When no local directory is configured, media is stored directly to remote providers only, with temporary files used for thumbnail generation when needed.
|
|
… as optional storage provider This allows off-site media storage without local cache by making the local media directory behave like any other MediaStorageProvider. When no local directory is configured, media is stored directly to remote providers only, with temporary files used for thumbnail generation when needed. Also fixes URL cache thumbnails to be served directly from local filesystem instead of being incorrectly routed through storage providers.
These were auto-formatted changes that weren't intended to be committed.
drallgood
changed the title
Add type ignore comment for os.path.join call since local_provider check guarantees local_media_directory is not None.
anoadragon453
requested changes
Nov 28, 2025
Member
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Several questions below.
From the linked issues, I think broadly what we're aiming for is to satisfy the use-case where media isn't stored on disk at all, and is only stored on (typically) remote providers.
"nit" below means a non-blocking change. Typically related to code style or readability.
| os.makedirs(dirname, exist_ok=True) | ||
|
|
||
| raise NotFoundError() | ||
| for provider in self.storage_providers: |
Member
There was a problem hiding this comment.
I think we could make use of the fetch_media method here.
Author
There was a problem hiding this comment.
not really. They have a different purpose. The ensure_media_is_in_local_cache is used to ensure a local copy so a thumbnail can be generated. While the fetch_media returns a Responder (to stream the file).
Address code review feedback: - Add enable_local_media_storage config option to disable local media store - Refactor MediaStorage.__init__ to accept local_provider as optional FileStorageProviderBackend argument - Rename fname to media_filepath and eliminate dirname variable - Deduplicate spam check code in store_into_file - Delete temp files immediately after upload to storage providers - Fix fetch_media to check local_provider first - Remove unnecessary list() wrapper - Revert storage_providers type to Sequence[StorageProvider] - Update tests to use new MediaStorage constructor signature
When no local provider is configured, temp files are now automatically cleaned up after use. This addresses the reviewer feedback about temp files building up on disk over time. The method now yields the file path within a context manager, and callers must use `async with` to ensure proper cleanup.
Author
|
@anoadragon453 thanks for the review. I reworked the suggested parts and found another area (the thumbnail generation) that needed fixing. Hope this is better? 😄 |
Add enable_local_media_storage to the JSON schema and regenerate the configuration documentation so the new flag appears under the Media Store section.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows off-site media storage without local cache by making the local media directory behave like any other MediaStorageProvider. When no local directory is configured, media is stored directly to remote providers only, with temporary files used for thumbnail generation when needed.
fixes #16751
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.