[release/10.0] Fix KeyRingProvider thread pool starvation on forced refresh#68001
Open
DeagleGross wants to merge 1 commit into
Open
Conversation
…67986) * Fix KeyRingProvider thread pool starvation on forced refresh The cold-start starvation fix (dotnet#66683/dotnet#66737) left a second variant: when a valid key ring is cached and a caller forces a refresh (e.g. during the two-minute startup auto-refresh window an incoming cookie names a key not in the current ring), GetCurrentKeyRingCoreNew scheduled the refresh onto TaskScheduler.Default and blocked on Task.Wait(). With every thread-pool thread parked on that wait, the queued refresh could not run, causing thread pool starvation and a multi-second freeze. See dotnet#66380. Unify the invariant that already protects cold start: any caller that must block for a value produces it inline on its own thread, so no caller ever blocks on TaskScheduler.Default work. Non-forced callers with a stale ring still dispatch the async refresh and return the stale ring without waiting, preserving the perf win from dotnet#54675. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec35c6be-ab9b-47d1-9563-d7a61490ebb2 * add debug assert * Harden forced-refresh path per PR review Addresses two issues raised in review of the forced-refresh starvation fix: - On inline refresh failure, only extend the lifetime of a ring that is already stale. A forced refresh can fail while the cached ring is still valid; previously WithTemporaryExtendedLifetime would then cut that valid ring's expiration to 2 minutes and swap its expiration token for CancellationToken.None, causing key changes to be ignored until the shortened lifetime elapsed. This now mirrors the guard in GetKeyRingFromCompletedTaskUnsynchronized. - After publishing an inline forced-refresh result, discard any async refresh task still in flight so its older result cannot later be consumed and clobber the fresher ring. The discarded task's exception is observed to avoid an unobserved TaskScheduler.UnobservedTaskException. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d900e559-5b83-4d0c-b179-e1cc2cda9792 * add regression tests --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec35c6be-ab9b-47d1-9563-d7a61490ebb2 Copilot-Session: d900e559-5b83-4d0c-b179-e1cc2cda9792 (cherry picked from commit a973708)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR backports a fix to Data Protection’s KeyRingProvider to eliminate a forced-refresh variant of thread-pool starvation by ensuring callers that must block compute the refreshed key ring inline (instead of queueing work to TaskScheduler.Default and waiting).
Changes:
- Update
KeyRingProvider.GetCurrentKeyRingCoreNewso cold-start and forced-refresh paths refresh inline under the cache lock, avoiding blocking on thread-pool scheduled work. - Harden forced-refresh behavior: don’t downgrade a still-valid cached ring on refresh failure; discard any in-flight async refresh after publishing an inline forced refresh (and observe discarded task faults).
- Add regression/unit tests covering forced refresh inline execution, failure-over-valid-ring behavior, and discarding in-flight async refresh results.
Show a summary per file
| File | Description |
|---|---|
| src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs | Ensures blocking refresh paths run inline; prevents async refresh task results from clobbering fresher forced-refresh results and observes discarded task faults. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs | Adds regression tests for forced-refresh starvation scenario and related edge cases. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #67986 to release/10.0.
Description
Fixes thread-pool starvation in Data Protection's
KeyRingProvider. The cold-start starvation fix left a second variant: when a valid key ring is cached and a caller forces a refresh (e.g. during the two-minute startup auto-refresh window an incoming cookie names a key not in the current ring),GetCurrentKeyRingCoreNewscheduled the refresh ontoTaskScheduler.Defaultand blocked onTask.Wait(). With every thread-pool thread parked on that wait, the queued refresh work item could not be picked up, causing a hang until the runtime injected more threads. See #66380.The fix unifies the invariant that already protects cold start: any caller that must block for a value produces it inline on its own thread, so no caller ever blocks on
TaskScheduler.Defaultwork. Non-forced callers with a stale ring still dispatch the async refresh and return the stale ring without waiting, preserving the perf win from #54675. It also hardens the forced-refresh path: a still-valid ring is not downgraded when a forced refresh fails, and an in-flight async refresh is discarded once an inline forced result is published so an older read cannot clobber the fresher ring.Fixes #66380.
Customer Impact
Customers running under thread-pool pressure (common right after app start, when many requests arrive carrying auth cookies) can experience request hangs / multi-second stalls when an incoming cookie references a key that is not yet in the cached key ring. Reported by customers on #66380. Until enough threads are injected by the runtime, requests that hit the forced-refresh path block, degrading availability during the startup window.
Regression?
Introduced by the async key-ring refresh change (#54675) in the same product line. This change completes the remediation started by the cold-start fix.
Risk
The change is confined to
KeyRingProvider. Blocking callers now run the refresh inline (the pre-#54675 behavior, which also remains available via theMicrosoft.AspNetCore.DataProtection.KeyManagement.DisableAsyncKeyRingUpdateAppContext switch). The non-blocking async path for stale-but-present rings is unchanged. Covered by new and existing unit tests and validated in CI onmain.Verification
Packaging changes reviewed?