ChannelDbConnectionPool replace connection by mdaigle · Pull Request #4429 · dotnet/SqlClient

mdaigle · 2026-07-08T01:50:36Z

Description

Provide a summary of the changes being introduced. Important topics to cover
include:

Description of the functionality.
API changes, backwards compatibility, deprecations, etc.
Documentation, localization.
Bug fixes.
Code hygiene, refactoring, improvements.
Engineering processes (CI, pipelines, test coverage)

High quality descriptions will lead to a smoother review experience.

Issues

Link to any relevant issues, bugs, or discussions (e.g., Closes #123, Fixes issue #456).

Testing

Describe the automated tests (unit, integration) you created or modified.
Provide justification for any gap in automated testing. List any manual testing
steps that were performed to ensure the changes work.

Guidelines

Please review the contribution guidelines before submitting a pull request:

Copilot

Pull request overview

Implements connection replacement support for the channel-based connection pool, including a new slot-level atomic replace operation and expanded unit test coverage around replacement behavior.

Changes:

Implement ChannelDbConnectionPool.ReplaceConnection(...) and introduce internal replacement acquisition logic (idle-preferred, otherwise create new).
Add ConnectionPoolSlots.TryReplace(...) to atomically swap a connection within an existing reserved slot.
Update/extend unit tests, including a new dedicated ChannelDbConnectionPoolReplaceConnectionTest suite and an updated existing replacement test.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs	Adds new tests validating `ConnectionPoolSlots.TryReplace` behaviors.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Updates the existing replace-connection test to exercise the new implementation.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs	Adds a new test file covering replacement scenarios (idle preference, capacity behavior, failure propagation).
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs	Changes `ReplaceConnection` return type to nullable.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs	Adds `TryReplace` helper to atomically swap a connection in-place.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Implements `ReplaceConnection`, adds replacement acquisition logic, and updates `PrepareConnection` to accept an optional transaction.

codecov · 2026-07-08T03:23:50Z

Codecov Report

❌ Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.07%. Comparing base (fdebcd2) to head (2f880ce).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs	75.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4429      +/-   ##
==========================================
- Coverage   65.83%   64.07%   -1.77%     
==========================================
  Files         287      282       -5     
  Lines       43763    66706   +22943     
==========================================
+ Hits        28812    42741   +13929     
- Misses      14951    23965    +9014

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`64.07% <79.59%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

+                if (!replaced)
+                {
+                    // This should never happen because oldConnection is checked out and its slot is stable.
+                    // Still, we need to check or we could vend a connection that is not tracked by the pool.
+                    // TODO: error types and localization
+                    throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced.");
+                }


+        /// <summary>
+        /// Verifies that when activating the replacement connection fails, the newly created
+        /// connection is returned to the pool rather than leaked, keeping the pool count stable.
+        /// </summary>
+        [Fact]
+        public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool()


+            // Assert — the new connection was returned to pool (not leaked).
+            // Pool count stays same because the new connection replaced the old one's slot
+            // and was then returned to idle.
+            Assert.Equal(countBefore, pool.Count);


+        // NOTE: Preferring an idle connection over a new physical one is being added in a follow-up PR
+        // (branch dev/automation/replace-conn-idle-path), along with its ReplaceConnection_PrefersIdleOverNewConnection test.


        [Fact]
        public void TestReplaceConnection()
        {


Introduce an optional System.Threading.RateLimiting policy that throttles new physical connection opens in the channel pool: when a permit is denied the caller waits for a returned connection instead of forcing a create, and leases are always released (including on failure) to avoid starvation. Adds NoOpAcquiredLease, wires the RateLimiting package into the product and test projects, and includes the 006-pool-rate-limiting spec. Also repairs two pre-existing build breaks in ChannelDbConnectionPoolTest (a dropped CountingSuccessfulConnectionFactory declaration and DbConnectionPoolGroupOptions calls missing the new idleTimeout argument).

The connection pool only needs a concurrency limiter (pooling against on-prem SQL Server), so change ChannelDbConnectionPool to take a concrete System.Threading.RateLimiting.ConcurrencyLimiter? instead of the abstract RateLimiter base. The limiter remains optional (null = no limiting), and AttemptAcquire(1)/RateLimitLease usage is unchanged (both inherited). Rework the three rate-limiter unit tests to use real ConcurrencyLimiter instances and assert via GetStatistics() (CurrentAvailablePermits, TotalFailedLeases) instead of the now-removed TestRateLimiter double. Update the spec and diagram to describe a concurrency limiter specifically, noting other limiter types can be added later if needed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Remove the two "options to consider" TODOs above the AttemptAcquire call and replace them with a comment explaining why non-blocking fast-fail was chosen over failing immediately or blocking on the limiter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Drop the two "options to consider" TODOs above the AttemptAcquire call. The rationale for choosing non-blocking fast-fail lives in the PR discussion rather than in code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…nel-rate-limiting

Remove the redundant leaseAcquired local; read lease.IsAcquired directly in the early-return guard and the finally-block poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate exercises a single-permit ConcurrencyLimiter with two sequential opens against distinct owners. A leaked lease on the success path would deny the second open, so asserting both create physical connections (CreateCount == 2) guards the release-on-success behavior at the behavioral level rather than only via the permit counter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Cover the previously untested concurrency behavior where a caller blocked purely by rate limiting is woken by another caller's lease release (the finally-block null poke) and then creates its own physical connection. RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysicalConnection is a [Theory] over the sync and async idle-channel wait mechanisms. It uses a new GatedSuccessfulConnectionFactory that blocks the first physical create so the permit is held in-flight while a second caller is denied and parks on the idle channel; releasing the gate triggers the release poke that must wake and satisfy the waiter. Verified the test fails (waiter times out) when the poke is disabled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…osal - Exclude OperationCanceledException from the creation-failure catch so a caller's own timeout/cancellation no longer poisons the pool blocking period. - Gate the finally idle-channel poke to non-faulted completion via a faulted flag, avoiding a redundant double wake on exception paths (cleanupCallback already writes a wake). - Document that the pool does not own the injected ConcurrencyLimiter and never disposes it (caller owns its lifetime). - Fix comment typo (rather then -> rather than) and trailing whitespace. - Reword spec User Story 1 / FR-002 from strict FIFO to best-effort idle-channel wait, matching the non-blocking AttemptAcquire implementation. - Dispose ConcurrencyLimiter instances in tests (using var) and drop the unused System.Collections.Generic using. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…cit method parameters.

…e/replace-conn-2 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

                    statistics = SqlStatistics.StartTimer(Statistics);

-                    if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides)))
+                    if (!(IsProviderRetriable ? TryOpenWithRetry(null, forceNewConnection: false, overrides) : TryOpen(null, forceNewConnection: false, overrides)))


+                        // Should never happen (oldConnection is checked out, so its slot is stable),
+                        // but guard against vending a connection the pool isn't tracking.
+                        // TODO: error types and localization
+                        throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced.");
+                    }


+    public class ChannelDbConnectionPoolReplaceConnectionTest
+    {


Revert the named-to-positional reversions that crept into the TryOpenInner call sites in SqlConnectionConcurrentOpenTests and SqlConnectionStateTransitionTests, restoring the readable forceNewConnection: false/true form to match the rest of the branch and the call sites on main. Also restore the accidental missing space after the comma in the TryOpenWithRetry parameter list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

+
+namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool
+{
+    public class ChannelDbConnectionPoolReplaceConnectionTest


+            SqlConnection owner = new();

-            // Act & Assert
-            Assert.Throws<NotImplementedException>(() => pool.ReplaceConnection(null!, null!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))));
+            pool.TryGetConnection(
+                owner,
+                taskCompletionSource: null,
+                TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)),
+                out DbConnectionInternal? oldConnection);
+
+            Assert.NotNull(oldConnection);
+
+            var newConnection = pool.ReplaceConnection(owner, oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)));
+            Assert.NotNull(newConnection);
+            Assert.NotSame(oldConnection, newConnection);
        }


+                        // Should never happen (oldConnection is checked out, so its slot is stable),
+                        // but guard against vending a connection the pool isn't tracking.
+                        // TODO: error types and localization
+                        throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced.");
+                    }


…t/SqlClient into dev/mdaigle/replace-conn-2

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

mdaigle · 2026-07-16T00:23:11Z

+                newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout);
+


Two parts here:

Cleanup: if CreatePooledConnection throws, newConnection is never assigned and oldConnection is deliberately left intact — it is the anchor the outer reconnect loop retries against (see the header comment on this method). So there is nothing to clean up on that path, and keeping the create call outside the try is correct.

Error state: not tripping the pool's blocking-period error state on a reconnect is intentional. A replace is a targeted recovery of a single broken connection with its own retry budget; letting one reconnect failure poison the pool-wide backoff would penalize unrelated callers acquiring fresh connections. Happy to revisit if we decide reconnect failures should honor/contribute to the error state, but this is a deliberate behavioral choice rather than an oversight.

mdaigle · 2026-07-16T00:23:10Z

+                    {
+                        // Should never happen (oldConnection is checked out, so its slot is stable),
+                        // but guard against vending a connection the pool isn't tracking.
+                        // TODO: error types and localization
+                        throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced.");
+                    }


This is a defensive guard for an invariant that shouldn't be reachable in practice: oldConnection is checked out by the caller, so its slot is stable and TryReplace cannot fail. Rather than add a one-off resource string for an unreachable path, the existing // TODO: error types and localization tracks converting this message (and the ad-hoc exception type) as part of a consistent pass over this method's error handling. Leaving the TODO in place to track it.

mdaigle · 2026-07-16T00:23:08Z

        {
            // Arrange
            var pool = ConstructPool(SuccessfulConnectionFactory);


Fixed in f430f80 — updated the summary to describe what the test actually asserts (a checked-out connection is replaced with a new, distinct instance), and moved it out of the "Not Implemented Method Tests" region into a dedicated "Replace Connection Tests" region.

- Remove stray blank doc line that split the forceNewConnection <remarks> sentence into two paragraphs in generated docs. - Correct the TestReplaceConnection summary (it no longer asserts NotImplementedException) and move it out of the 'Not Implemented Method Tests' region into a dedicated 'Replace Connection Tests' region. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

mdaigle · 2026-07-16T18:54:23Z

+                oldConnection.Dispose();
+            }
+
+            SqlClientDiagnostics.Metrics.SoftConnectRequest();


Good catch — addressed in a0659c6. The new-physical-connection branch of ReplaceConnection now mirrors OpenNewInternalConnection for the honor/clear halves:

Honors the blocking period via _errorState?.ThrowIfActive() before CreatePooledConnection, so a replacement fast-fails instead of hammering an unhealthy server.

Clears the backoff ramp via _errorState?.Clear() after a successful open, since a successful physical open proves the server is reachable.

Two deliberate asymmetries, both documented inline:

Idle reuse (the if branch) stays exempt — like the normal acquire path, the blocking period gates new physical creation only, not reuse of an already-slotted idle connection.

A reconnect failure still does not Enter the error state. A targeted reconnect must not be able to poison the whole pool; this is asserted by ReplaceConnection_CreationFails_OldConnectionRetainedForRetry (Assert.False(pool.ErrorOccurred)).

Added ReplaceConnection_NewPhysicalConnection_RespectsBlockingPeriod covering the observable half (create branch fast-fails while blocking). I intentionally did not add a clear-on-success test: because ThrowIfActive throws exactly when ErrorOccurred is true, the create branch only reaches Clear() when the error is already inactive, so Clear() only resets the unobservable ramp — a test asserting ErrorOccurred == false there would be tautological.

mdaigle · 2026-07-16T18:01:25Z

+                    {
+                        // Should never happen (oldConnection is checked out, so its slot is stable),
+                        // but guard against vending a connection the pool isn't tracking.
+                        // TODO: error types and localization
+                        throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced.");
+                    }


This is an unreachable defensive guard: oldConnection is checked out by the caller, so its slot is stable and TryReplace cannot fail. The inline // TODO: error types and localization tracks giving this path a proper (localized, resource-backed) exception as part of a consistent pass over this method's error handling, rather than adding a one-off resource string for a path that shouldn't occur in production. Keeping the TODO to track it.

mdaigle · 2026-07-16T18:01:22Z

+
+namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool
+{
+    public class ChannelDbConnectionPoolReplaceConnectionTest


Fixed in 81f6958 — added a class-level <summary> describing what the suite covers (idle reuse, new-connection creation, pool-slot accounting at/below capacity, and the failure paths that keep the old connection available for the caller's retry loop).

- Add a class-level XML summary to ChannelDbConnectionPoolReplaceConnectionTest describing the behavior under test. - Replace the try/catch that swallowed the expected InvalidOperationException in ReplaceConnection_ActivationFails_NewConnectionReturnedToPool with an explicit Assert.Throws, matching the sibling failure-path tests and making the intent fail-safe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

+using System;
+using System.Data.Common;
+using System.Transactions;
+using Microsoft.Data.Common;
+using Microsoft.Data.Common.ConnectionString;
+using Microsoft.Data.ProviderBase;
+using Microsoft.Data.SqlClient.ConnectionPool;
+using Xunit;


+        /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never
        /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is
        /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state


…path The create branch of ReplaceConnection now honors the pool's blocking-period error state (ThrowIfActive) before opening a new physical connection and clears the backoff ramp on a successful open, mirroring OpenNewInternalConnection. Idle reuse stays exempt, and a reconnect failure still does not enter the error state by design, so a targeted reconnect cannot poison the pool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

+using Microsoft.Data.Common;
+using Microsoft.Data.Common.ConnectionString;


Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 8, 2026 01:50

github-project-automation Bot added this to SqlClient Board Jul 8, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jul 8, 2026

Copilot started reviewing on behalf of mdaigle July 8, 2026 01:50 View session

mdaigle changed the title ~~Dev/mdaigle/replace conn 2~~ ChannelDbConnectionPool replace connection Jul 8, 2026

mdaigle added this to the 7.1.0-preview3 milestone Jul 8, 2026

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 10, 2026 00:02

Copilot started reviewing on behalf of mdaigle July 10, 2026 00:02 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

mdaigle and others added 19 commits July 13, 2026 16:38

Merge remote-tracking branch 'origin/main' into dev/mdaigle/pool-chan…

029ce7e

…nel-rate-limiting

Inline leaseAcquired into lease.IsAcquired checks

662b36d

Remove the redundant leaseAcquired local; read lease.IsAcquired directly in the early-return guard and the finally-block poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Remove instance-level ForceNewConnection property. Replace with expli…

9b91a45

…cit method parameters.

Fix initialization. Add doc comments.

08d24b3

Remove unnecessary internal API surface.

08eff42

Expose param.

e350b42

Address broken test and copilot comments.

e2e1a72

WIP

63edefe

Add unit tests.

949d9b2

Wording

d684191

improve error handling

23cfa38

Address copilot comments.

ae75925

Merge branch 'dev/mdaigle/pool-channel-rate-limiting' into dev/mdaigl…

f7f7f63

…e/replace-conn-2 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 15, 2026 23:17

Copilot started reviewing on behalf of mdaigle July 15, 2026 23:17 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 15, 2026 23:38

Copilot started reviewing on behalf of mdaigle July 15, 2026 23:38 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

mdaigle added 2 commits July 15, 2026 16:45

clean up comments

fbf37d7

Merge branch 'dev/mdaigle/replace-conn-2' of https://github.com/dotne…

0d19e4e

…t/SqlClient into dev/mdaigle/replace-conn-2

Copilot AI review requested due to automatic review settings July 15, 2026 23:45

Copilot started reviewing on behalf of mdaigle July 15, 2026 23:46 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 16, 2026 00:22

Copilot started reviewing on behalf of mdaigle July 16, 2026 00:22 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 16, 2026 18:00

Copilot started reviewing on behalf of mdaigle July 16, 2026 18:01 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 16, 2026 18:54

Copilot started reviewing on behalf of mdaigle July 16, 2026 18:54 View session

Condense blocking-period comments in ReplaceConnection

21c99a0

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

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread ...ata.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs

Comment on lines +8 to +9

using Microsoft.Data.Common;

using Microsoft.Data.Common.ConnectionString;

Copilot AI review requested due to automatic review settings July 16, 2026 18:59

Copilot started reviewing on behalf of mdaigle July 16, 2026 18:59 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Base automatically changed from dev/mdaigle/pool-channel-rate-limiting to main July 18, 2026 00:27

		// NOTE: Preferring an idle connection over a new physical one is being added in a follow-up PR
		// (branch dev/automation/replace-conn-idle-path), along with its ReplaceConnection_PrefersIdleOverNewConnection test.

		newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout);

		using Microsoft.Data.Common;
		using Microsoft.Data.Common.ConnectionString;

Uh oh!

Conversation

mdaigle commented Jul 8, 2026

Description

Issues

Testing

Guidelines

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 8, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!