Add connection-creation rate limiting to ChannelDbConnectionPool by mdaigle · Pull Request #4396 · dotnet/SqlClient

mdaigle · 2026-06-23T17:32:49Z

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.

Introduce an optional System.Threading.RateLimiting policy that throttles new physical connection opens in the channel pool: when a permit is denied the caller waits for a returned connection instead of forcing a create, and leases are always released (including on failure) to avoid starvation. Adds NoOpAcquiredLease, wires the RateLimiting package into the product and test projects, and includes the 006-pool-rate-limiting spec. Also repairs two pre-existing build breaks in ChannelDbConnectionPoolTest (a dropped CountingSuccessfulConnectionFactory declaration and DbConnectionPoolGroupOptions calls missing the new idleTimeout argument).

Copilot

Pull request overview

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

codecov · 2026-07-14T01:44:34Z

Codecov Report

❌ Patch coverage is 92.90780% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.26%. Comparing base (fdebcd2) to head (a804da8).
⚠️ Report is 15 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4396      +/-   ##
==========================================
- Coverage   65.83%   65.26%   -0.58%     
==========================================
  Files         287      285       -2     
  Lines       43763    66887   +23124     
==========================================
+ Hits        28812    43654   +14842     
- Misses      14951    23233    +8282

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`65.26% <92.90%> (?)`

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.

The connection pool only needs a concurrency limiter (pooling against on-prem SQL Server), so change ChannelDbConnectionPool to take a concrete System.Threading.RateLimiting.ConcurrencyLimiter? instead of the abstract RateLimiter base. The limiter remains optional (null = no limiting), and AttemptAcquire(1)/RateLimitLease usage is unchanged (both inherited). Rework the three rate-limiter unit tests to use real ConcurrencyLimiter instances and assert via GetStatistics() (CurrentAvailablePermits, TotalFailedLeases) instead of the now-removed TestRateLimiter double. Update the spec and diagram to describe a concurrency limiter specifically, noting other limiter types can be added later if needed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Remove the two "options to consider" TODOs above the AttemptAcquire call and replace them with a comment explaining why non-blocking fast-fail was chosen over failing immediately or blocking on the limiter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Drop the two "options to consider" TODOs above the AttemptAcquire call. The rationale for choosing non-blocking fast-fail lives in the PR discussion rather than in code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

mdaigle · 2026-07-14T18:37:13Z

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:

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.
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.

…nel-rate-limiting

Copilot

Pull request overview

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

Remove the redundant leaseAcquired local; read lease.IsAcquired directly in the early-return guard and the finally-block poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

- NoOpAcquiredLease: document that the shared Instance is stateless and its Dispose is an idempotent no-op, safe to dispose repeatedly/concurrently. - ChannelDbConnectionPool: add a class-level pointer to the feature spec so the FR-0xx requirement tags in comments are resolvable; note at lease.Dispose() that the no-limiter case disposes the idempotent singleton; document the faulted flag and how the finally uses it to avoid a redundant idle-channel poke. - Tests: merge the three ErrorOccurred_* blocking-period facts into a single Theory parameterized by the connection string's Pool Blocking Period; comment that a rising TotalFailedLeases is how the wake test detects requestB was denied a permit and parked on the idle channel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Replaces the hollow SuccessfulCreate_AfterFailure_ClearsErrorState test (which called pool.Clear() and made its final assertion vacuous) with Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate. The new test drives the blocking-period exit timer deterministically via an injected FakeTimeProvider, verifying the real recovery path: failure enters the blocking period, in-window requests fast-fail without touching the factory, and once the backoff elapses a create succeeds and clears the error state. Adds an optional TimeProvider parameter to the ChannelDbConnectionPool constructor (forwarded to BlockingPeriodErrorState) and to the ConstructPool test helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

The ConcurrencyLimiter field doc previously suggested a limiter could be shared across pools. Because the permit-release wake is signaled on this pool's own idle channel, a permit released by another pool would not wake waiters parked here. Reword the doc to state the limiter is expected to be scoped to a single pool and that cross-pool sharing is unsupported, while keeping the caller-owns-lifetime note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

The permit-release poke condition read lease.IsAcquired after lease.Dispose() had run. Even though the current ConcurrencyLimiter lease keeps IsAcquired stable post-dispose, using an object after disposal is fragile if the lease implementation changes. Capture the value into a local before disposing and use the captured bool in the poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

priyankatiwari08

LGTM. Most of my observations were already raised in the latest review comments and have been addressed in the recent resolution commits.

Copilot AI review requested due to automatic review settings June 23, 2026 17:32

github-project-automation Bot added this to SqlClient Board Jun 23, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jun 23, 2026

Copilot started reviewing on behalf of mdaigle June 23, 2026 17:33 View session

mdaigle mentioned this pull request Jun 23, 2026

Document pool rate limiting in Copilot instructions #4397

Draft

3 tasks

Copilot AI reviewed Jun 23, 2026

View reviewed changes

mdaigle mentioned this pull request Jun 30, 2026

Feature | Redesign the SqlClient Connection Pool to Improve Performance and Async Support #3356

Open

17 tasks

Base automatically changed from dev/mdaigle/pool-blocking-period-refactor to main July 13, 2026 23:12

Copilot AI review requested due to automatic review settings July 13, 2026 23:40

mdaigle force-pushed the dev/mdaigle/pool-channel-rate-limiting branch from 806140e to d3e2fe7 Compare July 13, 2026 23:40

Copilot started reviewing on behalf of mdaigle July 13, 2026 23:40 View session

mdaigle added this to the 7.1.0-preview3 milestone Jul 13, 2026

Copilot AI reviewed Jul 13, 2026

View reviewed changes

mdaigle mentioned this pull request Jul 14, 2026

[Design reference — not for merge] Pump-based, fast-path-free ChannelDbConnectionPool #4446

Draft

Copilot AI review requested due to automatic review settings July 14, 2026 18:00

Copilot started reviewing on behalf of mdaigle July 14, 2026 18:01 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated

Copilot AI review requested due to automatic review settings July 14, 2026 18:36

Copilot started reviewing on behalf of mdaigle July 14, 2026 18:37 View session

Merge remote-tracking branch 'origin/main' into dev/mdaigle/pool-chan…

029ce7e

…nel-rate-limiting

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 14, 2026 18:41

Copilot started reviewing on behalf of mdaigle July 14, 2026 18:42 View session

Inline leaseAcquired into lease.IsAcquired checks

662b36d

Remove the redundant leaseAcquired local; read lease.IsAcquired directly in the early-return guard and the finally-block poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 15, 2026 23:16

Copilot started reviewing on behalf of mdaigle July 15, 2026 23:16 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

paulmedynski requested changes Jul 16, 2026

View reviewed changes

github-project-automation Bot moved this from In review to Waiting for customer in SqlClient Board Jul 16, 2026

Copilot AI review requested due to automatic review settings July 16, 2026 17:05

mdaigle dismissed saurabh500’s stale review via 000531b July 16, 2026 17:05

Copilot started reviewing on behalf of mdaigle July 16, 2026 17:06 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated

Comment thread Directory.Packages.props

Copilot AI review requested due to automatic review settings July 16, 2026 17:15

Copilot started reviewing on behalf of mdaigle July 16, 2026 17:16 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated

Copilot AI review requested due to automatic review settings July 16, 2026 17:37

Copilot started reviewing on behalf of mdaigle July 16, 2026 17:38 View session

mdaigle requested a review from paulmedynski July 16, 2026 17:40

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

Copilot AI review requested due to automatic review settings July 16, 2026 17:48

Copilot started reviewing on behalf of mdaigle July 16, 2026 17:49 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

mdaigle mentioned this pull request Jul 16, 2026

Expand connection pool benchmark coverage for ChannelDbConnectionPool #4451

Draft

4 tasks

priyankatiwari08 approved these changes Jul 16, 2026

View reviewed changes

paulmedynski reviewed Jul 17, 2026

View reviewed changes

Comment thread ...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

Comment thread src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs

paulmedynski approved these changes Jul 17, 2026

View reviewed changes

mdaigle merged commit cabe2dd into main Jul 18, 2026
359 checks passed

github-project-automation Bot moved this from Waiting for customer to Done in SqlClient Board Jul 18, 2026

mdaigle deleted the dev/mdaigle/pool-channel-rate-limiting branch July 18, 2026 00:27

Uh oh!

Conversation

mdaigle commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

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

Stacking

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

mdaigle commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mdaigle commented Jun 23, 2026 •

edited

Loading

codecov Bot commented Jul 14, 2026 •

edited

Loading

mdaigle commented Jul 14, 2026 •

edited

Loading