From 4402884122c54c468e76755607f2a3d6550200d8 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Fri, 13 Nov 2026 20:07:12 +0000 Subject: [PATCH 1/3] fix(bigtable): do not retry on negative RetryInfo delay in RetryingVRpc --- .../v2/internal/middleware/RetryingVRpc.java | 12 +++- .../internal/middleware/RetryingVRpcTest.java | 58 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java index 0a0007ffdd3b..af6ef8c4225a 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java @@ -249,6 +249,8 @@ public void onClose(VRpcResult result) { Scheduled scheduled = new Scheduled(retryDelay); onStateChange(scheduled); } else { + // Immediate retry for client-initiated retries or server RetryInfo with 0s + // delay (e.g. after GOAWAY). onStateChange(new Idle()); } return; @@ -297,9 +299,15 @@ boolean shouldRetry(VRpcResult result) { context.getOperationInfo().getDeadline().timeRemaining(TimeUnit.MILLISECONDS); // If the error has RetryInfo, it means it comes from the server and should - // be retried as long as there's time remaining after the backoff. + // be retried as long as the delay is non-negative and there's time remaining + // after the backoff. The server caps RetryInfo-directed attempts and may send + // a 0s delay (e.g. after GOAWAY) to request an immediate retry on another session. if (result.getRetryInfo() != null && result.getRetryInfo().hasRetryDelay()) { - long retryDelayMs = Durations.toMillis(result.getRetryInfo().getRetryDelay()); + Duration retryDelay = result.getRetryInfo().getRetryDelay(); + if (Durations.compare(retryDelay, Durations.ZERO) < 0) { + return false; + } + long retryDelayMs = Durations.toMillis(retryDelay); return timeRemainingMs - retryDelayMs > 1; } diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java index 25a1d8bfeb36..c6f71323e8aa 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java @@ -521,4 +521,62 @@ public void requestNext() {} assertThat(vRpcException.getStatus().getCode()).isEqualTo(Status.Code.UNAVAILABLE); assertThat(attempts.get()).isEqualTo(1); } + + @Test + void negativeRetryDelayNotRetried() { + AtomicInteger attempts = new AtomicInteger(); + ErrorResponse.Builder errorBuilder = + ErrorResponse.newBuilder() + .setRpcId(1) + .setRetryInfo(RetryInfo.newBuilder().setRetryDelay(Durations.fromSeconds(-10)).build()) + .setClusterInfo(ClusterInformation.getDefaultInstance()); + errorBuilder + .getStatusBuilder() + .setCode(Code.UNAVAILABLE.getNumber()) + .setMessage("server unavailable"); + VRpc.VRpcResult negativeDelayResult = VRpc.VRpcResult.createServerError(errorBuilder.build()); + + RetryingVRpc retrying = + new RetryingVRpc<>( + () -> + new VRpc() { + private boolean done; + + @Override + public void start( + SessionFakeScriptedRequest req, + VRpcCallContext ctx, + VRpcListener listener) { + attempts.incrementAndGet(); + done = true; + ctx.getExecutor().execute(() -> listener.onClose(negativeDelayResult)); + } + + @Override + public void cancel(String message, Throwable cause) {} + + @Override + public boolean isDone() { + return done; + } + + @Override + public void requestNext() {} + }, + timer); + + UnaryResponseFuture f = new UnaryResponseFuture<>(); + retrying.start( + SessionFakeScriptedRequest.getDefaultInstance(), + VRpc.VRpcCallContext.create(Deadline.after(0, TimeUnit.MILLISECONDS), true, tracer), + f); + + ExecutionException executionException = assertThrows(ExecutionException.class, f::get); + assertThat(executionException.getCause()).isInstanceOf(VRpcException.class); + VRpcException vRpcException = (VRpcException) executionException.getCause(); + assertThat(vRpcException.getStatus().getCode()).isEqualTo(Status.Code.UNAVAILABLE); + assertThat(vRpcException.getStatus().getDescription()).isEqualTo("server unavailable"); + assertThat(vRpcException.getResult().getState()).isEqualTo(VRpc.VRpcResult.State.SERVER_RESULT); + assertThat(attempts.get()).isEqualTo(1); + } } From 908cdd3f02603e0cfafd121651eda70efe8c1c89 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Fri, 13 Nov 2026 20:43:14 +0000 Subject: [PATCH 2/3] docs(bigtable): note that negative retry_delay should never happen --- .../bigtable/data/v2/internal/middleware/RetryingVRpc.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java index af6ef8c4225a..0deeadef573f 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpc.java @@ -304,6 +304,8 @@ boolean shouldRetry(VRpcResult result) { // a 0s delay (e.g. after GOAWAY) to request an immediate retry on another session. if (result.getRetryInfo() != null && result.getRetryInfo().hasRetryDelay()) { Duration retryDelay = result.getRetryInfo().getRetryDelay(); + // A negative retry_delay should never happen in practice, but guard against it so it + // cannot invert the remaining-deadline check below. if (Durations.compare(retryDelay, Durations.ZERO) < 0) { return false; } From d27fdffdb1c7d213e0fa393305f520beeaaa4719 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Fri, 13 Nov 2026 21:01:43 +0000 Subject: [PATCH 3/3] test(bigtable): use 1h deadline in negativeRetryDelayNotRetried --- .../bigtable/data/v2/internal/middleware/RetryingVRpcTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java index c6f71323e8aa..503c577e3870 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/middleware/RetryingVRpcTest.java @@ -568,7 +568,7 @@ public void requestNext() {} UnaryResponseFuture f = new UnaryResponseFuture<>(); retrying.start( SessionFakeScriptedRequest.getDefaultInstance(), - VRpc.VRpcCallContext.create(Deadline.after(0, TimeUnit.MILLISECONDS), true, tracer), + VRpc.VRpcCallContext.create(Deadline.after(1, TimeUnit.HOURS), true, tracer), f); ExecutionException executionException = assertThrows(ExecutionException.class, f::get);