# Review: Var Collections Critical Correctness

**Spec:** `armdb/docs/superpowers/specs/26-05-15-var-collections-critical-correctness.md`
**Reviewer model:** Opus 4.7 (1M context)
**Date:** 2026-05-15
**Verdict:** требует уточнений перед реализацией — ядро дизайна корректное, но три пункта надо нормально приземлить, иначе кодеру придётся решать архитектурные вопросы по ходу.

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

Спека хорошо очерчивает scope, корректно опирается на codex-review и аккуратно вынесла в Non-Goals всё, что не должно зацепиться (file_id, migrate, multi-block, compaction-stale-DiskLoc). Главная проблема — три места, где дизайн не доведён до уровня "пиши код", и кодер либо угадает, либо вынесет вопрос обратно.

## Что хорошо

- Scope и Non-Goals честно разделены — все пять fix-ов внутри slice не затрагивают on-disk формат.
- Тестовая стратегия привязана к поведению (duplicate insert, oversized, encrypted locked read), а не к API.
- Решение оставить `strip_mark` как no-op compatibility helper снижает blast radius — разумный компромисс для slice patch.
- Признание performance trade-off для атомарного mark-поля честное; то, что unsafe pointer-pun ушёл — важнее микро-стоимости.

## Что надо уточнить до реализации

### 1. Encrypted locked-read — placement helper не определён

Спека говорит "the helper should live near shard storage logic". Но `VarTree::read_value_locked` и `VarMap::read_value_locked` вызываются под уже взятым shard mutex (сигнатура: `inner: &ShardInner`). Если завести метод на `Shard`, который сам берёт `self.inner.lock()`, — deadlock.

Текущий `Shard::read_value` (`shard.rs:356`) это иллюстрирует — он берёт lock сам и потому из locked-path неиспользуем. Нужно явно зафиксировать одно из:

- (a) helper принимает `&ShardInner` и работает без повторного lock, плюс достаёт `cipher`/`tag_file` из `inner`, ИЛИ
- (b) caller под shard lock сам собирает `(file, tag_file, cipher, file_id)` и передаёт их в чистую free-function в `io::direct`.

Без выбора это — главное открытое решение, и его уместно вынести в спеку, а не оставлять кодеру.

### 2. Explicit mark — ordering и атомарность пары `(mark, next)`

Спека описывает только переход на отдельное атомарное поле и ordering "Acquire/AcqRel". Но текущая логика SkipList опирается на _одну_ atomic load, которая возвращает и mark, и next-pointer (low-bit тэгирование на `tower(0)`). При расщеплении на два разных атомика появляется окно:

```
T1 (читатель):                      T2 (писатель):
load mark      -> false                -
                                       mark.fetch_or(1) -> true
                                       splice node out
                                       retire node via seize
load tower(0)  -> старый next       -
```

Сам по себе seize защищает от UAF, но семантика "пропускать marked при обходе" сохраняется только если читатель проверяет mark _после_ того, как зашёл по next-указателю на ноду. В коде так и сделано в `read_value_cached` (`var_tree.rs:697`) и в обходах — `is_marked()` проверяется на текущей ноде, после чего читается её tower для перехода. Этого, скорее всего, достаточно, но спека должна это явно зафиксировать (мини-абзац: "после splitа корректность держится на том, что читатель проверяет mark текущей ноды, и при `true` пропускает значение; новый next читается отдельно и может указывать на уже-marked соседа, который будет пропущен на следующем шаге").

Без этой фиксации, у кодера возникнет вопрос: достаточно ли `Acquire` для mark.load или нужен `SeqCst` между mark и tower — а это уже архитектурное решение в пограничной зоне, к нему нельзя подходить эмпирически.

### 3. Oversized entry — error variant надо выбрать в спеке

Спека оставляет на усмотрение: "use existing variant if it fits cleanly, otherwise add narrow runtime error variant". Это решение легко закрыть прямо здесь. Глядя на `error.rs`:

- `DbError::Client("entry exceeds write_buffer_size")` — 400, попадает в caller-controlled input. Самый чистый выбор.
- `DbError::Config(_)` — это про config validation, не подходит семантически.
- Новый вариант — overkill для slice.

Рекомендую зафиксировать `DbError::Client(...)`.

## Помельче, но стоит поправить

### 4. Iterator Send scope — список неполный

Спека упоминает `VarIter`, `ConstIter`, `TypedIter`. Найден ещё `unsafe impl Send for VarTypedShard` (`var_typed_tree.rs:495`) — но это _другая_ безопасность (raw pointer на `VarShard`, не `LocalGuard`), и она, вероятно, корректна. Стоит явно сказать в спеке: "проверены все unsafe Send в скиплистах и итераторах; снимаются только те, что несут `seize::LocalGuard` — это `VarIter`, `ConstIter`, `TypedIter`. Остальные (`SkipList`, `*Node`, `VarTypedShard`, `ByteView`, `AlignedBuf`) остаются как есть." Это закрывает следующий вопрос рецензента "а почему не сняли вот эти?".

### 5. Insert_locked — деталь с `disk_loc`-box в отказанной ноде

При `InsertResult::Exists` нужно освободить новую ноду. В `put_locked` это сделано аккуратно (`var_tree.rs:348-353`): сначала `disk.store(null, Relaxed)`, потом `dealloc_node` — чтобы не освободить _чужой_ DiskLoc box. Для `insert_locked` поведение должно быть идентичным: ноду создаём с новым `disk_loc` (свежий box), отказываем, и при `dealloc` этот box должен освободиться нормально (он не shared). То есть null-trick из `put_locked` здесь не нужен, но это надо явно проговорить, потому что код выглядит почти симметрично — и кодер по аналогии может скопировать ненужный null-store.

В спеке полезно одной строкой: "новая нода уникально владеет своим `DiskLoc` box до вставки в индекс; `dealloc_node` корректно его освободит без дополнительных манипуляций — в отличие от `put_locked`, где нода уже жила в индексе и могла отдать `DiskLoc` соседу через `swap_disk`."

### 6. Replication apply_entry — тоже использует `index.insert`

`VarTree::apply_entry` (`var_tree.rs:996`) при репликации вставляет ноды напрямую через `self.index.insert`. Спека про этот путь не упоминает. Поведение фикса `insert_locked` стоит сверить с replication: если на реплике произошёл `InsertResult::Exists`, что делать? (Вероятно, аналогично — swap disk + retire старого, как в `put_locked`. Но это уже не "if absent", это "apply", т.е. логика ближе к put.) Полстроки в Non-Goals или в Design: "replication apply_entry behaviour out of scope; уже использует put-like семантику и не затрагивается данным fix."

### 7. Тест encrypted locked-read — нужно гарантировать промах кэша

`VarTree::read_value_locked` (`var_tree.rs:947`) сначала смотрит write buffer, потом block cache (только для значений в одном блоке), и только потом идёт на диск через `pread_value`. Чтобы тест действительно дёрнул сломанный путь, надо:

- записать значение, **flush** (rotate или явный flush),
- _не_ читать его через публичный `get` до теста (иначе блок попадёт в кэш и `read_value_locked` прочитает плэйнтекст из кэша),
- использовать значение, **пересекающее границу 4096** — тогда условие `start + len <= 4096` ложно и cache lookup пропускается, чтение пойдёт через `pread_value`.

Без этих оговорок тест может пройти на дырявой реализации. Спека про условия теста полезно расширить — это разница между "тест есть" и "тест ловит баг".

### 8. Strip_mark callers — стратегия cleanup

Спека говорит "becomes unnecessary on new code paths or stays as a no-op compatibility helper until callers are cleaned up". Это формулировка с двумя исходами — кодеру неясно, который выбрать. Текущие callers `strip_mark`:

- `skiplist/mod.rs:98` (в `get`),
- `var_tree.rs:686, 694, 416, 428, ...` (migrate и first/last/iterators),
- `skiplist/iter.rs` (предположительно).

После переноса mark в отдельное поле, tower(0) больше не несёт mark-бит в low-bit. `strip_mark` становится тождественной функцией. Два чистых варианта:

- (A) удалить `strip_mark` и все её вызовы (≈10 мест),
- (B) оставить как `#[inline] pub fn strip_mark<T>(p: *mut T) -> *mut T { p }` и не трогать callers.

(A) чище, (B) меньше дифф. Спека пусть выберет один — в slice patch (B) логичнее как минимальное изменение.

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

Не вполне. Три блокера на decision (1, 2, 3 выше), плюс пять мелких уточнений. После апдейта по этим пунктам можно идти в plan + код.

## Не нашёл проблем в

- Scope/Non-Goals формулировках (чисто).
- Тестовом плане в части duplicate insert и oversized.
- Решении не трогать on-disk формат и `DiskLoc.file_id`.
- Verification — `cargo nextest run -p armdb` достаточно для проверки, full workspace необязателен.
