Skip to content

Extend re-entrancy detection for broken trace chains#2009

Open
gnodet wants to merge 3 commits into
masterfrom
the-re-entrancy-detection-added-in-pr-1957-merge
Open

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

Conversation

@gnodet

@gnodet gnodet commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

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

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

gnodet and others added 2 commits July 22, 2026 15:28
…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

cstamas commented Jul 22, 2026

Copy link
Copy Markdown
Member

See #2008

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

@gnodet
gnodet marked this pull request as ready for review July 23, 2026 11:43
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

gnodet commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

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.

@gnodet 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants