diff --git a/src/documents/consumer.py b/src/documents/consumer.py index 9d7769858c..555a02db48 100644 --- a/src/documents/consumer.py +++ b/src/documents/consumer.py @@ -607,13 +607,12 @@ class ConsumerPlugin( # now that everything is done, we can start to store the document # in the system. This will be a transaction and reasonably fast. try: - # File moves requested by saves in here run once, after - # commit and after media.lock is released (in production - # this nests under the consume task's scope). media.lock is - # taken before the transaction: the version path locks the - # root row with select_for_update, and workflows (email - # actions) and file placement below take media.lock again, - # re-entering instead of waiting while rows are locked. + # File moves requested in here run after commit and after + # media.lock is released. media.lock is taken before the + # transaction: the version path locks the root row with + # select_for_update, and workflow email actions and file + # placement below re-enter media.lock instead of waiting + # on it while rows are locked. with defer_file_moves(), media_lock(), transaction.atomic(): # store the document. if self.input_doc.root_document_id: diff --git a/src/documents/file_moves.py b/src/documents/file_moves.py index 305b36f32a..ab6680324f 100644 --- a/src/documents/file_moves.py +++ b/src/documents/file_moves.py @@ -60,8 +60,8 @@ def in_user_transaction() -> bool: created by Django's test case machinery. Django marks the atomic blocks TestCase (and pytest-django) wrap around - each test with _from_testcase, the same flag it uses for durable checks. - Ignoring them keeps tests on the same path as production. + each test with _from_testcase. Ignoring them keeps tests on the same path + as production. """ connection = transaction.get_connection() return any(not block._from_testcase for block in connection.atomic_blocks) @@ -124,11 +124,10 @@ def move_files_for_document(pk: int) -> Document | None: the document does not exist, has no filename yet, or the lock timed out. """ # Cheap check without the lock: most saves during consumption happen - # before the file is placed. filename, once set, is never cleared, so a - # non-empty value here stays non-empty under the lock. This goes through - # values_list() (not the manager's filter()) so the check stays off + # before the file is placed. filename is never cleared once set, so this + # stays true under the lock. values_list() keeps the check off # Document.global_objects.filter(), which tests patch to inject database - # errors and to observe the mover's own writes. + # errors into and observe the mover's own writes. filename = ( Document.global_objects.values_list("filename", flat=True).filter(pk=pk).first() ) @@ -208,9 +207,8 @@ def flush_pending_file_moves() -> None: """ Perform the current scope's pending moves now, keeping the scope open. - When called from a task running eagerly inside a request, this flushes the - request's collector early, which is harmless: the moves still run after - commit and against fresh state. + From an eager task inside a request this flushes the request's collector + early, which is harmless: moves still run after commit, on fresh state. """ collector = _active_collector.get() if collector is None: @@ -339,7 +337,6 @@ def _move_document_files(document: Document) -> None: updates["archive_filename"] = document.archive_filename if updates: # global_objects so a trashed document's path is fixed too. - # modified only changes when a filename column changes. updates["modified"] = timezone.now() # Don't save() here to prevent infinite recursion. Document.global_objects.filter(pk=document.pk).update(**updates) @@ -426,9 +423,9 @@ def request_file_move(document: Document) -> None: Ask for the document's files to follow its committed state. Inside an open scope the request is recorded and performed when the scope - ends. Otherwise the move happens now, and only the final filename columns - are copied onto the caller's instance, so code that saves and then reads - source_path keeps working without any other in-memory field being touched. + ends. Otherwise the move happens now and only the final filename columns + are copied onto the caller's instance, so source_path is current after + save() while other in-memory fields are left alone. """ collector = _active_collector.get() if collector is not None and collector.add(document.pk): @@ -445,10 +442,9 @@ async def adefer_file_moves() -> AsyncIterator[None]: """ Async twin of defer_file_moves(). - The collector is closed on the event loop (no DB access), and only when - something was collected does the flush, including the transaction check - that must run in the thread owning the DB connection, run through - sync_to_async so the blocking media.lock never runs on the event loop. + When something was collected, the flush runs through sync_to_async: its + transaction check needs the thread owning the DB connection, and the + blocking media.lock must never run on the event loop. """ current = _active_collector.get() if current is not None and not current.closed: diff --git a/src/documents/tests/test_file_moves.py b/src/documents/tests/test_file_moves.py index 985fa1739b..8a0c934e73 100644 --- a/src/documents/tests/test_file_moves.py +++ b/src/documents/tests/test_file_moves.py @@ -172,8 +172,7 @@ class TestMoveFilesForDocument: """ GIVEN: - A trashed document whose DB filename is stale - - Its file sits at the generated path (checksum matches), placed - after the creation-time move already failed + - Its file already sits at the generated path (checksum matches) WHEN: - move_files_for_document is called THEN: @@ -627,8 +626,7 @@ class TestFileMovesThroughApi: - A tag + title PATCH and a note POST are made through the API THEN: - media.lock is acquired at least once - - media.lock is never first acquired while a transaction is open, - which is the lock ordering that let two such requests deadlock + - media.lock is never first acquired while a transaction is open """ tag = TagFactory() diff --git a/src/documents/tests/test_management.py b/src/documents/tests/test_management.py index aea36a4f66..ceb472116a 100644 --- a/src/documents/tests/test_management.py +++ b/src/documents/tests/test_management.py @@ -285,8 +285,7 @@ class TestRenamer(DirectoriesMixin, FileSystemAssertsMixin, TestCase): WHEN: - document_renamer runs THEN: - - Each document is moved by a direct call, one per document, - not by a fake post_save signal + - move_files_for_document is called once per document - Both files are at their new paths """ docs = [] diff --git a/src/paperless/middleware.py b/src/paperless/middleware.py index 7c30c07ce3..a0c6f253b1 100644 --- a/src/paperless/middleware.py +++ b/src/paperless/middleware.py @@ -39,8 +39,6 @@ class FileMoveMiddleware: Open a document file move scope around each request, so every document the request changes is moved once, after its writes have committed, before the response is returned. - - Supports both sync and async middleware chains. """ sync_capable = True diff --git a/src/paperless_testing/locks.py b/src/paperless_testing/locks.py index b610d15113..79aabb7b92 100644 --- a/src/paperless_testing/locks.py +++ b/src/paperless_testing/locks.py @@ -27,7 +27,6 @@ def record_media_lock_acquisitions() -> Iterator[list[MediaLockAcquisition]]: Reentrant acquisitions by a thread that already holds the lock never wait, so they are not recorded. Each record notes whether a real (non test case) transaction was open at that moment, which is the lock ordering hazard. - Usable from pytest tests and unittest TestCase classes alike. """ from django.conf import settings from filelock import BaseFileLock