fix(libstore/filetransfer): re-enable TCP keep-alive and handle S3's XML errors - #15855
Conversation
|
We should first implement retries for the S3 400 socket closed errors though. |
|
Yes, there are errors embedded in xml that we need to handle, otherwise this change will break hydra again... |
|
I can start working on the xml error handling, but the idea was that the 60 -> 30s change fixes what was encountered in Hydra. |
This allows curl to detect drops from both a server (eg: S3) as well as intermediate networking. 30s is to be shorter than commonly used 60s limits on the server side.
51644aa to
fcf04ab
Compare
|
Updated and rebased with S3 error handling (so far just regex like is done in s3-binary-cache-store.cc, not with full XML parsing). Error codes from https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList |
S3 returns transient errors as HTTP 400/503 inside XML. Without parsing these errors are treated as non-retryable.
fcf04ab to
631c492
Compare
tomberek
changed the title
| // S3 returns certain retryable errors as HTTP 400/500/503 with XML error codes. | ||
| // These take precedence over the generic HTTP status handling below. | ||
| // Only parse the response body on status codes where S3 XML errors can appear. | ||
| static constexpr std::array<std::string_view, 12> s3RetryableErrors{{ |
There was a problem hiding this comment.
@tomberek don't know how much of a difference it makes, but saves some heap allocation.
There was a problem hiding this comment.
Also just de-compiled because I was interested, and it seems that it also generates less code for my variant.
| "SlowDown", // HTTP 503 - throttling | ||
| "ServiceUnavailable", // HTTP 503 - temporary unavailability | ||
| "Throttling", // HTTP 400 - throttling variant | ||
| "ThrottledException", // HTTP 400 - throttling variant |
There was a problem hiding this comment.
I found a few more errors in the aws-cpp sdk (see commit for link) that are generally re-tried.
The original set only covered 5 error codes. The AWS C++ SDK treats additional error codes as retryable, including throttling variants (Throttling, ThrottledException, RequestThrottled), internal error aliases (InternalFailure, InternalServerError), and clock-related errors (RequestExpired, RequestTimeTooSkewed). Also use constexpr std::array<std::string_view> instead of std::set<std::string> to avoid heap allocation. Reference: https://github.com/aws/aws-sdk-cpp/blob/3d8614fbd6d2/src/aws-cpp-sdk-core/source/client/CoreErrors.cpp#L27-L55
919d49f to
e3f58bc
Compare
tomberek
added
the
backport 2.34-maintenance
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.34-maintenance
git worktree add -d .worktree/backport-15855-to-2.34-maintenance origin/2.34-maintenance
cd .worktree/backport-15855-to-2.34-maintenance
git switch --create backport-15855-to-2.34-maintenance
git cherry-pick -x 534d3760833283d249f220ac86d5ea8f7f13be8e 631c492f10680808573fe13ba57e937af509edb9 e3f58bc32f5ba830c3ac5ed123fc8dd695fcc204 378753a040c9582f9e2b1c39569f094c1c44f0bf |
4 participants
This allows curl to detect drops from both a server (eg: S3) as well as intermediate networking. 30s is to be shorter than commonly used 60s limits on the server side. The theory is that 60s timeout on both server and client led to races that were sometimes lost.
un-reverts: #15522
Motivation
Had connections that died and without keepalive they are not detected for a long time.