Skip validation and decoration on re-entrant RepositorySystem calls

[gnodet]

Summary

Maven 4's ArtifactDescriptorReader → ModelBuilder → ModelResolver chain re-enters RepositorySystem during collectDependencies. This breaks the resolver's single-crossing contract:

• Validation rejects intermediate state: MavenValidator (registered via the ValidatorFactory SPI) rejects uninterpolated ${...} expressions that are valid intermediate state in transitive POMs during model building
• Artifact decorators run redundantly: decorators applied on inner calls corrupt the resolution result
• Reported as: MAVEN #12474 — Invalid Collect Request: null

How it works

On the outermost call to any RepositorySystem public method, a sentinel marker is stamped into the request's RequestTrace. On re-entry (whether on the same thread or a pool thread), isReentrant() walks the trace ancestry — if the marker is found, validation and decoration are skipped.

This leverages the existing RequestTrace infrastructure which is already propagated across threads by callers (Maven's model builder explicitly copies traces to pool threads via session.setCurrentTrace(trace)), requiring no ThreadLocal or session-scoped state.

Changes

• DefaultRepositorySystem: all public resolution methods check isReentrant(trace) before validating/decorating
• readArtifactDescriptor: additionally skips artifact decoration on re-entry
• Methods without trace-bearing requests (install, deploy, newResolutionRepositories, newDeploymentRepository, flattenDependencyNodes) always validate — they are terminal operations that don't participate in re-entrancy
• New test class DefaultRepositorySystemReentrancyTest with 5 tests covering:
  • Outermost calls run validation
  • Re-entrant calls skip validation
  • Re-entrant calls allow uninterpolated expressions (the bug scenario)
  • Re-entrant readArtifactDescriptor skips decoration
  • Independent calls each validate independently

Test plan

• All 445 existing tests pass in maven-resolver-impl
• 5 new re-entrancy tests pass
• CI build passes
• Integration test with Maven 4 against the reproducer from #12474

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds re-entrancy detection to DefaultRepositorySystem so that when Maven 4 re-enters RepositorySystem during an in-flight resolution (via RequestTrace propagation), the inner calls skip session/request validation and artifact decoration to avoid rejecting intermediate model states and to prevent decorator-induced corruption.

Changes:

• Introduces a RequestTrace sentinel marker and isReentrant(...) traversal to detect nested RepositorySystem invocations.
• Gates validation (and, for readArtifactDescriptor, decoration) on the outermost call for several public RepositorySystem methods.
• Adds DefaultRepositorySystemReentrancyTest covering outermost vs re-entrant validation and readArtifactDescriptor decoration behavior.

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 RequestTrace-based re-entrancy detection and conditionally skips validation/decoration on inner calls.
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java	Adds unit tests validating the intended re-entrancy behavior and the reported Maven 4 intermediate-state scenario.

---

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

[gnodet]

Good catch — fixed in eb1e2af. resolveArtifacts() now stamps REPOSITORY_SYSTEM_CALL into each request's trace on outermost calls, matching the single-request methods.

[gnodet]

Good catch — fixed in eb1e2af. resolveArtifacts() now stamps REPOSITORY_SYSTEM_CALL into each request's trace on outermost calls, matching the single-request methods.

[gnodet]

Same fix applied in eb1e2af — resolveMetadata() now also stamps the marker into each request's trace on outermost calls.

[gnodet]

Review Summary

Well-designed re-entrancy detection using RequestTrace ancestry instead of ThreadLocal, correctly handling both same-thread and cross-thread re-entry.

What's good

• Design choice: Using RequestTrace ancestry instead of ThreadLocal is the right call. ThreadLocal would break when BfDependencyCollector dispatches work to pool threads (a known re-entrancy path). RequestTrace is explicitly propagated across threads by callers like Maven's model builder, making it the correct carrier for this marker.

• Sentinel pattern: The private static final sentinel with identity comparison (==) is clean and avoids accidental collisions with user data in the trace chain.

• Documentation: The Javadoc on REPOSITORY_SYSTEM_CALL is excellent — it explains the architectural assumption, why it breaks, the two re-entrancy paths (same-thread and cross-thread), and why RequestTrace is the right mechanism. This documentation is invaluable for future maintainers.

• resolveDependencies correctness: When calling dependencyCollector.collectDependencies directly (bypassing the public facade), the trace passed to the CollectRequest already carries the marker in its ancestry, so downstream re-entry through the public facade is correctly detected.

Minor observations (non-blocking)

• Batch trace assumption (line ~280): The comment "All requests in a batch share the same trace context, so checking any one is sufficient" is a stated assumption but is not enforced. In practice this always holds since batch requests are constructed within a single operation, but a defensive check could prevent subtle bugs for future consumers.

• Null-check ordering (line ~222): requireNonNull(request) now runs before validateSession(session) on outermost calls (previously session was validated first). This is necessary for the re-entrancy check but changes which exception is thrown when both are null. Negligible impact.

• Shutdown on re-entry (line ~580): On re-entrant calls, validateSystem() is also skipped since it's inside validateSession(). If shutdown() is called mid-operation, a re-entrant call would proceed. This is an extremely unlikely race and consistent with the principle that the outermost call owns the lifecycle contract.

Looks good overall. The tests cover the key single-threaded scenarios. Cross-thread re-entrancy isn't explicitly tested, but since the mechanism is purely trace-chain-based (no thread affinity), the single-threaded tests are sufficient to validate the logic.

---

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

[gnodet]

Thanks for the thorough review! Addressing the observations:

1. Batch trace assumption: Agreed — in practice all requests in a batch share the same trace context since they're constructed within a single operation. Adding an assertion or enforcement would be defensive but could also be confusing since there's no documented contract for this. Happy to add a comment clarifying the assumption if desired.

2. Null-check ordering: Correct, requireNonNull(request) now precedes validateSession(session) since we need the request to access the trace. The behavioral change is negligible (different exception when both are null).

3. Shutdown on re-entry: Right — the outermost call owns the lifecycle contract, and re-entrant calls within an in-flight operation should proceed regardless. This is consistent with the general principle that shutdown is cooperative, not preemptive.

Regarding cross-thread testing: as you noted, the mechanism is purely trace-chain-based with no thread affinity, so the single-threaded tests are sufficient. The BfDependencyCollector.SmartExecutor propagates the RequestTrace to pool threads, which is what makes the trace-based approach work across thread boundaries.

A companion PR for Maven will follow to remove the consumer-side dependency filtering workarounds that are no longer needed with this fix.