From f0d4012decf8a1daa4d6f7aae8b19f71984cc950 Mon Sep 17 00:00:00 2001 From: YeongJae Min Date: Tue, 2 Jun 2026 10:23:58 +0900 Subject: [PATCH] Clean Redis principal index during expiration cleanup When Redis expiration events are unavailable, the expires key may be gone while the session hash remains available in the grace period. Clean the principal index during the existing expiration cleanup flow when that session is already expired. Addresses gh-1715. Signed-off-by: YeongJae Min --- ...onRepositoryStaticMasterReplicaITests.java | 157 ++++++++++++++++++ .../redis/RedisIndexedSessionRepository.java | 20 ++- .../RedisIndexedSessionRepositoryTests.java | 54 ++++++ 3 files changed, 228 insertions(+), 3 deletions(-) create mode 100644 spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryStaticMasterReplicaITests.java diff --git a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryStaticMasterReplicaITests.java b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryStaticMasterReplicaITests.java new file mode 100644 index 000000000..1e36305f6 --- /dev/null +++ b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryStaticMasterReplicaITests.java @@ -0,0 +1,157 @@ +/* + * Copyright 2014-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.session.data.redis; + +import java.time.Duration; +import java.util.UUID; + +import com.redis.testcontainers.RedisContainer; +import io.lettuce.core.ReadFrom; +import org.junit.jupiter.api.Test; +import org.testcontainers.containers.Container; +import org.testcontainers.containers.Network; + +import org.springframework.data.redis.connection.RedisStaticMasterReplicaConfiguration; +import org.springframework.data.redis.connection.lettuce.LettuceClientConfiguration; +import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory; +import org.springframework.data.redis.core.RedisTemplate; +import org.springframework.data.redis.serializer.RedisSerializer; +import org.springframework.session.FindByIndexNameSessionRepository; +import org.springframework.session.data.redis.RedisIndexedSessionRepository.RedisSession; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.awaitility.Awaitility.await; + +/** + * Integration tests for {@link RedisIndexedSessionRepository} using + * {@link RedisStaticMasterReplicaConfiguration}. + */ +class RedisIndexedSessionRepositoryStaticMasterReplicaITests { + + private static final String INDEX_NAME = FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME; + + @Test // gh-1715 + void cleanUpExpiredSessionsWhenStaticMasterReplicaExpirationEventUnavailableThenRemovesPrincipalIndex() + throws Exception { + Network network = Network.newNetwork(); + RedisContainer master = redisContainer(); + master.withNetwork(network); + master.withNetworkAliases("redis-master"); + RedisContainer replica = redisContainer(); + replica.withNetwork(network); + replica.withCommand("redis-server", "--replicaof", "redis-master", "6379"); + try { + master.start(); + replica.start(); + awaitReplica(replica); + LettuceConnectionFactory connectionFactory = createStaticMasterReplicaConnectionFactory(master, replica); + try { + connectionFactory.afterPropertiesSet(); + RedisTemplate redis = createRedisTemplate(connectionFactory); + RedisIndexedSessionRepository repository = new RedisIndexedSessionRepository(redis); + String namespace = "RedisIndexedSessionRepositoryStaticMasterReplicaITests" + UUID.randomUUID(); + repository.setRedisKeyNamespace(namespace); + repository.setDefaultMaxInactiveInterval(Duration.ofSeconds(1)); + + String principalName = "findByPrincipalNameStaticMasterReplica" + UUID.randomUUID(); + RedisSession session = repository.createSession(); + session.setAttribute(INDEX_NAME, principalName); + + repository.save(session); + + String sessionKey = namespace + ":sessions:" + session.getId(); + String expiresKey = namespace + ":sessions:expires:" + session.getId(); + String principalIndexKey = namespace + ":index:" + INDEX_NAME + ":" + principalName; + awaitReplicaContainsSessionAndIndex(replica, sessionKey, principalIndexKey); + assertThat(repository.findByIndexNameAndIndexValue(INDEX_NAME, principalName)).hasSize(1); + + await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> { + assertThat(redis.hasKey(expiresKey)).isFalse(); + assertThat(repository.findById(session.getId())).isNull(); + }); + + assertThat(redis.boundSetOps(principalIndexKey).members()).contains(session.getId()); + assertThat(repository.findByIndexNameAndIndexValue(INDEX_NAME, principalName)).isEmpty(); + + await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> { + String expirationKey = namespace + ":expirations:" + roundDownMinute(System.currentTimeMillis()); + redis.boundSetOps(expirationKey).add("expires:" + session.getId()); + repository.cleanUpExpiredSessions(); + assertThat(redis.boundSetOps(principalIndexKey).members()).doesNotContain(session.getId()); + }); + } + finally { + connectionFactory.destroy(); + } + } + finally { + replica.stop(); + master.stop(); + network.close(); + } + } + + private static RedisContainer redisContainer() { + return new RedisContainer(RedisContainer.DEFAULT_IMAGE_NAME.withTag(RedisContainer.DEFAULT_TAG)); + } + + private static long roundDownMinute(long timeInMillis) { + long minuteInMillis = Duration.ofMinutes(1).toMillis(); + return timeInMillis - (timeInMillis % minuteInMillis); + } + + private static LettuceConnectionFactory createStaticMasterReplicaConnectionFactory(RedisContainer master, + RedisContainer replica) { + RedisStaticMasterReplicaConfiguration configuration = new RedisStaticMasterReplicaConfiguration( + master.getHost(), master.getFirstMappedPort()); + configuration.node(replica.getHost(), replica.getFirstMappedPort()); + LettuceClientConfiguration clientConfiguration = LettuceClientConfiguration.builder() + .readFrom(ReadFrom.REPLICA_PREFERRED) + .commandTimeout(Duration.ofSeconds(5)) + .build(); + return new LettuceConnectionFactory(configuration, clientConfiguration); + } + + private static RedisTemplate createRedisTemplate(LettuceConnectionFactory connectionFactory) { + RedisTemplate redisTemplate = new RedisTemplate<>(); + redisTemplate.setKeySerializer(RedisSerializer.string()); + redisTemplate.setHashKeySerializer(RedisSerializer.string()); + redisTemplate.setConnectionFactory(connectionFactory); + redisTemplate.afterPropertiesSet(); + return redisTemplate; + } + + private static void awaitReplica(RedisContainer replica) { + await().atMost(Duration.ofSeconds(30)).untilAsserted(() -> { + Container.ExecResult result = replica.execInContainer("redis-cli", "INFO", "replication"); + String output = result.getStdout(); + assertThat(output).contains("master_link_status:up"); + assertThat(output.contains("role:slave") || output.contains("role:replica")).isTrue(); + }); + } + + private static void awaitReplicaContainsSessionAndIndex(RedisContainer replica, String sessionKey, + String principalIndexKey) { + await().atMost(Duration.ofSeconds(30)).untilAsserted(() -> { + Container.ExecResult sessionResult = replica.execInContainer("redis-cli", "EXISTS", sessionKey); + Container.ExecResult indexResult = replica.execInContainer("redis-cli", "SCARD", principalIndexKey); + assertThat(sessionResult.getStdout().trim()).isEqualTo("1"); + assertThat(indexResult.getStdout().trim()).isEqualTo("1"); + }); + } + +} diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java index 891ff88f9..f3d73380c 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java @@ -630,6 +630,15 @@ private void cleanupPrincipalIndex(RedisSession session) { } } + private void cleanupPrincipalIndexForExpiredSession(String sessionId) { + RedisSession session = getSession(sessionId, true); + if (session == null || !session.isExpired()) { + return; + } + cleanupPrincipalIndex(session); + this.expirationStore.remove(session.getId()); + } + private void handleCreated(RedisSession session) { publishEvent(new SessionCreatedEvent(this, session)); } @@ -1066,7 +1075,12 @@ public void cleanupExpiredSessions() { return; } for (Object sessionId : sessionsToExpire) { - touch(getSessionKey((String) sessionId)); + String sessionIdToExpire = (String) sessionId; + boolean exists = touch(getSessionKey(sessionIdToExpire)); + if (!exists && sessionIdToExpire.startsWith(SESSION_EXPIRES_PREFIX)) { + RedisIndexedSessionRepository.this.cleanupPrincipalIndexForExpiredSession( + sessionIdToExpire.substring(SESSION_EXPIRES_PREFIX.length())); + } } } @@ -1076,8 +1090,8 @@ public void cleanupExpiredSessions() { * gh-93 * @param sessionKey the key */ - private void touch(String sessionKey) { - RedisIndexedSessionRepository.this.sessionRedisOperations.hasKey(sessionKey); + private boolean touch(String sessionKey) { + return Boolean.TRUE.equals(RedisIndexedSessionRepository.this.sessionRedisOperations.hasKey(sessionKey)); } String getExpirationKey(long expires) { diff --git a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java index 1ac304e65..3ff2f4568 100644 --- a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java +++ b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java @@ -458,6 +458,60 @@ void cleanupExpiredSessions() { } } + @Test + void cleanupExpiredSessionsWhenExpireKeyMissingAndSessionExpiredThenCleanupPrincipalIndex() { + String expiredId = "expired-id"; + given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations); + given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + expiredId)); + given(this.redisOperations.hasKey(getKey("expires:" + expiredId))).willReturn(false); + given(this.redisOperations.boundHashOps(getKey(expiredId))) + .willReturn(this.boundHashOperations); + Map map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(), + RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 1, RedisSessionMapper.LAST_ACCESSED_TIME_KEY, + Instant.now().minus(5, ChronoUnit.MINUTES).toEpochMilli(), + RedisSessionMapper.ATTRIBUTE_PREFIX + FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME, + "principal"); + given(this.boundHashOperations.entries()).willReturn(map); + + this.redisRepository.cleanUpExpiredSessions(); + + verify(this.boundSetOperations).remove(expiredId); + verify(this.boundSetOperations).remove("expires:" + expiredId); + } + + @Test + void cleanupExpiredSessionsWhenExpireKeyExistsThenDoesNotLookupSession() { + String sessionId = "session-id"; + given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations); + given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + sessionId)); + given(this.redisOperations.hasKey(getKey("expires:" + sessionId))).willReturn(true); + + this.redisRepository.cleanUpExpiredSessions(); + + verify(this.redisOperations, never()).boundHashOps(getKey(sessionId)); + } + + @Test + void cleanupExpiredSessionsWhenExpireKeyMissingAndSessionNotExpiredThenDoesNotCleanupPrincipalIndex() { + String sessionId = "session-id"; + given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations); + given(this.boundSetOperations.members()).willReturn(Collections.singleton("expires:" + sessionId)); + given(this.redisOperations.hasKey(getKey("expires:" + sessionId))).willReturn(false); + given(this.redisOperations.boundHashOps(getKey(sessionId))) + .willReturn(this.boundHashOperations); + Map map = map(RedisSessionMapper.CREATION_TIME_KEY, Instant.EPOCH.toEpochMilli(), + RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, 3600, RedisSessionMapper.LAST_ACCESSED_TIME_KEY, + Instant.now().toEpochMilli(), + RedisSessionMapper.ATTRIBUTE_PREFIX + FindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME, + "principal"); + given(this.boundHashOperations.entries()).willReturn(map); + + this.redisRepository.cleanUpExpiredSessions(); + + verify(this.boundSetOperations, never()).remove(sessionId); + verify(this.boundSetOperations, never()).remove("expires:" + sessionId); + } + @Test void onMessageCreated() { MapSession session = this.cached;