Skip to content

fix(sdk-core): avoid duplicate concurrent endpoint-discovery refresh calls - #7284

Open
Adityaj0 wants to merge 2 commits into
aws:masterfrom
Adityaj0:fix-endpoint-discovery-refresh-race
Open

fix(sdk-core): avoid duplicate concurrent endpoint-discovery refresh calls#7284
Adityaj0 wants to merge 2 commits into
aws:masterfrom
Adityaj0:fix-endpoint-discovery-refresh-race

Conversation

@Adityaj0

Copy link
Copy Markdown

Motivation and Context

fixes #7283

EndpointDiscoveryRefreshCache#returnCachedOrDefaultEndpoint decides whether to refresh an expired cached endpoint with an unguarded check-then-act:

if (endpoint.expirationTime().isBefore(Instant.now())) {
    cache.put(key, endpoint.toBuilder().expirationTime(Instant.now().plusSeconds(60)).build());
    refreshCacheAsync(request, key);
}

Every thread that reads the same expired entry before any of them writes back independently decides to refresh, firing duplicate refreshCacheAsync calls against the real endpoint-discovery API for the same key. The sibling "no cached entry yet" branch a few lines above already handles this correctly with cache.putIfAbsent as a compare-and-swap; this branch never got the equivalent treatment.

Modifications

Guard the refresh decision with cache.replace(key, oldValue, newValue), a CAS against the exact stale endpoint value the caller read from cache.get(key). Only the thread whose CAS succeeds — i.e. the entry hasn't already been replaced by a racing thread — proceeds to call refreshCacheAsync:

if (endpoint.expirationTime().isBefore(Instant.now())) {
    EndpointDiscoveryEndpoint refreshedEndpoint =
        endpoint.toBuilder().expirationTime(Instant.now().plusSeconds(60)).build();
    if (cache.replace(key, endpoint, refreshedEndpoint)) {
        refreshCacheAsync(request, key);
    }
}

EndpointDiscoveryEndpoint doesn't override equals(), so ConcurrentHashMap#replace(key, oldValue, newValue)'s equality check falls back to reference identity — which is exactly right here, since all racing threads observe the same object reference from the earlier cache.get(key) read in get()/getAsync().

Testing

Added get_concurrentCallsOnExpiredEntry_onlyRefreshesOnce to EndpointDiscoveryRefreshCacheTest: primes the cache with an already-expired entry (via reflection into the private cache field, simulating the exact moment an entry expires), then fires 50 threads at get() simultaneously and asserts the mocked loader's discoverEndpoint is invoked exactly once.

I don't have mvn/a Maven-resolvable environment available locally, so I wasn't able to run this exact test file through the project's normal build. Instead I verified the underlying fix independently: I extracted a byte-for-byte faithful copy of the real EndpointDiscoveryRefreshCache.java and EndpointDiscoveryEndpoint.java (only the auxiliary EndpointDiscoveryRequest/EndpointDiscoveryCacheLoader/EndpointDiscoveryFailedException types were replaced with minimal stand-ins matching their real signatures, to avoid pulling in the rest of sdk-core's dependency graph) into a standalone harness, compiled with plain javac (Java 21), and ran the same 50-thread concurrent-get()-on-expired-entry scenario as the new test:

  • Against the original code: 1–2 duplicate discoverEndpoint calls across 5 runs (expected: 1).
  • Against the fixed code: exactly 1 call, consistently across repeated runs.

I'm flagging this explicitly so a maintainer/CI can confirm the added test file itself compiles and passes cleanly in the real build — the fix's correctness is verified, but the exact test file hasn't been run through the project's own toolchain by me.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of ./mvnw package succeeds -- not verified locally (no mvn available in my environment); verified the underlying fix via an independent standalone harness instead, see Testing section
  • I have added tests to exercise the new behavior
  • My change is not breaking the existing public API

…calls

EndpointDiscoveryRefreshCache#returnCachedOrDefaultEndpoint decides
whether to kick off a background refresh of an expired cached endpoint
with a plain check-then-act: read the cached entry's expirationTime,
and if it's in the past, cache.put() a bumped-expiration copy and call
refreshCacheAsync. Neither the read nor the put/decision is guarded,
unlike the sibling "no cached entry yet" branch a few lines above,
which correctly uses cache.putIfAbsent as a compare-and-swap so only
one caller wins and triggers discovery.

Every thread that reads the same expired entry before any of them
writes back independently decides it's expired and independently
calls refreshCacheAsync, firing duplicate calls against the real
endpoint-discovery API for the same cache key -- exactly at the
moment of highest concurrent load, right when an entry expires.

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.
@Adityaj0
Adityaj0 requested a review from a team as a code owner August 15, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in EndpointDiscoveryRefreshCache can trigger duplicate concurrent endpoint-discovery refresh calls

1 participant