Add connection-creation rate limiting to ChannelDbConnectionPool

[mdaigle]

Summary

This is Part 2 of 3 splitting #4376 ("Dev/mdaigle/pool rate limit") into stacked, reviewable PRs. It stacks on #4395 (Part 1) — review/merge that first.

This PR adds an optional connection-creation rate-limiting policy to ChannelDbConnectionPool, decoupled from the blocking-period error state extracted in Part 1.

Changes

• ChannelDbConnectionPool — when an optional System.Threading.RateLimiting.ConcurrencyLimiter is configured (null = no limiting), a new physical connection open requires a permit. If the permit is denied, the caller waits for an existing connection to be returned instead of forcing a second create. Leases are always released — including on a failed open — so the pool cannot starve future callers. The pool takes a concrete ConcurrencyLimiter? rather than the abstract RateLimiter base because a concurrency limiter is the only throttling we currently need (pooling against on-prem SQL Server); support for other limiter types can be added later if a concrete need arises.
• NoOpAcquiredLease — lightweight always-acquired lease used when no limiter is configured.
• Package wiring — adds System.Threading.RateLimiting to Directory.Packages.props and references it from the product project plus the ManualTests/UnitTests projects.
• Spec — specs/006-pool-rate-limiting/ (spec + diagrams).
• Tests — ChannelDbConnectionPoolTest covers permit-available, permit-denied/reuse, and lease-disposal-on-failure using real ConcurrencyLimiter instances (asserting via GetStatistics()).

Note: this PR also repairs two pre-existing build breaks in ChannelDbConnectionPoolTest that were present on the source branch (a dropped CountingSuccessfulConnectionFactory class declaration, and DbConnectionPoolGroupOptions calls missing the idleTimeout argument added on main).

Design alternative considered (and rejected): pump-based pool — #4446

We prototyped a more invasive pump-based, fast-path-free rewrite of ChannelDbConnectionPool in #4446 and decided not to adopt it for this work. It's kept open as a documented reference.

• What it solves: a time-based / token-bucket limiter has no per-connection event to wake a parked waiter when tokens replenish on a timer (a lost-wakeup problem). The pump re-drives demand from background tasks to cover that gap.
• Why we don't need it here: this PR uses a concurrency limiter, where token availability is lease-release — a concrete event. That maps directly onto the pool's existing "write a wake to the channel when a slot frees" model, so the lost-wakeup problem never arises.
• Why the pump would cost us: it downgrades error precision (a creation error is delivered to a waiter, not the originating caller), regresses the pure-sync path (routes through sync-over-async), and adds ~440 lines of concurrency-critical accounting. Its only independent upsides — strict FIFO and parallel creation — are things we explicitly don't want here (best-effort ordering is fine; warmup is serial).

See #4446 for the full design and trade-off analysis.

Stacking

• Part 1 (Extract pool blocking-period error state into BlockingPeriodErrorState #4395) → main
• Part 2 (this PR) → Part 1 branch
• Part 3 (GitHub instructions) → this branch

Checklist

• Tests added (sync + async paths)
• No public API changes
• No breaking changes
• Verified against customer repro (n/a — new opt-in policy)

[Copilot]

Pull request overview

Adds opt-in connection-creation rate limiting to the v2 ChannelDbConnectionPool using System.Threading.RateLimiting.RateLimiter, plus introduces a lightweight always-acquired lease for the “no limiter configured” case. The PR also wires the new dependency via central package management, adds a feature spec/diagrams, and extends unit tests to cover rate limiting + blocking-period behaviors (stacked on Part 1 / #4395).

Changes:

• Add optional RateLimiter integration to throttle physical connection creation in ChannelDbConnectionPool, with a NoOpAcquiredLease fallback.
• Wire System.Threading.RateLimiting into product + test projects (and Directory.Packages.props).
• Add spec + diagrams and expand ChannelDbConnectionPoolTest coverage for rate limiting and blocking-period behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Adds optional rate-limiter gating for new physical opens and integrates blocking-period error state.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs	Adds a singleton no-op acquired RateLimitLease for the “no limiter” path.
Directory.Packages.props	Adds centralized package versions for System.Threading.RateLimiting.
src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj	References System.Threading.RateLimiting for product build.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj	References System.Threading.RateLimiting for unit test compilation across TFMs.
src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj	References System.Threading.RateLimiting for manual test compilation across TFMs.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Adds/repairs tests covering blocking-period behavior and rate-limiter permit/lease handling.
specs/006-pool-rate-limiting/spec.md	Documents requirements and acceptance scenarios for rate limiting + blocking period.
specs/006-pool-rate-limiting/diagrams.md	Adds diagrams comparing existing vs. new rate-limiting flow.

[Copilot]

Pull request overview

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

[codecov]

Codecov Report

❌ Patch coverage is 92.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.09%. Comparing base (fdebcd2) to head (0d60cd4).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs	95.57%	5 Missing ⚠️
...Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs	58.33%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4396      +/-   ##
==========================================
- Coverage   65.83%   64.09%   -1.75%     
==========================================
  Files         287      285       -2     
  Lines       43763    66875   +23112     
==========================================
+ Hits        28812    42862   +14050     
- Misses      14951    24013    +9062     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	64.09% <92.00%> (?)

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 9 out of 9 changed files in this pull request and generated 2 comments.

[mdaigle]

Why the rate-limit acquire uses non-blocking fast-fail (context for the AttemptAcquire(1) call in ChannelDbConnectionPool, replacing the removed TODOs):

We considered two alternatives and rejected both:

1. Fail immediately and surface an error to the caller. Rejected because a denied permit doesn't mean the pool can't serve the request — an in-use connection may be returned momentarily. Falling back to the idle-channel wait recycles that connection instead of erroring out.
2. Block on acquire (subject to the overall timeout). Simpler, but it queues the caller on the limiter and forces a brand-new physical open even when a returning connection would satisfy it sooner. It also couples the caller's wait to limiter capacity rather than to the pool's existing "wake on returned/created connection" signaling.

Non-blocking fast-fail plus the idle-channel fallback prefers connection reuse and funnels both sources of capacity (a returned connection or a released permit) through a single wait path.

Rearchitecture option we also rejected: route everything through an async channel read (prototyped as the pump-based pool in #4446).

Instead of the current fast-path-plus-fallback, every caller would await a read on the idle channel and a background pump would drive connection creation, writing results back to the channel. This is genuinely simpler in one sense — there's a single wait path and no synchronous fast path to reason about — but we rejected it because:

• Error precision is lost. A creation failure is delivered to a waiter reading the channel, not the caller whose open triggered it, so the originating request can't reliably get its own exception.
• The pure-sync path regresses. Routing sync callers through an async channel read means sync-over-async, adding overhead and thread-pool dependence on what is otherwise a straight-line synchronous open.
• Significant added complexity. The pump needs ~440 lines of concurrency-critical demand/accounting logic, and its only real upsides (strict FIFO ordering and parallel creation) are things we explicitly don't need here — best-effort ordering is fine and warmup is serial.

For a concurrency limiter, permit availability is a concrete event (a lease release), which maps cleanly onto the pool's existing "wake on returned/created connection" signaling — so the pump's main justification (covering timer-based token replenishment with no per-connection wake event) doesn't apply. See #4446 for the full design and trade-off analysis.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[mdaigle]

Copilot review comments — resolution summary

Addressed in commit 0d60cd4f:

• OperationCanceledException poisoning the blocking period — the creation-failure catch now excludes OperationCanceledException (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException). A caller's own connect timeout/cancellation is client-side contention, not a physical-connection failure, so it no longer enters the error state / fast-fail backoff for other callers. The OCE propagates up to the normal timeout path.
• Redundant idle-channel wake on exception paths — added a faulted flag; the finally poke now only fires on non-faulted completion. On exception paths the cleanupCallback already writes a wake, so this avoids a double wake.
• Limiter ownership / disposal — documented on the field that the pool does not own the injected ConcurrencyLimiter and never disposes it; the caller that constructs the limiter owns its lifetime (a limiter may be shared across pools or outlive one). This is intentional, so the pool does not add a Dispose of it.
• Comment typo / trailing whitespace — "rather then" → "rather than" and stripped trailing whitespace.
• Spec FIFO wording — User Story 1 and FR-002 reworded from strict FIFO to best-effort: callers that can't immediately create fall back to waiting on the idle channel (satisfied by a returned connection or a best-effort wake on permit release); rate-limit retries are best-effort, not strictly ordered. This matches the non-blocking AttemptAcquire implementation.
• Test limiter disposal + unused using — ConcurrencyLimiter locals in the rate-limiter tests are now using var, and the unused System.Collections.Generic using was removed (the build now compiles with zero warnings).

Not applied, with rationale:

• Bump System.Threading.RateLimiting 8.0.0 → 8.0.1 — that version does not exist on nuget.org for this package; it releases 8.0.0 then jumps to 9.0.0. The 8.0.1 siblings (ConfigurationManager, Pkcs, TimeProvider) do have 8.0.1, but RateLimiting does not, so 8.0.0 is kept for the net8 band (net9 band stays 9.0.13).
• Broadcast wake for all parked waiters when entering the error state — currently entering the error state writes a single null poke; other parked waiters unblock on their own timeout. Making the blocking-period transition fast-fail every waiter is a broader change to the blocking-period design and is out of scope for this PR; tracking separately rather than risking a wide behavioral change here.

Validation: product builds clean for net8.0 + net9.0; UnitTests build clean (0 warnings); ConnectionPool filter = 248 passed; rate-limiter tests re-run 3× with no flakiness.

[Copilot]

Pull request overview

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

[saurabh500]

@mdaigle how does the application set the rate limit? What is the experience?

[mdaigle]

@mdaigle how does the application set the rate limit? What is the experience?

@saurabh500 I haven't exposed it at the moment. I don't want to make it configurable by customers until we decide that it's needed. For the initial release, I will at most set a default concurrency for on-prem if testing shows it to be necessary.