LOW: Use-after-close race in MavenNexusClassSource

[sfloess]

loadClassData() checks volatile 'closed' but doesn't hold lock, allowing resource creation after close().

Location: src/main/java/org/flossware/classloader/MavenNexusClassSource.java:125
Severity: LOW - Race condition
Impact: Temp file leaks

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

[sfloess]

✅ Fixed in commit aa7edc2

Solution

Replace the synchronized(this) intrinsic monitor with a ReentrantReadWriteLock to eliminate the use-after-close race condition while preserving concurrent class loading throughput.

The core problem: loadClassData() checks the volatile closed flag but does not hold any lock that coordinates with close(). Between the check and the resource creation in ensureJarCached(), close() can run, set closed = true, clean up all existing resources, and return -- leaving any subsequently-created temp files and JarFile handles orphaned (leaked).

The fix uses a ReentrantReadWriteLock:

• loadClassData() acquires the read lock (shared) -- multiple concurrent class loads can proceed in parallel, preserving the existing FutureTask-based parallel download pattern.
• close() acquires the write lock (exclusive) -- it waits for all in-progress loadClassData() calls to complete before setting closed = true and cleaning up resources. Once the write lock is held, no new loadClassData() call can enter.
• addArtifact() acquires the write lock since it mutates the artifacts list that loadClassData() iterates.
• getArtifacts() and getDescription() acquire the read lock for safe concurrent access.

This approach is strictly superior to the prior synchronized(this) attempt which serialized all class loading (defeating the FutureTask parallelism) and held the monitor during network I/O.

Details

Commit: aa7edc2
Files Modified: src/main/java/org/flossware/classloader/MavenNexusClassSource.java
Confidence: 92%
Consensus: 82% agreement across 3 AI models

Rationale

The ReentrantReadWriteLock pattern is the standard Java concurrency solution for this class of use-after-close race conditions. It provides two critical guarantees:

1. No resource creation after close(): The write lock in close() cannot be acquired until all read locks (held by in-progress loadClassData() calls) are released. Once close() holds the write lock and sets closed = true, no new loadClassData() call can proceed past the closed check. This completely eliminates the window where a thread could pass the closed check, get preempted, have close() run, and then resume to create orphaned resources.

2. Concurrent class loading preserved: Unlike the previous synchronized(this) approach which serialized all class loading (including network I/O), the read lock is shared -- multiple loadClassData() calls can proceed concurrently, preserving the FutureTask-based parallel download pattern that the class was designed around.

The volatile closed field is retained for the fast-path early-exit check in close() (line 478) before acquiring the write lock, and for the lock-free isClosed() method. The authoritative closed check happens under the lock.

---

🤖 AI Attribution

Worker AI (Selected Solution)

• Model: opus
• Confidence: 92%
• Approach: Replace the synchronized(this) intrinsic monitor with a ReentrantReadWriteLock to eliminate the use-after-close race condition while preserving concurrent class loading throughput.

The core problem: loadClassData() checks the volatile closed flag but does not hold any lock that coordinates with close(). Between the check and the resource creation in ensureJarCached(), close() can run, set closed = true, clean up all existing resources, and return -- leaving any subsequently-created temp files and JarFile handles orphaned (leaked).

The fix uses a ReentrantReadWriteLock:

• loadClassData() acquires the read lock (shared) -- multiple concurrent class loads can proceed in parallel, preserving the existing FutureTask-based parallel download pattern.
• close() acquires the write lock (exclusive) -- it waits for all in-progress loadClassData() calls to complete before setting closed = true and cleaning up resources. Once the write lock is held, no new loadClassData() call can enter.
• addArtifact() acquires the write lock since it mutates the artifacts list that loadClassData() iterates.
• getArtifacts() and getDescription() acquire the read lock for safe concurrent access.

This approach is strictly superior to the prior synchronized(this) attempt which serialized all class loading (defeating the FutureTask parallelism) and held the monitor during network I/O.

• Rationale: The ReentrantReadWriteLock pattern is the standard Java concurrency solution for this class of use-after-close race conditions. It provides two critical guarantees:

1. No resource creation after close(): The write lock in close() cannot be acquired until all read locks (held by in-progress loadClassData() calls) are released. Once close() holds the write lock and sets closed = true, no new loadClassData() call can proceed past the closed check. This completely eliminates the window where a thread could pass the closed check, get preempted, have close() run, and then resume to create orphaned resources.

2. Concurrent class loading preserved: Unlike the previous synchronized(this) approach which serialized all class loading (including network I/O), the read lock is shared -- multiple loadClassData() calls can proceed concurrently, preserving the FutureTask-based parallel download pattern that the class was designed around.

The volatile closed field is retained for the fast-path early-exit check in close() (line 478) before acquiring the write lock, and for the lock-free isClosed() method. The authoritative closed check happens under the lock.

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 (ReentrantReadWriteLock) is the best choice for several reasons:

1. CORRECTNESS: It completely eliminates the race condition. The read lock in loadClassData() prevents close() from proceeding (close() needs the write lock) until all in-progress loads complete. Once close() acquires the write lock, no new loadClassData() call can proceed past the closed check. This is airtight -- there is no window for resource creation after close() sets closed=true.

2. CONCURRENCY PRESERVATION: This is the critical differentiator from Fix 1. Fix 1 uses synchronized(this) which serializes ALL class loading through the slow path (cache misses), defeating the FutureTask-based parallel download pattern that the class was designed around. Fix 2's read lock is shared, so multiple concurrent loadClassData() calls can proceed in parallel as intended. Network I/O (downloads of different artifacts) runs concurrently, not serialized.

3. COMPREHENSIVE SCOPE: Fix 2 also addresses secondary concurrency issues that Fix 1 and Fix 3 ignore: addArtifact() was previously unsynchronized while loadClassData() iterates the artifacts list (for-each loop), which could cause ConcurrentModificationException. Fix 2 uses the write lock in addArtifact() and the read lock during iteration, providing safe concurrent access. Similarly, getArtifacts() and getDescription() are properly synchronized.

4. Fix 3 has a fundamental design flaw: synchronizing only the flag check but not the resource creation still leaves a race window. After releasing the synchronized block (having confirmed closed==false), close() can interleave before ensureJarCached() creates the temp file. The fix claims to add re-checks inside the FutureTask Callable with synchronized(MavenNexusClassSource.this), but this fragile approach of scattering synchronized blocks at multiple granular points is error-prone -- any future code path that creates resources must remember to add its own synchronized check. The file path typo (sflooss vs sfloess) also suggests less careful review.

5. STANDARD PATTERN: ReentrantReadWriteLock is the canonical Java concurrency solution for shared-reader/exclusive-writer scenarios. It is well-understood, well-documented, and its correctness properties are formally specified in the Java Memory Model. The implementation in the working tree is clean and idiomatic.

6. The current working tree already has Fix 2 applied and the implementation is clean, well-commented, and follows established patterns in the codebase.

• Consensus Score: 82%

Multi-Model Consensus

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

Alternative Proposals (Not Selected)

1. haiku (Confidence: 92%)

   • Approach: Use double-checked locking pattern to synchronize access to the closed flag and resource creation in loadClassData(). The fix wraps the closed flag check and all resource-creation operations with a synchronized(this) lock, preventing any race condition where a thread could create resources after close() has set closed=true.
   • Reason Not Selected: Not selected by opus arbiter

2. sonnet (Confidence: 95%)

   • Approach: Add synchronized blocks to protect the closed flag check in loadClassData() and ensureJarCached(). This prevents the use-after-close race by ensuring that once close() sets the closed flag, no new resource creation (temp files) can begin. The fix uses the existing synchronization pattern (synchronized on this) to ensure atomicity between checking closed and creating resources.
   • Reason Not Selected: Not selected by opus arbiter

---

🤖 Automatically fixed and committed by code-solve workflow