# Review: VarTree/VarMap Large-Value Cached Read

**Spec:** `armdb/docs/superpowers/specs/26-05-17-vartree-varmap-large-value-cached-read.md`
**Reviewer model:** GPT-5 Codex
**Date:** 2026-05-17
**Verdict:** требует правок перед implementation. Основной диагноз и выбор locked fallback правильные, но спека сейчас противоречит собственному hot-path требованию и оставляет тесты слишком расплывчатыми: они могут не доказать disk/encrypted fallback и cached-first panic boundary.

## Краткий итог

Проблема подтверждается текущим кодом. `extract_from_block` в `armdb/src/var_tree.rs:851` и `armdb/src/var_map.rs:607` умеет собрать только текущий блок плюс один следующий; если `second_len > 4096`, срез `next[..second_len]` паникует. Делегирование значений с `start + len > 8192` в locked path выглядит правильным узким решением, потому что `ShardInner::read_value_from_disk_locked` уже читает произвольную длину и учитывает encryption.

Но перед implementation нужно уточнить несколько деталей ниже.

## Что хорошо

- Порог `start + len > 8192` выбран правильно: это ровно случай, когда значение требует больше двух 4096-byte блоков.
- Не надо расширять `extract_from_block` до N-block assembly в этом срезе; это реально отдельная оптимизация.
- Refactor `read_value_locked` в `DbResult` helper плюс старый `Option` wrapper хорошо ложится на текущие вызовы `cas`, `update`, `put`/`delete` с `NEEDS_OLD_VALUE` и shard handles.
- Повторное использование `read_value_from_disk_locked` сохраняет уже исправленный encrypted-read путь без нового relock helper.

## Что надо исправить

### 1. Hot-path goal противоречит routing design

В Goals спека требует оставить small-value path "unchanged - no extra branches, no extra locks, no perf regression" (`spec:38-39`). Но предложенный дизайн добавляет `if start + len > 8192` до cache lookup (`spec:63-74`).

Этот branch нужен именно до `self.cache.get(...)`: текущий код сначала может попасть в cache-hit branch, а затем вызвать `extract_from_block` (`armdb/src/var_tree.rs:794-799`, `armdb/src/var_map.rs:583-587`). Если first block уже cached, large value упадет до любого disk fallback. Поэтому одновременно "no extra branches" и "filter before cache hit" выполнить нельзя.

Нужно переписать goal на что-то вроде: "small values keep the existing cache/write-buffer/disk ordering; only one early size check is added and it must not take locks for small values". Если perf-regression является жестким требованием, добавьте microbenchmark или явно перенесите benchmark в follow-up.

### 2. Large-value e2e tests могут не проверить disk/encrypted fallback

Спека говорит: "put a 50 KiB value, flush to immutable, get" (`spec:206-207`), но не фиксирует, как именно значение окажется на disk/immutable. Это важно: `append_entry` прямо документирует, что данные не пишутся на диск сразу, а остаются в write buffer (`armdb/src/shard.rs:749-775`), и rotation происходит только после `active.write_offset >= max_file_size` (`armdb/src/shard.rs:797-802`).

Если тест оставить на default/`Config::test()` с `max_file_size = 256 MB` и `write_buffer_size = 1 MB`, 50 KiB значение будет прочитано из write buffer в `read_value_locked_result`, а не через `read_value_from_disk_locked`. Такой тест пройдет даже при сломанном disk/encryption fallback.

Нужно зафиксировать test geometry, например:

- `shard_count = 1`;
- `write_buffer_size` больше serialized entry, например 64 KiB или 128 KiB;
- `max_file_size = 4096` или другой малый размер, чтобы 50 KiB entry принудительно rotated/flushed в immutable;
- для encrypted теста - cold cache перед `get`, чтобы проверялся именно `pread_value_encrypted`.

Тогда тест действительно докажет путь, ради которого вводится locked fallback.

### 3. Warm-cache regression должен кэшировать именно first block large value

`large_value_read_after_cache_warmup` описан как "same, with small reads issued first to warm the block cache" (`spec:208-210`). Это слишком расплывчато. Small reads могут прогреть вообще другие block offsets и не покрыть главный риск: first block большого значения уже есть в `BlockCache`, значит старый код сразу идет в `extract_from_block` и паникует.

Нужно явно сделать одно из двух:

- вызвать public `warmup()` после того, как large value оказался в immutable, и затем `get()` этого же large key;
- или в module unit test вручную положить first block large value в `self.cache` и проверить, что `read_value_cached_inner` сначала применяет large fallback, а не cache-hit extraction.

Иначе тест не закрепляет самое важное размещение size-check перед cache-hit branch.

### 4. Concurrent compaction test слишком недетерминированный

`large_value_read_concurrent_with_compaction` допускает `Some(correct_bytes)` или `None`, но "never panic" (`spec:214-216`). Такой тест может вообще не попасть в stale window и останется слабым. В текущем коде уже есть более надежный паттерн: unit tests берут stale snapshot `DiskLoc`, запускают compaction, затем напрямую проверяют `read_value_cached_inner(&snap)` на `DbError::StaleDiskLoc` (`armdb/src/var_tree.rs:1491-1507`, `armdb/src/var_map.rs:896-912`).

Для этой спеки лучше добавить детерминированный large-value вариант:

- создать value с `start + len > 8192`;
- захватить старый `DiskLoc`;
- сделать file compactable и удалить старый immutable через compaction;
- проверить, что large fallback возвращает `StaleDiskLoc`, а публичный retry path затем не паникует.

Concurrent smoke можно оставить дополнительным, но не как основной proof для retry semantics.

### 5. Формулировка "values up to write_buffer_size" неточная

Goals говорят, что должны читаться "Values up to `write_buffer_size`" (`spec:36-37`), а Tests говорят, что 50 KiB "stays well below `write_buffer_size`" (`spec:218-220`). Реальная граница записи в коде - не `value.len() <= write_buffer_size`, а `entry_size(key_len, value_len) <= write_buffer_size` (`armdb/src/shard.rs:758-763`).

Лучше везде заменить на "values whose serialized entry fits the existing write_buffer_size cap" и, если нужны boundary tests, считать `EntryHeader + key_len + padding`.

### 6. Retry semantics нужно явно разделить для VarTree и VarMap

В разделе retry написано, что retry берет свежий `*node.load_disk()` (`spec:161-164`). Это верно для `VarTree::read_value_cached`, но `VarMap` не имеет node и retry находится в `VarMap::get`, где каждый проход заново lock-ает shard index и копирует `DiskLoc` (`armdb/src/var_map.rs:163-172`).

Нужно коротко дописать оба механизма, чтобы implementation не переносил VarTree-only mental model на VarMap.

### 7. Log assertion для wrapper test может быть лишним

`read_value_locked_returns_none_on_stale_with_log` требует проверить и `None`, и tracing error (`spec:198-202`). Если в repo нет готового tracing-capture helper, проверка логов добавит больше хрупкости, чем пользы. Для backward-compat достаточно закрепить `None` на `StaleDiskLoc`; точный текст лога можно оставить как behavior note, если нет простой существующей инфраструктуры.

## Готовность

Пока не готова к implementation plan. После правок по пунктам 1-4 core design будет достаточно конкретным: large values уходят в locked fallback до cache-hit extraction, stale errors проходят в существующий retry, а тесты реально доказывают disk/encrypted/cached-first сценарии. Пункты 5-7 лучше поправить в той же редакции, чтобы не оставлять исполнителю двусмысленности.
