diff --git a/src/documents/tasks.py b/src/documents/tasks.py index 4ddc84a3d3..6bc8030557 100644 --- a/src/documents/tasks.py +++ b/src/documents/tasks.py @@ -421,7 +421,13 @@ def update_document_content_maybe_archive_file( thumbnail = parser.get_thumbnail(document.source_path, mime_type) - with transaction.atomic(): + # media.lock first, then the transaction: never wait on media.lock + # while this transaction holds the document row. Also keeps the + # unique archive filename generation under the lock. + with media_lock(), transaction.atomic(): + # Loaded before the (long) parse; read current metadata now + # that the lock is held, for the archive filename. + document.refresh_from_db() oldDocument = Document.objects.get(pk=document.pk) if parser.get_archive_path(): checksum = compute_checksum(parser.get_archive_path()) @@ -486,11 +492,10 @@ def update_document_content_maybe_archive_file( modified=timezone.now(), ) - with media_lock(): - if parser.get_archive_path(): - create_source_path_directory(document.archive_path) - shutil.move(parser.get_archive_path(), document.archive_path) - shutil.move(thumbnail, document.thumbnail_path) + if parser.get_archive_path(): + create_source_path_directory(document.archive_path) + shutil.move(parser.get_archive_path(), document.archive_path) + shutil.move(thumbnail, document.thumbnail_path) document.refresh_from_db() logger.info( diff --git a/src/documents/tests/test_management.py b/src/documents/tests/test_management.py index cac9b03665..5f0bb3664c 100644 --- a/src/documents/tests/test_management.py +++ b/src/documents/tests/test_management.py @@ -23,6 +23,7 @@ from documents.tasks import update_document_content_maybe_archive_file from paperless_testing.assertions import FileSystemAssertsMixin from paperless_testing.dirs import DirectoriesMixin from paperless_testing.factories import DocumentFactory +from paperless_testing.locks import record_media_lock_acquisitions from paperless_testing.samples import SIMPLE_DIGITAL_PDF sample_file: Path = SIMPLE_DIGITAL_PDF @@ -63,6 +64,28 @@ class TestArchiver(DirectoriesMixin, FileSystemAssertsMixin, TestCase): self.assertTrue(filecmp.cmp(sample_file, doc.source_path)) self.assertEqual(doc.archive_filename, "none/A.pdf") + def test_handle_document_takes_media_lock_outside_transaction(self) -> None: + """ + GIVEN: + - A document whose archive version is being generated + WHEN: + - update_document_content_maybe_archive_file runs + THEN: + - media.lock is acquired + - media.lock is never first acquired while a transaction holds the + document row + """ + doc = self.make_models() + shutil.copy(sample_file, Path(self.dirs.originals_dir) / f"{doc.id:07}.pdf") + + with record_media_lock_acquisitions() as acquisitions: + update_document_content_maybe_archive_file(doc.pk) + + self.assertTrue(acquisitions) + self.assertFalse(any(a.in_user_transaction for a in acquisitions)) + doc.refresh_from_db() + self.assertIsFile(doc.archive_path) + def test_unknown_mime_type(self) -> None: doc = self.make_models() doc.mime_type = "sdgfh"