Implement synapse issue #16751: Treat local_media_directory as optional storage provider #19204
Conversation
… 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
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.
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.
|
@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.
There was a problem hiding this comment.
Looking mostly good now! Thanks for making those changes.
Couple more things, plus the following unit tests are currently failing:
tests.replication.test_multi_media_repo.AuthenticatedMediaRepoShardTestCase.test_download_simple_file_racetests.replication.test_multi_media_repo.MediaRepoShardTestCase.test_download_simple_file_race
These tests assume that the files we fetch remain on disk, and this assumption should continue to hold as enable_local_media_storage is True in tests.
It'd also be good to see some tests with enable_local_media_storage set to False, so we can check the happy path works, and that files aren't pooling up on disk.
Note that you can use the @override_config decorator on tests to dynamically modify homeserver configuration for just that test case.
- Move database insert outside store_into_file context manager to ensure proper cleanup of duplicate files on constraint violations - Add explicit file cleanup on IntegrityError when concurrent downloads create duplicate files - Refactor duplicate exception handling into _store_remote_media_with_cleanup helper method - Add tests for enable_local_media_storage=False to verify no local file accumulation when local storage is disabled - Remove unnecessary ensure_media_is_in_local_cache call in thumbnailer.py Fixes race condition where two workers concurrently downloading the same media would result in two files being stored instead of one.
|
I think I fixed all tests and also added the suggested tests. It's a bit tricky to get proper tests with external providers up and running so I went with a compromise. |
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Just a few small changes, and I think this is good to go! Thanks for your patience with the back and forth.
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
When enable_local_media_storage is set to false and no media_storage_providers are configured, all media requests will return 404 errors as there will be no storage backend available. This addresses reviewer feedback to document the expected behavior in this configuration scenario.
When enable_local_media_storage is disabled and no media_storage_providers are configured, log a warning at startup to inform administrators that all media requests will return 404 errors due to the lack of storage backend.
4 participants
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.