Address PR #4302 review feedback and drop spec FR-00x labels

  Code review fixes (WaitHandleDbConnectionPool):
  - TryGetConnection shutdown short-circuit now releases the PoolSemaphore
    slot when WaitAny was woken by SEMAPHORE_HANDLE (or
    WAIT_ABANDONED+SEMAPHORE_HANDLE), preventing a semaphore-slot leak that
    would starve future waiters.
  - Shutdown() releases Volatile.Read(ref _waitCount) slots instead of
    MaxPoolSize, fixing ArgumentOutOfRangeException when MaxPoolSize == 0
    (unlimited pool) and ensuring every parked waiter is woken under high
    contention. Kept SemaphoreFullException guard.

  Test fix:
  - Shutdown_UnblocksSyncWaiter replaces Thread.Sleep(200) with a bounded
    poll on the private _waitCount field (via the existing reflection
    helper) so the test is deterministic on slow CI agents.

  Documentation cleanup:
  - Removed inline 'FR-00x' spec-traceability labels from comments and test
    section banners across both pool implementations and both shutdown
    test files. Explanatory text is preserved; the spec coverage will live
    in the PR description instead of the source.

Author: priyankatiwari08

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

@@ -95,7 +95,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool
 
         /// <summary>
         /// Tracks whether <see cref="Shutdown"/> has already initiated the shutdown sequence so that
-        /// repeated calls are observed as no-ops (FR-006). Updated atomically via
+        /// repeated calls are observed as no-ops. Updated atomically via
         /// <see cref="Interlocked.CompareExchange(ref int, int, int)"/>.
         /// </summary>
         private int _shutdownInitiated;
@@ -273,7 +273,7 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti
         /// <inheritdoc />
         public void Shutdown()
         {
-            // FR-006: idempotent. Compare-and-exchange ensures only one caller performs shutdown work.
+            // idempotent. Compare-and-exchange ensures only one caller performs shutdown work.
             if (Interlocked.CompareExchange(ref _shutdownInitiated, 1, 0) != 0)
             {
                 return;

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs

@@ -329,7 +329,7 @@ public bool IsRunning
 
         private void CleanupCallback(object state)
         {
-            // FR-005: if the pool is shutting down, skip work. Shutdown disposes the timer, but
+            // If the pool is shutting down, skip work. Shutdown disposes the timer, but
             // a callback may already be in-flight when Shutdown runs; this guard ensures it does
             // not perform pruning or re-arm pool create requests.
             if (State == ShuttingDown)
@@ -775,7 +775,7 @@ private void DestroyObject(DbConnectionInternal obj)
 
         private void ErrorCallback(object state)
         {
-            // FR-005: skip work if the pool is shutting down. The shutdown path disposes the
+            // Skip work if the pool is shutting down. The shutdown path disposes the
             // timer; this guard handles the in-flight-callback race.
             if (State == ShuttingDown)
             {
@@ -971,11 +971,26 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj
                     {
                         waitResult = WaitHandle.WaitAny(_waitHandles.GetHandles(allowCreate), unchecked((int)waitForMultipleObjectsTimeout));
 
-                        // FR-007: after waking, observe shutdown state and bail out so waiters
-                        // do not spin against a drained pool.
+                        // After waking, observe shutdown state and bail out so waiters
+                        // do not spin against a drained pool. If WaitAny consumed a
+                        // PoolSemaphore slot, release it back so the accounting stays
+                        // balanced; otherwise the slot would leak and other waiters
+                        // (or callers that arrive after Shutdown completes its own
+                        // Release loop) would starve.
                         if (State != Running)
                         {
                             SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Pool is shutting down; abandoning wait.", Id);
+                            if (waitResult == SEMAPHORE_HANDLE || waitResult == WAIT_ABANDONED + SEMAPHORE_HANDLE)
+                            {
+                                try
+                                {
+                                    _waitHandles.PoolSemaphore.Release(1);
+                                }
+                                catch (SemaphoreFullException)
+                                {
+                                    // Pool semaphore was already saturated by Shutdown's bulk release; safe to ignore.
+                                }
+                            }
                             Interlocked.Decrement(ref _waitCount);
                             connection = null;
                             return false;
@@ -1507,35 +1522,40 @@ public void Shutdown()
         {
             SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.Shutdown|RES|INFO|CPOOL> {0}", Id);
 
-            // FR-006: idempotent. Subsequent calls observe ShuttingDown and bail.
+            // Idempotent: subsequent calls observe ShuttingDown and bail.
             if (State == ShuttingDown)
             {
                 return;
             }
             State = ShuttingDown;
 
-            // FR-005: dispose all background timers so they no longer schedule new work.
+            // Dispose all background timers so they no longer schedule new work.
             // Note that any timer callback already in flight may still observe State == ShuttingDown
             // and short-circuit (see CleanupCallback / ErrorCallback).
             Timer cleanup = Interlocked.Exchange(ref _cleanupTimer, null);
             cleanup?.Dispose();
             Timer error = Interlocked.Exchange(ref _errorTimer, null);
             error?.Dispose();
 
-            // FR-007: wake any threads parked in WaitHandle.WaitAny by releasing the pool semaphore
-            // up to its max count. Waiters check State == Running after wake-up and bail.
-            // We may release more than necessary; SemaphoreFullException is harmless when the
-            // semaphore is already saturated.
-            try
+            // Wake any threads parked in WaitHandle.WaitAny by releasing as many semaphore
+            // slots as there are recorded waiters. Using _waitCount (rather than MaxPoolSize)
+            // avoids ArgumentOutOfRangeException when MaxPoolSize == 0 (unlimited) and ensures
+            // we wake every parked waiter even when _waitCount exceeds MaxPoolSize. Waiters
+            // observe State != Running after wake-up and bail.
+            int waitersToWake = Volatile.Read(ref _waitCount);
+            if (waitersToWake > 0)
             {
-                _waitHandles.PoolSemaphore.Release(MaxPoolSize);
-            }
-            catch (SemaphoreFullException)
-            {
-                // Already at max count - all slots free. Nothing to do.
+                try
+                {
+                    _waitHandles.PoolSemaphore.Release(waitersToWake);
+                }
+                catch (SemaphoreFullException)
+                {
+                    // Semaphore already saturated; nothing to do.
+                }
             }
 
-            // FR-003: drain idle stacks and destroy contained connections. Active checked-out
+            // Drain idle stacks and destroy contained connections. Active checked-out
             // connections are destroyed when they are returned (see DeactivateObject's
             // State is ShuttingDown branch).
             while (_stackNew.TryPop(out DbConnectionInternal newObj))

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolShutdownTest.cs

@@ -15,7 +15,6 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool
 {
     /// <summary>
     /// Deterministic tests for <see cref="ChannelDbConnectionPool"/> shutdown behavior.
-    /// Verifies the contract defined by spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope).
     /// </summary>
     public class ChannelDbConnectionPoolShutdownTest
     {
@@ -41,7 +40,7 @@ private static ChannelDbConnectionPool ConstructPool(int maxPoolSize = 5)
                 new DbConnectionPoolProviderInfo());
         }
 
-        // FR-001
+        // State transitions to ShuttingDown on Shutdown.
         [Fact]
         public void Shutdown_TransitionsState_ToShuttingDown()
         {
@@ -55,7 +54,7 @@ public void Shutdown_TransitionsState_ToShuttingDown()
             Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State);
         }
 
-        // FR-003
+        // Drains buffered idle connections.
         [Fact]
         public void Shutdown_DrainsIdleConnections()
         {
@@ -84,7 +83,7 @@ public void Shutdown_DrainsIdleConnections()
             Assert.Equal(0, pool.Count);
         }
 
-        // FR-004 - returned connection while shutting down is destroyed, not pooled.
+        // Returned connection while shutting down is destroyed, not pooled.
         [Fact]
         public void Shutdown_ReturnedConnection_IsDestroyedNotPooled()
         {
@@ -104,7 +103,7 @@ public void Shutdown_ReturnedConnection_IsDestroyedNotPooled()
             Assert.Equal(0, pool.Count);
         }
 
-        // FR-006
+        // Shutdown is idempotent.
         [Fact]
         public void Shutdown_IsIdempotent()
         {
@@ -116,7 +115,7 @@ public void Shutdown_IsIdempotent()
             Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State);
         }
 
-        // FR-007 - async waiter is unblocked when the pool shuts down.
+        // Async waiter is unblocked when the pool shuts down.
         [Fact]
         public async Task Shutdown_UnblocksAsyncWaiter()
         {
@@ -153,7 +152,7 @@ public async Task Shutdown_UnblocksAsyncWaiter()
             }
         }
 
-        // Story 3 acceptance #3 - sync get fails fast after shutdown.
+        // Sync get fails fast after shutdown.
         // The factory-level retry guard checks IsRunning, but the pool itself must not vend
         // new connections after Shutdown. We verify by exhausting the pool first then
         // checking that returned connections are destroyed (Count goes back to 0).

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolShutdownTest.cs

@@ -14,8 +14,7 @@
 namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool
 {
     /// <summary>
-    /// Deterministic tests for <see cref="WaitHandleDbConnectionPool"/> shutdown behavior
-    /// per spec 004-pool-shutdown FR-001 .. FR-007 (P1 scope).
+    /// Deterministic tests for <see cref="WaitHandleDbConnectionPool"/> shutdown behavior.
     /// </summary>
     public class WaitHandleDbConnectionPoolShutdownTest
     {
@@ -50,7 +49,7 @@ private static T GetPrivateField<T>(object instance, string fieldName)
             return (T)field!.GetValue(instance)!;
         }
 
-        // FR-001
+        // State transitions to ShuttingDown on Shutdown.
         [Fact]
         public void Shutdown_TransitionsState_ToShuttingDown()
         {
@@ -63,7 +62,7 @@ public void Shutdown_TransitionsState_ToShuttingDown()
             Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State);
         }
 
-        // FR-005 - cleanup timer is disposed.
+        // Cleanup timer is disposed.
         [Fact]
         public void Shutdown_DisposesCleanupTimer()
         {
@@ -77,7 +76,7 @@ public void Shutdown_DisposesCleanupTimer()
             Assert.Null(afterTimer);
         }
 
-        // FR-005 - error timer is disposed when present.
+        // Error timer is disposed when present.
         [Fact]
         public void Shutdown_DisposesErrorTimer_WhenPresent()
         {
@@ -94,7 +93,7 @@ public void Shutdown_DisposesErrorTimer_WhenPresent()
             Assert.Null(afterTimer);
         }
 
-        // FR-003 - drains idle stacks.
+        // Drains idle stacks.
         [Fact]
         public void Shutdown_DrainsIdleStacks()
         {
@@ -119,7 +118,7 @@ public void Shutdown_DrainsIdleStacks()
             Assert.Equal(0, pool.Count);
         }
 
-        // FR-006
+        // Shutdown is idempotent.
         [Fact]
         public void Shutdown_IsIdempotent()
         {
@@ -130,7 +129,7 @@ public void Shutdown_IsIdempotent()
             Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State);
         }
 
-        // FR-005 acceptance #3 - cleanup-callback after shutdown is a no-op.
+        // Cleanup callback after shutdown is a no-op.
         [Fact]
         public void CleanupCallback_AfterShutdown_IsNoOp()
         {
@@ -147,7 +146,7 @@ public void CleanupCallback_AfterShutdown_IsNoOp()
             Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State);
         }
 
-        // FR-005 acceptance #3 - error-callback after shutdown is a no-op.
+        // Error callback after shutdown is a no-op.
         [Fact]
         public void ErrorCallback_AfterShutdown_IsNoOp()
         {
@@ -161,7 +160,7 @@ public void ErrorCallback_AfterShutdown_IsNoOp()
             Assert.Null(ex);
         }
 
-        // FR-007 - sync caller arriving after shutdown gets a null connection (factory will
+        // Sync caller arriving after shutdown gets a null connection (factory will
         // see this and return up the retry chain). The pool's TryGetConnection short-circuits
         // on State != Running.
         [Fact]
@@ -180,7 +179,7 @@ public void TryGetConnection_AfterShutdown_ReturnsNullWithoutBlocking()
             Assert.Null(conn);
         }
 
-        // FR-007 - shutdown wakes up a thread parked in WaitHandle.WaitAny.
+        // Shutdown wakes up a thread parked in WaitHandle.WaitAny.
         [Fact]
         public void Shutdown_UnblocksSyncWaiter()
         {
@@ -213,8 +212,15 @@ public void Shutdown_UnblocksSyncWaiter()
             { IsBackground = true };
             t.Start();
 
-            // Give the waiter time to enter WaitAny.
-            Thread.Sleep(200);
+            // Wait deterministically until the worker has incremented _waitCount, which
+            // happens immediately before it enters WaitHandle.WaitAny. Polling avoids the
+            // CI-flakiness of a fixed Thread.Sleep on slow agents.
+            var deadline = DateTime.UtcNow.AddSeconds(5);
+            while (DateTime.UtcNow < deadline && GetPrivateField<int>(pool, "_waitCount") < 1)
+            {
+                Thread.Yield();
+            }
+            Assert.True(GetPrivateField<int>(pool, "_waitCount") >= 1, "Waiter did not park within 5s.");
             Assert.True(t.IsAlive, "Waiter should be parked, but thread already exited.");
 
             pool.Shutdown();