Race condition in EndpointDiscoveryRefreshCache can trigger duplicate concurrent endpoint-discovery refresh calls
- Dominant language
- Java
- Stars
- 2.6k
- Forks
- 1k
- Avg merge
- 2d 9h
- Merged PRs (30d)
- 51
Description
### Describe the bug
`EndpointDiscoveryRefreshCache#returnCachedOrDefaultEndpoint` decides whether to kick off a background refresh of an expired cached endpoint with an unguarded check-then-act:
```java
// core/sdk-core/src/main/java/software/amazon/awssdk/core/endpointdiscovery/EndpointDiscoveryRefreshCache.java
private URI returnCachedOrDefaultEndpoint(String key, EndpointDiscoveryEndpoint endpoint, EndpointDiscoveryRequest request) {
...
if (endpoint.expirationTime().isBefore(Instant.now())) {
cache.put(key, endpoint.toBuilder().expirationTime(Instant.now().plusSeconds(60)).build());
refreshCacheAsync(request, key);
}
return endpoint.endpoint();
}
```
Neither the `isBefore` read nor the `cache.put`/`refreshCacheAsync` decision is guarded by any synchronization, CAS, or "refresh in flight" flag. This is inconsistent with the sibling branch a few lines above (the "no cached entry yet" case), which correctly uses `cache.putIfAbsent` as a compare-and-swap so only one caller wins and triggers discovery:
```java
if (endpoint == null) {
EndpointDiscoveryEndpoint previousValue = cache.putIfAbsent(key, tempEndpoint);
if (previousValue != null) {
return previousValue.endpoint();
}
refreshCacheAsync(request, key);
return tempEndpoint.endpoint();
}
```
Every thread that reads the same expired cache entry before any of them writes back independently decides it's expired and independently calls `refreshCacheAsync`, firing duplicate calls against the real endpoint-discovery service API for the same cache key — exactly at the moment of highest concurrent load, right when an entry expires.
### Expected Behavior
Only one background refresh call should be triggered per cache-key expiration, regardless of how many threads concurrently observe the expired entry — matching the guarantee already provided for the "no cached entry yet" case.
### Current Behavior
Multiple threads racing on the same just-expired entry can each independently call `refreshCacheAsync`, resulting in duplicate concurrent calls to the endpoint-discovery API for the same key.
### Reproduction Steps
Since this class's control flow only depends on `java.net.URI`/`java.time.Instant`/`ConcurrentHashMap`, I extracted a byte-for-byte faithful copy of `EndpointDiscoveryRefreshCache` and `EndpointDiscoveryEndpoint` (only `EndpointDiscoveryRequest`/`EndpointDiscoveryCacheLoader`/`EndpointDiscoveryFailedException` were stubbed with matching signatures to avoid pulling in the rest of `sdk-core`'s dependency graph) into a standalone harness, compiled and run with plain `javac`/`java` (Java 21).
The harness primes the cache with an already-expired entry (via reflection into the private `cache` field, to simulate the exact moment an entry expires), then releases 50 threads simultaneously to call `get()` against it, counting how many times the background discovery call actually fires. Expected: 1. Observed against the current code (5 runs):
```
Concurrent callers: 50
Background discovery (refresh) calls fired: 2
Concurrent callers: 50
Background discovery (refresh) calls fired: 2
Concurrent callers: 50
Background discovery (refresh) calls fired: 1
Concurrent callers: 50
Background discovery (refresh) calls fired: 2
Concurrent callers: 50
Background discovery (refresh) calls fired: 1
```
Under real network latency for the discovery call (vs. this instant in-process stub), the race window is much wider and the duplicate-call count would be higher for a burst of concurrent requests hitting a just-expired key.
### Possible Solution
Guard the refresh decision with `cache.replace(key, oldValue, newValue)` — a compare-and-swap against the exact stale value each caller read — so only the thread that actually wins the race replaces the entry and triggers the refresh. I have a fix (verified against the same harness — repeated runs now consistently show exactly 1 refresh call) and will open a PR shortly.
### Version
Reproduced against current `master` (commit `e377d53f`, 2.53.2-SNAPSHOT).
Contributor guide
Assessment
This issue has not been assessed yet.