Update handlers.py

src/documents/signals/handlers.py

@@ -19,6 +19,7 @@ from django.db import DatabaseError
 from django.db import close_old_connections
 from django.db import connections
 from django.db import models
+from django.db import transaction
 from django.db.models import Q
 from django.dispatch import receiver
 from django.utils import timezone
@@ -453,62 +454,94 @@ def update_filename_and_move_files(
         # This will in turn cause this logic to move the file where it belongs.
         return
 
-    with FileLock(settings.MEDIA_LOCK):
-        try:
-            # If this was waiting for the lock, the filename or archive_filename
-            # of this document may have been updated.  This happens if multiple updates
-            # get queued from the UI for the same document
-            # So freshen up the data before doing anything
-            instance.refresh_from_db()
+    move_original = False
+    move_archive = False
+    old_filename = None
+    old_archive_filename = None
+    old_source_path = None
+    old_archive_path = None
 
-            old_filename = instance.filename
-            old_source_path = instance.source_path
+    try:
+        with transaction.atomic():
+            Document.global_objects.select_for_update().get(pk=instance.pk)
+            with FileLock(settings.MEDIA_LOCK):
+                # If this was waiting for the lock, the filename or archive_filename
+                # of this document may have been updated.  This happens if multiple updates
+                # get queued from the UI for the same document
+                # So freshen up the data before doing anything
+                instance.refresh_from_db()
 
-            candidate_filename = generate_filename(instance)
-            candidate_source_path = (
-                settings.ORIGINALS_DIR / candidate_filename
-            ).resolve()
-            if candidate_filename == Path(old_filename):
-                new_filename = Path(old_filename)
-            elif (
-                candidate_source_path.exists()
-                and candidate_source_path != old_source_path
-            ):
-                # Only fall back to unique search when there is an actual conflict
-                new_filename = generate_unique_filename(instance)
-            else:
-                new_filename = candidate_filename
+                old_filename = instance.filename
+                old_source_path = instance.source_path
 
-            # Need to convert to string to be able to save it to the db
-            instance.filename = str(new_filename)
-            move_original = old_filename != instance.filename
-
-            old_archive_filename = instance.archive_filename
-            old_archive_path = instance.archive_path
-
-            if instance.has_archive_version:
-                archive_candidate = generate_filename(instance, archive_filename=True)
-                archive_candidate_path = (
-                    settings.ARCHIVE_DIR / archive_candidate
+                candidate_filename = generate_filename(instance)
+                candidate_source_path = (
+                    settings.ORIGINALS_DIR / candidate_filename
                 ).resolve()
-                if archive_candidate == Path(old_archive_filename):
-                    new_archive_filename = Path(old_archive_filename)
+                if candidate_filename == Path(old_filename):
+                    new_filename = Path(old_filename)
                 elif (
-                    archive_candidate_path.exists()
-                    and archive_candidate_path != old_archive_path
+                    candidate_source_path.exists()
+                    and candidate_source_path != old_source_path
                 ):
-                    new_archive_filename = generate_unique_filename(
+                    # Only fall back to unique search when there is an actual conflict
+                    new_filename = generate_unique_filename(instance)
+                else:
+                    new_filename = candidate_filename
+
+                # Need to convert to string to be able to save it to the db
+                instance.filename = str(new_filename)
+                move_original = old_filename != instance.filename
+
+                old_archive_filename = instance.archive_filename
+                old_archive_path = instance.archive_path
+
+                if instance.has_archive_version:
+                    archive_candidate = generate_filename(
                         instance,
                         archive_filename=True,
                     )
+                    archive_candidate_path = (
+                        settings.ARCHIVE_DIR / archive_candidate
+                    ).resolve()
+                    if archive_candidate == Path(old_archive_filename):
+                        new_archive_filename = Path(old_archive_filename)
+                    elif (
+                        archive_candidate_path.exists()
+                        and archive_candidate_path != old_archive_path
+                    ):
+                        new_archive_filename = generate_unique_filename(
+                            instance,
+                            archive_filename=True,
+                        )
+                    else:
+                        new_archive_filename = archive_candidate
+
+                    instance.archive_filename = str(new_archive_filename)
+
+                    move_archive = old_archive_filename != instance.archive_filename
                 else:
-                    new_archive_filename = archive_candidate
+                    move_archive = False
 
-                instance.archive_filename = str(new_archive_filename)
+                if move_original:
+                    validate_move(
+                        instance,
+                        old_source_path,
+                        instance.source_path,
+                        settings.ORIGINALS_DIR,
+                    )
+                    create_source_path_directory(instance.source_path)
+                    shutil.move(old_source_path, instance.source_path)
 
-                move_archive = old_archive_filename != instance.archive_filename
-            else:
-                move_archive = False
+                if move_archive:
+                    validate_move(
+                        instance,
+                        old_archive_path,
+                        instance.archive_path,
+                        settings.ARCHIVE_DIR,
+                    )
+                    create_source_path_directory(instance.archive_path)
+                    shutil.move(old_archive_path, instance.archive_path)
 
             if not move_original and not move_archive:
                 # Just update modified. Also, don't save() here to prevent infinite recursion.
@@ -517,26 +550,6 @@ def update_filename_and_move_files(
                 )
                 return
 
-            if move_original:
-                validate_move(
-                    instance,
-                    old_source_path,
-                    instance.source_path,
-                    settings.ORIGINALS_DIR,
-                )
-                create_source_path_directory(instance.source_path)
-                shutil.move(old_source_path, instance.source_path)
-
-            if move_archive:
-                validate_move(
-                    instance,
-                    old_archive_path,
-                    instance.archive_path,
-                    settings.ARCHIVE_DIR,
-                )
-                create_source_path_directory(instance.archive_path)
-                shutil.move(old_archive_path, instance.archive_path)
-
             # Don't save() here to prevent infinite recursion.
             Document.global_objects.filter(pk=instance.pk).update(
                 filename=instance.filename,
@@ -546,22 +559,24 @@ def update_filename_and_move_files(
             # Clear any caching for this document.  Slightly overkill, but not terrible
             clear_document_caches(instance.pk)
 
-        except (OSError, DatabaseError, CannotMoveFilesException) as e:
-            logger.warning(f"Exception during file handling: {e}")
-            # This happens when either:
-            #  - moving the files failed due to file system errors
-            #  - saving to the database failed due to database errors
-            # In both cases, we need to revert to the original state.
+    except (OSError, DatabaseError, CannotMoveFilesException) as e:
+        logger.warning(f"Exception during file handling: {e}")
+        # This happens when either:
+        #  - moving the files failed due to file system errors
+        #  - saving to the database failed due to database errors
+        # In both cases, we need to revert to the original state.
 
+        if move_original or move_archive:
             # Try to move files to their original location.
             try:
-                if move_original and instance.source_path.is_file():
-                    logger.info("Restoring previous original path")
-                    shutil.move(instance.source_path, old_source_path)
+                with FileLock(settings.MEDIA_LOCK):
+                    if move_original and instance.source_path.is_file():
+                        logger.info("Restoring previous original path")
+                        shutil.move(instance.source_path, old_source_path)
 
-                if move_archive and instance.archive_path.is_file():
-                    logger.info("Restoring previous archive path")
-                    shutil.move(instance.archive_path, old_archive_path)
+                    if move_archive and instance.archive_path.is_file():
+                        logger.info("Restoring previous archive path")
+                        shutil.move(instance.archive_path, old_archive_path)
 
             except Exception:
                 # This is fine, since:
@@ -574,23 +589,29 @@ def update_filename_and_move_files(
                 #  anyway.
                 pass
 
-            # restore old values on the instance
+        # restore old values on the instance
+        if old_filename is not None:
             instance.filename = old_filename
+        if old_archive_filename is not None:
             instance.archive_filename = old_archive_filename
 
-        # finally, remove any empty sub folders. This will do nothing if
-        # something has failed above.
-        if not old_source_path.is_file():
-            delete_empty_directories(
-                Path(old_source_path).parent,
-                root=settings.ORIGINALS_DIR,
-            )
+    # finally, remove any empty sub folders. This will do nothing if
+    # something has failed above.
+    if old_source_path and not old_source_path.is_file():
+        delete_empty_directories(
+            Path(old_source_path).parent,
+            root=settings.ORIGINALS_DIR,
+        )
 
-        if instance.has_archive_version and not old_archive_path.is_file():
-            delete_empty_directories(
-                Path(old_archive_path).parent,
-                root=settings.ARCHIVE_DIR,
-            )
+    if (
+        instance.has_archive_version
+        and old_archive_path
+        and not old_archive_path.is_file()
+    ):
+        delete_empty_directories(
+            Path(old_archive_path).parent,
+            root=settings.ARCHIVE_DIR,
+        )
 
 
 @shared_task