Lock SMTP OAuth token refresh per realm - #53131
Open
prasanna164-code wants to merge 1 commit into
Open
prasanna164-code wants to merge 1 commit into
prasanna164-code wants to merge 1 commit into
Conversation
gatherValidToken held a lock shared by all realms for the whole token request, so a slow token endpoint in one realm delayed token-based SMTP mail in every other realm. The lock is now per realm, and the cache is read again once the lock is held so waiting requests reuse the new token instead of each fetching their own. Closes keycloak#52678 Signed-off-by: Prasanna Sankaran <prasanna164@gmail.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The lock registry grows indefinitely, and the same-realm regression test relies on nondeterministic timing.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Introduces realm-scoped locking for SMTP OAuth token refreshes, preventing one realm from blocking others.
Changes:
- Adds per-realm token-refresh locks and cache rechecking.
- Adds cross-realm and same-realm concurrency tests.
| File | Description |
|---|---|
services/src/main/java/org/keycloak/email/TokenAuthEmailAuthenticator.java |
Implements realm-scoped token refresh synchronization. |
tests/base/src/test/java/org/keycloak/tests/admin/SMTPTokenAuthConcurrencyTest.java |
Tests SMTP OAuth concurrency behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static final int FALLBACK_EXPIRES_AT_IN_SECONDS = 60; | ||
|
|
||
| private final Map<String, TokenAuthEmailAuthenticator.TokenStoreEntry> tokenStore = new ConcurrentHashMap<>(); | ||
| private final Map<String, Object> tokenLocks = new ConcurrentHashMap<>(); |
|
|
||
| Future<Response> second = executor.submit(() -> testSmtpConnection(adminB2, realmB, heldTokenUrl())); | ||
| // let the second request miss the cache and wait for the first one before the token is returned | ||
| Thread.sleep(2000); |
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.
Closes #52678
TokenAuthEmailAuthenticatoris created once per server, andgatherValidTokensynchronized on the shared token map for the whole token request. A slow or unresponsive token endpoint in one realm therefore held up token-based SMTP mail in every other realm, for as long as the HTTP client timeouts allowed.The lock is now per realm, so only concurrent refreshes for the same realm wait on each other. The cache is also read again once the lock is held. Before, the re-check used the entry read before waiting, so every request that had queued up fetched its own token.
SMTPTokenAuthConcurrencyTestcovers both cases with a token endpoint that holds the request until the test releases it, and both tests fail without the change. I put them in a new class rather thanSMTPConnectionTestbecause they need two realms and a mock token endpoint.A token endpoint that keeps the connection open still delays mail for its own realm for as long as it does so, because the token request has no overall timeout. I kept that out of this change. I can follow up with a bounded wait or a request timeout in a separate PR, or add it here if you'd rather have it in one place.