perf: recording fewer, cheaper move undo steps

[triceo]

Lazily allocate the variable-change list, merge a list-change's before/after undo steps into one, and reduce MoveDirector.executeTemporary() overhead.

Brings 5-10 % of extra throughput in score director benchmarks.

[Copilot]

🟡 Changes recommended

VariableChangeRecordingScoreDirector currently misses a guard against toIndex < fromIndex in list-variable validation and allocates the change list in undoChanges() even on no-op paths, both of which undermine correctness/performance goals.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR reduces overhead in temporary move execution and list-variable undo recording by merging list-variable before/after undo steps, lazily allocating the change list, and adding validation to prevent undo-time list corruption.

Changes:

• Replace separate list-variable after-change undo actions with a merged before-change action that clears/restores the mutated range and notifies once per bracket.
• Introduce PendingListChangeTracker to pair and validate list-variable brackets (including cross-instance pairing via getNonDelegating()), and document the new stricter contract for custom moves.
• Refactor MoveDirector.executeTemporary* APIs to reduce overhead and avoid exposing a live undo-move list to callers.

File summaries

File	Description
docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-to-latest.adoc	Documents stricter list-variable bracket requirements for custom moves.
core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverTest.java	Updates a test move to use correct list-variable change ranges.
core/src/test/java/ai/timefold/solver/core/impl/phase/custom/DefaultPhaseCommandContextTest.java	Fixes test setup to match new executeTemporary* expectations (solution descriptor available).
core/src/test/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirectorTest.java	Adds coverage for merged undo behavior and fail-fast bracket validation.
core/src/test/java/ai/timefold/solver/core/impl/move/PendingListChangeTrackerTest.java	Adds unit tests for the pending-bracket tracker behavior.
core/src/test/java/ai/timefold/solver/core/impl/move/MoveDirectorUndoShadowVariableTest.java	End-to-end validation that undo keeps shadow variables and score consistent across complex list operations.
core/src/test/java/ai/timefold/solver/core/impl/move/MoveDirectorTest.java	Adapts tests to the revised executeTemporary* API and semantics.
core/src/test/java/ai/timefold/solver/core/impl/move/ListVariableBeforeChangeActionTest.java	Updates tests to the merged list-variable undo mechanism and rebase behavior.
core/src/test/java/ai/timefold/solver/core/impl/move/ListVariableAfterChangeActionTest.java	Removes tests for the deleted after-change action type.
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListSwapMoveTest.java	Updates temporary execution callback signature for revised API.
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListRuinRecreateMoveTest.java	Updates temporary execution calls to the revised API.
core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListAssignMoveTest.java	Updates temporary execution callback signature for revised API.
core/src/main/java/ai/timefold/solver/core/impl/score/director/RevertableScoreDirector.java	Updates Javadoc references to the new executeTemporary signature.
core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java	Updates internal temporary-move execution to the new postprocessor-based API.
core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java	Implements lazy change-list allocation, pending bracket tracking/validation, and merged list undo behavior.
core/src/main/java/ai/timefold/solver/core/impl/move/PendingListChangeTracker.java	Adds a specialized tracker to pair/validate list-variable bracket events efficiently.
core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java	Refactors temporary execution APIs, adds “producing undo move” variant, and clarifies semantics/contract.
core/src/main/java/ai/timefold/solver/core/impl/move/ListVariableBeforeChangeAction.java	Replaces record with a merged-action implementation capable of clearing/restoring and notifying once.
core/src/main/java/ai/timefold/solver/core/impl/move/ListVariableAfterChangeAction.java	Removes the after-change action type (behavior folded into the before-change action).
core/src/main/java/ai/timefold/solver/core/impl/move/EphemeralMoveDirector.java	Removes AutoCloseable to prevent try-with-resources undo on exceptional paths.
core/src/main/java/ai/timefold/solver/core/impl/move/ChangeAction.java	Updates sealed permits list due to removal of ListVariableAfterChangeAction.
core/src/main/java/ai/timefold/solver/core/impl/exhaustivesearch/decider/AbstractExhaustiveSearchDecider.java	Switches exhaustive search to the new API that safely returns an already-applied undo move.

Review details

• Files reviewed: 22/22 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

VariableChangeRecordingScoreDirector.undoChanges() currently forces change-list allocation even when empty (and createUndoMove() marks the list escaped even when returning an empty list), which undermines the intended performance improvement.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java:69

• undoChanges() currently allocates the change list even when there are no recorded changes (and createUndoMove() marks the list as escaped even when returning an empty list), which undermines the intended lazy-allocation/performance optimization; avoid allocation by checking variableChangeList for null/empty and only setting variableChangesEscaped when returning the real list.

• Files reviewed: 22/22 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new fail-fast exception messages in VariableChangeRecordingScoreDirector misleadingly point users to AbstractSelectorBasedMove instead of generic Move implementations used by custom moves.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java:143

• The exception hint mentions AbstractSelectorBasedMove even though these notifications can come from any Move implementation, which makes the guidance misleading.

This issue also appears in the following locations of the same file:

• line 157
• line 162
• line 175

core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java:160

• The exception hint mentions AbstractSelectorBasedMove even though these notifications can come from any Move implementation, which makes the guidance misleading.

            throw new IllegalArgumentException("""
                    The fromIndex of afterListVariableChanged (%d) must match its beforeListVariableChanged counterpart (%d).
                    Maybe check implementation of your %s."""
                    .formatted(fromIndex, requiredFromIndex, AbstractSelectorBasedMove.class.getSimpleName()));

core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java:166

• The exception hint mentions AbstractSelectorBasedMove even though these notifications can come from any Move implementation, which makes the guidance misleading.

            throw new IllegalArgumentException("""
                    The afterListVariableChanged (%d, %d) of entity (%s) has toIndex (%d) smaller than fromIndex (%d).
                    Maybe check implementation of your %s."""
                    .formatted(fromIndex, toIndex, entity, toIndex, fromIndex,
                            AbstractSelectorBasedMove.class.getSimpleName()));

core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java:181

• The exception hint mentions AbstractSelectorBasedMove even though these notifications can come from any Move implementation, which makes the guidance misleading.

            throw new IllegalArgumentException("""
                    The afterListVariableChanged (%d, %d) of entity (%s) reports a length change of (%d), \
                    but its list variable actually changed length by (%d).
                    Maybe check implementation of your %s; \
                    its beforeListVariableChanged/afterListVariableChanged range must cover everything it changed."""
                    .formatted(fromIndex, toIndex, entity, reportedLengthDelta, actualLengthDelta,
                            AbstractSelectorBasedMove.class.getSimpleName()));

• Files reviewed: 22/22 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

PendingListChangeTracker currently overwrites an existing pending bracket in the overflow-map path before throwing, which can corrupt internal state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 22/22 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core move undo and temporary execution semantics in performance-critical paths, warranting careful human validation despite strong test additions.

Review details

• Files reviewed: 22/22 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A few updated/new documentation and Javadoc statements are inaccurate or violate repository documentation conventions, and they should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-to-latest.adoc:113

• This example line is not using Semantic Line Breaks as required by CONSTITUTION.md, which makes it harder to edit without rewrapping; please split it across logical line breaks.

For example, appending one element to the end of a list is `beforeListVariableChanged(entity, "valueList", size, size)` followed by `afterListVariableChanged(entity, "valueList", size, size + 1)`.

• Files reviewed: 23/23 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There is at least one confirmed correctness issue (state corruption on exception) in the new PendingListChangeTracker logic, and the test isolation change in AbstractMeterTest does not actually guarantee isolation from concurrently running tests.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

docs/src/modules/ROOT/pages/upgrading-timefold-solver/upgrade-to-latest.adoc:113

• The append example is easy to misread as calling size() twice (before and after the mutation), which would change the indices and trigger the new validation.

core/src/main/java/ai/timefold/solver/core/impl/move/PendingListChangeTracker.java:41

• In the overflow-map branch, calling Map.put() replaces the previously-open bracket before throwing, leaving the tracker in a corrupted state if the exception is caught and execution continues.

        if (overflowMap != null) {
            // Map.put returns the entry it replaced; a non-null one is a bracket that was still open.
            var previousAction = overflowMap.put(entity, action);
            if (previousAction != null) {
                throw bracketAlreadyOpen(entity, previousAction, action);
            }
            return;

core/src/test/java/ai/timefold/solver/core/testutil/AbstractMeterTest.java:30

• @ResourceLock(Resources.GLOBAL) does not prevent tests that do not acquire this lock from running concurrently, so these tests can still have their measurements overwritten by other solver tests.

@ResourceLock(Resources.GLOBAL)

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical list-change correctness issues and additional cleanup problems remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java:690

• The new public overload's Javadoc omits @param and @throws, violating the repository's required public-API documentation contract and leaving the callback and fatal-exception semantics undocumented; add tags for move, postprocessor, and the documented failure path.

    public final @Nullable <Result_> Result_ executeTemporary(Move<Solution_> move,
            TemporaryScorePostprocessor<Score_, Result_> postprocessor) {

core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java:717

• The new public method's Javadoc likewise omits @param and @throws, so callers are not told the arguments or the fatal-exception behavior described in the body; add tags for move, scoreConsumer, and the failure path.

    public final Move<Solution_> executeTemporaryProducingUndoMove(Move<Solution_> move,
            Consumer<InnerScore<Score_>> scoreConsumer) {

core/src/main/java/ai/timefold/solver/core/impl/move/PendingListChangeTracker.java:39

• The overflow path overwrites the existing action before throwing, so after a duplicate beforeListVariableChanged the tracker no longer points at the original bracket; if the exception is handled or cleanup continues, the original action is later treated as unmatched and undo can fail or corrupt state.

            var previousAction = overflowMap.put(entity, action);
            if (previousAction != null) {
                throw bracketAlreadyOpen(entity, previousAction, action);

core/src/test/java/ai/timefold/solver/core/testutil/AbstractMeterTest.java:30

• @ResourceLock(Resources.GLOBAL) only coordinates tests that declare the same resource, so it does not provide the isolation promised by the class comment from any test that does not request this lock and later touches Metrics.globalRegistry; use @Isolated or apply the same lock to every such test.

@ResourceLock(Resources.GLOBAL)

• Files reviewed: 23/23 changed files
• Comments generated: 2
• Review effort level: Lite