Skip to content

Cancel periodic user storage sync tasks when the realm is removed - #54774

Merged
pedroigor merged 3 commits into
keycloak:mainfrom
Inf1mum:fix/54744-storage-sync-cancel
Nov 9, 2026
Merged

pedroigor merged 3 commits into
keycloak:mainfrom
Inf1mum:fix/54744-storage-sync-cancel

Conversation

@Inf1mum

@Inf1mum Inf1mum commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

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 UserStorageSyncTask timers stayed scheduled and kept failing with ModelIllegalStateException: Realm with id ... not found once per period until restart. This PR implements both fixes suggested in the issue analysis:

1. Systemic fix: realm removal cancels the tasks

  • JpaRealmProvider.removeRealm now calls session.users().preRemove(realm, component) for every UserStorageProvider component of the realm — the same call RealmAdapter.removeComponent makes. This way UserStorageManager.preRemove(RealmModel, ComponentModel) fires StoreSyncEvent(removed=true) for each provider, and the timers are cancelled when the removal transaction commits.
  • UserStorageEventListener previously returned early in its StoreSyncEvent branch (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 (notifyStoreSyncClusterUpdate now takes realmId so it works without a loaded realm).

2. Defensive fix: self-cancel on the next tick

  • UserStorageSyncTask.getStorageModel now calls cancel(session) in the realm == null branch, the same way the component == null branch 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

  • Added UserStorageSyncTaskTest (unit test in model/storage-private) covering both branches: realm == null and component == null now cancel the scheduled task before failing with ModelIllegalStateException. It uses JDK-proxy stubs for the session as the module has no mockito dependency.
  • mvn compile -pl model/jpa,services -am passes.
  • An end-to-end check of the LDAP sync-then-delete-realm flow lives in 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

  • In removeRealm the per-component preRemove calls happen after the realm-wide session.users().preRemove(adapter), so removeImportedUsers(realm, componentId) inside each component preRemove is effectively a no-op (users are already gone); the meaningful effect is the enlisted StoreSyncEvent.
  • The StoreSyncEvent firing stays enlisted after transaction completion (enlistAfterCompletion), so a rolled-back realm removal does not leave cancelled timers behind.

@Inf1mum
Inf1mum requested a review from a team as a code owner November 6, 2026 15:32
Copilot AI balanced review requested due to automatic review settings November 6, 2026 15:32

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.

🟡 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

Copilot AI balanced review requested due to automatic review settings November 7, 2026 22:06

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.

🟢 Approval recommended

No active review issues are identified for the change that cancels user-storage sync tasks when realms are deleted.

1 open issue

Review effort: Balanced

Inf1mum and others added 3 commits November 9, 2026 16:20
… tasks

Covers the realm removal hook for user storage providers from three angles:
both sync timers are cancelled once the realm removal commits, they are
preserved when the removal transaction is rolled back, and the removal event
handling cancels the dangling tasks without failing when the realm is already
gone, e.g. when it was deleted on another cluster node.

Signed-off-by: Inf1mum <vadimlazuk5@gmail.com>
…alms

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
@pedroigor
pedroigor force-pushed the fix/54744-storage-sync-cancel branch from e64e184 to a529a51 Compare November 9, 2026 19:24
Copilot AI balanced review requested due to automatic review settings November 9, 2026 19:24

@pedroigor pedroigor 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.

Thanks, @Inf1mum.

I added a commit that keeps sync-task cleanup in UserStorageManager, covering all user-storage providers while preserving cancellation after commit.

This avoids repeating provider-specific database cleanup after the realm-wide cleanup has already run.

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.

🟢 Approval recommended

No active review issues are reported for the change to cancel user-storage sync tasks when their realm is deleted.

1 open issue

Review effort: Balanced

@pedroigor
pedroigor merged commit f323cd4 into keycloak:main Nov 9, 2026
96 checks passed
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.

Periodic user storage sync tasks are not cancelled when the realm is deleted

3 participants