Fix #1881 | Make the connection factory pruning timer demand-driven

[apoorvdeshmukh]

Fixes #1881

SqlConnectionFactory is a process-wide singleton whose constructor unconditionally started a repeating timer (4 min due time, 30 s period). PruneConnectionPoolGroups no-ops when _poolsToRelease, _poolGroupsToRelease and _connectionPoolGroups are all empty, but nothing ever checked for that — so the process was woken every 30 s forever, even after every SqlConnection was disposed and ClearAllPools() was called, and even with Pooling=False.

The only _pruningTimer.Dispose() lived in Unload(), wrapped in #if NET and hooked solely to AssemblyLoadContext.Unloading, so it never ran in a normal app on .NET and had no counterpart at all on .NET Framework.

Change

The timer is now created disarmed and armed only on the idle → active transition, mirroring the arm/disarm pattern already used by PoolPruner:

• Armed in GetConnectionPoolGroup. Registering a pool group is the only way pruning work enters the factory — pools and pool groups are queued for release only while a group is still registered, and a disabled group can never re-acquire a pool.
• Disarmed at the tail of PruneConnectionPoolGroups once all three collections drain.
• Coordinated through a dedicated _pruningTimerLock (outermost; the disarm path re-reads all three counts while holding it) with a _pruningTimerDisposed guard, so an unload racing a new connection cannot throw ObjectDisposedException on a background thread.

To make the state machine testable without waiting minutes, this adds a protected constructor overload taking the due time and period, plus internal IsPruningTimerActive / RunPruningPass().

Worth calling out in review

1. Added the missing .NET Framework unload hook (AppDomain.DomainUnload + ProcessExit), matching the existing pattern in SqlDiagnosticListener and SqlClientMetrics. Without it _pruningTimerDisposed is never assigned on net462 (CS0649 under warnings-as-errors), and the timer was never disposed there at all. Slightly beyond the minimal fix, but it closes the disposal gap named in the issue title.
2. Behaviour change: the 4-minute due time now applies per idle → active transition rather than once per process. Equivalent for any continuously-active application, strictly better when idle.
3. An eager drain on ClearAllPools() is intentionally not included here.

No public API, connection string keyword or AppContext switch changes.

Validation

Repro: open/close a connection, call ClearAllPools(), accelerate the timer by reflection, then sample tick counts.

	ticks after drain	_pruningTimerEnabled	re-arm on new connection
before — Pooling=True	+15 per 3 s, indefinitely	—	—
before — Pooling=False	+15 per 3 s, indefinitely	—	—
after — Pooling=True	0	False	PASS
after — Pooling=False	0	False	PASS

The pooled run drains ReleasePool=24 / ReleasePoolGroup=1 before quiescing, so the release paths are exercised.

Adds 4 unit tests covering the arm/disarm state machine. Full UnitTests suite on net9.0 shows the same pre-existing failures with and without the change (native SNI won't initialize in this environment). Builds clean on net8.0, net9.0 and net462.

• Tests added or updated
• Public API changes documented — n/a, no public API changes
• Verified against customer repro
• Ensure no breaking changes introduced

Suggested release notes

• Fixed the SqlConnectionFactory pruning timer firing every 30 seconds for the lifetime of the process after all connections were disposed, and even when Pooling=False. It is now started on demand and stopped once all pruning work drains. (#4479)
• Fixed the SqlConnectionFactory pruning timer not being disposed at shutdown on .NET Framework. (#4479)

[Copilot]

Pull request overview

This PR addresses #1881 by making SqlConnectionFactory’s connection-pool pruning timer demand-driven, so idle processes aren’t woken every pruning period once all pruning work has drained (including after ClearAllPools() and when Pooling=False). It also adds a .NET Framework shutdown hook to ensure the timer is disposed during app-domain/process shutdown, matching the disposal coverage that previously only existed for .NET AssemblyLoadContext unloading.

Changes:

• Create the pruning timer in a disarmed state and arm it only when pruning work is introduced (GetConnectionPoolGroup, QueuePoolForRelease, QueuePoolGroupForRelease), then disarm it once all tracked pruning work drains.
• Add a dedicated _pruningTimerLock plus _pruningTimerDisposed guard to coordinate state transitions and avoid ObjectDisposedException races.
• Add unit tests that drive pruning deterministically via new internal test hooks (IsPruningTimerActive, RunPruningPass()) and a protected constructor overload to control timer cadence.

Reviewed changes

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

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs	Refactors pruning timer lifecycle to arm/disarm on demand, adds disposal coordination and a .NET Framework unload hook, and introduces test hooks for deterministic validation.
src/Microsoft.Data.SqlClient/tests/UnitTests/SqlConnectionFactoryPruningTimerTest.cs	Adds unit tests covering timer initial state, arming on work publication, remaining armed while draining, disarming on quiescence, and re-arming on subsequent work.

[codecov]

Codecov Report

❌ Patch coverage is 96.61017% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.61%. Comparing base (a684c99) to head (d292224).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs	96.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4479      +/-   ##
==========================================
- Coverage   64.64%   62.61%   -2.03%     
==========================================
  Files         288      283       -5     
  Lines       44009    66985   +22976     
==========================================
+ Hits        28451    41945   +13494     
- Misses      15558    25040    +9482     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	62.61% <96.61%> (?)

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.

[paulmedynski]

This whole apparatus seems over-engineered, but that's a pre-existing concern. Your changes make sense superficially, with just one question about the tests.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

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

• The lock-ordering comment contradicts the implementation: StopPruningTimerIfIdle/HasPruningWork acquire _pruningTimerLock and then take the release-list locks, so it is possible to hold _pruningTimerLock while locking _poolsToRelease/_poolGroupsToRelease. The comment should be updated to reflect the actual ordering requirement (i.e., _pruningTimerLock is outermost and should not be taken while holding the release-list locks) to avoid future deadlock-prone changes based on incorrect guidance.

        // Lock ordering: this lock may be taken while holding "this", a pool group lock, or
        // either release-list lock, but the reverse is never true. StopPruningTimerIfIdle takes
        // _pruningTimerLock first and only then peeks at the release lists, and it deliberately
        // reads _connectionPoolGroups without taking "this" (the dictionary is swapped wholesale,
        // so reading the reference is safe). That keeps the lock graph acyclic.

[mdaigle]

One duplicate test, otherwise looks good.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

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

• QueuePoolForRelease can be invoked from DbConnectionPoolGroup.Clear() (e.g., via ClearPool(DbConnection) on a stale PoolGroup) even after the pruning timer has disarmed. In that case, the pool is added to _poolsToRelease but no background pruning pass will run to drain/dispose it unless the timer is re-armed when this work is published.

            }

            SqlClientDiagnostics.Metrics.EnterInactiveConnectionPool();
            SqlClientDiagnostics.Metrics.ExitActiveConnectionPool();
        }