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

[macastelaz]

Description

This PR introduces robust dynamic mTLS certificate rotation capabilities for
HTTP/JSON and gRPC transport channels, ensuring that certificates can be
rotated in long-lived environments without prematurely severing active, in-
flight RPCs or streams.

🚀 Core Features & Architectural Updates

• Dynamic Certificate Rotation: Implemented RefreshingHttpJsonChannel and
overhauled the gRPC ChannelPool to support dynamic, thread-safe, hot-swapping
of the underlying active transport channels whenever workload certificates
rotate dynamically on the filesystem.
• Preemptive Drop Mitigation: Refactored the internal channel rotation
pipeline (via refreshAll() and refreshSafely()) so that newly formed
connections are seamlessly brought online while preceding active streams are
cleanly drained and gracefully retired. This mitigates GFE connection drop
errors that previously occurred during hard resource refreshes.
• Core Retry Integration: Aligned streaming algorithm Callables and Retry
mechanisms with the dynamic refresh paradigm to ensure transparent retry
policies are respected, avoiding double-wrapped exceptions when traversing
rotated transports.

🔒 System Hardening & Bug Fixes

During the development of these features, several deep-dive reviews were
conducted over the GAX codebase, resulting in the following critical fixes:

• HTTP/JSON Teardown Thread-Safety: Fixed a race condition in
RefreshingHttpJsonChannel.java where shutdown() was calculating state
dynamically from underlying sub-channels without a lock. This allowed a
concurrent refresh() to spawn completely new channels after teardown began,
permanently leaking the channel pool.
• Outstanding RPC Memory Leak (ChannelPool.java): Fixed an uncontrolled
exception escape hatch in ReleasingClientCall.start(). If a pre-existing
cancellation exception was detected, the method aborted forcefully. This
bypassed onClose and never executed entry.release(), leaving the sub-channel
permanently trapped with an outstanding RPC count and preventing graceful
cleanup during rotations.
• Transport Channel Override Drops: Fixed merge() operations in
GrpcCallContext and HttpJsonCallContext that intentionally dropped custom
outer transportChannel references in favor of strict this.transportChannel
defaults. Context overrides now safely propagate custom overrides.
• Cross-Platform Compatibility: Fixed naively concatenated pathing for
certificates (Windows compatibility) and properly escaped JSON strings inside
CertificateBasedAccess.

⚠️ Behavioral & Security Boundary Changes

• mTLS Fail-Open Security Fix (CertificateBasedAccess.java):

  • Fix: If an environment strictly mandated mTLS but provided an invalid explicit config via GOOGLE_API_CERTIFICATE_CONFIG (e.g. typos, malformed
    JSON), the system previously swallowed the I/O exception, failed-open to
    a null filepath, and allowed a standard non-mTLS auth connection without
    notifying the developer. The system now correctly fails-closed (crashing
    startup by throwing an IllegalStateException) upon parsing failure,
    preventing unintentional security downgrade rollbacks.

• Infinity Timeout Boundary Enforcement (GrpcCallContext & HttpJsonCallContext):

  • Fix: Deadlines in GAX strictly prevent expansion (enforcing top-level
    user limits into downstream libraries). However, a logical flaw permitted
    bypassing this if a downstream caller submitted an unconstrained/infinite
    timeout limit (represented as null), quietly erasing strict prior
    deadlines. Override evaluations now properly reject null expansion
    boundaries.

🧪 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

• The following pastes include example test applications which use GAPIC client libraries and force a certificate refresh between calls. The pastes also include the logged application output showing successful detection and handling of the rotation event (including automatic retries).
  • gRPC: https://paste.googleplex.com/6340550226739200
  • Http/JSON: https://paste.googleplex.com/5043729177116672
  • Streaming: https://paste.googleplex.com/5449234957598720
• The following paste shows error handling when a cert rotation results in a corrupted certificate on disk - note the underlying java application does not crash and will resume successful calls after a valid cert is written to disk: https://paste.googleplex.com/5066238580621312
• The following paste shows disk protection through the use of a 1s TTL on the cached cert: https://paste.googleplex.com/6292261372559360

[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.

[nbayati]

I took a closer look at how GAX handles streaming retries and realized my earlier comment about not retrying streaming calls was too broad.

I think keeping isRetryable = false on BidiStreamingCallable and ClientStreamingCallable is the right call since bidi and client streams don't have a StreamResumptionStrategy in GAX and pass onError() directly to the caller.

But for ServerStreamingAttemptCallable, I think we should restore isRetryable = true on UnauthenticatedException (as you originally had in a2210c69a11) and add the attemptGeneration check from AttemptCallable. Because ServerStreamingAttemptCallable runs through StreamingRetryAlgorithm.shouldRetry(), GAX checks if (!attemptException.canResume()) return false; before evaluating isRetryable(). What do you think?

[nbayati]

In multi-channel pools (entries.size() > 1), if createSingleChannel() succeeds on at least one channel (anyCreated = true) but fails with an IOException on another, refreshAll() leaves the unrefreshed sub-channel holding the expired mTLS certificate in the pool and still returns true. Because refresh() and refreshSafely() immediately update activeCertFingerprint to the new disk certificate whenever refreshAll() returns true, shouldRefresh() evaluates to false on all subsequent checks. Any new RPCs routed to the unrefreshed sub-channel capture attemptGeneration == getGeneration() and fail with non-retryable UnauthenticatedException in AttemptCallable, leaving that fraction of pool traffic failing until the next certificate rotation on disk.

In addition, catching only IOException inside the loop risks leaking channels if channelFactory.createSingleChannel() throws an unchecked RuntimeException after earlier channels in newEntries have already been created—the loop aborts before entries.getAndSet(...) is reached, leaving those newly created ManagedChannel instances unclosed.

To allow partial progress while ensuring activeCertFingerprint is only updated once every sub-channel has rotated, we can track boolean allCreated = true;, catch Exception so already-created channels in newEntries are not leaked, and return allCreated.

Could we also add a unit test with ChannelPoolSettings.staticallySized(2) verifying that when the first channel succeeds and the second fails during refresh, shouldRefresh() remains true and a subsequent refresh() call completes rotation of the remaining channel?

[macastelaz]

Thanks for flagging this! In a multi-channel pool, updating activeCertFingerprint when only a subset of channels rotated would indeed leave unrotated sub-channels failing with non-retryable 401s until the next disk rotation.

Updated refreshAll() to:
1. Track both anyCreated (to commit partial progress to entries and increment generation) and allCreated (returned by refreshAll() so refresh() only updates activeCertFingerprint once all sub-channels have successfully rotated).
2. Catch Exception inside the creation loop so unchecked RuntimeExceptions do not abort the loop, and track newly created entries in a try/finally block so any Error thrown before entries.getAndSet(...) shuts down already-created channels instead of leaking them.
3. Added unit tests in ChannelPoolTest covering partial rotation recovery with staticallySized(2) and exception safety (RuntimeException / Error).

[nbayati]

Calling withChannel(null) clears this.channel to null but retains the existing transportChannel reference because of the newChannel == null check. If that context is later passed to defaultContext.merge(overrideContext), merge() falls back to the default context's channel (since overrideContext.channel is null) but keeps the override context's stale transportChannel (since overrideContext.transportChannel is non-null). As a result, RPCs execute over the default channel, while authentication failure recovery in AttemptCallable checks and refreshes the wrong TransportChannel.

To make sure transportChannel is cleared whenever the channel is cleared or replaced, we can tighten this check to (newChannel != null && newChannel.equals(channel)) ? transportChannel : null.

We can also add an assertion in GrpcCallContextTest.testWithChannelWithCustomChannelClearsTransportChannel verifying that baseContext.withChannel(null).getTransportChannel() returns null.

[macastelaz]

Agreed! Tightened the condition in GrpcCallContext.withChannel(...) to (newChannel != null && newChannel.equals(channel)) ? transportChannel : null so that clearing the channel via withChannel(null) also clears transportChannel and allows merge() to cleanly fall back to the default context's transportChannel.

Updated GrpcCallContextTest accordingly.

[nbayati]

Similar to GrpcCallContext.java:L666, calling withChannel(null) here clears this.channel to null while retaining the stale this.transportChannel reference, which causes merge() to pair the default HttpJsonChannel with a stale TransportChannel. We should update this condition to (newChannel != null && newChannel.equals(this.channel)) ? this.transportChannel : null, and update the corresponding unit test.

[macastelaz]

Fixed here as well—updated HttpJsonCallContext.withChannel(...) to (newChannel != null && newChannel.equals(channel)) ? transportChannel : null and updated HttpJsonCallContextTest to verify that withChannel(null) clears transportChannel so merge() falls back to the default context's transportChannel`. Updated the test too

[nbayati]

We might have an issue here. Caching lastDiskCheck unconditionally for 1 second across all threads can stall recovery or block rotation if a disk check happens right before or during a certificate update:

1. Because lastDiskCheck is shared across the client instance, if one thread hits a 401 and checks disk a few milliseconds before the platform finishes writing the new certificate, that stale or empty fingerprint is cached for a second. During that time, every concurrent thread hitting a 401 reads the cached value from memory, assumes the certificate on disk has not changed (shouldRefresh() == false), and fails its RPC without retrying.

2. Because refresh() also calls getOrUpdateDiskFingerprint() without clearing lastDiskCheck, a stale cache entry from one thread will cause a concurrent refresh() on another thread to either record the old fingerprint as activeCertFingerprint (in ChannelPool) or skip the channel swap altogether (in RefreshingHttpJsonChannel).

[macastelaz]

Caching negative results (when the cert on disk still matches activeCertFingerprint or is empty mid-write) defeats recovery for any requests failing slightly after a pre-rotation check, and reading from the cache inside refresh() risks committing a stale hash.

Updated both ChannelPool and RefreshingHttpJsonChannel (via the now shared CertificateRotationTracker helper) to:
1. Only cache positive rotation detections (!fingerprint.isEmpty() && !fingerprint.equalsIgnoreCase(activeCertFingerprint)) in lastDiskCheck. If the cert on disk hasn't changed yet or is mid-write, it is never cached in lastDiskCheck, allowing subsequent 401s milliseconds later to immediately detect the newly written cert while still coalescing disk I/O across concurrent threads once the new cert is present.
2. Read the disk fingerprint directly via readDiskFingerprint() (bypassing lastDiskCheck) inside refresh() / refreshSafely(), and clear lastDiskCheck = null upon updating activeCertFingerprint in markRefreshed(...).
3. Added unit tests in ChannelPoolTest and RefreshingHttpJsonChannelTest verifying immediate detection after a prior negative check within the 1-second window without manual cache invalidation.

[nbayati]

Previously, useMtlsClientCertificate() acted as a simple check for whether GOOGLE_API_USE_CLIENT_CERTIFICATE=true, telling GAX whether to attempt mTLS. This PR changes it to check getWorkloadCertPath(envProvider, propProvider) != null, which only looks for X.509 workload certificates (cert_configs.workload) on disk.

Because getWorkloadCertPath() returns null when no workload certificate file is present, useMtlsClientCertificate() now returns false for ECP setups and custom MtlsProvider instances, even when the user explicitly sets GOOGLE_API_USE_CLIENT_CERTIFICATE=true. As a result, GAX treats mTLS as disabled, skips ECP initialization (SecureConnectProvider), and silently downgrades requests to regular TLS.

[macastelaz]

Oops - thanks for catching this! I've updated MtlsUtils.useMtlsClientCertificate(...) to:

1. Return false if GOOGLE_API_USE_CLIENT_CERTIFICATE is "false".
2. Return true if getWorkloadCertPath(envProvider, propProvider) != null (which also preserves fail-closed validation for malformed config files).
3. Fall back to "true".equalsIgnoreCase(useClientCertificate) when no workload cert is present, restoring full ECP and custom MtlsProvider support.

Updated MtlsUtilsTest and CertificateBasedAccessTest accordingly.

[nbayati]

Like RefreshingHttpJsonChannel.java:L362, ReleasingClientCall doesn't synchronize start() and cancel(), which can permanently leak entry.outstandingRpcs or race with gRPC's async onClose() delivery. AffinityChannel.newCall() (line 759) is also missing a try/catch (Throwable t) around entry.channel.newCall(...), so the entry leaks if call creation throws.

We should apply the same fixes here so the entry is always released: synchronize start() and cancel() on a shared lock, mark cancellationException as volatile, wrap super.cancel() in try/finally, and catch Throwable in both start() and AffinityChannel.newCall().

[macastelaz]

Applied the exact same fixes to ChannelPool.ReleasingClientCall:

1. Synchronized start(...) and cancel(...) on a shared callLock and guarded against duplicate start(...) invocations.
2. Marked cancellationException as volatile and read/wrote it under callLock.
3. Wrapped super.cancel(message, cause) in try / finally so entry.release() always runs when cancelled prior to start(...).
4. Widened catch (Exception e) to catch (Throwable t) in start(...) so Error subclasses release entry. 5. Added corresponding unit and multithreaded stress tests in ChannelPoolTest.

[nbayati]

Acquiring the channel entry here increments outstandingRpcs before creating the underlying call (entry.channel.newCall(...)). If call creation throws a synchronous RuntimeException or Error, execution unwinds before ReleasingClientCall is constructed. This permanently leaks the reference count and prevents the underlying gRPC channel from ever shutting down during certificate rotation (refreshAll()) or pool shutdown.

We should wrap entry.channel.newCall(...) in a try / catch (Throwable t) block and call entry.release() before rethrowing so the reference count is always decremented on failure.

[macastelaz]

Good catch! Wrapped entry.channel.newCall(methodDescriptor, callOptions) in AffinityChannel.newCall(...) in a try / catch (Throwable t) block that calls entry.release() before rethrowing t.

Also added unit test coverage in ChannelPoolTest verifying that entry.outstandingRpcs is released when newCall(...) throws.

[nbayati]

Like ChannelPool.java:L759, catching only Exception here lets Error subclasses (e.g., AssertionError, LinkageError) thrown during newCall() escape without calling entry.release(). That permanently leaks entry.outstandingCalls and prevents retired HTTP/JSON channels from shutting down after rotation.

We should widen this to catch (Throwable t) so the entry is always released before rethrowing, and add a test case in RefreshingHttpJsonChannelTest.

[macastelaz]

Fixed! Widened catch (Exception e) to catch (Throwable t) in RefreshingHttpJsonChannel.newCall(...) so entry.release() is always invoked if an Error is thrown.

Also made sure to add unit test coverage in RefreshingHttpJsonChannelTest - newCall_throwsError_releasesEntry).

[nbayati]

I think retrieving workloadCertPath unconditionally enables certificate rotation whenever a certificate path is configured on disk, even when the channel does not actually use workload mTLS. When a caller supplies a custom httpTransport, channelFactory reuses that static instance without calling createHttpTransport(); if an HTTP 401 occurs after a cert update on disk, RefreshingHttpJsonChannel tears down and recreates the channel around the exact same transport instance (which cannot reload TLS certificates), triggering a pointless retry. Likewise, when this.mtlsProvider is null or certificateBasedAccess.useMtlsClientCertificate() is false, createHttpTransport() returns null and the channel uses plain non-mTLS transport, yet RefreshingHttpJsonChannel still monitors disk rotation and needlessly tears down and recreates the non-mTLS channel on 401 responses.

Also moving createHttpTransport() inside channelFactory.get() catches checked IOException and GeneralSecurityException and wraps them in a RuntimeException. During initial channel creation, that unchecked exception escapes createChannel() and breaks the declared throws IOException contract on getTransportChannel().

[macastelaz]

Fixed both issues:

1. Gated workloadCertPath resolution on this.httpTransport == null && this.mtlsProvider != null && certificateBasedAccess.useMtlsClientCertificate(), ensuring RefreshingHttpJsonChannel is only used when workload mTLS is actively managed by the channel provider.
2. Extracted createSingleManagedChannel() (which throws IOException("Failed to initialize mTLS HttpTransport") if createHttpTransport() returns null when mTLS is active) and invoked it directly during initial createChannel() before constructing RefreshingHttpJsonChannel(initialChannel, channelFactory, workloadCertPath) (with a try / catch (Throwable t) guard that shuts down initialChannel if wrapper construction fails), preserving the checked throws IOException contract on getTransportChannel().
3. In RefreshingHttpJsonChannel.refresh(), wrapped channelFactory.get() in a try / catch (Throwable t) block that logs a warning and returns early without updating activeEntry, activeCertFingerprint, or generation if channel creation fails during rotation.
4. Added unit tests in InstantiatingHttpJsonChannelProviderTest and RefreshingHttpJsonChannelTest covering custom HttpTransport, checked IOException propagation on startup failure, null keystore rejection, and mid-rotation creation failures.

[nbayati]

Similar to InstantiatingHttpJsonChannelProvider.java:L217, passing certificateBasedAccess.getWorkloadCertPath() enables rotation tracking unconditionally, even when DirectPath is enabled or mtlsProvider is null.

If a certificate read fails mid-write during rotation, createMtlsChannelCredentials() returns null and silently falls back to an unauthenticated channel via ManagedChannelBuilder.forAddress(serviceAddress, port). Because activeCertFingerprint still updates to the new hash, rotation gets stuck in this unauthenticated state and will not retry.

[macastelaz]

Great catch again!

1. Updated InstantiatingGrpcChannelProvider.createChannel() to only pass workloadCertPath to ChannelPool.create(...) when !this.canUseDirectPath() && this.mtlsProvider != null && certificateBasedAccess.useMtlsClientCertificate().
2. Updated createChannelBuilder() so that when mTLS is active (!this.canUseDirectPath() && this.mtlsProvider != null && certificateBasedAccess.useMtlsClientCertificate()) and createMtlsChannelCredentials()returnsnull, it throws new IOException("Failed to initialize mTLS channel credentials")instead of silently falling back toManagedChannelBuilder.forAddress(serviceAddress, port). During rotation, ChannelPool.refreshAll()catches theIOException, retains the existing authenticated channel, and leaves activeCertFingerprint` unchanged so rotation retries cleanly once the cert write finishes.
3. Added unit tests in InstantiatingGrpcChannelProviderTest verifying both behaviors (channelCreation_directPathOrNoMtls_doesNotEnableRotationTracking and createChannelBuilder_whenMtlsActiveAndCredentialsNull_throwsIOException)

[lqiu96]

nit: Can we enhance the javadocs a bit more explain the different possible outputs

From what I can see, there seem to be these three:

1. String -> Valid happy path with working, readable file
2. Exception -> Invalid State (non-readable or invalid file) and this isn't recoverable and will just fail
3. Null -> Othercase, but does this indicate that we can proceed (sort of like a fail open)?

[macastelaz]

Done - the javadoc now covers all three possible outcomes

[lqiu96]

qq, this seems to diverge from the above where we throw an IllegalStateException. Is there a reason for an exception here now?

[macastelaz]

Good question! The distinction comes down to configuration validation vs. best-effort digest calculation across both initialization and runtime:
- getWorkloadCertPath(...) enforces the mTLS configuration contract: if an explicit or default certificate_config.json is malformed or points to non-existent/unreadable paths, we fail closed (IllegalStateException) so we never silently downgrade an intended mTLS setup to regular TLS.
- getCertificateFingerprint(@Nullable String certPath) is a best-effort SHA-256 helper used both when initializing activeCertFingerprint and when polling during live RPCs (shouldRefresh() / refresh()). Because external certificate rotators may temporarily truncate or rewrite the file on disk either during startup or mid-RPC:
- At init time, returning null (normalized to activeCertFingerprint = "") allows initialization to proceed with an empty baseline so that as soon as the write completes on disk, !currentDiskFingerprint.equals("") triggers a refresh to load the valid cert.
- At runtime, returning null (normalized to "") allows shouldRefresh() / refresh() to short-circuit on currentDiskFingerprint.isEmpty() and keep using the active healthy channel until the file write completes.

[lqiu96]

qq, I think we would want to short-circuit on the workloadCertPath != null right? Rather than trying to refresh even when the path is null?

[macastelaz]

refreshSafely() is scheduled by ChannelPool when ChannelPoolSettings.isPreemptiveRefreshEnabled() is true (the periodic ~50-minute channel refresh to mitigate hourly GFE disconnects), which applies to all channels even when workloadCertPath == null. If we short-circuited when workloadCertPath == null, it would disable periodic preemptive channel refreshes for non-mTLS channels.

However:
1. Added Javadoc on refreshSafely() explaining that it handles periodic preemptive channel refreshes for all channels regardless of workloadCertPath.
2. Added a guard if (workloadCertPath != null && currentDiskFingerprint.isEmpty()) return; so that if workloadCertPath is configured and the certificate file is currently unreadable/mid-write on disk during a scheduled preemptive refresh, we skip calling refreshAll() rather than attempting to load a half-written cert.

[lqiu96]

qq, can you add a comment to explain the significance of 1s here?

[macastelaz]

Done! Extracted this into a named constant POSITIVE_ROTATION_CACHE_TTL_NANOS = TimeUnit.SECONDS.toNanos(1) in the shared CertificateRotationTracker class with Javadoc explaining its purpose.

[lqiu96]

It looks like the logic throughout the PR already checks the certPath and that the file is readable (asserted above in getWorkloadCertPath).

Would it be possible for this method to be simplified by just reading the bytes and doing the encoding?

[macastelaz]

Agreed! Simplified MtlsUtils.getCertificateFingerprint(...) by removing the redundant File.isFile() / File.canRead() checks and directly calling Files.readAllBytes(Paths.get(certPath)) and computing the SHA-256 hex digest inside the try / catch block.

[lqiu96]

What are the consequences of returning null here and how realistic would be for this file to not be able to be parsed?

The possible scenario I have in my head is this: activeFingerprint is say abc1234 and then the cert is rotated. Fingerprint is unable to be parsed and returns null and I believe will be converted to "". IIUC, that would be a fingerprint mismatch but not the intended effect (I could be wrong on this point).

[macastelaz]

That scenario is explicitly guarded against:

WhengetCertificateFingerprint(...) returns null (or when the file is temporarily 0 bytes mid-write), WorkloadCertificateUtils.getCertificateFingerprint(...) normalizes it to "". Both shouldRefresh() and refresh() explicitly check if(currentDiskFingerprint.isEmpty()) return false; (or return;) before evaluating !currentDiskFingerprint.equalsIgnoreCase(activeCertFingerprint).

As a result, an unreadable or empty mid-write file is never treated as a fingerprint mismatch—it safely short-circuits and keeps the existing active channel in use until the new certificate file finishes writing to disk. Added Javadoc to WorkloadCertificateUtils.getCertificateFingerprint(...) documenting this contract.

[macastelaz]

I took a closer look at how GAX handles streaming retries and realized my earlier comment about not retrying streaming calls was too broad.

I think keeping isRetryable = false on BidiStreamingCallable and ClientStreamingCallable is the right call since bidi and client streams don't have a StreamResumptionStrategy in GAX and pass onError() directly to the caller.

But for ServerStreamingAttemptCallable, I think we should restore isRetryable = true on UnauthenticatedException (as you originally had in a2210c69a11) and add the attemptGeneration check from AttemptCallable. Because ServerStreamingAttemptCallable runs through StreamingRetryAlgorithm.shouldRetry(), GAX checks if (!attemptException.canResume()) return false; before evaluating isRetryable(). What do you think?

There's a lot to unpack here but let me try my best to work through it:

In general, I agree that restoring generation-gated retries in ServerStreamingAttemptCallable fits cleanly into GAX's streaming retry pipeline for all the reasons you noted. Here's where my latest commit lands:

1. Bidi & Client Streaming (isRetryable = false): Kept isRetryable = false in BidiStreamingCallable and ClientStreamingCallable (while still triggering transportChannel.refresh() on UnauthenticatedException when shouldRefresh() is true), since neither uses BasicRetryingFuture or a StreamResumptionStrategy.
2. Generation Check in ServerStreamingAttemptCallable: Snapshotted attemptGeneration at the start of each attempt in ServerStreamingAttemptCallable.call(). In onErrorImpl(Throwable t), after refreshing the channel if shouldRefresh() is true, we check transportChannel.getGeneration() > attemptGeneration and only reconstruct UnauthenticatedException(..., isRetryable = true, ...) (re-wrapped in ServerStreamingAttemptException(newEx, attemptEx.canResume(), attemptEx.hasSeenResponses())) when the channel actually rotated.
3. End-to-End Interaction with StreamingRetryAlgorithm & ApiResultRetryAlgorithm:

• In StreamingRetryAlgorithm.shouldRetry(...), if(!attemptException.canResume()) return false; runs before super.shouldRetry(...):
  - For standard streams using SimpleStreamResumptionStrategy, an mTLS 401 before any messages are received (hasSeenResponses() == false) has canResume() == true and retries seamlessly on the rotated channel; if any messages were already delivered (hasSeenResponses() == true), canResume() is false so GAX will not replay already-emitted responses.
  - For streams with custom resumption strategies (resume tokens where canResume() == true), getResumeRequest(initialRequest) resumes from the last checkpoint on the rotated channel.
  - In StreamingRetryAlgorithm.createNextAttempt(...), unwrapping previousThrowable = previousThrowable.getCause() before super.createNextAttempt(...) ensures ApiResultRetryAlgorithm sees the unwrapped UnauthenticatedException(isRetryable = true) and grants the single zero-delay rotation retry even when UNAUTHENTICATED is not in context.getRetryableCodes() or maxAttempts == 1.

4. Tests: Added unit tests in ServerStreamingAttemptCallableTest covering both resumable streams when generation advances (testUnauthenticatedRefreshWithGenerationAdvanceRetries) and non-resumable streams where canResume() is false (testUnauthenticatedRefreshWithNonResumableStreamDoesNotRetry).

[lqiu96]

confirming my understanding: This check is needed ensure that we only do one mtls refresh per RPC? Otherwise we have this possibility of unlimited refreshes?

[macastelaz]

Yes, your understanding is right. There are two complementary guards that prevent unbounded retries:

1. At the attempt/transport layer (AttemptCallable / ServerStreamingAttemptCallable): each attempt snapshots attemptGeneration = channel.getGeneration() before issuing the RPC and only marks the UnauthenticatedException retryable if channel.getGeneration() > attemptGeneration (i.e., when the channel actually rotated during or after that attempt). If the retry on the rotated channel fails again with 401, channel.getGeneration() == attemptGeneration, so isRetryable is false.
2. At the retry budget layer (ApiResultRetryAlgorithm.createNextAttempt): because we keep .setAttemptCount(previousSettings.getAttemptCount()) unchanged while incrementing .setOverallAttemptCount(previousSettings.getOverallAttemptCount() + 1) so non-idempotent RPCs (maxAttempts = 1) can execute their rotation retry, checking previousSettings.getOverallAttemptCount() == previousSettings.getAttemptCount() guarantees this uncounted rotation retry is granted at most once per RPC (overallAttemptCount == attemptCount is true before the free retry and overallAttemptCount == attemptCount + 1 thereafter), preventing attemptCount from ever freezing at 0 in a loop.

[lqiu96]

I know we have some funky behavior with inheritance in Gax (primarily due to lots of legacy).

I believe we should aim to use the Retrying Context variant as much as possible. Is it possible to not override the non-RetryingContext variant for both shouldRetry and createNextAttempt?

[macastelaz]

Good question! I think unfortunately we need to override the non-RetryingContext variants because RetryAlgorithm.createNextAttemptBasedOnResult and RetryAlgorithm.shouldRetryBasedOnResult explicitly branch on context != null and call the non-RetryingContext methods directly whenever context == null:

• If we don't override the 3-arg createNextAttempt(Throwable, ResponseT, TimedAttemptSettings), calls with context == null fall through to the parent BasicResultRetryAlgorithm.createNextAttempt(...), which always returns null.
• If we don't override the 2-arg shouldRetry(Throwable, ResponseT) (which was pre-existing in ApiResultRetryAlgorithm), calls with context == null fall through to BasicResultRetryAlgorithm.shouldRetry(...), which returns previousThrowable != null (true for any exception, even non-ApiExceptions).

Also, shouldRetry(RetryingContext, Throwable, ResponseT) itself delegates to shouldRetry(previousThrowable, previousResponse) when context.getRetryableCodes() == null.

To make the RetryingContext variant primary as you suggested, I flipped the delegation so that the 4-arg createNextAttempt(@nullable RetryingContext context, ...) holds the implementation, and the 3-arg overload is just a 1-line delegate to createNextAttempt(null, previousThrowable, previousResponse, previousSettings).

LMK if I may be missing something here though

[lqiu96]

double check my understanding: Generation increment can occur even if some channels do not refresh (I believe the condition is that as long as one channel is been refreshed). I'm worried this may end up creating some mixed state of channel lifecycle. Since the retry logic is dependent on generation age compared against attempt count, I think it may end up resulting in requests retrying on old channels that haven't refreshed.

E.g. Channel A fails to refresh, but Channel B is able to refresh. Generation age increases which triggers a request to refresh. But the request refreshing on Channel A would continue to fail as it doesn't have the new cert.

[macastelaz]

Your understanding is 100% right: because generation.incrementAndGet() currently runs inside if(anyCreated), a partial refresh (e.g., Channel B succeeds while Channel A fails) will increment generation, and if a retry round-robins onto the unrefreshed Channel A during that window, that retry can fail and consume that RPC's single rotation retry.

When K < N channels succeed in recreating during a pool refresh, each possible design has a distinct tradeoff:

• Option 1: Current behavior (if (anyCreated) swap + return allCreated)
• Behavior: Swaps in the K new channels, leaves the N - K old channels in place, increments generation, and returns false so shouldRefresh() stays true.
• Tradeoff: Preserves partial progress, keeps load evenly spread across all N pool slots, and self-heals on subsequent calls—at the cost that a retry can round-robin onto one of the N - K unrefreshed channels during that window.
• Option 2: All-or-nothing swap (if (allCreated) swap only)
• Behavior: Discards all K newly created channels and keeps all N old channels until 100% of channels in the pool succeed in recreating.
• Tradeoff: Never creates a mixed-generation pool (when generation increments, 100% of channels have the new cert)—at the cost of throwing away healthy new channels and keeping 100% of traffic on the old/expired cert if even 1 of N channels fails (e.g. a transient priming blip).
• Option 3: Per-entry generation routing
• Behavior: Tracks generation per Entry and routes traffic only to entries at the highest generation.
• Tradeoff: 0% stale-cert errors immediately without discarding healthy new channels—at the cost of concentrating 100% of pool traffic onto K channels (hotspotting / stream saturation if K ≪ N).

We currently have Option 1 implemented per @nbayati's earlier comment (#13995 (comment)), but if you and @nbayati prefer switching to Option 2 (all-or-nothing swap), we can make that update.

Thoughts?

[lqiu96]

Hmm, here are my initial thoughts (open to more discussion). I think Option 2 is the most breaking as that ends up potentially all requests failing if any channel fails to create. I'll need to look at the code again as I do wonder what the odds that we cannot create a gRPC channel.

I think this primarily will depend on how we feel about potential requests resulting in a non-retriable 401 issue for users. I think it would make more sense for this not show up for the users at all and I think that Option 1 leaves some possibility where requests are routed to a channel in the unrefreshed channel.

Looking at ChannelPool, I see that the refreshInterval is every minute so theoretically we should be able to scale back up to match the demand over time (I believe we normally cap the amount of growth per resize).

Tradeoff being that until the resize occurs, we will be concentrating high traffic requests into a potentially smaller pool of channels (and given a hard limit of 100 concurrent requests per channel, we may be surfacing a different type of issue to them: client side request queuing).

I think I'll need a deeper look at this, but my current leaning is for option #3

[macastelaz]

I agree that brief client-side queuing / higher concurrency on the healthy channels is much better than surfacing an unrecoverable 401 back to the caller, and that Option 2 is too risky if a single channel fails to create.

One nice detail about your observation on resize() and pool recovery under Option 3:

• If on a partial refresh (anyCreated == true) we populate entries with only the K newly created channels (and shut down all old channels), then:

1. getEntry(affinity) (Math.abs(affinity % localEntries.size())) stays O(1) on the hot path with zero stale-channel routing because entries only contains refreshed channels.
2. Dynamic resize() sees localEntries.size() == K and will naturally expand the pool back up on the next resize interval.
3. For fixed-size pools (where dynamic resize() isn't active), we can preserve the target channel count (N) across partial refreshes so that the next healing refreshAll() pass restores all N channels instead of stopping at K.

@nbayati — curious to get your thoughts on this as well! If both of you are good with Option 3, I'm happy to update ChannelPool.refreshAll() and the corresponding tests accordingly.