Skip to content

fix: fail fast on planning list variable sources#2469

Open
fodzal wants to merge 3 commits into
TimefoldAI:mainfrom
fodzal:fix/bare-list-source-staleness
Open

fix: fail fast on planning list variable sources#2469
fodzal wants to merge 3 commits into
TimefoldAI:mainfrom
fodzal:fix/bare-list-source-staleness

Conversation

@fodzal

@fodzal fodzal commented Jul 6, 2026

Copy link
Copy Markdown

Bug

A declarative shadow variable that uses a genuine @PlanningListVariable as a source,
e.g. @ShadowSources("values"), is computed once when the variable reference graph is
built and then never updated again during solving: list variable change events are only
delivered to variable listeners, never to the declarative shadow variable session, so the
processor registered for the list variable never fires. The Javadoc of @ShadowVariable
documented a genuine @PlanningListVariable as a valid source.

Minimal model that hits it:

@PlanningEntity
public class Machine {

    @PlanningListVariable
    List<Task> tasks;

    @ShadowVariable(supplierName = "calcTotalLoad")
    Integer totalLoad;

    @ShadowSources("tasks")
    public Integer calcTotalLoad() {
        return tasks.stream().mapToInt(Task::getDuration).sum();
    }
}

Observed behavior on current main:

  • totalLoad is computed when the graph is built (empty lists → 0) and stays stale for
    the entire solve, no matter how many tasks are assigned.
  • A constraint reading it never matches, so the solver returns overloaded, infeasible
    solutions with a perfect score.
  • With PHASE_ASSERT, the solve fails with the generic "Solver corruption was detected"
    message, pointing users at their own constraints or variable listeners.
  • The staleness is invisible to FULL_ASSERT's stale shadow variable detection, because
    forceTriggerAllVariableListeners() simulates list changes through the same
    before/afterListVariableChanged path that never reaches the session.
  • Confusingly, non-solving flows such as SolutionManager.updateShadowVariables() compute
    the same model correctly (verified, including across list mutations between calls),
    since the graph is rebuilt on every call — so unit tests pass while solving is broken.

What

As suggested in review, this PR no longer retriggers bare planning list variable sources.
Instead:

  • A @ShadowSources path that accesses a genuine @PlanningListVariable now fails fast
    in RootVariableSource, with an error message explaining why such a source cannot work.
  • The mention of @PlanningListVariable as a valid source is removed from the Javadoc of
    ShadowVariable; the Javadoc of ShadowSources and the reference documentation state
    the restriction with the same rationale.

The first commit is the original retrigger approach, kept for review context; the second
commit replaces it, so the net diff against main is only the fail-fast and its
documentation (6 files, +46/−6).

Why fail fast instead of retriggering

As pointed out in review, retriggering would only allow accessing the facts of entities
inside the @PlanningListVariable and the properties of the list itself — it would not
allow accessing the declarative shadow variables of entities inside the
@PlanningListVariable. Things like list.getLast().getEndTime() would still be wrong,
since the edges of the graph are not updated: no topological ordering, and no update when
an element's declarative shadow variable changes without a list change. Since most people
would source on the list variable and expect exactly that to work, retriggering would
confuse users and lead to more score corruptions — trading a deterministic freeze for
intermittent corruption.

Notes for full transparency:

  • Bare list variable sources did compute correct values in non-solving flows such as
    SolutionManager.updateShadowVariables(), since the graph is rebuilt on every call;
    only solving was broken. The fail-fast removes that niche usage too, as its correctness
    was accidental.
  • @InverseRelationShadowVariable collections (e.g. @ShadowSources("entityList") for a
    basic variable) are unaffected: the check requires the @PlanningListVariable
    annotation.

Follow-up

Accessing the declarative shadow variables of entities inside the @PlanningListVariable
(e.g. @ShadowSources("visits[].endTime")), with the edges of the graph properly created
and updated on list changes, is implemented and tested locally and will be submitted as a
separate PR once this lands. That PR will also extend this fail-fast's error message to
point users to the new syntax.

…ist variable

A declarative shadow variable using a genuine planning list variable as
a source, e.g. @Shadowsources("values"), was computed once when the
variable reference graph was built and then never retriggered during
solving: list variable change events were only delivered to variable
listeners, never to the declarative shadow variable session, so the
graph processor registered for the list variable never fired. The
shadow variable silently kept its stale value, and the staleness was
invisible even to FULL_ASSERT, whose forced re-trigger goes through the
same list variable events.

Forward before/afterListVariableChanged to the declarative shadow
variable session, like basic variable changes already are.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fodzal fodzal marked this pull request as ready for review July 6, 2026 12:19

@Christopher-Chianelli Christopher-Chianelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, what this PR does is the following:

  • Allow accessing the facts of entities inside the @PlanningListVariable and the properties of the list itself.

What the PR does not allow is:

  • Allow accessing the declarative shadow variables of entities inside the @PlanningListVariable.

This allow you to calculate things from the facts non-incrementally, but you still cannot do things like list.getLast().getEndTime() since the edges of the graph are not updated. The way I see it is, most people would source on the list variable and expect things like getLast().getEndTime() to work. As such, allowing this will probably confuse users and lead to more score corruptions. I would rather remove the mention in the Javadoc of ShadowVariable and add a fail-fast to RootVariableSource instead.

My comments are applicable if this change would be accepted, but I am leaning towards a reject and add a fail-fast due to the above.

notificationQueuesAreEmpty = false;
}
if (shadowVariableSession != null) {
// Declarative shadow variables may use the list variable itself as a source.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointless comment

listVariableChangedNotificationList.add(notification);
}
if (shadowVariableSession != null) {
// Declarative shadow variables may use the list variable itself as a source.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

}

@PlanningSolution
public static class Solution {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modify TestdataDeclarativeSimpleListEntity to have a declarative shadow variable count that computes size, which will allow you to remove these classes.

class BareListVariableSourceStalenessTest {

@Test
void bareListVariableSourceIsRetriggeredDuringSolving() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the assertion to the end of the test in ListModelTest#simpleList.

public @NonNull SimpleScore calculateScore(@NonNull Solution solution) {
var total = 0;
for (var owner : solution.owners) {
if (owner.valueCount == null || owner.valueCount != owner.values.size()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add as a new constraint in TestdataDeclarativeSimpleListConstraintProvider; you can remove throwing the exception; provided you penalize by the count variable, the FULL_ASSERT will catch stale shadow variables.

…ing them

As suggested in review, retriggering a bare planning list variable
source only makes suppliers reading facts or the list's structure
correct; a supplier reading a declarative shadow variable of an element
(e.g. visits.getLast().getEndTime()) would still be computed without
graph edges, so without topological ordering, and would not be
retriggered when the element's shadow variable changes without a list
change. Most users would expect exactly that to work, so retriggering
would trade a total freeze for intermittent corruption.

Instead, fail fast when a @Shadowsources path accesses a genuine
planning list variable, remove the mention of @PlanningListVariable as
a valid source from the @ShadowVariable javadoc, and document the
change in the migration guide.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use the "which is not allowed" ending, the "Maybe ...?" suffix, and the
"the shadow variable would not be updated" phrasing, consistently
across the error message, the Javadoc and the reference documentation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fodzal

fodzal commented Jul 8, 2026

Copy link
Copy Markdown
Author

Thanks for the review — I reworked the PR accordingly so that it fails fast in that case.

Retriggering would only have allowed accessing the facts of entities inside the @PlanningListVariable and the properties of the list itself, and most people would indeed expect getLast().getEndTime() to work — leading to more score corruptions. The PR is now exactly what you suggested: the mention is removed from the Javadoc of ShadowVariable, and RootVariableSource fails fast.

I am working on a new PR to authorize Shadow Sources like visits[].end_time. I will post soon.

@fodzal fodzal changed the title fix: retrigger declarative shadow variables sourced from a planning list variable fix: fail fast on planning list variable sources Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants