Keycloak / PR #10

Fix concurrent group access to prevent NullPointerException

keycloak/keycloak#40940
Golden recall: 100% Extra findings: 4

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 getSubGroupsCount() returns null — violates the GroupModel contract ("Never returns null") and can cause NPEs on unboxing HIGH ✓ Found FriendlyReviewer: "Returning null from getSubGroupsCount() violates the GroupModel interface contract... can cause NPEs on unboxing" (GroupAdapter.java L275)
#2 The reader thread is not waited on; deletedAll is toggled to true and assertion is immediate — flaky test MEDIUM ✓ Found FriendlyReviewer: "Background thread is never joined before assertion, making test flaky. Add thread.join(timeout) before checking exceptions" (GroupTest.java L139)

Supplementary Findings (5 findings)

Finding File Severity Legitimate?
getDelegateForUpdate() uses modelSupplier.get() which may return a stale cached model if already resolved by a prior read — write-skew on all write operations GroupAdapter.java:60 HIGH ✓ Real data consistency bug in a PR meant to fix concurrency
LazyModel not thread-safe — no synchronization on the cached model field, visibility issues under concurrent access GroupAdapter.java:54 MEDIUM ✓ Genuine thread-safety gap
isUpdated() has a TOCTOU race — after checking invalidated flag, another thread may set it, returning stale data GroupAdapter.java:72 MEDIUM ✓ Real race condition in concurrency fix PR
Missing cleanup registration for created groups — 100 groups leak if the test fails before deletion GroupTest.java:118 MEDIUM ✓ Test pollution
Typo in variable name: groupUuuids should be groupUuids GroupTest.java:118 LOW ✓ Minor

Honest Analysis

FriendlyReviewer detected both golden bugs in this PR. The getSubGroupsCount() returning null (#1) was flagged with the correct severity and an accurate description of the contract violation. The flaky test (#2) was also correctly identified with a concrete fix suggestion (thread.join()).

The most significant finding FriendlyReviewer contributed beyond the golden set is the write-skew bug in getDelegateForUpdate(). When modelSupplier.get() returns a stale cached model, all subsequent write operations in that session operate on stale data. This is a real data consistency issue — and notably, it exists in a PR whose stated purpose is precisely to fix concurrent group access.

The additional findings on LazyModel thread-safety and the TOCTOU race in isUpdated() are genuine concurrency concerns that reinforce the same theme: the PR addresses one surface-level bug (the NPE) but leaves deeper structural issues untouched. FriendlyReviewer surfaced all of them.

One finding is minor — the groupUuuids typo — but it costs nothing to include. The cleanup registration gap for test groups (possible test pollution) is a legitimate medium-severity concern.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
2 high, 4 medium, 1 low

The code review identified several critical and medium-severity issues. The most significant is a data consistency bug in GroupAdapter.java where getDelegateForUpdate() can use a stale cached delegate model, leading to write-skew. Another critical issue is that getSubGroupsCount() returns null, violating the GroupModel interface contract and risking NPEs. Concurrency concerns include non-thread-safe LazyModel caching and a TOCTOU race in isUpdated(). The new concurrency test in GroupTest.java has multiple problems: missing cleanup registration, a typo, and a lack of thread joining, making it flaky. The GroupUtils.java change is a safe cleanup of unused code.

These might need a close look

  • 🔴 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (L60)
    CRITICAL: getDelegateForUpdate() uses modelSupplier.get() which may return a stale cached model if already resolved by a prior read. This causes all subsequent write operations to operate on stale data. Replace with getGroupModel() to always fetch fresh from the database.
  • 🔴 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (L275)
    CRITICAL: Returning null from getSubGroupsCount() violates the GroupModel interface contract 'Never returns {@code null}'. This can cause NPEs on unboxing. Either throw IllegalStateException or return 0L when model is null.

Worth checking

  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (L54)
    MEDIUM: LazyModel is not thread-safe; it lacks synchronization on the cached model field. Since GroupAdapter is accessed concurrently, this can lead to visibility issues. Consider adding volatile and DCL or eliminating the caching layer.
  • 🟡 model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/GroupAdapter.java (L72)
    MEDIUM: isUpdated() has a TOCTOU race: after checking invalidated flag, another thread may set it, causing stale data to be returned. Check invalidated again after fetching from DB or synchronize the read path.
  • 🟡 tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (L118)
    Missing cleanup registration for created groups. If test fails before deletion, 100 groups leak. Follow the pattern in createGroup() to register cleanup immediately.
  • 🟡 tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (L139)
    Background thread is never joined before assertion, making test flaky. Add thread.join(timeout) before checking exceptions. Also consider thread cleanup in a finally block.

Small things (take or leave)

  • 🔵 tests/base/src/test/java/org/keycloak/tests/admin/group/GroupTest.java (L118)
    Typo in variable name: groupUuuids should be groupUuids.