fix CopyObject swallowing errors embedded in a 200 OK response

[huanghaoyuanhhy]

Fixes #2305

Sets expect200OKWithError on CopyObject so executeMethod parses the body for an embedded instead of trusting the 200 status, matching what CompleteMultipartUpload and RemoveObjects already do.

Added Test200CopyObjectWithError, modeled on Test200MultipartUploadWithError: it fails before the change (CopyObject returns nil) and passes after, including the retry. Ran go test -short -race ./... and go build, both clean.

Summary by CodeRabbit

• Bug Fixes

  • Copy operations now correctly detect and report errors embedded in otherwise successful responses.
  • Failed copy requests are retried according to the configured retry policy.

• Tests

  • Added coverage verifying error handling and retry behavior for copy operations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2c5522fd-b257-420e-8fed-c0a442547d41

📥 Commits

Reviewing files that changed from the base of the PR and between 89b67d7 and 4ec64b4.

📒 Files selected for processing (1)

• api-copy-object.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

CopyObject now parses embedded S3 errors in 200 OK responses. A test verifies the returned error and three retries.

Changes

CopyObject embedded error handling

Layer / File(s)	Summary
Embedded error detection and retry validation api-copy-object.go, 200OKwithError_test.go	CopyObject sets expect200OKWithError for its request. The test verifies the returned InternalError and exactly three retries.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: klauspost

Merge Risk: ⚪ Minimal · up to 6c296

CopyObject now reports embedded S3 errors and retries them; no concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing CopyObject handling of errors embedded in a 200 OK response.
Linked Issues check	✅ Passed	The PR satisfies #2305. CopyObject now sets expect200OKWithError: true. The request layer can therefore parse an embedded <Error> document in an HTTP 200 response. Test200CopyObjectWithError v…
Out of Scope Changes check	✅ Passed	The changes stay within #2305. The production change enables existing 200-with-error handling for CopyObject. The test covers the linked failure mode and retries. No unrelated production or public A…

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit found an error tucked in green,
Behind a status bright and clean.
CopyObject now reads the hidden sign,
Retries three times in a steady line.
The failed copy no longer hides,
While carrots cheer from both sides.}

_{Comment @coderabbitai help to get the list of available commands.}

[klauspost]

Windows CI unrelated, fixed in #2307

[jiuker]

Please add this document URL https://repost.aws/knowledge-center/s3-resolve-200-internalerror in the comments, and there's no need for such a lengthy explanation.

[jiuker]

Code is fine. Just for the comments.

[jiuker]

LGTM