Fix access token expiry eviction in connection pool V2 - #4734
Conversation
Validate access token expiry before general checkout, preserving return and transaction-affinity behavior. Cover callback cache refresh and physical reuse across both pool implementations and sync/async opens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches V1 semantics and includes focused coverage for all affected checkout paths.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes connection pool V2 so expired access-token connections are rejected during checkout, matching V1 behavior.
Changes:
- Adds checkout-time token-expiry validation while preserving transaction affinity.
- Adds comprehensive sync/async unit and integration coverage.
- Clarifies
AccessTokenCallbackpooling and refresh documentation.
| File | Description |
|---|---|
DbConnectionPoolAccessTokenTest.cs |
Covers token eviction, replacement, transactions, and callback caching. |
AADFedAuthTokenRefreshTest.cs |
Adds live Entra ID token-refresh coverage. |
ChannelDbConnectionPool.cs |
Rejects expiring connections during general checkout. |
SqlConnection.xml |
Documents callback refresh and pooled reuse behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4734 +/- ##
==========================================
- Coverage 71.96% 64.83% -7.14%
==========================================
Files 291 285 -6
Lines 45110 68089 +22979
==========================================
+ Hits 32462 44143 +11681
- Misses 12648 23946 +11298
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
priyankatiwari08
left a comment
There was a problem hiding this comment.
Approach matches V1 and the test coverage is thorough (idle, new, queued-waiter, transacted, callback refresh). Two things to confirm:
- Expired-but-idle connections still occupy pool slots and are invisible to the pruner/warmup, so a low-traffic pool with MinPoolSize > 0 can sit full of dead-token connections.
- Rejecting a freshly created connection at checkout can churn create/destroy until the open timeout if the callback keeps returning short-lived tokens.
This is acceptable.
Callback is client's implementation so yes, also client's responsibility. |
Validate access token expiry before general checkout, preserving return and transaction-affinity behavior. Cover callback cache refresh and physical reuse across both pool implementations and sync/async opens. Co-authored-by: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
V2 did not check
IsAccessTokenExpiredbefore handing out pooled connections. This adds the missing checkout validation, matching V1 while preserving return-to-pool behavior and same-transaction reuse.Adds sync/async coverage for idle, newly created, and queued connections, transaction affinity, and callback token-cache refresh/reuse. Clarifies callback documentation. No public API changes.
Testing
Checklist
Suggested release note: Fixed connection pool V2 reusing connections with expired or nearly expired access tokens.