From 7f6fd028c1a56352f878f8460e85ee183a169bf7 Mon Sep 17 00:00:00 2001 From: Tycho Hob Date: Fri, 16 Oct 2026 15:34:58 -0400 Subject: [PATCH 1/2] fix: queue clipboard cleanup only after the copy commits Copying to the clipboard marks the user's previous StagedContent rows EXPIRED and enqueued `delete_expired_clipboards` from inside the request's transaction. An idle CMS worker could start the task at once and delete the expired row, which cascades to the user's UserClipboard row, while the request went on to `update_or_create` that same row. The two took the locks in opposite orders, MySQL killed the request with a deadlock (1213), and the copy answered 500. Enqueue the cleanup with `transaction.on_commit`, and make both clipboard save functions atomic so the clipboard row is repointed at the new content before the cleanup can run, even when the caller has no transaction open. Closes #39118 Co-Authored-By: Claude Opus 5.5 --- .../core/djangoapps/content_staging/api.py | 23 ++++++++++--- .../content_staging/tests/test_clipboard.py | 33 +++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 1700f2358739..a3dfb7e596d6 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[int]) -> 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. From d9473431360b5a433f7eacf5e3d1671bee775c1f Mon Sep 17 00:00:00 2001 From: Tycho Hob Date: Fri, 16 Oct 2026 16:18:21 -0400 Subject: [PATCH 2/2] fix: type expired clipboard ids as StagedContent ids mypy rejected `list[int]` for the ids passed to the clipboard cleanup helper: they are `StagedContent.ID`s, as `delete_expired_clipboards` declares. Co-Authored-By: Claude Opus 5.5 --- openedx/core/djangoapps/content_staging/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index a3dfb7e596d6..643f89c3f6e3 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -130,7 +130,7 @@ def _save_data_to_staged_content( return staged_content -def _enqueue_clipboard_cleanup(expired_ids: list[int]) -> None: +def _enqueue_clipboard_cleanup(expired_ids: list[_StagedContent.ID]) -> None: """ Enqueue the task that deletes expired clipboard StagedContent rows. """