Skip to content

[Core] Don't leave callers waiting forever when a token refresh is cancelled - #50856

Merged
Alan Zimmer (alzimmermsft) merged 2 commits into
Azure:mainfrom
prasanna164-code:core-token-cache-cancel
Oct 19, 2026
Merged

Alan Zimmer (alzimmermsft) merged 2 commits into
Azure:mainfrom
prasanna164-code:core-token-cache-cancel

Conversation

@prasanna164-code

@prasanna164-code prasanna164-code commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #50855.

AccessTokenCache and SimpleTokenCache let one caller refresh the token while concurrent callers wait on a Sinks.One. The refresh runs inside Mono.using, and its cleanup only resets wip:

return Mono.using(() -> wip,
    ignored -> tokenRefresh.materialize()
        .flatMap(processTokenRefreshResult(sinksOne, now, fallback))
        .doOnError(sinksOne::tryEmitError),
    w -> w.set(null));

On success the sink receives the token and on failure it receives the error, but if the refreshing caller is cancelled the sink is never completed. Every caller waiting on sinksOne.asMono() then stays pending indefinitely, even after a later caller has fetched a new token. On the async path BearerTokenAuthenticationPolicy uses AccessTokenCache, so concurrent requests from an async client can hang when the request that started the refresh is cancelled, for example by a timeout(...) or a disposed subscription while a slow token request is in progress.

Change

  • The Mono.using cleanup in both classes now completes the sink empty after clearing wip. It is the only place that completes the sink empty: the NO REFRESH branch no longer does, so waiting callers are released only once wip no longer refers to the finished refresh. When the refresh already completed the sink with a token or an error, the cleanup's emission is a no-op.
  • Waiting callers that are released this way, because the refresh was cancelled or completed without a token, fall back to the current cached token if it is still valid, and otherwise acquire a new one:
    • AccessTokenCache: the fallback reads the latest cached token and completes empty if there is none or it has expired. The existing repeatWhenEmpty in getToken then retries. Previously the fallback returned the cachedToken captured before waiting, so the comment "defer to updated cache in case just missed onNext()" didn't hold, and the value could be null or already expired.
    • SimpleTokenCache: getToken has no repeatWhenEmpty, so the fallback starts a new acquisition when the cached token isn't valid. It already read the live cache field. The acquisition logic moved from getToken() into a private acquireToken(), which getToken() delegates to, so the retry doesn't go through the overridable public method.
  • The wip == null branch ("the refreshing thread has finished") uses the same fallback. It previously returned Mono.just(cachedToken) (Mono.just(cache) in SimpleTokenCache). That errors with a NullPointerException when no token has been cached yet, and can return an expired token.

No public API changes.

Waiting callers released this way make a new token request instead of reusing the cancelled one. If a SimpleTokenCache supplier keeps completing empty, the released callers retry one at a time until each has made its own attempt. That is at most one extra token request per waiting caller; previously they all completed empty. If the retried request fails, the waiters receive that error, subject to the existing refreshDelay back-off that SimpleTokenCache applies after a failed refresh. I considered keeping the refresh running after its caller cancels so that the waiters could share it. That changes cancellation semantics, though: the token request would outlive every caller, and it would have to run outside the caller's Reactor Context. A second token request after a cancellation seemed the safer trade-off.

Related observations, not addressed here

  • containerregistry's implementation/authentication/AccessTokenCacheImpl has the same pattern, with doFinally(ignored -> wip.set(null)) and waiters on sinksOne.asMono(), so the Container Registry credential policy can hang the same way. I've kept this PR to azure-core and am happy to send the same change for Container Registry separately.
  • A cancelled or failed forced refresh in AccessTokenCache (refreshOnContextChange with a different tenant, scopes or claims) leaves this.tokenRequestContext set to the new context without a matching token. The next request for that context then gets the previously cached token for the old context. For example, after a cancelled refresh for tenant B, a request for tenant B receives tenant A's token, until that token reaches its refresh time. This happens on main and on this branch alike, and the sync path has the same ordering. It needs a different change, recording the context only once a token for it has been acquired, so I'd suggest handling it separately as well.

Testing

New tests in AccessTokenCacheTests and SimpleTokenCacheTests:

  • waitingCallerGetsTokenWhenRefreshingCallerIsCancelled: the first token request never completes, a second caller waits on that refresh, and the first caller is disposed. The second caller must receive a token from a new acquisition. This fails on main, where the second caller never completes.
  • waitingCallerGetsTokenWhenRefreshCompletesEmpty: the first token request completes empty while a second caller waits. The waiter must receive a token from a second acquisition. With SimpleTokenCache this completes empty on main.

Outside the module's tests, comparing the released 1.60.0 with this change:

  • The pipeline reproduction from the issue: two concurrent requests through BearerTokenAuthenticationPolicy, with a timeout on the first. Request B never completes on 1.60.0 and completes with this change.
  • 2,000 callers waiting on one refresh, run with a 256 KB thread stack, including callers that call getToken() again from their own callback. The refresh completes with a token, fails, completes empty or is cancelled. Later token requests return a token, complete empty or fail, synchronously or asynchronously. On 1.60.0 every waiter hangs when the refresh is cancelled. With this change every waiter completes, and nothing overflows the stack.
  • A randomized stress run: 40 concurrent callers per round, about a third cancelled at random times, 40 rounds. Token requests either always return a token or complete empty 30% of the time. On 1.60.0, between 104 and 170 of the non-cancelled callers per run never completed. With this change none hung and none failed.

azure-core passes its full module build (11,894 tests), including Checkstyle, SpotBugs and RevApi.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

AccessTokenCache and SimpleTokenCache let one caller refresh the token
while concurrent callers wait on a Sinks.One. When the refreshing caller
was cancelled, the Mono.using cleanup only reset wip and never completed
the sink, so every waiting caller stayed pending indefinitely.

The cleanup now also completes the sink empty, a no-op when the refresh
already completed it. Released callers use the current cached token if
it is still valid and otherwise acquire a new one. The branch for an
already finished refresh uses the same fallback instead of returning a
captured token that could be null or expired.

Fixes Azure#50855
Copilot AI balanced review requested due to automatic review settings October 18, 2026 20:19
@github-actions github-actions Bot added Azure.Core azure-core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Oct 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution prasanna164-code! We will review the pull request and get back to you soon.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
34 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An empty refresh in SimpleTokenCache can trigger recursive resubscription before cleanup and overflow the stack.

1 open issue
What changed in this PR

Addresses #50855 by releasing concurrent token-cache callers when a refresh is cancelled, without changing public APIs.

Changes:

  • Completes waiting sinks during refresh cleanup.
  • Uses valid cached tokens or retries acquisition.
  • Adds cancellation regression tests and a changelog entry.
File Description
sdk/​core/​azure-core/​src/​test/​java/​com/​azure/​core/​credential/​SimpleTokenCacheTests.java Tests waiter recovery after cancellation.
sdk/​core/​azure-core/​src/​test/​java/​com/​azure/​core/​credential/​AccessTokenCacheTests.java Tests waiter recovery after cancellation.
sdk/​core/​azure-core/​src/​main/​java/​com/​azure/​core/​credential/​SimpleTokenCache.java Releases waiters and adds reacquisition fallback.
sdk/​core/​azure-core/​src/​main/​java/​com/​azure/​core/​credential/​AccessTokenCache.java Releases waiters and validates the latest cached token.
sdk/​core/​azure-core/​CHANGELOG.md Documents the cancellation fix.

Review effort: Balanced

…resh

When the token request completed empty, the NO REFRESH branch released
waiting callers while wip still held the finished refresh. In
SimpleTokenCache a released waiter then re-entered the acquisition, found
the same completed sink, and recursed until StackOverflowError.

Waiting callers are now released only by the Mono.using cleanup, after
wip has been cleared, so a retrying waiter always starts a new
acquisition. SimpleTokenCache retries through a private acquireToken()
instead of the overridable getToken().

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cancellation and retry ordering in shared authentication caches warrants human verification despite the targeted regression coverage.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@alzimmermsft

Copy link
Copy Markdown
Member

Thanks for fixing this prasanna164-code! Let me know about the comment I had just in case I need to approve this again.

@prasanna164-code

Copy link
Copy Markdown
Contributor Author

Alan Zimmer (@alzimmermsft) I've replied to your question on the test above. No code changes since your approval, so the re-request notification can be ignored; it was a misclick on my end. Sorry for the noise!

@alzimmermsft
Alan Zimmer (alzimmermsft) merged commit 9d12974 into Azure:main Oct 19, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Core azure-core Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] AccessTokenCache/SimpleTokenCache: concurrent callers wait forever when the caller refreshing the token is cancelled

3 participants