Skip to content

test(auth): fix flaky refreshCredentialsIfRequired_asyncSingleThread test - #14790

Draft
lqiu96 wants to merge 2 commits into
mainfrom
fix/auth-cab-async-singlethread-12575
Draft

lqiu96 wants to merge 2 commits into
mainfrom
fix/auth-cab-async-singlethread-12575

Conversation

@lqiu96

@lqiu96 lqiu96 commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

Fixes: #12575

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a CountDownLatch to coordinate thread synchronization in the asynchronous single-thread test refreshCredentialsIfRequired_asyncSingleThread. The feedback points out that relying on the exact number of internal clock calls for synchronization is fragile and prone to flakiness. Instead, the reviewer recommends using a simpler CountDownLatch(1) to pause the background thread until assertions are complete, which decouples the synchronization from the clock and simplifies the mock clock implementation.

Comment on lines +292 to +319
int numRefresh = 3;
// Wait for all numRefresh calls to make both clock.currentTimeMillis() calls (numRefresh * 2)
// plus 1 extra permit so the background STS request remains paused in
// MockStsTransportFactory.create() until after we assert that the request count is still 1.
CountDownLatch refreshEntryLatch = new CountDownLatch(numRefresh * 2 + 1);
final ClientSideCredentialAccessBoundaryFactory factory =
getClientSideCredentialAccessBoundaryFactory(RefreshType.ASYNC);
getClientSideCredentialAccessBoundaryFactory(RefreshType.ASYNC, refreshEntryLatch);

// Call refreshCredentialsIfRequired() once to initialize the intermediate credentials. This
// should make one request to the STS endpoint.
factory.refreshCredentialsIfRequired();
assertEquals(1, mockStsTransportFactory.transport.getRequestCount());

// Subsequent calls to refreshCredentialsIfRequired() in an async mode should NOT
// immediately call the STS endpoint. They should schedule an asynchronous refresh.
int numRefresh = 3;
for (int i = 0; i < numRefresh; i++) {
factory.refreshCredentialsIfRequired();
}
try {
// Subsequent calls to refreshCredentialsIfRequired() in an async mode should NOT
// immediately call the STS endpoint. They should schedule an asynchronous refresh.
for (int i = 0; i < numRefresh; i++) {
factory.refreshCredentialsIfRequired();
}

// Verify that only the initial call resulted in an immediate STS request. The async refresh
// is still pending.
assertEquals(1, mockStsTransportFactory.transport.getRequestCount());
// Verify that only the initial call resulted in an immediate STS request. The async refresh
// is still pending.
assertEquals(1, mockStsTransportFactory.transport.getRequestCount());
} finally {
while (refreshEntryLatch.getCount() > 0) {
refreshEntryLatch.countDown();
}
}

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.

medium

Relying on the exact number of clock.currentTimeMillis() calls (numRefresh * 2 + 1) to coordinate thread synchronization is fragile. If the internal implementation of refreshCredentialsIfRequired() changes to call the clock more or fewer times, this test will become flaky or fail.

Instead, we can use a simple CountDownLatch(1) to pause the background thread in MockStsTransportFactory.create() until the main thread has finished its assertions. This completely decouples the synchronization from the internal clock calls and makes the test much more robust and maintainable.

You can then also revert the changes to createMockClock (and its signature) so it doesn't need to accept or interact with the latch.

    int numRefresh = 3;
    CountDownLatch refreshEntryLatch = new CountDownLatch(1);
    final ClientSideCredentialAccessBoundaryFactory factory =
        getClientSideCredentialAccessBoundaryFactory(RefreshType.ASYNC, refreshEntryLatch);

    // Call refreshCredentialsIfRequired() once to initialize the intermediate credentials. This
    // should make one request to the STS endpoint.
    factory.refreshCredentialsIfRequired();
    assertEquals(1, mockStsTransportFactory.transport.getRequestCount());

    try {
      // Subsequent calls to refreshCredentialsIfRequired() in an async mode should NOT
      // immediately call the STS endpoint. They should schedule an asynchronous refresh.
      for (int i = 0; i < numRefresh; i++) {
        factory.refreshCredentialsIfRequired();
      }

      // Verify that only the initial call resulted in an immediate STS request. The async refresh
      // is still pending.
      assertEquals(1, mockStsTransportFactory.transport.getRequestCount());
    } finally {
      refreshEntryLatch.countDown();
    }

@lqiu96
lqiu96 force-pushed the fix/auth-cab-async-singlethread-12575 branch from 1b0fef3 to 55f9f23 Compare October 27, 2026 21:21

This branch has not been deployed

No deployments
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.

[google-auth-library-java] Flaky Test: ClientSideCredentialAccessBoundaryFactoryTest.refreshCredentialsIfRequired_asyncSingleThread

1 participant