Skip to content

Commit c373c2b

Browse files
fix: Remove too eager fail-fast in concat node's retract (#831)
A dead tuple might be retracted in concat if a "complex No-op" `calculateScore` occurs. A "complex No-op" refers to the following situlation: - Move A - Calculate Score - Undo Move A - Move A again (but potentially expressed a different way) - Calculate Score (here is where a dead tuple will be retracted) The fail-fast seems to be too eager.
1 parent 5bfa796 commit c373c2b

File tree

2 files changed

+80
-4
lines changed

2 files changed

+80
-4
lines changed

core/src/main/java/ai/timefold/solver/core/impl/score/stream/bavet/common/AbstractConcatNode.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,10 @@ public final void retractLeft(LeftTuple_ tuple) {
8787
}
8888
TupleState state = outTuple.state;
8989
if (!state.isActive()) {
90-
throw new IllegalStateException("Impossible state: The tuple (" + outTuple.state + ") in node (" + this
91-
+ ") is in an unexpected state (" + outTuple.state + ").");
90+
// No fail fast for inactive tuples, since the same tuple can be
91+
// passed twice if they are from the same source;
92+
// @see BavetRegressionTest#concatSameTupleDeadAndAlive for an example.
93+
return;
9294
}
9395
propagationQueue.retract(outTuple, state == TupleState.CREATING ? TupleState.ABORTING : TupleState.DYING);
9496
}
@@ -128,8 +130,10 @@ public final void retractRight(RightTuple_ tuple) {
128130
}
129131
TupleState state = outTuple.state;
130132
if (!state.isActive()) {
131-
throw new IllegalStateException("Impossible state: The tuple (" + outTuple.state + ") in node (" + this
132-
+ ") is in an unexpected state (" + outTuple.state + ").");
133+
// No fail fast for inactive tuples, since the same tuple can be
134+
// passed twice if they are from the same source;
135+
// @see BavetRegressionTest#concatSameTupleDeadAndAlive for an example.
136+
return;
133137
}
134138
propagationQueue.retract(outTuple, state == TupleState.CREATING ? TupleState.ABORTING : TupleState.DYING);
135139
}

core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -396,4 +396,76 @@ public void mapPlanningEntityChanges() {
396396
assertMatch(entity2));
397397
}
398398

399+
/**
400+
* @see <a href="https://github.com/TimefoldAI/timefold-solver/issues/828">Timefold Solver Github Issue 828</a>
401+
*/
402+
@TestTemplate
403+
public void concatSameTupleDeadAndAlive() {
404+
InnerScoreDirector<TestdataSolution, SimpleScore> scoreDirector =
405+
buildScoreDirector(TestdataSolution.buildSolutionDescriptor(),
406+
factory -> new Constraint[] {
407+
factory.forEach(TestdataEntity.class)
408+
.filter(e -> e.getValue().getCode().equals("A"))
409+
.concat(factory.forEach(TestdataEntity.class))
410+
.penalize(SimpleScore.ONE)
411+
.asConstraint(TEST_CONSTRAINT_NAME)
412+
});
413+
414+
TestdataSolution solution = TestdataSolution.generateSolution(2, 2);
415+
TestdataEntity entity1 = solution.getEntityList().get(0);
416+
TestdataEntity entity2 = solution.getEntityList().get(1);
417+
TestdataValue valueA = solution.getValueList().get(0);
418+
valueA.setCode("A");
419+
TestdataValue valueB = solution.getValueList().get(1);
420+
valueB.setCode("B");
421+
422+
scoreDirector.setWorkingSolution(solution);
423+
assertScore(scoreDirector,
424+
assertMatch(entity1),
425+
assertMatch(entity1),
426+
assertMatch(entity2));
427+
428+
scoreDirector.beforeVariableChanged(entity1, "value");
429+
entity1.setValue(valueB);
430+
scoreDirector.afterVariableChanged(entity1, "value");
431+
scoreDirector.beforeVariableChanged(entity2, "value");
432+
entity2.setValue(valueA);
433+
scoreDirector.afterVariableChanged(entity2, "value");
434+
assertScore(scoreDirector,
435+
assertMatch(entity1),
436+
assertMatch(entity2),
437+
assertMatch(entity2));
438+
439+
scoreDirector.beforeVariableChanged(entity1, "value");
440+
entity1.setValue(valueA);
441+
scoreDirector.afterVariableChanged(entity1, "value");
442+
scoreDirector.beforeVariableChanged(entity2, "value");
443+
entity2.setValue(valueB);
444+
scoreDirector.afterVariableChanged(entity2, "value");
445+
// Do not recalculate score, since this is undo
446+
447+
scoreDirector.beforeVariableChanged(entity2, "value");
448+
entity2.setValue(valueA);
449+
scoreDirector.afterVariableChanged(entity2, "value");
450+
scoreDirector.beforeVariableChanged(entity1, "value");
451+
entity1.setValue(valueB);
452+
scoreDirector.afterVariableChanged(entity1, "value");
453+
assertScore(scoreDirector,
454+
assertMatch(entity1),
455+
assertMatch(entity2),
456+
assertMatch(entity2));
457+
458+
scoreDirector.beforeVariableChanged(entity2, "value");
459+
entity2.setValue(valueB);
460+
scoreDirector.afterVariableChanged(entity2, "value");
461+
scoreDirector.beforeVariableChanged(entity1, "value");
462+
entity1.setValue(valueA);
463+
scoreDirector.afterVariableChanged(entity1, "value");
464+
465+
assertScore(scoreDirector,
466+
assertMatch(entity1),
467+
assertMatch(entity1),
468+
assertMatch(entity2));
469+
}
470+
399471
}

0 commit comments

Comments
 (0)