From 8e1807cdbe08a6e059ff79220247cf4b3c058979 Mon Sep 17 00:00:00 2001
From: David Steele <dwsteele@users.noreply.github.com>
Date: Thu, 8 Jul 2021 13:06:52 -0400
Subject: [PATCH] Detect errors in S3 multi-part upload finalize.

Multi-part upload may fail despite returning an HTTP success code. Check for the ETag field in the result and if not present consider the upload to have failed. This will trigger a retry at the local job level.
---
 doc/xml/release.xml              | 20 ++++++++++
 src/storage/s3/write.c           | 10 ++++-
 test/src/module/storage/s3Test.c | 66 +++++++++++++++++++++++++++++++-
 3 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/doc/xml/release.xml b/doc/xml/release.xml
index 49d6aacd8..1f8a7a1e9 100644
--- a/doc/xml/release.xml
+++ b/doc/xml/release.xml
@@ -15,6 +15,21 @@
         <release date="XXXX-XX-XX" version="2.35dev" title="UNDER DEVELOPMENT">
             <release-core-list>
                 <release-bug-list>
+                    <release-item>
+                        <github-issue id="1116"/>
+                        <github-issue id="1433"/>
+                        <github-pull-request id="1438"/>
+
+                        <release-item-contributor-list>
+                            <release-item-ideator id="marco.montagna"/>
+                            <release-item-ideator id="lev.kokotov"/>
+                            <release-item-contributor id="david.steele"/>
+                            <release-item-reviewer id="cynthia.shang"/>
+                        </release-item-contributor-list>
+
+                        <p>Detect errors in <proper>S3</proper> multi-part upload finalize.</p>
+                    </release-item>
+
                     <release-item>
                         <github-issue id="1447"/>
                         <github-pull-request id="1452"/>
@@ -10172,6 +10187,11 @@
             <contributor-id type="github">marco44</contributor-id>
         </contributor>
 
+        <contributor id="marco.montagna">
+            <contributor-name-display>Marco Montagna</contributor-name-display>
+            <contributor-id type="github">mmontagna</contributor-id>
+        </contributor>
+
         <contributor id="markus.nullmeier">
             <contributor-name-display>Markus Nullmeier</contributor-name-display>
             <contributor-id type="github">mnullmei</contributor-id>
diff --git a/src/storage/s3/write.c b/src/storage/s3/write.c
index 4662e9051..ea79e8c46 100644
--- a/src/storage/s3/write.c
+++ b/src/storage/s3/write.c
@@ -235,10 +235,18 @@ storageWriteS3Close(THIS_VOID)
                 }
 
                 // Finalize the multi-part upload
-                storageS3RequestP(
+                HttpRequest *request = storageS3RequestAsyncP(
                     this->storage, HTTP_VERB_POST_STR, this->interface.name,
                     .query = httpQueryAdd(httpQueryNewP(), S3_QUERY_UPLOAD_ID_STR, this->uploadId),
                     .content = xmlDocumentBuf(partList));
+                HttpResponse *response = storageS3ResponseP(request);
+
+                // Error if there is no etag in the result. This indicates that the request did not succeed despite the success code.
+                if (xmlNodeChild(
+                        xmlDocumentRoot(xmlDocumentNewBuf(httpResponseContent(response))), S3_XML_TAG_ETAG_STR, false) == NULL)
+                {
+                    httpRequestError(request, response);
+                }
             }
             // Else upload all the data in a single put
             else
diff --git a/test/src/module/storage/s3Test.c b/test/src/module/storage/s3Test.c
index b83232ca1..b7281ea8a 100644
--- a/test/src/module/storage/s3Test.c
+++ b/test/src/module/storage/s3Test.c
@@ -686,11 +686,69 @@ testRun(void)
                         "<Part><PartNumber>1</PartNumber><ETag>WxRt1</ETag></Part>"
                         "<Part><PartNumber>2</PartNumber><ETag>WxRt2</ETag></Part>"
                         "</CompleteMultipartUpload>\n");
-                testResponseP(service);
+                testResponseP(
+                    service,
+                    .content =
+                        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                        "<CompleteMultipartUploadResult><ETag>XXX</ETag></CompleteMultipartUploadResult>");
 
                 TEST_ASSIGN(write, storageNewWriteP(s3, STRDEF("file.txt")), "new write");
                 TEST_RESULT_VOID(storagePutP(write, BUFSTRDEF("12345678901234567890123456789012")), "write");
 
+                // -----------------------------------------------------------------------------------------------------------------
+                TEST_TITLE("error in success response of multipart upload");
+
+                testRequestP(service, s3, HTTP_VERB_POST, "/file.txt?uploads=");
+                testResponseP(
+                    service,
+                    .content =
+                        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                        "<InitiateMultipartUploadResult xmlns=\"http://s3.amazonaws.com/doc/2006-03-01/\">"
+                        "<Bucket>bucket</Bucket>"
+                        "<Key>file.txt</Key>"
+                        "<UploadId>WxRt</UploadId>"
+                        "</InitiateMultipartUploadResult>");
+
+                testRequestP(service, s3, HTTP_VERB_PUT, "/file.txt?partNumber=1&uploadId=WxRt", .content = "1234567890123456");
+                testResponseP(service, .header = "etag:WxRt1");
+
+                testRequestP(service, s3, HTTP_VERB_PUT, "/file.txt?partNumber=2&uploadId=WxRt", .content = "7890123456789012");
+                testResponseP(service, .header = "eTag:WxRt2");
+
+                testRequestP(
+                    service, s3, HTTP_VERB_POST, "/file.txt?uploadId=WxRt",
+                    .content =
+                        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+                        "<CompleteMultipartUpload>"
+                        "<Part><PartNumber>1</PartNumber><ETag>WxRt1</ETag></Part>"
+                        "<Part><PartNumber>2</PartNumber><ETag>WxRt2</ETag></Part>"
+                        "</CompleteMultipartUpload>\n");
+                testResponseP(
+                    service,
+                    .content =
+                        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                        "<Error><Code>AccessDenied</Code><Message>Access Denied</Message></Error>");
+
+                TEST_ASSIGN(write, storageNewWriteP(s3, STRDEF("file.txt")), "new write");
+                TEST_ERROR(
+                    storagePutP(write, BUFSTRDEF("12345678901234567890123456789012")), ProtocolError,
+                    "HTTP request failed with 200 (OK):\n"
+                    "*** Path/Query ***:\n"
+                    "/file.txt?uploadId=WxRt\n"
+                    "*** Request Headers ***:\n"
+                    "authorization: <redacted>\n"
+                    "content-length: 205\n"
+                    "content-md5: 37smUM6Ah2/EjZbp420dPw==\n"
+                    "host: bucket.s3.amazonaws.com\n"
+                    "x-amz-content-sha256: 0838a79dfbddc2128d28fb4fa8d605e0a8e6d1355094000f39b6eb3feff4641f\n"
+                    "x-amz-date: <redacted>\n"
+                    "x-amz-security-token: <redacted>\n"
+                    "*** Response Headers ***:\n"
+                    "content-length: 110\n"
+                        "*** Response Content ***:\n"
+                    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                        "<Error><Code>AccessDenied</Code><Message>Access Denied</Message></Error>");
+
                 // -----------------------------------------------------------------------------------------------------------------
                 TEST_TITLE("write file in chunks with something left over on close");
 
@@ -719,7 +777,11 @@ testRun(void)
                         "<Part><PartNumber>1</PartNumber><ETag>RR551</ETag></Part>"
                         "<Part><PartNumber>2</PartNumber><ETag>RR552</ETag></Part>"
                         "</CompleteMultipartUpload>\n");
-                testResponseP(service);
+                testResponseP(
+                    service,
+                    .content =
+                        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                        "<CompleteMultipartUploadResult><ETag>XXX</ETag></CompleteMultipartUploadResult>");
 
                 TEST_ASSIGN(write, storageNewWriteP(s3, STRDEF("file.txt")), "new write");
                 TEST_RESULT_VOID(storagePutP(write, BUFSTRDEF("12345678901234567890")), "write");