+
Skip to content

UserSession Offline removed from DB if not in cache #40831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 7, 2025

Conversation

pruivo
Copy link
Contributor

@pruivo pruivo commented Jul 1, 2025

Fixes #40754

The method org.keycloak.models.sessions.infinispan.InfinispanUserSessionProvider#importUserSession(org.keycloak.models.RealmModel, org.keycloak.models.UserSessionModel) was rewritten. The main differences are that I'm using putIfAbsent to import the session, and I'm importing all the client sessions in parallel.

I also took the opportunity to fix the code warnings (unused code, parameters with constant values, missing types, etc).

@pruivo pruivo requested a review from ahus1 July 1, 2025 12:55
@ahus1
Copy link
Contributor

ahus1 commented Jul 1, 2025

Thank you for the PR. I assume you want to enable users to have a size-limited offline user session and client session cache.

If you want to go down that route, there are IMHO some additional changes necessary:

  • We recently added safeguards that disallow setting a cache limit. If we now support a cache limit, having a cache limit of 10_000 for the offline cache would be a sensible new default, and we should allow that CLI parameter then.
  • When using a size-limited cache, we found in our testing that this should be used with a single owner, as the eviction could lead to a situation where the entry is present at the backup owner, while the primary owner already forgot about it, and then the backup owner will read it from the local cache still. That should be part of the safeguard/automatic configuration
  • As an unbounded cache for the offline sessions is a bad idea, we would only support size-limited offline caches, so no need to keep the old behavior.

Note that when we upgrade the non-persistent user sessions like this, we would need to maintain this code side-by-side with the persistent user sessions. Some of the logic (for example batching the writes to the DB) are quite different in the two implementations, and will require understanding both in the team.

@pruivo
Copy link
Contributor Author

pruivo commented Jul 1, 2025

I assume you want to enable users to have a size-limited offline user session and client session cache.

What I want is InfinispanUserSessionProvider to be an "infinispan only " provider with no database access, even for offline sessions... but I will piss off a lot of people if I do that 🤣

Yes, I want the users to have options: small deployments to use unbounded caches and large deployment with bounded caches.

If you want to go down that route, there are IMHO some additional changes necessary:

Do you want me to do them in this PR or another one?

When using a size-limited cache, we found in our testing that this should be used with a single owner, as the eviction could lead to a situation where the entry is present at the backup owner, while the primary owner already forgot about it, and then the backup owner will read it from the local cache still. That should be part of the safeguard/automatic configuration

Read-only is fine; the problem is when updates are involved. Because we use computeIfPresent to update the cache, some owners may miss the update.

Some of the logic (for example batching the writes to the DB) are quite different in the two implementations, and will require understanding both in the team.

Honestly, the only database accesses I see on InfinispanUserSessionProvider are for create, import, and remove. If you look at the user and client session adapters, the setter methods never interact with the database (except for org.keycloak.models.sessions.infinispan.UserSessionAdapter#setLastSessionRefresh) 🤷
For example:

public void setAction(String action) {
ClientSessionUpdateTask task = new ClientSessionUpdateTask() {
@Override
public void runUpdate(AuthenticatedClientSessionEntity entity) {
entity.setAction(action);
}
@Override
public boolean isOffline() {
return offline;
}
};
update(task);
}

@ahus1
Copy link
Contributor

ahus1 commented Jul 1, 2025

Do you want me to do them in this PR or another one?

Due to the latest hardcoded safeguards here, I think we need to do additional changes, otherwise users won't be able to use it with a size-limited cache, as we would simply ignore those settings:

var builder = holder.getNamedConfigurationBuilders().get(name);
if (builder == null) {
throw cacheNotFound(name);
}
if (builder.memory().maxCount() != -1) {
logger.infof("Persistent user sessions disabled and memory limit is set. Ignoring cache limits to avoid losing sessions for cache %s.", name);
builder.memory().maxCount(-1);
}
if (builder.clustering().hash().attributes().attribute(HashConfiguration.NUM_OWNERS).get() == 1 &&
builder.persistence().stores().stream().noneMatch(p -> p.attributes().attribute(AbstractStoreConfiguration.SHARED).get())
) {
logger.infof("Persistent user sessions disabled with number of owners set to default value 1 for cache %s and no shared persistence store configured. Setting num_owners=2 to avoid data loss.", name);
builder.clustering().hash().numOwners(2);
}

Please schedule a 1:1 if you want to discuss it in more details, that might be quicker.

@pruivo pruivo marked this pull request as ready for review July 2, 2025 07:40
@pruivo pruivo requested a review from a team as a code owner July 2, 2025 07:40
@pruivo
Copy link
Contributor Author

pruivo commented Jul 2, 2025

@ahus1 I updated/removed the safeguards.

@ahus1 ahus1 requested a review from a team as a code owner July 2, 2025 18:59
@ahus1
Copy link
Contributor

ahus1 commented Jul 2, 2025

I had a look, updated some docs and pushed it in a more opinionated and IMHO safe-to-use direction. Please let me know your thoughts. Happy to jump on a call during the next days.

@ahus1 ahus1 self-assigned this Jul 2, 2025
@pruivo
Copy link
Contributor Author

pruivo commented Jul 2, 2025

@ahus1 I'm ok with the changes.

pruivo and others added 2 commits July 7, 2025 18:42
Fixes keycloak#40754

Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

I gave it another test locally and approve it. Thank you for the PR.

@ahus1 ahus1 merged commit 9322d71 into keycloak:main Jul 7, 2025
76 checks passed
@ahus1
Copy link
Contributor

ahus1 commented Jul 7, 2025

@pruivo - it would be nice if you could add the enhancement of parallel loading of client sessions also to PersistentUserSessionProvider#importSessionsWithExpiration. Thanks!

@pruivo
Copy link
Contributor Author

pruivo commented Jul 10, 2025

@pruivo - it would be nice if you could add the enhancement of parallel loading of client sessions also to PersistentUserSessionProvider#importSessionsWithExpiration. Thanks!

Issue to track this idea: #41074

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

Successfully merging this pull request may close these issues.

UserSession Offline removed from DB if not in cache
2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载