Re-enable TCP keep-alive and handle S3's XML errors#463
Conversation
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.
S3 returns transient errors as HTTP 400/503 inside XML. Without parsing these errors are treated as non-retryable.
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
📝 WalkthroughWalkthroughThis PR improves file transfer robustness by configuring TCP keepalive for better connection monitoring and implementing S3-specific error code parsing to enable smarter retry classification based on the error response body. ChangesFile Transfer Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libstore/filetransfer.cc`:
- Around line 711-718: Replace the non-portable ECMAScript regex in
s3ErrorCodeRegex used to parse S3 XML error bodies: the current pattern uses
[^]* which is unsupported and can throw std::regex_error; change it to use a
portable equivalent that matches any char including newlines (e.g., use [\s\S]*
in the ECMAScript pattern) so the regex in s3ErrorCodeRegex (and its use with
std::regex_search on errorSink->s) works across libstdc++ and libc++.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3af6cd2f-7ee5-47d4-80bb-423227110dfd
📒 Files selected for processing (1)
src/libstore/filetransfer.cc
| // S3 error responses have the form <Error><Code>...</Code>...</Error>. | ||
| // Require the <Error> root to avoid matching unrelated XML with a <Code> element. | ||
| static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>"); | ||
| std::smatch s3Match; | ||
| bool isS3XmlStatus = httpStatus == 400 || httpStatus == 403 || httpStatus == 500 || httpStatus == 503; | ||
| auto s3ErrorCode = | ||
| (isS3XmlStatus && errorSink && std::regex_search(errorSink->s, s3Match, s3ErrorCodeRegex)) | ||
| ? s3Match[1].str() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C++ std::regex with ECMAScript grammar, is the pattern [^]*valid and portable across libstdc++ and libc++? If not, what portable pattern should be used to match any characters (including newlines) between<Error...>and?
💡 Result:
In C++ std::regex using the ECMAScript grammar, the pattern [^]* is not portable and is generally considered invalid or unreliable across different standard library implementations [1]. While some regex engines may interpret it as a character class containing nothing (which is logically invalid) or handle it as a non-greedy match, it is not standard-compliant behavior to rely on this syntax for matching any character [2][3]. To match any character, including newlines, the most portable and idiomatic pattern in C++ std::regex is [\s\S]. Explanation: 1. The dot (.) character in ECMAScript regex does not match line terminators (such as \n or \r) [4][5]. 2. The character class [\s\S] explicitly matches any whitespace character (\s) or any non-whitespace character (\S), which together cover the entire set of possible characters, including all newlines [5][6]. 3. Using [\s\S] (or [\s\S]+ for one or more) is the standard workaround for the lack of a "dot-all" flag in C++ std::regex [5][6]. For your specific use case of matching content between <Error...> and , you can use a non-greedy pattern: <Error[\s\S]*? This will match the literal <Error, followed by the shortest possible sequence of any characters (including newlines), ending at the first occurrence of .
Citations:
- 1: https://stackoverflow.com/questions/23484952/is-0-0-in-a-c-style-regex-string-a-valid-escape-sequence-in-c-regular-e
- 2: https://en.cppreference.com/cpp/regex/ecmascript
- 3: https://cppreference.dev/w/cpp/regex/ecmascript
- 4: https://www.cplusplus.com/reference/regex/ECMAScript
- 5: https://github.com/tc39/proposal-regexp-dotall-flag
- 6: https://dev.to/gaspricecheck/why-your-regex-is-silently-breaking-on-react-ssr-output-2h
🏁 Script executed:
cd src/libstore && grep -n "s3ErrorCodeRegex" filetransfer.cc
Repository: DeterminateSystems/nix-src
Length of output: 276
Fix portable ECMAScript regex for S3 XML error parsing.
The pattern [^]* at line 713 is not portable across C++ standard library implementations (libstdc++ and libc++) and can trigger std::regex_error. The portable approach is to use [\s\S]* to match any character including newlines in ECMAScript regex.
Proposed fix
- static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>");
+ static const std::regex s3ErrorCodeRegex(
+ R"(<Error\b[^>]*>[\s\S]*?<Code>([^<]+)</Code>)",
+ std::regex::ECMAScript);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
// S3 error responses have the form <Error><Code>...</Code>...</Error>.
// Require the <Error> root to avoid matching unrelated XML with a <Code> element.
static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>");
std::smatch s3Match;
bool isS3XmlStatus = httpStatus == 400 || httpStatus == 403 || httpStatus == 500 || httpStatus == 503;
auto s3ErrorCode =
(isS3XmlStatus && errorSink && std::regex_search(errorSink->s, s3Match, s3ErrorCodeRegex))
? s3Match[1].str()
// S3 error responses have the form <Error><Code>...</Code>...</Error>.
// Require the <Error> root to avoid matching unrelated XML with a <Code> element.
static const std::regex s3ErrorCodeRegex(
R"(<Error\b[^>]*>[\s\S]*?<Code>([^<]+)</Code>)",
std::regex::ECMAScript);
std::smatch s3Match;
bool isS3XmlStatus = httpStatus == 400 || httpStatus == 403 || httpStatus == 500 || httpStatus == 503;
auto s3ErrorCode =
(isS3XmlStatus && errorSink && std::regex_search(errorSink->s, s3Match, s3ErrorCodeRegex))
? s3Match[1].str()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libstore/filetransfer.cc` around lines 711 - 718, Replace the
non-portable ECMAScript regex in s3ErrorCodeRegex used to parse S3 XML error
bodies: the current pattern uses [^]* which is unsupported and can throw
std::regex_error; change it to use a portable equivalent that matches any char
including newlines (e.g., use [\s\S]* in the ECMAScript pattern) so the regex in
s3ErrorCodeRegex (and its use with std::regex_search on errorSink->s) works
across libstdc++ and libc++.
There was a problem hiding this comment.
@tomberek Might be worth looking into this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I compiled a small test program that constructed std::regex foo("[^]"); and it compiled and ran just fine. I think the robit is wrong (or at least not right in a way we care about here).
EDIT: on both linux and darwin
| }}; | ||
| // S3 error responses have the form <Error><Code>...</Code>...</Error>. | ||
| // Require the <Error> root to avoid matching unrelated XML with a <Code> element. | ||
| static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>"); |
There was a problem hiding this comment.
Woah, I've never seen [^]* used like that........ that's actually blowing my mind.
| }}; | ||
| // S3 error responses have the form <Error><Code>...</Code>...</Error>. | ||
| // Require the <Error> root to avoid matching unrelated XML with a <Code> element. | ||
| static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>"); |
There was a problem hiding this comment.
I am a tiny bit worried about:
- applying this regex on ALL 400/403/500/503 requests (that seems like it could be bad-ish for performance?) -- is there any way we could sniff headers or something to see if it's S3-like before regex'ing the request body?
- the lack of tests around this behavior -- even just unit tests validating the transition from some sample XML to the retryable error code would be swell...
There was a problem hiding this comment.
The upstream PR has a test but it didn't apply cleanly so I left it out.
| // S3 error responses have the form <Error><Code>...</Code>...</Error>. | ||
| // Require the <Error> root to avoid matching unrelated XML with a <Code> element. | ||
| static std::regex s3ErrorCodeRegex("<Error>[^]*<Code>([^<]+)</Code>"); | ||
| std::smatch s3Match; | ||
| bool isS3XmlStatus = httpStatus == 400 || httpStatus == 403 || httpStatus == 500 || httpStatus == 503; | ||
| auto s3ErrorCode = | ||
| (isS3XmlStatus && errorSink && std::regex_search(errorSink->s, s3Match, s3ErrorCodeRegex)) | ||
| ? s3Match[1].str() |
There was a problem hiding this comment.
I compiled a small test program that constructed std::regex foo("[^]"); and it compiled and ran just fine. I think the robit is wrong (or at least not right in a way we care about here).
EDIT: on both linux and darwin
|
Another thing to check is Darwin... there are regex differences that have crept in in the past. |
4 participants
Motivation
Upstream NixOS#15855.
Context
Summary by CodeRabbit