diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 80f33c9c3..b59c7729f 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: 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 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/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/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/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..bdf41a2c5 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/Skip.cs +++ b/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."); } + + /// + /// 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); + } +} + +/// +/// 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 +{ + 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( + () => TestConfig.IsServerRunning(key.Host, key.Port), + LazyThreadSafetyMode.ExecutionAndPublication)); + return probe.Value; + } } 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/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/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/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/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) => 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..c59039478 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -29,6 +29,30 @@ 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. + /// + /// + /// 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; @@ -452,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;