From 1b176a8a40b0c44c8689eb324648fe910296b2b9 Mon Sep 17 00:00:00 2001 From: Divyansh Vijayvergia Date: Thu, 30 Jul 2026 21:22:29 +0000 Subject: [PATCH 1/4] Do not retry requests with a consumed streaming body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a request with a streaming body (e.g. Files.upload) receives a retriable HTTP response, the SDK retried by re-sending the same InputStream. The stream was already consumed by the first attempt, so the retry transmitted an empty body — silently uploading a 0-byte file (HTTP 204) or surfacing as a confusing InternalError. InputStream-backed bodies cannot be rewound, so retrying is unsafe once the body has been sent. Skip the retry when the request has a streaming body and an HTTP response was received (which proves the body was transmitted), and surface the original error so the caller can retry with a fresh stream. Transport-level IOErrors (no response received, e.g. a pre-send ConnectException) still retry, since the stream may not have been read. --- .../com/databricks/sdk/core/ApiClient.java | 16 +++++++ .../databricks/sdk/core/ApiClientTest.java | 42 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java index 3cbd0d61a..b45644af6 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java @@ -275,6 +275,22 @@ private Response executeInner(Request in, String path, RequestOptions options) { if (!retryStrategy.isRetriable(databricksError)) { throw databricksError; } + + // A streaming request body (e.g. Files.upload) is backed by a single-use InputStream that the + // first attempt consumes as it is sent. Receiving an HTTP response (response != null) proves + // the body was already transmitted, so retrying would re-send an empty body and silently + // upload 0 bytes (or surface as a confusing downstream error). Since the stream cannot be + // rewound, surface the original error instead so the caller can retry with a fresh stream. + // Transport-level IOErrors (response == null, e.g. a pre-send ConnectException) are left to + // retry as before, since in that case the stream may not have been read. + if (in.isBodyStreaming() && response != null) { + LOG.debug( + "Not retrying {} despite a retriable error: the request has a non-repeatable streaming" + + " body that was already consumed by the previous attempt", + in.getRequestLine()); + throw databricksError; + } + if (attemptNumber == maxAttempts) { throw new DatabricksException( String.format("Request %s failed after %d retries", in, maxAttempts), databricksError); diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java index fa1bb6a6c..c339c4823 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java @@ -6,6 +6,7 @@ import com.databricks.sdk.core.error.PrivateLinkValidationError; import com.databricks.sdk.core.error.details.ErrorDetails; import com.databricks.sdk.core.error.details.ErrorInfo; +import com.databricks.sdk.core.error.platform.TemporarilyUnavailable; import com.databricks.sdk.core.error.platform.TooManyRequests; import com.databricks.sdk.core.http.Request; import com.databricks.sdk.core.http.Response; @@ -15,10 +16,12 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.MalformedURLException; import java.net.URL; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; import java.time.*; import java.util.*; import org.apache.http.impl.EnglishReasonPhraseCatalog; @@ -484,6 +487,45 @@ void privateLinkRedirectBecomesPrivateLinkValidationError() throws MalformedURLE assertTrue(e.getMessage().contains("AWS PrivateLink")); } + @Test + void doesNotRetryStreamingBodyAfterResponse() throws IOException { + // Regression test: a streaming request body (e.g. Files.upload) is backed by a single-use + // InputStream that the first attempt consumes. Receiving a retriable HTTP response means the + // body was already sent, so a retry would upload an empty body. Verify the client surfaces the + // original error to the caller instead of retrying. + String path = "/api/2.0/fs/files/Volumes/main/default/vol/f.json"; + String url = "http://my.host" + path; + byte[] contents = "file-contents".getBytes(StandardCharsets.UTF_8); + Request stub = new Request("PUT", url, new ByteArrayInputStream(contents)); + // If the guard fails and a retry is issued, it would consume the success response and pass. + ApiClient client = + getApiClient( + stub, + Arrays.asList(getTransientError(stub, 503, (String) null), getSuccessResponse(stub))); + + ByteArrayInputStream body = new ByteArrayInputStream(contents); + DatabricksError exception = + assertThrows( + DatabricksError.class, + () -> client.execute(new Request("PUT", path, body), Void.class)); + + assertInstanceOf(TemporarilyUnavailable.class, exception); + assertEquals(503, exception.getStatusCode()); + } + + @Test + void retriesNonStreamingBodyOn503() throws IOException { + // Complement to doesNotRetryStreamingBodyAfterResponse: a string-bodied request is repeatable + // (a fresh entity is built per attempt), so the same 503 must still be retried as before. This + // confirms the streaming guard is scoped narrowly and does not regress ordinary requests. + Request req = getExampleNonIdempotentRequest(); + runApiClientTest( + req, + Arrays.asList(getTransientError(req, 503, (String) null), getSuccessResponse(req)), + MyEndpointResponse.class, + new MyEndpointResponse().setKey("value")); + } + @Test void testDefaultWorkspaceIdReturnsNullWhenNotSet() { Request req = getBasicRequest(); From 4e1294454458b96005984fd34c8c558c14cbea92 Mon Sep 17 00:00:00 2001 From: Divyansh Vijayvergia Date: Fri, 31 Jul 2026 08:44:29 +0000 Subject: [PATCH 2/4] Add NEXT_CHANGELOG entry for streaming-body retry fix --- NEXT_CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index aae1f29cc..043b93d23 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -8,6 +8,8 @@ ### Bug Fixes +* Fixed requests with a streaming body (e.g. `files().upload()`) silently uploading an empty body when retried. A single-use `InputStream` body is consumed by the first attempt, so retrying a retriable error (e.g. HTTP 503) re-sent an empty stream, which could write a 0-byte file or surface as a confusing error. The SDK no longer retries a streaming request once its body has been sent, and instead surfaces the original error so the caller can retry with a fresh stream. + ### Security Vulnerabilities ### Documentation From 31df6ead0c3c8b93d4003c50277f44b9c00afe2f Mon Sep 17 00:00:00 2001 From: Divyansh Vijayvergia Date: Mon, 3 Aug 2026 09:58:01 +0000 Subject: [PATCH 3/4] Strengthen streaming-retry test to assert bytes sent per attempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous regression test used a mock HTTP client that never read the request body, so it only verified that no retry was issued — not the actual 0-byte symptom. Replace it with a transport that drains the body like CommonsHttpClient does and records bytes sent per attempt. The test now asserts the streaming upload is sent exactly once at full size and the original 503 is surfaced; without the fix it fails showing "[13, 0]" (a full first attempt followed by an empty-body retry). Also extend the complementary test to assert a string body is retried on 503 AND re-sends the full body on the retry, confirming the guard is scoped to non-repeatable streaming bodies only. --- .../databricks/sdk/core/ApiClientTest.java | 135 ++++++++++++++---- 1 file changed, 105 insertions(+), 30 deletions(-) diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java index c339c4823..c873840cc 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java @@ -8,6 +8,7 @@ import com.databricks.sdk.core.error.details.ErrorInfo; import com.databricks.sdk.core.error.platform.TemporarilyUnavailable; import com.databricks.sdk.core.error.platform.TooManyRequests; +import com.databricks.sdk.core.http.HttpClient; import com.databricks.sdk.core.http.Request; import com.databricks.sdk.core.http.Response; import com.databricks.sdk.core.utils.FakeTimer; @@ -18,6 +19,7 @@ import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; import java.net.MalformedURLException; import java.net.URL; import java.net.UnknownHostException; @@ -487,43 +489,116 @@ void privateLinkRedirectBecomesPrivateLinkValidationError() throws MalformedURLE assertTrue(e.getMessage().contains("AWS PrivateLink")); } + /** + * A fake HttpClient that reads the request body to EOF on every call, mirroring how the real + * CommonsHttpClient drains the entity onto the wire. It records the number of body bytes actually + * transmitted per attempt, so tests can assert what a retry would (or would not) send. The status + * code returned for each attempt is supplied up front. + */ + private static class BodyReadingHttpClient implements HttpClient { + private final Deque statusCodes; + final List bytesReadPerAttempt = new ArrayList<>(); + + BodyReadingHttpClient(Integer... statusCodesInOrder) { + this.statusCodes = new ArrayDeque<>(Arrays.asList(statusCodesInOrder)); + } + + @Override + public Response execute(Request in) throws IOException { + // The SDK issues a best-effort GET /.well-known/databricks-config host-metadata pre-flight + // through this same client before the request under test. Ignore it: return a benign 404 + // (the SDK falls back to user config) without recording it or consuming a status code. + if (in.getUrl().contains("/.well-known/")) { + return new Response(in, 404, "Not Found", Collections.emptyMap()); + } + int total = 0; + if (in.isBodyStreaming() && in.getBodyStream() != null) { + InputStream is = in.getBodyStream(); + byte[] buf = new byte[4096]; + int r; + while ((r = is.read(buf)) != -1) { + total += r; + } + } else if (in.isBodyString() && in.getBodyString() != null) { + total = in.getBodyString().getBytes(StandardCharsets.UTF_8).length; + } + bytesReadPerAttempt.add(total); + int status = statusCodes.isEmpty() ? 204 : statusCodes.removeFirst(); + String reason = EnglishReasonPhraseCatalog.INSTANCE.getReason(status, Locale.ENGLISH); + return new Response(in, status, reason, Collections.emptyMap()); + } + } + + private ApiClient apiClientWith(HttpClient httpClient) { + DatabricksConfig config = + new DatabricksConfig() + .setHost("http://my.host") + .setCredentialsProvider(new DummyCredentialsProvider()) + .setHttpClient(httpClient); + return new ApiClient(config, new FakeTimer()); + } + @Test void doesNotRetryStreamingBodyAfterResponse() throws IOException { - // Regression test: a streaming request body (e.g. Files.upload) is backed by a single-use - // InputStream that the first attempt consumes. Receiving a retriable HTTP response means the - // body was already sent, so a retry would upload an empty body. Verify the client surfaces the - // original error to the caller instead of retrying. - String path = "/api/2.0/fs/files/Volumes/main/default/vol/f.json"; - String url = "http://my.host" + path; + // Regression test for the streaming-upload retry bug: a streaming request body (e.g. + // Files.upload) is backed by a single-use InputStream that the first attempt consumes as it is + // sent. If the SDK retried after a 503, it would re-send the now-exhausted stream as a 0-byte + // body, silently uploading an empty file. Using a transport that actually reads the body (like + // the real CommonsHttpClient) and would return 204 on a retry, we assert on the bytes actually + // transmitted per attempt: without the fix this records [13, 0] (an empty retry that "succeeds" + // with 204); with the fix the upload is attempted exactly once and the original 503 is thrown. byte[] contents = "file-contents".getBytes(StandardCharsets.UTF_8); - Request stub = new Request("PUT", url, new ByteArrayInputStream(contents)); - // If the guard fails and a retry is issued, it would consume the success response and pass. - ApiClient client = - getApiClient( - stub, - Arrays.asList(getTransientError(stub, 503, (String) null), getSuccessResponse(stub))); - - ByteArrayInputStream body = new ByteArrayInputStream(contents); - DatabricksError exception = - assertThrows( - DatabricksError.class, - () -> client.execute(new Request("PUT", path, body), Void.class)); + // Second status (204) is what a buggy empty-body retry would receive; the fix means it is never + // reached, but supplying it lets this test capture the empty retry as a byte count if it were. + BodyReadingHttpClient hc = new BodyReadingHttpClient(503, 204); + ApiClient client = apiClientWith(hc); + + InputStream body = new ByteArrayInputStream(contents); + DatabricksError thrown = null; + try { + client.execute(new Request("PUT", "/api/2.0/fs/files/Volumes/c/s/v/f", body), Void.class); + } catch (DatabricksError e) { + thrown = e; + } - assertInstanceOf(TemporarilyUnavailable.class, exception); - assertEquals(503, exception.getStatusCode()); + // The upload must be attempted exactly once: retrying is unsafe once the body has been sent. + // Without the guard this is [13, 0] (the empty-body retry) — the message points right at it. + assertEquals( + 1, + hc.bytesReadPerAttempt.size(), + "streaming upload must not be retried after the body was sent; the retry would send an empty" + + " body. Bytes sent per attempt: " + + hc.bytesReadPerAttempt); + // That single attempt transmitted the full body ... + assertEquals(contents.length, hc.bytesReadPerAttempt.get(0)); + // ... and the stream is now exhausted, which is exactly why a resend would upload 0 bytes. + assertEquals(-1, body.read(), "stream is single-use and should be fully consumed"); + // The original transient error is surfaced to the caller (who can retry with a fresh stream) + // rather than being masked by a bogus 204 success. + assertNotNull(thrown, "the original 503 must be surfaced to the caller"); + assertInstanceOf(TemporarilyUnavailable.class, thrown); + assertEquals(503, thrown.getStatusCode()); } @Test - void retriesNonStreamingBodyOn503() throws IOException { - // Complement to doesNotRetryStreamingBodyAfterResponse: a string-bodied request is repeatable - // (a fresh entity is built per attempt), so the same 503 must still be retried as before. This - // confirms the streaming guard is scoped narrowly and does not regress ordinary requests. - Request req = getExampleNonIdempotentRequest(); - runApiClientTest( - req, - Arrays.asList(getTransientError(req, 503, (String) null), getSuccessResponse(req)), - MyEndpointResponse.class, - new MyEndpointResponse().setKey("value")); + void retriesNonStreamingBodyOn503AndResendsFullBody() throws IOException { + // Complement to doesNotRetryStreamingBodyAfterResponse: a string-bodied request is repeatable, + // so the same 503 must still be retried, and crucially the retry must re-send the full body + // (a fresh entity is built per attempt). This confirms the streaming guard is scoped narrowly + // and does not regress ordinary requests. + String jsonBody = "{\"key\":\"value\"}"; + BodyReadingHttpClient hc = new BodyReadingHttpClient(503, 200); + ApiClient client = apiClientWith(hc); + + client.execute( + new Request("POST", "/api/2.0/sql/statements/", jsonBody), MyEndpointResponse.class); + + // Two attempts were made: the 503 was retried ... + assertEquals(2, hc.bytesReadPerAttempt.size()); + // ... and both attempts sent the full body (the string body is re-sendable, unlike a stream). + int expected = jsonBody.getBytes(StandardCharsets.UTF_8).length; + assertEquals(expected, hc.bytesReadPerAttempt.get(0)); + assertEquals(expected, hc.bytesReadPerAttempt.get(1)); } @Test From 17bd3098189bc854b26b81d3c168565b9ba2b821 Mon Sep 17 00:00:00 2001 From: Divyansh Vijayvergia Date: Tue, 4 Aug 2026 13:25:49 +0000 Subject: [PATCH 4/4] Tighten comments in streaming-retry fix and test Condense over-verbose comments, soften "proves" to "means", and drop a redundant assertNotNull. No behavior change; the test still fails without the guard on the byte-count assertion (bytes per attempt [13, 0]). --- .../com/databricks/sdk/core/ApiClient.java | 10 +++--- .../databricks/sdk/core/ApiClientTest.java | 33 +++++++------------ 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java index b45644af6..09b833e1a 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java @@ -277,12 +277,10 @@ private Response executeInner(Request in, String path, RequestOptions options) { } // A streaming request body (e.g. Files.upload) is backed by a single-use InputStream that the - // first attempt consumes as it is sent. Receiving an HTTP response (response != null) proves - // the body was already transmitted, so retrying would re-send an empty body and silently - // upload 0 bytes (or surface as a confusing downstream error). Since the stream cannot be - // rewound, surface the original error instead so the caller can retry with a fresh stream. - // Transport-level IOErrors (response == null, e.g. a pre-send ConnectException) are left to - // retry as before, since in that case the stream may not have been read. + // first attempt consumes as it is sent. Receiving an HTTP response (response != null) means + // the body was already sent, so retrying would re-send an empty stream and upload 0 bytes. + // The stream cannot be rewound, so surface the original error and let the caller retry with a + // fresh stream. Transport IOErrors (response == null) still retry, as the body may be unsent. if (in.isBodyStreaming() && response != null) { LOG.debug( "Not retrying {} despite a retriable error: the request has a non-repeatable streaming" diff --git a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java index c873840cc..9f47e26b6 100644 --- a/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java +++ b/databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java @@ -540,20 +540,17 @@ private ApiClient apiClientWith(HttpClient httpClient) { @Test void doesNotRetryStreamingBodyAfterResponse() throws IOException { - // Regression test for the streaming-upload retry bug: a streaming request body (e.g. - // Files.upload) is backed by a single-use InputStream that the first attempt consumes as it is - // sent. If the SDK retried after a 503, it would re-send the now-exhausted stream as a 0-byte - // body, silently uploading an empty file. Using a transport that actually reads the body (like - // the real CommonsHttpClient) and would return 204 on a retry, we assert on the bytes actually - // transmitted per attempt: without the fix this records [13, 0] (an empty retry that "succeeds" - // with 204); with the fix the upload is attempted exactly once and the original 503 is thrown. + // A streaming body (e.g. Files.upload) is a single-use InputStream consumed by the first + // attempt, so retrying a 503 would re-send an empty stream and silently upload 0 bytes. The + // upload must be attempted exactly once and the 503 surfaced to the caller. byte[] contents = "file-contents".getBytes(StandardCharsets.UTF_8); - // Second status (204) is what a buggy empty-body retry would receive; the fix means it is never - // reached, but supplying it lets this test capture the empty retry as a byte count if it were. + // The 204 is what a buggy empty-body retry would receive; the guard means it is never reached. BodyReadingHttpClient hc = new BodyReadingHttpClient(503, 204); ApiClient client = apiClientWith(hc); InputStream body = new ByteArrayInputStream(contents); + // Catch rather than assertThrows so the byte-count assertions below run first: without the fix + // no exception is thrown, and those assertions give the more informative [13, 0] failure. DatabricksError thrown = null; try { client.execute(new Request("PUT", "/api/2.0/fs/files/Volumes/c/s/v/f", body), Void.class); @@ -561,21 +558,14 @@ void doesNotRetryStreamingBodyAfterResponse() throws IOException { thrown = e; } - // The upload must be attempted exactly once: retrying is unsafe once the body has been sent. - // Without the guard this is [13, 0] (the empty-body retry) — the message points right at it. + // Exactly one attempt, sending the full body. Without the guard this is [13, 0]: a full first + // attempt followed by an empty-body retry. assertEquals( 1, hc.bytesReadPerAttempt.size(), - "streaming upload must not be retried after the body was sent; the retry would send an empty" - + " body. Bytes sent per attempt: " - + hc.bytesReadPerAttempt); - // That single attempt transmitted the full body ... + "streaming upload must not be retried; bytes sent per attempt: " + hc.bytesReadPerAttempt); assertEquals(contents.length, hc.bytesReadPerAttempt.get(0)); - // ... and the stream is now exhausted, which is exactly why a resend would upload 0 bytes. assertEquals(-1, body.read(), "stream is single-use and should be fully consumed"); - // The original transient error is surfaced to the caller (who can retry with a fresh stream) - // rather than being masked by a bogus 204 success. - assertNotNull(thrown, "the original 503 must be surfaced to the caller"); assertInstanceOf(TemporarilyUnavailable.class, thrown); assertEquals(503, thrown.getStatusCode()); } @@ -593,10 +583,9 @@ void retriesNonStreamingBodyOn503AndResendsFullBody() throws IOException { client.execute( new Request("POST", "/api/2.0/sql/statements/", jsonBody), MyEndpointResponse.class); - // Two attempts were made: the 503 was retried ... - assertEquals(2, hc.bytesReadPerAttempt.size()); - // ... and both attempts sent the full body (the string body is re-sendable, unlike a stream). + // The 503 was retried, and both attempts sent the full body (a string body is re-sendable). int expected = jsonBody.getBytes(StandardCharsets.UTF_8).length; + assertEquals(2, hc.bytesReadPerAttempt.size()); assertEquals(expected, hc.bytesReadPerAttempt.get(0)); assertEquals(expected, hc.bytesReadPerAttempt.get(1)); }