chore: fail fast when score corruption detected (TimefoldAI#1411)

Also add a whole bunch of unrelated fixes to flaky tests, because with this PR, they started showing up more frequently. Closes TimefoldAI#1402 --------- Co-authored-by: Frederico Gonçalves <[email protected]>
triceo · Feb 25, 2025 · cf65791 · cf65791
1 parent c8b63e6
commit cf65791
Show file tree

Hide file tree

Showing 81 changed files with 1,008 additions and 595 deletions.
diff --git a/benchmark/src/main/java/ai/timefold/solver/benchmark/impl/report/BenchmarkReport.java b/benchmark/src/main/java/ai/timefold/solver/benchmark/impl/report/BenchmarkReport.java
@@ -316,10 +316,11 @@ public List<String> getWarningList() {
                     + " Maybe reduce the parallelBenchmarkCount.");
         }
         EnvironmentMode environmentMode = plannerBenchmarkResult.getEnvironmentMode();
-        if (environmentMode != null && environmentMode.isAsserted()) {
-            warningList.add("The environmentMode (" + environmentMode + ") is asserting."
-                    + " This decreases performance."
-                    + " Maybe set the environmentMode to " + EnvironmentMode.REPRODUCIBLE + ".");
+        if (environmentMode != null && environmentMode.isStepAssertOrMore()) {
+            // Phase assert performance impact is negligible.
+            warningList.add(
+                    "The environmentMode (%s) is step-asserting or more. This decreases performance. Maybe set the environmentMode to %s."
+                            .formatted(environmentMode, EnvironmentMode.PHASE_ASSERT));
         }
         LoggingLevel loggingLevelTimefoldCore = plannerBenchmarkResult.getLoggingLevelTimefoldSolverCore();
         if (loggingLevelTimefoldCore == LoggingLevel.TRACE) {

diff --git a/benchmark/src/main/resources/benchmark.xsd b/benchmark/src/main/resources/benchmark.xsd
@@ -2630,9 +2630,18 @@
       <xs:enumeration value="FAST_ASSERT"/>
 
 
+      <xs:enumeration value="STEP_ASSERT"/>
+
+
+      <xs:enumeration value="PHASE_ASSERT"/>
+
+
       <xs:enumeration value="REPRODUCIBLE"/>
 
 
+      <xs:enumeration value="NO_ASSERT"/>
+
+
       <xs:enumeration value="NON_REPRODUCIBLE"/>
 
 

diff --git a/benchmark/src/test/java/ai/timefold/solver/benchmark/api/PlannerBenchmarkFactoryTest.java b/benchmark/src/test/java/ai/timefold/solver/benchmark/api/PlannerBenchmarkFactoryTest.java
@@ -20,11 +20,11 @@
 import ai.timefold.solver.core.api.solver.DivertingClassLoader;
 import ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig;
 import ai.timefold.solver.core.config.solver.SolverConfig;
-import ai.timefold.solver.core.impl.phase.custom.NoChangeCustomPhaseCommand;
 import ai.timefold.solver.core.impl.testdata.domain.TestdataEntity;
 import ai.timefold.solver.core.impl.testdata.domain.TestdataSolution;
 import ai.timefold.solver.core.impl.testdata.domain.TestdataValue;
 import ai.timefold.solver.core.impl.testdata.util.PlannerTestUtils;
+import ai.timefold.solver.core.impl.testutil.NoChangeCustomPhaseCommand;
 
 import org.jspecify.annotations.NonNull;
 import org.junit.jupiter.api.BeforeAll;

diff --git a/core/src/build/revapi-differences.json b/core/src/build/revapi-differences.json
@@ -102,6 +102,44 @@
           "oldValue": "{\"terminationClass\", \"terminationCompositionStyle\", \"spentLimit\", \"millisecondsSpentLimit\", \"secondsSpentLimit\", \"minutesSpentLimit\", \"hoursSpentLimit\", \"daysSpentLimit\", \"unimprovedSpentLimit\", \"unimprovedMillisecondsSpentLimit\", \"unimprovedSecondsSpentLimit\", \"unimprovedMinutesSpentLimit\", \"unimprovedHoursSpentLimit\", \"unimprovedDaysSpentLimit\", \"unimprovedScoreDifferenceThreshold\", \"bestScoreLimit\", \"bestScoreFeasible\", \"stepCountLimit\", \"unimprovedStepCountLimit\", \"scoreCalculationCountLimit\", \"terminationConfigList\"}",
           "newValue": "{\"terminationClass\", \"terminationCompositionStyle\", \"diminishedReturnsConfig\", \"spentLimit\", \"millisecondsSpentLimit\", \"secondsSpentLimit\", \"minutesSpentLimit\", \"hoursSpentLimit\", \"daysSpentLimit\", \"unimprovedSpentLimit\", \"unimprovedMillisecondsSpentLimit\", \"unimprovedSecondsSpentLimit\", \"unimprovedMinutesSpentLimit\", \"unimprovedHoursSpentLimit\", \"unimprovedDaysSpentLimit\", \"unimprovedScoreDifferenceThreshold\", \"bestScoreLimit\", \"bestScoreFeasible\", \"stepCountLimit\", \"unimprovedStepCountLimit\", \"scoreCalculationCountLimit\", \"moveCountLimit\", \"terminationConfigList\"}",
           "justification": "Added support for the new diminished returns termination type"
+        },
+        {
+          "ignore": true,
+          "code": "java.method.returnTypeTypeParametersChanged",
+          "old": "method java.util.List<java.lang.Class<? extends ai.timefold.solver.core.impl.phase.custom.CustomPhaseCommand>> ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::getCustomPhaseCommandClassList()",
+          "new": "method java.util.List<java.lang.Class<? extends ai.timefold.solver.core.api.solver.phase.PhaseCommand>> ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::getCustomPhaseCommandClassList()",
+          "justification": "PhaseCommand is the new interface for custom phase commands, CustomPhaseCommand is deprecated and extends it"
+        },
+        {
+          "ignore": true,
+          "code": "java.method.returnTypeTypeParametersChanged",
+          "old": "method java.util.List<ai.timefold.solver.core.impl.phase.custom.CustomPhaseCommand> ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::getCustomPhaseCommandList()",
+          "new": "method java.util.List<? extends ai.timefold.solver.core.api.solver.phase.PhaseCommand> ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::getCustomPhaseCommandList()",
+          "justification": "PhaseCommand is the new interface for custom phase commands, CustomPhaseCommand is deprecated and extends it"
+        },
+        {
+          "ignore": true,
+          "code": "java.method.parameterTypeParameterChanged",
+          "old": "parameter void ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::setCustomPhaseCommandClassList(===java.util.List<java.lang.Class<? extends ai.timefold.solver.core.impl.phase.custom.CustomPhaseCommand>>===)",
+          "new": "parameter void ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::setCustomPhaseCommandClassList(===java.util.List<java.lang.Class<? extends ai.timefold.solver.core.api.solver.phase.PhaseCommand>>===)",
+          "parameterIndex": "0",
+          "justification": "PhaseCommand is the new interface for custom phase commands, CustomPhaseCommand is deprecated and extends it"
+        },
+        {
+          "ignore": true,
+          "code": "java.method.parameterTypeParameterChanged",
+          "old": "parameter void ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::setCustomPhaseCommandList(===java.util.List<ai.timefold.solver.core.impl.phase.custom.CustomPhaseCommand>===)",
+          "new": "parameter void ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig::setCustomPhaseCommandList(===java.util.List<? extends ai.timefold.solver.core.api.solver.phase.PhaseCommand>===)",
+          "parameterIndex": "0",
+          "justification": "PhaseCommand is the new interface for custom phase commands, CustomPhaseCommand is deprecated and extends it"
+        },
+        {
+          "ignore": true,
+          "code": "java.annotation.added",
+          "old": "class ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig",
+          "new": "class ai.timefold.solver.core.config.phase.custom.CustomPhaseConfig",
+          "annotation": "@org.jspecify.annotations.NullMarked",
+          "justification": "Less verbose nullability, no practical impact."
         }
       ]
     }

diff --git a/core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommand.java b/core/src/main/java/ai/timefold/solver/core/api/solver/phase/PhaseCommand.java
@@ -0,0 +1,46 @@
+package ai.timefold.solver.core.api.solver.phase;
+
+import java.util.function.BooleanSupplier;
+
+import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
+import ai.timefold.solver.core.api.score.Score;
+import ai.timefold.solver.core.api.score.director.ScoreDirector;
+import ai.timefold.solver.core.api.solver.Solver;
+import ai.timefold.solver.core.api.solver.change.ProblemChange;
+import ai.timefold.solver.core.impl.phase.Phase;
+import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
+
+import org.jspecify.annotations.NullMarked;
+
+/**
+ * Runs a custom algorithm as a {@link Phase} of the {@link Solver} that changes the planning variables.
+ * To change problem facts, use {@link Solver#addProblemChange(ProblemChange)} instead.
+ * <p>
+ * To add custom properties, configure custom properties and add public setters for them.
+ *
+ * @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
+ */
+@NullMarked
+public interface PhaseCommand<Solution_> {
+
+    /**
+     * Changes {@link PlanningSolution working solution} of {@link ScoreDirector#getWorkingSolution()}.
+     * When the {@link PlanningSolution working solution} is modified,
+     * the {@link ScoreDirector} must be correctly notified
+     * (through {@link ScoreDirector#beforeVariableChanged(Object, String)} and
+     * {@link ScoreDirector#afterVariableChanged(Object, String)}),
+     * otherwise calculated {@link Score}s will be corrupted.
+     * <p>
+     * Don't forget to call {@link ScoreDirector#triggerVariableListeners()} after each set of changes
+     * (especially before every {@link InnerScoreDirector#calculateScore()} call)
+     * to ensure all shadow variables are updated.
+     *
+     * @param scoreDirector the {@link ScoreDirector} that needs to get notified of the changes.
+     * @param isPhaseTerminated long-running command implementations should check this periodically
+     *        and terminate early if it returns true.
+     *        Otherwise the terminations configured by the user will have no effect,
+     *        as the solver can only terminate itself when a command has ended.
+     */
+    void changeWorkingSolution(ScoreDirector<Solution_> scoreDirector, BooleanSupplier isPhaseTerminated);
+
+}
diff --git a/core/src/main/java/ai/timefold/solver/core/config/phase/NoChangePhaseConfig.java b/core/src/main/java/ai/timefold/solver/core/config/phase/NoChangePhaseConfig.java
@@ -4,6 +4,7 @@
 
 import org.jspecify.annotations.NonNull;
 
+@Deprecated(forRemoval = true, since = "1.20.0")
 public class NoChangePhaseConfig extends PhaseConfig<NoChangePhaseConfig> {
 
     public static final String XML_ELEMENT_NAME = "noChangePhase";

diff --git a/core/src/main/java/ai/timefold/solver/core/config/phase/custom/CustomPhaseConfig.java b/core/src/main/java/ai/timefold/solver/core/config/phase/custom/CustomPhaseConfig.java
@@ -12,18 +12,19 @@
 import jakarta.xml.bind.annotation.XmlType;
 import jakarta.xml.bind.annotation.adapters.XmlJavaTypeAdapter;
 
+import ai.timefold.solver.core.api.solver.phase.PhaseCommand;
 import ai.timefold.solver.core.config.phase.PhaseConfig;
 import ai.timefold.solver.core.config.util.ConfigUtils;
 import ai.timefold.solver.core.impl.io.jaxb.adapter.JaxbCustomPropertiesAdapter;
-import ai.timefold.solver.core.impl.phase.custom.CustomPhaseCommand;
 
-import org.jspecify.annotations.NonNull;
+import org.jspecify.annotations.NullMarked;
 import org.jspecify.annotations.Nullable;
 
 @XmlType(propOrder = {
         "customPhaseCommandClassList",
         "customProperties",
 })
+@NullMarked
 public class CustomPhaseConfig extends PhaseConfig<CustomPhaseConfig> {
 
     public static final String XML_ELEMENT_NAME = "customPhase";
@@ -32,60 +33,57 @@ public class CustomPhaseConfig extends PhaseConfig<CustomPhaseConfig> {
     // and also because the input config file should match the output config file
 
     @XmlElement(name = "customPhaseCommandClass")
-    protected List<Class<? extends CustomPhaseCommand>> customPhaseCommandClassList = null;
+    protected List<Class<? extends PhaseCommand>> customPhaseCommandClassList = null;
 
     @XmlJavaTypeAdapter(JaxbCustomPropertiesAdapter.class)
     protected Map<String, String> customProperties = null;
 
     @XmlTransient
-    protected List<CustomPhaseCommand> customPhaseCommandList = null;
+    protected List<? extends PhaseCommand> customPhaseCommandList = null;
 
     // ************************************************************************
     // Constructors and simple getters/setters
     // ************************************************************************
 
-    public @Nullable List<Class<? extends CustomPhaseCommand>> getCustomPhaseCommandClassList() {
+    public @Nullable List<Class<? extends PhaseCommand>> getCustomPhaseCommandClassList() {
         return customPhaseCommandClassList;
     }
 
-    public void setCustomPhaseCommandClassList(
-            @Nullable List<Class<? extends CustomPhaseCommand>> customPhaseCommandClassList) {
+    public void setCustomPhaseCommandClassList(@Nullable List<Class<? extends PhaseCommand>> customPhaseCommandClassList) {
         this.customPhaseCommandClassList = customPhaseCommandClassList;
     }
 
-    public @Nullable Map<@NonNull String, @NonNull String> getCustomProperties() {
+    public @Nullable Map<String, String> getCustomProperties() {
         return customProperties;
     }
 
-    public void setCustomProperties(@Nullable Map<@NonNull String, @NonNull String> customProperties) {
+    public void setCustomProperties(@Nullable Map<String, String> customProperties) {
         this.customProperties = customProperties;
     }
 
-    public @Nullable List<@NonNull CustomPhaseCommand> getCustomPhaseCommandList() {
+    public @Nullable List<? extends PhaseCommand> getCustomPhaseCommandList() {
         return customPhaseCommandList;
     }
 
-    public void setCustomPhaseCommandList(@Nullable List<@NonNull CustomPhaseCommand> customPhaseCommandList) {
+    public void setCustomPhaseCommandList(@Nullable List<? extends PhaseCommand> customPhaseCommandList) {
         this.customPhaseCommandList = customPhaseCommandList;
     }
 
     // ************************************************************************
     // With methods
     // ************************************************************************
 
-    public @NonNull CustomPhaseConfig withCustomPhaseCommandClassList(
-            @NonNull List<@NonNull Class<? extends CustomPhaseCommand>> customPhaseCommandClassList) {
+    public CustomPhaseConfig withCustomPhaseCommandClassList(List<Class<? extends PhaseCommand>> customPhaseCommandClassList) {
         this.customPhaseCommandClassList = customPhaseCommandClassList;
         return this;
     }
 
-    public @NonNull CustomPhaseConfig withCustomProperties(@NonNull Map<@NonNull String, @NonNull String> customProperties) {
+    public CustomPhaseConfig withCustomProperties(Map<String, String> customProperties) {
         this.customProperties = customProperties;
         return this;
     }
 
-    public @NonNull CustomPhaseConfig
-            withCustomPhaseCommandList(@NonNull List<@NonNull CustomPhaseCommand> customPhaseCommandList) {
+    public CustomPhaseConfig withCustomPhaseCommandList(List<? extends PhaseCommand> customPhaseCommandList) {
         boolean hasNullCommand = Objects.requireNonNullElse(customPhaseCommandList, Collections.emptyList())
                 .stream().anyMatch(Objects::isNull);
         if (hasNullCommand) {
@@ -96,8 +94,7 @@ public void setCustomPhaseCommandList(@Nullable List<@NonNull CustomPhaseCommand
         return this;
     }
 
-    public <Solution_> @NonNull CustomPhaseConfig
-            withCustomPhaseCommands(@NonNull CustomPhaseCommand<Solution_> @NonNull... customPhaseCommands) {
+    public <Solution_> CustomPhaseConfig withCustomPhaseCommands(PhaseCommand<Solution_>... customPhaseCommands) {
         boolean hasNullCommand = Arrays.stream(customPhaseCommands).anyMatch(Objects::isNull);
         if (hasNullCommand) {
             throw new IllegalArgumentException(
@@ -107,25 +104,26 @@ public void setCustomPhaseCommandList(@Nullable List<@NonNull CustomPhaseCommand
         return this;
     }
 
+    @SuppressWarnings({ "unchecked", "rawtypes" })
     @Override
-    public @NonNull CustomPhaseConfig inherit(@NonNull CustomPhaseConfig inheritedConfig) {
+    public CustomPhaseConfig inherit(CustomPhaseConfig inheritedConfig) {
         super.inherit(inheritedConfig);
         customPhaseCommandClassList = ConfigUtils.inheritMergeableListProperty(
                 customPhaseCommandClassList, inheritedConfig.getCustomPhaseCommandClassList());
         customPhaseCommandList = ConfigUtils.inheritMergeableListProperty(
-                customPhaseCommandList, inheritedConfig.getCustomPhaseCommandList());
+                customPhaseCommandList, (List) inheritedConfig.getCustomPhaseCommandList());
         customProperties = ConfigUtils.inheritMergeableMapProperty(
                 customProperties, inheritedConfig.getCustomProperties());
         return this;
     }
 
     @Override
-    public @NonNull CustomPhaseConfig copyConfig() {
+    public CustomPhaseConfig copyConfig() {
         return new CustomPhaseConfig().inherit(this);
     }
 
     @Override
-    public void visitReferencedClasses(@NonNull Consumer<Class<?>> classVisitor) {
+    public void visitReferencedClasses(Consumer<Class<?>> classVisitor) {
         if (terminationConfig != null) {
             terminationConfig.visitReferencedClasses(classVisitor);
         }