From 8e1ccbb1525e1385f41185030c9d66886be54c06 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Fri, 26 Jun 2026 09:32:47 -0700 Subject: [PATCH] Handle 200 OK errors in PutObject (multipart) in S3 CRT client --- .../next-release/bugfix-AmazonS3-4405f32.json | 6 + .../crt/S3CrtResponseHandlerAdapter.java | 158 ++++++++++++ .../s3/crt/S3Crt200ErrorInBodyTest.java | 226 ++++++++++++++++++ .../crt/S3CrtResponseHandlerAdapterTest.java | 35 +++ 4 files changed, 425 insertions(+) create mode 100644 .changes/next-release/bugfix-AmazonS3-4405f32.json create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3Crt200ErrorInBodyTest.java diff --git a/.changes/next-release/bugfix-AmazonS3-4405f32.json b/.changes/next-release/bugfix-AmazonS3-4405f32.json new file mode 100644 index 000000000000..50e37852b4df --- /dev/null +++ b/.changes/next-release/bugfix-AmazonS3-4405f32.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "Amazon S3", + "contributor": "", + "description": "Handle 200 OK errors in PutObject (multipart) in S3 CRT client" +} diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java index 55531992a92c..7c778b173687 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java @@ -21,6 +21,9 @@ import java.nio.ByteBuffer; import java.time.Duration; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -37,6 +40,8 @@ import software.amazon.awssdk.crt.s3.S3FinishedResponseContext; import software.amazon.awssdk.crt.s3.S3MetaRequestProgress; import software.amazon.awssdk.crt.s3.S3MetaRequestResponseHandler; +import software.amazon.awssdk.http.AbortableInputStream; +import software.amazon.awssdk.http.SdkHttpFullResponse; import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.services.s3.model.S3Exception; @@ -220,6 +225,15 @@ private void handleServiceError(int responseStatus, HttpHeader[] headers, byte[] s3Exception.addSuppressed(sdkClientException); failResponseHandlerAndFuture(s3Exception); notifyResponsePublisherErrorIfNeeded(s3Exception); + } else if (responseStatus == 200) { + // handleServiceError is only called when crtCode != SUCCESS. If the response status is 200, this is the + // S3 "200 with error in body" case (e.g. CompleteMultipartUpload returning HTTP 200 with XML). + // We wrap the response in ErrorFlaggedSdkHttpResponse which overrides isSuccessful() to return false. + // This causes the downstream SDK pipeline (XmlResponseParserUtils, DecorateErrorFromResponseBodyUnmarshaller) + // to parse the body and detect the error, producing a properly modeled S3Exception. + SdkHttpFullResponse errorFlagged = new ErrorFlaggedSdkHttpResponse(errorResponse.build()); + initiateResponseHandling(errorFlagged); + onErrorResponseComplete(errorPayload); } else { initiateResponseHandling(errorResponse.build()); onErrorResponseComplete(errorPayload); @@ -303,4 +317,148 @@ private static SdkHttpResponse.Builder populateSdkHttpResponse(SdkHttpResponse.B private static class NoOpPublisherListener implements PublisherListener { } + + /** + * An {@link SdkHttpFullResponse} wrapper that overrides {@link #isSuccessful()} to always return {@code false}. + * This forces the SDK response pipeline ({@code XmlResponseParserUtils}, {@code DecorateErrorFromResponseBodyUnmarshaller}) + * to parse the response body even for HTTP 200 responses, enabling detection of S3's "200 with error in body" pattern. + * + *

The {@link #toBuilder()} method returns a builder whose {@code build()} also produces an + * {@code ErrorFlaggedSdkHttpResponse}, ensuring the override survives the rebuild cycle in + * {@code AsyncResponseHandler}.

+ */ + static final class ErrorFlaggedSdkHttpResponse implements SdkHttpFullResponse { + private final SdkHttpFullResponse delegate; + + ErrorFlaggedSdkHttpResponse(SdkHttpResponse delegate) { + // Ensure we have a full response to delegate to + if (delegate instanceof SdkHttpFullResponse) { + this.delegate = (SdkHttpFullResponse) delegate; + } else { + this.delegate = SdkHttpFullResponse.builder() + .statusCode(delegate.statusCode()) + .headers(delegate.headers()) + .build(); + } + } + + private ErrorFlaggedSdkHttpResponse(SdkHttpFullResponse delegate, @SuppressWarnings("unused") boolean internal) { + this.delegate = delegate; + } + + @Override + public boolean isSuccessful() { + return false; + } + + @Override + public int statusCode() { + return delegate.statusCode(); + } + + @Override + public Optional statusText() { + return delegate.statusText(); + } + + @Override + public Map> headers() { + return delegate.headers(); + } + + @Override + public Optional content() { + return delegate.content(); + } + + @Override + public Builder toBuilder() { + return new ErrorFlaggedBuilder(delegate.toBuilder()); + } + } + + /** + * A builder that wraps a standard {@link SdkHttpFullResponse.Builder} but produces an + * {@link ErrorFlaggedSdkHttpResponse} on {@code build()}, preserving the {@code isSuccessful()=false} override. + */ + private static final class ErrorFlaggedBuilder implements SdkHttpFullResponse.Builder { + private final SdkHttpFullResponse.Builder delegate; + + ErrorFlaggedBuilder(SdkHttpFullResponse.Builder delegate) { + this.delegate = delegate; + } + + @Override + public SdkHttpFullResponse build() { + return new ErrorFlaggedSdkHttpResponse(delegate.build(), true); + } + + @Override + public String statusText() { + return delegate.statusText(); + } + + @Override + public SdkHttpFullResponse.Builder statusText(String statusText) { + delegate.statusText(statusText); + return this; + } + + @Override + public int statusCode() { + return delegate.statusCode(); + } + + @Override + public SdkHttpFullResponse.Builder statusCode(int statusCode) { + delegate.statusCode(statusCode); + return this; + } + + @Override + public Map> headers() { + return delegate.headers(); + } + + @Override + public SdkHttpFullResponse.Builder putHeader(String headerName, List headerValues) { + delegate.putHeader(headerName, headerValues); + return this; + } + + @Override + public SdkHttpFullResponse.Builder appendHeader(String headerName, String headerValue) { + delegate.appendHeader(headerName, headerValue); + return this; + } + + @Override + public SdkHttpFullResponse.Builder headers(Map> headers) { + delegate.headers(headers); + return this; + } + + @Override + public SdkHttpFullResponse.Builder removeHeader(String headerName) { + delegate.removeHeader(headerName); + return this; + } + + @Override + public SdkHttpFullResponse.Builder clearHeaders() { + delegate.clearHeaders(); + return this; + } + + @Override + public AbortableInputStream content() { + return delegate.content(); + } + + @Override + public SdkHttpFullResponse.Builder content(AbortableInputStream content) { + delegate.content(content); + return this; + } + } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3Crt200ErrorInBodyTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3Crt200ErrorInBodyTest.java new file mode 100644 index 000000000000..ad4ae215480b --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/S3Crt200ErrorInBodyTest.java @@ -0,0 +1,226 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file 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 software.amazon.awssdk.services.s3.crt; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.delete; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.matching; +import static com.github.tomakehurst.wiremock.client.WireMock.post; +import static com.github.tomakehurst.wiremock.client.WireMock.put; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import java.io.IOException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Random; +import java.util.concurrent.CompletionException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.io.TempDir; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.crt.Log; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.model.PutObjectRequest; +import software.amazon.awssdk.services.s3.model.S3Exception; + +/** + * Tests that the CRT S3 client correctly detects and reports errors when S3 returns HTTP 200 with an error + * in the response body. This is a documented S3 API behavior for operations like CompleteMultipartUpload + * and CopyObject. + * + * @see + * CompleteMultipartUpload — note on 200 responses with error body + */ +@WireMockTest +@Timeout(30) +public class S3Crt200ErrorInBodyTest { + + private static final String BUCKET = "test-bucket"; + private static final String KEY = "test-key"; + private static final String UPLOAD_ID = "test-upload-id"; + private static final String ETAG = "\"d8e8fca2dc0f896fd7cb4cb0031ba249\""; + + private static final String ERROR_CODE = "ServiceUnavailable"; + private static final String ERROR_MESSAGE = "Service is temporarily unavailable. Please retry the request."; + private static final String REQUEST_ID = "test-request-id-001"; + + private static final String ERROR_BODY = + "" + + "" + + "" + ERROR_CODE + "" + + "" + ERROR_MESSAGE + "" + + "" + REQUEST_ID + "" + + ""; + + // 5 MB minimum part size for CRT multipart + private static final long PART_SIZE_BYTES = 5L * 1024 * 1024; + private static final int FILE_SIZE_BYTES = 11 * 1024 * 1024; + + private S3AsyncClient crtClient; + + @TempDir + Path tempDir; + + @BeforeAll + public static void setUpBeforeAll() { + System.setProperty("aws.crt.debugnative", "true"); + Log.initLoggingToStdout(Log.LogLevel.Warn); + } + + @BeforeEach + public void setup(WireMockRuntimeInfo wiremock) { + crtClient = S3AsyncClient.crtBuilder() + .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort())) + .forcePathStyle(true) + .region(Region.US_EAST_1) + .credentialsProvider(StaticCredentialsProvider.create( + AwsBasicCredentials.create("test-key", "test-secret"))) + .minimumPartSizeInBytes(PART_SIZE_BYTES) + .thresholdInBytes(PART_SIZE_BYTES) + .retryConfiguration(S3CrtRetryConfiguration.builder().numRetries(0).build()) + .build(); + } + + @AfterEach + public void tearDown() { + if (crtClient != null) { + crtClient.close(); + } + } + + @Test + @DisplayName("CRT putObject (multipart) should fail with S3Exception when CompleteMultipartUpload returns 200+error") + void putObject_completeMultipartReturns200WithError_shouldFailWithS3Exception() throws IOException { + stubCreateMultipartUpload(); + stubUploadParts(); + stubCompleteMultipartUploadWith200PlusErrorBody(); + stubAbortMultipartUpload(); + + Path testFile = createTestFile(); + + assertThatThrownBy(() -> + crtClient.putObject( + PutObjectRequest.builder().bucket(BUCKET).key(KEY).build(), + testFile + ).join() + ).isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(S3Exception.class) + .satisfies(thrown -> { + S3Exception s3e = (S3Exception) thrown.getCause(); + assertThat(s3e.statusCode()).isEqualTo(200); + assertThat(s3e.awsErrorDetails().errorCode()).isEqualTo(ERROR_CODE); + assertThat(s3e.awsErrorDetails().errorMessage()).isEqualTo(ERROR_MESSAGE); + assertThat(s3e.requestId()).isEqualTo(REQUEST_ID); + }); + } + + @Test + @DisplayName("CRT copyObject should fail with S3Exception when CopyObject returns 200+error") + void copyObject_returns200WithError_shouldFailWithS3Exception() { + // For CRT copy, stub HEAD (source object check) to succeed, then the actual copy to return 200+error. + // CRT may issue a HEAD on the source object to determine size for single-part vs multi-part copy. + stubFor(com.github.tomakehurst.wiremock.client.WireMock.head( + urlPathEqualTo("/" + BUCKET + "/source-key")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Length", "1024") + .withHeader("ETag", ETAG))); + + // The actual copy request (PUT with x-amz-copy-source) + stubFor(put(urlPathEqualTo("/" + BUCKET + "/" + KEY)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/xml") + .withBody(ERROR_BODY))); + + assertThatThrownBy(() -> + crtClient.copyObject(r -> r + .sourceBucket(BUCKET) + .sourceKey("source-key") + .destinationBucket(BUCKET) + .destinationKey(KEY) + ).join() + ).isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(S3Exception.class) + .satisfies(thrown -> { + S3Exception s3e = (S3Exception) thrown.getCause(); + assertThat(s3e.statusCode()).isEqualTo(200); + assertThat(s3e.awsErrorDetails().errorCode()).isEqualTo(ERROR_CODE); + assertThat(s3e.awsErrorDetails().errorMessage()).isEqualTo(ERROR_MESSAGE); + }); + } + + private void stubCreateMultipartUpload() { + stubFor(post(urlPathEqualTo("/" + BUCKET + "/" + KEY)) + .withQueryParam("uploads", equalTo("")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/xml") + .withBody( + "" + + "" + + "" + BUCKET + "" + + "" + KEY + "" + + "" + UPLOAD_ID + "" + + "" + ))); + } + + private void stubUploadParts() { + stubFor(put(urlPathEqualTo("/" + BUCKET + "/" + KEY)) + .withQueryParam("partNumber", matching("[0-9]+")) + .withQueryParam("uploadId", equalTo(UPLOAD_ID)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("ETag", ETAG))); + } + + private void stubCompleteMultipartUploadWith200PlusErrorBody() { + stubFor(post(urlPathEqualTo("/" + BUCKET + "/" + KEY)) + .withQueryParam("uploadId", equalTo(UPLOAD_ID)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/xml") + .withBody(ERROR_BODY))); + } + + private void stubAbortMultipartUpload() { + stubFor(delete(urlPathEqualTo("/" + BUCKET + "/" + KEY)) + .withQueryParam("uploadId", equalTo(UPLOAD_ID)) + .willReturn(aResponse().withStatus(204))); + } + + private Path createTestFile() throws IOException { + Path file = tempDir.resolve("upload-test.dat"); + byte[] data = new byte[FILE_SIZE_BYTES]; + new Random(42).nextBytes(data); + Files.write(file, data); + return file; + } +} diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapterTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapterTest.java index 89e3301738f1..d8c55f5820cc 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapterTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapterTest.java @@ -205,6 +205,41 @@ public void requestFailedMidwayDueToIoError_shouldInvokeOnError() { verify(s3MetaRequest).close(); } + @Test + public void errorWithHttp200Status_shouldCompleteFutureExceptionally() { + // Simulates the S3 "200 OK with error in body" case (e.g. CompleteMultipartUpload returning + // ServiceUnavailable). CRT detects the error and reports a non-zero error code with responseStatus=200. + // responseHandlingInitiated is false because CRT never calls onResponseBody for this case. + // + // The adapter wraps the response in an ErrorFlaggedSdkHttpResponse (isSuccessful()=false) and routes + // it through the normal SDK pipeline. In this unit test we verify the adapter passes a non-successful + // response to the handler and sends the error payload through the stream. + responseHandlerAdapter.onResponseHeaders(200, new HttpHeader[0]); + + byte[] errorPayload = ("ServiceUnavailable" + + "Service is temporarily unavailable." + + "test-request-id") + .getBytes(StandardCharsets.UTF_8); + + S3FinishedResponseContext errorContext = stubResponseContext(1, 200, errorPayload); + List headers = new ArrayList<>(); + headers.add(new HttpHeader(X_AMZN_REQUEST_ID_HEADER_ALTERNATE, "req-id-123")); + headers.add(new HttpHeader(X_AMZ_ID_2_HEADER, "ext-id-456")); + when(errorContext.getErrorHeaders()).thenReturn(headers.toArray(new HttpHeader[0])); + + responseHandlerAdapter.onFinished(errorContext); + + // Verify the response handler received a non-successful response with status 200 + assertThat(sdkResponseHandler.sdkHttpResponse).isNotNull(); + assertThat(sdkResponseHandler.sdkHttpResponse.isSuccessful()).isFalse(); + assertThat(sdkResponseHandler.sdkHttpResponse.statusCode()).isEqualTo(200); + + // The result future completes normally (the full SDK pipeline in production would produce + // an S3Exception from the error body via DecorateErrorFromResponseBodyUnmarshaller) + assertThat(future).isCompleted(); + verify(s3MetaRequest).close(); + } + @Test public void requestFailedWithCause_shouldCompleteFutureExceptionallyWithCause() { RuntimeException cause = new RuntimeException("error");