fix: clone the solution before sending it to the consumer

Author: zepfred

core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java

@@ -204,7 +204,7 @@ public void setMonitorTagMap(Map<String, String> monitorTagMap) {
             restartSolver = checkProblemFactChanges();
         }
         outerSolvingEnded(solverScope);
-        return solverScope.getBestSolution();
+        return solverScope.cloneBestSolution();
     }
 
     public void outerSolvingStarted(SolverScope<Solution_> solverScope) {

core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java

@@ -24,7 +24,10 @@ public void fireBestSolutionChanged(SolverScope<Solution_> solverScope, Solution
         var timeMillisSpent = solverScope.getBestSolutionTimeMillisSpent();
         var bestScore = solverScope.getBestScore();
         if (it.hasNext()) {
-            var event = new BestSolutionChangedEvent<>(solver, timeMillisSpent, newBestSolution, bestScore.raw(),
+            // We need to clone the new best solution, or we may share the same instance with user consumers.
+            // Reusing the instance can lead to inconsistent states if intermediary consumers change the solution.
+            var newBestSolutionCloned = solverScope.getScoreDirector().cloneSolution(newBestSolution);
+            var event = new BestSolutionChangedEvent<>(solver, timeMillisSpent, newBestSolutionCloned, bestScore.raw(),
                     bestScore.fullyAssigned());
             do {
                 it.next().bestSolutionChanged(event);

core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java

@@ -131,11 +131,12 @@ public void updateBestSolutionAndFireIfInitialized(SolverScope<Solution_> solver
     private void updateBestSolutionAndFire(SolverScope<Solution_> solverScope, InnerScore<?> bestScore,
             Solution_ bestSolution) {
         updateBestSolutionWithoutFiring(solverScope, bestScore, bestSolution);
-        solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution());
+        solverEventSupport.fireBestSolutionChanged(solverScope, bestSolution);
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })
     private void updateBestSolutionWithoutFiring(SolverScope<Solution_> solverScope) {
+        // We clone the existing working solution to set it as the best current solution
         var newBestSolution = solverScope.getScoreDirector().cloneWorkingSolution();
         var newBestScore = solverScope.getSolutionDescriptor().<Score> getScore(newBestSolution);
         var innerScore = InnerScore.withUnassignedCount(newBestScore, -solverScope.getScoreDirector().getWorkingInitScore());
@@ -144,11 +145,10 @@ private void updateBestSolutionWithoutFiring(SolverScope<Solution_> solverScope)
 
     private void updateBestSolutionWithoutFiring(SolverScope<Solution_> solverScope, InnerScore<?> bestScore,
             Solution_ bestSolution) {
-        if (bestScore.fullyAssigned()) {
-            if (!solverScope.isBestSolutionInitialized()) {
-                solverScope.setStartingInitializedScore(bestScore.raw());
-            }
+        if (bestScore.fullyAssigned() && !solverScope.isBestSolutionInitialized()) {
+            solverScope.setStartingInitializedScore(bestScore.raw());
         }
+
         solverScope.setBestSolution(bestSolution);
         solverScope.setBestScore(bestScore);
         solverScope.setBestSolutionTimeMillis(solverScope.getClock().millis());

core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java

@@ -224,6 +224,10 @@ public Solution_ getBestSolution() {
         return bestSolution.get();
     }
 
+    public Solution_ cloneBestSolution() {
+        return scoreDirector.cloneSolution(bestSolution.get());
+    }
+
     /**
      * The {@link PlanningSolution best solution} must never be the same instance
      * as the {@link PlanningSolution working solution}, it should be a (un)changed clone.
@@ -332,7 +336,7 @@ public long getMoveEvaluationSpeed() {
 
     public void setWorkingSolutionFromBestSolution() {
         // The workingSolution must never be the same instance as the bestSolution.
-        scoreDirector.setWorkingSolution(scoreDirector.cloneSolution(getBestSolution()));
+        scoreDirector.setWorkingSolution(cloneBestSolution());
     }
 
     public SolverScope<Solution_> createChildThreadSolverScope(ChildThreadType childThreadType) {

core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java

@@ -91,7 +91,8 @@ void solveWithInitializedSolution() {
                 new TestdataEntity("e3", v3)));
 
         var solution = PlannerTestUtils.solve(solverConfig, inputProblem, false);
-        assertThat(inputProblem).isSameAs(solution);
+        assertThat(inputProblem.getEntityList().stream().map(TestdataEntity::getValue).toList())
+                .isEqualTo(solution.getEntityList().stream().map(TestdataEntity::getValue).toList());
     }
 
     @Test

core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java

@@ -13,14 +13,17 @@
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 
 import ai.timefold.solver.core.api.solver.Solver;
+import ai.timefold.solver.core.api.solver.SolverConfigOverride;
 import ai.timefold.solver.core.api.solver.SolverManager;
 import ai.timefold.solver.core.api.solver.change.ProblemChange;
 import ai.timefold.solver.core.api.solver.change.ProblemChangeDirector;
 import ai.timefold.solver.core.config.solver.SolverConfig;
 import ai.timefold.solver.core.config.solver.SolverManagerConfig;
+import ai.timefold.solver.core.config.solver.termination.TerminationConfig;
 import ai.timefold.solver.core.testdomain.TestdataEasyScoreCalculator;
 import ai.timefold.solver.core.testdomain.TestdataEntity;
 import ai.timefold.solver.core.testdomain.TestdataSolution;
@@ -186,6 +189,36 @@ void problemChangeBarrageIntermediateBestSolutionConsumer() throws InterruptedEx
 
     }
 
+    @Test
+    void testCloningBetweenPhases() throws ExecutionException, InterruptedException {
+        var solverConfig = new SolverConfig()
+                .withSolutionClass(TestdataSolution.class)
+                .withEntityClasses(TestdataEntity.class)
+                .withEasyScoreCalculatorClass(TestdataEasyScoreCalculator.class);
+
+        try (var solverManager = SolverManager.<TestdataSolution, UUID> create(solverConfig, new SolverManagerConfig())) {
+            var problem = TestdataSolution.generateSolution(2, 2);
+            problem.getEntityList().forEach(entity -> entity.setValue(null));
+            var countDownLatch = new CountDownLatch(1);
+            var solution = solverManager.solveBuilder()
+                    .withProblemId(UUID.randomUUID())
+                    .withProblem(problem)
+                    .withBestSolutionConsumer(
+                            intermediate -> {
+                                // Add a new entity to the cloned solution
+                                intermediate.getEntityList().add(new TestdataEntity("Bad Entity 3"));
+                                countDownLatch.countDown();
+                            })
+                    .withConfigOverride(new SolverConfigOverride<TestdataSolution>()
+                            .withTerminationConfig(new TerminationConfig().withMoveCountLimit(100L)))
+                    .run()
+                    .getFinalBestSolution();
+            countDownLatch.await();
+            // At this point, the events generated by the best solution consumer should not add any new entities
+            assertThat(solution.getEntityList()).hasSize(2);
+        }
+    }
+
     private record RecordedFuture(int id, CompletableFuture<Void> future) {
 
         boolean isDone() {