Implement cache lock when saving files

Implement a locking mechanism to storage.py that prevents race
conditions when saving files to the mirrors.

Implement tests.

Refs. TS-2404

Author: Roffenlund

django/thunderstore/core/storage.py

@@ -1,6 +1,7 @@
 from typing import IO, Any, Dict, Optional, TypedDict
 
 from django.conf import settings
+from django.core.cache import cache
 from django.utils.deconstruct import deconstructible
 from storages.backends.s3boto3 import S3Boto3Storage  # type: ignore
 
@@ -46,11 +47,20 @@ def save(
         Calling .save() closes the file, so use temporary copies for
         mirrors and call the main bucket with the actual file last.
         """
-        for storage_mirror in self.mirrors:
-            with TemporarySpooledCopy(content) as tmp_content:
-                storage_mirror.save(name, tmp_content, max_length)
 
-        return super().save(name, content, max_length)
+        cache_key = f"cache_mirror_storage_{name}"
+
+        if not cache.add(cache_key, "LOCKED", timeout=5):
+            raise Exception("Another save operation is in progress for this file.")
+
+        try:
+            for storage_mirror in self.mirrors:
+                with TemporarySpooledCopy(content) as tmp_content:
+                    storage_mirror.save(name, tmp_content, max_length)
+
+            return super().save(name, content, max_length)
+        finally:
+            cache.delete(cache_key)
 
     def delete(self, name: str) -> None:
         """

django/thunderstore/core/tests/test_storage.py

@@ -1,4 +1,7 @@
+from unittest.mock import patch
+
 import pytest
+from django.core.cache import cache
 from django.core.files.storage import default_storage
 from django.test import override_settings
 from PIL import Image  # type: ignore
@@ -7,19 +10,9 @@
 from thunderstore.repository.factories import PackageVersionFactory
 from thunderstore.repository.models.package_version import get_version_png_filepath
 
-
-def test_get_storage_class_or_stub(mocker) -> None:
-    assert get_storage_class_or_stub("non.stub.class") == "non.stub.class"
-    mocker.patch("sys.argv", ["manage.py", "migrate"])
-    assert (
-        get_storage_class_or_stub("non.stub.class")
-        == "thunderstore.utils.makemigrations.StubStorage"
-    )
-
-
-@override_settings(
-    DEFAULT_FILE_STORAGE="thunderstore.core.storage.MirroredS3Storage",
-    S3_MIRRORS=(
+mirrored_storage_settings = {
+    "DEFAULT_FILE_STORAGE": "thunderstore.core.storage.MirroredS3Storage",
+    "S3_MIRRORS": (
         {
             "access_key": "thunderstore",
             "secret_key": "thunderstore",
@@ -34,7 +27,19 @@ def test_get_storage_class_or_stub(mocker) -> None:
             "object_parameters": {},
         },
     ),
-)
+}
+
+
+def test_get_storage_class_or_stub(mocker) -> None:
+    assert get_storage_class_or_stub("non.stub.class") == "non.stub.class"
+    mocker.patch("sys.argv", ["manage.py", "migrate"])
+    assert (
+        get_storage_class_or_stub("non.stub.class")
+        == "thunderstore.utils.makemigrations.StubStorage"
+    )
+
+
+@override_settings(**mirrored_storage_settings)
 @pytest.mark.django_db
 def test_mirrored_storage(dummy_image: Image) -> None:
     pv = PackageVersionFactory(icon=None, name="MirrorStorageTest")
@@ -55,3 +60,48 @@ def test_mirrored_storage(dummy_image: Image) -> None:
     assert not default_storage.exists(icon_path)
     for mirror_storage in default_storage.mirrors:
         assert not mirror_storage.exists(icon_path)
+
+
+def setup_package_version(dummy_image):
+    pv = PackageVersionFactory(icon=None, name="MirrorStorageTest")
+    icon_path = get_version_png_filepath(pv, "")
+    pv.icon = dummy_image
+    return pv, icon_path
+
+
+@override_settings(**mirrored_storage_settings)
+@pytest.mark.django_db
+def test_mirrored_storage_save_lock_failure(dummy_image: Image) -> None:
+    """Test that save operation raises an exception if another save is in progress."""
+
+    message = "Another save operation is in progress for this file."
+    pv, icon_path = setup_package_version(dummy_image)
+    pv.icon = dummy_image
+    cache.add(f"cache_mirror_storage_{icon_path}", "LOCKED", timeout=5)
+
+    with pytest.raises(Exception, match=message):
+        pv.save()
+
+    assert cache.get(f"cache_mirror_storage_{icon_path}") == "LOCKED"
+
+
+@override_settings(**mirrored_storage_settings)
+@pytest.mark.django_db
+@patch("thunderstore.core.storage.TemporarySpooledCopy")
+@patch("django.core.cache.cache.add")
+@patch("django.core.cache.cache.delete")
+def test_mirrored_storage_save_cleanup_on_exception(
+    mock_cache_delete, mock_cache_add, mock_temporary_spooled_copy, dummy_image: Image
+) -> None:
+    """Test that the lock is released even if an exception occurs during save."""
+
+    message = "Error"
+    mock_temporary_spooled_copy.side_effect = Exception(message)
+    mock_cache_add.return_value = True
+    pv, icon_path = setup_package_version(dummy_image)
+
+    with pytest.raises(Exception, match=message):
+        pv.save()
+
+    mock_cache_delete.assert_called_once_with(f"cache_mirror_storage_{icon_path}")
+    assert cache.get(f"cache_mirror_storage_{icon_path}") is None