Skip to content

Fix Windows CI flaps - #1147

Merged
mtmk merged 1 commit into
mainfrom
fix-windows-ci-flaps
May 15, 2026
Merged

Fix Windows CI flaps#1147
mtmk merged 1 commit into
mainfrom
fix-windows-ci-flaps

Conversation

@mtmk

@mtmk mtmk commented May 15, 2026

Copy link
Copy Markdown
Member

Cherry-pick of #1145 (against release/3.0) onto main, squashed into one commit. See that PR for the analysis and verification.

Fixes #1142.

Drop xunit MaxParallelThreads on Windows from 4.0x to 1.0x. With 4x parallelism on 4-core runners, simultaneous mock-server tests starved the thread pool and NatsReadProtocolProcessor.ReadLoopAsync didn't get scheduled, so SetupReaderWriterAsync timed out waiting for INFO.

Also bump ConnectTimeout to 30s on the six previously flapping tests, raise NatsUtils.ConnectRetryAsync budget 30s -> 60s, and widen TlsPreferTest's fallback to accept TimeoutException as evidence of a TLS attempt.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 1m 16s —— View job


PR Review: Fix Windows CI flaps

  • Gather context and read PR diff
  • Examine changed files for correctness and issues
  • Post review summary

Summary

This is a targeted, well-reasoned fix for Windows CI flakiness. All changes are confined to test infrastructure — no production code is touched.

Changes reviewed:

File Change Assessment
xunit.runsettings MaxParallelThreads 4.0x → 1.0x Correct fix for resource contention; will slow total CI runtime but stabilizes flaky tests
NatsUtils.cs ConnectRetryAsync default timeout 30s → 60s Safe; only affects how long tests wait for retry connections
5 test files Added explicit ConnectTimeout = TimeSpan.FromSeconds(30) Prevents default (likely short) timeout from causing spurious failures on loaded Windows runners
TlsPreferTest.cs Added TimeoutException to TLS-evidence exception types See note below

One item worth a second look:

In TlsPreferTest.cs (line 308), TimeoutException is added to the set of exceptions treated as evidence that the TLS path was attempted. The comment explains the reasoning well: a 30-second connect timeout expiring during the TLS handshake (after INFO was already read) implies the TLS upgrade path was entered. The existing clientSentConnect.Should().BeFalse() guard (line 286) rules out the false-positive scenario where plaintext CONNECT succeeded before the timeout. The logic is sound, but it does make this assertion slightly weaker — a TimeoutException before even reading INFO would also pass it. Given the MaxReconnectRetry = 0 setting and the controlled fake-server setup, this is unlikely to cause a false pass, but it's worth keeping an eye on.

No correctness bugs, security issues, or async/await problems found. The MaxParallelThreads reduction from 4.0x to 1.0x is the highest-impact change and will noticeably increase total CI wall-clock time. If CI time becomes a concern, a middle ground (e.g. 2.0x) could be revisited after confirming stability at 1.0x.

Looks good to merge.

@scottf scottf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mtmk
mtmk merged commit 85bc6d9 into main May 15, 2026
27 checks passed
@mtmk
mtmk deleted the fix-windows-ci-flaps branch May 15, 2026 13:46
This was referenced May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows CI flap on TlsPreferTest after merging #745 into release/3.0

2 participants