Fix for ChainedWorkspaceReader

[cstamas]

The reader instances once set, are immutable, but repository instances returned by readers may change (their key).

[gnodet]

Hi @cstamas, thanks for the clean-up! The reasoning is sound — since WorkspaceRepository is immutable and Key captures repository keys at construction time, the lazy-recreation logic in getRepository() is indeed redundant given that the readers list is now fixed at construction time.

A few minor points worth discussing before merging:

1. Null-handling behaviour change (see inline): passing a literal null varargs now throws NPE immediately (previously it was silently treated as an empty list), while individual null elements in the array are now silently dropped (previously they were included and would NPE later). Neither case is part of the documented contract, but it’s a subtle behaviour change that could surprise callers.

2. Undocumented assumption about reader stability (see inline): the correctness of the new code rests on the assumption that WorkspaceReader implementations return a stable WorkspaceRepository (same key) for their entire lifetime. WorkspaceRepository is immutable, which makes this very likely in practice, but WorkspaceReader is an interface. A brief comment capturing this assumption would make the code easier to reason about for future contributors.

3. Minor style (see inline): Collectors.toCollection(ArrayList::new) can be simplified.

4. Tests: there are no unit tests for ChainedWorkspaceReader. A basic test covering construction, findArtifact, findVersions, and getRepository would be a nice addition — especially one that verifies the null-filtering behaviour now that it is an explicit choice.

The @Override additions are good hygiene — thanks for those.

Overall this is a reasonable simplification; just wanted to flag the subtle behaviour differences. Happy to see it land once the above is addressed (or consciously accepted).

— Guillaume (gnodet)

[gnodet]

Nit: note that this slightly changes the null-handling behaviour compared to the previous code. The old Collections.addAll(this.readers, readers) would silently accept a null varargs array (treating it as empty), whereas Arrays.stream(readers) will throw NullPointerException if readers itself is null. On the other hand, individual null elements in the array were previously included (causing an NPE later when iterating), while now they are filtered out. Worth documenting these behaviour changes in Javadoc or a comment, even if null inputs aren’t part of the intended public contract.

[gnodet]

The simplification here hinges on the assumption that WorkspaceReader.getRepository() always returns a repository whose getKey() is stable for the lifetime of the reader. WorkspaceRepository itself is indeed immutable (its key field is final), but WorkspaceReader is an interface — an implementation could theoretically return a different WorkspaceRepository instance with a different key on each invocation. The old code defended against that case by re-checking the key on every call. Would be good to document this assumption, e.g. with a comment like // Assumes WorkspaceReader.getRepository() returns a stable (immutable) repository for the reader’s lifetime.

[gnodet]

Minor style point: since the collected list is immediately wrapped in Collections.unmodifiableList, the exact ArrayList implementation doesn’t matter. You could simplify to .collect(Collectors.toList()), or, since maven-resolver targets Java 11+, even use Stream.of(readers).filter(Objects::nonNull).collect(Collectors.toUnmodifiableList()) (Java 10+).

[cstamas]

@gnodet Windows failed with

Error:  org.eclipse.aether.named.ipc.IpcNamedLockFactoryIT.exclusiveAccess(TestInfo) -- Time elapsed: 25.01 s <<< ERROR!
java.util.concurrent.TimeoutException: exclusiveAccess(org.junit.jupiter.api.TestInfo) timed out after 25 seconds
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
	Suppressed: java.lang.InterruptedException
		at java.base/java.lang.Object.wait0(Native Method)
		at java.base/java.lang.Object.wait(Object.java:389)
		at java.base/java.lang.Thread.join(Thread.java:1887)
		at java.base/java.lang.Thread.join(Thread.java:1963)
		at org.eclipse.aether.named.ipc.NamedLockFactoryTestSupport.exclusiveAccess(NamedLockFactoryTestSupport.java:147)
		at java.base/java.lang.reflect.Method.invoke(Method.java:565)
		... 2 more

See https://github.com/apache/maven-resolver/actions/runs/27139141318/job/80106163823?pr=1909

[gnodet]

The IpcNamedLockFactoryIT.exclusiveAccess() Windows CI failure has been investigated and a fix is up at #1910.

Root cause: When the loser thread's lock acquisition times out in doLockExclusively(), the cleanup call client.unlock(contextId) could throw RuntimeException (e.g., due to Windows-specific socket behavior). This exception propagated out of Access.run() — which only catches InterruptedException — causing the thread to crash without calling loser.countDown(). The winner thread then waited on loser.await() forever, and Surefire killed the JVM at 25 seconds.

A secondary issue: client.unlock() used Long.MAX_VALUE timeout, so if the server/receiver was unresponsive, the thread would hang indefinitely instead of failing fast.

[cstamas]

Fixed method getRepository(), as it is expected to use unstable keys (ie in IDE integration). See constructor WorkspaceRepository(String, Object) for explanation.
So observation was right, thanks! @gnodet

[gnodet]

Thanks @cstamas — the previous concerns are well addressed:

• Null handling: the null-filtering behaviour is now explicit and well-tested (constructNullReader, constructNullVararg, constructMultipleWithNulls). Good.
• Reader stability / unstable keys: understood and agreed — IDE integrations do use unstable keys. The AtomicReference + getAndUpdate approach in getRepository() handles this correctly and thread-safely. The comment from WorkspaceRepository(String, Object) constructor already documents this expectation.
• Tests: comprehensive coverage including concurrency — nice work.

Two minor observations (non-blocking):

1. concurrentKeyChange test: ArrayList is not thread-safe — the concurrent results.add() from multiple threads could corrupt the list. Consider Collections.synchronizedList(new ArrayList<>(threads)) or a ConcurrentLinkedQueue. Unlikely to cause issues in practice with only 10 threads, but it's technically racy.

2. getAndUpdate + get() in getRepository(): getAndUpdate returns the old value, then get() fetches the current one — which another thread may have changed in between. Using updateAndGet instead would return the value this call produced. Given unstable keys, this is inconsequential in practice (any returned value is potentially stale by the time the caller uses it), but updateAndGet would be slightly more precise.

Stream style (Arrays.stream().filter().forEach() vs collectors) — no change needed, it's readable as-is.

LGTM overall. Approving.

Claude Code on behalf of Guillaume Nodet