Skip to content

SNOW-3745081: fix retry in GCS storage client#2688

Merged
sfc-gh-dszmolka merged 5 commits into
masterfrom
SNOW-3745081-fix-gcs-storageexception-retry
Jul 7, 2026
Merged

SNOW-3745081: fix retry in GCS storage client#2688
sfc-gh-dszmolka merged 5 commits into
masterfrom
SNOW-3745081-fix-gcs-storageexception-retry

Conversation

@sfc-gh-dszmolka

@sfc-gh-dszmolka sfc-gh-dszmolka commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Problem

GCS uploads silently gave up after a single failure despite putgetmaxretries being configured
(default: 25). Customers saw the driver throw immediately on the first HTTP 503 ("We encountered
an internal error. Please try again.") with no retries.

The bug affects both GCS access strategies:

  • GCSDefaultAccessStrategy — the GCS Java SDK path, which is the default when using internal
    GCS stages with an access token.
  • GCSAccessStrategyAwsSdk — the S3-compatible path, used when
    SNOWFLAKE_GCS_FORCE_VIRTUAL_STYLE_DOMAINS=true.

Root cause

SnowflakeGCSClient.upload() delegates the actual cloud SDK call to an inner private method,
uploadWithDownScopedToken(). When the SDK raises an exception (e.g. on HTTP 503), the inner
method catches it and always re-throws it wrapped inside a SnowflakeSQLLoggedException
before control returns to the outer retry loop.

The outer retry loop calls the strategy's handleStorageException() with this wrapped exception.
Both handlers only recognised the SDK exception when it appeared as the direct argument, not
when it was buried inside a wrapper:

// GCSDefaultAccessStrategy — BEFORE
if (ex instanceof StorageException) { /* retry */ }
else { return false; }  // ← SnowflakeSQLLoggedException(StorageException) fell here → no retry

// GCSAccessStrategyAwsSdk — BEFORE
Throwable cause = ex.getCause();  // one level only — missed deeper wrapping
if (cause instanceof SdkException) { /* retry */ }
else { return false; }  // ← same problem

The exception chains reaching the outer handler are:

GCSDefaultAccessStrategy:  SnowflakeSQLLoggedException → StorageException(503)
GCSAccessStrategyAwsSdk:   SnowflakeSQLLoggedException → CompletionException → SdkException(503)

A similar bug was fixed for Azure in a previous commit; this change applies the same pattern
to both GCS strategies.


Changes

1. SnowflakeUtil.java — shared cause-chain helper

Added a generic findFirstCauseOfType(Throwable ex, Class<T> type) utility method that walks
the full cause chain and returns the first exception of the given type, or null if none is
found. Both GCS strategies use this instead of hand-written traversal loops, keeping them
consistent with each other and with the Azure fix pattern.


2. GCSDefaultAccessStrategy.java — bug fix + defensive improvements

Why this was needed:
handleStorageException checked ex instanceof StorageException to decide whether to retry.
Because uploadWithDownScopedToken always wraps the SDK exception in SnowflakeSQLLoggedException,
the instanceof check always failed and the method returned false — no retry was ever
attempted, regardless of putgetmaxretries.

What changed:

  • Cause-chain walking via SnowflakeUtil.findFirstCauseOfType(ex, StorageException.class) so wrapped exceptions are detected at any nesting depth.
  • Non-retryable 400/404 guard: HTTP 400 and 404 during UPLOAD/DOWNLOAD throw immediately (mirrors S3ErrorHandler.isClientException400Or404); scoped to upload/download only to preserve retry behaviour for compareRemoteFiles 404s (SNOW-14521 race condition).
  • Else-branch rethrow: unrecognised exceptions (no StorageException in chain) are now re-thrown instead of silently returning false; InterruptedException and SocketTimeoutException still return false as before.
  • Consolidated logging: failure-terminal logger.error calls pass ex as the final argument (SLF4J convention), removing the previous duplicate stack-trace log line.

3. GCSDefaultAccessStrategyTest.java (new file) — regression tests for the fix

Three unit tests covering handleStorageException:

Test Scenario Expected
testHandleStorageExceptionDirect503BelowMaxRetries Direct StorageException(503), below max retries Does not throw — signals retry
testHandleStorageExceptionWrapped503BelowMaxRetries RuntimeExceptionStorageException(503), below max retries Does not throw — cause chain walked ← key regression test
testHandleStorageExceptionWrapped503OverMaxRetries RuntimeExceptionStorageException(503), over max retries Throws SnowflakeSQLException

4. GCSAccessStrategyAwsSdk.java — same wrapping bug fixed + SDK retry alignment

Why this was needed:
GCSAccessStrategyAwsSdk.handleStorageException only walked one level deep
(ex.getCause() instanceof SdkException). Because uploadWithDownScopedToken wraps the SDK
exception inside CompletionException which is in turn wrapped in SnowflakeSQLLoggedException,
the one-level check missed it and returned false — no retry was attempted.

Additionally, the AWS SDK was running with its own hardcoded internal retry policy (standard
mode: 3 retries with SDK-internal backoff), independently of the JDBC outer loop. This created
a double retry loop: the SDK would retry 3 times internally, and if it eventually threw,
the JDBC outer loop would retry the entire thing again — multiplying the actual attempts.

What changed:

  • Cause-chain walking via SnowflakeUtil.findFirstCauseOfType(ex, SdkException.class), consistent with the GCS default strategy fix.
  • SDK internal retries disabled via RetryPolicy.defaultRetryPolicy().toBuilder().numRetries(0): built from the default policy to preserve the AWS circuit breaker and throttling conditions; numRetries(0) hands full retry control to the JDBC outer loop.
  • Non-retryable 400/404 guard via S3ErrorHandler.isClientException400Or404, consistent with SnowflakeS3Client.

About why no new unit test was added:
The handleStorageException change in GCSAccessStrategyAwsSdk follows the same pattern as
the default strategy fix (cause-chain walking). Constructing a meaningful mock test requires
either a real SdkException hierarchy or a functional AWS SDK mock — both are integration-test
territory. The existing SnowflakeGCSClientTest tests cover construction of
GCSAccessStrategyAwsSdk through SnowflakeGCSClient.createSnowflakeGCSClient() and continue
to pass unchanged. The behaviour is verified by the E2E tests described below.

@sfc-gh-dszmolka sfc-gh-dszmolka marked this pull request as ready for review July 7, 2026 07:43
@sfc-gh-dszmolka sfc-gh-dszmolka requested a review from a team as a code owner July 7, 2026 07:43
@sfc-gh-dszmolka sfc-gh-dszmolka changed the title fix retry in GCS storage client SNOW-3745081: fix retry in GCS storage client Jul 7, 2026
@sfc-gh-dszmolka

Copy link
Copy Markdown
Contributor Author

E2E Tests Performed After Review-Comment Changes

All tests ran against live Snowflake account located in GCS (London, europer-west2) using the fixed JAR built from branch SNOW-3745081-fix-gcs-storageexception-retry.

Retry / mitmproxy tests — PUT @~ (user stage), maxRetries=5

# Strategy Scenario Outcome
1 GCS Default mitmproxy: connection refused on every attempt 5 retries exhausted, error surfaced to caller
2 GCS Default mitmproxy: 503 on attempts 1–3, pass on attempt 4 Retried successfully, upload completed on 4th attempt
3 GCS AWS SDK mitmproxy: connection refused on every attempt Initially failed after 1 attempt (exposed findFirstCauseOfType bug in AWS handler); fixed and re-run → 5 retries exhausted, error surfaced
4 GCS AWS SDK mitmproxy: 503 on attempts 1–3, pass on attempt 4 Fixed and re-run → retried successfully, upload completed on 4th attempt

OVERWRITE tests — PUT @~, direct connection (no proxy)

# Strategy Scenario Outcome
5 GCS AWS SDK OVERWRITE=TRUE, new file Uploaded successfully to stage
6 GCS AWS SDK OVERWRITE=FALSE, file already on stage Upload skipped by filterExistingFiles
7 GCS AWS SDK OVERWRITE=FALSE, file not on stage Uploaded successfully
8 GCS Default OVERWRITE=TRUE, new file Uploaded successfully to stage
9 GCS Default OVERWRITE=FALSE, file already on stage Upload skipped by filterExistingFiles
10 GCS Default OVERWRITE=FALSE, file not on stage Uploaded successfully

Download tests — GET, direct connection (no proxy)

# Strategy Scenario Outcome
11 GCS Default GET existing file Downloaded successfully, 0 retries
12 GCS Default GET non-existing file Server returned empty file list; no GCS call made, statement completed cleanly
13 GCS Default GET existing file, local copy already present MD5 from listObjects response matched local file; download marked SKIPPED
14 GCS AWS SDK GET existing file Downloaded successfully, 0 retries
15 GCS AWS SDK GET non-existing file Server returned empty file list; no GCS call made, statement completed cleanly
16 GCS AWS SDK GET existing file, local copy already present HEAD request returned 200 OK; digest compared, file re-downloaded

@sfc-gh-rkowalski sfc-gh-rkowalski left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sfc-gh-dszmolka sfc-gh-dszmolka enabled auto-merge (squash) July 7, 2026 12:46
@sfc-gh-dszmolka sfc-gh-dszmolka merged commit 66bfc5d into master Jul 7, 2026
135 of 138 checks passed
@sfc-gh-dszmolka sfc-gh-dszmolka deleted the SNOW-3745081-fix-gcs-storageexception-retry branch July 7, 2026 13:21
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants