perf(storage): use direct awaitables for value and extent reads

[thweetkomputer]

Context

Ordinary value reads enter LoadValue and LoadValueLocal Tasks before submitting I/O. External values additionally create routing and physical-read Tasks for each extent. These wrapper frames can be replaced with awaitables that suspend the existing caller directly.

Behavior before and after

Callers retain co_await LoadValue(...) and co_await LoadValueLocal(...). Both functions return a shared ordinary C++ awaitable. Local inline reads and staged copies create no loader coroutine frames, including when the bounded read-buffer pool is exhausted. This applies to GET, writes, collection readers, replication, and backup.

Grouped values enter LoadGroupedValue directly without a LoadValueSlow forwarding frame. Whole-extent and slice reads share another awaitable: local routing and physical reading create no child task, and cross-owner routing uses the existing runtime task bridge without a surrounding routing coroutine.

Multi-step representation loading and relocation retries retain lazy helper Tasks. Logical reads follow same-mutation relocations before returning; physical reads report stale locations to their enclosing retry loop. Transaction admission and device routing remain unchanged.

Implementation

• Make LoadValue and LoadValueLocal ordinary factories for LoadValueAwaitable; GET keeps a single loader call.
• Share buffer acquisition, staged copying, pin ownership, identity/CRC checks, and lease transfer in one implementation.
• Extend the buffer pool's existing FIFO handoff with an owner-local scheduled continuation. This retains the assigned slot and foreground/background scheduling, avoids running decoding or nested resumes inside buffer release, and removes LoadValueLocalWaiting.
• Select the grouped graph reader directly; retain its borrowed digest in the outer awaitable.
• Replace extent routing and slice loader Tasks with ReadExtentAwaitable, preserving the physical pin across buffer waiting, I/O, and validation. Slice reads validate the entire extent before copying requested bytes. Remove the unused ReadExtentInto wrapper.
• Inspect logical record identity at I/O completion to select the parent or relocation continuation. Payload CRC and large copies run in the scheduled caller. Relocation failures enter index resolution without rereading the stale address. Successful reads fence population once, after the CQE scheduling gap; the completion handler checks population only before a retry. An existing database-epoch snapshot avoids a redundant atomic epoch load.
• Narrow an existing checkpoint test's log assertion to falling back to record scan, excluding unrelated kernel receive-buffer fallback messages.

Design decisions and alternatives

Awaitable state resides in the existing caller's frame; eliminating child frames does not eliminate the state itself. This does not depend on compiler coroutine-allocation elision. Pool continuations use the runtime's existing scheduled-work mechanism on the same worker without changing the Bycorf submodule. Cross-owner dispatch retains the runtime's Task bridge, and complex graph/relocation flows retain their orchestration Tasks.

Documentation and comments

Updated docs/architecture/04-storage-and-recovery.md to describe direct value/extent reads, pool continuation ownership, and manifest-order external assembly. API and implementation comments cover handoff lifetime, scheduling, physical pins, grouped argument lifetime, and slice validation.

Test plan

cmake --build build_debug --target lavik lavik_unit_tests lavik_extent_recovery_e2e_test lavik_grouped_recovery_e2e_test --parallel 4
env NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost build_debug/lavik_unit_tests --gtest_filter='LoadValueAwaitableTestPeer.*:BufferPool*'
env NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost build_debug/lavik_unit_tests
env NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost ctest --test-dir build_debug --output-on-failure -R '^lavik_(defrag_payload|extent_recovery|grouped_recovery|rdb_backup)_e2e$'
pre-commit run --files CMakeLists.txt docs/architecture/04-storage-and-recovery.md include/lavik/storage/engine.h include/lavik/storage/buffer_pool.h src/storage/buffer_pool.cpp src/storage/engine/impl.h src/storage/engine/read.cpp tests/load_value_awaitable_test.cpp tests/grouped/recovery_e2e_test.cpp
git diff --check

Passed all 832 unit tests, including 13 awaitable tests and 10 buffer-pool tests, plus all four storage end-to-end suites. Focused tests exercise FIFO handoff, background continuation, pool-to-disk submission, population invalidation before completion and after scheduling, relocation during buffer waiting, extent slice CRC before copying, pin/lease cleanup, and error handling. Existing end-to-end coverage includes physical-owner reassignment and indirect/grouped/external representations.

Also passed concurrent GET/SET/MULTI, FLUSHDB, and cold recovery using a temporary copy of an existing test script against this branch's binary:

git show perf/remote-get-awaitable:tests/remote_get_e2e.py > /tmp/lavik-direct-get-e2e.py
env NO_PROXY=127.0.0.1,localhost no_proxy=127.0.0.1,localhost PYTHONPATH=/home/ubuntu/lavik/tests python3 /tmp/lavik-direct-get-e2e.py build_debug/lavik

That additional script comes from a separate local branch and is not part of this PR.

Risk assessment

The sensitive changes are pin/lease lifetime, buffer continuation handoff, and selecting retry continuations before returning to the caller. Tests cover these boundaries and affected storage representations. Shutdown retains the runtime's existing drain-before-destruction requirement. Validation used io_uring; SPDK hardware was unavailable. Throughput of the latest head has not been measured here; the target host and billion-key dataset are unavailable in this environment. Earlier target-host results for bc13523 do not validate the throughput of this head or isolate the cost of completion-side parsing.

Rollback plan

Revert this PR. No configuration, wire-format, or durable-format changes.

Reviewer guide

Start with the value/extent factories and awaitables in read.cpp, then AcquireReadAwaiter::SuspendWith and pool release in buffer_pool.cpp, and the focused tests. The branch is based on main and does not include the separate connection-worker remote-read feature.

Follow-up work

Compare throughput and latency against main on the target hardware.

[coderabbitai]

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: 0aa19436-a89b-4f21-ae86-db26572e2710

---

---

---

---

---

---

---

📥 Commits

Reviewing files that changed from the base of the PR and between a244d85 and fcad839.

---

---

---

---

---

---

---

📒 Files selected for processing (9)

• CMakeLists.txt
• docs/architecture/04-storage-and-recovery.md
• include/lavik/storage/buffer_pool.h
• include/lavik/storage/engine.h
• src/storage/buffer_pool.cpp
• src/storage/engine/impl.h
• src/storage/engine/read.cpp
• tests/grouped/recovery_e2e_test.cpp
• tests/load_value_awaitable_test.cpp

---

---

---

---

---

---

---

🚧 Files skipped from review as they are similar to previous changes (1)

• docs/architecture/04-storage-and-recovery.md

---

---

---

---

---

---

---

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

Value and extent reads now use awaitables to manage buffer waits, I/O, validation, and retries. The change adds tests for these paths and updates architecture documentation. A recovery test now checks specifically for the record-scan fallback message.

Changes

Storage read awaitables

Layer / File(s)	Summary
Owner-local buffer handoff include/lavik/storage/buffer_pool.h, src/storage/buffer_pool.cpp	The read-buffer awaiter accepts an owner-local continuation. The pool posts it to the owner worker when a buffer is released to a waiter.
Value read awaitable and retries include/lavik/storage/engine.h, src/storage/engine/impl.h, src/storage/engine/read.cpp, tests/load_value_awaitable_test.cpp, CMakeLists.txt, docs/architecture/04-storage-and-recovery.md	LoadValue and LoadValueLocal return awaitables for value reads. Local reads manage buffer and pin state, validate records, and use the slow path for grouped, external, remote, or retry flows. Tests cover handoff, population changes, relocation, I/O errors, and resource cleanup.
External extent reads src/storage/engine/impl.h, src/storage/engine/read.cpp, tests/load_value_awaitable_test.cpp, docs/architecture/04-storage-and-recovery.md	Extent reads use an awaitable that retains the pin through buffer waiting, I/O, and validation. Slice reads validate the full extent before copying requested bytes.

Recovery log assertion

Layer / File(s)	Summary
Narrow the fallback assertion tests/grouped/recovery_e2e_test.cpp	The reassigned-owner checkpoint test checks for the record-scan fallback message instead of any fallback message.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant CallerTask
  participant LoadValueAwaitable
  participant RegisteredBufferPool
  participant DiskIO
  participant LoadValueSlow
  CallerTask->>LoadValueAwaitable: Start value read
  LoadValueAwaitable->>RegisteredBufferPool: Acquire read buffer
  RegisteredBufferPool-->>LoadValueAwaitable: Assign buffer lease
  LoadValueAwaitable->>DiskIO: Submit record read
  DiskIO-->>LoadValueAwaitable: Complete read
  LoadValueAwaitable->>LoadValueSlow: Retry after relocation

Loading

Suggested reviewers: liunyl

Merge Risk: ⚪ Minimal · up to fcad8

This change moves value and extent reads onto direct awaitables in the caller's coroutine. Review found no confirmed defects that block merging. As a follow-up, the owner may want to confirm that cross-owner extent reads always resume their caller through the runtime bridge.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 3.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 99 functions across 8 files. (2 skipped: 2…	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.
Title check		The title clearly and concisely describes the main change: replacing wrapper tasks with direct awaitables for storage value and extent reads.
Description check		The description covers all required template sections, including context, behavior changes, implementation, design decisions, documentation, test results, risks, rollback, reviewer guidance, and follo…

---

Full details: Docstring Coverage

Explanation

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

---

• Fix all pre-merge checks with AI

✨ 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 buffer queue,
Then hops where owner callbacks brew.
It nibbles bytes, checks each extent,
And follows where relocated records went.
All pins released, it bounds away.}

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