Extend re-entrancy detection for broken trace chains by gnodet · Pull Request #2009 · apache/maven-resolver

gnodet · 2026-07-22T15:29:15Z

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 to remove the consumer-side workarounds for uninterpolated expressions.

Context

PR #1957 — original re-entrancy detection (trace-based)
apache/maven#12481 — Maven PR that removes consumer-side workarounds
MavenITgh12305 — the IT test that exercises this scenario

Test plan

New test: sessionScopedDetectionSkipsValidationWhenTraceChainIsBroken — verifies inner calls with broken trace + uninterpolated expressions succeed via session-scoped detection
New test: collectDependenciesAcceptsManagedDepsWithUninterpolatedExpressions — reproduces the MavenITgh12305 scenario
New test: collectDependenciesStillRejectsInvalidDirectDependencies — ensures direct deps are still validated
New test: sessionScopedDepthIsProperlyDecrementedOnExit — verifies counter cleanup for independent calls
CI: all existing + new tests pass

🤖 Generated with Claude Code

…naged dependency validation The trace-based re-entrancy detection added in PR #1957 does not cover all code paths. Maven 4's RequestTraceHelper converts between Maven API traces and resolver RequestTrace objects, creating a fresh trace chain that loses the REPOSITORY_SYSTEM_CALL marker. This causes MavenValidator to reject re-entrant calls that carry uninterpolated property expressions. This commit addresses the gap with two complementary fixes: 1. Session-scoped re-entrancy detection: Each public RepositorySystem method now maintains a depth counter in SessionData via enterSessionScope()/ exitSessionScope(). 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. 2. Skip managed dependency validation in validateCollectRequest(): 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 dependencies with uninterpolated property expressions (e.g. ${osgi.version}) that are never actually used. If a managed dependency IS matched and its coordinates are invalid, the error surfaces during version/artifact resolution. Together, these changes allow Maven PR #12481 to remove the consumer-side workarounds for uninterpolated expressions in DefaultArtifactDescriptorReader, ArtifactDescriptorReaderDelegate, DefaultProjectDependenciesResolver, and DefaultDependencyResolver. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

cstamas · 2026-07-22T18:05:12Z

See #2008

MavenValidator should just not implement new method. But in some other, non-Maven use cases, uninterpolated depMgt may be seen as error.

Revert the managed dependency validation removal from DefaultRepositorySystemValidator. As cstamas noted, managed deps should remain validatable for non-Maven use cases — the proper separation between direct and managed dependency validation belongs in PR #2008 (validateManagedDependency method). This PR now focuses solely on session-scoped re-entrancy detection via an AtomicInteger depth counter in session data, which covers the broken trace chain scenario independently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet · 2026-07-23T12:30:48Z

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 7400592. 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.

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.

Extends re-entrancy detection in DefaultRepositorySystem to handle cases where consumers rebuild/break RequestTrace chains (e.g., Maven 4 trace conversion), and adjusts validation behavior to avoid rejecting builds due to uninterpolated expressions in managed dependencies.

Changes:

Add session-scoped re-entrancy depth tracking (via SessionData) to supplement trace-marker-based detection.
Wrap public RepositorySystem entry points with try/finally guards to ensure depth counters are decremented reliably.
Add new tests covering broken trace chains, managed dependency validation behavior, and depth cleanup.

Reviewed changes

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

File	Description
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java	Adds session-scoped depth-based re-entrancy detection and applies it across public entry points.
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java	Adds regression tests for broken trace chains/uninterpolated expressions and validates depth counter cleanup.

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

gnodet · 2026-07-28T08:01:25Z

+    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));
+    }


Fixed in 48c7a29. 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.

gnodet · 2026-07-28T08:01:32Z

+            try {
+                systemRef.get().resolveVersionRange(s, innerRequest);
+            } catch (Exception e) {
+                fail("Re-entrant call with broken trace chain should succeed via session-scoped detection: " + e);
+            }


Fixed in 48c7a29. Changed to fail(msg, e) to preserve the full stack trace.

gnodet

The session-scoped re-entrancy depth counter is a sound approach for catching broken trace chains. The try-finally guard pattern is consistently and correctly applied across all 8 public methods. A few observations:

Medium severity:

🧵 Session-scoped vs thread-scoped counter: The AtomicInteger depth counter in SessionData is shared across all threads using the same session. Re-entrancy is inherently per-call-stack, so in parallel builds with a shared session, Thread B's independent outermost call could see depth > 0 from Thread A and skip validation ("fail-open"). Practical impact is limited since the primary trace-based isReentrant(trace) is per-call-stack and the session counter is a fallback for the Maven 4 RequestTraceHelper edge case (single-threaded model builder). A ThreadLocal would be more semantically correct but may be over-engineering for this specific use case.

Low severity:

fail("msg: " + e) at test line 340 discards the exception stack trace. JUnit 5's fail(String, Throwable) overload preserves the full context for easier diagnosis.
No test verifies that the depth counter is properly decremented when the delegate throws an exception. The try-finally pattern guarantees correctness, but a test proving cleanup on error would strengthen confidence.

Strengths:

Clean revert of the managed dependency validation concern to the already-merged PR #2008.
Consistent try-finally exitGuard across all 8 relevant methods; terminal operations (install, deploy, etc.) correctly left unmodified.
The anonymous Object subclass with toString() for the session data key is a good pattern for identity-based key safety.

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

Claude Code on behalf of Guillaume Nodet

Address review feedback: re-entrancy is per-call-stack, so a ThreadLocal is the correct semantic. The previous AtomicInteger in SessionData was shared across threads, causing false positives (skipped validation) in parallel builds where multiple threads share a single session. Also fix fail() call in test to preserve the exception stack trace via fail(msg, cause) instead of fail(msg + e), and add a test verifying the depth counter is properly decremented when the delegate throws. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

Re-review after latest commit (48c7a29).

All three concerns from the previous review have been directly and correctly addressed:

Thread-scoped depth counter — session-scoped AtomicInteger replaced with ThreadLocal<int[]>, giving per-call-stack semantics that avoid false positives in parallel builds. The int[] pattern avoids boxing overhead and makes the captured-Runnable decrement trivial.
Stack trace preservation — fail(msg + e) changed to fail(msg, e) to preserve exception stack traces.
Exception cleanup test — new test threadScopedDepthIsDecrementedWhenDelegateThrows confirms the try-finally guard works on the exception path.

The Runnable-based exitGuard with try-finally is consistently applied across all 8 relevant public methods. Terminal operations (install, deploy, etc.) are correctly left unmodified.

A couple of minor, non-blocking observations:

The method enterSessionScope no longer touches session state — it operates on a ThreadLocal. The name is a holdover from the previous implementation. Consider renaming to enterReentrantScope or enterThreadScope to match the actual semantics.
The PR branch is behind master due to the #2008 merge (validator factory abstain support). A rebase will be needed before merge, with mechanical updates to the test constructors (singletonList → singletonMap).
The PR description still mentions "Skip managed dependency validation" as a change, but that was reverted in commit 7400592. The description should be updated to reflect the current scope (thread-scoped re-entrancy detection only).

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

Claude Code on behalf of gnodet

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

Re-review (3rd pass) after commit 94d9fda.

The only change since the last review is a trivial spotless formatting fix (line wrapping in one test variable declaration). The full implementation remains correct:

ThreadLocal-based per-thread depth tracking avoids false positives in parallel builds
Consistent try-finally guards across all 8 re-entrancy-aware public methods
Comprehensive test coverage including exception safety path
CI is fully green across all matrix jobs

All concerns from previous reviews remain addressed. Rebase against master is still needed for the #2008 constructor signature change (List<ValidatorFactory> → Map<String, ValidatorFactory>) but the update is mechanical.

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

Claude Code on behalf of gnodet

gnodet and others added 2 commits July 22, 2026 15:28

fix spotless formatting violation in re-entrancy test

7e400b2

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet marked this pull request as ready for review July 23, 2026 11:43

gnodet changed the title ~~Extend re-entrancy detection for broken trace chains and managed dep validation~~ Extend re-entrancy detection for broken trace chains Jul 23, 2026

elharo requested a review from Copilot July 24, 2026 12:47

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Copilot started reviewing on behalf of elharo July 24, 2026 13:14 View session

gnodet commented Jul 24, 2026

View reviewed changes

gnodet commented Jul 28, 2026

View reviewed changes

fix: spotless formatting violation in test

94d9fda

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet commented Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extend re-entrancy detection for broken trace chains - #2009

Extend re-entrancy detection for broken trace chains#2009
gnodet wants to merge 5 commits into
masterfrom
the-re-entrancy-detection-added-in-pr-1957-merge

gnodet commented Jul 22, 2026

Uh oh!

cstamas commented Jul 22, 2026 •

edited

Loading

Uh oh!

gnodet commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

gnodet Jul 28, 2026

Uh oh!

gnodet Jul 28, 2026

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gnodet commented Jul 22, 2026

Summary

Context

Test plan

Uh oh!

cstamas commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnodet commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

gnodet Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cstamas commented Jul 22, 2026 •

edited

Loading