diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 1700f2358739..643f89c3f6e3 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/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, diff --git a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py b/openedx/core/djangoapps/content_staging/tests/test_clipboard.py index d588cbed950d..adc398b2056a 100644 --- a/openedx/core/djangoapps/content_staging/tests/test_clipboard.py +++ b/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.