Prevent concurrent session cleanup on different instances in the cluster - #22199

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.oauth.OfflineTokenTest#offlineTokenBrowserFlowIdleTimeExpired

Keycloak CI - Base IT (6)

java.lang.AssertionError: expected:<0> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
        at org.keycloak.testsuite.oauth.OfflineTokenTest.testOfflineSessionExpiration(OfflineTokenTest.java:915)
        at org.keycloak.testsuite.oauth.OfflineTokenTest.offlineTokenBrowserFlowIdleTimeExpired(OfflineTokenTest.java:758)
...

Report flaky test

The unreported flaky was reported previously in #22243, so it is unrelated to this change. I ran it locally multiple times without being able to reproduce it.

@mhajas - can you please review this change when you have the time? Thank you!

Thank you @ahus1, nice catch. This seems like the correct thing to do.

I tried to track why ScheduledTaskRunner was used, and it seems it was changed from ClusterAwareScheduledTaskRunner to ScheduledTaskRunner here: https://github.com/keycloak/keycloak/pull/3511/files#diff-c5ac0bf69a5ce4796ee527c633a6422baa9281e1e74dbcd086029ee73391f779

The PR is quite big and I didn't go through it properly, however, I don't see any reason for this change. It seems irrelevant to the issue the PR is fixing, so I am leaning towards merging this.

Any thoughts @ahus1 @mposolda?

@mhajas - thank you for tracking this down.

I have been looking at the current code, and all activities in ClearExpiredUserSessions don't do anything on the Infinispan parts as they rely only the expiry via Infinispan (they are no-ops).

The only activities happening there are UserSessionPersisterProvider, which eventually end up in JpaUserSessionPersisterProvider.

So given these implementations, it is IMHO safe to proceed, as there is no need to execute it in parallel on all nodes (and it would actually be harmful for the JPA implementation, as stated in the related ticket).

@ahus1 @mhajas It's long ago and I don't recall exactly, but AFAIR when this commit was done https://github.com/keycloak/keycloak/pull/3511/files#diff-cd5a52710a689873698d76128144c7f52bf6baffe2a4855f6f3b59c1e3f92ef0, we did not relied on infinispan expiration, but we had our own expiration task and triggered expiration manually. And I think that with distributable session caches, every node was responsible to cleanup just the sessions owned by him (the session entities where particular node was primary owner). This was later changed to rely on infinispan expiration (AFAIR it probably happened during initial cross-dc work somewhen in 2017).

For the old store, the change looks fine to me.

Thanks for the response @mposolda!