Fix active connection pools being retired during creation

[priyankatiwari08]

Fixes #4755.

Summary

• Make empty-pool retirement atomic with admission in both WaitHandle and Channel pools. Count == 0 does not mean idle while physical creation, queued acquisition, or background warmup is in flight.
• Keep admission through async worker completion (including caller cancellation), protect replacement/background creation, and reject stale references after retirement. Preserve blocking-period throttling, explicit Clear/Shutdown semantics, and demand-driven timer cleanup.
• Independent follow-up to the CI investigation in Re-issue session isolation level on TransactionScope re-enlistment #4335; no isolation-level changes or unrelated issue closures. The race is reproduced on main; its involvement in the earlier CI failures remains inferred, not directly observed.

Validation

• Unchanged main d189eeb7: all four deterministic physical-create/pruning cases fail (both pools, sync/async). With this fix they pass.
• 24 targeted pruning cases, plus existing pool/factory lifecycle coverage: 435 passed each on net462, net8.0, net9.0 and net10.0.
• SQL Server 2022: 4 passed each on net8.0 and net10.0. Inject pruning at physical creation, then assert three sequential TransactionScope opens reuse the same session without DTC promotion.
• Unix-targeted net8.0 driver build: zero warnings/errors. Local SQL used a process-only development-certificate override; no service/configuration changes.

Released 6.1 V1 comparison

• Confirmed on unmodified NuGet 6.1.0 and 6.1.7, using V1 (WaitHandleDbConnectionPool), .NET 8.0.31 on Windows, SQL Server 2022.
• Both sync/async controls complete three opens on the same SPID without promotion. Both injected cases, on each version, retire the running zero-count pool during first physical creation; the first open succeeds and the second reaches EnlistNonNull -> GetExportCookie and fails because implicit DTC is disabled.
• This establishes that the pruning defect predates 7.1; it is not V2-exclusive. The deterministic forced-interleaving harness and results are in Pool pruning can retire a pool during physical connection creation and cause unexpected transaction promotion (reproduces on 6.1 V1) #4755. Natural-timer frequency and earliest affected release remain unestablished.
• This is distinct from [7.1] 7.1.0 regression: in-flight OpenAsync fails instantly with pool "Timeout expired" when ClearAllPools()/ClearPool() runs #4737's 7.1-specific async timeout after explicit ClearAllPools/Shutdown.

Checklist

• Tests added or updated
• Public API changes documented (none)
• Verified against the reproduced pruning interleaving
• No breaking public API or switch changes introduced

Suggested release note: Prevent pruning from retiring connection pools with in-flight acquisition or creation, avoiding lost transaction-affine reuse and unintended promotion.

Publication note: the app prepended priyankatiwari08- to the requested dev/automation/... branch name. Upstream direct push was rejected by repository rules, so this PR is published from the existing contributor fork.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The synchronization changes affect core pool lifecycle behavior across both implementations and warrant final human concurrency review.

Review effort: Balanced
Findings: 2

Open (2)

• Add behavior-focused summary for Arm · New
• Document both helper parameters · New

What changed in this PR

Makes pool pruning atomic with connection admission to prevent active pools from being retired during creation.

Changes:

• Adds a shared pruning guard to both pool implementations.
• Handles queued, cancelled, replacement, and warmup operations.
• Adds deterministic unit and transaction-level integration coverage.

File	Description
TransactedConnectionPoolTest.cs	Updates the pool test double.
DbConnectionPoolInstrumentationTest.cs	Handles nullable replacement results.
DbConnectionPoolGroupPruningTest.cs	Adds pruning race coverage.
PoolPruningTransactionTest.cs	Verifies transaction-affine reuse.
WaitHandleDbConnectionPool.cs	Protects admission and creation from pruning.
PoolPruningGuard.cs	Implements atomic admission and retirement.
IDbConnectionPool.cs	Adds the pruning contract.
DbConnectionPoolGroup.cs	Delegates retirement decisions to pools.
ChannelDbConnectionPool.cs	Protects asynchronous and background operations.
connection-pooling.instructions.md	Documents pruning invariants.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

❌ Patch coverage is 80.54054% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.90%. Comparing base (671d010) to head (7ecf69b).
⚠️ Report is 34 commits behind head on main.

Files with missing lines	Patch %	Lines
.../Data/SqlClient/ConnectionPool/PoolPruningGuard.cs	74.54%	14 Missing ⚠️
...lient/ConnectionPool/WaitHandleDbConnectionPool.cs	83.95%	13 Missing ⚠️
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs	81.25%	9 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (671d010) and HEAD (7ecf69b). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (671d010)	HEAD (7ecf69b)
CI-SqlClient	1	0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4754      +/-   ##
==========================================
- Coverage   71.96%   64.90%   -7.06%     
==========================================
  Files         291      287       -4     
  Lines       45110    68714   +23604     
==========================================
+ Hits        32462    44602   +12140     
- Misses      12648    24112   +11464     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.90% <80.54%> (?)

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]

Copilot review overview

🔵 Needs a closer look

Three new test helper overrides lack the repository-required XML documentation.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Document both helper parameters
• Add behavior-focused summary for Arm

Previously missed (2)

In code that hasn't changed since last review

Document EventListener test helper and SqlClient event setup

src/​Microsoft.Data.SqlClient/​tests/​ManualTests/​SQL/​ConnectionPoolTest/​PoolPruningTransactionTest.cs:87

This EventListener helper override lacks the required XML summary and parameter documentation for test helpers (.github/instructions/testing.instructions.md:181-194). Describe why SqlClient events are enabled for this regression test.

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

Add XML documentation for test helper override

src/​Microsoft.Data.SqlClient/​tests/​UnitTests/​ConnectionPool/​DbConnectionPoolGroupPruningTest.cs:378

This test helper override is missing the XML documentation required for helper methods, including parameter and return documentation (.github/instructions/testing.instructions.md:181-194). Document the pruning hook and mock-connection behavior so the deterministic setup remains clear.

[priyankatiwari08]

Addressed the latest review summary in 7da3941: added XML summaries and parameter/return documentation for both EventListener overrides and CreateConnection. Documentation only; behavior unchanged.

[Copilot]

Copilot review overview

🟡 Changes recommended

Queued background warmup remains unprotected from pruning in both pool implementations.

Review effort: Balanced
Findings: 2

Open (2)

• Acquire warmup admission before scheduling the worker · New
• Acquire replenishment admission before queueing the callback · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Core concurrent lifecycle behavior changes across both pool implementations warrant final human review despite comprehensive deterministic coverage.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Acquire replenishment admission before queueing the callback
• Acquire warmup admission before scheduling the worker

[Copilot]

Copilot review overview

🔵 Needs a closer look

Both implementations can still retire a zero-count pool between removing its last connection and admitting minimum-size replenishment.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Prevent pool retirement before scheduling minimum-size warmup

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​ChannelDbConnectionPool.cs:1970

The admission starts after RemoveConnection has already decremented _connectionSlots and only then called RequestWarmup. If that was the last connection with MinPoolSize > 0, group pruning can retire the zero-count pool in between; this TryEnter then returns null (or the earlier state check exits), so the pool never refills. Acquire the pruning admission before removing the slot and transfer it through warmup scheduling.

Acquire pruning admission before zero-count replenishment

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​WaitHandleDbConnectionPool.cs:1534

The pruning admission is still acquired too late for minimum-size replenishment. DeactivateObject removes the last connection with DestroyObject before calling QueuePoolCreateRequest; a concurrent group prune can therefore observe Count == 0, mark the guard pruned, and make this TryEnter return null. With MinPoolSize > 0, the required replenishment is then lost. Hold an admission across the decrement-to-zero and transfer it to the queued worker.

[Copilot]

Copilot review overview

🟡 Changes recommended

The guard adds an allocation and monitor contention to every pooled checkout, creating a hot-path performance regression.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Avoid per-checkout Lease allocation and monitor contention · New
• Update return contract for null when retired before admission · New

[Copilot]

🔵 Needs a closer look

A stale caller can observe rejected admission before the pool becomes non-running and receive a false pool timeout.

0 open findings

2 resolved since last review

• Avoid per-checkout Lease allocation and monitor contention
• Update return contract for null when retired before admission

Previously missed (1)

In code that hasn't changed since last review

Prevent stale pool race between Pruned publication and shutdown

src/​Microsoft.Data.SqlClient/​src/​Microsoft/​Data/​SqlClient/​ConnectionPool/​PoolPruningGuard.cs:79

Publishing Pruned before Shutdown() creates a stale-reference race. A caller that already fetched this pool can observe TryEnter() returning inactive while pool.IsRunning is still true; SqlConnectionFactory.TryGetConnection then treats the (null, running) result as PooledOpenTimeout instead of retrying against the replacement pool. Make the retirement transition expose a non-running pool before admission starts returning inactive (without leaving callers spinning through lengthy shutdown cleanup), and add an interleaving test for this boundary.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.