# Review: VarTree / VarMap migrate hook semantics

**Spec:** `armdb/docs/superpowers/specs/26-05-16-vartree-varmap-migrate-hooks.md`
**Reviewer model:** GPT-5 Codex
**Date:** 2026-05-16
**Verdict:** требует правок перед реализацией

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

Основная идея спеки корректная: `VarTree`/`VarMap::migrate` действительно
расходятся с контрактом `hooks.md`, а выравнивание с `TypedTree`/`TypedMap`
через `on_init` + hook-free записи выглядит правильным направлением.

Но в текущем виде спека не готова к исполнению: verification не собирает
целевые модули, `VarMap::replay_init` описан не по реальной структуре кода, а
refactor locked-path не фиксирует, что делать с `insert_locked`.

## Что хорошо

- Контракт `migrate`: `Keep`/`Update` дают `on_init`, `Update`/`Delete` пишут
  hook-free, `on_write` в миграции не вызывается — совпадает с `TypedTree` /
  `TypedMap`.
- `Db` orchestration для `open_var_typed_tree` / `open_var_typed_map` правильно
  целится в уже существующую модель `run_migration(...) -> replay_init()`.
- `VarTypedHookAdapter` действительно уже умеет декодировать `on_init`, так что
  отдельный adapter-layer design не нужен.
- Scope не лезет в on-disk format, recovery и оставшиеся follow-up из
  var-collections correctness spec.

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

### 1. Verification сейчас не проверяет целевой код

`armdb/Cargo.toml:20` не включает `var-collections` в default features, а
`armdb/src/lib.rs:306-309` компилирует `var_map`/`var_tree` только под
`#[cfg(feature = "var-collections")]`.

Команды из спеки:

- `cargo nextest run -p armdb -E 'test(var_tree) | test(var_map)'`
- `cargo nextest run -p armdb`
- `cargo nextest run -p armdb --features encryption`
- `cargo clippy -p armdb -- -D warnings`

не являются достаточной проверкой этого slice: без `--features var-collections`
они могут вообще не собрать изменяемые модули.

Нужно заменить verification на feature-aware набор, например:

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

### 2. `VarMap::replay_init` псевдокод не соответствует реальному lock/index layout

Спека описывает `for shard in self.shards.iter() { let inner = shard.read();
for (key, disk_loc) in inner.index.iter() { ... } }`, но текущий `VarMap`
хранит индексы отдельно как `self.indexes[shard_id]: Mutex<HashMap<K,
DiskLoc>>`; публичные writes берут shard lock, затем index lock
(`armdb/src/var_map.rs:199-200`, `222-223`, `235-236`).

При этом чтение значения через `read_value_cached_inner` может брать shard lock
для write-buffer path (`armdb/src/var_map.rs:529` и ниже). Если реализация
пойдёт по псевдокоду и будет держать index lock во время disk/cache read, она
получит обратный порядок lock-ов относительно public write path. Сейчас
`VarMap::get` избегает этого: копирует `DiskLoc` под index lock и отпускает lock
до чтения (`armdb/src/var_map.rs:159-172`).

Спека должна явно закрепить для `VarMap::replay_init`:

- snapshot `(K, DiskLoc)` делается под `indexes[i]` lock;
- lock отпускается до `read_value_cached_inner`;
- при `DbError::StaleDiskLoc` либо повторяется snapshot текущего `DiskLoc` из
  index, либо используется helper, повторяющий паттерн `get`;
- `on_init` вызывается только для значения, прочитанного из актуального live
  `DiskLoc`.

Без этого кодеру придётся принимать lock-order решение уже в implementation.

### 3. Hook-free refactor не фиксирует стратегию для `insert_locked`

В Goals сказано, что публичные `put`/`insert`/`delete`/`cas`/`update`/
`fetch_update` продолжают фитить `on_write` ровно один раз, а в Design —
что public methods зовут `::<true>`, а `atomic()` зовёт `::<false>`.

Но concrete design описывает const-generic rewrite только для
`put_locked`/`delete_locked`. `insert_locked` сейчас устроен иначе:
public `insert` вызывает `insert_locked`, а затем сам вызывает `on_write`
(`armdb/src/var_tree.rs:244-252`, `armdb/src/var_map.rs:217-225`); shard-level
`atomic().insert` вызывает тот же `insert_locked`, но без hook
(`armdb/src/var_tree.rs:1123-1132`, `armdb/src/var_map.rs:738-746`).

Нужно выбрать и записать один из вариантов:

- оставить `insert_locked` hook-free, а public `insert` оставить единственным
  hook site для insert;
- или добавить `insert_locked_inner<const HOOKS: bool>`, где public `insert`
  вызывает `::<true>`, а `atomic().insert` вызывает `::<false>`.

Текущий текст оставляет неоднозначность и может привести либо к двойному
`on_write` на public insert, либо к появлению hooks внутри `atomic().insert`.

### 4. Тесты `replay_init` лучше разделить на raw и Db orchestration

Тестовый план покрывает `replay_init` через `Db::open_var_typed_tree` /
`Db::open_var_typed_map`. Это нужно для проверки wrapper/orchestration, но не
ловит напрямую ошибки raw `VarTree<K, H>::replay_init` и
`VarMap<K, H>::replay_init`.

Так как `replay_init` планируется `pub(crate)`, module tests в
`var_tree.rs`/`var_map.rs` могут вызвать его напрямую. Стоит добавить прямые
raw tests для `WriteHook`-пути и оставить отдельные `Db::open_var_typed_*`
тесты для adapter + `run_migration` ветки.

## Готова ли спека

Пока нет. После правок по пунктам 1-3 спека станет достаточно конкретной для
плана и реализации. Пункт 4 не блокирует дизайн, но заметно снижает риск
пропустить ошибку именно в raw `VarMap::replay_init`.
