ChannelDbConnectionPool replace connection

[mdaigle]

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]

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.88%. Comparing base (f5be62b) to head (8736037).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4429      +/-   ##
==========================================
- Coverage   64.77%   62.88%   -1.90%     
==========================================
  Files         288      283       -5     
  Lines       44011    66985   +22974     
==========================================
+ Hits        28510    42126   +13616     
- Misses      15501    24859    +9358     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	62.88% <96.49%> (?)

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.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs:1619

• This call mixes a named argument (forceNewConnection: false) with a following positional argument (overrides), which is a C# compile-time error (positional arguments cannot follow named arguments). Name the overrides argument as well (or revert to fully positional arguments).

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

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

• The PR description is still the template and does not describe what changed or what testing was performed. Please update the PR description with a concise summary of the behavior change (ReplaceConnection implementation + slot replacement) and which unit tests were added/updated.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "<prov.DbConnectionPool.ReplaceConnection|RES|CPOOL> {0}, replacing connection.", Id);

[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:377

• In ReplaceConnection’s create-new branch, the catch block always calls newConnection.DeactivateConnection(). If ActivateConnection(...) throws, EnterActiveConnection() was never executed (it runs after Activate returns), so DeactivateConnection() will decrement the active-connection metric without a matching increment, potentially driving the counter negative. Consider tracking whether ActivateConnection completed successfully and only calling DeactivateConnection() in that case; otherwise just Dispose() the newly created connection.

                    newConnection.ClearGeneration = _clearGeneration;

                    lock (newConnection)
                    {
                        // PostPop requires a lock on the connection.

[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:398

• In the create-new branch, the catch block unconditionally calls newConnection.DeactivateConnection() before Dispose(). If the exception is thrown by PostPop (before an owner is set / before the connection is activated), DeactivateConnection can be an invalid operation and may throw, masking the original failure. Guard the deactivation so it only runs when PostPop succeeded (e.g., Owner != null), while still always disposing the new connection.

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

[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/Resources/Strings.resx:2162

• The new resource key SQL_ConnectionPoolReplaceConnectionFailed was added only to the invariant (default) Strings.resx. All localized satellite .resx files (e.g., Strings.de.resx, Strings.fr.resx, etc.) are missing this key, so non-invariant cultures will fall back to the English text for this message (and some localization validation workflows may fail due to key mismatches). Consider adding this key to each Strings.<culture>.resx (even if the value is temporarily the English string) to keep resource sets in sync.

  <data name="SQL_ConnectionPoolReplaceConnectionFailed" xml:space="preserve">
    <value>Could not replace the connection because it is no longer in the connection pool.</value>
  </data>

[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:400

• In ReplaceConnection's create-new branch, the catch block unconditionally calls newConnection.DeactivateConnection(). If ActivateConnection throws (or anything throws before ActivateConnection completes), DbConnectionInternal.ActivateConnection never reaches Metrics.EnterActiveConnection, but DeactivateConnection always calls Metrics.ExitActiveConnection (DbConnectionInternal.cs:519-538). That can drive the ActiveConnections counter negative and misreport pool usage.

Consider restructuring so DeactivateConnection is only invoked after a successful ActivateConnection (e.g., a nested try/catch around ActivateConnection that disposes on failure, or a boolean flag set after ActivateConnection returns).

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

[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 (2)

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

• Same as above: this TODO is misleading because the code already activates the new connection with the old connection’s enlisted transaction. Rewording the comment helps keep the intent clear.

                    // TODO: Full transaction enlistment support (Story 2).
                    newConnection.ActivateConnection(oldConnection.EnlistedTransaction);

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

• The TODO about "Full transaction enlistment support" is now misleading: this branch already passes the enlisted transaction into PrepareConnection/ActivateConnection. Consider replacing the TODO with an accurate comment describing what the code is doing (or removing it) to avoid confusion.

This issue also appears on line 375 of the same file.

            if (newConnection is not null)
            {
                // TODO: Full transaction enlistment support (Story 2).
                PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction);
                oldConnection.DeactivateConnection();

[mdaigle]

We call CreatePooledConnection directly to skip rate limiting and because we want to swap the new connection into a currently occupied slot.