Fix Windows CI flaps - #1145
Conversation
Default ConnectTimeout (2s) is too tight on contended Windows runners. Bump to 30s in the tests that have flapped on Windows non-net8.0 (PingCancellationTest, Utf8SubjectMockServerTest, ProtocolParserSizeCheckTest, TlsPreferTest, PinnedClientTest.Pin_id_from_headers, CancellationTokenTests.NextAsync). Widen the TlsPreferTest exception fallback to also accept TimeoutException as evidence of a TLS attempt (otherwise a stalled handshake under contention slips through). Bump NatsUtils.ConnectRetryAsync overall budget 30s -> 60s for the same reason.
|
Claude finished @mtmk's task in 2m 46s —— View job PR Review
Test-only, no production source changes. Changes are correct and well-targeted.
|
| // A connect-timeout while the client was already past the INFO read is also evidence the | ||
| // TLS path was attempted: the plaintext path would have completed the handshake within the | ||
| // timeout, while a stalled TLS handshake under runner contention shows up as a timeout. | ||
| var tlsRelated = causes.Any(c => c is AuthenticationException || c is SocketException || c is IOException || c is TimeoutException); |
There was a problem hiding this comment.
The TimeoutException addition is a reasonable fallback, but carries a small theoretical false-positive: if the client times out before receiving INFO (e.g. the fake server is unexpectedly slow to accept the TCP connection), neither clientAttemptedTls nor clientSentConnect will be set, yet TimeoutException would satisfy this check and the test passes without ever having verified the TLS upgrade path.
In practice the risk is negligible because the server is on loopback and writes INFO immediately. But it's worth noting that clientSentConnect.Should().BeFalse() is the authoritative bug-guard here — this tlsRelated check is belt-and-suspenders secondary evidence.
No change needed; just flagging the reasoning so future readers understand the trade-off.
PR #1145 run 1 showed PingCancellation/Utf8Subject mock-server tests still hanging 75s+ even with bumped ConnectTimeout. Stack always at SetupReaderWriterAsync waiting for INFO — the NatsReadProtocolProcessor's Task.Run never gets a thread. Under 4.0x parallelism we run 16 tests at once on 4-core Windows runners; many doing TCP mock-servers at the same time starves the thread pool. Drop to 1.0x to remove the contention.
|
covered by #1147 |
Test-only fixes for the Windows non-net8.0 flaps tracked in #1142. Targets
release/3.0for verification; cherry-pick tomainafter merge.The main lever is
tests/xunit.runsettings:MaxParallelThreads4.0x -> 1.0x. With 4x parallelism on 4-core Windows runners, 16 tests run at once, each constructing aNatsConnectionwhoseNatsReadProtocolProcessor.ReadLoopAsyncis started viaTask.Run. Many simultaneous mock-server tests starved the thread pool, so the read loop didn't get scheduled andSetupReaderWriterAsynctimed out waiting forINFO. Serial execution removes the contention.Also bumped
ConnectTimeoutto 30s on the six previously flapping tests,NatsUtils.ConnectRetryAsyncbudget 30s -> 60s, and widenedTlsPreferTest's fallback to acceptTimeoutExceptionas evidence of a TLS attempt. Defense in depth; helpful if parallelism ever creeps back up.Trade-off: Windows CI per job is ~13-19 min, up from ~7 min.
Verified: 12 of 12 Windows jobs green across 3 runs.
Fixes #1142.