Fix concurrent group access to prevent NullPointerException

[ShashankFC]

Test 10nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#10

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 40s —— View job

---

Code Review

I've reviewed the changes in this PR that address the NullPointerException when accessing groups concurrently. Here's my detailed analysis:

---

Summary

This PR fixes a race condition in the Keycloak group caching layer (Infinispan) where concurrent reads during group deletion could cause a NullPointerException. The fix adds a null-safety check and includes a concurrency test.

---

Changes Analysis

1. GroupAdapter.java (model/infinispan) - Main Fix ✅

Change: In getSubGroupsCount() method (lines 273-275):

// Before:
return getGroupModel().getSubGroupsCount();

// After:
GroupModel model = modelSupplier.get();
return model == null ? null : model.getSubGroupsCount();

Analysis:

• Good: This properly handles the case where a group has been deleted from the database while still being accessed from cache
• Good: Uses the existing modelSupplier field consistently with other methods in the class (e.g., getSubGroupsStream() at line 274 already follows this pattern)
• Good: Returns null which is the correct contract for getSubGroupsCount() when the group doesn't exist
• Issue: Consider if isUpdated() check could also benefit from the same null-safety pattern

2. CachedGroup.java (model/infinispan) - Minor Addition ✅

Change: Added @Override annotation to getRealm() method (line 61)

Analysis:

• Good: This is a code hygiene improvement that makes the code more maintainable
• Good: Helps catch compile-time errors if the interface signature changes
• Note: This appears to implement the InRealm interface based on the class declaration at line 38

3. GroupUtils.java (services) - Dead Code Removal ✅

Change: Removed unused groupMatchesSearchOrIsPathElement() method (10 lines)

Analysis:

• Good: Removing dead code improves maintainability
• Verified: Method was indeed unused (no references found in the codebase)
• Good: This cleanup is related to the concurrency work as it may have been part of an earlier iteration

4. GroupTest.java (tests) - Concurrency Test ✅

Change: Added new test createMultiDeleteMultiReadMulti() (lines 118-158)

Analysis:

• Good: Excellent concurrency test that reproduces the race condition
• Good: Creates 100 groups, then concurrently reads them while deleting
• Good: Validates that no exceptions are thrown during concurrent operations
• Good: Uses proper thread synchronization with AtomicBoolean and CopyOnWriteArrayList
• Issue: The test doesn't give the read thread enough time to start before deletion begins - consider adding a small delay or countdown latch
• Issue: The thread is not properly joined - the test might complete before the reader thread finishes
• Minor: Empty catch block at line 145 could at least log for debugging

---

Potential Issues & Suggestions

Critical Issues: None ✅

High Priority Issues:

1. Test Thread Synchronization (GroupTest.java:118-158)

   • The spawned reader thread may not complete before the test ends
   • Suggestion: Store the thread reference and call thread.join() after setting deletedAll = true

   Fix this →

2. Test Race Condition Window (GroupTest.java:139)

   • Reader thread might not start before all groups are deleted
   • Suggestion: Add a CountDownLatch to ensure the reader thread is actively running before deletion begins

   Fix this →

Medium Priority:

3. Silent Exception Handling (GroupTest.java:145)

   • Empty catch block makes debugging harder if unexpected exceptions occur
   • Suggestion: At minimum, add a comment explaining why exceptions are ignored, or use a logger

4. Consistency Check (GroupAdapter.java:70-76)

   • The isUpdated() method also uses cacheSession.getGroupDelegate().getGroupById() which could return null
   • Consider: Should line 74 also handle null with a null-check?

Low Priority:

5. Test Documentation (GroupTest.java:118)
   • No Javadoc or comment explaining what concurrency issue this test prevents
   • Suggestion: Add a comment referencing the issue/ticket number this fixes

---

Architecture & Thread Safety

The fix properly addresses the concurrency issue in Keycloak's two-tier caching architecture:

1. Infinispan Cache (first tier) - Holds CachedGroup objects
2. Database (second tier) - The source of truth accessed via modelSupplier

Race Condition:

• Thread A: Deletes group from DB → invalidates cache
• Thread B: Has cached reference → tries to load from DB → gets null → NPE ❌

The Fix:

• Properly handles null return from modelSupplier.get() ✅

This follows the same pattern already used in other methods like getSubGroupsStream() (line 256-257).

---

Test Coverage

✅ Good: New test specifically targets the concurrency issue
✅ Good: Tests the actual scenario (concurrent read during delete)
⚠️ Improvement Needed: Test thread synchronization needs strengthening

---

Verdict

Overall: Approve with Minor Suggestions ✅

The core fix is solid and addresses the NullPointerException correctly. The main concerns are:

1. Test thread synchronization should be improved to ensure reliability
2. Empty catch block should be documented or improved

The fix is safe to merge after addressing the test synchronization issue, as it's a defensive null-check that aligns with existing patterns in the codebase.

---