Fix thread-safety issue in DefaultModelValidator (#11618)

[abhu85]

Summary

• Fix thread-safety issue where concurrent model validation causes ClassCastException
• Aligns compat layer with Maven 4 implementation which already uses thread-safe collections
• Adds regression test for concurrent validation

Problem

DefaultModelValidator is a @Singleton with a shared mutable HashSet (validIds). When multiple threads validate models concurrently (e.g., using breadth-first dependency collector with aether.dependencyCollector.impl=bf), concurrent read/write operations on the HashSet cause:

java.lang.ClassCastException: class java.util.HashMap$Node cannot
be cast to class java.util.HashMap$TreeNode
    at java.util.HashMap$TreeNode.putTreeVal(HashMap.java:2079)
    at java.util.HashMap.putVal(HashMap.java:634)
    at java.util.HashSet.add(HashSet.java:220)
    at o.a.m.model.validation.DefaultModelValidator.validateId(DefaultModelValidator.java:1145)

This affects users of:

• Clojure tools-deps
• Leiningen with aether.dependencyCollector.impl=bf
• NetBeans IDE

Fix Approach

Replace new HashSet<>() with ConcurrentHashMap.newKeySet() - identical to the solution already implemented in Maven 4's maven-impl module (lines 296, 298).

Files Changed

File	Change
compat/maven-model-builder/.../DefaultModelValidator.java	Replace HashSet → ConcurrentHashMap.newKeySet() + explanatory comment
compat/maven-model-builder/.../DefaultModelValidatorTest.java	Add concurrent validation test (10 threads × 100 models)

Test Plan

• New test testConcurrentValidation() added
• mvn -pl compat/maven-model-builder test
• mvn -Prun-its verify (optional, if reviewer deems necessary)

Compatibility

• Backwards compatible: ConcurrentHashMap.newKeySet() provides same Set interface
• No behavioral change: Same validation logic, just thread-safe storage
• Aligns with Maven 4: Mirrors the implementation in maven-impl module

Risk Assessment

Low risk:

• Minimal diff (3 lines of production code including comment)
• Same fix pattern already proven in Maven 4
• Thread-safe collection is a drop-in replacement

Fixes #11618

[abhu85]

Hi maintainers 👋

Friendly ping on this PR - it's been open for about a week. This is a minimal fix (3 lines of production code) for the thread-safety issue in
DefaultModelValidator that causes ClassCastException during concurrent model validation (#11618).

The fix replaces HashSet with ConcurrentHashMap.newKeySet() - the same approach already used in Maven 4's maven-impl module.

I notice the mergeable state shows as "unstable" - happy to rebase if needed. Would appreciate a review when you have a moment. Thanks!

[gnodet]

Claude Code on behalf of Guillaume Nodet

The fix is correct: replacing HashSet with ConcurrentHashMap.newKeySet() on a @Singleton field that's accessed concurrently is the right approach, and it mirrors what maven-impl already does (lines 296-298). The production change is minimal and low-risk.

Two observations on the test — one stylistic, one substantive (the unbounded cache is a pre-existing concern, not introduced by this PR).

[gnodet]

Nit: the rest of the file uses top-level imports. These fully-qualified inline references work, but adding proper imports would be more consistent with the existing style:

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

[gnodet]

Minor: the thread is created with new Thread(...).start() but never joined or named. doneLatch.await(30, ...) handles termination, but if a thread hangs, the unnamed daemon-less threads will keep the JVM alive after the test times out. Consider Thread.ofVirtual() or at minimum setting the thread as daemon / giving it a name for debuggability. Not a blocker.

[gnodet]

The fix itself is correct. As a pre-existing observation (not introduced by this PR): validIds grows unboundedly across all validations for the lifetime of the singleton — every unique groupId/artifactId/version ever validated is cached forever. In a long-running embedded scenario (IDE, daemon), this is a slow memory leak. The maven-impl module has the same pattern. Might be worth a follow-up issue to scope the cache per-session or use a bounded/weak-ref structure, but that's out of scope here.

[gnodet]

LGTM — the ConcurrentHashMap.newKeySet() fix is correct and minimal.

Claude Code on behalf of Guillaume Nodet