Stabilize Windows CI - #3168
Merged
Merged
Conversation
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Windows CI is runs on a potato. Maybe half a potato. One of the little ones. Additionally, it runs against an uncontrolled server (
apt-getwhatever is GA). The primary build is Ubuntu - we definitely want to retain Windows checks, but right not it is failing more times than not, with most builds taking 4+ runs, which is not ideal.The potato is a VM running Windows running WSL running Ubuntu running ¿16? Redis instances, many of which all chatter to each-other in spiderwebs (cluster with replicas, etc). It is ambitious and unrealistic to "fix" this.
So!
apt-getChecklist