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.

3 files changed, 33 insertions, 32 deletions

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])
-
-            for provider in self.storage_providers:
-                with start_active_span(str(provider)):
-                    await provider.store_file(path, 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])
+
+                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