From 8bd9ff0783c26d9ce4d08b396e5620c57eef2e67 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 30 May 2024 11:22:19 +0100 Subject: [PATCH] Ensure we delete media if we reject due to spam check (#17246) Fixes up #17239 We need to keep the spam check within the `try/except` block. Also makes it so that we don't enter the top span twice. Also also ensures that we get the right thumbnail length. --- changelog.d/17246.misc | 1 + synapse/media/media_repository.py | 5 +++ synapse/media/media_storage.py | 57 ++++++++++++++----------------- 3 files changed, 32 insertions(+), 31 deletions(-) create mode 100644 changelog.d/17246.misc diff --git a/changelog.d/17246.misc b/changelog.d/17246.misc new file mode 100644 index 0000000000..9fca36bb29 --- /dev/null +++ b/changelog.d/17246.misc @@ -0,0 +1 @@ +Fix errors in logs about closing incorrect logging contexts when media gets rejected by a module. diff --git a/synapse/media/media_repository.py b/synapse/media/media_repository.py index 9da8495950..9c29e09653 100644 --- a/synapse/media/media_repository.py +++ b/synapse/media/media_repository.py @@ -1049,6 +1049,11 @@ class MediaRepository: finally: t_byte_source.close() + # We flush and close the file to ensure that the bytes have + # been written before getting the size. + f.flush() + f.close() + t_len = os.path.getsize(fname) # Write to database diff --git a/synapse/media/media_storage.py b/synapse/media/media_storage.py index 9979c48eac..b3cd3fd8f4 100644 --- a/synapse/media/media_storage.py +++ b/synapse/media/media_storage.py @@ -137,42 +137,37 @@ class MediaStorage: dirname = os.path.dirname(fname) os.makedirs(dirname, exist_ok=True) - main_media_repo_write_trace_scope = start_active_span( - "writing to main media repo" - ) - main_media_repo_write_trace_scope.__enter__() - - with main_media_repo_write_trace_scope: - try: + try: + with start_active_span("writing to main media repo"): with open(fname, "wb") as f: yield f, fname - except Exception as e: - try: - os.remove(fname) - except Exception: - pass - - raise e from None - - with start_active_span("writing to other storage providers"): - spam_check = ( - await self._spam_checker_module_callbacks.check_media_file_for_spam( - ReadableFileWrapper(self.clock, fname), file_info + with start_active_span("writing to other storage providers"): + spam_check = ( + await self._spam_checker_module_callbacks.check_media_file_for_spam( + ReadableFileWrapper(self.clock, fname), file_info + ) ) - ) - if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: - logger.info("Blocking media due to spam checker") - # Note that we'll delete the stored media, due to the - # try/except below. The media also won't be stored in - # the DB. - # We currently ignore any additional field returned by - # the spam-check API. - raise SpamMediaException(errcode=spam_check[0]) + if spam_check != self._spam_checker_module_callbacks.NOT_SPAM: + logger.info("Blocking media due to spam checker") + # Note that we'll delete the stored media, due to the + # try/except below. The media also won't be stored in + # the DB. + # We currently ignore any additional field returned by + # the spam-check API. + raise SpamMediaException(errcode=spam_check[0]) - for provider in self.storage_providers: - with start_active_span(str(provider)): - await provider.store_file(path, file_info) + for provider in self.storage_providers: + with start_active_span(str(provider)): + await provider.store_file(path, file_info) + + except Exception as e: + try: + os.remove(fname) + except Exception: + pass + + raise e from None async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: """Attempts to fetch media described by file_info from the local cache