Keep the error body of a failed streaming request, and fix the three framing defects the review found (#14)

* fix(http): keep the error body of a failed streaming request

send() fills HttpResponse::body on every path including failures; send_stream()
was the one entry point that dropped it. A non-2xx answer to a streaming request
is an error document, not an event stream: SseParser finds no event boundary in
it, emits nothing, and the bytes stay in its private buffer. Callers were left
with a status line and no reason.

Capture the body when the status is not 2xx. Events are still parsed and
dispatched exactly as before, and nothing is copied on a 2xx stream, so the
success path is byte-identical.

The copy is bounded by stream_error_body_limit (1 MiB) so a server answering 5xx
with an endless body cannot grow the buffer without limit. Truncation lives in an
exported append_within_limit, in the same spirit as parse_chunk_size_line, with
three unit tests for under, across and past the limit; a live test against
httpbin's /status/418 covers the wiring.

* Honour a declared Content-Length in send_stream, and reject a chunk size rather than salvaging it

Review of the change this branch already carries. The defect it reports is real
and the fix is placed correctly --- `dispatch` is the single funnel for every
body byte on both framing paths, and `captureBody` is decided after the headers
are read, where the status is final. Measured against master, one program, one
source file:

    master   status=418 events=0 body.size()=0
    this     status=418 events=0 body.size()=135

What follows is what that fix could not do on its own.

--- 1. A DECLARED LENGTH, WHICH IS WHY THE TEST HAD TO CLOSE THE CONNECTION ----

`send_stream` had no branch for `Content-Length`: a response that was not
chunked was read until the connection closed, whatever its headers said. On this
library's own defaults --- `keepAlive = true`, so the request carries
`Connection: keep-alive` --- the server does not close, and the read loop ran
until `readTimeoutMs` expired. Measured against httpbin's `/status/418`:

    keepAlive = false   status=418 body=135   elapsed  1370 ms
    keepAlive = true    status=418 body=135   elapsed  9379 ms   (timeout 8000)

The error body arrived either way, and on the defaults it arrived a full read
timeout late --- sixty seconds, as the defaults stand. `send()` has had this
branch throughout, which is the same asymmetry between the two entry points that
this branch exists to remove.

The live test set `keepAlive = false`, "so the server closes and the read loop
ends". That comment was the defect, and the test was examining the one
arrangement in which it does not appear. It now runs on the defaults and asserts
the elapsed time.

    after: keepAlive = true    status=418 body=135   elapsed  1192 ms

--- 2. A CHUNK SIZE THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK ---------------

`parse_hex` returns what it accumulated when it meets a character it does not
recognise, and zero for an empty line --- and `read_line` returns an empty line
on a timeout or a closed connection. So a stream that was cut short read as a
stream that ended cleanly and this loop reported success. #9 established
`parse_chunk_size_line` for exactly this and it reached `download_to_file`
alone; `send` and `send_stream` were left on the old one.

--- 3. Content-Length WAS PARSED BY KEEPING THE DIGITS ------------------------

Measured, by compiling that parser on its own:

    "135"                  -> 135
    "abc"                  -> 0                     <- a refusal read as a real zero
    "12abc"                -> 12                    <- stops twelve bytes in
    "-1"                   -> 1                     <- the sign is discarded
    "99999999999999999999" -> 7766279631452241919   <- wraps, in silence

The last two are the ones no care at the call site could recover from, because
what it receives is a plausible number. `parse_content_length` is exported and
shaped like `parse_chunk_size_line`, for the reason #9 gave: it is the half of
the body framing that can be examined without a server. Both readers use it.

--- criteria -----------------------------------------------------------------

Six unit tests over the two pure parsers, and two live ones: the failed stream
now runs on the DEFAULT configuration with the elapsed time asserted, and a
chunked 2xx is asserted to leave `body` empty and to return promptly --- the
success path is the one this change restructured around, so it is observed
rather than assumed.

17 tests from 6 suites pass, plus 3 in test_resolver.

---------

Co-authored-by: Cloud_Yun <yunfeng66645@gmail.com>

Author: Sunrisepeak

mcpp.toml

@@ -1,7 +1,7 @@
 [package]
 namespace   = "mcpplibs"
 name        = "tinyhttps"
-version     = "0.2.9"
+version     = "0.2.10"
 description = "Minimal C++23 HTTP/HTTPS client with SSE streaming support"
 license     = "Apache-2.0"
 repo        = "https://github.com/mcpplibs/tinyhttps"

src/http.cppm

@@ -242,6 +242,37 @@ static int parse_hex(std::string_view s) {
     return result;
 }
 
+// `Content-Length`, rejected rather than salvaged.
+//
+// Both readers parsed this by walking the characters and keeping the digits, so
+// `12abc` was 12, `abc` was 0 — indistinguishable from a genuine `0` — and a
+// value past the width of the accumulator wrapped in silence. A length is the
+// number of bytes the reader will then trust, so a wrong one is not a cosmetic
+// error: too small leaves the next response's bytes in the stream, and too
+// large waits for bytes that are not coming.
+//
+// Shaped like `parse_chunk_size_line` above, and exported for the same reason:
+// it is the half of the body framing that can be examined without a server.
+export std::optional<std::int64_t>
+parse_content_length(std::string_view value) {
+    // A field value may carry optional whitespace on either side (RFC 9110).
+    while (!value.empty() && (value.front() == ' ' || value.front() == '\t'))
+        value.remove_prefix(1);
+    while (!value.empty() && (value.back() == ' ' || value.back() == '\t'))
+        value.remove_suffix(1);
+    if (value.empty()) return std::nullopt;
+
+    std::uint64_t parsed {};
+    auto [end, error] = std::from_chars(
+        value.data(), value.data() + value.size(), parsed, 10);
+    if (error != std::errc{} || end != value.data() + value.size()
+        || parsed > static_cast<std::uint64_t>(
+            std::numeric_limits<std::int64_t>::max())) {
+        return std::nullopt;
+    }
+    return static_cast<std::int64_t>(parsed);
+}
+
 export std::optional<std::int64_t>
 parse_chunk_size_line(std::string_view line) {
     if (line.empty()) return std::nullopt;
@@ -256,6 +287,25 @@ parse_chunk_size_line(std::string_view line) {
     return static_cast<std::int64_t>(value);
 }
 
+// A streaming request that fails is answered with an error document, not an
+// event stream, so SseParser finds no event boundary in it and yields nothing.
+// The bytes are still worth keeping: without them a caller can report the
+// status line but never the reason. Bounded, so a server answering 5xx with an
+// endless body cannot grow the buffer without limit.
+export inline constexpr std::size_t stream_error_body_limit = 1024 * 1024;
+
+// Appends as much of `data` as `limit` still allows. Returns false once the
+// buffer is full, so a caller can stop copying without tracking sizes itself.
+export bool append_within_limit(std::string& buffer, std::string_view data,
+                                std::size_t limit) {
+    if (buffer.size() >= limit) return false;
+    const std::size_t room = limit - buffer.size();
+    // Not std::min: <winsock2.h> defines a `min` macro on Windows.
+    const std::size_t take = data.size() < room ? data.size() : room;
+    buffer.append(data.substr(0, take));
+    return take == data.size();
+}
+
 // Case-insensitive string comparison
 static bool iequals(std::string_view a, std::string_view b) {
     if (a.size() != b.size()) return false;
@@ -459,12 +509,11 @@ private:
                     chunked = true;
                 }
                 if (iequals(key, "Content-Length")) {
-                    contentLength = 0;
-                    for (char c : valStr) {
-                        if (c >= '0' && c <= '9') {
-                            contentLength = contentLength * 10 + (c - '0');
-                        }
-                    }
+                    // Rejected rather than salvaged; parse_content_length says
+                    // why. A malformed value leaves this at -1, which is the
+                    // same state as an absent header and is a framing this
+                    // reader already handles.
+                    contentLength = parse_content_length(valStr).value_or(-1);
                 }
                 if (iequals(key, "Connection") && iequals(valStr, "close")) {
                     connectionClose = true;
@@ -704,6 +753,7 @@ public:
         // Read headers
         bool chunked = false;
         bool connectionClose = false;
+        std::int64_t contentLength = -1;
 
         while (true) {
             std::string headerLine = read_line(*sock, config_.readTimeoutMs);
@@ -726,14 +776,27 @@ public:
                 if (iequals(key, "Connection") && iequals(valStr, "close")) {
                     connectionClose = true;
                 }
+                // send() reads this and send_stream() did not, which is the
+                // same asymmetry this change exists to remove. See the body
+                // loop below for what its absence cost.
+                if (iequals(key, "Content-Length")) {
+                    contentLength = parse_content_length(valStr).value_or(-1);
+                }
             }
         }
 
         // Stream body incrementally, feeding chunks to SseParser
         SseParser parser;
         bool stopped = false;
+        // send() fills `body` on every path including failures; without this
+        // send_stream would be the one entry point that drops it. Capturing is
+        // additive — events are still parsed and dispatched exactly as before.
+        const bool captureBody = !response.ok();
 
         auto dispatch = [&](std::string_view data) -> bool {
+            if (captureBody) {
+                append_within_limit(response.body, data, stream_error_body_limit);
+            }
             auto events = parser.feed(data);
             for (const auto& ev : events) {
                 if (!callback(ev)) {
@@ -756,7 +819,22 @@ public:
                     sizeLine.pop_back();
                 }
 
-                int chunkSize = parse_hex(sizeLine);
+                // A CHUNK HEADER THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK.
+                //
+                // `parse_hex` returned what it had accumulated when it met a
+                // character it did not recognise, and zero for an empty line —
+                // and `read_line` returns an empty line on a timeout or a
+                // closed connection. So a stream that was cut short read as a
+                // stream that ended cleanly, and this loop reported success.
+                // #9 established `parse_chunk_size_line` for exactly this and
+                // it reached only `download_to_file`.
+                auto parsedChunkSize = parse_chunk_size_line(sizeLine);
+                if (!parsedChunkSize) {
+                    response.statusText = "Invalid chunk size: " + sizeLine;
+                    connectionClose = true;
+                    break;
+                }
+                int chunkSize = static_cast<int>(*parsedChunkSize);
                 if (chunkSize == 0) {
                     // Terminal chunk — read trailing \r\n
                     read_line(*sock, config_.readTimeoutMs);
@@ -775,8 +853,44 @@ public:
                     break;
                 }
             }
+        } else if (contentLength >= 0) {
+            // A DECLARED LENGTH IS READ AND THE READER THEN STOPS.
+            //
+            // This branch did not exist: a response that was not chunked was
+            // read until the connection closed, whatever its headers said. On
+            // the library's own defaults — `keepAlive = true`, so the request
+            // carries `Connection: keep-alive` — the server does not close,
+            // and the loop below ran until `readTimeoutMs` expired.
+            //
+            // Measured against httpbin's `/status/418`, which answers with a
+            // Content-Length and keeps the connection:
+            //
+            //     keepAlive = false   status=418 body=135   elapsed  1370 ms
+            //     keepAlive = true    status=418 body=135   elapsed  9379 ms   (readTimeoutMs = 8000)
+            //
+            // The error body arrived either way, and on the defaults it arrived
+            // a full read timeout late — sixty seconds, as the defaults stand.
+            // `send()` has had this branch throughout, which is why the same
+            // request through it returns at once.
+            std::int64_t remaining = contentLength;
+            char buf[4096];
+            while (!stopped && remaining > 0) {
+                if (!sock->wait_readable(config_.readTimeoutMs)) {
+                    break;
+                }
+                const auto want = static_cast<std::size_t>(
+                    remaining < static_cast<std::int64_t>(sizeof buf)
+                        ? remaining : static_cast<std::int64_t>(sizeof buf));
+                int ret = sock->read(buf, want);
+                if (ret <= 0) break;
+                remaining -= ret;
+                if (!dispatch(std::string_view(buf, static_cast<std::size_t>(ret)))) {
+                    break;
+                }
+            }
         } else {
-            // Not chunked — read until connection closes
+            // Neither chunked nor a declared length: the end of the body is the
+            // end of the connection, so the connection must not be reused.
             connectionClose = true;
             char buf[4096];
             while (!stopped) {

tests/test_download.cpp

@@ -20,6 +20,75 @@ TEST(ChunkedProtocol, AcceptsValidSizeAndTerminalChunk) {
     EXPECT_EQ(*https::parse_chunk_size_line("0"), 0);
 }
 
+// `Content-Length` decides how many bytes a reader will trust, so a wrong
+// answer is not cosmetic: too small leaves the next response's bytes in the
+// stream and too large waits for bytes that are not coming.
+//
+// Both readers used to keep the digits and discard everything else. Measured,
+// by compiling that parser on its own:
+//
+//     "135"                  -> 135
+//     "0"                    -> 0
+//     "abc"                  -> 0                     <- a refusal read as a real zero
+//     "12abc"                -> 12                    <- stops twelve bytes in
+//     ""                     -> 0
+//     "-1"                   -> 1                     <- the sign is discarded
+//     "99999999999999999999" -> 7766279631452241919   <- wraps, in silence
+//
+// The last two are the ones no amount of care at the call site could recover
+// from, because the value it receives is a plausible number.
+TEST(ContentLength, AcceptsAWellFormedValue) {
+    ASSERT_TRUE(https::parse_content_length("135").has_value());
+    EXPECT_EQ(*https::parse_content_length("135"), 135);
+    // A field value may carry optional whitespace on either side.
+    ASSERT_TRUE(https::parse_content_length("  135\t").has_value());
+    EXPECT_EQ(*https::parse_content_length("  135\t"), 135);
+}
+
+// The one a salvaging parser cannot express. `abc` used to yield 0, which is
+// indistinguishable from a server that genuinely declared an empty body — and
+// the two call for opposite behaviour.
+TEST(ContentLength, AZeroIsDistinguishableFromARefusal) {
+    ASSERT_TRUE(https::parse_content_length("0").has_value());
+    EXPECT_EQ(*https::parse_content_length("0"), 0);
+    EXPECT_FALSE(https::parse_content_length("abc").has_value());
+}
+
+TEST(ContentLength, RejectsEmptyTrailingGarbageAndOverflow) {
+    EXPECT_FALSE(https::parse_content_length("").has_value());
+    EXPECT_FALSE(https::parse_content_length("   ").has_value());
+    // `12abc` used to be 12: the reader would then stop twelve bytes in and
+    // leave the rest of the body to be read as the next response.
+    EXPECT_FALSE(https::parse_content_length("12abc").has_value());
+    EXPECT_FALSE(https::parse_content_length("-1").has_value());
+    EXPECT_FALSE(https::parse_content_length("+1").has_value());
+    // Past the width of the accumulator, which used to wrap in silence.
+    EXPECT_FALSE(https::parse_content_length("99999999999999999999").has_value());
+}
+
+TEST(StreamErrorBody, KeepsEverythingWhileUnderLimit) {
+    std::string buffer;
+    EXPECT_TRUE(https::append_within_limit(buffer, "abc", 8));
+    EXPECT_TRUE(https::append_within_limit(buffer, "de", 8));
+    EXPECT_EQ(buffer, "abcde");
+}
+
+TEST(StreamErrorBody, TruncatesTheChunkThatCrossesTheLimit) {
+    std::string buffer = "abc";
+    EXPECT_FALSE(https::append_within_limit(buffer, "defgh", 5));
+    EXPECT_EQ(buffer, "abcde");
+}
+
+TEST(StreamErrorBody, RefusesFurtherDataOnceFull) {
+    std::string buffer = "abcde";
+    EXPECT_FALSE(https::append_within_limit(buffer, "f", 5));
+    EXPECT_EQ(buffer, "abcde");
+    // A zero limit must not append anything, not even an empty append.
+    std::string empty;
+    EXPECT_FALSE(https::append_within_limit(empty, "a", 0));
+    EXPECT_TRUE(empty.empty());
+}
+
 TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
     https::DownloadToFileResult result;
     result.bytesWritten = 42;
@@ -36,6 +105,86 @@ TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
     EXPECT_FALSE(result.lastModified.empty());
 }
 
+// Test that a failed streaming request carries its error body, against a real
+// HTTPS endpoint. httpbin's /status/418 answers non-2xx with a body, which is
+// exactly the shape SseParser cannot turn into events.
+
+class StreamErrorBodyLiveTest : public ::testing::Test {
+protected:
+    void SetUp() override { https::Socket::platform_init(); }
+};
+
+// ON THE LIBRARY'S OWN DEFAULTS, AND THE READ TIMEOUT IS PART OF THE
+// OBSERVATION RATHER THAN A SAFETY NET.
+//
+// The first form of this test set `keepAlive = false`, "so the server closes
+// and the read loop ends". That comment was the defect: `send_stream` had no
+// branch for a declared `Content-Length`, so a response that was not chunked
+// was read until the connection closed — and on the defaults the server does
+// not close. The body arrived, one full `readTimeoutMs` late.
+//
+//     keepAlive = false   status=418 body=135   elapsed  1370 ms
+//     keepAlive = true    status=418 body=135   elapsed  9379 ms   (timeout 8000)
+//
+// So the configuration under test is the default one, and the elapsed time is
+// asserted. A test that closed the connection to make the loop end would be
+// examining the one arrangement in which the defect does not appear.
+TEST_F(StreamErrorBodyLiveTest, FailedStreamKeepsTheErrorBody) {
+    https::HttpClientConfig cfg;
+    cfg.connectTimeoutMs = 15000;
+    cfg.readTimeoutMs = 30000;
+    // keepAlive is left at its default, which is true.
+    https::HttpClient client(cfg);
+
+    https::HttpRequest req;
+    req.method = https::Method::GET;
+    req.url = "https://httpbin.org/status/418";
+
+    int events = 0;
+    const auto started = std::chrono::steady_clock::now();
+    auto res = client.send_stream(req, [&](const https::SseEvent&) {
+        ++events;
+        return true;
+    });
+    const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
+        std::chrono::steady_clock::now() - started).count();
+
+    EXPECT_EQ(res.statusCode, 418);
+    EXPECT_EQ(events, 0) << "an error document is not an event stream";
+    EXPECT_FALSE(res.body.empty()) << "error body was dropped";
+    EXPECT_NE(res.body.find("teapot"), std::string::npos);
+    // Generous against a slow runner and still an order of magnitude below the
+    // read timeout, which is what the defect consumed.
+    EXPECT_LT(elapsed, 15000)
+        << "the body arrived after " << elapsed
+        << " ms; a declared Content-Length was not honoured and the reader "
+           "waited for a close that keep-alive was never going to bring";
+}
+
+// The success path, on the framing an event stream actually uses. A 2xx is not
+// captured, so `body` stays empty and the bytes reach the parser — and it must
+// still return promptly, since the chunked branch is the one this change did
+// not restructure.
+TEST_F(StreamErrorBodyLiveTest, AChunkedSuccessIsUnchangedAndPrompt) {
+    https::HttpClientConfig cfg;
+    cfg.connectTimeoutMs = 15000;
+    cfg.readTimeoutMs = 30000;
+    https::HttpClient client(cfg);
+
+    https::HttpRequest req;
+    req.method = https::Method::GET;
+    req.url = "https://httpbin.org/stream/3";
+
+    const auto started = std::chrono::steady_clock::now();
+    auto res = client.send_stream(req, [](const https::SseEvent&) { return true; });
+    const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
+        std::chrono::steady_clock::now() - started).count();
+
+    EXPECT_EQ(res.statusCode, 200);
+    EXPECT_TRUE(res.body.empty()) << "a 2xx body is not captured; it is the caller's stream";
+    EXPECT_LT(elapsed, 15000) << "the chunked reader did not terminate promptly";
+}
+
 // Test download_to_file against a real HTTPS endpoint.
 // Uses httpbin.org which returns known-size responses.