Fix #12607: DefaultMaven session scope leak on constructor exception

[elharo]

Fixes #12607

Problem

In DefaultMaven.doExecute(), sessionScope.enter() was called outside the try-finally block:

sessionScope.enter();
MavenChainedWorkspaceReader chainedWorkspaceReader =
        new MavenChainedWorkspaceReader(request.getWorkspaceReader(), ideWorkspaceReader);
try (CloseableSession closeableSession = ...) {
    ...
} finally {
    sessionScope.exit();
}

If new MavenChainedWorkspaceReader(...) threw (it can throw an NPE if a WorkspaceReader's getRepository() returns null), the sessionScope.exit() in the finally block would never execute. This leaked the session scope's thread-local state.

Fix

Wrapped subsequent code in an outer try-finally, ensuring sessionScope.exit() is always called regardless of where the exception occurs.

Testing

Added DefaultMavenSessionScopeTest which:

1. Creates a mock WorkspaceReader whose getRepository() returns null (triggering NPE in MavenChainedWorkspaceReader)
2. Calls DefaultMaven.execute() with this bad reader
3. Verifies the session scope is properly exited (empty internal values list) even though the request failed

[gnodet]

LGTM ✅

Correct and minimal fix for a real session scope thread-local leak when MavenChainedWorkspaceReader construction fails between enter() and the try block. The test validates the exact failure scenario.

Details:

• The production code change moves the MavenChainedWorkspaceReader construction (and newCloseableSession call) inside a new outer try block that guarantees sessionScope.exit() runs in all paths after sessionScope.enter(). The normal execution path is unchanged; only the error path is fixed.

Minor observations (non-blocking):

• The test verifies scope cleanup via reflection on a parent class field (getSuperclass().getDeclaredField("values")). This couples the test to SessionScope's internal hierarchy/naming, but is acceptable since no public API exists to query scope state.
• null is passed for the non-nullable superPomProvider constructor parameter — works because the NPE occurs before it's used, but mock(SuperPomProvider.class) would be slightly more robust.

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

Claude Code on behalf of gnodet

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

Fixes a session-scope leak in DefaultMaven.doExecute() when MavenChainedWorkspaceReader construction throws, and adds a regression test to ensure SessionScope.exit() always runs.

Changes:

• Wraps workspace-reader construction and session creation in an outer try/finally to always call sessionScope.exit().
• Adds a JUnit test that simulates a failing WorkspaceReader and asserts the SessionScope state is cleaned up.

Reviewed changes

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

File	Description
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java	Ensures sessionScope.exit() executes even if workspace reader construction fails.
impl/maven-core/src/test/java/org/apache/maven/DefaultMavenSessionScopeTest.java	Adds regression coverage for session-scope cleanup on workspace-reader errors.

---

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