Skip to content

Add S3ErrorCode to S3BlobStore #12979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
spraza wants to merge 3 commits into from
Closed

Add S3ErrorCode to S3BlobStore #12979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a2829e8
Add S3ErrorCode to S3BlobStore
spraza Apr 10, 2026
43c8cb1
add definitions to fix compiltion failures
spraza Apr 13, 2026
5f16bf0
Merge branch 'release-7.3' into backup-observability-v1
spraza Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions fdbclient/S3BlobStore.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,36 @@ std::string awsCanonicalURI(const std::string& resource, std::vector<std::string
return canonicalURI;
}

// ref: https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html
std::string parseErrorCodeFromS3(std::string xmlResponse) {
// Copy XML string to a modifiable buffer
try {
std::vector<char> xmlBuffer(xmlResponse.begin(), xmlResponse.end());
xmlBuffer.push_back('\0'); // Ensure null-terminated string
// Parse the XML
xml_document<> doc;
doc.parse<0>(&xmlBuffer[0]);
// Find the root node
xml_node<>* root = doc.first_node("Error");
if (!root) {
TraceEvent(SevWarn, "ParseS3XMLResponseNoError").detail("Response", xmlResponse).log();
return "";
}
// Find the <Code> node
xml_node<>* codeNode = root->first_node("Code");
if (!codeNode) {
TraceEvent(SevWarn, "ParseS3XMLResponseNoErrorCode").detail("Response", xmlResponse).log();
return "";
}
return std::string(codeNode->value());
} catch (Error e) {
TraceEvent("BackupParseS3ErrorCodeFailure").errorUnsuppressed(e);
throw backup_parse_s3_response_failure();

@gxglass gxglass Apr 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is problematic per the AI. It changes the retry behavior of the caller in a way that is different from today.

if we can't parse the XML just return an empty "" as per various intermediate failures above.

} catch (...) {
throw backup_parse_s3_response_failure();
}
}

// Do a request, get a Response.
// Request content is provided as UnsentPacketQueue *pContent which will be depleted as bytes are sent but the queue
// itself must live for the life of this actor and be destroyed by the caller
Expand Down Expand Up @@ -891,6 +921,7 @@ ACTOR Future<Reference<HTTP::IncomingResponse>> doRequest_impl(Reference<S3BlobS

state int maxTries = std::min(bstore->knobs.request_tries, bstore->knobs.connect_tries);
state int thisTry = 1;
state int badRequestCode = 400;
state double nextRetryDelay = 2.0;

loop {
Expand Down Expand Up @@ -1031,6 +1062,14 @@ ACTOR Future<Reference<HTTP::IncomingResponse>> doRequest_impl(Reference<S3BlobS
event.suppressFor(60);
if (!err.present()) {
event.detail("ResponseCode", r->code);
std::string s3Error = parseErrorCodeFromS3(r->data.content);

@gxglass gxglass Apr 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A really simple alternative here would be to say

event.detail("Details", r->data.content);

event.detail("S3ErrorCode", s3Error);
if (r->code == badRequestCode) {
TraceEvent(SevWarnAlways, "S3BlobStoreBadRequest")
.detail("HttpCode", r->code)
.detail("HttpResponseContent", r->data.content)
.detail("S3Error", s3Error);
}
}

event.detail("ConnectionEstablished", connectionEstablished);
Expand Down
1 change: 1 addition & 0 deletions flow/include/flow/error_definitions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ ERROR( backup_auth_unreadable, 2318, "Cannot read or parse one or more sources o
ERROR( backup_does_not_exist, 2319, "Backup does not exist")
ERROR( backup_not_filterable_with_key_ranges, 2320, "Backup before 6.3 cannot be filtered with key ranges")
ERROR( backup_not_overlapped_with_keys_filter, 2321, "Backup key ranges doesn't overlap with key ranges filter")
ERROR( backup_parse_s3_response_failure, 2322, "cannot parse s3 response properly" )

@spraza spraza Apr 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@saintstack in #11965, the code was 2323, because 2322 was used by another error not in this PR. Over here, I used 2322. Please review this part too.

@saintstack saintstack Apr 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok. still +1

ERROR( restore_invalid_version, 2361, "Invalid restore version")
ERROR( restore_corrupted_data, 2362, "Corrupted backup data")
ERROR( restore_missing_data, 2363, "Missing backup data")
Expand Down