Reimplement endorsement of strict version constraints by jvandort · Pull Request #34893 · gradle/gradle

jvandort · 2025-09-04T16:20:00Z

Prior to this PR, whenever a node was deselected for any reason, if it had an incoming edge that endorsed strict versions, we would "reselect" the source node of the edge. In this case re-selection means removing all outgoing edges and re-queueing the source node.

See:

gradle/platforms/software/dependency-management/src/main/java/org/gradle/api/internal/artifacts/ivyservice/resolveengine/graph/builder/NodeState.java

Lines 1092 to 1113 in c055893

    
           private void reselectEndorsingNode() { 
        
               if (incomingEdges.size() == 1) { 
        
                   EdgeState singleEdge = incomingEdges.get(0); 
        
                   NodeState from = singleEdge.getFrom(); 
        
                   if (singleEdge.getDependencyState().getDependency().isEndorsingStrictVersions()) { 
        
                       // pass my own component because we are already in the process of re-selecting it 
        
                       from.reselect(); 
        
                   } 
        
               } else { 
        
                   for (EdgeState incoming : new ArrayList<>(incomingEdges)) { 
        
                       if (incoming.getDependencyState().getDependency().isEndorsingStrictVersions()) { 
        
                           // pass my own component because we are already in the process of re-selecting it 
        
                           incoming.getFrom().reselect(); 
        
                       } 
        
                   } 
        
               } 
        
           } 
        
           private void reselect() { 
        
               resolveState.onMoreSelected(this); 
        
               removeOutgoingEdges(); 
        
           }

If the source node was the root node, that means we would attempt to de-select the root node and add it back to the queue. This effectively restarts the whole graph traversal from the beginning. If it weren't for some shady logic that short-circuits unstable graphs (#29936) this would lead to an infinite loop. However, due the short-circuit we instead produced a mostly correct graph, but sometimes produced incorrect graphs. This occasionally led to critical failures in the resolution engine.

In this PR, we re-implement the way strict versions are inherited between nodes in a dependency graph. Strict versions are now handled much more similarly to how exclusions are propagated down a dependency graph.

Now, when visiting a node's dependencies, we compute the inherited (ancestor's) strict versions for that node on-demand. We compare the value to the pervious traversal's inherited strict versions, propagating any changes to existing outgoing edges.

When adding or removing edges from nodes, we use the endorsing-ness of the edge to determine if additional nodes need to be re-queued to account for changes from endorsed dependencies.

This new approach allows us to avoid the potential of reprocessing the root node, an operation which currently can lead to buggy graphs.

Fixes #33508

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

ljacomet · 2025-09-09T14:10:34Z

@@ -207,9 +204,7 @@ private void traverseGraph(final ResolveState resolveState) {
                // Initialize and collect any new outgoing edges of this node
                dependencies.clear();
                node.visitOutgoingDependencies(dependencies);


💅 Since we are cleaning the logic here, maybe we could continue cleaning up names: dependencies is really edges - and it becomes that deeper down.
visitOutgoingDependencies could be visitOutgoingDependenciesAndCollectEdges - a mouthful bu that's what it does.

jvandort · 2025-09-11T19:06:32Z

Adhoc perf test for failing run: https://builds.gradle.org/buildConfiguration/Gradle_Master_Util_Performance_AdHocPerformanceScenarioLinuxAmd64/103025122

bot-gradle · 2025-09-22T00:52:32Z

The following builds have failed:

All Performance Tests (Trigger)
- Build Scan

bot-gradle · 2025-09-22T01:05:43Z

The following builds have failed:

Pull Request Feedback (Trigger) (Check)
- Build Scan

jvandort · 2025-09-22T03:16:37Z

@bot-gradle test apt prf

bot-gradle · 2025-09-22T03:31:32Z

The following builds have passed:

Pull Request Feedback (Trigger) (Check)
- Build Scan

bot-gradle · 2025-09-22T03:39:07Z

The following builds have passed:

All Performance Tests (Trigger)
- Build Scan

jvandort · 2025-09-23T01:05:39Z

We still need to add a test demonstrating fixed behavior

Prior to this commit, whenever a node was deselected for any reason, if it had an incoming edge that endorsed strict versions, we would "reselect" the source node of the edge. In this case re-selection means removing all outgoing edges and re-queueing the source node. If the source node was the root node, that means we would attempt to de-select the root node and add it back to the queue. This effectively restarts the whole graph traversal from the beginning. If it weren't for some shady logic that short-circuits unstable graphs, this would lead to an infinite loop. However, due the short-circuit we instead produced a mostly correct graph, but sometimes produced incorrect graphs. This occasionally led to critical failures in the resolution engine. In this commit, we re-implement the way strict versions are inherited between nodes in a dependency graph. Strict versions are now handled much more similarly to how exclusions are propagated down a dependency graph. Now, when visiting a node's dependencies, we compute the inherited (ancestor's) strict versions for that node on-demand. We compare the value to the pervious traversal's inherited strict versions, propagating any changes to existing outgoing edges. When adding or removing edges from nodes, we use the endorsing-ness of the edge to determine if additional nodes need to be re-queued to account for changes from endorsed dependencies. This reduces pressure on the unstable-graph-short-circuit logic, avoiding potential for broken graphs when using endorsed strict versions. Additionally, we: - Rework resolveEdges to run in a single pass instead of multiple passes, by instead recomputing selectors in visitDependencies - Improve computeSelector by properly calling use/release on the old/new selectors, ensuring the selector properly tracks the number of edges using it - Update dependency visiting logic to pass-down computed exclusions and ancestors strict versions instead of relying on state, ensuring it is more difficult to accidentally use stale computed state - Rename methods, fields, variables as needed. Add Javadoc and TODOs as needed. Reactor methods as needed for readability.

Calculating these endorsed strict versions for each parent of a node each time a node is processed is expensive. Instead, we cache the result of this computation and ensure we invalidate it each time an input to the calculation is changed.

autonomousapps · 2025-09-30T22:16:15Z

I can confirm that this resolves at least one of the issues in Block's cash-server build.

This reproducer is mostly minified from the original reproducer, though could likey be reduced down more.

ljacomet

note that I moved the test according to the TODO it had

jvandort force-pushed the jvandort/dm/engine/reimplement-strict-version-constraints branch 4 times, most recently from 8152fb5 to e2418c7 Compare September 5, 2025 22:52