Emit pool metrics and traces, and fix Count semantics in ChannelDbConnectionPool

[mdaigle]

Rebased onto main and now also contains the tracing parity work that was previously PR #4505.

ChannelDbConnectionPool did not emit any of the connection pool counters that WaitHandleDbConnectionPool emits, reported Count as the reservation count (which includes in-flight creations), and traced far less than the wait handle pool.

Bringing the two pools to counter parity surfaced several counter bugs that affect both pools. Those fixes are called out separately below.

Metrics

• Wire the pooled/free connection and soft/hard connect/disconnect counters through ChannelDbConnectionPool and IdleConnectionChannel, at the call sites matching WaitHandleDbConnectionPool.

Count semantics

• Add ConnectionPoolSlots.ConnectionCount and point Count at it, so Count reflects connections that actually belong to the pool rather than reservations held for connections still being opened. This matches WaitHandleDbConnectionPool and fixes the SQL Express user instance path, which branches on pool.Count <= 0.
• Gate warmup on ReservationCount instead of Count, matching the max-pool-size gate and the pruner. Count excludes in-flight creations, so warmup created duplicates for connections other threads were already opening.

Tracing parity

• Add pooler traces to match the wait handle pool, including a per-reason trace for connections rejected by the liveness gate and a trace for connections held by a transaction (which otherwise vanish from the trace stream after deactivation).
• Traces in TransactedConnectionPool keep the existing format, since it is shared with the wait handle pool and reformatting it would change v1's trace stream. PoolPruner is constructed only by ChannelDbConnectionPool, so it takes the new format. Converting the rest of the repo to a single trace format is left to its own PR.

Injectable metrics sink

• Add IDbConnectionPool.Metrics and let both pool implementations take a SqlClientMetrics in their constructor, defaulting to the global SqlClientDiagnostics.Metrics. SqlConnectionFactory holds its own sink and feeds it to each connection through DbConnectionInternal's constructor, so a test can observe connection-level counters too.
• Pools take their sink independently of the factory's so they can be constructed and asserted on in isolation. Both default to the same process-wide instance, so they agree in production.
• This gives the tests a per-test counter instance, so the metric tests can assert exact absolute counts rather than deltas against process-wide state. The metric tests are parameterized over both pool implementations, which is what establishes the parity.

Counter bugs fixed in both pools

Each of these drove a publicly documented gauge away from reality permanently, since a gauge never recovers from an unmatched increment or decrement.

Bug	Gauge affected	Symptom
ReplaceConnection counted the replacement checkout but nothing for the connection it displaced	active-soft-connects	Drifted up by one per replacement
ReplaceConnection disposed the displaced connection without recording its destruction	active-hard-connections	Drifted up by one per replacement (v1 only; v2 already handled this)
CreateObject swapped the replacement into the old connection's place but only counted the addition	number-of-pooled-connections	Drifted up by one per replacement (v1 only)
Both pools counted the checkout after activating, so a failed activation returned the connection and emitted an unpaired soft disconnect	active-soft-connects	Went negative
ActivateConnection counted after calling Activate while DeactivateConnection counts before calling Deactivate	number-of-active-connections	Went negative on failed activation
The wait handle pool counted a soft connect even when it vended no connection	active-soft-connects	Drifted up
ConnectionPoolSlots.Add disposed an opened connection on its failure path without recording the destruction	active-hard-connections	Drifted up (v2 only)

Counter semantics these were fixed against, for the record: a soft connect is the pool vending a connection, a soft disconnect is a connection returning to the pool, a hard connect is a new physical connection, and a hard disconnect is a connection being destroyed.

Known gap

number-of-reclaimed-connections reads flat zero under ChannelDbConnectionPool. ReclaimedConnectionRequest is only emitted by the wait handle pool, because the channel pool does not yet reclaim emancipated connections. That work is tracked in #4490 and the counter will be wired up there.

Suggested release note entry

Fixed several connection pool event counters that could drift permanently or report negative values. active-soft-connects, active-hard-connections, number-of-pooled-connections, and number-of-active-connections were miscounted when a connection was replaced or when a pooled connection failed to activate.

• Tests added or updated
• Public API changes documented (no public API changes)
• Ensure no breaking changes introduced

Tests: the pool instrumentation tests are consolidated in DbConnectionPoolInstrumentationTest and parameterized over both pools. They assert every counter's exact value, treating any counter the test does not name as zero, so an unexpected emission fails. New coverage in this PR: failed activation leaves all counters balanced, concurrent checkout/return settles every gauge to a consistent resting state, and idle pruning records the destruction it performs. The failed-activation test is what surfaced the two negative gauges. The stasis gauge also gained unit coverage, which it previously had none of: all three ways a pool puts a connection in stasis, and both ways it leaves.

Not verified: net462. This branch was developed on macOS, where that target cannot be built.

Deferred to separate PRs: emancipated-connection reclamation (#4490), surfacing the underlying cause of a pooled-open timeout as an inner exception, and the async idle-connection fast path.

[Copilot]

Pull request overview

This PR closes parity gaps between ChannelDbConnectionPool (V2) and WaitHandleDbConnectionPool (V1) discovered via differential testing, focusing on correct pooling semantics and consistent diagnostics/metrics behavior across implementations.

Changes:

• Emit pool metrics in the channel-based pool to match the wait-handle pool (pooled/free/active connection counters and connect/disconnect-related counters).
• Fix ChannelDbConnectionPool.Count semantics to report tracked connections (slot occupancy) rather than in-flight reservations.
• Add an async idle-connection fast path so OpenAsync can complete synchronously on a warm pool (excluding transactional requests), and update/parameterize tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Implements async idle fast path, fixes Count to use tracked connections, and wires metrics at key lifecycle points.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs	Adds ConnectionCount to distinguish tracked connections from reservations.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs	Emits free-connection metrics on idle enqueue/dequeue to centralize counter correctness.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Updates stress test to avoid hanging when async acquisition can complete synchronously.
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs	Parameterizes pooled connection metrics test across pool versions via ConnectionPoolVersionScope.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs	Parameterizes resiliency SPID test across pool versions via ConnectionPoolVersionScope.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs	Fixes Task.Factory.StartNew(async …) by unwrapping the nested task so failures/timeouts are observed correctly.

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• The new async idle fast-path can return a pooled connection without ever emitting the standard "Getting connection." pooler trace (it bypasses GetInternalConnection, where that trace is currently emitted). This makes trace captures inconsistent: a successful async open from the idle channel will only show "Popped from general pool" without the usual request-start marker, reducing diagnosability and trace parity with WaitHandleDbConnectionPool.

            Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction;

            // Try to satisfy the request synchronously from the idle channel before paying for a

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

• Pruning is documented as respecting the MinPoolSize floor, but the loop condition uses ReservationCount. Since ReservationCount includes in-flight opens, pruning can run even when the actual tracked-connection count (Count/ConnectionCount) is already at MinPoolSize, potentially dropping the pool below its minimum. WaitHandleDbConnectionPool’s cleanup loop uses Count > MinPoolSize for this floor check.

            while (count > 0
                && IsRunning
                && _connectionSlots.ReservationCount > MinPoolSize
                && _idleChannel.TryRead(out var connection))

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolInstrumentationTest.cs:33

• This test class mutates process-wide AppContext switches (e.g., IdleTimeoutEviction_EmitsReasonTrace sets UseLegacyIdleTimeoutBehavior). Without serializing via AppContextSwitchTestCollection, this can race with other tests and cause cross-test contamination/flakiness.

namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool
{
    /// <summary>
    /// Verifies the diagnostic instrumentation of <see cref="ChannelDbConnectionPool"/>: the pooler
    /// trace events emitted across the connection lifecycle.
    /// </summary>
    public class ChannelDbConnectionPoolInstrumentationTest
    {
        /// <summary>

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

• PruneConnections uses _connectionSlots.ReservationCount to enforce the MinPoolSize floor. ReservationCount includes in-flight connection creations, so pruning can drop the pool below MinPoolSize (in terms of actual tracked connections) while a create is pending; if that create later fails, the pool can end up permanently under the configured minimum. Since Count now reflects tracked connections (WaitHandle pool semantics), use Count/ConnectionCount here for parity and to keep the floor based on actual connections.

            while (count > 0
                && IsRunning
                && _connectionSlots.ReservationCount > MinPoolSize
                && _idleChannel.TryRead(out var connection))

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolInstrumentationTest.cs:33

• This test class mutates process-wide cached AppContext switch values (UseLegacyIdleTimeoutBehavior) and also asserts against process-wide metric gauges. To avoid cross-test interference/flakiness, it should be placed in the existing non-parallel AppContextSwitchTests collection (same pattern used by ChannelDbConnectionPoolTest and *IdleTimeoutTest).

    public class ChannelDbConnectionPoolInstrumentationTest
    {

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• Pruning currently uses _connectionSlots.ReservationCount > MinPoolSize as its floor check. After this PR changes Count to reflect actual tracked connections (excluding in-flight creates), using ReservationCount here can cause the pruner to remove idle connections based on reservations held for connections still opening. If those in-flight opens fail/cancel, the pool can end up below MinPoolSize even though the loop condition was intended to enforce the floor. This should use Count (or _connectionSlots.ConnectionCount) to match the new semantics and the WaitHandle pool’s pruning logic.

            while (count > 0
                && IsRunning
                && _connectionSlots.ReservationCount > MinPoolSize
                && _idleChannel.TryRead(out var connection))

[Copilot]

Pull request overview

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

Suppressed comments (4)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:777

• The created pooled connection also receives the factory sink, so its active-connection counters do not honor IDbConnectionPool.Metrics when the injected sinks differ. Select pool.Metrics for pooled creation and retain the factory sink only when pool is null.

                metrics: Metrics);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:228

• Pooled hard-connects are still emitted through the factory's sink rather than the owning pool's Metrics. Because the pool and factory accept independent injected sinks, a pool-scoped sink can miss every hard connect (the tests currently hide this by passing the same fake to both). Route pooled connects through pool.Metrics; keep the factory sink for non-pooled connections.

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

            Metrics.HardConnectRequest();

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

• The tracing-parity behavior is untested: the new instrumentation suite asserts counters only, and the connection-pool tests contain no listener/assertions for TryPoolerTraceEvent. Add trace capture tests for lifecycle events and each liveness-rejection/transaction reason so missing or malformed events do not silently regress.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
                Id,
                MinPoolSize,
                MaxPoolSize);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:101

• The new in-flight reservation distinction is not covered by a regression test. Existing ConnectionPoolSlotsTest assertions only inspect ReservationCount after creation callbacks finish, so they cannot catch ConnectionCount accidentally tracking reservations again. Add a gated creation test that observes ReservationCount == 1 and ConnectionCount == 0 while creation is blocked, then verifies both are 1 after insertion (and that the channel pool's Count follows ConnectionCount).

        /// <summary>
        /// Gets the number of connections currently tracked by this collection. Unlike
        /// <see cref="ReservationCount"/>, this excludes reservations held for connections that are
        /// still being opened, so it reports connections that actually belong to the pool.
        /// </summary>
        internal int ConnectionCount => _connectionCount;

[cheenamalhotra]

Closing the metrics gap before v2 becomes the default is the right call, and parameterizing the tests over both pools is the correct way to lock parity in. Ran the new tests locally, 13/13 green on net9.0.

Most of my comments are about counters that don't balance, and about Count now meaning two different things inside the same class. Two more asks that don't belong on a line:

• Can we get the full v1 vs v2 counter delta written down in the description and a release note? ReclaimedConnectionRequest is never emitted by the channel pool (deferred to #4490), so that counter reads a flat zero the moment someone flips UseConnectionPoolV2. Anyone comparing dashboards across the switch will open a support case, and I'd rather we documented it than debugged it.
• The new per-connection traces add roughly four events per checkout on the hottest path we have. TryPoolerTraceEvent is IsEnabled-gated, but the Id / ObjectID arguments still box into the params object[] at every call site whether tracing is on or not. Can we confirm with a benchmark that tracing-off throughput is unchanged?

[cheenamalhotra]

We emit a soft connect for the replacement but never a soft disconnect for the connection being retired, so one logical checkout produces two soft connects. activeSoftConnections climbs by one per replacement and never comes back down.

Same in WaitHandle, so not a regression, but ReplaceConnection is the connection resiliency path. A customer on a flaky network watches "connections currently in use" grow without bound while the pool is actually idle, and that counter is one of the first things we ask them for.

ReplaceConnection_CountsHardDisconnectForDiscardedConnection now asserts softConnects: 2, softDisconnects: 0 with a single connection in hand, so the test reads as if this were intended. Can we either emit the missing SoftDisconnectRequest() for oldConnection in both pools, or say plainly in the test that the imbalance is known and file it?

[mdaigle]

Fixed in both pools. ReplaceConnection now records a soft disconnect for the connection it displaces, plus a hard disconnect, since that connection is destroyed rather than returned.

Chasing this turned up a second one in v1: CreateObject swaps the replacement into the old connection's place in _objectList but only counted the addition, so number-of-pooled-connections drifted up by one per replacement. Fixed alongside.

[cheenamalhotra]

The soft connect lands after PrepareConnection, but ReturnInternalConnection emits its soft disconnect unconditionally on the first line. So if activation throws, the async path's catch returns the connection and we decrement a counter we never incremented, and activeSoftConnections goes negative.

The sync path is worse. There's no catch around this at all, so a PrepareConnection failure leaves the connection neither returned nor disposed. It keeps its slot and its pooledConnections and activeHardConnections contributions until GC gets to it, and this pool has no emancipated reclamation to notice.

Can we move the soft connect to the handoff point the way WaitHandle does, and wrap PrepareConnection so a failure goes through RemoveConnection?

[mdaigle]

Ordering fixed in both pools. The imbalance was real and a bit worse than described: number-of-active-connections also went negative, because ActivateConnection counted after calling Activate while DeactivateConnection counts before calling Deactivate. Both now count before the virtual call. Added FailedActivation_LeavesCountersBalanced over both pools, which is what caught it.

Two corrections though. PrepareConnection has its own try/catch that calls ReturnInternalConnection and rethrows, so the sync path doesn't leak the connection. And the wait handle pool had the identical ordering, so this wasn't a v2 regression.

[cheenamalhotra]

Agreed on the fix for the user instance path, but this leaves the class with two definitions of "how big is the pool" that now differ by exactly the in-flight opens:

• PruneConnections gates on ReservationCount > MinPoolSize
• RequestWarmup / RunWarmupLoopAsync gate on Count < MinPoolSize

So warmup can't see opens that are already in flight and will over-create above MinPoolSize, while the pruner stays conservative. Can we pick a definition per decision and say at each site why that one is right?

Separately, SqlConnectionFactory.PruneConnectionPoolGroups reads pool.Count == 0 to decide a released pool has drained, and emits ExitInactiveConnectionPool() off the back of it. With in-flight opens no longer counted, a pool can be declared empty while an open that started before shutdown is still landing, and we decrement the gauge for a pool that still owns a connection. WaitHandle's _totalObjects behaves the same way so this isn't new, but this PR is what moves the value. Worth a line in the remarks confirming it's deliberate.

[mdaigle]

Warmup now gates on ReservationCount, matching the max-pool-size gate and the pruner.

Documented the convention on Count itself, including why the SQL Express user instance path in SqlConnectionFactory specifically needs Count rather than ReservationCount.

[cheenamalhotra]

The catch below hands a fully opened connection to cleanupCallback, which disposes it. That connection was already counted by HardConnectRequest in the factory, and nothing emits the matching HardDisconnectRequest, so activeHardConnections leaks.

It's the same failure shape you deliberately balanced in ReplaceConnection's catch, which is what makes it stand out. Only reachable through the "reserved but no empty slot" bug path today, but that callback is the general cleanup hook and the next caller to use it inherits the hole.

Minor while you're here: _connectionCount is declared volatile but only ever mutated through Interlocked. Pick one. (_reservations does the same, so low priority.)

[mdaigle]

Both addressed. The cleanup path now records the hard disconnect for a connection that opened but never reached the pool.

On volatile: dropped it from the fields, but the reads go through Volatile.Read now rather than becoming plain reads. TryReserve's spin loop reads the field outside the Interlocked call, so an unordered read there could be hoisted.

[cheenamalhotra]

Only ActivateConnection and DeactivateConnection were moved onto this. SetInStasis, TerminateStasis, the non-pooled HardDisconnectRequest and both Enter/ExitNonPooledConnection calls still go to SqlClientDiagnostics.Metrics, so one object reports to two sinks depending on which counter fires.

The test consequence is the part I care about: AssertCounters claims any counter it doesn't name must be zero, but stasisConnections and nonPooledConnections can never be non-zero on the fake. Those rows pass vacuously and give us false confidence, and stasis is the counter sitting right next to the #3640 double-deactivation we're regression testing here.

Can we route the rest through the instance too? If there's a reason not to, the vacuous rows should come out of AssertCounters with a note saying why.

[mdaigle]

Routed the remaining sites through the instance sink.

Worth noting this is a no-op in production. The factory hands every connection the process-wide instance, so Metrics already is SqlClientDiagnostics.Metrics there. The value is that a test can now observe those counters.

[cheenamalhotra]

The pool and the factory now each take their own optional ISqlClientMetrics, with no relationship between them. Wire them to different instances and the pooled/free counters land in one sink while the hard connect and active connection counters land in another, silently and with nothing asserting otherwise.

Production is safe because both default to the global, but this is a seam we're adding for tests and it has a wrong way to hold it. Can the pool just take its sink from ConnectionFactory.Metrics so there's one owner, or at minimum Debug.Assert they're the same instance?

Also, is this interface intended to be the seam for a future public metrics / OpenTelemetry story? If so let's say so here, so we don't relitigate a 22-member internal interface next time someone reads it as test-only scaffolding.

[mdaigle]

This one is deliberate, and I've documented it on the property. Pools take their sink independently of the factory's so a pool can be constructed and asserted on without standing up a factory. Both default to the same process-wide instance, so they agree in production, and the tests pass a single instance to both.

[cheenamalhotra]

TransactedConnectionPool and PoolPruner are shared with WaitHandleDbConnectionPool, so reformatting the traces here changes what v1 users see too, and a v1 trace stream now carries both formats at once.

The new format also drops the RES|CPOOL tokens. Those are what support tooling and customer greps filter on, so this is consumer visible whichever pool you're on.

I'm fine with the new format, but can we either convert everything in one pass in its own PR, or leave the shared components alone in this one? Either way it needs a release note.

[mdaigle]

Agreed, reverted. PoolPruner is now unchanged from main, and TransactedConnectionPool keeps only the metrics injection.

Converting the repo to one trace format seems worth doing, just in its own PR.

[mdaigle]

Correction on this: PoolPruner keeps the new format after all. It is constructed only by ChannelDbConnectionPool and its constructor is typed to it, so it is not shared with the wait handle pool.

TransactedConnectionPool is genuinely shared and stays on the existing format.

[cheenamalhotra]

This block can't fail as written. stasisConnections and nonPooledConnections are only ever emitted through SqlClientDiagnostics.Metrics (see my comment on DbConnectionInternal), so the fake never sees them and these rows are decorative.

I like the "everything unnamed is zero" design, it's the right shape. It just needs the emitters actually pointed at the injected sink, otherwise it advertises coverage we don't have.

[mdaigle]

The stasis row is meaningful now that the routing is fixed.

Keeping the nonPooledConnections row at zero on purpose. AssertCounters treats any counter a test doesn't name as zero, so these rows exist to catch unexpected emissions rather than to assert a behavior.

[cheenamalhotra]

FailingSqlConnectionFactory and TestConnectionCreateException aren't referenced by any test in this file. Dead code, or is a test missing?

If it's the latter, the ones I'd most like to see land with it:

• Failed activation, asserting the counters stay balanced (the path I flagged in GetInternalConnection).
• Pruning and idle-timeout eviction, which are the two channel-pool-only paths that touch pooledConnections and freeConnections.
• A concurrent checkout/return loop asserting every gauge returns to zero at the end.

That last one matters most. Every test here is single threaded, and gauge drift under concurrency is the failure mode that actually reaches customers. Good coverage otherwise, and nice that the commit/rollback pair genuinely drives the transaction completed path via the base EnlistedTransaction setter rather than faking it.

[mdaigle]

FailingSqlConnectionFactory is gone. Repurposed TestConnectionCreateException for a fake that opens successfully and then fails to activate, which drives the new failed-activation test.

[priyankatiwari08]

Following on @cheenamalhotra's benchmark ask — it should cover the metrics calls too, not just tracing. SoftConnectRequest/SoftDisconnectRequest fire on every pooled open/close and moved from a direct call on a sealed SqlClientMetrics via a static field (JIT-inlinable, no-op when disabled) to a virtual ISqlClientMetrics call through an instance field. Same hot path, different mechanism — worth measuring in the same run.

[mdaigle]

This run will show v1 pool compared to v1 on main: https://dev.azure.com/SqlClientDrivers/ADO.Net/_build/results?buildId=167201

This will show v2 compared to v2 on main:
https://dev.azure.com/SqlClientDrivers/ADO.Net/_build/results?buildId=167202

[Copilot]

Pull request overview

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

Suppressed comments (7)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:777

• A pooled internal connection must use its owning pool's sink, but this passes the factory sink. If the new pool-level injection is used without separately configuring the factory, activation/deactivation counters go to a different instance than the pool's free/pooled/disconnect counters, defeating exact pool-scoped assertions. Select pool.Metrics when a pool is present and retain the factory sink for non-pooled connections.

                metrics: Metrics);

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

• PrepareConnection returns an activation failure through ReturnInternalConnection, which now emits SoftDisconnectRequest. Because the matching soft connect is recorded only after PrepareConnection succeeds, every failed enlistment/activation decrements the active-soft gauge even though no checkout was counted. Emit SoftConnectRequest before preparation so the return path balances it, matching the failure-safe accounting used by replacement.

            PrepareConnection(owningConnection, connection, transaction);
            Metrics.SoftConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:228

• This pooled-connect counter bypasses the pool-scoped sink and reports to the factory sink instead. A pool constructed with its own Metrics (the purpose of the new injection point) loses hard-connect events unless its caller also knows to inject the identical object into the factory. Route pooled events through pool.Metrics; the factory sink remains appropriate for non-pooled creation.

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

            Metrics.HardConnectRequest();

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

• The new and rewritten messages use a different payload grammar from the established pooler traces. Existing sites consistently emit the <prov.DbConnectionPool.Method|RES|...|CPOOL> markers (for example, WaitHandleDbConnectionPool.cs:1187 and DbConnectionInternal.cs:547), while this change uses Class.Method | INFO |. Besides preventing the stated tracing parity, rewriting existing payloads can break consumers that classify these string events by their markers. Keep the established pooler trace format across the new/updated messages.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:101

• The core Count fix has no regression test for the behavior that differs from ReservationCount: while an Add callback holds a reservation and has not yet returned a connection, ConnectionCount must remain zero, then become one only after insertion and return to zero on removal. Existing pool-count tests observe only completed opens, so they would also pass with the old reservation-based implementation. Add a deterministic blocked-create test (and replacement/removal assertions) for this new counter.

        internal int ConnectionCount => _connectionCount;

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

• No automated test subscribes to the pooler EventSource or asserts any of the newly added trace payloads, including these four distinct liveness-rejection reasons. As a result, missing events and incompatible message formats can regress while all new instrumentation tests still pass because they assert counters only. Add trace-listener tests covering construction/acquisition, each rejection reason, transaction holding, removal, and pruning.

                SqlClientEventSource.Log.TryPoolerTraceEvent(
                    "ChannelDbConnectionPool.IsLiveConnection | INFO | {0}, Connection {1}, exceeded the connection idle timeout and removed.",
                    Id,
                    connection.ObjectID);

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

• The idle-reuse branch leaves soft checkout metrics incorrect in both outcomes. On success, the final SoftConnectRequest records checkout of the idle replacement but retiring oldConnection never records a soft disconnect, so the active-soft gauge grows by one. On activation failure, PrepareConnection returns the candidate through ReturnInternalConnection, which records a soft disconnect before this method has recorded its soft connect, so the gauge drops while the old connection remains checked out. Record the candidate soft connect before PrepareConnection in the idle branch, balance the old checkout when it is retired, and keep the create-new branch's metrics conditional on successful activation.

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

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ReplaceConnection | INFO | {0}, replacing connection.", Id);

[Copilot]

Pull request overview

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

Suppressed comments (6)

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

• When replacement reuses an idle connection, the old checked-out connection is retired via RemoveConnection, but that branch never emits SoftDisconnectRequest. The unconditional soft connect here therefore leaves the active-soft gauge one too high after every idle-backed replacement. Balance the old checkout in both replacement branches.

            Metrics.SoftConnectRequest();

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

• PrepareConnection calls ReturnInternalConnection when activation throws, before the caller reaches its matching SoftConnectRequest. This unconditional decrement therefore makes the active-soft counter negative on activation failures (including the existing idle-reuse activation-failure path). Ensure rollback either records the attempted soft connect first or returns without emitting a soft disconnect.

            Metrics.SoftDisconnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:228

• This is a pooled operation, but it records the hard connect on the factory's independently injected sink rather than the owning pool's sink. Constructing a pool with a test sink and a default factory then produces zero hard connects and one hard disconnect on that pool sink after disposal. Route this through pool.Metrics so the new pool-scoped injection is self-consistent.

            Metrics.HardConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:101

• The key new semantic—excluding a reservation while its create callback is still in flight—is not covered by the existing slot or pool tests; they only inspect counts after creation completes. Add a blocked-create test that observes ReservationCount == 1 and ConnectionCount == 0, then verifies add/remove transitions, to protect the SQL Express user-instance fix.

        /// <summary>
        /// Gets the number of connections currently tracked by this collection. Unlike
        /// <see cref="ReservationCount"/>, this excludes reservations held for connections that are
        /// still being opened, so it reports connections that actually belong to the pool.
        /// </summary>
        internal int ConnectionCount => _connectionCount;

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

• Tracing parity is a primary behavior of this change, but no unit test subscribes to PoolerTrace or asserts any of the newly added lifecycle and liveness-rejection messages. Add listener-based coverage for representative lifecycle events and each rejection reason so trace regressions are detected.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
                Id,
                MinPoolSize,
                MaxPoolSize);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:195

• Non-pooled teardown still records HardDisconnectRequest on SqlClientDiagnostics.Metrics (DbConnectionInternal.cs:508), while this change records the matching connect on the factory's injected sink. A custom sink therefore permanently reports an extra active hard connection. Keep non-pooled connect/disconnect on the same sink (or leave both global); only pooled accounting can safely use the pool-scoped sink as currently structured.

                Metrics.HardConnectRequest();

[mdaigle]

Thanks for the thorough pass. Everything is addressed; a few notes on the two PR-level points.

Reclaimed connections. Correct, number-of-reclaimed-connections reads flat zero under the channel pool, since ReclaimedConnectionRequest is only emitted by the wait handle pool. The channel pool doesn't reclaim emancipated connections yet. That's #4490 and the counter gets wired up there. Called out as a known gap in the description.

Boxing in the trace calls. I don't think this one applies. TryPoolerTraceEvent has no params object[] overload; it's generic T0 through T3, so value-type arguments go through generic instantiation rather than boxing. ToString/string.Format only run inside the IsPoolerTraceEnabled() guard, so nothing is allocated when tracing is off. Happy to benchmark if you still want it, but there doesn't appear to be anything to measure on that path.

On the counter fixes. Pushing on the ReplaceConnection imbalance turned up more than the original comment: the same class of bug appears in five places across both pools, and two of them drive gauges negative rather than just upward. There's a table in the description. The one worth a second look is that active-soft-connects is somewhat redundant with number-of-active-connections, and it's .NET-only with no .NET Framework perf counter behind it. I've kept and fixed it rather than touching it, since it's public surface, but retiring it might be worth considering at a major version.

The counter semantics I fixed against: a soft connect is the pool vending a connection, a soft disconnect is a connection returning to the pool, a hard connect is a new physical connection, a hard disconnect is a connection being destroyed.

[Copilot]

Pull request overview

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

Suppressed comments (6)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs:613

• The delegated-transaction cleanup path has the same sink mismatch: this exit uses the injected connection sink, while the asynchronous non-pooled completion path enters the process-wide sink at SqlConnectionFactory.cs:1106. Once the transaction ends, the two gauges become permanently unbalanced. Route the entry and exit through the same metrics instance.

                Metrics.ExitNonPooledConnection();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs:527

• This exit now uses the injected connection sink, but the matching synchronous non-pooled entry still calls SqlClientDiagnostics.Metrics.EnterNonPooledConnection() in SqlConnectionFactory.cs:487. With an injected factory sink, closing a non-pooled connection therefore decrements the test sink below zero and leaves the process-wide gauge incremented. Route both sides through the same sink.

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

                                Metrics.ExitNonPooledConnection();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:103

• Add a concurrency-focused test that holds ConnectionPoolSlots.Add inside its creation callback and verifies ReservationCount == 1 while ConnectionCount (and ChannelDbConnectionPool.Count) remains zero, then verifies add/remove transitions. Existing slot tests assert only ReservationCount, so they cannot detect this semantic regression or an unbalanced _connectionCount.

        internal int ConnectionCount => Volatile.Read(ref _connectionCount);

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

• These new and rewritten channel-pool traces use a bespoke ChannelDbConnectionPool.Method | INFO | prefix instead of the established pooler format used throughout the legacy implementation, for example WaitHandleDbConnectionPool.cs:489 and :527 (<prov.DbConnectionPool.Method|RES|CPOOL>). That makes the two implementations non-parallel for consumers that group/filter messages by the existing method and category tags. Preserve the established prefix structure while adding the new events.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
                Id,
                MinPoolSize,
                MaxPoolSize);

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

• Add trace-listener tests for the newly introduced lifecycle events (creation, waits, liveness rejection reasons, transaction parking, removal, and pruning). No test under the connection-pool suites subscribes to pooler traces or asserts any of these messages, so the stated tracing parity—and even whether these calls are observable—can regress unnoticed.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.OpenNewInternalConnection | INFO | {0}, Creating new connection.", Id);

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

• The soft-connect is recorded too late and the idle-reuse branch never records the replaced connection's soft disconnect. On successful idle reuse, the active-soft gauge increases from one checked-out connection to two; if activation of the idle connection fails, PrepareConnection returns it and emits a soft disconnect before this line is reached, driving the gauge negative. Record the replacement checkout before activation and balance the old checkout in both successful branches.

            Metrics.SoftConnectRequest();

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

• Add a regression test that holds a user-requested creation in flight, invokes warmup with MinPoolSize satisfied by that reservation, and verifies no duplicate creation is scheduled. The current warmup tests cover a user request arriving during warmup, but not this opposite ordering that specifically distinguishes ReservationCount from Count.

            if (_connectionSlots.ReservationCount >= MinPoolSize)

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

• The soft counters are still unbalanced when replacement reuses an idle connection. This common SoftConnectRequest runs only after PrepareConnection: if activation fails, PrepareConnection returns the idle connection and emits a soft disconnect with no matching connect; if activation succeeds, RemoveConnection(oldConnection) does not emit the soft disconnect for the displaced checkout. Count the idle checkout before activation and balance the old checkout after successful replacement, as the wait-handle path does.

            Metrics.SoftConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:124

• The injected sink is not used consistently for non-pooled connections. Non-pooled opens still call the global sink at SqlConnectionFactory.cs:487 and :1106, while closing now calls the connection's injected sink at DbConnectionInternal.cs:527. With a custom factory sink, each lifecycle leaves the global gauge incremented and the injected gauge decremented. Route both EnterNonPooledConnection calls through this Metrics instance.

        protected ISqlClientMetrics Metrics { get; }

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

• Add a deterministic regression test that blocks creation after a slot is reserved and verifies Count remains zero until the connection is actually installed. Existing count assertions only run after creation completes, so they would not catch Count reverting to reservation semantics or the SQL Express first-connection failure this change targets.

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

        public int Count => _connectionSlots.ConnectionCount;

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

• The soft-connect is recorded only after both replacement branches complete. In the idle-reuse branch, PrepareConnection returns a failed activation through ReturnInternalConnection, emitting a soft disconnect before this line is reached; on success, RemoveConnection(oldConnection) never emits the displaced connection's soft disconnect. Thus failed idle activation lowers active-soft-connects, while successful idle replacement raises it permanently. Record the checkout before preparing an idle connection and balance the displaced checkout on successful idle replacement; add both cases to the metric test.

            Metrics.SoftConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:92

• The injected factory sink is not used by all factory-owned counters. In particular, both EnterNonPooledConnection call sites still target SqlClientDiagnostics.Metrics, while the created connection now targets this injected sink for ExitNonPooledConnection. A factory constructed with a test sink therefore leaves the global gauge incremented and drives the injected gauge to -1 on close. Route the remaining factory counter calls through Metrics as well.

            Metrics = metrics ?? SqlClientDiagnostics.Metrics;

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

• The tracing parity work has no automated trace assertions: the unit-test tree contains no pooler-trace listener or checks for these newly added messages. Add focused coverage for lifecycle, timeout/shutdown, liveness-rejection, transaction-held, and pruning traces so message omissions and format regressions are detected.

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

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
                Id,
                MinPoolSize,
                MaxPoolSize);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:103

• Add unit coverage for the new ConnectionCount invariant. ConnectionPoolSlotsTest currently verifies only ReservationCount, so it would not catch missed increments/decrements or prove the key distinction that an in-flight reservation raises ReservationCount while ConnectionCount remains unchanged. Cover construction, add/remove/replace, failed creation, and a blocked create callback.

        /// <summary>
        /// Gets the number of connections currently tracked by this collection. Unlike
        /// <see cref="ReservationCount"/>, this excludes reservations held for connections that are
        /// still being opened, so it reports connections that actually belong to the pool.
        /// </summary>
        internal int ConnectionCount => Volatile.Read(ref _connectionCount);

[Copilot]

Pull request overview

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

Suppressed comments (5)

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

• SoftConnectRequest is emitted only after the replacement has been prepared. When GetIdleConnection() supplies the replacement and activation throws, PrepareConnection returns that connection through ReturnInternalConnection, which emits SoftDisconnectRequest, but execution never reaches this line. That drives active-soft-connects negative—the same failure mode this PR fixes on normal checkout. Count the idle replacement before calling PrepareConnection (so its failure return balances it), while keeping the create-new branch balanced separately.

            Metrics.SoftConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:195

• The injected factory sink is not used consistently for non-pooled connections. This hard-connect event and the connection's later hard-disconnect/ExitNonPooledConnection now go to Metrics, but both non-pooled success paths in TryGetConnection still call SqlClientDiagnostics.Metrics.EnterNonPooledConnection() (lines 487 and 1106). With a test sink, closing a non-pooled connection therefore leaves its non-pooled gauge at -1 and the global gauge at +1. Route those enter events through the factory's Metrics as well.

                Metrics.HardConnectRequest();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs:66

• The connection is published to the channel before the free gauge is incremented. A concurrent reader can consume it and call ExitFreeConnection during this window, making number-of-free-connections transiently negative and observable by an EventCounter poll. Increment the count/gauge before publishing, and roll both back if TryWrite fails.

                    Interlocked.Increment(ref _count);
                    _metrics.EnterFreeConnection();

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

• The new tracing behavior is not exercised by any test: there is no pool test that subscribes to the EventSource or asserts any of the added lifecycle/rejection messages. Because trace parity is a primary behavior of this PR and these exact strings are the operator-facing contract, add listener-based coverage for the representative success, timeout/shutdown, transaction-held, and liveness-rejection paths.

            SqlClientEventSource.Log.TryPoolerTraceEvent(
                "ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
                Id,
                MinPoolSize,
                MaxPoolSize);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs:103

• ConnectionCount has no direct regression coverage. Existing ConnectionPoolSlotsTest assertions only inspect ReservationCount, so they would not detect a missing increment/decrement or prove the defining behavior that an in-flight Add reservation is excluded. Add a gated create-callback test that observes both counts while creation is blocked, then verifies add, remove, and replace semantics.

        /// Gets the number of connections currently tracked by this collection. Unlike
        /// <see cref="ReservationCount"/>, this excludes reservations held for connections that are
        /// still being opened, so it reports connections that actually belong to the pool.
        /// </summary>
        internal int ConnectionCount => Volatile.Read(ref _connectionCount);