From e80013b6f9daa037b9b138399d477b34ff0a7e1e Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 10 Sep 2026 14:13:01 +0000 Subject: [PATCH 1/3] fix(bigtable): report the real error behind session-path UNKNOWNs DivertingUnaryCallable.translateException is the only place the session path converts a failure into the caller's exception, and it defaulted to UNKNOWN for anything that was not a StatusException/StatusRuntimeException. A production incident surfaced application-visible UNKNOWN errors with no matching UNKNOWN in CSM or on the server, and the bare status code left nothing to diagnose from. Three changes: - Walk the full cause chain when looking for a gRPC status. Unwrapping previously stopped at CompletionException/ExecutionException, so a valid StatusRuntimeException wrapped in any other type lost its code and was reported as UNKNOWN. Bounded at depth 32 with a self-cycle guard. - Map CancellationException to CANCELLED, matching csm.attributes.Util#extractStatus. The two mappings disagreed, so the same failure could be CANCELLED in metrics and UNKNOWN to the caller. - When the fall-through to UNKNOWN is genuine, name the throwable. The message now carries the cause-chain class names and the underlying message, and the first occurrence per callable is logged at WARNING with a full stack (FINE thereafter, since a storm is exactly when this fires most and an unconditional WARNING would flood the log). Adds DivertingUnaryCallableTest covering the status mapping, and SessionPathErrorEscapeTest, which injects faults at the seam between SessionPoolMap.apply and the session machinery to establish which throw sites escape synchronously (metrics.newTableTracer does) and which are converted to a status inside the op chain first (SessionPool.newCall is caught by RetryingVRpc.start and becomes CANCELLED). --- .../compat/ops/DivertingUnaryCallable.java | 95 ++++- .../api/SessionPathErrorEscapeTest.java | 266 ++++++++++++++ .../ops/DivertingUnaryCallableTest.java | 326 ++++++++++++++++++ 3 files changed, 680 insertions(+), 7 deletions(-) create mode 100644 java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java create mode 100644 java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java index 1097bf96379e..3317a081dbc7 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java @@ -33,13 +33,26 @@ import io.grpc.StatusException; import io.grpc.StatusRuntimeException; import java.time.Duration; +import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Level; +import java.util.logging.Logger; +import javax.annotation.Nullable; /** A callable to fork traffic between classic and session based operations. */ public class DivertingUnaryCallable extends UnaryCallable { + private static final Logger LOGGER = Logger.getLogger(DivertingUnaryCallable.class.getName()); + + /** Bounds the cause walk so a self-referential or pathologically deep chain can't spin. */ + private static final int MAX_CAUSE_DEPTH = 32; + + /** Gates the WARNING-level log for unrecognized throwables to the first occurrence. */ + private final AtomicBoolean loggedUnrecognized = new AtomicBoolean(); + private final ClientConfigurationManager configurationManager; private final UnaryCallable classic; @@ -121,16 +134,84 @@ ApiException translateException(Throwable e) { } } - Status.Code code = Status.Code.UNKNOWN; + Status.Code code = findStatusCode(cause); + if (code != null) { + return ApiExceptionFactory.createException( + cause.getMessage(), e, GrpcStatusCode.of(code), false); + } + + // Nothing in the chain carries a gRPC status, so the only honest answer is UNKNOWN. Name the + // throwable in the message rather than letting a bare "UNKNOWN" reach the caller: this is the + // client's last chance to say what actually failed, and everything downstream (CSM, the + // application's own error counters, the support case) sees only the code. + reportUnrecognized(cause); + return ApiExceptionFactory.createException( + describeUnrecognized(cause), e, GrpcStatusCode.of(Status.Code.UNKNOWN), false); + } - if (cause instanceof StatusRuntimeException) { - code = ((StatusRuntimeException) cause).getStatus().getCode(); + /** + * Returns the gRPC code for {@code t}, or null if nothing in its cause chain carries one. + * + *

Unlike a plain {@code instanceof} on the top-level throwable, this walks the whole chain: a + * perfectly good {@link StatusRuntimeException} wrapped in any type other than Completion/ + * ExecutionException would otherwise lose its code and be reported as UNKNOWN. Mirrors {@code + * csm.attributes.Util#extractStatus}, including its {@link CancellationException} case, so the + * status the application sees agrees with the one CSM records for the same failure. + */ + @Nullable + private static Status.Code findStatusCode(Throwable t) { + if (t instanceof CancellationException) { + return Status.Code.CANCELLED; } - if (cause instanceof StatusException) { - code = ((StatusException) cause).getStatus().getCode(); + Throwable current = t; + for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) { + if (current instanceof StatusRuntimeException) { + return ((StatusRuntimeException) current).getStatus().getCode(); + } + if (current instanceof StatusException) { + return ((StatusException) current).getStatus().getCode(); + } + Throwable next = current.getCause(); + if (next == current) { + break; // self-referential chain + } + current = next; } + return null; + } - return ApiExceptionFactory.createException( - cause.getMessage(), e, GrpcStatusCode.of(code), false); + /** Renders the cause chain as class names, so the message identifies the failure by itself. */ + private static String describeUnrecognized(Throwable cause) { + StringBuilder chain = new StringBuilder(); + Throwable current = cause; + for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) { + if (chain.length() > 0) { + chain.append(" <- "); + } + chain.append(current.getClass().getName()); + Throwable next = current.getCause(); + if (next == current) { + break; + } + current = next; + } + String message = cause.getMessage(); + return "Session operation failed with an error that carries no gRPC status; reporting UNKNOWN." + + " Cause chain: " + + chain + + (message != null ? ". Message: " + message : ""); + } + + /** + * Logs the first unrecognized throwable per callable at WARNING with a full stack, and the rest + * at FINE. A storm is exactly when this fires most, so an unconditional WARNING would flood the + * log at the moment the operator can least afford it. + */ + private void reportUnrecognized(Throwable cause) { + if (loggedUnrecognized.compareAndSet(false, true)) { + LOGGER.log(Level.WARNING, describeUnrecognized(cause), cause); + } else if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.log(Level.FINE, describeUnrecognized(cause), cause); + } } } diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java new file mode 100644 index 000000000000..1d0da8d315e5 --- /dev/null +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java @@ -0,0 +1,266 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.bigtable.data.v2.internal.api; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.bigtable.v2.CloseSessionRequest; +import com.google.bigtable.v2.SessionReadRowRequest; +import com.google.bigtable.v2.SessionReadRowResponse; +import com.google.bigtable.v2.VirtualRpcResponse; +import com.google.cloud.bigtable.data.v2.internal.csm.NoopMetrics; +import com.google.cloud.bigtable.data.v2.internal.csm.attributes.ClientInfo; +import com.google.cloud.bigtable.data.v2.internal.csm.tracers.VRpcTracer; +import com.google.cloud.bigtable.data.v2.internal.middleware.VRpc; +import com.google.cloud.bigtable.data.v2.internal.middleware.VRpc.VRpcResult; +import com.google.cloud.bigtable.data.v2.internal.session.BigtableTimer; +import com.google.cloud.bigtable.data.v2.internal.session.SessionPool; +import com.google.cloud.bigtable.data.v2.internal.session.SessionPoolInfo; +import com.google.cloud.bigtable.data.v2.internal.session.VRpcDescriptor; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.protobuf.Message; +import io.grpc.Deadline; +import io.grpc.Metadata; +import io.grpc.Status; +import java.time.Duration; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import javax.annotation.Nullable; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.mockito.Mockito; + +/** + * Fault injection at the seam between {@code SessionPoolMap.apply} and the session machinery. + * + *

Context: a production incident produced application-visible UNKNOWN errors with no matching + * UNKNOWN in CSM or on the server. An UNKNOWN requires a throwable with no grpc Status to reach + * {@code DivertingUnaryCallable.translateException}. That can only happen if the throw escapes + * {@link TableBase#readRow} synchronously, because everything inside the op chain is converted to a + * Status first. These tests establish which throw sites actually escape. + */ +@Timeout(30) +public class SessionPathErrorEscapeTest { + + private static final ClientInfo CLIENT_INFO = + ClientInfo.builder() + .setInstanceName( + InstanceName.builder() + .setProjectId("fake-project") + .setInstanceId("fake-instance") + .build()) + .setAppProfileId("default") + .build(); + + private final BigtableTimer mockTimer = Mockito.mock(BigtableTimer.class); + private final Deadline deadline = Deadline.after(1, TimeUnit.MINUTES); + + // ----------------------------------------------------------------------------------------- + // Escapes: reaches SessionPoolMap.apply's `catch (Throwable)` with no Status attached, and so + // becomes an application UNKNOWN. + // ----------------------------------------------------------------------------------------- + + @Test + public void tracerConstructionThrow_escapesReadRowSynchronously() { + // metrics.newTableTracer is called on the caller's thread in TableBase.readRow, outside any + // try/catch, before the op chain exists. MetricsImpl's implementation splits the method name + // and calls into a user-supplied ApiTracerFactory, so a throw here is reachable in production. + // Nothing downstream can convert it, so it propagates out of readRow. + CountingMetrics metrics = new CountingMetrics(); + metrics.throwOnNewTracer = new IllegalStateException("tracer factory blew up"); + TableBase table = newTable(new FakeSessionPool(), metrics); + UnaryResponseFuture listener = new UnaryResponseFuture<>(); + + IllegalStateException thrown = + assertThrows( + IllegalStateException.class, + () -> table.readRow(SessionReadRowRequest.getDefaultInstance(), listener, deadline)); + + assertThat(thrown).hasMessageThat().isEqualTo("tracer factory blew up"); + // The listener never hears about it -- the caller's future would hang if SessionPoolMap.apply + // did not convert the escaping throw into a failed future. + assertThat(listener.isDone()).isFalse(); + // And CSM has no record of the operation at all: it never started, so it never finished. + assertThat(metrics.operationsFinished.get()).isEqualTo(0); + } + + // ----------------------------------------------------------------------------------------- + // Does NOT escape: converted to a Status inside the op chain, so it lands in CSM with a real + // code and can never be the source of an application UNKNOWN. + // ----------------------------------------------------------------------------------------- + + @Test + public void sessionPoolNewCallThrow_isConvertedToCancelled() { + // A throw from SessionPool.newCall / PendingCall.start -- the shape SessionList raises on a + // close/drain race ("NEW session was closed", "double close") -- is caught by + // RetryingVRpc.start's try/catch and turned into Status.CANCELLED. It reaches the listener as + // a VRpcException, which IS a StatusRuntimeException, so translateException maps it cleanly. + // + // This rules the SessionList race out as a source of application UNKNOWN: it would show up as + // CANCELLED in both CSM and the application. + CountingMetrics metrics = new CountingMetrics(); + FakeSessionPool pool = new FakeSessionPool(); + pool.throwOnNewCall = new IllegalStateException("double close"); + TableBase table = newTable(pool, metrics); + UnaryResponseFuture listener = new UnaryResponseFuture<>(); + + table.readRow(SessionReadRowRequest.getDefaultInstance(), listener, deadline); + + assertThat(listener.isCompletedExceptionally()).isTrue(); + ExecutionException ee = + assertThrows(ExecutionException.class, () -> listener.get(5, TimeUnit.SECONDS)); + assertThat(ee).hasCauseThat().isInstanceOf(VRpcException.class); + VRpcException vrpc = (VRpcException) ee.getCause(); + assertThat(vrpc.getStatus().getCode()).isEqualTo(Status.Code.CANCELLED); + // The original throw survives as the cause, and the operation IS recorded in CSM. + assertThat(Status.fromThrowable(vrpc).getCause()).isInstanceOf(IllegalStateException.class); + assertThat(metrics.operationsFinished.get()).isEqualTo(1); + } + + // ----------------------------------------------------------------------------------------- + // The quietest failure mode: an application error that CSM records as a success. + // ----------------------------------------------------------------------------------------- + + @Test + public void okResultWithoutMessage_failsCallerButRecordsOkInCsm() { + // UnaryResponseFuture.onClose completes the caller exceptionally with a bare + // IllegalStateException when the vRPC closes OK but no message arrived. The VRpcResult status + // is OK, so the tracer records OK and the server saw a success -- yet the application gets an + // exception, and translateException has no Status to read, so it presents it as UNKNOWN. + // + // This is the only path found that yields application UNKNOWN with *no* error anywhere in CSM + // or on the server, which is the signature reported in production. + UnaryResponseFuture listener = new UnaryResponseFuture<>(); + VRpcResult okResult = VRpcResult.createServerOk(VirtualRpcResponse.getDefaultInstance()); + assertThat(okResult.getStatus().isOk()).isTrue(); + + listener.onClose(okResult); + + assertThat(listener.isCompletedExceptionally()).isTrue(); + ExecutionException ee = + assertThrows(ExecutionException.class, () -> listener.get(5, TimeUnit.SECONDS)); + assertThat(ee).hasCauseThat().isInstanceOf(IllegalStateException.class); + assertThat(ee).hasCauseThat().hasMessageThat().contains("missing result"); + // No grpc Status anywhere on it -- this is exactly the input that translateException defaults + // to UNKNOWN. See + // DivertingUnaryCallableTest#translateException_nonStatusThrowableBecomesUnknown. + assertThat(ee.getCause()).isNotInstanceOf(io.grpc.StatusRuntimeException.class); + } + + @Test + public void okResultWithMessage_completesNormally() { + // Control for the test above: the same OK result with a message delivered first succeeds. + UnaryResponseFuture listener = new UnaryResponseFuture<>(); + SessionReadRowResponse response = SessionReadRowResponse.getDefaultInstance(); + + listener.onMessage(response); + listener.onClose(VRpcResult.createServerOk(VirtualRpcResponse.getDefaultInstance())); + + assertThat(listener.isCompletedExceptionally()).isFalse(); + } + + // ----------------------------------------------------------------------------------------- + + private TableBase newTable(FakeSessionPool pool, CountingMetrics metrics) { + return new TableBase( + pool, + VRpcDescriptor.READ_ROW, + VRpcDescriptor.MUTATE_ROW, + metrics, + mockTimer, + MoreExecutors.directExecutor()); + } + + /** NoopMetrics that counts operation completions and can be told to throw on tracer creation. */ + private static final class CountingMetrics extends NoopMetrics { + final AtomicInteger operationsFinished = new AtomicInteger(); + @Nullable RuntimeException throwOnNewTracer; + + @Override + public VRpcTracer newTableTracer( + SessionPoolInfo poolInfo, VRpcDescriptor descriptor, Deadline deadline) { + if (throwOnNewTracer != null) { + throw throwOnNewTracer; + } + return new NoopVrpcTracer() { + @Override + public void onOperationFinish(VRpcResult result) { + operationsFinished.incrementAndGet(); + } + }; + } + } + + /** SessionPool whose newCall can be told to throw, simulating a close/drain race. */ + private static final class FakeSessionPool + implements SessionPool { + @Nullable RuntimeException throwOnNewCall; + + @Override + public void start(com.google.bigtable.v2.OpenTableRequest openReq, Metadata md) {} + + @Override + public void close(CloseSessionRequest req) {} + + @Override + public boolean awaitTerminated(Duration timeout) { + return true; + } + + @Override + public SessionPoolInfo getInfo() { + return SessionPoolInfo.create(CLIENT_INFO, VRpcDescriptor.TABLE_SESSION, "fake-pool"); + } + + @Override + public VRpc newCall( + VRpcDescriptor desc) { + if (throwOnNewCall != null) { + throw throwOnNewCall; + } + return new NeverCompletingVRpc<>(); + } + + @Override + public int getConsecutiveUnimplementedFailures() { + return 0; + } + + @Override + public boolean hasSession() { + return true; + } + } + + private static final class NeverCompletingVRpc implements VRpc { + @Override + public void start(Object req, VRpcCallContext ctx, VRpcListener listener) {} + + @Override + public void cancel(@Nullable String message, @Nullable Throwable cause) {} + + @Override + public boolean isDone() { + return false; + } + + @Override + public void requestNext() {} + } +} diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java new file mode 100644 index 000000000000..12b55bd922c4 --- /dev/null +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java @@ -0,0 +1,326 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.google.cloud.bigtable.data.v2.internal.compat.ops; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.api.core.ApiFuture; +import com.google.api.gax.grpc.GrpcCallContext; +import com.google.api.gax.grpc.GrpcStatusCode; +import com.google.api.gax.rpc.ApiCallContext; +import com.google.api.gax.rpc.ApiException; +import com.google.api.gax.rpc.UnaryCallable; +import com.google.bigtable.v2.ClientConfiguration; +import com.google.cloud.bigtable.data.v2.internal.util.ClientConfigurationManager; +import io.grpc.Deadline; +import io.grpc.Status; +import io.grpc.StatusException; +import io.grpc.StatusRuntimeException; +import java.io.Closeable; +import java.time.Duration; +import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +/** + * Pins the status mapping the session path presents to the application. + * + *

Context: a production incident showed application-visible UNKNOWN errors with no matching + * UNKNOWN anywhere in CSM or on the server. {@link DivertingUnaryCallable#translateException} is + * the only place the session path converts a failure into the caller's exception, and it defaults + * to UNKNOWN for anything that is not a {@link StatusException}/{@link StatusRuntimeException}. + * These tests establish which throwables take that default. + */ +class DivertingUnaryCallableTest { + + private static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(10); + + /** + * translateException reads no instance state, so a field-less instance is enough to exercise it + * directly. The end-to-end tests below build a fully wired callable instead. + */ + private final DivertingUnaryCallable bare = + new DivertingUnaryCallable<>(null, null, null, DEFAULT_TIMEOUT); + + private static Status.Code codeOf(ApiException e) { + return ((GrpcStatusCode) e.getStatusCode()).getTransportCode(); + } + + // --------------------------------------------------------------------------------------------- + // (1) The mechanism: which throwables become UNKNOWN. + // --------------------------------------------------------------------------------------------- + + @Test + void translateException_nonStatusThrowableBecomesUnknown() { + // IllegalStateException is the shape thrown by SessionList ("NEW session was closed", "double + // close"), DebugTagTracer, and UnaryResponseFuture's OK-without-message branch. None of them + // carry a grpc Status, so all of them arrive at the caller as UNKNOWN. + ApiException translated = bare.translateException(new IllegalStateException("double close")); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(translated).hasMessageThat().contains("double close"); + } + + @Test + void translateException_rejectedExecutionBecomesUnknown() { + // A saturated or shutting-down executor is the other realistic non-Status throwable on this + // path; SessionPoolMap's javadoc calls it out explicitly. + ApiException translated = + bare.translateException(new RejectedExecutionException("executor saturated")); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + } + + @Test + void translateException_retainsOriginalThrowableAsCause() { + // The original is preserved in the exception chain -- what the default loses is the *status* + // and any counter, not the throwable itself. Worth pinning so a future "just log the cause" + // fix isn't mistaken for a complete one. + IllegalStateException original = new IllegalStateException("double close"); + + ApiException translated = bare.translateException(original); + + assertThat(translated).hasCauseThat().isSameInstanceAs(original); + } + + // --------------------------------------------------------------------------------------------- + // Controls: the normal error path must keep its status, or every session failure would be + // UNKNOWN and the mapping above would be uninteresting. + // --------------------------------------------------------------------------------------------- + + @Test + void translateException_statusRuntimeExceptionKeepsItsCode() { + ApiException translated = + bare.translateException( + Status.DEADLINE_EXCEEDED.withDescription("too slow").asRuntimeException()); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.DEADLINE_EXCEEDED); + } + + @Test + void translateException_statusExceptionKeepsItsCode() { + ApiException translated = + bare.translateException(Status.UNAVAILABLE.withDescription("no session").asException()); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNAVAILABLE); + } + + @Test + void translateException_unwrapsCompletionAndExecutionException() { + // The async plumbing wraps failures in these two; both must be seen through. + ApiException viaCompletion = + bare.translateException(new CompletionException(Status.NOT_FOUND.asRuntimeException())); + ApiException viaExecution = + bare.translateException(new ExecutionException(Status.NOT_FOUND.asRuntimeException())); + ApiException viaNested = + bare.translateException( + new CompletionException(new ExecutionException(Status.NOT_FOUND.asRuntimeException()))); + + assertThat(codeOf(viaCompletion)).isEqualTo(Status.Code.NOT_FOUND); + assertThat(codeOf(viaExecution)).isEqualTo(Status.Code.NOT_FOUND); + assertThat(codeOf(viaNested)).isEqualTo(Status.Code.NOT_FOUND); + } + + @Test + void translateException_findsStatusDeepInCauseChain() { + // Regression guard for the original defect: unwrapping used to stop at Completion/ + // ExecutionException, so a perfectly good StatusRuntimeException wrapped in anything else was + // reported as UNKNOWN. The whole chain is walked now. + ApiException oneDeep = + bare.translateException( + new RuntimeException("wrapper", Status.DEADLINE_EXCEEDED.asRuntimeException())); + ApiException threeDeep = + bare.translateException( + new IllegalStateException( + "outer", + new RuntimeException( + "middle", + new IllegalArgumentException("inner", Status.ABORTED.asException())))); + + assertThat(codeOf(oneDeep)).isEqualTo(Status.Code.DEADLINE_EXCEEDED); + assertThat(codeOf(threeDeep)).isEqualTo(Status.Code.ABORTED); + } + + @Test + void translateException_cancellationExceptionBecomesCancelled() { + // csm.attributes.Util#extractStatus special-cases CancellationException. Before this fix the + // two mappings disagreed, so one failure could be CANCELLED in CSM and UNKNOWN to the caller. + ApiException translated = bare.translateException(new CancellationException("caller gave up")); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.CANCELLED); + } + + @Test + void translateException_toleratesSelfReferentialCauseChain() { + // A throwable that is its own cause must not spin the walk. + SelfCausedException looping = new SelfCausedException(); + + ApiException translated = bare.translateException(looping); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + } + + // --------------------------------------------------------------------------------------------- + // The point of the fix: an UNKNOWN must say what it actually was. + // --------------------------------------------------------------------------------------------- + + @Test + void translateException_unknownMessageNamesTheCauseChain() { + // This message is the whole diagnostic value of the change. Without it, an operator sees a + // bare UNKNOWN with no counterpart in CSM or on the server and has nothing to work from. + ApiException translated = + bare.translateException( + new IllegalStateException( + "Unary rpc completed OK but missing result", + new RejectedExecutionException("executor saturated"))); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(translated).hasMessageThat().contains("java.lang.IllegalStateException"); + assertThat(translated) + .hasMessageThat() + .contains("java.util.concurrent.RejectedExecutionException"); + assertThat(translated).hasMessageThat().contains("Unary rpc completed OK but missing result"); + } + + @Test + void translateException_unknownMessageSurvivesNullCauseMessage() { + // NullPointerException usually has no message; the chain must still identify it. + ApiException translated = bare.translateException(new NullPointerException()); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(translated).hasMessageThat().contains("java.lang.NullPointerException"); + } + + // --------------------------------------------------------------------------------------------- + // (2) End to end: a synchronous throw below the shim reaches the application as UNKNOWN. + // --------------------------------------------------------------------------------------------- + + @Test + void futureCall_shimFailureWithNonStatusThrowableSurfacesAsUnknown() { + DivertingUnaryCallable callable = + newCallable( + (request, deadline) -> { + CompletableFuture f = new CompletableFuture<>(); + f.completeExceptionally(new IllegalStateException("double close")); + return f; + }); + + ApiException surfaced = failureOf(callable.futureCall("req", GrpcCallContext.createDefault())); + + assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNKNOWN); + } + + @Test + void futureCall_sessionPoolMapSyncThrowSurfacesAsUnknown() { + // The full seam, wired as production wires it: TableBase.readRow throws synchronously on the + // caller thread -> SessionPoolMap.apply's `catch (Throwable)` converts it to a failed future + // -> translateException defaults it to UNKNOWN. No grpc Status is involved at any point, which + // is why this failure mode can produce an application UNKNOWN with no server-side counterpart. + SessionPoolMap poolMap = new SessionPoolMap<>(key -> new NoopHandle()); + DivertingUnaryCallable callable = + newCallable( + (request, deadline) -> + poolMap.apply( + "table", + handle -> { + throw new IllegalStateException("NEW session was closed"); + })); + + ApiException surfaced = failureOf(callable.futureCall("req", GrpcCallContext.createDefault())); + + assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNKNOWN); + assertThat(surfaced).hasCauseThat().isNotNull(); + } + + @Test + void futureCall_sessionPoolMapStatusThrowKeepsItsCode() { + // Same seam, but the throw already carries a Status. Contrast with the test above: the seam + // itself is not lossy -- the loss happens only when the throwable has no Status to begin with. + SessionPoolMap poolMap = new SessionPoolMap<>(key -> new NoopHandle()); + DivertingUnaryCallable callable = + newCallable( + (request, deadline) -> + poolMap.apply( + "table", + handle -> { + throw Status.UNAVAILABLE.withDescription("pool wedged").asRuntimeException(); + })); + + ApiException surfaced = failureOf(callable.futureCall("req", GrpcCallContext.createDefault())); + + assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNAVAILABLE); + } + + // --------------------------------------------------------------------------------------------- + + private static DivertingUnaryCallable newCallable(ShimFn shim) { + ClientConfiguration.Builder config = ClientConfiguration.newBuilder(); + config.getSessionConfigurationBuilder().setSessionLoad(1.0f); + + ClientConfigurationManager configManager = Mockito.mock(ClientConfigurationManager.class); + Mockito.when(configManager.getClientConfiguration()).thenReturn(config.build()); + + return new DivertingUnaryCallable<>(configManager, new FailingClassic(), shim, DEFAULT_TIMEOUT); + } + + /** Extracts the ApiException the application would observe from {@code future.get()}. */ + private static ApiException failureOf(ApiFuture future) { + ExecutionException ee = + assertThrows(ExecutionException.class, () -> future.get(5, TimeUnit.SECONDS)); + assertThat(ee).hasCauseThat().isInstanceOf(ApiException.class); + return (ApiException) ee.getCause(); + } + + /** sessionLoad is pinned to 1.0 in these tests, so the classic path must never be taken. */ + private static final class FailingClassic extends UnaryCallable { + @Override + public ApiFuture futureCall(String request, ApiCallContext context) { + throw new AssertionError("classic path taken despite sessionLoad=1.0"); + } + } + + private static final class NoopHandle implements Closeable { + @Override + public void close() {} + } + + /** Its own cause — exercises the cycle guard in the cause walk. */ + private static final class SelfCausedException extends RuntimeException { + @Override + public synchronized Throwable getCause() { + return this; + } + } + + /** + * UnaryShim extends Closeable, so it has two abstract methods and cannot be a lambda target on + * its own. Defaulting close() away makes call() the single abstract method. + */ + @FunctionalInterface + private interface ShimFn extends UnaryShim { + @Override + CompletableFuture call(String request, Deadline deadline); + + @Override + default void close() {} + } +} From 6f45d1319d15f910a48968f7700c9f31d92db4f5 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 10 Sep 2026 16:44:17 +0000 Subject: [PATCH 2/3] fix(bigtable): find wrapped cancellations, guard the null-cause message Review feedback on #14349: - findStatusCode checked CancellationException only at the top level, so a cancellation wrapped in anything other than Completion/ExecutionException fell through to UNKNOWN -- the same defect the chain walk exists to fix. Moved into the loop, after the Status checks so outermost still wins. - describeUnrecognized dereferenced a possibly-null cause. Unreachable today, but an NPE raised while building the error message would destroy exactly the diagnostic the message carries. --- .../compat/ops/DivertingUnaryCallable.java | 27 ++++++++---- .../ops/DivertingUnaryCallableTest.java | 42 +++++++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java index 3317a081dbc7..50f0c3a034b1 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java @@ -154,15 +154,16 @@ ApiException translateException(Throwable e) { * *

Unlike a plain {@code instanceof} on the top-level throwable, this walks the whole chain: a * perfectly good {@link StatusRuntimeException} wrapped in any type other than Completion/ - * ExecutionException would otherwise lose its code and be reported as UNKNOWN. Mirrors {@code - * csm.attributes.Util#extractStatus}, including its {@link CancellationException} case, so the - * status the application sees agrees with the one CSM records for the same failure. + * ExecutionException would otherwise lose its code and be reported as UNKNOWN. {@link + * CancellationException} is treated as CANCELLED, matching {@code + * csm.attributes.Util#extractStatus}, so the status the application sees agrees with the one CSM + * records for the same failure. It is checked at every level rather than only the top, since a + * wrapped cancellation reported as UNKNOWN is the same defect this method exists to fix; CSM only + * looks at the top level, so a nested cancellation is the one case where the two can still + * disagree, and it disagrees in the direction of the more specific code. */ @Nullable - private static Status.Code findStatusCode(Throwable t) { - if (t instanceof CancellationException) { - return Status.Code.CANCELLED; - } + private static Status.Code findStatusCode(@Nullable Throwable t) { Throwable current = t; for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) { if (current instanceof StatusRuntimeException) { @@ -171,6 +172,9 @@ private static Status.Code findStatusCode(Throwable t) { if (current instanceof StatusException) { return ((StatusException) current).getStatus().getCode(); } + if (current instanceof CancellationException) { + return Status.Code.CANCELLED; + } Throwable next = current.getCause(); if (next == current) { break; // self-referential chain @@ -181,7 +185,12 @@ private static Status.Code findStatusCode(Throwable t) { } /** Renders the cause chain as class names, so the message identifies the failure by itself. */ - private static String describeUnrecognized(Throwable cause) { + private static String describeUnrecognized(@Nullable Throwable cause) { + // No caller reaches here with null today, but this is the diagnostic path: an NPE thrown while + // building the error message would destroy exactly the information the message exists to carry. + if (cause == null) { + return "Session operation failed with a null error; reporting UNKNOWN."; + } StringBuilder chain = new StringBuilder(); Throwable current = cause; for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) { @@ -207,7 +216,7 @@ private static String describeUnrecognized(Throwable cause) { * at FINE. A storm is exactly when this fires most, so an unconditional WARNING would flood the * log at the moment the operator can least afford it. */ - private void reportUnrecognized(Throwable cause) { + private void reportUnrecognized(@Nullable Throwable cause) { if (loggedUnrecognized.compareAndSet(false, true)) { LOGGER.log(Level.WARNING, describeUnrecognized(cause), cause); } else if (LOGGER.isLoggable(Level.FINE)) { diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java index 12b55bd922c4..946d7aa3656b 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java @@ -169,6 +169,37 @@ void translateException_cancellationExceptionBecomesCancelled() { assertThat(codeOf(translated)).isEqualTo(Status.Code.CANCELLED); } + @Test + void translateException_findsCancellationDeepInCauseChain() { + // A cancellation wrapped in anything other than Completion/ExecutionException would otherwise + // fall through to UNKNOWN -- the same defect as a wrapped StatusRuntimeException. Note this is + // strictly more specific than csm.attributes.Util#extractStatus, which only checks the top + // level, so a nested cancellation is CANCELLED here and UNKNOWN in CSM. + ApiException translated = + bare.translateException( + new IllegalStateException("wrapper", new CancellationException("caller gave up"))); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.CANCELLED); + } + + @Test + void translateException_statusOutranksCancellationAtTheSameDepth() { + // A CancellationException wrapping a Status keeps CANCELLED -- outermost wins -- but a Status + // wrapping a cancellation keeps the Status. Pins the walk order, which is what decides this. + CancellationException outer = new CancellationException("caller gave up"); + outer.initCause(Status.DEADLINE_EXCEEDED.asRuntimeException()); + + ApiException cancellationOutside = bare.translateException(outer); + ApiException statusOutside = + bare.translateException( + Status.DEADLINE_EXCEEDED + .withCause(new CancellationException("caller gave up")) + .asRuntimeException()); + + assertThat(codeOf(cancellationOutside)).isEqualTo(Status.Code.CANCELLED); + assertThat(codeOf(statusOutside)).isEqualTo(Status.Code.DEADLINE_EXCEEDED); + } + @Test void translateException_toleratesSelfReferentialCauseChain() { // A throwable that is its own cause must not spin the walk. @@ -210,6 +241,17 @@ void translateException_unknownMessageSurvivesNullCauseMessage() { assertThat(translated).hasMessageThat().contains("java.lang.NullPointerException"); } + @Test + void translateException_nullThrowableStillProducesUnknown() { + // CompletableFuture#handle never hands us a null, so this is unreachable in production. It is + // pinned anyway because this is the diagnostic path: an NPE raised while *building* the error + // message would replace the failure the message exists to report. + ApiException translated = bare.translateException(null); + + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(translated).hasMessageThat().contains("null error"); + } + // --------------------------------------------------------------------------------------------- // (2) End to end: a synchronous throw below the shim reaches the application as UNKNOWN. // --------------------------------------------------------------------------------------------- From 0866ee0fd1d64356e08ab253c01123e5a9621d70 Mon Sep 17 00:00:00 2001 From: Mattie Fu Date: Thu, 10 Sep 2026 18:08:55 +0000 Subject: [PATCH 3/3] fix(bigtable): classify statusless session errors instead of UNKNOWN Review feedback on #14349: - IllegalStateException now maps to INTERNAL and RejectedExecutionException to RESOURCE_EXHAUSTED, rather than both falling through to UNKNOWN. UNKNOWN is left for types that genuinely say nothing. A carried Status still wins over an inferred code; within each category the outermost match wins. - MAX_CAUSE_DEPTH 32 -> 8. - Corrected the CSM claims in the comments. CSM takes its session-path status from VRpcResult, never from translateException's ApiException, so nothing here changes what CSM records -- and for the throwables that reach this code CSM has either no record of the operation or a recorded success. - Dropped tracerConstructionThrow_escapesReadRowSynchronously. - Removed section banners, gave every test a "// Verifies ..." lead, and dropped the references to production. - Documented that the SessionList race is injected rather than raced for, and that a missing row does not reach UnaryResponseFuture's OK-without-message branch (the server sends a response with `row` unset, which ReadRowShim turns into a null row), so throwing there is right. - sessionPoolNewCallThrow_isConvertedToCancelled now also asserts the status CSM recorded, not just that it recorded something. --- .../compat/ops/DivertingUnaryCallable.java | 103 +++++++++---- .../api/SessionPathErrorEscapeTest.java | 94 ++++-------- .../ops/DivertingUnaryCallableTest.java | 140 ++++++++++-------- 3 files changed, 178 insertions(+), 159 deletions(-) diff --git a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java index 50f0c3a034b1..b40093057e9a 100644 --- a/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java +++ b/java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallable.java @@ -37,6 +37,7 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; @@ -48,10 +49,10 @@ public class DivertingUnaryCallable extends UnaryCallableUnlike a plain {@code instanceof} on the top-level throwable, this walks the whole chain: a * perfectly good {@link StatusRuntimeException} wrapped in any type other than Completion/ - * ExecutionException would otherwise lose its code and be reported as UNKNOWN. {@link - * CancellationException} is treated as CANCELLED, matching {@code - * csm.attributes.Util#extractStatus}, so the status the application sees agrees with the one CSM - * records for the same failure. It is checked at every level rather than only the top, since a - * wrapped cancellation reported as UNKNOWN is the same defect this method exists to fix; CSM only - * looks at the top level, so a nested cancellation is the one case where the two can still - * disagree, and it disagrees in the direction of the more specific code. + * ExecutionException would otherwise lose its code and fall through to {@link + * #classifyStatuslessCause}, which cannot recover it. + * + *

{@link CancellationException} is treated as CANCELLED so that the two paths a caller can be + * routed down agree: the classic path reports it that way via {@code + * csm.attributes.Util#extractStatus}, and the same failure should not change code just because + * sessionLoad diverted the request. It is checked at every level rather than only the top, unlike + * {@code extractStatus}, because a wrapped cancellation losing its code is the same defect the + * chain walk exists to fix. */ @Nullable private static Status.Code findStatusCode(@Nullable Throwable t) { @@ -184,12 +190,48 @@ private static Status.Code findStatusCode(@Nullable Throwable t) { return null; } + /** + * Infers a code for a throwable whose chain carries no gRPC status, falling back to UNKNOWN. + * + *

UNKNOWN is the honest answer only when the type says nothing. For the two types this path + * actually sees it says plenty, so reporting UNKNOWN throws away a classification the caller can + * act on: + * + *

    + *
  • {@link IllegalStateException} means a client-side invariant was violated -- {@code + * SessionList}'s close/drain checks, and {@code UnaryResponseFuture}'s OK-without-message + * branch. That is a bug in the client, which is what INTERNAL means. + *
  • {@link RejectedExecutionException} means an executor refused the work, so the client is + * out of a resource it needs: RESOURCE_EXHAUSTED. + *
+ * + *

Walks the chain outermost-first, like {@link #findStatusCode}, but with lower precedence: a + * real status anywhere in the chain still wins over a type inferred here. + */ + private static Status.Code classifyStatuslessCause(@Nullable Throwable cause) { + Throwable current = cause; + for (int depth = 0; current != null && depth < MAX_CAUSE_DEPTH; depth++) { + if (current instanceof RejectedExecutionException) { + return Status.Code.RESOURCE_EXHAUSTED; + } + if (current instanceof IllegalStateException) { + return Status.Code.INTERNAL; + } + Throwable next = current.getCause(); + if (next == current) { + break; // self-referential chain + } + current = next; + } + return Status.Code.UNKNOWN; + } + /** Renders the cause chain as class names, so the message identifies the failure by itself. */ - private static String describeUnrecognized(@Nullable Throwable cause) { + private static String describeStatusless(@Nullable Throwable cause, Status.Code reported) { // No caller reaches here with null today, but this is the diagnostic path: an NPE thrown while // building the error message would destroy exactly the information the message exists to carry. if (cause == null) { - return "Session operation failed with a null error; reporting UNKNOWN."; + return "Session operation failed with a null error; reporting " + reported + "."; } StringBuilder chain = new StringBuilder(); Throwable current = cause; @@ -205,22 +247,23 @@ private static String describeUnrecognized(@Nullable Throwable cause) { current = next; } String message = cause.getMessage(); - return "Session operation failed with an error that carries no gRPC status; reporting UNKNOWN." - + " Cause chain: " + return "Session operation failed with an error that carries no gRPC status; reporting " + + reported + + ". Cause chain: " + chain + (message != null ? ". Message: " + message : ""); } /** - * Logs the first unrecognized throwable per callable at WARNING with a full stack, and the rest - * at FINE. A storm is exactly when this fires most, so an unconditional WARNING would flood the - * log at the moment the operator can least afford it. + * Logs the first statusless throwable per callable at WARNING with a full stack, and the rest at + * FINE. A storm is exactly when this fires most, so an unconditional WARNING would flood the log + * at the moment the operator can least afford it. */ - private void reportUnrecognized(@Nullable Throwable cause) { - if (loggedUnrecognized.compareAndSet(false, true)) { - LOGGER.log(Level.WARNING, describeUnrecognized(cause), cause); + private void reportStatusless(@Nullable Throwable cause, Status.Code reported) { + if (loggedStatusless.compareAndSet(false, true)) { + LOGGER.log(Level.WARNING, describeStatusless(cause, reported), cause); } else if (LOGGER.isLoggable(Level.FINE)) { - LOGGER.log(Level.FINE, describeUnrecognized(cause), cause); + LOGGER.log(Level.FINE, describeStatusless(cause, reported), cause); } } } diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java index 1d0da8d315e5..5e0bd54433b9 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/api/SessionPathErrorEscapeTest.java @@ -41,6 +41,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; @@ -49,11 +50,10 @@ /** * Fault injection at the seam between {@code SessionPoolMap.apply} and the session machinery. * - *

Context: a production incident produced application-visible UNKNOWN errors with no matching - * UNKNOWN in CSM or on the server. An UNKNOWN requires a throwable with no grpc Status to reach - * {@code DivertingUnaryCallable.translateException}. That can only happen if the throw escapes - * {@link TableBase#readRow} synchronously, because everything inside the op chain is converted to a - * Status first. These tests establish which throw sites actually escape. + *

An application-visible UNKNOWN requires a throwable with no grpc Status to reach {@code + * DivertingUnaryCallable.translateException}. That can only happen if the throw escapes {@link + * TableBase#readRow} synchronously, because everything inside the op chain is converted to a Status + * first. These tests establish which throw sites actually escape. */ @Timeout(30) public class SessionPathErrorEscapeTest { @@ -71,49 +71,17 @@ public class SessionPathErrorEscapeTest { private final BigtableTimer mockTimer = Mockito.mock(BigtableTimer.class); private final Deadline deadline = Deadline.after(1, TimeUnit.MINUTES); - // ----------------------------------------------------------------------------------------- - // Escapes: reaches SessionPoolMap.apply's `catch (Throwable)` with no Status attached, and so - // becomes an application UNKNOWN. - // ----------------------------------------------------------------------------------------- - - @Test - public void tracerConstructionThrow_escapesReadRowSynchronously() { - // metrics.newTableTracer is called on the caller's thread in TableBase.readRow, outside any - // try/catch, before the op chain exists. MetricsImpl's implementation splits the method name - // and calls into a user-supplied ApiTracerFactory, so a throw here is reachable in production. - // Nothing downstream can convert it, so it propagates out of readRow. - CountingMetrics metrics = new CountingMetrics(); - metrics.throwOnNewTracer = new IllegalStateException("tracer factory blew up"); - TableBase table = newTable(new FakeSessionPool(), metrics); - UnaryResponseFuture listener = new UnaryResponseFuture<>(); - - IllegalStateException thrown = - assertThrows( - IllegalStateException.class, - () -> table.readRow(SessionReadRowRequest.getDefaultInstance(), listener, deadline)); - - assertThat(thrown).hasMessageThat().isEqualTo("tracer factory blew up"); - // The listener never hears about it -- the caller's future would hang if SessionPoolMap.apply - // did not convert the escaping throw into a failed future. - assertThat(listener.isDone()).isFalse(); - // And CSM has no record of the operation at all: it never started, so it never finished. - assertThat(metrics.operationsFinished.get()).isEqualTo(0); - } - - // ----------------------------------------------------------------------------------------- - // Does NOT escape: converted to a Status inside the op chain, so it lands in CSM with a real - // code and can never be the source of an application UNKNOWN. - // ----------------------------------------------------------------------------------------- - @Test public void sessionPoolNewCallThrow_isConvertedToCancelled() { - // A throw from SessionPool.newCall / PendingCall.start -- the shape SessionList raises on a - // close/drain race ("NEW session was closed", "double close") -- is caught by - // RetryingVRpc.start's try/catch and turned into Status.CANCELLED. It reaches the listener as - // a VRpcException, which IS a StatusRuntimeException, so translateException maps it cleanly. + // Verifies that a throw out of SessionPool.newCall does not escape as a bare throwable: it is + // caught by RetryingVRpc.start and turned into CANCELLED, both for the caller and in CSM. // - // This rules the SessionList race out as a source of application UNKNOWN: it would show up as - // CANCELLED in both CSM and the application. + // The throw is injected directly rather than raced for -- FakeSessionPool.newCall throws the + // IllegalStateException that SessionList would raise on a real close/drain race ("NEW session + // was closed", "double close"). The test is about what RetryingVRpc does with such a throw, not + // about reproducing the interleaving that produces it, so injecting it keeps the test + // deterministic. It also means the SessionList race cannot be a source of an application + // UNKNOWN: whatever wins the race, the throw surfaces as CANCELLED in both places. CountingMetrics metrics = new CountingMetrics(); FakeSessionPool pool = new FakeSessionPool(); pool.throwOnNewCall = new IllegalStateException("double close"); @@ -128,24 +96,26 @@ public void sessionPoolNewCallThrow_isConvertedToCancelled() { assertThat(ee).hasCauseThat().isInstanceOf(VRpcException.class); VRpcException vrpc = (VRpcException) ee.getCause(); assertThat(vrpc.getStatus().getCode()).isEqualTo(Status.Code.CANCELLED); - // The original throw survives as the cause, and the operation IS recorded in CSM. + // The original throw survives as the cause, and CSM records the same CANCELLED the caller saw. assertThat(Status.fromThrowable(vrpc).getCause()).isInstanceOf(IllegalStateException.class); assertThat(metrics.operationsFinished.get()).isEqualTo(1); + assertThat(metrics.lastOperationStatus.get()).isEqualTo(Status.Code.CANCELLED); } - // ----------------------------------------------------------------------------------------- - // The quietest failure mode: an application error that CSM records as a success. - // ----------------------------------------------------------------------------------------- - @Test public void okResultWithoutMessage_failsCallerButRecordsOkInCsm() { + // Verifies the quietest failure mode: an application error that CSM records as a success. + // // UnaryResponseFuture.onClose completes the caller exceptionally with a bare // IllegalStateException when the vRPC closes OK but no message arrived. The VRpcResult status // is OK, so the tracer records OK and the server saw a success -- yet the application gets an - // exception, and translateException has no Status to read, so it presents it as UNKNOWN. + // exception, and translateException has no Status to read from it. // - // This is the only path found that yields application UNKNOWN with *no* error anywhere in CSM - // or on the server, which is the signature reported in production. + // A row that does not exist does NOT take this path: the server still sends one + // SessionReadRowResponse, with `row` unset, and ReadRowShim#buildRow turns that into a null + // row. So reaching here means the server closed OK without sending the message at all, which + // is a protocol violation rather than a normal not-found -- throwing is right, and the code + // below pins what the caller sees when it happens. UnaryResponseFuture listener = new UnaryResponseFuture<>(); VRpcResult okResult = VRpcResult.createServerOk(VirtualRpcResponse.getDefaultInstance()); assertThat(okResult.getStatus().isOk()).isTrue(); @@ -157,15 +127,15 @@ public void okResultWithoutMessage_failsCallerButRecordsOkInCsm() { assertThrows(ExecutionException.class, () -> listener.get(5, TimeUnit.SECONDS)); assertThat(ee).hasCauseThat().isInstanceOf(IllegalStateException.class); assertThat(ee).hasCauseThat().hasMessageThat().contains("missing result"); - // No grpc Status anywhere on it -- this is exactly the input that translateException defaults - // to UNKNOWN. See - // DivertingUnaryCallableTest#translateException_nonStatusThrowableBecomesUnknown. + // No grpc Status anywhere on it -- this is exactly the input translateException has to classify + // without help. See DivertingUnaryCallableTest#translateException_illegalStateBecomesInternal. assertThat(ee.getCause()).isNotInstanceOf(io.grpc.StatusRuntimeException.class); } @Test public void okResultWithMessage_completesNormally() { - // Control for the test above: the same OK result with a message delivered first succeeds. + // Verifies the control for the test above: the same OK result with a message delivered first + // completes the caller normally. UnaryResponseFuture listener = new UnaryResponseFuture<>(); SessionReadRowResponse response = SessionReadRowResponse.getDefaultInstance(); @@ -175,8 +145,6 @@ public void okResultWithMessage_completesNormally() { assertThat(listener.isCompletedExceptionally()).isFalse(); } - // ----------------------------------------------------------------------------------------- - private TableBase newTable(FakeSessionPool pool, CountingMetrics metrics) { return new TableBase( pool, @@ -187,21 +155,19 @@ private TableBase newTable(FakeSessionPool pool, CountingMetrics metrics) { MoreExecutors.directExecutor()); } - /** NoopMetrics that counts operation completions and can be told to throw on tracer creation. */ + /** NoopMetrics that records what the operation-level tracer was told at completion. */ private static final class CountingMetrics extends NoopMetrics { final AtomicInteger operationsFinished = new AtomicInteger(); - @Nullable RuntimeException throwOnNewTracer; + final AtomicReference lastOperationStatus = new AtomicReference<>(); @Override public VRpcTracer newTableTracer( SessionPoolInfo poolInfo, VRpcDescriptor descriptor, Deadline deadline) { - if (throwOnNewTracer != null) { - throw throwOnNewTracer; - } return new NoopVrpcTracer() { @Override public void onOperationFinish(VRpcResult result) { operationsFinished.incrementAndGet(); + lastOperationStatus.set(result.getStatus().getCode()); } }; } diff --git a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java index 946d7aa3656b..197b3ef3d95b 100644 --- a/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java +++ b/java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/internal/compat/ops/DivertingUnaryCallableTest.java @@ -44,11 +44,10 @@ /** * Pins the status mapping the session path presents to the application. * - *

Context: a production incident showed application-visible UNKNOWN errors with no matching - * UNKNOWN anywhere in CSM or on the server. {@link DivertingUnaryCallable#translateException} is - * the only place the session path converts a failure into the caller's exception, and it defaults - * to UNKNOWN for anything that is not a {@link StatusException}/{@link StatusRuntimeException}. - * These tests establish which throwables take that default. + *

{@link DivertingUnaryCallable#translateException} is the only place the session path converts + * a failure into the caller's exception. A throwable that carries a {@link StatusException}/{@link + * StatusRuntimeException} keeps its code; anything else has to be classified from its type, and + * only what cannot be classified is reported as UNKNOWN. These tests pin that mapping. */ class DivertingUnaryCallableTest { @@ -65,36 +64,58 @@ private static Status.Code codeOf(ApiException e) { return ((GrpcStatusCode) e.getStatusCode()).getTransportCode(); } - // --------------------------------------------------------------------------------------------- - // (1) The mechanism: which throwables become UNKNOWN. - // --------------------------------------------------------------------------------------------- - @Test - void translateException_nonStatusThrowableBecomesUnknown() { + void translateException_illegalStateBecomesInternal() { + // Verifies that a violated client-side invariant is reported as INTERNAL rather than UNKNOWN. // IllegalStateException is the shape thrown by SessionList ("NEW session was closed", "double // close"), DebugTagTracer, and UnaryResponseFuture's OK-without-message branch. None of them - // carry a grpc Status, so all of them arrive at the caller as UNKNOWN. + // carry a grpc Status, but all of them mean the same thing: a bug on the client side. ApiException translated = bare.translateException(new IllegalStateException("double close")); - assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(codeOf(translated)).isEqualTo(Status.Code.INTERNAL); assertThat(translated).hasMessageThat().contains("double close"); } @Test - void translateException_rejectedExecutionBecomesUnknown() { - // A saturated or shutting-down executor is the other realistic non-Status throwable on this - // path; SessionPoolMap's javadoc calls it out explicitly. + void translateException_rejectedExecutionBecomesResourceExhausted() { + // Verifies that an executor refusing work is reported as RESOURCE_EXHAUSTED. A saturated or + // shutting-down executor is the other realistic non-Status throwable on this path; + // SessionPoolMap's javadoc calls it out explicitly. ApiException translated = bare.translateException(new RejectedExecutionException("executor saturated")); + assertThat(codeOf(translated)).isEqualTo(Status.Code.RESOURCE_EXHAUSTED); + } + + @Test + void translateException_unclassifiableThrowableBecomesUnknown() { + // Verifies that UNKNOWN is now reserved for types that genuinely say nothing about the failure. + ApiException translated = bare.translateException(new RuntimeException("something else")); + assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); } + @Test + void translateException_classifiesOutermostRecognizedTypeFirst() { + // Verifies the precedence between two classifiable types in one chain: outermost wins, matching + // how a carried Status is found. + ApiException rejectedOutside = + bare.translateException( + new RejectedExecutionException( + "executor saturated", new IllegalStateException("double close"))); + ApiException illegalStateOutside = + bare.translateException( + new IllegalStateException( + "double close", new RejectedExecutionException("executor saturated"))); + + assertThat(codeOf(rejectedOutside)).isEqualTo(Status.Code.RESOURCE_EXHAUSTED); + assertThat(codeOf(illegalStateOutside)).isEqualTo(Status.Code.INTERNAL); + } + @Test void translateException_retainsOriginalThrowableAsCause() { - // The original is preserved in the exception chain -- what the default loses is the *status* - // and any counter, not the throwable itself. Worth pinning so a future "just log the cause" - // fix isn't mistaken for a complete one. + // Verifies the original throwable is preserved in the exception chain, not just described in + // the message. IllegalStateException original = new IllegalStateException("double close"); ApiException translated = bare.translateException(original); @@ -102,13 +123,9 @@ void translateException_retainsOriginalThrowableAsCause() { assertThat(translated).hasCauseThat().isSameInstanceAs(original); } - // --------------------------------------------------------------------------------------------- - // Controls: the normal error path must keep its status, or every session failure would be - // UNKNOWN and the mapping above would be uninteresting. - // --------------------------------------------------------------------------------------------- - @Test void translateException_statusRuntimeExceptionKeepsItsCode() { + // Verifies the normal error path is untouched: a carried Status keeps its code. ApiException translated = bare.translateException( Status.DEADLINE_EXCEEDED.withDescription("too slow").asRuntimeException()); @@ -118,6 +135,7 @@ void translateException_statusRuntimeExceptionKeepsItsCode() { @Test void translateException_statusExceptionKeepsItsCode() { + // Verifies the checked variant of the same, which arrives from a different grpc entry point. ApiException translated = bare.translateException(Status.UNAVAILABLE.withDescription("no session").asException()); @@ -126,7 +144,7 @@ void translateException_statusExceptionKeepsItsCode() { @Test void translateException_unwrapsCompletionAndExecutionException() { - // The async plumbing wraps failures in these two; both must be seen through. + // Verifies the two wrappers the async plumbing adds are both seen through, including nested. ApiException viaCompletion = bare.translateException(new CompletionException(Status.NOT_FOUND.asRuntimeException())); ApiException viaExecution = @@ -142,9 +160,8 @@ void translateException_unwrapsCompletionAndExecutionException() { @Test void translateException_findsStatusDeepInCauseChain() { - // Regression guard for the original defect: unwrapping used to stop at Completion/ - // ExecutionException, so a perfectly good StatusRuntimeException wrapped in anything else was - // reported as UNKNOWN. The whole chain is walked now. + // Verifies a Status is found at any depth. Unwrapping used to stop at Completion/ + // ExecutionException, so a StatusRuntimeException wrapped in anything else lost its code. ApiException oneDeep = bare.translateException( new RuntimeException("wrapper", Status.DEADLINE_EXCEEDED.asRuntimeException())); @@ -162,8 +179,9 @@ void translateException_findsStatusDeepInCauseChain() { @Test void translateException_cancellationExceptionBecomesCancelled() { - // csm.attributes.Util#extractStatus special-cases CancellationException. Before this fix the - // two mappings disagreed, so one failure could be CANCELLED in CSM and UNKNOWN to the caller. + // Verifies CancellationException maps to CANCELLED, the same code the classic path reports for + // it via csm.attributes.Util#extractStatus. The two used to disagree, so the same failure got a + // different code depending on whether sessionLoad happened to divert the request. ApiException translated = bare.translateException(new CancellationException("caller gave up")); assertThat(codeOf(translated)).isEqualTo(Status.Code.CANCELLED); @@ -171,10 +189,9 @@ void translateException_cancellationExceptionBecomesCancelled() { @Test void translateException_findsCancellationDeepInCauseChain() { - // A cancellation wrapped in anything other than Completion/ExecutionException would otherwise - // fall through to UNKNOWN -- the same defect as a wrapped StatusRuntimeException. Note this is - // strictly more specific than csm.attributes.Util#extractStatus, which only checks the top - // level, so a nested cancellation is CANCELLED here and UNKNOWN in CSM. + // Verifies a wrapped cancellation is still CANCELLED. This is stricter than + // csm.attributes.Util#extractStatus, which only checks the top level, so a nested cancellation + // is CANCELLED here and UNKNOWN on the classic path. ApiException translated = bare.translateException( new IllegalStateException("wrapper", new CancellationException("caller gave up"))); @@ -184,8 +201,7 @@ void translateException_findsCancellationDeepInCauseChain() { @Test void translateException_statusOutranksCancellationAtTheSameDepth() { - // A CancellationException wrapping a Status keeps CANCELLED -- outermost wins -- but a Status - // wrapping a cancellation keeps the Status. Pins the walk order, which is what decides this. + // Verifies the walk order between the two: outermost wins, whichever it is. CancellationException outer = new CancellationException("caller gave up"); outer.initCause(Status.DEADLINE_EXCEEDED.asRuntimeException()); @@ -202,7 +218,7 @@ void translateException_statusOutranksCancellationAtTheSameDepth() { @Test void translateException_toleratesSelfReferentialCauseChain() { - // A throwable that is its own cause must not spin the walk. + // Verifies a throwable that is its own cause does not spin the walk. SelfCausedException looping = new SelfCausedException(); ApiException translated = bare.translateException(looping); @@ -210,21 +226,18 @@ void translateException_toleratesSelfReferentialCauseChain() { assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); } - // --------------------------------------------------------------------------------------------- - // The point of the fix: an UNKNOWN must say what it actually was. - // --------------------------------------------------------------------------------------------- - @Test - void translateException_unknownMessageNamesTheCauseChain() { - // This message is the whole diagnostic value of the change. Without it, an operator sees a - // bare UNKNOWN with no counterpart in CSM or on the server and has nothing to work from. + void translateException_messageNamesTheCauseChain() { + // Verifies the message identifies the failure by itself. Without it the caller gets a code and + // nothing else, which is all that reaches CSM, their error counters, or a support case. ApiException translated = bare.translateException( new IllegalStateException( "Unary rpc completed OK but missing result", new RejectedExecutionException("executor saturated"))); - assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); + assertThat(codeOf(translated)).isEqualTo(Status.Code.INTERNAL); + assertThat(translated).hasMessageThat().contains("INTERNAL"); assertThat(translated).hasMessageThat().contains("java.lang.IllegalStateException"); assertThat(translated) .hasMessageThat() @@ -233,8 +246,9 @@ void translateException_unknownMessageNamesTheCauseChain() { } @Test - void translateException_unknownMessageSurvivesNullCauseMessage() { - // NullPointerException usually has no message; the chain must still identify it. + void translateException_messageSurvivesNullCauseMessage() { + // Verifies the chain still identifies the throwable when it has no message of its own, as a + // NullPointerException usually does not. ApiException translated = bare.translateException(new NullPointerException()); assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); @@ -243,21 +257,19 @@ void translateException_unknownMessageSurvivesNullCauseMessage() { @Test void translateException_nullThrowableStillProducesUnknown() { - // CompletableFuture#handle never hands us a null, so this is unreachable in production. It is - // pinned anyway because this is the diagnostic path: an NPE raised while *building* the error - // message would replace the failure the message exists to report. + // Verifies the diagnostic path itself cannot throw. CompletableFuture#handle never hands us a + // null, but an NPE raised while *building* the error message would replace the very failure + // the message exists to report. ApiException translated = bare.translateException(null); assertThat(codeOf(translated)).isEqualTo(Status.Code.UNKNOWN); assertThat(translated).hasMessageThat().contains("null error"); } - // --------------------------------------------------------------------------------------------- - // (2) End to end: a synchronous throw below the shim reaches the application as UNKNOWN. - // --------------------------------------------------------------------------------------------- - @Test - void futureCall_shimFailureWithNonStatusThrowableSurfacesAsUnknown() { + void futureCall_shimFailureWithNonStatusThrowableSurfacesAsInternal() { + // Verifies the mapping end to end: a shim future that fails with a statusless throwable reaches + // the application through the real futureCall plumbing, not just translateException. DivertingUnaryCallable callable = newCallable( (request, deadline) -> { @@ -268,15 +280,16 @@ void futureCall_shimFailureWithNonStatusThrowableSurfacesAsUnknown() { ApiException surfaced = failureOf(callable.futureCall("req", GrpcCallContext.createDefault())); - assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNKNOWN); + assertThat(codeOf(surfaced)).isEqualTo(Status.Code.INTERNAL); } @Test - void futureCall_sessionPoolMapSyncThrowSurfacesAsUnknown() { - // The full seam, wired as production wires it: TableBase.readRow throws synchronously on the - // caller thread -> SessionPoolMap.apply's `catch (Throwable)` converts it to a failed future - // -> translateException defaults it to UNKNOWN. No grpc Status is involved at any point, which - // is why this failure mode can produce an application UNKNOWN with no server-side counterpart. + void futureCall_sessionPoolMapSyncThrowSurfacesAsInternal() { + // Verifies the full seam: TableBase.readRow throws synchronously on the caller thread -> + // SessionPoolMap.apply's `catch (Throwable)` converts it to a failed future -> + // translateException + // classifies it. No grpc Status is involved at any point, so the code comes entirely from the + // throwable's type. SessionPoolMap poolMap = new SessionPoolMap<>(key -> new NoopHandle()); DivertingUnaryCallable callable = newCallable( @@ -289,14 +302,13 @@ void futureCall_sessionPoolMapSyncThrowSurfacesAsUnknown() { ApiException surfaced = failureOf(callable.futureCall("req", GrpcCallContext.createDefault())); - assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNKNOWN); + assertThat(codeOf(surfaced)).isEqualTo(Status.Code.INTERNAL); assertThat(surfaced).hasCauseThat().isNotNull(); } @Test void futureCall_sessionPoolMapStatusThrowKeepsItsCode() { - // Same seam, but the throw already carries a Status. Contrast with the test above: the seam - // itself is not lossy -- the loss happens only when the throwable has no Status to begin with. + // Verifies the same seam is not itself lossy: a throw that already carries a Status keeps it. SessionPoolMap poolMap = new SessionPoolMap<>(key -> new NoopHandle()); DivertingUnaryCallable callable = newCallable( @@ -312,8 +324,6 @@ void futureCall_sessionPoolMapStatusThrowKeepsItsCode() { assertThat(codeOf(surfaced)).isEqualTo(Status.Code.UNAVAILABLE); } - // --------------------------------------------------------------------------------------------- - private static DivertingUnaryCallable newCallable(ShimFn shim) { ClientConfiguration.Builder config = ClientConfiguration.newBuilder(); config.getSessionConfigurationBuilder().setSessionLoad(1.0f);