[slachiewicz]

Summary of Fixes

1. #1990: ChainedRepositoryListener and ChainedTransferListener now log exceptions from listeners at WARNING level in handleError() instead of silently swallowing them. Added unit tests verifying that an exception in one listener does not disrupt delivery to subsequent listeners in the multicast chain.
2. #1992: Refactored DefaultArtifactResolver and DefaultMetadataResolver to eliminate redundant internal/finally SyncContext.close() calls, ensuring SyncContext instances are closed strictly once. Added unit test testSyncContextIsClosedExactlyOnce verifying single-close lifecycle.
3. #1350 ([MRESOLVER-674]): Seeded Random in GenericVersionTest.testCompareUuidRandom with a fixed seed to ensure deterministic, reproducible test runs across CI environments.

[gnodet]

Two confirmed resource leaks (HIGH) in DefaultArtifactResolver and DefaultMetadataResolver where removing try-with-resources creates a gap: if the second syncContextFactory.newInstance() call throws, the first shared SyncContext is never closed. The original code handled this correctly via try-with-resources semantics. The test for the SyncContext fix is technically valid but misleading — it uses a single SyncContext instance for both shared and exclusive roles, making the assertion comment inaccurate.

The listener error handling improvement and test derandomization are sound.

---

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

[gnodet]

Two confirmed resource leaks (HIGH) in DefaultArtifactResolver and DefaultMetadataResolver where removing try-with-resources creates a gap: if the second syncContextFactory.newInstance() call throws, the first shared SyncContext is never closed. The original code handled this correctly via try-with-resources semantics. The test for the SyncContext fix is technically valid but misleading — it uses a single SyncContext instance for both shared and exclusive roles, making the assertion comment inaccurate.

The listener error handling improvement and test derandomization are sound.

---

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

[gnodet]

Re-review after follow-up commits (0d76d59, a9d1d5c). All previous findings are addressed:

1. ✅ Resource leak (HIGH) — DefaultArtifactResolver: Restored try-with-resources using CloseOnceSyncContext wrapper. The creation window is now safe: if newInstance(session, false) throws, try-with-resources closes the shared context. The CloseOnceSyncContext correctly delegates close() at most once, and the resolve() finally block properly handles the case where current == shared (closes exclusive too via nested try-finally).

2. ✅ Resource leak (HIGH) — DefaultMetadataResolver: Same fix applied symmetrically.

3. ✅ Misleading test (MEDIUM): Now uses two distinct counting instances dispatched by the shared flag, each asserted closed exactly once. New testSharedSyncContextIsClosedWhenExclusiveCannotBeCreated test directly covers the resource leak scenario. Test also added for DefaultMetadataResolver.

4. ✅ JUL imports (LOW): Fully-qualified class names replaced with proper imports. JUL deliberately kept since maven-resolver-util has no SLF4J dependency — reasonable tradeoff.

The CloseOnceSyncContext wrapper is well designed: package-private, clear Javadoc, IllegalStateException on acquire-after-close. The closed field correctly omits volatile since SyncContext is single-threaded by contract.

Note: No milestone is set — this should target 2.0.23.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[cstamas]

Applied two changes:

• while SyncContext is ST, the common pattern in Resolver is use of AtomicBoolean to implement idempotent close method
• applied same pattern to only one existing SyncContext implementation, thus making close method idempotent