feat(gax): support transparent retries during mTLS certificate rotations

[macastelaz]

Description

This PR adds transparent retries for mTLS workload certificate rotation to the gRPC and HTTP/JSON transports. When a request fails with UNAUTHENTICATED and the workload certificate on disk has changed, the transport is rebuilt with the new certificate and the request is retried once, without interrupting in-flight RPCs or streams.

🚀 Core Features & Architectural Updates

• Rotation detection: CertificateRotationTracker compares the SHA-256 fingerprint of the workload certificate on disk with the certificate the transport was built with. The check only runs after an auth failure (never on the request path), and positive results are cached for at most 1 second so a burst of failures doesn't trigger a burst of file reads. Empty or mid-write files are ignored.
• gRPC: ChannelPool replaces all of its channels when a rotation is detected. Calls already in flight finish on their old channels, which are shut down once idle. During a rotation refresh, a channel that can't be recreated is dropped rather than kept, so no traffic keeps using the old certificate; a statically sized pool is refilled in the background.
• HTTP/JSON: RefreshingHttpJsonChannel swaps the channel's HttpTransport for one built with the new certificate. Calls already created keep using the transport they started with, so in-flight requests aren't interrupted.
• Retries: AttemptCallable (unary) and ServerStreamingAttemptCallable (server streaming) refresh the transport after an UNAUTHENTICATED failure. If the transport moved to a new certificate during or after the attempt, the failure is flagged for a single immediate retry on the refreshed channel; its configured isRetryable() value is unchanged.
• ApiResultRetryAlgorithm gives that retry once, immediately, without using a regular attempt.
• If the free retry also fails after another rotation, the call stops instead of falling back to backoff retries.
• For server streams, the free retry becomes available again once the stream has made progress, and the stream resumes through the existing resumption strategy.
• Client and bidi streams refresh the transport on UNAUTHENTICATED but don't retry; the next stream uses the new certificate.
• Plumbing: TransportChannel gains shouldRefresh(), refresh() and getGeneration(), which default to no-ops. ApiCallContext.getTransportChannel() gives retrying callables access to the channel, and GrpcCallContext / HttpJsonCallContext carry it through merge() and withChannel().

🔒 System Hardening & Bug Fixes

• Outstanding RPC leak (ChannelPool.ReleasingClientCall): if a call was cancelled before start(), start() threw without releasing the channel entry, leaving the channel with a permanently outstanding RPC count so it could never be cleaned up after a refresh. The entry is now released on that path and on cancellation before start.
• HTTP/JSON refresh vs. shutdown: shutdown() / shutdownNow() are serialized with refresh() so a transport swap can't race with teardown.

⚠️ Behavioral & Security Boundary Changes

• mTLS is enabled automatically when a workload certificate config is present (MtlsUtils, auth library):

  • When GOOGLE_API_CERTIFICATE_CONFIG, or the default gcloud certificate_config.json, contains a workload certificate, client certificates are used even if GOOGLE_API_USE_CLIENT_CERTIFICATE is unset.
  • GOOGLE_API_USE_CLIENT_CERTIFICATE=false still turns mTLS off.

• mTLS misconfiguration now fails closed (MtlsUtils):

  • Previously, an invalid explicit config was swallowed and the client silently connected without mTLS.
  • Now client creation throws IllegalStateException in these cases:
    • the explicit GOOGLE_API_CERTIFICATE_CONFIG file is missing, unreadable or malformed;
    • the default config file exists but is unreadable or malformed;
    • a configured certificate or key file can't be read.
  • When mTLS is required but the transport can't load the client certificate, gRPC and HTTP/JSON channel creation fails with an IOException instead of falling back to a non-mTLS connection.

🧪 Testing

Automated Testing

• Added and updated comprehensive unit-tests reflecting the thread-safety
fixes inside ChannelPoolTest.java and RefreshingHttpJsonChannelTest.java.
• Corrected edge case test configurations to leverage realistic mocked X.509
certificates to properly exercise deep WorkloadCertificateUtils.
getCertificateFingerprint() filesystem caching mechanisms.

Manual Testing

End-to-end tests with real generated GAPIC clients against local mTLS servers, run on this PR's head. Each scenario runs in its own JVM and checks the exact sequence of client certificates and outcomes the server saw.

• Setup
  • Clients: KeyManagementServiceClient over gRPC and HTTP/JSON, and GrpcBigQueryReadStub for server-streaming ReadRows with offset-based resumption.
  • Servers: local gRPC and HTTPS servers that require a client certificate and return UNAUTHENTICATED / 401 for "revoked" certificates (by CN).
  • Rotation: the workload certificate under GOOGLE_API_CERTIFICATE_CONFIG is replaced by atomic rename, key first, then cert.
• gRPC unary:
  • basic rotation: one rejected attempt, then a free retry on the new cert, and no extra attempts afterwards;
  • the server rejects every cert: one free retry, then stop;
  • a second rotation during the free retry: stops after 2 attempts, with no backoff retry;
  • UNAVAILABLE, then rotation;
  • corrupt cert on disk: no crash, the call fails, and the client recovers once a valid cert is written;
  • 20-call bursts across 2 rotations less than 1s apart: all succeed with exactly 1 refresh each;
  • a 3-channel pool: no stale channel after refresh;
  • 8 threads across 4 rotations, then close() with calls in flight: 70,624/70,624 calls succeeded, no hangs.
• gRPC server streaming:
  • rotation before the stream starts;
  • rotation mid-stream: resumes at the right offset with no gaps or duplicates;
  • rotation retry re-armed after the stream makes progress, following an earlier UNAVAILABLE retry;
  • bounded: a second rotation without progress stops the stream, with no third attempt.
• HTTP/JSON: basic rotation; reject-all; double rotation; a slow call started before rotation completes on the old cert while new calls use the new cert; corrupt cert, then restore; 8-thread stress with 4 rotations, then close() (49,561/49,561 calls succeeded, no hangs). The socket factory was verified for both Conscrypt and plain JDK TLS.
• No mTLS (GOOGLE_API_USE_CLIENT_CERTIFICATE=false, or no certificate config): a single attempt with no client cert and no refresh, over both gRPC and HTTP/JSON, even when the cert files on disk change.
• Results:
  • JDK 26 (JDK TLS): all 22 scenarios pass.
  • JDK 21 (Conscrypt): all gRPC, streaming and no-mTLS scenarios pass.
  • HTTP/JSON with mTLS and Conscrypt on TLS 1.3 fails with an existing Unknown authType: GENERIC handshake error, which fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext #14556 fixes. With fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext #14556 applied on top of this PR, all 22 scenarios pass on JDK 21 with Conscrypt active.
  • Harness source: https://paste.googleplex.com/6120902048219136
  • Logs: https://paste.googleplex.com/5467233782988800 (this PR on JDK 26 and JDK 21), https://paste.googleplex.com/5563956459077632 (this PR + fix(gax-httpjson): use Conscrypt TrustManagerFactory for mTLS SSLContext #14556 on JDK 21)

[gemini-code-assist]

Code Review

This pull request introduces support for dynamic mTLS certificate rotation across both gRPC and HTTP/JSON transports by enabling thread-safe channel hot-swapping and automatic refreshing upon encountering an UnauthenticatedException. Key additions include the RefreshingHttpJsonChannel and updates to various callables to intercept and retry unauthenticated errors. However, several critical issues were identified during review: a bug in ChannelPool.refresh() that breaks the GFE channel refresh mechanism for non-mTLS connections; a potential resource leak in RefreshingHttpJsonChannel due to a missing cancel override; regressions caused by the removal of Conscrypt security provider configurations; and incomplete exception wrapping in several streaming callables that results in the loss of the original stack trace, cause, and suppressed exceptions of UnauthenticatedException.

[nbayati]

Some feedback on the auth side of things.

[nbayati]

Could this break the ECP flow? Do we need to check that "workload" exists but "certificate_file" does not exist?

[macastelaz]

This method is currently only called from google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/IdentityPoolCredentials.java and from what I understand, ECP is not applicable for IdentityPool credentials so throwing here would be acceptable - but let me know if I'm missing something or if you'd like to see other handling here.

[nbayati]

A couple of issues with the UnauthenticatedException handling across ServerStreamingAttemptCallable, BidiStreamingCallable, and ClientStreamingCallable:

1. transportChannel.refresh() is invoked without a try-catch. If refresh() throws an unchecked exception, the terminal onError callback never fires, which will leave stream observers or retrying futures hanging indefinitely. Any refresh failure should be caught and logged so the original error still reaches the observer.
2. We shouldn't re-wrap the exception with isRetryable = true:
   • For client and bidi streaming, GAX has no stream retry mechanism, so marking it retryable is inert internally and misleading to callers.
   • For server streaming, setting isRetryable = true causes StreamingRetryAlgorithm to attempt to resume the stream. In the "Graceful Certificate Rotation Handling" section of go/sdk-mds-bound-token , we said streaming calls should not be auto-retried mid-stream. Instead, we should trigger the channel refresh so subsequent calls use the new connection, but propagate the original error directly without marking it retryable. Let me know if you don't agree with this though, maybe it's a shortcoming of the original HLD that we need to revisit and update.

[lqiu96]

qq, do you think we can check this first? If another thread has already refreshed the channels (e.g. acquired the lock first), then we don't need to check the channel via shouldRefresh.

channel.getGeneration() > attemptGeneration should indicate that we need to retry and we can skip the check above. IIUC, the check above is needed for the first call to acquire the lock

[macastelaz]

Good call, done. In both AttemptCallable and ServerStreamingAttemptCallable the handler now checks getGeneration() > attemptGeneration first, and only calls shouldRefresh()/refresh() if the generation hasn't moved. As you said, the disk check is still needed for the first failing call to detect the rotation. Retry decisions don't change; this just skips the cert read and hash for calls that fail after another thread has already refreshed. I also moved shouldRefresh() inside the try so a failed fingerprint read can't hide the original UNAUTHENTICATED. Added testGenerationAlreadyAdvanced_skipsShouldRefresh and the streaming equivalent.

[lqiu96]

Hmm, I didn't realize refreshAll() increments the generation. refreshSafely gets called every 50 minutes as part of channelpool so I think we will need to distinguish between a rotation (e.g. 401 channel refresh) vs a refresh from a background task

[macastelaz]

Good catch. Generation was bumped on every refreshAll(), including the 50-minute preemptive refresh, so a non-rotation UNAUTHENTICATED that overlapped a background refresh got a free retry. The increment is now separate from refreshAll() and only happens when the pool switches to a new certificate: in refresh() after the rotation swap, and in refreshSafely() only when the disk fingerprint differs from the active one and every channel was recreated. That also keeps "a generation bump means every channel has the new certificate" true for the preemptive path. It still happens after the swap and before the new fingerprint is marked active. HttpJson isn't affected since RefreshingHttpJsonChannel has no periodic refresh. Added preemptiveRefresh_withoutRotation_doesNotIncrementGeneration and related tests.

While in here I also fixed a related shutdown issue: shutdown()/shutdownNow() cancelled the refresh future while holding entryWriteLock, which the running refresh itself holds, so shutdown waited for an in-progress refresh instead of interrupting it. The futures are now cancelled before taking the lock; setting isShutdown and shutting down the entries still happen under it. Added shutdown_interruptsInProgressRefresh.

[lqiu96]

(Flagged from Gemini) I think I may have been wrong on the original behavior of hijacking the isRetryable parameter. Gemini flags that overloads the value when user configures custom status code params.

Suggestion from Gemini:

    @NullMarked
    public class UnauthenticatedException extends ApiException {
      private final boolean channelRefreshed;

      // Existing public constructors delegate with channelRefreshed = false ...

      private UnauthenticatedException(
          String message,
          Throwable cause,
          StatusCode statusCode,
          boolean retryable,
          ErrorDetails errorDetails,
          boolean channelRefreshed) {
        super(message, cause, statusCode, retryable, errorDetails);
        this.channelRefreshed = channelRefreshed;
      }

      boolean isChannelRefreshed() {
        return channelRefreshed;
      }

      UnauthenticatedException withChannelRefreshed() {
        UnauthenticatedException newEx =
            new UnauthenticatedException(
                getMessage(), getCause(), getStatusCode(), true, getErrorDetails(), true);
        newEx.setStackTrace(getStackTrace());
        for (Throwable suppressed : getSuppressed()) {
          newEx.addSuppressed(suppressed);
        }
        return newEx;
      }
    }

if shouldRetry, then we will call unauthenticatedException.withChannelRefreshed() to mark that we need to retry and pass this over to an update ApiResultRetryAlgo:

     @Override
      public @Nullable TimedAttemptSettings createNextAttempt(
          @Nullable RetryingContext context,
          @Nullable Throwable previousThrowable,
          @Nullable ResponseT previousResponse,
          TimedAttemptSettings previousSettings) {
        if (previousThrowable instanceof UnauthenticatedException
            && ((UnauthenticatedException) previousThrowable).isChannelRefreshed()) {
          if (previousSettings.getOverallAttemptCount() == previousSettings.getAttemptCount()) {
            RetrySettings globalSettings = previousSettings.getGlobalSettings();
            if (globalSettings.getMaxAttempts() == 0
                && globalSettings.getTotalTimeoutDuration().isZero()) {
              globalSettings = globalSettings.toBuilder().setMaxAttempts(1).build();
            }
            return previousSettings.toBuilder()
                .setGlobalSettings(globalSettings)
                .setRetryDelayDuration(java.time.Duration.ZERO)
                .setRandomizedRetryDelayDuration(java.time.Duration.ZERO)
                .setAttemptCount(previousSettings.getAttemptCount())
                .setOverallAttemptCount(previousSettings.getOverallAttemptCount() + 1)
                .build();
          }
          // The single rotation retry has already been used. Return exhausted settings so the retry
          // framework stops, rather than returning null and falling back to exponential backoff.
          int exhaustedAttemptCount = previousSettings.getAttemptCount() + 1;
          return previousSettings.toBuilder()
              .setGlobalSettings(
                  previousSettings.getGlobalSettings().toBuilder()
                      .setMaxAttempts(exhaustedAttemptCount)
                      .build())
              .setAttemptCount(exhaustedAttemptCount)
              .setOverallAttemptCount(previousSettings.getOverallAttemptCount() + 1)
              .build();
        }
        return null;
      }

I'll need to take a look at this tomorrow.

[macastelaz]

Thanks, agreed, this was a real problem and not only for mTLS. The exception factories set isRetryable() from the method's retry codes. So when UNAUTHENTICATED is configured as retryable, every 401 already arrived retryable, and the algorithm treated it as a rotation retry: one zero-delay retry and then stop, instead of the configured attempts and backoff. The early return in shouldRetry also overrode per-call retry codes. I went with your channelRefreshed flag, with a few adjustments:

• withChannelRefreshed() keeps the original isRetryable() instead of forcing true, so a surfaced error still reflects the configured retry codes.
• createNextAttempt and both shouldRetry overloads key on isChannelRefreshed(), so per-call retry codes are no longer overridden for ordinary 401s.
• Once the free retry is used, a flagged failure still returns exhausted settings, as in your snippet.

Server streaming goes through the same algorithm via StreamingRetryAlgorithm, so ServerStreamingAttemptCallable just calls withChannelRefreshed(). Both new methods are package-private, so there's no public API change.

Two notes for transparency. (1) Because of the last bullet, a second rotation within a single call stops that call, even for clients that configured UNAUTHENTICATED as retryable. This keeps the "stop after one rotation retry" behavior from the earlier round. (2) UnauthenticatedException had no explicit serialVersionUID, so the new methods would have changed the computed one. I pinned it to the value from the current releases (2.83–2.87) and made the flag transient, so the serialized form is unchanged. Tests check the UID, deserializing an exception serialized by the released jar, and the existing retry behaviour for configured and unconfigured UNAUTHENTICATED, per-call retry codes, and a null context.

[lqiu96]

(Flagged by Gemini)

Gemini noticed that this is supposed to log the current active fingerprint, but rotationTracker.getActiveCertFingerprint(); returns the old fingerprint as the new fingerprint value doesn't get set until after refreshAll() is called.

Hmm, perhaps we can move this log to after refreshAll() completes (or maybe we hold the diskFingerprint in a static var so that it always has the latest state)

[macastelaz]

Agreed, at that point the tracker still holds the old fingerprint. refreshAll now just logs "Refreshing all channels" (as before this PR), and the fingerprint is logged after a successful switch, next to markRefreshed(), using the fingerprint read from disk. No static or extra state. Added refresh_onRotation_logsNewCertificateFingerprint.

[lqiu96]

readDiskFingerprint looks to read from a file which may not be needed for cases where there is no mtls cert required.

Can we do a split where we check for the non-mtls case first:

1. If workloadCertPath == null, we simply refreshAll() without having to load the certificate
2. Otherwise, we proceed with the logic to read the cert and then to try to rotate?

[lqiu96]

Ah, basically what we have for refresh() down below

[macastelaz]

Done. refreshSafely() now matches refresh(): with no workloadCertPath it calls refreshAll() and returns without touching the certificate. Only the mTLS path reads the cert. (readDiskFingerprint() already returned "" without any file I/O when the path was null, so behavior is unchanged, but the split makes that explicit.)

[lqiu96]

IIUC, if workloadCertPath != null, the only case for currentDiskFingerprint being non-empty is during a rotation right?

Can we add a comment to note this (perhaps a debug log here as well that it'll recreate on the next refresh)

[macastelaz]

Yes, assuming you meant empty: with a configured path, an empty fingerprint means the certificate couldn't be read. In practice that's almost always a rotation in progress, where the file is empty or only partly written. A missing or unreadable file looks the same. Added a comment and a FINE log saying the refresh is skipped and the channels will be recreated on the next refresh, plus preemptiveRefresh_whenCertUnreadable_skipsRefreshAndLogs.

[lqiu96]

I know this is existing code, but I think it may be possible to clean this up. The looping through and finding a match via contains seems less than ideal, especially given the freq that we need to refresh and that we want to speed this up mid-call

Perhaps we can track newEntries and removedEntries.

1. Try to create a new channel, if success then add the new channel to newEntries
2. If failed, then if we dropUnfrefeshed, add it to shutdownEntries, otherwise, we add the old, non-refreshed channel to new entries

We should be able to just loop through shutdownEntries without checking if the final exists.

Perhaps to handle the case with createdEntries in finally, we'll need to also add new entries to createdEntries

[macastelaz]

Agreed, done as you suggested. refreshAll now makes one pass that builds keptEntries and retiredEntries. A successful recreate keeps the new channel and retires the old one. A failure retires the old channel when dropUnrefreshedChannels is set, and keeps it otherwise. After the swap we shut down retiredEntries directly, so the contains() scan is gone. New channels are still tracked in createdEntries so the finally block can shut them down if an Error aborts before the swap. Every write to entries already happens under entryWriteLock, so a plain set replaces the getAndSet. I also added tests confirming that each channel keeps its slot when old channels are kept, and that a rotation where every channel fails leaves the pool untouched (…allChannelsFail_leavesPoolUnchangedAndDoesNotRefill).

[lqiu96]

qq, I'm not sure I understand why we check for isStaticSize here. Most of the GAPICs would be static size == 1, but customers can override this value.

[lqiu96]

Oh nvm. I remembered out earlier convo about letting resize() populate this back up. We'll probably incur a bit of queueing if channel size drops, then the potential issue where there is a bunch of consecutive resizes. Let me think about this again.

[macastelaz]

I went ahead and removed the isStaticSize check. Any rotation refresh that drops channels now schedules the one-time refill, for dynamic pools as well as static ones. Without the refill, a dynamic pool that drops from N channels to a few only grows back by maxResizeDelta per resize interval, so traffic queues on fewer channels and you get the consecutive-resize pattern you mentioned. The refill avoids both. It runs once on the background executor, takes no retries, and is a no-op if resize() has already grown the pool or the pool has shut down. Happy to revisit if you'd rather leave dynamic pools to resize().

[lqiu96]

I think the target size should probably be the number of old entries in the channelPool (not the initial channel count as this doesn't account for potential channel/ load change - growth or loss of load).

I also realized that the load calculation is stored within a channel itself, so refreshing a channel ends up losing that data which is not idea, but is existing behavior so can be resolved in the future. Maybe something like a snapshot of load stored in channel pool and not in the channel or something.

[macastelaz]

Agreed, the refill now targets the number of channels the pool had before the refresh, not initialChannelCount, so it keeps whatever size resizing had settled on. For a static pool the two are the same. Added …partialFailureAfterResize_refillsToPreRefreshSize.

On the load stats: agreed. Each channel's outstanding-RPC peak is lost when it's recreated, which predates this PR. Keeping a load snapshot at the pool level would add new state, so I'd prefer a follow-up. I can open an issue if that works for you.

[lqiu96]

nit: Can we change the name to be from rotated to certChanged. I think rotated is a bit unclear as it may pertain to channel or cert.

Additionally, I might be missing this case, but why do we need need to markRefresh on L478 below when rotated is false? IIUC, think we should mark it only when we need to rotate?

If the cert changed, then we should opt refresh based on that: refreshAll(certChanged). But we would only do completeCertificateSwitch if certChanged is true as well.

I think the logic would be

if (refreshAll(certChanged) && certChanged) { completeCertificateSwitch(currentDiskFingerprint) };

refreshAll(false) && false is the normal refresh cycle for mtls with no certChanged so it shouldn't call completeCertificateSwitch

refreshAll(true) && true is to refresh the channels with certChanged

• But if refreshAll fails to refresh all the channels, then refresh() on the next call will try to update it.

[macastelaz]

Agreed on all three. Renamed to certChanged. The markRefreshed call in the no-change branch was redundant (the fingerprint already equals the active one), so it's gone. The periodic refresh now calls refreshAll(certChanged) and only completes the switch when the cert changed. A normal mTLS cycle keeps any channel that fails to rebuild, as before. When the cert changed, channels still on the old certificate are dropped (and the pool refilled), the same as the reactive refresh(). If no channel can be rebuilt, nothing changes and the next refresh retries. This extends the drop-on-rotation behavior we agreed for the reactive path (option 3) to the periodic refresh when it detects a changed cert, so it supersedes my earlier note that the periodic refresh always keeps per-slot fallback; that now only applies when the cert is unchanged. Both invariants still hold: the cert is only marked active, and the generation only bumps, once every channel left in the pool uses it. Updated the partial-failure test and added tests for the all-fail and no-change cases.

[lqiu96]

I think we should probably have a check similar to what we have in resize:

      // Double-check fingerprint inside the lock
      if (rotationTracker.isAlreadyActive(currentDiskFingerprint)) {
        LOG.fine(
            "Channel pool was already refreshed by a concurrent thread, skipping duplicate"
                + " refresh");
        return;
      }

[macastelaz]

For the periodic refresh, an isAlreadyActive early return would skip it in steady state (the cert on disk is normally the active one), and it still needs to recycle channels for the GFE disconnects. When the cert is unchanged it now just refreshes the channels without switching, and the 'already refreshed by a concurrent thread' case is covered by the pre-lock generation check from the other thread.

[lqiu96]

Double check my on this: synchronized ensures that only one refresh request goes out at once, but only ensure that we don't refresh via the rotationTracker.isAlreadyActive call.

This looks like it will still read the disk X - 1 times, if we have X refresh/ refreshSafely calls queued. I think we can optimize this a bit by checking the generation before the lock (e.g. preLockGenerationCount) and comparing it once it obtains the lock (generation.get()). If the obtained generation lock is after the pre-lock then we know that the channels have been refreshed.

I think we'll need to add this to refresh and refreshSafely since they are independent (50 min interval vs cert refresh)

[macastelaz]

Good catch: refresh() reads the fingerprint uncached inside the lock, so callers queued behind a rotation refresh each re-read the cert file. Both refresh() and the periodic refresh now snapshot the generation before taking the lock and return early if it advanced while waiting, since a concurrent refresh already switched every channel to a new certificate. If that refresh couldn't rebuild anything, the generation doesn't move, so the next waiter still retries. Added tests that rotate the cert again while a caller waits on the lock, to confirm it skips without reading the disk.

[lqiu96]

ah, look at this and I think it has the same sync concern with multiple concurrent requests at once. I think we can reduce the need to read from disk by checking the generation.

[macastelaz]

Agreed, same fix applied here: refresh() snapshots the generation before taking refreshLock and returns if a concurrent refresh already swapped the transport, so queued callers no longer re-read the cert. Added a test for it.

[lqiu96]

(Gemini flagged)

This should be locked by the diskCheckLock as getOrUpdateDiskFingerprint can overwrite the lastDiskCheck.

--
I think this may end up with a race to update the lastDiskCheck. If markRefreshed and getOrUpdateDiskFingerprint, should we keep this lastDiskCheck from getOrUpdateDiskFingerprint?

[macastelaz]

Agreed. markRefreshed now updates the active fingerprint and clears lastDiskCheck while holding diskCheckLock, so an in-progress disk check can no longer write a stale result back after the switch. Previously that could make shouldRefresh() return a stale true for up to the 1s cache TTL. That only caused a no-op refresh(), since retries are decided by the generation. Keeping the result from getOrUpdateDiskFingerprint wouldn't be safe, since it can be the old certificate. Lock order is always the pool/refresh lock first, then diskCheckLock, so there's no deadlock. The trade-off is that markRefreshed may wait for one in-progress cert read, and refresh() already reads the cert under the pool lock anyway. Added a test.

[lqiu96]

(Gemini flagged)

Not this line, but gemini says that resize() should also check for isShutdown before resizing the channels

[macastelaz]

Good catch. A resize that was already waiting for the lock when shutdown() ran could still expand the pool afterwards and leak channels (cancel(true) doesn't interrupt a thread blocked on a monitor). resizeSafely now returns if the pool is shut down, matching refresh and the refill. Added a test.

[lqiu96]

Took another round of review on the PR. Changes look fine with me.

I think some small nits:

1. Let's see if we can update some of full qualified names to use the short name (org.mockito.Mockito.mock, java.util.concurrent.atomic.AtomicLong)
2. CertificateRotationTracker probably should have some tests if possible

[macastelaz]

Thanks! Replaced the fully qualified names added in this PR with imports (except com.google.auth.oauth2.EnvironmentProvider in CertificateBasedAccess, which clashes with the gax EnvironmentProvider already imported there), and added CertificateRotationTrackerTest covering the baseline read, rotation detection, the positive-only 1s cache and its expiry, concurrent callers sharing one disk read, recovery when the cert is unreadable at startup, readDiskFingerprint, markRefreshed (including the concurrent race), and isAlreadyActive.