fix(cache): judge stale endpoints from the client's own cluster topology, not an admin command

[cosmin-staicu]

Follow-up to #153, from review feedback on the merged PR.

The bug

The membership check called IServer.ClusterNodesRawAsync(). CLUSTER is an admin command in StackExchange.Redis, so with the default allowAdmin=false the client refuses it before it leaves the process with a RedisCommandException. That type derives from Exception, not RedisServerException, so the "not a cluster" filter never matched. The exception fell through to the outer catch, was tracked, and nothing reconnected. Every scan interval repeated it. The unit tests mocked IServer, so the client-side gate was never on the test path. Verified with a probe against a live server on 3.1.13: both ClusterNodesRawAsync and ExecuteAsync("CLUSTER", "NODES") are refused, since the string command maps back to RedisCommand.CLUSTER.

The fix

• The scan calls IConnectionMultiplexer.ConfigureAsync(), which re-runs the handshake on every connected node. That handshake issues CLUSTER NODES as an internal call (SetInternalCall, written directly to the connection), so the admin gate does not apply. The scan then reads IServer.ClusterConfiguration from a connected node and compares node EndPoints directly. The hand-written CLUSTER NODES parser goes away.
• When no connected node reports a cluster configuration (standalone server, or a Redis user without permission to run CLUSTER NODES), membership can never be judged. The scan emits one Redis.StaleEndpointScanDisabled event, disposes its timer, and stops for the lifetime of the connector. This addresses the second review point: a permanent permission problem no longer produces an unbounded telemetry stream.
• Transient refresh failures (timeouts, connection errors) are still tracked and the scan keeps running.
• If the refresh itself leaves no node connected, the scan does nothing and retries next interval rather than disabling itself.
• ClusterConfiguration has no public constructor, so the read of it sits behind a small internal IClusterTopologyReader seam that tests replace.

Tests

• Unit tests rewritten around the new seam: reconnect on a departed node, node still a member, disable-once on missing configuration, timer stops after disabling, transient failure keeps scanning, no connected node after refresh waits, swapped multiplexer during refresh neither reconnects again nor disables.
• New integration test RefreshClusterMembership_ReachesTheServer_UnderTheDefaultConnectionString runs the real refresh against the CI Redis with the default connection string. It skips itself if the connection string sets allowAdmin. This is the test that would have caught the original bug.
• Full suite passes locally on net8.0 and net10.0. The integration tests were run against a local Redis as well.

Docs

resilience.md, settings.md and the changelog describe the admin-free mechanism and the disable behaviour.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V9jRarnMLeZRZ5rYySRhkh

[Copilot]

🟡 Changes recommended

Four moderate correctness issues remain in topology refresh, identity matching, and scan disabling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Reworks stale Redis endpoint detection to use refreshed client topology without requiring admin commands.

Changes:

• Refreshes cluster membership through ConfigureAsync.
• Disables unsupported scans after one telemetry event.
• Adds unit, integration, and documentation coverage.

Required changes (moderate, 1 vote each):

• Preserve both hostname and endpoint identities when checking membership.
• Treat ConfigureAsync() returning false as inconclusive.
• Make multiplexer identity validation and scan disabling atomic.
• Only judge membership from topology known to have been refreshed.

File summaries

File	Description
tests/UiPath.Caching.Tests/Redis/RedisConnectorStaleEndpointTests.cs	Tests revised stale-scan behavior.
tests/UiPath.Caching.Tests/Redis/RedisConnectorIntegrationTests.cs	Tests refresh under default non-admin configuration.
src/UiPath.Caching/Redis/RedisConnector.cs	Implements topology refresh and scan disabling.
src/UiPath.Caching/Redis/ClusterTopologyReader.cs	Adds cluster-topology membership reading.
docs/reference/settings.md	Documents configuration behavior.
docs/how-to/resilience.md	Explains topology-based stale endpoint detection.
CHANGELOG.md	Records the fix.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 4
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Two moderate topology-reliability issues must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Transient topology refresh failures can incorrectly disable stale-endpoint scanning permanently.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/UiPath.Caching/Redis/RedisConnector.cs:489

• The PR and resilience docs state that a refresh leaving no configured node connected must be inconclusive, but this branch accepts a replaced configuration without rechecking server.IsConnected. If the handshake installs the configuration and that connection then drops before ConfigureAsync completes, the scan can still judge membership and force a reconnect from a node that is no longer connected. Only accept the changed configuration while its server remains connected; otherwise retry next interval.

            else
            {
                return new(Conclusive: true, _topologyReader.GetMembers(after));
            }

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Two moderate topology-handling issues could prevent correct stale-endpoint recovery.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/UiPath.Caching/Redis/RedisConnector.cs:516

• _nullTopologyRefreshes is mutated here before the caller verifies that this multiplexer is still current. If ForceReconnect swaps multiplexers while ConfigureAsync is pending, the discarded old result still increments (or resets) this shared counter, so a replacement can disable scanning before receiving three of its own null-topology results. Scope/reset the streak per multiplexer, or update it only after the identity check under the swap lock.

        return neverConfigured && ++_nullTopologyRefreshes >= NullTopologyRefreshLimit ? new(Conclusive: true, Members: null) : ClusterMembership.Inconclusive;

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The integration test has a critical compilation issue and does not prove that CLUSTER NODES reaches Redis.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Two moderate race-related correctness issues could cause premature reconnection or scan disabling.

Review details

Suppressed comments (2)

src/UiPath.Caching/Redis/RedisConnector.cs:493

• A replaced configuration is accepted without re-checking that its server is still connected. If the configured node installs a topology and then disconnects during the refresh, this returns a conclusive result and may force a reconnect, despite the stated behavior that a refresh leaving no connected node must defer judgment. Gate the changed configuration on server.IsConnected; the existing test only covers the null-configuration branch.

            else
            {
                _nullTopologyRefreshes = 0;
                return new(Conclusive: true, _topologyReader.GetMembers(after));

src/UiPath.Caching/Redis/RedisConnector.cs:504

• This counter is mutated before ScanStaleEndpointsAsync verifies that the inspected multiplexer is still current. In the clusterConfigurationKnown: false swap scenario, the old scan increments the streak after ForceReconnect swaps multiplexers; the later identity check prevents immediate disabling but leaves that increment behind, so the replacement can be permanently disabled after fewer than three of its own null-topology refreshes. Tie the streak to the judged multiplexer or commit counter updates only after validating its identity.

        // A node with no configuration may have lost only the topology reply; a persistent absence means it cannot answer it.
        return neverConfigured && ++_nullTopologyRefreshes >= NullTopologyRefreshLimit ? new(Conclusive: true, Members: null) : ClusterMembership.Inconclusive;

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud