Expand checks for S3 200 errors to more operations - #3493
Open
Madrigal wants to merge 8 commits into
Open
Conversation
Madrigal
marked this pull request as draft
July 24, 2026 21:57
lucix-aws
approved these changes
Jul 30, 2026
Madrigal
marked this pull request as ready for review
July 30, 2026 17:30
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expand S3 200-with-error handling to all qualifying operations
Motivation
We have a customization to identify when s3 returns a response with a 200 status code but it actually contains an error. We in Go had it just for a handful of operation, but internal specification mentions that this can happen in way more places. Changed our codegen to expand on which operations we add this check.
Changes
Codegen (
S3ErrorWith200Status.java): Replaced the hardcoded 3-operationallowlist with the Smithy selector from the internal specification. The
middleware is now registered on all S3 operations whose output does NOT contain
an
@httpPayloadmember targeting a@streamingshape or astring. Thismatches the selector:
Runtime (
handle_200_error.go): Updated the empty-body case to pass throughinstead of returning a deserialization error. Per the specification's "Response
parsing runtime safety" requirement, the SDK MUST NOT fail the request if parsing
fails — it must continue regular response handling. This was necessary because
many S3 operations (DELETE operations, WriteGetObjectResponse, etc.) can
legitimately return empty 200 responses.
Test (
handle_200_error_test.go): Updated the200NoResponsetest case toexpect success on an empty body, matching the new behavior.
Backwards compatibility
This change may surface errors that were previously silently swallowed. For
operations beyond the original 3, a 200 response containing an
<Error>XMLroot element will now be treated as an error and retried (if retryable) rather
than being deserialized as a successful response. In practice:
S3 operations. Previously, only CopyObject, CompleteMultipartUpload, and
UploadPartCopy retried them. This is a correctness improvement — customers
were silently receiving error bodies treated as success on other operations.
that return empty bodies (e.g. DELETE, PUT with no response content) are
unaffected.
(GetObject, SelectObjectContent, GetBucketPolicy) are excluded by the
codegen filter, so their behavior is unchanged.
The net effect is that customers who previously received silent data corruption
(error XML deserialized as a successful response) will now correctly receive an
error or benefit from automatic retry.