Make a lone large split bulk read linear instead of quadratic

[carpentry-agent]

Follow-up to #12, which made a buffered read of coalesced or pipelined values linear. One case it left open: a single large bulk-string value split across many small socket reads was still O(N²) in the value size.

The pathology

While one bulk value is incomplete, each outer pass of Redis.read-n did O(k) work on the whole k-byte accumulator:

1. decode-available decodes from offset 0 and computes (String.length s) — an O(k) strlen.
2. The prefix compaction (String.byte-slice &acc 0 (String.length &acc)) — when nothing was consumed, Pair.b is 0, so it copies the whole buffer to itself, O(k).
3. The socket append (String.concat &[acc s]) allocates k+|s| and copies, O(k).

TcpStream.read pulls ≤4096 bytes per read, so an N-byte value takes ~N/4096 reads each doing O(k) work ⇒ Θ(N²/4096). This backs every read / next-message / pipeline-exec, so a GET on a big key or any large bulk reply degraded quadratically.

The fix — a length-aware reader

When a decode pass consumes nothing and the head of the buffer is a bulk string whose $<len>\r\n header has fully arrived but whose payload has not, read-n now:

• reads straight into an (Array Byte) via TcpStream.read-append (realloc-doubling growth) until the value's exact declared length is present — byte appends are amortized O(1) and the length check is O(1) — then
• materializes back to a String once, and resumes normal decoding.

Two small pieces support this:

• RESP.parse-bulk-header (private) — the $<len>\r\n header parse, now shared between decode-one-at's $ branch and the new helper instead of duplicated.
• RESP.bulk-fill-target (public) — given (buffer, start, slen), returns (Just target) when the head is a header-complete-but-data-incomplete bulk (target = the buffer length at which it becomes decodable), else Nothing. Guards against a bogus huge declared length overflowing the offset arithmetic.

Decoding an incomplete bulk was already cheap (the $ branch bails at the completeness check without scanning the payload); the pathology was purely the per-read whole-buffer copy/strlen, and that is what this removes.

Scope

• Covered: a lone top-level bulk (the GET big-key / large-bulk-reply case) is now O(N).
• Unchanged: the coalesced multi-value / pipeline path from Make buffered reads linear instead of quadratic #12 stays linear.
• Not yet linear (but still correct): a large bulk nested inside an array reply — the head at offset 0 is *, so bulk-fill-target returns Nothing and the prior accumulate path is used. It reassembles correctly, just not linearly. A follow-up could extend the length-aware reader into array element positions.

Redis.buf leftover semantics are preserved: bytes past the value (a reply that coalesced into the same read) are stored back on the connection.

Tests

test/resp.carp's server-free feed-n harness was updated to mirror the new accumulation (fill-to-target, then decode), so it exercises bulk-fill-target rather than the old concat-per-chunk loop. New cases:

• a 100000-byte bulk split into hundreds of tiny chunks reassembles to the exact payload with an empty leftover;
• a +OK\r\n reply coalesced right after the large bulk: the bulk decodes and the trailing reply is retained as leftover (neither over-consumed by the fill nor dropped).

The full suite is green (131 asserts, was 129). Also verified locally: carp -b examples/simple.carp, carp -x gendocs.carp, carp-fmt -c, and angler all clean. The real byte-array fill path (to-bytes → read-append → from-bytes → decode) was separately confirmed end-to-end to reassemble a 100000-byte payload and retain the coalesced reply.

---

Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

[hellerve]

way overcommented.

[carpentry-agent]

Trimmed the comment blocks and the bulk-fill-target doc down to concise one-liners — code unchanged, suite still 131/0.

[carpentry-reviewer]

Build & Tests

Checked out claude/linear-single-bulk-read at b79d4f7 and ran the full CI gate set locally:

• carp -b examples/simple.carp — builds clean (only the pre-existing prn/StringBuf warning).
• carp -x test/resp.carp — 131 / 0.
• angler, carp-fmt --check, carp -x gendocs.carp — all clean.

Matches the green CI.

Prior feedback

@hellerve's CHANGES_REQUESTED ("way overcommented") is addressed. The prior 9-line parse-bulk-header block, the 12-line bulk-fill-target doc, and the multi-line inline blocks in read-n/the tests are gone (net −25 lines, behaviour identical). What remains — the 2-line header note (redis.carp:42), the read-n fill comment stating the O(N) win (redis.carp:300), and the overflow-guard note (redis.carp:239) — is in line with the terse "why" comments #12 already established in this module (find-cr-after, decode-one-at's :/array cases).

Findings

Verified the length-aware fill is correct, not just green:

• bulk-fill-target (redis.carp:229) returns (Just target) only for a $ head whose header CRLF has arrived, len >= 0, no data-start+len+2 Int overflow, and target > slen. Everything else — array heads, a partial header, an empty buffer, an oversized declared length — falls through to Nothing and the old accumulate path. All correct; traced each branch.
• read-n (redis.carp:298) fills an (Array Byte) via read-append until length ≥ target, then materializes to a String once. A reply coalesced past target is retained as leftover (the new test covers exactly this), and error/EOF both stop cleanly with the partial buffer stored back.

Two non-blocking notes: bulk-fill-target is public solely so the server-free feed-n harness can call it — consistent with the already-public decode-available/decode-first test helpers, so fine. And the live-socket path (read-append against a real TcpStream) is exercised only by the author's manual check, not an automated test — but that's inherent to this repo's server-free test design, not new here.

Verdict: merge

Blocking feedback addressed and the code verified correct with all gates green — but the final call on comment density is yours, since your CHANGES_REQUESTED still stands and it's a subjective one.

[hellerve]

still way too many comments.

[carpentry-agent]

Cut every explanatory comment this PR added — parse-bulk-header, the two inline notes in bulk-fill-target, and the fill note in read-n are gone. All that's left is a one-line doc on bulk-fill-target (it's public for the test harness, so it matches decode-available/decode-first). The two ; blocks still in the diff are #12's, unchanged. 131/0.

[carpentry-reviewer]

Build & Tests

Checked out claude/linear-single-bulk-read at 7d555af (on current master, merge-base = origin/master HEAD fa36c39):

• carp -x test/resp.carp — 131 / 0.
• carp-fmt --check, angler, carp -x gendocs.carp — all clean. Matches green CI (ubuntu + macos).

Prior feedback

Your second CHANGES_REQUESTED ("still way too many comments") is addressed as fully as this PR can. Diffing this round (b79d4f7 → 7d555af):

• Every comment this PR added is now gone — the parse-bulk-header header note, the two inline notes in bulk-fill-target, and the fill note in read-n.
• What remains in the PR diff is exactly one 2-line ; block (redis.carp:102-103, "payload + trailing CRLF … avoids overflow"). I confirmed that one is pre-existing on master (origin/master:redis.carp:97-98, from #12) — it only appears in the diff because extracting parse-bulk-header shifted the surrounding lines. Removing it would mean editing already-merged #12 code, out of scope for this follow-up.
• The lone remaining doc is on the public bulk-fill-target, consistent with the module's other public test helpers (decode-available / decode-first).

So there's nothing left in this PR's own footprint to trim.

Findings

Removing the overflow-guard comment exposed a nested (if … (if … …)) to angler, which was rewritten as a cond. Verified behavior-preserving — Carp's cond on these 5 forms expands to exactly (if (< len 0) Nothing (if (> len …) Nothing (let …))), i.e. the original tree — and angler is clean on it. The rest of the read-n diff is pure re-indentation (logic tokens unchanged), and the suite (incl. the 100000-byte-split and coalesced-leftover cases) still passes 131/0.

Verdict: merge

Correctness is unchanged and verified; your blocking feedback is addressed to the extent this PR can — every comment it introduced is removed. The final density judgment is yours, but there's nothing further here for the agent to cut.

[hellerve]

why is this a comment instead of a doc annotation?

[hellerve]

See above.

[carpentry-agent]

Converted the feed-n and chunk-by descriptions from free-floating ; comment blocks into (doc ...) annotations on their respective functions, per your two inline comments on test/resp.carp ("why is this a comment instead of a doc annotation?" on feed-n, and "See above." on chunk-by). The prose is unchanged apart from moving it into the doc string, so it's now retrievable via (doc feed-n) / (doc chunk-by) like the module's other helpers. Suite still 131/0; carp-fmt -c and angler clean.

[carpentry-reviewer]

Build & Tests

Checked out claude/linear-single-bulk-read at 9c11365 (== PR head SHA; merge-base fa36c39 = current origin/master HEAD). Ran the full CI gate locally:

• carp -x test/resp.carp — 131 / 0.
• carp -b examples/simple.carp, angler, carp-fmt --check, carp -x gendocs.carp — all clean (only the pre-existing prn/StringBuf warning). Matches the green CI (ubuntu + macos).

Prior feedback

Your two inline comments on test/resp.carp ("why is this a comment instead of a doc annotation?" on feed-n, "See above." on chunk-by) are addressed. Commit 9c11365 moves both ; prose blocks into (doc feed-n …) / (doc chunk-by …) immediately preceding each defn. The text is unchanged apart from the move ("run"→"runs", "split"→"splits", added backticks/period), so the descriptions are now retrievable via (doc feed-n) / (doc chunk-by) like the library's other helpers. The doc forms name real bindings — a stale name would warn on eval and the run is clean — confirming they're attached, not silently dropped.

No implementation (redis.carp) change this round; the length-aware O(N) fill verified correct in the prior rounds is untouched.

Findings

None. This cycle's diff is +7/−5 in test/resp.carp, purely the comment→doc relocation; behaviour is identical (suite still 131/0, same cases).

Verdict: merge

Every explicit ask is now resolved and the change is correct with all gates green. The remaining comment-density judgment is subjective and yours — but there's nothing left in this PR's own footprint for the agent to act on.