Skip to content

Handle 200 OK errors in PutObject (multipart) in S3 CRT client#7084

Open
alextwoods wants to merge 1 commit into
masterfrom
alexwoo/fix_crt_s3_200_error_mp
Open

Handle 200 OK errors in PutObject (multipart) in S3 CRT client#7084
alextwoods wants to merge 1 commit into
masterfrom
alexwoo/fix_crt_s3_200_error_mp

Conversation

@alextwoods

@alextwoods alextwoods commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Handle 200 OK errors in PutObject (multipart) in S3 CRT client

Motivation and Context

Fixes: #7068

The CRT client already detects 200ok errors during MPU in PutObject, however, they are not correctly handled in our Java code.

The reason the normal handling of 200OK errors does not work for this case is that the 200OK error handling is normally only applied for non-streaming operations. PutObject is a streaming operation, but under the hood, the CRT may do a MPU, and complete multipart upload is not streaming and may respond with a 200 OK error.

Modifications

Wrap the SDK response object in a new ErrorFlaggedSdkHttpResponse which returns false for isSuccessful, causing the SDK's normal error parsing to be used.

Note, earlier PR #7071 does correctly raise an exception, but does not parse the code/message from the body. To avoid brining in xml parsing to the CRT handler, this PR wraps the sdk response object and relies on the existing error unmarshalling pipeline.

Testing

New and existing unit tests + ran repro from #7068 to confirm.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • [] My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.

License

  • I confirm that this pull request can be released under the Apache 2 license

@alextwoods alextwoods requested a review from a team as a code owner June 26, 2026 16:37
// 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());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Naming is hard - My other alternative here was something like "SdkHttpResponse200OkError" or something like that.


@BeforeAll
public static void setUpBeforeAll() {
System.setProperty("aws.crt.debugnative", "true");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CRT: client silently reports success for failed multipart uploads when S3 returns HTTP 200 with error body, causing undetectable data loss

3 participants