fix: queue clipboard cleanup only after the copy commits

openedx/core/djangoapps/content_staging/api.py

@@ -120,15 +120,26 @@ def _save_data_to_staged_content(
             log.exception(f"Unable to copy static files to staged content for component {copied_from}")
 
     if expired_ids:
-        # Enqueue a (potentially slow) task to delete the old staged content
-        try:
-            delete_expired_clipboards.delay(expired_ids)
-        except Exception:  # pylint: disable=broad-except
-            log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}")
+        # Enqueue a (potentially slow) task to delete the old staged content, but only once the
+        # surrounding transaction has committed. Deleting an expired row also deletes (by cascade)
+        # a UserClipboard that still points at it, and the caller is about to update that same
+        # row; a worker that picked the task up straight away would take the two rows' locks in
+        # the opposite order and deadlock the request.
+        transaction.on_commit(lambda: _enqueue_clipboard_cleanup(expired_ids))
 
     return staged_content
 
 
+def _enqueue_clipboard_cleanup(expired_ids: list[_StagedContent.ID]) -> None:
+    """
+    Enqueue the task that deletes expired clipboard StagedContent rows.
+    """
+    try:
+        delete_expired_clipboards.delay(expired_ids)
+    except Exception:  # pylint: disable=broad-except
+        log.exception(f"Unable to enqueue cleanup task for StagedContents: {','.join(str(x) for x in expired_ids)}")
+
+
 def _save_static_assets_to_staged_content(
     static_files: list[StaticFile], usage_key: UsageKey | ContainerKey, staged_content: _StagedContent
 ):
@@ -178,6 +189,7 @@ def _save_static_assets_to_staged_content(
             log.exception(f"Unable to copy static file {f.name} to clipboard for component {usage_key}")
 
 
+@transaction.atomic
 def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int | None = None) -> UserClipboardData:
     """
     Copy an XBlock's OLX to the user's clipboard.
@@ -197,6 +209,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int
     return _user_clipboard_model_to_data(clipboard)
 
 
+@transaction.atomic
 def save_content_to_user_clipboard(
     user_id: int,
     block_type: str,

openedx/core/djangoapps/content_staging/tests/test_clipboard.py

@@ -4,6 +4,7 @@
 """
 from textwrap import dedent
 from typing import cast
+from unittest.mock import patch
 from xml.etree import ElementTree
 
 import ddt
@@ -309,6 +310,38 @@ def test_copy_several_things(self) -> None:
         # The OLX link from the video will no longer work:
         assert client.get(old_olx_url).status_code == 404
 
+    def test_expired_content_cleanup_waits_for_commit(self) -> None:
+        """
+        Test that the user's previous clipboard content is only queued for
+        deletion once the copy that replaced it has committed, by which point
+        the user's clipboard no longer points at it.
+
+        Queuing it from inside the transaction let an idle worker delete the
+        old content (and, by cascade, the clipboard row the copy was about to
+        update) while the copy was still running, deadlocking the copy.
+        """
+        course_key, client = self._setup_course()
+        video_key = course_key.make_usage_key("video", "sample_video")
+        response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(video_key)}, format="json")
+        assert response.status_code == 200
+        old_content_id = response.json()["content"]["id"]
+
+        def check_clipboard_has_moved_on(expired_ids):
+            assert expired_ids == [old_content_id]
+            clipboard = python_api.get_user_clipboard(self.user.id)
+            assert clipboard is not None
+            assert clipboard.content.id != old_content_id
+
+        html_key = course_key.make_usage_key("html", "toyhtml")
+        with patch.object(
+            python_api.delete_expired_clipboards, "delay", side_effect=check_clipboard_has_moved_on,
+        ) as mock_delay:
+            with self.captureOnCommitCallbacks(execute=True):
+                response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(html_key)}, format="json")
+                assert response.status_code == 200
+                mock_delay.assert_not_called()
+            mock_delay.assert_called_once_with([old_content_id])
+
     def test_copy_static_assets(self) -> None:
         """
         Test copying an HTML from the course that references a static asset file.