Repository navigation
Conversation
…ycloak#54744) Signed-off-by: Inf1mum <vadim.lazuk@mail.ru>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Regression tests are missing for realm deletion with both sync timers, including cancellation after commit, preservation on rollback, and missing-realm listener handling.
1 open issue
What changed in this PR
This PR addresses #54744 by cancelling periodic user-storage sync tasks when a realm is deleted, preventing recurring failures for missing realms.
Changes:
- Invoke per-provider removal hooks during realm deletion.
- Cancel tasks and notify cluster nodes even when the realm no longer exists.
- Add defensive self-cancellation and unit tests for missing realms and components.
| File | Description |
|---|---|
model/storage-private/src/test/java/org/keycloak/storage/UserStorageSyncTaskTest.java |
Tests self-cancellation for missing realms and components. |
model/storage-private/src/main/java/org/keycloak/storage/UserStorageSyncTask.java |
Cancels the task when its realm is missing. |
model/storage-private/src/main/java/org/keycloak/storage/UserStorageEventListener.java |
Handles cancellation and cluster notifications after realm deletion. |
model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java |
Calls user-storage component removal hooks when deleting a realm. |
Review effort: Balanced
Comment on lines
+215
to
+216
| adapter.getComponentsStream(id, UserStorageProvider.class.getName()) | ||
| .forEach(component -> session.users().preRemove(adapter, component)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #54744
What was changed
When a realm with a user storage provider (e.g. LDAP) that has periodic full or changed users sync enabled was deleted, its
UserStorageSyncTasktimers stayed scheduled and kept failing withModelIllegalStateException: Realm with id ... not foundonce per period until restart. This PR implements both fixes suggested in the issue analysis:1. Systemic fix: realm removal cancels the tasks
JpaRealmProvider.removeRealmnow callssession.users().preRemove(realm, component)for everyUserStorageProvidercomponent of the realm — the same callRealmAdapter.removeComponentmakes. This wayUserStorageManager.preRemove(RealmModel, ComponentModel)firesStoreSyncEvent(removed=true)for each provider, and the timers are cancelled when the removal transaction commits.UserStorageEventListenerpreviously returned early in itsStoreSyncEventbranch (and in the cluster-event branch) when the realm could not be found — which is exactly the situation right after a realm removal, so the event was swallowed. It now still cancels the tasks for the removed provider in that case and notifies the other cluster nodes (notifyStoreSyncClusterUpdatenow takesrealmIdso it works without a loaded realm).2. Defensive fix: self-cancel on the next tick
UserStorageSyncTask.getStorageModelnow callscancel(session)in therealm == nullbranch, the same way thecomponent == nullbranch already does. Even if the event path is missed (e.g. a node that was down during the notification), a task fires at most once after the realm is deleted instead of once per period.Testing
UserStorageSyncTaskTest(unit test inmodel/storage-private) covering both branches:realm == nullandcomponent == nullnow cancel the scheduled task before failing withModelIllegalStateException. It uses JDK-proxy stubs for the session as the module has no mockito dependency.mvn compile -pl model/jpa,services -ampasses.SyncFederationTest(integration-arquillian, requires a running LDAP server), which could not be run locally; the unit test above covers the task-cancellation logic itself.Notes for reviewers
removeRealmthe per-componentpreRemovecalls happen after the realm-widesession.users().preRemove(adapter), soremoveImportedUsers(realm, componentId)inside each componentpreRemoveis effectively a no-op (users are already gone); the meaningful effect is the enlistedStoreSyncEvent.StoreSyncEventfiring stays enlisted after transaction completion (enlistAfterCompletion), so a rolled-back realm removal does not leave cancelled timers behind.