Repository navigation
Fix KeyRingProvider thread pool starvation on forced refresh - #67986
Merged
DeagleGross merged 4 commits intoJul 24, 2026
Merged
DeagleGross merged 4 commits into
DeagleGross merged 4 commits into
Conversation
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
DeagleGross
added
bug
DeagleGross
marked this pull request as ready for review
July 23, 2026 17:41
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a remaining thread-pool starvation scenario in KeyRingProvider.GetCurrentKeyRingCoreNew: when a key ring is already cached but a caller forces a refresh, the refresh work could be queued to TaskScheduler.Default while the caller blocks, potentially deadlocking under thread-pool saturation. The update enforces an invariant that any caller who must block for a refreshed ring performs the refresh inline on its own thread, while non-forced callers with a stale ring still trigger an async refresh and return the stale value.
Changes:
- Run forced-refresh (and cold-start) key ring refresh inline rather than scheduling and blocking on thread-pool work.
- Preserve the async-refresh performance optimization for non-forced callers when a stale ring is still available.
- Add a regression test ensuring forced refresh with a cached ring performs the refresh on the calling thread.
Show a summary per file
| File | Description |
|---|---|
| src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs | Reworks forced-refresh + stale/cold-start handling to avoid thread-pool starvation by running blocking refresh work inline. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs | Adds regression coverage asserting forced refresh with a cached ring refreshes on the calling thread. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
1 task done
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
halter73
approved these changes
Jul 23, 2026
halter73
left a comment
halter73
left a comment
Member
There was a problem hiding this comment.
It might be worth adding regression tests for the issues Copilot pointed out earlier though.
3 tasks
javiercn
approved these changes
Jul 24, 2026
5 of 8 tasks
wtgodbe
pushed a commit
that referenced
this pull request
Aug 11, 2026
…#68001) * Fix KeyRingProvider thread pool starvation on forced refresh The cold-start starvation fix (#66683/#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 #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 #54675. * 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. * add regression tests --------- (cherry picked from commit a973708) 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
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.
The cold-start starvation fix (#66683/#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),
GetCurrentKeyRingCoreNewscheduled 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 #66380 (comment).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 #54675.
Fixes #66380