chore: address comments

Author: zepfred

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

@@ -118,33 +118,25 @@ public <Score_ extends Score<Score_>> void processWorkingSolutionDuringMove(Inne
 
     public void updateBestSolutionAndFire(SolverScope<Solution_> solverScope) {
         updateBestSolutionWithoutFiring(solverScope);
-        // We need to clone the solution again, or we may use the same instance employed by the solver scope,
-        // which is restored between solver phases.
-        // The operation won't block the solving process.
-        solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution());
+        solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution());
     }
 
     public void updateBestSolutionAndFireIfInitialized(SolverScope<Solution_> solverScope) {
         updateBestSolutionWithoutFiring(solverScope);
         if (solverScope.isBestSolutionInitialized()) {
-            // We need to clone the solution again, or we may use the same instance employed by the solver scope,
-            // which is restored between solver phases.
-            // The operation won't block the solving process.
-            solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution());
+            solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.getBestSolution());
         }
     }
 
     private void updateBestSolutionAndFire(SolverScope<Solution_> solverScope, InnerScore<?> bestScore,
             Solution_ bestSolution) {
         updateBestSolutionWithoutFiring(solverScope, bestScore, bestSolution);
-        // We need to clone the solution again, or we may use the same instance employed by the solver scope,
-        // which is restored between solver phases.
-        // The operation won't block the solving process.
-        solverEventSupport.fireBestSolutionChanged(solverScope, solverScope.cloneBestSolution());
+        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());

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

@@ -205,6 +205,7 @@ void testCloningBetweenPhases() throws ExecutionException, InterruptedException
                     .withProblem(problem)
                     .withBestSolutionConsumer(
                             intermediate -> {
+                                // Add a new entity to the cloned solution
                                 intermediate.getEntityList().add(new TestdataEntity("Bad Entity 3"));
                                 countDownLatch.countDown();
                             })
@@ -213,6 +214,7 @@ void testCloningBetweenPhases() throws ExecutionException, InterruptedException
                     .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);
         }
     }