# VarTree/VarMap large-value cached read

**Status:** draft
**Date:** 2026-05-17
**Scope:** `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`
**Source review:** `armdb/docs/reviews/codex-vartree-varmap.md` — issue #4 (cached variable-value reads only support up to two blocks)
**Related fixes:**
- #5 (oversized entry rejection) — commit `9143d19`
- #6 (encrypted `read_value_locked`) — commit `007c6ae`
- #9 (stale `DiskLoc` during compaction) — PR #9

## Context

`extract_from_block` in `var_tree.rs:851` and `var_map.rs:607` handles at most
two 4096-byte blocks. The current code:

```rust
let next = next_block()?;
let first_part = &block[start..];
let second_len = len - first_part.len();
combined.extend_from_slice(first_part);
combined.extend_from_slice(&next[..second_len]);
```

panics on `next[..second_len]` whenever `second_len > 4096`. Issue #5 capped
entry size against `write_buffer_size` (default 1 MiB), but the cached read
remained limited to roughly `8192 - start` bytes. Any value larger than this
that is read through the lock-free cached path triggers a panic.

The locked path (`read_value_locked`) already reads arbitrary value sizes
correctly via `pread_value` / `pread_value_encrypted`. Only the lock-free
cached path is affected.

## Goals

1. Eliminate the panic. Values whose serialized entry fits within
   `write_buffer_size` (the existing cap from #5, where
   `entry_size(key_len, value_len) ≤ write_buffer_size`) must read correctly
   through both lock-free and locked paths regardless of how many 4 KiB blocks
   the value spans.
2. Small-value path (`start + len ≤ 8192`) keeps the existing
   cache → write-buffer → disk ordering. One early size check is added in
   front; it must not take locks for small values and must not change which
   layer answers a small read. A microbenchmark in the test suite is out of
   scope; perf parity is enforced by code review against the small-value
   ordering.
3. Propagate `StaleDiskLoc` uniformly so the existing
   `MAX_STALE_RETRIES` retry loop applies to large-value reads.
4. Preserve encryption correctness via the existing
   `read_value_from_disk_locked` helper (issue #6 already fixed this).

## Non-Goals

- Multi-block assembly from block cache for large values (Approach B from
  brainstorming). Block cache stores 4 KiB blocks, not values; large values
  rarely benefit and the optimization is non-trivial. Deferred to a separate
  PR if profiling shows a hot spot.
- Changing `extract_from_block`'s two-block signature.
- Caching entire large values.
- Changing `write_buffer_size` or the entry-size cap from #5.

## Design

### Routing

Lock-free cached path detects values that require more than two blocks and
delegates to the locked path **before** any cache lookup. Position matters:
without the early check, a cache hit on the value's first block would invoke
`extract_from_block` directly and panic.

```rust
fn read_value_cached_inner(&self, disk: &DiskLoc) -> DbResult<ByteView> {
    let len = disk.len as usize;
    let start = (disk.offset & 4095) as usize;

    // Large values (>2 blocks) bypass the cached path. This check MUST sit
    // before the cache lookup: a warm first block would otherwise route a
    // large value into `extract_from_block`, which only supports two blocks
    // and would panic on `next[..second_len]` when `second_len > 4096`.
    if start + len > 8192 {
        let shard = &self.engine.shards()[disk.shard_id as usize];
        let inner = shard.lock();
        return self.read_value_locked_result(disk, &inner);
    }

    // existing 3-step cached path: cache → write_buf → disk + cache
    // ...
}
```

Threshold `start + len > 8192` is exactly the boundary at which
`extract_from_block` would otherwise panic. It is not a configurable cap; it
is the partition between cached and locked paths.

`extract_from_block` remains two-block. Its 2-block specialization is
intentional and now provably safe: callers filter out the large case.

### `read_value_locked` refactor

Both `VarTree::read_value_locked` (`var_tree.rs:1014`) and
`VarMap::read_value_locked` (`var_map.rs:650`) currently return
`Option<ByteView>` and bury `StaleDiskLoc` behind a `tracing::error!` + `None`.
This prevents the cached path from retrying.

Split into a `DbResult` variant + thin `Option` wrapper:

```rust
fn read_value_locked_result(&self, disk: &DiskLoc, inner: &ShardInner)
    -> DbResult<ByteView>
{
    let len = disk.len as usize;

    // 1. Write buffer (active file, unflushed)
    if inner.active.file_id == disk.file_id as u32
        && let Some(bytes) = inner.write_buf.read(disk.offset as u64, len)
    {
        return Ok(ByteView::new(bytes));
    }

    // 2. Block cache (single-block fast path)
    let block_offset = disk.offset as u64 & !4095;
    let start = (disk.offset & 4095) as usize;
    if start + len <= 4096 {
        let cache_key = BlockKey {
            shard_id: disk.shard_id,
            file_id: disk.file_id as u32,
            block_offset,
        };
        if let Some(block) = self.cache.get(&cache_key) {
            return Ok(ByteView::new(&block[start..start + len]));
        }
    }

    // 3. Disk read via encryption-aware helper (already correct for #6)
    let bytes = inner.read_value_from_disk_locked(disk)?;
    Ok(ByteView::new(&bytes))
}

fn read_value_locked(&self, disk: &DiskLoc, inner: &ShardInner)
    -> Option<ByteView>
{
    match self.read_value_locked_result(disk, inner) {
        Ok(b) => Some(b),
        Err(DbError::StaleDiskLoc) => {
            tracing::error!(
                file_id = disk.file_id,
                shard_id = disk.shard_id,
                "stale DiskLoc under shard lock - programming bug",
            );
            None
        }
        Err(_) => None,
    }
}
```

The wrapper preserves existing semantics for `cas`, `update`, `put`/`delete`
with `NEEDS_OLD_VALUE`, and the shard-handle `get` — all keep their current
`Option<ByteView>` shape. The new `_result` variant is internal, used only by
the cached fallback.

Both `VarTree` and `VarMap` apply the same refactor (their `read_value_locked`
methods are structurally identical).

### Retry semantics

`read_value_locked_result` propagates `StaleDiskLoc` instead of swallowing it,
so existing retry loops apply uniformly to both cached and locked paths. The
two collections retry differently because they index differently:

- **VarTree** retries in `read_value_cached` (`var_tree.rs`). Each iteration
  re-reads `*node.load_disk()` from the SkipList node held under the
  `seize::LocalGuard`, then calls `read_value_cached_inner`. If
  `read_value_cached_inner` returns `Err(StaleDiskLoc)` (either from the
  large-value locked fallback or from the existing cached disk read), the
  loop retries up to `MAX_STALE_RETRIES`.
- **VarMap** retries in `VarMap::get` (`var_map.rs:163-172`). Each iteration
  re-locks the per-shard index, re-copies the `DiskLoc`, then calls
  `read_value_cached_inner`. Stale propagation works the same — the locked
  fallback returns `Err(StaleDiskLoc)` and the retry takes a fresh snapshot.

Under the shard lock, stale `DiskLoc` is rare (compaction races only between
snapshot and lock acquisition). If it persists past the retry budget, the
caller sees `None`.

### Encryption

No new work. The locked path's step 3 (`read_value_from_disk_locked`) routes
through `pread_value_encrypted` when `feature = "encryption"` is on and the
shard has an `encryption_key`. Large-value reads inherit this correctly.

## Tests

### Unit — `extract_from_block`

Pin the boundary; do not test the dead `> 8192` panic path (callers filter it
out).

- `extract_from_block_single_block` — value entirely in one block
  (`start + len ≤ 4096`).
- `extract_from_block_two_blocks_exact` — value ends exactly at the second
  block's end (`start + len == 8192`).
- `extract_from_block_two_blocks_partial` — value spans two blocks partially.
- Existing `extract_from_block_propagates_next_block_error` and
  `extract_from_block_multi_block_first_cached_second_stale` — kept as is.

### Unit — `read_value_locked_result` (VarTree and VarMap)

- `read_value_locked_result_ok_from_write_buf` — value in active write buffer.
- `read_value_locked_result_ok_from_cache_single_block` — value cached, single
  block.
- `read_value_locked_result_ok_from_disk_immutable` — value on immutable file,
  read via `read_value_from_disk_locked`.
- `read_value_locked_result_propagates_stale_disk_loc` — simulate stale by
  retiring the immutable file from `inner.immutable` between snapshot and
  read; assert `Err(DbError::StaleDiskLoc)`.

### Unit — `read_value_locked` (Option) backward-compat

- `read_value_locked_returns_none_on_stale` — assert that the existing
  `Option` wrapper still returns `None` on `StaleDiskLoc`, so `cas`/`update`
  callers do not regress. Do not assert exact tracing output; a behavior note
  in the test is enough.

### Unit — first-block-cached large value (deterministic)

`large_value_with_first_block_cached_uses_fallback` — pins the
"size check sits before cache lookup" invariant. Manually inserts the first
4 KiB block of a large value into `self.cache` with the matching `BlockKey`,
then calls `read_value_cached_inner` with a `DiskLoc` whose
`start + len > 8192`. Asserts the call returns `Ok(correct_bytes)` via the
locked fallback and does not panic. Without the early size check this test
panics.

### Integration — large-value end-to-end (VarTree and VarMap)

All large-value integration tests use a fixed geometry that forces
serialized entries onto an immutable file, so the read traverses
`read_value_from_disk_locked` rather than the write buffer:

```
shard_count       = 1
write_buffer_size = 128 * 1024     // fits the entry
max_file_size     = 4096           // forces rotation after one entry
```

With the entry serialized into the active file, the next `flush_write_buf` +
rotation moves it to `inner.immutable`. Tests perform a small follow-up
write that triggers rotation before issuing the read.

- `large_value_read_via_locked_fallback` — put a 50 KiB value; trigger
  rotation; `get` returns correct bytes via `read_value_from_disk_locked`,
  no panic.
- `large_value_read_encrypted` (`cfg(feature = "encryption")`) — same with
  `encryption_key` configured; drop the block cache or wait past warmup so
  the read must go to disk; assert decryption via `pread_value_encrypted`
  returns correct bytes.
- `large_value_stale_disk_loc_deterministic` — pins retry of large-value
  fallback. Insert a 50 KiB value, snapshot its `DiskLoc`, force compaction
  to remove the old immutable file, then call `read_value_cached_inner` on
  the stale snapshot. Asserts `Err(DbError::StaleDiskLoc)`. Then call the
  public `get`/equivalent and assert it returns `Some(correct_bytes)` via
  the retry path. Mirrors the existing pattern at
  `armdb/src/var_tree.rs:1491-1507` and `armdb/src/var_map.rs:896-912`.

Value size 50 KiB lives in the "occasionally 8 KiB – 100 KiB" use case,
requires ~13 blocks (well past the 2-block threshold), and the serialized
entry fits comfortably in `write_buffer_size = 128 KiB`.

## Verification

```sh
cargo check -p armdb --features var-collections
cargo nextest run -p armdb --features var-collections \
    -E 'test(var_tree) | test(var_map)'
cargo nextest run -p armdb --features 'var-collections encryption' \
    -E 'test(large_value) | test(extract_from_block) | test(read_value_locked)'
cargo clippy -p armdb --features var-collections -- -D warnings
cargo fmt --check
```

## Files Changed

- `armdb/src/var_tree.rs` — split `read_value_locked` into `_result` + `Option`
  wrapper; add large-value fallback in `read_value_cached_inner`; add tests.
- `armdb/src/var_map.rs` — same refactor and tests.

## Out of Scope (explicit)

- Multi-block cached assembly (Approach B from brainstorming).
- Changing `extract_from_block` to N-block signature.
- Caching large values whole.
- Changing `write_buffer_size` or the entry-size cap.

## Follow-Up

If profiling shows large cached values are a hot spot, add multi-block
assembly that tries the block cache first and falls back to a single `pread`
on cache miss of any constituent block. Tracked separately.
