fix: remove all duplicate children refs when deleting/moving xblocks

cms/djangoapps/contentstore/views/tests/test_block.py

@@ -2132,6 +2132,57 @@ def test_move_parented_child(self):
             [self.problem_usage_key],  # problem child created in setUp
         )
 
+    def test_move_parented_child_with_duplicate_children(self):
+        """
+        Test that moving a child from one parent to another removes ALL
+        references from the old parent, even when duplicates exist (data corruption).
+
+        Regression test for a bug where list.remove() only removed the first
+        occurrence, leaving dangling references in the old parent.
+        """
+        unit_1_key = self.response_usage_key(
+            self.create_xblock(
+                parent_usage_key=self.seq_usage_key,
+                category="vertical",
+                display_name="unit 1",
+            )
+        )
+        unit_2_key = self.response_usage_key(
+            self.create_xblock(
+                parent_usage_key=self.seq2_usage_key,
+                category="vertical",
+                display_name="unit 2",
+            )
+        )
+
+        # Corrupt: add unit_1 to seq1's children 3 more times (4 total)
+        seq1 = self.get_item_from_modulestore(self.seq_usage_key)
+        for _ in range(3):
+            seq1.children.append(unit_1_key)
+        self.store.update_item(seq1, self.user.id)
+
+        # Verify corruption
+        seq1 = self.get_item_from_modulestore(self.seq_usage_key)
+        self.assertEqual(seq1.children.count(unit_1_key), 4)
+
+        # Move unit_1 from seq1 to seq2 by setting seq2's children
+        resp = self.client.ajax_post(
+            self.seq2_update_url, data={"children": [str(unit_1_key), str(unit_2_key)]}
+        )
+        self.assertEqual(resp.status_code, 200)
+
+        # Verify: unit_1 completely removed from seq1, present in seq2
+        seq1_after = self.get_item_from_modulestore(self.seq_usage_key)
+        self.assertNotIn(
+            unit_1_key,
+            seq1_after.children,
+            "Dangling references to moved block remain in old parent's children list",
+        )
+        self.assertListEqual(
+            self.get_item_from_modulestore(self.seq2_usage_key).children,
+            [unit_1_key, unit_2_key],
+        )
+
     def test_move_orphaned_child_error(self):
         """
         Test moving an orphan returns an error

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

@@ -373,7 +373,7 @@ def _save_xblock(
                 old_parent_location = store.get_parent_location(new_child)
                 if old_parent_location:
                     old_parent = store.get_item(old_parent_location)
-                    old_parent.children.remove(new_child)
+                    old_parent.children = [c for c in old_parent.children if c != new_child]
                     old_parent = save_xblock_with_callback(old_parent, user)
                 else:
                     # the Studio UI currently doesn't present orphaned children, so assume this is an error

xmodule/modulestore/draft_and_published.py

@@ -163,7 +163,7 @@ def update_item_parent(self, item_location, new_parent_location, old_parent_loca
 
         # Remove item from the list of children of old parent.
         if source_item.location in old_parent_item.children:
-            old_parent_item.children.remove(source_item.location)
+            old_parent_item.children = [c for c in old_parent_item.children if c != source_item.location]
             self.update_item(old_parent_item, user_id)  # pylint: disable=no-member
             log.info(
                 '%s removed from %s children',

xmodule/modulestore/split_mongo/split.py

@@ -2540,7 +2540,7 @@ def delete_item(self, usage_locator, user_id, force=False):  # lint-amnesty, pyl
             parent_block_keys = self._get_parents_from_structure(block_key, original_structure)
             for parent_block_key in parent_block_keys:
                 parent_block = new_blocks[parent_block_key]
-                parent_block.fields['children'].remove(block_key)
+                parent_block.fields['children'] = [c for c in parent_block.fields['children'] if c != block_key]
                 parent_block.edit_info.edited_on = datetime.datetime.now(UTC)
                 parent_block.edit_info.edited_by = user_id
                 parent_block.edit_info.previous_version = parent_block.edit_info.update_version

xmodule/modulestore/tests/test_mixed_modulestore.py

@@ -1701,6 +1701,47 @@ def test_move_1_moved_1_deleted(self, store_type):
         assert problem_item2.parent == self.vertical_x1a  # lint-amnesty, pylint: disable=no-member
         assert problem_item2.location in problem_item2.get_parent().children
 
+    @ddt.data(ModuleStoreEnum.Type.split)
+    def test_update_item_parent_with_duplicate_children(self, store_type):
+        """
+        Test that update_item_parent removes ALL references to a block from the
+        old parent's children list when duplicates exist (data corruption).
+
+        Regression test for a bug where list.remove() only removed the first
+        occurrence, leaving dangling references in the old parent.
+        """
+        self.initdb(store_type)
+        self._create_block_hierarchy()
+        self.course = self.store.publish(self.course.location, self.user_id)  # lint-amnesty, pylint: disable=attribute-defined-outside-init
+
+        item_location = self.problem_x1a_1  # lint-amnesty, pylint: disable=no-member
+        new_parent_location = self.vertical_y1a  # lint-amnesty, pylint: disable=no-member
+        old_parent_location = self.vertical_x1a  # lint-amnesty, pylint: disable=no-member
+
+        # Corrupt: add the item to old parent's children 3 more times
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
+            old_parent = self.store.get_item(old_parent_location)
+            for _ in range(3):
+                old_parent.children.append(item_location)
+            self.store.update_item(old_parent, self.user_id)
+
+            # Verify corruption: item appears 4 times
+            old_parent = self.store.get_item(old_parent_location)
+            assert old_parent.children.count(item_location) == 4
+
+        # Move item - should remove ALL 4 references from old parent
+        self.store.update_item_parent(
+            item_location, new_parent_location, old_parent_location, self.user_id
+        )
+
+        # Verify: item is in new parent and completely gone from old parent
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
+            old_parent = self.store.get_item(old_parent_location)
+            new_parent = self.store.get_item(new_parent_location)
+            assert item_location not in old_parent.children, \
+                "Dangling references to moved block remain in old parent's children list"
+            assert item_location in new_parent.children
+
     @ddt.data(ModuleStoreEnum.Type.split)
     def test_get_parent_locations_moved_child(self, default_ms):
         self.initdb(default_ms)

xmodule/modulestore/tests/test_split_modulestore.py

@@ -1552,6 +1552,66 @@ def check_subtree(node):
                         check_subtree(sub)
         check_subtree(nodes[0])
 
+    def test_delete_item_with_duplicate_children(self):
+        """
+        Test that delete_item removes ALL references to a block from its parent's
+        children list, even when the block appears multiple times (data corruption).
+
+        Regression test for a bug where list.remove() only removed the first
+        occurrence, leaving dangling references that crashed Studio.
+        """
+        store = modulestore()
+        user = self.user_id
+
+        # Create a course with a chapter containing a vertical
+        course = store.create_course('testx', 'dup_children', 'run', user, BRANCH_NAME_DRAFT)
+        course_loc = course.location.version_agnostic()
+        chapter = store.create_child(user, course_loc, 'chapter')
+        chapter_loc = chapter.location.map_into_course(course_loc.course_key)
+        vertical = store.create_child(user, chapter_loc, 'vertical')
+        vertical_loc = vertical.location.map_into_course(course_loc.course_key)
+
+        # Manually corrupt: add the vertical to the chapter's children 3 more times
+        # (simulating the production data corruption)
+        # pylint: disable=protected-access
+        course_key = course_loc.course_key.for_branch(BRANCH_NAME_DRAFT)
+        course_structure = store._lookup_course(course_key)
+        structure = course_structure.structure
+        new_structure = store.version_structure(course_key, structure, user)
+        new_blocks = new_structure['blocks']
+        new_id = new_structure['_id']
+
+        chapter_block_key = BlockKey.from_usage_key(chapter_loc)
+        vertical_block_key = BlockKey.from_usage_key(vertical_loc)
+        chapter_block = new_blocks[chapter_block_key]
+
+        # Verify it starts with 1 child
+        assert chapter_block.fields['children'].count(vertical_block_key) == 1
+
+        # Add 3 more duplicates (total 4, matching the production corruption)
+        for _ in range(3):
+            chapter_block.fields['children'].append(vertical_block_key)
+        assert chapter_block.fields['children'].count(vertical_block_key) == 4
+
+        chapter_block.edit_info.previous_version = chapter_block.edit_info.update_version
+        chapter_block.edit_info.update_version = new_id
+        store.update_structure(course_key, new_structure)
+        index_entry = store._get_index_if_valid(course_key)
+        if index_entry:
+            store._update_head(course_key, index_entry, course_key.branch, new_id)
+
+        # Now delete the vertical - this should remove ALL 4 references
+        store.delete_item(vertical_loc, user)
+
+        # Verify: vertical is gone and chapter has no dangling references
+        assert not store.has_item(vertical_loc.version_agnostic())
+
+        # Re-fetch the chapter and check its children list
+        updated_chapter = store.get_item(chapter_loc.version_agnostic())
+        assert vertical_block_key not in [
+            BlockKey.from_usage_key(c) for c in updated_chapter.children
+        ], "Dangling references to deleted block remain in parent's children list"
+
     def create_course_for_deletion(self):
         """
         Create a course we can delete