# Review: Compaction Stale DiskLoc

**Spec:** `armdb/docs/superpowers/specs/26-05-16-compaction-stale-diskloc.md`
**Reviewer model:** GPT-5 Codex
**Date:** 2026-05-16
**Verdict:** требует изменений перед implementation plan. Диагноз гонки и общий retry-подход правильные, но текущая спека не доказывает фикс тестами и пропускает несколько важных read-path деталей.

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

Спека верно описывает окно между копированием старого `DiskLoc` и удалением старого immutable-файла во время compaction. Решение "вернуть внутренний `StaleDiskLoc` и перечитать свежий `DiskLoc` из индекса" выглядит правильным и достаточно узким.

Но в текущем виде её нельзя отдавать в implementation: предложенные тесты могут проходить на старом коде, test hooks недоступны из выбранного места тестов, а encrypted `read_block` и multi-block extraction всё ещё могут обходить `StaleDiskLoc`.

## Что хорошо

- Race condition разложен по реальным фазам `compact_shard_inner`: add new immutable -> batch `update_if_match` -> remove old immutable.
- Scope узкий: без on-disk format changes, без deferred file retirement и без изменения фаз compaction.
- Retry от свежего индекса лучше соответствует текущей архитектуре, чем попытка читать старый файл под shard lock.
- `VarMap` отдельно учтён: у него нет `VarNode`, поэтому нужен повторный lock HashMap-индекса.

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

### 1. Регрессионные тесты не воспроизводят stale-copy

В Test hooks спека предлагает:

1. скопировать stale `DiskLoc`;
2. довести compaction до "index updated + old immutable removed";
3. вызвать `tree.get(&key)` / `map.get(&key)`.

Это не моделирует исходную гонку. После шага 3 публичный `get()` заново читает индекс: `VarTree::get` делает `node.load_disk()` прямо перед `read_value_cached` (`armdb/src/var_tree.rs:197-210`), а `VarMap::get` заново берёт `HashMap` lock и копирует текущий `DiskLoc` (`armdb/src/var_map.rs:161-165`). После test-hook compaction индекс уже указывает на `F_new`, поэтому старый код тоже может вернуть `Some(value)`.

Нужно зафиксировать тестовую точку, которая реально начинает read с уже скопированного `F_old`: например, hook/pause после `node.load_disk()` и до disk read, либо test-only helper, который сначала вызывает inner-read со stale `DiskLoc`, получает `StaleDiskLoc`, а затем доказывает retry через повторное чтение node/index. Для `VarMap` нужен аналогичный путь с key + stale disk.

### 2. Предложенные test hooks недоступны из `armdb/tests`

Спека предлагает `#[cfg(test)] pub(crate)` hooks в `armdb/src/compaction.rs`, а тесты разместить в `armdb/tests/var_correctness_tests.rs`.

Это несовместимо с Rust visibility/model:

- integration tests в `armdb/tests` видят только public API crate;
- `pub(crate)` для них недоступен;
- `#[cfg(test)]` на library-коде не является нормальным API для integration test crate;
- `shard` module приватный, `VarTree.engine`/`VarMap.engine` приватные, поэтому тест #1 не сможет получить `Shard`, вызвать `shard.lock()` и затем `inner.read_value_from_disk_locked(...)`.

Нужно выбрать один исполнимый вариант: перенести эти тесты в unit tests внутри crate modules, либо добавить явно gated public/dev test-support API, либо построить тест на публичном API с реальной синхронизацией.

### 3. Encrypted `Shard::read_block` может обойти `StaleDiskLoc`

Спека меняет только первый lookup `inner.immutable.iter().find(...)` в `Shard::read_block`. Но текущий encrypted path делает два независимых прохода под разными lock:

- сначала захватывает `Arc<ImmutableFile>` (`armdb/src/shard.rs:409-430`);
- читает data block;
- затем снова lock-ает shard и ищет tag file по `file_id` в `inner.immutable` (`armdb/src/shard.rs:448-461`).

Если `F_old` был найден в первом lock, но удалён compaction до второго lock, `StaleDiskLoc` уже не возникнет. `tag_file` станет `None`, decrypt будет пропущен, и caller получит encrypted bytes как будто это успешный read; full block ещё может попасть в cache.

Нужно в дизайне явно переносить tag-file decision вместе с captured immutable arc из первого critical section, либо считать "missing tag for encrypted file after captured immutable" ошибкой/Stale. Иначе encrypted cached reads остаются некорректными.

### 4. `StaleDiskLoc` должен проходить через второй block read

Текущий `read_value_cached` может читать два блока, если value пересекает 4096-byte boundary. Первый block read и `next_block` сейчас идут через `Option`: `extract_from_block` принимает closure `FnOnce() -> Option<Arc<AlignedBuf>>`, а `get_or_read_block` глотает любую ошибку `Shard::read_block` и возвращает `None` (`armdb/src/var_tree.rs:755`, `armdb/src/var_tree.rs:772-774`, `armdb/src/var_map.rs:523-543`, `armdb/src/var_map.rs:581-588`).

Спека говорит, что `read_value_cached_inner` возвращает `DbResult<Option<ByteView>>`, но не фиксирует, что это должно касаться и второго block read. Если первый блок найден в cache, а второй stale, старый `Option`-shape снова превратит race в `None`, без retry.

Это не "multi-block large-value cache" follow-up: текущий код уже поддерживает value, пересекающее границу одного блока. Нужно явно refactor-нуть `get_or_read_block`/`extract_from_block` так, чтобы `DbError::StaleDiskLoc` не терялся ни на первом, ни на втором block read.

### 5. `VarTree::first` требует явной guard-lifetime правки

Спека предлагает передавать `&VarNode<K>` в `read_value_cached`, чтобы retry мог перечитать `node.load_disk()`. Это верно только пока `seize::LocalGuard` жив.

В текущем `VarTree::first` guard создаётся на входе, но перед чтением значения стоит `let _ = guard;` (`armdb/src/var_tree.rs:430-441`). При переходе на `read_value_cached(node)` это место надо явно переписать так, чтобы guard жил до конца retry и до копирования key/value. Иначе retry будет обращаться к node без активной epoch-защиты.

### 6. `warmup` выпал из scope

`VarTree::warmup` и `VarMap::warmup` тоже копируют `DiskLoc`/block coordinates из индекса, отпускают защиту/lock, а потом вызывают `Shard::read_block` (`armdb/src/var_tree.rs:634-677`, `armdb/src/var_map.rs:348-375`).

После добавления `StaleDiskLoc` этот public API может начать отдавать transient error во время concurrent compaction. Это не обязательно должно входить в первый slice, но спека должна выбрать: retry/rescan для `warmup`, либо явный Non-Goal с ожидаемым поведением.

## Мелкие замечания

- В Rust source лучше не заносить em dash в error string (`"stale DiskLoc - file removed by compaction"` вместо `"stale DiskLoc — file removed by compaction"`), чтобы не расширять charset без необходимости.
- Формулировка "every public path that can produce it is wrapped in retry" сейчас неверна из-за `warmup` и прямых/internal `read_block` callers. Её стоит сузить до public `get`/iteration paths или расширить scope.
- Test #1 про `Shard::read_block` и `ShardInner::read_value_from_disk_locked` лучше делать как module unit test рядом с `shard.rs`, если не вводится public test-support API.

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

Пока не готова. После правок по пунктам 1-4 можно считать core design достаточно приземлённым для implementation plan. Пункты 5-6 тоже лучше закрыть в этой же редакции спеки, чтобы кодеру не пришлось принимать concurrency/API решения по ходу.
