Decode chunked bodies in Carp and report broken framing - #22
Conversation
`ResponseStream.poll-chunked` collapsed every outcome of the C decoder into "set done, return Nothing": a clean end of stream, a parse error, a socket error and the 16 MiB chunk-size cap were indistinguishable. Since `drain-stream` polls until Nothing and its callers wrap whatever they collected in `Result.Success`, a hostile or merely buggy server could hand the caller half a body under a clean 200. The decoder now lives in Carp, mirroring `TransferEncoding.dechunk` from the `http` library chunk for chunk — strict `1*HEXDIG` sizes with an overflow guard, chunk extensions after `;` ignored, no size cap, and the same error strings — so the org has one set of chunked semantics instead of two. That drops three bugs the C version shipped: the 16 MiB cap that truncated legitimate large chunks, `strtol` accepting a `0x` prefix that RFC 9112 §7.1 forbids, and the unconsumed trailer section. `ResponseStream` carries the framing or transport error in a new `error` field; `poll` keeps its `Maybe` shape for the streams interface, and the buffered request functions now return `Result.Error` with the decoder's message instead of a short `Success`. Responses that carry no body per RFC 9110 §6.4.1 (HEAD, 1xx, 204, 304) are not decoded at all, so a HEAD against a chunked endpoint stays a clean empty 200. `poll-chunked` used to rebuild the whole remaining buffer once per chunk, which is quadratic in the chunk count; it now keeps a byte offset and only compacts when it reads from the socket. The dead `decoded` field is gone.
|
One correction to my own description, and a divergence worth knowing about "strict so this decoder accepts a chunk-size line RFC 9112 §7.1 has no production for — I have not re-run this branch to confirm the whitespace cases; the expression is The divergence. carpentry-org/http#42, opened minutes before this one in the The right end state is one parse, and |
|
Measured it rather than leaving it as a reading. Built this branch at
So the four framings carpentry-org/http#42 rejects are all still accepted here, (First run of this probe was worthless and I nearly believed it: port 8799 was Unrelated finding from that request log, filed as carpentry-org/http#43: the |
There was a problem hiding this comment.
Build & Tests
bash test/run.sh — 145 passed, 0 failed, matching the PR. Also ran carp -x --log-memory test/http-client.carp against the same servers: 145/0 again, no leak diagnostics. CI green on both legs.
Findings
1. The headline fix is real — and main is worse than the description says
/chunked-oversize asserts only that the size line stops being a parse error; the data is never sent, so it is not evidence that the 16 MiB cap removal works. So I sent the data. Local origin server, one chunk of exactly 20,971,520 bytes:
main |
3d25304 |
|
|---|---|---|
| 20 MiB single chunk | 200 len=0 |
200 len=20971520 |
The cap removal is genuine. Note that main returns an empty body, not a partially-truncated one — the description calls the old behaviour "silently truncated a legitimate large response mid-body", and it is in fact a clean 200 with nothing at all in it.
The incremental decoder also holds up under split reads, which is where a hand-rolled pull decoder usually breaks. All of these decode correctly on the branch: size line split mid-line, size digits split across reads (1 then 0\r\n...), chunk data split, the data-terminating CRLF split across two reads, the trailer section split, and the entire body delivered one byte per write. 0\r\n\r\n followed by trailing garbage stops cleanly, and a 10 KB chunk extension is ignored.
2. Regression: poll-raw now loses complete non-chunked responses
http-client.carp:207 turns any transport error on the raw path into a stream failure:
(Result.Error e)
(do (fail! s (fmt "read error: %s" &e)) (Maybe.Nothing))
The description mentions this only in passing ("the socket-error arms in poll-chunked and poll-raw did the same"); the consequence is neither discussed nor tested. All 13 new assertions are chunked or HEAD — nothing covers the raw path at all.
The consequence is that a response which arrived completely is discarded if the peer hangs up abruptly. Reproducer with no socket-option trickery: a server that answers before reading the request body and then closes, so the unread body sitting in its receive queue makes the kernel send RST instead of FIN. That is the ordinary early-reject upload shape — 413, 401 or 400 answered before draining.
POST 200 KB, server replies 413 Payload Too Large with Content-Length: 17 and body payload too large, then closes:
| result (3 runs each, identical every time) | |
|---|---|
main |
413 [payload too large] |
3d25304 |
ERROR: read error: Connection reset by peer |
All 17 declared bytes were received. The caller now loses both the status code and the body.
The chunked path is right to be strict, because chunked framing tells you whether the body ended — that is this PR's whole point. The raw path has no framing: it ends at EOF, so a transport error there is indistinguishable from "the server finished and hung up rudely". As written the two are conflated in the direction that fails a request which used to succeed.
The information to separate them is already in hand: read-headers parses the Response before the stream is built and it is stored on the stream as parsed-response (http-client.carp:119), so poll-raw can consult Content-Length and treat a reset as a clean end once the declared length has been delivered. The alternative is to leave poll-raw's error arm as it was — the justification in this PR is entirely about chunked framing and does not carry over to a stream whose only terminator is EOF. Either way it wants a test on the raw path, since there is currently none.
3. The divergence with carpentry-org/http#42 — confirmed, and there is a fifth shape
You raised this yourself and measured four framings. Confirmed independently against this branch — String.trim is still at http-client.carp:228 — and there is one more:
| wire | 3d25304 |
http#42 |
|---|---|---|
SP + 5\r\n... |
200 [hello] |
rejected |
5 + SP + \r\n... |
200 [hello] |
rejected |
HTAB + 5\r\n... |
200 [hello] |
rejected |
SP + 0 + SP + \r\n\r\n |
200 [] |
rejected |
5 + CR + \r\n... (stray CR) |
200 [hello] |
rejected |
The stray-CR case is the fifth; it is not in your table, and http#42 rejects it too. It does not change your conclusion, only widens it.
4. The disclosed quadratic is real, and is not a regression
Wall time for Client.get on a single chunk, same server both sides:
| body | main |
3d25304 |
|---|---|---|
| 256 KiB | 10 ms | 9 ms |
| 512 KiB | 26 ms | 32 ms |
| 1 MiB | 184 ms | 166 ms |
| 2 MiB | 933 ms | 759 ms |
| 4 MiB | 3984 ms | 3172 ms |
Both are quadratic — double the body, roughly quadruple the time — and the branch is slightly faster at every size, so fill! made nothing worse. The "Not done" section describes it accurately.
Worth knowing what it costs now the cap is gone, though: that curve puts the 20 MiB body from finding 1 at roughly 80 seconds. Removing the cap is still clearly right — a silent empty body is worse than a slow one — but "16 MiB responses are now supported" and "16 MiB responses are now practical" are different claims, and only the first is true today.
Many small chunks is linear on both sides (2,000 chunks: 5 ms vs 4 ms; 8,000 chunks: 18 ms vs 16 ms), so the byte-offset rewrite is a modest win rather than the large one that "quadratic in the chunk count" suggests. The old per-poll rebuild was bounded by the socket read size, so it never got the chance to be quadratic in practice.
Checked, nothing wrong
- Redirects never read the 3xx body — the connection is closed at
http-client.carp:552before following — so malformed redirect framing cannot fail a chain. fail!is guarded by(Maybe.nothing? (error s))at both call sites (lines 220 and 250), so a transport error is never overwritten by a framing message.bodyless?(http-client.carp:410) works for 204 and 304 as well asHEAD: both return their own status with an empty body against a server that announceschunked.skip-trailers!terminates —find-crlfonly scans forward andpstrictly increases each iteration.- A complete chunked body followed by RST is fine:
doneis set at the zero-size chunk, before any further read.
Verdict: revise
The chunked work is good, and the 20 MiB measurement shows the fix matters more than the description claims. Finding 2 is the blocker: poll-raw now fails a complete, correctly-delivered non-chunked response whenever the peer resets, which an early-reject upload does routinely — a regression on the commonest response shape in the library, introduced by a PR about chunked framing.
The new fatal-error arm in poll-raw treated any transport error as a stream failure. On the chunked path that is right: chunked framing says whether the body ended. A raw body has no framing, so an error there was conflated with "the server finished and hung up rudely" — and it lost the whole response. The shape that hits is an early-reject upload: a 413, 401 or 400 answered before the request body is drained, so the unread body makes the kernel send RST rather than FIN. POST 200 KB to a server answering 413 with Content-Length: 17, three runs each side: main 413 [payload too large] 3d25304 ERROR: read error: Connection reset by peer All 17 declared bytes had arrived. Reverting the arm would fix that and keep silently truncating a body that really was cut short, so instead the stream now carries the length the response declared and spends it as bytes are delivered. A transport error or an EOF is a truncation only when that budget is unspent; otherwise the body is complete and the stream ends clean. Content-Length that is absent, unparseable or on a bodyless response leaves the budget empty, which is the lenient reading main had. The chunked path is untouched and stays strict. A clean close short of Content-Length is now an error too — main and 3d25304 both returned a 200 with a short body for it. Six raw-path assertions, where the thirteen this branch had were all chunked or HEAD. Five of them fail on 3d25304; the sixth fails on main, and both truncation assertions fail if `truncated?` is forced to false.
|
Finding 2 is fixed in Reproduced firstBefore touching anything I stood up the early-reject server you describe: it
Same as yours. (Checked the listening PID was my own server and logged the I then widened it to the four shapes the choice actually turns on, and this is
Note row 4: the regression is not confined to the early-reject shape. An Which fix, and why the other one losesI took (a), consulting the declared length, because (b) is only correct on the Row 2 is the case that decides it. Reverting The mechanism is a byte budget rather than a check at the end. You named the cases where (a) cannot decide, and they resolve to the lenient
One thing that is new strictness rather than a restoration, and worth your What the new tests pinSix assertions and five
The reset is forced with Teeth checked both ways rather than asserted: five of the six fail on Findings 3 and 43 — the 4 — the quadratic 1 — description corrected. The oversize-cap bullet said the old code Checks
|
There was a problem hiding this comment.
Build & Tests
bash test/run.sh at ddee6c2 — 151 passed, 0 failed, exit code read from
the unpiped command, up from the 145 I measured at 3d25304 and matching the
PR. CI green on both legs; that workflow builds angler and carp-fmt fresh
from their HEADs, so lint and format are covered there rather than by my local
binaries, which predate both tool HEADs. carp -x gendocs.carp leaves the tree
clean. No CHANGELOG in this repo, so none was owed.
Prior feedback
Finding 2, the blocker, is fixed. I did not reuse the PR's numbers: I stood
up my own origin — five routes, SO_LINGER(1, 0) for the resets, a pause before
each so the client has read the response — and ran the same probe binary against
main and against this head.
| shape | main |
ddee6c2 |
|---|---|---|
complete Content-Length body, then RST |
200 [complete!!] |
200 [complete!!] |
| 10 of 64 declared bytes, then RST | 200 [only-ten-b] |
ERROR read error: Connection reset by peer |
| 10 of 64 declared bytes, then FIN | 200 [only-ten-b] |
ERROR truncated body: ... |
no Content-Length, then RST |
200 [eof-delimited] |
200 [eof-delimited] |
| 413 answered before draining, then RST | 413 [payload too large] |
413 [payload too large] |
Row for row what the PR claims, in both columns. The regression I reported is
gone, row 4 shows the EOF-delimited case you found on your own was gone with it,
and rows 2 and 3 are the strictness the change exists for — neither is a
restoration of main, which is silent on both.
The budget is what those two error rows measure. Three mutants, each applied
to http-client.carp alone with test/run.sh re-run:
| mutant | suite |
|---|---|
truncated? forced to false (main's lenient arm) |
149 / 2 — reset-short, close-short |
declared-length never seeds the budget |
149 / 2 — the same two |
truncated? forced to true (3d25304's strict arm) |
103 / 48 |
The first matches the PR's own claim exactly. The second is the one I added: it
separates "the budget is consulted" from "the budget is seeded", and the same
two assertions die, so Content-Length is really being parsed rather than the
tests passing on an empty budget. The third is the blunt direction and it takes
a third of the suite with it, which is the measure of how much of this library
now depends on the lenient reading being available.
Findings 3 and 4 are correctly still open. carpentry-org/http#42 is still
OPEN, so the String.trim divergence genuinely cannot be resolved here
without pinning this decoder to an unmerged PR in another repo; deferring is
right. The quadratic fill! disclosure now carries the 80-second figure, which
was the useful half. Finding 1's description wording is corrected.
Findings
1. A body containing a NUL byte is a sixth shape, and its behaviour changed
Same probe binary, same two commits, Content-Length-framed and non-chunked:
| body | main |
ddee6c2 |
|---|---|---|
plain-ascii-body, clean close or RST |
200, 16 bytes |
200, 16 bytes |
| multibyte UTF-8, 24 bytes / 18 chars, clean close or RST | 200, 24 bytes |
200, 24 bytes |
abc\0def-tail-bytes, 20 bytes, clean close |
200 [abc] |
ERROR truncated body: ... |
| same, then RST | 200 [abc] |
ERROR read error: ... |
charge! spends String.length, which is strlen, so a read whose bytes stop
being visible at an embedded NUL under-charges the budget, truncated? stays
true, and the response is rejected. The multibyte row is the control that says
this is about NUL and not about non-ASCII: String.length is bytes here, so
ordinary UTF-8 bodies charge correctly and nothing there moved.
I do not think the new behaviour is wrong. Those 17 bytes were already lost
on main — a clean 200 carrying 3 of 20 declared bytes — so erroring is the
honest answer, and the line lands exactly where data was lost rather than
somewhere arbitrary. But it is a row your table does not have and no test
covers, and the description's list of cases that "leave the budget empty and the
lenient reading main had" does not reach it. The practical consequence is that
Client.get of any binary body containing a NUL — an image, a gzip stream — now
fails outright where it used to return a truncated 200. One line in the
description would save the next person that surprise.
2. "nothing that was public changed shape or signature" is not accurate
ResponseStream gained pos, error and remaining and lost decoded across
this PR, so the deftype-generated ResponseStream.init went from seven
positional arguments to nine, with two of the surviving ones changing type, and
the decoded accessor is gone. There is no hand-written init and neither it
nor the type is hidden.
ResponseStream is not in save-docs (gendocs.carp:40), so none of this is in
the generated docs — but it is reachable, and there is a caller in the org
today: llm's test suite builds one directly, at the 0.5.4 seven-field shape
(test/llm.carp, make-test-response-stream). So the 0.5.5 that
#23 asks for will not compile llm's tests until they
are updated.
Not asking for a change to the code — additive fields on a 0.x type is the right
call, and llm's side is a one-line fix that llm #19 happens to have just made
cheaper by collapsing two ResponseStream.init call sites into one. It is the
sentence in the description that wants correcting, because a maintainer reading
it would plan the 0.5.5 release as drop-in for consumers, and it is not.
Checked, nothing wrong
- The chunked path really is untouched.
poll-chunkedcontains no
reference toremaining,charge!,truncated?orend-raw!, so a response
that is bothchunkedand carries aContent-Lengthseeds a budget nobody
spends or reads — inert rather than a false truncation. Content-Length: 0then RST is clean: the budget isJust 0,truncated?
is false.- The keep-alive hang the budget could have caused does not exist: the client
always sendsConnection: close(http-client.carp:379), sopoll-raw
reading past the declared length to EOF cannot stall on a server that would
otherwise hold the connection open. - Both stream constructors were updated — the plain path and the cookie-jar
path each seeddeclared-length— sopost-with-jaris not left on the old
behaviour, which thereject-earlypair pins.
Verdict: merge
The blocker is fixed and I confirmed it against my own origin rather than the
PR's numbers, in both columns and on all five shapes. The two truncation
assertions have teeth from two independent directions. Both findings are
description accuracy, not defects: one behaviour row that is undisclosed but
defensible, and one release-planning sentence that is wrong about the type's
surface.
The defect
ResponseStream.poll-chunkedcollapsed all four outcomes ofchunked_decode_oneinto one arm —; rc < 0: end of stream (-1) or parse error (-2)— and the socket-error arms inpoll-chunkedandpoll-rawdid the same.drain-streampolls untilNothing, andrequest-with-*wrapped whatever it collected inResult.Success, so a truncated body was indistinguishable from a complete one.Measured on
mainwith the new routes intest/server.py(GET, printed asstatus [body]):main/chunked-bad-hexzz\r\nhello\r\n0\r\n\r\n200 []Error: invalid chunk size 'zz'/chunked-hex-prefix0x5\r\nhello\r\n0\r\n\r\n200 [hello]Error: invalid chunk size '0x5'/chunked-truncated10\r\nshortthen close200 []Error: truncated chunk data/chunked-no-terminator5\r\nhello\r\nthen close200 [hello]Error: missing terminating zero-size chunk/chunked-missing-crlf5\r\nhelloXX0\r\n\r\n200 [hello]Error: chunk data missing CRLF/chunked-oversize200 []Error: truncated chunk dataEvery one of those is a silent short body today, under a clean
200.What changed
The decoder moved to Carp.
src/chunked.hand thechunked-decode-one-registration are gone. The new incremental decoder mirrorsTransferEncoding.dechunkin the siblinghttplibrary chunk for chunk: strict1*HEXDIGsizes via aparse-hexwith an overflow guard, chunk extensions after;ignored, no size cap, and byte-for-byte the same error strings. One RFC, one set of semantics in the org, and no business logic in C. (dechunkitself isprivate-adjacent and decodes a complete body, so it can't be called from a pull-based stream — this is a port, not a call.)That drops three bugs the C version shipped:
CHUNKED_MAX_CHUNK_SIZEcap — an oversize chunk returned-2, which silently dropped a legitimate large response. Not truncated mid-body, as this description first said: the reviewer sent a real 20,971,520-byte chunk andmainanswered200with an empty body, against200 len=20971520on this branch;strtol(buf, &endptr, 16)accepting a0x/0Xprefix — the leading-isxdigitguard passes on the0, andendptrlands on the\r, so0x5parsed as 5. RFC 9112 §7.1 ischunk-size = 1*HEXDIG. The hand-rolled scan also has no locale orerrnobehaviour to reason about;skip-trailers!now consumes it (as far as it is buffered). This one is currently unobservable from outside, since the client does not reuse connections; the test pins that trailers decode cleanly rather than that the old code broke.The error is surfaced.
ResponseStreamgains anerror (Maybe String)field and its accessor.pollkeeps itsMaybeshape — it implements the streamspollinterface — and its doc now says thatNothingmeans "done or failed, checkerror".drain-streamreturns(Result String String), and the four buffered entry points (request-with-max-redirects,request-with-config,request-with-jar,request-with-jar-and-config) fold that into their result through one newcollect-responsehelper, replacing four copies of the samematch/let-do/closeblock.Bodyless responses are not decoded. Surfacing the error meant a
HEADagainst a chunked endpoint — headers announcechunked, no body follows — would have started failing with "missing terminating zero-size chunk".bodyless?skips chunked decoding for the responses RFC 9110 §6.4.1 says carry no body:HEAD, 1xx, 204, 304.A raw-path transport error is judged against
Content-Length. Making the error fatal was right for the chunked path, whose framing says whether the body ended, and wrong for the raw path, which has none: it ends at EOF, so an error there is indistinguishable from "the server finished and hung up rudely". The commitddee6c2message has the measurements; in short, an early-reject upload (a 413, 401 or 400 answered before the request body is drained, so the unread body makes the kernel send RST rather than FIN) lost its status and its body.ResponseStreamnow carries the length the response declared and spends it as bytes are delivered; a transport error or an EOF is a truncation only when that budget is unspent. Content-Length absent, unparseable, or on a bodyless response leaves the budget empty and the lenient readingmainhad. The chunked path is untouched.main3d25304413 [payload too large]ERROR: read error: Connection reset by peer413 [payload too large]Content-Lengthbody, then RST200 [complete!!]ERROR: read error: …200 [complete!!]200 [only-ten-b]ERROR: read error: …ERROR: read error: …200 [only-ten-b]200 [only-ten-b]ERROR: truncated body: …Content-Length, then RST200 [eof-delimited]ERROR: read error: …200 [eof-delimited]Row 4 is new strictness rather than a restoration: a clean close short of the declared length was a silent short body on both
mainand3d25304.The quadratic rebuild is gone.
poll-chunkedused toStringBuf.clear+ appendbuf+ appenddecoded+to-string+ byte-slice the tail once per chunk, which is quadratic in the chunk count. It now keeps a byte offset (pos) intobufand only compacts when it actually reads from the socket. Thedecodedfield was only ever written as@""and is deleted.Adding fields to
ResponseStreamis additive on a 0.x type; nothing that was public changed shape or signature.Tests
test/http-client.carpgains 19 assertions andtest/server.py13 routes. Thirteen of them cover the chunked work: six fail onmain(the table above), and the rest are the "still decodes exactly as before" pins the change needs: the chunk-extension case, the trailer case, theHEADcase,clean after 40 chunksfor/chunked-utf8/40, andclean after 1 chunksfor a trailered body. The existing/chunked-utf8/40,/chunked-foldedand/not-chunkedassertions are untouched and still pass.The remaining six are the raw path, which had no coverage at all before
ddee6c2: the early reject on both the plain and the cookie-jar path, a completeContent-Lengthbody outliving a reset, a reset and a clean close short of the declared length, and an EOF-delimited body ending at a reset. Five fail on3d25304; the sixth fails onmain, and both truncation assertions fail iftruncated?is forced tofalse.bash test/run.sh: 151 passed, 0 failed (132 onmain).carp -x --log-memory test/http-client.carpagainst the same servers: 151/0, no leak diagnostics.carp-fmt --check,angler(rebuilt from HEAD at185a9a2) andcarp -x gendocs.carpare all clean.Not done
A chunk larger than 16 MiB is tested by its size line only — the data is never sent, so the assertion is that the oversize size is no longer a parse error. Actually streaming 16 MiB would be slow for a reason this PR does not fix:
fill!appends each 4 KiB socket read to the unconsumed tail, so a single chunk that spans many reads still copies quadratically in the number of reads. Fixing that needs a real growable byte buffer rather thanString, which is a separate change. The reviewer measured it as not a regression —mainis slightly slower at every size — but also that the curve puts a 20 MiB body at roughly 80 seconds, so "16 MiB responses are now supported" is true and "practical" is not.The
String.trimaround the chunk size, which accepts framings carpentry-org/http#42 rejects, is unchanged and still open. It cannot be resolved here until #42 merges and the pin moves.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.