Fix flaky simulated-server tests at the root cause#4467
Conversation
Address the underlying causes of flakiness in the SimulatedServerTests suite instead of quarantining individual cases. TDS test harness (TDS.EndPoint / TDS.Servers): - Replace the polling accept loop (Pending() + Thread.Sleep) with a blocking AcceptTcpClient, and reorder Stop() to close the listener socket before joining the listener thread so the blocking accept is interrupted cleanly. - Remove the artificial 1s socket ReceiveTimeout that aborted TDS parsing mid-handshake under CI CPU load. - Deterministically join the per-connection processor task on Dispose (removing the "deadlock" TODO) so no background socket I/O or counter mutation outlives disposal. - Make transient-server request counters atomic (Interlocked). - Make the transient-delay sleep interruptible via a CancellationTokenSource so Dispose returns promptly even mid-delay. Tests: - ConnectionTimeout tests now connect to a held-open black-hole TcpListener that owns its port for the test lifetime, eliminating the ephemeral-port reuse race (which let a sibling server answer and the connection unexpectedly succeed). Timeout data values are capped to keep runtime low. - Disable pooling on simple non-pool connection tests to avoid leaking pooled connections to dead ephemeral ports. Pool-dependent tests (transient retry, failover pool-clear) intentionally keep pooling on. - Un-quarantine the tests fixed by the above: remove the flaky traits and stale failure-documentation comments.
There was a problem hiding this comment.
Pull request overview
This PR targets chronic flakiness in the SimulatedServerTests suite by fixing underlying defects in the TDS test harness (endpoint lifecycle, socket handling, counter concurrency) and updating tests to avoid port-reuse/pooling-related cross-test interference, then un-quarantining the affected tests.
Changes:
- Hardened the TDS endpoint/server harness (blocking accept, teardown ordering, task shutdown, cancellation-based delay interruption,
Interlockedcounters). - Updated simulated-server connection tests to disable pooling where not needed and replaced “dispose-to-timeout” with a held-open black-hole listener; reduced long timeout cases.
- Removed
[Trait("Category", "flaky")]markers and stale flakiness documentation from multiple tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs | Disables pooling for ephemeral-port tests; reworks connection-timeout tests to use a held-open black-hole listener; removes flaky quarantines/comments. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTestsAzure.cs | Un-quarantines a previously flaky routed-timeout test. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs | Un-quarantines routed delay/timeout tests and removes stale failure commentary. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs | Un-quarantines multiple failover scenarios and removes stale flakiness documentation. |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientTdsErrorTdsServer.cs | Makes request counting thread-safe via Interlocked. |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/TransientDelayTdsServer.cs | Makes delay interruptible on dispose and makes request counting thread-safe. |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs | Removes artificial socket receive timeout; switches to a background task; ensures dispose requests stop and waits for processor task. |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPoint.cs | Reorders shutdown to stop listener before join; replaces polling accept loop with blocking AcceptTcpClient. |
- TcpListener does not implement IDisposable on .NET Framework, so the black-hole
listener in the connection-timeout tests broke the net462 build (and the CodeQL
csharp analysis, which builds net462). Stop() the listener in a finally block
instead of using a `using` declaration.
- Re-quarantine three tests that still failed on net8/net9 CI legs after the
harness fixes (they are timing/MultiSubnetFailover-sensitive):
ConnectionTests.NetworkError_RetryEnabled_ShouldSucceed_Async and
ConnectionFailoverTests.NetworkError_WithUserProvidedPartner_{RetryDisabled,RetryEnabled}_ShouldConnectToFailoverPartner.
…rantine
- Failover tests (NetworkError_WithUserProvidedPartner_{RetryDisabled,RetryEnabled}_
ShouldConnectToFailoverPartner): the primary now uses a permanent (5 min,
interruptible) login delay so the client always times out on the primary and
fails over -- removing the delay-vs-timeout race. Assert on completed-login
counts (Login7Count: primary 0, failover 1) which are robust to abandoned
pre-login attempts, instead of raw/abandoned-adjusted pre-login math.
- MSF test (NetworkError_RetryEnabled_ShouldSucceed_Async): drop the
PreLoginCount > 1 assertion, which tested DNS/timing-dependent MultiSubnetFailover
fan-out rather than driver behavior; assert a completed pre-login instead.
- Remove the flaky traits from all three; the suite now has no quarantined tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs:172
- Dispose() waits up to 30s for the processor task, but it ignores the return value from Wait(timeout). If the wait ever times out, Dispose will return while the processor may still be running, which contradicts the comment about deterministic teardown and can reintroduce cross-test interference. Consider failing fast (or at least logging) when the processor does not stop within the bounded wait.
try
{
ProcessorTask?.Wait(TimeSpan.FromSeconds(30));
}
CI (net9/net10 Linux) surfaced two more MSF=true failures with the same root cause as the earlier ones: asserting `server.PreLoginCount > 1` tests the DNS/timing-dependent dual-stack fan-out of localhost, not driver behavior, and is non-deterministic across environments. Relax the MSF branch in NetworkDelay_RetryDisabled(_Async) and NetworkDelayAtRoutedLocation_RetryDisabled_ShouldSucceed to assert a completed pre-login (PreLoginCount - AbandonedPreLoginCount >= 1). The deterministic non-MSF branches are unchanged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:502
- ConnectionTimeoutTest starts the black-hole listener on IPv4 loopback (IPAddress.Loopback) but uses "localhost" in the DataSource. On dual-stack hosts, "localhost" may resolve to ::1 first; that can cause an immediate IPv6 connection failure and make the test pass without actually exercising the intended timeout-on-read behavior. Use an IPv4 literal to match the listener address so the test deterministically hits the black-hole socket.
var connStr = new SqlConnectionStringBuilder()
{
DataSource = $"localhost,{port}",
ConnectTimeout = timeout,
Encrypt = SqlConnectionEncryptOption.Optional,
Pooling = false, // Disable pooling so this expected timeout failure does not poison a shared pool
}.ConnectionString;
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:552
- ConnectionTimeoutTestAsync starts the black-hole listener on IPv4 loopback (IPAddress.Loopback) but uses "localhost" in the DataSource. On dual-stack hosts, "localhost" may resolve to ::1 first; that can cause an immediate IPv6 connection failure and make the test pass without exercising the intended timeout path. Use an IPv4 literal to match the listener address.
var connStr = new SqlConnectionStringBuilder()
{
DataSource = $"localhost,{port}",
ConnectTimeout = timeout,
Encrypt = SqlConnectionEncryptOption.Optional,
Pooling = false, // Disable pooling so this expected timeout failure does not poison a shared pool
}.ConnectionString;
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs:191
- RunConnectionHandler reads the Connection field after the task has been queued. Dispose() sets Connection=null before waiting, so there is a race where the task can start after Dispose and dereference a null TcpClient (connection.GetStream()), crashing the processor task and potentially skipping OnConnectionClosed (leaving disposed connections in the handler’s Connections list). Add a null check and still raise OnConnectionClosed to ensure cleanup.
private void RunConnectionHandler()
{
TcpClient connection = Connection;
try
{
// Get network stream
NetworkStream rawStream = connection.GetStream();
PrepareForProcessingData(rawStream);
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs:177
- Dispose() waits for ProcessorTask with a timeout but ignores the return value. If the wait times out, background socket I/O/counter mutation can still outlive Dispose(), which contradicts the comment and can reintroduce flakiness. Consider checking the boolean return from Wait(...) and logging when shutdown didn’t complete within the bound (or otherwise ensuring the task is fully stopped).
// Deterministically wait for the processor task to finish so that no
// background work (socket I/O, counter mutation) outlives Dispose().
// A bounded wait guards against an unexpected hang.
try
{
ProcessorTask?.Wait(TimeSpan.FromSeconds(30));
}
catch (AggregateException)
{
// Exceptions observed by the processor task are already logged in
// RunConnectionHandler; nothing else to do here.
}
RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysicalConnection(async: True) failed on a busy net462 CI leg after exactly 5s with "Timed out waiting for the second request to be denied by the rate limiter". Root cause: the pool's async open path uses Task.Run and calls the rate limiter's AttemptAcquire on a thread-pool thread (ChannelDbConnectionPool.cs). Caller A blocks a worker thread inside gated creation while holding the only permit, so on a busy/low-core agent thread-pool ramp-up delays caller B's dispatched permit attempt past the 5s SpinUntil, which asserts B's denial was recorded. Fix: pre-warm the worker-thread floor (SetMinThreads) so both dispatched bodies are scheduled promptly, removing the ramp-up delay. Raising the floor is benign and intentionally not restored so it cannot be lowered under parallel tests. Note: the other reported failure, MarsSessionPoolingTest.ExecuteReader_NoCloses, is an Azure-SQL integration test that is already quarantined ([Trait flaky]); its flakiness is server-side MARS session accounting and is not reproducible here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:498
- ConnectionTimeoutTest binds the black-hole TcpListener to IPv4 loopback (127.0.0.1) but connects via "localhost". On many hosts (especially Linux), localhost can resolve to ::1 first, causing an immediate connection refusal instead of the intended pre-login timeout and weakening the test.
DataSource = $"localhost,{port}",
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:548
- ConnectionTimeoutTestAsync binds the black-hole TcpListener to IPv4 loopback (127.0.0.1) but connects via "localhost". If localhost resolves to ::1 first, the connection can fail fast (connection refused) rather than timing out waiting for pre-login, reducing coverage and reintroducing flakiness across environments.
DataSource = $"localhost,{port}",
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs:173
- ServerEndPointConnection.Dispose() uses a bounded wait to avoid hangs, but it ignores the Wait() return value. If the processor task fails to exit within the timeout, background work can still outlive Dispose() (the flakiness root cause described in the PR) with no signal. Consider logging when the wait times out so failures are diagnosable.
try
{
ProcessorTask?.Wait(TimeSpan.FromSeconds(30));
}
catch (AggregateException)
- ConnectionTimeout tests: target 127.0.0.1 instead of "localhost" so the client always reaches the IPv4 black-hole listener (localhost may resolve to ::1 and give connection-refused instead of a pre-login timeout). The async test's CancellationTokenSource is now a safety net (ConnectTimeout + 30s) so the observed failure is the driver's timeout, not the token. - ConnectionFailoverTests: remove stale single-error-code Assert.Fail comment. - TDSServerEndPoint: dispose the accepted TcpClient if connection setup throws. - TDSServerEndPointConnection: log if the processor task does not finish within the 30s Dispose wait; null-guard Connection in the handler; suppress expected teardown-exception logging when a stop was requested. - TransientDelayTdsServer: make Dispose idempotent and dispose the CTS in finally.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.EndPoint/TDSServerEndPointConnection.cs:182
Dispose()waits onProcessorTask, which can create an underlying wait handle; since the task is no longer disposed (it used to be), this can unnecessarily hold unmanaged resources until GC. Consider disposing the task once it has completed (and nulling the reference) after the wait/catch block.
try
{
// Surface a hang: if the processor task does not complete within the bound,
// log it rather than silently returning while background work may continue.
if (ProcessorTask != null && !ProcessorTask.Wait(TimeSpan.FromSeconds(30)))
{
Log("Processor task did not complete within 30 seconds during Dispose.");
}
}
catch (AggregateException)
{
// Exceptions observed by the processor task are already logged in
// RunConnectionHandler; nothing else to do here.
}
mdaigle
left a comment
There was a problem hiding this comment.
The results look promising 🚀
| { | ||
| // WaitHandle.WaitOne returns immediately once the token is cancelled; | ||
| // otherwise it blocks for the full delay duration. | ||
| _disposeCts.Token.WaitHandle.WaitOne(Arguments.DelayDuration); |
There was a problem hiding this comment.
Interesting! I didn't know about the WaitHandle property.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4467 +/- ##
==========================================
- Coverage 72.25% 63.79% -8.46%
==========================================
Files 292 283 -9
Lines 44029 66821 +22792
==========================================
+ Hits 31813 42629 +10816
- Misses 12216 24192 +11976
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TransientFault_NoFailover_DoesNotClearPool and TransientFault_WithUserProvidedPartner_Pooling_ShouldNotClearPool_NotFailover failed on a busy Azure CI leg with an off-by-one connection.DataSource (Expected "localhost,<primary>", Actual "localhost,<primary-1>" = the failover partner's port). Neither reproduces locally (0/40 stress iterations). The off-by-one means the connection intermittently fails over to the partner on a login-phase transient error instead of retrying the primary -- the failover-alternation / parser-state timing behavior these tests were written to guard. That is driver behavior under load, not a harness race, so it cannot be made deterministic at the test level, and forcing it green would mask a possible real race. Re-quarantining (they were flaky-tagged before this PR's sweep) and noting it as a candidate for a separate driver-behavior investigation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs:558
- ConnectionTimeoutTestAsync has the same issue as the sync variant:
ConnectRetryCountdefaults to 1, which can extend the elapsed time if a timeout is considered transient and retried. Disable connect retry here as well so the test measures the driver’s ownConnectTimeout.
var connStr = new SqlConnectionStringBuilder()
{
// Target 127.0.0.1 explicitly (not "localhost") so the client always
// connects to the IPv4 black-hole listener above rather than resolving to
// ::1, which would produce connection-refused instead of a pre-login timeout.
DataSource = $"127.0.0.1,{port}",
ConnectTimeout = timeout,
Encrypt = SqlConnectionEncryptOption.Optional,
Pooling = false, // Disable pooling so this expected timeout failure does not poison a shared pool
}.ConnectionString;
| var connStr = new SqlConnectionStringBuilder() | ||
| { | ||
| // Target 127.0.0.1 explicitly (not "localhost") so the client always | ||
| // connects to the IPv4 black-hole listener above rather than resolving to | ||
| // ::1, which would produce connection-refused instead of a pre-login timeout. | ||
| DataSource = $"127.0.0.1,{port}", | ||
| ConnectTimeout = timeout, | ||
| Encrypt = SqlConnectionEncryptOption.Optional, | ||
| Pooling = false, // Disable pooling so this expected timeout failure does not poison a shared pool | ||
| }.ConnectionString; |
…in Release - ConnectionTimeout tests: set ConnectRetryCount = 0 so a single timeout attempt is made and the default connection-resiliency retry cannot extend the wall clock past the "timeout + 3s" assertion (review r3637533345). - AlwaysEncrypted TestSqlCommandCancellationToken: quarantine only for non-DEBUG builds. Its determinism failpoint (Thread.Sleep in SqlCommand.Encryption.cs) is compiled under DEBUG only; in Release the injected pause is gone and the fixed CancelAfter window races a real, network-latency-dependent AE query, so the exact-message assertion flakes. Under DEBUG it stays deterministic and gating.
Description
The
SimulatedServerTestssuite has been chronically flaky, and the recent trend has been to quarantine individual[InlineData]cases with[Trait("Category", "flaky")]. Investigation of the failing CI runs showed the failures are not individually flaky tests — they cluster around shared test-harness defects, and the tests that fail are frequently not the ones tagged flaky (their near-identical siblings are). This PR fixes the underlying causes and un-quarantines the tests that are addressed.Root causes fixed
TDS test harness (
TDS.EndPoint/TDS.Servers)"Timer must be stopped.", off-by-one port assertions). The timeout tests now connect to a held-open black-holeTcpListenerthat owns its port for the test lifetime.ReceiveTimeout. The per-connection socket had a hard 1-second receive timeout that aborted TDS parsing mid-handshake under CPU load, producingWin32Exception: The wait operation timed outand post-login timeout failures. Removed; a blocked read is now unblocked deterministically by socket close on dispose.Pending()+Thread.Sleep(10)with a blockingAcceptTcpClient, and reorderedStop()to close the listener socket before joining the listener thread so the blocking accept is interrupted cleanly.Dispose()previously did not join the background processor task (a known "deadlock" TODO), so socket I/O and counter mutation could continue after a test asserted, corrupting prelogin/login counts (Expected 2, Actual 3).Dispose()now deterministically joins the task (bounded wait).Interlocked.Thread.Sleepis now an interruptible wait cancelled onDispose, so teardown returns promptly even mid-delay (instead of blocking up to the delay duration once the task is joined).Tests
[InlineData]values (a real 60s timeout adds 60s for no extra coverage now that the timeout genuinely fires).flakytraits and the stale failure-documentation comments.