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

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

This pull request implements the connection replacement logic in the ChannelDbConnectionPool, adds atomic slot replacement support to ConnectionPoolSlots, and thoroughly tests the new behavior. It also improves XML documentation and error handling for connection replacement scenarios.

This functionality is used to replace broken connections during command execution.

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 92.18750% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.63%. Comparing base (25dba6b) to head (553330b).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4429      +/-   ##
==========================================
- Coverage   64.61%   62.63%   -1.98%     
==========================================
  Files         288      283       -5     
  Lines       44046    67041   +22995     
==========================================
+ Hits        28459    41990   +13531     
- Misses      15587    25051    +9464

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`62.63% <92.18%> (?)`

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.

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.

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

paulmedynski

Looks great, with one suggestion that I should have made the first time!

xUnit constructs a fresh test-class instance per test, so the tunable factory can be an instance field rather than being newed up in every test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

The replace tests built pools from 'Data Source=localhost;' and relied on the default Pool Blocking Period (Auto). Auto derives the policy from ADP.IsAzureSqlServerEndpoint(), which reads the process-wide mutable ADP.s_azureSqlServerEndpoints list. ConnectionRoutingTestsAzure registers 'localhost' as an Azure endpoint in its constructor and only restores the list on dispose, and xUnit runs separate collections in parallel. A pool constructed inside that window is treated as Azure, so the blocking-period error state is never created and every assertion expecting ErrorOccurred to become true fails. Pin Pool Blocking Period=AlwaysBlock so the tests no longer depend on that global state. This also stops the ErrorOccurred=false assertions from passing vacuously when the error state was simply absent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

Comments suppressed due to low confidence (1)

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

In ReplaceConnection's create-new branch, the catch block always calls newConnection.DeactivateConnection(). If PostPop(...) or ActivateConnection(...) throws, ActivateConnection never reaches SqlClientDiagnostics.Metrics.EnterActiveConnection(), so DeactivateConnection() will decrement the active-connection counter without a matching increment (and can drive metrics negative). It also runs even when activation never completed.

Only deactivate when activation actually succeeded; otherwise dispose directly.

                catch
                {
                    newConnection.DeactivateConnection();
                    newConnection.Dispose();
                    throw;

The simulated-server Azure routing tests register "localhost" as an Azure endpoint in the process-wide ADP.s_azureSqlServerEndpoints list for the duration of their class. A pool resolves whether the blocking period is enabled exactly once, in its constructor, so any pool built inside that window classifies localhost as Azure and silently never blocks. Tests asserting ErrorOccurred == true therefore flake under parallel collection execution. Pin Pool Blocking Period=AlwaysBlock where the test needs blocking enabled but is not testing endpoint classification, and switch the one theory that genuinely exercises the Auto => non-Azure mapping to a data source that cannot be poisoned. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Conflict in ChannelDbConnectionPoolTest.cs between this branch's blocking-period flake fix and the quarantine applied on main in #4476. Both sides targeted the same failures. #4476 quarantined them as racing the background warmup work from #4452; this branch instead identified and fixed the actual root cause: the simulated-server Azure routing tests register "localhost" as an Azure endpoint in the process-wide ADP.s_azureSqlServerEndpoints list, and a pool resolves whether the blocking period is enabled exactly once at construction, so a pool built inside that window silently never blocks. The quarantined set corroborates this: it is exactly the tests asserting ErrorOccurred == true, while the sibling tests asserting false - which would race identically under a warmup hypothesis, but pass vacuously when the error state is never created - were not quarantined. The affected pools in ChannelDbConnectionPoolTest also use minPoolSize 0, so no warmup runs at all. Resolved by keeping the root-cause fix and lifting the quarantine from the 12 tests it repairs. Unrelated quarantines from #4476 are left in place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

Copilot

Pull request overview

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

Files not reviewed (1)

src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

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

Copilot started reviewing on behalf of mdaigle July 29, 2026 16:21 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle mentioned this pull request Jul 29, 2026

ChannelDbConnectionPool transaction support #4487

Draft

4 tasks

paulmedynski requested changes Jul 29, 2026

View reviewed changes

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

Use a single tunable factory instance member in the replace tests

7049271

xUnit constructs a fresh test-class instance per test, so the tunable factory can be an instance field rather than being newed up in every test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 29, 2026 18:10

Copilot started reviewing on behalf of mdaigle July 29, 2026 18:11 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

paulmedynski previously approved these changes Jul 29, 2026

View reviewed changes

paulmedynski moved this from Waiting for customer to In review in SqlClient Board Jul 29, 2026

cheenamalhotra previously approved these changes Jul 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 30, 2026 16:31

mdaigle dismissed stale reviews from cheenamalhotra and paulmedynski via f17552f July 30, 2026 16:32

Copilot started reviewing on behalf of mdaigle July 30, 2026 16:32 View session

paulmedynski previously approved these changes Jul 30, 2026

View reviewed changes

Copilot AI reviewed Jul 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 30, 2026 16:44

mdaigle dismissed paulmedynski’s stale review via f4fab68 July 30, 2026 16:44

Copilot started reviewing on behalf of mdaigle July 30, 2026 16:44 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

Potential fix for pull request finding

65da630

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 30, 2026 16:49

Copilot started reviewing on behalf of mdaigle July 30, 2026 16:49 View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

cheenamalhotra approved these changes Jul 30, 2026

View reviewed changes

Uh oh!

Conversation

mdaigle commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

paulmedynski left a comment

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!

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!

4 participants

mdaigle commented Jul 8, 2026 •

edited

Loading

codecov Bot commented Jul 8, 2026 •

edited

Loading