test(redis): scan a cluster node by node again, and ask the server which it is - #2348
Merged
Merged
Conversation
…ich it is 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
giokur
marked this pull request as ready for review
September 23, 2026 14:00
Contributor
🦩 Flamingo Code Review3 finding(s) — 1 action required · 2 recommended · 0 informational Mode: advisory · Rules cited: Inline comments: 3 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-23 14:00 UTC · updated 2026-09-23 14:01 UTC · workflow run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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) <noreply@anthropic.com>
giokur
enabled auto-merge (squash)
September 23, 2026 14:18
mikhail-nosan
approved these changes
Sep 23, 2026
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
2 participants
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.
The password-reset lookup broke on qa, and the same code path is what the production tenant report uses to
rotate its own credentials.
What broke
#2207 replaced a client that never consulted cluster topology with
JedisCluster, which must runCLUSTER SLOTSfirst:new Jedis(node)per seed,SCANeach node directlynew JedisCluster(nodes, config)→ topology discovery → fails from the test podVerify that user can reset passwordpassed on 09-18, 09-21, 09-22 and at 06:09 on 09-23; it failed at11:24 on 09-23, the first run on a build containing #2207.
Why it matters beyond the test
ReportCredentialRotatorbinds its token source toRedis::getResetToken. Rotation runs before anytenant is collected, and its failure path is fatal, so the production tenant report would post nothing
four times a day — and the account is left mid-rotation, a state the rotator's own comment says a re-run
cannot repair. Production runs promoted tags (
values-prod.yaml: 1.0.69), so the exposure arrives withthe next promotion rather than now.
The change
The cluster path scans node by node again.
SCANonly ever reports the keys of the node answering it,so the seeds are walked one at a time, as before #2207. No topology discovery, because nothing here needs
it.
The Memorystore path is untouched — single
JedisPooled, TLS from the published CA, AUTH — exactly as#2207 built it.
Which topology to use is now asked, not configured. SaaS Redis is migrating 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 INFOis answered by both topologies, so a single round tripsettles it and the result is remembered for the JVM. An environment migrates without anyone editing
config;
RedisConfig.setCluster(...)still pins it as an escape hatch, and a probe that cannot connectassumes a cluster — what every environment but dev is today.
Compiles. Worth noting the runner's
TestLibRedisBridgecurrently callssetClusterunconditionally froma defaulted property, so detection only takes over once that is changed to pass through only when the
property is set — a separate saas-shared change. Until then this PR still fixes qa, because the pinned
value there is
trueand the cluster path works again.🤖 Generated with Claude Code