You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/07/22 15:29:19 UTC
[PR] Extend re-entrancy detection for broken trace chains and managed dep validation [maven-resolver]
gnodet opened a new pull request, #2009:
URL: https://github.com/apache/maven-resolver/pull/2009
## Summary
The trace-based re-entrancy detection added in PR #1957 does not fully cover all code paths. Specifically, Maven 4's `RequestTraceHelper` converts between Maven API traces and resolver `RequestTrace` objects via `toResolver()`, creating a fresh trace chain that loses the `REPOSITORY_SYSTEM_CALL` marker. This causes `MavenValidator` to reject the collect request in the `MavenITgh12305` integration test (PR apache/maven#12481) with `Invalid Collect Request: null -> []`.
This PR addresses the gap with two complementary fixes:
- **Session-scoped re-entrancy detection** (`DefaultRepositorySystem`): Each public method now maintains a depth counter in `SessionData` via `enterSessionScope()`/try-finally decrement. When the counter is > 0 on entry, the call is detected as re-entrant regardless of whether the `RequestTrace` chain carries the marker. This catches cases where consumers rebuild the trace chain from a different tracing system.
- **Skip managed dependency validation** (`DefaultRepositorySystemValidator`): Managed dependencies are declarative constraints that only take effect when a matching dependency is encountered during collection. Validating them eagerly rejects valid builds where a BOM imports managed deps with uninterpolated expressions (e.g. `${osgi.version}`) that are never actually used. If a managed dependency IS matched and its coordinates are invalid, the error surfaces naturally during version/artifact resolution.
Together these changes allow [apache/maven#12481](https://github.com/apache/maven/pull/12481) to remove the consumer-side workarounds for uninterpolated expressions.
## Context
- [PR #1957](https://github.com/apache/maven-resolver/pull/1957) — original re-entrancy detection (trace-based)
- [apache/maven#12481](https://github.com/apache/maven/pull/12481) — Maven PR that removes consumer-side workarounds
- [MavenITgh12305](https://github.com/apache/maven/blob/master/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12305InvalidCollectRequestUninterpolatedManagedDepsTest.java) — the IT test that exercises this scenario
## Test plan
- [x] New test: `sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken` — verifies inner calls with broken trace + uninterpolated expressions succeed via session-scoped detection
- [x] New test: `collectDependenciesAcceptsManagedDepsWithUninterpolatedExpressions` — reproduces the MavenITgh12305 scenario
- [x] New test: `collectDependenciesStillRejectsInvalidDirectDependencies` — ensures direct deps are still validated
- [x] New test: `sessionScopedDepthIsProperlyDecrementedOnExit` — verifies counter cleanup for independent calls
- [ ] CI: all existing + new tests pass
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: [PR] Extend re-entrancy detection for broken trace chains [maven-resolver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#discussion_r3645488199
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java:
##########
@@ -587,6 +670,41 @@ private static boolean isReentrant(RequestTrace trace) {
return false;
}
+ /**
+ * Combined re-entrancy check using both {@link RequestTrace} ancestry and session-scoped
+ * depth tracking. Either mechanism detecting re-entrancy is sufficient to skip validation.
+ * <p>
+ * The trace-based check is the primary mechanism and works when callers properly propagate
+ * traces. The session-based check is a fallback for callers that rebuild the trace chain
+ * from a different tracing system (e.g. Maven 4's trace conversion loses the resolver's
+ * re-entrancy marker).
+ *
+ * @param trace the current request trace (may be {@code null})
+ * @param session the current repository system session
+ * @return {@code true} if this is a re-entrant call, {@code false} if it is the outermost call
+ */
+ private static boolean isReentrant(RequestTrace trace, RepositorySystemSession session) {
+ return isReentrant(trace) || getReentryDepth(session).get() > 0;
+ }
+
+ /**
+ * Increments the session-scoped re-entrancy depth counter. Must be called on every outermost
+ * entry into a public {@code RepositorySystem} method, and the returned {@link Runnable} must
+ * be invoked in a {@code finally} block to decrement the counter on exit.
+ *
+ * @param session the current repository system session
+ * @return a {@link Runnable} that decrements the depth counter when invoked
+ */
+ private static Runnable enterSessionScope(RepositorySystemSession session) {
+ AtomicInteger depth = getReentryDepth(session);
+ depth.incrementAndGet();
+ return depth::decrementAndGet;
+ }
+
+ private static AtomicInteger getReentryDepth(RepositorySystemSession session) {
+ return (AtomicInteger) session.getData().computeIfAbsent(SESSION_REENTRY_DEPTH_KEY, () -> new AtomicInteger(0));
+ }
Review Comment:
The session-scoped `AtomicInteger` depth counter is shared across all threads using the same `RepositorySystemSession`. If the session is used concurrently (parallel resolution), one thread can make another thread’s *non-reentrant* call look re-entrant (`depth > 0`), causing validation to be skipped incorrectly. Consider making the depth tracking thread-scoped (e.g., store a `ThreadLocal<Integer>`/`ThreadLocal<AtomicInteger>` in session data, or store a per-thread counter keyed by `Thread.currentThread()`/thread id with proper cleanup) so only true call-stack re-entrancy on the same thread suppresses validation. Update the Javadoc to reflect the chosen semantics.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:
##########
@@ -303,4 +312,120 @@ void outerCallWithNullTraceStillStampsMarker() throws Exception {
system.resolveVersionRange(session, innerRequest);
assertEquals(countBefore, validationCount.get(), "Re-entrant call should skip validation");
}
+
+ @Test
+ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception {
+ // Simulates Maven 4's flow where the model builder converts between Maven API traces
+ // and resolver traces via RequestTraceHelper, losing the REPOSITORY_SYSTEM_CALL marker.
+ //
+ // The DependencyCollector, called from within collectDependencies, re-enters
+ // RepositorySystem.resolveVersionRange with a FRESH trace (no marker in ancestry)
+ // and an uninterpolated expression like ${project.version}. Without session-scoped
+ // detection, the validator would reject the expression; with it, the call is
+ // detected as re-entrant and validation is skipped.
+ AtomicReference<DefaultRepositorySystem> systemRef = new AtomicReference<>();
+
+ DependencyCollector reentrantCollector = (s, request) -> {
+ // Inside collectDependencies, simulate a re-entrant call with a FRESH trace
+ // (no marker in ancestry — this is the broken path) and an uninterpolated
+ // expression. If session-scoped detection fails, the validator rejects this.
+ VersionRangeRequest innerRequest = new VersionRangeRequest(
+ new DefaultArtifact("g:inner:${project.version}"), Collections.emptyList(), null);
+ // Fresh trace — no REPOSITORY_SYSTEM_CALL marker (simulates Maven's trace conversion)
+ innerRequest.setTrace(RequestTrace.newChild(null, "MavenModelResolver"));
+ try {
+ systemRef.get().resolveVersionRange(s, innerRequest);
+ } catch (Exception e) {
+ fail("Re-entrant call with broken trace chain should succeed via session-scoped detection: " + e);
+ }
Review Comment:
Catching `Exception` and concatenating `e` into the failure message drops the stack trace from the assertion output, which makes failures harder to diagnose. Prefer `fail(message, e)` (or an assertion that preserves the throwable) so the original exception is reported with full context.
--
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: [PR] Extend re-entrancy detection for broken trace chains and managed dep validation [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#issuecomment-5058397913
Thanks for the feedback @cstamas! You're right — removing managed dependency validation entirely is too broad.
I've reverted the managed dependency validation changes in 74005921. This PR now focuses solely on **session-scoped re-entrancy detection** (the `AtomicInteger` depth counter in session data), which addresses the broken trace chain scenario independently of the managed dependency question.
The managed dependency separation (direct vs managed validation) is better handled by #2008's `validateManagedDependency()` approach, which lets MavenValidator abstain while keeping the validation available for non-Maven consumers.
--
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: [PR] Extend re-entrancy detection for broken trace chains and managed dep validation [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#issuecomment-5049680971
See https://github.com/apache/maven-resolver/pull/2008
MavenValidator should just not implement new method.
--
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: [PR] Extend re-entrancy detection for broken trace chains [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009
--
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: [PR] Extend re-entrancy detection for broken trace chains [maven-resolver]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#issuecomment-5222335179
@gnodet Please assign appropriate label to PR according to the type of change.
--
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: [PR] Extend re-entrancy detection for broken trace chains [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2009:
URL: https://github.com/apache/maven-resolver/pull/2009#discussion_r3663824849
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java:
##########
@@ -587,6 +670,41 @@ private static boolean isReentrant(RequestTrace trace) {
return false;
}
+ /**
+ * Combined re-entrancy check using both {@link RequestTrace} ancestry and session-scoped
+ * depth tracking. Either mechanism detecting re-entrancy is sufficient to skip validation.
+ * <p>
+ * The trace-based check is the primary mechanism and works when callers properly propagate
+ * traces. The session-based check is a fallback for callers that rebuild the trace chain
+ * from a different tracing system (e.g. Maven 4's trace conversion loses the resolver's
+ * re-entrancy marker).
+ *
+ * @param trace the current request trace (may be {@code null})
+ * @param session the current repository system session
+ * @return {@code true} if this is a re-entrant call, {@code false} if it is the outermost call
+ */
+ private static boolean isReentrant(RequestTrace trace, RepositorySystemSession session) {
+ return isReentrant(trace) || getReentryDepth(session).get() > 0;
+ }
+
+ /**
+ * Increments the session-scoped re-entrancy depth counter. Must be called on every outermost
+ * entry into a public {@code RepositorySystem} method, and the returned {@link Runnable} must
+ * be invoked in a {@code finally} block to decrement the counter on exit.
+ *
+ * @param session the current repository system session
+ * @return a {@link Runnable} that decrements the depth counter when invoked
+ */
+ private static Runnable enterSessionScope(RepositorySystemSession session) {
+ AtomicInteger depth = getReentryDepth(session);
+ depth.incrementAndGet();
+ return depth::decrementAndGet;
+ }
+
+ private static AtomicInteger getReentryDepth(RepositorySystemSession session) {
+ return (AtomicInteger) session.getData().computeIfAbsent(SESSION_REENTRY_DEPTH_KEY, () -> new AtomicInteger(0));
+ }
Review Comment:
Fixed in 48c7a292. Replaced the session-scoped `AtomicInteger` with a `ThreadLocal<int[]>` — re-entrancy is per-call-stack, so thread-scoped is the correct semantic. This avoids false positives in parallel builds where multiple threads share a session.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:
##########
@@ -303,4 +312,120 @@ void outerCallWithNullTraceStillStampsMarker() throws Exception {
system.resolveVersionRange(session, innerRequest);
assertEquals(countBefore, validationCount.get(), "Re-entrant call should skip validation");
}
+
+ @Test
+ void sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken() throws Exception {
+ // Simulates Maven 4's flow where the model builder converts between Maven API traces
+ // and resolver traces via RequestTraceHelper, losing the REPOSITORY_SYSTEM_CALL marker.
+ //
+ // The DependencyCollector, called from within collectDependencies, re-enters
+ // RepositorySystem.resolveVersionRange with a FRESH trace (no marker in ancestry)
+ // and an uninterpolated expression like ${project.version}. Without session-scoped
+ // detection, the validator would reject the expression; with it, the call is
+ // detected as re-entrant and validation is skipped.
+ AtomicReference<DefaultRepositorySystem> systemRef = new AtomicReference<>();
+
+ DependencyCollector reentrantCollector = (s, request) -> {
+ // Inside collectDependencies, simulate a re-entrant call with a FRESH trace
+ // (no marker in ancestry — this is the broken path) and an uninterpolated
+ // expression. If session-scoped detection fails, the validator rejects this.
+ VersionRangeRequest innerRequest = new VersionRangeRequest(
+ new DefaultArtifact("g:inner:${project.version}"), Collections.emptyList(), null);
+ // Fresh trace — no REPOSITORY_SYSTEM_CALL marker (simulates Maven's trace conversion)
+ innerRequest.setTrace(RequestTrace.newChild(null, "MavenModelResolver"));
+ try {
+ systemRef.get().resolveVersionRange(s, innerRequest);
+ } catch (Exception e) {
+ fail("Re-entrant call with broken trace chain should succeed via session-scoped detection: " + e);
+ }
Review Comment:
Fixed in 48c7a292. Changed to `fail(msg, e)` to preserve the full stack trace.
--
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