MEDIUM: perArtifactLocks.clear() creates new race after close()

[sfloess]

perArtifactLocks.clear() removes lock objects while other threads may still be synchronized on them.

Location: src/main/java/org/flossware/classloader/MavenRepositoryClassSource.java:419
Severity: MEDIUM - Resource leak
Impact: Concurrent downloads of same artifact after close()

🤖 AI Attribution: opus (80% confidence, 1/1 consensus)

[sfloess]

✅ Fixed in commit aa7edc2

Solution

Remove the perArtifactLocks.clear() call from close() and add a closed re-check inside the synchronized(artifactLock) block in ensureJarCached().

The root cause is that perArtifactLocks.clear() in close() destroys lock objects that other threads may still be synchronized on, or may have already retrieved via computeIfAbsent. After the map is cleared, a subsequent computeIfAbsent call with the same key creates a different lock object, allowing two threads to enter the critical section for the same artifact concurrently. This defeats the per-artifact synchronization and can lead to duplicate concurrent downloads of the same artifact.

The fix has two parts:

1. Stop clearing perArtifactLocks -- the lock objects are plain Object instances with no resources to release; they will be garbage-collected with the parent instance.
2. Add a closed check inside ensureJarCached() after acquiring the per-artifact lock, to prevent threads that passed the initial closed check from proceeding after close() has already cleaned up all cached JAR files and temp paths.

Details

Commit: aa7edc2
Files Modified: src/main/java/org/flossware/classloader/MavenRepositoryClassSource.java
Confidence: 95%
Consensus: 90% agreement across 3 AI models

Rationale

The race condition occurs because perArtifactLocks.clear() removes lock objects from the ConcurrentHashMap while other threads may still be holding references to those objects (either synchronized on them or about to synchronize). Once cleared, computeIfAbsent creates a new, different lock object for the same artifact key. Two threads can then hold different lock objects for the same key, both enter the critical section in ensureJarCached(), and both download the same artifact concurrently -- exactly the scenario the per-artifact locking was designed to prevent.

Removing the clear() call is safe because the lock objects are plain Object instances with zero resource cost. They contain no file handles, no network connections, no memory buffers. They exist solely as synchronization monitors. The entire perArtifactLocks map and all its entries will be garbage-collected when the MavenRepositoryClassSource instance becomes unreachable.

The additional closed check inside ensureJarCached() after acquiring the lock closes a secondary race: a thread that passed the initial closed check in loadClassData() could block on the per-artifact lock while close() runs, then proceed to download a JAR into a source whose cached resources have already been cleaned up. The re-check ensures such threads fail fast with a clear error message.

---

🤖 AI Attribution

Worker AI (Selected Solution)

• Model: opus
• Confidence: 95%
• Approach: Remove the perArtifactLocks.clear() call from close() and add a closed re-check inside the synchronized(artifactLock) block in ensureJarCached().

The root cause is that perArtifactLocks.clear() in close() destroys lock objects that other threads may still be synchronized on, or may have already retrieved via computeIfAbsent. After the map is cleared, a subsequent computeIfAbsent call with the same key creates a different lock object, allowing two threads to enter the critical section for the same artifact concurrently. This defeats the per-artifact synchronization and can lead to duplicate concurrent downloads of the same artifact.

The fix has two parts:

1. Stop clearing perArtifactLocks -- the lock objects are plain Object instances with no resources to release; they will be garbage-collected with the parent instance.
2. Add a closed check inside ensureJarCached() after acquiring the per-artifact lock, to prevent threads that passed the initial closed check from proceeding after close() has already cleaned up all cached JAR files and temp paths.

• Rationale: The race condition occurs because perArtifactLocks.clear() removes lock objects from the ConcurrentHashMap while other threads may still be holding references to those objects (either synchronized on them or about to synchronize). Once cleared, computeIfAbsent creates a new, different lock object for the same artifact key. Two threads can then hold different lock objects for the same key, both enter the critical section in ensureJarCached(), and both download the same artifact concurrently -- exactly the scenario the per-artifact locking was designed to prevent.

Removing the clear() call is safe because the lock objects are plain Object instances with zero resource cost. They contain no file handles, no network connections, no memory buffers. They exist solely as synchronization monitors. The entire perArtifactLocks map and all its entries will be garbage-collected when the MavenRepositoryClassSource instance becomes unreachable.

The additional closed check inside ensureJarCached() after acquiring the lock closes a secondary race: a thread that passed the initial closed check in loadClassData() could block on the per-artifact lock while close() runs, then proceed to download a JAR into a source whose cached resources have already been cleaned up. The re-check ensures such threads fail fast with a clear error message.

Arbiter Decision

• Arbiter Model: opus
• Decision: Selected Fix Bump org.eclipse.jgit:org.eclipse.jgit from 6.8.0.202311291450-r to 6.10.1.202505221210-r #2
• Reasoning: Fix 2 is the best choice for the following reasons:

1. CORRECTNESS AND COMPLETENESS: Fix 2 correctly identifies and addresses the exact race condition described in issue MEDIUM: perArtifactLocks.clear() creates new race after close() #368 with a precisely scoped two-part fix: (a) removing perArtifactLocks.clear() to prevent lock object invalidation, and (b) adding a closed re-check inside the synchronized block in ensureJarCached() to handle the secondary race where a thread passes the initial closed check but blocks on the per-artifact lock while close() runs. Both parts are necessary and sufficient to resolve the issue.

2. CONFIDENCE LEVEL: Fix 2 has the highest confidence at 95%, tied with Fix 3 but meaningfully above Fix 1's 92%.

3. IMPLEMENTATION CLARITY: Fix 2's rationale is the most precise and well-structured of the three. It clearly articulates both the primary race (clear() destroying lock objects mid-synchronization, leading to different lock objects for the same key) and the secondary race (thread passing initial closed check, blocking on lock, then proceeding after close() has cleaned up resources). Fix 3 is similarly good but slightly less detailed on the secondary race scenario.

4. MINIMAL RISK: Fix 2 is tightly scoped to only the race condition issue. Fix 1 includes three tangential changes (CopyOnWriteArrayList for artifacts, readFully EOF exception fix, expanded comments/javadoc) that, while individually reasonable, expand the blast radius and increase review/regression risk for a concurrency fix. Mixing unrelated improvements with a concurrency bug fix is poor practice -- it makes bisection harder and increases the chance of introducing new issues. Fix 2 follows the principle of minimal intervention: change only what is necessary to fix the reported bug.

Fix 2 vs Fix 3: Both are strong candidates with 95% confidence and similar approaches. Fix 2 edges ahead because its rationale more thoroughly explains the secondary race scenario (the thread that passed the initial closed check, blocked on the lock, and then would proceed into a cleaned-up state), and it explicitly frames this as a "secondary race" that the closed re-check closes. This level of precision in the rationale gives higher confidence that the implementer fully understood all the concurrency implications.

• Consensus Score: 90%

Multi-Model Consensus

• Models Proposed Solutions: 3
• Best Solution Selected By: opus

Alternative Proposals (Not Selected)

1. haiku (Confidence: 92%)
   • Approach: Remove the perArtifactLocks.clear() call in the close() method to prevent a race condition where threads could hold references to garbage-collected lock objects. The fix keeps the lock objects alive (they are lightweight Object instances with no resource cost) while clearing the resource caches (jarFileCache and jarPathCache). This prevents new threads from creating different lock objects for the same artifact after close() has been called, which would allow multiple threads to enter the critical section of ensureJarCached() concurrently.

The fix includes:

1. Removing perArtifactLocks.clear() from the close() method

2. Adding comprehensive comments explaining why the locks should not be cleared

3. Adding a closed flag check inside the synchronized block in ensureJarCached() for safety

4. Switching the artifacts collection to CopyOnWriteArrayList for thread-safe concurrent iteration during artifact additions

5. Fixing an exception handling bug in readFully() that was silently returning on EOF instead of throwing

   • Reason Not Selected: Not selected by opus arbiter

6. sonnet (Confidence: 95%)

   • Approach: Remove the perArtifactLocks.clear() call from the close() method to prevent creating a race condition where threads synchronized on old lock objects can't coordinate with threads that get new lock objects after the map is cleared. Instead, rely on the closed flag and existing synchronization to prevent new operations, while allowing in-flight operations to complete safely on their original lock objects.
   • Reason Not Selected: Not selected by opus arbiter

---

🤖 Automatically fixed and committed by code-solve workflow