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

[yspbwx2010]

Fixes #11.

send() fills HttpResponse::body on every path including failures; send_stream()
was the one entry point that dropped it, because the whole body went into SseParser
and a non-2xx answer is an error document rather than an event stream — no \n\n
boundary, no events, bytes stranded in the parser's private buffer.

This captures the body when the status is not 2xx:

• Additive only. Events are still parsed and dispatched exactly as before; on a
  2xx stream nothing is copied and the success path is byte-identical.
• Bounded. stream_error_body_limit (1 MiB) caps the copy so a server answering
  5xx with an endless body can't grow the buffer without limit. Happy to make it a
  HttpClientConfig field instead if you'd rather it be tunable.
• Tested. The truncation logic is in an exported append_within_limit, in the
  same spirit as parse_chunk_size_line from fix(http): reject incomplete chunked transfers #9 — three unit tests cover under,
  across and past the limit. The wiring (captureBody = !response.ok()) is covered
  by a live test against httpbin's /status/418, which answers non-2xx with a body
  — exactly the shape SseParser cannot turn into events. Commenting out the capture
  line makes that one test fail with error body was dropped, and nothing else.

All 13 tests pass locally (gcc 16.1.0, x86_64-linux-gnu).

version bumped to 0.2.10; drop that hunk if you'd rather bump on release.

[Sunrisepeak]

Reviewed against the source and by measurement. The defect is real, the diagnosis is exact, and the fix is placed correctly. Merging this. What follows is what the review found beyond it — three of them are being sent as a follow-up rather than asked of you, because I have no push access to your fork.

The report holds

One program, one source file, built against master and against this branch, target/ wiped between them so nothing stale could answer:

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

events=0 is worth stating separately, because it confirms the mechanism rather than just the symptom: httpbin's teapot document does contain a blank line, so SseParser does find a boundary — and the block it extracts has no data:, event: or id: field, so dispatch_event_ returns without pushing. The bytes really were reaching the parser and dying there.

The fix is in the right place

dispatch is the single funnel for every body byte on both framing paths, so capturing there is the one change that covers chunked and non-chunked alike. captureBody is decided after the headers are read, where statusCode is final, and send_stream follows no redirects, so it cannot be computed against a status that later changes. Capturing at dispatch is also better than the alternative the issue floated — reaching into SseParser's buffer — because the parser's residual is not the whole body once any event has been dispatched.

append_within_limit is correct on the boundary cases, and exporting it to test it is the right precedent to follow from #9.

1. A declared Content-Length, which is why your test had to close the connection

This is the one that matters most, and your own test contains the evidence:

cfg.keepAlive = false;  // so the server closes and the read loop ends

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 defaults (keepAlive = true, so the request carries Connection: keep-alive) the server does not close, and the loop runs until readTimeoutMs expires. Measured against /status/418:

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

The body arrives either way — your fix works — but on the configuration a caller actually gets, it arrives a full read timeout late. With the shipped default of readTimeoutMs = 60000, that is a minute.

send() has had this branch throughout, which is the same asymmetry between the two entry points your issue is about. So the comment quoted above was not a test detail; it was the defect, and the test was examining the one arrangement in which it does not appear.

After adding the branch: keepAlive = true → 1192 ms.

2. A chunk size that does not parse is not a terminal chunk

send_stream's chunked loop still calls parse_hex, which returns what it accumulated when it meets a character it does not recognise, and 0 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 reads as a stream that ended cleanly, and the loop reports success.

#9 established parse_chunk_size_line for exactly this, and it reached download_to_file alone: send (line 511) and send_stream (785) were left on the old parser. That is the same shape as the asymmetry you reported, one layer down.

3. Content-Length was parsed by keeping the digits

Both readers walked the characters and kept the digits. Measured, by compiling that parser on its own:

"135"                  -> 135
"abc"                  -> 0                     <- a refusal, read as a real zero
"12abc"                -> 12                    <- the reader 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. A parse_content_length shaped like parse_chunk_size_line — exported, so it can be examined without a server — now serves both readers, with six unit tests over the two parsers.

And on openkal, since this library is one of the consumers there

tinyhttps builds and runs on openkal-musl: statically linked, zero PT_INTERP, TLS and DNS working, and your fix behaves identically there (body.size()=135).

One thing worth knowing, because it is invisible from inside this library. Socket::wait_readable is poll_fd, and on openkal-linux before 0.7.1 the bounded wait was performed on the descriptor below the one it then transferred on. The consequence was not a failure — it was that readTimeoutMs silently did nothing:

/delay/10 with readTimeoutMs = 2000
  host gnu (control)      status=0    elapsed  3051 ms   <- timeout fired
  openkal-linux 0.7.0     status=200  elapsed 11350 ms   <- waited out the server
  openkal-linux 0.7.1     status=0    elapsed  2994 ms   <- restored

Nothing in tinyhttps needs to change for that; it is fixed beneath. It is recorded here so that anyone reading a timeout that did not fire on openkal knows which layer to look at.

What happens next

Your commit is merged as it stands. The three items above land as a follow-up PR, with the live test moved onto the default configuration and asserting the elapsed time — a test that closes the connection to make the loop end is examining the case where the defect is absent.

Thank you for the issue: it was precisely stated, the repro was exact, and the two things you said you had ruled out were in fact ruled out.

[Sunrisepeak]

Delivered in #14, which carries your commit as its first commit with your authorship intact — git log on master will show it under your name, not mine.

Closing this one rather than merging it, for a mechanical reason and not a judgement on the change: its head is on a fork I have no push access to, so the three follow-up items could not be added here. maintainerCanModify is set, but it grants the GitHub UI rather than my git credentials, and a push to cloud-teahouse/tinyhttps answers 403. Landing one PR was the alternative to landing a correct change and then immediately amending it in a second.

Your report was right on every point that mattered, and two of them were the difficult kind:

• The mechanism, not just the symptom. You said SseParser finds no boundary and the bytes stay in its buffer. Confirmed — and the interesting part is that for httpbin's teapot document it does find a boundary, because that document contains a blank line; the block simply carries no data:, event: or id: field, so dispatch_event_ returns without pushing. The bytes reach the parser and die there either way, which is what you described.
• The placement. Capturing in dispatch covers both framing paths and is better than reaching into the parser's buffer, which is no longer the whole body once any event has been dispatched. Deciding captureBody after the headers is right too — send_stream follows no redirects, so the status cannot change under it.

What the review added is in #14 and in the comment above: a declared Content-Length (which is why your test had to close the connection — that line was the defect, not a test detail), a chunk size that is rejected rather than salvaged, and a Content-Length parser that does not read -1 as 1.

Thank you. The issue was precisely stated, the reproduction was exact, and the two things you said you had ruled out really were ruled out — which is rarer than it should be.

[Sunrisepeak]