From c9ab202d0c7dd1b89d64d9d6cb47dc22cdbdc274 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 12:03:43 -0700 Subject: [PATCH 01/15] Add background pool warmup spec Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 93 +++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 specs/003-pool-warmup/spec.md diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md new file mode 100644 index 0000000000..adba9df34a --- /dev/null +++ b/specs/003-pool-warmup/spec.md @@ -0,0 +1,93 @@ +# Feature Specification: Background Pool Warmup + +**Feature Branch**: `dev/mdaigle/pool-warmup` (branched from `dev/mdaigle/pool-channel-rate-limiting`) +**Created**: 2026-07-16 +**Status**: Draft + +## Description + +Implement background pool warmup for `ChannelDbConnectionPool`. When the pool starts, it asynchronously pre-creates connections up to Min Pool Size in the background, serially (one at a time), through the shared rate-limiting mechanism used by user-initiated requests. + +Whenever the pool count drops below Min Pool Size for any reason (pruning, connection destruction, idle timeout expiry), the pool automatically triggers replenishment using the same serial, rate-limited creation path. + +## User Scenarios & Testing + +### User Story 1 — Background Warmup on Pool Creation (P1) + +When a pool is created with Min Pool Size > 0, it pre-creates connections in the background so early application requests find ready-to-use connections. + +**Acceptance Scenarios**: + +1. **Given** a pool is created with minimum pool size of N, **When** startup completes, **Then** the pool begins creating connections in the background up to N. +2. **Given** a pool is warming up, **When** a user opens a connection before warmup finishes, **Then** the user request is served immediately without waiting for warmup to complete. +3. **Given** a pool is warming up, **When** warmup creates each connection, **Then** each connection is created one at a time (serially), not in parallel. + +--- + +### User Story 2 — Warmup Through Shared Rate Limiter (P1) + +Warmup creates connections through the same rate-limiting mechanism as user-initiated requests, preventing warmup from overwhelming the server. + +**Acceptance Scenarios**: + +1. **Given** warmup is creating connections, **When** a user request also needs a new connection, **Then** both warmup and user requests compete fairly through the shared rate limiter. +2. **Given** the rate limiter is at capacity, **When** warmup attempts to create the next connection, **Then** warmup waits its turn rather than bypassing the limiter. + +--- + +### User Story 3 — Warmup Failure Resilience (P1) + +If a connection fails to open during warmup, the failure is silently absorbed. The pool remains fully operational — user requests create connections on demand as needed. Warmup failures never trigger the pool-level error state. + +**Acceptance Scenarios**: + +1. **Given** warmup is in progress, **When** a connection fails to open, **Then** the failure is traced/logged but not propagated as an exception. +2. **Given** warmup fails for all connections, **When** a user subsequently opens a connection, **Then** the user request creates a connection on demand and succeeds normally. +3. **Given** warmup fails, **When** the pool's error state is checked, **Then** the pool is NOT in error state — warmup failures do not trigger the pool-level blocking-period mechanism. + +--- + +### User Story 4 — Warmup Cancellation on Shutdown (P2) + +When a pool is shut down while warmup is still in progress, warmup stops promptly. No new connections are created after shutdown begins. + +**Acceptance Scenarios**: + +1. **Given** warmup is in progress, **When** the pool is shut down, **Then** warmup stops and no further connections are created. +2. **Given** warmup is in progress, **When** the pool is shut down, **Then** any connections already created by warmup are cleaned up as part of normal shutdown. + +--- + +### User Story 5 — Replenishment on Any Below-Minimum Event (P2) + +Whenever the pool count drops below Min Pool Size for any reason, the pool automatically triggers warmup-style replenishment to restore to the minimum. + +**Acceptance Scenarios**: + +1. **Given** pruning has reduced the pool below the minimum pool size, **When** the pruning cycle completes, **Then** the pool queues a replenishment request. +2. **Given** a connection is destroyed on return to the pool (broken, lifetime expired), **When** the pool count drops below the minimum, **Then** the pool queues a replenishment request. +3. **Given** idle timeout expiry removes a connection, **When** the pool count drops below minimum, **Then** the pool queues a replenishment request. +4. **Given** replenishment is triggered, **When** connections are created, **Then** they are created serially through the shared rate limiter, identical to initial warmup. +5. **Given** the pool is at or above the minimum pool size, **When** a connection is destroyed, **Then** no replenishment is triggered. + +## Implementation Notes + +- Warmup starts automatically when the pool's `Startup()` is called. +- Uses async connection creation — no blocking the caller, no sync-over-async. +- Creates connections serially (one at a time) through the shared rate limiter. +- Warmup failures are traced/logged but do not propagate or trigger error state. +- Warmup stops if the pool is cleared (generation change) to avoid creating stale-generation connections. +- Concurrent warmup/replenishment requests are coalesced — only one warmup loop executes at a time. +- Warmup is a no-op when Min Pool Size = 0. + +## Acceptance Criteria + +- When a pool starts with Min Pool Size > 0, background warmup begins immediately. +- After warmup completes, the pool contains Min Pool Size idle connections. +- Warmup uses async I/O throughout — no sync-over-async. +- Warmup connections are created serially through the shared rate limiter. +- Warmup errors are traced/logged and do not crash the application or trigger error state. +- If the pool is shut down or cleared during warmup, warmup stops gracefully. +- After any event that reduces the pool below Min Pool Size, the pool restores to minimum within one warmup pass. +- Concurrent warmup/replenishment requests are coalesced so only one warmup loop executes at a time. +- Unit tests validate warmup behavior for various Min Pool Size values (0, 1, N) and all replenishment triggers. From 2edbdbb540a4545ed6ec3efac04382ff1a512b47 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 12:23:08 -0700 Subject: [PATCH 02/15] Implement background pool warmup and replenishment for ChannelDbConnectionPool Adds serial, rate-limited background warmup that pre-creates connections up to Min Pool Size on Startup and replenishes whenever the pool drops below the minimum (destroy-on-return, idle-timeout eviction, pruning). Warmup failures are absorbed without triggering the pool error state, warmup is coalesced to a single loop, cancels promptly on shutdown, and stops on generation change. Implements specs/003-pool-warmup/spec.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 300 ++++++++++- .../ChannelDbConnectionPoolWarmupTest.cs | 473 ++++++++++++++++++ 2 files changed, 761 insertions(+), 12 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 298c38b9db..fc00675f8e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -127,6 +127,28 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// this pool group. See . /// private readonly BlockingPeriodErrorState? _errorState; + + /// + /// Cancels in-flight background warmup/replenishment when the pool shuts down so no new + /// connections are created after shutdown begins (Story 4). The token is also passed to + /// physical connection creation so a blocked open is unwound promptly on shutdown. + /// + private readonly CancellationTokenSource _warmupCts = new(); + + /// + /// Coalescing guard ensuring only one warmup loop executes at a time. Transitioned + /// 0 -> 1 via by the first + /// requester to start the loop, and reset to 0 by the loop when it drains. + /// + private int _warmupLoopRunning; + + /// + /// Set to 1 by whenever warmup/replenishment is needed and + /// drained to 0 by the running loop before each pass. Lets requests that arrive while a + /// pass is executing coalesce into a single follow-up pass rather than spawning parallel + /// loops. + /// + private int _warmupRequested; #endregion /// @@ -349,6 +371,20 @@ public void Shutdown() // routes returning connections to RemoveConnection. State = ShuttingDown; + // Cancel any in-flight background warmup/replenishment so no new connections are + // created after shutdown begins (Story 4). The warmup loop also observes the + // State transition above, but cancelling here unwinds a create that is currently + // blocked and stops the loop promptly. Cancel is non-throwing and idempotent. + try + { + _warmupCts.Cancel(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, _warmupCts.Cancel threw, continuing shutdown: {1}", Id, ex); + } + // Each cleanup step is independent and best-effort. A failure in one step must not // prevent later steps from running, otherwise the pool can be left half-shut-down // (e.g. timer disposed but channel never completed -> waiters stuck forever). @@ -422,6 +458,18 @@ public void Shutdown() " {0}, RemoveConnection threw during drain, continuing: {1}", Id, ex); } } + + // Release the warmup cancellation source now that the loop has been signalled to stop + // and no further connections will be created. Dispose is best-effort and idempotent. + try + { + _warmupCts.Dispose(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, _warmupCts.Dispose threw, continuing shutdown: {1}", Id, ex); + } } /// @@ -432,14 +480,16 @@ public void Shutdown() /// public void Startup() { - // Startup is currently a no-op for this pool: State is set to Running in the - // constructor, and PoolPruner (when present, i.e. MinPoolSize < MaxPoolSize) is - // also constructed eagerly there; its timer arms/disarms via UpdateTimer() calls - // from OpenNewInternalConnection and RemoveConnection as the pool grows/shrinks. - // This method exists as the symmetrical counterpart of Shutdown and as a hook - // for future warmup behavior. + // State is set to Running in the constructor, and PoolPruner (when present, i.e. + // MinPoolSize < MaxPoolSize) is also constructed eagerly there; its timer arms/disarms + // via UpdateTimer() calls from OpenNewInternalConnection and RemoveConnection as the + // pool grows/shrinks. SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}", Id); + + // Kick off background warmup so the pool pre-creates connections up to MinPoolSize + // without blocking the caller (Story 1). No-op when MinPoolSize == 0. + RequestWarmup(); } /// @@ -557,6 +607,12 @@ public bool TryGetConnection( /// The cancellation token to cancel the operation. /// The overall timeout budget. Passed through to the physical connection /// so it uses the remaining budget rather than starting a fresh timeout. + /// When true, the call originates from background warmup/replenishment + /// rather than a user request. Warmup deliberately does not interact with the pool-level + /// blocking-period error state: it neither fast-fails when the state is active, nor enters + /// the state on failure, nor clears it on success. A warmup creation failure is traced and + /// absorbed (null is returned) instead of propagating, so warmup can never crash the + /// application or poison the pool for user requests (Story 3). /// The new internal connection, or null if the pool has no available slot or the /// rate limiter is currently saturated. In the latter case the caller should fall back to /// the idle-channel wait; the rate limiter will write a null to the idle channel when a @@ -567,12 +623,17 @@ public bool TryGetConnection( private DbConnectionInternal? OpenNewInternalConnection( DbConnection? owningConnection, CancellationToken cancellationToken, - TimeoutTimer timeout) + TimeoutTimer timeout, + bool isWarmup = false) { cancellationToken.ThrowIfCancellationRequested(); - // Fast-fail in the error state. FR-006. - _errorState?.ThrowIfActive(); + // Fast-fail in the error state for user requests. FR-006. Warmup never fast-fails on + // the blocking-period state: it is best-effort and must not observe or mutate it. + if (!isWarmup) + { + _errorState?.ThrowIfActive(); + } try { @@ -688,15 +749,29 @@ _connectionCreationRateLimiter is not null && // start the pruning timer so idle connections can be reclaimed. Pruner?.UpdateTimer(); - // A successful creation clears error/backoff state - // FR-009. - _errorState?.Clear(); + // A successful creation clears error/backoff state (FR-009). Warmup does not + // interact with the blocking-period state at all, so it neither clears nor + // enters it. + if (!isWarmup) + { + _errorState?.Clear(); + } } return connection; } catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) { + // Warmup failures are silently absorbed: trace and return null instead of + // propagating, and never enter the blocking-period state (Story 3). The pool + // remains fully operational and user requests continue to create on demand. + if (isWarmup) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Warmup connection creation failed, absorbing: {1}", Id, ex); + return null; + } + // Enter the blocking period error state on creation failure if configured. // We deliberately exclude OperationCanceledException: that is thrown when the // caller's own timeout/cancellation budget expires while waiting, which is @@ -770,6 +845,14 @@ private void RemoveConnection(DbConnectionInternal connection) // If this removal brought us back to MinPoolSize, disable the pruning timer. Pruner?.UpdateTimer(); + + // Any removal that drops the pool below MinPoolSize triggers replenishment through the + // shared serial, rate-limited warmup path (Story 5). This is the single choke point for + // every below-minimum event: connections destroyed on return (broken/lifetime), idle + // timeout eviction, and pruning all funnel through RemoveConnection. RequestWarmup is a + // no-op when MinPoolSize == 0, the pool is not Running, or a warmup loop is already + // running/queued, so calling it unconditionally here is cheap. + RequestWarmup(); } /// @@ -962,6 +1045,199 @@ private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection } #endregion + #region Warmup + /// + /// Delay before retrying a warmup creation that was denied by the shared rate limiter, so + /// warmup waits its turn instead of bypassing the limiter or spinning (Story 2). + /// + private static readonly TimeSpan s_warmupRateLimitRetryDelay = TimeSpan.FromMilliseconds(50); + + /// + /// Requests background warmup/replenishment: the pool asynchronously pre-creates connections + /// up to , serially and through the shared rate limiter. Safe to + /// call from any thread and from hot paths; it is cheap and non-blocking. + /// + /// This is the single entry point for every warmup trigger: pool startup and any event that + /// drops the pool below (connection destruction on return, idle + /// timeout eviction, pruning). Concurrent requests are coalesced so that only one warmup + /// loop ever runs at a time (implementation note: warmup coalescing). + /// + private void RequestWarmup() + { + // No-op when there is nothing to pre-create (MinPoolSize == 0), the pool is not running, + // or shutdown has cancelled background activity. + if (MinPoolSize == 0 || State != Running || _warmupCts.IsCancellationRequested) + { + return; + } + + // Record that (more) warmup is needed. A loop that is already running observes this and + // executes an additional pass; see RunWarmupLoopAsync. + Volatile.Write(ref _warmupRequested, 1); + + TryStartWarmupLoop(); + } + + /// + /// Starts the warmup loop if one is not already running. The single-loop guard + /// () coalesces concurrent requests: the loser of the CAS + /// leaves its request flag set for the winning loop to pick up. + /// + private void TryStartWarmupLoop() + { + if (Interlocked.CompareExchange(ref _warmupLoopRunning, 1, 0) != 0) + { + return; + } + + // Run on the thread pool so warmup never blocks the caller (Startup or a returning + // connection). Fire-and-forget: the loop absorbs its own exceptions and always releases + // the single-loop guard on exit. + _ = Task.Run(RunWarmupLoopAsync); + } + + /// + /// The coalesced warmup loop. Runs warmup passes as long as requests keep arriving, then + /// releases the single-loop guard. Absorbs all exceptions so a warmup failure can never + /// surface as an unhandled exception on the thread pool (Story 3). + /// + private async Task RunWarmupLoopAsync() + { + CancellationToken token; + try + { + token = _warmupCts.Token; + } + catch (ObjectDisposedException) + { + // The pool shut down and disposed the cancellation source before this loop started. + // There is nothing to warm up; release the guard and exit. + Volatile.Write(ref _warmupLoopRunning, 0); + return; + } + + try + { + do + { + // Drain the request flag before the pass so a request that arrives during the + // pass re-arms it and schedules a follow-up pass rather than being lost. + Volatile.Write(ref _warmupRequested, 0); + + await WarmupPassAsync(token).ConfigureAwait(false); + } + while (Volatile.Read(ref _warmupRequested) == 1 && + State == Running && + !token.IsCancellationRequested); + } + catch (Exception ex) + { + // Defense in depth: WarmupPassAsync already absorbs per-connection failures, but the + // loop itself must never throw onto the thread pool. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Warmup loop failed, absorbing: {1}", Id, ex); + } + finally + { + // Release the single-loop guard. + Volatile.Write(ref _warmupLoopRunning, 0); + + // Close the race where a request arrived after the loop condition read + // _warmupRequested but before we released the guard: if a request is still pending + // and the pool is running, restart the loop. + if (Volatile.Read(ref _warmupRequested) == 1 && + State == Running && + !token.IsCancellationRequested) + { + TryStartWarmupLoop(); + } + } + } + + /// + /// Executes a single warmup pass: creates connections one at a time (serially) until the + /// pool reaches or a stop condition is hit. Each creation goes + /// through the same slot-reservation and rate-limited path as user requests + /// ( with isWarmup: true), and freshly created + /// connections are published to the idle channel so waiting or future user requests can use + /// them. All per-connection failures are absorbed (Story 3). + /// + /// Cancelled on shutdown to stop warmup promptly (Story 4). + private async Task WarmupPassAsync(CancellationToken token) + { + while (State == Running && + !token.IsCancellationRequested && + Count < MinPoolSize) + { + // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since warmup + // has no owning Open() call to inherit a budget from. Matches the replenishment + // behavior of the legacy WaitHandle pool. + TimeoutTimer timeout = TimeoutTimer.StartNew( + TimeSpan.FromMilliseconds(PoolGroupOptions.CreationTimeout)); + + DbConnectionInternal? connection; + try + { + // owningConnection is null: warmup connections are created unattached and enter + // the pool as idle. isWarmup suppresses all blocking-period error-state + // interaction and converts creation failures into an absorbed null. + connection = OpenNewInternalConnection( + owningConnection: null, + cancellationToken: token, + timeout: timeout, + isWarmup: true); + } + catch (OperationCanceledException) + { + // Shutdown/cancellation unwound the in-flight create. Stop warming up. + break; + } + + if (connection is null) + { + // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), so a + // null return means the shared rate limiter is currently saturated. Wait our + // turn rather than bypassing it (Story 2), then retry. Capacity frees up as user + // requests release their permits. + try + { + await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + break; + } + + continue; + } + + // If a Clear happened while we were creating, the connection now belongs to a + // previous generation. Don't pool a stale connection (implementation note: warmup + // stops on generation change). Destroying it re-triggers warmup for the current + // generation via RemoveConnection. + if (connection.ClearGeneration != _clearGeneration) + { + RemoveConnection(connection); + break; + } + + // Publish the freshly created connection as idle. It was PrePush'd at creation + // (CreatePooledConnection) and never activated, so it is in the correct state to + // enter the idle channel directly. + if (!_idleChannel.TryWrite(connection)) + { + // Channel completed (pool shutting down). Destroy instead of pooling. + RemoveConnection(connection); + break; + } + + // Yield between creations so warmup stays cooperative and responsive to + // cancellation. There is no sync-over-async anywhere in this path. + await Task.Yield(); + } + } + #endregion + #region Pruning /// /// Manages idle connection pruning. Null when the pool is fixed-size (MinPoolSize >= MaxPoolSize) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs new file mode 100644 index 0000000000..3f9c9660cb --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -0,0 +1,473 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Data.Common; +using System.Threading; +using System.Threading.RateLimiting; +using System.Threading.Tasks; +using System.Transactions; +using Microsoft.Data.Common; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Unit tests for background pool warmup and replenishment in + /// . See specs/003-pool-warmup/spec.md. + /// + public class ChannelDbConnectionPoolWarmupTest + { + private const int DefaultTimeoutMs = 5000; + + #region Helpers + + private static ChannelDbConnectionPool ConstructPool( + SqlConnectionFactory connectionFactory, + int minPoolSize = 0, + int maxPoolSize = 50, + int idleTimeout = 0, + int loadBalanceTimeout = 0, + ConcurrencyLimiter? connectionCreationRateLimiter = null) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: 15, + loadBalanceTimeout: loadBalanceTimeout, + hasTransactionAffinity: true, + idleTimeout: idleTimeout); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + + return new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter); + } + + /// + /// Spins until is true or the timeout elapses. Warmup runs on + /// background tasks, so tests observe its effects by polling pool counters. + /// + private static bool WaitFor(Func condition, int timeoutMs = DefaultTimeoutMs) + => SpinWait.SpinUntil(condition, timeoutMs); + + /// + /// Checks out a single connection from the pool synchronously. + /// + private static DbConnectionInternal CheckOut(ChannelDbConnectionPool pool) + { + bool completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + Assert.True(completed); + Assert.NotNull(connection); + return connection!; + } + + #endregion + + #region Story 1 - Background warmup on pool creation + + // Warmup is a no-op when MinPoolSize == 0: no connections are ever created in the background. + [Fact] + public void Startup_MinPoolSizeZero_DoesNotWarmUp() + { + var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 0, maxPoolSize: 10); + + pool.Startup(); + + // Give any (erroneous) background warmup a chance to run before asserting it did not. + Assert.False( + WaitFor(() => factory.CreateCount > 0, timeoutMs: 500), + "Warmup created a connection even though MinPoolSize is 0."); + Assert.Equal(0, pool.Count); + } + + // Warmup pre-creates connections up to MinPoolSize for various sizes (1 and N). + [Theory] + [InlineData(1)] + [InlineData(3)] + [InlineData(10)] + public void Startup_WithMinPoolSize_WarmsUpToMinimum(int minPoolSize) + { + var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: minPoolSize, maxPoolSize: minPoolSize + 10); + + pool.Startup(); + + Assert.True( + WaitFor(() => pool.Count >= minPoolSize), + $"Warmup did not reach MinPoolSize; Count={pool.Count}, expected {minPoolSize}."); + + // Warmup must not overshoot the minimum. + Assert.False( + WaitFor(() => pool.Count > minPoolSize, timeoutMs: 500), + $"Warmup overshot MinPoolSize; Count={pool.Count}."); + Assert.Equal(minPoolSize, pool.Count); + Assert.Equal(minPoolSize, pool.IdleCount); + Assert.Equal(minPoolSize, factory.CreateCount); + } + + // A user request during warmup is served immediately and does not wait for warmup to finish. + // The gated factory blocks warmup's first (serial) creation, yet a user open still completes + // by creating its own connection, proving warmup does not block user requests and does not + // create connections in parallel. + [Fact] + public async Task Startup_UserRequestDuringWarmup_ServedImmediately() + { + using var createGate = new ManualResetEventSlim(initialState: false); + var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + + // Warmup begins and its single, serial creation blocks on the gate. + pool.Startup(); + Assert.True( + factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + "Timed out waiting for warmup to begin its first creation."); + + // A user request runs while warmup is still blocked. It must complete promptly by + // creating its own (second) connection rather than waiting for warmup. + Task userOpen = Task.Run(() => CheckOut(pool)); + Task completed = await Task.WhenAny(userOpen, Task.Delay(TimeSpan.FromSeconds(5))); + Assert.True(completed == userOpen, "User request was blocked behind warmup."); + Assert.NotNull(await userOpen); + + // Warmup is still parked on its single in-flight creation (serial, not parallel): only + // warmup's first creation (count 1) and the user's creation (count 2) have occurred. + Assert.Equal(2, factory.CreateCount); + + createGate.Set(); + } + + #endregion + + #region Story 2 - Warmup through the shared rate limiter + + // Warmup creates connections through the shared rate limiter and still reaches MinPoolSize. + [Fact] + public void Warmup_ThroughSharedRateLimiter_ReachesMinimum() + { + using var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); + using var pool = ConstructPool( + factory, + minPoolSize: 3, + maxPoolSize: 10, + connectionCreationRateLimiter: rateLimiter); + + pool.Startup(); + + Assert.True( + WaitFor(() => pool.Count >= 3), + $"Warmup did not reach MinPoolSize through the rate limiter; Count={pool.Count}."); + Assert.Equal(3, pool.Count); + // Every warmup creation acquired (and released) a permit from the shared limiter. + Assert.True(rateLimiter.GetStatistics()!.TotalSuccessfulLeases >= 3); + } + + // When the shared rate limiter is saturated by a user request, warmup waits its turn rather + // than bypassing the limiter: the user request that arrives while warmup holds the only + // permit is denied by the same limiter. + [Fact] + public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() + { + using var createGate = new ManualResetEventSlim(initialState: false); + var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); + using var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + using var pool = ConstructPool( + factory, + minPoolSize: 2, + maxPoolSize: 5, + connectionCreationRateLimiter: rateLimiter); + + // Warmup begins, acquires the only permit, and blocks in creation while holding it. + pool.Startup(); + Assert.True( + factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + "Timed out waiting for warmup to begin its first creation."); + + // A concurrent user request competes for the same limiter and is denied a permit, + // proving warmup and user requests share one limiter (warmup did not bypass it). + long failedBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; + var tcs = new TaskCompletionSource(); + pool.TryGetConnection(new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _); + + Assert.True( + WaitFor(() => rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore), + "User request was not denied by the shared rate limiter while warmup held the permit."); + + // Release warmup; the pool must still converge to MinPoolSize and the user request + // completes once capacity frees up. + createGate.Set(); + Assert.True(WaitFor(() => pool.Count >= 2), $"Pool did not reach MinPoolSize; Count={pool.Count}."); + + DbConnectionInternal? userConnection = await tcs.Task; + Assert.NotNull(userConnection); + } + + #endregion + + #region Story 3 - Warmup failure resilience + + // Warmup creation failures are absorbed: no exception surfaces, the pool stays empty, and the + // pool is NOT put into the blocking-period error state. + [Fact] + public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() + { + var factory = new WarmupFailingConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + + pool.Startup(); + + // Let warmup attempt (and fail) its creations. + Assert.True( + WaitFor(() => factory.WarmupAttemptCount >= 1), + "Warmup never attempted a creation."); + + // Pool never accumulates idle connections, but nothing crashes and the pool is not in + // error state. (Count reflects in-flight reservations transiently, so IdleCount is the + // reliable measure of successfully pooled connections.) + Assert.False( + WaitFor(() => pool.IdleCount > 0, timeoutMs: 500), + "A warmup connection was pooled despite all creations failing."); + Assert.Equal(0, pool.IdleCount); + Assert.False(pool.ErrorOccurred, "Warmup failures must not trigger the pool error state."); + } + + // After warmup fails, a subsequent user request creates a connection on demand and succeeds. + // The factory fails only warmup creations (owning connection is null) and succeeds for user + // requests, so the two paths are cleanly separated. + [Fact] + public void Warmup_Fails_UserRequestStillSucceeds() + { + var factory = new WarmupFailingConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); + + pool.Startup(); + Assert.True(WaitFor(() => factory.WarmupAttemptCount >= 1), "Warmup never attempted a creation."); + + // A real user request (non-null owning connection) succeeds on demand. + DbConnectionInternal connection = CheckOut(pool); + Assert.NotNull(connection); + Assert.False(pool.ErrorOccurred); + } + + #endregion + + #region Story 4 - Warmup cancellation on shutdown + + // When the pool is shut down mid-warmup, warmup stops promptly: no connections created after + // shutdown begins, and any in-flight connection is cleaned up. + [Fact] + public void Shutdown_DuringWarmup_StopsAndCleansUp() + { + using var createGate = new ManualResetEventSlim(initialState: false); + var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + + // Warmup begins and blocks on its first serial creation. + pool.Startup(); + Assert.True( + factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + "Timed out waiting for warmup to begin its first creation."); + + // Shut down while warmup is parked. This cancels warmup and completes the idle channel. + pool.Shutdown(); + + // Release the in-flight creation; it must be cleaned up, not pooled. + createGate.Set(); + + // No further warmup creations happen after shutdown, and nothing lingers in the pool. + Assert.True( + WaitFor(() => pool.Count == 0), + $"In-flight warmup connection was not cleaned up on shutdown; Count={pool.Count}."); + Assert.False( + WaitFor(() => factory.CreateCount > 1, timeoutMs: 500), + $"Warmup created additional connections after shutdown; CreateCount={factory.CreateCount}."); + Assert.Equal(1, factory.CreateCount); + } + + #endregion + + #region Story 5 - Replenishment on any below-minimum event + + // Destroying a connection on return (non-poolable/broken) drops the pool below MinPoolSize and + // triggers replenishment back to the minimum. + [Fact] + public void Replenish_AfterDoomedReturn_RefillsToMinimum() + { + var factory = new DoomableConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); + + pool.Startup(); + Assert.True(WaitFor(() => pool.Count >= 2), $"Initial warmup failed; Count={pool.Count}."); + + // Check out a connection, mark it non-poolable, and return it: it is destroyed on return. + var owner = new SqlConnection(); + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection); + Assert.NotNull(connection); + ((DoomableStubConnection)connection!).MarkDoNotPool(); + pool.ReturnInternalConnection(connection, owner); + + // Replenishment restores the pool to MinPoolSize. + Assert.True( + WaitFor(() => pool.Count >= 2), + $"Pool did not replenish to MinPoolSize after a doomed return; Count={pool.Count}."); + Assert.Equal(2, pool.Count); + } + + // Clearing the pool drops it to zero (below MinPoolSize) and triggers replenishment back to + // the minimum with fresh-generation connections. + [Fact] + public void Replenish_AfterClear_RefillsToMinimum() + { + var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + + pool.Startup(); + Assert.True(WaitFor(() => pool.Count >= 3), $"Initial warmup failed; Count={pool.Count}."); + + pool.Clear(); + + Assert.True( + WaitFor(() => pool.Count >= 3), + $"Pool did not replenish to MinPoolSize after Clear; Count={pool.Count}."); + Assert.Equal(3, pool.Count); + } + + // Destroying a connection while still at/above the minimum does NOT trigger replenishment + // beyond the minimum: the pool stays at MinPoolSize. + [Fact] + public void Replenish_DestroyWhileAtMinimum_NoOvershoot() + { + var factory = new DoomableConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); + + pool.Startup(); + Assert.True(WaitFor(() => pool.Count >= 2), $"Initial warmup failed; Count={pool.Count}."); + + // Grow above the minimum. Two checkouts reuse the two idle warmup connections; a third + // finds no idle connection and creates a new one, bringing the total to 3. + var owner1 = new SqlConnection(); + var owner2 = new SqlConnection(); + var owner3 = new SqlConnection(); + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c2); + pool.TryGetConnection(owner3, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? c3); + Assert.NotNull(c1); + Assert.NotNull(c2); + Assert.NotNull(c3); + Assert.True(WaitFor(() => pool.Count == 3), $"Expected pool to grow to 3; Count={pool.Count}."); + + // Destroy the third on return. Dropping from 3 to 2 stays at the minimum, so no + // replenishment overshoot occurs. + ((DoomableStubConnection)c3!).MarkDoNotPool(); + pool.ReturnInternalConnection(c3, owner3); + + Assert.True(WaitFor(() => pool.Count == 2), $"Pool did not settle at MinPoolSize; Count={pool.Count}."); + Assert.False( + WaitFor(() => pool.Count > 2, timeoutMs: 500), + $"Pool overshot MinPoolSize after a destroy at the minimum; Count={pool.Count}."); + Assert.Equal(2, pool.Count); + } + + #endregion + + #region Test doubles + + /// + /// A stub internal connection that can be marked non-poolable so a return destroys it. + /// + private sealed class DoomableStubConnection : DbConnectionInternal + { + internal void MarkDoNotPool() => DoNotPoolThisConnection(); + + public override string ServerVersion => throw new NotImplementedException(); + + public override ConnectionCapabilities Capabilities => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + => throw new NotImplementedException(); + + public override void EnlistTransaction(Transaction transaction) + { + } + + protected override void Activate(Transaction transaction) + { + } + + protected override void Deactivate() + { + } + + internal override void ResetConnection() + { + } + } + + /// + /// Produces instances so replenishment tests can force a + /// connection to be destroyed on return. + /// + private sealed class DoomableConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + => new DoomableStubConnection(); + } + + /// + /// Fails only warmup creations (identified by a null owning connection) and succeeds for real + /// user requests. Lets tests assert that warmup failures are absorbed while on-demand user + /// creation still works. + /// + private sealed class WarmupFailingConnectionFactory : SqlConnectionFactory + { + private int _warmupAttemptCount; + + internal int WarmupAttemptCount => Volatile.Read(ref _warmupAttemptCount); + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (owningConnection is null) + { + Interlocked.Increment(ref _warmupAttemptCount); + throw ADP.PooledOpenTimeout(); + } + + return new DoomableStubConnection(); + } + } + + #endregion + } +} From b4549e669388f949540a20c587f0d7947e8f47c8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 10:09:50 -0700 Subject: [PATCH 03/15] Address review: cheap warmup fast-path and distinguish rate-limit from failure - RequestWarmup() now returns early when Count >= MinPoolSize, so hot-path callers (e.g. every RemoveConnection) don't schedule a thread-pool work item just to run a pass that immediately exits. - OpenNewInternalConnection(isWarmup: true) now rethrows genuine creation failures (still without entering the blocking-period error state) instead of absorbing them as null. A null return now unambiguously means the rate limiter is saturated. WarmupPassAsync catches real failures, traces, and stops the pass rather than retrying on a 50ms cadence, avoiding a noisy spin when opens persistently fail. - Strengthen the failure test to assert warmup does not spin on persistent creation failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 52 ++++++++++++++----- .../ChannelDbConnectionPoolWarmupTest.cs | 10 ++++ 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index fc00675f8e..51029b55aa 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -610,9 +610,9 @@ public bool TryGetConnection( /// When true, the call originates from background warmup/replenishment /// rather than a user request. Warmup deliberately does not interact with the pool-level /// blocking-period error state: it neither fast-fails when the state is active, nor enters - /// the state on failure, nor clears it on success. A warmup creation failure is traced and - /// absorbed (null is returned) instead of propagating, so warmup can never crash the - /// application or poison the pool for user requests (Story 3). + /// the state on failure, nor clears it on success. A warmup creation failure still throws + /// (without entering the error state) so the caller can distinguish it from a rate-limit + /// backoff, which is signalled by a null return; see (Story 3). /// The new internal connection, or null if the pool has no available slot or the /// rate limiter is currently saturated. In the latter case the caller should fall back to /// the idle-channel wait; the rate limiter will write a null to the idle channel when a @@ -762,14 +762,14 @@ _connectionCreationRateLimiter is not null && } catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) { - // Warmup failures are silently absorbed: trace and return null instead of - // propagating, and never enter the blocking-period state (Story 3). The pool - // remains fully operational and user requests continue to create on demand. + // Warmup deliberately does not enter the blocking-period state on failure (Story 3): + // a warmup creation failure must never poison the pool for user requests. We still + // rethrow so the warmup caller can distinguish a genuine creation failure from a + // rate-limit backoff (which is signalled by a null return, not an exception) and + // apply the right policy. WarmupPassAsync traces and absorbs the exception there. if (isWarmup) { - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Warmup connection creation failed, absorbing: {1}", Id, ex); - return null; + throw; } // Enter the blocking period error state on creation failure if configured. @@ -1071,6 +1071,16 @@ private void RequestWarmup() return; } + // Fast path: the pool is already at or above the minimum, so there is nothing to warm + // up. Return before touching the request flag or scheduling a thread-pool work item. + // This keeps hot-path callers (e.g. RemoveConnection on every return) cheap. The check + // is best-effort under concurrency; a below-minimum condition missed here is still + // observed by WarmupPassAsync's loop guard on the next trigger. + if (Count >= MinPoolSize) + { + return; + } + // Record that (more) warmup is needed. A loop that is already running observes this and // executes an additional pass; see RunWarmupLoopAsync. Volatile.Write(ref _warmupRequested, 1); @@ -1180,7 +1190,8 @@ private async Task WarmupPassAsync(CancellationToken token) { // owningConnection is null: warmup connections are created unattached and enter // the pool as idle. isWarmup suppresses all blocking-period error-state - // interaction and converts creation failures into an absorbed null. + // interaction. A null return means the shared rate limiter was saturated; a + // thrown exception means the physical connection open genuinely failed. connection = OpenNewInternalConnection( owningConnection: null, cancellationToken: token, @@ -1192,13 +1203,26 @@ private async Task WarmupPassAsync(CancellationToken token) // Shutdown/cancellation unwound the in-flight create. Stop warming up. break; } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) + { + // A genuine connection-open failure (not rate limiting). Trace and absorb it, + // then stop this pass rather than retrying on a tight cadence: if opens are + // failing, hammering the server every few milliseconds would be wasteful and + // noisy. The pool stays fully operational and user requests continue to create + // on demand; the next below-minimum trigger (or a returning connection) will + // re-request warmup and try again (Story 3). + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Warmup connection creation failed, stopping pass: {1}", Id, ex); + break; + } if (connection is null) { - // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), so a - // null return means the shared rate limiter is currently saturated. Wait our - // turn rather than bypassing it (Story 2), then retry. Capacity frees up as user - // requests release their permits. + // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), and + // creation failures throw rather than return null, so a null return means the + // shared rate limiter is currently saturated. Wait our turn rather than + // bypassing it (Story 2), then retry. Capacity frees up as user requests release + // their permits. try { await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 3f9c9660cb..86fbaed34b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -249,6 +249,16 @@ public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() "A warmup connection was pooled despite all creations failing."); Assert.Equal(0, pool.IdleCount); Assert.False(pool.ErrorOccurred, "Warmup failures must not trigger the pool error state."); + + // A genuine creation failure stops the warmup pass instead of retrying on a tight + // cadence, so the failing factory is not hammered. Without a fresh below-minimum trigger + // the attempt count stays bounded (a single failed pass), rather than spinning up dozens + // of attempts against a server that is failing to accept connections. + int attemptsAfterFailure = factory.WarmupAttemptCount; + Thread.Sleep(300); + Assert.True( + factory.WarmupAttemptCount <= attemptsAfterFailure + 1, + $"Warmup spun on a persistent failure: {factory.WarmupAttemptCount} attempts observed."); } // After warmup fails, a subsequent user request creates a connection on demand and succeeds. From 8299ab9f3e1592dbbf52a994e5342ea0b119ea41 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 10:16:39 -0700 Subject: [PATCH 04/15] Address review: quiet expected disposal in Shutdown, assert async request contract - Shutdown now treats ObjectDisposedException from _warmupCts.Cancel()/Dispose() as an expected no-op instead of tracing it as a failure, so it never emits misleading trace noise. (Shutdown is already guarded idempotent via the _shutdownInitiated CAS, but this hardens the per-step cleanup.) - Warmup_RateLimiterSaturated_UserSharesSameLimiter now asserts the queued async TryGetConnection call returns false with no inline connection, making the async-request contract explicit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 10 ++++++++++ .../ChannelDbConnectionPoolWarmupTest.cs | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 51029b55aa..9b61d7e1c6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -379,6 +379,11 @@ public void Shutdown() { _warmupCts.Cancel(); } + catch (ObjectDisposedException) + { + // Expected no-op: the cancellation source is already disposed, so there is nothing + // left to cancel. This is not a failure, so it is not traced. + } catch (Exception ex) { SqlClientEventSource.Log.TryPoolerTraceEvent( @@ -465,6 +470,11 @@ public void Shutdown() { _warmupCts.Dispose(); } + catch (ObjectDisposedException) + { + // Expected no-op: the cancellation source is already disposed. Dispose is documented + // as safe to call repeatedly, so this is not a failure and is not traced. + } catch (Exception ex) { SqlClientEventSource.Log.TryPoolerTraceEvent( diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 86fbaed34b..1b6370f060 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -207,7 +207,14 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() // proving warmup and user requests share one limiter (warmup did not bypass it). long failedBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; var tcs = new TaskCompletionSource(); - pool.TryGetConnection(new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _); + bool completedSynchronously = pool.TryGetConnection( + new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? immediateConnection); + + // The async request cannot be satisfied inline: there is no idle connection and the + // shared limiter's only permit is held by warmup. TryGetConnection therefore returns + // false with no connection, deferring the result to the TaskCompletionSource. + Assert.False(completedSynchronously, "Async request unexpectedly completed synchronously."); + Assert.Null(immediateConnection); Assert.True( WaitFor(() => rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore), From e27e0c6f0bc4211b3d13254197326fdb465cdf52 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 10:22:18 -0700 Subject: [PATCH 05/15] Address review: null-safe owning connection in factory, fix warmup cancellation docs - SqlConnectionFactory.CreateConnection: guard the User Instance path's write to sqlOwningConnection._applyTransientFaultHandling with a null check. Warmup creates connections with a null owning connection, which would otherwise NullReferenceException for 'User Instance=true' connection strings. Matches the null-safe reads already present earlier in the method. - Correct the _warmupCts field and Shutdown comments: the token stops the warmup loop at its await points / pre-create checks, but does not abort an in-progress synchronous physical open (the factory does not yet accept a cancellation token), so the docs no longer claim behavior the implementation doesn't provide. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 12 ++++++++---- .../Microsoft/Data/SqlClient/SqlConnectionFactory.cs | 10 ++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9b61d7e1c6..e52ae21d1b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -130,8 +130,11 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// /// Cancels in-flight background warmup/replenishment when the pool shuts down so no new - /// connections are created after shutdown begins (Story 4). The token is also passed to - /// physical connection creation so a blocked open is unwound promptly on shutdown. + /// connections are created after shutdown begins (Story 4). The token is observed at the + /// warmup loop's await points and before each creation attempt, so it stops the loop and + /// prevents further attempts promptly. It cannot abort an already in-progress physical open: + /// that path is synchronous and the connection factory does not yet accept a cancellation + /// token (see the TODO in ). /// private readonly CancellationTokenSource _warmupCts = new(); @@ -373,8 +376,9 @@ public void Shutdown() // Cancel any in-flight background warmup/replenishment so no new connections are // created after shutdown begins (Story 4). The warmup loop also observes the - // State transition above, but cancelling here unwinds a create that is currently - // blocked and stops the loop promptly. Cancel is non-throwing and idempotent. + // State transition above; cancelling here stops the loop promptly by tripping its + // await points and pre-create checks. It does not abort an already in-progress + // synchronous physical open (see _warmupCts field docs). Cancel is idempotent. try { _warmupCts.Cancel(); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs index 3aa89f5332..8840864958 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs @@ -650,8 +650,14 @@ protected virtual DbConnectionInternal CreateConnection( // used below to connect to the SQL Express User Instance. instanceName = sseConnection.InstanceName; - // Set future transient fault handling based on connection options - sqlOwningConnection._applyTransientFaultHandling = opt != null && opt.ConnectRetryCount > 0; + // Set future transient fault handling based on connection options. + // sqlOwningConnection is null for background warmup/replenishment creations, + // which have no owning connection to carry this state forward; only propagate + // it when an owning connection is present (matches the null-safe reads above). + if (sqlOwningConnection != null) + { + sqlOwningConnection._applyTransientFaultHandling = opt != null && opt.ConnectRetryCount > 0; + } if (!instanceName.StartsWith(@"\\.\", StringComparison.Ordinal)) { From d050a04011ffd79dd0bd71325a84947e432785dd Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 10:34:11 -0700 Subject: [PATCH 06/15] Address review: align spec async wording with implementation Warmup runs on a background task (never blocking callers, no sync-over-async in the loop), but the physical connection open is still synchronous because the factory does not yet expose an async open. Reword the implementation note and acceptance criterion so the spec no longer claims 'async I/O throughout'. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index adba9df34a..9661058a29 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -73,7 +73,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom ## Implementation Notes - Warmup starts automatically when the pool's `Startup()` is called. -- Uses async connection creation — no blocking the caller, no sync-over-async. +- Warmup runs on a background task, so it never blocks the caller (`Startup`/connection return) and uses no sync-over-async in the loop. The physical connection open itself is currently synchronous (executed on the background task); the connection factory does not yet expose an async open. - Creates connections serially (one at a time) through the shared rate limiter. - Warmup failures are traced/logged but do not propagate or trigger error state. - Warmup stops if the pool is cleared (generation change) to avoid creating stale-generation connections. @@ -84,7 +84,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - When a pool starts with Min Pool Size > 0, background warmup begins immediately. - After warmup completes, the pool contains Min Pool Size idle connections. -- Warmup uses async I/O throughout — no sync-over-async. +- Warmup runs on a background task without blocking callers and without sync-over-async in the loop (the physical open is currently synchronous, executed on the background task, pending async-open support in the factory). - Warmup connections are created serially through the shared rate limiter. - Warmup errors are traced/logged and do not crash the application or trigger error state. - If the pool is shut down or cleared during warmup, warmup stops gracefully. From 97babec233d3221c9d14d8c8e0e64d3b242c6c06 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 11:18:10 -0700 Subject: [PATCH 07/15] Make warmup handshake tests deterministic via a scheduler seam Startup_UserRequestDuringWarmup_ServedImmediately flaked on CI, timing out while waiting for warmup's first background creation. Root cause is thread-pool scheduling latency: warmup runs on a queued work item whose first physical create blocks a worker on the test gate, and under xUnit parallel execution the shared pool can be momentarily starved, delaying the work item past the wait. Rather than mutating global ThreadPool.SetMinThreads (which would perturb the environment other parallel tests share), add a narrow, opt-in test seam: the pool constructor now accepts an optional warmupLoopScheduler that controls how the warmup loop is launched. Production is unchanged (defaults to Task.Run); the warmup tests inject a scheduler that runs the loop on a dedicated thread, so warmup's gated first create can never be delayed by thread-pool starvation. This mirrors the existing timeProvider test-injection precedent on the constructor. Also replace the 5s handshake waits with a generous 30s upper bound (only fires on a genuine hang) and run the user request on a dedicated thread so the CreateCount==2 seriality assertion is deterministic. Applied to the three gated tests that share the handshake primitive. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 25 +++++- .../ChannelDbConnectionPoolWarmupTest.cs | 85 +++++++++++++++---- 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index e52ae21d1b..6b8ae90968 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -152,6 +152,15 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// loops. /// private int _warmupRequested; + + /// + /// Optional test seam controlling how the warmup loop is launched. When null (production), + /// the loop runs via on the thread pool. Tests inject a + /// scheduler that runs the loop on a dedicated thread so warmup's first (synchronous, + /// possibly gated) physical open cannot be delayed by thread-pool starvation, keeping + /// timing-sensitive assertions deterministic without mutating global thread-pool state. + /// + private readonly Action>? _warmupLoopScheduler; #endregion /// @@ -163,7 +172,8 @@ internal ChannelDbConnectionPool( DbConnectionPoolIdentity identity, DbConnectionPoolProviderInfo connectionPoolProviderInfo, ConcurrencyLimiter? connectionCreationRateLimiter = null, - TimeProvider? timeProvider = null) + TimeProvider? timeProvider = null, + Action>? warmupLoopScheduler = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; @@ -174,6 +184,7 @@ internal ChannelDbConnectionPool( MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); TransactedConnectionPool = new(this); _connectionCreationRateLimiter = connectionCreationRateLimiter; + _warmupLoopScheduler = warmupLoopScheduler; _connectionSlots = new(MaxPoolSize); _idleChannel = new(); @@ -1116,8 +1127,16 @@ private void TryStartWarmupLoop() // Run on the thread pool so warmup never blocks the caller (Startup or a returning // connection). Fire-and-forget: the loop absorbs its own exceptions and always releases - // the single-loop guard on exit. - _ = Task.Run(RunWarmupLoopAsync); + // the single-loop guard on exit. Tests may supply an alternate scheduler (e.g. a + // dedicated thread) so warmup startup is not subject to thread-pool scheduling latency. + if (_warmupLoopScheduler is not null) + { + _warmupLoopScheduler(RunWarmupLoopAsync); + } + else + { + _ = Task.Run(RunWarmupLoopAsync); + } } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 1b6370f060..8e00c7c0c2 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -32,7 +32,8 @@ private static ChannelDbConnectionPool ConstructPool( int maxPoolSize = 50, int idleTimeout = 0, int loadBalanceTimeout = 0, - ConcurrencyLimiter? connectionCreationRateLimiter = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null, + Action>? warmupLoopScheduler = null) { var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -53,7 +54,9 @@ private static ChannelDbConnectionPool ConstructPool( dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter); + connectionCreationRateLimiter, + timeProvider: null, + warmupLoopScheduler: warmupLoopScheduler); } /// @@ -78,6 +81,35 @@ private static DbConnectionInternal CheckOut(ChannelDbConnectionPool pool) return connection!; } + /// + /// Generous upper bound for waits that guard a background handshake which, in correct code, + /// completes in milliseconds. It is only ever hit on a genuine failure/hang, so it does not + /// make passing runs slow or introduce timing races. + /// + private static readonly TimeSpan HandshakeTimeout = TimeSpan.FromSeconds(30); + + /// + /// Runs the warmup loop on a dedicated foreground-of-its-own background thread rather than a + /// shared thread-pool worker. + /// + /// The gated test factory blocks warmup's first physical create synchronously. In production + /// warmup runs via Task.Run, so that blocking would occupy a thread-pool worker; under + /// xUnit's parallel execution the shared pool can be momentarily starved and the warmup work + /// item may not be scheduled for seconds, making handshake-based assertions flaky. Giving the + /// loop its own thread removes that dependency on thread-pool scheduling entirely - without + /// mutating any global thread-pool configuration that other tests share. + /// + /// + private static readonly Action> DedicatedThreadWarmupScheduler = loop => + { + var thread = new Thread(() => loop().GetAwaiter().GetResult()) + { + IsBackground = true, + Name = "WarmupTest.WarmupLoop", + }; + thread.Start(); + }; + #endregion #region Story 1 - Background warmup on pool creation @@ -132,23 +164,43 @@ public async Task Startup_UserRequestDuringWarmup_ServedImmediately() { using var createGate = new ManualResetEventSlim(initialState: false); var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); - using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + // Run warmup on its own thread so its gated first create can't be delayed by thread-pool + // starvation, making the handshake below deterministic. + using var pool = ConstructPool( + factory, minPoolSize: 3, maxPoolSize: 10, + warmupLoopScheduler: DedicatedThreadWarmupScheduler); - // Warmup begins and its single, serial creation blocks on the gate. + // Warmup begins and its single, serial creation blocks on the gate. Waiting on this + // explicit signal (not a sleep) establishes a deterministic happens-before: warmup is now + // parked inside its first create. pool.Startup(); Assert.True( - factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + factory.FirstCreateStarted.Wait(HandshakeTimeout), "Timed out waiting for warmup to begin its first creation."); - // A user request runs while warmup is still blocked. It must complete promptly by - // creating its own (second) connection rather than waiting for warmup. - Task userOpen = Task.Run(() => CheckOut(pool)); - Task completed = await Task.WhenAny(userOpen, Task.Delay(TimeSpan.FromSeconds(5))); + // Exactly one creation - warmup's - is in flight and parked on the closed gate. The + // warmup loop is serial, so it cannot advance to a second create until the gate opens. + Assert.Equal(1, factory.CreateCount); + + // Issue a user request while warmup is parked. Run it on a dedicated (LongRunning) thread + // so it never competes with warmup for the same pool worker. Because warmup does not block + // user requests, it must complete promptly by creating its own connection. A generous + // upper bound converts a hypothetical "user blocked behind warmup" regression into a clean + // failure instead of an indefinite hang; correct code completes in milliseconds. + Task userOpen = Task.Factory.StartNew( + () => CheckOut(pool), + CancellationToken.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + + Task completed = await Task.WhenAny(userOpen, Task.Delay(HandshakeTimeout)); Assert.True(completed == userOpen, "User request was blocked behind warmup."); Assert.NotNull(await userOpen); - // Warmup is still parked on its single in-flight creation (serial, not parallel): only - // warmup's first creation (count 1) and the user's creation (count 2) have occurred. + // Seriality: warmup is still parked on its single in-flight create (the gate is closed), + // so the only additional creation is the user's. CreateCount == 2 proves warmup did not + // create connections in parallel. This is deterministic: nothing can advance warmup's + // loop until the gate is set below. Assert.Equal(2, factory.CreateCount); createGate.Set(); @@ -195,12 +247,13 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() factory, minPoolSize: 2, maxPoolSize: 5, - connectionCreationRateLimiter: rateLimiter); + connectionCreationRateLimiter: rateLimiter, + warmupLoopScheduler: DedicatedThreadWarmupScheduler); // Warmup begins, acquires the only permit, and blocks in creation while holding it. pool.Startup(); Assert.True( - factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + factory.FirstCreateStarted.Wait(HandshakeTimeout), "Timed out waiting for warmup to begin its first creation."); // A concurrent user request competes for the same limiter and is denied a permit, @@ -297,12 +350,14 @@ public void Shutdown_DuringWarmup_StopsAndCleansUp() { using var createGate = new ManualResetEventSlim(initialState: false); var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); - using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); + using var pool = ConstructPool( + factory, minPoolSize: 3, maxPoolSize: 10, + warmupLoopScheduler: DedicatedThreadWarmupScheduler); // Warmup begins and blocks on its first serial creation. pool.Startup(); Assert.True( - factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + factory.FirstCreateStarted.Wait(HandshakeTimeout), "Timed out waiting for warmup to begin its first creation."); // Shut down while warmup is parked. This cancels warmup and completes the idle channel. From cd93d5e4474d6bc60f682a0b200a1ca1eb6535d6 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 12:08:03 -0700 Subject: [PATCH 08/15] Address review: warmup respects error state, condense loop, harden scheduling - Warmup now respects the pool's blocking-period error state (mirroring the WaitHandle pool): the loop stands down while ErrorOccurred is set, though it still never enters or clears that state. Added CanWarmup(token) capturing the running/cancellation/error-state guard, plus a unit test that drives the pool into the error state while warmup is parked mid-create and asserts warmup stops instead of replenishing. Updated the spec (Story 3 scenario 4). - Condensed the warmup region: merged RunWarmupLoopAsync and WarmupPassAsync into a single method and factored the repeated loop guard into CanWarmup, reducing fragmentation. - Removed the explicit stale-generation check in the warmup pass; a Clear that races an in-flight create is handled by the existing lazy liveness/generation check on retrieval. Updated the spec note accordingly. - Hardened TryStartWarmupLoop: if scheduling the loop throws (injected scheduler or Task.Run), release the single-loop guard and trace so warmup isn't pinned off for the life of the pool. - Converted test-method comments to XML docs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 5 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 275 +++++++++--------- .../ChannelDbConnectionPoolWarmupTest.cs | 153 ++++++++-- 3 files changed, 272 insertions(+), 161 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index 9661058a29..c409346c3d 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -44,6 +44,7 @@ If a connection fails to open during warmup, the failure is silently absorbed. T 1. **Given** warmup is in progress, **When** a connection fails to open, **Then** the failure is traced/logged but not propagated as an exception. 2. **Given** warmup fails for all connections, **When** a user subsequently opens a connection, **Then** the user request creates a connection on demand and succeeds normally. 3. **Given** warmup fails, **When** the pool's error state is checked, **Then** the pool is NOT in error state — warmup failures do not trigger the pool-level blocking-period mechanism. +4. **Given** the pool is already in the blocking-period error state (driven there by failing user requests), **When** warmup would otherwise replenish, **Then** warmup stands down while the error state is active rather than piling more doomed opens onto a struggling server. Warmup respects the error state but never enters or clears it (mirroring the legacy WaitHandle pool). --- @@ -75,8 +76,8 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Warmup starts automatically when the pool's `Startup()` is called. - Warmup runs on a background task, so it never blocks the caller (`Startup`/connection return) and uses no sync-over-async in the loop. The physical connection open itself is currently synchronous (executed on the background task); the connection factory does not yet expose an async open. - Creates connections serially (one at a time) through the shared rate limiter. -- Warmup failures are traced/logged but do not propagate or trigger error state. -- Warmup stops if the pool is cleared (generation change) to avoid creating stale-generation connections. +- Warmup failures are traced/logged but do not propagate or trigger error state. Warmup does, however, respect an already-active blocking-period error state and stands down while it is active. +- If a `Clear` races with an in-flight warmup creation, the freshly created (now stale-generation) connection is not special-cased by warmup; it is harmlessly discarded by the liveness/generation check on its next retrieval. After a `Clear`, replenishment refills the pool to the minimum with fresh-generation connections. - Concurrent warmup/replenishment requests are coalesced — only one warmup loop executes at a time. - Warmup is a no-op when Min Pool Size = 0. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 6b8ae90968..ee46d2bdab 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -633,11 +633,14 @@ public bool TryGetConnection( /// The overall timeout budget. Passed through to the physical connection /// so it uses the remaining budget rather than starting a fresh timeout. /// When true, the call originates from background warmup/replenishment - /// rather than a user request. Warmup deliberately does not interact with the pool-level - /// blocking-period error state: it neither fast-fails when the state is active, nor enters - /// the state on failure, nor clears it on success. A warmup creation failure still throws - /// (without entering the error state) so the caller can distinguish it from a rate-limit - /// backoff, which is signalled by a null return; see (Story 3). + /// rather than a user request. Warmup does not enter the pool-level blocking-period error + /// state on failure, nor clear it on success - a best-effort warmup must never poison the + /// pool for user requests, nor mask a real fault by clearing it. Warmup does, however, + /// respect the error state: the warmup loop stands down while it is active (see + /// ), mirroring the legacy WaitHandle pool. A warmup creation failure + /// still throws (without entering the error state) so the caller can distinguish it from a + /// rate-limit backoff, which is signalled by a null return; see + /// (Story 3). /// The new internal connection, or null if the pool has no available slot or the /// rate limiter is currently saturated. In the latter case the caller should fall back to /// the idle-channel wait; the rate limiter will write a null to the idle channel when a @@ -653,8 +656,10 @@ public bool TryGetConnection( { cancellationToken.ThrowIfCancellationRequested(); - // Fast-fail in the error state for user requests. FR-006. Warmup never fast-fails on - // the blocking-period state: it is best-effort and must not observe or mutate it. + // Fast-fail in the error state for user requests. FR-006. Warmup does not fast-fail here + // (it neither reads nor mutates the blocking-period state at creation time); instead the + // warmup loop stands down while the error state is active via CanWarmup, so it does not + // reach this path while blocking. if (!isWarmup) { _errorState?.ThrowIfActive(); @@ -775,8 +780,8 @@ _connectionCreationRateLimiter is not null && Pruner?.UpdateTimer(); // A successful creation clears error/backoff state (FR-009). Warmup does not - // interact with the blocking-period state at all, so it neither clears nor - // enters it. + // enter or clear the blocking-period state (though it does stand down while the + // state is active; see CanWarmup), so it must not clear it on success either. if (!isWarmup) { _errorState?.Clear(); @@ -791,7 +796,7 @@ _connectionCreationRateLimiter is not null && // a warmup creation failure must never poison the pool for user requests. We still // rethrow so the warmup caller can distinguish a genuine creation failure from a // rate-limit backoff (which is signalled by a null return, not an exception) and - // apply the right policy. WarmupPassAsync traces and absorbs the exception there. + // apply the right policy. RunWarmupLoopAsync traces and absorbs the exception there. if (isWarmup) { throw; @@ -1114,9 +1119,22 @@ private void RequestWarmup() } /// - /// Starts the warmup loop if one is not already running. The single-loop guard + /// True while warmup/replenishment is permitted to keep creating connections: the pool is + /// running, shutdown has not cancelled background work, and the pool is not in the + /// blocking-period error state. Warmup does not enter or clear the error state, but it does + /// respect it - while user requests are failing and the pool is blocking, warmup stands down + /// rather than piling more doomed opens onto a struggling server. This mirrors the legacy + /// WaitHandle pool, which skips replenishment while ErrorOccurred is set. + /// + private bool CanWarmup(CancellationToken token) + => State == Running && !token.IsCancellationRequested && !ErrorOccurred; + + /// + /// Starts the coalesced warmup loop if one is not already running. The single-loop guard /// () coalesces concurrent requests: the loser of the CAS - /// leaves its request flag set for the winning loop to pick up. + /// leaves its request flag set for the winning loop to pick up. Runs on the thread pool so + /// warmup never blocks the caller (Startup or a returning connection); tests may supply an + /// alternate scheduler so warmup startup is not subject to thread-pool scheduling latency. /// private void TryStartWarmupLoop() { @@ -1125,24 +1143,38 @@ private void TryStartWarmupLoop() return; } - // Run on the thread pool so warmup never blocks the caller (Startup or a returning - // connection). Fire-and-forget: the loop absorbs its own exceptions and always releases - // the single-loop guard on exit. Tests may supply an alternate scheduler (e.g. a - // dedicated thread) so warmup startup is not subject to thread-pool scheduling latency. - if (_warmupLoopScheduler is not null) + try { - _warmupLoopScheduler(RunWarmupLoopAsync); + // Fire-and-forget: the loop absorbs its own exceptions and always releases the + // single-loop guard on exit. + if (_warmupLoopScheduler is not null) + { + _warmupLoopScheduler(RunWarmupLoopAsync); + } + else + { + _ = Task.Run(RunWarmupLoopAsync); + } } - else + catch (Exception ex) { - _ = Task.Run(RunWarmupLoopAsync); + // Scheduling the loop failed (e.g. an injected scheduler threw, or the thread pool + // refused the work item). Release the guard so warmup isn't permanently pinned off + // for the life of the pool; the next below-minimum trigger will try again. + Volatile.Write(ref _warmupLoopRunning, 0); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Failed to schedule warmup loop, absorbing: {1}", Id, ex); } } /// - /// The coalesced warmup loop. Runs warmup passes as long as requests keep arriving, then - /// releases the single-loop guard. Absorbs all exceptions so a warmup failure can never - /// surface as an unhandled exception on the thread pool (Story 3). + /// The coalesced warmup loop: creates connections one at a time (serially) up to + /// , re-running as long as new requests keep arriving, then releases + /// the single-loop guard. Each creation goes through the same slot-reservation and + /// rate-limited path as user requests ( with + /// isWarmup: true), and freshly created connections are published to the idle channel + /// for waiting or future user requests. All failures are absorbed so warmup can never surface + /// an unhandled exception or poison the pool (Story 3). /// private async Task RunWarmupLoopAsync() { @@ -1163,136 +1195,107 @@ private async Task RunWarmupLoopAsync() { do { - // Drain the request flag before the pass so a request that arrives during the - // pass re-arms it and schedules a follow-up pass rather than being lost. + // Drain the request flag before each pass so a request that arrives mid-pass + // re-arms it and schedules a follow-up pass rather than being lost. Volatile.Write(ref _warmupRequested, 0); - await WarmupPassAsync(token).ConfigureAwait(false); + while (CanWarmup(token) && Count < MinPoolSize) + { + // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since + // warmup has no owning Open() call to inherit a budget from. Matches the + // replenishment behavior of the legacy WaitHandle pool. + TimeoutTimer timeout = TimeoutTimer.StartNew( + TimeSpan.FromMilliseconds(PoolGroupOptions.CreationTimeout)); + + DbConnectionInternal? connection; + try + { + // owningConnection is null: warmup connections are created unattached and + // enter the pool as idle. A null return means the shared rate limiter was + // saturated; a thrown exception means the physical open genuinely failed. + connection = OpenNewInternalConnection( + owningConnection: null, + cancellationToken: token, + timeout: timeout, + isWarmup: true); + } + catch (OperationCanceledException) + { + // Shutdown/cancellation unwound the in-flight create. Stop warming up. + break; + } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) + { + // A genuine connection-open failure (not rate limiting). Trace and absorb + // it, then stop this pass rather than retrying on a tight cadence: if opens + // are failing, hammering the server every few milliseconds would be + // wasteful and noisy. The pool stays fully operational and user requests + // continue to create on demand; the next below-minimum trigger will + // re-request warmup and try again (Story 3). + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Warmup connection creation failed, stopping pass: {1}", Id, ex); + break; + } + + if (connection is null) + { + // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), + // and creation failures throw rather than return null, so a null return + // means the shared rate limiter is currently saturated. Wait our turn + // rather than bypassing it (Story 2), then retry. Capacity frees up as user + // requests release their permits. + try + { + await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + break; + } + + continue; + } + + // Publish the freshly created connection as idle. It was PrePush'd at creation + // (CreatePooledConnection) and never activated, so it is in the correct state + // to enter the idle channel directly. If a Clear raced and bumped the + // generation, the stale connection is harmlessly removed by IsLiveConnection + // on its next retrieval, so we don't check the generation here. + if (!_idleChannel.TryWrite(connection)) + { + // Channel completed (pool shutting down). Destroy instead of pooling. + RemoveConnection(connection); + break; + } + + // OpenNewInternalConnection is synchronous and blocks the loop's thread for + // the duration of the physical open. Yield between creations so a multi- + // connection warmup returns its thread-pool worker to the scheduler between + // opens (rather than monopolizing one worker for the whole sequence) and stays + // responsive to cancellation. There is no sync-over-async anywhere in this path. + await Task.Yield(); + } } - while (Volatile.Read(ref _warmupRequested) == 1 && - State == Running && - !token.IsCancellationRequested); + while (Volatile.Read(ref _warmupRequested) == 1 && CanWarmup(token)); } catch (Exception ex) { - // Defense in depth: WarmupPassAsync already absorbs per-connection failures, but the - // loop itself must never throw onto the thread pool. + // Defense in depth: the loop must never throw onto the thread pool. SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Warmup loop failed, absorbing: {1}", Id, ex); } finally { - // Release the single-loop guard. + // Release the single-loop guard, then close the race where a request arrived after + // the loop condition was read but before we released the guard: if a request is still + // pending and warmup is still permitted, restart the loop. Volatile.Write(ref _warmupLoopRunning, 0); - - // Close the race where a request arrived after the loop condition read - // _warmupRequested but before we released the guard: if a request is still pending - // and the pool is running, restart the loop. - if (Volatile.Read(ref _warmupRequested) == 1 && - State == Running && - !token.IsCancellationRequested) + if (Volatile.Read(ref _warmupRequested) == 1 && CanWarmup(token)) { TryStartWarmupLoop(); } } } - - /// - /// Executes a single warmup pass: creates connections one at a time (serially) until the - /// pool reaches or a stop condition is hit. Each creation goes - /// through the same slot-reservation and rate-limited path as user requests - /// ( with isWarmup: true), and freshly created - /// connections are published to the idle channel so waiting or future user requests can use - /// them. All per-connection failures are absorbed (Story 3). - /// - /// Cancelled on shutdown to stop warmup promptly (Story 4). - private async Task WarmupPassAsync(CancellationToken token) - { - while (State == Running && - !token.IsCancellationRequested && - Count < MinPoolSize) - { - // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since warmup - // has no owning Open() call to inherit a budget from. Matches the replenishment - // behavior of the legacy WaitHandle pool. - TimeoutTimer timeout = TimeoutTimer.StartNew( - TimeSpan.FromMilliseconds(PoolGroupOptions.CreationTimeout)); - - DbConnectionInternal? connection; - try - { - // owningConnection is null: warmup connections are created unattached and enter - // the pool as idle. isWarmup suppresses all blocking-period error-state - // interaction. A null return means the shared rate limiter was saturated; a - // thrown exception means the physical connection open genuinely failed. - connection = OpenNewInternalConnection( - owningConnection: null, - cancellationToken: token, - timeout: timeout, - isWarmup: true); - } - catch (OperationCanceledException) - { - // Shutdown/cancellation unwound the in-flight create. Stop warming up. - break; - } - catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) - { - // A genuine connection-open failure (not rate limiting). Trace and absorb it, - // then stop this pass rather than retrying on a tight cadence: if opens are - // failing, hammering the server every few milliseconds would be wasteful and - // noisy. The pool stays fully operational and user requests continue to create - // on demand; the next below-minimum trigger (or a returning connection) will - // re-request warmup and try again (Story 3). - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Warmup connection creation failed, stopping pass: {1}", Id, ex); - break; - } - - if (connection is null) - { - // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), and - // creation failures throw rather than return null, so a null return means the - // shared rate limiter is currently saturated. Wait our turn rather than - // bypassing it (Story 2), then retry. Capacity frees up as user requests release - // their permits. - try - { - await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - break; - } - - continue; - } - - // If a Clear happened while we were creating, the connection now belongs to a - // previous generation. Don't pool a stale connection (implementation note: warmup - // stops on generation change). Destroying it re-triggers warmup for the current - // generation via RemoveConnection. - if (connection.ClearGeneration != _clearGeneration) - { - RemoveConnection(connection); - break; - } - - // Publish the freshly created connection as idle. It was PrePush'd at creation - // (CreatePooledConnection) and never activated, so it is in the correct state to - // enter the idle channel directly. - if (!_idleChannel.TryWrite(connection)) - { - // Channel completed (pool shutting down). Destroy instead of pooling. - RemoveConnection(connection); - break; - } - - // Yield between creations so warmup stays cooperative and responsive to - // cancellation. There is no sync-over-async anywhere in this path. - await Task.Yield(); - } - } #endregion #region Pruning diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 8e00c7c0c2..1ac430ef91 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -114,7 +114,9 @@ private static DbConnectionInternal CheckOut(ChannelDbConnectionPool pool) #region Story 1 - Background warmup on pool creation - // Warmup is a no-op when MinPoolSize == 0: no connections are ever created in the background. + /// + /// Warmup is a no-op when MinPoolSize == 0: no connections are ever created in the background. + /// [Fact] public void Startup_MinPoolSizeZero_DoesNotWarmUp() { @@ -130,7 +132,9 @@ public void Startup_MinPoolSizeZero_DoesNotWarmUp() Assert.Equal(0, pool.Count); } - // Warmup pre-creates connections up to MinPoolSize for various sizes (1 and N). + /// + /// Warmup pre-creates connections up to MinPoolSize for various sizes (1 and N). + /// [Theory] [InlineData(1)] [InlineData(3)] @@ -155,10 +159,12 @@ public void Startup_WithMinPoolSize_WarmsUpToMinimum(int minPoolSize) Assert.Equal(minPoolSize, factory.CreateCount); } - // A user request during warmup is served immediately and does not wait for warmup to finish. - // The gated factory blocks warmup's first (serial) creation, yet a user open still completes - // by creating its own connection, proving warmup does not block user requests and does not - // create connections in parallel. + /// + /// A user request during warmup is served immediately and does not wait for warmup to finish. + /// The gated factory blocks warmup's first (serial) creation, yet a user open still completes + /// by creating its own connection, proving warmup does not block user requests and does not + /// create connections in parallel. + /// [Fact] public async Task Startup_UserRequestDuringWarmup_ServedImmediately() { @@ -210,7 +216,9 @@ public async Task Startup_UserRequestDuringWarmup_ServedImmediately() #region Story 2 - Warmup through the shared rate limiter - // Warmup creates connections through the shared rate limiter and still reaches MinPoolSize. + /// + /// Warmup creates connections through the shared rate limiter and still reaches MinPoolSize. + /// [Fact] public void Warmup_ThroughSharedRateLimiter_ReachesMinimum() { @@ -233,9 +241,11 @@ public void Warmup_ThroughSharedRateLimiter_ReachesMinimum() Assert.True(rateLimiter.GetStatistics()!.TotalSuccessfulLeases >= 3); } - // When the shared rate limiter is saturated by a user request, warmup waits its turn rather - // than bypassing the limiter: the user request that arrives while warmup holds the only - // permit is denied by the same limiter. + /// + /// When the shared rate limiter is saturated by a user request, warmup waits its turn rather + /// than bypassing the limiter: the user request that arrives while warmup holds the only + /// permit is denied by the same limiter. + /// [Fact] public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() { @@ -286,8 +296,10 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() #region Story 3 - Warmup failure resilience - // Warmup creation failures are absorbed: no exception surfaces, the pool stays empty, and the - // pool is NOT put into the blocking-period error state. + /// + /// Warmup creation failures are absorbed: no exception surfaces, the pool stays empty, and the + /// pool is NOT put into the blocking-period error state. + /// [Fact] public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() { @@ -321,9 +333,11 @@ public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() $"Warmup spun on a persistent failure: {factory.WarmupAttemptCount} attempts observed."); } - // After warmup fails, a subsequent user request creates a connection on demand and succeeds. - // The factory fails only warmup creations (owning connection is null) and succeeds for user - // requests, so the two paths are cleanly separated. + /// + /// After warmup fails, a subsequent user request creates a connection on demand and succeeds. + /// The factory fails only warmup creations (owning connection is null) and succeeds for user + /// requests, so the two paths are cleanly separated. + /// [Fact] public void Warmup_Fails_UserRequestStillSucceeds() { @@ -339,12 +353,55 @@ public void Warmup_Fails_UserRequestStillSucceeds() Assert.False(pool.ErrorOccurred); } + /// + /// Warmup respects the pool's blocking-period error state: once user requests have driven the + /// pool into the error state, warmup stands down instead of continuing to replenish (mirroring + /// the legacy WaitHandle pool), even though warmup itself never enters or clears that state. + /// + [Fact] + public void Warmup_RespectsErrorState_StandsDownWhileBlocking() + { + using var firstWarmupGate = new ManualResetEventSlim(initialState: false); + var factory = new ErrorRespectingConnectionFactory(firstWarmupGate); + // Run warmup on its own thread so it parks inside its first create deterministically. + using var pool = ConstructPool( + factory, minPoolSize: 3, maxPoolSize: 10, + warmupLoopScheduler: DedicatedThreadWarmupScheduler); + + // Warmup starts and parks inside its first (successful) create on the gate. + pool.Startup(); + Assert.True( + factory.FirstWarmupStarted.Wait(HandshakeTimeout), + "Timed out waiting for warmup to begin its first creation."); + + // While warmup is parked, a user request fails and drives the pool into the error state. + Assert.ThrowsAny(() => CheckOut(pool)); + Assert.True(WaitFor(() => pool.ErrorOccurred), "User failure did not enter the pool error state."); + + // Release warmup's in-flight create. It completes and is pooled, but the loop must then + // stand down because the pool is in the error state - it must NOT keep creating up to + // MinPoolSize while user requests are still being blocked. + firstWarmupGate.Set(); + + Assert.True( + WaitFor(() => pool.IdleCount == 1), + $"Warmup's in-flight connection was not pooled; IdleCount={pool.IdleCount}."); + Assert.False( + WaitFor(() => factory.WarmupCreateCount > 1, timeoutMs: 500), + $"Warmup kept creating while the pool was in the error state; WarmupCreateCount={factory.WarmupCreateCount}."); + Assert.Equal(1, factory.WarmupCreateCount); + // Warmup must not clear the error state it merely deferred to. + Assert.True(pool.ErrorOccurred); + } + #endregion #region Story 4 - Warmup cancellation on shutdown - // When the pool is shut down mid-warmup, warmup stops promptly: no connections created after - // shutdown begins, and any in-flight connection is cleaned up. + /// + /// When the pool is shut down mid-warmup, warmup stops promptly: no connections created after + /// shutdown begins, and any in-flight connection is cleaned up. + /// [Fact] public void Shutdown_DuringWarmup_StopsAndCleansUp() { @@ -380,8 +437,10 @@ public void Shutdown_DuringWarmup_StopsAndCleansUp() #region Story 5 - Replenishment on any below-minimum event - // Destroying a connection on return (non-poolable/broken) drops the pool below MinPoolSize and - // triggers replenishment back to the minimum. + /// + /// Destroying a connection on return (non-poolable/broken) drops the pool below MinPoolSize and + /// triggers replenishment back to the minimum. + /// [Fact] public void Replenish_AfterDoomedReturn_RefillsToMinimum() { @@ -405,8 +464,10 @@ public void Replenish_AfterDoomedReturn_RefillsToMinimum() Assert.Equal(2, pool.Count); } - // Clearing the pool drops it to zero (below MinPoolSize) and triggers replenishment back to - // the minimum with fresh-generation connections. + /// + /// Clearing the pool drops it to zero (below MinPoolSize) and triggers replenishment back to + /// the minimum with fresh-generation connections. + /// [Fact] public void Replenish_AfterClear_RefillsToMinimum() { @@ -424,8 +485,10 @@ public void Replenish_AfterClear_RefillsToMinimum() Assert.Equal(3, pool.Count); } - // Destroying a connection while still at/above the minimum does NOT trigger replenishment - // beyond the minimum: the pool stays at MinPoolSize. + /// + /// Destroying a connection while still at/above the minimum does NOT trigger replenishment + /// beyond the minimum: the pool stays at MinPoolSize. + /// [Fact] public void Replenish_DestroyWhileAtMinimum_NoOvershoot() { @@ -540,6 +603,50 @@ protected override DbConnectionInternal CreateConnection( } } + /// + /// Fails user requests (non-null owning connection) so the pool enters its blocking-period + /// error state, while succeeding for warmup creations (null owning connection). The first + /// warmup creation blocks on a caller-supplied gate so a test can drive the pool into the + /// error state while warmup is parked mid-create, then observe whether warmup stands down. + /// + private sealed class ErrorRespectingConnectionFactory : SqlConnectionFactory + { + private readonly ManualResetEventSlim _firstWarmupGate; + private int _warmupCreateCount; + + internal ManualResetEventSlim FirstWarmupStarted { get; } = new(initialState: false); + + internal int WarmupCreateCount => Volatile.Read(ref _warmupCreateCount); + + internal ErrorRespectingConnectionFactory(ManualResetEventSlim firstWarmupGate) + => _firstWarmupGate = firstWarmupGate; + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (owningConnection is not null) + { + // User request: fail so the pool enters the blocking-period error state. + throw ADP.PooledOpenTimeout(); + } + + // Warmup creation: block the first one so the test can set the error state while + // warmup is parked, then let it complete and be pooled. + if (Interlocked.Increment(ref _warmupCreateCount) == 1) + { + FirstWarmupStarted.Set(); + _firstWarmupGate.Wait(); + } + + return new DoomableStubConnection(); + } + } + #endregion } } From 41b76540f9384f44ed47e646c88e2509104721ad Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 13:39:01 -0700 Subject: [PATCH 09/15] Address review: full error-state parity, simpler coalescing, harden warmup loop - Warmup now fully mirrors the WaitHandle pool's replenishment: it goes through the same OpenNewInternalConnection path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it. Removed the isWarmup parameter and all its error-state special-casing; warmup-specific behavior (absorbing the rethrow, standing down while blocking) now lives entirely in the warmup loop and CanWarmup. Updated Story 3 and the two affected tests (a user request during the blocking window now fast-fails). - Dropped the _warmupRequested counter: coalescing relies solely on the _warmupLoopRunning guard, and the single-pass loop re-reads Count each iteration to absorb below-minimum drops that happen while it runs. Requests arriving while the guard is set are simply dropped. - Defaulted _warmupLoopScheduler to Task.Run so TryStartWarmupLoop no longer needs a null-check conditional. - RunWarmupLoopAsync now releases the single-loop guard in a finally covering every exit path (including the ObjectDisposedException early return), so the flag can't be leaked. Documented why the loop is resilient to _warmupCts being disposed mid-run (token is read once under a guard; Cancel always precedes Dispose, so post-capture reads observe cancellation, not disposal). - Redesigned the stand-down test to gate the warmup loop's start (its own successful create would now clear the error state), and bounded a previously unbounded await in the rate-limiter test with a timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 16 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 287 ++++++++---------- .../ChannelDbConnectionPoolWarmupTest.cs | 137 +++++---- 3 files changed, 206 insertions(+), 234 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index c409346c3d..4c444c3408 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -37,14 +37,14 @@ Warmup creates connections through the same rate-limiting mechanism as user-init ### User Story 3 — Warmup Failure Resilience (P1) -If a connection fails to open during warmup, the failure is silently absorbed. The pool remains fully operational — user requests create connections on demand as needed. Warmup failures never trigger the pool-level error state. +If a connection fails to open during warmup, the failure is silently absorbed on the warmup loop — it is never propagated to the caller as an unhandled exception. Warmup participates in the pool's blocking-period error state exactly like an on-demand creation (mirroring the legacy WaitHandle pool): a genuine open failure enters the error state, and a successful open clears it. The pool remains operational — once the blocking period expires, user requests create connections on demand as needed. **Acceptance Scenarios**: -1. **Given** warmup is in progress, **When** a connection fails to open, **Then** the failure is traced/logged but not propagated as an exception. -2. **Given** warmup fails for all connections, **When** a user subsequently opens a connection, **Then** the user request creates a connection on demand and succeeds normally. -3. **Given** warmup fails, **When** the pool's error state is checked, **Then** the pool is NOT in error state — warmup failures do not trigger the pool-level blocking-period mechanism. -4. **Given** the pool is already in the blocking-period error state (driven there by failing user requests), **When** warmup would otherwise replenish, **Then** warmup stands down while the error state is active rather than piling more doomed opens onto a struggling server. Warmup respects the error state but never enters or clears it (mirroring the legacy WaitHandle pool). +1. **Given** warmup is in progress, **When** a connection fails to open, **Then** the failure is traced/logged and absorbed by the warmup loop rather than surfacing as an unhandled exception. +2. **Given** warmup fails and has entered the blocking-period error state, **When** a user opens a connection during the blocking window, **Then** the user request fast-fails with the cached exception (matching the WaitHandle pool); once the blocking period expires it creates a connection on demand and succeeds normally. +3. **Given** warmup's open fails, **When** the pool's error state is checked, **Then** the pool IS in the blocking-period error state — warmup uses the same shared creation path as user requests and enters/clears that state identically. +4. **Given** the pool is already in the blocking-period error state (driven there by a failing request), **When** warmup would otherwise replenish, **Then** warmup stands down while the error state is active rather than piling more doomed opens onto a struggling server (the loop's `CanWarmup` guard skips replenishment while `ErrorOccurred`, mirroring the WaitHandle pool). --- @@ -76,9 +76,9 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Warmup starts automatically when the pool's `Startup()` is called. - Warmup runs on a background task, so it never blocks the caller (`Startup`/connection return) and uses no sync-over-async in the loop. The physical connection open itself is currently synchronous (executed on the background task); the connection factory does not yet expose an async open. - Creates connections serially (one at a time) through the shared rate limiter. -- Warmup failures are traced/logged but do not propagate or trigger error state. Warmup does, however, respect an already-active blocking-period error state and stands down while it is active. +- Warmup failures are traced/logged and absorbed by the warmup loop (never surfaced as an unhandled exception). Warmup goes through the same shared creation path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it (mirroring the WaitHandle pool). While the error state is active, the warmup loop stands down (`CanWarmup`) rather than replenishing. - If a `Clear` races with an in-flight warmup creation, the freshly created (now stale-generation) connection is not special-cased by warmup; it is harmlessly discarded by the liveness/generation check on its next retrieval. After a `Clear`, replenishment refills the pool to the minimum with fresh-generation connections. -- Concurrent warmup/replenishment requests are coalesced — only one warmup loop executes at a time. +- Concurrent warmup/replenishment requests are coalesced — a single `_warmupLoopRunning` guard ensures only one warmup loop executes at a time, and requests that arrive while it is running are dropped (the running loop re-reads the pool count each iteration and drives to the minimum on its own). - Warmup is a no-op when Min Pool Size = 0. ## Acceptance Criteria @@ -87,7 +87,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - After warmup completes, the pool contains Min Pool Size idle connections. - Warmup runs on a background task without blocking callers and without sync-over-async in the loop (the physical open is currently synchronous, executed on the background task, pending async-open support in the factory). - Warmup connections are created serially through the shared rate limiter. -- Warmup errors are traced/logged and do not crash the application or trigger error state. +- Warmup errors are traced/logged and never crash the application. Because warmup shares the on-demand creation path, a genuine open failure enters the pool's blocking-period error state (as it would for a user request) and a successful open clears it. - If the pool is shut down or cleared during warmup, warmup stops gracefully. - After any event that reduces the pool below Min Pool Size, the pool restores to minimum within one warmup pass. - Concurrent warmup/replenishment requests are coalesced so only one warmup loop executes at a time. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ee46d2bdab..f3e0ba6dfa 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -146,21 +146,13 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable private int _warmupLoopRunning; /// - /// Set to 1 by whenever warmup/replenishment is needed and - /// drained to 0 by the running loop before each pass. Lets requests that arrive while a - /// pass is executing coalesce into a single follow-up pass rather than spawning parallel - /// loops. - /// - private int _warmupRequested; - - /// - /// Optional test seam controlling how the warmup loop is launched. When null (production), - /// the loop runs via on the thread pool. Tests inject a + /// Test seam controlling how the warmup loop is launched. Defaults to + /// on the thread pool in production. Tests inject a /// scheduler that runs the loop on a dedicated thread so warmup's first (synchronous, /// possibly gated) physical open cannot be delayed by thread-pool starvation, keeping /// timing-sensitive assertions deterministic without mutating global thread-pool state. /// - private readonly Action>? _warmupLoopScheduler; + private readonly Action> _warmupLoopScheduler; #endregion /// @@ -184,7 +176,7 @@ internal ChannelDbConnectionPool( MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); TransactedConnectionPool = new(this); _connectionCreationRateLimiter = connectionCreationRateLimiter; - _warmupLoopScheduler = warmupLoopScheduler; + _warmupLoopScheduler = warmupLoopScheduler ?? (static loop => Task.Run(loop)); _connectionSlots = new(MaxPoolSize); _idleChannel = new(); @@ -632,15 +624,6 @@ public bool TryGetConnection( /// The cancellation token to cancel the operation. /// The overall timeout budget. Passed through to the physical connection /// so it uses the remaining budget rather than starting a fresh timeout. - /// When true, the call originates from background warmup/replenishment - /// rather than a user request. Warmup does not enter the pool-level blocking-period error - /// state on failure, nor clear it on success - a best-effort warmup must never poison the - /// pool for user requests, nor mask a real fault by clearing it. Warmup does, however, - /// respect the error state: the warmup loop stands down while it is active (see - /// ), mirroring the legacy WaitHandle pool. A warmup creation failure - /// still throws (without entering the error state) so the caller can distinguish it from a - /// rate-limit backoff, which is signalled by a null return; see - /// (Story 3). /// The new internal connection, or null if the pool has no available slot or the /// rate limiter is currently saturated. In the latter case the caller should fall back to /// the idle-channel wait; the rate limiter will write a null to the idle channel when a @@ -651,19 +634,16 @@ public bool TryGetConnection( private DbConnectionInternal? OpenNewInternalConnection( DbConnection? owningConnection, CancellationToken cancellationToken, - TimeoutTimer timeout, - bool isWarmup = false) + TimeoutTimer timeout) { cancellationToken.ThrowIfCancellationRequested(); - // Fast-fail in the error state for user requests. FR-006. Warmup does not fast-fail here - // (it neither reads nor mutates the blocking-period state at creation time); instead the - // warmup loop stands down while the error state is active via CanWarmup, so it does not - // reach this path while blocking. - if (!isWarmup) - { - _errorState?.ThrowIfActive(); - } + // Fast-fail if the pool is in the blocking-period error state. FR-006. Warmup goes + // through this same path (it has no isWarmup exemption): it mirrors the legacy WaitHandle + // pool, whose replenishment enters/clears the same error state as user requests. In + // practice the warmup loop already stands down before reaching here (CanWarmup checks + // ErrorOccurred); this covers the narrow race where the state flips in between. + _errorState?.ThrowIfActive(); try { @@ -779,30 +759,21 @@ _connectionCreationRateLimiter is not null && // start the pruning timer so idle connections can be reclaimed. Pruner?.UpdateTimer(); - // A successful creation clears error/backoff state (FR-009). Warmup does not - // enter or clear the blocking-period state (though it does stand down while the - // state is active; see CanWarmup), so it must not clear it on success either. - if (!isWarmup) - { - _errorState?.Clear(); - } + // A successful creation clears error/backoff state (FR-009). Warmup goes through + // this same path and clears the state on success too, mirroring the legacy + // WaitHandle pool: a connection that opens proves the server is reachable. + _errorState?.Clear(); } return connection; } catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) { - // Warmup deliberately does not enter the blocking-period state on failure (Story 3): - // a warmup creation failure must never poison the pool for user requests. We still - // rethrow so the warmup caller can distinguish a genuine creation failure from a - // rate-limit backoff (which is signalled by a null return, not an exception) and - // apply the right policy. RunWarmupLoopAsync traces and absorbs the exception there. - if (isWarmup) - { - throw; - } - - // Enter the blocking period error state on creation failure if configured. + // Enter the blocking period error state on creation failure if configured. Warmup + // goes through this same path (the warmup loop absorbs the rethrow in its own catch), + // mirroring the legacy WaitHandle pool, whose replenishment failures also enter the + // error state. + // // We deliberately exclude OperationCanceledException: that is thrown when the // caller's own timeout/cancellation budget expires while waiting, which is // client-side contention rather than a physical connection creation failure and @@ -1102,39 +1073,38 @@ private void RequestWarmup() } // Fast path: the pool is already at or above the minimum, so there is nothing to warm - // up. Return before touching the request flag or scheduling a thread-pool work item. - // This keeps hot-path callers (e.g. RemoveConnection on every return) cheap. The check - // is best-effort under concurrency; a below-minimum condition missed here is still - // observed by WarmupPassAsync's loop guard on the next trigger. + // up. Return before scheduling a thread-pool work item. This keeps hot-path callers + // (e.g. RemoveConnection on every return) cheap. The check is best-effort under + // concurrency; a below-minimum condition missed here is still observed by the running + // loop, which re-reads Count on every iteration. if (Count >= MinPoolSize) { return; } - // Record that (more) warmup is needed. A loop that is already running observes this and - // executes an additional pass; see RunWarmupLoopAsync. - Volatile.Write(ref _warmupRequested, 1); - TryStartWarmupLoop(); } /// /// True while warmup/replenishment is permitted to keep creating connections: the pool is /// running, shutdown has not cancelled background work, and the pool is not in the - /// blocking-period error state. Warmup does not enter or clear the error state, but it does - /// respect it - while user requests are failing and the pool is blocking, warmup stands down - /// rather than piling more doomed opens onto a struggling server. This mirrors the legacy - /// WaitHandle pool, which skips replenishment while ErrorOccurred is set. + /// blocking-period error state. While user requests are failing and the pool is blocking, + /// warmup stands down rather than piling more doomed opens onto a struggling server. This + /// mirrors the legacy WaitHandle pool, which skips replenishment while ErrorOccurred + /// is set. (Warmup does participate in the error state on its own creations - entering it on + /// failure and clearing it on success, via .) /// private bool CanWarmup(CancellationToken token) => State == Running && !token.IsCancellationRequested && !ErrorOccurred; /// /// Starts the coalesced warmup loop if one is not already running. The single-loop guard - /// () coalesces concurrent requests: the loser of the CAS - /// leaves its request flag set for the winning loop to pick up. Runs on the thread pool so - /// warmup never blocks the caller (Startup or a returning connection); tests may supply an - /// alternate scheduler so warmup startup is not subject to thread-pool scheduling latency. + /// () coalesces concurrent requests: a request that arrives + /// while a loop is already running is simply dropped, because that loop re-reads + /// on every iteration and will drive the pool to + /// regardless. Runs on the thread pool so warmup never blocks the caller (Startup or a + /// returning connection); tests may supply an alternate scheduler so warmup startup is not + /// subject to thread-pool scheduling latency. /// private void TryStartWarmupLoop() { @@ -1147,14 +1117,7 @@ private void TryStartWarmupLoop() { // Fire-and-forget: the loop absorbs its own exceptions and always releases the // single-loop guard on exit. - if (_warmupLoopScheduler is not null) - { - _warmupLoopScheduler(RunWarmupLoopAsync); - } - else - { - _ = Task.Run(RunWarmupLoopAsync); - } + _warmupLoopScheduler(RunWarmupLoopAsync); } catch (Exception ex) { @@ -1169,114 +1132,107 @@ private void TryStartWarmupLoop() /// /// The coalesced warmup loop: creates connections one at a time (serially) up to - /// , re-running as long as new requests keep arriving, then releases - /// the single-loop guard. Each creation goes through the same slot-reservation and - /// rate-limited path as user requests ( with - /// isWarmup: true), and freshly created connections are published to the idle channel - /// for waiting or future user requests. All failures are absorbed so warmup can never surface - /// an unhandled exception or poison the pool (Story 3). + /// , then releases the single-loop guard. Each creation goes through + /// the same slot-reservation and rate-limited path as user requests + /// (), and freshly created connections are published + /// to the idle channel for waiting or future user requests. All failures are absorbed so + /// warmup can never surface an unhandled exception (Story 3); a genuine open failure still + /// enters the pool's blocking-period error state via the shared creation path, mirroring the + /// legacy WaitHandle pool. /// private async Task RunWarmupLoopAsync() { - CancellationToken token; - try - { - token = _warmupCts.Token; - } - catch (ObjectDisposedException) - { - // The pool shut down and disposed the cancellation source before this loop started. - // There is nothing to warm up; release the guard and exit. - Volatile.Write(ref _warmupLoopRunning, 0); - return; - } - try { - do + CancellationToken token; + try + { + token = _warmupCts.Token; + } + catch (ObjectDisposedException) { - // Drain the request flag before each pass so a request that arrives mid-pass - // re-arms it and schedules a follow-up pass rather than being lost. - Volatile.Write(ref _warmupRequested, 0); + // The pool shut down and disposed the cancellation source before this loop + // started. There is nothing to warm up; the finally releases the guard. + return; + } - while (CanWarmup(token) && Count < MinPoolSize) + // Single pass: re-reads Count each iteration, so any below-minimum drop that happens + // while the loop is running is picked up here without a separate request flag. + while (CanWarmup(token) && Count < MinPoolSize) + { + // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since + // warmup has no owning Open() call to inherit a budget from. Matches the + // replenishment behavior of the legacy WaitHandle pool. + TimeoutTimer timeout = TimeoutTimer.StartNew( + TimeSpan.FromMilliseconds(PoolGroupOptions.CreationTimeout)); + + DbConnectionInternal? connection; + try + { + // owningConnection is null: warmup connections are created unattached and + // enter the pool as idle. A null return means the shared rate limiter was + // saturated; a thrown exception means the physical open genuinely failed. + connection = OpenNewInternalConnection( + owningConnection: null, + cancellationToken: token, + timeout: timeout); + } + catch (OperationCanceledException) + { + // Shutdown/cancellation unwound the in-flight create. Stop warming up. + break; + } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) { - // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since - // warmup has no owning Open() call to inherit a budget from. Matches the - // replenishment behavior of the legacy WaitHandle pool. - TimeoutTimer timeout = TimeoutTimer.StartNew( - TimeSpan.FromMilliseconds(PoolGroupOptions.CreationTimeout)); + // A genuine connection-open failure (not rate limiting). It has already + // entered the pool's blocking-period error state (see OpenNewInternalConnection); + // trace and absorb the rethrow here, then stop this pass rather than retrying + // on a tight cadence. The pool stays operational: user requests fast-fail + // during the blocking window and resume creating on demand once it expires, + // and the next below-minimum trigger re-requests warmup (Story 3). + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Warmup connection creation failed, stopping pass: {1}", Id, ex); + break; + } - DbConnectionInternal? connection; + if (connection is null) + { + // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), + // and creation failures throw rather than return null, so a null return + // means the shared rate limiter is currently saturated. Wait our turn + // rather than bypassing it (Story 2), then retry. Capacity frees up as user + // requests release their permits. try { - // owningConnection is null: warmup connections are created unattached and - // enter the pool as idle. A null return means the shared rate limiter was - // saturated; a thrown exception means the physical open genuinely failed. - connection = OpenNewInternalConnection( - owningConnection: null, - cancellationToken: token, - timeout: timeout, - isWarmup: true); + await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); } catch (OperationCanceledException) { - // Shutdown/cancellation unwound the in-flight create. Stop warming up. - break; - } - catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) - { - // A genuine connection-open failure (not rate limiting). Trace and absorb - // it, then stop this pass rather than retrying on a tight cadence: if opens - // are failing, hammering the server every few milliseconds would be - // wasteful and noisy. The pool stays fully operational and user requests - // continue to create on demand; the next below-minimum trigger will - // re-request warmup and try again (Story 3). - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Warmup connection creation failed, stopping pass: {1}", Id, ex); break; } - if (connection is null) - { - // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), - // and creation failures throw rather than return null, so a null return - // means the shared rate limiter is currently saturated. Wait our turn - // rather than bypassing it (Story 2), then retry. Capacity frees up as user - // requests release their permits. - try - { - await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - break; - } - - continue; - } - - // Publish the freshly created connection as idle. It was PrePush'd at creation - // (CreatePooledConnection) and never activated, so it is in the correct state - // to enter the idle channel directly. If a Clear raced and bumped the - // generation, the stale connection is harmlessly removed by IsLiveConnection - // on its next retrieval, so we don't check the generation here. - if (!_idleChannel.TryWrite(connection)) - { - // Channel completed (pool shutting down). Destroy instead of pooling. - RemoveConnection(connection); - break; - } + continue; + } - // OpenNewInternalConnection is synchronous and blocks the loop's thread for - // the duration of the physical open. Yield between creations so a multi- - // connection warmup returns its thread-pool worker to the scheduler between - // opens (rather than monopolizing one worker for the whole sequence) and stays - // responsive to cancellation. There is no sync-over-async anywhere in this path. - await Task.Yield(); + // Publish the freshly created connection as idle. It was PrePush'd at creation + // (CreatePooledConnection) and never activated, so it is in the correct state + // to enter the idle channel directly. If a Clear raced and bumped the + // generation, the stale connection is harmlessly removed by IsLiveConnection + // on its next retrieval, so we don't check the generation here. + if (!_idleChannel.TryWrite(connection)) + { + // Channel completed (pool shutting down). Destroy instead of pooling. + RemoveConnection(connection); + break; } + + // OpenNewInternalConnection is synchronous and blocks the loop's thread for + // the duration of the physical open. Yield between creations so a multi- + // connection warmup returns its thread-pool worker to the scheduler between + // opens (rather than monopolizing one worker for the whole sequence) and stays + // responsive to cancellation. There is no sync-over-async anywhere in this path. + await Task.Yield(); } - while (Volatile.Read(ref _warmupRequested) == 1 && CanWarmup(token)); } catch (Exception ex) { @@ -1286,14 +1242,9 @@ private async Task RunWarmupLoopAsync() } finally { - // Release the single-loop guard, then close the race where a request arrived after - // the loop condition was read but before we released the guard: if a request is still - // pending and warmup is still permitted, restart the loop. + // Always release the single-loop guard, whatever exit path we took, so a future + // below-minimum trigger can start a new loop. Volatile.Write(ref _warmupLoopRunning, 0); - if (Volatile.Read(ref _warmupRequested) == 1 && CanWarmup(token)) - { - TryStartWarmupLoop(); - } } } #endregion diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 1ac430ef91..b48db93b93 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -288,7 +288,13 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() createGate.Set(); Assert.True(WaitFor(() => pool.Count >= 2), $"Pool did not reach MinPoolSize; Count={pool.Count}."); - DbConnectionInternal? userConnection = await tcs.Task; + // Bound the wait so a regression that never completes the async request fails fast + // instead of hanging the whole test run. + Task userRequest = tcs.Task; + Assert.True( + userRequest == await Task.WhenAny(userRequest, Task.Delay(HandshakeTimeout)), + "Timed out waiting for the deferred user request to complete."); + DbConnectionInternal? userConnection = await userRequest; Assert.NotNull(userConnection); } @@ -297,11 +303,13 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() #region Story 3 - Warmup failure resilience /// - /// Warmup creation failures are absorbed: no exception surfaces, the pool stays empty, and the - /// pool is NOT put into the blocking-period error state. + /// Warmup creation failures are absorbed - no exception surfaces onto the thread pool and no + /// connections are pooled - but, mirroring the legacy WaitHandle pool, a genuine open failure + /// does enter the pool's blocking-period error state. Warmup then stops the pass rather than + /// spinning on the persistent failure. /// [Fact] - public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() + public void Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() { var factory = new WarmupFailingConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); @@ -313,14 +321,17 @@ public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() WaitFor(() => factory.WarmupAttemptCount >= 1), "Warmup never attempted a creation."); - // Pool never accumulates idle connections, but nothing crashes and the pool is not in - // error state. (Count reflects in-flight reservations transiently, so IdleCount is the - // reliable measure of successfully pooled connections.) + // Pool never accumulates idle connections and nothing crashes. The failure enters the + // blocking-period error state, exactly as an on-demand creation failure would (Count + // reflects in-flight reservations transiently, so IdleCount is the reliable measure of + // successfully pooled connections). Assert.False( WaitFor(() => pool.IdleCount > 0, timeoutMs: 500), "A warmup connection was pooled despite all creations failing."); Assert.Equal(0, pool.IdleCount); - Assert.False(pool.ErrorOccurred, "Warmup failures must not trigger the pool error state."); + Assert.True( + WaitFor(() => pool.ErrorOccurred), + "Warmup failure did not enter the pool error state as the WaitHandle pool does."); // A genuine creation failure stops the warmup pass instead of retrying on a tight // cadence, so the failing factory is not hammered. Without a fresh below-minimum trigger @@ -334,12 +345,13 @@ public void Warmup_AllCreationsFail_AbsorbedAndNoErrorState() } /// - /// After warmup fails, a subsequent user request creates a connection on demand and succeeds. - /// The factory fails only warmup creations (owning connection is null) and succeeds for user - /// requests, so the two paths are cleanly separated. + /// After warmup fails, the pool enters its blocking-period error state (mirroring the + /// WaitHandle pool), so a user request during the blocking window fast-fails with the cached + /// exception rather than attempting a fresh on-demand open. The pool remains operational and + /// resumes creating on demand once the blocking period expires. /// [Fact] - public void Warmup_Fails_UserRequestStillSucceeds() + public void Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() { var factory = new WarmupFailingConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); @@ -347,51 +359,71 @@ public void Warmup_Fails_UserRequestStillSucceeds() pool.Startup(); Assert.True(WaitFor(() => factory.WarmupAttemptCount >= 1), "Warmup never attempted a creation."); - // A real user request (non-null owning connection) succeeds on demand. - DbConnectionInternal connection = CheckOut(pool); - Assert.NotNull(connection); - Assert.False(pool.ErrorOccurred); + // Warmup's failure drives the pool into the blocking-period error state. + Assert.True(WaitFor(() => pool.ErrorOccurred), "Warmup failure did not enter the pool error state."); + + // A user request while the pool is blocking fast-fails with the cached exception. + Assert.ThrowsAny(() => CheckOut(pool)); } /// /// Warmup respects the pool's blocking-period error state: once user requests have driven the - /// pool into the error state, warmup stands down instead of continuing to replenish (mirroring - /// the legacy WaitHandle pool), even though warmup itself never enters or clears that state. + /// pool into the error state, warmup stands down (CanWarmup is false) and creates + /// nothing, mirroring the legacy WaitHandle pool, which skips replenishment while the pool is + /// blocking. The warmup loop is held at its first instruction until the pool is blocking so + /// the stand-down is observed deterministically. /// [Fact] public void Warmup_RespectsErrorState_StandsDownWhileBlocking() { - using var firstWarmupGate = new ManualResetEventSlim(initialState: false); - var factory = new ErrorRespectingConnectionFactory(firstWarmupGate); - // Run warmup on its own thread so it parks inside its first create deterministically. + using var warmupStartGate = new ManualResetEventSlim(initialState: false); + using var warmupLoopFinished = new ManualResetEventSlim(initialState: false); + + // Park the warmup loop before its very first iteration until the test has driven the pool + // into the error state, then signal when the loop has run to completion. + Action> gatedStartScheduler = loop => + { + var thread = new Thread(() => + { + warmupStartGate.Wait(); + try + { + loop().GetAwaiter().GetResult(); + } + finally + { + warmupLoopFinished.Set(); + } + }) + { + IsBackground = true, + Name = "WarmupTest.GatedStart", + }; + thread.Start(); + }; + + var factory = new UserFailingWarmupCountingConnectionFactory(); using var pool = ConstructPool( factory, minPoolSize: 3, maxPoolSize: 10, - warmupLoopScheduler: DedicatedThreadWarmupScheduler); + warmupLoopScheduler: gatedStartScheduler); - // Warmup starts and parks inside its first (successful) create on the gate. + // Warmup is scheduled on Startup but parked before it runs a single iteration. pool.Startup(); - Assert.True( - factory.FirstWarmupStarted.Wait(HandshakeTimeout), - "Timed out waiting for warmup to begin its first creation."); - // While warmup is parked, a user request fails and drives the pool into the error state. + // While warmup is parked, a failing user request drives the pool into the error state. Assert.ThrowsAny(() => CheckOut(pool)); Assert.True(WaitFor(() => pool.ErrorOccurred), "User failure did not enter the pool error state."); - // Release warmup's in-flight create. It completes and is pooled, but the loop must then - // stand down because the pool is in the error state - it must NOT keep creating up to - // MinPoolSize while user requests are still being blocked. - firstWarmupGate.Set(); - + // Release the warmup loop. Because the pool is blocking, CanWarmup is false, so the loop + // must exit immediately without creating any connections. + warmupStartGate.Set(); Assert.True( - WaitFor(() => pool.IdleCount == 1), - $"Warmup's in-flight connection was not pooled; IdleCount={pool.IdleCount}."); - Assert.False( - WaitFor(() => factory.WarmupCreateCount > 1, timeoutMs: 500), - $"Warmup kept creating while the pool was in the error state; WarmupCreateCount={factory.WarmupCreateCount}."); - Assert.Equal(1, factory.WarmupCreateCount); - // Warmup must not clear the error state it merely deferred to. - Assert.True(pool.ErrorOccurred); + warmupLoopFinished.Wait(HandshakeTimeout), + "Warmup loop did not complete after being released."); + + Assert.Equal(0, factory.WarmupCreateCount); + Assert.Equal(0, pool.IdleCount); + Assert.True(pool.ErrorOccurred, "Warmup must not clear the error state it stood down for."); } #endregion @@ -605,22 +637,16 @@ protected override DbConnectionInternal CreateConnection( /// /// Fails user requests (non-null owning connection) so the pool enters its blocking-period - /// error state, while succeeding for warmup creations (null owning connection). The first - /// warmup creation blocks on a caller-supplied gate so a test can drive the pool into the - /// error state while warmup is parked mid-create, then observe whether warmup stands down. + /// error state, while succeeding for and counting warmup creations (null owning connection). + /// Lets a test drive the pool into the error state and then verify warmup stands down + /// (creates nothing) rather than continuing to replenish. /// - private sealed class ErrorRespectingConnectionFactory : SqlConnectionFactory + private sealed class UserFailingWarmupCountingConnectionFactory : SqlConnectionFactory { - private readonly ManualResetEventSlim _firstWarmupGate; private int _warmupCreateCount; - internal ManualResetEventSlim FirstWarmupStarted { get; } = new(initialState: false); - internal int WarmupCreateCount => Volatile.Read(ref _warmupCreateCount); - internal ErrorRespectingConnectionFactory(ManualResetEventSlim firstWarmupGate) - => _firstWarmupGate = firstWarmupGate; - protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, ConnectionPoolKey poolKey, @@ -635,14 +661,9 @@ protected override DbConnectionInternal CreateConnection( throw ADP.PooledOpenTimeout(); } - // Warmup creation: block the first one so the test can set the error state while - // warmup is parked, then let it complete and be pooled. - if (Interlocked.Increment(ref _warmupCreateCount) == 1) - { - FirstWarmupStarted.Set(); - _firstWarmupGate.Wait(); - } - + // Warmup creation: count it. In the stand-down test this must never be reached while + // the pool is blocking. + Interlocked.Increment(ref _warmupCreateCount); return new DoomableStubConnection(); } } From bc4a3d4840de345fbb839efd11c3cfdc71a44ebf Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 13:56:31 -0700 Subject: [PATCH 10/15] Inline single-use warmup helpers; reconcile spec/comments Merge TryStartWarmupLoop into RequestWarmup and inline the CanWarmup guard into the warmup loop condition, since each was used from only one call site. Soften residual CanWarmup references in test comments and the spec now that the method no longer exists. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 4 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 56 ++++++++----------- .../ChannelDbConnectionPoolWarmupTest.cs | 12 ++-- 3 files changed, 31 insertions(+), 41 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index 4c444c3408..a5d8bb00c7 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -44,7 +44,7 @@ If a connection fails to open during warmup, the failure is silently absorbed on 1. **Given** warmup is in progress, **When** a connection fails to open, **Then** the failure is traced/logged and absorbed by the warmup loop rather than surfacing as an unhandled exception. 2. **Given** warmup fails and has entered the blocking-period error state, **When** a user opens a connection during the blocking window, **Then** the user request fast-fails with the cached exception (matching the WaitHandle pool); once the blocking period expires it creates a connection on demand and succeeds normally. 3. **Given** warmup's open fails, **When** the pool's error state is checked, **Then** the pool IS in the blocking-period error state — warmup uses the same shared creation path as user requests and enters/clears that state identically. -4. **Given** the pool is already in the blocking-period error state (driven there by a failing request), **When** warmup would otherwise replenish, **Then** warmup stands down while the error state is active rather than piling more doomed opens onto a struggling server (the loop's `CanWarmup` guard skips replenishment while `ErrorOccurred`, mirroring the WaitHandle pool). +4. **Given** the pool is already in the blocking-period error state (driven there by a failing request), **When** warmup would otherwise replenish, **Then** warmup stands down while the error state is active rather than piling more doomed opens onto a struggling server (the warmup loop skips replenishment while `ErrorOccurred`, mirroring the WaitHandle pool). --- @@ -76,7 +76,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Warmup starts automatically when the pool's `Startup()` is called. - Warmup runs on a background task, so it never blocks the caller (`Startup`/connection return) and uses no sync-over-async in the loop. The physical connection open itself is currently synchronous (executed on the background task); the connection factory does not yet expose an async open. - Creates connections serially (one at a time) through the shared rate limiter. -- Warmup failures are traced/logged and absorbed by the warmup loop (never surfaced as an unhandled exception). Warmup goes through the same shared creation path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it (mirroring the WaitHandle pool). While the error state is active, the warmup loop stands down (`CanWarmup`) rather than replenishing. +- Warmup failures are traced/logged and absorbed by the warmup loop (never surfaced as an unhandled exception). Warmup goes through the same shared creation path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it (mirroring the WaitHandle pool). While the error state is active, the warmup loop stands down rather than replenishing. - If a `Clear` races with an in-flight warmup creation, the freshly created (now stale-generation) connection is not special-cased by warmup; it is harmlessly discarded by the liveness/generation check on its next retrieval. After a `Clear`, replenishment refills the pool to the minimum with fresh-generation connections. - Concurrent warmup/replenishment requests are coalesced — a single `_warmupLoopRunning` guard ensures only one warmup loop executes at a time, and requests that arrive while it is running are dropped (the running loop re-reads the pool count each iteration and drives to the minimum on its own). - Warmup is a no-op when Min Pool Size = 0. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index f3e0ba6dfa..bf92347f98 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -641,8 +641,8 @@ public bool TryGetConnection( // Fast-fail if the pool is in the blocking-period error state. FR-006. Warmup goes // through this same path (it has no isWarmup exemption): it mirrors the legacy WaitHandle // pool, whose replenishment enters/clears the same error state as user requests. In - // practice the warmup loop already stands down before reaching here (CanWarmup checks - // ErrorOccurred); this covers the narrow race where the state flips in between. + // practice the warmup loop already stands down before reaching here (its loop condition + // checks ErrorOccurred); this covers the narrow race where the state flips in between. _errorState?.ThrowIfActive(); try @@ -1060,8 +1060,11 @@ private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection /// /// This is the single entry point for every warmup trigger: pool startup and any event that /// drops the pool below (connection destruction on return, idle - /// timeout eviction, pruning). Concurrent requests are coalesced so that only one warmup - /// loop ever runs at a time (implementation note: warmup coalescing). + /// timeout eviction, pruning). Concurrent requests are coalesced by the + /// guard so that only one warmup loop ever runs at a time: a + /// request that arrives while a loop is already running is simply dropped, because that loop + /// re-reads on every iteration and will drive the pool to + /// regardless. /// private void RequestWarmup() { @@ -1082,32 +1085,7 @@ private void RequestWarmup() return; } - TryStartWarmupLoop(); - } - - /// - /// True while warmup/replenishment is permitted to keep creating connections: the pool is - /// running, shutdown has not cancelled background work, and the pool is not in the - /// blocking-period error state. While user requests are failing and the pool is blocking, - /// warmup stands down rather than piling more doomed opens onto a struggling server. This - /// mirrors the legacy WaitHandle pool, which skips replenishment while ErrorOccurred - /// is set. (Warmup does participate in the error state on its own creations - entering it on - /// failure and clearing it on success, via .) - /// - private bool CanWarmup(CancellationToken token) - => State == Running && !token.IsCancellationRequested && !ErrorOccurred; - - /// - /// Starts the coalesced warmup loop if one is not already running. The single-loop guard - /// () coalesces concurrent requests: a request that arrives - /// while a loop is already running is simply dropped, because that loop re-reads - /// on every iteration and will drive the pool to - /// regardless. Runs on the thread pool so warmup never blocks the caller (Startup or a - /// returning connection); tests may supply an alternate scheduler so warmup startup is not - /// subject to thread-pool scheduling latency. - /// - private void TryStartWarmupLoop() - { + // Coalesce: only start a loop if one is not already running. if (Interlocked.CompareExchange(ref _warmupLoopRunning, 1, 0) != 0) { return; @@ -1115,7 +1093,8 @@ private void TryStartWarmupLoop() try { - // Fire-and-forget: the loop absorbs its own exceptions and always releases the + // Fire-and-forget on the thread pool (or an injected test scheduler) so warmup never + // blocks the caller. The loop absorbs its own exceptions and always releases the // single-loop guard on exit. _warmupLoopScheduler(RunWarmupLoopAsync); } @@ -1126,7 +1105,7 @@ private void TryStartWarmupLoop() // for the life of the pool; the next below-minimum trigger will try again. Volatile.Write(ref _warmupLoopRunning, 0); SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Failed to schedule warmup loop, absorbing: {1}", Id, ex); + " {0}, Failed to schedule warmup loop, absorbing: {1}", Id, ex); } } @@ -1158,7 +1137,18 @@ private async Task RunWarmupLoopAsync() // Single pass: re-reads Count each iteration, so any below-minimum drop that happens // while the loop is running is picked up here without a separate request flag. - while (CanWarmup(token) && Count < MinPoolSize) + // + // The loop keeps creating while the pool is running, shutdown has not cancelled + // background work, and the pool is not in the blocking-period error state. While user + // requests are failing and the pool is blocking (ErrorOccurred), warmup stands down + // rather than piling more doomed opens onto a struggling server - mirroring the legacy + // WaitHandle pool, which skips replenishment while blocking. (Warmup still + // participates in the error state on its own creations - entering it on failure and + // clearing it on success - via OpenNewInternalConnection.) + while (State == Running + && !token.IsCancellationRequested + && !ErrorOccurred + && Count < MinPoolSize) { // Fresh per-attempt timeout budget based on the pool's CreationTimeout, since // warmup has no owning Open() call to inherit a budget from. Matches the diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index b48db93b93..9edb34c015 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -368,10 +368,10 @@ public void Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() /// /// Warmup respects the pool's blocking-period error state: once user requests have driven the - /// pool into the error state, warmup stands down (CanWarmup is false) and creates - /// nothing, mirroring the legacy WaitHandle pool, which skips replenishment while the pool is - /// blocking. The warmup loop is held at its first instruction until the pool is blocking so - /// the stand-down is observed deterministically. + /// pool into the error state, warmup stands down (its loop condition sees ErrorOccurred) and + /// creates nothing, mirroring the legacy WaitHandle pool, which skips replenishment while the + /// pool is blocking. The warmup loop is held at its first instruction until the pool is + /// blocking so the stand-down is observed deterministically. /// [Fact] public void Warmup_RespectsErrorState_StandsDownWhileBlocking() @@ -414,8 +414,8 @@ public void Warmup_RespectsErrorState_StandsDownWhileBlocking() Assert.ThrowsAny(() => CheckOut(pool)); Assert.True(WaitFor(() => pool.ErrorOccurred), "User failure did not enter the pool error state."); - // Release the warmup loop. Because the pool is blocking, CanWarmup is false, so the loop - // must exit immediately without creating any connections. + // Release the warmup loop. Because the pool is blocking, its loop condition sees the + // error state, so the loop must exit immediately without creating any connections. warmupStartGate.Set(); Assert.True( warmupLoopFinished.Wait(HandshakeTimeout), From 702139943b085766c8614e595e79447731585243 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 14:45:46 -0700 Subject: [PATCH 11/15] Expose warmup task and remove test scheduler seam Expose the warmup loop as an internal WarmupLoopTask property so tests can await a pass to deterministic completion instead of polling. Remove the test-only warmup loop scheduler seam (always use Task.Run) and make RequestWarmup internal so tests can invoke it directly. Remove the rate-limit retry loop in RunWarmupLoopAsync: on limiter saturation the pass now ends, relying on concurrent user creations plus re-triggering via RemoveConnection to converge to MinPoolSize. Rewrite the warmup unit tests to await WarmupLoopTask rather than spin-poll, and drive the error-state stand-down test via a failing user request plus a direct RequestWarmup invocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 4 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 70 +++-- .../ChannelDbConnectionPoolWarmupTest.cs | 263 +++++++----------- 3 files changed, 142 insertions(+), 195 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index a5d8bb00c7..5c5aec793a 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -31,7 +31,7 @@ Warmup creates connections through the same rate-limiting mechanism as user-init **Acceptance Scenarios**: 1. **Given** warmup is creating connections, **When** a user request also needs a new connection, **Then** both warmup and user requests compete fairly through the shared rate limiter. -2. **Given** the rate limiter is at capacity, **When** warmup attempts to create the next connection, **Then** warmup waits its turn rather than bypassing the limiter. +2. **Given** the rate limiter is at capacity, **When** warmup attempts to create the next connection, **Then** warmup does not bypass the limiter: it ends the current pass without creating (rather than spinning or waiting on a permit). Saturation only occurs while user requests are actively creating connections, which fill the pool themselves; any later drop below the minimum re-triggers warmup once capacity has freed up. --- @@ -75,7 +75,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Warmup starts automatically when the pool's `Startup()` is called. - Warmup runs on a background task, so it never blocks the caller (`Startup`/connection return) and uses no sync-over-async in the loop. The physical connection open itself is currently synchronous (executed on the background task); the connection factory does not yet expose an async open. -- Creates connections serially (one at a time) through the shared rate limiter. +- Creates connections serially (one at a time) through the shared rate limiter. If the limiter is saturated (no permit available), warmup ends the pass rather than bypassing or spinning on the limiter; convergence to the minimum is handled by the concurrent user creations that saturated it and by re-triggering on the next below-minimum event. - Warmup failures are traced/logged and absorbed by the warmup loop (never surfaced as an unhandled exception). Warmup goes through the same shared creation path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it (mirroring the WaitHandle pool). While the error state is active, the warmup loop stands down rather than replenishing. - If a `Clear` races with an in-flight warmup creation, the freshly created (now stale-generation) connection is not special-cased by warmup; it is harmlessly discarded by the liveness/generation check on its next retrieval. After a `Clear`, replenishment refills the pool to the minimum with fresh-generation connections. - Concurrent warmup/replenishment requests are coalesced — a single `_warmupLoopRunning` guard ensures only one warmup loop executes at a time, and requests that arrive while it is running are dropped (the running loop re-reads the pool count each iteration and drives to the minimum on its own). diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index bf92347f98..5459f81f4e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -146,15 +146,22 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable private int _warmupLoopRunning; /// - /// Test seam controlling how the warmup loop is launched. Defaults to - /// on the thread pool in production. Tests inject a - /// scheduler that runs the loop on a dedicated thread so warmup's first (synchronous, - /// possibly gated) physical open cannot be delayed by thread-pool starvation, keeping - /// timing-sensitive assertions deterministic without mutating global thread-pool state. + /// The most recently launched warmup/replenishment loop, or null if warmup has never been + /// requested (e.g. MinPoolSize == 0 or the pool was already at the minimum). Because + /// concurrent requests are coalesced (see ), this may + /// reference an already-completed loop rather than one started by the latest trigger. /// - private readonly Action> _warmupLoopScheduler; + private Task? _warmupLoopTask; #endregion + /// + /// The most recently launched warmup/replenishment loop task, exposed so tests can await a + /// warmup pass to a deterministic completion instead of polling pool counters. May be null + /// (warmup never requested) or reference an already-completed pass (requests are coalesced); + /// a caller that only needs "some warmup pass has finished" can await it regardless. + /// + internal Task? WarmupLoopTask => Volatile.Read(ref _warmupLoopTask); + /// /// Initializes a new PoolingDataSource. /// @@ -164,8 +171,7 @@ internal ChannelDbConnectionPool( DbConnectionPoolIdentity identity, DbConnectionPoolProviderInfo connectionPoolProviderInfo, ConcurrencyLimiter? connectionCreationRateLimiter = null, - TimeProvider? timeProvider = null, - Action>? warmupLoopScheduler = null) + TimeProvider? timeProvider = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; @@ -176,7 +182,6 @@ internal ChannelDbConnectionPool( MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); TransactedConnectionPool = new(this); _connectionCreationRateLimiter = connectionCreationRateLimiter; - _warmupLoopScheduler = warmupLoopScheduler ?? (static loop => Task.Run(loop)); _connectionSlots = new(MaxPoolSize); _idleChannel = new(); @@ -1047,12 +1052,6 @@ private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection #endregion #region Warmup - /// - /// Delay before retrying a warmup creation that was denied by the shared rate limiter, so - /// warmup waits its turn instead of bypassing the limiter or spinning (Story 2). - /// - private static readonly TimeSpan s_warmupRateLimitRetryDelay = TimeSpan.FromMilliseconds(50); - /// /// Requests background warmup/replenishment: the pool asynchronously pre-creates connections /// up to , serially and through the shared rate limiter. Safe to @@ -1060,13 +1059,14 @@ private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection /// /// This is the single entry point for every warmup trigger: pool startup and any event that /// drops the pool below (connection destruction on return, idle - /// timeout eviction, pruning). Concurrent requests are coalesced by the + /// timeout eviction, pruning). It is also invoked directly by tests to exercise the loop + /// deterministically. Concurrent requests are coalesced by the /// guard so that only one warmup loop ever runs at a time: a /// request that arrives while a loop is already running is simply dropped, because that loop /// re-reads on every iteration and will drive the pool to /// regardless. /// - private void RequestWarmup() + internal void RequestWarmup() { // No-op when there is nothing to pre-create (MinPoolSize == 0), the pool is not running, // or shutdown has cancelled background activity. @@ -1093,16 +1093,16 @@ private void RequestWarmup() try { - // Fire-and-forget on the thread pool (or an injected test scheduler) so warmup never - // blocks the caller. The loop absorbs its own exceptions and always releases the - // single-loop guard on exit. - _warmupLoopScheduler(RunWarmupLoopAsync); + // Fire-and-forget on the thread pool so warmup never blocks the caller. The loop + // absorbs its own exceptions and always releases the single-loop guard on exit. The + // task is published so tests can await a warmup pass to a deterministic completion. + _warmupLoopTask = Task.Run(RunWarmupLoopAsync); } catch (Exception ex) { - // Scheduling the loop failed (e.g. an injected scheduler threw, or the thread pool - // refused the work item). Release the guard so warmup isn't permanently pinned off - // for the life of the pool; the next below-minimum trigger will try again. + // Scheduling the loop failed (e.g. the thread pool refused the work item). Release the + // guard so warmup isn't permanently pinned off for the life of the pool; the next + // below-minimum trigger will try again. Volatile.Write(ref _warmupLoopRunning, 0); SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Failed to schedule warmup loop, absorbing: {1}", Id, ex); @@ -1188,20 +1188,14 @@ private async Task RunWarmupLoopAsync() if (connection is null) { // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), - // and creation failures throw rather than return null, so a null return - // means the shared rate limiter is currently saturated. Wait our turn - // rather than bypassing it (Story 2), then retry. Capacity frees up as user - // requests release their permits. - try - { - await Task.Delay(s_warmupRateLimitRetryDelay, token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - break; - } - - continue; + // and creation failures throw rather than return null, so a null return means + // the shared rate limiter is currently saturated. Rather than bypassing the + // limiter or spinning on it (Story 2), end this warmup pass. Saturation only + // happens while user requests are actively creating connections - those very + // creations fill the pool toward MinPoolSize, and any later drop below the + // minimum re-triggers warmup through RemoveConnection - so warmup does not need + // to compete for a permit here. + break; } // Publish the freshly created connection as idle. It was PrePush'd at creation diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 9edb34c015..7b3d369423 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -32,8 +32,7 @@ private static ChannelDbConnectionPool ConstructPool( int maxPoolSize = 50, int idleTimeout = 0, int loadBalanceTimeout = 0, - ConcurrencyLimiter? connectionCreationRateLimiter = null, - Action>? warmupLoopScheduler = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null) { var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -54,9 +53,7 @@ private static ChannelDbConnectionPool ConstructPool( dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter, - timeProvider: null, - warmupLoopScheduler: warmupLoopScheduler); + connectionCreationRateLimiter); } /// @@ -89,26 +86,25 @@ private static DbConnectionInternal CheckOut(ChannelDbConnectionPool pool) private static readonly TimeSpan HandshakeTimeout = TimeSpan.FromSeconds(30); /// - /// Runs the warmup loop on a dedicated foreground-of-its-own background thread rather than a - /// shared thread-pool worker. - /// - /// The gated test factory blocks warmup's first physical create synchronously. In production - /// warmup runs via Task.Run, so that blocking would occupy a thread-pool worker; under - /// xUnit's parallel execution the shared pool can be momentarily starved and the warmup work - /// item may not be scheduled for seconds, making handshake-based assertions flaky. Giving the - /// loop its own thread removes that dependency on thread-pool scheduling entirely - without - /// mutating any global thread-pool configuration that other tests share. - /// + /// Awaits background warmup passes until the pool settles at . + /// A single coalesced pass can race with concurrent removals - for example a + /// draining the pool while replenishment refills + /// it - so this re-requests warmup until the pool is full. Only safe with a factory whose + /// warmup creations succeed and where the pool is not in the error state, so every pass makes + /// forward progress and the loop terminates. /// - private static readonly Action> DedicatedThreadWarmupScheduler = loop => + private static async Task AwaitWarmupToMinimum(ChannelDbConnectionPool pool, int minPoolSize) { - var thread = new Thread(() => loop().GetAwaiter().GetResult()) + while (pool.Count < minPoolSize) { - IsBackground = true, - Name = "WarmupTest.WarmupLoop", - }; - thread.Start(); - }; + pool.RequestWarmup(); + Task? warmup = pool.WarmupLoopTask; + if (warmup is not null) + { + await warmup; + } + } + } #endregion @@ -125,10 +121,10 @@ public void Startup_MinPoolSizeZero_DoesNotWarmUp() pool.Startup(); - // Give any (erroneous) background warmup a chance to run before asserting it did not. - Assert.False( - WaitFor(() => factory.CreateCount > 0, timeoutMs: 500), - "Warmup created a connection even though MinPoolSize is 0."); + // MinPoolSize == 0, so warmup is a no-op: no loop is ever launched (WarmupLoopTask stays + // null) and nothing is created in the background. + Assert.Null(pool.WarmupLoopTask); + Assert.Equal(0, factory.CreateCount); Assert.Equal(0, pool.Count); } @@ -139,21 +135,18 @@ public void Startup_MinPoolSizeZero_DoesNotWarmUp() [InlineData(1)] [InlineData(3)] [InlineData(10)] - public void Startup_WithMinPoolSize_WarmsUpToMinimum(int minPoolSize) + public async Task Startup_WithMinPoolSize_WarmsUpToMinimum(int minPoolSize) { var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: minPoolSize, maxPoolSize: minPoolSize + 10); pool.Startup(); - Assert.True( - WaitFor(() => pool.Count >= minPoolSize), - $"Warmup did not reach MinPoolSize; Count={pool.Count}, expected {minPoolSize}."); + // Awaiting the warmup loop to completion is deterministic: the single serial pass runs + // with no competing removals, so when it finishes the pool is exactly at MinPoolSize. + await pool.WarmupLoopTask!; - // Warmup must not overshoot the minimum. - Assert.False( - WaitFor(() => pool.Count > minPoolSize, timeoutMs: 500), - $"Warmup overshot MinPoolSize; Count={pool.Count}."); + // Warmup reaches exactly MinPoolSize (no overshoot) and pools each created connection. Assert.Equal(minPoolSize, pool.Count); Assert.Equal(minPoolSize, pool.IdleCount); Assert.Equal(minPoolSize, factory.CreateCount); @@ -170,11 +163,7 @@ public async Task Startup_UserRequestDuringWarmup_ServedImmediately() { using var createGate = new ManualResetEventSlim(initialState: false); var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); - // Run warmup on its own thread so its gated first create can't be delayed by thread-pool - // starvation, making the handshake below deterministic. - using var pool = ConstructPool( - factory, minPoolSize: 3, maxPoolSize: 10, - warmupLoopScheduler: DedicatedThreadWarmupScheduler); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); // Warmup begins and its single, serial creation blocks on the gate. Waiting on this // explicit signal (not a sleep) establishes a deterministic happens-before: warmup is now @@ -220,7 +209,7 @@ public async Task Startup_UserRequestDuringWarmup_ServedImmediately() /// Warmup creates connections through the shared rate limiter and still reaches MinPoolSize. /// [Fact] - public void Warmup_ThroughSharedRateLimiter_ReachesMinimum() + public async Task Warmup_ThroughSharedRateLimiter_ReachesMinimum() { using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); @@ -233,9 +222,10 @@ public void Warmup_ThroughSharedRateLimiter_ReachesMinimum() pool.Startup(); - Assert.True( - WaitFor(() => pool.Count >= 3), - $"Warmup did not reach MinPoolSize through the rate limiter; Count={pool.Count}."); + // Warmup is the only actor here, so each serial create acquires and releases the single + // permit in turn; the pass never sees a saturated limiter and reaches MinPoolSize. + await pool.WarmupLoopTask!; + Assert.Equal(3, pool.Count); // Every warmup creation acquired (and released) a permit from the shared limiter. Assert.True(rateLimiter.GetStatistics()!.TotalSuccessfulLeases >= 3); @@ -257,8 +247,7 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() factory, minPoolSize: 2, maxPoolSize: 5, - connectionCreationRateLimiter: rateLimiter, - warmupLoopScheduler: DedicatedThreadWarmupScheduler); + connectionCreationRateLimiter: rateLimiter); // Warmup begins, acquires the only permit, and blocks in creation while holding it. pool.Startup(); @@ -283,10 +272,10 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() WaitFor(() => rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore), "User request was not denied by the shared rate limiter while warmup held the permit."); - // Release warmup; the pool must still converge to MinPoolSize and the user request - // completes once capacity frees up. + // Release warmup. With rate-limit retry removed, warmup's pass may end early if the + // user request grabs the freed permit first - but the pool must still converge to + // MinPoolSize: whichever creations win the permit (warmup's or the user's) fill it. createGate.Set(); - Assert.True(WaitFor(() => pool.Count >= 2), $"Pool did not reach MinPoolSize; Count={pool.Count}."); // Bound the wait so a regression that never completes the async request fails fast // instead of hanging the whole test run. @@ -296,6 +285,12 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() "Timed out waiting for the deferred user request to complete."); DbConnectionInternal? userConnection = await userRequest; Assert.NotNull(userConnection); + + // Once both the warmup pass and the deferred user request have settled, the pool holds + // exactly MinPoolSize connections (a warmup idle connection plus the user's, or two + // warmup connections one of which the user took). + await AwaitWarmupToMinimum(pool, 2); + Assert.Equal(2, pool.Count); } #endregion @@ -309,39 +304,28 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() /// spinning on the persistent failure. /// [Fact] - public void Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() + public async Task Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() { var factory = new WarmupFailingConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); pool.Startup(); - // Let warmup attempt (and fail) its creations. - Assert.True( - WaitFor(() => factory.WarmupAttemptCount >= 1), - "Warmup never attempted a creation."); - - // Pool never accumulates idle connections and nothing crashes. The failure enters the - // blocking-period error state, exactly as an on-demand creation failure would (Count - // reflects in-flight reservations transiently, so IdleCount is the reliable measure of - // successfully pooled connections). - Assert.False( - WaitFor(() => pool.IdleCount > 0, timeoutMs: 500), - "A warmup connection was pooled despite all creations failing."); + // Await the warmup pass. The first open fails: OpenNewInternalConnection enters the + // blocking-period error state and rethrows, the warmup loop absorbs the rethrow and ends + // the pass. Awaiting is deterministic - no exception surfaces onto the thread pool. + await pool.WarmupLoopTask!; + + // Exactly one attempt was made: a genuine failure ends the pass rather than spinning on + // the persistent failure, and nothing is pooled. + Assert.Equal(1, factory.WarmupAttemptCount); Assert.Equal(0, pool.IdleCount); - Assert.True( - WaitFor(() => pool.ErrorOccurred), - "Warmup failure did not enter the pool error state as the WaitHandle pool does."); - // A genuine creation failure stops the warmup pass instead of retrying on a tight - // cadence, so the failing factory is not hammered. Without a fresh below-minimum trigger - // the attempt count stays bounded (a single failed pass), rather than spinning up dozens - // of attempts against a server that is failing to accept connections. - int attemptsAfterFailure = factory.WarmupAttemptCount; - Thread.Sleep(300); + // Mirroring the legacy WaitHandle pool, the failed open entered the blocking-period error + // state, exactly as an on-demand creation failure would. Assert.True( - factory.WarmupAttemptCount <= attemptsAfterFailure + 1, - $"Warmup spun on a persistent failure: {factory.WarmupAttemptCount} attempts observed."); + pool.ErrorOccurred, + "Warmup failure did not enter the pool error state as the WaitHandle pool does."); } /// @@ -351,75 +335,46 @@ public void Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() /// resumes creating on demand once the blocking period expires. /// [Fact] - public void Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() + public async Task Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() { var factory = new WarmupFailingConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); pool.Startup(); - Assert.True(WaitFor(() => factory.WarmupAttemptCount >= 1), "Warmup never attempted a creation."); // Warmup's failure drives the pool into the blocking-period error state. - Assert.True(WaitFor(() => pool.ErrorOccurred), "Warmup failure did not enter the pool error state."); + await pool.WarmupLoopTask!; + Assert.True(pool.ErrorOccurred, "Warmup failure did not enter the pool error state."); // A user request while the pool is blocking fast-fails with the cached exception. Assert.ThrowsAny(() => CheckOut(pool)); } /// - /// Warmup respects the pool's blocking-period error state: once user requests have driven the - /// pool into the error state, warmup stands down (its loop condition sees ErrorOccurred) and - /// creates nothing, mirroring the legacy WaitHandle pool, which skips replenishment while the - /// pool is blocking. The warmup loop is held at its first instruction until the pool is - /// blocking so the stand-down is observed deterministically. + /// Warmup respects the pool's blocking-period error state: once a failing user request has + /// driven the pool into the error state, invoking warmup must make it stand down (its loop + /// condition sees ErrorOccurred) and create nothing, mirroring the legacy WaitHandle pool, + /// which skips replenishment while the pool is blocking. Driving the error state with a user + /// request (rather than warmup itself) keeps the two behaviors independent, and awaiting the + /// warmup task makes the stand-down deterministic. /// [Fact] - public void Warmup_RespectsErrorState_StandsDownWhileBlocking() + public async Task Warmup_RespectsErrorState_StandsDownWhileBlocking() { - using var warmupStartGate = new ManualResetEventSlim(initialState: false); - using var warmupLoopFinished = new ManualResetEventSlim(initialState: false); - - // Park the warmup loop before its very first iteration until the test has driven the pool - // into the error state, then signal when the loop has run to completion. - Action> gatedStartScheduler = loop => - { - var thread = new Thread(() => - { - warmupStartGate.Wait(); - try - { - loop().GetAwaiter().GetResult(); - } - finally - { - warmupLoopFinished.Set(); - } - }) - { - IsBackground = true, - Name = "WarmupTest.GatedStart", - }; - thread.Start(); - }; - var factory = new UserFailingWarmupCountingConnectionFactory(); - using var pool = ConstructPool( - factory, minPoolSize: 3, maxPoolSize: 10, - warmupLoopScheduler: gatedStartScheduler); - - // Warmup is scheduled on Startup but parked before it runs a single iteration. - pool.Startup(); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); - // While warmup is parked, a failing user request drives the pool into the error state. + // Drive the pool into the blocking-period error state with a failing user request. We do + // NOT call Startup here: that would kick off a *successful* warmup and fill the pool, + // which is not the behavior under test. Assert.ThrowsAny(() => CheckOut(pool)); - Assert.True(WaitFor(() => pool.ErrorOccurred), "User failure did not enter the pool error state."); + Assert.True(pool.ErrorOccurred, "Failing user request did not enter the pool error state."); - // Release the warmup loop. Because the pool is blocking, its loop condition sees the - // error state, so the loop must exit immediately without creating any connections. - warmupStartGate.Set(); - Assert.True( - warmupLoopFinished.Wait(HandshakeTimeout), - "Warmup loop did not complete after being released."); + // Now request warmup directly. Because the pool is blocking, the loop's error-state guard + // makes it stand down immediately: it creates nothing rather than piling doomed opens + // onto a struggling server. + pool.RequestWarmup(); + await pool.WarmupLoopTask!; Assert.Equal(0, factory.WarmupCreateCount); Assert.Equal(0, pool.IdleCount); @@ -435,13 +390,11 @@ public void Warmup_RespectsErrorState_StandsDownWhileBlocking() /// shutdown begins, and any in-flight connection is cleaned up. /// [Fact] - public void Shutdown_DuringWarmup_StopsAndCleansUp() + public async Task Shutdown_DuringWarmup_StopsAndCleansUp() { using var createGate = new ManualResetEventSlim(initialState: false); var factory = new ChannelDbConnectionPoolTest.GatedSuccessfulConnectionFactory(createGate); - using var pool = ConstructPool( - factory, minPoolSize: 3, maxPoolSize: 10, - warmupLoopScheduler: DedicatedThreadWarmupScheduler); + using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); // Warmup begins and blocks on its first serial creation. pool.Startup(); @@ -452,16 +405,13 @@ public void Shutdown_DuringWarmup_StopsAndCleansUp() // Shut down while warmup is parked. This cancels warmup and completes the idle channel. pool.Shutdown(); - // Release the in-flight creation; it must be cleaned up, not pooled. + // Release the in-flight creation and await the loop to completion. The freshly created + // connection cannot be published to the completed idle channel, so it is destroyed, and + // the loop stops without creating anything further. createGate.Set(); + await pool.WarmupLoopTask!; - // No further warmup creations happen after shutdown, and nothing lingers in the pool. - Assert.True( - WaitFor(() => pool.Count == 0), - $"In-flight warmup connection was not cleaned up on shutdown; Count={pool.Count}."); - Assert.False( - WaitFor(() => factory.CreateCount > 1, timeoutMs: 500), - $"Warmup created additional connections after shutdown; CreateCount={factory.CreateCount}."); + Assert.Equal(0, pool.Count); Assert.Equal(1, factory.CreateCount); } @@ -474,25 +424,26 @@ public void Shutdown_DuringWarmup_StopsAndCleansUp() /// triggers replenishment back to the minimum. /// [Fact] - public void Replenish_AfterDoomedReturn_RefillsToMinimum() + public async Task Replenish_AfterDoomedReturn_RefillsToMinimum() { var factory = new DoomableConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); pool.Startup(); - Assert.True(WaitFor(() => pool.Count >= 2), $"Initial warmup failed; Count={pool.Count}."); + await pool.WarmupLoopTask!; + Assert.Equal(2, pool.Count); - // Check out a connection, mark it non-poolable, and return it: it is destroyed on return. + // Check out a connection, mark it non-poolable, and return it: it is destroyed on return, + // dropping the pool to 1 and triggering replenishment through RemoveConnection. var owner = new SqlConnection(); pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection); Assert.NotNull(connection); ((DoomableStubConnection)connection!).MarkDoNotPool(); pool.ReturnInternalConnection(connection, owner); - // Replenishment restores the pool to MinPoolSize. - Assert.True( - WaitFor(() => pool.Count >= 2), - $"Pool did not replenish to MinPoolSize after a doomed return; Count={pool.Count}."); + // The doomed return re-triggered warmup synchronously; awaiting the replenishment pass + // (a single serial create with no competing removal) restores the pool to MinPoolSize. + await pool.WarmupLoopTask!; Assert.Equal(2, pool.Count); } @@ -501,19 +452,20 @@ public void Replenish_AfterDoomedReturn_RefillsToMinimum() /// the minimum with fresh-generation connections. /// [Fact] - public void Replenish_AfterClear_RefillsToMinimum() + public async Task Replenish_AfterClear_RefillsToMinimum() { var factory = new ChannelDbConnectionPoolTest.CountingSuccessfulConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 3, maxPoolSize: 10); pool.Startup(); - Assert.True(WaitFor(() => pool.Count >= 3), $"Initial warmup failed; Count={pool.Count}."); + await pool.WarmupLoopTask!; + Assert.Equal(3, pool.Count); pool.Clear(); - Assert.True( - WaitFor(() => pool.Count >= 3), - $"Pool did not replenish to MinPoolSize after Clear; Count={pool.Count}."); + // Clear drains the pool concurrently with replenishment, so a single warmup pass can race + // with the drain; await passes until the pool settles at MinPoolSize. + await AwaitWarmupToMinimum(pool, 3); Assert.Equal(3, pool.Count); } @@ -522,16 +474,17 @@ public void Replenish_AfterClear_RefillsToMinimum() /// beyond the minimum: the pool stays at MinPoolSize. /// [Fact] - public void Replenish_DestroyWhileAtMinimum_NoOvershoot() + public async Task Replenish_DestroyWhileAtMinimum_NoOvershoot() { var factory = new DoomableConnectionFactory(); using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10); pool.Startup(); - Assert.True(WaitFor(() => pool.Count >= 2), $"Initial warmup failed; Count={pool.Count}."); + await pool.WarmupLoopTask!; + Assert.Equal(2, pool.Count); // Grow above the minimum. Two checkouts reuse the two idle warmup connections; a third - // finds no idle connection and creates a new one, bringing the total to 3. + // finds no idle connection and creates a new one synchronously, bringing the total to 3. var owner1 = new SqlConnection(); var owner2 = new SqlConnection(); var owner3 = new SqlConnection(); @@ -541,17 +494,17 @@ public void Replenish_DestroyWhileAtMinimum_NoOvershoot() Assert.NotNull(c1); Assert.NotNull(c2); Assert.NotNull(c3); - Assert.True(WaitFor(() => pool.Count == 3), $"Expected pool to grow to 3; Count={pool.Count}."); + Assert.Equal(3, pool.Count); - // Destroy the third on return. Dropping from 3 to 2 stays at the minimum, so no - // replenishment overshoot occurs. + // Destroy the third on return. Dropping from 3 to 2 stays at the minimum, so RequestWarmup + // hits its fast path and launches no new pass. + Task? warmupBeforeReturn = pool.WarmupLoopTask; ((DoomableStubConnection)c3!).MarkDoNotPool(); pool.ReturnInternalConnection(c3, owner3); - Assert.True(WaitFor(() => pool.Count == 2), $"Pool did not settle at MinPoolSize; Count={pool.Count}."); - Assert.False( - WaitFor(() => pool.Count > 2, timeoutMs: 500), - $"Pool overshot MinPoolSize after a destroy at the minimum; Count={pool.Count}."); + // No replenishment was triggered: the exposed warmup task is unchanged (no new pass) and + // the pool settled at MinPoolSize without overshooting. + Assert.Same(warmupBeforeReturn, pool.WarmupLoopTask); Assert.Equal(2, pool.Count); } From 275b5912ace608328d20862145bdb221e213611c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 15:09:14 -0700 Subject: [PATCH 12/15] Address review comments on warmup task exposure - Make WarmupLoopTask a plain auto-property (test-only, internal) instead of a Volatile.Read backing field, and move it into the properties region. - Release the single-loop guard with Interlocked.Exchange at both reset sites, symmetric with the Interlocked.CompareExchange acquire. - Rewrite Warmup_RateLimiterSaturated to await the deferred user request and assert the shared-limiter denial after the pool settles, removing the spin-based WaitFor poll. - Remove the WaitFor and CheckOut test helpers (inlined at their call sites). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 35 ++++++------ .../ChannelDbConnectionPoolWarmupTest.cs | 54 ++++++++----------- 2 files changed, 38 insertions(+), 51 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 5459f81f4e..ae2555d02e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -144,24 +144,8 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// requester to start the loop, and reset to 0 by the loop when it drains. /// private int _warmupLoopRunning; - - /// - /// The most recently launched warmup/replenishment loop, or null if warmup has never been - /// requested (e.g. MinPoolSize == 0 or the pool was already at the minimum). Because - /// concurrent requests are coalesced (see ), this may - /// reference an already-completed loop rather than one started by the latest trigger. - /// - private Task? _warmupLoopTask; #endregion - /// - /// The most recently launched warmup/replenishment loop task, exposed so tests can await a - /// warmup pass to a deterministic completion instead of polling pool counters. May be null - /// (warmup never requested) or reference an already-completed pass (requests are coalesced); - /// a caller that only needs "some warmup pass has finished" can await it regardless. - /// - internal Task? WarmupLoopTask => Volatile.Read(ref _warmupLoopTask); - /// /// Initializes a new PoolingDataSource. /// @@ -254,6 +238,16 @@ public ConcurrentDictionary< private uint MaxPoolSize { get; } private int MinPoolSize => PoolGroupOptions.MinPoolSize; + + /// + /// The most recently launched warmup/replenishment loop task, exposed so tests can await a + /// warmup pass to a deterministic completion instead of polling pool counters. May be null + /// (warmup never requested) or reference an already-completed pass (requests are coalesced); + /// a caller that only needs "some warmup pass has finished" can await it regardless. Only + /// consumed by unit tests, which read it after synchronously triggering warmup on their own + /// thread, so a plain auto-property is sufficient. + /// + internal Task? WarmupLoopTask { get; private set; } #endregion #region Methods @@ -1096,14 +1090,14 @@ internal void RequestWarmup() // Fire-and-forget on the thread pool so warmup never blocks the caller. The loop // absorbs its own exceptions and always releases the single-loop guard on exit. The // task is published so tests can await a warmup pass to a deterministic completion. - _warmupLoopTask = Task.Run(RunWarmupLoopAsync); + WarmupLoopTask = Task.Run(RunWarmupLoopAsync); } catch (Exception ex) { // Scheduling the loop failed (e.g. the thread pool refused the work item). Release the // guard so warmup isn't permanently pinned off for the life of the pool; the next // below-minimum trigger will try again. - Volatile.Write(ref _warmupLoopRunning, 0); + Interlocked.Exchange(ref _warmupLoopRunning, 0); SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Failed to schedule warmup loop, absorbing: {1}", Id, ex); } @@ -1227,8 +1221,9 @@ private async Task RunWarmupLoopAsync() finally { // Always release the single-loop guard, whatever exit path we took, so a future - // below-minimum trigger can start a new loop. - Volatile.Write(ref _warmupLoopRunning, 0); + // below-minimum trigger can start a new loop. Interlocked.Exchange mirrors the + // Interlocked.CompareExchange acquire in RequestWarmup. + Interlocked.Exchange(ref _warmupLoopRunning, 0); } } #endregion diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 7b3d369423..e55863c880 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -22,8 +22,6 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool /// public class ChannelDbConnectionPoolWarmupTest { - private const int DefaultTimeoutMs = 5000; - #region Helpers private static ChannelDbConnectionPool ConstructPool( @@ -56,28 +54,6 @@ private static ChannelDbConnectionPool ConstructPool( connectionCreationRateLimiter); } - /// - /// Spins until is true or the timeout elapses. Warmup runs on - /// background tasks, so tests observe its effects by polling pool counters. - /// - private static bool WaitFor(Func condition, int timeoutMs = DefaultTimeoutMs) - => SpinWait.SpinUntil(condition, timeoutMs); - - /// - /// Checks out a single connection from the pool synchronously. - /// - private static DbConnectionInternal CheckOut(ChannelDbConnectionPool pool) - { - bool completed = pool.TryGetConnection( - new SqlConnection(), - taskCompletionSource: null, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), - out DbConnectionInternal? connection); - Assert.True(completed); - Assert.NotNull(connection); - return connection!; - } - /// /// Generous upper bound for waits that guard a background handshake which, in correct code, /// completes in milliseconds. It is only ever hit on a genuine failure/hang, so it does not @@ -183,7 +159,17 @@ public async Task Startup_UserRequestDuringWarmup_ServedImmediately() // upper bound converts a hypothetical "user blocked behind warmup" regression into a clean // failure instead of an indefinite hang; correct code completes in milliseconds. Task userOpen = Task.Factory.StartNew( - () => CheckOut(pool), + () => + { + bool completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + Assert.True(completed); + Assert.NotNull(connection); + return connection!; + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default); @@ -268,15 +254,12 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() Assert.False(completedSynchronously, "Async request unexpectedly completed synchronously."); Assert.Null(immediateConnection); - Assert.True( - WaitFor(() => rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore), - "User request was not denied by the shared rate limiter while warmup held the permit."); - // Release warmup. With rate-limit retry removed, warmup's pass may end early if the // user request grabs the freed permit first - but the pool must still converge to // MinPoolSize: whichever creations win the permit (warmup's or the user's) fill it. createGate.Set(); + // Await the deferred user request directly rather than spin-polling limiter statistics. // Bound the wait so a regression that never completes the async request fails fast // instead of hanging the whole test run. Task userRequest = tcs.Task; @@ -291,6 +274,13 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() // warmup connections one of which the user took). await AwaitWarmupToMinimum(pool, 2); Assert.Equal(2, pool.Count); + + // The user request was denied a permit by the shared limiter while warmup held it, then + // completed once warmup released it. By now that denial has definitely been recorded, so + // this is a deterministic post-settle assertion, not a race. + Assert.True( + rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore, + "User request was not denied by the shared rate limiter while warmup held the permit."); } #endregion @@ -347,7 +337,8 @@ public async Task Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() Assert.True(pool.ErrorOccurred, "Warmup failure did not enter the pool error state."); // A user request while the pool is blocking fast-fails with the cached exception. - Assert.ThrowsAny(() => CheckOut(pool)); + Assert.ThrowsAny(() => pool.TryGetConnection( + new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); } /// @@ -367,7 +358,8 @@ public async Task Warmup_RespectsErrorState_StandsDownWhileBlocking() // Drive the pool into the blocking-period error state with a failing user request. We do // NOT call Startup here: that would kick off a *successful* warmup and fill the pool, // which is not the behavior under test. - Assert.ThrowsAny(() => CheckOut(pool)); + Assert.ThrowsAny(() => pool.TryGetConnection( + new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred, "Failing user request did not enter the pool error state."); // Now request warmup directly. Because the pool is blocking, the loop's error-state guard From c07b1395c22f1dc310cf4675da1d51e14be7a6ad Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 20 Jul 2026 15:11:58 -0700 Subject: [PATCH 13/15] Reconcile warmup spec with implementation The implementation is the source of truth. Correct drifted claims: - Pruning respects the Min Pool Size floor and never itself reduces the pool below the minimum; reword the Description and Story 5 accordingly. - Idle-timeout eviction is lazy (on retrieval), not a background sweep. - Replenishment restores to minimum in a single pass only when not racing a concurrent drain (Clear) or a saturated rate limiter; otherwise convergence completes across re-triggered passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/003-pool-warmup/spec.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/specs/003-pool-warmup/spec.md b/specs/003-pool-warmup/spec.md index 5c5aec793a..8c41b88470 100644 --- a/specs/003-pool-warmup/spec.md +++ b/specs/003-pool-warmup/spec.md @@ -8,7 +8,7 @@ Implement background pool warmup for `ChannelDbConnectionPool`. When the pool starts, it asynchronously pre-creates connections up to Min Pool Size in the background, serially (one at a time), through the shared rate-limiting mechanism used by user-initiated requests. -Whenever the pool count drops below Min Pool Size for any reason (pruning, connection destruction, idle timeout expiry), the pool automatically triggers replenishment using the same serial, rate-limited creation path. +Whenever the pool count drops below Min Pool Size for any reason (a connection destroyed on return, idle-timeout eviction, or any other removal that crosses the floor), the pool automatically triggers replenishment using the same serial, rate-limited creation path. Pruning itself respects the Min Pool Size floor and never reduces the pool below the minimum; it is only mentioned here because it shares the same removal choke point that triggers replenishment. ## User Scenarios & Testing @@ -61,13 +61,13 @@ When a pool is shut down while warmup is still in progress, warmup stops promptl ### User Story 5 — Replenishment on Any Below-Minimum Event (P2) -Whenever the pool count drops below Min Pool Size for any reason, the pool automatically triggers warmup-style replenishment to restore to the minimum. +Whenever the pool count drops below Min Pool Size, the pool automatically triggers warmup-style replenishment to restore to the minimum. Every connection removal funnels through a single choke point (`RemoveConnection`), which triggers replenishment when the removal leaves the pool below the minimum. **Acceptance Scenarios**: -1. **Given** pruning has reduced the pool below the minimum pool size, **When** the pruning cycle completes, **Then** the pool queues a replenishment request. +1. **Given** a pruning cycle runs, **When** it removes idle connections, **Then** it stops at the Min Pool Size floor and does not reduce the pool below the minimum (pruning shares the removal choke point but does not itself cause a below-minimum replenishment). 2. **Given** a connection is destroyed on return to the pool (broken, lifetime expired), **When** the pool count drops below the minimum, **Then** the pool queues a replenishment request. -3. **Given** idle timeout expiry removes a connection, **When** the pool count drops below minimum, **Then** the pool queues a replenishment request. +3. **Given** idle-timeout eviction removes an expired connection on retrieval, **When** the pool count drops below minimum, **Then** the pool queues a replenishment request. 4. **Given** replenishment is triggered, **When** connections are created, **Then** they are created serially through the shared rate limiter, identical to initial warmup. 5. **Given** the pool is at or above the minimum pool size, **When** a connection is destroyed, **Then** no replenishment is triggered. @@ -78,6 +78,7 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Creates connections serially (one at a time) through the shared rate limiter. If the limiter is saturated (no permit available), warmup ends the pass rather than bypassing or spinning on the limiter; convergence to the minimum is handled by the concurrent user creations that saturated it and by re-triggering on the next below-minimum event. - Warmup failures are traced/logged and absorbed by the warmup loop (never surfaced as an unhandled exception). Warmup goes through the same shared creation path as user requests, so a genuine open failure enters the blocking-period error state and a successful open clears it (mirroring the WaitHandle pool). While the error state is active, the warmup loop stands down rather than replenishing. - If a `Clear` races with an in-flight warmup creation, the freshly created (now stale-generation) connection is not special-cased by warmup; it is harmlessly discarded by the liveness/generation check on its next retrieval. After a `Clear`, replenishment refills the pool to the minimum with fresh-generation connections. +- Idle-timeout eviction is lazy: an idle connection past its timeout is removed when it is next retrieved (via the liveness check), not by a background sweep. Pruning is separate and always respects the Min Pool Size floor. - Concurrent warmup/replenishment requests are coalesced — a single `_warmupLoopRunning` guard ensures only one warmup loop executes at a time, and requests that arrive while it is running are dropped (the running loop re-reads the pool count each iteration and drives to the minimum on its own). - Warmup is a no-op when Min Pool Size = 0. @@ -89,6 +90,6 @@ Whenever the pool count drops below Min Pool Size for any reason, the pool autom - Warmup connections are created serially through the shared rate limiter. - Warmup errors are traced/logged and never crash the application. Because warmup shares the on-demand creation path, a genuine open failure enters the pool's blocking-period error state (as it would for a user request) and a successful open clears it. - If the pool is shut down or cleared during warmup, warmup stops gracefully. -- After any event that reduces the pool below Min Pool Size, the pool restores to minimum within one warmup pass. +- After any event that reduces the pool below Min Pool Size, the pool restores to the minimum through replenishment: a single warmup pass suffices when it is not racing a concurrent drain (e.g. `Clear`) or a saturated rate limiter; otherwise convergence completes across subsequent re-triggered passes. - Concurrent warmup/replenishment requests are coalesced so only one warmup loop executes at a time. - Unit tests validate warmup behavior for various Min Pool Size values (0, 1, N) and all replenishment triggers. From a0c8a28d4279f442d4584bf1b60fb5a13946da4f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 21 Jul 2026 08:46:08 -0700 Subject: [PATCH 14/15] Fix flaky rate-limiter warmup test and guard error state after shutdown - Fix CI failure in Warmup_RateLimiterSaturated_UserSharesSameLimiter: the post-settle TotalFailedLeases assertion raced the permit being released, so the deferred user request could acquire the freed permit and never record a denial. Prove shared-limiter saturation deterministically instead, by attempting to acquire the limiter's only permit from the test thread while warmup still holds it (a no-op denied lease), with no spin. - Bound AwaitWarmupToMinimum with HandshakeTimeout so a regression that never reaches the minimum surfaces as a deterministic timeout instead of hanging the test run. - Only enter the blocking-period error state while the pool is Running: an in-flight synchronous open can fail after Shutdown has disposed _errorState, and entering then would re-arm its exit timer past teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 12 +++++- .../ChannelDbConnectionPoolWarmupTest.cs | 43 ++++++++++++------- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ae2555d02e..a948d04844 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -778,7 +778,17 @@ _connectionCreationRateLimiter is not null && // client-side contention rather than a physical connection creation failure and // must not poison the pool into fast-fail/backoff for other callers. // FR-006, FR-007. - _errorState?.Enter(ex); + // + // Only enter the error state while the pool is still Running. A synchronous physical + // open cannot be cancelled once it is in progress (see _warmupCts field docs), so a + // warmup/user open that began before Shutdown can complete with a failure after + // Shutdown has already disposed _errorState. Entering here would re-arm the exit + // timer and keep callbacks/logging alive past teardown, so we skip it once shutdown + // has begun; the throw below still propagates the failure to the caller/warmup loop. + if (State == Running) + { + _errorState?.Enter(ex); + } throw; } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index e55863c880..af2e66081e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -67,12 +67,19 @@ private static ChannelDbConnectionPool ConstructPool( /// draining the pool while replenishment refills /// it - so this re-requests warmup until the pool is full. Only safe with a factory whose /// warmup creations succeed and where the pool is not in the error state, so every pass makes - /// forward progress and the loop terminates. + /// forward progress and the loop terminates. Bounded by so a + /// regression that prevents the pool from ever reaching the minimum surfaces as a + /// deterministic timeout instead of hanging the whole test run. /// private static async Task AwaitWarmupToMinimum(ChannelDbConnectionPool pool, int minPoolSize) { + DateTime deadline = DateTime.UtcNow + HandshakeTimeout; while (pool.Count < minPoolSize) { + Assert.True( + DateTime.UtcNow < deadline, + $"Pool did not reach MinPoolSize {minPoolSize} within {HandshakeTimeout}; Count={pool.Count}."); + pool.RequestWarmup(); Task? warmup = pool.WarmupLoopTask; if (warmup is not null) @@ -241,19 +248,32 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() factory.FirstCreateStarted.Wait(HandshakeTimeout), "Timed out waiting for warmup to begin its first creation."); - // A concurrent user request competes for the same limiter and is denied a permit, - // proving warmup and user requests share one limiter (warmup did not bypass it). - long failedBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; + // A concurrent user request competes for the same limiter. There is no idle connection + // and warmup holds the only permit, so the request cannot be satisfied inline: it is + // deferred to the TaskCompletionSource, proving warmup did not bypass the limiter. var tcs = new TaskCompletionSource(); bool completedSynchronously = pool.TryGetConnection( new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? immediateConnection); - - // The async request cannot be satisfied inline: there is no idle connection and the - // shared limiter's only permit is held by warmup. TryGetConnection therefore returns - // false with no connection, deferring the result to the TaskCompletionSource. Assert.False(completedSynchronously, "Async request unexpectedly completed synchronously."); Assert.Null(immediateConnection); + // Deterministically prove the shared limiter is saturated by warmup: while warmup holds + // its only permit (guaranteed by FirstCreateStarted above and the still-closed gate), + // attempting to acquire that same limiter from the test thread is denied. This is the + // shared-limiter proof without any spin - the denial is observed synchronously here + // rather than by polling for the deferred user request's transient failed-lease, which + // races the permit being released below. The denied attempt is a no-op lease. + long failedBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; + using (RateLimitLease probe = rateLimiter.AttemptAcquire(1)) + { + Assert.False( + probe.IsAcquired, + "Shared rate limiter was not saturated while warmup held its permit."); + } + Assert.True( + rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore, + "Denied acquire was not recorded by the shared rate limiter."); + // Release warmup. With rate-limit retry removed, warmup's pass may end early if the // user request grabs the freed permit first - but the pool must still converge to // MinPoolSize: whichever creations win the permit (warmup's or the user's) fill it. @@ -274,13 +294,6 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() // warmup connections one of which the user took). await AwaitWarmupToMinimum(pool, 2); Assert.Equal(2, pool.Count); - - // The user request was denied a permit by the shared limiter while warmup held it, then - // completed once warmup released it. By now that denial has definitely been recorded, so - // this is a deterministic post-settle assertion, not a race. - Assert.True( - rateLimiter.GetStatistics()!.TotalFailedLeases > failedBefore, - "User request was not denied by the shared rate limiter while warmup held the permit."); } #endregion From 143fb354dea2a423798839dca1f69e77828f8eb7 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 21 Jul 2026 09:12:59 -0700 Subject: [PATCH 15/15] Address review: add idle-timeout eviction replenishment test; add CreateCount to DoomableConnectionFactory - Add Replenish_AfterIdleTimeoutEviction_RefillsToMinimum test (Story 5): covers the case where an idle connection is evicted by IsLiveConnection due to idle-timeout, dropping the pool below MinPoolSize and triggering replenishment via RemoveConnection -> RequestWarmup. The test backdates ReturnedTime to simulate expiry, verifies TryGetConnection evicts the stale connection and returns the fresh one, then awaits WarmupLoopTask and asserts CreateCount increased by 1 and pool.Count returns to MinPoolSize. - Add CreateCount tracking to DoomableConnectionFactory so replenishment tests can assert exactly how many physical connections were created. - Add using Microsoft.Data.SqlClient.Tests.Common for LocalAppContextSwitchesHelper. - Update PR description: remove stale 'test seam' mention; RequestWarmup now always uses Task.Run directly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolWarmupTest.cs | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index af2e66081e..266aa73ea8 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -12,6 +12,7 @@ using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Tests.Common; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool @@ -513,6 +514,55 @@ public async Task Replenish_DestroyWhileAtMinimum_NoOvershoot() Assert.Equal(2, pool.Count); } + /// + /// An idle connection evicted by the idle-timeout check (IsLiveConnection) drops the pool + /// below MinPoolSize and triggers replenishment via RemoveConnection → RequestWarmup, + /// restoring the pool to the minimum. + /// + [Fact] + public async Task Replenish_AfterIdleTimeoutEviction_RefillsToMinimum() + { + using LocalAppContextSwitchesHelper switchesHelper = new(); + switchesHelper.UseLegacyIdleTimeoutBehavior = false; + + // Pool with a 1-second idle timeout and MinPoolSize=2 so an evicted connection + // triggers replenishment. + var factory = new DoomableConnectionFactory(); + using var pool = ConstructPool(factory, minPoolSize: 2, maxPoolSize: 10, idleTimeout: 1); + + pool.Startup(); + await pool.WarmupLoopTask!; + Assert.Equal(2, pool.Count); + int createCountAfterWarmup = factory.CreateCount; + + // Check out both warm-up connections. + var owner1 = new SqlConnection(); + var owner2 = new SqlConnection(); + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + Assert.NotNull(conn1); + Assert.NotNull(conn2); + + // Return conn1 first, then backdate its ReturnedTime to simulate idle expiry beyond the 1s timeout. + pool.ReturnInternalConnection(conn1!, owner1); + conn1!.ReturnedTime = DateTime.UtcNow - TimeSpan.FromSeconds(2); + + // Return conn2 normally — not expired. + pool.ReturnInternalConnection(conn2!, owner2); + + // TryGetConnection pops conn1 (head of channel), finds it idle-expired, calls + // RemoveConnection (which triggers RequestWarmup), then pops conn2 and returns it. + var owner3 = new SqlConnection(); + pool.TryGetConnection(owner3, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn3); + Assert.NotNull(conn3); + Assert.Same(conn2, conn3); + + // The eviction triggered replenishment; await the pass and verify the pool refilled to MinPoolSize. + await pool.WarmupLoopTask!; + Assert.Equal(2, pool.Count); + Assert.Equal(createCountAfterWarmup + 1, factory.CreateCount); + } + #endregion #region Test doubles @@ -554,6 +604,10 @@ internal override void ResetConnection() /// private sealed class DoomableConnectionFactory : SqlConnectionFactory { + private int _createCount; + + internal int CreateCount => Volatile.Read(ref _createCount); + protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, ConnectionPoolKey poolKey, @@ -561,7 +615,10 @@ protected override DbConnectionInternal CreateConnection( IDbConnectionPool pool, DbConnection owningConnection, TimeoutTimer timeout) - => new DoomableStubConnection(); + { + Interlocked.Increment(ref _createCount); + return new DoomableStubConnection(); + } } ///