From d7713654fc34670f7609a53a7e91a07e657215b3 Mon Sep 17 00:00:00 2001 From: Will Howes Date: Wed, 20 May 2026 18:08:29 +0000 Subject: [PATCH 1/5] fix(gax): adjust RetryAlgorithm logic for timeout v. exception --- .../com/google/api/gax/retrying/RetryAlgorithm.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java index 16d60d148d38..accb9fd0dc15 100644 --- a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java +++ b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java @@ -232,8 +232,15 @@ public boolean shouldRetry( ResponseT previousResponse, TimedAttemptSettings nextAttemptSettings) throws CancellationException { - return shouldRetryBasedOnResult(context, previousThrowable, previousResponse) - && shouldRetryBasedOnTiming(context, nextAttemptSettings); + boolean retryBasedOnResult = shouldRetryBasedOnResult(context, previousThrowable, previousResponse); + + // Short-circuit when operation has succeeded to avoid erroneously throwing CancellationException below + if (!retryBasedOnResult && previousThrowable == null) { + return false; + } + + boolean retryBasedOnTiming = shouldRetryBasedOnTiming(context, nextAttemptSettings); // throws CancellationException + return retryBasedOnResult && retryBasedOnTiming; } boolean shouldRetryBasedOnResult( From 901c2e72323a24e84cf3cbadf453f8f5ce00c2aa Mon Sep 17 00:00:00 2001 From: Will Howes Date: Wed, 20 May 2026 19:27:47 +0000 Subject: [PATCH 2/5] test(gax): add tests for RetryAlgorithm changes --- .../api/gax/retrying/RetryAlgorithmTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java index fb9e2f17f308..8eb0e9f39e15 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java @@ -31,6 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -201,4 +202,34 @@ void testShouldRetryWithContext_noPreviousSettings() { assertFalse(algorithm.shouldRetry(context, previousThrowable, previousResult, null)); } + + @Test + void testShouldRetry_prevThrowableNotNull_shouldRetryBasedOnResultFalse_callsTimedAlgorithm() { + ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); + TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); + RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); + Throwable previousThrowable = new Throwable(); + Object previousResult = new Object(); + TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); + when(resultAlgorithm.shouldRetry(previousThrowable, previousResult)).thenReturn(false); + + algorithm.shouldRetry(previousThrowable, previousResult, previousSettings); + + verify(timedAlgorithm).shouldRetry(previousSettings); + } + + @Test + void testShouldRetry_prevThrowableNull_shouldRetryBasedOnResultFalse_shortCircuits() { + ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); + TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); + RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); + Object previousResult = new Object(); + TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); + when(resultAlgorithm.shouldRetry(null, previousResult)).thenReturn(false); + + boolean shouldRetry = algorithm.shouldRetry(null, previousResult, previousSettings); + + assertFalse(shouldRetry); + verify(timedAlgorithm, never()).shouldRetry(previousSettings); + } } From aa95c0b3ca6070f199b1475c7cda923ee1a737f8 Mon Sep 17 00:00:00 2001 From: Will Howes Date: Wed, 20 May 2026 20:31:59 +0000 Subject: [PATCH 3/5] fix(gax): fix linter issues --- .../java/com/google/api/gax/retrying/RetryAlgorithm.java | 9 ++++++--- .../com/google/api/gax/retrying/RetryAlgorithmTest.java | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java index accb9fd0dc15..5fd3674f37c9 100644 --- a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java +++ b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java @@ -232,14 +232,17 @@ public boolean shouldRetry( ResponseT previousResponse, TimedAttemptSettings nextAttemptSettings) throws CancellationException { - boolean retryBasedOnResult = shouldRetryBasedOnResult(context, previousThrowable, previousResponse); + boolean retryBasedOnResult = + shouldRetryBasedOnResult(context, previousThrowable, previousResponse); - // Short-circuit when operation has succeeded to avoid erroneously throwing CancellationException below + // Short-circuit when operation has succeeded to avoid erroneously throwing + // CancellationException below if (!retryBasedOnResult && previousThrowable == null) { return false; } - boolean retryBasedOnTiming = shouldRetryBasedOnTiming(context, nextAttemptSettings); // throws CancellationException + // throws CancellationException + boolean retryBasedOnTiming = shouldRetryBasedOnTiming(context, nextAttemptSettings); return retryBasedOnResult && retryBasedOnTiming; } diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java index 8eb0e9f39e15..2fdef08eebab 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java @@ -204,7 +204,7 @@ void testShouldRetryWithContext_noPreviousSettings() { } @Test - void testShouldRetry_prevThrowableNotNull_shouldRetryBasedOnResultFalse_callsTimedAlgorithm() { + void testShouldRetry_resultFalseAndThrowableNotNull_callsTimed() { ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); @@ -219,7 +219,7 @@ void testShouldRetry_prevThrowableNotNull_shouldRetryBasedOnResultFalse_callsTim } @Test - void testShouldRetry_prevThrowableNull_shouldRetryBasedOnResultFalse_shortCircuits() { + void testShouldRetry_resultFalseAndThrowableNull_shortCircuits() { ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); From ddbfe1d7449f8ef3b7bcbeaeca75e7137afb96c9 Mon Sep 17 00:00:00 2001 From: Will Howes Date: Wed, 20 May 2026 20:36:30 +0000 Subject: [PATCH 4/5] fix(gax): add assertion to test --- .../java/com/google/api/gax/retrying/RetryAlgorithmTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java index 2fdef08eebab..2121bf816c84 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java @@ -213,8 +213,10 @@ void testShouldRetry_resultFalseAndThrowableNotNull_callsTimed() { TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); when(resultAlgorithm.shouldRetry(previousThrowable, previousResult)).thenReturn(false); - algorithm.shouldRetry(previousThrowable, previousResult, previousSettings); + boolean shouldRetry = + algorithm.shouldRetry(previousThrowable, previousResult, previousSettings); + assertFalse(shouldRetry); verify(timedAlgorithm).shouldRetry(previousSettings); } From 17c1ee42dce8018f425cc70cd311d1a55053f393 Mon Sep 17 00:00:00 2001 From: Will Howes Date: Wed, 20 May 2026 17:27:51 -0700 Subject: [PATCH 5/5] Defer to timing-related exception in createNextAttempt instead of shouldRetry --- .../api/gax/retrying/RetryAlgorithm.java | 42 ++++++------- .../api/gax/retrying/RetryAlgorithmTest.java | 59 +++++++++++++------ .../gax/rpc/StreamingRetryAlgorithmTest.java | 9 ++- 3 files changed, 68 insertions(+), 42 deletions(-) diff --git a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java index 5fd3674f37c9..6ebf0f9a7e75 100644 --- a/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java +++ b/sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java @@ -153,19 +153,25 @@ public TimedAttemptSettings createNextAttempt( Throwable previousThrowable, ResponseT previousResponse, TimedAttemptSettings previousSettings) { - // a small optimization that avoids calling relatively heavy methods - // like timedAlgorithm.createNextAttempt(), when it is not necessary. - if (!shouldRetryBasedOnResult(context, previousThrowable, previousResponse)) { - return null; + if (shouldRetryBasedOnResult(context, previousThrowable, previousResponse)) { + TimedAttemptSettings newSettings = + createNextAttemptBasedOnResult( + context, previousThrowable, previousResponse, previousSettings); + if (newSettings == null) { + newSettings = createNextAttemptBasedOnTiming(context, previousSettings); + } + return newSettings; } - - TimedAttemptSettings newSettings = - createNextAttemptBasedOnResult( - context, previousThrowable, previousResponse, previousSettings); - if (newSettings == null) { - newSettings = createNextAttemptBasedOnTiming(context, previousSettings); + // If we shouldn't retry based on result and the last attempt failed with an + // exception, defer to any exception thrown by shouldRetryBasedOnTiming. + // This lets a timeout-related exception get propagated when justified + // rather than e.g. exceptions caused by very short RPC deadlines on a + // final polling attempt. + if (previousThrowable != null) { + TimedAttemptSettings nextSettings = createNextAttemptBasedOnTiming(context, previousSettings); + shouldRetryBasedOnTiming(context, nextSettings); } - return newSettings; + return null; } private TimedAttemptSettings createNextAttemptBasedOnResult( @@ -232,18 +238,8 @@ public boolean shouldRetry( ResponseT previousResponse, TimedAttemptSettings nextAttemptSettings) throws CancellationException { - boolean retryBasedOnResult = - shouldRetryBasedOnResult(context, previousThrowable, previousResponse); - - // Short-circuit when operation has succeeded to avoid erroneously throwing - // CancellationException below - if (!retryBasedOnResult && previousThrowable == null) { - return false; - } - - // throws CancellationException - boolean retryBasedOnTiming = shouldRetryBasedOnTiming(context, nextAttemptSettings); - return retryBasedOnResult && retryBasedOnTiming; + return shouldRetryBasedOnResult(context, previousThrowable, previousResponse) + && shouldRetryBasedOnTiming(context, nextAttemptSettings); } boolean shouldRetryBasedOnResult( diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java index 2121bf816c84..62cdaf859730 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/retrying/RetryAlgorithmTest.java @@ -30,11 +30,14 @@ package com.google.api.gax.retrying; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.util.concurrent.CancellationException; import org.junit.jupiter.api.Test; @SuppressWarnings({"unchecked", "deprecation"}) @@ -114,6 +117,45 @@ void testNextAttemptWithContext() { verify(resultAlgorithm).shouldRetry(context, previousThrowable, previousResult); } + @Test + void testCreateNextAttempt_exceptionAndTimeout_defersToTimingException() { + ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); + TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); + RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); + Throwable throwable = new Throwable(); + Object result = new Object(); + TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); + TimedAttemptSettings nextSettings = mock(TimedAttemptSettings.class); + + when(resultAlgorithm.shouldRetry(throwable, result)).thenReturn(false); + when(timedAlgorithm.createNextAttempt(previousSettings)).thenReturn(nextSettings); + when(timedAlgorithm.shouldRetry(nextSettings)).thenThrow(new CancellationException()); + + assertThrows( + CancellationException.class, + () -> algorithm.createNextAttempt(throwable, result, previousSettings)); + } + + @Test + void testCreateNextAttempt_exceptionAndNoTimeout_returnsNull() { + ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); + TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); + RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); + Throwable throwable = new Throwable(); + Object result = new Object(); + TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); + TimedAttemptSettings nextSettings = mock(TimedAttemptSettings.class); + + when(resultAlgorithm.shouldRetry(throwable, result)).thenReturn(false); + when(timedAlgorithm.createNextAttempt(previousSettings)).thenReturn(nextSettings); + when(timedAlgorithm.shouldRetry(nextSettings)).thenReturn(true); + + TimedAttemptSettings nextAttemptSettings = + algorithm.createNextAttempt(throwable, result, previousSettings); + + assertNull(nextAttemptSettings); + } + @Test void testShouldRetry() { ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); @@ -203,23 +245,6 @@ void testShouldRetryWithContext_noPreviousSettings() { assertFalse(algorithm.shouldRetry(context, previousThrowable, previousResult, null)); } - @Test - void testShouldRetry_resultFalseAndThrowableNotNull_callsTimed() { - ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); - TimedRetryAlgorithm timedAlgorithm = mock(TimedRetryAlgorithm.class); - RetryAlgorithm algorithm = new RetryAlgorithm<>(resultAlgorithm, timedAlgorithm); - Throwable previousThrowable = new Throwable(); - Object previousResult = new Object(); - TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); - when(resultAlgorithm.shouldRetry(previousThrowable, previousResult)).thenReturn(false); - - boolean shouldRetry = - algorithm.shouldRetry(previousThrowable, previousResult, previousSettings); - - assertFalse(shouldRetry); - verify(timedAlgorithm).shouldRetry(previousSettings); - } - @Test void testShouldRetry_resultFalseAndThrowableNull_shortCircuits() { ResultRetryAlgorithm resultAlgorithm = mock(ResultRetryAlgorithm.class); diff --git a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/rpc/StreamingRetryAlgorithmTest.java b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/rpc/StreamingRetryAlgorithmTest.java index 066e7ab4df43..43778d52e8c8 100644 --- a/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/rpc/StreamingRetryAlgorithmTest.java +++ b/sdk-platform-java/gax-java/gax/src/test/java/com/google/api/gax/rpc/StreamingRetryAlgorithmTest.java @@ -116,12 +116,17 @@ void testNextAttemptReturnsNullWhenShouldNotRetry() { StreamingRetryAlgorithm algorithm = new StreamingRetryAlgorithm<>(resultAlgorithm, timedAlgorithm); + TimedAttemptSettings previousSettings = mock(TimedAttemptSettings.class); + when(previousSettings.getGlobalSettings()).thenReturn(DEFAULT_RETRY_SETTINGS); + when(previousSettings.getRpcTimeoutDuration()) + .thenReturn(DEFAULT_RETRY_SETTINGS.getInitialRpcTimeoutDuration()); + TimedAttemptSettings attempt = - algorithm.createNextAttempt(context, exception, null, mock(TimedAttemptSettings.class)); + algorithm.createNextAttempt(context, exception, null, previousSettings); assertThat(attempt).isNull(); TimedAttemptSettings attemptWithoutContext = - algorithm.createNextAttempt(exception, null, mock(TimedAttemptSettings.class)); + algorithm.createNextAttempt(exception, null, previousSettings); assertThat(attemptWithoutContext).isNull(); }