Fix #2013: reuse DependencyManager instance to restore pool cache transparency by gnodet · Pull Request #2014 · apache/maven-resolver

gnodet · 2026-07-24T23:44:20Z

Summary

Fixes #2013 — BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation).

Root cause: TransitiveDependencyManager.deriveChildManager() always created a new instance (with a unique path field and incrementing depth), making every pool key unique. The BF collector's pool cache includes the DependencyManager in its GraphKey, so distinct-but-semantically-equal managers caused pool misses. On a miss, the GACE-based skipper was consulted and could prune a node's children to zero — even though the same node had children when served from the cache via a different traversal path.

Fix: AbstractDependencyManager.deriveChildManager() now returns this when:

No new management data (versions, scopes, optionals, local paths, exclusions) was collected at the current depth, AND
Management is already being applied (depth >= applyFrom — the isApplied() guard)

The isApplied() guard is critical: at depth < applyFrom, returning this would freeze the depth counter and prevent isApplied() from ever returning true for descendants, breaking management rule application.

This optimization addresses the common case for transitive dependencies whose POMs do not declare <dependencyManagement>. ClassicDependencyManager (Maven 3.x) was not affected because its deriveUntil=2 caused isDerived() to return this at depth ≥ 2.

Changes

AbstractDependencyManager.deriveChildManager() — added instance-reuse optimization before the newInstance() call
DependencyManagerTest — new unit test testDeriveChildManagerReusesInstanceWhenNoNewManagementData() verifying the optimization with assertSame/assertNotSame at different depths
BfWithSkipperDependencyCollectorTest — new integration test testPoolCacheTransparencyWithTransitiveDependencyManager() with a diamond-like graph (root → b → c → d and root → b-alt → c → d) verifying that c under b-alt retains its children

Test plan

Unit test: DependencyManagerTest.testDeriveChildManagerReusesInstanceWhenNoNewManagementData — verifies instance reuse at depth ≥ applyFrom with no management data, and new instance creation otherwise
Integration test: BfWithSkipperDependencyCollectorTest.testPoolCacheTransparencyWithTransitiveDependencyManager — end-to-end test demonstrating the bug is fixed
All existing tests pass (maven-resolver-util: 447 tests, maven-resolver-impl: 468 tests)

🤖 Generated with Claude Code

…ta is collected AbstractDependencyManager.deriveChildManager() now returns `this` when no new management data (versions, scopes, optionals, local paths, exclusions) was collected at the current depth AND management is already being applied (depth >= applyFrom). This avoids creating distinct-but-semantically-equal DependencyManager instances that defeat the BfDependencyCollector pool cache — the pool key includes the manager, so unique managers cause pool misses, which lets the skipper prune subtrees that should have been served from the cache. The isApplied() guard is necessary: at depth < applyFrom, returning `this` would freeze the depth counter and prevent management rules from ever being applied to descendants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

Clean, well-targeted fix for the cache-transparency bug described in #2013. The optimization to reuse the DependencyManager instance when no new management data is collected is semantically correct — it's placed at exactly the right point in deriveChildManager (after all management-data collection but before newInstance()), and the isApplied() guard prevents freezing depth below the applyFrom threshold.

A couple of minor, non-blocking observations:

The unit test only covers TransitiveDependencyManager. DefaultDependencyManager (applyFrom=0) also benefits from the optimization and could be tested with a similar assertSame/assertNotSame pattern. The existing testDefault test indirectly covers management correctness, so this is a minor coverage gap.
An additional test where depth-1 context contributes management data (making this.managedVersions non-null), then the next depth has an empty context (optimization fires), and a subsequent depth has management data for the same key would explicitly document the "nearer-to-root wins" invariant — analysis of getManagedVersion confirms it iterates the path from root (first match wins), so this is functionally correct already.

Thread safety is fine: the optimization returns this without mutation, and newInstance creates new ArrayList copies of this.path, so concurrent calls from different BF collector branches are safe.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

…tests Add two tests suggested by review on PR #2014: 1. testDeriveChildManagerReusesInstanceWithDefaultManager — verifies the optimization also works with DefaultDependencyManager (applyFrom=0), where instance reuse fires at the very first derivation. 2. testNearerToRootWinsAfterOptimizationReusesInstance — documents that the "nearer-to-root wins" invariant holds after the optimization reuses an instance: root's version management at depth 1 still wins when a deeper POM tries to override the same key, because containsManagedVersion finds it in ancestors and skips collection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet · 2026-07-28T07:54:49Z

Thanks for the review! Both suggestions addressed in 0802960:

DefaultDependencyManager coverage — added testDeriveChildManagerReusesInstanceWithDefaultManager which verifies the optimization fires at the very first derivation with applyFrom=0 (assertSame), and that it correctly creates new instances when management data is present (assertNotSame).
"Nearer-to-root wins" invariant — added testNearerToRootWinsAfterOptimizationReusesInstance which explicitly documents the invariant: root contributes x:1.0 at depth 1, the optimization reuses the instance at depth 2→3, then a deeper POM tries to override x with 2.0 — the test verifies containsManagedVersion finds it in ancestors, skips collection (so the optimization fires again), and getManagedVersion still returns 1.0.

gnodet

Re-review after new commit (0802960).

The new commit cleanly addresses both suggestions from the previous review:

DefaultDependencyManager coverage — new test verifies the optimization fires at the very first derivation (depth 0→1) since applyFrom=0 makes isApplied() true immediately. The assertSame/assertNotSame assertions are accurate.
Nearer-to-root-wins invariant test — verifies that when a depth-1 context contributes management data and the next depth has an empty context (optimization reuses this), a subsequent depth with management data for the same key correctly inherits the nearer-to-root value. The assertion chain through containsManagedVersion → skip local collection → all five managed variables stay null is sound.

Test coverage for the optimization is now comprehensive across both manager implementations, with and without management data, and across the applyFrom boundary. No issues found.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

Copilot

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes #2013 by making AbstractDependencyManager.deriveChildManager() reuse the same DependencyManager instance when no new management data is introduced (and management is already applied), preventing BF collector pool-cache key churn and restoring cache transparency.

Changes:

Added an instance-reuse fast path in AbstractDependencyManager.deriveChildManager() when derivation is a semantic no-op at applied depths.
Added unit tests validating reuse behavior across TransitiveDependencyManager and DefaultDependencyManager, plus a “nearer-to-root wins” invariant test.
Added an integration test scenario (with new artifact-description fixtures) reproducing the BF pool-cache transparency issue and asserting consistent graph structure.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java	Reuses manager instances for no-op derivations at applied depths to avoid BF pool-cache misses.
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java	Adds unit tests for instance-reuse behavior and precedence invariants.
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java	Adds integration test ensuring pool-cache transparency with `TransitiveDependencyManager`.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini	Adds root fixture for diamond-like dependency graph.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini	Adds `b` fixture depending on `c`.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini	Adds `b-alt` fixture depending on `c`.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini	Adds `c` fixture depending on `d`.
maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini	Adds `d` fixture with no dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        // root has two children: b and b-alt
+        DependencyNode rootNode = result.getRoot();
+        assertEquals(2, rootNode.getChildren().size(), "root should have 2 children (b, b-alt)");
+
+        // b → c
+        DependencyNode b = rootNode.getChildren().get(0);
+        assertEquals("b", b.getArtifact().getArtifactId());
+        assertFalse(b.getChildren().isEmpty(), "b should have children");
+
+        // b → c → d
+        DependencyNode cUnderB = b.getChildren().get(0);
+        assertEquals("c", cUnderB.getArtifact().getArtifactId());
+        assertFalse(cUnderB.getChildren().isEmpty(), "c under b should have children (d)");
+
+        // b-alt → c  (this is the key assertion: c under b-alt must also have children)
+        DependencyNode bAlt = rootNode.getChildren().get(1);
+        assertEquals("b-alt", bAlt.getArtifact().getArtifactId());
+        assertFalse(bAlt.getChildren().isEmpty(), "b-alt should have children");
+
+        DependencyNode cUnderBAlt = bAlt.getChildren().get(0);
+        assertEquals("c", cUnderBAlt.getArtifact().getArtifactId());


+        if (managedVersions == null
+                && managedScopes == null
+                && managedOptionals == null
+                && managedLocalPaths == null
+                && managedExclusions == null
+                && isApplied()) {
+            return this;
+        }


gnodet marked this pull request as ready for review July 24, 2026 23:50

gnodet requested a review from cstamas July 24, 2026 23:50

gnodet commented Jul 27, 2026

View reviewed changes

gnodet commented Jul 28, 2026

View reviewed changes

elharo requested a review from Copilot July 29, 2026 11:00

elharo approved these changes Jul 29, 2026

View reviewed changes

Copilot AI reviewed Jul 29, 2026

View reviewed changes

Copilot started reviewing on behalf of elharo July 29, 2026 11:39 View session

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #2013: reuse DependencyManager instance to restore pool cache transparency - #2014

Fix #2013: reuse DependencyManager instance to restore pool cache transparency#2014
gnodet wants to merge 2 commits into
masterfrom
fix/2013

gnodet commented Jul 24, 2026

Uh oh!

gnodet left a comment

Uh oh!

gnodet commented Jul 28, 2026

Uh oh!

gnodet left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gnodet commented Jul 24, 2026

Summary

Changes

Test plan

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet commented Jul 28, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants