fix(http): keep the error body of a failed streaming request by yspbwx2010 · Pull Request #12 · mcpplibs/tinyhttps · GitHub
Skip to content

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

Closed
yspbwx2010 wants to merge 1 commit into
mcpplibs:masterfrom
cloud-teahouse:fix/stream-error-body
Closed

fix(http): keep the error body of a failed streaming request#12
yspbwx2010 wants to merge 1 commit into
mcpplibs:masterfrom
cloud-teahouse:fix/stream-error-body

Conversation

@yspbwx2010

@yspbwx2010 yspbwx2010 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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.

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.
@Sunrisepeak

Copy link
Copy Markdown
Member

@Sunrisepeak

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

send_stream never fills HttpResponse::body, so a failed streaming request carries no reason

2 participants