From 764be98024d51fa2ff971385baa9631017e06456 Mon Sep 17 00:00:00 2001 From: giokur Date: Wed, 23 Sep 2026 14:51:41 +0200 Subject: [PATCH 1/3] test(redis): scan a cluster node by node again, and ask the server which it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The password-reset lookup stopped working on qa: Reading the password-reset token from Redis failed JedisClusterOperationException: Could not initialize cluster slots cache #2207 replaced a client that never needed cluster topology with JedisCluster, which must run CLUSTER SLOTS before anything else. The advertised node addresses are not reachable from the test pod, so discovery fails and the lookup returns null — the all-tests run of 2026-09-23 11:24 failed "Verify that user can reset password" on it, after five clean runs. This is not only a test: ReportCredentialRotator binds its token source to Redis::getResetToken, and rotation runs before any tenant is collected, so the production tenant report would post nothing four times a day and leave the account half-rotated — a state its own comment says a re-run cannot repair. Prod is on promoted tags today, so the exposure arrives with the next promotion. SCAN is per-node in a cluster, so the seeds are walked one at a time again, as they were before #2207. The single-client path stays exactly as #2207 built it for Memorystore, along with its TLS and AUTH. Which topology to use is now asked rather than configured. SaaS Redis is moving to Memorystore for Valkey — one node, cluster mode disabled — one environment at a time, so the answer differs per environment and changes as the migration proceeds. CLUSTER INFO is answered by both, so one round trip settles it, the result is remembered for the JVM, and an environment migrates without anyone editing config. RedisConfig.setCluster still pins it where someone wants to, and a probe that cannot connect assumes a cluster, which is what every environment but dev is today. Co-Authored-By: Claude Opus 5 (1M context) --- .../openframe/test/config/RedisConfig.java | 18 ++- .../com/openframe/test/data/redis/Redis.java | 108 ++++++++++++++---- 2 files changed, 99 insertions(+), 27 deletions(-) diff --git a/openframe-test-service-core/src/main/java/com/openframe/test/config/RedisConfig.java b/openframe-test-service-core/src/main/java/com/openframe/test/config/RedisConfig.java index c5d3137a8b..1de5a36af8 100644 --- a/openframe-test-service-core/src/main/java/com/openframe/test/config/RedisConfig.java +++ b/openframe-test-service-core/src/main/java/com/openframe/test/config/RedisConfig.java @@ -69,20 +69,28 @@ public static String getPassword() { } /** - * Whether the server runs in cluster mode. True for the in-cluster shard set every environment - * but dev still uses; dev points at a single Memorystore instance, where a cluster client fails - * on topology discovery because cluster mode is disabled server-side. + * Pins cluster mode instead of letting {@link com.openframe.test.data.redis.Redis} ask the server. + * An override for the case where the probe cannot be trusted; leaving it unset is the normal path. */ public static void setCluster(boolean enabled) { cluster = enabled; } - public static boolean isCluster() { + /** + * The pinned answer, or {@code null} when nobody pinned one and the server should be asked. + * + *

SaaS Redis is migrating to Memorystore for Valkey — a single node with cluster mode disabled — + * one environment at a time, so the answer differs per environment and changes under us as the + * migration proceeds. Detecting it costs one {@code CLUSTER INFO}, which both topologies answer, so + * an environment moves without anyone editing config, and this override exists only as an escape + * hatch. + */ + public static Boolean getConfiguredCluster() { if (cluster != null) { return cluster; } String env = System.getenv("REDIS_CLUSTER"); - return (env == null || env.trim().isEmpty()) || Boolean.parseBoolean(env); + return (env == null || env.trim().isEmpty()) ? null : Boolean.parseBoolean(env); } public static Set getClusterNodes() { diff --git a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java index 8b5ba15273..02c2821290 100644 --- a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java +++ b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java @@ -3,8 +3,9 @@ import com.openframe.test.config.RedisConfig; import lombok.extern.slf4j.Slf4j; import redis.clients.jedis.DefaultJedisClientConfig; +import redis.clients.jedis.HostAndPort; +import redis.clients.jedis.Jedis; import redis.clients.jedis.JedisClientConfig; -import redis.clients.jedis.JedisCluster; import redis.clients.jedis.JedisPooled; import redis.clients.jedis.UnifiedJedis; import redis.clients.jedis.params.ScanParams; @@ -26,6 +27,10 @@ @Slf4j public class Redis { + /** What the server answered to CLUSTER INFO, remembered for the JVM. Null until first asked. */ + private static volatile Boolean detectedCluster; + + /** * Find the password-reset token for {@code email}. The auth-server stores it under the tenant-scoped, * hash-tagged key {@code of:{}:pwdreset:} with the email as the value. @@ -40,38 +45,97 @@ public class Redis { */ public static String getResetToken(String email) { String pattern = "of:{" + RedisConfig.getTenant() + "}:pwdreset:*"; - try (UnifiedJedis client = client()) { - ScanParams scanParams = new ScanParams().match(pattern).count(100); - String cursor = ScanParams.SCAN_POINTER_START; - do { - ScanResult scanResult = client.scan(cursor, scanParams); - List keys = scanResult.getResult(); - if (!keys.isEmpty()) { - // One slot for the whole batch, so this is a single round trip rather than a GET per key. - List emails = client.mget(keys.toArray(new String[0])); - for (int i = 0; i < keys.size(); i++) { - if (email.equals(emails.get(i))) { - return keys.get(i).split(":pwdreset:")[1]; + try { + JedisClientConfig config = clientConfig(); + if (clusterMode(config)) { + // SCAN is per-node: a cluster only ever reports the keys of the node answering it, so the + // seeds are walked one by one. This deliberately does not use JedisCluster — that needs + // CLUSTER SLOTS to succeed first, and the advertised addresses are not reachable from the + // test pod, which is how this lookup broke on qa (JedisClusterOperationException: could + // not initialize cluster slots cache) and took the password-reset case with it. + for (HostAndPort node : RedisConfig.getClusterNodes()) { + try (UnifiedJedis client = new JedisPooled(node, config)) { + String token = findToken(client, pattern, email); + if (token != null) { + return token; } + } catch (Exception e) { + // Node unreachable or holding none of the slots — try the next seed. + log.debug("Seed {} did not answer the password-reset scan: {}", node, e.toString()); } } - cursor = scanResult.getCursor(); - } while (!cursor.equals(ScanParams.SCAN_POINTER_START)); + return null; + } + try (UnifiedJedis client = new JedisPooled(RedisConfig.getNode(), config)) { + return findToken(client, pattern, email); + } } catch (Exception e) { log.warn("Reading the password-reset token from Redis failed", e); + return null; } + } + + /** Walks one server's keyspace for the tenant's reset keys and returns the token whose value is the email. */ + private static String findToken(UnifiedJedis client, String pattern, String email) { + ScanParams scanParams = new ScanParams().match(pattern).count(100); + String cursor = ScanParams.SCAN_POINTER_START; + do { + ScanResult scanResult = client.scan(cursor, scanParams); + List keys = scanResult.getResult(); + if (!keys.isEmpty()) { + // The keys share the tenant hash tag, so one slot for the whole batch: a single round + // trip rather than a GET per key. + List emails = client.mget(keys.toArray(new String[0])); + for (int i = 0; i < keys.size(); i++) { + if (email.equals(emails.get(i))) { + return keys.get(i).split(":pwdreset:")[1]; + } + } + } + cursor = scanResult.getCursor(); + } while (!cursor.equals(ScanParams.SCAN_POINTER_START)); return null; } /** - * The client is built per call rather than cached: a caller polls at most a few dozen times, and a - * cached static client would trade those handshakes for a topology-staleness problem. + * Whether the server runs in cluster mode, asked once and remembered. + * + *

SaaS Redis is moving to Memorystore for Valkey — one node, cluster mode disabled, TLS and AUTH — + * one environment at a time, so this differs per environment and changes as the migration proceeds. + * {@code CLUSTER INFO} is answered by both topologies, so one round trip settles it and an + * environment migrates without anyone editing config. {@link RedisConfig#getConfiguredCluster()} + * still wins where someone pinned an answer. + * + *

A probe that cannot connect assumes a cluster: that is what every environment but dev is today, + * and it keeps the behaviour unchanged where the probe itself is the thing that is broken. */ - private static UnifiedJedis client() throws GeneralSecurityException, IOException { - JedisClientConfig config = clientConfig(); - return RedisConfig.isCluster() - ? new JedisCluster(RedisConfig.getClusterNodes(), config) - : new JedisPooled(RedisConfig.getNode(), config); + private static boolean clusterMode(JedisClientConfig config) { + Boolean pinned = RedisConfig.getConfiguredCluster(); + if (pinned != null) { + return pinned; + } + Boolean known = detectedCluster; + if (known != null) { + return known; + } + synchronized (Redis.class) { + if (detectedCluster == null) { + detectedCluster = probeCluster(config); + } + return detectedCluster; + } + } + + private static boolean probeCluster(JedisClientConfig config) { + HostAndPort node = RedisConfig.getNode(); + try (Jedis jedis = new Jedis(node, config)) { + boolean enabled = jedis.clusterInfo().contains("cluster_enabled:1"); + log.info("Redis at {} reports cluster mode {}", node, enabled ? "enabled" : "disabled"); + return enabled; + } catch (Exception e) { + log.warn("Could not read CLUSTER INFO from {}; assuming a cluster", node, e); + return true; + } } /** From 99b0cd323d46b2f0c91ed092289a4f30e700a6fb Mon Sep 17 00:00:00 2001 From: giokur Date: Wed, 23 Sep 2026 15:23:08 +0200 Subject: [PATCH 2/3] test(redis): say so when no seed could be scanned at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every seed throwing and every seed answering with nothing both produce null, and the caller polls on null until its timeout. Only one of those is worth waking someone for: ReportCredentialRotator has already requested the reset by then, so a token that exists but is unreachable leaves the tenant-report account half-rotated — the state its own comment says a re-run cannot repair. Counts the seeds that were actually scanned and warns when that is none. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/com/openframe/test/data/redis/Redis.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java index 02c2821290..9fa517c237 100644 --- a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java +++ b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java @@ -23,6 +23,7 @@ import java.security.cert.CertificateFactory; import java.util.Collection; import java.util.List; +import java.util.Set; @Slf4j public class Redis { @@ -53,9 +54,12 @@ public static String getResetToken(String email) { // CLUSTER SLOTS to succeed first, and the advertised addresses are not reachable from the // test pod, which is how this lookup broke on qa (JedisClusterOperationException: could // not initialize cluster slots cache) and took the password-reset case with it. - for (HostAndPort node : RedisConfig.getClusterNodes()) { + Set seeds = RedisConfig.getClusterNodes(); + int scanned = 0; + for (HostAndPort node : seeds) { try (UnifiedJedis client = new JedisPooled(node, config)) { String token = findToken(client, pattern, email); + scanned++; if (token != null) { return token; } @@ -64,6 +68,14 @@ public static String getResetToken(String email) { log.debug("Seed {} did not answer the password-reset scan: {}", node, e.toString()); } } + if (scanned == 0) { + // "Scanned every node and the token is not there yet" and "could not scan anything" are + // the same null to the caller, and the caller polls on it until a timeout. Only one of + // those is worth waking someone for: a reset that never lands leaves the tenant-report + // account half-rotated, which a re-run cannot repair. + log.warn("None of the {} Redis seeds could be scanned for the password-reset token; " + + "the token may exist and be unreachable rather than absent", seeds.size()); + } return null; } try (UnifiedJedis client = new JedisPooled(RedisConfig.getNode(), config)) { From 1eb00639b14b7627f746725b6212671f326508ef Mon Sep 17 00:00:00 2001 From: giokur Date: Wed, 23 Sep 2026 16:04:14 +0200 Subject: [PATCH 3/3] test(redis): remember only an answer the server gave Review on #2348: a probe that fails caches its guess for the life of the JVM. The guess itself stays - assuming a cluster is right for every environment but dev, and it keeps behaviour unchanged when the probe is the broken part - but it is no longer remembered. probeCluster returns null for "the server did not say", clusterMode answers true for that one lookup, and the next lookup asks again. This pod runs for days between rollouts, and one dropped SYN on the egress NAT is enough to lose a probe; pinning a guess on that for days is the failure the detection was meant to remove, not add. Also drops the auto-unboxing return of the field inside the synchronized block in favour of a local, now that probeCluster really can return null. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/openframe/test/data/redis/Redis.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java index 9fa517c237..ee8f8805ae 100644 --- a/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java +++ b/openframe-test-service-core/src/main/java/com/openframe/test/data/redis/Redis.java @@ -118,8 +118,10 @@ private static String findToken(UnifiedJedis client, String pattern, String emai * environment migrates without anyone editing config. {@link RedisConfig#getConfiguredCluster()} * still wins where someone pinned an answer. * - *

A probe that cannot connect assumes a cluster: that is what every environment but dev is today, - * and it keeps the behaviour unchanged where the probe itself is the thing that is broken. + *

A probe that cannot connect assumes a cluster for that one lookup - that is what every + * environment but dev is today, so it keeps the behaviour unchanged where the probe itself is the + * thing that is broken. Only an answer the server actually gave is remembered: this pod lives for + * days, and a probe lost to one dropped SYN must not pin a guess for all of them. */ private static boolean clusterMode(JedisClientConfig config) { Boolean pinned = RedisConfig.getConfiguredCluster(); @@ -131,22 +133,29 @@ private static boolean clusterMode(JedisClientConfig config) { return known; } synchronized (Redis.class) { - if (detectedCluster == null) { - detectedCluster = probeCluster(config); + Boolean answered = detectedCluster; + if (answered != null) { + return answered; } - return detectedCluster; + Boolean probed = probeCluster(config); + if (probed == null) { + return true; + } + detectedCluster = probed; + return probed; } } - private static boolean probeCluster(JedisClientConfig config) { + /** What the server says about itself, or {@code null} when it did not answer. */ + private static Boolean probeCluster(JedisClientConfig config) { HostAndPort node = RedisConfig.getNode(); try (Jedis jedis = new Jedis(node, config)) { boolean enabled = jedis.clusterInfo().contains("cluster_enabled:1"); log.info("Redis at {} reports cluster mode {}", node, enabled ? "enabled" : "disabled"); return enabled; } catch (Exception e) { - log.warn("Could not read CLUSTER INFO from {}; assuming a cluster", node, e); - return true; + log.warn("Could not read CLUSTER INFO from {}; assuming a cluster for this lookup", node, e); + return null; } }