[I] BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation) [maven-resolver]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet opened a new issue, #2013:
URL: https://github.com/apache/maven-resolver/issues/2013

   ### 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](https://github.com/apache/maven-resolver/blob/master/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java):
   
   ```java
   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](https://github.com/apache/maven-shade-plugin/issues/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](https://github.com/apache/maven-shade-plugin/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation) [maven-resolver]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet commented on issue #2013:
URL: https://github.com/apache/maven-resolver/issues/2013#issuecomment-5075559762

   ### 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()`](https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java) 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 #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](https://github.com/apache/maven-resolver/pull/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:
   
   ```java
   // 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation) [maven-resolver]

["gnodet (via GitHub)" <gi...@apache.org>]

gnodet closed issue #2013: BfDependencyCollector pool cache hit/miss changes graph structure (cache-transparency violation)
URL: https://github.com/apache/maven-resolver/issues/2013


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org