Skip to content

Avoid caching user data while external storage is unavailable - #51322

Open
hkwi wants to merge 5 commits into
keycloak:mainfrom
hkwi:fix-51321-unavailable-lazy-user-cache
Open

Avoid caching user data while external storage is unavailable#51322
hkwi wants to merge 5 commits into
keycloak:mainfrom
hkwi:fix-51321-unavailable-lazy-user-cache

Conversation

@hkwi

@hkwi hkwi commented Jul 31, 2026

Copy link
Copy Markdown

Summary

  • allow lazy cache loaders to return a value without committing it to the cache
  • keep cached-user lazy data uninitialized when user storage is temporarily unavailable
  • retry external group loading after the provider recovers

This prevents an empty group result observed during an LDAP or other user-storage outage from remaining cached until explicit user-cache invalidation.

Closes #51321

Testing

  • ./mvnw -pl model/infinispan -am -DskipTests -DskipTestsuite -DskipExamples -DskipProtoLockCheck install
  • ./mvnw -pl model/infinispan -Dtest=DefaultLazyLoaderTest test
  • ./mvnw -pl testsuite/integration-arquillian/tests/base -am -DskipTests -DskipExamples -DskipProtoLockCheck install
  • ./mvnw -f testsuite/integration-arquillian/pom.xml -Dtest=UserStorageFailureTest#testUnavailableStorageDoesNotCacheEmptyGroups install
  • ./mvnw spotless:check

AI assistance

I used OpenAI Codex to assist with investigation, implementation, tests, and pull request preparation. I reviewed and understand the submitted changes.

Copilot AI balanced review requested due to automatic review settings July 31, 2026 03:31
@hkwi
hkwi requested review from a team as code owners July 31, 2026 03:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Keep lazy cached user data uninitialized when the user storage provider is temporarily unavailable so later requests retry the provider.

Closes keycloak#51321

Signed-off-by: Hiroaki KAWAI <hiroaki.kawai@gmail.com>
Copilot AI review requested due to automatic review settings August 3, 2026 08:42
@hkwi
hkwi force-pushed the fix-51321-unavailable-lazy-user-cache branch from 31832fb to dfed839 Compare August 3, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageFailureTest.java:436

  • groupName and the realm group are created before the try, so a failure in either setup transaction or the cache assertion leaves this static provider state and group behind, potentially cascading into later tests. Start the try before setup so the existing cleanup always runs.
            FailableHardcodedStorageProvider.groupName = groupName;

Copilot AI review requested due to automatic review settings August 4, 2026 03:10
Signed-off-by: Hiroaki KAWAI <hiroaki.kawai@gmail.com>
@hkwi
hkwi force-pushed the fix-51321-unavailable-lazy-user-cache branch from a0e7f20 to dc732a0 Compare August 4, 2026 03:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 4, 2026 03:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/DefaultLazyLoader.java:45

  • The new non-cacheable path contradicts the existing DefaultLazyLoader/LazyLoader contract that source data is fetched only once: a rejected source is fetched again on every subsequent get. Please document the retry behavior and the predicate's semantics in both API Javadocs so callers do not rely on the old guarantee.
    public DefaultLazyLoader(Function<S, D> loader, Supplier<D> fallback, Predicate<S> cacheable) {

Signed-off-by: Hiroaki KAWAI <hiroaki.kawai@gmail.com>
Copilot AI review requested due to automatic review settings August 4, 2026 04:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 5, 2026 00:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedUser.java:84

  • UserStorageManager.validateUser can wrap a StorageUnavailableUserModelDelegate in another ReadOnlyUserModelDelegate for a managed member of a disabled organization (UserStorageManager.java:126-130). This direct check then treats the source as available and permanently caches its local fallback data; inspect the delegate chain so the unavailable marker survives wrappers.
        return !(userModel instanceof StorageUnavailableUserModelDelegate);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LDAP outage can cache empty groups until user-cache invalidation

2 participants