BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation)

[gnodet]

Summary

The BfDependencyCollector.doRecurse() method has a cache-transparency issue: whether a dependency node retains its children depends on whether the pool cache hits or misses — a performance optimization is changing the semantic result.

Root cause

doRecurse() uses two independent caching/dedup layers with different keys:

1. Pool cache — keyed on (artifact, repos, selector, manager, traverser, filter), i.e. it includes the DependencyManager (compared via equals).
2. GACE skipper — keyed on artifact identity only (G:A:C:E, no version, no manager).

The interaction at BfDependencyCollector.java lines ~400-436:

List<DependencyNode> children = pool.getChildren(key); // key includes DependencyManager
if (children == null) {        // POOL MISS → skipper is consulted
    if (!skipper.skipResolution(child, parents)) {
        // resolve children, cache in pool
    }
    // else: node gets ZERO children (skipper marked it as duplicate)
} else {                       // POOL HIT → skipper is NEVER consulted
    child.setChildren(children);
}

When the pool hits, the skipper is bypassed and the node gets cached children. When the pool misses, the skipper IS consulted and may mark the node as skippedAsDuplicate, giving it zero children. The ConflictResolver (in default Verbosity.NONE) then prunes childless conflict-loser nodes entirely.

This means the graph structure depends on whether the pool cache hits or misses — which is a function of DependencyManager.equals(), not of the dependency being resolved.

How this manifests

Discovered via apache/maven-shade-plugin#819. Consider a project where:

• Direct dependency a depends on b (no classifier) and b:alt (classifier "alt")
• Both b and b:alt share a transitive dependency c

Under Maven 3.10 (ClassicDependencyManager): at depth ≥ 2, deriveChildManager() returns the same instance → pool key for c under b equals pool key for c under b:alt → pool hit → skipper is never consulted → c appears under both variants.

Under Maven 4 (TransitiveDependencyManager): deriveChildManager() always creates a new instance with a unique path field (added in #1539) → pool key differs → pool miss → skipper marks c under b:alt as skippedAsDuplicate → c gets zero children → ConflictResolver prunes it → c disappears from b:alt's subtree.

The same Resolver 2.0.20 produces different graphs under Maven 3.10 vs Maven 4, purely due to the DependencyManager choice.

Reproducer

Standalone reproducer (no plugin build needed): https://github.com/aschemaven/mshade-819-reproducer

./run.sh <maven-home>

Results:

Maven	Resolver	c under b	c under b:alt
3.9.16	1.x	✅ present	✅ present
3.10.0-rc-1	2.0.20	✅ present	✅ present
4.0.0-rc-5	2.0.13	✅ present	❌ pruned
4.0.0-rc-5 + -Daether.conflictResolver.verbose=true	2.0.13	✅ present	✅ present

Suggested fix

Make the pool cache transparent — either:

1. Always consult the skipper regardless of pool hit/miss, but let the pool provide cached children if the skipper says "don't skip".
2. On pool miss + skipper skip, still try to provide cached children from the winning node's pool entry (since the GACE keys match), so the node retains its children even when the pool key differs.

Either approach would make the graph structure independent of the DependencyManager.equals() behavior.

Workaround

The maven-shade-plugin has a workaround in PR #820: collecting with ConflictResolver.CONFIG_PROP_VERBOSE = STANDARD so pruned nodes are retained as markers. This is a valid fix on the plugin side, but other plugins that walk the dependency graph may be silently affected by the same issue.

[gnodet]

Deeper root cause: TransitiveDependencyManager always creates new instances

The cache-transparency issue described above is a symptom. The deeper root cause is that TransitiveDependencyManager always creates a new instance in deriveChildManager(), even when it collects no new management data at the current depth.

Why it happens

AbstractDependencyManager.deriveChildManager() checks isDerived() (i.e. depth < deriveUntil) to decide whether to call newInstance():

• ClassicDependencyManager sets deriveUntil = 2 → at depth ≥ 2, isDerived() returns false → deriveChildManager() returns this → same instance → pool key matches → no problem.
• TransitiveDependencyManager sets deriveUntil = Integer.MAX_VALUE → isDerived() always returns true → deriveChildManager() always calls newInstance() → each new instance gets a unique path (ancestor chain, added in DependencyManager improvement #1539) → every pool key is unique → pool never hits → skipper kicks in → duplicate nodes get pruned.

This is why Maven 3.10 (ClassicDependencyManager by default) and Maven 4 (TransitiveDependencyManager by default) produce different graphs with the same Resolver version — it's not a Resolver regression, it's that TransitiveDependencyManager defeats the pool cache by design.

Prior art: PR #1489

@cstamas explored this in PR #1489 ("Move out depth from DepMgr") — which attempted to remove depth from the DependencyManager to make managers equal regardless of tree position. It was closed with -1 because it required breaking the DependencyManager API (adding depth as a parameter to deriveChildManager() and manageDependency()).

Suggested non-breaking fix

There's a simpler optimization that doesn't break any API. At the end of AbstractDependencyManager.deriveChildManager(), just before calling newInstance(...), if all collected maps are null (no dependency contributed new management data at the current depth), return this instead of creating a new instance:

// In AbstractDependencyManager.deriveChildManager():
if (managedVersions == null && managedScopes == null && managedOptionals == null 
        && managedLocalPaths == null && managedExclusions == null) {
    return this;  // no new management data → reuse this manager
}
return newInstance(/* ... */);

This would make TransitiveDependencyManager return the same instance when a dependency contributes no management rules — which is the common case for most transitive dependencies (only a handful of POMs actually declare <dependencyManagement>). Pool keys would match in the typical case, the skipper wouldn't be consulted for already-cached subtrees, and the shade plugin scenario (and any similar plugin walking the dependency graph) would work without needing CONFIG_PROP_VERBOSE.

This fix is purely an optimization — it preserves the current behavior for nodes that DO contribute management data, while avoiding unnecessary new instances for nodes that don't.