[thweetkomputer]

Context

A 1 MiB SET currently passes through roughly 256 small receive/parse rounds and copies each body fragment into a growing argument string. Grouped String page construction also copies the segment through a const initializer-list element.

Uses the buffered-read API merged in eloqdata/bycorf#15. The Bycorf submodule is pinned to current upstream main 0f7b4c0730d515422531528ecacb65457afdd939, the same revision as Lavik main.

Behavior before and after

Small requests keep their existing receive path. Once a bulk argument of at least 32 KiB has started arriving, the server can prepay a bounded destination and receive directly into the final argument string. The runtime's opt-in read drains already queued buffers without waiting to fill the destination. TCP and TLS preserve command framing and pipeline order; TLS retains its existing decrypted-read behavior.

Grouped String pages now move the owned segment into a vector entry. A full 1 MiB SET avoids 128 additional segment-string allocations/copies; partial String updates also avoid the corresponding changed-segment copy.

Implementation

• Add parser APIs for bounded reservation, writable body spans, and checked partial commits. Normal parsing remains supported, including fragmented terminators and RESP3 verbatim validation.
• Prepay at most 1 MiB of additional body storage, cap direct reads and uninterrupted receive work at 256 KiB, and recheck the live query limit after reads. Declaration-only requests do not preallocate their advertised body.
• Charge each destination before allocation; received bytes inherit that charge. Completed commands and MULTI retain their usual wire-byte ownership, while disconnect/error teardown releases unread reservations.
• Replace StringPage's initializer-list construction with an in-place entry and string move.

Design decisions and alternatives

Increasing only the application buffer would still leave the runtime returning one 4 KiB receive buffer per call. The opt-in runtime API avoids enlarging every worker's receive pool or changing existing ReadSome callers. String resize initializes writable storage safely; this removes intermediate copies and growth but does not eliminate every memory write. Client accounting remains a request-byte quota, not exact allocator-capacity accounting.

Documentation and comments

Updated the request-serving architecture, client-buffer API comments, and metrics documentation/help to describe prepaid direct-read destinations and their lifetime. The grouped storage format and publication model are unchanged.

Test plan

Initial validation used GCC 13 Debug on ARM64, before merging current Lavik main:

./scripts/configure_debug.sh -DFETCHCONTENT_SOURCE_DIR_GOOGLETEST=/home/ubuntu/lavik/build_debug/_deps/googletest-src -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER_LAUNCHER=ccache
cmake --build build_debug --target lavik lavik_unit_tests lavik_grouped_ordered_write_e2e_test --parallel 8
ctest --test-dir build_debug -R '^(RespParserTest|ReplyBuilderTest|MemoryTest|GroupedCollectionTest)[.]' --output-on-failure
ctest --test-dir build_debug -R '^lavik_(large_bulk|reply_fragments)_e2e$' --output-on-failure --verbose
GTEST_FILTER='GroupedStringWriteE2e.*:BatchWindows/GroupedStringCrashE2e.*' ctest --test-dir build_debug -R '^lavik_grouped_ordered_write_e2e$' --output-on-failure --verbose
pre-commit run
git diff --cached --check

All 83 selected unit tests, both network suites, and all 7 selected String write/crash-recovery cases passed, without skips. Network coverage includes TCP/TLS binary roundtrips across multiple reservation windows, mixed small/large pipelines, MSET, MULTI/EXEC/DISCARD, partial EOF, malformed framing, live query-limit shrink, and quota rejection/release. The runtime dependency's five checks also passed.

After merging Lavik main 1af140496, the only conflict was the Bycorf submodule reference. It now points to upstream main 0f7b4c0730d515422531528ecacb65457afdd939, whose tree is identical to previously tested runtime head c4ec8208a15279e9484a07f74bdec51cfe8e6b5f. The non-submodule patch against the new base has the same stable patch ID as the original PR. Combined String ownership/locking review, format/lint checks, Python syntax compilation and diff checks passed. Local functional tests were not rerun for this merge-only update; CI is rerunning on the final merged commit.

This host falls back from provided-buffer-ring setup to one-shot receive. Batched queue behavior is covered by deterministic runtime queue tests; production multishot throughput and the original 80-connection workload have not been benchmarked here.

Risk assessment

The main risk is confusing prepaid bytes with received bytes or publishing an unwritten tail. Separate parser state and integration checks cover these boundaries. A slow sender that starts a large body may hold a bounded prepaid window; it counts against the existing client quota and is released on disconnect. Small-request throughput has not been claimed from the functional checks.

Rollback plan

Revert this PR, including the runtime pin. No data migration is required.

Reviewer guide

Review parser state and server charge transfer together, then the network ownership tests. The first commit contains only the StringPage copy fix for isolated comparison. The merged main includes the worker store-lock preparation change from #340 and record CRC controls from #341. Both are baseline changes; this PR retains only the receive and StringPage copy optimizations.

Follow-up work

Rerun the original large-value benchmark to measure throughput and tail latency; no end-to-end speedup is asserted from source inspection.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 20 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 99ec3736-d5ee-42da-997f-39249475bb6f

📥 Commits

Reviewing files that changed from the base of the PR and between 2665acc and 0329bda.

📒 Files selected for processing: 11

• CMakeLists.txt
• docs/architecture/02-request-serving.md
• docs/operations/metrics.md
• include/lavik/memory.h
• include/lavik/resp.h
• src/metrics.cpp
• src/redis/resp.cpp
• src/redis/server.cpp
• src/storage/engine/grouped/string.cpp
• tests/large_bulk_e2e.py
• tests/resp_test.cpp

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2d8a0542-bcfb-48fa-907a-dddf9c25f206

📥 Commits

Reviewing files that changed from the base of the PR and between 045df0b and 2665acc.

📒 Files selected for processing: 12

• CMakeLists.txt
• bycorf
• docs/architecture/02-request-serving.md
• docs/operations/metrics.md
• include/lavik/memory.h
• include/lavik/resp.h
• src/metrics.cpp
• src/redis/resp.cpp
• src/redis/server.cpp
• src/storage/engine/grouped/string.cpp
• tests/large_bulk_e2e.py
• tests/resp_test.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The RESP parser and server now support bounded direct reads into large bulk arguments. The change adds client-buffer reservation and query-limit checks, plus unit and TCP/TLS end-to-end tests. It also updates the bycorf reference and changes StringPage entry construction.

Changes

Large RESP bulk receive

Layer / File(s)	Summary
Parser bulk-read API and implementation include/lavik/resp.h, src/redis/resp.cpp, tests/resp_test.cpp	The parser exposes bounded writable storage for bulk arguments and commits bytes received into that storage. Tests cover partial commits, query limits, resets, pipelined input, and malformed terminators.
Server reads and quota validation src/redis/server.cpp, include/lavik/memory.h, src/metrics.cpp, docs/architecture/02-request-serving.md, docs/operations/metrics.md, tests/large_bulk_e2e.py, CMakeLists.txt, bycorf	The server reserves client-buffer capacity before direct reads and refreshes query limits before committing bytes. Documentation, metrics, and TCP/TLS tests cover request-byte accounting and large bulk requests. The bycorf subproject reference changes.

StringPage initialization

Layer / File(s)	Summary
Append the moved string entry src/storage/engine/grouped/string.cpp	StringPage creates an empty entries vector and appends the moved string. Snapshot metadata remains unchanged.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant ReadCommandBatch
  participant RespCommandParser
  participant ClientBufferReservation
  participant BycorfRead
  ReadCommandBatch->>RespCommandParser: Get bulk reservation size
  ReadCommandBatch->>ClientBufferReservation: Reserve destination bytes
  ReadCommandBatch->>RespCommandParser: Prepare argument storage
  ReadCommandBatch->>RespCommandParser: Get writable bulk buffer
  ReadCommandBatch->>BycorfRead: Read bytes into writable buffer
  BycorfRead-->>ReadCommandBatch: Return received bytes
  ReadCommandBatch->>RespCommandParser: Commit bytes after query-limit refresh

Loading

Suggested reviewers: liunyl

Merge Risk: ⚪ Minimal · up to 2665a

The change adds a bounded direct-read path for large Redis bulk arguments and is covered by unit and TCP/TLS tests. No merge-blocking risk is evident.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 10.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 8 files. (4 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description covers all required template sections. It explains the context, behavior changes, implementation, design decisions, documentation updates, test commands and results, risks, rollback, r…
Title check		The title is concise and accurately describes the two primary changes: reducing large Redis String receive copies and grouped String segment copies.

Full details: Docstring Coverage

Explanation

Docstring coverage is 10.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 8 files. (4 skipped: 4 unsupported.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the bulk bytes flow,
Through bounded windows, row by row.
The parser keeps each payload near,
While tests make TCP paths clear.
Then hops away, with bytes in tow.}

_{Comment @coderabbitai help to get the list of available commands.}