Repository navigation
Conversation
…ion sessions Closes keycloak#55015 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
…remembering empty realms Closes keycloak#55015 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The expiration test can pass even when cleanup fails because the lookup deletes expired entities itself; it must query the entity directly to verify removal after an empty-realm probe.
1 open issue
What changed in this PR
This PR reduces database work during JPA authentication session expiration by checking whether a realm has sessions before querying all 64 buckets.
Changes:
- Adds a one-row realm lookup using the existing index, reducing empty-realm expiration from 64 queries to one.
- Keeps the check stateless, without caching results between runs.
- Adds stateless-suite integration coverage for empty realms and expiration after session creation.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/suites/StatelessTestSuite.java |
Registers the new expiration tests. |
tests/base/src/test/java/org/keycloak/tests/model/AuthenticationSessionExpirationQueryCountTest.java |
Tests query behavior and expiration after an empty-realm check. |
model/jpa/src/main/java/org/keycloak/authentication/jpa/RootAuthenticationSessionEntity.java |
Adds the realm session-existence query. |
model/jpa/src/main/java/org/keycloak/authentication/jpa/AuthenticationSessionExpirationAction.java |
Skips bucket queries when the realm has no sessions. |
Review effort: Balanced
Updated test to assert null on RootAuthenticationSessionEntity retrieval. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
Closes keycloak#55015 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test fails compilation due to a missing JpaConnectionProvider import, and the expiration change adds unnecessary transaction overhead for every due realm.
2 open issues
1 resolved since last review
Review effort: Balanced
|
|
||
| import org.keycloak.authentication.jpa.AuthenticationSessionExpirationAction; | ||
| import org.keycloak.common.Profile; | ||
| import org.keycloak.common.util.Time; |
Comment on lines
+129
to
+130
| return KeycloakModelUtils.runJobInTransactionWithResult(factory, session -> | ||
| session.getProvider(ServerConfigStorageProvider.class).replace(key, lastCheck, Long.toString(currentTime))); |
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.
Closes #55015
The expiration first looks for a single root authentication session of the realm with a one-row lookup, and only runs the per-bucket queries if one exists. The lookup is a prefix seek on
IDX_ROOT_AUTH_SESSION_REALM, so no schema change is needed.Decision: nothing is remembered between runs, which is the same pattern as for user sessions in #54886. A remembered "empty since T" mark would have depended on the clock and on node-local state.
RealmAwareExpirationTasklets whichever node wins the per-realm check run the realm, so such a mark would only help while the same node keeps winning.Trade-off: realms that do have sessions run one extra cheap query per run, 65 instead of 64. The saving is for realms without sessions, which go from 64 queries to 1. Authentication sessions are short-lived, so the benefit depends on how many realms are idle. This is an enhancement, not a regression, as JPA authentication sessions did not exist before 26.8.
Decision: the check runs before the per-realm claim in
RealmAwareExpirationTaskvia a newExpirationAction.hasWork(defaulttrue). A realm without sessions is not claimed, so its last-check row is no longer written every interval. In exchange, every node probes such a realm on each of its own runs, which is one cheap read per node and interval instead of one cluster-wide write. A skipped realm keeps its old last-check time, so the first run that finds sessions cleans it without waiting.Assumption: the probe shares the transaction that reads the last-check row, which commits before any delete transaction starts, so it needs no separate transaction.
The integration tests run in the stateless test suite, as JPA authentication sessions need that feature. They check
hasWorkfor an empty realm and for a realm whose session is created and then expired.ExpirationTaskTestcovers that a realm without work is neither processed nor claimed.