From 83b1a5b1071ecb41b798f9f090acebcbe60f8973 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Thu, 11 Jun 2026 17:11:04 +0000 Subject: [PATCH] fix(auth): resolve flakiness in ClientSideCredentialAccessBoundaryFactoryTest Synchronize the decision to refresh and task creation to avoid race conditions. Use Double-Checked Locking to minimize lock contention on the hot path. Resolves: #12871 --- ...ntSideCredentialAccessBoundaryFactory.java | 65 ++++++++++++------- ...deCredentialAccessBoundaryFactoryTest.java | 16 +++-- 2 files changed, 50 insertions(+), 31 deletions(-) diff --git a/google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java b/google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java index 46ab2fd2d24c..136c931a8984 100644 --- a/google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java +++ b/google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java @@ -146,7 +146,7 @@ public class ClientSideCredentialAccessBoundaryFactory { private final Duration refreshMargin; private RefreshTask refreshTask; private final Object refreshLock = new byte[0]; - private IntermediateCredentials intermediateCredentials = null; + private volatile IntermediateCredentials intermediateCredentials = null; private final Clock clock; private final CelCompiler celCompiler; @@ -234,8 +234,26 @@ void refreshCredentialsIfRequired() throws IOException { return; } - // If a refresh is required, create or retrieve the refresh task. - RefreshTask currentRefreshTask = getOrCreateRefreshTask(); + RefreshTask currentRefreshTask; + // Synchronize both the decision to refresh and the task creation to prevent a race + // condition. Without this, multiple threads might concurrently determine a refresh is + // needed (e.g., in ASYNC mode) and return ASYNC. The first thread would start the refresh + // task. If that task completes quickly and clears the `refreshTask` reference, a subsequent + // thread that also determined a refresh was needed would then see `refreshTask` as null + // and incorrectly start a second, redundant refresh task. + synchronized (refreshLock) { + // Re-evaluate the refresh type under the lock, as another thread might have completed + // the refresh while this thread was waiting for the lock. + refreshType = determineRefreshType(); + + if (refreshType == RefreshType.NONE) { + // No refresh needed, token is still valid. + return; + } + + // If a refresh is required, create or retrieve the refresh task. + currentRefreshTask = getOrCreateRefreshTask(); + } // Handle the refresh based on the determined refresh type. switch (refreshType) { @@ -283,16 +301,14 @@ void refreshCredentialsIfRequired() throws IOException { } } + // Assumes the caller holds refreshLock. private RefreshType determineRefreshType() { - AccessToken intermediateAccessToken; - synchronized (refreshLock) { - if (intermediateCredentials == null - || intermediateCredentials.intermediateAccessToken == null) { - // A blocking refresh is needed if the intermediate access token doesn't exist. - return RefreshType.BLOCKING; - } - intermediateAccessToken = intermediateCredentials.intermediateAccessToken; + if (intermediateCredentials == null + || intermediateCredentials.intermediateAccessToken == null) { + // A blocking refresh is needed if the intermediate access token doesn't exist. + return RefreshType.BLOCKING; } + AccessToken intermediateAccessToken = intermediateCredentials.intermediateAccessToken; Date expirationTime = intermediateAccessToken.getExpirationTime(); if (expirationTime == null) { @@ -322,23 +338,22 @@ private RefreshType determineRefreshType() { * responsibility of the caller to execute it. The task will clear the single flight slot upon * completion. */ + // Assumes the caller holds refreshLock. private RefreshTask getOrCreateRefreshTask() { - synchronized (refreshLock) { - if (refreshTask != null) { - // An existing refresh task is already in progress. Return a NEW RefreshTask instance with - // the existing task, but set isNew to false. This indicates to the caller that a new - // refresh task was NOT created. - return new RefreshTask(refreshTask.task, false); - } + if (refreshTask != null) { + // An existing refresh task is already in progress. Return a NEW RefreshTask instance with + // the existing task, but set isNew to false. This indicates to the caller that a new + // refresh task was NOT created. + return new RefreshTask(refreshTask.task, false); + } - final ListenableFutureTask task = - ListenableFutureTask.create(this::fetchIntermediateCredentials); + final ListenableFutureTask task = + ListenableFutureTask.create(this::fetchIntermediateCredentials); - // Store the new refresh task in the refreshTask field before returning. This ensures that - // subsequent calls to this method will return the existing task while it's still in progress. - refreshTask = new RefreshTask(task, true); - return refreshTask; - } + // Store the new refresh task in the refreshTask field before returning. This ensures that + // subsequent calls to this method will return the existing task while it's still in progress. + refreshTask = new RefreshTask(task, true); + return refreshTask; } /** diff --git a/google-auth-library-java/cab-token-generator/javatests/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java b/google-auth-library-java/cab-token-generator/javatests/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java index 5d8aa4946bb4..551f636cac02 100644 --- a/google-auth-library-java/cab-token-generator/javatests/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java +++ b/google-auth-library-java/cab-token-generator/javatests/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java @@ -70,7 +70,6 @@ import java.util.Map; import java.util.concurrent.CountDownLatch; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; /** @@ -325,7 +324,6 @@ void refreshCredentialsIfRequired_blockingMultiThread() throws IOException, Inte } @Test - @Disabled("Flaky test: https://github.com/googleapis/google-cloud-java/issues/12871") void refreshCredentialsIfRequired_asyncMultiThread() throws IOException, InterruptedException { final ClientSideCredentialAccessBoundaryFactory factory = getClientSideCredentialAccessBoundaryFactory(RefreshType.ASYNC); @@ -623,10 +621,16 @@ private Clock createMockClock(RefreshType refreshType, GoogleCredentials sourceC // (within the refresh margin). mockedTimeInMillis = expirationTimeInMillis - refreshMarginInMillis + 60000; when(mockClock.currentTimeMillis()) - .thenReturn( - mockedTimeInMillis, // First call: Stale (triggers the async refresh) - currentTimeInMillis // Subsequent calls: Fresh (skips redundant refreshes) - ); + .thenAnswer( + invocation -> { + // If the async refresh has already been triggered (request count >= 2), + // return the fresh time to skip redundant refreshes. + // Note: 1st request was the initial blocking refresh. + if (mockStsTransportFactory.transport.getRequestCount() >= 2) { + return currentTimeInMillis; + } + return mockedTimeInMillis; + }); break; case BLOCKING: // Set mocked time so that the token requires immediate refresh (just after the minimum