addressing Driver team review comments

sfc-gh-dszmolka · sfc-gh-dszmolka · commit 69b2fa5e5363 · 2026-07-07T12:32:18.000+02:00
diff --git a/src/main/java/net/snowflake/client/internal/jdbc/SnowflakeUtil.java b/src/main/java/net/snowflake/client/internal/jdbc/SnowflakeUtil.java
@@ -649,6 +649,22 @@ public static Throwable getRootCause(Exception ex) {
     return cause;
   }
 
+  /**
+   * Walks the exception cause chain and returns the first {@link Throwable} that is an instance of
+   * {@code type}, or {@code null} if none is found. Unlike {@link #getRootCause(Exception)}, which
+   * always returns the deepest cause, this method stops at the first match of the requested type.
+   */
+  public static <T extends Throwable> T findFirstCauseOfType(Throwable ex, Class<T> type) {
+    Throwable cause = ex;
+    while (cause != null) {
+      if (type.isInstance(cause)) {
+        return type.cast(cause);
+      }
+      cause = cause.getCause();
+    }
+    return null;
+  }
+
   public static boolean isBlank(String input) {
     if ("".equals(input) || input == null) {
       return true;
diff --git a/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/GCSAccessStrategyAwsSdk.java b/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/GCSAccessStrategyAwsSdk.java
@@ -9,7 +9,6 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.time.Duration;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -34,7 +33,6 @@
 import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption;
 import software.amazon.awssdk.core.exception.SdkException;
 import software.amazon.awssdk.core.retry.RetryPolicy;
-import software.amazon.awssdk.core.retry.backoff.FullJitterBackoffStrategy;
 import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient;
 import software.amazon.awssdk.http.nio.netty.ProxyConfiguration;
 import software.amazon.awssdk.regions.Region;
@@ -57,13 +55,7 @@ class GCSAccessStrategyAwsSdk implements GCSAccessStrategy {
   private static final SFLogger logger = SFLoggerFactory.getLogger(GCSAccessStrategyAwsSdk.class);
   private final S3AsyncClient amazonClient;
 
-  GCSAccessStrategyAwsSdk(
-      StageInfo stage,
-      SFBaseSession session,
-      int maxRetries,
-      int retryBackoffMin,
-      int retryBackoffMaxExponent)
-      throws SnowflakeSQLException {
+  GCSAccessStrategyAwsSdk(StageInfo stage, SFBaseSession session) throws SnowflakeSQLException {
     String accessToken = (String) stage.getCredentials().get("GCS_ACCESS_TOKEN");
 
     Optional<String> oEndpoint = stage.gcsCustomEndpoint();
@@ -94,20 +86,12 @@ class GCSAccessStrategyAwsSdk implements GCSAccessStrategy {
     overrideConfiguration.putAdvancedOption(
         SdkAdvancedClientOption.SIGNER, new AwsSdkGCPSigner(accessToken));
 
-    // Mirror the JDBC driver's retry settings so putGetMaxRetries and backoff parameters
-    // apply consistently to this strategy. maxBackoffMs = retryBackoffMin <<
-    // retryBackoffMaxExponent
-    // (e.g. 1000ms << 4 = 16 000ms), matching S3ErrorHandler.retryRequestWithExponentialBackoff.
-    long maxBackoffMs = (long) retryBackoffMin << retryBackoffMaxExponent;
+    // Disable the SDK's own internal retries so the outer JDBC retry loop in
+    // SnowflakeGCSClient.upload() is the sole controller of retry count and backoff.
+    // Starting from the default policy (rather than an empty builder) preserves the AWS
+    // token-bucket circuit breaker and throttling-aware retry conditions.
     overrideConfiguration.retryPolicy(
-        RetryPolicy.builder()
-            .numRetries(maxRetries)
-            .backoffStrategy(
-                FullJitterBackoffStrategy.builder()
-                    .baseDelay(Duration.ofMillis(retryBackoffMin))
-                    .maxBackoffTime(Duration.ofMillis(maxBackoffMs))
-                    .build())
-            .build());
+        RetryPolicy.defaultRetryPolicy().toBuilder().numRetries(0).build());
 
     ProxyConfiguration proxyConfiguration;
     if (session != null) {
@@ -285,17 +269,31 @@ public boolean handleStorageException(
       String queryId,
       SnowflakeGCSClient gcsClient)
       throws SnowflakeSQLException {
-    Throwable cause = ex.getCause();
-    if (cause instanceof SdkException) {
-      logger.debug("GCSAccessStrategyAwsSdk: " + cause.getMessage());
+    // Walk the full cause chain to find an SdkException. The exception may arrive wrapped
+    // (e.g. inside SnowflakeSQLLoggedException → CompletionException) when it originates from the
+    // inner uploadWithDownScopedToken wrapper. Walking the chain ensures that transient errors
+    // (503, connection refused, etc.) are always treated as retryable regardless of wrapping depth,
+    // consistent with how GCSDefaultAccessStrategy handles StorageException.
+    SdkException sdkEx = SnowflakeUtil.findFirstCauseOfType(ex, SdkException.class);
+    if (sdkEx != null) {
+      if (sdkEx != ex) {
+        logger.debug(
+            "GCSAccessStrategyAwsSdk: found SdkException ({}) wrapped inside {} during {};"
+                + " treating as retryable",
+            sdkEx.getMessage(),
+            ex.getClass().getSimpleName(),
+            operation);
+      } else {
+        logger.debug("GCSAccessStrategyAwsSdk: {}", sdkEx.getMessage());
+      }
 
       if (retryCount > gcsClient.getMaxRetries()
-          || S3ErrorHandler.isClientException400Or404(cause)) {
+          || S3ErrorHandler.isClientException400Or404(sdkEx)) {
         throwIfClientExceptionOrMaxRetryReached(
-            operation, session, command, queryId, gcsClient, cause);
+            operation, session, command, queryId, gcsClient, sdkEx);
       } else {
         retryRequestWithExponentialBackoff(
-            ex, retryCount, operation, session, command, gcsClient, queryId, cause);
+            ex, retryCount, operation, session, command, gcsClient, queryId, sdkEx);
       }
       return true;
     } else {
diff --git a/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/GCSDefaultAccessStrategy.java b/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/GCSDefaultAccessStrategy.java
@@ -18,6 +18,7 @@
 import com.google.cloud.storage.StorageOptions;
 import java.io.File;
 import java.io.InputStream;
+import java.net.SocketTimeoutException;
 import java.nio.channels.Channels;
 import java.util.Map;
 import net.snowflake.client.api.exception.SnowflakeSQLException;
@@ -29,6 +30,7 @@
 import net.snowflake.client.internal.log.SFLoggerFactory;
 import net.snowflake.client.internal.util.SFPair;
 import net.snowflake.common.core.SqlState;
+import org.apache.http.HttpStatus;
 
 class GCSDefaultAccessStrategy implements GCSAccessStrategy {
   private static final SFLogger logger = SFLoggerFactory.getLogger(GCSDefaultAccessStrategy.class);
@@ -209,32 +211,55 @@ public boolean handleStorageException(
     // re-wraps it before re-throwing. Walking the chain ensures that a transient 503 or similar
     // GCS error is always treated as retryable regardless of how many layers of wrapping were
     // added.
-    StorageException se = null;
-    if (ex instanceof StorageException) {
-      se = (StorageException) ex;
-    } else {
-      Throwable cause = ex.getCause();
-      while (cause != null) {
-        if (cause instanceof StorageException) {
-          se = (StorageException) cause;
-          break;
-        }
-        cause = cause.getCause();
-      }
-      if (se != null) {
-        logger.debug(
-            "GCSDefaultAccessStrategy: found StorageException (HTTP {}) wrapped inside {} during {};"
-                + " treating as retryable",
-            se.getCode(),
-            ex.getClass().getSimpleName(),
-            operation);
-      }
+    StorageException se = SnowflakeUtil.findFirstCauseOfType(ex, StorageException.class);
+    if (se != null && se != ex) {
+      logger.debug(
+          "GCSDefaultAccessStrategy: found StorageException (HTTP {}) wrapped inside {} during {};"
+              + " treating as retryable",
+          se.getCode(),
+          ex.getClass().getSimpleName(),
+          operation);
     }
 
     if (se != null) {
       // NOTE: this code path only handles Access token based operations.
       // Presigned URL operations do not raise StorageException.
 
+      // Non-transient client errors must not be retried during upload/download, mirroring
+      // S3ErrorHandler.isClientException400Or404 (400 and 404 only).
+      // 401 is excluded here because it is handled below via token refresh.
+      //
+      // WHY THE SCOPE IS LIMITED TO UPLOAD AND DOWNLOAD:
+      // When OVERWRITE=FALSE, SnowflakeFileTransferAgent.filterExistingFiles() calls
+      // storageClient.listObjects() (returns an empty collection on miss — never a 404) and
+      // then storageClient.getObjectMetadata() on each listed object to compare digests.
+      // If the object disappears between the list and the metadata fetch (a race condition),
+      // GCSDefaultAccessStrategy.getObjectMetadata() throws StorageProviderException wrapping a
+      // synthetic StorageException(404). The SnowflakeFileTransferAgent.compareAndSkipRemoteFiles()
+      // SNOW-14521 handler attempts to recognise this 404 via isServiceException404(), but that
+      // method only checks SdkServiceException (AWS SDK); it does not recognise the GCS-native
+      // StorageException, so the exception is re-thrown and eventually reaches
+      // handleStorageException() with operation="compareRemoteFiles". Applying the
+      // immediate-throw rule for 404 here would break that retry path — the correct behaviour is
+      // to retry the list/compare cycle via the normal max-retries logic.
+      // NOTE: for the upload path itself, OVERWRITE=FALSE produces only a LIST (GET ?prefix=...)
+      // call and, if the file is found, skips the upload before getObjectMetadata() is ever
+      // reached. No 404 is generated by the upload-path existence check.
+      if ((StorageHelper.UPLOAD.equals(operation) || StorageHelper.DOWNLOAD.equals(operation))
+          && (se.getCode() == HttpStatus.SC_BAD_REQUEST
+              || se.getCode() == HttpStatus.SC_NOT_FOUND)) {
+        throw new SnowflakeSQLLoggedException(
+            queryId,
+            session,
+            SqlState.SYSTEM_ERROR,
+            StorageHelper.getOperationException(operation).getMessageCode(),
+            se,
+            operation,
+            se.getCode(),
+            se.getMessage(),
+            se.getReason());
+      }
+
       // If we have exceeded the max number of retries, propagate the error.
       if (retryCount > gcsClient.getMaxRetries()) {
         logger.error(
@@ -243,8 +268,8 @@ public boolean handleStorageException(
             gcsClient.getMaxRetries(),
             se.getCode(),
             se.getMessage(),
-            operation);
-        logger.error("Stack trace: ", ex);
+            operation,
+            ex);
         throw new SnowflakeSQLLoggedException(
             queryId,
             session,
@@ -260,8 +285,8 @@ public boolean handleStorageException(
             "Encountered exception ({}) during {}, retry count: {}",
             se.getMessage(),
             operation,
-            retryCount);
-        logger.debug("Stack trace: ", ex);
+            retryCount,
+            ex);
 
         // exponential backoff up to a limit
         int backoffInMillis = gcsClient.getRetryBackoffMin();
@@ -294,13 +319,29 @@ public boolean handleStorageException(
       }
       return true;
     } else {
+      // InterruptedException and SocketTimeoutException are handled by the outer retry loop in
+      // SnowflakeGCSClient.handleStorageException (retry within max retries, throw when exhausted).
+      // Return false so that logic is preserved rather than bypassed.
+      if (ex instanceof InterruptedException
+          || SnowflakeUtil.getRootCause(ex) instanceof SocketTimeoutException) {
+        return false;
+      }
       logger.error(
-          "GCSDefaultAccessStrategy: unhandled exception type {} during {}, not retrying: {}",
+          "GCSDefaultAccessStrategy: unhandled exception type {} during {}, not retrying",
           ex.getClass().getName(),
           operation,
-          ex.getMessage());
-      logger.error("Stack trace: ", ex);
-      return false;
+          ex);
+      // Re-throw as-is to avoid double-wrapping an already well-formed SQL exception.
+      if (ex instanceof SnowflakeSQLException) {
+        throw (SnowflakeSQLException) ex;
+      }
+      throw new SnowflakeSQLLoggedException(
+          queryId,
+          session,
+          SqlState.SYSTEM_ERROR,
+          StorageHelper.getOperationException(operation).getMessageCode(),
+          ex,
+          "Encountered exception during " + operation + ": " + ex.getMessage());
     }
   }
 
diff --git a/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/SnowflakeGCSClient.java b/src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/SnowflakeGCSClient.java
@@ -1244,13 +1244,7 @@ private void setupGCSClient(
       boolean overrideAwsAccessStrategy =
           Boolean.valueOf(systemGetEnv("SNOWFLAKE_GCS_FORCE_VIRTUAL_STYLE_DOMAINS"));
       if (stage.getUseVirtualUrl() || overrideAwsAccessStrategy) {
-        this.gcsAccessStrategy =
-            new GCSAccessStrategyAwsSdk(
-                stage,
-                session,
-                this.getMaxRetries(),
-                this.getRetryBackoffMin(),
-                this.getRetryBackoffMaxExponent());
+        this.gcsAccessStrategy = new GCSAccessStrategyAwsSdk(stage, session);
       } else {
         this.gcsAccessStrategy = new GCSDefaultAccessStrategy(stage, session);
       }
