diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java index 87d917905d3..6feb998fec9 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java @@ -204,7 +204,7 @@ public void setMonitorTagMap(Map monitorTagMap) { restartSolver = checkProblemFactChanges(); } outerSolvingEnded(solverScope); - return solverScope.getBestSolution(); + return solverScope.cloneBestSolution(); } public void outerSolvingStarted(SolverScope solverScope) { diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java index 8a91bf010cc..a1357b1504b 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/event/SolverEventSupport.java @@ -24,7 +24,10 @@ public void fireBestSolutionChanged(SolverScope 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); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java index da841efcfe6..ac7fe8b5808 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/solver/recaller/BestSolutionRecaller.java @@ -131,11 +131,12 @@ public void updateBestSolutionAndFireIfInitialized(SolverScope solver private void updateBestSolutionAndFire(SolverScope 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 solverScope) { + // We clone the existing working solution to set it as the best current solution var newBestSolution = solverScope.getScoreDirector().cloneWorkingSolution(); var newBestScore = solverScope.getSolutionDescriptor(). getScore(newBestSolution); var innerScore = InnerScore.withUnassignedCount(newBestScore, -solverScope.getScoreDirector().getWorkingInitScore()); @@ -144,11 +145,10 @@ private void updateBestSolutionWithoutFiring(SolverScope solverScope) private void updateBestSolutionWithoutFiring(SolverScope 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()); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java index 8305c56dfcc..20f9f83cf7b 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java +++ b/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 createChildThreadSolverScope(ChildThreadType childThreadType) { diff --git a/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java b/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java index 058625f2dd2..e86fadf5dd0 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhaseTest.java +++ b/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 diff --git a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java b/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java index 7c18f79254c..f11baee1ddd 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/solver/BestSolutionHolderTest.java +++ b/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. 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() + .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 future) { boolean isDone() {