fix: avoid using stale references in entity value ranges filtering nodes (#1822)

(cherry picked from commit fa704e9)

Author: zepfred

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/iterator/AbstractRandomSwapIterator.java

@@ -5,16 +5,14 @@
 import ai.timefold.solver.core.impl.heuristic.move.Move;
 
 public abstract class AbstractRandomSwapIterator<Solution_, Move_ extends Move<Solution_>, SubSelection_>
-        implements Iterator<Move_> {
+        extends UpcomingSelectionIterator<Move_> {
 
     protected final Iterable<SubSelection_> leftSubSelector;
     protected final Iterable<SubSelection_> rightSubSelector;
 
     protected Iterator<SubSelection_> leftSubSelectionIterator;
     protected Iterator<SubSelection_> rightSubSelectionIterator;
 
-    private SubSelection_ leftSubSelection;
-
     public AbstractRandomSwapIterator(Iterable<SubSelection_> leftSubSelector,
             Iterable<SubSelection_> rightSubSelector) {
         this.leftSubSelector = leftSubSelector;
@@ -25,34 +23,24 @@ public AbstractRandomSwapIterator(Iterable<SubSelection_> leftSubSelector,
     }
 
     @Override
-    public boolean hasNext() {
-        leftSubSelection = null;
+    protected Move_ createUpcomingSelection() {
+        // Ideally, this code should have read:
+        //     SubS leftSubSelection = leftSubSelectionIterator.next();
+        //     SubS rightSubSelection = rightSubSelectionIterator.next();
+        // But empty selectors and ending selectors (such as non-random or shuffled) make it more complex
         if (!leftSubSelectionIterator.hasNext()) {
             leftSubSelectionIterator = leftSubSelector.iterator();
             if (!leftSubSelectionIterator.hasNext()) {
-                return false;
+                return noUpcomingSelection();
             }
         }
-        // The right selection may depend on selecting a left element first. E.g., FilteringEntityByEntitySelector
-        leftSubSelection = leftSubSelectionIterator.next();
+        SubSelection_ leftSubSelection = leftSubSelectionIterator.next();
         if (!rightSubSelectionIterator.hasNext()) {
             rightSubSelectionIterator = rightSubSelector.iterator();
             if (!rightSubSelectionIterator.hasNext()) {
-                return false;
+                return noUpcomingSelection();
             }
         }
-        return true;
-    }
-
-    @Override
-    public Move_ next() {
-        if (leftSubSelection == null) {
-            throw new IllegalStateException("Impossible state: no left element has been selected.");
-        }
-        // Ideally, this code should have read:
-        //     SubS leftSubSelection = leftSubSelectionIterator.next();
-        //     SubS rightSubSelection = rightSubSelectionIterator.next();
-        // But empty selectors and ending selectors (such as non-random or shuffled) make it more complex
         SubSelection_ rightSubSelection = rightSubSelectionIterator.next();
         return newSwapSelection(leftSubSelection, rightSubSelection);
     }

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityByEntitySelector.java

@@ -71,11 +71,6 @@ public final class FilteringEntityByEntitySelector<Solution_> extends AbstractDe
     private ValueRangeManager<Solution_> valueRangeManager;
     private List<Object> allEntities;
 
-    private static long countIterations = 0;
-    private static long countAttempts = 0;
-    private static long countSuccess = 0;
-    private static long countFails = 0;
-
     public FilteringEntityByEntitySelector(EntitySelector<Solution_> childEntitySelector,
             EntitySelector<Solution_> replayingEntitySelector, boolean randomSelection) {
         this.childEntitySelector = childEntitySelector;
@@ -123,11 +118,6 @@ public void phaseEnded(AbstractPhaseScope<Solution_> phaseScope) {
         this.replayedEntity = null;
         this.valueRangeManager = null;
         this.basicVariableDescriptors = null;
-        logger.warn(
-                "Iterations ({}), total attempts ({}), average attempts ({}), succeed count ({}), succeed rate ({}), fails count({}), fails rate ({})",
-                countIterations, countAttempts, countAttempts / (double) countIterations, countSuccess,
-                countSuccess / (double) countIterations, countFails,
-                countFails / (double) countIterations);
     }
 
     // ************************************************************************
@@ -168,7 +158,7 @@ private Object selectReplayedEntity() {
 
     @Override
     public Iterator<Object> endingIterator() {
-        return new OriginalFilteringValueRangeIterator<>(this::selectReplayedEntity, childEntitySelector.iterator(),
+        return new OriginalFilteringValueRangeIterator<>(this::selectReplayedEntity, childEntitySelector.endingIterator(),
                 basicVariableDescriptors, valueRangeManager);
     }
 
@@ -179,12 +169,12 @@ public Iterator<Object> iterator() {
                 throw new IllegalArgumentException(
                         "Impossible state: childEntitySelector must provide a never-ending approach.");
             }
-            // Experiments have shown that a large number of attempts do not scale well,
-            // and 10 seems like an appropriate limit. 
+            // Experiments have shown that a large number of attempts do not scale well.
             // So we won't spend excessive time trying to generate a single move for the current selection.
             // If we are unable to generate a move, the move iterator can still be used in later iterations.
+            var maxBailoutSize = Math.min(10, allEntities.size());
             return new RandomFilteringValueRangeIterator<>(this::selectReplayedEntity, childEntitySelector.iterator(),
-                    basicVariableDescriptors, valueRangeManager, 10);
+                    basicVariableDescriptors, valueRangeManager, maxBailoutSize);
         } else {
             return new OriginalFilteringValueRangeIterator<>(this::selectReplayedEntity, childEntitySelector.iterator(),
                     basicVariableDescriptors, valueRangeManager);
@@ -350,13 +340,13 @@ void load(Object replayedEntity) {
 
         @Override
         public boolean hasNext() {
-            initialize();
+            checkReplayedEntity();
             return entityIterator.hasNext();
         }
 
         @Override
         public Object next() {
-            if (!hasNext()) {
+            if (!entityIterator.hasNext()) {
                 throw new NoSuchElementException();
             }
             var bailoutSize = maxBailoutSize;

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/entity/decorator/FilteringEntityByValueSelector.java

@@ -230,10 +230,7 @@ private RandomFilteringValueRangeIterator(Supplier<Object> upcomingValueSupplier
             this.workingRandom = workingRandom;
         }
 
-        private void initialize() {
-            if (entityList != null) {
-                return;
-            }
+        private void checkReplayedValue() {
             var oldUpcomingValue = currentUpcomingValue;
             currentUpcomingValue = upcomingValueSupplier.get();
             if (currentUpcomingValue == null) {
@@ -249,7 +246,7 @@ private void loadValues() {
 
         @Override
         public boolean hasNext() {
-            initialize();
+            checkReplayedValue();
             return entityList != null && !entityList.isEmpty();
         }
 

core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/value/decorator/FilteringValueRangeSelector.java

@@ -357,7 +357,7 @@ public Object next() {
     private class RandomFilteringValueRangeIterator extends AbstractFilteringValueRangeIterator {
 
         private final Random workingRandom;
-        private int maxBailoutSize = 1;
+        private int maxBailoutSize;
         private Object replayedValue;
         private List<Object> reachableValueList = null;
 
@@ -371,8 +371,8 @@ private RandomFilteringValueRangeIterator(Supplier<Object> upcomingValueSupplier
         @Override
         void processUpcomingValue(Object upcomingValue, List<Object> upcomingList) {
             this.replayedValue = upcomingValue;
-            this.reachableValueList = upcomingList;
-            this.maxBailoutSize = upcomingList.size();
+            this.reachableValueList = Objects.requireNonNull(upcomingList);
+            this.maxBailoutSize = reachableValueList.size();
         }
 
         @Override
@@ -386,12 +386,11 @@ public Object next() {
             if (hasNoData()) {
                 throw new NoSuchElementException();
             }
-            Object next;
             var bailoutSize = maxBailoutSize;
             do {
                 bailoutSize--;
-                var index = workingRandom.nextInt(Objects.requireNonNull(reachableValueList).size());
-                next = reachableValueList.get(index);
+                var index = workingRandom.nextInt(reachableValueList.size());
+                var next = reachableValueList.get(index);
                 if (isReachable(next)) {
                     return next;
                 }

core/src/main/java/ai/timefold/solver/core/impl/localsearch/DefaultLocalSearchPhase.java

@@ -142,16 +142,28 @@ public void stepEnded(LocalSearchStepScope<Solution_> stepScope) {
         collectMetrics(stepScope);
         var phaseScope = stepScope.getPhaseScope();
         if (logger.isDebugEnabled()) {
-            logger.debug("{}    LS step ({}), time spent ({}), score ({}), {} best score ({})," +
-                    " accepted/selected move count ({}/{}), picked move ({}).",
-                    logIndentation,
-                    stepScope.getStepIndex(),
-                    phaseScope.calculateSolverTimeMillisSpentUpToNow(),
-                    stepScope.getScore().raw(),
-                    (stepScope.getBestScoreImproved() ? "new" : "   "), phaseScope.getBestScore().raw(),
-                    stepScope.getAcceptedMoveCount(),
-                    stepScope.getSelectedMoveCount(),
-                    stepScope.getStepString());
+            if (stepScope.getAcceptedMoveCount() == 0 && phaseTermination.isPhaseTerminated(phaseScope)) {
+                // Terminated early
+                logger.debug("{}    LS step ({}), time spent ({}), score ({}), {} best score ({})," +
+                        " terminated prematurely after selecting {} moves.",
+                        logIndentation,
+                        stepScope.getStepIndex(),
+                        phaseScope.calculateSolverTimeMillisSpentUpToNow(),
+                        stepScope.getScore().raw(),
+                        (stepScope.getBestScoreImproved() ? "new" : "   "), phaseScope.getBestScore().raw(),
+                        stepScope.getSelectedMoveCount());
+            } else {
+                logger.debug("{}    LS step ({}), time spent ({}), score ({}), {} best score ({})," +
+                        " accepted/selected move count ({}/{}), picked move ({}).",
+                        logIndentation,
+                        stepScope.getStepIndex(),
+                        phaseScope.calculateSolverTimeMillisSpentUpToNow(),
+                        stepScope.getScore().raw(),
+                        (stepScope.getBestScoreImproved() ? "new" : "   "), phaseScope.getBestScore().raw(),
+                        stepScope.getAcceptedMoveCount(),
+                        stepScope.getSelectedMoveCount(),
+                        stepScope.getStepString());
+            }
         }
     }
 

core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java

@@ -213,7 +213,6 @@ void randomWithEntityValueRange() {
 
         // select C for V5 and first unpinned pos C[1]
         // 3 - pick random value in ElementPositionRandomIterator and force generating a random position
-        // 0 - pick entity C in RandomFilteringValueRangeIterator
         // 1 - pick random position, v3 and v4 are reachable
         // 0 - remaining call
         valueSelector = mockIterableValueSelector(getEntityRangeListVariableDescriptor(scoreDirector), v5, v5, v5, v5, v5); // simulate five positions
@@ -223,7 +222,7 @@ void randomWithEntityValueRange() {
         doReturn(List.of(v5).iterator(), Collections.emptyIterator()).when(valueSelector).iterator();
         checkEntityValueRange(new FilteringEntityByValueSelector<>(mockEntitySelector(a, b, c), valueSelector, true),
                 new FilteringValueRangeSelector<>(filteringValueRangeSelector, replayinValueSelector, true, false),
-                scoreDirector, new TestRandom(3, 0, 1, 0), "C[1]");
+                scoreDirector, new TestRandom(3, 1, 0), "C[1]");
     }
 
     private void checkEntityValueRange(FilteringEntityByValueSelector<TestdataListEntityProvidingSolution> entitySelector,

core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/SwapMoveSelectorTest.java

@@ -337,9 +337,8 @@ void singleVarRandomSelectionWithEntityValueRange() {
         e1.setValue(v1);
         e2.setValue(v3);
         e3.setValue(v4);
-        // select left A, select right B
         // select left A, select right C
-        random.reset(0, 1, 2, 1);
+        random.reset(0, 2, 1, 1, 1, 1);
         scoreDirector.setWorkingSolution(solution);
         assertCodesOfNeverEndingIterableSelector(moveSelector, expectedSize, "A<->C");
     }

core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListChangeMoveSelectorTest.java

@@ -411,8 +411,8 @@ void randomWithEntityValueRange() {
 
         assertCodesOfNeverEndingMoveSelector(moveSelector,
                 "1 {A[1]->A[1]}",
-                "3 {B[0]->A[0]}",
-                "3 {B[0]->A[0]}",
+                "3 {B[0]->B[0]}",
+                "3 {B[0]->B[0]}",
                 "3 {B[0]->B[1]}",
                 "3 {B[0]->B[1]}");
     }
@@ -422,7 +422,7 @@ void randomWithEntityValueRangeAndFiltering() {
         var v1 = new TestdataListEntityProvidingValue("1");
         var v2 = new TestdataListEntityProvidingValue("2");
         var v3 = new TestdataListEntityProvidingValue("3");
-        var a = new TestdataListEntityProvidingEntity("A", List.of(v1, v2), List.of(v2, v1));
+        var a = new TestdataListEntityProvidingEntity("A", List.of(v1, v2, v3), List.of(v2, v1));
         var b = new TestdataListEntityProvidingEntity("B", List.of(v2, v3), List.of(v3));
         var solution = new TestdataListEntityProvidingSolution();
         solution.setEntityList(List.of(a, b));
@@ -446,7 +446,7 @@ void randomWithEntityValueRangeAndFiltering() {
         assertCodesOfNeverEndingMoveSelector(moveSelector,
                 "1 {A[1]->A[1]}",
                 "3 {B[0]->A[0]}",
-                "3 {B[0]->A[0]}",
+                "3 {B[0]->B[0]}",
                 "3 {B[0]->A[2]}",
                 "3 {B[0]->A[1]}");
     }

core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ListSwapMoveSelectorTest.java

@@ -337,8 +337,7 @@ void randomWithEntityValueRange() {
                 "2 {A[0]} <-> 3 {B[0]}",
                 "1 {A[1]} <-> 2 {A[0]}",
                 "3 {B[0]} <-> 2 {A[0]}",
-                "1 {A[1]} <-> 2 {A[0]}",
-                "3 {B[0]} <-> 2 {A[0]}");
+                "1 {A[1]} <-> 2 {A[0]}");
     }
 
     @Test