From 87c7f738dc1def89dd09bb4f0ed47dca8d932316 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 6 Aug 2026 16:18:29 +0100 Subject: [PATCH 1/4] Tests: probe for absent servers instead of discovering them by timeout Cluster tests set connectTimeout=10000 and only skip once a connection attempt has failed, so with no cluster running each of ~50 tests burned 10 seconds before reporting a 1ms skip. Sentinel was the same shape via SentinelBase.InitializeAsync, which polls for 15 seconds per test and then reports a failure rather than a skip. Add Skip.IfNoServer, backed by a single short TCP connect per endpoint cached for the run, and route the six cluster configuration overrides through TestBase.GetClusterConfiguration so the probe happens before a configuration is handed out. This only reports whether anything is listening: a server that is up but unreachable still fails, as it must. Also, while in SentinelBase: wait on the primary connection rather than only the sentinel connection (the retry loop already used the former as its success condition, but the assert checked the latter, so it could pass having never reached the primary), and give the assert a message. Full suite against a primary/replica/secure/failover-only topology goes from 10m34s with 27 failures to 49s with none attributable to the missing servers; against the full topology the skip count is unchanged at 150, confirming nothing is newly suppressed. CI: the Windows job now starts that smaller topology by default, since the cluster and sentinel instances gossip continuously and that job is a fractional vCPU running Windows running WSL running redis. Dispatch with windows-topology=full for everything. Also allows pinning the apt redis version instead of silently testing whatever is GA, and polls for readiness rather than sleeping a fixed 5 seconds. --- .github/workflows/CI.yml | 119 ++++++++++-------- .../ClusterShardedTests.cs | 2 +- .../StackExchange.Redis.Tests/ClusterTests.cs | 2 +- .../ConnectCustomConfigTests.cs | 3 + .../GetServerTests.cs | 2 +- .../StackExchange.Redis.Tests/Helpers/Skip.cs | 87 ++++++++++++- .../StackExchange.Redis.Tests/HotKeysTests.cs | 2 +- .../PubSubKeyNotificationTests.cs | 2 +- .../PubSubMultiserverTests.cs | 2 +- .../StackExchange.Redis.Tests/SentinelBase.cs | 40 ++++-- tests/StackExchange.Redis.Tests/TestBase.cs | 15 +++ 11 files changed, 209 insertions(+), 67 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 80f33c9c3..1a8889a02 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -14,6 +14,18 @@ on: description: 'Tag of the client libs test image to use' required: false default: '' + windows-topology: + description: 'Servers to start on the Windows job: basic (primary/replica/secure/failover) or full (adds cluster + sentinel)' + required: false + default: 'basic' + type: choice + options: + - basic + - full + windows-redis-version: + description: 'Pin the apt redis version on the Windows job, e.g. 6:8.0.2-1rl1~jammy1 (blank = whatever is GA)' + required: false + default: '' jobs: main: name: StackExchange.Redis (Ubuntu) @@ -67,65 +79,74 @@ jobs: # fixed there, but the guard above is worth keeping either way. printf '#!/bin/sh\nexit 101\n' > /usr/sbin/policy-rc.d chmod +x /usr/sbin/policy-rc.d - apt-get install -y redis + # A blank pin means "whatever is GA today", which is not reproducible: a run from last + # month and a run from today can test different servers. Supply windows-redis-version + # via workflow_dispatch to pin it; `apt-cache madison redis-server` lists the candidates. + PIN='${{ github.event.inputs.windows-redis-version }}' + if [ -n "$PIN" ]; then + case "$PIN" in + *[!A-Za-z0-9.:+~_-]*) echo "refusing suspicious version pin: $PIN" >&2; exit 1 ;; + esac + echo "pinning redis to $PIN" + apt-get install -y "redis-server=$PIN" "redis-tools=$PIN" \ + || { echo "pin not available; candidates:" >&2; apt-cache madison redis-server >&2; exit 1; } + else + apt-get install -y redis + fi + redis-server --version mkdir redis - name: Run redis-server shell: wsl-bash {0} working-directory: ./tests/RedisConfigs/redis run: | - pwd - ls . - # Run each server instance in order + # This runner is a fractional-vCPU VM running Windows running WSL running Ubuntu running + # redis, and the cluster/sentinel instances gossip and ping continuously even when idle. + # That background load is a large part of why this job intermittently sees 5-second + # timeouts on trivial commands, so the default here is the small topology; tests needing + # cluster or sentinel skip themselves as inconclusive when those servers are absent, and + # the Ubuntu job covers them on every run. Dispatch with windows-topology=full to get + # everything (and `docker compose --file tests/RedisConfigs/docker-compose.yml up -d` + # always brings up the complete set locally). + TOPOLOGY='${{ github.event.inputs.windows-topology || 'basic' }}' + echo "topology: $TOPOLOGY" + redis-server ../Basic/primary-6379.conf & redis-server ../Basic/replica-6380.conf & redis-server ../Basic/secure-6381.conf & redis-server ../Failover/primary-6382.conf & redis-server ../Failover/replica-6383.conf & - redis-server ../Cluster/cluster-7000.conf --dir ../Cluster & - redis-server ../Cluster/cluster-7001.conf --dir ../Cluster & - redis-server ../Cluster/cluster-7002.conf --dir ../Cluster & - redis-server ../Cluster/cluster-7003.conf --dir ../Cluster & - redis-server ../Cluster/cluster-7004.conf --dir ../Cluster & - redis-server ../Cluster/cluster-7005.conf --dir ../Cluster & - redis-server ../Sentinel/redis-7010.conf & - redis-server ../Sentinel/redis-7011.conf & - redis-server ../Sentinel/sentinel-26379.conf --sentinel & - redis-server ../Sentinel/sentinel-26380.conf --sentinel & - redis-server ../Sentinel/sentinel-26381.conf --sentinel & - # Wait for server instances to get ready - sleep 5 - echo "Checking redis-server version with port 6379" - redis-cli -p 6379 INFO SERVER | grep redis_version || echo "Failed to get version for port 6379" - echo "Checking redis-server version with port 6380" - redis-cli -p 6380 INFO SERVER | grep redis_version || echo "Failed to get version for port 6380" - echo "Checking redis-server version with port 6381" - redis-cli -p 6381 INFO SERVER | grep redis_version || echo "Failed to get version for port 6381" - echo "Checking redis-server version with port 6382" - redis-cli -p 6382 INFO SERVER | grep redis_version || echo "Failed to get version for port 6382" - echo "Checking redis-server version with port 6383" - redis-cli -p 6383 INFO SERVER | grep redis_version || echo "Failed to get version for port 6383" - echo "Checking redis-server version with port 7000" - redis-cli -p 7000 INFO SERVER | grep redis_version || echo "Failed to get version for port 7000" - echo "Checking redis-server version with port 7001" - redis-cli -p 7001 INFO SERVER | grep redis_version || echo "Failed to get version for port 7001" - echo "Checking redis-server version with port 7002" - redis-cli -p 7002 INFO SERVER | grep redis_version || echo "Failed to get version for port 7002" - echo "Checking redis-server version with port 7003" - redis-cli -p 7003 INFO SERVER | grep redis_version || echo "Failed to get version for port 7003" - echo "Checking redis-server version with port 7004" - redis-cli -p 7004 INFO SERVER | grep redis_version || echo "Failed to get version for port 7004" - echo "Checking redis-server version with port 7005" - redis-cli -p 7005 INFO SERVER | grep redis_version || echo "Failed to get version for port 7005" - echo "Checking redis-server version with port 7010" - redis-cli -p 7010 INFO SERVER | grep redis_version || echo "Failed to get version for port 7010" - echo "Checking redis-server version with port 7011" - redis-cli -p 7011 INFO SERVER | grep redis_version || echo "Failed to get version for port 7011" - echo "Checking redis-server version with port 26379" - redis-cli -p 26379 INFO SERVER | grep redis_version || echo "Failed to get version for port 26379" - echo "Checking redis-server version with port 26380" - redis-cli -p 26380 INFO SERVER | grep redis_version || echo "Failed to get version for port 26380" - echo "Checking redis-server version with port 26381" - redis-cli -p 26381 INFO SERVER | grep redis_version || echo "Failed to get version for port 26381" + PORTS="6379 6380 6381 6382 6383" + + if [ "$TOPOLOGY" = "full" ]; then + for p in 7000 7001 7002 7003 7004 7005; do + redis-server "../Cluster/cluster-$p.conf" --dir ../Cluster & + done + redis-server ../Sentinel/redis-7010.conf & + redis-server ../Sentinel/redis-7011.conf & + for p in 26379 26380 26381; do + redis-server "../Sentinel/sentinel-$p.conf" --sentinel & + done + PORTS="$PORTS 7000 7001 7002 7003 7004 7005 7010 7011 26379 26380 26381" + fi + + # Poll rather than `sleep 5`: on a fast machine this returns almost immediately, and on a + # slow one it actually waits long enough. A fixed sleep gets both cases wrong. + for p in $PORTS; do + ready="" + for _ in $(seq 1 30); do + out=$(redis-cli -p "$p" PING 2>&1) + # secure-6381 answers NOAUTH without credentials, which still proves it is listening + case "$out" in + PONG|*NOAUTH*) ready="yes"; break ;; + esac + sleep 1 + done + if [ -n "$ready" ]; then + echo "port $p ready: $(redis-cli -p "$p" INFO SERVER 2>/dev/null | grep redis_version || echo 'version unavailable')" + else + echo "port $p DID NOT COME UP" + fi + done continue-on-error: true - name: .NET Build diff --git a/tests/StackExchange.Redis.Tests/ClusterShardedTests.cs b/tests/StackExchange.Redis.Tests/ClusterShardedTests.cs index 0826df837..de6bfc6b4 100644 --- a/tests/StackExchange.Redis.Tests/ClusterShardedTests.cs +++ b/tests/StackExchange.Redis.Tests/ClusterShardedTests.cs @@ -11,7 +11,7 @@ namespace StackExchange.Redis.Tests; [Collection(NonParallelCollection.Name)] public class ClusterShardedTests(ITestOutputHelper output) : TestBase(output) { - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; + protected override string GetConfiguration() => GetClusterConfiguration(); [Fact] [Trait(TestCategories.Category, TestCategories.SimulatedConnectionFailure)] diff --git a/tests/StackExchange.Redis.Tests/ClusterTests.cs b/tests/StackExchange.Redis.Tests/ClusterTests.cs index af58346e9..5db215731 100644 --- a/tests/StackExchange.Redis.Tests/ClusterTests.cs +++ b/tests/StackExchange.Redis.Tests/ClusterTests.cs @@ -23,7 +23,7 @@ public enum StreamConsumerGroupRoutingOperation DeleteConsumerGroup, } - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; + protected override string GetConfiguration() => GetClusterConfiguration(); [Fact] public async Task ExportConfiguration() diff --git a/tests/StackExchange.Redis.Tests/ConnectCustomConfigTests.cs b/tests/StackExchange.Redis.Tests/ConnectCustomConfigTests.cs index d0e67f35f..f01541b61 100644 --- a/tests/StackExchange.Redis.Tests/ConnectCustomConfigTests.cs +++ b/tests/StackExchange.Redis.Tests/ConnectCustomConfigTests.cs @@ -36,6 +36,9 @@ public async Task DisabledCommandsStillConnect(string disabledCommands) [InlineData("config,info,get,cluster")] public async Task DisabledCommandsStillConnectCluster(string disabledCommands) { + // passes the cluster configuration directly rather than via GetConfiguration(), so it needs + // its own guard to skip promptly when the cluster is not running + Skip.IfNoCluster(); await using var conn = Create(allowAdmin: true, configuration: TestConfig.Current.ClusterServersAndPorts, disabledCommands: disabledCommands.Split(','), log: Writer); var db = conn.GetDatabase(); diff --git a/tests/StackExchange.Redis.Tests/GetServerTests.cs b/tests/StackExchange.Redis.Tests/GetServerTests.cs index 50cb9e7ef..1f6634845 100644 --- a/tests/StackExchange.Redis.Tests/GetServerTests.cs +++ b/tests/StackExchange.Redis.Tests/GetServerTests.cs @@ -135,7 +135,7 @@ public async Task GetServerWithDefaultKey(bool explicitNull) [RunPerProtocol] public class GetServerTestsCluster(ITestOutputHelper output, SharedConnectionFixture fixture) : GetServerTestsBase(output, fixture) { - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts; + protected override string GetConfiguration() => GetClusterConfiguration(string.Empty); protected override bool IsCluster => true; } diff --git a/tests/StackExchange.Redis.Tests/Helpers/Skip.cs b/tests/StackExchange.Redis.Tests/Helpers/Skip.cs index 72d62a3dc..d51ba7893 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/Skip.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/Skip.cs @@ -1,5 +1,8 @@ -using System; +using System; +using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; +using System.Net.Sockets; +using System.Threading; using Xunit; namespace StackExchange.Redis.Tests; @@ -21,6 +24,88 @@ internal static void IfMissingDatabase(IConnectionMultiplexer conn, int dbId) var dbCount = conn.GetServer(conn.GetEndPoints()[0]).DatabaseCount; Assert.SkipWhen(dbId >= dbCount, $"Database '{dbId}' is not supported on this server."); } + + /// + /// Skips the test when nothing is listening on :. + /// + /// + /// Tests that build their own connection (rather than using the shared fixture) otherwise each + /// pay a full connect timeout before failing, which is both slow and reported as a failure + /// instead of a skip. This is a single TCP connect per endpoint, cached for the process, so the + /// second and subsequent tests needing an absent server skip immediately. + /// + /// Note this deliberately only reports whether *something* is accepting connections: if a + /// server is listening but the client cannot talk to it, that is a real failure and must stay + /// one, so it is not covered here. + /// + /// + public static void IfNoServer(string? host, int port) + { + Assert.SkipWhen(!ServerProbe.IsListening(host, port), $"Nothing is listening on {host}:{port}, skipping test."); + } + + /// + /// Skips the test when the cluster nodes are not running. + /// + public static void IfNoCluster() + { + var config = TestConfig.Current; + IfNoServer(config.ClusterServer, config.ClusterStartPort); + } + + /// + /// Skips the test when the sentinel instances are not running. + /// + public static void IfNoSentinel() + { + var config = TestConfig.Current; + IfNoServer(config.SentinelServer, config.SentinelPortA); + } + + /// + /// Skips the test when the failover pair is not running. + /// + public static void IfNoFailoverPair() + { + var config = TestConfig.Current; + IfNoServer(config.FailoverPrimaryServer, config.FailoverPrimaryPort); + IfNoServer(config.FailoverReplicaServer, config.FailoverReplicaPort); + } +} + +internal static class ServerProbe +{ + // Generous on purpose: a listening server accepts effectively instantly even on a slow or + // heavily contended machine (the kernel completes the handshake from the backlog), so this + // only ever waits this long when nothing is there. Being too aggressive here would risk + // declaring a live-but-busy server absent and silently skipping tests that should have run. + private static readonly TimeSpan ProbeTimeout = TimeSpan.FromSeconds(2); + + private static readonly ConcurrentDictionary<(string Host, int Port), Lazy> Cache = new(); + + internal static bool IsListening(string? host, int port) + { + if (host.IsNullOrEmpty()) return false; + + var probe = Cache.GetOrAdd( + (host, port), + static key => new Lazy(() => Probe(key.Host, key.Port), LazyThreadSafetyMode.ExecutionAndPublication)); + return probe.Value; + } + + private static bool Probe(string host, int port) + { + try + { + using var client = new TcpClient(); + return client.ConnectAsync(host, port).Wait(ProbeTimeout); + } + catch + { + // refused, unresolvable, unreachable: all "no server here" + return false; + } + } } public class SkipTestException(string reason) : Exception(reason) diff --git a/tests/StackExchange.Redis.Tests/HotKeysTests.cs b/tests/StackExchange.Redis.Tests/HotKeysTests.cs index 4b43b04a0..ec1de6f9d 100644 --- a/tests/StackExchange.Redis.Tests/HotKeysTests.cs +++ b/tests/StackExchange.Redis.Tests/HotKeysTests.cs @@ -8,7 +8,7 @@ namespace StackExchange.Redis.Tests; [Collection(NonParallelCollection.Name)] public class HotKeysClusterTests(ITestOutputHelper output, SharedConnectionFixture fixture) : HotKeysTests(output, fixture) { - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; + protected override string GetConfiguration() => GetClusterConfiguration(); [Theory] [InlineData(true)] diff --git a/tests/StackExchange.Redis.Tests/PubSubKeyNotificationTests.cs b/tests/StackExchange.Redis.Tests/PubSubKeyNotificationTests.cs index 51fd4c80a..6c85a94bd 100644 --- a/tests/StackExchange.Redis.Tests/PubSubKeyNotificationTests.cs +++ b/tests/StackExchange.Redis.Tests/PubSubKeyNotificationTests.cs @@ -15,7 +15,7 @@ namespace StackExchange.Redis.Tests; public sealed class PubSubKeyNotificationTestsCluster(ITestOutputHelper output, ITestContextAccessor context, SharedConnectionFixture fixture) : PubSubKeyNotificationTests(output, context, fixture) { - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts; + protected override string GetConfiguration() => GetClusterConfiguration(string.Empty); } // ReSharper disable once UnusedMember.Global - used via test framework diff --git a/tests/StackExchange.Redis.Tests/PubSubMultiserverTests.cs b/tests/StackExchange.Redis.Tests/PubSubMultiserverTests.cs index f33e125ae..e4e97e63a 100644 --- a/tests/StackExchange.Redis.Tests/PubSubMultiserverTests.cs +++ b/tests/StackExchange.Redis.Tests/PubSubMultiserverTests.cs @@ -8,7 +8,7 @@ namespace StackExchange.Redis.Tests; [RunPerProtocol] public class PubSubMultiserverTests(ITestOutputHelper output, SharedConnectionFixture fixture) : TestBase(output, fixture) { - protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; + protected override string GetConfiguration() => GetClusterConfiguration(); [Fact] public async Task ChannelSharding() diff --git a/tests/StackExchange.Redis.Tests/SentinelBase.cs b/tests/StackExchange.Redis.Tests/SentinelBase.cs index 826b9c613..b1e1b75b3 100644 --- a/tests/StackExchange.Redis.Tests/SentinelBase.cs +++ b/tests/StackExchange.Redis.Tests/SentinelBase.cs @@ -10,6 +10,9 @@ namespace StackExchange.Redis.Tests; public class SentinelBase : TestBase, IAsyncLifetime { + private const int ConnectAttempts = 150; + private static readonly TimeSpan ConnectRetryDelay = TimeSpan.FromMilliseconds(100); + protected static string ServiceName => TestConfig.Current.SentinelSeviceName; protected static ConfigurationOptions ServiceOptions => new ConfigurationOptions { ServiceName = ServiceName, AllowAdmin = true }; @@ -24,6 +27,10 @@ public SentinelBase(ITestOutputHelper output) : base(output) { Skip.IfNoConfig(nameof(TestConfig.Config.SentinelServer), TestConfig.Current.SentinelServer); Skip.IfNoConfig(nameof(TestConfig.Config.SentinelSeviceName), TestConfig.Current.SentinelSeviceName); + // Config being present only says where the sentinels *would* be. If they are not running, + // InitializeAsync below spends 15 seconds polling before asserting, once per test, and + // reports a failure rather than a skip; probe the endpoint instead (cached per run). + Skip.IfNoSentinel(); } #nullable enable @@ -37,19 +44,30 @@ public async ValueTask InitializeAsync() options.EndPoints.Add(TestConfig.Current.SentinelServer, TestConfig.Current.SentinelPortC); Conn = ConnectionMultiplexer.SentinelConnect(options, Writer); - for (var i = 0; i < 150; i++) + // Two things have to come up: the sentinel connection, and a connection to the primary that + // sentinel reports. The latter is what most of these tests actually use, so wait for it + // explicitly - waiting only on the former lets a broken primary surface later, from + // somewhere much less obviously related. + var sw = Stopwatch.StartNew(); + bool sentinelConnected = false, primaryConnected = false; + for (var i = 0; i < ConnectAttempts && !primaryConnected; i++) { - await Task.Delay(100).ForAwait(); - if (Conn.IsConnected) - { - await using var checkConn = Conn.GetSentinelMasterConnection(options, Writer); - if (checkConn.IsConnected) - { - break; - } - } + if (i != 0) await Task.Delay(ConnectRetryDelay).ForAwait(); // check first, then back off + sentinelConnected = Conn.IsConnected; + if (!sentinelConnected) continue; + + await using var checkConn = Conn.GetSentinelMasterConnection(options, Writer); + primaryConnected = checkConn.IsConnected; } - Assert.True(Conn.IsConnected); + + var diagnostics = $"Sentinel setup did not become usable within {sw.Elapsed.TotalSeconds:0.0}s: " + + $"sentinel connected = {sentinelConnected}, primary connected = {primaryConnected}. " + + $"Service name '{ServiceName}', sentinels at {TestConfig.Current.SentinelServer}:" + + $"{TestConfig.Current.SentinelPortA}/{TestConfig.Current.SentinelPortB}/{TestConfig.Current.SentinelPortC}. " + + "Something is listening on the sentinel port (this test skips otherwise), so this is a " + + "sentinel or primary/replica state problem rather than a missing server: check the " + + "primary and replica for that service name (7010/7011 in the standard test topology)."; + Assert.True(primaryConnected, diagnostics); SentinelServerA = Conn.GetServer(TestConfig.Current.SentinelServer, TestConfig.Current.SentinelPortA)!; SentinelServerB = Conn.GetServer(TestConfig.Current.SentinelServer, TestConfig.Current.SentinelPortB)!; SentinelServerC = Conn.GetServer(TestConfig.Current.SentinelServer, TestConfig.Current.SentinelPortC)!; diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index 5a5ffe691..71402f7ec 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -29,6 +29,21 @@ protected virtual string GetConfiguration() } internal static string GetDefaultConfiguration() => TestConfig.Current.PrimaryServerAndPort; + /// + /// Cluster endpoints for tests that need the cluster, skipping the test when it is not running. + /// + /// + /// The probe has to happen here rather than when connecting: the cluster configuration carries a + /// deliberately long connectTimeout, so a test that discovers the absence by connecting + /// pays that timeout in full before it can skip. With ~50 cluster tests that is the difference + /// between a one-minute run and a nine-minute one. + /// + protected static string GetClusterConfiguration(string suffix = ",connectTimeout=10000") + { + Skip.IfNoCluster(); + return TestConfig.Current.ClusterServersAndPorts + suffix; + } + private readonly SharedConnectionFixture? _sharedConnectionFixture; private readonly InProcServerFixture? _inProcServerFixture; From d6b471102fc1bf8bfd0de99fd5f4a6f12669e4eb Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 6 Aug 2026 16:41:31 +0100 Subject: [PATCH 2/4] CI: don't let the readiness probe kill its own step The wsl-bash shell runs with -euo pipefail, so `out=$(redis-cli ... PING)` aborts the entire step the first time a server is not yet accepting connections - which is the exact state the poll exists to wait out. The previous code never hit this because every redis-cli call was guarded with `|| echo ...`. Effect on the last run: the step died 41ms after launching the servers, continue-on-error painted it green, and the suite then ran against a Redis that wasn't there - thousands of "not possible to connect" failures across MultiPrimaryTests, RespProtocolTests and friends, ten minutes later and nowhere near the cause. So: tolerate the expected probe failure, and drop continue-on-error in favour of failing this step explicitly when a required port never comes up. One clear "no server on: 6379" beats several thousand connection errors in a later step. --- .github/workflows/CI.yml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 1a8889a02..5c9c38b01 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -131,10 +131,15 @@ jobs: # Poll rather than `sleep 5`: on a fast machine this returns almost immediately, and on a # slow one it actually waits long enough. A fixed sleep gets both cases wrong. + # + # NB: this shell runs with -euo pipefail, so a bare `out=$(redis-cli ...)` aborts the whole + # step the instant a server is not yet accepting connections - i.e. precisely the state we + # are here to wait out. Every probe therefore tolerates failure explicitly. + missing="" for p in $PORTS; do ready="" for _ in $(seq 1 30); do - out=$(redis-cli -p "$p" PING 2>&1) + out=$(redis-cli -p "$p" PING 2>&1 || true) # secure-6381 answers NOAUTH without credentials, which still proves it is listening case "$out" in PONG|*NOAUTH*) ready="yes"; break ;; @@ -145,9 +150,17 @@ jobs: echo "port $p ready: $(redis-cli -p "$p" INFO SERVER 2>/dev/null | grep redis_version || echo 'version unavailable')" else echo "port $p DID NOT COME UP" + missing="$missing $p" fi done - continue-on-error: true + + # Deliberately fatal, and deliberately not continue-on-error: running ~5000 tests against + # servers that never started produces thousands of confusing connection failures ten + # minutes later, instead of one clear message here. + if [ -n "$missing" ]; then + echo "::error::no server on:$missing" + exit 1 + fi - name: .NET Build run: dotnet build Build.csproj -c Release /p:CI=true From 25df412ef4cd0c66213f5bbe5b11d8bbb09a4f07 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 10 Aug 2026 10:02:42 +0100 Subject: [PATCH 3/4] Attack the actual Windows CI failures, not the topology Three post-change runs said the reduced topology was not the lever: each failed on a different set of tests, none needing the servers it omits, and the job got no faster. So windows-topology defaults back to full - this job is our only coverage of .NET Framework and of Windows' own socket/TLS stacks, and that is worth keeping complete. `basic` stays available via workflow_dispatch as a bisection tool. What the failures actually show is a machine that cannot honour the library's defaults, so: - Raise the thread pool floor (TestConfig static ctor) to max(64, cores*8). The pool grows ~1-2 threads/sec past its minimum, and the suite opens many connections at once; that ramp is what turns a healthy server into "Timeout performing PING (5000ms)" when a synchronous caller is parked waiting for a completion that cannot get a thread. Free on a fast machine. - Add a timeout floor, REDIS_TESTS_MIN_TIMEOUT_MS, set to 20s on the Windows job only, applied to SyncTimeout and AsyncTimeout and only where the test did not ask for a specific timeout - a test choosing a short timeout is testing timeout behaviour. Not applied to ConnectTimeout: bisection showed that flooring it breaks tests that simulate a failure and then allow a fixed window for the heartbeat to reconnect, because a stalled attempt can no longer be retried inside that window. SyncTimeout breaks that same test, so ConnectFailTimeoutTests.NoticesConnectFail now states its dependency explicitly. - Fix two genuinely fragile tests. SelectByLatency compared injected latencies 10ms and 15ms apart and waited a fixed 100ms to "settle"; scheduling noise on a contended machine exceeds that margin and inverts the ordering, so the margins are now 150/300ms and it polls until selection converges. And SelectByWeight's failure was really Assert.True(conn.IsConnected) immediately after ConnectGroupAsync, which races health-check probes that need a round trip; that pattern appeared at six sites, all now waiting via GroupWait. Also drops a duplicate endpoint probe in favour of caching the existing TestConfig.IsServerRunning. Verified: full suite green both with and without the floor (0 failed, 5731 passed, 150 skipped - the same skip count as before any of this), and the whole traversal builds across every TFM. --- .github/workflows/CI.yml | 25 ++++---- .../ConnectFailTimeoutTests.cs | 6 +- .../StackExchange.Redis.Tests/Helpers/Skip.cs | 30 +++------ .../Helpers/TestConfig.cs | 30 +++++++++ .../MultiGroupTests/BasicMultiGroupTests.cs | 64 ++++++++++++------- .../CircuitBreakerRerouteTests.cs | 4 +- .../GroupConfigResolutionTests.cs | 4 +- .../MultiGroupTests/GroupWait.cs | 33 ++++++++++ tests/StackExchange.Redis.Tests/TestBase.cs | 15 +++++ 9 files changed, 151 insertions(+), 60 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/MultiGroupTests/GroupWait.cs diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 5c9c38b01..b59c7729f 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -15,9 +15,9 @@ on: required: false default: '' windows-topology: - description: 'Servers to start on the Windows job: basic (primary/replica/secure/failover) or full (adds cluster + sentinel)' + description: 'Servers to start on the Windows job: full (everything) or basic (primary/replica/secure/failover only)' required: false - default: 'basic' + default: 'full' type: choice options: - basic @@ -51,6 +51,11 @@ jobs: DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION: "1" # Note this doesn't work yet for Windows - see https://github.com/dotnet/runtime/issues/68340 TERM: xterm DOCKER_BUILDKIT: 1 + # This runner routinely fails the library's 5s defaults for reasons that are nothing to do + # with the code under test (fractional vCPU, nested virtualisation, thread-pool ramp). Raise + # the floor for connections that do not ask for a specific timeout; tests that deliberately + # pick a short timeout to exercise timeout behaviour are unaffected. + REDIS_TESTS_MIN_TIMEOUT_MS: "20000" steps: - name: Checkout code uses: actions/checkout@v6 @@ -99,15 +104,13 @@ jobs: shell: wsl-bash {0} working-directory: ./tests/RedisConfigs/redis run: | - # This runner is a fractional-vCPU VM running Windows running WSL running Ubuntu running - # redis, and the cluster/sentinel instances gossip and ping continuously even when idle. - # That background load is a large part of why this job intermittently sees 5-second - # timeouts on trivial commands, so the default here is the small topology; tests needing - # cluster or sentinel skip themselves as inconclusive when those servers are absent, and - # the Ubuntu job covers them on every run. Dispatch with windows-topology=full to get - # everything (and `docker compose --file tests/RedisConfigs/docker-compose.yml up -d` - # always brings up the complete set locally). - TOPOLOGY='${{ github.event.inputs.windows-topology || 'basic' }}' + # Defaults to the whole topology: this job is the only coverage of .NET Framework and of + # Windows' own socket/TLS stacks, so we would rather keep it complete. `basic` starts only + # primary/replica/secure/failover, which is useful when bisecting whether cluster and + # sentinel background load is implicated in a failure - measured across three runs it was + # not, so it is opt-in rather than the default. Tests needing absent servers skip promptly + # either way (see Skip.IfNoCluster / Skip.IfNoSentinel). + TOPOLOGY='${{ github.event.inputs.windows-topology || 'full' }}' echo "topology: $TOPOLOGY" redis-server ../Basic/primary-6379.conf & diff --git a/tests/StackExchange.Redis.Tests/ConnectFailTimeoutTests.cs b/tests/StackExchange.Redis.Tests/ConnectFailTimeoutTests.cs index 5f41103d0..a3976c116 100644 --- a/tests/StackExchange.Redis.Tests/ConnectFailTimeoutTests.cs +++ b/tests/StackExchange.Redis.Tests/ConnectFailTimeoutTests.cs @@ -11,7 +11,11 @@ public class ConnectFailTimeoutTests(ITestOutputHelper output) : TestBase(output public async Task NoticesConnectFail() { SetExpectedAmbientFailureCount(-1); - await using var conn = Create(allowAdmin: true, backlogPolicy: BacklogPolicy.FailFast, allowSimulateConnectionFailure: true); + // syncTimeout is explicit because this test depends on it: it simulates a failure, expects the + // next synchronous call to give up, and then allows a fixed window for the heartbeat to + // reconnect. A longer sync timeout (see TestConfig.MinTimeoutMilliseconds, raised on slow CI) + // changes what that call does and breaks the scenario. + await using var conn = Create(allowAdmin: true, backlogPolicy: BacklogPolicy.FailFast, allowSimulateConnectionFailure: true, syncTimeout: 5000); var server = conn.GetServer(conn.GetEndPoints()[0]); Assert.SkipUnless(server.CanSimulateConnectionFailure(), "Skipping because server cannot simulate connection failure"); diff --git a/tests/StackExchange.Redis.Tests/Helpers/Skip.cs b/tests/StackExchange.Redis.Tests/Helpers/Skip.cs index d51ba7893..bdf41a2c5 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/Skip.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/Skip.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; -using System.Net.Sockets; using System.Threading; using Xunit; @@ -73,14 +72,13 @@ public static void IfNoFailoverPair() } } +/// +/// Caches per endpoint for the lifetime of the run, so that +/// a whole class of tests needing an absent server pays one connect attempt between them rather than +/// one each. +/// internal static class ServerProbe { - // Generous on purpose: a listening server accepts effectively instantly even on a slow or - // heavily contended machine (the kernel completes the handshake from the backlog), so this - // only ever waits this long when nothing is there. Being too aggressive here would risk - // declaring a live-but-busy server absent and silently skipping tests that should have run. - private static readonly TimeSpan ProbeTimeout = TimeSpan.FromSeconds(2); - private static readonly ConcurrentDictionary<(string Host, int Port), Lazy> Cache = new(); internal static bool IsListening(string? host, int port) @@ -89,23 +87,11 @@ internal static bool IsListening(string? host, int port) var probe = Cache.GetOrAdd( (host, port), - static key => new Lazy(() => Probe(key.Host, key.Port), LazyThreadSafetyMode.ExecutionAndPublication)); + static key => new Lazy( + () => TestConfig.IsServerRunning(key.Host, key.Port), + LazyThreadSafetyMode.ExecutionAndPublication)); return probe.Value; } - - private static bool Probe(string host, int port) - { - try - { - using var client = new TcpClient(); - return client.ConnectAsync(host, port).Wait(ProbeTimeout); - } - catch - { - // refused, unresolvable, unreachable: all "no server here" - return false; - } - } } public class SkipTestException(string reason) : Exception(reason) diff --git a/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs b/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs index 59520eab4..09947fa25 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs @@ -13,6 +13,19 @@ public static class TestConfig public static Config Current { get; } + /// + /// A floor, in milliseconds, for connection timeouts, from REDIS_TESTS_MIN_TIMEOUT_MS; + /// zero (the default) leaves the library's own defaults alone. + /// + /// + /// This exists for CI machines that cannot honour the library's 5s defaults for reasons that have + /// nothing to do with the code under test. It deliberately applies only where a test has not + /// asked for a specific timeout, so tests that pick a short one to exercise timeout behaviour + /// keep working. + /// + public static int MinTimeoutMilliseconds { get; } = + int.TryParse(Environment.GetEnvironmentVariable("REDIS_TESTS_MIN_TIMEOUT_MS"), out var ms) && ms > 0 ? ms : 0; + #if NET private static int _db = 17; #else @@ -27,6 +40,23 @@ public static int GetDedicatedDB(IConnectionMultiplexer? conn = null) static TestConfig() { + // The suite opens a lot of connections at once (xunit runs 2x cores' worth of collections in + // parallel), and the thread pool grows only ~1-2 threads per second past its minimum. On a + // slow or contended machine that ramp is what turns a perfectly healthy server into + // "Timeout performing PING (5000ms)": a synchronous caller parks waiting for a completion + // that cannot get a thread. Raising the floor costs nothing on a fast machine, and it is the + // same advice we give users in docs/Timeouts.md. + try + { + ThreadPool.GetMinThreads(out var workerThreads, out var completionPortThreads); + var target = Math.Max(64, Environment.ProcessorCount * 8); + ThreadPool.SetMinThreads(Math.Max(workerThreads, target), Math.Max(completionPortThreads, target)); + } + catch (Exception ex) + { + Console.WriteLine("Unable to raise ThreadPool minimums: " + ex.Message); + } + Current = new Config(); try { diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/BasicMultiGroupTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/BasicMultiGroupTests.cs index 23eaaa7bd..699c15491 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/BasicMultiGroupTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/BasicMultiGroupTests.cs @@ -1,5 +1,6 @@ -using System; +using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Net; using System.Threading.Tasks; @@ -134,7 +135,7 @@ public async Task SelectByWeight(InbuiltProbe probe, ServerType serverType) ]; MultiGroupOptions options = new MultiGroupOptions.Builder { HealthCheck = healthCheck }; await using var conn = await ConnectionMultiplexer.ConnectGroupAsync(members, options); - Assert.True(conn.IsConnected); + await GroupWait.AssertConnectedAsync(conn); var typed = Assert.IsType(conn); // (R.4.1) If multiple member databases are configured, then I want to failover to the one with the highest weight. @@ -151,6 +152,31 @@ public async Task SelectByWeight(InbuiltProbe probe, ServerType serverType) WriteLatency(conn); } + /// + /// Refreshes latencies and re-selects until the expected group wins, returning whatever was + /// selected last so the caller's assert reports the real value. + /// + /// + /// Replaces "heartbeat, sleep 100ms, hope it settled": the latency probes are asynchronous, so a + /// fixed delay is simultaneously too long here and too short on a loaded machine. + /// + private async Task SelectPreferredAsync(MultiGroupMultiplexer typed, IDatabase db, EndPoint expected) + { + EndPoint? ep = null; + var watch = Stopwatch.StartNew(); + while (watch.Elapsed < TimeSpan.FromSeconds(10)) + { + typed.OnHeartbeat(); // update latencies + typed.SelectPreferredGroup(); + ep = await db.IdentifyEndpointAsync(); + if (Equals(ep, expected)) break; + await Task.Delay(50, TestContext.Current.CancellationToken); + } + + WriteLatency(typed); + return ep; + } + private void WriteLatency(IConnectionGroup conn) { var typed = Assert.IsType(conn); @@ -186,31 +212,25 @@ static ConfigurationOptions Check(ConfigurationOptions options) await using var conn = await ConnectionMultiplexer.ConnectGroupAsync(members); conn.ConnectionChanged += (_, args) => log.WriteLine($"Connection changed: {args.Type}, from {args.PreviousGroup?.Name ?? "(nil)"} to {args.Group.Name}"); - Assert.True(conn.IsConnected); - server0.SetLatency(TimeSpan.FromMilliseconds(10)); + await GroupWait.AssertConnectedAsync(conn); + + // Injected latencies are deliberately far apart. Measured latency includes scheduling noise, + // and on a contended machine (CI in particular) that noise comfortably exceeds the ~10ms + // margins this test used to rely on, which inverts the ordering and fails for no good reason. + server0.SetLatency(TimeSpan.FromMilliseconds(150)); server1.SetLatency(TimeSpan.Zero); - server2.SetLatency(TimeSpan.FromMilliseconds(15)); + server2.SetLatency(TimeSpan.FromMilliseconds(300)); var typed = Assert.IsType(conn); - typed.OnHeartbeat(); // update latencies - await Task.Delay(100); // allow time to settle - typed.SelectPreferredGroup(); - WriteLatency(typed); + var db = conn.GetDatabase(); // select lowest latency - var db = conn.GetDatabase(); - var ep = await db.IdentifyEndpointAsync(); - Assert.Equal(canada, ep); + Assert.Equal(canada, await SelectPreferredAsync(typed, db, canada)); // change latency and update - server0.SetLatency(TimeSpan.FromMilliseconds(10)); - server1.SetLatency(TimeSpan.FromMilliseconds(10)); + server0.SetLatency(TimeSpan.FromMilliseconds(150)); + server1.SetLatency(TimeSpan.FromMilliseconds(150)); server2.SetLatency(TimeSpan.Zero); - typed.OnHeartbeat(); // update latencies - await Task.Delay(100); // allow time to settle - typed.SelectPreferredGroup(); - ep = await db.IdentifyEndpointAsync(); - WriteLatency(typed); - Assert.Equal(tokyo, ep); + Assert.Equal(tokyo, await SelectPreferredAsync(typed, db, tokyo)); } [Fact] @@ -241,7 +261,7 @@ public async Task PubSubRouted() new(server2.GetClientConfig()) { Weight = 3 }, ]; await using var conn = await ConnectionMultiplexer.ConnectGroupAsync(members); - Assert.True(conn.IsConnected); + await GroupWait.AssertConnectedAsync(conn); var typed = Assert.IsType(conn); var multi = conn.GetSubscriber(); await multi.SubscribeAsync(channel, (x, y) => capture.Seen(nameof(conn), x, y)); @@ -321,7 +341,7 @@ public async Task PubSubOrderedRouted() new(server2.GetClientConfig()) { Weight = 3 }, ]; await using var conn = await ConnectionMultiplexer.ConnectGroupAsync(members); - Assert.True(conn.IsConnected); + await GroupWait.AssertConnectedAsync(conn); var typed = Assert.IsType(conn); var multi = conn.GetSubscriber(); _ = capture.WriteSeen(nameof(conn), await multi.SubscribeAsync(channel)); diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/CircuitBreakerRerouteTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/CircuitBreakerRerouteTests.cs index 86d3dc01f..4cfb15d24 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/CircuitBreakerRerouteTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/CircuitBreakerRerouteTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Net; using System.Threading; using System.Threading.Tasks; @@ -70,7 +70,7 @@ public async Task CircuitBreakerTrip_ReroutesAwayFromMember() }; // sanity: A (highest weight) is the active member to begin with - Assert.True(conn.IsConnected); + await GroupWait.AssertConnectedAsync(conn); Assert.Same(members[0], conn.ActiveMember); // arm the breaker and hold A unhealthy, then drive a *faulting* command to the active member (A): diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupConfigResolutionTests.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupConfigResolutionTests.cs index 2aca640ba..cec701ff8 100644 --- a/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupConfigResolutionTests.cs +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupConfigResolutionTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Net; using System.Threading.Tasks; using StackExchange.Redis.Availability; @@ -95,7 +95,7 @@ public async Task DisabledHealthCheckLeavesMemberSelectableOnConnectivityAlone() ]; await using var conn = await ConnectionMultiplexer.ConnectGroupAsync(members, options); - Assert.True(conn.IsConnected); + await GroupWait.AssertConnectedAsync(conn); Assert.Equal("beta", conn.ActiveMember?.Name); Assert.All(members, member => Assert.False(member.IsUnhealthy)); } diff --git a/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupWait.cs b/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupWait.cs new file mode 100644 index 000000000..5b2ac0dc3 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/MultiGroupTests/GroupWait.cs @@ -0,0 +1,33 @@ +using System; +using System.Diagnostics; +using System.Threading.Tasks; +using StackExchange.Redis.Availability; +using Xunit; + +namespace StackExchange.Redis.Tests.MultiGroupTests; + +internal static class GroupWait +{ + private static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(10); + + /// + /// Waits for the group to report connected, instead of asserting it the instant + /// ConnectGroupAsync returns. + /// + /// + /// Health-check probes that involve a round trip (Ping, StringSet) can still be in + /// flight when the connect task completes, so asserting immediately is a race that a slow or + /// contended machine loses. Waiting costs nothing when it is already connected. + /// + internal static async Task AssertConnectedAsync(IConnectionGroup conn, TimeSpan? timeout = null) + { + var limit = timeout ?? DefaultTimeout; + var watch = Stopwatch.StartNew(); + while (!conn.IsConnected && watch.Elapsed < limit) + { + await Task.Delay(25, TestContext.Current.CancellationToken); + } + + Assert.True(conn.IsConnected, $"group did not report connected within {limit.TotalSeconds:0.#}s"); + } +} diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index 71402f7ec..c59039478 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -29,6 +29,15 @@ protected virtual string GetConfiguration() } internal static string GetDefaultConfiguration() => TestConfig.Current.PrimaryServerAndPort; + /// + /// Applies as a lower bound, for CI machines that + /// cannot honour the library's defaults. Only ever raises, and only called where the test did not + /// request a specific timeout - a test asking for a short timeout is testing timeout behaviour, + /// and silently lengthening it would defeat the test. + /// + private static int RaiseToFloor(int timeout) => + TestConfig.MinTimeoutMilliseconds > timeout ? TestConfig.MinTimeoutMilliseconds : timeout; + /// /// Cluster endpoints for tests that need the cluster, skipping the test when it is not running. /// @@ -467,9 +476,15 @@ public static ConnectionMultiplexer CreateDefault( if (clientName is not null) config.ClientName = clientName; else if (!string.IsNullOrEmpty(caller)) config.ClientName = caller; if (syncTimeout is not null) config.SyncTimeout = syncTimeout.Value; + else config.SyncTimeout = RaiseToFloor(config.SyncTimeout); if (asyncTimeout is not null) config.AsyncTimeout = asyncTimeout.Value; + else config.AsyncTimeout = RaiseToFloor(config.AsyncTimeout); if (allowAdmin is not null) config.AllowAdmin = allowAdmin.Value; if (keepAlive is not null) config.KeepAlive = keepAlive.Value; + // No floor on ConnectTimeout, deliberately: tests that simulate a failure and then allow a + // fixed window for the heartbeat to reconnect (ConnectFailTimeoutTests.NoticesConnectFail) + // break when a stalled connect attempt can no longer be retried inside that window. Verified + // by bisection: flooring ConnectTimeout fails that test on its own, as does SyncTimeout. if (connectTimeout is not null) config.ConnectTimeout = connectTimeout.Value; if (proxy is not null) config.Proxy = proxy.Value; if (defaultDatabase is not null) config.DefaultDatabase = defaultDatabase.Value; From 41b7197ca26c06d04ad03624abaa534258744344 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 10 Aug 2026 10:37:10 +0100 Subject: [PATCH 4/4] Tests: wait for the subscription connection before asserting on it PubSubGetAllAnyOrder asserts sub.IsConnected() immediately after connecting, but the subscription connection is separate from the interactive one and can still be coming up at that point - so on a slow or contended machine the assert loses the race. This was the only remaining failure on the Windows CI job, reported as the rather opaque "IsConnected" (the nameof used as the assert message). Wait via the existing UntilConditionAsync helper first. Same treatment for the identical assert in FailoverTests. Note this cannot be demonstrated locally: the race needs a machine slow enough to lose it, so a local run proves only that nothing regressed. --- tests/StackExchange.Redis.Tests/FailoverTests.cs | 1 + tests/StackExchange.Redis.Tests/PubSubTests.cs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/tests/StackExchange.Redis.Tests/FailoverTests.cs b/tests/StackExchange.Redis.Tests/FailoverTests.cs index da470631c..e94acdef5 100644 --- a/tests/StackExchange.Redis.Tests/FailoverTests.cs +++ b/tests/StackExchange.Redis.Tests/FailoverTests.cs @@ -205,6 +205,7 @@ public async Task SubscriptionsSurviveConnectionFailureAsync() RedisChannel channel = RedisChannel.Literal(Me()); var sub = conn.GetSubscriber(); int counter = 0; + await UntilConditionAsync(TimeSpan.FromSeconds(10), () => sub.IsConnected()).ForAwait(); Assert.True(sub.IsConnected()); await sub.SubscribeAsync(channel, (arg1, arg2) => Interlocked.Increment(ref counter)).ConfigureAwait(false); diff --git a/tests/StackExchange.Redis.Tests/PubSubTests.cs b/tests/StackExchange.Redis.Tests/PubSubTests.cs index b4ba7b5de..6d5219908 100644 --- a/tests/StackExchange.Redis.Tests/PubSubTests.cs +++ b/tests/StackExchange.Redis.Tests/PubSubTests.cs @@ -425,6 +425,10 @@ public async Task PubSubGetAllAnyOrder() const int count = 1000; var syncLock = new object(); + // The subscription connection is a separate connection from the interactive one, and can still + // be coming up when the connect call returns; asserting it immediately is a race that a slow or + // contended machine loses (this is the "IsConnected" failure seen on the Windows CI job). + await UntilConditionAsync(TimeSpan.FromSeconds(10), () => sub.IsConnected()).ForAwait(); Assert.True(sub.IsConnected(), nameof(sub.IsConnected)); var data = new HashSet(); await sub.SubscribeAsync(channel, (_, val) =>