From f9d3868b7e26325a7cf9ba0e700c9bce9f272b12 Mon Sep 17 00:00:00 2001 From: kingoftech-v01 Date: Fri, 10 Apr 2026 05:21:38 +0000 Subject: [PATCH] fix: seed ItemBankMixin.make_selection to eliminate selection race The XBlock runtime's selected_children() and the ContentLibraryTransformer both call ItemBankMixin.make_selection() independently on a single MFE page load. Previously each path instantiated an unseeded random.Random(), so the two code paths picked different random subsets for the same (user, block). Whichever wrote to StudentModule last won, so after the learner completed the items rendered by the first path, a refresh would load the other path's selection and vertical_is_complete would report the vertical as incomplete, causing the green completion checkmark to disappear. Add an optional, keyword-only seed argument to ItemBankMixin.make_selection and thread a stable f"{user_id}:{block_usage_key}" seed through both call sites. Both paths now converge on the same selection by construction, so the ordering of parallel requests no longer matters. Route the final shuffle through the seeded Random instance so the order of the returned selection is also deterministic. Closes #38027 --- .../transformers/library_content.py | 6 +- xmodule/item_bank_block.py | 19 ++++- xmodule/tests/test_item_bank.py | 81 +++++++++++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/course_blocks/transformers/library_content.py b/lms/djangoapps/course_blocks/transformers/library_content.py index 6a029fbe2832..fc98ffb372b0 100644 --- a/lms/djangoapps/course_blocks/transformers/library_content.py +++ b/lms/djangoapps/course_blocks/transformers/library_content.py @@ -112,7 +112,11 @@ def transform_block_filters(self, usage_info, block_structure): if block_class is None: logger.error('Failed to load block class for %s', block_key) continue - block_keys = block_class.make_selection(selected, library_children, max_count) + # Seed deterministically so this transformer and the XBlock runtime's + # selected_children() path converge on the same selection for the same + # (user, block). See issue #38027. + seed = f"{usage_info.user.id}:{block_key}" + block_keys = block_class.make_selection(selected, library_children, max_count, seed=seed) selected = block_keys['selected'] # Save back any changes diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index 5a5ab0400815..5c49885f3fb2 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -93,7 +93,7 @@ def completion_mode(cls): # pylint: disable=no-self-argument return XBlockCompletionMode.AGGREGATOR @classmethod - def make_selection(cls, selected, children, max_count): + def make_selection(cls, selected, children, max_count, *, seed=None): """ Dynamically selects block_ids indicating which of the possible children are displayed to the current user. @@ -101,6 +101,11 @@ def make_selection(cls, selected, children, max_count): selected - list of (block_type, block_id) pairs assigned to this student children - children of this block max_count - number of components to display to each student + seed - optional deterministic seed (any hashable). When provided, two calls with the + same seed, children, and max_count produce the same selection. Callers that run + in independent code paths (e.g. the XBlock runtime and the ContentLibraryTransformer) + MUST pass a stable seed derived from (user_id, block_usage_key) so that both paths + converge on the same selection. See issue #38027. NOTE/TODO: We like to treat `self.selected` as a list of 2-tuples, but when we load a block from persistence, all @@ -124,7 +129,7 @@ def make_selection(cls, selected, children, max_count): Order of invalid/overlimit/added is ALPHABETICAL, by type then id. This is arbitrary, but it makes the events more consistent and easier to test. """ - rand = random.Random() + rand = random.Random(seed) selected_keys = {tuple(k) for k in selected} # set of (block_type, block_id) tuples assigned to this student @@ -156,7 +161,7 @@ def make_selection(cls, selected, children, max_count): if any((invalid_block_keys, overlimit_block_keys, added_block_keys)): selected = list(selected_keys) - random.shuffle(selected) + rand.shuffle(selected) return { 'selected': selected, @@ -249,7 +254,13 @@ def selected_children(self): if max_count < 0: max_count = len(self.children) - block_keys = self.make_selection(self.selected, self.children, max_count) # pylint: disable=no-member + # Seed deterministically so the XBlock runtime and ContentLibraryTransformer, + # which both call make_selection independently on a page load, converge on the + # same selection. See issue #38027. + seed = f"{self.get_user_id()}:{self.usage_key}" + block_keys = self.make_selection( # pylint: disable=no-member + self.selected, self.children, max_count, seed=seed, + ) self.publish_selected_children_events( block_keys, diff --git a/xmodule/tests/test_item_bank.py b/xmodule/tests/test_item_bank.py index 2063381670d7..52def2852579 100644 --- a/xmodule/tests/test_item_bank.py +++ b/xmodule/tests/test_item_bank.py @@ -520,3 +520,84 @@ def shuffle(_self, selection): return mock_result return shuffle + + +class ItemBankMakeSelectionDeterminismTests(MixedSplitTestCase): + """ + Regression tests for bug #38027: make_selection must be deterministic + across independent call sites when a seed is supplied, so that the XBlock + runtime and ContentLibraryTransformer converge on the same random subset + for a given (user, block) pair. + """ + + @staticmethod + def _children_pool(count=10): + """Build a simple children pool for make_selection.""" + return [Mock(block_type="problem", block_id=f"p{i}") for i in range(count)] + + def test_unit_make_selection_with_seed_is_deterministic(self): + """Same seed produces the same selection on every call.""" + children = self._children_pool() + seed = "user-42:block-foo/bar" + first = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + for _ in range(5): + again = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + assert first['selected'] == again['selected'] + assert first['added'] == again['added'] + + def test_unit_make_selection_different_seeds_can_differ(self): + """Different seeds across many users should produce at least two distinct subsets.""" + children = self._children_pool() + subsets = set() + for uid in range(20): + result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=f"user-{uid}:block-x", + ) + subsets.add(tuple(sorted(result['selected']))) + assert len(subsets) > 1 + + def test_make_selection_preserves_signature_without_seed(self): + """ + Backward compatibility: callers that don't pass seed still work + (keyword-only default is None). + """ + children = self._children_pool() + result = ItemBankBlock.make_selection([], children, max_count=2) + assert len(result['selected']) == 2 + + def test_integration_transformer_and_xblock_seed_match(self): + """ + Integration test for bug #38027: the XBlock runtime and transformer + produce the same selection for the same (user, block) pair because + they use the same seed format. + """ + children = self._children_pool(8) + user_id = 777 + block_key = "block-v1:Org+Course+Run+type@itembank+block@abc" + seed_from_xblock = f"{user_id}:{block_key}" + seed_from_transformer = f"{user_id}:{block_key}" + + xblock_result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=seed_from_xblock, + ) + transformer_result = ItemBankBlock.make_selection( + [], children, max_count=3, seed=seed_from_transformer, + ) + assert xblock_result['selected'] == transformer_result['selected'] + + def test_bug_38027_regression_selection_stable_across_rerun(self): + """ + Regression for bug #38027: re-running make_selection with a non-empty + 'selected' from a previous run must be a no-op (no shuffle, no + additions), so the learner's checkmark does not disappear on refresh. + """ + children = self._children_pool(8) + seed = "user-777:block-abc" + first = ItemBankBlock.make_selection([], children, max_count=3, seed=seed) + second = ItemBankBlock.make_selection( + first['selected'], children, max_count=3, seed=seed, + ) + assert second['added'] == [] + assert second['invalid'] == [] + assert second['overlimit'] == [] + assert sorted(second['selected']) == sorted(first['selected'])