Skip to content

feat(auth): fix ClientSideCredentialAccessBoundary race condition#12681

Open
vverman wants to merge 1 commit intogoogleapis:mainfrom
vverman:fix-cab-race-condition
Open

feat(auth): fix ClientSideCredentialAccessBoundary race condition#12681
vverman wants to merge 1 commit intogoogleapis:mainfrom
vverman:fix-cab-race-condition

Conversation

@vverman
Copy link
Copy Markdown
Contributor

@vverman vverman commented Apr 5, 2026

This change addresses a race condition in ClientSideCredentialAccessBoundaryFactory that occurred when multiple concurrent calls were made to generateToken.

The fix involves:

  • Waiting on the RefreshTask itself rather than its internal task.
  • Using a single listener in RefreshTask to ensure finishRefreshTask completes before the outer future unblocks waiting threads.
  • Adding a regression test generateToken_freshInstance_concurrent_noNpe.

This change addresses a race condition in ClientSideCredentialAccessBoundaryFactory that occurred when multiple concurrent calls were made to generateToken.

The fix involves:
- Waiting on the RefreshTask itself rather than its internal task.
- Using a single listener in RefreshTask to ensure finishRefreshTask completes before the outer future unblocks waiting threads.
- Adding a regression test generateToken_freshInstance_concurrent_noNpe.
@vverman vverman requested review from a team as code owners April 5, 2026 00:12
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

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 refactors the RefreshTask in ClientSideCredentialAccessBoundaryFactory to use a single listener, ensuring internal state updates occur before the future completes. It also simplifies the refresh logic and adds a concurrent test case to address a race condition. Feedback was provided to enhance the robustness of error handling in the task listener, specifically by catching Throwable and handling null causes in ExecutionException to avoid potential hangs.

Comment on lines 505 to 509
} catch (ExecutionException e) {
Throwable cause = e.getCause();
RefreshTask.this.setException(cause);
}
},
MoreExecutors.directExecutor());

// Add callback to set the result or exception based on the outcome.
Futures.addCallback(
task,
new FutureCallback<IntermediateCredentials>() {
@Override
public void onSuccess(IntermediateCredentials result) {
RefreshTask.this.set(result);
}

@Override
public void onFailure(@Nullable Throwable t) {
RefreshTask.this.setException(
t != null ? t : new IOException("Refresh failed with null Throwable."));
RefreshTask.this.setException(e.getCause());
} catch (Exception e) {
RefreshTask.this.setException(e);
}
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

The error handling in this listener is less robust than the previous implementation. Specifically:

  1. Catching Exception instead of Throwable: If finishRefreshTask or Futures.getDone throws an Error (e.g., OutOfMemoryError or NoClassDefFoundError), it will not be caught. This would cause the listener to terminate abruptly without completing the RefreshTask future, leading to all threads waiting on currentRefreshTask.get() hanging indefinitely.
  2. Potential NullPointerException: RefreshTask.this.setException(e.getCause()) will throw a NullPointerException if e.getCause() is null (which is technically possible for an ExecutionException, though rare from Future.get()). Since AbstractFuture.setException does not allow null arguments, this would also cause the listener to crash and the future to hang.

Using Throwable and adding a null check for the cause ensures the future is always completed even in extreme failure scenarios.

            } catch (ExecutionException e) {
              Throwable cause = e.getCause();
              RefreshTask.this.setException(cause != null ? cause : e);
            } catch (Throwable t) {
              RefreshTask.this.setException(t);
            }

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.

1 participant