Address Paul's review: doc/comment clarity and test consolidation

- NoOpAcquiredLease: document that the shared Instance is stateless and its
  Dispose is an idempotent no-op, safe to dispose repeatedly/concurrently.
- ChannelDbConnectionPool: add a class-level pointer to the feature spec so the
  FR-0xx requirement tags in comments are resolvable; note at lease.Dispose()
  that the no-limiter case disposes the idempotent singleton; document the
  faulted flag and how the finally uses it to avoid a redundant idle-channel poke.
- Tests: merge the three ErrorOccurred_* blocking-period facts into a single
  Theory parameterized by the connection string's Pool Blocking Period; comment
  that a rising TotalFailedLeases is how the wake test detects requestB was
  denied a permit and parked on the idle channel.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: mdaigle

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

@@ -48,6 +48,13 @@ namespace Microsoft.Data.SqlClient.ConnectionPool
     ///
     /// The trade-off is slightly higher memory overhead per pool instance due to the channel infrastructure,
     /// but this is generally offset by the performance benefits in async-heavy workloads.
+    ///
+    /// <para>
+    /// Comments in this file reference requirements by tag (e.g. <c>FR-001</c>). These tags are
+    /// defined in the feature spec at <c>specs/006-pool-rate-limiting/spec.md</c> (see the
+    /// "Functional Requirements" section); consult it for the authoritative description of each
+    /// requirement.
+    /// </para>
     /// </summary>
     internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable
     {
@@ -581,6 +588,13 @@ public bool TryGetConnection(
                         // FR-001, FR-002, FR-003.
 
                         RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance;
+
+                        // Tracks whether we are leaving this block via an exception rather than a
+                        // normal return. It is read in the finally below to decide whether to poke
+                        // the idle channel: every return path sets it to false first, so by the time
+                        // the finally runs it is true only when an exception is propagating. This
+                        // lets us skip the wake on exception paths, where cleanupCallback already
+                        // pokes, and avoid a redundant double wake.
                         bool faulted = true;
                         try
                         {
@@ -624,6 +638,9 @@ public bool TryGetConnection(
                             // null poke and retry its acquire before the permit is actually returned,
                             // fail to acquire, and fall back to waiting with no subsequent signal -
                             // stalling connection creation even though the limiter has capacity.
+                            // When no limiter is configured this is NoOpAcquiredLease.Instance, whose
+                            // Dispose is an idempotent no-op, so disposing the shared singleton here
+                            // is safe.
                             lease.Dispose();
 
                             // After releasing, signal a waiter on the idle channel that they may now

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

@@ -19,7 +19,10 @@ namespace Microsoft.Data.SqlClient.ConnectionPool
     internal sealed class NoOpAcquiredLease : RateLimitLease
     {
         /// <summary>
-        /// The shared singleton instance.
+        /// The shared singleton instance. Because it is stateless and its <see cref="Dispose"/>
+        /// releases nothing, the same instance is handed to every caller that opens without a
+        /// configured limiter and may be disposed any number of times, on any thread, without
+        /// affecting other callers.
         /// </summary>
         public static readonly NoOpAcquiredLease Instance = new();
 
@@ -39,7 +42,9 @@ public override bool TryGetMetadata(string metadataName, out object? metadata)
 
         protected override void Dispose(bool disposing)
         {
-            // No resources to release.
+            // No resources to release. This is intentionally idempotent: the shared Instance is
+            // handed to every no-limiter open, so Dispose may be called repeatedly and
+            // concurrently with no effect.
         }
     }
 }

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

@@ -1610,46 +1610,21 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly()
 
         /// <summary>
         /// Verifies that a connection creation failure enters the blocking-period error state when
-        /// blocking is enabled for the pool.
+        /// blocking is enabled for the pool, and stays out of it when blocking is disabled. The
+        /// blocking policy is driven by the connection string's Pool Blocking Period:
+        /// Default/Auto enable blocking for a non-Azure host (localhost), AlwaysBlock forces it on,
+        /// and NeverBlock suppresses it. FR-006, FR-007.
         /// </summary>
-        [Fact]
-        public void ErrorOccurred_FailureWithBlockingEnabled_BecomesTrue()
-        {
-            // Arrange
-            // Default PoolBlockingPeriod is Auto; localhost is non-Azure so blocking is enabled.
-            var dbConnectionPoolGroup = new DbConnectionPoolGroup(
-                new SqlConnectionOptions("Data Source=localhost;"),
-                new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null),
-                new DbConnectionPoolGroupOptions(
-                    poolByIdentity: false,
-                    minPoolSize: 0,
-                    maxPoolSize: 4,
-                    creationTimeout: 15,
-                    loadBalanceTimeout: 0,
-                    hasTransactionAffinity: true,
-                    idleTimeout: 0));
-            var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup);
-
-            // Act
-            Assert.False(pool.ErrorOccurred);
-
-            Assert.Throws<InvalidOperationException>(() =>
-                pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _));
-
-            // Assert
-            Assert.True(pool.ErrorOccurred);
-        }
-
-        /// <summary>
-        /// Verifies that a connection creation failure does not enter the blocking-period error state
-        /// when the connection string disables blocking with NeverBlock.
-        /// </summary>
-        [Fact]
-        public void ErrorOccurred_FailureWithNeverBlock_StaysFalse()
+        [Theory]
+        [InlineData("", true)]                                // Default (unspecified) => Auto => blocks for localhost
+        [InlineData("Pool Blocking Period=Auto;", true)]      // Auto => blocks for non-Azure host
+        [InlineData("Pool Blocking Period=NeverBlock;", false)]
+        [InlineData("Pool Blocking Period=AlwaysBlock;", true)]
+        public void ErrorOccurred_OnFailure_FollowsBlockingPeriod(string blockingPeriodClause, bool expectErrorOccurred)
         {
             // Arrange
             var dbConnectionPoolGroup = new DbConnectionPoolGroup(
-                new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"),
+                new SqlConnectionOptions($"Data Source=localhost;{blockingPeriodClause}"),
                 new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null),
                 new DbConnectionPoolGroupOptions(
                     poolByIdentity: false,
@@ -1662,40 +1637,13 @@ public void ErrorOccurred_FailureWithNeverBlock_StaysFalse()
             var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup);
 
             // Act
-            Assert.Throws<InvalidOperationException>(() =>
-                pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _));
-
-            // Assert - FR-007: NeverBlock must not enter the error state.
             Assert.False(pool.ErrorOccurred);
-        }
 
-        /// <summary>
-        /// Verifies that a connection creation failure enters the blocking-period error state when
-        /// the connection string explicitly enables AlwaysBlock.
-        /// </summary>
-        [Fact]
-        public void ErrorOccurred_FailureWithAlwaysBlock_BecomesTrue()
-        {
-            // Arrange
-            var dbConnectionPoolGroup = new DbConnectionPoolGroup(
-                new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"),
-                new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null),
-                new DbConnectionPoolGroupOptions(
-                    poolByIdentity: false,
-                    minPoolSize: 0,
-                    maxPoolSize: 4,
-                    creationTimeout: 15,
-                    loadBalanceTimeout: 0,
-                    hasTransactionAffinity: true,
-                    idleTimeout: 0));
-            var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup);
-
-            // Act
             Assert.Throws<InvalidOperationException>(() =>
                 pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _));
 
             // Assert
-            Assert.True(pool.ErrorOccurred);
+            Assert.Equal(expectErrorOccurred, pool.ErrorOccurred);
         }
 
         /// <summary>
@@ -1956,6 +1904,9 @@ public async Task RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysical
                 "Timed out waiting for the first open to begin physical creation.");
 
             // Caller B is denied a permit (A holds it) and must fall back to the idle-channel wait.
+            // A denied AttemptAcquire increments the limiter's TotalFailedLeases, so watching that
+            // counter rise is how we confirm B was refused a permit and is now parked on the idle
+            // channel waiting for a wakeup (rather than still racing to acquire).
             long failedLeasesBefore = rateLimiter.GetStatistics()!.TotalFailedLeases;
             Task<DbConnectionInternal?> requestB = Open(new SqlConnection());
             Assert.True(