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

maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java

@@ -25,6 +25,7 @@
 import org.eclipse.aether.collection.CollectResult;
 import org.eclipse.aether.collection.DependencyCollectionException;
 import org.eclipse.aether.graph.Dependency;
+import org.eclipse.aether.graph.DependencyNode;
 import org.eclipse.aether.graph.Exclusion;
 import org.eclipse.aether.impl.ArtifactDescriptorReader;
 import org.eclipse.aether.internal.impl.StubRemoteRepositoryManager;
@@ -37,6 +38,8 @@
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
  * UT for {@link BfDependencyCollector}.
@@ -68,6 +71,75 @@ private Dependency newDep(String coords, String scope, Collection<Exclusion> exc
         return d.setExclusions(exclusions);
     }
 
+    /**
+     * Verifies that the pool cache is transparent w.r.t. the DependencyManager: the graph
+     * structure must not depend on whether the pool hits or misses.
+     * <p>
+     * Scenario (from <a href="https://github.com/apache/maven-resolver/issues/2013">#2013</a>):
+     * <pre>
+     *   root
+     *   ├── b      → c → d
+     *   └── b-alt  → c → d   (c is a shared transitive dependency)
+     * </pre>
+     * With {@link TransitiveDependencyManager}, {@code deriveChildManager()} used to always
+     * create a new instance (unique {@code path} field), making every pool key unique.
+     * The pool would miss for {@code c} under {@code b-alt}, the skipper would mark it as
+     * a duplicate, and the node would end up with zero children — even though the same
+     * {@code c} under {@code b} had children.
+     * <p>
+     * The fix in {@code AbstractDependencyManager.deriveChildManager()} reuses the same
+     * manager instance when no new management data is collected, so the pool key matches
+     * and children are preserved.
+     */
+    @Test
+    void testPoolCacheTransparencyWithTransitiveDependencyManager() throws DependencyCollectionException {
+        collector = setupCollector(newReader("pool-cache-transparency/"));
+        parser = new DependencyGraphParser("artifact-descriptions/pool-cache-transparency/");
+        session.setDependencyManager(new TransitiveDependencyManager(null));
+
+        Dependency root = newDep("gid:root:ext:1.0", "compile");
+        CollectRequest request = new CollectRequest(root, Collections.singletonList(repository));
+        CollectResult result = collector.collectDependencies(session, request);
+
+        assertEquals(0, result.getExceptions().size());
+
+        // 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());
+
+        // Before the fix, this assertion would fail: c under b-alt had zero children
+        // because the pool key differed (different DependencyManager instance) and the
+        // skipper marked it as a duplicate.
+        assertFalse(
+                cUnderBAlt.getChildren().isEmpty(),
+                "c under b-alt should have children (d) — pool cache must be transparent");
+
+        // Verify that c's child is d in both subtrees
+        assertEquals("d", cUnderB.getChildren().get(0).getArtifact().getArtifactId());
+        assertTrue(
+                cUnderBAlt.getChildren().stream()
+                        .anyMatch(n -> "d".equals(n.getArtifact().getArtifactId())),
+                "c under b-alt should have d as a child");
+    }
+
     @Test
     void testSkipperWithDifferentExclusion() throws DependencyCollectionException {
         collector = setupCollector(newReader("managed/"));

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini

@@ -0,0 +1,2 @@
+[dependencies]
+gid:c:ext:1.0

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini

@@ -0,0 +1,2 @@
+[dependencies]
+gid:c:ext:1.0

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini

@@ -0,0 +1,2 @@
+[dependencies]
+gid:d:ext:1.0

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini

@@ -0,0 +1 @@
+[dependencies]

maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini

@@ -0,0 +1,3 @@
+[dependencies]
+gid:b:ext:1.0
+gid:b-alt:ext:1.0

maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java

@@ -379,6 +379,23 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context)
             }
         }
 
+        // Optimization: when no new management data was collected at this depth and management
+        // is already being applied (depth >= applyFrom), reuse this instance. This avoids creating
+        // unnecessarily distinct DependencyManager instances that would defeat the BF collector's
+        // pool cache — the pool key includes the manager, so distinct-but-semantically-equal
+        // managers cause pool misses, which in turn lets the skipper prune subtrees that should
+        // have been served from the cache. This is the common case for transitive dependencies
+        // whose POMs do not declare <dependencyManagement>.
+        // See https://github.com/apache/maven-resolver/issues/2013
+        if (managedVersions == null
+                && managedScopes == null
+                && managedOptionals == null
+                && managedLocalPaths == null
+                && managedExclusions == null
+                && isApplied()) {
+            return this;
+        }
+
         return newInstance(
                 managedVersions != null ? managedVersions.done() : null,
                 managedScopes != null ? managedScopes.done() : null,

maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java

@@ -204,6 +204,101 @@ void testTransitive() {
         assertNull(mngt);
     }
 
+    /**
+     * Verifies the instance-reuse optimization in {@link AbstractDependencyManager#deriveChildManager}:
+     * when no new management data is collected and management is already being applied
+     * (depth >= applyFrom), deriveChildManager should return the same instance.
+     * This is critical for BF collector pool cache transparency (issue #2013).
+     */
+    @Test
+    void testDeriveChildManagerReusesInstanceWhenNoNewManagementData() {
+        DependencyManager manager = new TransitiveDependencyManager(null);
+
+        // depth=0 → depth=1: root → first level (applyFrom=2, so not yet applied)
+        DependencyManager depth1 = manager.deriveChildManager(newContext());
+        assertNotSame(manager, depth1, "depth 0→1: must return new instance (not yet applied)");
+
+        // depth=1 → depth=2: no managed deps, now at applyFrom=2 (applied)
+        DependencyManager depth2 = depth1.deriveChildManager(newContext());
+        assertNotSame(depth1, depth2, "depth 1→2: must return new instance (crossing applyFrom boundary)");
+
+        // depth=2 → depth=3: no managed deps, already applied → should reuse
+        DependencyManager depth3 = depth2.deriveChildManager(newContext());
+        assertSame(depth2, depth3, "depth 2→3 with no managed deps: should reuse instance");
+
+        // depth=3 → depth=4: still no managed deps → should keep reusing
+        DependencyManager depth4 = depth3.deriveChildManager(newContext());
+        assertSame(depth3, depth4, "depth 3→4 with no managed deps: should reuse instance");
+
+        // depth=2 → depth=3 with managed deps: should NOT reuse
+        DependencyManager depth3WithMgmt =
+                depth2.deriveChildManager(newContext(new Dependency(new DefaultArtifact("new:dep:1.0"), "compile")));
+        assertNotSame(depth2, depth3WithMgmt, "depth 2→3 with managed deps: must return new instance");
+    }
+
+    /**
+     * Verifies the instance-reuse optimization with {@link DefaultDependencyManager} (applyFrom=0).
+     * Since management is applied from depth 0, the optimization should fire at the very first
+     * derivation when no management data is collected.
+     */
+    @Test
+    void testDeriveChildManagerReusesInstanceWithDefaultManager() {
+        DependencyManager manager = new DefaultDependencyManager(null);
+
+        // DefaultDependencyManager has applyFrom=0, so isApplied() is true from the start.
+        // depth=0 → depth=1: no managed deps → should reuse (already applied)
+        DependencyManager depth1 = manager.deriveChildManager(newContext());
+        assertSame(manager, depth1, "depth 0→1 with no managed deps: should reuse (applyFrom=0)");
+
+        // depth=0 → depth=1 with managed deps: should NOT reuse
+        DependencyManager depth1WithMgmt =
+                manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("mgd:dep:1.0"), "compile")));
+        assertNotSame(manager, depth1WithMgmt, "depth 0→1 with managed deps: must return new instance");
+
+        // depth=1 (with mgmt) → depth=2: no managed deps → should reuse
+        DependencyManager depth2 = depth1WithMgmt.deriveChildManager(newContext());
+        assertSame(depth1WithMgmt, depth2, "depth 1→2 with no managed deps: should reuse");
+    }
+
+    /**
+     * Verifies that the "nearer-to-root wins" invariant holds when the optimization
+     * reuses instances. Scenario: root contributes version management at depth 1,
+     * the optimization fires at depth 2→3 (empty context), and a deeper POM tries
+     * to override the same key — the root's version must still win.
+     */
+    @Test
+    void testNearerToRootWinsAfterOptimizationReusesInstance() {
+        DependencyManager manager = new TransitiveDependencyManager(null);
+
+        // depth=0 → depth=1: root contributes version 1.0 for artifact "x"
+        DependencyManager depth1 =
+                manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:1.0"), "compile")));
+
+        // depth=1 → depth=2: empty context (new instance because applyFrom boundary)
+        DependencyManager depth2 = depth1.deriveChildManager(newContext());
+        assertNotSame(depth1, depth2, "depth 1→2: new instance (crossing applyFrom boundary)");
+
+        // depth=2 → depth=3: empty context, optimization fires
+        DependencyManager depth3 = depth2.deriveChildManager(newContext());
+        assertSame(depth2, depth3, "depth 2→3: optimization should reuse instance");
+
+        // Verify root's version still applies after optimization reuse
+        DependencyManagement mngt = depth3.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null));
+        assertNotNull(mngt, "management should apply at depth >= applyFrom");
+        assertEquals("1.0", mngt.getVersion(), "root's version must apply after optimization reuse");
+
+        // depth=3 → depth=4: context tries to override "x" with 2.0, but containsManagedVersion
+        // finds it in ancestors → no new data collected → optimization fires again
+        DependencyManager depth4 =
+                depth3.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:2.0"), "compile")));
+        assertSame(depth3, depth4, "optimization should fire when context only has already-managed keys");
+
+        // Root's version still wins ("nearer-to-root wins" invariant)
+        mngt = depth4.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null));
+        assertNotNull(mngt, "management should still apply");
+        assertEquals("1.0", mngt.getVersion(), "nearer-to-root version must win over deeper override attempt");
+    }
+
     @Test
     void testDefault() {
         DependencyManager manager = new DefaultDependencyManager(null);