[MNG-8671] Add request trace context to derived sessions - #13009
Conversation
Add Session.withContext(RequestTrace) and propagate the scoped trace through derived sessions and Resolver operations. Restore prior traces exactly and cover nesting, concurrency, repository derivation, install, and deploy paths. Signed-off-by: goutamadwant <workwithgoutam@gmail.com>
820c9aa to
9ef77c6
Compare
gnodet
left a comment
There was a problem hiding this comment.
Well-designed enhancement adding request trace context to derived sessions. The implementation is thread-safe, the restore logic (using previousMvnTrace instead of mvnTrace().parent()) is an improvement over the prior parent-based approach, tests are comprehensive (propagation, nesting, concurrency, validation, install/deploy), and all Session subclasses are consistently updated.
Two minor observations below — neither blocks merging.
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
| public void setCurrentTrace(RequestTrace trace) { | ||
| getTraceHolder().set(trace); | ||
| if (trace == context) { | ||
| getTraceHolder().remove(); | ||
| } else { | ||
| getTraceHolder().set(trace); | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor hygiene nit: when setCurrentTrace(null) is called (e.g. in DefaultModelBuilder's PhasingExecutor finally blocks), it takes the else branch and calls getTraceHolder().set(null) rather than remove(). This retains a null entry in the ThreadLocal map rather than cleaning it up — a minor concern in thread-pool scenarios.
Consider:
| public void setCurrentTrace(RequestTrace trace) { | |
| getTraceHolder().set(trace); | |
| if (trace == context) { | |
| getTraceHolder().remove(); | |
| } else { | |
| getTraceHolder().set(trace); | |
| } | |
| } | |
| public void setCurrentTrace(RequestTrace trace) { | |
| if (trace == context || trace == null) { | |
| getTraceHolder().remove(); | |
| } else { | |
| getTraceHolder().set(trace); | |
| } | |
| } |
| } | ||
|
|
||
| @Override | ||
| public Session withContext(RequestTrace trace) { |
There was a problem hiding this comment.
Nit: withContext() returns null despite the @Nonnull contract on Session.withContext(). This is consistent with the existing stub pattern (withLocalRepository() and withRemoteRepositories() also return null), so it matches convention — just noting for awareness.
Remove the per-thread trace entry when restoring a null trace instead of retaining a null ThreadLocal value.
gnodet
left a comment
There was a problem hiding this comment.
Re-review after 1911a16c — the new commit addresses the previous review's finding about setCurrentTrace(null) retaining a null ThreadLocal entry. The fix correctly adds trace == null to the remove() branch, ensuring proper cleanup in thread-pool scenarios.
Previous findings status:
- ThreadLocal cleanup on null trace — ✅ Addressed.
getTraceHolder().remove()is now called for bothnullandcontexttraces. - SessionStub.withContext() returning null — Informational only, no change expected. Consistent with existing stub pattern.
No new issues found in the follow-up commit.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Fixes #10420
Description
Add
Session.withContext(RequestTrace)so callers can create a derived Maven session with a non-null base trace context.The scoped context is preserved across local and remote repository derivation and propagated through Resolver operations, including artifact resolution, installation, and deployment. Nested and explicit request traces restore the exact previous trace after completion.
The active trace remains thread-isolated while being visible to Maven sessions that share the same Resolver session.
Tests
mvn --batch-mode verify: passed.Following this checklist to help us incorporate your contribution quickly and easily:
This pull request addresses one issue without unrelated changes.
The description explains what the pull request does, how, and why.
The commit has a meaningful subject and body.
Unit tests cover the behavioral changes.
mvn verifypasses.The complete Core IT command passed in one environment. See the JDK-specific results above.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.