Stabilize Windows CI

.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: full (everything) or basic (primary/replica/secure/failover only)'
+        required: false
+        default: 'full'
+        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)
@@ -39,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
@@ -67,66 +84,86 @@ 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
+          # 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 &
           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"
-        continue-on-error: true
+          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.
+          #
+          # 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 || true)
+              # 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"
+              missing="$missing $p"
+            fi
+          done
+
+          # 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

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)]

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()

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();

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");

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);
 

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;
 }

tests/StackExchange.Redis.Tests/Helpers/Skip.cs

@@ -1,5 +1,7 @@
-using System;
+using System;
+using System.Collections.Concurrent;
 using System.Diagnostics.CodeAnalysis;
+using System.Threading;
 using Xunit;
 
 namespace StackExchange.Redis.Tests;
@@ -21,6 +23,75 @@ 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.");
     }
+
+    /// <summary>
+    /// Skips the test when nothing is listening on <paramref name="host"/>:<paramref name="port"/>.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// <para>
+    /// 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.
+    /// </para>
+    /// </remarks>
+    public static void IfNoServer(string? host, int port)
+    {
+        Assert.SkipWhen(!ServerProbe.IsListening(host, port), $"Nothing is listening on {host}:{port}, skipping test.");
+    }
+
+    /// <summary>
+    /// Skips the test when the cluster nodes are not running.
+    /// </summary>
+    public static void IfNoCluster()
+    {
+        var config = TestConfig.Current;
+        IfNoServer(config.ClusterServer, config.ClusterStartPort);
+    }
+
+    /// <summary>
+    /// Skips the test when the sentinel instances are not running.
+    /// </summary>
+    public static void IfNoSentinel()
+    {
+        var config = TestConfig.Current;
+        IfNoServer(config.SentinelServer, config.SentinelPortA);
+    }
+
+    /// <summary>
+    /// Skips the test when the failover pair is not running.
+    /// </summary>
+    public static void IfNoFailoverPair()
+    {
+        var config = TestConfig.Current;
+        IfNoServer(config.FailoverPrimaryServer, config.FailoverPrimaryPort);
+        IfNoServer(config.FailoverReplicaServer, config.FailoverReplicaPort);
+    }
+}
+
+/// <summary>
+/// Caches <see cref="TestConfig.IsServerRunning"/> 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.
+/// </summary>
+internal static class ServerProbe
+{
+    private static readonly ConcurrentDictionary<(string Host, int Port), Lazy<bool>> 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<bool>(
+                () => TestConfig.IsServerRunning(key.Host, key.Port),
+                LazyThreadSafetyMode.ExecutionAndPublication));
+        return probe.Value;
+    }
 }
 
 public class SkipTestException(string reason) : Exception(reason)

tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs

@@ -13,6 +13,19 @@ public static class TestConfig
 
     public static Config Current { get; }
 
+    /// <summary>
+    /// A floor, in milliseconds, for connection timeouts, from <c>REDIS_TESTS_MIN_TIMEOUT_MS</c>;
+    /// zero (the default) leaves the library's own defaults alone.
+    /// </summary>
+    /// <remarks>
+    /// 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.
+    /// </remarks>
+    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
         {

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)]